LG-9813 Prevent user from starting IdV if currently rate-limited#8477
LG-9813 Prevent user from starting IdV if currently rate-limited#8477soniaconnolly merged 20 commits intomainfrom
Conversation
This is the DocumentCapture throttle
…vStepConcern Both IdvController and IdvStepConcern need to check if the user is throttled and redirect as needed. changelog: User-facing Improvements, Identity Verification, Give a user immediate feedback if they return to IdV while they are still rate-limited
PhoneController#update checks for success before rate limiting the user. Do not redirect to error page from before_action for the PhoneController for its rate limiter. Co-authored-by John Maxwell <john.maxwell@gsa.gov>
to avoid duplicate logging
| call('document_capture', :view, true) | ||
|
|
||
| render :show, locals: extra_view_variables | ||
| if !rate_limit_redirect!(:idv_doc_auth) |
There was a problem hiding this comment.
Wondering if it'd be clearer / more conventional to call a before_action to do the redirect logic, so it doesn't need to be considered as part of the action itself (i.e. the action would only be reached if nothing else redirected).
There was a problem hiding this comment.
It's not the pattern we've been using for these post-FSM controllers, but it might be good to shift in that direction. The analytics wouldn't be logged in that case, and it would have to be only :show. The other thing I was thinking was to make the before_action in IdvStepConcern cover the show case, and just delegate to the controller for update to check if the last attempt succeeded.
That way controllers don't have to do their own checking on show
Now handled by IdvStepConcern :confirm_not_rate_limited before action
| def check_rate_limited_and_redirect | ||
| rate_limited = false | ||
| %i[idv_resolution idv_doc_auth proof_address].each do |throttle_type| | ||
| next if throttle_and_controller_match(throttle_type) && action_name == 'update' |
There was a problem hiding this comment.
Instead of checking action_name here, could we use this method like this in controllers to be more explicit:
before_action :check_rate_limited_and_redirect, only: :showThere was a problem hiding this comment.
We want it to check on update everywhere except in the controller that uses that particular rate limit.
There was a problem hiding this comment.
We added LG-9970 to address the issue with update and doomed final attempts when rate_limited in a followup PR.
And rename the method to confirm_not_rate_limited Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>
3 participants
🎫 Ticket
LG-9813
🛠 Summary of changes
Add new RateLimitConcern.
Check in IdvController (
/verify) and IdvStepConcern (steps after Upload/HybridHandoff) if a user is rate-limited, and show them the appropriate error screen. Rate limits can be from DocumentCapture, VerifyInfo, or PhoneStep.Do not redirect if the current controller is the one that updates that particular rate limit, because the update action may check for success before rate limiting. PhoneController currently checks for success and the other two don't (yet).
Notes:
Future work:
📜 Testing Plan
Create an account (recommend using text MFA, remember this browser)
Start IdV. In the DocumentCapture step, upload a yaml to trigger an error, available for download here.
Click Try again online and keep uploading error yamls until rate limited
Try to start Idv again. Navigate to
/verifyand/verify/document_captureExpect to see rate limit error screen.
Log out and back in.
Try to start Idv again. Navigate to
/verifyand/verify/document_captureExpect to see rate limit error screen.
Same as above, with SSN 123-45-6666 to fail VerifyInfo
Same as above, with phone # 7035555555 to fail Phone step