From d8d5f42a76f585fc1e14dfade24582e6308e3f9b Mon Sep 17 00:00:00 2001
From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com>
Date: Tue, 15 Sep 2026 14:07:53 +0100
Subject: [PATCH 1/4] Move unnecessary domain check

This check isn't needed as it's done in profile when creating a student account.

In the case a student is logged in, we know if they are part of a school or not (now by checking role, and later the school_id claim as part of their token) so don't need to check their domain.

If a student is logged out, we don't know which domain they will try to sign up with so can't check

This check might be needed if we were allowing the provisioning of teacher accounts but we're not doing this.
---
 app/controllers/api/join_controller.rb    |  2 +-
 app/models/school.rb                      |  9 -----
 app/services/join_status_service.rb       |  2 --
 spec/models/school_spec.rb                | 32 -----------------
 spec/requests/join_controller_spec.rb     | 44 -----------------------
 spec/services/join_status_service_spec.rb |  8 -----
 6 files changed, 1 insertion(+), 96 deletions(-)

diff --git a/app/controllers/api/join_controller.rb b/app/controllers/api/join_controller.rb
index bd56e04c2..1b2589ee2 100644
--- a/app/controllers/api/join_controller.rb
+++ b/app/controllers/api/join_controller.rb
@@ -14,7 +14,7 @@ def show
 
     def create
       case action_status
-      when :wrong_school, :domain_mismatch, :not_a_student
+      when :wrong_school, :not_a_student
         render json: { error: action_status.to_s }, status: :forbidden
       when :already_member
         render json: {}, status: :ok
diff --git a/app/models/school.rb b/app/models/school.rb
index 3ddb8c92d..b224422c6 100644
--- a/app/models/school.rb
+++ b/app/models/school.rb
@@ -156,15 +156,6 @@ def valid_domain?(candidate_domain)
     false
   end
 
-  def email_domain_in_school_domains?(email)
-    return false if email.blank?
-
-    local, separator, domain = email.to_s.rpartition('@')
-    return false if separator.empty? || local.blank? || domain.blank?
-
-    valid_domain?(domain.strip.downcase)
-  end
-
   private
 
   # Ensure the reference is nil, not an empty string
diff --git a/app/services/join_status_service.rb b/app/services/join_status_service.rb
index 0e03ee2b4..47e5c8187 100644
--- a/app/services/join_status_service.rb
+++ b/app/services/join_status_service.rb
@@ -9,7 +9,6 @@
 #   :joinable             — user can be enrolled as a student of this class
 #   :not_a_student        — user has a non-student role
 #   :wrong_school         — user is a student of a different school
-#   :domain_mismatch      — user's email domain isn't registered for the school
 class JoinStatusService
   def initialize(school:, school_class:, user:)
     @school = school
@@ -30,7 +29,6 @@ def call
   def new_user_join_status
     return :not_a_student if user_has_non_student_account_type?
     return :wrong_school if user_in_different_school?
-    return :domain_mismatch unless @school.email_domain_in_school_domains?(@user.email)
 
     :joinable
   end
diff --git a/spec/models/school_spec.rb b/spec/models/school_spec.rb
index cca6f977c..20cffcc64 100644
--- a/spec/models/school_spec.rb
+++ b/spec/models/school_spec.rb
@@ -775,36 +775,4 @@
       expect(school.valid_domain?(unregistered_domain)).to be(false)
     end
   end
-
-  describe '#email_domain_in_school_domains?' do
-    before do
-      SchoolEmailDomain.create!(school:, domain: 'valid.edu')
-    end
-
-    it 'returns true when the email domain is registered for the school' do
-      expect(school.email_domain_in_school_domains?('user@valid.edu')).to be(true)
-    end
-
-    it 'returns false when the email domain is not registered for the school' do
-      expect(school.email_domain_in_school_domains?('user@other.edu')).to be(false)
-    end
-
-    it 'normalizes case and surrounding whitespace on the domain' do
-      expect(school.email_domain_in_school_domains?('User@VALID.EDU  ')).to be(true)
-    end
-
-    it 'returns false when the email is blank' do
-      expect(school.email_domain_in_school_domains?('')).to be(false)
-      expect(school.email_domain_in_school_domains?(nil)).to be(false)
-    end
-
-    it 'returns false when the email has no @ separator' do
-      expect(school.email_domain_in_school_domains?('not-an-email')).to be(false)
-    end
-
-    it 'returns false when the local part or domain is missing' do
-      expect(school.email_domain_in_school_domains?('@valid.edu')).to be(false)
-      expect(school.email_domain_in_school_domains?('user@')).to be(false)
-    end
-  end
 end
diff --git a/spec/requests/join_controller_spec.rb b/spec/requests/join_controller_spec.rb
index ed6817390..beddd9d94 100644
--- a/spec/requests/join_controller_spec.rb
+++ b/spec/requests/join_controller_spec.rb
@@ -71,26 +71,6 @@
         data = JSON.parse(response.body, symbolize_names: true)
         expect(data[:status]).to eq('wrong_school')
       end
-
-      context 'when the email domain is not registered for the school' do
-        let(:student) { build(:student, email: 'student@other.edu') }
-
-        it 'returns status: domain_mismatch' do
-          get "/api/join/#{school_class.join_code}", headers: headers
-
-          data = JSON.parse(response.body, symbolize_names: true)
-          expect(data[:status]).to eq('domain_mismatch')
-        end
-
-        it 'returns status: joinable when the user is already a student of the school' do
-          create(:student_role, school:, user_id: student.id)
-
-          get "/api/join/#{school_class.join_code}", headers: headers
-
-          data = JSON.parse(response.body, symbolize_names: true)
-          expect(data[:status]).to eq('joinable')
-        end
-      end
     end
 
     context 'when the user is authenticated as a teacher' do
@@ -208,30 +188,6 @@
         expect(response).to have_http_status(:internal_server_error)
         expect(response.body).to include('Unexpected join action_status')
       end
-
-      context 'when the email domain is not registered for the school' do
-        let(:student) { build(:student, email: 'student@other.edu') }
-
-        it 'responds with 403 domain_mismatch and does not enroll the user' do
-          expect do
-            post "/api/join/#{school_class.join_code}", headers: headers
-          end.not_to change(ClassStudent, :count)
-
-          expect(response).to have_http_status(:forbidden)
-          data = JSON.parse(response.body, symbolize_names: true)
-          expect(data[:error]).to eq('domain_mismatch')
-        end
-
-        it 'enrolls the user when they are already a student of the school' do
-          create(:student_role, school:, user_id: student.id)
-
-          expect do
-            post "/api/join/#{school_class.join_code}", headers: headers
-          end.to change(ClassStudent, :count).by(1)
-
-          expect(response).to have_http_status(:ok)
-        end
-      end
     end
 
     context 'when the user is authenticated as a teacher' do
diff --git a/spec/services/join_status_service_spec.rb b/spec/services/join_status_service_spec.rb
index 3b2bcfea3..e555f267c 100644
--- a/spec/services/join_status_service_spec.rb
+++ b/spec/services/join_status_service_spec.rb
@@ -86,14 +86,6 @@
       end
     end
 
-    context "when the user's email domain is not registered for the school" do
-      let(:user) { build(:student, email: 'user@other.edu') }
-
-      it 'returns :domain_mismatch' do
-        expect(service.call).to eq(:domain_mismatch)
-      end
-    end
-
     context 'when the user has no prior role and their email domain matches the school' do
       it 'returns :joinable' do
         expect(service.call).to eq(:joinable)

From 55007ab238beef026d74fe2923d44307e8249732 Mon Sep 17 00:00:00 2001
From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com>
Date: Tue, 15 Sep 2026 14:37:25 +0100
Subject: [PATCH 2/4] Give stranded student accounts a role in their school

Previously a student account whose token carried a school claim
but who had no Role record was stranded: they could authenticate
but appeared to belong to no school, so school-scoped endpoints
returned nothing for them.

This could happen for the new SSO student flow - most of the time
students will join an class and get a roll as part of that, but this is
needed in case a student uses SSO without a class link.

This change reads the school_id claim from the user profile and,
on each authenticated request, has StudentRoleService create the
missing student role for that school. The service is a no-op for
non-student accounts and for students who already have a role,
so it is safe to run on every request.

I've added the e2e spec coverage into the school index route as that's the first request made after login and a good way to test that the user has access to the school.
---
 app/controllers/concerns/identifiable.rb     |  2 +
 app/models/user.rb                           |  5 ++
 app/services/student_role_service.rb         | 11 ++++
 spec/factories/user.rb                       | 13 ++--
 spec/features/school/listing_schools_spec.rb | 63 +++++++++++++-------
 spec/lib/profile_api_client_spec.rb          |  4 +-
 spec/requests/join_controller_spec.rb        | 19 +++---
 spec/services/student_role_service_spec.rb   | 30 ++++++++++
 spec/support/user_profile_mock.rb            |  3 +-
 9 files changed, 112 insertions(+), 38 deletions(-)
 create mode 100644 app/services/student_role_service.rb
 create mode 100644 spec/services/student_role_service_spec.rb

diff --git a/app/controllers/concerns/identifiable.rb b/app/controllers/concerns/identifiable.rb
index af43b0b3e..956cd6d80 100644
--- a/app/controllers/concerns/identifiable.rb
+++ b/app/controllers/concerns/identifiable.rb
@@ -16,6 +16,8 @@ def load_current_user
 
     @current_user = User.from_token(token:)
     return if @current_user.blank?
+
+    StudentRoleService.ensure_student_role(@current_user)
     return unless RequestStore.respond_to?(:active?) && RequestStore.active?
 
     RequestStore.store[:safeguarding_flag_users_by_token] ||= {}
diff --git a/app/models/user.rb b/app/models/user.rb
index a3fcc0870..0c8957b1e 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -17,6 +17,7 @@ class User
     picture
     postcode
     profile
+    school_id
     token
     username
     roles
@@ -54,6 +55,10 @@ def student?
     Role.student.exists?(user_id: id)
   end
 
+  def school_id
+    @school_id if student_account_type?
+  end
+
   def student_account_type?
     sub.to_s.starts_with?('student:')
   end
diff --git a/app/services/student_role_service.rb b/app/services/student_role_service.rb
new file mode 100644
index 000000000..0c7b08d78
--- /dev/null
+++ b/app/services/student_role_service.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class StudentRoleService
+  class << self
+    def ensure_student_role(user)
+      return unless user.student_account_type?
+
+      Role.student.find_or_create_by!(school_id: user.school_id, user_id: user.id)
+    end
+  end
+end
diff --git a/spec/factories/user.rb b/spec/factories/user.rb
index e3240bd69..f0fc71afd 100644
--- a/spec/factories/user.rb
+++ b/spec/factories/user.rb
@@ -18,10 +18,15 @@
     end
 
     factory :student do
+      transient do
+        school { nil }
+      end
+
       email { nil }
       username { Faker::Internet.username }
       sso_providers { [] } # standard students have no SSO providers
       sub { "student:#{id}" }
+      school_id { school&.id || create(:school).id }
 
       trait :sso do
         email { Faker::Internet.email }
@@ -29,12 +34,8 @@
         sso_providers { ['google'] } # SSO students have SSO providers
       end
 
-      transient do
-        school { nil }
-      end
-
-      after(:create) do |user, context|
-        create(:student_role, user_id: user.id, school: context.school)
+      after(:create) do |user|
+        create(:student_role, user_id: user.id, school_id: user.school_id)
       end
     end
 
diff --git a/spec/features/school/listing_schools_spec.rb b/spec/features/school/listing_schools_spec.rb
index 6b45e7384..46dc3a8f6 100644
--- a/spec/features/school/listing_schools_spec.rb
+++ b/spec/features/school/listing_schools_spec.rb
@@ -3,37 +3,56 @@
 require 'rails_helper'
 
 RSpec.describe 'Listing schools', type: :request do
-  before do
-    school = create(:school, name: 'Test School')
-    owner = create(:owner, school:)
-    authenticated_in_hydra_as(owner)
-  end
-
   let(:headers) { { Authorization: UserProfileMock::TOKEN } }
+  let(:school) { create(:school, name: 'Test School') }
 
-  it 'responds 200 OK' do
-    get('/api/schools', headers:)
-    expect(response).to have_http_status(:ok)
+  it 'responds 401 Unauthorized when no token is given' do
+    get '/api/schools'
+    expect(response).to have_http_status(:unauthorized)
   end
 
-  it 'responds with the schools JSON' do
-    get('/api/schools', headers:)
-    data = JSON.parse(response.body, symbolize_names: true)
+  context 'when the user is a school owner' do
+    let(:user) { create(:owner, school:) }
 
-    expect(data.first[:name]).to eq('Test School')
-  end
+    before do
+      authenticated_in_hydra_as(user)
+    end
 
-  it 'only includes schools the user belongs to' do
-    create(:school, id: SecureRandom.uuid)
+    it 'responds 200 OK' do
+      get('/api/schools', headers:)
+      expect(response).to have_http_status(:ok)
+    end
 
-    get('/api/schools', headers:)
-    data = JSON.parse(response.body, symbolize_names: true)
+    it 'responds with the schools JSON' do
+      get('/api/schools', headers:)
+      data = JSON.parse(response.body, symbolize_names: true)
+
+      expect(data.first[:name]).to eq('Test School')
+    end
 
-    expect(data.size).to eq(1)
+    it 'only includes schools the user belongs to' do
+      create(:school, id: SecureRandom.uuid)
+
+      get('/api/schools', headers:)
+      data = JSON.parse(response.body, symbolize_names: true)
+
+      expect(data.size).to eq(1)
+    end
+
+    it 'responds 401 Unauthorized when no token is given' do
+      get '/api/schools'
+      expect(response).to have_http_status(:unauthorized)
+    end
   end
 
-  it 'responds 401 Unauthorized when no token is given' do
-    get '/api/schools'
-    expect(response).to have_http_status(:unauthorized)
+  it 'creates a student role when the students token includes a school claim but they have no school already' do
+    user = build(:student, school_id: school.id)
+    authenticated_in_hydra_as(user, :student)
+
+    expect { get('/api/schools', headers:) }
+      .to change { Role.student.where(school:, user_id: user.id).count }.by(1)
+    data = JSON.parse(response.body, symbolize_names: true)
+
+    expect(data.first[:name]).to eq('Test School')
   end
 end
diff --git a/spec/lib/profile_api_client_spec.rb b/spec/lib/profile_api_client_spec.rb
index 0b9dfb446..1033baab1 100644
--- a/spec/lib/profile_api_client_spec.rb
+++ b/spec/lib/profile_api_client_spec.rb
@@ -410,8 +410,8 @@ def list_school_students
     let(:username) { 'username' }
     let(:password) { 'password' }
     let(:name) { 'name' }
-    let(:school) { build(:school, id: SecureRandom.uuid) }
-    let(:student) { create(:student, school:) }
+    let(:school) { create(:school, id: SecureRandom.uuid) }
+    let(:student) { create(:student, school_id: school.id) }
     let(:update_student_url) { "#{api_url}/api/v1/schools/#{school.id}/students/#{student.id}" }
 
     before do
diff --git a/spec/requests/join_controller_spec.rb b/spec/requests/join_controller_spec.rb
index beddd9d94..03281452f 100644
--- a/spec/requests/join_controller_spec.rb
+++ b/spec/requests/join_controller_spec.rb
@@ -5,7 +5,7 @@
 RSpec.describe 'Join endpoint' do
   let(:school) { create(:school, code: '12-34-56') }
   let(:school_class) { create(:school_class, school:, join_code: 'B123-C456') }
-  let(:student) { build(:student, email: 'student@example.edu') }
+  let(:student) { build(:student, email: 'student@example.edu', school_id: school.id) }
   let(:teacher) { build(:teacher, email: 'teacher@example.edu') }
   let(:owner) { build(:owner, email: 'owner@example.edu') }
   let(:headers) { { Authorization: UserProfileMock::TOKEN } }
@@ -43,9 +43,8 @@
     end
 
     context 'when the user is authenticated as a student' do
-      before { authenticated_in_hydra_as(student, :student) }
-
       it 'returns status: joinable when the user can join' do
+        authenticated_in_hydra_as(student, :student)
         get "/api/join/#{school_class.join_code}", headers: headers
 
         data = JSON.parse(response.body, symbolize_names: true)
@@ -53,6 +52,7 @@
       end
 
       it 'returns status: already_member when the user is already in the class' do
+        authenticated_in_hydra_as(student, :student)
         create(:student_role, school:, user_id: student.id)
         ClassStudent.create!(school_class:, student_id: student.id)
 
@@ -64,7 +64,8 @@
 
       it 'returns status: wrong_school when the user belongs to a different school' do
         other_school = create(:school)
-        create(:student_role, school: other_school, user_id: student.id)
+        student = build(:student, school_id: other_school.id)
+        authenticated_in_hydra_as(student, :student)
 
         get "/api/join/#{school_class.join_code}", headers: headers
 
@@ -129,9 +130,8 @@
     end
 
     context 'when the user is authenticated as a student' do
-      before { authenticated_in_hydra_as(student, :student) }
-
       it 'adds the user to the school' do
+        authenticated_in_hydra_as(student, :student)
         expect do
           post "/api/join/#{school_class.join_code}", headers: headers
         end.to change(ClassStudent, :count).by(1).and change(Role, :count).by(1)
@@ -143,6 +143,7 @@
       end
 
       it 'is idempotent when the user is already in the class' do
+        authenticated_in_hydra_as(student, :student)
         create(:student_role, school:, user_id: student.id)
         ClassStudent.create!(school_class:, student_id: student.id)
 
@@ -154,6 +155,7 @@
       end
 
       it 'does not duplicate the school role if the user is already in the school' do
+        authenticated_in_hydra_as(student, :student)
         create(:student_role, school:, user_id: student.id)
 
         expect do
@@ -165,7 +167,8 @@
 
       it 'responds with 403 wrong_school when the user belongs to a different school' do
         other_school = create(:school)
-        create(:student_role, school: other_school, user_id: student.id)
+        student = build(:student, school_id: other_school.id)
+        authenticated_in_hydra_as(student, :student)
 
         post "/api/join/#{school_class.join_code}", headers: headers
 
@@ -175,12 +178,14 @@
       end
 
       it 'responds with 404 when the join code does not exist' do
+        authenticated_in_hydra_as(student, :student)
         post '/api/join/INVALID123', headers: headers
         expect(response).to have_http_status(:not_found)
       end
 
       # rubocop:disable-next RSpec/AnyInstance
       it 'responds with 500 when action_status returns an unexpected value' do
+        authenticated_in_hydra_as(student, :student)
         allow_any_instance_of(Api::JoinController).to receive(:action_status).and_return(:something_unexpected)
 
         post "/api/join/#{school_class.join_code}", headers: headers
diff --git a/spec/services/student_role_service_spec.rb b/spec/services/student_role_service_spec.rb
new file mode 100644
index 000000000..369a53e31
--- /dev/null
+++ b/spec/services/student_role_service_spec.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe StudentRoleService do
+  let(:school) { create(:school) }
+
+  describe '.ensure_student_role' do
+    it 'does not create a role for a user without a student account' do
+      user = build(:user, school_id: school.id)
+
+      expect { described_class.ensure_student_role(user) }.not_to change(Role, :count)
+    end
+
+    it 'creates a student role in the school of a student account' do
+      user = build(:student, school_id: school.id)
+
+      described_class.ensure_student_role(user)
+
+      expect(Role.student.find_by(user_id: user.id, school_id: school.id)).to be_present
+    end
+
+    it 'does not create another role when the student role already exists' do
+      user = build(:student, school_id: school.id)
+      create(:student_role, user_id: user.id, school:)
+
+      expect { described_class.ensure_student_role(user) }.not_to change(Role, :count)
+    end
+  end
+end
diff --git a/spec/support/user_profile_mock.rb b/spec/support/user_profile_mock.rb
index c694d158c..3a7232dbc 100644
--- a/spec/support/user_profile_mock.rb
+++ b/spec/support/user_profile_mock.rb
@@ -32,7 +32,8 @@ def user_to_hash(user, user_type, id_field = :id)
       name: user.name,
       email: user.email,
       username: user.username,
-      roles: user.roles
+      roles: user.roles,
+      school_id: user.school_id
     }
   end
 

From 1db92085e972bb6f11209a6e00b945dbc59700d3 Mon Sep 17 00:00:00 2001
From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com>
Date: Tue, 15 Sep 2026 14:43:57 +0100
Subject: [PATCH 3/4] Simplify join controller

Now we're creating roles automatically in the the student roles service this code can be simplified. Note that the request specs fail if the StudentRoleService action was commented out.

I've also changed the code that created class students as this was more complex that it needed to be.
---
 app/controllers/api/join_controller.rb | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/app/controllers/api/join_controller.rb b/app/controllers/api/join_controller.rb
index 1b2589ee2..2ad41fd06 100644
--- a/app/controllers/api/join_controller.rb
+++ b/app/controllers/api/join_controller.rb
@@ -19,7 +19,7 @@ def create
       when :already_member
         render json: {}, status: :ok
       when :joinable
-        add_student_to_school_and_class
+        add_student_to_class
         render json: {}, status: :ok
       else
         raise "Unexpected join action_status: #{action_status.inspect}"
@@ -43,13 +43,8 @@ def action_status
       @action_status ||= JoinStatusService.new(school: @school, school_class: @school_class, user: current_user).call
     end
 
-    def add_student_to_school_and_class
-      ActiveRecord::Base.transaction do
-        Role.find_or_create_by!(school: @school, user_id: current_user.id, role: :student)
-        ClassStudent.find_or_create_by!(school_class: @school_class, student_id: current_user.id) do |class_student|
-          class_student.student = current_user
-        end
-      end
+    def add_student_to_class
+      @school_class.students.find_or_create_by!(student_id: current_user.id)
     rescue ActiveRecord::RecordNotUnique
       # Concurrent join request for the same user/class — DB unique index
       # caught a race we couldn't catch at validation time. Already enrolled.

From eb868cec6e3738d1ba7dfe56cb77604e9f7bf338 Mon Sep 17 00:00:00 2001
From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com>
Date: Tue, 15 Sep 2026 16:46:32 +0100
Subject: [PATCH 4/4] Don't error if school id isn't set in token yet

While eventually this will be set in student tokens, there might be a period of time when deploying where it isn't. Make sure we don't error in this case
---
 app/services/student_role_service.rb       | 2 +-
 spec/services/student_role_service_spec.rb | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/app/services/student_role_service.rb b/app/services/student_role_service.rb
index 0c7b08d78..ca892568f 100644
--- a/app/services/student_role_service.rb
+++ b/app/services/student_role_service.rb
@@ -3,7 +3,7 @@
 class StudentRoleService
   class << self
     def ensure_student_role(user)
-      return unless user.student_account_type?
+      return unless user.student_account_type? && user.school_id.present?
 
       Role.student.find_or_create_by!(school_id: user.school_id, user_id: user.id)
     end
diff --git a/spec/services/student_role_service_spec.rb b/spec/services/student_role_service_spec.rb
index 369a53e31..2659a980b 100644
--- a/spec/services/student_role_service_spec.rb
+++ b/spec/services/student_role_service_spec.rb
@@ -12,6 +12,12 @@
       expect { described_class.ensure_student_role(user) }.not_to change(Role, :count)
     end
 
+    it 'does not create a role when the school_id is not set' do
+      user = build(:student, school_id: nil)
+
+      expect { described_class.ensure_student_role(user) }.not_to change(Role, :count)
+    end
+
     it 'creates a student role in the school of a student account' do
       user = build(:student, school_id: school.id)