Skip to content

LG-11189 Count successful doc auth proofing towards the rate limit #9370

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.

Sign up for GitHub

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

Merged
jmhooper merged 2 commits into from
Oct 12, 2023
Merged

LG-11189 Count successful doc auth proofing towards the rate limit #9370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
63248fb
LG-11189 Count successful doc auth proofing towards the rate limit
jmhooper Oct 12, 2023
22d34d0
a little extra testing
jmhooper Oct 12, 2023
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
2 changes: 1 addition & 1 deletion app/controllers/idv/document_capture_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ class DocumentCaptureController < ApplicationController
include IdvStepConcern
include StepIndicatorConcern

before_action :confirm_not_rate_limited
before_action :confirm_not_rate_limited, except: [:update]
Copy link
Copy Markdown
Contributor

@soniaconnolly soniaconnolly Oct 12, 2023

Choose a reason for hiding this comment

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

Are we still rate-limiting update somewhere? Otherwise, is it possible for someone to post unlimited updates?

Copy link
Copy Markdown
Contributor

@soniaconnolly soniaconnolly Oct 12, 2023

Choose a reason for hiding this comment

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

Looks like the specs you linked cover the update case. Do we want to add a line that checks that the rate limiter is limited after that final successful attempt here?

Copy link
Copy Markdown
Contributor Author

@jmhooper jmhooper Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So, the update case for doc auth is interesting because it does not actually submit any data. It checks for the doc auth result and advances if it exist. It does not result in a vendor call. That is done by Idv::ImageUploadsController within Idv::ApiImageUploadForm. We do not need to consider the rate limiter in the update action for Idv::DocumentCaptureController.

Copy link
Copy Markdown
Contributor Author

@jmhooper jmhooper Oct 12, 2023

Choose a reason for hiding this comment

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

The above said, I think a line in the feature spec is reasonable and not too expensive. I'll add one.

before_action :confirm_hybrid_handoff_complete
before_action :confirm_document_capture_needed
before_action :override_csp_to_allow_acuant
Expand Down
1 change: 0 additions & 1 deletion app/forms/idv/api_image_upload_form.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ def validate_pii_from_doc(client_response)

if client_response.success? && response.success?
store_pii(client_response)
rate_limiter.reset!
end

response
Expand Down
4 changes: 4 additions & 0 deletions spec/features/idv/doc_auth/document_capture_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@
it 'proceeds to the next page with valid info' do
attach_and_submit_images
expect(page).to have_current_path(idv_ssn_url)

visit idv_document_capture_path

expect(page).to have_current_path(idv_session_errors_rate_limited_path)
end
end
end
Expand Down