Fix adding new phone number redirects to SP (LG-3784)#4660
Merged
mitchellhenke merged 6 commits intomasterfrom Feb 11, 2021
Merged
Fix adding new phone number redirects to SP (LG-3784)#4660mitchellhenke merged 6 commits intomasterfrom
mitchellhenke merged 6 commits intomasterfrom
Conversation
mitchellhenke
requested review from
achapm,
stevegsa and
zachmargolis
and removed request for
zachmargolis
zachmargolis
reviewed
Feb 9, 2021
| click_submit_default | ||
|
|
||
| expect(current_url).to start_with('http://localhost:7654/auth/result') | ||
| expect(page.get_rack_session.keys).to include('sp') |
Contributor
There was a problem hiding this comment.
Should we just flip the assertion to expect it's not there? expect().to_not include('sp')
Ditto for the one below
mitchellhenke
deleted the
mitchellhenke/new-phone-number-bad-redirect-lg-3784
branch
zachmargolis
added a commit
that referenced
this pull request
Feb 25, 2021
This reverts commit 9155949.
zachmargolis
added a commit
that referenced
this pull request
Feb 25, 2021
zachmargolis
added a commit
that referenced
this pull request
Feb 25, 2021
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
4 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.
related to #1561 and #2426
The issue at hand is authenticating with an SP, then going back to the account page and adding a phone causes the user to be redirected back to the SP, when they'd expect to go back to the account page. The root issue is adding a phone ends up here, and hitting the SP redirect due to the SP context still existing in the session: https://github.com/18F/identity-idp/blob/6a6b321/app/controllers/application_controller.rb#L162-L166
The crux of the changes is adding
session.delete(:sp)as part ofdelete_branded_experience, which is called when redirecting back to an SP. My understanding is at that point, the SP authentication is complete, and it should be safe to delete any session data related to it.There were some tests and pieces of functionality that relied upon the SP session existing after successful authentication though:
sp_sessionto prevent multiple quick SP authentications, but it should be safe to move it to the regularsessionsp_sessionto add the redirect_uris to theform-actionCSP header. We shouldn't need to add those toform-actionsince we aren't submitting forms to them, just redirecting back./two_factor_authenticationand still be redirected back to the SP. It wasn't clear from LG-554 Fix already authenticated users redirecting to account page #2426 why that behavior was being verified, but I think we get into muddy territory if SP context is still around after completing authentication?