-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: LG13235 consume eipp data from sinatra #10618
feat: LG13235 consume eipp data from sinatra #10618
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -38,6 +38,12 @@ module SupportedComponentValues | |||
implied_component_values: [P1], | |||
requirements: [:biometric_comparison], | |||
).freeze | |||
Pe = ComponentValue.new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does Pe
mean here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Proofing Enhanced?" 馃槅 I really put it out there for feedback. I initially had it as "P2" since it was a step up from the P1
component. I'm really happy to change it to whatever is more intuitive though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh ok, I wonder what the convention was here? It isn't obvious to me what any of the other names in the file mean either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that P2
is the way to go. This isn't necessarily as step up from the base proofing experience since it is scoped to in-person. For the same reason proofing with a biometric was named Pb
instead of P2
.
The naming convention for vectors of trust is described in [RFC 8485]. They are an upper-case letter followed by a lower-case letter or a number.
Pe = ComponentValue.new( | ||
name: 'Pe', | ||
description: 'Enhanced In Person Proofing is required', | ||
implied_component_values: [P1], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also what does P1 signify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was understanding the "P" to signify proofing related components and "1" was because it was the initial component in the proofing category...I could be wrong though 馃槄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm ok, i guess i wonder what implied_component_values
is supposed to be? This file is totally new to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description in this PR is super helpful!
The P component represents identity proofing. It contains the following values:
1: Identity proofing is performed
@KeithNava I started to set up the Sinatra app side. I am testing with your branch. Can you add enhanced_ipp_required to the Service Provider session on this branch? I am able to login with your branch (and not with main). Update: I can see the vtr on your branch has Pe included when I sign in from my branch ( |
Nice work Keith! 馃憦 馃コ
|
@@ -5,7 +5,7 @@ class ServiceProviderRequest | |||
# since these objects are serialized to/from Redis and may be present | |||
# upon deployment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KeithNava I am not sure what precautions are needed. I would ask to ensure you handled everything for a smooth deployment since you made some modifications.
a075b2b
to
a9b5056
Compare
@@ -138,6 +138,10 @@ def biometric_comparison_needed? | |||
!current_user.identity_verified_with_biometric_comparison? | |||
end | |||
|
|||
def enhanced_ipp_required? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this method doing on the authorization controller? I'm not sure I see where it is used.
@@ -38,6 +38,10 @@ def service_provider | |||
request.service_provider | |||
end | |||
|
|||
def enhanced_ipp_required? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have been trying to get away from including these methods on the protocol classes or adding the to the session. This was the way we would forward along SP request requirements before we built the AuthnContextResolver
and resolved them based on VTRs and ARC values downstream.
@@ -20,6 +20,7 @@ def initialize( | |||
# rubocop:disable Lint/UnusedMethodArgument | |||
ial: nil, | |||
aal: nil, | |||
enhanced_ipp_required: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 things here:
- This is going to be problematic in the 50/50 state because this could get serialized to the session by a new instance and read from an old instance. That would result in an
ArgumentError
because the old instance will not know about this arg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking back over this and I forgot to write out the second thing. How embarrassing. I believe I was going to describe here what I described in this comment: #10618 (comment)
allow(controller).to receive(:pii_requested_but_locked?).and_return(false) | ||
end | ||
|
||
it 'redirects to the redirect_uri immediately when pii is unlocked if client-side redirect is disabled' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an odd test-case here since PII being locked isn't necessarily related to enhanced in-person. I am not actually sure that we need changes to the authorization controller since its behavior should not change. It should continue forward the VTR along and we will operate on it downstream when we decide if we need enhanced in-person proofing.
It looks like this change is adding the
We should be able to introduce the |
This one looks like a good model implementation for what you need to do to add a new vector. Thank you @KeithNava! |
looks like there are just lint fixes that are needed to be able to merge this |
LGTM! 馃コ I tested your last commit with branch @KeithNava Can you have this merged in before the next deployment? I cannot push my Sinatra PR in until this makes it into prod. Thanks |
e650498
to
5107a9b
Compare
Thanks so much everyone! I learned a lot during this review, really appreciated all the feedback! 馃挴 |
Adding the ability to pull Enhanced IPP from the VTR parameter
馃帿 Ticket
Link to the relevant ticket:
LG-13235
馃洜 Summary of changes
Allow the application to recognize the Enhanced In Person Proofing flow coming from the Vector of Trust request (VTR) parameter by introducing a new value of
Pe
which represents the Enhanced Proofing component.馃摐 Testing Plan
Most of the confirmation is done through the spec tests but the follow on ticket to actually enable the passing of the values from the Sinatra application would be the true end to end test -> https://cm-jira.usa.gov/browse/LG-12858