Conversation
There was a problem hiding this comment.
(nothing needed for this PR, mentioning)
ok so I can't believe I am figuring this out today, but I think it's a really weird pattern the way we assign the transient asserted_attributes to the user object like this, I would love to live in a world where we refactor how we pass the user attributes around, but that seems like not the right idea for this PR right this moment
There was a problem hiding this comment.
Right, I am planning to dig into why we have this strange pattern if I have some time at the end.
vrajmohan
force-pushed
the
support-biometric-acr-in-saml
branch
from
0bb24b8 to
d0e342a
Compare
Sgtpluck
left a comment
Sgtpluck
left a comment
There was a problem hiding this comment.
Some minor suggestions and questions, but it looks good! i'll mark it as approved because i think you can move forward with these minor edits
There was a problem hiding this comment.
not a huge deal, but part of the reason for memoization is to make an application more efficient. the resolve method is doing the bulk of the work. did you consider memoizing resolved_authn_context_result along with authn_context_resolver as it's called multiple times?
There was a problem hiding this comment.
I considered it. When vtr is not present, which is most of the time, it would be using acr_result which is memoized.
app/services/attribute_asserter.rb
Outdated
There was a problem hiding this comment.
same suggestion as noted above about memoizing this value
changelog: Upcoming Features, IdV with Biometric Comparison, Supporting biometric acr in SAML 1. Updated AttributeAsserter to send correct ial 3. Renamed AuthnContextResolver#resolve to AuthnContextResolver#result and memoized it 3. Moved asserted_ial_value from OpenidConnectUserInfoPresenter to AuthnContextResolver 4. Used common asserted_ial_acr method from both AttributeAsserter and OpenidConnectUserInfoPresenter
vrajmohan
force-pushed
the
support-biometric-acr-in-saml
branch
from
e8f1677 to
294c49f
Compare
4 participants
changelog: Upcoming Features, IdV with Biometric Comparison, Supporting biometric acr in SAML
🎫 Ticket
https://gitlab.login.gov/lg-people/lg-people-appdev/Melba/backlog-fy24/-/issues/44
📜 Testing Plan
The testing plan should be similar to what was done for OIDC - https://docs.google.com/document/d/1_TJBdEiErT460qoJh-jiw9E3mlPCffguMn28--4LJK8/edit