Skip to content
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-11189 Count successful doc auth proofing towards the rate limit #9370

Merged
merged 2 commits into from
Oct 12, 2023

Conversation

jmhooper
Copy link
Member

Prior to this commit we reset the doc auth rate limiter on success. This was done to prevent users from being rate limited after successfully completing a step. The logic that caused that issue was addressed in #9343.

This commit starts counting successful attempts to towards the rate limit. This protects our vendors from abuse and makes it easier for us to make this step re-entrant to support the back button.

@jmhooper
Copy link
Member Author

jmhooper commented Oct 12, 2023

Look here and here for the test cases that cover the rate limiting behavior for doc auth

Prior to this commit we reset the doc auth rate limiter on success. This was done to prevent users from being rate limited after successfully completing a step. The logic that caused that issue was addressed in #9343.

This commit starts counting successful attempts to towards the rate limit. This protects our vendors from abuse and makes it easier for us to make this step re-entrant to support the back button.

changelog: Improvements, Rate Limiting, The idv doc auth rate limiter was modified to rate limit on successful doc auth attempts as well as on failed doc auth proofing attempts
@jmhooper jmhooper force-pushed the jmhooper-dont-reset-doc-auth-rate-limit-on-success branch from 76aa8fe to 63248fb Compare October 12, 2023 16:12
@jmhooper jmhooper marked this pull request as ready for review October 12, 2023 16:30
@jmhooper jmhooper requested a review from a team October 12, 2023 16:30
@@ -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
Contributor

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
Contributor

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
Member 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.

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
Member Author

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.

Copy link
Contributor

@soniaconnolly soniaconnolly left a comment

Choose a reason for hiding this comment

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

LGTM, amazingly succinct! Recommend adding a spec line as already commented.

@jmhooper jmhooper merged commit b2705e0 into main Oct 12, 2023
2 checks passed
@jmhooper jmhooper deleted the jmhooper-dont-reset-doc-auth-rate-limit-on-success branch October 12, 2023 21:06
night-jellyfish added a commit that referenced this pull request May 1, 2024
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](#10538 (comment)), and as I found [a past
discussion for](#9370 (comment)), this code should never be reached if rate limited.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants