-
Notifications
You must be signed in to change notification settings - Fork 166
LG-9813 Prevent user from starting IdV if currently rate-limited #8477
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
Merged
Merged
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
21b7940
Fix #confirm_no_pending_gpo_profile specs
soniaconnolly 16ee4d8
Redirect in IdvStepConcern if user is idv_doc_auth throttled
soniaconnolly d9a07bb
Add throttle checks to idv_controller
soniaconnolly a8b45f3
refactor
soniaconnolly 094a255
refactor check_throttles_and_redirect
e1ba74b
lint
soniaconnolly 2845a36
Move check_throttled_and_redirect to IdvSession concern and use in Id…
soniaconnolly 4e1752d
rename throttled_type method arg to throttle_type
soniaconnolly 317ae83
Add extra analytics arg for :proof_address
soniaconnolly 6740f48
Let PhoneController check its own rate limiter
soniaconnolly 4a30faa
Extract RateLimitConcern
soniaconnolly 5d87f47
Move specs from IdvStepConcernSpec to RateLimitConcernSpec
soniaconnolly ec4ebec
Check if throttle and controller match
soniaconnolly a2a82cb
Check if throttle and controller match at the beginning
soniaconnolly 2ecc6c3
Check desktop DocumentCapture rate limit on show
soniaconnolly a4599a2
lint
soniaconnolly f5c6111
Check for matching controllers only on update
soniaconnolly b0f0ef4
Remove DocumentCaptureController#show throttle check
soniaconnolly 6ceec4f
Move IdvController rate limit check to a before_action
soniaconnolly a35215d
Rename method in spec too
soniaconnolly File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| module RateLimitConcern | ||
| extend ActiveSupport::Concern | ||
|
|
||
| def confirm_not_rate_limited | ||
| 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' | ||
|
|
||
| if rate_limit_redirect!(throttle_type) | ||
| rate_limited = true | ||
| break | ||
| end | ||
| end | ||
| rate_limited | ||
| end | ||
|
|
||
| def rate_limit_redirect!(throttle_type) | ||
| if idv_attempter_rate_limited?(throttle_type) | ||
| track_rate_limited_event(throttle_type) | ||
| rate_limited_redirect(throttle_type) | ||
| return true | ||
| end | ||
| end | ||
|
|
||
| def track_rate_limited_event(throttle_type) | ||
| analytics_args = { throttle_type: throttle_type } | ||
| analytics_args[:step_name] = :phone if throttle_type == :proof_address | ||
|
|
||
| irs_attempts_api_tracker.idv_verification_rate_limited(throttle_context: 'single-session') | ||
| analytics.throttler_rate_limit_triggered(**analytics_args) | ||
| end | ||
|
|
||
| def rate_limited_redirect(throttle_type) | ||
| case throttle_type | ||
| when :idv_resolution | ||
| redirect_to idv_session_errors_failure_url | ||
| when :idv_doc_auth | ||
| redirect_to idv_session_errors_throttled_url | ||
| when :proof_address | ||
| redirect_to idv_phone_errors_failure_url if self.class != Idv::PhoneController | ||
| end | ||
| end | ||
|
|
||
| def throttle_and_controller_match(throttle_type) | ||
| case throttle_type | ||
| when :idv_resolution | ||
| self.instance_of?(Idv::VerifyInfoController) || | ||
| self.instance_of?(Idv::InPerson::VerifyInfoController) | ||
| when :idv_doc_auth | ||
| self.instance_of?(Idv::DocumentCaptureController) || | ||
| self.instance_of?(Idv::HybridMobile::DocumentCaptureController) | ||
| when :proof_address | ||
| self.instance_of?(Idv::PhoneController) | ||
| end | ||
| end | ||
|
|
||
| def idv_attempter_rate_limited?(throttle_type) | ||
| Throttle.new( | ||
| user: effective_user, | ||
| throttle_type: throttle_type, | ||
| ).throttled? | ||
| end | ||
| end | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| require 'rails_helper' | ||
|
|
||
| describe 'RateLimitConcern' do | ||
| let(:user) { create(:user, :fully_registered, email: 'old_email@example.com') } | ||
|
|
||
| module Idv | ||
| class StepController < ApplicationController | ||
| include RateLimitConcern | ||
|
|
||
| def show | ||
| render plain: 'Hello' | ||
| end | ||
|
|
||
| def update | ||
| render plain: 'Bye' | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe '#confirm_not_rate_limited' do | ||
| controller Idv::StepController do | ||
| before_action :confirm_not_rate_limited | ||
| end | ||
|
|
||
| before(:each) do | ||
| sign_in(user) | ||
| allow(subject).to receive(:current_user).and_return(user) | ||
| routes.draw do | ||
| get 'show' => 'idv/step#show' | ||
| put 'update' => 'idv/step#update' | ||
| end | ||
| end | ||
|
|
||
| context 'user is not throttled' do | ||
| let(:user) { create(:user, :fully_registered) } | ||
|
|
||
| it 'does not redirect' do | ||
| get :show | ||
|
|
||
| expect(response.body).to eq 'Hello' | ||
| expect(response.status).to eq 200 | ||
| end | ||
| end | ||
|
|
||
| context 'with idv_doc_auth throttle (DocumentCapture)' do | ||
| it 'redirects to idv_doc_auth throttled error page' do | ||
| throttle = Throttle.new(user: user, throttle_type: :idv_doc_auth) | ||
| throttle.increment_to_throttled! | ||
|
|
||
| get :show | ||
|
|
||
| expect(response).to redirect_to idv_session_errors_throttled_url | ||
| end | ||
| end | ||
|
|
||
| context 'with idv_resolution throttle (VerifyInfo)' do | ||
| it 'redirects to idv_resolution throttled error page' do | ||
| throttle = Throttle.new(user: user, throttle_type: :idv_resolution) | ||
| throttle.increment_to_throttled! | ||
|
|
||
| get :show | ||
|
|
||
| expect(response).to redirect_to idv_session_errors_failure_url | ||
| end | ||
| end | ||
|
|
||
| context 'with proof_address throttle (PhoneStep)' do | ||
| before do | ||
| throttle = Throttle.new(user: user, throttle_type: :proof_address) | ||
| throttle.increment_to_throttled! | ||
| end | ||
|
|
||
| it 'redirects to proof_address throttled error page' do | ||
| get :show | ||
|
|
||
| expect(response).to redirect_to idv_phone_errors_failure_url | ||
| end | ||
|
|
||
| context 'controller and throttle match' do | ||
| before do | ||
| allow(subject).to receive(:throttle_and_controller_match). | ||
| and_return(true) | ||
| end | ||
|
|
||
| it 'redirects on show' do | ||
| get :show | ||
|
|
||
| expect(response).to redirect_to idv_phone_errors_failure_url | ||
| end | ||
|
|
||
| it 'does not redirect on update' do | ||
| put :update | ||
|
|
||
| expect(response.body).to eq 'Bye' | ||
| expect(response.status).to eq 200 | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Instead of checking
action_namehere, could we use this method like this in controllers to be more explicit: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 want it to check on update everywhere except in the controller that uses that particular rate limit.
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 added LG-9970 to address the issue with update and doomed final attempts when rate_limited in a followup PR.