Don't delete SP info from session after redirect#1561
Merged
Conversation
Contributor
There was a problem hiding this comment.
should we make these positive expectations?
expect(page.get_rack_session.keys).to include('sp')
Contributor
Author
There was a problem hiding this comment.
Good idea. Fixed. PTAL
Contributor
There was a problem hiding this comment.
same, should we make sure this keeps the sp in the session?
expect(session.key?(:sp)).to eq(true)
**Why**: I don't think there's a valid reason to do that. Digging into the git history, it looks like the deletion of the SP info from the session was introduced in the same PR that implemented the branded experience. I believe the reasoning was that unless the branded experience was deleted, returning to the IdP after signing out from the SP would keep the branded experience around, which is not the case since the entire session is deleted upon logout. Deleting the SP info from the session is resulting in an exception in the following scenario: - A user launches a mobile app that integrates with login.gov - The user completes the account creation process on their mobile device - Once they arrive on the completions page and click "Continue", they are prompted to launch the mobile app. Whether the user chooses to launch the app or click Cancel, the IdP web page stays where it is. - If the user comes back to the IdP page at a later time and tries to hit the Continue button again, they will get an error because `CompletionsController#update` depends on `sp_session[:request_url]` being present. **How**: - Don't delete the SP info from the session - To allow the user to visit the IdP after being redirected back to the SP, update `SessionsController#check_user_needs_redirect` to redirect to `signed_in_path` instead of `after_sign_in_path_for(current_user)` because the latter will redirect back to `sp_session[:request_url]` now that we are no longer deleting `sp_session`.
monfresh
force-pushed
the
mb-preserve-sp-in-session-after-redirect
branch
from
786aa4c to
913b4a2
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Why: I don't think there's a valid reason to do that. Digging
into the git history, it looks like the deletion of the SP info from
the session was introduced in the same PR that implemented the branded
experience. I believe the reasoning was that unless the branded
experience was deleted, returning to the IdP after signing out from the
SP would keep the branded experience around, which is not the case since
the entire session is deleted upon logout.
Deleting the SP info from the session is resulting in an exception in
the following scenario:
they are prompted to launch the mobile app. Whether the user chooses to
launch the app or click Cancel, the IdP web page stays where it is.
hit the Continue button again, they will get an error because
CompletionsController#updatedepends onsp_session[:request_url]being present.
How:
the SP, update
SessionsController#check_user_needs_redirecttoredirect to
signed_in_pathinstead ofafter_sign_in_path_for(current_user)because the latter will redirectback to
sp_session[:request_url]now that we are no longer deletingsp_session.