Skip to content

Use Redis for one-time passcode sending rate limits #7725

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
mitchellhenke merged 6 commits into from
Feb 3, 2023
Merged

Use Redis for one-time passcode sending rate limits #7725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cda6f9a
Use Redis for one-time passcode sending rate limits
Jan 30, 2023
79b8eec
Update app/services/otp_rate_limiter.rb
Jan 30, 2023
92d4422
Update app/services/otp_rate_limiter.rb
Jan 30, 2023
38453c2
fix specs
Jan 30, 2023
846346f
remove unused methods
Jan 31, 2023
7ea58d4
remove OtpRequestsTracker
Jan 31, 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
21 changes: 0 additions & 21 deletions app/models/otp_requests_tracker.rb
View file
Open in desktop

This file was deleted.

32 changes: 7 additions & 25 deletions app/services/otp_rate_limiter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,14 @@ def exceeded_otp_send_limit?
end

def max_requests_reached?
return throttle.throttled? if IdentityConfig.store.redis_throttle_otp_rate_limiter_read_enabled

entry_for_current_phone.otp_send_count > otp_maxretry_times
throttle.throttled?
end

def rate_limit_period_expired?
return throttle.expired? if IdentityConfig.store.redis_throttle_otp_rate_limiter_read_enabled
otp_last_sent_at.present? && (otp_last_sent_at + otp_findtime) < Time.zone.now
throttle.expired?
end

def reset_count_and_otp_last_sent_at
entry_for_current_phone.update(otp_last_sent_at: Time.zone.now, otp_send_count: 0)

throttle.reset!
end

Expand All @@ -36,30 +31,21 @@ def lock_out_user
end

def increment
# DO NOT MEMOIZE
@entry = OtpRequestsTracker.atomic_increment(entry_for_current_phone.id)
throttle.increment!
nil
end

def otp_last_sent_at
if IdentityConfig.store.redis_throttle_otp_rate_limiter_read_enabled
throttle.attempted_at
else
entry_for_current_phone.otp_last_sent_at
end
throttle.attempted_at
end

def throttle
@throttle ||= Throttle.new(throttle_type: :phone_otp, target: throttle_key)
end

private

attr_reader :phone, :user, :phone_confirmed

# rubocop:disable Naming/MemoizedInstanceVariableName
def entry_for_current_phone
@entry ||= OtpRequestsTracker.find_or_create_with_phone_and_confirmed(phone, phone_confirmed)
Copy link
Copy Markdown
Contributor

@aduth aduth Jan 31, 2023

Choose a reason for hiding this comment

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

Same note about this method not being used anywhere and could be removed from OtpRequestsTracker.

Copy link
Copy Markdown
Contributor

@aduth aduth Jan 31, 2023

Choose a reason for hiding this comment

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

Just to clarify, are we planning to drop the otp_requests_trackers table altogether at some point?

Copy link
Copy Markdown
Contributor Author

@mitchellhenke mitchellhenke Jan 31, 2023

Choose a reason for hiding this comment

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

Yeah, plan is to drop everything related to OtpRequestsTracker in this, and drop the table if we're content with the functionality.

end
# rubocop:enable Naming/MemoizedInstanceVariableName

def otp_findtime
IdentityConfig.store.otp_delivery_blocklist_findtime.minutes
end
Expand All @@ -72,10 +58,6 @@ def phone_fingerprint
@phone_fingerprint ||= Pii::Fingerprinter.fingerprint(PhoneFormatter.format(phone))
end

def throttle
@throttle ||= Throttle.new(throttle_type: :phone_otp, target: throttle_key)
end

def throttle_key
"#{phone_fingerprint}:#{phone_confirmed}"
end
Expand Down
3 changes: 0 additions & 3 deletions config/application.yml.default
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,6 @@ rails_mailer_previews_enabled: false
reauthn_window: 120
recovery_code_length: 4
redis_irs_attempt_api_url: redis://localhost:6379/2
redis_throttle_otp_rate_limiter_read_enabled: false
redis_throttle_url: redis://localhost:6379/1
redis_url: redis://localhost:6379/0
redis_pool_size: 10
Expand Down Expand Up @@ -378,7 +377,6 @@ development:
rails_mailer_previews_enabled: true
rack_timeout_service_timeout_seconds: 9_999_999_999
recurring_jobs_disabled_names: "[]"
redis_throttle_otp_rate_limiter_read_enabled: true
s3_report_bucket_prefix: ''
s3_report_public_bucket_prefix: ''
saml_endpoint_configs: '[{"suffix":"2021","secret_key_passphrase":"trust-but-verify"},{"suffix":"2022","secret_key_passphrase":"trust-but-verify"}]'
Expand Down Expand Up @@ -539,7 +537,6 @@ test:
piv_cac_verify_token_secret: 3ac13bfa23e22adae321194c083e783faf89469f6f85dcc0802b27475c94b5c3891b5657bd87d0c1ad65de459166440512f2311018db90d57b15d8ab6660748f
poll_rate_for_verify_in_seconds: 0
recurring_jobs_disabled_names: '["disabled job"]'
redis_throttle_otp_rate_limiter_read_enabled: true
reg_confirmed_email_max_attempts: 3
reg_unconfirmed_email_max_attempts: 4
reg_unconfirmed_email_window_in_minutes: 70
Expand Down
1 change: 0 additions & 1 deletion lib/identity_config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,6 @@ def self.build_store(config_map)
config.add(:recurring_jobs_disabled_names, type: :json)
config.add(:redis_irs_attempt_api_url)
config.add(:redis_throttle_url)
config.add(:redis_throttle_otp_rate_limiter_read_enabled, type: :boolean)
config.add(:redis_url)
config.add(:redis_pool_size, type: :integer)
config.add(:redis_session_pool_size, type: :integer)
Expand Down
63 changes: 0 additions & 63 deletions spec/models/otp_requests_tracker_spec.rb
View file
Open in desktop

This file was deleted.

11 changes: 1 addition & 10 deletions spec/services/otp_rate_limiter_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,6 @@
OtpRateLimiter.new(phone: phone, user: current_user, phone_confirmed: true)
end
let(:phone_fingerprint) { Pii::Fingerprinter.fingerprint(phone) }
let(:rate_limited_phone) do
OtpRequestsTracker.find_by(phone_fingerprint: phone_fingerprint, phone_confirmed: false)
end

describe '#exceeded_otp_send_limit?' do
it 'is false by default' do
Expand Down Expand Up @@ -65,17 +62,11 @@
otp_rate_limiter.increment

expect { otp_rate_limiter.increment }.
to change { rate_limited_phone.reload.otp_send_count }.from(1).to(2)
to change { otp_rate_limiter.throttle.attempts }.from(1).to(2)
end
end

describe '#lock_out_user' do
before do
otp_rate_limiter.increment
rate_limited_phone.otp_last_sent_at = 5.minutes.ago
rate_limited_phone.otp_send_count = 0
end

it 'sets the second_factor_locked_at' do
expect(current_user.second_factor_locked_at).to be_nil

Expand Down