Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable rack-attack only for specific tests #384

Closed
stephanebruckert opened this issue Sep 25, 2018 · 13 comments
Closed

Disable rack-attack only for specific tests #384

stephanebruckert opened this issue Sep 25, 2018 · 13 comments

Comments

@stephanebruckert
Copy link

I have 2 types of tests:

  1. usual acceptance tests for which I want to disable rack-attack
  2. the rack-attack tests for which I want to keep rack-attack

For the first type of tests, I managed to disable rack-attack following #220 (comment)

But then I fail to enable them again in order to test my Allow2Ban.

It doesn't look like there is a solution for this. It doesn't seem easy probably because rack-attack is loaded as an initializer?

Some help would be much appreciated! Thanks

@stephanebruckert stephanebruckert changed the title Disable rack-attack in tests only for specific tests Disable rack-attack only for specific tests Sep 25, 2018
@grzuy
Copy link
Collaborator

grzuy commented Sep 29, 2018

Hi @stephanebruckert,

Can you please provide code examples on how are you setting up Rack::Attack and how are you trying to re-enable in the tests in order to better understand what the issue is?

Thank you!

@javierjulio
Copy link

The only way I believe I was able to resolve this was to clear the RackAttack cache for all request tests. We have an around filter in our rails_helper file just like this:

  config.around(:each, type: :request) do |example|
    Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new
    example.run
    Rack::Attack.cache.store.clear
  end

@javierjulio
Copy link

We configure RackAttack following what is documented in the README for Rails since we want the middleware for all environments. We don't want to remove it from any but we'd like to do instead is enable it only for a specific test. Otherwise we have to clear the cache since our request tests will start to fail as they hit the request limits we are throttling.

@grzuy
Copy link
Collaborator

grzuy commented Aug 30, 2019

I think we could add a configurable enabled boolean, that turns ON/OFF rack-attack. It will help with test suites, development and also occasionally in production, to temporary disable for some reason.

@grzuy
Copy link
Collaborator

grzuy commented Oct 7, 2019

I think we could add a configurable enabled boolean, that turns ON/OFF rack-attack. It will help with test suites, development and also occasionally in production, to temporary disable for some reason.

Heads up. The above was implemented in #431. So hopefully we'll be able to close this issue soon.

@grzuy
Copy link
Collaborator

grzuy commented Oct 9, 2019

#431 merged.
Will be released most probably in 6.2.

@grzuy grzuy closed this as completed Oct 9, 2019
@javierjulio
Copy link

@grzuy we received a Dependabot update today so I applied the change from #431. So much better to enable RackAttack for specific tests where we want to test throttling but being able to leave good defaults in place too e.g. middleware used in all environments. Thanks @fatkodima and @grzuy! ❤️

In my earlier comment #384 (comment) its still necessary to clear the RackAttack cache even with the change from #431. I see @fatkodima has another PR #436 that seems to address that issue. I just wanted to bring this up in case it was thought that just disabling RackAttack might be sufficient for cases like this.

For now, if you have the RackAttack middleware in all environments and you disable RackAttack in your test suite by default, then for any throttling tests, you'll need the following:

  before do
    Rack::Attack.enabled = true
    Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new
  end

  after do
    Rack::Attack.cache.store.clear
    Rack::Attack.enabled = false
  end

With this I'm able to get our tests to pass as expected but only if clearing the cache between each test. Overall though, this is a great improvement as we isolate this just to the tests where we are confirming our RackAttack config.

@grzuy
Copy link
Collaborator

grzuy commented Oct 18, 2019

Hi @javierjulio,

Yes, we are aware we still need to fix #389 to offer real test case isolation.

Happy to read you're successfully using latest improvements by @fatkodima.
Thank you for the feedback.

@javierjulio
Copy link

No problem, happy to help and share in case others come across this. Thanks, I didn't know about #389 so I'll keep an eye on it. This enabled flag feature has been really helpful. Thanks again!

@grzuy
Copy link
Collaborator

grzuy commented Oct 21, 2019

@grzuy we received a Dependabot update today so I applied the change from #431. So much better to enable RackAttack for specific tests where we want to test throttling but being able to leave good defaults in place too e.g. middleware used in all environments. Thanks @fatkodima and @grzuy! heart

In my earlier comment #384 (comment) its still necessary to clear the RackAttack cache even with the change from #431. I see @fatkodima has another PR #436 that seems to address that issue. I just wanted to bring this up in case it was thought that just disabling RackAttack might be sufficient for cases like this.

For now, if you have the RackAttack middleware in all environments and you disable RackAttack in your test suite by default, then for any throttling tests, you'll need the following:

  before do
    Rack::Attack.enabled = true
    Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new
  end

  after do
    Rack::Attack.cache.store.clear
    Rack::Attack.enabled = false
  end

With this I'm able to get our tests to pass as expected but only if clearing the cache between each test. Overall though, this is a great improvement as we isolate this just to the tests where we are confirming our RackAttack config.

Hi @javierjulio ,

Going through this again, I am not sure we'll have something in Rack::Attack which will avoid you having to make an extra call to clear the storage.

Were you expecting something in that regard?
If so, what specifically?

Thanks.

@javierjulio
Copy link

javierjulio commented Oct 21, 2019

@grzuy no, not necessarily. I'm fine with clearing the cache or some reset method as you mentioned in #389 (which I commented there separately too #389 (comment)). I didn't expect that it would go away, just be replaced by something that doesn't dig into implementation. No problem, happy to help, and thanks for following up!

@grzuy
Copy link
Collaborator

grzuy commented Oct 21, 2019

Re-posted comment in #389 a couple of minutes ago. Sorry for the duplication.

@grzuy
Copy link
Collaborator

grzuy commented Apr 26, 2020

For what is worth, Rack::Attack.reset! was relased in v6.3.0.

While Rack::Attack.enabled= was relased in v6.2.0 as mentioned previously.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants