-
Notifications
You must be signed in to change notification settings - Fork 115
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-13136: Fix rate limiting logic when user is on last try #10538
Changes from all commits
6d97f55
67569af
bc1c63f
c3ee24a
a9891c5
043a3b3
3bb3c34
cdc8919
d147bee
bfc3b31
33e333b
208a5b8
53dc047
9a22735
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -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! | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. persist it immediately in case we get racing condition in poller There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This timestamp is used at another place
In this case, guess there is no racing concern, so no need to persist immediately. Also, should we reset it after doc auth since it use used at this place also, maybe not. |
||||
|
||||
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 | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need explicitly save failed result and result timestamp when finger print check disabled. |
||||
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, | ||||
) | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding delays in mock client to facilitate tests that reflect reality, useful for hybrid flow poller testing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good idea. 👍🏻 |
||
@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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. disable check so we can resubmit same images, easier to test. |
||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add delays longer than a polling cycle which is 5 sec. |
||
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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may take a detailed look, is it necessary? what's our assumption when this method is invoked? what are the edge cases?