Check validity of User when using DfE Sign in callback

app/controllers/dfe_sign_in_controller.rb

@@ -10,8 +10,7 @@ def callback
     @target_path = session['post_dfe_sign_in_path']
     @local_user = local_user
 
-    if @local_user
-      DsiProfile.update_profile_from_dfe_sign_in(dfe_user: @dfe_sign_in_user, local_user: @local_user)
+    if @local_user && DsiProfile.update_profile_from_dfe_sign_in(dfe_user: @dfe_sign_in_user, local_user: @local_user)
       @local_user.update!(last_signed_in_at: Time.zone.now)
 
       if @local_user.is_a?(SupportUser)

app/lib/dsi_profile.rb

@@ -6,6 +6,7 @@ def self.update_profile_from_dfe_sign_in(dfe_user:, local_user:)
     end
     fields_to_update[:first_name] = dfe_user.first_name if dfe_user.first_name.present?
     fields_to_update[:last_name] = dfe_user.last_name if dfe_user.last_name.present?
+
     local_user.update(fields_to_update) if fields_to_update.present?
   end
 end

app/models/dfe_sign_in_user.rb

@@ -28,12 +28,12 @@ def needs_dsi_signout?
 
   def self.begin_session!(session, omniauth_payload)
     session['dfe_sign_in_user'] = {
-      'email_address' => omniauth_payload['info']['email'],
+      'email_address' => omniauth_payload.dig('info', 'email'),
       'dfe_sign_in_uid' => omniauth_payload['uid'],
-      'first_name' => omniauth_payload['info']['first_name'],
-      'last_name' => omniauth_payload['info']['last_name'],
+      'first_name' => omniauth_payload.dig('info', 'first_name'),
+      'last_name' => omniauth_payload.dig('info', 'last_name'),
       'last_active_at' => Time.zone.now,
-      'id_token' => omniauth_payload['credentials']['id_token'],
+      'id_token' => omniauth_payload.dig('credentials', 'id_token'),
     }
   end
 

app/models/provider_user.rb

@@ -8,8 +8,12 @@ class ProviderUser < ApplicationRecord
   attr_accessor :impersonator
 
   validates :dfe_sign_in_uid, uniqueness: true, allow_nil: true
+  validates :email_address,
+            presence: true,
+            uniqueness: { case_sensitive: false },
+            format: { with: URI::MailTo::EMAIL_REGEXP }
 
-  before_save :downcase_email_address
+  normalizes :email_address, with: ->(email) { email.downcase.strip }
 
   audited except: [:last_signed_in_at]
   has_associated_audits
@@ -60,10 +64,4 @@ def authorisation
   def can_manage_organisations?
     provider_permissions.exists?(manage_organisations: true)
   end
-
-private
-
-  def downcase_email_address
-    self.email_address = email_address.downcase
-  end
 end

app/models/support_user.rb

@@ -4,9 +4,12 @@ class SupportUser < ApplicationRecord
 
   attr_accessor :impersonated_provider_user
   validates :dfe_sign_in_uid, presence: true
-  validates :email_address, presence: true, uniqueness: true
+  validates :email_address,
+            presence: true,
+            uniqueness: { case_sensitive: false },
+            format: { with: URI::MailTo::EMAIL_REGEXP }
 
-  before_validation :downcase_email_address
+  normalizes :email_address, with: ->(email) { email.downcase.strip }
 
   audited except: [:last_signed_in_at]
 
@@ -24,10 +27,4 @@ def self.load_from_session(session)
   def display_name
     [first_name, last_name].join(' ').presence || email_address
   end
-
-private
-
-  def downcase_email_address
-    self.email_address = email_address.downcase
-  end
 end

spec/lib/dsi_profile_spec.rb

@@ -2,9 +2,8 @@
 
 RSpec.describe DsiProfile do
   describe '#update_profile_from_dfe_sign_in' do
-    let(:provider_user) { create(:provider_user) }
-    let(:support_user) { create(:provider_user) }
-    let(:email_address) { Faker::Internet.email }
+    let(:provider_user) { create(:provider_user, email_address: '[email protected]') }
+    let(:email_address) { '[email protected]' }
     let(:dfe_user) do
       DfESignInUser.new(
         email_address:,
@@ -14,11 +13,12 @@
       )
     end
 
-    context 'local_user\'s email_address' do
+    context "local_user's email_address" do
       it 'is updated if uid is previously known' do
-        expect {
-          described_class.update_profile_from_dfe_sign_in dfe_user:, local_user: provider_user
-        }.to change(provider_user, :email_address).to(email_address)
+        result = described_class.update_profile_from_dfe_sign_in dfe_user:, local_user: provider_user
+
+        expect(result).to be_truthy
+        expect(provider_user.reload.email_address).to eq(email_address)
       end
 
       it 'is not updated if uid is not yet established' do
@@ -41,6 +41,17 @@
           described_class.update_profile_from_dfe_sign_in dfe_user: dfe_user_no_email, local_user: provider_user
         }.not_to change(provider_user, :email_address)
       end
+
+      context 'the email is already used by another user' do
+        it 'is not updated' do
+          _other_provider_user = create(:provider_user, email_address: email_address)
+
+          result = described_class.update_profile_from_dfe_sign_in(dfe_user: dfe_user, local_user: provider_user)
+
+          expect(result).to be_falsey
+          expect(provider_user.reload.email_address).not_to eq(email_address)
+        end
+      end
     end
 
     context 'local_user\'s profile fields' do

spec/models/provider_user_spec.rb

@@ -1,6 +1,13 @@
 require 'rails_helper'
 
 RSpec.describe ProviderUser do
+  describe 'validations' do
+    let!(:existing_provider_user) { create(:provider_user) }
+
+    it { is_expected.to validate_presence_of(:email_address) }
+    it { is_expected.to validate_uniqueness_of(:email_address).case_insensitive }
+  end
+
   describe '#downcase_email_address' do
     it 'saves email_address in lower case' do
       provider_user = create(:provider_user, email_address: '[email protected]')

spec/models/support_user_spec.rb

@@ -2,11 +2,10 @@
 
 RSpec.describe SupportUser do
   describe 'validations' do
-    it 'flags email addresses that differ only by case as duplicates' do
-      create(:support_user, email_address: '[email protected]')
-      duplicate_support_user = build(:support_user, email_address: '[email protected]')
-      expect(duplicate_support_user).not_to be_valid
-    end
+    let!(:existing_support_user) { create(:support_user) }
+
+    it { is_expected.to validate_presence_of(:email_address) }
+    it { is_expected.to validate_uniqueness_of(:email_address).case_insensitive }
   end
 
   describe '#downcase_email_address' do

spec/requests/dfe_sign_in_controller_callbacks_spec.rb

@@ -0,0 +1,106 @@
+require 'rails_helper'
+
+RSpec.describe 'DfESignInController#callbacks' do
+  include DfESignInHelpers
+
+  describe 'GET /auth/dfe/callback' do
+    let(:omni_auth_hash) do
+      fake_dfe_sign_in_auth_hash(
+        email_address: '[email protected]',
+        dfe_sign_in_uid: 'DFE_SIGN_IN_UID',
+        first_name: '',
+        last_name: '',
+      )
+    end
+
+    before do
+      OmniAuth.config.test_mode = true
+      OmniAuth.config.mock_auth[:dfe] = omni_auth_hash
+    end
+
+    context 'there are no DfE sign omniauth values set' do
+      let(:omni_auth_hash) { nil }
+
+      it 'is forbidden by default' do
+        get auth_dfe_callback_path
+
+        expect(response).to have_http_status(:forbidden)
+      end
+    end
+
+    context 'when the Support User does not exist' do
+      it 'does not sign in' do
+        get support_interface_sign_in_path # makes sure the session[:post_dfe_sign_in_path] is set
+        get auth_dfe_callback_path
+
+        expect(response).to have_http_status(:forbidden)
+      end
+    end
+
+    context 'when Support User exists with matching dfe_sign_in_uid' do
+      let!(:support_user) { create(:support_user, dfe_sign_in_uid: 'DFE_SIGN_IN_UID') }
+
+      it 'signs the Support User in' do
+        get support_interface_sign_in_path # makes sure the session[:post_dfe_sign_in_path] is set
+        get auth_dfe_callback_path
+
+        expect(response).to redirect_to(support_interface_path)
+      end
+
+      it 'redirects to the Support interface when the post_dfe_sign_in_path is set to the Provider Interface' do
+        # FIXME: Reliance on the session[:post_dfe_sign_in_path] is an anti-pattern
+        skip('The use of session[:post_dfe_sign_in_path] is an anti-pattern')
+
+        get provider_interface_sign_in_path # makes sure the session[:post_dfe_sign_in_path] is set
+        get auth_dfe_callback_path
+
+        expect(response).to redirect_to(support_interface_path)
+      end
+    end
+
+    context 'when a different Support User exists with the same email address' do
+      let!(:support_user) { create(:support_user, dfe_sign_in_uid: 'DFE_SIGN_IN_UID') }
+      let!(:existing_support_user) { create(:support_user, email_address: '[email protected]') }
+
+      it 'does not sign the Support User in' do
+        get support_interface_sign_in_path # makes sure the session[:post_dfe_sign_in_path] is set
+        get auth_dfe_callback_path
+
+        expect(response).to have_http_status(:forbidden)
+      end
+    end
+
+    context 'when Provider User exists with matching dfe_sign_in_uid' do
+      let!(:provider_user) { create(:provider_user, dfe_sign_in_uid: 'DFE_SIGN_IN_UID') }
+
+      it 'signs the Provider User in' do
+        get provider_interface_sign_in_path # makes sure the session[:post_dfe_sign_in_path] is set
+        get auth_dfe_callback_path
+
+        expect(response).to redirect_to(provider_interface_path)
+      end
+
+      it 'redirects to the Provider interface when the post_dfe_sign_in_path is set to the Support Interface' do
+        # FIXME: Reliance on the session[:post_dfe_sign_in_path] is an anti-pattern
+        skip('The use of session[:post_dfe_sign_in_path] is an anti-pattern')
+
+        get support_interface_sign_in_path # makes sure the session[:post_dfe_sign_in_path] is set
+        get auth_dfe_callback_path
+
+        expect(response).to redirect_to(provider_interface_path)
+      end
+    end
+
+    context 'when a different Provider User exists with the same email address' do
+      let!(:provider_user) { create(:provider_user, dfe_sign_in_uid: 'DFE_SIGN_IN_UID') }
+      let!(:existing_provider_user) { create(:provider_user, email_address: '[email protected]') }
+
+      it 'does not sign the Provider User in' do
+        get provider_interface_sign_in_path # makes sure the session[:post_dfe_sign_in_path] is set
+        get auth_dfe_callback_path
+
+        expect(response).to have_http_status(:forbidden)
+      end
+    end
+  end
+end