Add ability to use multiple instances of middleware

[fatkodima]

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

[grzuy]

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.

[fatkodima]

Removed code for extracted Configuration class.

[jellybob]

You need to make the final statement in this branch configuration, otherwise @configuration gets set to the return value of the last statement in the block, which in my case was an instance of Throttle.

Otherwise, this seems to work nicely, I've monkey patched this change into a project I'm working on it does the job wonderfully.

[grzuy]

Thanks @jellybob for the code review.

Ideally this problem would reveal in any new test case covering the new block argument.

[grzuy]

Thanks @fatkodima ,

Happy to merge this if we add some test cases using the added optional block argument

[fatkodima]

Updated.

[santib]

the PR goal (per its title) seems to be supporting the ability to have multiples instances of Rack::Attack. I'm wondering.. should we have a test that demonstrates how that works? Not only for catching regressions but also to document this new "feature". Thoughts?

[santib]

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.

[franzliedke]

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

[fatkodima]

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

[santib]

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.

[fatkodima]

Fixed tests.

[santib]

can we actually test that multiple instances of the middleware work together?

Maybe I'm missing something but I tried adding a second middleware

  def app
    Rack::Builder.new do
      use Rack::Attack do
        blocklist_ip("1.2.3.4")
      end
      use Rack::Attack do
        blocklist_ip("4.3.2.1")
      end

      run lambda { |_env| [200, {}, ['Hello World']] }
    end.to_app
  end

  it "can be configured via a block" do
    get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
    assert_equal 403, last_response.status

    get "/", {}, "REMOTE_ADDR" => "4.3.2.1"
    assert_equal 403, last_response.status
  end

and it didn't work 🤔 thoughts?

[fatkodima]

Multiple instances on the same rack app are not working because of this guard clause

rack-attack/lib/rack/attack.rb

Line 105 in b6ce502

return @app.call(env) if !self.class.enabled || env["rack.attack.called"]

[santib]

ahh ok I see.. that was introduced to guarantee idempotency. But with this PR, that's no longer accurate.

What I mean is that in the past it was safe to just have Rack::Attack execute once because all configs were the same, but with this PR we could have different configs of Rack::Attack and only the first one to run will execute, the rest will silently be skipped, right?

At this point I'm starting to doubt if the benefits of the PR are greater than its drawbacks, especially around generating confusion to end users and inflicting pain on them 🤔

Pros I see:

• Nicer way to config using the block rather than the Rack::Attack class.

Cons I see:

• There are now 2 ways to config Rack::Attack which can increase confusion, especially since you can't use both at the same time.
• Idempotency guard no longer works as expected, now only the config of the first middleware to run, will take place.
• Potential conflicts between the automatically added middleware to Rails apps and the usage of the middleware with the new block method.

WDYT? Please let me know your thoughts, I'm open to discussion and changing my mind 🙌

[ioquatix]

I think this is still a good direction.

Ideally we retain compatibility with any existing configuration mechanism and redirect it to a per-instance configuration.

I don't think the idempotency fix is correct. It seems like XY problem.

[fatkodima]

@ioquatix Do you have suggestions how to proceed?

I wish the middleware was not automatically added to rails apps some time ago 😐.

[ioquatix]

I don't have time to provide detailed feedback until after RubyKaigi, but I can take a look then. Is it part of Rails' default stack?

[fatkodima]

Is it part of Rails' default stack?

Yes,

rack-attack/lib/rack/attack/railtie.rb

Lines 11 to 15 in b6ce502

	class Railtie < ::Rails::Railtie
	initializer "rack-attack.middleware" do |app|
	app.middleware.use(Rack::Attack)
	end
	end

but I can take a look then

👍

[santib]

Maybe we can introduce some configs so that you either get the old way to config + idempotency + the railtie, OR you get the new way to config rack attack with the block + ability to have multiple middlewares added + no railtie?

What I mean is that I’m ok with both approaches, but I think the key here is not mixing both of them at the same time 🤔

[ioquatix]

It's fairly straight forward to configure using a Railtie, so we could have a default middleware which gets configured via Rails' own configuration system, but that doesn't mean we can't have non-global instances.