8000 feature #40284 [RateLimiter][Security] Allow to use no lock in the ra… · symfony/symfony@79f6a5c · GitHub
[go: up one dir, main page]
Skip to content

Commit 79f6a5c

Browse files
fabpotfabpot
committed
feature #40284 [RateLimiter][Security] Allow to use no lock in the rate limiter/login throttling (wouterj)
This PR was merged into the 5.3-dev branch. Discussion ---------- [RateLimiter][Security] Allow to use no lock in the rate limiter/login throttling | 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). Commits ------- 45be875 [Security][RateLimiter] Allow to use no lock in the rate limiter/login throttling
2 parents 59fbe57 + 45be875 commit 79f6a5c

File tree

8 files changed

+64
-8
lines changed

8 files changed

+64
-8
lines changed

‎UPGRADE-5.3.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,8 @@ Security
8484
SecurityBundle
8585
--------------
8686

87+
* [BC break] Add `login_throttling.lock_factory` setting defaulting to `null`. Set this option
88+
to `lock.factory` if you need precise login rate limiting with synchronous requests.
8789
* Deprecate `UserPasswordEncoderCommand` class and the corresponding `user:encode-password` command,
8890
use `UserPasswordHashCommand` and `user:hash-password` instead
8991
* Deprecate the `security.encoder_factory.generic` service, the `security.encoder_factory` and `Symfony\Component\Security\Core\Encoder\EncoderFactoryInterface` aliases,

‎src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1874,7 +1874,7 @@ private function addRateLimiterSection(ArrayNodeDefinition $rootNode, callable $
18741874
->arrayPrototype()
18751875
->children()
18761876
->scalarNode('lock_factory')
1877-
->info('The service ID of the lock factory used by this limiter')
1877+
->info('The service ID of the lock factory used by this limiter (or null to disable locking)')
18781878
->defaultValue('lock.factory')
18791879
->end()
18801880
->scalarNode('cache_pool')

‎src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ class FrameworkExtension extends Extension
205205
private $httpClientConfigEnabled = false;
206206
private $notifierConfigEnabled = false;
207207
private $propertyAccessConfigEnabled = false;
208-
private $lockConfigEnabled = false;
208+
private static $lockConfigEnabled = false;
209209

210210
/**
211211
* Responds to the app.config configuration parameter.
@@ -438,7 +438,7 @@ public function load(array $configs, ContainerBuilder $container)
438438
$this->registerPropertyInfoConfiguration($container, $loader);
439439
}
440440

441-
if ($this->lockConfigEnabled = $this->isConfigEnabled($container, $config['lock'])) {
441+
if (self::$lockConfigEnabled = $this->isConfigEnabled($container, $config['lock'])) {
442442
$this->registerLockConfiguration($config['lock'], $container, $loader);
443443
}
444444

@@ -2344,10 +2344,6 @@ private function registerNotifierConfiguration(array $config, ContainerBuilder $
23442344

23452345
private function registerRateLimiterConfiguration(array $config, ContainerBuilder $container, PhpFileLoader $loader)
23462346
{
2347-
if (!$this->lockConfigEnabled) {
2348-
throw new LogicException('Rate limiter support cannot be enabled without enabling the Lock component.');
2349-
}
2350-
23512347
$loader->load('rate_limiter.php');
23522348

23532349
foreach ($config['limiters'] as $name => $limiterConfig) {
@@ -2362,7 +2358,13 @@ public static function registerRateLimiter(ContainerBuilder $container, string $
23622358

23632359
$limiter = $container->setDefinition($limiterId = 'limiter.'.$name, new ChildDefinition('limiter'));
23642360

2365-
$limiter->addArgument(new Reference($limiterConfig['lock_factory']));
2361+
if (null !== $limiterConfig['lock_factory']) {
2362+
if (!self::$lockConfigEnabled) {
2363+
throw new LogicException(sprintf('Rate limiter "%s" requires the Lock component to be installed and configured.', $name));
2364+
}
2365+
2366+
$limiter->replaceArgument(2, new Reference($limiterConfig['lock_factory']));
2367+
}
23662368
unset($limiterConfig['lock_factory']);
23672369

23682370
$storageId = $limiterConfig['storage_service'] ?? null;

‎src/Symfony/Bundle/FrameworkBundle/Resources/config/rate_limiter.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
->args([
2525
abstract_arg('config'),
2626
abstract_arg('storage'),
27+
null,
2728
])
2829
;
2930
};

‎src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/PhpFrameworkExtensionTest.php

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313

1414
use Symfony\Component\Config\FileLocator;
1515
use Symfony\Component\DependencyInjection\ContainerBuilder;
16+
use Symfony\Component\DependencyInjection\Exception\LogicException;
17+
use Symfony\Component\DependencyInjection\Exception\OutOfBoundsException;
1618
use Symfony\Component\DependencyInjection\Loader\PhpFileLoader;
1719
use Symfony\Component\Workflow\Exception\InvalidDefinitionException;
1820

@@ -82,4 +84,49 @@ public function testWorkflowValidationStateMachine()
8284
]);
8385
});
8486
}
87+
88+
public function testRateLimiterWithLockFactory()
89+
{
90+
try {
91+
$this->createContainerFromClosure(function (ContainerBuilder $container) {
92+
$container->loadFromExtension('framework', [
93+
'rate_limiter' => [
94+
'with_lock' => ['policy' => 'fixed_window', 'limit' => 10, 'interval' => '1 hour'],
95+
],
96+
]);
97+
});
98+
99+
$this->fail('No LogicException thrown');
100+
} catch (LogicException $e) {
101+
$this->assertEquals('Rate limiter "with_lock" requires the Lock component to be installed and configured.', $e->getMessage());
102+
}
103+
104+
$container = $this->createContainerFromClosure(function (ContainerBuilder $container) {
105+
$container->loadFromExtension('framework', [
106+
'lock' => true,
107+
'rate_limiter' => [
108+
'with_lock' => ['policy' => 'fixed_window', 'limit' => 10, 'interval' => '1 hour'],
109+
],
110+
]);
111+
});
112+
113+
$withLock = $container->getDefinition('limiter.with_lock');
114+
$this->assertEquals('lock.factory', (string) $withLock->getArgument(2));
115+
}
116+
117+
public function testRateLimiterLockFactory()
118+
{
119+
$container = $this->createContainerFromClosure(function (ContainerBuilder $container) {
120+
$container->loadFromExtension('framework', [
121+
'rate_limiter' => [
122+
'without_lock' => ['policy' => 'fixed_window', 'limit' => 10, 'interval' => '1 hour', 'lock_factory' => null],
123+
],
124+
]);
125+
});
126+
127+
$this->expectException(OutOfBoundsException::class);
128+
$this->expectExceptionMessage('The argument "2" doesn\'t exist.');
129+
130+
$container->getDefinition('limiter.without_lock')->getArgument(2);
131+
}
85132
}

‎src/Symfony/Bundle/FrameworkBundle/composer.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
"symfony/messenger": "^5.2",
5252
"symfony/mime": "^4.4|^5.0",
5353
"symfony/process": "^4.4|^5.0",
54+
"symfony/rate-limiter": "^5.2",
5455
"symfony/security-bundle": "^5.3",
5556
"symfony/serializer": "^5.2",
5657
"symfony/stopwatch": "^4.4|^5.0",

‎src/Symfony/Bundle/SecurityBundle/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ CHANGELOG
44
5.3
55
---
66

7+
* [BC break] Add `login_throttling.lock_factory` setting defaulting to `null` (instead of `lock.factory`)
78
* Add the `debug:firewall` command.
89
* Deprecate `UserPasswordEncoderCommand` class and the corresponding `user:encode-password` command,
910
use `UserPasswordHashCommand` and `user:hash-password` instead

‎src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/LoginThrottlingFactory.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ public function addConfiguration(NodeDefinition $builder)
5454
->children()
5555
->scalarNode('limiter')->info(sprintf('A service id implementing "%s".', RequestRateLimiterInterface::class))->end()
5656
->integerNode('max_attempts')->defaultValue(5)->end()
57+
->scalarNode('lock_factory')->info('The service ID of the lock factory used by the login rate limiter (or null to disable locking)')->defaultNull()->end()
5758
->end();
5859
}
5960

@@ -76,6 +77,7 @@ public function createAuthenticator(ContainerBuilder $container, string $firewal
7677
'policy' => 'fixed_window',
7778
'limit' => $config['max_attempts'],
7879
'interval' => '1 minute',
80+
'lock_factory' => $config['lock_factory'],
7981
];
8082
FrameworkExtension::registerRateLimiter($container, $localId = '_login_local_'.$firewallName, $limiterOptions);
8183

0 commit comments

Comments
 (0)
0