diff --git a/app/services/otp_rate_limiter.rb b/app/services/otp_rate_limiter.rb index bb62c837b55..4afde203a54 100644 --- a/app/services/otp_rate_limiter.rb +++ b/app/services/otp_rate_limiter.rb @@ -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 @@ -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 @@ -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 @@ -47,10 +60,6 @@ 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 @@ -58,4 +67,16 @@ def otp_findtime 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 diff --git a/app/services/throttle.rb b/app/services/throttle.rb index e4f868061b6..41a73b4e29c 100644 --- a/app/services/throttle.rb +++ b/app/services/throttle.rb @@ -4,63 +4,12 @@ class Throttle attr_reader :throttle_type - 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, - }, - }.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 @@ -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 diff --git a/config/application.yml.default b/config/application.yml.default index e23b855a73e..045c581db35 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -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 @@ -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"}]' @@ -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 diff --git a/lib/identity_config.rb b/lib/identity_config.rb index 6d1f4874454..e0f59434b30 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -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) diff --git a/spec/features/event_disavowal_spec.rb b/spec/features/event_disavowal_spec.rb index bc713c647a9..eecbeadf2f3 100644 --- a/spec/features/event_disavowal_spec.rb +++ b/spec/features/event_disavowal_spec.rb @@ -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 diff --git a/spec/features/idv/phone_otp_rate_limiting_spec.rb b/spec/features/idv/phone_otp_rate_limiting_spec.rb index f2e2928da53..30ccda4c15e 100644 --- a/spec/features/idv/phone_otp_rate_limiting_spec.rb +++ b/spec/features/idv/phone_otp_rate_limiting_spec.rb @@ -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', @@ -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) diff --git a/spec/features/idv/threatmetrix_pending_spec.rb b/spec/features/idv/threatmetrix_pending_spec.rb index d59fa5c1d13..0201fd47ffd 100644 --- a/spec/features/idv/threatmetrix_pending_spec.rb +++ b/spec/features/idv/threatmetrix_pending_spec.rb @@ -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 @@ -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) @@ -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) @@ -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 diff --git a/spec/services/idv/steps/in_person/verify_step_spec.rb b/spec/services/idv/steps/in_person/verify_step_spec.rb index a6efd623436..994d8302d03 100644 --- a/spec/services/idv/steps/in_person/verify_step_spec.rb +++ b/spec/services/idv/steps/in_person/verify_step_spec.rb @@ -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) diff --git a/spec/services/idv/steps/verify_step_spec.rb b/spec/services/idv/steps/verify_step_spec.rb index febd9f52289..e21c263ba3a 100644 --- a/spec/services/idv/steps/verify_step_spec.rb +++ b/spec/services/idv/steps/verify_step_spec.rb @@ -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) diff --git a/spec/services/otp_rate_limiter_spec.rb b/spec/services/otp_rate_limiter_spec.rb index dc1eae240ae..d8f390c6750 100644 --- a/spec/services/otp_rate_limiter_spec.rb +++ b/spec/services/otp_rate_limiter_spec.rb @@ -29,6 +29,16 @@ expect(otp_rate_limiter.exceeded_otp_send_limit?).to eq(true) end + it 'is is false after maxretry_times attemps in findtime minutes' do + expect(otp_rate_limiter.exceeded_otp_send_limit?).to eq(false) + + IdentityConfig.store.otp_delivery_blocklist_maxretry.times do + otp_rate_limiter.increment + end + + expect(otp_rate_limiter.exceeded_otp_send_limit?).to eq(false) + end + it 'tracks verified phones separately. limiting one does not limit the other' do expect(otp_rate_limiter.exceeded_otp_send_limit?).to eq(false) @@ -43,14 +53,10 @@ describe '#increment' do it 'updates otp_last_sent_at' do - tracker = OtpRequestsTracker.find_or_create_with_phone_and_confirmed( - phone, - false, - ) otp_rate_limiter.increment - old_otp_last_sent_at = tracker.reload.otp_last_sent_at + old_otp_last_sent_at = otp_rate_limiter.otp_last_sent_at otp_rate_limiter.increment - new_otp_last_sent_at = tracker.reload.otp_last_sent_at + new_otp_last_sent_at = otp_rate_limiter.otp_last_sent_at expect(new_otp_last_sent_at).to be > old_otp_last_sent_at end diff --git a/spec/services/throttle_spec.rb b/spec/services/throttle_spec.rb index e45965d0f13..ac1f01364bb 100644 --- a/spec/services/throttle_spec.rb +++ b/spec/services/throttle_spec.rb @@ -4,14 +4,10 @@ let(:throttle_type) { :idv_doc_auth } let(:max_attempts) { 3 } let(:attempt_window) { 10 } - - before do - stub_const( - 'Throttle::THROTTLE_CONFIG', - { - throttle_type => { max_attempts: max_attempts, attempt_window: attempt_window }, - }.with_indifferent_access.freeze, - ) + before(:each) do + allow(IdentityConfig.store).to receive(:doc_auth_max_attempts).and_return(max_attempts) + allow(IdentityConfig.store).to receive(:doc_auth_attempt_window_in_minutes). + and_return(attempt_window) end describe '.new' do @@ -83,8 +79,8 @@ describe '#throttled?' do let(:throttle_type) { :idv_doc_auth } - let(:max_attempts) { IdentityConfig.store.doc_auth_max_attempts } - let(:attempt_window_in_minutes) { IdentityConfig.store.doc_auth_attempt_window_in_minutes } + let(:max_attempts) { Throttle.max_attempts(throttle_type) } + let(:attempt_window_in_minutes) { Throttle.attempt_window_in_minutes(throttle_type) } subject(:throttle) { Throttle.new(target: '1', throttle_type: throttle_type) } @@ -110,7 +106,7 @@ throttle.increment! end - travel(attempt_window_in_minutes.minutes) do + travel(attempt_window_in_minutes.minutes + 1) do expect(throttle.throttled?).to eq(false) end end