feat: LG13235 consume eipp data from sinatra by KeithNava · Pull Request #10618 · 18F/identity-idp

KeithNava · 2024-05-14T10:38:46Z

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

JackRyan1989

LGTM

app/services/vot/parser.rb

svalexander · 2024-05-14T16:03:07Z

app/services/vot/supported_component_values.rb

what does Pe mean here?

"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.

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.

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.

svalexander · 2024-05-14T16:04:48Z

app/services/vot/supported_component_values.rb

also what does P1 signify?

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 😅

hmm ok, i guess i wonder what implied_component_values is supposed to be? This file is totally new to me

The description in this PR is super helpful!

The P component represents identity proofing. It contains the following values: 1: Identity proofing is performed

app/services/vot/supported_component_values.rb

gina-yamada · 2024-05-14T16:37:32Z

@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 (yamada/LG-12858-EIPP-Option) after selected Enhance IPP.

gina-yamada · 2024-05-16T15:07:01Z

Nice work Keith! 👏 🥳

I got our branches to work together by adding to app/forms/openid_connect_authorize_form.rb this function:

def enhanced_ipp_required?
   parsed_vector_of_trust&.enhanced_ipp?
end

Not needed to work but I think we should add it to saml. app/models/federated_protocols/saml.rb

def enhanced_ipp_required?
   false
end

What do you think about moving enhanced_ipp_required on sp_session to be next to biometric_comparison_required? I think the acr values and vtr values are nice on the end. Only my preference so if you don't like it- ignore it.

gina-yamada · 2024-05-16T15:14:23Z

app/models/service_provider_request.rb

@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.

jmhooper · 2024-05-20T18:16:50Z

app/controllers/openid_connect/authorization_controller.rb

What is this method doing on the authorization controller? I'm not sure I see where it is used.

jmhooper · 2024-05-20T18:17:48Z

app/models/federated_protocols/oidc.rb

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.

jmhooper · 2024-05-20T18:18:45Z

app/models/service_provider_request.rb

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.

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)

jmhooper · 2024-05-20T18:26:28Z

spec/controllers/openid_connect/authorization_controller_spec.rb

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.

jmhooper · 2024-05-20T18:34:43Z

It looks like this change is adding the enhanced_ipp_required to sp_session. We have actually been moving away from this pattern. Recently I actually merged a change to stop writing biometric_comparison_required in the session (ref). This was done for a few reasons:

It is difficult to keep in sync between ACR values and VTR with the code for determining what is required spread amongst other concerns
It doesn't work well with multiple vector of trust requests where the vector that is in-use can change over the course of a request
It makes adding and removing new requirements challenging because they SP request and SP session need to be modified and both introduce 50/50 state concerns

We should be able to introduce the Pe vector component and update the parser to return the requirement for enhance in-person. The VTR will be passed along into the SP session and the AuthnContextResolver will pick it up there. From there we can call resolved_authn_context_result.enhanced_ipp? without needing to make any changes to the SAML/OIDC interface code or the code that writes into the SP session.

jmhooper · 2024-05-21T14:33:19Z

This one looks like a good model implementation for what you need to do to add a new vector. Thank you @KeithNava!

svalexander · 2024-05-21T15:32:12Z

looks like there are just lint fixes that are needed to be able to merge this

gina-yamada · 2024-05-21T17:55:04Z

LGTM! 🥳 I tested your last commit with branch yamada/LG-12858-EIPP-Option and I can see vtr values C1.P1.Pb on sp_session.

@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

This reverts commit 36c248949d604d654e25f637f1867eccade2668f.

KeithNava · 2024-05-21T20:42:07Z

Thanks so much everyone! I learned a lot during this review, really appreciated all the feedback! 💯

KeithNava requested review from a team and svalexander May 14, 2024 10:38

JackRyan1989 approved these changes May 14, 2024

View reviewed changes

gina-yamada reviewed May 14, 2024

View reviewed changes

app/services/vot/parser.rb Outdated Show resolved Hide resolved

svalexander reviewed May 14, 2024

View reviewed changes

aduth reviewed May 14, 2024

View reviewed changes

app/services/vot/supported_component_values.rb Outdated Show resolved Hide resolved

gina-yamada mentioned this pull request May 15, 2024

LG-12858 - Add new vtr option (enhanced ipp) 18F/identity-oidc-sinatra#161

Merged

gina-yamada reviewed May 16, 2024

View reviewed changes

KeithNava force-pushed the keithw/LG-13235-consume-eipp-data-from-sinatra branch 2 times, most recently from a075b2b to a9b5056 Compare May 20, 2024 13:19

jmhooper reviewed May 20, 2024

View reviewed changes

jmhooper approved these changes May 21, 2024

View reviewed changes

KeithNava added 8 commits May 21, 2024 14:58

feat: add eipp to vot parser

40bc058

feat: update authn context resolver tests for ipp

bd3d18e

feat: add enhanced_ipp_required required to service provider

90403c3

feat: update oidc connect parsing logic for enhance_ipp

8c0f520

feat: default saml parsing for eipp to false

ac8717e

feat: update ordering of params in sp session

91093cb

feat: update spec for service provider session

bece22c

fix: merge conflicts

fddc65f

KeithNava added 3 commits May 21, 2024 14:58

feat: add vot parser method for sp session

3d24a05

Revert "feat: add enhanced_ipp_required required to service provider"

4d9bbdf

This reverts commit 36c248949d604d654e25f637f1867eccade2668f.

feat: lintfix

5107a9b

KeithNava force-pushed the keithw/LG-13235-consume-eipp-data-from-sinatra branch from e650498 to 5107a9b Compare May 21, 2024 19:56

KeithNava merged commit 56cb759 into main May 21, 2024

KeithNava deleted the keithw/LG-13235-consume-eipp-data-from-sinatra branch May 21, 2024 20:42

matthinz mentioned this pull request May 23, 2024

Deploy RC 382 to Production #10692

Merged

Conversation

KeithNava commented May 14, 2024

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

Uh oh!

JackRyan1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gina-yamada commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gina-yamada commented May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmhooper commented May 20, 2024

Uh oh!

jmhooper commented May 21, 2024

Uh oh!

svalexander commented May 21, 2024

Uh oh!

gina-yamada commented May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KeithNava commented May 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gina-yamada commented May 14, 2024 •

edited

Loading

gina-yamada commented May 16, 2024 •

edited

Loading

gina-yamada commented May 21, 2024 •

edited

Loading