From 397a7ce7b49c95ddf899e1f96b3803797c4e0492 Mon Sep 17 00:00:00 2001 From: Vincent Boisard Date: Tue, 8 Dec 2015 10:53:53 +0100 Subject: [PATCH 1/8] feature: support for ActiveSupport::MemCacheStore --- lib/rack/attack/store_proxy.rb | 4 ++-- lib/rack/attack/store_proxy/dalli_proxy.rb | 2 +- spec/integration/rack_attack_cache_spec.rb | 3 +++ 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/rack/attack/store_proxy.rb b/lib/rack/attack/store_proxy.rb index 5c476ff..94fb169 100644 --- a/lib/rack/attack/store_proxy.rb +++ b/lib/rack/attack/store_proxy.rb @@ -6,14 +6,14 @@ module Rack def self.build(store) # RedisStore#increment needs different behavior, so detect that # (method has an arity of 2; must call #expire separately - if defined?(::ActiveSupport::Cache::RedisStore) && store.is_a?(::ActiveSupport::Cache::RedisStore) + if (defined?(::ActiveSupport::Cache::RedisStore) && store.is_a?(::ActiveSupport::Cache::RedisStore)) || + (defined?(::ActiveSupport::Cache::MemCacheStore) && store.is_a?(::ActiveSupport::Cache::MemCacheStore)) # ActiveSupport::Cache::RedisStore doesn't expose any way to set an expiry, # so use the raw Redis::Store instead store = store.instance_variable_get(:@data) end klass = PROXIES.find { |proxy| proxy.handle?(store) } - klass ? klass.new(store) : store end diff --git a/lib/rack/attack/store_proxy/dalli_proxy.rb b/lib/rack/attack/store_proxy/dalli_proxy.rb index 703f2d6..48067fc 100644 --- a/lib/rack/attack/store_proxy/dalli_proxy.rb +++ b/lib/rack/attack/store_proxy/dalli_proxy.rb @@ -12,7 +12,7 @@ module Rack if defined?(::ConnectionPool) && store.is_a?(::ConnectionPool) store.with { |conn| conn.is_a?(::Dalli::Client) } else - store.is_a?(::Dalli::Client) + store.is_a?(::Dalli::Client) || store.is_a?(::ActiveSupport::Cache::MemCacheStore) end end diff --git a/spec/integration/rack_attack_cache_spec.rb b/spec/integration/rack_attack_cache_spec.rb index 06aa51a..6eb27ef 100644 --- a/spec/integration/rack_attack_cache_spec.rb +++ b/spec/integration/rack_attack_cache_spec.rb @@ -17,12 +17,14 @@ describe Rack::Attack::Cache do end require 'active_support/cache/dalli_store' + require 'active_support/cache/mem_cache_store' require 'active_support/cache/redis_store' require 'connection_pool' cache_stores = [ ActiveSupport::Cache::MemoryStore.new, ActiveSupport::Cache::DalliStore.new("127.0.0.1"), ActiveSupport::Cache::RedisStore.new("127.0.0.1"), + ActiveSupport::Cache::MemCacheStore.new("127.0.0.1"), Dalli::Client.new, ConnectionPool.new { Dalli::Client.new }, Redis::Store.new @@ -54,6 +56,7 @@ describe Rack::Attack::Cache do @cache.send(:do_count, @key, @expires_in).must_equal 2 end end + describe "do_count after expires_in" do it "must be 1" do @cache.send(:do_count, @key, @expires_in) From faa06387190db1e860116872f0d8ca675998a75f Mon Sep 17 00:00:00 2001 From: Vincent Boisard Date: Tue, 8 Dec 2015 18:55:06 +0100 Subject: [PATCH 2/8] fix: Do not attempt to process Memcache clients with DalliProxy --- gemfiles/activesupport3.2.gemfile | 1 + lib/rack/attack/store_proxy.rb | 13 +++++++++---- lib/rack/attack/store_proxy/dalli_proxy.rb | 2 +- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/gemfiles/activesupport3.2.gemfile b/gemfiles/activesupport3.2.gemfile index 61efc24..3068f49 100644 --- a/gemfiles/activesupport3.2.gemfile +++ b/gemfiles/activesupport3.2.gemfile @@ -3,5 +3,6 @@ source "https://rubygems.org" gem "activesupport", "~> 3.2.0" +gem "memcache-client" gemspec :path => "../" diff --git a/lib/rack/attack/store_proxy.rb b/lib/rack/attack/store_proxy.rb index 94fb169..f28ea79 100644 --- a/lib/rack/attack/store_proxy.rb +++ b/lib/rack/attack/store_proxy.rb @@ -8,11 +8,16 @@ module Rack # (method has an arity of 2; must call #expire separately if (defined?(::ActiveSupport::Cache::RedisStore) && store.is_a?(::ActiveSupport::Cache::RedisStore)) || (defined?(::ActiveSupport::Cache::MemCacheStore) && store.is_a?(::ActiveSupport::Cache::MemCacheStore)) - # ActiveSupport::Cache::RedisStore doesn't expose any way to set an expiry, - # so use the raw Redis::Store instead - store = store.instance_variable_get(:@data) - end + # ActiveSupport::Cache::RedisStore doesn't expose any way to set an expiry, + # so use the raw Redis::Store instead. + # We also want to use the underlying Dalli client instead of ::ActiveSupport::Cache::MemCacheStore, + # but not the Memcache client if using Rails 3.x + client = store.instance_variable_get(:@data) + if client.is_a?(Redis::Store) || client.is_a?(Dalli::Client) + store = store.instance_variable_get(:@data) + end + end klass = PROXIES.find { |proxy| proxy.handle?(store) } klass ? klass.new(store) : store end diff --git a/lib/rack/attack/store_proxy/dalli_proxy.rb b/lib/rack/attack/store_proxy/dalli_proxy.rb index 48067fc..703f2d6 100644 --- a/lib/rack/attack/store_proxy/dalli_proxy.rb +++ b/lib/rack/attack/store_proxy/dalli_proxy.rb @@ -12,7 +12,7 @@ module Rack if defined?(::ConnectionPool) && store.is_a?(::ConnectionPool) store.with { |conn| conn.is_a?(::Dalli::Client) } else - store.is_a?(::Dalli::Client) || store.is_a?(::ActiveSupport::Cache::MemCacheStore) + store.is_a?(::Dalli::Client) end end From d880bd88e0fcd00ec3e5a2525bde06e6524c56c8 Mon Sep 17 00:00:00 2001 From: Vincent Boisard Date: Wed, 16 Dec 2015 16:57:54 +0100 Subject: [PATCH 3/8] fix: workaround MemCacheClient + MemCache backend by using a dedicated proxy --- lib/rack/attack.rb | 25 ++++----- lib/rack/attack/store_proxy.rb | 4 +- .../attack/store_proxy/mem_cache_proxy.rb | 51 +++++++++++++++++++ 3 files changed, 66 insertions(+), 14 deletions(-) create mode 100644 lib/rack/attack/store_proxy/mem_cache_proxy.rb diff --git a/lib/rack/attack.rb b/lib/rack/attack.rb index d8308a5..14a4ed9 100644 --- a/lib/rack/attack.rb +++ b/lib/rack/attack.rb @@ -2,18 +2,19 @@ require 'rack' require 'forwardable' class Rack::Attack - autoload :Cache, 'rack/attack/cache' - autoload :Check, 'rack/attack/check' - autoload :Throttle, 'rack/attack/throttle' - autoload :Whitelist, 'rack/attack/whitelist' - autoload :Blacklist, 'rack/attack/blacklist' - autoload :Track, 'rack/attack/track' - autoload :StoreProxy, 'rack/attack/store_proxy' - autoload :DalliProxy, 'rack/attack/store_proxy/dalli_proxy' - autoload :RedisStoreProxy, 'rack/attack/store_proxy/redis_store_proxy' - autoload :Fail2Ban, 'rack/attack/fail2ban' - autoload :Allow2Ban, 'rack/attack/allow2ban' - autoload :Request, 'rack/attack/request' + autoload :Cache, 'rack/attack/cache' + autoload :Check, 'rack/attack/check' + autoload :Throttle, 'rack/attack/throttle' + autoload :Whitelist, 'rack/attack/whitelist' + autoload :Blacklist, 'rack/attack/blacklist' + autoload :Track, 'rack/attack/track' + autoload :StoreProxy, 'rack/attack/store_proxy' + autoload :DalliProxy, 'rack/attack/store_proxy/dalli_proxy' + autoload :MemCacheProxy, 'rack/attack/store_proxy/mem_cache_proxy' + autoload :RedisStoreProxy, 'rack/attack/store_proxy/redis_store_proxy' + autoload :Fail2Ban, 'rack/attack/fail2ban' + autoload :Allow2Ban, 'rack/attack/allow2ban' + autoload :Request, 'rack/attack/request' class << self diff --git a/lib/rack/attack/store_proxy.rb b/lib/rack/attack/store_proxy.rb index f28ea79..b408c5a 100644 --- a/lib/rack/attack/store_proxy.rb +++ b/lib/rack/attack/store_proxy.rb @@ -1,7 +1,7 @@ module Rack class Attack module StoreProxy - PROXIES = [DalliProxy, RedisStoreProxy] + PROXIES = [DalliProxy, MemCacheProxy, RedisStoreProxy] def self.build(store) # RedisStore#increment needs different behavior, so detect that @@ -14,7 +14,7 @@ module Rack # We also want to use the underlying Dalli client instead of ::ActiveSupport::Cache::MemCacheStore, # but not the Memcache client if using Rails 3.x client = store.instance_variable_get(:@data) - if client.is_a?(Redis::Store) || client.is_a?(Dalli::Client) + if client.is_a?(Redis::Store) || client.is_a?(Dalli::Client) || client.is_a?(MemCache) store = store.instance_variable_get(:@data) end end diff --git a/lib/rack/attack/store_proxy/mem_cache_proxy.rb b/lib/rack/attack/store_proxy/mem_cache_proxy.rb new file mode 100644 index 0000000..098e048 --- /dev/null +++ b/lib/rack/attack/store_proxy/mem_cache_proxy.rb @@ -0,0 +1,51 @@ +module Rack + class Attack + module StoreProxy + class MemCacheProxy < SimpleDelegator + def self.handle?(store) + defined?(::MemCache) && store.is_a?(::MemCache) + end + + def initialize(store) + super(store) + stub_with_if_missing + end + + def read(key) + # Second argument: reading raw value + get(key, true) + rescue MemCache::MemCacheError + end + + def write(key, value, options={}) + # Third argument: writing raw value + set(key, value, options.fetch(:expires_in, 0), true) + rescue MemCache::MemCacheError + end + + def increment(key, amount, options={}) + incr(key, amount) + rescue MemCache::MemCacheError + end + + def delete(key, options={}) + with do |client| + client.delete(key) + end + rescue MemCache::MemCacheError + end + + private + + def stub_with_if_missing + unless __getobj__.respond_to?(:with) + class << self + def with; yield __getobj__; end + end + end + end + + end + end + end +end From 32df84df54deccac3bf33e7204d82ed7f05f8f17 Mon Sep 17 00:00:00 2001 From: Vincent Boisard Date: Tue, 29 Dec 2015 10:19:13 +0100 Subject: [PATCH 4/8] fix: check whether client class is defined before checking client itself --- lib/rack/attack/store_proxy.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/rack/attack/store_proxy.rb b/lib/rack/attack/store_proxy.rb index b408c5a..d88ad67 100644 --- a/lib/rack/attack/store_proxy.rb +++ b/lib/rack/attack/store_proxy.rb @@ -12,9 +12,10 @@ module Rack # ActiveSupport::Cache::RedisStore doesn't expose any way to set an expiry, # so use the raw Redis::Store instead. # We also want to use the underlying Dalli client instead of ::ActiveSupport::Cache::MemCacheStore, - # but not the Memcache client if using Rails 3.x + # and the MemCache client if using Rails 3.x client = store.instance_variable_get(:@data) - if client.is_a?(Redis::Store) || client.is_a?(Dalli::Client) || client.is_a?(MemCache) + if (defined?(::Redis::Store) && client.is_a?(Redis::Store)) || + (defined?(Dalli::Client) && client.is_a?(Dalli::Client)) || (defined?(MemCache) && client.is_a?(MemCache)) store = store.instance_variable_get(:@data) end end From ba9f2c3be6b89b238aa03fd984c104d397569487 Mon Sep 17 00:00:00 2001 From: Vincent Boisard Date: Thu, 7 Jan 2016 21:00:36 +0100 Subject: [PATCH 5/8] fix: move dev depencies to gemspec --- gemfiles/activesupport3.2.gemfile | 1 - rack-attack.gemspec | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/gemfiles/activesupport3.2.gemfile b/gemfiles/activesupport3.2.gemfile index aa24027..67813b6 100644 --- a/gemfiles/activesupport3.2.gemfile +++ b/gemfiles/activesupport3.2.gemfile @@ -3,7 +3,6 @@ source "https://rubygems.org" gem "activesupport", "~> 3.2.0" -gem "memcache-client" gem "actionpack", "~> 3.2.0" group :development do diff --git a/rack-attack.gemspec b/rack-attack.gemspec index 057aa6d..1fcdd42 100644 --- a/rack-attack.gemspec +++ b/rack-attack.gemspec @@ -32,4 +32,5 @@ Gem::Specification.new do |s| s.add_development_dependency 'redis-activesupport' s.add_development_dependency 'dalli' s.add_development_dependency 'connection_pool' + s.add_development_dependency 'memcache-client' end From 8d124d868ef834332912544106b828c75b38f52a Mon Sep 17 00:00:00 2001 From: Vincent Boisard Date: Thu, 7 Jan 2016 21:16:35 +0100 Subject: [PATCH 6/8] refactor unwieldy Rack::Attack::StoreProxy.build method --- lib/rack/attack/store_proxy.rb | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/lib/rack/attack/store_proxy.rb b/lib/rack/attack/store_proxy.rb index d88ad67..c88fb6b 100644 --- a/lib/rack/attack/store_proxy.rb +++ b/lib/rack/attack/store_proxy.rb @@ -2,25 +2,30 @@ module Rack class Attack module StoreProxy PROXIES = [DalliProxy, MemCacheProxy, RedisStoreProxy] + USE_BASE_CLIENT = ['Redis::Store', 'Dalli::Client', 'MemCache'] def self.build(store) # RedisStore#increment needs different behavior, so detect that # (method has an arity of 2; must call #expire separately - if (defined?(::ActiveSupport::Cache::RedisStore) && store.is_a?(::ActiveSupport::Cache::RedisStore)) || - (defined?(::ActiveSupport::Cache::MemCacheStore) && store.is_a?(::ActiveSupport::Cache::MemCacheStore)) + client = fetch_client(store) + klass = PROXIES.find { |proxy| proxy.handle?(client) } + klass ? klass.new(client) : client + end - # ActiveSupport::Cache::RedisStore doesn't expose any way to set an expiry, - # so use the raw Redis::Store instead. - # We also want to use the underlying Dalli client instead of ::ActiveSupport::Cache::MemCacheStore, - # and the MemCache client if using Rails 3.x - client = store.instance_variable_get(:@data) - if (defined?(::Redis::Store) && client.is_a?(Redis::Store)) || - (defined?(Dalli::Client) && client.is_a?(Dalli::Client)) || (defined?(MemCache) && client.is_a?(MemCache)) - store = store.instance_variable_get(:@data) - end + def self.fetch_client(store) + client = store.instance_variable_get(:@data) + # RedisStore#increment needs different behavior, so detect that + # (method has an arity of 2; must call #expire separately + # + # ActiveSupport::Cache::RedisStore doesn't expose any way to set an expiry, + # so use the raw Redis::Store instead. + # + # We also want to use the underlying Dalli client instead of ::ActiveSupport::Cache::MemCacheStore, + # and the MemCache client if using Rails 3.x + USE_BASE_CLIENT.each do |klass| + return client if !client.nil? && Object.const_defined?(klass) && client.is_a?(Object.const_get(klass)) end - klass = PROXIES.find { |proxy| proxy.handle?(store) } - klass ? klass.new(store) : store + return store end end From c34bace7739715c8dadff29fbd85cb04fe950ea6 Mon Sep 17 00:00:00 2001 From: Vincent Boisard Date: Thu, 7 Jan 2016 22:45:11 +0100 Subject: [PATCH 7/8] style: remove extraneous whitespace --- lib/rack/attack.rb | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/lib/rack/attack.rb b/lib/rack/attack.rb index f91feb9..672f233 100644 --- a/lib/rack/attack.rb +++ b/lib/rack/attack.rb @@ -2,20 +2,20 @@ require 'rack' require 'forwardable' class Rack::Attack - autoload :Cache, 'rack/attack/cache' - autoload :PathNormalizer, 'rack/attack/path_normalizer' - autoload :Check, 'rack/attack/check' - autoload :Throttle, 'rack/attack/throttle' - autoload :Whitelist, 'rack/attack/whitelist' - autoload :Blacklist, 'rack/attack/blacklist' - autoload :Track, 'rack/attack/track' - autoload :StoreProxy, 'rack/attack/store_proxy' - autoload :DalliProxy, 'rack/attack/store_proxy/dalli_proxy' - autoload :MemCacheProxy, 'rack/attack/store_proxy/mem_cache_proxy' - autoload :RedisStoreProxy, 'rack/attack/store_proxy/redis_store_proxy' - autoload :Fail2Ban, 'rack/attack/fail2ban' - autoload :Allow2Ban, 'rack/attack/allow2ban' - autoload :Request, 'rack/attack/request' + autoload :Cache, 'rack/attack/cache' + autoload :PathNormalizer, 'rack/attack/path_normalizer' + autoload :Check, 'rack/attack/check' + autoload :Throttle, 'rack/attack/throttle' + autoload :Whitelist, 'rack/attack/whitelist' + autoload :Blacklist, 'rack/attack/blacklist' + autoload :Track, 'rack/attack/track' + autoload :StoreProxy, 'rack/attack/store_proxy' + autoload :DalliProxy, 'rack/attack/store_proxy/dalli_proxy' + autoload :MemCacheProxy, 'rack/attack/store_proxy/mem_cache_proxy' + autoload :RedisStoreProxy, 'rack/attack/store_proxy/redis_store_proxy' + autoload :Fail2Ban, 'rack/attack/fail2ban' + autoload :Allow2Ban, 'rack/attack/allow2ban' + autoload :Request, 'rack/attack/request' class << self From 585d1fd02cea23e3df4ff8fa8ee57b4a37b0cfdf Mon Sep 17 00:00:00 2001 From: Vincent Boisard Date: Tue, 12 Jan 2016 11:45:44 +0100 Subject: [PATCH 8/8] Revert "refactor unwieldy Rack::Attack::StoreProxy.build method" This reverts commit 8d124d868ef834332912544106b828c75b38f52a. --- lib/rack/attack/store_proxy.rb | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/lib/rack/attack/store_proxy.rb b/lib/rack/attack/store_proxy.rb index c88fb6b..d88ad67 100644 --- a/lib/rack/attack/store_proxy.rb +++ b/lib/rack/attack/store_proxy.rb @@ -2,30 +2,25 @@ module Rack class Attack module StoreProxy PROXIES = [DalliProxy, MemCacheProxy, RedisStoreProxy] - USE_BASE_CLIENT = ['Redis::Store', 'Dalli::Client', 'MemCache'] def self.build(store) # RedisStore#increment needs different behavior, so detect that # (method has an arity of 2; must call #expire separately - client = fetch_client(store) - klass = PROXIES.find { |proxy| proxy.handle?(client) } - klass ? klass.new(client) : client - end + if (defined?(::ActiveSupport::Cache::RedisStore) && store.is_a?(::ActiveSupport::Cache::RedisStore)) || + (defined?(::ActiveSupport::Cache::MemCacheStore) && store.is_a?(::ActiveSupport::Cache::MemCacheStore)) - def self.fetch_client(store) - client = store.instance_variable_get(:@data) - # RedisStore#increment needs different behavior, so detect that - # (method has an arity of 2; must call #expire separately - # - # ActiveSupport::Cache::RedisStore doesn't expose any way to set an expiry, - # so use the raw Redis::Store instead. - # - # We also want to use the underlying Dalli client instead of ::ActiveSupport::Cache::MemCacheStore, - # and the MemCache client if using Rails 3.x - USE_BASE_CLIENT.each do |klass| - return client if !client.nil? && Object.const_defined?(klass) && client.is_a?(Object.const_get(klass)) + # ActiveSupport::Cache::RedisStore doesn't expose any way to set an expiry, + # so use the raw Redis::Store instead. + # We also want to use the underlying Dalli client instead of ::ActiveSupport::Cache::MemCacheStore, + # and the MemCache client if using Rails 3.x + client = store.instance_variable_get(:@data) + if (defined?(::Redis::Store) && client.is_a?(Redis::Store)) || + (defined?(Dalli::Client) && client.is_a?(Dalli::Client)) || (defined?(MemCache) && client.is_a?(MemCache)) + store = store.instance_variable_get(:@data) + end end - return store + klass = PROXIES.find { |proxy| proxy.handle?(store) } + klass ? klass.new(store) : store end end