Improve error handling and sign out flow when authenticating with DfE Sign-in #150
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Extract methods to make the flow a little clearer (including the difference in behaviour when bypass mode is active). This also prepares the controller for further changes related to storing the organisation details in the session, which we'll need to display a more informative 401 page when authorisation fails.
- The id_token is needed to sign out of DSI properly when signing out of the service, so that users can change the organisation they sign in with, for example. We'll implement this in a subsequent commit. - The organisation name is needed so we can display it on the 401 page. - Use reset_session in the SignOutController to clear the various DSI attributes we set. - Add a system spec for signing out.
When the user clicks the Sign out link in the header, we now sign them out of DSI before sending them to the sign out page. We do this with the following: - Ensure the token_id is set in the sign out link in the header (this token is made available in the session cookie during the sign in phase). - Add a dedicated strategy file (DfEOpenIDConnect) so we can extract this token and make it part of the post_logout_redirect_uri. This is required for the OIDC library used by DSI to redirect correctly after handling the sign out.
Amend the content so that we: - include the name of the organisation the user is signed in as - provide a sign out link with the appropriate token set to ensure a full DSI sign out occurs, followed by a redirect back to the service
Simplify the UI so that the "sign out and start again" link is the only way for the user to continue.
Sometimes an error arises during the omniauth libary's communication with the auth provider (DSI in this case). In those cases, we need some kind of basic error handling. Here we: - Add an AuthFailuresController - Configure omniauth to use it as a failure app - Add specific handling for "sessionexpired" errors - Add generic error handling for anything else - Add a FlashMessageComponent so we can present a generic error message to the user
malcolmbaig
force-pushed
the
53-update-dsi-sign-out
branch
from
33c7a0a to
3af8dd7
Compare
There are cases where the auth flow between this service and DSI can get into an inconsistent state, resulting in CSRF and state errors from the auth provider. For example: - authenticate with an unauthorised organisation - open a new tab to attempt a new sign in - submit the form and get a state error from DSI To resolve these cases, and to make the auth flow more robust, we do the following: - Reset the session on the sign in page instead of clearing specific session attributes in the omniauth callback controller. This gives us a simpler baseline state from which to attempt authentication, regardless of how the user arrives at the page (whether through direct navigation, or a redirect following an error). - Add an oauth_failure param when redirecting after an oauth error, so that we can continue to display a flash message on the sign in page despite having reset the session. - Add system specs for this behaviour.
malcolmbaig
force-pushed
the
53-update-dsi-sign-out
branch
from
3af8dd7 to
ccc8cdf
Compare
steventux
approved these changes
Nov 8, 2023
|
|
||
| redirect_to root_path | ||
| def check_user_access_to_service |
There was a problem hiding this comment.
@felixclack made a point on a related PR that renaming this method to role simplifies the code somewhat. Not a blocker.
| @@ -1,7 +1,15 @@ | |||
| class SignInController < ApplicationController | |||
| skip_before_action :authenticate_dsi_user! | |||
| skip_before_action :handle_expired_session! | |||
| before_action :reset_session | |||
Use memoization instead of local variable assignment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
@steventux completed some work on AYTQ recently that improves error handling and sign out behaviour:
DFE-Digital/access-your-teaching-qualifications#365
https://github.com/DFE-Digital/access-your-teaching-qualifications/pull/
We want to implement this in CCBL as well.
Changes proposed in this pull request
The commit history provides the best summary of how the work in the PRs above has been extracted out and grouped.
In brief though:
dfeomniauth strategy to support signing out of DSIGuidance to review
Can be tested manually if your local CCBL is pointing to dev DSI.
Link to Trello card
https://trello.com/c/1Cfdo0ip/53-cbl-complete-the-dfe-signin-integration
Checklist