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
5 changes: 5 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,10 @@ def import_in_progress?
.exists?(description: id)
end

def valid_domain?(candidate_domain)
school_email_domains.exists?(domain: candidate_domain)
end

private

# Ensure the reference is nil, not an empty string
Expand Down
42 changes: 42 additions & 0 deletions app/models/school_email_domain.rb
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
Comment thread
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
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.

@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
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.

153 changes: 153 additions & 0 deletions spec/models/school_email_domain_spec.rb
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
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
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
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
28 changes: 28 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,21 @@
expect(school.reopen).to be(false)
end
end

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

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?(invalid_domain)).to be(false)
end
end
end
Loading