Skip to content
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.

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

LG-14973 Add PII check to Socure flow #11747

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions app/controllers/concerns/idv/document_capture_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ def error_hash(message)
{
message: message || I18n.t('doc_auth.errors.general.network_error'),
socure: stored_result&.errors&.dig(:socure),
pii_validation: stored_result&.errors&.dig(:pii_validation),
}
end

Expand Down
2 changes: 2 additions & 0 deletions app/controllers/concerns/idv/socure_errors_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ def error_code_for(result)
result.errors.dig(:socure, :reason_codes).first
elsif result.errors[:network]
:network
elsif result.errors[:pii_validation]
:pii_validation
else
# No error information available (shouldn't happen). Default
# to :network if it does.
Expand Down
1 change: 1 addition & 0 deletions app/controllers/idv/document_capture_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ def analytics_arguments
skip_hybrid_handoff: idv_session.skip_hybrid_handoff,
liveness_checking_required: resolved_authn_context_result.facial_match?,
selfie_check_required: resolved_authn_context_result.facial_match?,
pii_like_keypaths: [[:pii]],
}.merge(ab_test_analytics_buckets)
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ def analytics_arguments
analytics_id: 'Doc Auth',
liveness_checking_required: resolved_authn_context_result.facial_match?,
selfie_check_required: resolved_authn_context_result.facial_match?,
pii_like_keypaths: [[:pii]],
}.merge(
ab_test_analytics_buckets,
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ def analytics_arguments
analytics_id: 'Doc Auth',
liveness_checking_required: false,
selfie_check_required: false,
pii_like_keypaths: [[:pii]],
}
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ def track_event(error_code:)
attributes = {
error_code:,
remaining_submit_attempts:,
pii_like_keypaths: [[:pii]],
}

analytics.idv_doc_auth_socure_error_visited(**attributes)
Expand Down
1 change: 1 addition & 0 deletions app/controllers/idv/socure/document_capture_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ def analytics_arguments
skip_hybrid_handoff: idv_session.skip_hybrid_handoff,
liveness_checking_required: resolved_authn_context_result.facial_match?,
selfie_check_required: resolved_authn_context_result.facial_match?,
pii_like_keypaths: [[:pii]],
}.merge(ab_test_analytics_buckets)
end
end
Expand Down
3 changes: 3 additions & 0 deletions app/controllers/idv/socure/errors_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ def remaining_submit_attempts
def track_event(error_code:)
attributes = { error_code: }.merge(ab_test_analytics_buckets)
attributes[:remaining_submit_attempts] = remaining_submit_attempts
attributes[:pii_like_keypaths] = [[:pii]]

analytics.idv_doc_auth_socure_error_visited(**attributes)
end
Expand All @@ -71,6 +72,8 @@ def error_code_for(result)
result.errors.dig(:socure, :reason_codes).first
elsif result.errors[:network]
:network
elsif result.errors[:pii_validation]
:pii_validation
else
# No error information available (shouldn't happen). Default
# to :network if it does.
Expand Down
1 change: 1 addition & 0 deletions app/jobs/socure_docv_results_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ def log_verification_request(docv_result_response:, vendor_request_time_in_ms:)
remaining_submit_attempts: rate_limiter&.remaining_count,
vendor_request_time_in_ms:,
async:,
pii_like_keypaths: [[:pii]],
).except(:attention_with_barcode, :selfie_live, :selfie_quality_good,
:selfie_status),
)
Expand Down
22 changes: 15 additions & 7 deletions app/services/doc_auth/socure/responses/docv_result_response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ def initialize(http_response:,
@pii_from_doc = read_pii

super(
success: successful_result?,
success: successful_result? && pii_valid?,
errors: error_messages,
pii_from_doc:,
extra: extra_attributes,
pii_from_doc: @pii_from_doc,
)
rescue StandardError => e
NewRelic::Agent.notice_error(e)
Expand Down Expand Up @@ -96,11 +96,13 @@ def successful_result?
end

def error_messages
return {} if successful_result?

{
socure: { reason_codes: get_data(DATA_PATHS[:reason_codes]) },
}
if !successful_result?
{ socure: { reason_codes: get_data(DATA_PATHS[:reason_codes]) } }
elsif !pii_valid?
{ pii_validation: 'failed' }
Copy link
Contributor

Choose a reason for hiding this comment

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

we could call this like, local_attribute_validation or something and not have to worry about having pii in the key name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kinda prefer the clarity, since we have a ticket coming up shortly to actually display more specific errors about pii, and would likely want to use pii* keys anyway; might as well cope with it now.

else
{}
end
end

def read_pii
Expand Down Expand Up @@ -176,6 +178,12 @@ def parse_date(date_string)
Rails.logger.info(message)
nil
end

def pii_valid?
return @pii_valid if !@pii_valid.nil?

@pii_valid = Idv::DocPiiForm.new(pii: pii_from_doc.to_h).submit.success?
amirbey marked this conversation as resolved.
Show resolved Hide resolved
end
end
end
end
Expand Down
23 changes: 23 additions & 0 deletions spec/features/idv/doc_auth/socure_document_capture_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,29 @@
it_behaves_like 'a properly categorized Socure error', 'I856', 'doc_auth.headers.id_not_found'
end

context 'Pii validation fails' do
before do
allow_any_instance_of(Idv::DocPiiForm).to receive(:zipcode).and_return(:invalid_junk)
end

it 'presents as a type 1 error' do
stub_docv_verification_data_pass(docv_transaction_token: @docv_transaction_token)

visit_idp_from_oidc_sp_with_ial2
@user = sign_in_and_2fa_user

complete_doc_auth_steps_before_document_capture_step
click_idv_continue

socure_docv_upload_documents(
docv_transaction_token: @docv_transaction_token,
)
visit idv_socure_document_capture_update_path

expect(page).to have_content(t('doc_auth.headers.unreadable_id'))
end
end

def expect_rate_limited_header(expected_to_be_present)
review_issues_h1_heading = strip_tags(t('doc_auth.errors.rate_limited_heading'))
if expected_to_be_present
Expand Down
45 changes: 45 additions & 0 deletions spec/features/idv/hybrid_mobile/hybrid_socure_mobile_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -570,5 +570,50 @@

it_behaves_like 'document request API failure'
end

context 'Pii validation fails' do
before do
allow_any_instance_of(Idv::DocPiiForm).to receive(:zipcode).and_return(:invalid_junk)
end

it 'presents as a type 1 error', js: true do
user = nil

perform_in_browser(:desktop) do
visit_idp_from_sp_with_ial2(sp)
user = sign_up_and_2fa_ial1_user

complete_doc_auth_steps_before_hybrid_handoff_step
clear_and_fill_in(:doc_auth_phone, phone_number)
click_send_link

expect(page).to have_content(t('doc_auth.headings.text_message'))
expect(page).to have_content(t('doc_auth.info.you_entered'))
expect(page).to have_content('+1 415-555-0199')

# Confirm that Continue button is not shown when polling is enabled
expect(page).not_to have_content(t('doc_auth.buttons.continue'))
end

expect(@sms_link).to be_present

perform_in_browser(:mobile) do
visit @sms_link

stub_docv_verification_data_pass(docv_transaction_token: @docv_transaction_token)

click_idv_continue

socure_docv_upload_documents(docv_transaction_token: @docv_transaction_token)
visit idv_hybrid_mobile_socure_document_capture_update_url

expect(page).to have_content(t('doc_auth.headers.unreadable_id'))
end

perform_in_browser(:desktop) do
expect(page).to have_current_path(idv_link_sent_path)
end
end
end
end
end
2 changes: 2 additions & 0 deletions spec/forms/idv/api_image_upload_form_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
fileName: front_image_file_name,
}
end

let(:back_image_metadata) do
{
width: 20,
Expand Down Expand Up @@ -732,6 +733,7 @@
end
end
end

describe '#store_failed_images' do
let(:doc_pii_response) { instance_double(Idv::DocAuthFormResponse) }
let(:client_response) { instance_double(DocAuth::Response) }
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
require 'rails_helper'

RSpec.describe DocAuth::Socure::Responses::DocvResultResponse do
subject(:docv_response) do
http_response = Struct.new(:body).new(SocureDocvFixtures.pass_json)
described_class.new(http_response:)
end

context 'Socure says OK and the PII is valid' do
it 'succeeds' do
expect(docv_response.success?).to be(true)
end
end

context 'Socure says OK but the PII is invalid' do
before do
allow_any_instance_of(Idv::DocPiiForm).to receive(:zipcode).and_return(:invalid_junk)
end

it 'fails' do
expect(docv_response.success?).to be(false)
end

it 'with a pii failure error' do
expect(docv_response.errors).to eq({ pii_validation: 'failed' })
end
end
end