LG-12262 Stop reading from sp_session[:ial2]#10129
Merged
Conversation
Comment on lines
58
to
59
Contributor
There was a problem hiding this comment.
probs a thought for a separate PR:
this is making me think resolved_authn_context_result might not be the clearest name? because otherwise we could essentially make this is a delegate_to kind of thing, renaming the property for clarity makes it seem like we have two names for the same thing
Like this is what the RP requested, so maybe it's like authentication_request?
Contributor
Author
There was a problem hiding this comment.
Yeah, I didn't love "AuthnContextResolver" and it has been turtles all the way down. Open to other suggestions.
Contributor
There was a problem hiding this comment.
requested_authenticationwould let us keep the same property names sorequested_authentication.identity_proofing?makes it clear it was what was requested?auth_request.identity_proofing_required?might be another?
zachmargolis
approved these changes
Feb 21, 2024
jmhooper
force-pushed
the
jmhooper-remove-sp-session-ial2
branch
from
c3ae51f to
c306c05
Compare
jmhooper
force-pushed
the
jmhooper-remove-sp-session-ial2
branch
from
c306c05 to
2e53e56
Compare
We are replacing the `sp_sesison[:ial2]` value with checks against `resolved_authn_context_result.identity_proofing?`. This commit removes the places where we are reading `sp_sesison[:ial2]`. Once this is merged and fully deployed we can stop writing `sp_session[:ial2]`. [skip changelog]
jmhooper
force-pushed
the
jmhooper-remove-sp-session-ial2
branch
from
2e53e56 to
e91011a
Compare
jmhooper
added a commit
that referenced
this pull request
Feb 26, 2024
A previous change (ref: #10129) stopped reading from `sp_session[:ial2]` and started using the result from the `AuthnContextResolver` instead. This change follows up by removing writes to the `ial2` property in the SP session. This should not be merged until the change to stop reading is fully merged and deployed. [skip changelog]
jmhooper
added a commit
that referenced
this pull request
Feb 26, 2024
A previous change (ref: #10129) stopped reading from `sp_session[:ial2]` and started using the result from the `AuthnContextResolver` instead. This change follows up by removing writes to the `ial2` property in the SP session. This should not be merged until the change to stop reading is fully merged and deployed. [skip changelog]
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.
We are replacing the
sp_sesison[:ial2]value with checks againstresolved_authn_context_result.identity_proofing?. This commit removes the places where we are readingsp_sesison[:ial2]. Once this is merged and fully deployed we can stop writingsp_session[:ial2].