From 149864daf8a640b72227256f78ce78ae26397d90 Mon Sep 17 00:00:00 2001 From: David Young Date: Fri, 1 Nov 2024 11:00:59 +0000 Subject: [PATCH 1/4] Added validations Added email presence and uniqueness validations to Support and Provider Users. Also replaced `before` callbacks with Rails `normalizes` for downcasing email addresses --- app/models/provider_user.rb | 12 +++++------- app/models/support_user.rb | 13 +++++-------- spec/models/provider_user_spec.rb | 7 +++++++ spec/models/support_user_spec.rb | 9 ++++----- 4 files changed, 21 insertions(+), 20 deletions(-) diff --git a/app/models/provider_user.rb b/app/models/provider_user.rb index 428f69118e8..226c1faee7c 100644 --- a/app/models/provider_user.rb +++ b/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 diff --git a/app/models/support_user.rb b/app/models/support_user.rb index f7ec59320c2..e7140cdc855 100644 --- a/app/models/support_user.rb +++ b/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 diff --git a/spec/models/provider_user_spec.rb b/spec/models/provider_user_spec.rb index db30c568ba2..3a994402655 100644 --- a/spec/models/provider_user_spec.rb +++ b/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: 'Bob.Roberts@example.com') diff --git a/spec/models/support_user_spec.rb b/spec/models/support_user_spec.rb index 7dabd3552ce..a8b2f20f1c0 100644 --- a/spec/models/support_user_spec.rb +++ b/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: 'bob@example.com') - duplicate_support_user = build(:support_user, email_address: 'Bob@example.com') - 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 From ba85cad2d8d5d28a17e679992814a58d6e68e244 Mon Sep 17 00:00:00 2001 From: David Young Date: Fri, 1 Nov 2024 11:03:05 +0000 Subject: [PATCH 2/4] DfE Sign in callback checks validity of user Using the new validation rules we now check that the `DsiProfile.update_profile_from_dfe_sign_in` has happened before continuing. There seems to be a lack of tests around the request which will be added in another commit --- app/controllers/dfe_sign_in_controller.rb | 3 +-- spec/lib/dsi_profile_spec.rb | 25 ++++++++++++++++------- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/app/controllers/dfe_sign_in_controller.rb b/app/controllers/dfe_sign_in_controller.rb index 5d51df6d13b..c6bdb0b3ea2 100644 --- a/app/controllers/dfe_sign_in_controller.rb +++ b/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) diff --git a/spec/lib/dsi_profile_spec.rb b/spec/lib/dsi_profile_spec.rb index 302150f5a79..6346abfd7a4 100644 --- a/spec/lib/dsi_profile_spec.rb +++ b/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: 'original_provider@email.address') } + let(:email_address) { 'provider_user@email.address' } 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 From ab91b0650be9d7696eb92ff8139cc0e7c4049982 Mon Sep 17 00:00:00 2001 From: David Young Date: Fri, 1 Nov 2024 11:29:11 +0000 Subject: [PATCH 3/4] Basic request spec created for DfESignInController#callback --- app/lib/dsi_profile.rb | 1 + app/models/dfe_sign_in_user.rb | 8 ++++---- .../dfe_sign_in_controller_callbacks_spec.rb | 17 +++++++++++++++++ 3 files changed, 22 insertions(+), 4 deletions(-) create mode 100644 spec/requests/dfe_sign_in_controller_callbacks_spec.rb diff --git a/app/lib/dsi_profile.rb b/app/lib/dsi_profile.rb index 171873100e5..df08e810acc 100644 --- a/app/lib/dsi_profile.rb +++ b/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 diff --git a/app/models/dfe_sign_in_user.rb b/app/models/dfe_sign_in_user.rb index 5be6ddb9154..f428e3d3c14 100644 --- a/app/models/dfe_sign_in_user.rb +++ b/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 diff --git a/spec/requests/dfe_sign_in_controller_callbacks_spec.rb b/spec/requests/dfe_sign_in_controller_callbacks_spec.rb new file mode 100644 index 00000000000..f85d86e7b9d --- /dev/null +++ b/spec/requests/dfe_sign_in_controller_callbacks_spec.rb @@ -0,0 +1,17 @@ +require 'rails_helper' + +RSpec.describe 'DfESignInController#callbacks' do + include DfESignInHelpers + + before do + OmniAuth.config.test_mode = true + end + + describe 'GET /auth/dfe/callback' do + it 'is forbidden by default' do + get auth_dfe_callback_path + + expect(response).to have_http_status(:forbidden) + end + end +end From 3bcdc5fc06ac2487800e5760d62d727e908e99f7 Mon Sep 17 00:00:00 2001 From: David Young Date: Fri, 1 Nov 2024 12:52:03 +0000 Subject: [PATCH 4/4] Added Request specs for DfE Sign in Added several scenarios to ensure the correct behaviours take place when signing in using DfE Sign in (OAuth) --- .../dfe_sign_in_controller_callbacks_spec.rb | 103 ++++++++++++++++-- 1 file changed, 96 insertions(+), 7 deletions(-) diff --git a/spec/requests/dfe_sign_in_controller_callbacks_spec.rb b/spec/requests/dfe_sign_in_controller_callbacks_spec.rb index f85d86e7b9d..fa96be9d106 100644 --- a/spec/requests/dfe_sign_in_controller_callbacks_spec.rb +++ b/spec/requests/dfe_sign_in_controller_callbacks_spec.rb @@ -3,15 +3,104 @@ RSpec.describe 'DfESignInController#callbacks' do include DfESignInHelpers - before do - OmniAuth.config.test_mode = true - end - describe 'GET /auth/dfe/callback' do - it 'is forbidden by default' do - get auth_dfe_callback_path + let(:omni_auth_hash) do + fake_dfe_sign_in_auth_hash( + email_address: 'some@email.address', + 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: 'some@email.address') } + + 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: 'some@email.address') } + + 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) + expect(response).to have_http_status(:forbidden) + end end end end