diff --git a/app/models/otp_requests_tracker.rb b/app/models/otp_requests_tracker.rb deleted file mode 100644 index e95ca79b878..00000000000 --- a/app/models/otp_requests_tracker.rb +++ /dev/null @@ -1,21 +0,0 @@ -class OtpRequestsTracker < ApplicationRecord - def self.find_or_create_with_phone_and_confirmed(phone, phone_confirmed) - create_or_find_by( - phone_fingerprint: Pii::Fingerprinter.fingerprint(phone.strip), - phone_confirmed: phone_confirmed, - ) - end - - def self.atomic_increment(id) - now = Time.zone.now - # The following sql offers superior db performance with one write and no locking overhead - query = sanitize_sql_array( - ['UPDATE otp_requests_trackers ' \ - 'SET otp_send_count = otp_send_count + 1,' \ - 'otp_last_sent_at = ?, updated_at = ? ' \ - 'WHERE id = ?', now, now, id], - ) - OtpRequestsTracker.connection.execute(query) - OtpRequestsTracker.find(id) - end -end diff --git a/app/services/otp_rate_limiter.rb b/app/services/otp_rate_limiter.rb index 4afde203a54..b0aa75d31d7 100644 --- a/app/services/otp_rate_limiter.rb +++ b/app/services/otp_rate_limiter.rb @@ -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 @@ -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) - end - # rubocop:enable Naming/MemoizedInstanceVariableName - def otp_findtime IdentityConfig.store.otp_delivery_blocklist_findtime.minutes end @@ -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 diff --git a/config/application.yml.default b/config/application.yml.default index 3182161f491..045af54461f 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -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 @@ -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"}]' @@ -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 diff --git a/lib/identity_config.rb b/lib/identity_config.rb index c06adb8dcb6..5fc3ed0a0b0 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -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) diff --git a/spec/models/otp_requests_tracker_spec.rb b/spec/models/otp_requests_tracker_spec.rb deleted file mode 100644 index 75230f999e0..00000000000 --- a/spec/models/otp_requests_tracker_spec.rb +++ /dev/null @@ -1,63 +0,0 @@ -require 'rails_helper' - -describe OtpRequestsTracker do - let(:phone) { '+1 703 555 1212' } - let(:phone_fingerprint) { Pii::Fingerprinter.fingerprint(phone) } - - describe '.find_or_create_with_phone_and_confirmed' do - context 'match found' do - it 'returns the existing record and does not change it' do - OtpRequestsTracker.create( - phone_fingerprint: phone_fingerprint, - phone_confirmed: true, - otp_send_count: 3, - otp_last_sent_at: Time.zone.now - 1.hour, - ) - - existing = OtpRequestsTracker.where(phone_fingerprint: phone_fingerprint).first - - expect { OtpRequestsTracker.find_or_create_with_phone_and_confirmed(phone, true) }. - to_not change(OtpRequestsTracker, :count) - expect { OtpRequestsTracker.find_or_create_with_phone_and_confirmed(phone, true) }. - to_not change(existing, :otp_send_count) - expect { OtpRequestsTracker.find_or_create_with_phone_and_confirmed(phone, true) }. - to_not change(existing, :otp_last_sent_at) - end - end - - context 'match not found' do - it 'creates new record with otp_send_count = 0' do - expect { OtpRequestsTracker.find_or_create_with_phone_and_confirmed(phone, true) }. - to change(OtpRequestsTracker, :count).by(1) - - existing = OtpRequestsTracker.where(phone_fingerprint: phone_fingerprint).first - - expect(existing.otp_send_count).to eq 0 - end - end - end - - describe '.atomic_increment' do - it 'updates otp_last_sent_at' do - old_ort = OtpRequestsTracker.create( - phone_fingerprint: phone_fingerprint, - otp_send_count: 3, - phone_confirmed: true, - otp_last_sent_at: Time.zone.now - 1.hour, - ) - new_ort = OtpRequestsTracker.atomic_increment(old_ort.id) - expect(new_ort.otp_last_sent_at).to be > old_ort.otp_last_sent_at - end - - it 'increments the otp_send_count' do - old_ort = OtpRequestsTracker.create( - phone_fingerprint: phone_fingerprint, - otp_send_count: 3, - phone_confirmed: true, - otp_last_sent_at: Time.zone.now, - ) - new_ort = OtpRequestsTracker.atomic_increment(old_ort.id) - expect(new_ort.otp_send_count - 1).to eq(old_ort.otp_send_count) - end - end -end diff --git a/spec/services/otp_rate_limiter_spec.rb b/spec/services/otp_rate_limiter_spec.rb index d8f390c6750..88c2ba2f0f4 100644 --- a/spec/services/otp_rate_limiter_spec.rb +++ b/spec/services/otp_rate_limiter_spec.rb @@ -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 @@ -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