8000 [RateLimiter][Security] Add a `login_throttling.interval` (in `security.firewalls`) option to change the default throttling interval. by damienfa · Pull Request #40339 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
Dismiss alert

[RateLimiter][Security] Add a login_throttling.interval (in security.firewalls) option to change the default throttling interval. #40339

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 2, 2021
Merged

[RateLimiter][Security] Add a login_throttling.interval (in security.firewalls) option to change the default throttling interval. #40339

merged 2 commits into from
Mar 2, 2021

Conversation

damienfa
Copy link
Contributor
@damienfa damienfa commented Mar 1, 2021
Q A
Branch 5.x
Bug fix no
New feature yes
Deprecations no
License MIT
Doc PR ⚠️ no doc

The only way to customize the default rate-limiter's options of the login_throttling (means fixed_window / 1 minute / 5 tokens) are through a custom limiter, which implies to declare a rate-limiter factory in the "framework.rate_limiter", a service which use this factory etc. It's really heavy just for changing an interval (moreover, 1 minute can be discutable).

In this PullRequest, I just propose to allow an interval option.

Example :

security:
  firewalls:
    main:
       login_throttling:
           max_attempts: 5
           interval: '15 minutes'

See functional tests.

🤷🏻‍♂️ This pull-request is a copy of this pull-request that I've created some weeks ago. On the original PR, I just needed to rebase on 5.x to pass the tests (fabbot etc.) but the rebase I've tried runs in a loop of conflicts and I'm stuck. I've never experienced this before... SORRY.

@damienfa damienfa requested review from chalasr and wouterj as code owners March 1, 2021 23:32
@damienfa damienfa mentioned this pull request Mar 1, 2021
Closed
wouterj
wouterj approved these changes Mar 2, 2021
View reviewed changes
Copy link
Member
@wouterj wouterj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for all the work!

wouterj
wouterj requested changes Mar 2, 2021
View reviewed changes
Copy link
Memb 8000 er
@wouterj wouterj left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was too quick. The Travis build failures are related to the changes in this PR if I'm correct:

There were 2 failures: 
1) Symfony\Bundle\SecurityBundle\Tests\Functional\FormLoginTest::testLoginThrottling with data set "invalid_password_again" ('johannes', 'also_wrong', 2)

Failed asserting that string matches format description.

--- Expected
+++ Actual
@@ @@
-%sToo many failed login attempts, please try again in 8 minutes%s
+Welcome! Too many failed login attempts, please try again in %minutes% minute. Too many failed login attempts, please try again in 1 minute. Username: Password:

/home/travis/build/symfony/symfony/src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php:134

2) Symfony\Bundle\SecurityBundle\Tests\Functional\FormLoginTest::testLoginThrottling with data set "invalid_password_again_bis" ('johannes', 'wrong_again', 4)

Failed asserting that string matches format description.

--- Expected
+++ Actual
@@ @@
-%sToo many failed login attempts, please try again in 8 minutes%s
+Welcome! Too many failed login attempts, please try again in %minutes% minute. Too many failed login attempts, please try again in 1 minute. Username: Password:

/home/travis/build/symfony/symfony/src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php:140

Are you sure the session is kept between multiple test...() calls? (I would expect the testcase to clear up everything after each test, to avoid side-effects)

@damienfa
Copy link
Contributor Author
damienfa commented Mar 2, 2021

@wouterj You're right, the tests failed for this reason. 🤔 But I have launched the tests with phpunit before pushing the PR and everything has run fine.

cd src/Symfony/Bundle/SecurityBundle
phpunit ./
image

May one of you have a clue as to what's wrong? Or at least, to reproduce the test failure ?

@wouterj
Copy link
Member
wouterj commented Mar 2, 2021

I'll have look :)

@wouterj
Copy link
Member
wouterj commented Mar 2, 2021

@damienfa I've been able to reproduce it locally and fix the tests (see the commit I pushed to this PR). You somehow lost the change in the security test config while creating this PR.

I think I also discovered why you didn't see this error locally. Instead of using a globally installed phpunit, you should use the ./phpunit utility that comes with this repo:

# given you're in the root of the repo (so `symfony/symfony`)
$ ./phpunit src/Symfony/Bundle/SecurityBundle

@damienfa
Copy link
Contributor Author
damienfa commented Mar 2, 2021

@wouterj Yes ! I have just come to the same conclusion 🤦🏻‍♂️ I'm now able to reproduce it locally.
In the rebase / merge process, I've also lost the interval: 8minutes in the yaml. I see in your commit that you fix it as well.
Thanks for your help !

wouterj
wouterj approved these changes Mar 2, 2021
View reviewed changes
Copy link
Member
@wouterj wouterj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All remaining builds failures are not related to these changes (and already tackled by other open PRs).

So ready to merge imho!

@fabpot
Copy link
Member
fabpot commented Mar 2, 2021

Thank you @damienfa.

@fabpot fabpot mentioned this pull request Mar 2, 2021
Closed
@fabpot fabpot merged commit 0f279b5 into symfony:5.x Mar 2, 2021
@fabpot fabpot mentioned this pull request Apr 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@wouterj wouterj wouterj approved these changes

@chalasr chalasr Awaiting requested review from chalasr

Assignees
No one assigned
Labels
RateLimiter Security Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@damienfa @wouterj @fabpot @carsonbot
0