Skip to content

LG-14813 default users requiring facial match to LN#11531

Merged
theabrad merged 11 commits intomainfrom
abrad-lg-14813-facial-match-default
Dec 5, 2024
Merged

LG-14813 default users requiring facial match to LN#11531
theabrad merged 11 commits intomainfrom
abrad-lg-14813-facial-match-default

Conversation

@theabrad
Copy link
Contributor

🎫 Ticket

Link to the relevant ticket:
LG-14813

🛠 Summary of changes

If a user is going through IdV with a Service Provider requiring facial match, we want to default them to Lexis Nexis and not have them go through Socure.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Set doc_auth_vendor_default: 'socure' in application.yml
  • Go through IdV with Facial Match required.
  • Confirm that you are sent through the LN flow instead of socure
  • Try going through IdV without facial match required.
  • Confirm you are sent through the Socure flow.

changelog: Upcoming Features, IdV Socure, default users requiring facial match to LN
Copy link
Contributor

@AShukla-GSA AShukla-GSA left a comment

Choose a reason for hiding this comment

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

LGTM!

# @returns[String] String identifying the vendor to use for doc auth.
def doc_auth_vendor
bucket = ab_test_bucket(:DOC_AUTH_VENDOR)
if resolved_authn_context_result.facial_match? && default_vendor_is_not_mock?
Copy link
Contributor

@amirbey amirbey Nov 21, 2024

Choose a reason for hiding this comment

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

could we have mock, lexisnexis and socure enabled at the same time. what if a facial match is required and the default vendor is :mock? would a user be routed to a vendor that does not support facial match? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, say default is socure and facial match is requested. Maybe:

bucket = ab_test_bucket(:DOC_AUTH_VENDOR
if resolved_authn_context_result.facial_match? && bucket != :mock
  bucket = :lexis_nexus
end

Copy link
Contributor

Choose a reason for hiding this comment

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

furthering Doug's point ... this appears to assume that mock or lexis nexis is enabled as a doc auth vendor. It seems that we'd need to know whether a vendor is enabled before routing to that vendor. what happens if only socure is enabled, would we have a doc auth vendor when a facial match is req'd 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, say default is socure and facial match is requested. Maybe:

bucket = ab_test_bucket(:DOC_AUTH_VENDOR
if resolved_authn_context_result.facial_match? && bucket != :mock
  bucket = :lexis_nexus
end

We want to bypass A/B test bucketing if facial match is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

furthering Doug's point ... this appears to assume that mock or lexis nexis is enabled as a doc auth vendor. It seems that we'd need to know whether a vendor is enabled before routing to that vendor. what happens if only socure is enabled, would we have a doc auth vendor when a facial match is req'd 🤔

If LN is not enabled should we just redirect them to an error page?

Copy link
Contributor

@amirbey amirbey Nov 26, 2024

Choose a reason for hiding this comment

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

furthering Doug's point ... this appears to assume that mock or lexis nexis is enabled as a doc auth vendor. It seems that we'd need to know whether a vendor is enabled before routing to that vendor. what happens if only socure is enabled, would we have a doc auth vendor when a facial match is req'd 🤔

If LN is not enabled should we just redirect them to an error page?

i tested the current behavior when a vendor is not defined, then the doc_auth_vendor defaults to (default_doc_auth_vendor which is nil). In the unlikely event there is a configuration error, if a user requires a facial match and only socure is configured, should we route the user to socure or standard document capture with a nil vendor (resulting in the screenshot below) @tahineemay?

Screenshot 2024-11-26 at 7 13 54 AM

Choose a reason for hiding this comment

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

In this unlikely case, that a user requires facial match and only Socure is configured, we have to throw and error to help surface the misconfiguration. We cannot have the user go through a non-IAL2 IDV, if that is their requirement.

Copy link
Contributor

@amirbey amirbey Nov 26, 2024

Choose a reason for hiding this comment

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

as per @tahineemay response we should return the doc_auth_vendor should be nil which is inline with the current behavior when there is no vendor 👍🏿

Copy link
Contributor

Choose a reason for hiding this comment

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

btw, when doc_auth_vendor is nil, this ticketed bug is throwing a 500. for your testing it maybe helpful to comment out the redirect_to_correct_vendor when testing doc_auth_vendor is nil 👍🏿

Comment on lines -67 to +68
let(:idv_vendor) { Idp::Constants::Vendors::LEXIS_NEXIS }
context 'when doc_auth_vendor is Lexis Nexis' do
let(:idv_vendor) { Idp::Constants::Vendors::LEXIS_NEXIS }
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks 🙏🏿

Copy link
Contributor

@amirbey amirbey left a comment

Choose a reason for hiding this comment

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

what if mock is default vendor and facial match is req'd? do we have testing for this?

@amirbey amirbey self-requested a review November 22, 2024 19:47
{}
end
let(:idv_vendor) { Idp::Constants::Vendors::MOCK }
let(:vot) { 'P1' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Vectors of Trust is deprecated and slated to be removed. 😢

Comment on lines +17 to +22

def lexis_nexis_not_enabled?
(IdentityConfig.store.doc_auth_vendor_default == Idp::Constants::Vendors::SOCURE ||
IdentityConfig.store.doc_auth_vendor_default.nil?) &&
IdentityConfig.store.doc_auth_vendor_lexis_nexis_percent == 0
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def lexis_nexis_not_enabled?
(IdentityConfig.store.doc_auth_vendor_default == Idp::Constants::Vendors::SOCURE ||
IdentityConfig.store.doc_auth_vendor_default.nil?) &&
IdentityConfig.store.doc_auth_vendor_lexis_nexis_percent == 0
end

theabrad and others added 3 commits December 3, 2024 13:15
Co-authored-by: Amir Reavis-Bey <amir.reavis-bey@gsa.gov>
Co-authored-by: Amir Reavis-Bey <amir.reavis-bey@gsa.gov>
Comment on lines +37 to +39
def default_vendor_is_not_mock?
IdentityConfig.store.doc_auth_vendor_default != Idp::Constants::Vendors::MOCK
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def default_vendor_is_not_mock?
IdentityConfig.store.doc_auth_vendor_default != Idp::Constants::Vendors::MOCK
end

doesn't seem we're still using this 🤔

Copy link
Contributor

@amirbey amirbey left a comment

Choose a reason for hiding this comment

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

LGTM ...thanks @theabrad

@theabrad theabrad merged commit 48da2e9 into main Dec 5, 2024
@theabrad theabrad deleted the abrad-lg-14813-facial-match-default branch December 5, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants