8000 [RateLimiter][Security] Allow to use no lock in the rate limiter/login throttling by wouterj · Pull Request #40284 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[RateLimiter][Security] Allow to use no lock in the rate limiter/login throttling #40284

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 1 commit into from
Feb 26, 2021
Merged

[RateLimiter][Security] Allow to use no lock in the rate limiter/login throttling #40284

merged 1 commit into from
Feb 26, 2021

Conversation

wouterj
Copy link
Member
@wouterj wouterj commented Feb 23, 2021
Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix -
License MIT
Doc PR tbd

This PR adds support for disabling lock in rate limiters. This was brought up by @Seldaek. In most cases (e.g. login throttling), it's not critical to strictly avoid even a single overflow of the window/token. At least, it's probably not always worth the extra load on the lock storage (e.g. redis).

It also directly disables locking by default for login throttling. I'm not sure about this, but I feel like this fits the 80% case where it's definitely not needed (and it's easier to use if you don't need to set-up locking first).

@wouterj wouterj requested a review from chalasr as a code owner February 23, 2021 21:17
@carsonbot carsonbot changed the title [Security][RateLimiter] Allow to use no lock in the rate limiter/login throttling [RateLimiter][Security] Allow to use no lock in the rate limiter/login throttling Feb 23, 2021
@wouterj wouterj added this to the 5.x milestone Feb 23, 2021
chalasr
chalasr reviewed Feb 23, 2021
View reviewed changes
Copy link
Member
@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

would be even better with a test :)

Nyholm
Nyholm requested changes Feb 24, 2021
View reviewed changes
Copy link
Member
@Nyholm Nyholm 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 this PR. I like this feature, but I fear it might be a BC break.

@wouterj wouterj force-pushed the rate-limiter-no-lock branch from b33b948 to 61d6233 Compare February 25, 2021 14:01
@wouterj
Copy link
Member Author
wouterj commented Feb 25, 2021

Thanks for the reviews! Updated UPGRADE and CHANGELOG & added some tests.

Status: Needs Review

@wouterj wouterj force-pushed the rate-limiter-no-lock branch 3 times, most recently from dbe11a1 to 2224d78 Compare February 25, 2021 14:21
Nyholm
Nyholm approved these changes Feb 25, 2021
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Im happy with this PR when CI is green.

@wouterj wouterj force-pushed the rate-limiter-no-lock branch 2 times, most recently from 67beb15 to eed20d0 Compare February 25, 2021 16:31
@wouterj wouterj force-pushed the rate-limiter-no-lock branch from eed20d0 to 45be875 Compare February 25, 2021 16:33
@fabpot
Copy link
Member
fabpot commented Feb 26, 2021

Thank you @wouterj.

@fabpot fabpot merged commit 79f6a5c into symfony:5.x Feb 26, 2021
@fabpot fabpot mentioned this pull request Feb 26, 2021
Closed
@wouterj wouterj deleted the rate-limiter-no-lock branch February 26, 2021 09:44
@Nyholm Nyholm mentioned this pull request Mar 2, 2021
Closed
@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

@Seldaek Seldaek Seldaek approved these changes

@Nyholm Nyholm Nyholm approved these changes

@chalasr chalasr chalasr approved these changes

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

Successfully merging this pull request may close these issues.
6 participants
@wouterj @fabpot @Seldaek @Nyholm @chalasr @carsonbot
0