LG-11873: doc auth w/ selfie in non-prod hosted envs by amirbey · Pull Request #9729 · 18F/identity-idp

amirbey · 2023-12-07T17:58:31Z

🎫 Ticket

LG-11873

🛠 Summary of changes

determine if doc auth with selfie is required based on the SP Session
only allow selfie to be sent to doc auth vendor if selfie FF is enabled
do not allow selfie to be sent to doc auth vendor if host env is production
indicate to the FE to display selfie field if hosted env is not production, selfie FF is enabled and requested by SP

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

app/forms/idv/api_image_upload_form.rb

dawei-nava · 2023-12-20T20:37:34Z

app/controllers/idv/image_uploads_controller.rb

This does not consider config following?

return false if Identity::Hostdata.env == 'prod' return false unless IdentityConfig.store.doc_auth_selfie_capture_enabled

Oh, i see, it's in the form. But should we make decision in one place?

fair point @dawei-nava 🤔 ...

consolidated ✅ 😄

If we put all the checks in decorated_sp_session.selfie_required?, then we don't have to duplicate this method here.

dawei-nava · 2023-12-20T21:12:02Z

app/views/idv/shared/_document_capture.html.erb

I think the controller already checks the hostdata env stuff in the concern? Or we wanna double check.

yes, it does ... but added check on the way out to FE be extra careful

aduth · 2023-12-21T14:38:49Z

app/controllers/concerns/idv/document_capture_concern.rb

I feel like the double-checking of both hostdata and config implies a distrust in our configuration, where we should be confident in using it? Assuming the default is false in production.

Suggested change

return false if Identity::Hostdata.env == 'prod'

return false unless IdentityConfig.store.doc_auth_selfie_capture_enabled

decorated_sp_session.selfie_required?

IdentityConfig.store.doc_auth_selfie_capture_enabled && decorated_sp_session.selfie_required?

@aduth - I understand that the FF is there and reliable. However, given the sensitivity of selfie not yet ready for our higher envs, we want it to be impossible for this to go live in production as this is an explicit requirement. No matter what config changes are made, this hardcoding guards any mistakes from a config standpoint.

decorated_sp_session.selfie_required? will also return false if IdentityConfig.store.doc_auth_selfie_capture_enabled is not set to true. I understand about wanting to make triple-sure this doesn't go live accidentally, and at the same time code encapsulation is also useful. Maybe we could put any extra checks in selfie_required and just use that everywhere instead of having an extra method liveness_checking_enabled?. Happy to rename the method if that would help.

I agree with Amir. Since this is such a sensitive feature, I think it's good to have an extra safeguard in place.

soniaconnolly

Great spec coverage! Overall looks good, some comments on repeated code. Commenting now, will approve later after running through the test plan.

soniaconnolly · 2023-12-21T16:19:01Z

app/controllers/concerns/idv/document_capture_concern.rb

decorated_sp_session.selfie_required? will also return false if IdentityConfig.store.doc_auth_selfie_capture_enabled is not set to true. I understand about wanting to make triple-sure this doesn't go live accidentally, and at the same time code encapsulation is also useful. Maybe we could put any extra checks in selfie_required and just use that everywhere instead of having an extra method liveness_checking_enabled?. Happy to rename the method if that would help.

soniaconnolly · 2023-12-21T16:32:09Z

app/controllers/idv/image_uploads_controller.rb

If we put all the checks in decorated_sp_session.selfie_required?, then we don't have to duplicate this method here.

soniaconnolly · 2023-12-21T16:34:58Z

app/forms/idv/api_image_upload_form.rb

Does this method add value?

@soniaconnolly - 👍🏿 ... it allows us to access liveness_checking_required for selfie validator

I think it also works if you use the attr_reader method of :liveness_checking_required in the validator.

i tried again to remove it and it worked 👍🏿

soniaconnolly · 2023-12-21T16:37:16Z

app/services/doc_auth/lexis_nexis/requests/true_id_request.rb

Same question as above: does this method add value? And if they both add value, let's call them the same thing.

@soniaconnolly - i see what you mean ... upon your suggestion, i attempted changing it but soon realized that it will be too similar naming (liveness_checking_required vs liveness_checking_required?) to the existing attribute name and open the opportunity to a bad typo mistake where we evaluate whether or not to put a selfie in the payload

soniaconnolly · 2023-12-21T16:45:31Z

spec/services/doc_auth/lexis_nexis/requests/true_id_request_spec.rb

Do we need to repeat this method definition in the spec?

@soniaconnolly - reusing the approach that was present but introducing the guards which we also want to test ... it allows us to reduce a lot of lines of coding in this test .... do you have any simple suggestions on how to improve this?

Paired on this and we ended up renaming the method to include_liveness_expected to make it clear that it's not overriding the method definition in code.

dawei-nava · 2023-12-21T18:36:05Z

app/forms/idv/api_image_upload_form.rb

Should we check the costs function actually adds the cost?

you're right @dawei-nava ... we shouldn't forget about this ... i think that's out of scope for this ticket ... created LG-11965 so we won't lose track of this ... 👍🏿

soniaconnolly

Ran through the test plan, LGTM!

Additionally tested logging in at Identity Verified level with a Biometric Comparison verified account, and that worked too.

Couple additional notes from testing, not in scope for this PR:

In development, I'm still seeing the "don't have a license" questionnaire at the end of Document Capture. As far as I know I don't have any relevant config flags set in my local application.yml. Maybe application.yml.default has it on in development? (Yep, that was it, putting in a PR to turn that off.)
Should we add something to VerifyInfo that indicates a selfie was captured?

eileen-nava · 2023-12-21T18:28:07Z

app/controllers/idv/image_uploads_controller.rb

This method looks very similar to liveness_checking_enabled? in DocumentCaptureConcern. Why not include the DocumentCaptureConcern at the top of the file and then call liveness_checking_enabled? here?

Putting all the checks in decorated_sp_session.selfie_required? would work, too. That also wouldn't require including DocumentCaptureConcern in this file. 🤔

eileen-nava · 2023-12-21T18:41:54Z

app/forms/idv/api_image_upload_form.rb

I think it also works if you use the attr_reader method of :liveness_checking_required in the validator.

eileen-nava · 2023-12-21T18:48:01Z

spec/controllers/idv/document_capture_controller_spec.rb

eileen-nava · 2023-12-21T18:49:16Z

spec/controllers/idv/document_capture_controller_spec.rb

eileen-nava · 2023-12-21T18:53:15Z

spec/controllers/idv/hybrid_mobile/document_capture_controller_spec.rb

I think this should say 'does not request FE to display selfie'

eileen-nava · 2023-12-21T18:57:54Z

spec/controllers/idv/image_uploads_controller_spec.rb

I know this predates this PR, but since you are working in this spec, do you mind combining the before statements on lines 32-40 into one before statement?

eileen-nava · 2023-12-21T19:05:41Z

spec/features/idv/doc_auth/document_capture_spec.rb

eileen-nava · 2023-12-21T19:11:21Z

spec/forms/idv/api_image_upload_form_spec.rb

Nit: Should the context block also say required? Do we want to be consistent in which term we use?

eileen-nava · 2023-12-21T19:13:52Z

spec/services/doc_auth/lexis_nexis/requests/true_id_request_spec.rb

Thanks for including this spec! 🙏🏻

eileen-nava · 2023-12-21T19:23:57Z

spec/services/doc_auth/lexis_nexis/requests/true_id_request_spec.rb

🤔 I wonder if it's worth updating this shared example in order to pass liveness_checking_required to it as an argument? Thinking out loud here.

Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov>

amirbey added the status - do not merge label Dec 7, 2023

aduth reviewed Dec 7, 2023

View reviewed changes

app/forms/idv/api_image_upload_form.rb Outdated Show resolved Hide resolved

amirbey changed the title ~~selfie in staging or lower envs only~~ SPIKE: selfie in staging or lower envs only Dec 7, 2023

amirbey force-pushed the amirbey/selfie-in-staging branch from a1a3d0c to 42070a7 Compare December 12, 2023 20:29

amirbey changed the title ~~SPIKE: selfie in staging or lower envs only~~ LG-11873: selfie in staging or lower envs only [DO NOT MERGE] Dec 13, 2023

amirbey self-assigned this Dec 15, 2023

amirbey force-pushed the amirbey/selfie-in-staging branch 2 times, most recently from 679a4f6 to 90f4ae9 Compare December 20, 2023 17:14

amirbey changed the title ~~LG-11873: selfie in staging or lower envs only [DO NOT MERGE]~~ LG-11873: doc auth w/ selfie in non-prod hosting envs Dec 20, 2023

amirbey requested review from aduth, charleyf, dawei-nava, eileen-nava, jmhooper and night-jellyfish and removed request for aduth December 20, 2023 20:18

amirbey marked this pull request as ready for review December 20, 2023 20:23

amirbey changed the title ~~LG-11873: doc auth w/ selfie in non-prod hosting envs~~ LG-11873: doc auth w/ selfie in non-prod hosted envs Dec 20, 2023

dawei-nava reviewed Dec 20, 2023

View reviewed changes

amirbey requested a review from dawei-nava December 20, 2023 21:33

aduth reviewed Dec 21, 2023

View reviewed changes

soniaconnolly reviewed Dec 21, 2023

View reviewed changes

dawei-nava reviewed Dec 21, 2023

View reviewed changes

amirbey requested a review from soniaconnolly December 21, 2023 19:34

amirbey removed the status - do not merge label Dec 21, 2023

soniaconnolly approved these changes Dec 21, 2023

View reviewed changes

amirbey force-pushed the amirbey/selfie-in-staging branch from 9d50c1e to a8c1a33 Compare December 21, 2023 20:02

eileen-nava reviewed Dec 21, 2023

View reviewed changes

amirbey and others added 25 commits December 21, 2023 17:09

make data-doc-auth-selfie-capture a boolean

c4f6d51

remove selfie param from hybrid doc auth controller spec

31a6eb2

test when selfie ff enabled

9714b0f

fix specs

db01b02

happy linting

0a689b2

happy linting

a77c6a4

use selfie_req'd from decorated sp session

7d1197e

remove unused code

d9ba84b

use decorated_sp_session.selfie_required in image uploads controller

a114e02

add selfie required to NullServiceProviderSession

38d2ff1

remove leftover selfie=true params from spec

be2b965

stub decorated sp session

4462d3b

fix document capture controller with use of decorated sp session

e2335d7

consolidate liveness check in images upload controller/form

5874855

update image uploads specs

5ecd255

check is prod in selfie req'd

3269f9e

test to ensure selfie_required is false when in prod

b99a74c

udpate specs since selfie req'd is handled in sp_session_decorator

7905db5

update image upload controller spec to use sp_decorator.selfie_required?

e31b06d

rename include liveness in true id spec

2e3dded

Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov>

linting

c14fdf4

remove liveness_check_required? method in api_image_upload_form

64c4299

combine before blocks

86660b6

require biometric comparison for selfie analytics testing

e9cc391

fix string that should be boolean

63c35f8

amirbey force-pushed the amirbey/selfie-in-staging branch from 002a432 to 63c35f8 Compare December 21, 2023 22:09

amirbey merged commit 099ef6b into main Dec 21, 2023

amirbey deleted the amirbey/selfie-in-staging branch December 21, 2023 22:36

charleyf mentioned this pull request Dec 22, 2023

LG-11906 Update Review Step UI with Selfie to Match Document Step with Selfie #9827

Merged

soniaconnolly mentioned this pull request Jan 2, 2024

Deploy RC 343 to Prod #9841

Merged

-      return false if Identity::Hostdata.env == 'prod'
-      return false unless IdentityConfig.store.doc_auth_selfie_capture_enabled
-      decorated_sp_session.selfie_required?
+      IdentityConfig.store.doc_auth_selfie_capture_enabled && decorated_sp_session.selfie_required?

Conversation

amirbey commented Dec 7, 2023 • edited by soniaconnolly Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirbey Dec 20, 2023 • 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!

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!

soniaconnolly left a comment

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!

amirbey Dec 21, 2023 • 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!

amirbey Dec 21, 2023 • 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!

soniaconnolly left a comment • 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!

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

amirbey commented Dec 7, 2023 •

edited by soniaconnolly

Loading

amirbey Dec 20, 2023 •

edited

Loading

amirbey Dec 21, 2023 •

edited

Loading

amirbey Dec 21, 2023 •

edited

Loading

soniaconnolly left a comment •

edited

Loading