[RateLimiter][Security] (obsolete) Add an easy way to customize "interval" in the default security.login_throttling #39927

damienfa · 2021-01-21T18:43:50Z

Q	A
Branch	5.2
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'

⚠️ At the time of writing, there is no Symfony documentation about the Login Throttling feature (only a blog post). Is there a reason to that ? Or may I propose something ?

…in_throttling" 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' ```

carsonbot · 2021-01-21T18:43:52Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

wouterj · 2021-01-21T23:54:31Z

Hi @damienfa! Thanks for your time creating this PR. I think such an option makes lots of sense!

Are you willing to add a test for this new feature? (I can help if you are unsure how to do this) Also note the failing CS check in "fabbot.io" is related to this PR (there seems to be a trailing whitespace). At last, can you please add a small note to src/Symfony/SecurityBundle/CHANGELOG.md to say you introduce this new option?

At the time of writing, there is no Symfony documentation about the Login Throttling feature (only a blog post). Is there a reason to that ? Or may I propose something ?

Lacking time was the main factor (and we have to be careful to mention it only works when using experimental authenticators). I'm happy to review if you propose some docs for this feature.

damienfa · 2021-01-22T00:40:57Z

Ok, thanks for your answer. I will take some time this week-end to fix the failing CS check (fabbot.io) and try to create some unit-test. Is there a test concerning the login_throttling feature or configuration ? I haven't find anything about that in the existing tests (and it's why I didn't implement it). Thanks,

wouterj · 2021-01-22T11:28:41Z

Hi! We have a functional test for login throttling here: https://github.com/symfony/symfony/blob/5.x/src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php#L115 (which uses this config: https://github.com/symfony/symfony/blob/5.x/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/StandardFormLogin/login_throttling.yml)

damienfa · 2021-02-06T12:29:35Z

Hello @wouterj , as discussed I've added some functional test (improving the existing one here with an interval and two more login attempts). Also, I've fixed the fabbot.io check (it was due to a trailing space).

However, the Integration test (7.4) is now failing, and I don't understand which relation it has with my commit 🤷🏻‍♂️ Any help ?

wouterj · 2021-02-06T12:31:25Z

No worries! The failures also occur in the 5.x branch: https://github.com/symfony/symfony/runs/1840808458 So they are certainly not your fault :)

I'll checkout this PR somewhere the coming days. Thanks for your update!

wouterj

2 minor comments, looks good otherwise!

src/Symfony/Bundle/SecurityBundle/CHANGELOG.md

Nyholm

Thank you for this PR. This is great!

Could you fix the minor things so this can be merged in 5.3?

src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php

…n provided (Pierrick VIGNAND) This PR was merged into the 4.4 branch. Discussion ---------- MockResponse total_time should not be simulated when provided | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | When you provide a `total_time` to a MockResponse, it is overriden. It should be simulated only when it is not provided I guess. Ex: `new MockResponse('{"foo":"bar"}', ['total_time' => 0.4])` Commits ------- 8dada95 fix: MockResponse total_time should not be simulated when provided

…f is missing (xabbuh) This PR was merged into the 4.4 branch. Discussion ---------- improve exception message if symfony/security-csrf is missing | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Fix symfony#37452 | License | MIT | Doc PR | Commits ------- 1a26ed4 improve exception message if symfony/security-csrf is missing

…tch-1 # Conflicts: # src/Symfony/Bundle/SecurityBundle/CHANGELOG.md

…in_throttling" 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' ```

…tch-1

…in_throttling" 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' ```

…tch-1

damienfa · 2021-03-01T23:39:29Z

Sorry, I have to close this merge request. Following this comment I needed to rebase on branch 5.x.
Unfortunately, the "rebase" goes a way I've never experienced (it runs in a loop of conflicts and useless merge).
To do this, I simply created a new PR (see here) from this one. It's more readable, clearer.

…al` (in `security.firewalls`) option to change the default throttling interval. (damienfa, wouterj) This PR was merged into the 5.3-dev branch. Discussion ---------- [RateLimiter][Security] Add a `login_throttling.interval` (in `security.firewalls`) option to change the default throttling interval. | 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 : ```yaml security: firewalls: main: login_throttling: max_attempts: 5 interval: '15 minutes' ``` See functional tests. 🤷🏻‍♂️ This pull-request is a copy of [this pull-request ](#39927) 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. Commits ------- d1a0342 Fix tests cc74095 changes rebased

damienfa requested review from chalasr and wouterj as code owners January 21, 2021 18:43

carsonbot added the Status: Needs Review label Jan 21, 2021

wouterj added RateLimiter Security labels Jan 21, 2021

carsonbot changed the title ~~Add an easy way to customize "interval" in the default security.login_throttling~~ [RateLimiter][Security] Add an easy way to customize "interval" in the default security.login_throttling Jan 21, 2021

nicolas-grekas added this to the 5.x milestone Jan 23, 2021

damienfa added 2 commits February 6, 2021 11:32

fix invisible caracters (fabbot.io)

752da9b

[RateLimiter][Security] Add and improve functional tests

9de6edf

add a note (dev) in CHANGELOG.md

a6882b8

maxhelias approved these changes Feb 16, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 16, 2021

wouterj approved these changes Feb 16, 2021

View reviewed changes

src/Symfony/Bundle/SecurityBundle/CHANGELOG.md Outdated Show resolved Hide resolved

src/Symfony/Bundle/SecurityBundle/CHANGELOG.md Outdated Show resolved Hide resolved

Nyholm requested changes Feb 24, 2021

View reviewed changes

src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Reviewed labels Feb 24, 2021

damienfa and others added 6 commits February 24, 2021 21:47

fix varname

02eace3

Merge branch '5.x' into patch-1

e130e61

fix: MockResponse total_time should not be simulated when provided

8dada95

improve exception message if symfony/security-csrf is missing

1a26ed4

damienfa added 6 commits March 1, 2021 23:46

fix invisible caracters (fabbot.io)

f5a6e33

[RateLimiter][Security] Add and improve functional tests

db7edf9

add a note (dev) in CHANGELOG.md

1e6df58

fix varname

11332e9

fix merge

26f36de

Merge branch 'patch-1' of https://github.com/damienfa/symfony into pa…

dbf6383

…tch-1 # Conflicts: # src/Symfony/Bundle/SecurityBundle/CHANGELOG.md

damienfa requested review from xabbuh and yceruto as code owners March 1, 2021 22:50

damienfa and others added 16 commits March 1, 2021 23:53

[RateLimiter][Security] Add and improve functional tests

b73ca4a

fix varname

dd3913b

fix merge

a73b8f5

add a note (dev) in CHANGELOG.md

e20fa16

fix merge

da3c6d8

Merge branch 'patch-1' of https://github.com/damienfa/symfony into pa…

12e313e

…tch-1

[RateLimiter][Security] Add and improve functional tests

8228086

fix varname

565362c

fix merge

75e08a4

add a note (dev) in CHANGELOG.md

ea6b73c

fix merge

5ace4f2

add a note (dev) in CHANGELOG.md

a312e40

fix merge

c4f1797

Merge branch 'patch-1' of https://github.com/damienfa/symfony into pa…

08aff8b

…tch-1

damienfa changed the title ~~[RateLimiter][Security] Add an easy way to customize "interval" in the default security.login_throttling~~ [RateLimiter][Security] (obsolete ) Add an easy way to customize "interval" in the default security.login_throttling Mar 1, 2021

damienfa changed the title ~~[RateLimiter][Security] (obsolete ) Add an easy way to customize "interval" in the default security.login_throttling~~ [RateLimiter][Security] (obsolete) Add an easy way to customize "interval" in the default security.login_throttling Mar 1, 2021

damienfa mentioned this pull request Mar 1, 2021

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

Merged

damienfa closed this Mar 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[RateLimiter][Security] (obsolete) Add an easy way to customize "interval" in the default security.login_throttling #39927

[RateLimiter][Security] (obsolete) Add an easy way to customize "interval" in the default security.login_throttling #39927

Uh oh!

damienfa commented Jan 21, 2021 •

edited by OskarStark

Loading

Uh oh!

carsonbot commented Jan 21, 2021

Uh oh!

wouterj commented Jan 21, 2021 •

edited

Loading

Uh oh!

damienfa commented Jan 22, 2021 via email

Uh oh!

wouterj commented Jan 22, 2021 •

edited

Loading

Uh oh!

damienfa commented Feb 6, 2021 •

edited

Loading

Uh oh!

wouterj commented Feb 6, 2021

Uh oh!

wouterj left a comment

Uh oh!

Uh oh!

Uh oh!

Nyholm left a comment

Uh oh!

Uh oh!

damienfa commented Mar 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Uh oh!

[RateLimiter][Security] (obsolete) Add an easy way to customize "interval" in the default security.login_throttling #39927

[RateLimiter][Security] (obsolete) Add an easy way to customize "interval" in the default security.login_throttling #39927

Uh oh!

Conversation

damienfa commented Jan 21, 2021 • edited by OskarStark Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carsonbot commented Jan 21, 2021

Uh oh!

wouterj commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damienfa commented Jan 22, 2021 via email

Uh oh!

wouterj commented Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damienfa commented Feb 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wouterj commented Feb 6, 2021

Uh oh!

wouterj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

damienfa commented Mar 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

damienfa commented Jan 21, 2021 •

edited by OskarStark

Loading

wouterj commented Jan 21, 2021 •

edited

Loading

wouterj commented Jan 22, 2021 •

edited

Loading

damienfa commented Feb 6, 2021 •

edited

Loading