From 6d97f55aeea9661be91a3cd33f3e32f743153994 Mon Sep 17 00:00:00 2001 From: Brittany Greaner Date: Wed, 1 May 2024 11:18:38 -0700 Subject: [PATCH 01/13] Fix rate limiting logic when user is on last try When we used `limited?` we found the user was prevented from completing their last chance at a request, due to the `>=`, since the attempts and max_attempts were equal. Instead we needed a way to only prevent that if the user had exceeded their max amount of tries. --- .../idv/capture_doc_status_controller.rb | 2 +- app/services/rate_limiter.rb | 4 +++ .../idv/capture_doc_status_controller_spec.rb | 18 +++++++++++++ spec/services/rate_limiter_spec.rb | 26 ++++++++++++++++++- 4 files changed, 48 insertions(+), 2 deletions(-) diff --git a/app/controllers/idv/capture_doc_status_controller.rb b/app/controllers/idv/capture_doc_status_controller.rb index bcc8907d953..251263320b5 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_limiter.exceed_max? :too_many_requests elsif confirmed_barcode_attention_result? || user_has_establishing_in_person_enrollment? :ok diff --git a/app/services/rate_limiter.rb b/app/services/rate_limiter.rb index f25751f9246..b63c64608ee 100644 --- a/app/services/rate_limiter.rb +++ b/app/services/rate_limiter.rb @@ -69,6 +69,10 @@ def maxed? attempts && attempts >= RateLimiter.max_attempts(rate_limit_type) end + def exceed_max? + !expired? && attempts && attempts > RateLimiter.max_attempts(rate_limit_type) + end + def increment! return if limited? value = nil diff --git a/spec/controllers/idv/capture_doc_status_controller_spec.rb b/spec/controllers/idv/capture_doc_status_controller_spec.rb index 9638ce86b28..f651106dc1a 100644 --- a/spec/controllers/idv/capture_doc_status_controller_spec.rb +++ b/spec/controllers/idv/capture_doc_status_controller_spec.rb @@ -87,6 +87,24 @@ end end + context 'when the user is on their last try' do + let(:max_attempts) { 1 } + + before do + allow(IdentityConfig.store).to receive(:doc_auth_max_attempts).and_return(max_attempts) + @rate_limiter = RateLimiter.new(rate_limit_type: :idv_doc_auth, user: user) + @rate_limiter.increment! + end + + it 'does not rate limit the request' do + expect(@rate_limiter.attempts).to eq(max_attempts) + + get :show + + expect(response.status).to eq(202) + end + end + context 'when result is pending' do it 'returns pending result' do get :show diff --git a/spec/services/rate_limiter_spec.rb b/spec/services/rate_limiter_spec.rb index 22a2e920922..bbf54c213ab 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! @@ -126,6 +126,30 @@ end end + describe '#exceed_max?' do + let(:max_attempts) { 3 } + + subject(:rate_limiter) { RateLimiter.new(target: '1', rate_limit_type: rate_limit_type) } + + it 'returns true when the amount of attempts is more than the max attempts' do + allow(subject).to receive(:attempts).and_return(4) + + expect(subject.exceed_max?).to eq(true) + end + + it 'returns false when the amount of attempts is equal to the max attempts' do + allow(subject).to receive(:attempts).and_return(3) + + expect(subject.exceed_max?).to eq(false) + end + + it 'returns false when the amount of attempts is less than the max attempts' do + allow(subject).to receive(:attempts).and_return(2) + + expect(subject.exceed_max?).to eq(false) + end + end + describe '#expires_at' do let(:attempted_at) { nil } let(:rate_limiter) { RateLimiter.new(target: '1', rate_limit_type: rate_limit_type) } From 67569af69d9f4a371c3dbbb5a593f17213bfa795 Mon Sep 17 00:00:00 2001 From: Brittany Greaner Date: Wed, 1 May 2024 14:19:29 -0700 Subject: [PATCH 02/13] Remove unnecessary logic In the previous commit, we were looking to prevent the `capture_doc_status_controller` from setting the rate limit too early. In this commit, we are instead removing the logic to even look at rate limiting here, since [as Dawei pointed out](https://github.com/18F/identity-idp/pull/10538#discussion_r1586655556), and as I found [a past discussion for](https://github.com/18F/identity-idp/pull/9370#discussion_r1357190769), this code should never be reached if rate limited. --- .../idv/capture_doc_status_controller.rb | 2 -- .../idv/capture_doc_status_controller_spec.rb | 31 ------------------- 2 files changed, 33 deletions(-) diff --git a/app/controllers/idv/capture_doc_status_controller.rb b/app/controllers/idv/capture_doc_status_controller.rb index 251263320b5..532df6600c3 100644 --- a/app/controllers/idv/capture_doc_status_controller.rb +++ b/app/controllers/idv/capture_doc_status_controller.rb @@ -28,8 +28,6 @@ def status :unauthorized elsif document_capture_session.cancelled_at :gone - elsif rate_limiter.exceed_max? - :too_many_requests elsif confirmed_barcode_attention_result? || user_has_establishing_in_person_enrollment? :ok elsif session_result.blank? || pending_barcode_attention_confirmation? || diff --git a/spec/controllers/idv/capture_doc_status_controller_spec.rb b/spec/controllers/idv/capture_doc_status_controller_spec.rb index f651106dc1a..fb8b24db184 100644 --- a/spec/controllers/idv/capture_doc_status_controller_spec.rb +++ b/spec/controllers/idv/capture_doc_status_controller_spec.rb @@ -74,37 +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 the user is on their last try' do - let(:max_attempts) { 1 } - - before do - allow(IdentityConfig.store).to receive(:doc_auth_max_attempts).and_return(max_attempts) - @rate_limiter = RateLimiter.new(rate_limit_type: :idv_doc_auth, user: user) - @rate_limiter.increment! - end - - it 'does not rate limit the request' do - expect(@rate_limiter.attempts).to eq(max_attempts) - - get :show - - expect(response.status).to eq(202) - end - end - context 'when result is pending' do it 'returns pending result' do get :show From bc1c63f22b0efc6644ec82bd08b5d97403be7ce4 Mon Sep 17 00:00:00 2001 From: Brittany Greaner Date: Wed, 1 May 2024 16:15:17 -0700 Subject: [PATCH 03/13] changelog: User-Facing Improvements, Doc Auth, Fix rate limiting logic when user is on last try From c3ee24a8ba23b828e604a2b2d7ab8e760cb070a5 Mon Sep 17 00:00:00 2001 From: Brittany Greaner Date: Wed, 1 May 2024 16:16:51 -0700 Subject: [PATCH 04/13] Rm added method and tests --- app/services/rate_limiter.rb | 4 ---- spec/services/rate_limiter_spec.rb | 24 ------------------------ 2 files changed, 28 deletions(-) diff --git a/app/services/rate_limiter.rb b/app/services/rate_limiter.rb index b63c64608ee..f25751f9246 100644 --- a/app/services/rate_limiter.rb +++ b/app/services/rate_limiter.rb @@ -69,10 +69,6 @@ def maxed? attempts && attempts >= RateLimiter.max_attempts(rate_limit_type) end - def exceed_max? - !expired? && attempts && attempts > RateLimiter.max_attempts(rate_limit_type) - end - def increment! return if limited? value = nil diff --git a/spec/services/rate_limiter_spec.rb b/spec/services/rate_limiter_spec.rb index bbf54c213ab..53d15892d93 100644 --- a/spec/services/rate_limiter_spec.rb +++ b/spec/services/rate_limiter_spec.rb @@ -126,30 +126,6 @@ end end - describe '#exceed_max?' do - let(:max_attempts) { 3 } - - subject(:rate_limiter) { RateLimiter.new(target: '1', rate_limit_type: rate_limit_type) } - - it 'returns true when the amount of attempts is more than the max attempts' do - allow(subject).to receive(:attempts).and_return(4) - - expect(subject.exceed_max?).to eq(true) - end - - it 'returns false when the amount of attempts is equal to the max attempts' do - allow(subject).to receive(:attempts).and_return(3) - - expect(subject.exceed_max?).to eq(false) - end - - it 'returns false when the amount of attempts is less than the max attempts' do - allow(subject).to receive(:attempts).and_return(2) - - expect(subject.exceed_max?).to eq(false) - end - end - describe '#expires_at' do let(:attempted_at) { nil } let(:rate_limiter) { RateLimiter.new(target: '1', rate_limit_type: rate_limit_type) } From a9891c5375ec7e6d325ee36eb7dbff3a497e61ab Mon Sep 17 00:00:00 2001 From: Brittany Greaner Date: Thu, 2 May 2024 16:54:16 -0700 Subject: [PATCH 05/13] WIP --- app/controllers/idv/link_sent_controller.rb | 2 +- spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) 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/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb index 401ba8da668..d20fffe0081 100644 --- a/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb +++ b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb @@ -151,7 +151,7 @@ ) end - it 'shows capture complete on mobile and error page on desktop', js: true do + it 'does not rate limit on last attempt if successful', js: true do user = nil perform_in_browser(:desktop) do @@ -182,7 +182,9 @@ end perform_in_browser(:desktop) do - expect(page).to have_current_path(idv_session_errors_rate_limited_path, wait: 10) + 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) end end end From 043a3b325ff1d9a405917f79ca69a0b4f314f44e Mon Sep 17 00:00:00 2001 From: Dawei Wang Date: Thu, 2 May 2024 20:30:55 -0400 Subject: [PATCH 06/13] LG-13136: complete remove rate limit check in polling controller. Fix test. --- app/controllers/idv/capture_doc_status_controller.rb | 7 +------ spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb | 9 ++++++--- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/app/controllers/idv/capture_doc_status_controller.rb b/app/controllers/idv/capture_doc_status_controller.rb index 532df6600c3..77adc460fd3 100644 --- a/app/controllers/idv/capture_doc_status_controller.rb +++ b/app/controllers/idv/capture_doc_status_controller.rb @@ -43,12 +43,7 @@ def status def redirect_url return unless document_capture_session - - if rate_limiter.limited? - idv_session_errors_rate_limited_url - elsif user_has_establishing_in_person_enrollment? - idv_in_person_url - end + user_has_establishing_in_person_enrollment? ? idv_in_person_url : nil end def session_result diff --git a/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb index d20fffe0081..f8a5039f72e 100644 --- a/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb +++ b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb @@ -173,17 +173,20 @@ 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! attach_and_submit_images expect(page).to have_current_path(idv_hybrid_mobile_capture_complete_url) - expect(page).not_to have_content(t('doc_auth.headings.capture_complete').tr(' ', ' ')) + 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_not have_content(t('doc_auth.headings.text_message'), wait: 10) + # we may need wait up to 5 seconds, the polling interval for the page to refresh + sleep(6) expect(page).to have_current_path(idv_ssn_path) end end From 3bb3c349ff679552da95604d34041504b1ad1743 Mon Sep 17 00:00:00 2001 From: Brittany Greaner Date: Fri, 3 May 2024 08:49:41 -0700 Subject: [PATCH 07/13] Use wait instead of sleep --- spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb index f8a5039f72e..d0de583b5ae 100644 --- a/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb +++ b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb @@ -185,9 +185,7 @@ 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) - # we may need wait up to 5 seconds, the polling interval for the page to refresh - sleep(6) - expect(page).to have_current_path(idv_ssn_path) + expect(page).to have_current_path(idv_ssn_path, wait: 10) end end end From d147bee0d9df829865aaa3dcf28676fb5404398c Mon Sep 17 00:00:00 2001 From: Brittany Greaner Date: Tue, 7 May 2024 14:52:27 -0700 Subject: [PATCH 08/13] Rm unnecessary user variable from specs --- .../idv/hybrid_mobile/hybrid_mobile_spec.rb | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb index d0de583b5ae..f1e750b4b1d 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 @@ -152,10 +150,8 @@ end it 'does not rate limit on last attempt if successful', 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 @@ -192,10 +188,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 @@ -274,10 +268,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 From bfc3b3118f30632224579483d1302a39a8fd9152 Mon Sep 17 00:00:00 2001 From: Brittany Greaner Date: Tue, 7 May 2024 16:35:45 -0700 Subject: [PATCH 09/13] Add back in test to make sure rate limiting happens on last page hybrid --- .../idv/hybrid_mobile/hybrid_mobile_spec.rb | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb index f1e750b4b1d..cb5f1111c3d 100644 --- a/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb +++ b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb @@ -184,6 +184,39 @@ 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 |i| + attach_and_submit_images + click_on t('idv.failure.button.warning') + end + + # final failure + attach_and_submit_images + + expect(page).to have_current_path(idv_hybrid_mobile_capture_complete_url) + expect(page).not_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 have_current_path(idv_session_errors_rate_limited_path, wait: 10) + end + end end context 'barcode read error on mobile (redo document capture)' do From 33e333b531d50f850e4bdfc37ffd13539fcdde5b Mon Sep 17 00:00:00 2001 From: Brittany Greaner Date: Tue, 7 May 2024 16:37:39 -0700 Subject: [PATCH 10/13] Rm unused index on loop --- spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb index cb5f1111c3d..8fc154157c9 100644 --- a/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb +++ b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb @@ -200,7 +200,7 @@ perform_in_browser(:mobile) do visit @sms_link - (max_attempts - 1).times do |i| + (max_attempts - 1).times do attach_and_submit_images click_on t('idv.failure.button.warning') end From 208a5b855d2171061bf2e0737f91a743eb9f420b Mon Sep 17 00:00:00 2001 From: Dawei Wang Date: Thu, 9 May 2024 14:01:16 -0400 Subject: [PATCH 11/13] LG-13136: deal with last rate limited failure. --- app/controllers/idv/capture_doc_status_controller.rb | 12 +++++++++++- .../features/idv/hybrid_mobile/hybrid_mobile_spec.rb | 11 ++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/app/controllers/idv/capture_doc_status_controller.rb b/app/controllers/idv/capture_doc_status_controller.rb index 77adc460fd3..76328844eb5 100644 --- a/app/controllers/idv/capture_doc_status_controller.rb +++ b/app/controllers/idv/capture_doc_status_controller.rb @@ -28,6 +28,8 @@ def status :unauthorized elsif document_capture_session.cancelled_at :gone + elsif rate_limited_and_failed? + :too_many_requests elsif confirmed_barcode_attention_result? || user_has_establishing_in_person_enrollment? :ok elsif session_result.blank? || pending_barcode_attention_confirmation? || @@ -43,7 +45,11 @@ def status def redirect_url return unless document_capture_session - user_has_establishing_in_person_enrollment? ? idv_in_person_url : nil + if rate_limited_and_failed? + idv_session_errors_rate_limited_url + elsif user_has_establishing_in_person_enrollment? + idv_in_person_url + end end def session_result @@ -96,5 +102,9 @@ def redo_document_capture_pending? document_capture_session.requested_at > session_result.captured_at end + + def rate_limited_and_failed? + rate_limiter.limited? && !session_result&.success? && session_result&.captured_at.present? + end end end diff --git a/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb index 8fc154157c9..723983ac6fa 100644 --- a/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb +++ b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb @@ -140,6 +140,7 @@ before do allow(IdentityConfig.store).to receive(:doc_auth_max_attempts).and_return(max_attempts) + # need fix, network error should not be counted for rate limiting DocAuth::Mock::DocAuthMockClient.mock_response!( method: :post_front_image, response: DocAuth::Response.new( @@ -205,7 +206,15 @@ click_on t('idv.failure.button.warning') end - # final failure + # final failure, not a network error, which currently will skip saving session result + DocAuth::Mock::DocAuthMockClient.reset! + DocAuth::Mock::DocAuthMockClient.mock_response!( + method: :post_front_image, + response: DocAuth::Response.new( + success: false, + errors: { general_error: I18n.t('doc_auth.errors.general.multiple_front_id_failures') }, + ), + ) attach_and_submit_images expect(page).to have_current_path(idv_hybrid_mobile_capture_complete_url) From 53dc047156f088b7649d43862e4a3cd9f82db178 Mon Sep 17 00:00:00 2001 From: Dawei Wang Date: Fri, 10 May 2024 09:06:32 -0400 Subject: [PATCH 12/13] LG-13136: dealing with multiple issues. --- .../idv/capture_doc_status_controller.rb | 8 ++-- app/forms/idv/api_image_upload_form.rb | 42 ++++++++++--------- .../doc_auth/mock/doc_auth_mock_client.rb | 12 ++++++ .../idv/hybrid_mobile/hybrid_mobile_spec.rb | 21 +++++----- spec/support/capybara.rb | 1 - 5 files changed, 50 insertions(+), 34 deletions(-) diff --git a/app/controllers/idv/capture_doc_status_controller.rb b/app/controllers/idv/capture_doc_status_controller.rb index 76328844eb5..ddd396dabbe 100644 --- a/app/controllers/idv/capture_doc_status_controller.rb +++ b/app/controllers/idv/capture_doc_status_controller.rb @@ -53,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 @@ -97,6 +98,7 @@ 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 @@ -104,7 +106,7 @@ def redo_document_capture_pending? end def rate_limited_and_failed? - rate_limiter.limited? && !session_result&.success? && session_result&.captured_at.present? + rate_limiter.limited? && !session_result&.success? && !redo_document_capture_pending? end end end diff --git a/app/forms/idv/api_image_upload_form.rb b/app/forms/idv/api_image_upload_form.rb index 5445711e84b..c32f3c59ef9 100644 --- a/app/forms/idv/api_image_upload_form.rb +++ b/app/forms/idv/api_image_upload_form.rb @@ -79,8 +79,12 @@ 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 +485,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..b51b1635edc 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,16 @@ def self.mock_response!(method:, response:) @response_mocks[method.to_sym] = response end + def self.response_delay(delay) + puts "Setting delay to #{delay} in #{object_id}" + @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 +61,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/features/idv/hybrid_mobile/hybrid_mobile_spec.rb b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb index 723983ac6fa..a74be0741d5 100644 --- a/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb +++ b/spec/features/idv/hybrid_mobile/hybrid_mobile_spec.rb @@ -140,16 +140,22 @@ before do allow(IdentityConfig.store).to receive(:doc_auth_max_attempts).and_return(max_attempts) - # need fix, network error should not be counted for rate limiting + 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 + 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 sign_in_and_2fa_user @@ -172,6 +178,7 @@ # 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) @@ -206,15 +213,7 @@ click_on t('idv.failure.button.warning') end - # final failure, not a network error, which currently will skip saving session result - DocAuth::Mock::DocAuthMockClient.reset! - DocAuth::Mock::DocAuthMockClient.mock_response!( - method: :post_front_image, - response: DocAuth::Response.new( - success: false, - errors: { general_error: I18n.t('doc_auth.errors.general.multiple_front_id_failures') }, - ), - ) + DocAuth::Mock::DocAuthMockClient.response_delay(8) attach_and_submit_images expect(page).to have_current_path(idv_hybrid_mobile_capture_complete_url) diff --git a/spec/support/capybara.rb b/spec/support/capybara.rb index 20e81f41152..ec692c92ef7 100644 --- a/spec/support/capybara.rb +++ b/spec/support/capybara.rb @@ -5,7 +5,6 @@ # temporary fix for local development feature tests # remove when we get a new working version of Chromedriver - Capybara.register_driver :headless_chrome do |app| options = Selenium::WebDriver::Chrome::Options.new options.add_argument("--headless#{ENV['CI'] ? '' : '=new'}") if !ENV['SHOW_BROWSER'] From 9a227356d5880354f7967d755c4a6f07e3df47e3 Mon Sep 17 00:00:00 2001 From: Dawei Wang Date: Fri, 10 May 2024 10:28:31 -0400 Subject: [PATCH 13/13] LG-13136: linter. --- app/forms/idv/api_image_upload_form.rb | 1 - app/services/doc_auth/mock/doc_auth_mock_client.rb | 1 - spec/support/capybara.rb | 1 + 3 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/forms/idv/api_image_upload_form.rb b/app/forms/idv/api_image_upload_form.rb index c32f3c59ef9..d11870d3305 100644 --- a/app/forms/idv/api_image_upload_form.rb +++ b/app/forms/idv/api_image_upload_form.rb @@ -79,7 +79,6 @@ def validate_form end def post_images_to_client - # user submit a request, set the requested_at timestamp document_capture_session.requested_at = Time.zone.now document_capture_session.save! 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 b51b1635edc..eb747c96589 100644 --- a/app/services/doc_auth/mock/doc_auth_mock_client.rb +++ b/app/services/doc_auth/mock/doc_auth_mock_client.rb @@ -23,7 +23,6 @@ def self.mock_response!(method:, response:) end def self.response_delay(delay) - puts "Setting delay to #{delay} in #{object_id}" @delay = delay end diff --git a/spec/support/capybara.rb b/spec/support/capybara.rb index ec692c92ef7..20e81f41152 100644 --- a/spec/support/capybara.rb +++ b/spec/support/capybara.rb @@ -5,6 +5,7 @@ # temporary fix for local development feature tests # remove when we get a new working version of Chromedriver + Capybara.register_driver :headless_chrome do |app| options = Selenium::WebDriver::Chrome::Options.new options.add_argument("--headless#{ENV['CI'] ? '' : '=new'}") if !ENV['SHOW_BROWSER']