Skip to content

LG-12557: session result success status + portrait match result not requested #10197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
amirbey wants to merge 13 commits into from
Closed

LG-12557: session result success status + portrait match result not requested #10197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7f61917
document capture session result check if liveness is required
amirbey Mar 1, 2024
e999531
rename result to form_response
amirbey Mar 1, 2024
49e3f99
do not store selfie result if liveness checking is disabled
amirbey Mar 1, 2024
3769854
update spec to use selfie_req'd param
amirbey Mar 4, 2024
ba7ca96
update docuement capture session result tests to evaluate selfie_stat…
amirbey Mar 4, 2024
1645661
add selfie_required param
amirbey Mar 4, 2024
d38248e
happy linting
amirbey Mar 4, 2024
e1693e8
test that selfie without a status failes when liveness check is required
amirbey Mar 4, 2024
10135fe
rename liveness_enabled to liveness_checking_enabled consistent with …
amirbey Mar 4, 2024
1b0202b
happy linting
amirbey Mar 4, 2024
8920b82
undo DocAuth::Mock#selfie_status changes made to recreate the bug
amirbey Mar 4, 2024
efd8015
undo mock change
amirbey Mar 5, 2024
cefc72a
update selfie_status comment
amirbey Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/idv/capture_doc_status_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def status
elsif session_result.blank? || pending_barcode_attention_confirmation? ||
redo_document_capture_pending?
:accepted
elsif !session_result.success?
elsif !session_result.success?(selfie_required: decorated_sp_session.selfie_required?)
Copy link
Contributor

@dawei-nava dawei-nava Mar 7, 2024

Choose a reason for hiding this comment

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

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

:unauthorized
else
:ok
Expand Down
11 changes: 7 additions & 4 deletions app/controllers/idv/document_capture_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,17 @@ def update
# Not used in standard flow, here for data consistency with hybrid flow.
document_capture_session.confirm_ocr

result = handle_stored_result
analytics.idv_doc_auth_document_capture_submitted(**result.to_h.merge(analytics_arguments))
form_response = handle_stored_result
Copy link
Contributor

@dawei-nava dawei-nava Mar 7, 2024

Choose a reason for hiding this comment

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

👍 to make it clear.

analytics.idv_doc_auth_document_capture_submitted(
**form_response.to_h.merge(analytics_arguments),
)

Funnel::DocAuth::RegisterStep.new(current_user.id, sp_session[:issuer]).
call('document_capture', :update, true)

cancel_establishing_in_person_enrollments

if result.success?
if form_response.success?
redirect_to idv_ssn_url
else
redirect_to idv_document_capture_url
Expand Down Expand Up @@ -98,7 +100,8 @@ def analytics_arguments
end

def handle_stored_result
if stored_result&.success? && selfie_requirement_met?
if stored_result&.success?(selfie_required: decorated_sp_session.selfie_required?) &&
selfie_requirement_met?
save_proofing_components(current_user)
extract_pii_from_doc(current_user, store_in_session: true)
flash[:success] = t('doc_auth.headings.capture_complete')
Expand Down
14 changes: 9 additions & 5 deletions app/controllers/idv/hybrid_mobile/document_capture_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,17 @@ def show

def update
document_capture_session.confirm_ocr
result = handle_stored_result
form_response = handle_stored_result

analytics.idv_doc_auth_document_capture_submitted(**result.to_h.merge(analytics_arguments))
analytics.idv_doc_auth_document_capture_submitted(
**form_response.to_h.merge(analytics_arguments),
)

Funnel::DocAuth::RegisterStep.new(document_capture_user.id, sp_session[:issuer]).
call('document_capture', :update, true)

# rate limiting redirect is in ImageUploadResponsePresenter
if result.success?
if form_response.success?
flash[:success] = t('doc_auth.headings.capture_complete')
redirect_to idv_hybrid_mobile_capture_complete_url
else
Expand Down Expand Up @@ -63,7 +65,8 @@ def analytics_arguments
end

def handle_stored_result
if stored_result&.success? && selfie_requirement_met?
if stored_result&.success?(selfie_required: decorated_sp_session.selfie_required?) &&
selfie_requirement_met?
save_proofing_components(document_capture_user)
extract_pii_from_doc(document_capture_user)
successful_response
Expand All @@ -74,7 +77,8 @@ def handle_stored_result
end

def confirm_document_capture_needed
return unless stored_result&.success?
return unless stored_result&.
success?(selfie_required: decorated_sp_session.selfie_required?)
return if redo_document_capture_pending?

redirect_to idv_hybrid_mobile_capture_complete_url
Expand Down
3 changes: 2 additions & 1 deletion app/controllers/idv/link_sent_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ def render_step_incomplete_error
end

def take_photo_with_phone_successful?
stored_result&.success? && selfie_requirement_met?
stored_result&.success?(selfie_required: decorated_sp_session.selfie_required?) &&
selfie_requirement_met?
end
end
end
5 changes: 3 additions & 2 deletions app/services/doc_auth/lexis_nexis/responses/true_id_response.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,13 @@ def billed?
end

# @return [:success, :fail, :not_processed]
# When selfie result is missing, return :not_processed
# When selfie result is missing or not requested:
# return :not_processed
# Otherwise:
# return :success if selfie check result == 'Pass'
# return :fail
def selfie_status
return :not_processed if selfie_result.nil?
return :not_processed if selfie_result.nil? || !@liveness_checking_enabled
Copy link
Contributor

@eileen-nava eileen-nava Mar 5, 2024

Choose a reason for hiding this comment

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

Good stuff. 👍🏻

Copy link
Contributor

@eileen-nava eileen-nava Mar 5, 2024

Choose a reason for hiding this comment

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

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

selfie_result == 'Pass' ? :success : :fail
end

Expand Down
2 changes: 1 addition & 1 deletion app/services/doc_auth/mock/result_response.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ def pii_from_doc
end

def success?
doc_auth_success? && (selfie_check_performed? ? selfie_passed? : true)
doc_auth_success? && (@selfie_required ? selfie_passed? : true)
end

def attention_with_barcode?
Expand Down
4 changes: 2 additions & 2 deletions app/services/doc_auth/selfie_concern.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def selfie_check_performed?
SELFIE_PERFORMED_STATUSES.include?(selfie_status)
end

private
private

SELFIE_PERFORMED_STATUSES = %i[success fail]

Expand All @@ -43,5 +43,5 @@ def selfie_check_performed?
def get_portrait_error(portrait_match_results)
portrait_match_results&.with_indifferent_access&.dig(:FaceErrorMessage)
end
end
end
end
4 changes: 2 additions & 2 deletions app/services/document_capture_session_result.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ def selfie_status
self[:selfie_status].to_sym
end

def success_status
def success_status(selfie_required:)
Copy link
Contributor

@zachmargolis zachmargolis Mar 6, 2024

Choose a reason for hiding this comment

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

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?

# 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
Copy link
Contributor

@eileen-nava eileen-nava Mar 5, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@amirbey amirbey Mar 6, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@eileen-nava eileen-nava Mar 6, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@amirbey amirbey Mar 6, 2024

Choose a reason for hiding this comment

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

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 😐

Copy link
Contributor

@dawei-nava dawei-nava Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@dawei-nava dawei-nava Mar 7, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@eileen-nava eileen-nava Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

end

alias_method :success?, :success_status
Expand Down
30 changes: 24 additions & 6 deletions spec/controllers/idv/image_uploads_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
let(:back_image) { DocAuthImageFixtures.document_back_image_multipart }
let(:selfie_img) { nil }
let(:state_id_number) { 'S59397998' }
let(:liveness_checking_required) { false }

describe '#create' do
subject(:action) do
Expand Down Expand Up @@ -354,6 +355,7 @@
context 'selfie included' do
let(:back_image) { DocAuthImageFixtures.portrait_match_success_yaml }
let(:selfie_img) { DocAuthImageFixtures.selfie_image_multipart }
let(:liveness_checking_required) { true }

before do
allow(controller.decorated_sp_session).to receive(:selfie_required?).and_return(true)
Expand All @@ -368,15 +370,19 @@
image_source: :unknown,
user_uuid: an_instance_of(String),
uuid_prefix: nil,
liveness_checking_required: true,
liveness_checking_required: liveness_checking_required,
Copy link
Contributor

@eileen-nava eileen-nava Mar 5, 2024

Choose a reason for hiding this comment

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

Nice clean-up. 👏🏻

images_cropped: false,
).and_call_original

action

expect(response.status).to eq(200)
expect(json[:success]).to eq(true)
expect(document_capture_session.reload.load_result.success?).to eq(true)
expect(
document_capture_session.reload.load_result.success?(
selfie_required: liveness_checking_required,
),
).to eq(true)
expect(document_capture_session.reload.load_result.selfie_check_performed?).to eq(true)
end
end
Expand All @@ -390,15 +396,18 @@
image_source: :unknown,
user_uuid: an_instance_of(String),
uuid_prefix: nil,
liveness_checking_required: false,
liveness_checking_required: liveness_checking_required,
images_cropped: false,
).and_call_original

action

expect(response.status).to eq(200)
expect(json[:success]).to eq(true)
expect(document_capture_session.reload.load_result.success?).to eq(true)
expect(
document_capture_session.reload.load_result.
success?(selfie_required: liveness_checking_required),
).to eq(true)
end

it 'tracks events' do
Expand Down Expand Up @@ -1271,12 +1280,17 @@

let(:back_image) { DocAuthImageFixtures.portrait_match_success_yaml }
let(:selfie_img) { DocAuthImageFixtures.selfie_image_multipart }
let(:liveness_checking_required) { true }

it 'returns a successful response' do
action
expect(response.status).to eq(200)
expect(json[:success]).to eq(true)
expect(document_capture_session.reload.load_result.success?).to eq(true)
expect(
document_capture_session.reload.load_result.success?(
selfie_required: liveness_checking_required,
),
).to eq(true)
expect(document_capture_session.reload.load_result.selfie_check_performed?).to eq(true)
end

Expand All @@ -1296,7 +1310,11 @@
action
expect(response.status).to eq(200)
expect(json[:success]).to eq(true)
expect(document_capture_session.reload.load_result.success?).to eq(true)
expect(
document_capture_session.reload.load_result.success?(
selfie_required: liveness_checking_required,
),
).to eq(true)
end
end
end
Expand Down
11 changes: 9 additions & 2 deletions spec/features/idv/doc_auth/document_capture_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,11 +192,18 @@
expect_step_indicator_current_step(t('step_indicator.flows.idv.verify_id'))
expect(page).not_to have_content(t('doc_auth.headings.document_capture_selfie'))

attach_and_submit_images
# doc auth is successful while liveness is not req'd
attach_images(
Rails.root.join(
'spec', 'fixtures',
'ial2_test_credential_no_liveness.yml'
),
)
submit_images

expect(page).to have_current_path(idv_ssn_url)
expect_costing_for_document
expect(DocAuthLog.find_by(user_id: user.id).state).to eq('MT')
expect(DocAuthLog.find_by(user_id: user.id).state).to eq('NY')

expect(page).to have_current_path(idv_ssn_url)
fill_out_ssn_form_ok
Expand Down
2 changes: 1 addition & 1 deletion spec/models/document_capture_session_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
record.store_result_from_response(doc_auth_response)
result = record.load_result

expect(result.success?).to eq(doc_auth_response.success?)
expect(result.success?(selfie_required: true)).to eq(doc_auth_response.success?)
expect(result.pii).to eq(doc_auth_response.pii_from_doc.deep_symbolize_keys)
end

Expand Down
Loading