diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e582ac..e114486 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,10 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Added + +- Throw helpful error message when using `allow2ban` but cache store is misconfigured ([#315](https://github.com/kickstarter/rack-attack/issues/315)) +- Throw helpful error message when using `fail2ban` but cache store is misconfigured ([#315](https://github.com/kickstarter/rack-attack/issues/315)) ## [5.1.0] - 2018-03-10 diff --git a/lib/rack/attack/cache.rb b/lib/rack/attack/cache.rb index aec048b..865e869 100644 --- a/lib/rack/attack/cache.rb +++ b/lib/rack/attack/cache.rb @@ -20,6 +20,9 @@ module Rack end def read(unprefixed_key) + enforce_store_presence! + enforce_store_method_presence!(:read) + store.read("#{prefix}:#{unprefixed_key}") end @@ -46,18 +49,29 @@ module Rack end def do_count(key, expires_in) + enforce_store_presence! + enforce_store_method_presence!(:increment) + + result = store.increment(key, 1, :expires_in => expires_in) + + # NB: Some stores return nil when incrementing uninitialized values + if result.nil? + enforce_store_method_presence!(:write) + + store.write(key, 1, :expires_in => expires_in) + end + result || 1 + end + + def enforce_store_presence! if store.nil? raise Rack::Attack::MissingStoreError - elsif !store.respond_to?(:increment) - raise Rack::Attack::MisconfiguredStoreError, "Store needs to respond to #increment" - else - result = store.increment(key, 1, :expires_in => expires_in) + end + end - # NB: Some stores return nil when incrementing uninitialized values - if result.nil? - store.write(key, 1, :expires_in => expires_in) - end - result || 1 + def enforce_store_method_presence!(method_name) + if !store.respond_to?(method_name) + raise Rack::Attack::MisconfiguredStoreError, "Store needs to respond to ##{method_name}" end end end diff --git a/spec/acceptance/cache_store_config_for_allow2ban_spec.rb b/spec/acceptance/cache_store_config_for_allow2ban_spec.rb index 385697b..aa7208e 100644 --- a/spec/acceptance/cache_store_config_for_allow2ban_spec.rb +++ b/spec/acceptance/cache_store_config_for_allow2ban_spec.rb @@ -9,18 +9,64 @@ describe "Cache store config when using allow2ban" do end end - it "gives error if no store was configured" do - assert_raises do - get "/" + it "gives semantic error if no store was configured" do + assert_raises(Rack::Attack::MissingStoreError) do + get "/scarce-resource" end end - it "gives error if incompatible store was configured" do - Rack::Attack.cache.store = Object.new + it "gives semantic error if store is missing #read method" do + basic_store_class = Class.new do + def write(key, value) + end - assert_raises do - get "/" + def increment(key, count, options = {}) + end end + + Rack::Attack.cache.store = basic_store_class.new + + raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do + get "/scarce-resource" + end + + assert_equal "Store needs to respond to #read", raised_exception.message + end + + it "gives semantic error if store is missing #write method" do + basic_store_class = Class.new do + def read(key) + end + + def increment(key, count, options = {}) + end + end + + Rack::Attack.cache.store = basic_store_class.new + + raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do + get "/scarce-resource" + end + + assert_equal "Store needs to respond to #write", raised_exception.message + end + + it "gives semantic error if store is missing #increment method" do + basic_store_class = Class.new do + def read(key) + end + + def write(key, value) + end + end + + Rack::Attack.cache.store = basic_store_class.new + + raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do + get "/scarce-resource" + end + + assert_equal "Store needs to respond to #increment", raised_exception.message end it "works with any object that responds to #read, #write and #increment" do diff --git a/spec/acceptance/cache_store_config_for_fail2ban_spec.rb b/spec/acceptance/cache_store_config_for_fail2ban_spec.rb index 320fb22..53b43e5 100644 --- a/spec/acceptance/cache_store_config_for_fail2ban_spec.rb +++ b/spec/acceptance/cache_store_config_for_fail2ban_spec.rb @@ -9,18 +9,64 @@ describe "Cache store config when using fail2ban" do end end - it "gives error if no store was configured" do - assert_raises do - get "/" + it "gives semantic error if no store was configured" do + assert_raises(Rack::Attack::MissingStoreError) do + get "/private-place" end end - it "gives error if incompatible store was configured" do - Rack::Attack.cache.store = Object.new + it "gives semantic error if store is missing #read method" do + basic_store_class = Class.new do + def write(key, value) + end - assert_raises do - get "/" + def increment(key, count, options = {}) + end end + + Rack::Attack.cache.store = basic_store_class.new + + raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do + get "/private-place" + end + + assert_equal "Store needs to respond to #read", raised_exception.message + end + + it "gives semantic error if store is missing #write method" do + basic_store_class = Class.new do + def read(key) + end + + def increment(key, count, options = {}) + end + end + + Rack::Attack.cache.store = basic_store_class.new + + raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do + get "/private-place" + end + + assert_equal "Store needs to respond to #write", raised_exception.message + end + + it "gives semantic error if store is missing #increment method" do + basic_store_class = Class.new do + def read(key) + end + + def write(key, value) + end + end + + Rack::Attack.cache.store = basic_store_class.new + + raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do + get "/private-place" + end + + assert_equal "Store needs to respond to #increment", raised_exception.message end it "works with any object that responds to #read, #write and #increment" do