Add flexible biometric proofing service selection (OIDC) by lmgeorge · Pull Request #10948 · 18F/identity-idp

lmgeorge · 2024-07-16T20:33:15Z

Why?

This is the initial work to replace our public-facing Vectors of Trust (VOT) API interface(s).

See: https://gitlab.login.gov/lg-people/lg-people-appdev/Melba/backlog-fy24/-/issues/39

How?

Maintains the current VoT implementation and API for current SPs
Adds support for selecting proofing with biometric comparison using new Authentication Context Class Reference values (ACR values) in our OIDC API
Unlike IALMax, the new ACR values are not a step-up flow. Instead, SPs will be able to fallback to a lower level of identity proofing for their current users that have successfully proofed in the past.

[skip changelog]

🛠 Summary of changes

The primary changes live in AuthnContextResolver.

It mimics the service selection logic of #selected_vtr_parser_result_from_vtr_list with the important caveat of only handling the ial2?bio=preferred ACR value. The AAL and other IAL ACR values are handled the same as before.

Note: ACR values are no longer sorted during resolution.

No new logic was added to replace/refactor how IAL ACR values are used except in OpenidConnectUserInfoPresenter, where the actual identity proofing ACR value is used instead of always mapping to IAL2_AUTHN_CONTEXT_CLASSREF.

app/presenters/openid_connect_user_info_presenter.rb

app/services/authn_context_resolver.rb

app/services/vot/authn_context_class_ref_component_values.rb

Sgtpluck

i'm still working through the changes, but here are some nitpicks to start

lib/saml_idp_constants.rb

app/services/vot/component_expander.rb

app/services/vot/component_value.rb

Sgtpluck · 2024-07-17T13:22:52Z

app/services/vot/parser.rb

same Data.define comment as above

I don't think the same case can be made here given how complex it is already.

This probably should have an initializer as well. Thoughts?

if it's being turned into a class, then yes i think we should have a real initializer rather than using Data.define. my personal preference would be to leave it alone for this PR, but the complexity comment is valid, so up to you whether to pull out the initializer or revert it to a class-like object!

ok, sorry thinking through this a little more:

as mentioned, the Data class was explicitly defined for use on class-like objects. it was not designed to be used in a class inheritance way as is being used here. if we wanted to fully convert the class, that would require removing this and updating all the references from using result.with.

i do not think we should do that as part of this PR (which is complicated enough already), so i'd like to more strongly encourage reverting this to Result = Data.define for the purposes of this PR, and adding a ticket to evaluate this more fully as part of next steps. i don't think converting this into a full class has any particularly strong benefits for the purpose of this change.

app/services/authn_context_resolver.rb

vrajmohan

Made a first pass at a code review.

app/services/vot/authn_context_class_ref_component_values.rb

app/services/vot/component_value.rb

spec/services/authn_context_resolver_spec.rb

app/services/authn_context_resolver.rb

app/services/vot/authn_context_class_ref_component_values.rb

lmgeorge · 2024-07-17T23:06:31Z

app/services/authn_context_resolver.rb

If :biometric_comparison is marked as false, then :two_pieces_of_fair_evidence should also be false and vice versa.

(This is partially why I started with string substitution, so I could avoid having that type of business logic in the resolver.)

ah great point! we can add that while we pair (or feel free to add it while applying other changes)

that makes sense, although having had a chat with hooper, it sounds like he intended the resolver to be the place to put that kind of logic, so that we can flexibly apply things depending on user/service provider (maybe other) context when necessary. let's definitely talk about the approach, i ended up changing more than i thought i was going to b/c i ran with hooper's suggestion and i want to make sure you're comfortable with it!

Okay, I'll make those changes and see if anything breaks (😅)

app/services/authn_context_resolver.rb

app/services/vot/component_value.rb

Sgtpluck · 2024-07-23T13:01:39Z

app/services/authn_context_resolver.rb

this method has gotten a lot more complicated with the inclusion of the meta-programming. i understand the desire to be explicit about what's in the result, but this has become harder for me to reason about in the process.

my personal preference would be closer to my original implementation (where we were just changing the values that needed to be changed, since we know, for instance, if the result is asking for biometric_comparison and it's required, we are not doing any data transformation on the result).

however, if you want to avoid making assumptions/keeping it explicit (which is fair), i would remove the self.method call and just call the methods directly. ruby automatically returns the last statement, so the correct result will be returned. the final line could be no_identity_proofing_acr_result(result) in order to capture any cases not found. this will make this method more direct and easier to reason about (imo).

other things i personally might consider is pulling all the asserted_ial_acr logic into a separate decorator (decorate_result_with_ial_acr), but that may be an optimization for another day.

I agree with Davida that the meta-programming may be a bit much for this implementation.

Re: meta-programming. That's fair, I was trying to solve for not having a million base case statements returning the IAL1 acr result. I'll try a different approach.

I don't know that it would be feasible to move the asserted_ial_acr logic separate from the updating the result attributes as they are tightly coupled.

i had a thought: this asserted_ial_acr is explicilty for dermining the return ial value.

for the purposes of actually answering the question "what is info[:ial]", we'll never be answering that question unless there is no identity profile. which means we should only be figuring out what the asserted_ial_acr if it's identity proofed. so i think we should be able to make this a lot less complicted knowing that use case?

I had forgotten while in the weeds of the code that the vtr param is used as a logic gate. A number of specs were failing because I wasn't looking at the full resolution graph of for biometric proofing, but rather only places where AuthnContextResolver#resolve and Vot::Parser::Result were being directly utilized.
I changed the behavior of the resolver with that narrow focus in mind, which meant absorbing some business logic around IAL level assertion.

I'm going to unpublish the PR and attempt to streamline the changes further.

Sgtpluck · 2024-07-23T13:25:25Z

app/services/vot/parser.rb

why did this type value get added?

I had a thought about the reporting on this, but it doesn't seem needed, so will remove.

jmhooper · 2024-07-23T14:27:30Z

app/services/vot/parser.rb

When I am using this class downstream to determine if I should perform biometric comparison what am I to make of biometric_comparison? and biometric_comparison_required?? Which one should I inspect to determine if I should perform biometric comparison or both? Is there something in the design of this that I am missing that should signal the answer to that question?

The idea is that if the SP wants to require a user to complete proofing with biometric comparison (instead of preferring it when the user has already completed biometric comparison), this would be used as opposed to testing the asserted_ial_acr value directly. I can revert this as it seems to confuse the issue.

jmhooper · 2024-07-23T14:28:41Z

app/services/vot/component_value.rb

Is the only real change here adding default values for these (and perhaps some documentation)? If so, does that makes sense as a separate pull request so it does not distract from the crux of the change here?

jmhooper · 2024-07-23T14:28:59Z

app/services/vot/component_value.rb

Where is this REQUIREMENTS constant used?

No where, I can remove this. I just needed to map the requirements in my head.

Sgtpluck · 2024-07-23T15:43:27Z

app/presenters/openid_connect_user_info_presenter.rb

please remove the commented out code!

good catch!

Sgtpluck · 2024-07-23T15:43:39Z

app/presenters/openid_connect_user_info_presenter.rb

please remove the commented out code

Sgtpluck · 2024-07-23T15:44:45Z

app/services/id_token_builder.rb

please remove commented out code

Sgtpluck

I'm really excited about these changes! The things that need to happen for this to be ready to go for me are:

removing commented out code
remove metaprogramming from the decorator method

and i would strongly recommend:

reverting the classes that were created out of the class-like objects to make this PR more compact and readable, along with a ticket to evaluate that change.

thanks! glad to see it's looking so good, excited to merge it in.

Sgtpluck · 2024-07-24T11:50:40Z

app/services/authn_context_resolver.rb

the original implementation of info[:ial] only returned either Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF or Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF. we should not be changing that implementation, it may break partners expecting a specific value back

**Why?** This is the initial work to replace our public-facing Vectors of Trust (VOT) API interface(s). See: https://gitlab.login.gov/lg-people/lg-people-appdev/Melba/backlog-fy24/-/issues/39 **How?** * Maintains the current VoT implementation and API for current SPs * Adds support for selecting proofing with biometric comparison using new Authentication Context Class Reference values (ACR values) in our OIDC API * Unlike IALMax, the new ACR values are not a step-up flow. Instead, SPs will be able to fallback to a lower level of identity proofing for their current users that have successfully proofed in the past. [skip changelog] contributors: davida marion <davida.marion@gsa.gov>

contributor: davida marion <davida.marion@gsa.gov> - Clean up some logic - Fix tests - Add AuthnContextResolver specs - Experiment with decorator rather than string replacement - Clean up method placement - Add condition that checks if biometrics is required

- Store acr_results in in assertion logic instance variable for performance - PR feedback: made component descriptions more specific and some rubocop fixes - wip: updated IAL ACR assertion logic, fixed broken tests, rubocop - wip: Fixed all broken tests except services/analytics.rb - PR fixes: fixed analytics tests - vtr handling updated

lmgeorge self-assigned this Jul 16, 2024

lmgeorge requested review from Sgtpluck, jmhooper and vrajmohan July 16, 2024 20:33

lmgeorge commented Jul 16, 2024

View reviewed changes

app/presenters/openid_connect_user_info_presenter.rb Outdated Show resolved Hide resolved

lmgeorge commented Jul 16, 2024

View reviewed changes

app/services/authn_context_resolver.rb Outdated Show resolved Hide resolved

lmgeorge commented Jul 16, 2024

View reviewed changes

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

Sgtpluck reviewed Jul 17, 2024

View reviewed changes

app/services/authn_context_resolver.rb Outdated Show resolved Hide resolved

Sgtpluck reviewed Jul 17, 2024

View reviewed changes

app/services/authn_context_resolver.rb Outdated Show resolved Hide resolved

Sgtpluck reviewed Jul 17, 2024

View reviewed changes

app/services/authn_context_resolver.rb Outdated Show resolved Hide resolved

vrajmohan reviewed Jul 17, 2024

View reviewed changes

Sgtpluck reviewed Jul 17, 2024

View reviewed changes

app/services/authn_context_resolver.rb Outdated Show resolved Hide resolved

lmgeorge force-pushed the lmgeorge/acr-values-resolution-oidc branch from 87c7eac to 5854808 Compare July 17, 2024 22:29

lmgeorge commented Jul 17, 2024

View reviewed changes

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

lmgeorge commented Jul 17, 2024

View reviewed changes

Sgtpluck reviewed Jul 18, 2024

View reviewed changes

app/services/authn_context_resolver.rb Outdated Show resolved Hide resolved

Sgtpluck reviewed Jul 19, 2024

View reviewed changes

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

Sgtpluck reviewed Jul 23, 2024

View reviewed changes

jmhooper reviewed Jul 23, 2024

View reviewed changes

lmgeorge marked this pull request as ready for review July 23, 2024 15:30

Sgtpluck reviewed Jul 23, 2024

View reviewed changes

app/services/id_token_builder.rb Outdated

Copy link

Contributor

Sgtpluck Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove commented out code

Sgtpluck reviewed Jul 23, 2024

View reviewed changes

lmgeorge marked this pull request as draft July 23, 2024 19:21

Sgtpluck reviewed Jul 24, 2024

View reviewed changes

zachmargolis mentioned this pull request Jul 26, 2024

Add biometric acr_values for OIDC protocol #10993

Merged

lmgeorge and others added 3 commits July 26, 2024 16:53

lmgeorge force-pushed the lmgeorge/acr-values-resolution-oidc branch from 66e6418 to dda9131 Compare July 26, 2024 21:12

lmgeorge marked this pull request as ready for review July 26, 2024 21:15

lmgeorge closed this Jul 26, 2024

Conversation

lmgeorge commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛠 Summary of changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sgtpluck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

vrajmohan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lmgeorge Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

lmgeorge commented Jul 16, 2024 •

edited

Loading

lmgeorge Jul 17, 2024 •

edited

Loading