diff --git a/app/services/rate_limiter.rb b/app/services/rate_limiter.rb index f278b178fe3..67f7f06f7f1 100644 --- a/app/services/rate_limiter.rb +++ b/app/services/rate_limiter.rb @@ -71,18 +71,19 @@ def increment! return if limited? value = nil + now = Time.zone.now REDIS_THROTTLE_POOL.with do |client| value, _success = client.multi do |multi| multi.incr(key) - multi.expire( + multi.expireat( key, - RateLimiter.attempt_window_in_minutes(rate_limit_type).minutes.seconds.to_i, + now + RateLimiter.attempt_window_in_minutes(rate_limit_type).minutes.seconds.to_i, ) end end @redis_attempts = value.to_i - @redis_attempted_at = Time.zone.now + @redis_attempted_at = now attempts end diff --git a/spec/features/idv/steps/request_letter_step_spec.rb b/spec/features/idv/steps/request_letter_step_spec.rb index 17c03d15e20..3279bfbab07 100644 --- a/spec/features/idv/steps/request_letter_step_spec.rb +++ b/spec/features/idv/steps/request_letter_step_spec.rb @@ -32,6 +32,12 @@ context 'the user has sent a letter but not verified an OTP' do let(:user) { user_with_2fa } + before do + # Without this, the check for GPO expiration leaves an expired + # OTP rate limiter laying around. + allow(IdentityConfig.store).to receive(:otp_delivery_blocklist_maxretry).and_return(3) + end + it 'if not rate limited, allow user to resend letter & redirect to letter enqueued step', :js do complete_idv_by_mail_and_sign_out diff --git a/spec/services/rate_limiter_spec.rb b/spec/services/rate_limiter_spec.rb index dae6a7297f9..22a2e920922 100644 --- a/spec/services/rate_limiter_spec.rb +++ b/spec/services/rate_limiter_spec.rb @@ -145,6 +145,21 @@ of(rate_limiter.attempted_at + attempt_window.minutes) end end + + context 'when we are out of sync with Redis' do + it 'expires at the correct time' do + fake_attempt_time = 1.year.ago + travel_to(fake_attempt_time) do + # Redis should immediately delete the rate limiter because + # we're supplying an expiration time which has long since + # passed. + rate_limiter.increment! + end + + new_rate_limiter = RateLimiter.new(target: '1', rate_limit_type: rate_limit_type) + expect(new_rate_limiter.expires_at).to be_nil + end + end end context 'with active rate_limiter' do