diff --git a/app/controllers/idv/capture_doc_status_controller.rb b/app/controllers/idv/capture_doc_status_controller.rb index bcc8907d953..ddd396dabbe 100644 --- a/app/controllers/idv/capture_doc_status_controller.rb +++ b/app/controllers/idv/capture_doc_status_controller.rb @@ -28,7 +28,7 @@ def status :unauthorized elsif document_capture_session.cancelled_at :gone - elsif rate_limiter.limited? + elsif rate_limited_and_failed? :too_many_requests elsif confirmed_barcode_attention_result? || user_has_establishing_in_person_enrollment? :ok @@ -45,8 +45,7 @@ def status def redirect_url return unless document_capture_session - - if rate_limiter.limited? + if rate_limited_and_failed? idv_session_errors_rate_limited_url elsif user_has_establishing_in_person_enrollment? idv_in_person_url @@ -54,8 +53,9 @@ def redirect_url end def session_result - return @session_result if defined?(@session_result) - @session_result = document_capture_session.load_result + # we need to reload fresh, otherwise when in real environment it almost always + # return outdated result, though this potentially increase load on backstore (redis) + document_capture_session.load_result end def document_capture_session @@ -98,10 +98,15 @@ def had_barcode_attention_result? end def redo_document_capture_pending? + return true if !session_result && document_capture_session&.requested_at.present? return unless session_result&.dig(:captured_at) return unless document_capture_session.requested_at document_capture_session.requested_at > session_result.captured_at end + + def rate_limited_and_failed? + rate_limiter.limited? && !session_result&.success? && !redo_document_capture_pending? + end end end diff --git a/app/controllers/idv/link_sent_controller.rb b/app/controllers/idv/link_sent_controller.rb index 15bc80e766e..107c9f2df1d 100644 --- a/app/controllers/idv/link_sent_controller.rb +++ b/app/controllers/idv/link_sent_controller.rb @@ -7,7 +7,7 @@ class LinkSentController < ApplicationController include IdvStepConcern include StepIndicatorConcern - before_action :confirm_not_rate_limited + before_action :confirm_not_rate_limited, except: [:update] before_action :confirm_step_allowed def show diff --git a/app/forms/idv/api_image_upload_form.rb b/app/forms/idv/api_image_upload_form.rb index 5445711e84b..d11870d3305 100644 --- a/app/forms/idv/api_image_upload_form.rb +++ b/app/forms/idv/api_image_upload_form.rb @@ -79,8 +79,11 @@ def validate_form end def post_images_to_client - timer = JobHelpers::Timer.new + # user submit a request, set the requested_at timestamp + document_capture_session.requested_at = Time.zone.now + document_capture_session.save! + timer = JobHelpers::Timer.new response = timer.time('vendor_request') do doc_auth_client.post_images( front_image: front_image_bytes, @@ -481,41 +484,41 @@ def track_event(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: [], - selfie: [], - } - 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 || {} failed_front_fingerprint = nil failed_back_fingerprint = nil - if errors_hash[:front] || errors_hash[:back] - if errors_hash[:front] + selfie_image_fingerprint = nil + if image_resubmission_check? + if errors_hash[:front] || errors_hash[:back] + if errors_hash[:front] + failed_front_fingerprint = extra_attributes[:front_image_fingerprint] + end + if errors_hash[:back] + failed_back_fingerprint = extra_attributes[:back_image_fingerprint] + end + elsif !client_response.doc_auth_success? failed_front_fingerprint = extra_attributes[:front_image_fingerprint] - end - if errors_hash[:back] failed_back_fingerprint = extra_attributes[:back_image_fingerprint] end - elsif !client_response.doc_auth_success? - failed_front_fingerprint = extra_attributes[:front_image_fingerprint] - failed_back_fingerprint = extra_attributes[:back_image_fingerprint] + selfie_image_fingerprint = extra_attributes[:selfie_image_fingerprint] end document_capture_session.store_failed_auth_data( front_image_fingerprint: failed_front_fingerprint, back_image_fingerprint: failed_back_fingerprint, - selfie_image_fingerprint: extra_attributes[:selfie_image_fingerprint], + selfie_image_fingerprint: selfie_image_fingerprint, doc_auth_success: client_response.doc_auth_success?, selfie_status: client_response.selfie_status, ) elsif doc_pii_response && !doc_pii_response.success? document_capture_session.store_failed_auth_data( - front_image_fingerprint: extra_attributes[:front_image_fingerprint], - back_image_fingerprint: extra_attributes[:back_image_fingerprint], - selfie_image_fingerprint: extra_attributes[:selfie_image_fingerprint], + front_image_fingerprint: image_resubmission_check? ? + extra_attributes[:front_image_fingerprint] : nil, + back_image_fingerprint: image_resubmission_check? ? + extra_attributes[:back_image_fingerprint] : nil, + selfie_image_fingerprint: image_resubmission_check? ? + extra_attributes[:selfie_image_fingerprint] : nil, doc_auth_success: client_response.doc_auth_success?, selfie_status: client_response.selfie_status, ) diff --git a/app/services/doc_auth/mock/doc_auth_mock_client.rb b/app/services/doc_auth/mock/doc_auth_mock_client.rb index 46e8471794a..eb747c96589 100644 --- a/app/services/doc_auth/mock/doc_auth_mock_client.rb +++ b/app/services/doc_auth/mock/doc_auth_mock_client.rb @@ -12,6 +12,7 @@ def initialize(**config_keywords) class << self attr_reader :response_mocks + attr_reader :delay attr_accessor :last_uploaded_front_image attr_accessor :last_uploaded_back_image end @@ -21,10 +22,15 @@ def self.mock_response!(method:, response:) @response_mocks[method.to_sym] = response end + def self.response_delay(delay) + @delay = delay + end + def self.reset! @response_mocks = {} @last_uploaded_front_image = nil @last_uploaded_back_image = nil + @delay = nil end # rubocop:disable Lint/UnusedMethodArgument @@ -54,6 +60,11 @@ def post_images( uuid_prefix: nil, liveness_checking_required: false ) + if self.class.delay + # mimic realistic situations where we have delays + # for testing result polling in hybrid flow + sleep self.class.delay + end return mocked_response_for_method(__method__) if method_mocked?(__method__) instance_id = SecureRandom.uuid diff --git a/spec/controllers/idv/capture_doc_status_controller_spec.rb b/spec/controllers/idv/capture_doc_status_controller_spec.rb index 9638ce86b28..fb8b24db184 100644 --- a/spec/controllers/idv/capture_doc_status_controller_spec.rb +++ b/spec/controllers/idv/capture_doc_status_controller_spec.rb @@ -74,19 +74,6 @@ end end - context 'when the user is rate limited' do - before do - RateLimiter.new(rate_limit_type: :idv_doc_auth, user: user).increment_to_limited! - end - - it 'returns rate_limited with redirect' do - get :show - - expect(response.status).to eq(429) - expect(JSON.parse(response.body)).to include('redirect') - end - end - context 'when result is pending' do it 'returns pending result' do get :show diff --git a/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb index 401ba8da668..a74be0741d5 100644 --- a/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb +++ b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb @@ -107,10 +107,8 @@ end it 'shows the waiting screen correctly after cancelling from mobile and restarting', js: true do - user = nil - perform_in_browser(:desktop) do - user = sign_in_and_2fa_user + sign_in_and_2fa_user complete_doc_auth_steps_before_hybrid_handoff_step clear_and_fill_in(:doc_auth_phone, phone_number) click_send_link @@ -142,20 +140,25 @@ before do allow(IdentityConfig.store).to receive(:doc_auth_max_attempts).and_return(max_attempts) + allow(IdentityConfig.store).to receive(:doc_auth_check_failed_image_resubmission_enabled). + and_return(false) + # network error should not be counted for rate limiting DocAuth::Mock::DocAuthMockClient.mock_response!( method: :post_front_image, response: DocAuth::Response.new( success: false, - errors: { network: I18n.t('doc_auth.errors.general.network_error') }, + errors: { general_error: I18n.t('doc_auth.errors.general.multiple_front_id_failures') }, ), ) end - it 'shows capture complete on mobile and error page on desktop', js: true do - user = nil + after do + DocAuth::Mock::DocAuthMockClient.reset! + end + it 'does not rate limit on last attempt if successful', js: true do perform_in_browser(:desktop) do - user = sign_in_and_2fa_user + sign_in_and_2fa_user complete_doc_auth_steps_before_hybrid_handoff_step clear_and_fill_in(:doc_auth_phone, phone_number) click_send_link @@ -173,7 +176,44 @@ click_on t('idv.failure.button.warning') end - # final failure + # reset to return mocked normal success response for the last attempt + DocAuth::Mock::DocAuthMockClient.reset! + DocAuth::Mock::DocAuthMockClient.response_delay(8) + attach_and_submit_images + + expect(page).to have_current_path(idv_hybrid_mobile_capture_complete_url) + expect(page).to have_content(t('doc_auth.headings.capture_complete').tr(' ', ' ')) + expect(page).to have_text(t('doc_auth.instructions.switch_back')) + end + + perform_in_browser(:desktop) do + expect(page).to_not have_current_path(idv_session_errors_rate_limited_path, wait: 10) + expect(page).to_not have_content(t('doc_auth.headings.text_message'), wait: 10) + expect(page).to have_current_path(idv_ssn_path, wait: 10) + end + end + + it 'shows capture complete on mobile and error page on desktop', js: true do + perform_in_browser(:desktop) do + sign_in_and_2fa_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')) + end + + expect(@sms_link).to be_present + + perform_in_browser(:mobile) do + visit @sms_link + + (max_attempts - 1).times do + attach_and_submit_images + click_on t('idv.failure.button.warning') + end + + DocAuth::Mock::DocAuthMockClient.response_delay(8) attach_and_submit_images expect(page).to have_current_path(idv_hybrid_mobile_capture_complete_url) @@ -189,10 +229,8 @@ context 'barcode read error on mobile (redo document capture)' do it 'continues to ssn on desktop when user selects Continue', js: true do - user = nil - perform_in_browser(:desktop) do - user = sign_in_and_2fa_user + sign_in_and_2fa_user complete_doc_auth_steps_before_hybrid_handoff_step clear_and_fill_in(:doc_auth_phone, phone_number) click_send_link @@ -271,10 +309,8 @@ to receive(:biometric_comparison_required?).and_return(true) end it 'continues to ssn on desktop when user selects Continue', js: true do - user = nil - perform_in_browser(:desktop) do - user = sign_in_and_2fa_user + sign_in_and_2fa_user complete_doc_auth_steps_before_document_capture_step mock_doc_auth_attention_with_barcode attach_and_submit_images diff --git a/spec/services/rate_limiter_spec.rb b/spec/services/rate_limiter_spec.rb index 22a2e920922..53d15892d93 100644 --- a/spec/services/rate_limiter_spec.rb +++ b/spec/services/rate_limiter_spec.rb @@ -106,7 +106,7 @@ expect(rate_limiter.limited?).to eq(true) end - it 'returns false if the attempts < max_attempts' do + it 'returns false if the attempts <= max_attempts' do (max_attempts - 1).times do expect(rate_limiter.limited?).to eq(false) rate_limiter.increment!