LG-8432: Update SAML ForceAuthn enforcement to track state in SP session object#7616
LG-8432: Update SAML ForceAuthn enforcement to track state in SP session object#7616julialeague merged 12 commits intomainfrom
Conversation
julialeague
changed the title
julialeague
changed the title
orenyk
left a comment
orenyk
left a comment
There was a problem hiding this comment.
left a few comments/questions, looks good overall!
| # for a SAML service provider request to avoid a sign-out/sign-in loop | ||
| # when ForceAuthn = true in the SAMLRequest (as handled | ||
| # in saml_idp_auth_concern.rb#sign_out_if_forceauthn_is_true_and_user_is_signed_in) | ||
| def sp_session_request_url_with_updated_params(final = false) |
There was a problem hiding this comment.
question: is there ever a case where we'd want to call this and not set the sp_session[:final_auth_request] = true? I'm not immediately seeing the need for the method parameter here but I could def be missing something.
There was a problem hiding this comment.
I removed this parameter after discussing this more with Oren. I introduced the parameter as a way to protect against a certain edge case, which I wasn't sure needed to be protected. Oren confirmed with me the case I had in mind is not needing to be protected from bypassing ForceAuthn.
There was a problem hiding this comment.
commit where I removed the param: 114280b
| end | ||
| end | ||
|
|
||
| # temporarily commenting out this spec because it needs to be updated to work |
There was a problem hiding this comment.
question: I'm guessing this was meant to be deleted?
| :validate_saml_request, | ||
| :validate_service_provider_and_authn_context, | ||
| :store_saml_request, | ||
| :store_saml_request |
There was a problem hiding this comment.
comment: I think this might violate our linters but I'm not sure
spec/features/saml/saml_spec.rb
Outdated
|
|
||
| # visit from SP with force_authn: true | ||
| visit_saml_authn_request_url(overrides: saml_request_overrides) | ||
| expect(page.has_content?( |
There was a problem hiding this comment.
nitpick: I think you can replace this with expect(page).to have_content(...)
spec/features/saml/saml_spec.rb
Outdated
| expect(page.has_content?( | ||
| 'Test SP is using Login.gov to allow you to sign in to your account safely and securely.' | ||
| )).to be true | ||
| expect(page.has_button?('Sign in')).to be true |
There was a problem hiding this comment.
nitpick: similarly, I think this can be expect(page).to have_button('Sign in')
julialeague
changed the title
orenyk
left a comment
orenyk
left a comment
There was a problem hiding this comment.
Looks good to me, nice work!
| session.fetch(:sp, {}) | ||
| end | ||
|
|
||
| # Retrieves the current service provider session hash's logged request URL, if present |
There was a problem hiding this comment.
praise: great job adding documentation to a somewhat unintuitive method!
2 participants
🎫 Ticket
https://cm-jira.usa.gov/browse/LG-8432
🛠 Summary of changes
Our recent updates to the SAML internal flow (see #6922) introduced an issue with our enforcement of the ForceAuthn attribute, where users would be stuck in an indefinite sign-out/sign-in loop for service providers passing ForceAuthn=true in their SAMLRequest. This update fixes that by checking whether we're at the final request which will redirect back to the service provider, and skips signing out if so (because the sign-out would have already been enforced before that point in our internal SAML flow). This is also currently behind a feature flag that will eventually be removed once we confirm everything works as expected (covered in another JIRA).
This issue was identified as a part of the rollout of the feature flag
saml_internal_post, first introduced in #6922 . We have a related comms plan where we also include the dates for the feature rollout: https://docs.google.com/document/d/159Mjrvmll-4uIKhzSoFEw_3pp20TWSI1f0ZH1Kvtdfc/edit#heading=h.hptnw14jf34z