Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
252e55c
LG-10427: add fileds in document capture session result to memorize f…
dawei-nava Sep 5, 2023
e3eeaaf
LG-10427: calling extra_attributes before validation completion will …
dawei-nava Sep 5, 2023
e830693
LG-10427: update test for capture session and result.
dawei-nava Aug 30, 2023
4dd960a
LG-10427: AssureID and TrueID services behave differently by using ht…
dawei-nava Aug 31, 2023
6ef091a
LG-10427: refactor test response generation for AssureID.
dawei-nava Sep 1, 2023
ade4e97
LG-10427: use meta programming. Linter issue.
dawei-nava Sep 1, 2023
b858847
LG-10427: remove commented change.
dawei-nava Sep 1, 2023
f893126
LG-10427: remove accidental change.
dawei-nava Sep 5, 2023
7d7de5a
LG-10427: implement client side validation.
dawei-nava Sep 6, 2023
fbc7f95
LG-10427: fix test.
dawei-nava Sep 6, 2023
0afd669
LG-10427: skip the warning screen when the backend detected duplicate…
dawei-nava Sep 7, 2023
9695888
LG-10427: add analytics event.
dawei-nava Sep 7, 2023
2944912
LG-10427: use i18n text. And avoid nested promises in acuant-capture.…
dawei-nava Sep 8, 2023
e348fe0
LG-10427: update translation. fix test.
dawei-nava Sep 8, 2023
2703c19
LG-10427: Linter, typo and test fix.
dawei-nava Sep 8, 2023
53256b2
LG-10427: address comment and add tests for storing failed image fing…
dawei-nava Sep 9, 2023
28c2a4f
LG-10427: linter
dawei-nava Sep 9, 2023
48d2bf6
changelog: User-facing Improvements, Document Authentication, Prevent…
dawei-nava Sep 9, 2023
d5a1c5c
LG-10427: linter
dawei-nava Sep 9, 2023
2f26c4e
LG-10427: add flag so we can facilitate tests in some cases.
dawei-nava Sep 9, 2023
3c4afc6
LG-10427: flaky test?
dawei-nava Sep 10, 2023
25ea516
LG-10427: linter.
dawei-nava Sep 10, 2023
64fb0b8
LG-10427: linter.
dawei-nava Sep 10, 2023
e224903
LG-10427: order of events.
dawei-nava Sep 10, 2023
f7636d5
LG-10427: linter
dawei-nava Sep 10, 2023
11ac7c2
LG-10427: address comments.
dawei-nava Sep 13, 2023
ae28a46
LG-10427: it should be a hash.
dawei-nava Sep 13, 2023
4363619
LG-10427: test fix and clean up.
dawei-nava Sep 13, 2023
1ef09cc
LG-10427: wording.
dawei-nava Sep 13, 2023
ca56d6d
LG-10427: wording.
dawei-nava Sep 13, 2023
aa69851
LG-10427: rebase.
dawei-nava Sep 18, 2023
0b89255
LG-10427: WIP, refactor with Andrew.
dawei-nava Sep 15, 2023
8f01682
LG-10427: skip warning, and mark it can complete when different file …
dawei-nava Sep 18, 2023
41cf775
LG-10427: address comment, testing and type.
dawei-nava Sep 18, 2023
7be6ffe
LG-10427: address comment, type.
dawei-nava Sep 18, 2023
0b5e64c
LG-10427: type mismatches.
dawei-nava Sep 18, 2023
f1625f0
LG-10427: missing property in tests.
dawei-nava Sep 18, 2023
2790ebb
LG-10427: clean up directive.
dawei-nava Sep 19, 2023
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
98 changes: 90 additions & 8 deletions app/forms/idv/api_image_upload_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ class ApiImageUploadForm
validates_presence_of :document_capture_session

validate :validate_images
validate :validate_duplicate_images, if: :image_resubmission_check?
validate :limit_if_rate_limited

def initialize(params, service_provider:, analytics: nil,
Expand Down Expand Up @@ -41,8 +42,9 @@ def submit
doc_pii_response: doc_pii_response,
)

failed_fingerprints = store_failed_images(client_response, doc_pii_response)
response.extra[:failed_image_fingerprints] = failed_fingerprints
track_event(response)

response
end

Expand Down Expand Up @@ -92,7 +94,6 @@ def post_images_to_client
client_response: response,
vendor_request_time_in_ms: timer.results['vendor_request'],
)

response
end

Expand Down Expand Up @@ -122,7 +123,8 @@ def validate_pii_from_doc(client_response)
end

def extra_attributes
return @extra_attributes if defined?(@extra_attributes)
return @extra_attributes if defined?(@extra_attributes) &&
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I want to be sure I understand the need for this update to the guard statement. Were you trying to ensure that the rate limiter didn't get stale attempts data?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@eileen-nava, yes, because the timing of the extra_attributes called, it may cache stale data ( the attempts).

@extra_attributes&.dig('attempts') == attempts
@extra_attributes = {
attempts: attempts,
remaining_attempts: remaining_attempts,
Expand All @@ -131,17 +133,26 @@ def extra_attributes
flow_path: params[:flow_path],
}

@extra_attributes[:front_image_fingerprint] = front_image_fingerprint
@extra_attributes[:back_image_fingerprint] = back_image_fingerprint
@extra_attributes.merge!(getting_started_ab_test_analytics_bucket)
@extra_attributes
end

def front_image_fingerprint
return @front_image_fingerprint if @front_image_fingerprint
if readable?(:front)
@extra_attributes[:front_image_fingerprint] =
@front_image_fingerprint =
Digest::SHA256.urlsafe_base64digest(front_image_bytes)
end
end

def back_image_fingerprint
return @back_image_fingerprint if @back_image_fingerprint
if readable?(:back)
@extra_attributes[:back_image_fingerprint] =
@back_image_fingerprint =
Digest::SHA256.urlsafe_base64digest(back_image_bytes)
end

@extra_attributes.merge!(getting_started_ab_test_analytics_bucket)
end

def remaining_attempts
Expand Down Expand Up @@ -199,8 +210,31 @@ def validate_images
end
end

def validate_duplicate_images
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🙌🏻

capture_result = document_capture_session&.load_result
return unless capture_result
error_sides = []
if capture_result&.failed_front_image?(front_image_fingerprint)
errors.add(
:front, t('doc_auth.errors.doc.resubmit_failed_image'), type: :duplicate_image
)
error_sides << 'front'
end

if capture_result&.failed_back_image?(back_image_fingerprint)
errors.add(
:back, t('doc_auth.errors.doc.resubmit_failed_image'), type: :duplicate_image
)
error_sides << 'back'
end
unless error_sides.empty?
analytics.idv_doc_auth_failed_image_resubmitted(
side: error_sides.length == 2 ? 'both' : error_sides[0], **extra_attributes,
Comment thread
eileen-nava marked this conversation as resolved.
Outdated
)
end
end

def limit_if_rate_limited
return unless document_capture_session
return unless rate_limited?

errors.add(:limit, t('errors.doc_auth.rate_limited_heading'), type: :rate_limited)
Expand Down Expand Up @@ -367,5 +401,53 @@ def track_event(response)
failure_reason: response.errors&.except(:hints)&.presence,
)
end

##
# Store failed image fingerprints in document_capture_session_result
Comment thread
eileen-nava marked this conversation as resolved.
Outdated
# when client_response is not successful and not a network error
# ( http status except handled status 438, 439, 440 ) or doc_pii_response is not successful.
# @param [Object] client_response
# @param [Object] doc_pii_response
# @return [Object] latest failed fingerprints
def store_failed_images(client_response, doc_pii_response)
unless image_resubmission_check?
return {
front: [],
back: [],
}
end
# doc auth failed due to non network error or doc_pii is not valid
if client_response && !client_response.success? && !client_response.network_error?
errors_hash = client_response.errors&.to_h || {}
## assume both sides' error presents or both sides' error missing
failed_front_fingerprint = extra_attributes[:front_image_fingerprint]
failed_back_fingerprint = extra_attributes[:back_image_fingerprint]
## not both sides' error present nor both sides' error missing
## equivalent to: only one side error presents
only_one_side_error = errors_hash[:front]&.present? ^ errors_hash[:back]&.present?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for leaving this comment. At first the ^ operator threw me a little bit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@eileen-nava , have to google to find out this XOR logic operator.

if only_one_side_error
## find which side is missing
failed_front_fingerprint = nil unless errors_hash[:front]&.present?
failed_back_fingerprint = nil unless errors_hash[:back]&.present?
end
document_capture_session.
store_failed_auth_image_fingerprint(failed_front_fingerprint, failed_back_fingerprint)
elsif doc_pii_response && !doc_pii_response.success?
document_capture_session.store_failed_auth_image_fingerprint(
extra_attributes[:front_image_fingerprint],
extra_attributes[:back_image_fingerprint],
)
end
# retrieve updated data from session
captured_result = document_capture_session&.load_result
{
front: captured_result&.failed_front_image_fingerprints || [],
back: captured_result&.failed_back_image_fingerprints || [],
}
end

def image_resubmission_check?
IdentityConfig.store.doc_auth_check_failed_image_resubmission_enabled
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,16 @@ interface ImageAnalyticsPayload {
* capture functionality
*/
acuantCaptureMode?: AcuantCaptureMode;

/**
* Fingerprint of the image, base64 encoded SHA-256 digest
Comment thread
eileen-nava marked this conversation as resolved.
Outdated
*/
fingerprint: string | null;

/**
*
*/
failedImageResubmission: boolean;
}

interface AcuantImageAnalyticsPayload extends ImageAnalyticsPayload {
Expand All @@ -75,6 +85,7 @@ interface AcuantImageAnalyticsPayload extends ImageAnalyticsPayload {
sharpnessScoreThreshold: number;
isAssessedAsBlurry: boolean;
assessment: AcuantImageAssessment;
isAssessedAsUnsupported: boolean;
}

interface AcuantCaptureProps {
Expand Down Expand Up @@ -178,6 +189,31 @@ export function getNormalizedAcuantCaptureFailureMessage(
}
}

function getFingerPrint(file: File): Promise<string | null> {
return new Promise((resolve) => {
const reader = new FileReader();
reader.onload = () => {
const dataBuffer = reader.result;
window.crypto.subtle
.digest('SHA-256', dataBuffer as ArrayBuffer)
.then((arrayBuffer) => {
const digestArray = new Uint8Array(arrayBuffer);
const strDigest = digestArray.reduce(
(data, byte) => data + String.fromCharCode(byte),
'',
);
const base64String = window.btoa(strDigest);
const urlSafeBase64String = base64String
.replace(/\+/g, '-')
.replace(/\//g, '_')
.replace(/=+$/, '');
resolve(urlSafeBase64String);
})
.catch(() => null);
};
reader.readAsArrayBuffer(file);
});
}
function getImageDimensions(file: File): Promise<{ width: number | null; height: number | null }> {
let objectURL: string;
return file.type.indexOf('image/') === 0
Expand All @@ -196,6 +232,22 @@ function getImageDimensions(file: File): Promise<{ width: number | null; height:
: Promise.resolve({ width: null, height: null });
}

function getImageMetadata(
file: File,
): Promise<{ width: number | null; height: number | null; fingerprint: string | null }> {
const dimension = getImageDimensions(file);
const fingerprint = getFingerPrint(file);
return new Promise<{ width: number | null; height: number | null; fingerprint: string | null }>(
function (resolve) {
Promise.all([dimension, fingerprint])
.then((results) => {
resolve({ width: results[0].width, height: results[0].height, fingerprint: results[1] });
})
.catch(() => ({ width: null, height: null, fingerprint: null }));
},
);
}

/**
* Pauses default focus trap behaviors for a single tick. If a focus transition occurs during this
* tick, the focus trap's deactivation will be overridden to prevent any default focus return, in
Expand Down Expand Up @@ -289,6 +341,7 @@ function AcuantCapture(
onResetFailedCaptureAttempts,
failedSubmissionAttempts,
forceNativeCamera,
failedSubmissionImageFingerprints,
} = useContext(FailedCaptureAttemptsContext);

const hasCapture = !isError && (isReady ? isCameraSupported : isMobile);
Expand Down Expand Up @@ -330,17 +383,19 @@ function AcuantCapture(
*/
async function onUpload(nextValue: File | null) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@dawei-nava I tested locally and attempted to resubmit a yml file that had already failed. Inline errors did display that said You already tried this image, and it failed. Please try adding a different image. Awesome!

However, I was still able to click "submit" and proceed to the next error screen when there were inline errors displaying above both upload zones. Is it possible to prevent the user from clicking submit and proceeding when there are inline errors present above the upload zones? Thanks!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I saw the same behavior. I also saw that if I reused both the same "front" and "back" image, the inline error only showed on the "back" image upload section. I'm not sure but I think we'd want it to display in both sections, or if only one, the "front" section first.

In addition, when I tried again with just a failure on the "front" image upload (ial2_test_credential_forces_error.yml) but a successful image for the "back" (ial2_test_credential.yml), validation succeeded and I moved on to the SSN page. If I switched those images around (ial2_test_credential_forces_error.yml for the back and ial2_test_credential.yml for the front) , the submission fails as expected.

The combination of these two makes me wonder if something is broken with the front upload section, but I'm going to test on main first to make sure this is a problem with this branch.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm...I'm seeing the second issue in main.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@night-jellyfish , that's because the mock client you cannot really testing two different files when upload image.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@eileen-nava, that's the original intention, seems there is a issue with ordering/extra stuff. Should work now.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@dawei-nava Thanks! I am logging off now but will look at this tomorrow. 👍🏻

Copy link
Copy Markdown
Contributor

@night-jellyfish night-jellyfish Sep 13, 2023

Choose a reason for hiding this comment

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

Thanks for the info, Dawei!

I pulled the updates and tested again with ial2_test_credential_no_dob.yml and saw the following:

  1. I now saw the inline error message on the front image (yay!)
  2. Unfortunately I was still able to submit the first time I re-added the same images (so, the second time I uploaded the images)
  3. If I tried a second time to re-add the images (so, third time uploading the same images) then I could not hit submit and it moved my cursor focus to the top image select.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interestingly, if I try again but use ial2_test_credential_forces_error.yml for both front and back, I see the opposite effect:

  1. I don't see the inline error for the front image (it says "Image updated" happily in green)
  2. But, I am not allowed to submit when I re-add that file (as expected)

So I'm seeing different results with different types of errors / yml files. Maybe this is a yml issue?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@night-jellyfish
With that file, first time to submit:
firsttime

Looks like it classifes only as a back side error.

Second time:
secondtime

It alerts it is duplicate submission for the back only.

Note: Sometimes it has cached/not compiled javascript files, run bundle exec rake javascript:build to manually rebuild javascript packs and force browser reload.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@eileen-nava issue should have been resolved.

let analyticsPayload: ImageAnalyticsPayload | undefined;
let hasFailed = false;
if (nextValue) {
const { width, height } = await getImageDimensions(nextValue);

const { width, height, fingerprint } = await getImageMetadata(nextValue);
hasFailed = failedSubmissionImageFingerprints[name]?.includes(fingerprint);
analyticsPayload = getAddAttemptAnalyticsPayload({
width,
height,
fingerprint,
mimeType: nextValue.type,
source: 'upload',
size: nextValue.size,
failedImageResubmission: hasFailed,
});

trackEvent(`IdV: ${name} image added`, analyticsPayload);
}

Expand Down Expand Up @@ -472,6 +527,8 @@ function AcuantCapture(
isAssessedAsBlurry,
assessment,
size: getDecodedBase64ByteSize(nextCapture.image.data),
fingerprint: null,
failedImageResubmission: false,
});

trackEvent(`IdV: ${name} image added`, analyticsPayload);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ function DocumentCapture({ onStepChange = () => {} }: DocumentCaptureProps) {
isFailedDocType: submissionError.isFailedDocType,
captureHints: submissionError.hints,
pii: submissionError.pii,
failedImageFingerprints: submissionError.failed_image_fingerprints,
})(ReviewIssuesStep)
: ReviewIssuesStep,
title: t('errors.doc_auth.rate_limited_heading'),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { t } from '@18f/identity-i18n';
import { FormError } from '@18f/identity-form-steps';
import { FormError, FormStepsContext } from '@18f/identity-form-steps';
import { useContext } from 'react';
import AcuantCapture from './acuant-capture';

/** @typedef {import('@18f/identity-form-steps').FormStepError<*>} FormStepError */
Expand Down Expand Up @@ -43,7 +44,7 @@ function DocumentSideAcuantCapture({
className,
}) {
const error = errors.find(({ field }) => field === side)?.error;

const { changeStepCanComplete } = useContext(FormStepsContext);
return (
<AcuantCapture
ref={registerField(side, { isRequired: true })}
Expand All @@ -54,12 +55,18 @@ function DocumentSideAcuantCapture({
/* i18n-tasks-use t('doc_auth.headings.front') */
bannerText={t(`doc_auth.headings.${side}`)}
value={value}
onChange={(nextValue, metadata) =>
onChange={(nextValue, metadata) => {
onChange({
[side]: nextValue,
[`${side}_image_metadata`]: JSON.stringify(metadata),
})
}
});
if (metadata?.failedImageResubmission) {
onError(new Error(t('doc_auth.errors.doc.resubmit_failed_image')), { field: side });
changeStepCanComplete(false);
} else {
changeStepCanComplete(true);
}
}}
onCameraAccessDeclined={() => {
onError(new CameraAccessDeclinedError(), { field: side });
onError(new CameraAccessDeclinedError(undefined, { isDetail: true }));
Expand Down
Loading