Check validity of User when using DfE Sign in callback #10020
Conversation
dcyoung-dev
had a problem deploying
to
review_aks
GitHub Actions
Failure
| @@ -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) } | |||
There was a problem hiding this comment.
support_user was never used in this spec
| @@ -41,6 +41,17 @@ | |||
| described_class.update_profile_from_dfe_sign_in dfe_user: dfe_user_no_email, local_user: provider_user | |||
There was a problem hiding this comment.
I was tempted to update all the specs in this context to be in the same format
- setup
- result
- expectation
Do you think it'd be worth while?
| @@ -1,6 +1,13 @@ | |||
| require 'rails_helper' | |||
|
|
|||
| RSpec.describe ProviderUser do | |||
| describe 'validations' do | |||
| let!(:existing_provider_user) { create(:provider_user) } | |||
There was a problem hiding this comment.
Required for the validate_uniqueness_of check - ShouldaMatchers will use the email address from an existing ProviderUser when testing for uniqueness
| 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 |
There was a problem hiding this comment.
Now covered by
it { is_expected.to validate_uniqueness_of(:email_address).case_insensitive
| context 'there are no DfE sign omniauth values set' do | ||
| let(:omni_auth_hash) { nil } | ||
|
|
||
| it 'is forbidden by default' do |
There was a problem hiding this comment.
Before changes in this PR - this spec fails with
Failure/Error: 'id_token' => omniauth_payload['credentials']['id_token'],
NoMethodError: undefined method `[]' for nil
| 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) |
There was a problem hiding this comment.
Before changes in this PR - this spec fails with
expected the response to have status code :forbidden (403) but it was :unprocessable_entity (422)
| 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 |
There was a problem hiding this comment.
Before changes in this PR - this spec fails with
ActiveRecord::RecordNotUnique: PG::UniqueViolation: ERROR: duplicate key value violates unique constraint "index_provider_users_on_email_address"
DETAIL: Key (email_address)=([email protected]) already exists.
Added email presence and uniqueness validations to Support and Provider Users. Also replaced `before` callbacks with Rails `normalizes` for downcasing email addresses
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
Added several scenarios to ensure the correct behaviours take place when signing in using DfE Sign in (OAuth)
dcyoung-dev
force-pushed
the
dy-bug-dsi-sign-in-duplicate-email-error
branch
from
d1e323e to
3bcdc5f
Compare
dcyoung-dev
temporarily deployed
to
review_aks
GitHub Actions
Inactive
## Context A bug was introduced in #10020 which stops the sign in by-pass from working locally or on review apps. ## Changes proposed in this pull request - Removed conditional that would return `nil` if there are no updates to be done
Context
We have
ActiveRecord::RecordNotUniqueerrors appearing in Sentry related to the email address of a DfE Sign in user already being used.The current implementation does not handle this error.
Changes proposed in this pull request
DfESignInController#callbackto check for a successfulupdatebefore redirecting appropriatelyGuidance to review
Things to check