rack-attack icon indicating copy to clipboard operation
rack-attack copied to clipboard

Add ability to use multiple instances of middleware

Open fatkodima opened this issue 6 years ago • 8 comments

This can be used as:

use Rack::Attack do
  ...
  throttle("requests by ip", limit: 5, period: 2) do |request|
    request.ip
  end

  blocklist("block all access to admin") do |request|
    request.path.start_with?("/admin")
  end

  self.blocklisted_response = lambda do |env|
    [503, {}, ['Blocked']]
  end
  ...
end

The code is a little bit more complicated than it should be as I'm not broke old style of reopening Rack::Attack. I would prefer to remain only new style of configuring, but then this will force all gem users to update their Rack::Attack settings and recently added feature of using middleware automatically would not make sense. Maybe this makes more sense for 7.0 release?

New tests and documentation are missing - would add them after agreeing on implementation.

Closes #178

fatkodima avatar Oct 13 '19 22:10 fatkodima

would add them after agreeing on implementation.

Hi @fatkodima ,

I think I see an opportunity to split this PR in 2 and have separate discussions.

If I understood correctly, I see that if you leave the initialize/instance_exec part out of this PR, the rest would just be a refactor encapsulating the config in a Configuration object, which I think is great and provides a lot of value for making code clear and having separation of concerns. That is something I would be ready to merge.

After that we can discuss the other piece which is the one actually making user-facing changes.

grzuy avatar Oct 16 '19 13:10 grzuy

Removed code for extracted Configuration class.

fatkodima avatar Oct 17 '19 11:10 fatkodima

Updated.

fatkodima avatar Nov 15 '23 10:11 fatkodima

Hi, I think this is a cool feature to add 💯

Probably it's minor, but wanted to leave here my only concern that is that if someone in the future uses the gem like:

use(Rack::Attack) do
  blocklist("my blocklist") do |req|
    req.path == '/login'
  end
end

and then runs Rack::Attack.blocklists (or any other method on the class), it will return the value from the class config which will differ. I mean, it makes sense, it's just that it can confuse end users while debugging, maybe? Not a blocker to go ahead.

EDIT: Also, if the block style was used when using the middleware, then if someone attempts to do

Rack::Attack.blocklist("my other blocklist") do |req|
  req.path == '/assets'
end

it won't work. Again, expected, but could be confusing for our end users.

santib avatar Dec 06 '23 01:12 santib

@fatkodima Great suggestion! Can I help out with the missing test? (I was working on a similar PR until @santib pointed me here...)

franzliedke avatar Jan 10 '24 14:01 franzliedke

Would appreciate 🙏 if someone would help me fix CI for older rubies.

fatkodima avatar Jan 11 '24 11:01 fatkodima

Would appreciate 🙏 if someone would help me fix CI for older rubies.

Seems like there are failures in every Ruby version (not just old ones) e.g. Ruby 3.3.

Note: I updated your branch with main since it was missing some fixes on the CI.

santib avatar May 04 '24 14:05 santib

Fixed tests.

fatkodima avatar May 04 '24 15:05 fatkodima