symfony · fabpot · Oct 1, 2020 · Sep 26, 2020
diff --git a/...ony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/LoginThrottlingFactory.php b/...ony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/LoginThrottlingFactory.php
@@ -17,8 +17,10 @@
 use Symfony\Component\DependencyInjection\ChildDefinition;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\HttpFoundation\RateLimiter\RequestRateLimiterInterface;
 use Symfony\Component\RateLimiter\Limiter;
 use Symfony\Component\Security\Http\EventListener\LoginThrottlingListener;
+use Symfony\Component\Security\Http\RateLimiter\DefaultLoginRateLimiter;
 
 /**
  * @author Wouter de Jong <wouter@wouterj.nl>
@@ -50,7 +52,7 @@ public function addConfiguration(NodeDefinition $builder)
     {
         $builder
             ->children()
-                ->scalarNode('limiter')->info('The name of the limiter that you defined under "framework.rate_limiter".')->end()
+                ->scalarNode('limiter')->info(sprintf('A service id implementing "%s".', RequestRateLimiterInterface::class))->end()
                 ->integerNode('max_attempts')->defaultValue(5)->end()
             ->end();
     }
@@ -70,18 +72,27 @@ public function createAuthenticator(ContainerBuilder $container, string $firewal
                 throw new \LogicException('You must either configure a rate limiter for "security.firewalls.'.$firewallName.'.login_throttling" or install symfony/framework-bundle:^5.2.');
             }
 
-            FrameworkExtension::registerRateLimiter($container, $config['limiter'] = '_login_'.$firewallName, [
+            $limiterOptions = [
                 'strategy' => 'fixed_window',
                 'limit' => $config['max_attempts'],
                 'interval' => '1 minute',
                 'lock_factory' => 'lock.factory',
                 'cache_pool' => 'cache.app',
-            ]);
+            ];
+            FrameworkExtension::registerRateLimiter($container, $localId = '_login_local_'.$firewallName, $limiterOptions);
+
+            $limiterOptions['limit'] = 5 * $config['max_attempts'];
+            FrameworkExtension::registerRateLimiter($container, $globalId = '_login_global_'.$firewallName, $limiterOptions);
+
+            $container->register($config['limiter'] = 'security.login_throttling.'.$firewallName.'.limiter', DefaultLoginRateLimiter::class)
+                ->addArgument(new Reference('limiter.'.$globalId))
+                ->addArgument(new Reference('limiter.'.$localId))
+            ;
         }
 
         $container
             ->setDefinition('security.listener.login_throttling.'.$firewallName, new ChildDefinition('security.listener.login_throttling'))
-            ->replaceArgument(1, new Reference('limiter.'.$config['limiter']))
+            ->replaceArgument(1, new Reference($config['limiter']))
             ->addTag('kernel.event_subscriber', ['dispatcher' => 'security.event_dispatcher.'.$firewallName]);
 
         return [];

diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator.php b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator.php
@@ -118,7 +118,7 @@
             ->abstract()
             ->args([
                 service('request_stack'),
-                abstract_arg('rate limiter'),
+                abstract_arg('request rate limiter'),
             ])
 
         // Authenticators

diff --git a/...urityBundle/Tests/Functional/Bundle/FormLoginBundle/Resources/views/Login/login.html.twig b/...urityBundle/Tests/Functional/Bundle/FormLoginBundle/Resources/views/Login/login.html.twig
@@ -4,6 +4,7 @@
 
     {% if error %}
         <div>{{ error.messageKey }}</div>
+        <div>{{ error.messageKey|replace(error.messageData) }}</div>
     {% endif %}
 
     <form action="{{ path('form_login_check') }}" method="post">

diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php
@@ -108,7 +108,10 @@ public function testFormLoginRedirectsToProtectedResourceAfterLogin(array $optio
         $this->assertStringContainsString('You\'re browsing to path "/protected_resource".', $text);
     }
 
-    public function testLoginThrottling()
+    /**
+     * @dataProvider provideInvalidCredentials
+     */
+    public function testLoginThrottling($username, $password)
     {
         if (!class_exists(LoginThrottlingListener::class)) {
             $this->markTestSkipped('Login throttling requires symfony/security-http:^5.2');
@@ -117,17 +120,23 @@ public function testLoginThrottling()
         $client = $this->createClient(['test_case' => 'StandardFormLogin', 'root_config' => 'login_throttling.yml', 'enable_authenticator_manager' => true]);
 
         $form = $client->request('GET', '/login')->selectButton('login')->form();
-        $form['_username'] = 'johannes';
-        $form['_password'] = 'wrong';
+        $form['_username'] = $username;
+        $form['_password'] = $password;
         $client->submit($form);
 
         $client->followRedirect()->selectButton('login')->form();
-        $form['_username'] = 'johannes';
-        $form['_password'] = 'wrong';
+        $form['_username'] = $username;
+        $form['_password'] = $password;
         $client->submit($form);
 
         $text = $client->followRedirect()->text(null, true);
-        $this->assertStringContainsString('Too many failed login attempts, please try again later.', $text);
+        $this->assertStringContainsString('Too many failed login attempts, please try again in 1 minute.', $text);
+    }
+
+    public function provideInvalidCredentials()
+    {
+        yield 'invalid_password' => ['johannes', 'wrong'];
+        yield 'invalid_username' => ['wrong', 'wrong'];
     }
 
     public function provideClientOptions()

diff --git a/src/Symfony/Component/HttpFoundation/CHANGELOG.md b/src/Symfony/Component/HttpFoundation/CHANGELOG.md
@@ -8,6 +8,7 @@ CHANGELOG
  * added `HeaderUtils::parseQuery()`: it does the same as `parse_str()` but preserves dots in variable names
  * added `File::getContent()`
  * added ability to use comma separated ip addresses for `RequestMatcher::matchIps()`
+ * added `RateLimiter\RequestRateLimiterInterface` and `RateLimiter\AbstractRequestRateLimiter`
 
 5.1.0
 -----

diff --git a/src/Symfony/Component/HttpFoundation/RateLimiter/AbstractRequestRateLimiter.php b/src/Symfony/Component/HttpFoundation/RateLimiter/AbstractRequestRateLimiter.php
@@ -0,0 +1,59 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpFoundation\RateLimiter;
+
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\RateLimiter\Limit;
+use Symfony\Component\RateLimiter\LimiterInterface;
+use Symfony\Component\RateLimiter\NoLimiter;
+
+/**
+ * An implementation of RequestRateLimiterInterface that
+ * fits most use-cases.
+ *
+ * @author Wouter de Jong <wouter@wouterj.nl>
+ *
+ * @experimental in Symfony 5.2
+ */
+abstract class AbstractRequestRateLimiter implements RequestRateLimiterInterface
+{
+    public function consume(Request $request): Limit
+    {
+        $limiters = $this->getLimiters($request);
+        if (0 === \count($limiters)) {
+            $limiters = [new NoLimiter()];
+        }
+
+        $minimalLimit = null;
+        foreach ($limiters as $limiter) {
+            $limit = $limiter->consume(1);
+
+            if (null === $minimalLimit || $limit->getRemainingTokens() < $minimalLimit->getRemainingTokens()) {
+                $minimalLimit = $limit;
+            }
+        }
+
+        return $minimalLimit;
+    }
+
+    public function reset(): void
+    {
+        foreach ($this->getLimiters($request) as $limiter) {
+            $limiter->reset();
+        }
+    }
+
+    /**
+     * @return LimiterInterface[] a set of limiters using keys extracted from the request
+     */
+    abstract protected function getLimiters(Request $request): array;
+}
diff --git a/src/Symfony/Component/HttpFoundation/RateLimiter/RequestRateLimiterInterface.php b/src/Symfony/Component/HttpFoundation/RateLimiter/RequestRateLimiterInterface.php
@@ -0,0 +1,32 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpFoundation\RateLimiter;
+
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\RateLimiter\Limit;
+
+/**
+ * A special type of limiter that deals with requests.
+ *
+ * This allows to limit on different types of information
+ * from the requests.
+ *
+ * @author Wouter de Jong <wouter@wouterj.nl>
+ *
+ * @experimental in Symfony 5.2
+ */
+interface RequestRateLimiterInterface
+{
+    public function consume(Request $request): Limit;
+
+    public function reset(): void;
+}
diff --git a/src/Symfony/Component/RateLimiter/NoLimiter.php b/src/Symfony/Component/RateLimiter/NoLimiter.php
@@ -25,7 +25,7 @@ final class NoLimiter implements LimiterInterface
 {
     public function consume(int $tokens = 1): Limit
     {
-        return new Limit(\INF, new \DateTimeImmutable(), true, 'no_limit');
+        return new Limit(\INF, new \DateTimeImmutable(), true);
     }
 
     public function reset(): void

diff --git a/...Symfony/Component/Security/Core/Exception/TooManyLoginAttemptsAuthenticationException.php b/...Symfony/Component/Security/Core/Exception/TooManyLoginAttemptsAuthenticationException.php
@@ -19,11 +19,46 @@
  */
 class TooManyLoginAttemptsAuthenticationException extends AuthenticationException
 {
+    private $threshold;
+
+    public function __construct(int $threshold = null)
+    {
+        $this->threshold = $threshold;
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function getMessageData(): array
+    {
+        return [
+            '%minutes%' => $this->threshold,
+        ];
+    }
+
     /**
      * {@inheritdoc}
      */
     public function getMessageKey(): string
     {
-        return 'Too many failed login attempts, please try again later.';
+        return 'Too many failed login attempts, please try again '.($this->threshold ? 'in %minutes% minute'.($this->threshold > 1 ? 's' : '').'.' : 'later.');
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function __serialize(): array
+    {
+        return [$this->threshold, parent::__serialize()];
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function __unserialize(array $data): void
+    {
+        [$this->threshold, $parentData] = $data;
+        $parentData = \is_array($parentData) ? $parentData : unserialize($parentData);
+        parent::__unserialize($parentData);
     }
 }
diff --git a/src/Symfony/Component/Security/Http/EventListener/LoginThrottlingListener.php b/src/Symfony/Component/Security/Http/EventListener/LoginThrottlingListener.php
@@ -12,13 +12,12 @@
 namespace Symfony\Component\Security\Http\EventListener;
 
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
-use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\RateLimiter\RequestRateLimiterInterface;
 use Symfony\Component\HttpFoundation\RequestStack;
-use Symfony\Component\RateLimiter\Limiter;
 use Symfony\Component\Security\Core\Exception\TooManyLoginAttemptsAuthenticationException;
+use Symfony\Component\Security\Core\Security;
 use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
 use Symfony\Component\Security\Http\Event\CheckPassportEvent;
-use Symfony\Component\Security\Http\Event\LoginSuccessEvent;
 
 /**
  * @author Wouter de Jong <wouter@wouterj.nl>
@@ -30,7 +29,7 @@ final class LoginThrottlingListener implements EventSubscriberInterface
     private $requestStack;
     private $limiter;
 
-    public function __construct(RequestStack $requestStack, Limiter $limiter)
+    public function __construct(RequestStack $requestStack, RequestRateLimiterInterface $limiter)
     {
         $this->requestStack = $requestStack;
         $this->limiter = $limiter;
@@ -44,33 +43,18 @@ public function checkPassport(CheckPassportEvent $event): void
         }
 
         $request = $this->requestStack->getMasterRequest();
-        $username = $passport->getBadge(UserBadge::class)->getUserIdentifier();
-        $limiterKey = $this->createLimiterKey($username, $request);
+        $request->attributes->set(Security::LAST_USERNAME, $passport->getBadge(UserBadge::class)->getUserIdentifier());
 
-        $limiter = $this->limiter->create($limiterKey);
-        if (!$limiter->consume()->isAccepted()) {
-            throw new TooManyLoginAttemptsAuthenticationException();
+        $limit = $this->limiter->consume($request);
+        if (!$limit->isAccepted()) {
+            throw new TooManyLoginAttemptsAuthenticationException(ceil(($limit->getRetryAfter()->getTimestamp() - time()) / 60));
         }
     }
 
-    public function onSuccessfulLogin(LoginSuccessEvent $event): void
-    {
-        $limiterKey = $this->createLimiterKey($event->getAuthenticatedToken()->getUsername(), $event->getRequest());
-        $limiter = $this->limiter->create($limiterKey);
-
-        $limiter->reset();
-    }
-
     public static function getSubscribedEvents(): array
     {
         return [
-            CheckPassportEvent::class => ['checkPassport', 64],
-            LoginSuccessEvent::class => 'onSuccessfulLogin',
+            CheckPassportEvent::class => ['checkPassport', 2080],
         ];
     }
-
-    private function createLimiterKey($username, Request $request): string
-    {
-        return $username.$request->getClientIp();
-    }
 }
diff --git a/src/Symfony/Component/Security/Http/RateLimiter/DefaultLoginRateLimiter.php b/src/Symfony/Component/Security/Http/RateLimiter/DefaultLoginRateLimiter.php
@@ -0,0 +1,47 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Security\Http\RateLimiter;
+
+use Symfony\Component\HttpFoundation\RateLimiter\AbstractRequestRateLimiter;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\RateLimiter\Limiter;
+use Symfony\Component\Security\Core\Security;
+
+/**
+ * A default login throttling limiter.
+ *
+ * This limiter prevents breadth-first attacks by enforcing
+ * a limit on username+IP and a (higher) limit on IP.
+ *
+ * @author Wouter de Jong <wouter@wouterj.nl>
+ *
+ * @experimental in Symfony 5.2
+ */
+final class DefaultLoginRateLimiter extends AbstractRequestRateLimiter
+{
+    private $globalLimiter;
+    private $localLimiter;
+
+    public function __construct(Limiter $globalLimiter, Limiter $localLimiter)
+    {
+        $this->globalLimiter = $globalLimiter;
+        $this->localLimiter = $localLimiter;
+    }
+
+    protected function getLimiters(Request $request): array
+    {
+        return [
+            $this->globalLimiter->create($request->getClientIp()),
+            $this->localLimiter->create($request->attributes->get(Security::LAST_USERNAME).$request->getClientIp()),
+        ];
+    }
+}