From fa2a1be73e071b9e162e296da0ad6aa283fca05f Mon Sep 17 00:00:00 2001 From: Tomas Apodaca Date: Mon, 1 May 2023 09:58:25 -0700 Subject: [PATCH 1/3] move update to VerifyInfoConcern --- .../concerns/idv/verify_info_concern.rb | 51 ++++++++++++++++ .../idv/in_person/verify_info_controller.rb | 59 ++++--------------- app/controllers/idv/verify_info_controller.rb | 51 ++-------------- .../in_person/verify_info_controller_spec.rb | 14 ++++- .../idv/verify_info_controller_spec.rb | 17 ++++++ 5 files changed, 98 insertions(+), 94 deletions(-) diff --git a/app/controllers/concerns/idv/verify_info_concern.rb b/app/controllers/concerns/idv/verify_info_concern.rb index a6a6d5160cf..109f0fbfabb 100644 --- a/app/controllers/concerns/idv/verify_info_concern.rb +++ b/app/controllers/concerns/idv/verify_info_concern.rb @@ -2,6 +2,57 @@ module Idv module VerifyInfoConcern extend ActiveSupport::Concern + def update + return if idv_session.verify_info_step_document_capture_session_uuid + analytics.idv_doc_auth_verify_submitted(**analytics_arguments) + Funnel::DocAuth::RegisterStep.new(current_user.id, sp_session[:issuer]). + call('verify', :update, true) + + modify_pii_for_update + + ssn_throttle.increment! + if ssn_throttle.throttled? + idv_failure_log_throttled(:proof_ssn) + analytics.throttler_rate_limit_triggered( + throttle_type: :proof_ssn, + step_name: 'verify_info', + ) + redirect_to idv_session_errors_ssn_failure_url + return + end + + if resolution_throttle.throttled? + idv_failure_log_throttled(:idv_resolution) + redirect_to throttled_url + return + end + + document_capture_session = DocumentCaptureSession.create( + user_id: current_user.id, + issuer: sp_session[:issuer], + ) + document_capture_session.requested_at = Time.zone.now + + idv_session.verify_info_step_document_capture_session_uuid = document_capture_session.uuid + idv_session.vendor_phone_confirmation = false + idv_session.user_phone_confirmation = false + + Idv::Agent.new(pii).proof_resolution( + document_capture_session, + should_proof_state_id: should_use_aamva?(pii), + trace_id: amzn_trace_id, + user_id: current_user.id, + threatmetrix_session_id: flow_session[:threatmetrix_session_id], + request_ip: request.remote_ip, + double_address_verification: current_user.establishing_in_person_enrollment&. + capture_secondary_id_enabled || false, + ) + + redirect_to after_update_url + end + + private + def should_use_aamva?(pii) aamva_state?(pii) && !aamva_disallowed_for_service_provider? end diff --git a/app/controllers/idv/in_person/verify_info_controller.rb b/app/controllers/idv/in_person/verify_info_controller.rb index cd2f1524efc..d6c7213398e 100644 --- a/app/controllers/idv/in_person/verify_info_controller.rb +++ b/app/controllers/idv/in_person/verify_info_controller.rb @@ -33,57 +33,24 @@ def show process_async_state(load_async_state) end - def update - return if idv_session.verify_info_step_document_capture_session_uuid - analytics.idv_doc_auth_verify_submitted(**analytics_arguments) - Funnel::DocAuth::RegisterStep.new(current_user.id, sp_session[:issuer]). - call('verify', :update, true) + private + def modify_pii_for_update pii[:uuid_prefix] = ServiceProvider.find_by(issuer: sp_session[:issuer])&.app_id - pii[:state_id_type] = 'drivers_license' unless pii.blank? - - ssn_throttle.increment! - if ssn_throttle.throttled? - idv_failure_log_throttled(:proof_ssn) - analytics.throttler_rate_limit_triggered( - throttle_type: :proof_ssn, - step_name: 'verify_info', - ) - redirect_to idv_session_errors_ssn_failure_url - return - end - - if resolution_throttle.throttled? - idv_failure_log_throttled(:idv_resolution) - redirect_to throttled_url - return - end + # state_id_type is hard-coded here because it's required for proofing against + # AAMVA. We're sticking with driver's license because most states don't discern + # between various ID types and driver's license is the most common one that will + # be supported. See also LG-3852 and related findings document. + pii[:state_id_type] = 'drivers_license' unless invalid_state? + end - document_capture_session = DocumentCaptureSession.create( - user_id: current_user.id, - issuer: sp_session[:issuer], - ) - document_capture_session.requested_at = Time.zone.now - - idv_session.verify_info_step_document_capture_session_uuid = document_capture_session.uuid - idv_session.vendor_phone_confirmation = false - idv_session.user_phone_confirmation = false - - Idv::Agent.new(pii).proof_resolution( - document_capture_session, - should_proof_state_id: should_use_aamva?(pii), - trace_id: amzn_trace_id, - user_id: current_user.id, - threatmetrix_session_id: flow_session[:threatmetrix_session_id], - request_ip: request.remote_ip, - double_address_verification: current_user.establishing_in_person_enrollment&. - capture_secondary_id_enabled || false, - ) - - redirect_to idv_in_person_verify_info_url + def invalid_state? + pii.blank? end - private + def after_update_url + idv_in_person_verify_info_url + end def prev_url idv_in_person_url diff --git a/app/controllers/idv/verify_info_controller.rb b/app/controllers/idv/verify_info_controller.rb index 75e85943187..c6056041302 100644 --- a/app/controllers/idv/verify_info_controller.rb +++ b/app/controllers/idv/verify_info_controller.rb @@ -32,56 +32,15 @@ def show process_async_state(load_async_state) end - def update - return if idv_session.verify_info_step_document_capture_session_uuid - analytics.idv_doc_auth_verify_submitted(**analytics_arguments) - Funnel::DocAuth::RegisterStep.new(current_user.id, sp_session[:issuer]). - call('verify', :update, true) + private + def modify_pii_for_update pii[:uuid_prefix] = ServiceProvider.find_by(issuer: sp_session[:issuer])&.app_id - - ssn_throttle.increment! - if ssn_throttle.throttled? - idv_failure_log_throttled(:proof_ssn) - analytics.throttler_rate_limit_triggered( - throttle_type: :proof_ssn, - step_name: 'verify_info', - ) - redirect_to idv_session_errors_ssn_failure_url - return - end - - if resolution_throttle.throttled? - idv_failure_log_throttled(:idv_resolution) - redirect_to throttled_url - return - end - - document_capture_session = DocumentCaptureSession.create( - user_id: current_user.id, - issuer: sp_session[:issuer], - ) - document_capture_session.requested_at = Time.zone.now - - idv_session.verify_info_step_document_capture_session_uuid = document_capture_session.uuid - idv_session.vendor_phone_confirmation = false - idv_session.user_phone_confirmation = false - - Idv::Agent.new(pii).proof_resolution( - document_capture_session, - should_proof_state_id: should_use_aamva?(pii), - trace_id: amzn_trace_id, - user_id: current_user.id, - threatmetrix_session_id: flow_session[:threatmetrix_session_id], - request_ip: request.remote_ip, - double_address_verification: current_user.establishing_in_person_enrollment&. - capture_secondary_id_enabled || false, - ) - - redirect_to idv_verify_info_url end - private + def after_update_url + idv_verify_info_url + end def prev_url idv_ssn_url diff --git a/spec/controllers/idv/in_person/verify_info_controller_spec.rb b/spec/controllers/idv/in_person/verify_info_controller_spec.rb index 6e9c97b6218..16487d5bd14 100644 --- a/spec/controllers/idv/in_person/verify_info_controller_spec.rb +++ b/spec/controllers/idv/in_person/verify_info_controller_spec.rb @@ -33,7 +33,7 @@ ) end - it 'confirms verify step not already complete' do + it 'confirms verify step needed' do expect(subject).to have_actions( :before, :confirm_verify_info_step_needed, @@ -84,18 +84,28 @@ end describe '#update' do + it 'redirects to the expected page' do + put :update + + expect(response).to redirect_to idv_in_person_verify_info_url + end + context 'double address verification is not enabled' do let(:capture_secondary_id_enabled) { false } let(:enrollment) { InPersonEnrollment.new(capture_secondary_id_enabled:) } before do allow(user).to receive(:establishing_in_person_enrollment).and_return(enrollment) end - it 'sets uuid_prefix on pii_from_user' do + + it 'sets uuid_prefix and state_id_type on pii_from_user' do expect(Idv::Agent).to receive(:new). with(hash_including(uuid_prefix: service_provider.app_id)).and_call_original + # our test data already has the expected value by default + flow_session[:pii_from_user].delete(:state_id_type) put :update + expect(flow_session[:pii_from_user][:state_id_type]).to eq 'drivers_license' expect(flow_session[:pii_from_user][:uuid_prefix]).to eq service_provider.app_id end diff --git a/spec/controllers/idv/verify_info_controller_spec.rb b/spec/controllers/idv/verify_info_controller_spec.rb index c3e5d44b08f..dd3e10429c0 100644 --- a/spec/controllers/idv/verify_info_controller_spec.rb +++ b/spec/controllers/idv/verify_info_controller_spec.rb @@ -289,6 +289,23 @@ ) end + it 'redirects to the expected page' do + put :update + + expect(response).to redirect_to idv_verify_info_url + end + + it 'modifies pii as expected' do + app_id = 'hello-world' + sp = create(:service_provider, app_id: app_id) + sp_session = { issuer: sp.issuer } + allow(controller).to receive(:sp_session).and_return(sp_session) + + put :update + + expect(flow_session[:pii_from_doc][:uuid_prefix]).to eq app_id + end + it 'updates DocAuthLog verify_submit_count' do doc_auth_log = DocAuthLog.create(user_id: user.id) From 874e6310c71fdfcbe4a92626dc6670fb3c468534 Mon Sep 17 00:00:00 2001 From: Tomas Apodaca Date: Mon, 1 May 2023 11:08:39 -0700 Subject: [PATCH 2/3] add changelog changelog: Internal, Refactor, Move verify info update to concern From cf15ec463e617468138c25b7af0b72c8278741b4 Mon Sep 17 00:00:00 2001 From: Tomas Apodaca Date: Mon, 1 May 2023 15:18:08 -0700 Subject: [PATCH 3/3] refactor session setting --- app/controllers/concerns/idv/verify_info_concern.rb | 3 ++- .../idv/in_person/verify_info_controller.rb | 11 +++++------ app/controllers/idv/verify_info_controller.rb | 5 ++--- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/app/controllers/concerns/idv/verify_info_concern.rb b/app/controllers/concerns/idv/verify_info_concern.rb index 109f0fbfabb..b0c818cf633 100644 --- a/app/controllers/concerns/idv/verify_info_concern.rb +++ b/app/controllers/concerns/idv/verify_info_concern.rb @@ -8,7 +8,8 @@ def update Funnel::DocAuth::RegisterStep.new(current_user.id, sp_session[:issuer]). call('verify', :update, true) - modify_pii_for_update + pii[:uuid_prefix] = ServiceProvider.find_by(issuer: sp_session[:issuer])&.app_id + set_state_id_type ssn_throttle.increment! if ssn_throttle.throttled? diff --git a/app/controllers/idv/in_person/verify_info_controller.rb b/app/controllers/idv/in_person/verify_info_controller.rb index d6c7213398e..368ee1edfe3 100644 --- a/app/controllers/idv/in_person/verify_info_controller.rb +++ b/app/controllers/idv/in_person/verify_info_controller.rb @@ -35,12 +35,11 @@ def show private - def modify_pii_for_update - pii[:uuid_prefix] = ServiceProvider.find_by(issuer: sp_session[:issuer])&.app_id - # state_id_type is hard-coded here because it's required for proofing against - # AAMVA. We're sticking with driver's license because most states don't discern - # between various ID types and driver's license is the most common one that will - # be supported. See also LG-3852 and related findings document. + # state_id_type is hard-coded here because it's required for proofing against + # AAMVA. We're sticking with driver's license because most states don't discern + # between various ID types and driver's license is the most common one that will + # be supported. See also LG-3852 and related findings document. + def set_state_id_type pii[:state_id_type] = 'drivers_license' unless invalid_state? end diff --git a/app/controllers/idv/verify_info_controller.rb b/app/controllers/idv/verify_info_controller.rb index c6056041302..f15f71593c9 100644 --- a/app/controllers/idv/verify_info_controller.rb +++ b/app/controllers/idv/verify_info_controller.rb @@ -34,9 +34,8 @@ def show private - def modify_pii_for_update - pii[:uuid_prefix] = ServiceProvider.find_by(issuer: sp_session[:issuer])&.app_id - end + # state ID type isn't manually set for Idv::VerifyInfoController + def set_state_id_type; end def after_update_url idv_verify_info_url