Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ gem 'paper_trail'
gem 'pg', '~> 1.6'
gem 'postmark-rails'
gem 'propshaft'
gem 'public_suffix', '~> 7.0'
gem 'puma', '~> 7.2'
gem 'rack_content_type_default', '~> 1.1'
gem 'rack-cors'
Expand Down
1 change: 1 addition & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,7 @@ DEPENDENCIES
postmark-rails
propshaft
pry-byebug
public_suffix (~> 7.0)
puma (~> 7.2)
rack-cors
rack_content_type_default (~> 1.1)
Expand Down
8 changes: 8 additions & 0 deletions app/models/school.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ class School < ApplicationRecord
has_many :projects, dependent: :nullify
has_many :roles, dependent: :nullify
has_many :school_projects, dependent: :nullify
has_many :school_email_domains, dependent: :destroy
Copy link
Copy Markdown
Contributor

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 if candidate_domain is registered for this school.

This would allow testing domains presented by registering users without exposing the internal implementation of school_email_domains.

Copy link
Copy Markdown
Author

@PetarSimonovic PetarSimonovic Apr 30, 2026

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 School using a straightforward school_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


VALID_URL_REGEX = %r{\A(?:https?://)?(?:www.)?[a-z0-9]+([-.]{1}[a-z0-9]+)*\.[a-z]{2,63}(\.[a-z]{2,63})*(/.*)?\z}ix

Expand Down Expand Up @@ -116,6 +117,13 @@ def import_in_progress?
.exists?(description: id)
end

def valid_domain?(candidate_domain)
validated_domain = SchoolEmailDomainValidator.call(candidate_domain)
school_email_domains.exists?(domain: validated_domain)
rescue ::SchoolEmailDomainValidator::Error
false
end

private

# Ensure the reference is nil, not an empty string
Expand Down
18 changes: 18 additions & 0 deletions app/models/school_email_domain.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# frozen_string_literal: true

class SchoolEmailDomain < ApplicationRecord
belongs_to :school

validates :domain, presence: true
Comment thread
PetarSimonovic marked this conversation as resolved.
validates :domain, uniqueness: { scope: :school_id }

before_validation :validate_domain

private

def validate_domain
self.domain = SchoolEmailDomainValidator.call(domain)
rescue ::SchoolEmailDomainValidator::Error => e
errors.add(:domain, e.error_code)
end
Comment thread
PetarSimonovic marked this conversation as resolved.
end
14 changes: 14 additions & 0 deletions db/migrate/20260420104937_create_school_email_domains.rb
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|
Comment thread
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
12 changes: 11 additions & 1 deletion db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

62 changes: 62 additions & 0 deletions lib/school_email_domain_validator.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
# frozen_string_literal: true

module SchoolEmailDomainValidator
class Error < StandardError
def error_code
:invalid
end
end

class BlankDomainError < Error
def error_code
:blank
end
end

class InvalidURIError < Error
def error_code
:invalid_uri
end
end

class InvalidHostError < Error
def error_code
:invalid_host
end
end

class PublicSuffixError < Error
def error_code
:invalid_public_suffix
end
end

def self.call(domain)
raise BlankDomainError if domain.blank?

validate_domain(domain)
end

def self.validate_domain(domain)
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
raise InvalidURIError
end

def self.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

raise InvalidHostError unless host&.match?(accounts_host_format)

raise PublicSuffixError, 'domain has no registered public suffix' unless PublicSuffix.valid?(host)

host
end
end
81 changes: 81 additions & 0 deletions spec/lib/school_email_domain_validator_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
# frozen_string_literal: true

require 'rails_helper'

RSpec.describe SchoolEmailDomainValidator do
describe '.call' do
it 'returns success with a normalised host for a valid domain' do
result = described_class.call('example.com')

expect(result).to eq('example.com')
end

context 'with a valid domain' do
it 'extracts the host for http' do
result = described_class.call('http://mail.school.edu/path?query=1')
expect(result).to eq('mail.school.edu')
end

it 'extracts the host for https' do
result = described_class.call('https://mail.school.edu/path?query=1')
expect(result).to eq('mail.school.edu')
end

it 'removes a trailing dot' do
result = described_class.call('example.edu.')
expect(result).to eq('example.edu')
end

it 'lowercases the host' do
result = described_class.call('EXAMPLE.EDU')
expect(result).to eq('example.edu')
end

it 'removes a leading @' do
result = described_class.call('@example.edu')
expect(result).to eq('example.edu')
end

it 'accepts a subdomain' do
result = described_class.call('mail.example.edu')
expect(result).to eq('mail.example.edu')
end

it 'accepts a hostname er a multi-part public suffix' do
result = described_class.call('school.example.co.uk')
expect(result).to eq('school.example.co.uk')
end

it 'accepts a district-style with a multi-part public suffix' do
result = described_class.call('school.k12.tx.us')
expect(result).to eq('school.k12.tx.us')
end
end

context 'when the domain is blank' do
it 'raises BlankDomain' do
expect { described_class.call('') }.to raise_error(SchoolEmailDomainValidator::BlankDomainError)
end
end

context 'when the domain has an invalid URI' do
it 'raises URI::InvalidURIError' do
expect { described_class.call('https://exa mple.com') }.to raise_error(SchoolEmailDomainValidator::InvalidURIError)
end
end

context 'when the host does not match accounts_host_format' do
it 'raises InvalidHostError' do
expect { described_class.call('school_domain.edu') }
.to raise_error(SchoolEmailDomainValidator::InvalidHostError)
end
end

context 'when the host fails PublicSuffix validation' do
it 'raises PublicSuffixError' do
expect { described_class.call('co.uk') }
.to raise_error(SchoolEmailDomainValidator::PublicSuffixError)
end
end
end
end
89 changes: 89 additions & 0 deletions spec/models/school_email_domain_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
# frozen_string_literal: true

require 'rails_helper'

RSpec.describe SchoolEmailDomain do
Comment thread
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
it 'accepts the domain' do
expect(school_email_domain.domain).to eq('example.edu')
end

describe 'domain normalisation' do
let(:domain) { '@EXAMPLE.EDU.' }

it 'normalises a domain' do
expect(school_email_domain.domain).to eq('example.edu')
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 +79 to +87
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fspeirs and @peconia I've grouped the invalid domains together so it might be easier to check edge cases like the one that Päivi mentioned

end
end
29 changes: 29 additions & 0 deletions spec/models/school_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@
expect(school.roles.size).to eq(2)
end

it 'has many school email domains' do
SchoolEmailDomain.create!(school:, domain: 'example.edu')
SchoolEmailDomain.create!(school:, domain: 'other.edu')
expect(school.school_email_domains.size).to eq(2)
end

context 'when a school is destroyed' do
let!(:school_class) { create(:school_class, school:, teacher_ids: [teacher.id]) }
let!(:lesson_1) { create(:lesson, user_id: teacher.id, school_class:) }
Expand Down Expand Up @@ -88,6 +94,11 @@
school.destroy!
expect(role.reload.school_id).to be_nil
end

it 'also destroys school email domains' do
SchoolEmailDomain.create!(school:, domain: 'example.edu')
expect { school.destroy! }.to change(SchoolEmailDomain, :count).by(-1)
end
end
end

Expand Down Expand Up @@ -671,4 +682,22 @@
expect(school.reopen).to be(false)
end
end

describe '#valid_domain?' do
let(:valid_domain) { 'valid.edu' }
let(:unregistered_domain) { 'invalid.edu' }
let(:invalid_domain) { 'not a domain' }

before do
SchoolEmailDomain.create!(school:, domain: valid_domain)
end

it 'returns true when school has registered the email domain' do
expect(school.valid_domain?(valid_domain)).to be(true)
end

it 'returns false when school has not registered the email domain' do
expect(school.valid_domain?(unregistered_domain)).to be(false)
end
end
end
Loading