Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 27 additions & 6 deletions app/services/otp_rate_limiter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ def initialize(phone:, user:, phone_confirmed:)
end

def exceeded_otp_send_limit?
return false if entry_for_current_phone.blank?

if rate_limit_period_expired?
reset_count_and_otp_last_sent_at
return false
Expand All @@ -17,15 +15,20 @@ 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
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
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

def lock_out_user
Expand All @@ -35,6 +38,16 @@ def lock_out_user
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
end

private
Expand All @@ -47,15 +60,23 @@ def entry_for_current_phone
end
# rubocop:enable Naming/MemoizedInstanceVariableName

def otp_last_sent_at
entry_for_current_phone.otp_last_sent_at
end

def otp_findtime
IdentityConfig.store.otp_delivery_blocklist_findtime.minutes
end

def otp_maxretry_times
IdentityConfig.store.otp_delivery_blocklist_maxretry
end

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
end
124 changes: 70 additions & 54 deletions app/services/throttle.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,63 +4,12 @@
class Throttle
attr_reader :throttle_type

THROTTLE_CONFIG = {
Copy link
Copy Markdown
Contributor

@aduth aduth Jan 25, 2023

Choose a reason for hiding this comment

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

Curious: What's the reason/context for refactoring this constant?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I was having trouble with the tests and having multiple avenues to stub since it can be accessed through the constant and the Throttle.attempt_window_in_minutes/1 method. It was a change I'd been considering for a bit and I swear there was a good reason to bring it into this PR in particular, but I'm struggling to remember the specifics 😓

Definitely open to improvements/reversion.

idv_doc_auth: {
max_attempts: IdentityConfig.store.doc_auth_max_attempts,
attempt_window: IdentityConfig.store.doc_auth_attempt_window_in_minutes,
},
reg_unconfirmed_email: {
max_attempts: IdentityConfig.store.reg_unconfirmed_email_max_attempts,
attempt_window: IdentityConfig.store.reg_unconfirmed_email_window_in_minutes,
},
reg_confirmed_email: {
max_attempts: IdentityConfig.store.reg_confirmed_email_max_attempts,
attempt_window: IdentityConfig.store.reg_confirmed_email_window_in_minutes,
},
reset_password_email: {
max_attempts: IdentityConfig.store.reset_password_email_max_attempts,
attempt_window: IdentityConfig.store.reset_password_email_window_in_minutes,
},
idv_resolution: {
max_attempts: IdentityConfig.store.idv_max_attempts,
attempt_window: IdentityConfig.store.idv_attempt_window_in_hours * 60,
},
idv_send_link: {
max_attempts: IdentityConfig.store.idv_send_link_max_attempts,
attempt_window: IdentityConfig.store.idv_send_link_attempt_window_in_minutes,
},
verify_personal_key: {
max_attempts: IdentityConfig.store.verify_personal_key_max_attempts,
attempt_window: IdentityConfig.store.verify_personal_key_attempt_window_in_minutes,
},
verify_gpo_key: {
max_attempts: IdentityConfig.store.verify_gpo_key_max_attempts,
attempt_window: IdentityConfig.store.verify_gpo_key_attempt_window_in_minutes,
},
proof_ssn: {
max_attempts: IdentityConfig.store.proof_ssn_max_attempts,
attempt_window: IdentityConfig.store.proof_ssn_max_attempt_window_in_minutes,
},
proof_address: {
max_attempts: IdentityConfig.store.proof_address_max_attempts,
attempt_window: IdentityConfig.store.proof_address_max_attempt_window_in_minutes,
},
phone_confirmation: {
max_attempts: IdentityConfig.store.phone_confirmation_max_attempts,
attempt_window: IdentityConfig.store.phone_confirmation_max_attempt_window_in_minutes,
},
inherited_proofing: {
max_attempts: IdentityConfig.store.inherited_proofing_max_attempts,
attempt_window: IdentityConfig.store.inherited_proofing_max_attempt_window_in_minutes,
},
}.with_indifferent_access.freeze

def initialize(throttle_type:, user: nil, target: nil)
@throttle_type = throttle_type
@user = user
@target = target

unless THROTTLE_CONFIG.key?(throttle_type)
unless Throttle.throttle_config.key?(throttle_type)
raise ArgumentError,
'throttle_type is not valid'
end
Expand Down Expand Up @@ -209,10 +158,77 @@ def key
end

def self.attempt_window_in_minutes(throttle_type)
THROTTLE_CONFIG.dig(throttle_type, :attempt_window)
throttle_config.dig(throttle_type, :attempt_window)
end

def self.max_attempts(throttle_type)
THROTTLE_CONFIG.dig(throttle_type, :max_attempts)
throttle_config.dig(throttle_type, :max_attempts)
end

def self.throttle_config
if Rails.env.production?
CACHED_THROTTLE_CONFIG
else
load_throttle_config
end
end

def self.load_throttle_config
{
idv_doc_auth: {
max_attempts: IdentityConfig.store.doc_auth_max_attempts,
attempt_window: IdentityConfig.store.doc_auth_attempt_window_in_minutes,
},
reg_unconfirmed_email: {
max_attempts: IdentityConfig.store.reg_unconfirmed_email_max_attempts,
attempt_window: IdentityConfig.store.reg_unconfirmed_email_window_in_minutes,
},
reg_confirmed_email: {
max_attempts: IdentityConfig.store.reg_confirmed_email_max_attempts,
attempt_window: IdentityConfig.store.reg_confirmed_email_window_in_minutes,
},
reset_password_email: {
max_attempts: IdentityConfig.store.reset_password_email_max_attempts,
attempt_window: IdentityConfig.store.reset_password_email_window_in_minutes,
},
idv_resolution: {
max_attempts: IdentityConfig.store.idv_max_attempts,
attempt_window: IdentityConfig.store.idv_attempt_window_in_hours * 60,
},
idv_send_link: {
max_attempts: IdentityConfig.store.idv_send_link_max_attempts,
attempt_window: IdentityConfig.store.idv_send_link_attempt_window_in_minutes,
},
verify_personal_key: {
max_attempts: IdentityConfig.store.verify_personal_key_max_attempts,
attempt_window: IdentityConfig.store.verify_personal_key_attempt_window_in_minutes,
},
verify_gpo_key: {
max_attempts: IdentityConfig.store.verify_gpo_key_max_attempts,
attempt_window: IdentityConfig.store.verify_gpo_key_attempt_window_in_minutes,
},
proof_ssn: {
max_attempts: IdentityConfig.store.proof_ssn_max_attempts,
attempt_window: IdentityConfig.store.proof_ssn_max_attempt_window_in_minutes,
},
proof_address: {
max_attempts: IdentityConfig.store.proof_address_max_attempts,
attempt_window: IdentityConfig.store.proof_address_max_attempt_window_in_minutes,
},
phone_confirmation: {
max_attempts: IdentityConfig.store.phone_confirmation_max_attempts,
attempt_window: IdentityConfig.store.phone_confirmation_max_attempt_window_in_minutes,
},
inherited_proofing: {
max_attempts: IdentityConfig.store.inherited_proofing_max_attempts,
attempt_window: IdentityConfig.store.inherited_proofing_max_attempt_window_in_minutes,
},
phone_otp: {
max_attempts: IdentityConfig.store.otp_delivery_blocklist_maxretry + 1,
attempt_window: IdentityConfig.store.otp_delivery_blocklist_findtime,
},
}.with_indifferent_access
end

CACHED_THROTTLE_CONFIG = self.load_throttle_config.with_indifferent_access.freeze
end
3 changes: 3 additions & 0 deletions config/application.yml.default
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ 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 @@ -375,6 +376,7 @@ 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 @@ -535,6 +537,7 @@ 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: 1 addition & 0 deletions lib/identity_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ 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
2 changes: 1 addition & 1 deletion spec/features/event_disavowal_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@
end

scenario 'disavowing a new device sign in' do
allow(IdentityConfig.store).to receive(:otp_delivery_blocklist_maxretry).and_return(3)
signin(user.email, user.password)
Capybara.reset_session!
visit root_path
OtpRequestsTracker.destroy_all # Prevent OTP rate limit from preventing sign in
signin(user.email, user.password)

disavow_last_action_and_reset_password
Expand Down
13 changes: 6 additions & 7 deletions spec/features/idv/phone_otp_rate_limiting_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,14 @@
let(:user) { user_with_2fa }

describe 'otp sends' do
let(:max_attempts) { IdentityConfig.store.otp_delivery_blocklist_maxretry + 1 }

it 'rate limits resends from the otp verification step' do
start_idv_from_sp
complete_idv_steps_before_phone_otp_verification_step(user)

(max_attempts - 1).times do
(Throttle.max_attempts(:phone_otp) - 1).times do
click_on t('links.two_factor_authentication.send_another_code')
end

expect_max_otp_request_rate_limiting
end

def expect_max_otp_request_rate_limiting
expect(page).to have_content t('titles.account_locked')
expect(page).to have_content t(
'two_factor_authentication.max_otp_requests_reached',
Expand Down Expand Up @@ -72,6 +66,11 @@ def expect_rate_limit_circumvention_to_be_disallowed(user)
def expect_rate_limit_to_expire(user)
retry_minutes = IdentityConfig.store.lockout_period_in_minutes + 1
travel_to(retry_minutes.minutes.from_now) do
# This is not good and we can likely drop it once we have upgraded to Redis 7 and switched
# Throttle to use EXPIRETIME rather than TTL
allow_any_instance_of(Throttle).to receive(:attempted_at).and_return(
retry_minutes.minutes.ago,
)
start_idv_from_sp
complete_idv_steps_before_phone_otp_verification_step(user)

Expand Down
4 changes: 1 addition & 3 deletions spec/features/idv/threatmetrix_pending_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
end

scenario 'users pending threatmetrix see sad face screen and cannot perform idv' do
allow(IdentityConfig.store).to receive(:otp_delivery_blocklist_maxretry).and_return(300)
user = create(:user, :signed_up)

start_idv_from_sp
Expand All @@ -26,7 +27,6 @@

# User unable to sign into OIDC with IdV
set_new_browser_session
OtpRequestsTracker.destroy_all
start_idv_from_sp(:oidc)
sign_in_live_with_2fa(user)

Expand All @@ -35,7 +35,6 @@

# User unable to sign into SAML with IdV
set_new_browser_session
OtpRequestsTracker.destroy_all
start_idv_from_sp(:saml)
sign_in_live_with_2fa(user)

Expand All @@ -44,7 +43,6 @@

# User able to sign for IAL1
set_new_browser_session
OtpRequestsTracker.destroy_all
visit_idp_from_sp_with_ial1(:oidc)
sign_in_live_with_2fa(user)
click_agree_and_continue
Expand Down
12 changes: 3 additions & 9 deletions spec/services/idv/steps/in_person/verify_step_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -110,15 +110,9 @@ def build_step(controller)
end

before do
stub_const(
'Throttle::THROTTLE_CONFIG',
{
proof_ssn: {
max_attempts: 2,
attempt_window: 10,
},
}.with_indifferent_access,
)
allow(IdentityConfig.store).to receive(:proof_ssn_max_attempts).and_return(2)
allow(IdentityConfig.store).to receive(:proof_ssn_max_attempt_window_in_minutes).
and_return(10)
end

def redirect(step)
Expand Down
12 changes: 3 additions & 9 deletions spec/services/idv/steps/verify_step_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -110,15 +110,9 @@ def build_step(controller)
end

before do
stub_const(
'Throttle::THROTTLE_CONFIG',
{
proof_ssn: {
max_attempts: 2,
attempt_window: 10,
},
}.with_indifferent_access,
)
allow(IdentityConfig.store).to receive(:proof_ssn_max_attempts).and_return(2)
allow(IdentityConfig.store).to receive(:proof_ssn_max_attempt_window_in_minutes).
and_return(10)
end

def redirect(step)
Expand Down
Loading