LG-12557: session result success status + portrait match result not requested by amirbey · Pull Request #10197 · 18F/identity-idp

amirbey · 2024-03-01T20:33:34Z

🎫 Ticket

LG-12557

🛠 Summary of changes

Document capture result to evaluate whether liveness check is required when evaluating success_status
TrueIDResponse only parse and store portrait match when a portrait match was requested

📜 Testing Plan

unit test coverage

[skip changelog]

…us basedon whether a selfie is required

…TrueIDReponse signature

eileen-nava

I didn't have time to do a full review but can look at this more tomorrow, too.

eileen-nava · 2024-03-05T21:23:18Z

app/services/doc_auth/lexis_nexis/responses/true_id_response.rb

        #   return :fail
        def selfie_status
-          return :not_processed if selfie_result.nil?
+          return :not_processed if selfie_result.nil? || !@liveness_checking_enabled


Good stuff. 👍🏻

eileen-nava · 2024-03-05T21:24:18Z

app/services/doc_auth/lexis_nexis/responses/true_id_response.rb

        #   return :fail
        def selfie_status
-          return :not_processed if selfie_result.nil?
+          return :not_processed if selfie_result.nil? || !@liveness_checking_enabled


Nit: Would you be okay with updating the comment on L106? Something like:
# When selfie result is missing or was not requested, return :not_processed

eileen-nava · 2024-03-05T21:28:05Z

spec/controllers/idv/image_uploads_controller_spec.rb

              user_uuid: an_instance_of(String),
              uuid_prefix: nil,
-              liveness_checking_required: true,
+              liveness_checking_required: liveness_checking_required,


Nice clean-up. 👏🏻

eileen-nava · 2024-03-05T22:14:22Z

app/services/document_capture_session_result.rb

+  def success_status(selfie_required:)
    # doc_auth_success : including document, attention_with_barcode and id type verification
-    !!doc_auth_success && selfie_status != :fail && !!pii
+    !!doc_auth_success && (selfie_required ? selfie_status == :success : true) && !!pii


What was the motivation for changing selfie_status != :fail to (selfie_required ? selfie_status == :success : true)?

I’d expect selfie_status to equal :not_processed whenever selfie_required equals false, which makes me expect that the previous implementation would still work.

with the presence of selfie_required argument we now have the ability to be more strict which is a more secure to way to say what is a pass as opposed to allowing anything that is !:fail to be a pass. it also safeguards against the potential for values of selfie_status to expand over time and requires us to be explicit about what passes.

it also safeguards against the potential for values of selfie_status to expand over time

Since we determine the values of selfie_status ourselves, I don’t think there’s as much of a need to safeguard against the value of selfie_status changing over time. I think that would be a bigger concern if we were determining success based solely on a value that is passed to us from the vendor, like selfie_result.

Both implementations work. I do think the previous implementation is more readable, but YMMV.

it's a small add but I do understand your readability sentiment 👍🏿

similar to this scenario where a selfie_status is returned when not requested, what if a selfie_status is not returned when requested

i think it is safer to be explicit as to what is a pass, as opposed to what is not a fail.

you'r right, we do determine the value of selfie_status (I say that with the assumption you are speaking about the TrueIDResponse.selfie_status) ... if in a future state a new key option is added to define selfie_status, a developer must remember to redefine what is a fail here selfie_status != :fail or we could run the risk of passing folks through that should not go through

something to consider 🤔 ... An easier and more readable option could be to simply use the :success attribute since it has all of these scenarios baked in and this bug nor the previous bug would be an issue 😐

I think may be we should create a response subclass for selfie required scenario(not now), where it's unambiguous and can decide success accordingly and from session result perspective it's simple and straight.

In retrospective and a little bit of topic, the session_result should perform it's designed purpose, store business result, more complex logic should be in service layer(seems what the api_image_upload_form does). The api_image_upload_form does input validation and is what a form should do. It also does some result validation(pii info using another form, yes it's handy to use form for validation in Rails), which sounds a little bit off.
Since we introduce selfie related function, it feels like we are bubbling up some of the logic here, not specific in this PR, but previous ones also.

(I’m not sure the status of this work because I was out yesterday and the jira ticket is in the review column, but I figured I should still respond.)

As discussed in past meetings, I will defer to the rest of the team about using the success attribute. I get the argument that you’d rather be explicit about what is a pass, as opposed to what is not a fail. I think my concern is similar to what Zach posted, that this seems like an unexpected place to pass a param.

zachmargolis · 2024-03-06T23:17:34Z

app/services/document_capture_session_result.rb

  end

-  def success_status
+  def success_status(selfie_required:)


this seems like a really unexpected place to pass a param, sepecially since this method is aliased as :success? as well, does that mean we need to update every callsite to pass the param? or add a default value?

is there any way we can pass the selfie_required bit as part of the data that is stored inside this struct so the callees don't have to pass this in?

dawei-nava · 2024-03-07T16:27:36Z

app/controllers/idv/capture_doc_status_controller.rb

              redo_document_capture_pending?
          :accepted
-        elsif !session_result.success?
+        elsif !session_result.success?(selfie_required: decorated_sp_session.selfie_required?)


This makes me think the session result should reflect all business logic?

dawei-nava · 2024-03-07T17:57:51Z

app/controllers/idv/document_capture_controller.rb


-      result = handle_stored_result
-      analytics.idv_doc_auth_document_capture_submitted(**result.to_h.merge(analytics_arguments))
+      form_response = handle_stored_result


👍 to make it clear.

amirbey · 2024-03-08T20:17:04Z

closed in favor of #10193

document capture session result check if liveness is required

7f61917

[skip changelog]

amirbey added the status - do not merge label Mar 1, 2024

rename result to form_response

e999531

amirbey self-assigned this Mar 1, 2024

amirbey added 9 commits March 1, 2024 16:10

do not store selfie result if liveness checking is disabled

49e3f99

update spec to use selfie_req'd param

3769854

update docuement capture session result tests to evaluate selfie_stat…

ba7ca96

…us basedon whether a selfie is required

add selfie_required param

1645661

happy linting

d38248e

test that selfie without a status failes when liveness check is required

e1693e8

rename liveness_enabled to liveness_checking_enabled consistent with …

10135fe

…TrueIDReponse signature

happy linting

1b0202b

undo DocAuth::Mock#selfie_status changes made to recreate the bug

8920b82

amirbey force-pushed the amirbey/LG-12557-selfie-status_required branch from 8704550 to 8920b82 Compare March 4, 2024 21:31

undo mock change

efd8015

amirbey removed the status - do not merge label Mar 5, 2024

amirbey requested review from a team and dawei-nava March 5, 2024 16:31

amirbey marked this pull request as ready for review March 5, 2024 16:32

amirbey changed the title ~~LG-12557: document capture session result check if liveness is required~~ LG-12557: portait match result received when not requested Mar 5, 2024

amirbey changed the title ~~LG-12557: portait match result received when not requested~~ LG-12557: failing portrait match result received when not requested Mar 5, 2024

amirbey changed the title ~~LG-12557: failing portrait match result received when not requested~~ LG-12557: session results success status + portrait match result not requested Mar 5, 2024

amirbey changed the title ~~LG-12557: session results success status + portrait match result not requested~~ LG-12557: session result success status + portrait match result not requested Mar 5, 2024

eileen-nava reviewed Mar 5, 2024

View reviewed changes

update selfie_status comment

cefc72a

zachmargolis reviewed Mar 6, 2024

View reviewed changes

dawei-nava reviewed Mar 7, 2024

View reviewed changes

amirbey closed this Mar 8, 2024

amirbey deleted the amirbey/LG-12557-selfie-status_required branch March 12, 2024 19:32

Conversation

amirbey commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

Uh oh!

eileen-nava 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!

dawei-nava Mar 7, 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!

eileen-nava Mar 8, 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!

Choose a reason for hiding this comment

Uh oh!

amirbey commented Mar 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

amirbey commented Mar 1, 2024 •

edited

Loading

dawei-nava Mar 7, 2024 •

edited

Loading

eileen-nava Mar 8, 2024 •

edited

Loading