From cb82b9f873d8c47dcd6b000c1d9a322a9cc5044d Mon Sep 17 00:00:00 2001 From: Santiago Bartesaghi Date: Wed, 10 Jan 2024 11:55:37 -0300 Subject: [PATCH] ci: freeze time in more specs (#643) * ci: freeze time in more specs * Introduce within_same_period helper method --------- Co-authored-by: Gonzalo <456459+grzuy@users.noreply.github.com> --- .../stores/active_support_dalli_store_spec.rb | 1 - ...ive_support_mem_cache_store_pooled_spec.rb | 1 - .../active_support_mem_cache_store_spec.rb | 1 - .../active_support_memory_store_spec.rb | 2 - ...e_support_redis_cache_store_pooled_spec.rb | 1 - .../active_support_redis_cache_store_spec.rb | 1 - .../connection_pool_dalli_client_spec.rb | 1 - spec/acceptance/stores/dalli_client_spec.rb | 1 - spec/acceptance/stores/redis_spec.rb | 1 - spec/acceptance/stores/redis_store_spec.rb | 2 - spec/rack_attack_throttle_spec.rb | 16 +++--- spec/support/cache_store_helper.rb | 56 ++++++++++--------- spec/support/freeze_time_helper.rb | 9 +++ 13 files changed, 48 insertions(+), 45 deletions(-) create mode 100644 spec/support/freeze_time_helper.rb diff --git a/spec/acceptance/stores/active_support_dalli_store_spec.rb b/spec/acceptance/stores/active_support_dalli_store_spec.rb index 7035516..4aded23 100644 --- a/spec/acceptance/stores/active_support_dalli_store_spec.rb +++ b/spec/acceptance/stores/active_support_dalli_store_spec.rb @@ -9,7 +9,6 @@ should_run = if should_run require_relative "../../support/cache_store_helper" require "active_support/cache/dalli_store" - require "timecop" describe "ActiveSupport::Cache::DalliStore as a cache backend" do before do diff --git a/spec/acceptance/stores/active_support_mem_cache_store_pooled_spec.rb b/spec/acceptance/stores/active_support_mem_cache_store_pooled_spec.rb index 8344b6e..a04ceda 100644 --- a/spec/acceptance/stores/active_support_mem_cache_store_pooled_spec.rb +++ b/spec/acceptance/stores/active_support_mem_cache_store_pooled_spec.rb @@ -4,7 +4,6 @@ require_relative "../../spec_helper" if defined?(::ConnectionPool) && defined?(::Dalli) require_relative "../../support/cache_store_helper" - require "timecop" describe "ActiveSupport::Cache::MemCacheStore (pooled) as a cache backend" do before do diff --git a/spec/acceptance/stores/active_support_mem_cache_store_spec.rb b/spec/acceptance/stores/active_support_mem_cache_store_spec.rb index 65abe7d..09f6351 100644 --- a/spec/acceptance/stores/active_support_mem_cache_store_spec.rb +++ b/spec/acceptance/stores/active_support_mem_cache_store_spec.rb @@ -4,7 +4,6 @@ require_relative "../../spec_helper" if defined?(::Dalli) require_relative "../../support/cache_store_helper" - require "timecop" describe "ActiveSupport::Cache::MemCacheStore as a cache backend" do before do diff --git a/spec/acceptance/stores/active_support_memory_store_spec.rb b/spec/acceptance/stores/active_support_memory_store_spec.rb index e047b44..4ed81e7 100644 --- a/spec/acceptance/stores/active_support_memory_store_spec.rb +++ b/spec/acceptance/stores/active_support_memory_store_spec.rb @@ -3,8 +3,6 @@ require_relative "../../spec_helper" require_relative "../../support/cache_store_helper" -require "timecop" - describe "ActiveSupport::Cache::MemoryStore as a cache backend" do before do Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new diff --git a/spec/acceptance/stores/active_support_redis_cache_store_pooled_spec.rb b/spec/acceptance/stores/active_support_redis_cache_store_pooled_spec.rb index fe95107..3da658f 100644 --- a/spec/acceptance/stores/active_support_redis_cache_store_pooled_spec.rb +++ b/spec/acceptance/stores/active_support_redis_cache_store_pooled_spec.rb @@ -10,7 +10,6 @@ should_run = if should_run require_relative "../../support/cache_store_helper" - require "timecop" describe "ActiveSupport::Cache::RedisCacheStore (pooled) as a cache backend" do before do diff --git a/spec/acceptance/stores/active_support_redis_cache_store_spec.rb b/spec/acceptance/stores/active_support_redis_cache_store_spec.rb index a824ede..99701a3 100644 --- a/spec/acceptance/stores/active_support_redis_cache_store_spec.rb +++ b/spec/acceptance/stores/active_support_redis_cache_store_spec.rb @@ -9,7 +9,6 @@ should_run = if should_run require_relative "../../support/cache_store_helper" - require "timecop" describe "ActiveSupport::Cache::RedisCacheStore as a cache backend" do before do diff --git a/spec/acceptance/stores/connection_pool_dalli_client_spec.rb b/spec/acceptance/stores/connection_pool_dalli_client_spec.rb index d532a29..9658480 100644 --- a/spec/acceptance/stores/connection_pool_dalli_client_spec.rb +++ b/spec/acceptance/stores/connection_pool_dalli_client_spec.rb @@ -6,7 +6,6 @@ if defined?(::Dalli) && defined?(::ConnectionPool) require_relative "../../support/cache_store_helper" require "connection_pool" require "dalli" - require "timecop" describe "ConnectionPool with Dalli::Client as a cache backend" do before do diff --git a/spec/acceptance/stores/dalli_client_spec.rb b/spec/acceptance/stores/dalli_client_spec.rb index 08038c2..f6841e4 100644 --- a/spec/acceptance/stores/dalli_client_spec.rb +++ b/spec/acceptance/stores/dalli_client_spec.rb @@ -5,7 +5,6 @@ require_relative "../../spec_helper" if defined?(::Dalli) require_relative "../../support/cache_store_helper" require "dalli" - require "timecop" describe "Dalli::Client as a cache backend" do before do diff --git a/spec/acceptance/stores/redis_spec.rb b/spec/acceptance/stores/redis_spec.rb index 4361566..bf68bc2 100644 --- a/spec/acceptance/stores/redis_spec.rb +++ b/spec/acceptance/stores/redis_spec.rb @@ -4,7 +4,6 @@ require_relative "../../spec_helper" if defined?(::Redis) require_relative "../../support/cache_store_helper" - require "timecop" describe "Plain redis as a cache backend" do before do diff --git a/spec/acceptance/stores/redis_store_spec.rb b/spec/acceptance/stores/redis_store_spec.rb index dee35bc..83d0e65 100644 --- a/spec/acceptance/stores/redis_store_spec.rb +++ b/spec/acceptance/stores/redis_store_spec.rb @@ -4,8 +4,6 @@ require_relative "../../spec_helper" require_relative "../../support/cache_store_helper" if defined?(::Redis::Store) - require "timecop" - describe "Redis::Store as a cache backend" do before do Rack::Attack.cache.store = ::Redis::Store.new diff --git a/spec/rack_attack_throttle_spec.rb b/spec/rack_attack_throttle_spec.rb index b6a32ee..1bf7f32 100644 --- a/spec/rack_attack_throttle_spec.rb +++ b/spec/rack_attack_throttle_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require_relative 'spec_helper' -require 'timecop' +require_relative 'support/freeze_time_helper' describe 'Rack::Attack.throttle' do before do @@ -16,7 +16,7 @@ describe 'Rack::Attack.throttle' do describe 'a single request' do it 'should set the counter for one request' do - Timecop.freeze do + within_same_period do get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' key = "rack::attack:#{Time.now.to_i / @period}:ip/sec:1.2.3.4" @@ -41,7 +41,7 @@ describe 'Rack::Attack.throttle' do describe "with 2 requests" do before do - Timecop.freeze do + within_same_period do 2.times { get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' } end end @@ -78,7 +78,7 @@ describe 'Rack::Attack.throttle with limit as proc' do describe 'a single request' do it 'should set the counter for one request' do - Timecop.freeze do + within_same_period do get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' key = "rack::attack:#{Time.now.to_i / @period}:ip/sec:1.2.3.4" @@ -112,7 +112,7 @@ describe 'Rack::Attack.throttle with period as proc' do describe 'a single request' do it 'should set the counter for one request' do - Timecop.freeze do + within_same_period do get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' key = "rack::attack:#{Time.now.to_i / @period}:ip/sec:1.2.3.4" @@ -147,7 +147,7 @@ describe 'Rack::Attack.throttle with block returning nil' do describe 'a single request' do it 'should not set the counter' do - Timecop.freeze do + within_same_period do get '/', {}, 'REMOTE_ADDR' => '1.2.3.4' key = "rack::attack:#{Time.now.to_i / @period}:ip/sec:1.2.3.4" @@ -179,7 +179,7 @@ describe 'Rack::Attack.throttle with throttle_discriminator_normalizer' do end it 'should not differentiate requests when throttle_discriminator_normalizer is enabled' do - Timecop.freeze do + within_same_period do post_logins key = "rack::attack:#{Time.now.to_i / @period}:logins/email:person@example.com" _(Rack::Attack.cache.store.read(key)).must_equal 3 @@ -191,7 +191,7 @@ describe 'Rack::Attack.throttle with throttle_discriminator_normalizer' do prev = Rack::Attack.throttle_discriminator_normalizer Rack::Attack.throttle_discriminator_normalizer = nil - Timecop.freeze do + within_same_period do post_logins @emails.each do |email| key = "rack::attack:#{Time.now.to_i / @period}:logins/email:#{email}" diff --git a/spec/support/cache_store_helper.rb b/spec/support/cache_store_helper.rb index 5b8f04b..8295ac0 100644 --- a/spec/support/cache_store_helper.rb +++ b/spec/support/cache_store_helper.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require_relative 'freeze_time_helper' + class Minitest::Spec def self.it_works_for_cache_backed_features(options) fetch_from_store = options.fetch(:fetch_from_store) @@ -9,11 +11,13 @@ class Minitest::Spec request.ip end - get "/", {}, "REMOTE_ADDR" => "1.2.3.4" - assert_equal 200, last_response.status + within_same_period do + get "/", {}, "REMOTE_ADDR" => "1.2.3.4" + assert_equal 200, last_response.status - get "/", {}, "REMOTE_ADDR" => "1.2.3.4" - assert_equal 429, last_response.status + get "/", {}, "REMOTE_ADDR" => "1.2.3.4" + assert_equal 429, last_response.status + end end it "works for fail2ban" do @@ -23,17 +27,19 @@ class Minitest::Spec end end - get "/" - assert_equal 200, last_response.status + within_same_period do + get "/" + assert_equal 200, last_response.status - get "/private-place" - assert_equal 403, last_response.status + get "/private-place" + assert_equal 403, last_response.status - get "/private-place" - assert_equal 403, last_response.status + get "/private-place" + assert_equal 403, last_response.status - get "/" - assert_equal 403, last_response.status + get "/" + assert_equal 403, last_response.status + end end it "works for allow2ban" do @@ -43,20 +49,22 @@ class Minitest::Spec end end - get "/" - assert_equal 200, last_response.status + within_same_period do + get "/" + assert_equal 200, last_response.status - get "/scarce-resource" - assert_equal 200, last_response.status + get "/scarce-resource" + assert_equal 200, last_response.status - get "/scarce-resource" - assert_equal 200, last_response.status + get "/scarce-resource" + assert_equal 200, last_response.status - get "/scarce-resource" - assert_equal 403, last_response.status + get "/scarce-resource" + assert_equal 403, last_response.status - get "/" - assert_equal 403, last_response.status + get "/" + assert_equal 403, last_response.status + end end it "doesn't leak keys" do @@ -66,9 +74,7 @@ class Minitest::Spec key = nil - # Freeze time during these statement to be sure that the key used by rack attack is the same - # we pre-calculate in local variable `key` - Timecop.freeze do + within_same_period do key = "rack::attack:#{Time.now.to_i}:by ip:1.2.3.4" get "/", {}, "REMOTE_ADDR" => "1.2.3.4" diff --git a/spec/support/freeze_time_helper.rb b/spec/support/freeze_time_helper.rb new file mode 100644 index 0000000..462c877 --- /dev/null +++ b/spec/support/freeze_time_helper.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require "timecop" + +class Minitest::Spec + def within_same_period(&block) + Timecop.freeze(&block) + end +end