-
Notifications
You must be signed in to change notification settings - Fork 5
Store school email domains #787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
1271659
89183cf
ec46038
d55bdf4
25e5d20
7ece122
92c8194
645db9a
719a809
70c6174
05bf918
f5eb365
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| class SchoolEmailDomain < ApplicationRecord | ||
| belongs_to :school | ||
|
|
||
| validates :domain, presence: true | ||
|
PetarSimonovic marked this conversation as resolved.
|
||
| validates :domain, uniqueness: { scope: :school_id } | ||
|
|
||
| before_validation :validate_domain | ||
|
|
||
| private | ||
|
|
||
| def validate_domain | ||
| return if domain.blank? | ||
|
|
||
| value = domain.strip.downcase | ||
| # Add a scheme unless it already has one, so URI can parse it | ||
| value = "http://#{value}" unless %r{\A[a-z][a-z0-9+\-.]*://}i.match?(value) | ||
| uri = URI.parse(value) | ||
| host = uri.host&.delete_suffix('.') | ||
|
|
||
| validate_host(host) | ||
| rescue URI::InvalidURIError | ||
| errors.add(:domain, :invalid) | ||
| end | ||
|
|
||
| def validate_host(host) | ||
| accounts_host_format = | ||
| /\A\s*(?:[A-Za-z0-9](?:[A-Za-z0-9-]{0,61}[A-Za-z0-9])?\.)+[A-Za-z]{2,63}\s*\z/i | ||
|
Comment on lines
+28
to
+29
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @peconia we're validating against the regex that Profile uses |
||
|
|
||
| unless host&.match?(accounts_host_format) | ||
| errors.add(:domain, :invalid) | ||
| return | ||
| end | ||
|
|
||
| if PublicSuffix.valid?(host) | ||
| self.domain = host | ||
| else | ||
| errors.add(:domain, :invalid) | ||
| end | ||
| end | ||
|
PetarSimonovic marked this conversation as resolved.
|
||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| class CreateSchoolEmailDomains < ActiveRecord::Migration[7.2] | ||
| def change | ||
| create_table :school_email_domains, id: :uuid do |t| | ||
|
PetarSimonovic marked this conversation as resolved.
|
||
| t.references :school, null: false, foreign_key: true, type: :uuid | ||
| t.string :domain, null: false | ||
|
|
||
| t.timestamps | ||
| end | ||
|
|
||
| add_index :school_email_domains, %i[school_id domain], unique: true | ||
| end | ||
| end | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'rails_helper' | ||
|
|
||
| RSpec.describe SchoolEmailDomain do | ||
|
PetarSimonovic marked this conversation as resolved.
|
||
| subject(:school_email_domain) { described_class.create!(school:, domain:) } | ||
|
|
||
| let(:school) { create(:school, creator_id: SecureRandom.uuid) } | ||
| let(:domain) { 'example.edu' } | ||
|
|
||
| describe 'associations' do | ||
| it { is_expected.to belong_to(:school) } | ||
| it { is_expected.to validate_presence_of(:domain) } | ||
| it { is_expected.to be_valid } | ||
| end | ||
|
|
||
| context 'with a valid domain' do | ||
| describe 'domain normalisation' do | ||
| context 'when given a full url' do | ||
| let(:domain) { 'http://mail.school.edu/path?query=1' } | ||
|
|
||
| it 'extracts the host' do | ||
| expect(school_email_domain.domain).to eq('mail.school.edu') | ||
| end | ||
| end | ||
|
|
||
| context 'when given a full https url' do | ||
| let(:domain) { 'https://mail.school.edu/path' } | ||
|
|
||
| it 'extracts the host' do | ||
| expect(school_email_domain.domain).to eq('mail.school.edu') | ||
| end | ||
| end | ||
|
|
||
| context 'when given a domain with a trailing dot' do | ||
| let(:domain) { 'EXAMPLE.EDU.' } | ||
|
|
||
| it 'stores the host without the trailing dot' do | ||
| expect(school_email_domain.domain).to eq('example.edu') | ||
| end | ||
| end | ||
|
|
||
| context 'when given a capitalised host' do | ||
| let(:domain) { 'EXAMPLE.EDU' } | ||
|
|
||
| it 'downcases the host' do | ||
| expect(school_email_domain.domain).to eq('example.edu') | ||
| end | ||
| end | ||
|
|
||
| context 'with a leading @' do | ||
| let(:domain) { '@example.edu' } | ||
|
|
||
| it 'removes the @' do | ||
| expect(school_email_domain.domain).to eq('example.edu') | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe 'public suffix list validation' do | ||
| context 'when there is at least one registrable label before the public suffix' do | ||
| let(:domain) { 'example.edu' } | ||
|
|
||
| it 'accepts the domain' do | ||
| expect(school_email_domain.domain).to eq('example.edu') | ||
| end | ||
| end | ||
|
|
||
| context 'when there is a subdomain before a valid public suffix' do | ||
| let(:domain) { 'mail.example.edu' } | ||
|
|
||
| it 'accepts the domain' do | ||
| expect(school_email_domain.domain).to eq('mail.example.edu') | ||
| end | ||
| end | ||
|
|
||
| context 'when there is a hostname under a multi-part public suffix' do | ||
| let(:domain) { 'school.example.co.uk' } | ||
|
|
||
| it 'accepts the domain' do | ||
| expect(school_email_domain.domain).to eq('school.example.co.uk') | ||
| end | ||
| end | ||
|
|
||
| context 'when given a district-style host with a multi-part public suffix' do | ||
| let(:domain) { 'school.k12.tx.us' } | ||
|
|
||
| it 'accepts the domain' do | ||
| expect(school_email_domain.domain).to eq('school.k12.tx.us') | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe 'domain uniqueness' do | ||
| context 'when the proposed domain matches the existing record' do | ||
| subject(:school_email_domain) { described_class.new(school:, domain:) } | ||
|
|
||
| let(:domain) { 'example.edu' } | ||
|
|
||
| before do | ||
| described_class.create!(school:, domain: 'example.edu') | ||
| school_email_domain.valid? | ||
| end | ||
|
|
||
| it 'rejects the duplicate' do | ||
| expect(school_email_domain).not_to be_valid | ||
| end | ||
|
|
||
| it 'records :taken on domain' do | ||
| expect(school_email_domain.errors.of_kind?(:domain, :taken)).to be(true) | ||
| end | ||
| end | ||
|
|
||
| context 'when the proposed domain matches after normalisation' do | ||
| subject(:school_email_domain) { described_class.new(school:, domain:) } | ||
|
|
||
| let(:domain) { 'http://EXAMPLE.EDU' } | ||
|
|
||
| before do | ||
| described_class.create!(school:, domain: 'example.edu') | ||
| school_email_domain.valid? | ||
| end | ||
|
|
||
| it 'rejects the duplicate' do | ||
| expect(school_email_domain).not_to be_valid | ||
| end | ||
|
|
||
| it 'records :taken on domain' do | ||
| expect(school_email_domain.errors.of_kind?(:domain, :taken)).to be(true) | ||
| end | ||
| end | ||
|
|
||
| it 'allows the same domain for a different school' do | ||
| described_class.create!(school:, domain: 'example.edu') | ||
| other_school = create(:school, creator_id: SecureRandom.uuid) | ||
| other_school_email_domain = described_class.new(school: other_school, domain: 'example.edu') | ||
|
|
||
| expect(other_school_email_domain).to be_valid | ||
| end | ||
| end | ||
| end | ||
|
|
||
| context 'with an invalid domain' do | ||
| it { is_expected.not_to allow_value('').for(:domain) } | ||
| it { is_expected.not_to allow_value(' ').for(:domain) } | ||
| it { is_expected.not_to allow_value('http://').for(:domain) } | ||
| it { is_expected.not_to allow_value('edu').for(:domain) } | ||
| it { is_expected.not_to allow_value('com').for(:domain) } | ||
| it { is_expected.not_to allow_value('co.uk').for(:domain) } | ||
| it { is_expected.not_to allow_value('http://invalid uri').for(:domain) } | ||
| it { is_expected.not_to allow_value('-wrong.edu').for(:domain) } | ||
|
Comment on lines
+143
to
+151
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Callers (including my work) would like a validator method on this called something like:
valid_domain?(candidate_domain)which returns true ifcandidate_domainis registered for this school.This would allow testing domains presented by registering users without exposing the internal implementation of
school_email_domains.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a validator method to
Schoolusing a straightforwardschool_email_domains.exists?I don't think we need to optimise for batch domain checks, since we're validating one student at a time
If we ever did need to perform some sort of batch validation, we could revisit this and perhaps pluck then memoise the domains