8000 Add PeekableRequestRateLimiterInterface and fix the LoginThrottlingLi… · symfony/symfony@a4005d5 · GitHub
[go: up one dir, main page]
Skip to content

Commit a4005d5

Browse files
SeldaekSeldaek
committed
Add PeekableRequestRateLimiterInterface and fix the LoginThrottlingListener to reduce amount of writes on the cache backend, fixes #40371 
1 parent 4c2d670 commit a4005d5

File tree

4 files changed

+81
-17
lines changed

4 files changed

+81
-17
lines changed

‎src/Symfony/Component/HttpFoundation/RateLimiter/AbstractRequestRateLimiter.php

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,24 @@
1717
use Symfony\Component\RateLimiter\RateLimit;
1818

1919
/**
20-
* An implementation of RequestRateLimiterInterface that
20+
* An implementation of PeekableRequestRateLimiterInterface that
2121
* fits most use-cases.
2222
*
2323
* @author Wouter de Jong <wouter@wouterj.nl>
2424
*/
25-
abstract class AbstractRequestRateLimiter implements RequestRateLimiterInterface
25+
abstract class AbstractRequestRateLimiter implements PeekableRequestRateLimiterInterface
2626
{
2727
public function consume(Request $request): RateLimit
28+
{
29+
return $this->doConsume($request, 1);
30+
}
31+
32+
public function peek(Request $request): RateLimit
33+
{
34+
return $this->doConsume($request, 0);
35+
}
36+
37+
private function doConsume(Request $request, int $tokens)
2838
{
2939
$limiters = $this->getLimiters($request);
3040
if (0 === \count($limiters)) {
@@ -33,7 +43,7 @@ public function consume(Request $request): RateLimit
3343

3444
$minimalRateLimit = null;
3545
foreach ($limiters as $limiter) {
36-
$rateLimit = $limiter->consume(1);
46+
$rateLimit = $limiter->consume($tokens);
3747

3848
if (null === $minimalRateLimit || $rateLimit->getRemainingTokens() < $minimalRateLimit->getRemainingTokens()) {
3949
$minimalRateLimit = $rateLimit;

‎src/Symfony/Component/HttpFoundation/RateLimiter/PeekableRequestRateLimiterInterface.php

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\HttpFoundation\RateLimiter;
13+
14+
use Symfony\Component\HttpFoundation\Request;
15+
use Symfony\Component\RateLimiter\RateLimit;
16+
17+
/**
18+
* A request limiter which allows peeking ahead.
19+
*
20+
* This is valuable to reduce the cache backend load in scenarios
21+
* like a login when we only want to consume a token on login failure,
22+
* and where the majority of requests will be successful and thus not
23+
* need to consume a token.
24+
*
25+
* This way we can peek ahead before allowing the request through, and
26+
* only consume if the request failed (1 backend op). This is compared
27+
* to always consuming and then resetting the limit if the request
28+
* is successful (2 backend ops).
29+
*
30+
* @author Jordi Boggiano <j.boggiano@seld.be>
31+
*/
32+
interface PeekableRequestRateLimiterInterface extends RequestRateLimiterInterface
33+
{
34+
public function peek(Request $request): RateLimit;
35+
}

‎src/Symfony/Component/Security/Http/EventListener/LoginThrottlingListener.php

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,14 @@
1212
namespace Symfony\Component\Security\Http\EventListener;
1313

1414
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
15+
use Symfony\Component\HttpFoundation\RateLimiter\PeekableRequestRateLimiterInterface;
1516
use Symfony\Component\HttpFoundation\RateLimiter\RequestRateLimiterInterface;
1617
use Symfony\Component\HttpFoundation\RequestStack;
1718
use Symfony\Component\Security\Core\Exception\TooManyLoginAttemptsAuthenticationException;
1819
use Symfony\Component\Security\Core\Security;
1920
use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
2021
use Symfony\Component\Security\Http\Event\CheckPassportEvent;
22+
use Symfony\Component\Security\Http\Event\LoginFailureEvent;
2123
use Symfony\Component\Security\Http\Event\LoginSuccessEvent;
2224

2325
/**
@@ -44,21 +46,41 @@ public function checkPassport(CheckPassportEvent $event): void
4446
$request = $this->requestStack->getMainRequest();
4547
$request->attributes->set(Security::LAST_USERNAME, $passport->getBadge(UserBadge::class)->getUserIdentifier());
4648

47-
$limit = $this->limiter->consume($request);
48-
if (!$limit->isAccepted()) {
49-
throw new TooManyLoginAttemptsAuthenticationException(ceil(($limit->getRetryAfter()->getTimestamp() - time()) / 60));
49+
if ($this->limiter instanceof PeekableRequestRateLimiterInterface) {
50+
$limit = $this->limiter->peek($request);
51+
// Checking isAccepted here is not enough as peek consumes 0 token, it will
52+
// be accepted even if there are 0 tokens remaining to be consumed. We check both
53+
// anyway for safety in case third party implementations behave unexpectedly.
54+
if (!$limit->isAccepted() || 0 === $limit->getRemainingTokens()) {
55+
throw new TooManyLoginAttemptsAuthenticationException(ceil(($limit->getRetryAfter()->getTimestamp() - time()) / 60));
56+
}
57+
} else {
58+
$limit = $this->limiter->consume($request);
59+
if (!$limit->isAccepted()) {
60+
throw new TooManyLoginAttemptsAuthenticationException(ceil(($limit->getRetryAfter()->getTimestamp() - time()) / 60));
61+
}
5062
}
5163
}
5264

5365
public function onSuccessfulLogin(LoginSuccessEvent $event): void
5466
{
55-
$this->limiter->reset($event->getRequest());
67+
if (!$this->limiter instanceof PeekableRequestRateLimiterInterface) {
68+
$this->limiter->reset($event->getRequest());
69+
}
70+
}
71+
72+
public function onFailedLogin(LoginFailureEvent $event): void
73+
{
74+
if ($this->limiter instanceof PeekableRequestRateLimiterInterface) {
75+
$this->limiter->consume($event->getRequest());
76+
}
5677
}
5778

5879
public static function getSubscribedEvents(): array
5980
{
6081
return [
6182
CheckPassportEvent::class => ['checkPassport', 2080],
83+
LoginFailureEvent::class => 'onFailedLogin',
6284
LoginSuccessEvent::class => 'onSuccessfulLogin',
6385
];
6486
}

‎src/Symfony/Component/Security/Http/Tests/EventListener/LoginThrottlingListenerTest.php

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,13 @@
1616
use Symfony\Component\HttpFoundation\RequestStack;
1717
use Symfony\Component\RateLimiter\RateLimiterFactory;
1818
use Symfony\Component\RateLimiter\Storage\InMemoryStorage;
19-
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
19+
use Symfony\Component\Security\Core\Exception\AuthenticationException;
2020
use Symfony\Component\Security\Core\Exception\TooManyLoginAttemptsAuthenticationException;
2121
use Symfony\Component\Security\Http\Authenticator\AuthenticatorInterface;
2222
use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
2323
use Symfony\Component\Security\Http\Authenticator\Passport\SelfValidatingPassport;
2424
use Symfony\Component\Security\Http\Event\CheckPassportEvent;
25-
use Symfony\Component\Security\Http\Event\LoginSuccessEvent;
25+
use Symfony\Component\Security\Http\Event\LoginFailureEvent;
2626
use Symfony\Component\Security\Http\EventListener\LoginThrottlingListener;
2727
use Symfony\Component\Security\Http\RateLimiter\DefaultLoginRateLimiter;
2828

@@ -61,12 +61,7 @@ public function testPreventsLoginWhenOverLocalThreshold()
6161

6262
for ($i = 0; $i < 3; ++$i) {
6363
$this->listener->checkPassport($this->createCheckPassportEvent($passport));
64-
}
65-
66-
$this->listener->onSuccessfulLogin($this->createLoginSuccessfulEvent($passport));
67-
68-
for ($i = 0; $i < 3; ++$i) {
69-
$this->listener->checkPassport($this->createCheckPassportEvent($passport));
64+
$this->listener->onFailedLogin($this->createLoginFailedEvent($passport));
7065
}
7166

7267
$this->expectException(TooManyLoginAttemptsAuthenticationException::class);
@@ -82,6 +77,7 @@ public function testPreventsLoginWithMultipleCase()
8277

8378
for ($i = 0; $i < 3; ++$i) {
8479
$this->listener->checkPassport($this->createCheckPassportEvent($passports[$i % 3]));
80+
$this->listener->onFailedLogin($this->createLoginFailedEvent($passports[$i % 3]));
8581
}
8682

8783
$this->expectException(TooManyLoginAttemptsAuthenticationException::class);
@@ -97,6 +93,7 @@ public function testPreventsLoginWhenOverGlobalThreshold()
9793

9894
for ($i = 0; $i < 6; ++$i) {
9995
$this->listener->checkPassport($this->createCheckPassportEvent($passports[$i % 2]));
96+
$this->listener->onFailedLogin($this->createLoginFailedEvent($passports[$i % 2]));
10097
}
10198

10299
$this->expectException(TooManyLoginAttemptsAuthenticationException::class);
@@ -108,9 +105,9 @@ private function createPassport($username)
108105
return new SelfValidatingPassport(new UserBadge($username));
109106
}
110107

111-
private function createLoginSuccessfulEvent($passport)
108+
private function createLoginFailedEvent($passport)
112109
{
113-
return new LoginSuccessEvent($this->createMock(AuthenticatorInterface::class), $passport, $this->createMock(TokenInterface::class), $this->requestStack->getCurrentRequest(), null, 'main');
110+
return new LoginFailureEvent($this->createMock(AuthenticationException::class), $this->createMock(AuthenticatorInterface::class), $this->requestStack->getCurrentRequest(), null, 'main', $passport);
114111
}
115112

116113
private function createCheckPassportEvent($passport)

0 commit comments

Comments
 (0)
0