From e363a7f30c174a4845a5b31aba8e4cafa48a1344 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Mon, 6 Feb 2023 12:52:06 -0600 Subject: [PATCH 1/6] Use Redis 7 EXPIRETIME command for calculating rate limit state changelog: Internal, Rate Limiting, Use Redis 7 EXPIRETIME command for calculating rate limit state --- app/services/throttle.rb | 19 +++++++------------ .../idv/phone_errors_controller_spec.rb | 3 +-- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/app/services/throttle.rb b/app/services/throttle.rb index 139c6a07d94..e067cfb0bcf 100644 --- a/app/services/throttle.rb +++ b/app/services/throttle.rb @@ -87,30 +87,25 @@ def increment! end # Retrieve the current state of the throttle from Redis - # We use TTL to calculate when the action was last attempted. - # This approach is introduces some skew since time passes - # between "now" and when we fetch the TTL, but it should be low - # relative to the overall length of the throttle window. - # - # When we upgrade to Redis 7, we can use the EXPIRETIME command instead. + # We use EXPIRETIME to calculate when the action was last attempted. def fetch_state! value = nil - ttl = nil + expiretime = nil REDIS_THROTTLE_POOL.with do |client| - value, ttl = client.multi do |multi| + value, expiretime = client.multi do |multi| multi.get(key) - multi.ttl(key) + multi.expiretime(key) end end @redis_attempts = value.to_i - if ttl < 0 + if expiretime < 0 @redis_attempted_at = nil else @redis_attempted_at = - Time.zone.now - - Throttle.attempt_window_in_minutes(throttle_type).minutes + ttl.seconds + ActiveSupport::TimeZone['UTC'].at(expiretime) - + Throttle.attempt_window_in_minutes(throttle_type).minutes end self diff --git a/spec/controllers/idv/phone_errors_controller_spec.rb b/spec/controllers/idv/phone_errors_controller_spec.rb index 8fe9548c67f..62eae401b87 100644 --- a/spec/controllers/idv/phone_errors_controller_spec.rb +++ b/spec/controllers/idv/phone_errors_controller_spec.rb @@ -215,8 +215,7 @@ context 'while throttled' do let(:user) { create(:user) } let(:attempted_at) do - d = DateTime.now # microsecond precision failing on CI - Time.zone.parse(d.to_s) + Time.zone.now.utc end before do From 3a6669f4b28ab926b279b0303201eaf106aee2f7 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Mon, 6 Feb 2023 12:55:40 -0600 Subject: [PATCH 2/6] use Redis 7.0 in CI --- .gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index bb34cf5bfa2..a3778151931 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -128,7 +128,7 @@ specs: services: - name: postgres:13.4 alias: db-postgres - - name: redis + - name: redis:7.0 alias: db-redis artifacts: expire_in: 31d From 16ae63a32daa2897eb1a9ddd17aa3889358566e0 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Mon, 6 Feb 2023 13:01:12 -0600 Subject: [PATCH 3/6] update Postgres version --- .gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index a3778151931..e171730e71a 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -126,7 +126,7 @@ specs: POSTGRES_HOST_AUTH_METHOD: trust RAILS_ENV: test services: - - name: postgres:13.4 + - name: postgres:13.9 alias: db-postgres - name: redis:7.0 alias: db-redis From dda7e0f0acdda19fbe1395b0f2589619d04f447d Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Wed, 3 May 2023 12:32:39 -0500 Subject: [PATCH 4/6] update readme --- docs/local-development.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/local-development.md b/docs/local-development.md index 1f33ac62437..a4d2db3e767 100644 --- a/docs/local-development.md +++ b/docs/local-development.md @@ -11,7 +11,7 @@ We recommend using [Homebrew](https://brew.sh/), [rbenv](https://github.com/rben - Ruby ~> 3.2.0 - [PostgreSQL](http://www.postgresql.org/download/) -- [Redis 5+](http://redis.io/) +- [Redis 7+](http://redis.io/) - [Node.js v16](https://nodejs.org) - [Yarn](https://yarnpkg.com/en/) - [chromedriver](https://formulae.brew.sh/cask/chromedriver) From dc08dd2ec11df89514244fdc66757cda7e16da08 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 17 Mar 2023 16:57:38 -0500 Subject: [PATCH 5/6] reduce max attempts --- spec/features/users/sign_up_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/features/users/sign_up_spec.rb b/spec/features/users/sign_up_spec.rb index f6a55c43fe6..4a1e197b1a0 100644 --- a/spec/features/users/sign_up_spec.rb +++ b/spec/features/users/sign_up_spec.rb @@ -110,6 +110,7 @@ scenario 'rate limits sign-up phone confirmation attempts' do allow(IdentityConfig.store).to receive(:otp_delivery_blocklist_maxretry).and_return(999) + allow(IdentityConfig.store).to receive(:phone_confirmation_max_attempts).and_return(1) sign_up_and_set_password From 3acf991d4510a4488625fbd538f39f57f9647683 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 23 Mar 2023 19:01:45 -0500 Subject: [PATCH 6/6] more reliable times --- app/services/throttle.rb | 9 +++++---- spec/controllers/idv/phone_errors_controller_spec.rb | 10 +++------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/app/services/throttle.rb b/app/services/throttle.rb index e067cfb0bcf..577e7900244 100644 --- a/app/services/throttle.rb +++ b/app/services/throttle.rb @@ -104,7 +104,7 @@ def fetch_state! @redis_attempted_at = nil else @redis_attempted_at = - ActiveSupport::TimeZone['UTC'].at(expiretime) - + ActiveSupport::TimeZone['UTC'].at(expiretime).in_time_zone(Time.zone) - Throttle.attempt_window_in_minutes(throttle_type).minutes end @@ -122,17 +122,18 @@ def reset! def increment_to_throttled! value = Throttle.max_attempts(throttle_type) + now = Time.zone.now REDIS_THROTTLE_POOL.with do |client| - client.setex( + client.set( key, - Throttle.attempt_window_in_minutes(throttle_type).minutes.seconds.to_i, value, + exat: now.to_i + Throttle.attempt_window_in_minutes(throttle_type).minutes.seconds.to_i, ) end @redis_attempts = value.to_i - @redis_attempted_at = Time.zone.now + @redis_attempted_at = now attempts end diff --git a/spec/controllers/idv/phone_errors_controller_spec.rb b/spec/controllers/idv/phone_errors_controller_spec.rb index 62eae401b87..876149999a1 100644 --- a/spec/controllers/idv/phone_errors_controller_spec.rb +++ b/spec/controllers/idv/phone_errors_controller_spec.rb @@ -214,15 +214,9 @@ context 'while throttled' do let(:user) { create(:user) } - let(:attempted_at) do - Time.zone.now.utc - end - - before do - Throttle.new(throttle_type: :proof_address, user: user).increment_to_throttled! - end it 'assigns expiration time' do + Throttle.new(throttle_type: :proof_address, user: user).increment_to_throttled! get action expect(assigns(:expires_at)).to be_kind_of(Time) @@ -230,6 +224,8 @@ it 'logs an event' do freeze_time do + attempted_at = Time.zone.now.utc + Throttle.new(throttle_type: :proof_address, user: user).increment_to_throttled! throttle_window = Throttle.attempt_window_in_minutes(:proof_address).minutes get action