Merge branch '5.2' into 5.x

nicolas-grekas · nicolas-grekas · commit 8fb0ed752ecf · 2021-05-12T15:40:11.000+02:00
* 5.2:
  [CI][Psalm] Install stable/released PHPUnit
  [Security] Add missing Finnish translations
  [Security][Guard] Prevent user enumeration via response content
diff --git a/.github/workflows/psalm.yml b/.github/workflows/psalm.yml
@@ -39,7 +39,7 @@ jobs:
         run: |
           echo "::group::modify composer.json"
           composer remove --no-update --no-interaction symfony/phpunit-bridge
-          composer require --no-update psalm/phar phpunit/phpunit php-http/discovery psr/event-dispatcher
+          composer require --no-update psalm/phar phpunit/phpunit:@stable php-http/discovery psr/event-dispatcher
           echo "::endgroup::"
           echo "::group::composer update"
           composer update --no-progress --ansi
diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php
@@ -502,7 +502,7 @@ private function createFirewall(ContainerBuilder $container, string $id, array $
                 ->replaceArgument(0, $authenticators)
                 ->replaceArgument(2, new Reference($firewallEventDispatcherId))
                 ->replaceArgument(3, $id)
-                ->replaceArgument(6, $firewall['required_badges'] ?? [])
+                ->replaceArgument(7, $firewall['required_badges'] ?? [])
                 ->addTag('monolog.logger', ['channel' => 'security'])
             ;
 
diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/guard.php b/src/Symfony/Bundle/SecurityBundle/Resources/config/guard.php
@@ -45,6 +45,7 @@
                 abstract_arg('Provider-shared Key'),
                 abstract_arg('Authenticators'),
                 service('logger')->nullOnInvalid(),
+                param('security.authentication.hide_user_not_found'),
             ])
             ->tag('monolog.logger', ['channel' => 'security'])
     ;
diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator.php b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator.php
@@ -44,6 +44,7 @@
                 abstract_arg('provider key'),
                 service('logger')->nullOnInvalid(),
                 param('security.authentication.manager.erase_credentials'),
+                param('security.authentication.hide_user_not_found'),
                 abstract_arg('required badges'),
             ])
             ->tag('monolog.logger', ['channel' => 'security'])
diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php
@@ -43,7 +43,7 @@ public function testAuthenticatorManager()
         $this->assertEquals(AuthenticatorManager::class, $authenticatorManager->getClass());
 
         // required badges
-        $this->assertEquals([CsrfTokenBadge::class, RememberMeBadge::class], $authenticatorManager->getArgument(6));
+        $this->assertEquals([CsrfTokenBadge::class, RememberMeBadge::class], $authenticatorManager->getArgument(7));
 
         // login link
         $expiredStorage = $container->getDefinition($expiredStorageId = 'security.authenticator.expired_login_link_storage.main');
diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/AuthenticatorTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/AuthenticatorTest.php
@@ -40,7 +40,7 @@ public function testFirewallUserProvider($email, $withinFirewall)
         if ($withinFirewall) {
             $this->assertJsonStringEqualsJsonString('{"email":"'.$email.'"}', $client->getResponse()->getContent());
         } else {
-            $this->assertJsonStringEqualsJsonString('{"error":"Username could not be found."}', $client->getResponse()->getContent());
+            $this->assertJsonStringEqualsJsonString('{"error":"Invalid credentials."}', $client->getResponse()->getContent());
         }
     }
 
diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php
@@ -142,7 +142,7 @@ public function testLoginThrottling()
 
                     break;
                 case 2: // Third attempt with unexisting username
-                    $this->assertStringContainsString('Username could not be found.', $text, 'Invalid response on 3rd attempt');
+                    $this->assertStringContainsString('Invalid credentials.', $text, 'Invalid response on 3rd attempt');
 
                     break;
                 case 3: // Fourth attempt : still login throttling !
diff --git a/src/Symfony/Component/Security/Core/Authentication/Provider/UserAuthenticationProvider.php b/src/Symfony/Component/Security/Core/Authentication/Provider/UserAuthenticationProvider.php
@@ -14,6 +14,7 @@
 use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
+use Symfony\Component\Security\Core\Exception\AccountStatusException;
 use Symfony\Component\Security\Core\Exception\AuthenticationException;
 use Symfony\Component\Security\Core\Exception\AuthenticationServiceException;
 use Symfony\Component\Security\Core\Exception\BadCredentialsException;
@@ -79,7 +80,7 @@ public function authenticate(TokenInterface $token)
             $this->userChecker->checkPreAuth($user);
             $this->checkAuthentication($user, $token);
             $this->userChecker->checkPostAuth($user);
-        } catch (BadCredentialsException $e) {
+        } catch (AccountStatusException $e) {
             if ($this->hideUserNotFoundExceptions) {
                 throw new BadCredentialsException('Bad credentials.', 0, $e);
             }
diff --git a/src/Symfony/Component/Security/Core/Resources/translations/security.fi.xlf b/src/Symfony/Component/Security/Core/Resources/translations/security.fi.xlf
@@ -70,6 +70,14 @@
                 <source>Invalid or expired login link.</source>
                 <target>Virheellinen tai vanhentunut kirjautumislinkki.</target>
             </trans-unit>
+            <trans-unit id="19">
+                <source>Too many failed login attempts, please try again in %minutes% minute.</source>
+                <target>Liian monta epäonnistunutta kirjautumisyritystä, yritä uudelleen %minutes% minuutin kuluttua.</target>
+            </trans-unit>
+            <trans-unit id="20">
+                <source>Too many failed login attempts, please try again in %minutes% minutes.</source>
+                <target>Liian monta epäonnistunutta kirjautumisyritystä, yritä uudelleen %minutes% minuutin kuluttua.</target>
+            </trans-unit>
         </body>
     </file>
 </xliff>
diff --git a/src/Symfony/Component/Security/Core/Tests/Authentication/Provider/UserAuthenticationProviderTest.php b/src/Symfony/Component/Security/Core/Tests/Authentication/Provider/UserAuthenticationProviderTest.php
@@ -83,7 +83,7 @@ public function testAuthenticateWhenProviderDoesNotReturnAnUserInterface()
 
     public function testAuthenticateWhenPreChecksFails()
     {
-        $this->expectException(CredentialsExpiredException::class);
+        $this->expectException(BadCredentialsException::class);
         $userChecker = $this->createMock(UserCheckerInterface::class);
         $userChecker->expects($this->once())
                     ->method('checkPreAuth')
@@ -101,7 +101,7 @@ public function testAuthenticateWhenPreChecksFails()
 
     public function testAuthenticateWhenPostChecksFails()
     {
-        $this->expectException(AccountExpiredException::class);
+        $this->expectException(BadCredentialsException::class);
         $userChecker = $this->createMock(UserCheckerInterface::class);
         $userChecker->expects($this->once())
                     ->method('checkPostAuth')
@@ -128,7 +128,7 @@ public function testAuthenticateWhenPostCheckAuthenticationFails()
         ;
         $provider->expects($this->once())
                  ->method('checkAuthentication')
-                 ->willThrowException(new BadCredentialsException())
+                 ->willThrowException(new CredentialsExpiredException())
         ;
 
         $provider->authenticate($this->getSupportedToken());
diff --git a/src/Symfony/Component/Security/Guard/Firewall/GuardAuthenticationListener.php b/src/Symfony/Component/Security/Guard/Firewall/GuardAuthenticationListener.php
@@ -17,7 +17,11 @@
 use Symfony\Component\HttpKernel\Event\RequestEvent;
 use Symfony\Component\Security\Core\Authentication\AuthenticationManagerInterface;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
+use Symfony\Component\Security\Core\Exception\AccountStatusException;
 use Symfony\Component\Security\Core\Exception\AuthenticationException;
+use Symfony\Component\Security\Core\Exception\BadCredentialsException;
+use Symfony\Component\Security\Core\Exception\CustomUserMessageAccountStatusException;
+use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
 use Symfony\Component\Security\Guard\AuthenticatorInterface;
 use Symfony\Component\Security\Guard\GuardAuthenticatorHandler;
 use Symfony\Component\Security\Guard\Token\PreAuthenticationGuardToken;
@@ -40,12 +44,13 @@ class GuardAuthenticationListener extends AbstractListener
     private $guardAuthenticators;
     private $logger;
     private $rememberMeServices;
+    private $hideUserNotFoundExceptions;
 
     /**
      * @param string                            $providerKey         The provider (i.e. firewall) key
      * @param iterable|AuthenticatorInterface[] $guardAuthenticators The authenticators, with keys that match what's passed to GuardAuthenticationProvider
      */
-    public function __construct(GuardAuthenticatorHandler $guardHandler, AuthenticationManagerInterface $authenticationManager, string $providerKey, iterable $guardAuthenticators, LoggerInterface $logger = null)
+    public function __construct(GuardAuthenticatorHandler $guardHandler, AuthenticationManagerInterface $authenticationManager, string $providerKey, iterable $guardAuthenticators, LoggerInterface $logger = null, bool $hideUserNotFoundExceptions = true)
     {
         if (empty($providerKey)) {
             throw new \InvalidArgumentException('$providerKey must not be empty.');
@@ -56,6 +61,7 @@ public function __construct(GuardAuthenticatorHandler $guardHandler, Authenticat
         $this->providerKey = $providerKey;
         $this->guardAuthenticators = $guardAuthenticators;
         $this->logger = $logger;
+        $this->hideUserNotFoundExceptions = $hideUserNotFoundExceptions;
     }
 
     /**
@@ -160,6 +166,12 @@ private function executeGuardAuthenticator(string $uniqueGuardKey, Authenticator
                 $this->logger->info('Guard authentication failed.', ['exception' => $e, 'authenticator' => \get_class($guardAuthenticator)]);
             }
 
+            // Avoid leaking error details in case of invalid user (e.g. user not found or invalid account status)
+            // to prevent user enumeration via response content
+            if ($this->hideUserNotFoundExceptions && ($e instanceof UsernameNotFoundException || ($e instanceof AccountStatusException && !$e instanceof CustomUserMessageAccountStatusException))) {
+                $e = new BadCredentialsException('Bad credentials.', 0, $e);
+            }
+
             $response = $this->guardHandler->handleAuthenticationFailure($e, $request, $guardAuthenticator, $this->providerKey);
 
             if ($response instanceof Response) {
diff --git a/src/Symfony/Component/Security/Guard/Tests/Firewall/GuardAuthenticationListenerTest.php b/src/Symfony/Component/Security/Guard/Tests/Firewall/GuardAuthenticationListenerTest.php
@@ -19,6 +19,9 @@
 use Symfony\Component\Security\Core\Authentication\AuthenticationProviderManager;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Exception\AuthenticationException;
+use Symfony\Component\Security\Core\Exception\BadCredentialsException;
+use Symfony\Component\Security\Core\Exception\LockedException;
+use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
 use Symfony\Component\Security\Guard\AuthenticatorInterface;
 use Symfony\Component\Security\Guard\Firewall\GuardAuthenticationListener;
 use Symfony\Component\Security\Guard\GuardAuthenticatorHandler;
@@ -211,6 +214,54 @@ public function testHandleCatchesAuthenticationException()
         $listener($this->event);
     }
 
+    /**
+     * @dataProvider exceptionsToHide
+     */
+    public function testHandleHidesInvalidUserExceptions(AuthenticationException $exceptionToHide)
+    {
+        $authenticator = $this->createMock(AuthenticatorInterface::class);
+        $providerKey = 'my_firewall2';
+
+        $authenticator
+            ->expects($this->once())
+            ->method('supports')
+            ->willReturn(true);
+        $authenticator
+            ->expects($this->once())
+            ->method('getCredentials')
+            ->willReturn(['username' => 'robin', 'password' => 'hood']);
+
+        $this->authenticationManager
+            ->expects($this->once())
+            ->method('authenticate')
+            ->willThrowException($exceptionToHide);
+
+        $this->guardAuthenticatorHandler
+            ->expects($this->once())
+            ->method('handleAuthenticationFailure')
+            ->with($this->callback(function ($e) use ($exceptionToHide) {
+                return $e instanceof BadCredentialsException && $exceptionToHide === $e->getPrevious();
+            }), $this->request, $authenticator, $providerKey);
+
+        $listener = new GuardAuthenticationListener(
+            $this->guardAuthenticatorHandler,
+            $this->authenticationManager,
+            $providerKey,
+            [$authenticator],
+            $this->logger
+        );
+
+        $listener($this->event);
+    }
+
+    public function exceptionsToHide()
+    {
+        return [
+            [new UsernameNotFoundException()],
+            [new LockedException()],
+        ];
+    }
+
     public function testSupportsReturnFalseSkipAuth()
     {
         $authenticator = $this->createMock(AuthenticatorInterface::class);
diff --git a/src/Symfony/Component/Security/Http/Authentication/AuthenticatorManager.php b/src/Symfony/Component/Security/Http/Authentication/AuthenticatorManager.php
@@ -18,8 +18,11 @@
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\AuthenticationEvents;
 use Symfony\Component\Security\Core\Event\AuthenticationSuccessEvent;
+use Symfony\Component\Security\Core\Exception\AccountStatusException;
 use Symfony\Component\Security\Core\Exception\AuthenticationException;
 use Symfony\Component\Security\Core\Exception\BadCredentialsException;
+use Symfony\Component\Security\Core\Exception\CustomUserMessageAccountStatusException;
+use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
 use Symfony\Component\Security\Core\User\UserInterface;
 use Symfony\Component\Security\Http\Authenticator\AuthenticatorInterface;
 use Symfony\Component\Security\Http\Authenticator\InteractiveAuthenticatorInterface;
@@ -48,19 +51,21 @@ class AuthenticatorManager implements AuthenticatorManagerInterface, UserAuthent
     private $eraseCredentials;
     private $logger;
     private $firewallName;
+    private $hideUserNotFoundExceptions;
     private $requiredBadges;
 
     /**
      * @param AuthenticatorInterface[] $authenticators
      */
-    public function __construct(iterable $authenticators, TokenStorageInterface $tokenStorage, EventDispatcherInterface $eventDispatcher, string $firewallName, ?LoggerInterface $logger = null, bool $eraseCredentials = true, array $requiredBadges = [])
+    public function __construct(iterable $authenticators, TokenStorageInterface $tokenStorage, EventDispatcherInterface $eventDispatcher, string $firewallName, ?LoggerInterface $logger = null, bool $eraseCredentials = true, bool $hideUserNotFoundExceptions = true, array $requiredBadges = [])
     {
         $this->authenticators = $authenticators;
         $this->tokenStorage = $tokenStorage;
         $this->eventDispatcher = $eventDispatcher;
         $this->firewallName = $firewallName;
         $this->logger = $logger;
         $this->eraseCredentials = $eraseCredentials;
+        $this->hideUserNotFoundExceptions = $hideUserNotFoundExceptions;
         $this->requiredBadges = $requiredBadges;
     }
 
@@ -251,6 +256,12 @@ private function handleAuthenticationFailure(AuthenticationException $authentica
             $this->logger->info('Authenticator failed.', ['exception' => $authenticationException, 'authenticator' => \get_class($authenticator)]);
         }
 
+        // Avoid leaking error details in case of invalid user (e.g. user not found or invalid account status)
+        // to prevent user enumeration via response content comparison
+        if ($this->hideUserNotFoundExceptions && ($authenticationException instanceof UsernameNotFoundException || ($authenticationException instanceof AccountStatusException && !$authenticationException instanceof CustomUserMessageAccountStatusException))) {
+            $authenticationException = new BadCredentialsException('Bad credentials.', 0, $authenticationException);
+        }
+
         $response = $authenticator->onAuthenticationFailure($request, $authenticationException);
         if (null !== $response && null !== $this->logger) {
             $this->logger->debug('The "{authenticator}" authenticator set the failure response.', ['authenticator' => \get_class($authenticator)]);
diff --git a/src/Symfony/Component/Security/Http/Tests/Authentication/AuthenticatorManagerTest.php b/src/Symfony/Component/Security/Http/Tests/Authentication/AuthenticatorManagerTest.php
@@ -19,6 +19,7 @@
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
 use Symfony\Component\Security\Core\Exception\BadCredentialsException;
+use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
 use Symfony\Component\Security\Core\User\InMemoryUser;
 use Symfony\Component\Security\Http\Authentication\AuthenticatorManager;
 use Symfony\Component\Security\Http\Authenticator\InteractiveAuthenticatorInterface;
@@ -268,6 +269,26 @@ public function testInteractiveAuthenticator()
         $this->assertSame($this->response, $response);
     }
 
+    public function testAuthenticateRequestHidesInvalidUserExceptions()
+    {
+        $invalidUserException = new UsernameNotFoundException();
+        $authenticator = $this->createMock(InteractiveAuthenticatorInterface::class);
+        $this->request->attributes->set('_security_authenticators', [$authenticator]);
+
+        $authenticator->expects($this->any())->method('authenticate')->willThrowException($invalidUserException);
+
+        $authenticator->expects($this->any())
+            ->method('onAuthenticationFailure')
+            ->with($this->equalTo($this->request), $this->callback(function ($e) use ($invalidUserException) {
+                return $e instanceof BadCredentialsException && $invalidUserException === $e->getPrevious();
+            }))
+            ->willReturn($this->response);
+
+        $manager = $this->createManager([$authenticator]);
+        $response = $manager->authenticateRequest($this->request);
+        $this->assertSame($this->response, $response);
+    }
+
     private function createAuthenticator($supports = true)
     {
         $authenticator = $this->createMock(InteractiveAuthenticatorInterface::class);
@@ -278,6 +299,6 @@ private function createAuthenticator($supports = true)
 
     private function createManager($authenticators, $firewallName = 'main', $eraseCredentials = true, array $requiredBadges = [])
     {
-        return new AuthenticatorManager($authenticators, $this->tokenStorage, $this->eventDispatcher, $firewallName, null, $eraseCredentials, $requiredBadges);
+        return new AuthenticatorManager($authenticators, $this->tokenStorage, $this->eventDispatcher, $firewallName, null, $eraseCredentials, true, $requiredBadges);
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -502,7 +502,7 @@ private function createFirewall(ContainerBuilder $container, string $id, array $`
`502`	`502`	`->replaceArgument(0, $authenticators)`
`503`	`503`	`->replaceArgument(2, new Reference($firewallEventDispatcherId))`
`504`	`504`	`->replaceArgument(3, $id)`
`505`		`- ->replaceArgument(6, $firewall['required_badges'] ?? [])`
	`505`	`+ ->replaceArgument(7, $firewall['required_badges'] ?? [])`
`506`	`506`	`->addTag('monolog.logger', ['channel' => 'security'])`
`507`	`507`	`;`
`508`	`508`
Original file line number	Diff line number	Diff line change
`@@ -45,6 +45,7 @@`
`45`	`45`	`abstract_arg('Provider-shared Key'),`
`46`	`46`	`abstract_arg('Authenticators'),`
`47`	`47`	`service('logger')->nullOnInvalid(),`
	`48`	`+ param('security.authentication.hide_user_not_found'),`
`48`	`49`	`])`
`49`	`50`	`->tag('monolog.logger', ['channel' => 'security'])`
`50`	`51`	`;`
Original file line number	Diff line number	Diff line change
`@@ -40,7 +40,7 @@ public function testFirewallUserProvider($email, $withinFirewall)`
`40`	`40`	`if ($withinFirewall) {`
`41`	`41`	`$this->assertJsonStringEqualsJsonString('{"email":"'.$email.'"}', $client->getResponse()->getContent());`
`42`	`42`	`} else {`
`43`		`- $this->assertJsonStringEqualsJsonString('{"error":"Username could not be found."}', $client->getResponse()->getContent());`
	`43`	`+ $this->assertJsonStringEqualsJsonString('{"error":"Invalid credentials."}', $client->getResponse()->getContent());`
`44`	`44`	`}`
`45`	`45`	`}`
`46`	`46`
Original file line number	Diff line number	Diff line change
`@@ -83,7 +83,7 @@ public function testAuthenticateWhenProviderDoesNotReturnAnUserInterface()`
`83`	`83`
`84`	`84`	`public function testAuthenticateWhenPreChecksFails()`
`85`	`85`	`{`
`86`		`- $this->expectException(CredentialsExpiredException::class);`
	`86`	`+ $this->expectException(BadCredentialsException::class);`
`87`	`87`	`$userChecker = $this->createMock(UserCheckerInterface::class);`
`88`	`88`	`$userChecker->expects($this->once())`
`89`	`89`	`->method('checkPreAuth')`
`@@ -101,7 +101,7 @@ public function testAuthenticateWhenPreChecksFails()`
`101`	`101`
`102`	`102`	`public function testAuthenticateWhenPostChecksFails()`
`103`	`103`	`{`
`104`		`- $this->expectException(AccountExpiredException::class);`
	`104`	`+ $this->expectException(BadCredentialsException::class);`
`105`	`105`	`$userChecker = $this->createMock(UserCheckerInterface::class);`
`106`	`106`	`$userChecker->expects($this->once())`
`107`	`107`	`->method('checkPostAuth')`
`@@ -128,7 +128,7 @@ public function testAuthenticateWhenPostCheckAuthenticationFails()`
`128`	`128`	`;`
`129`	`129`	`$provider->expects($this->once())`
`130`	`130`	`->method('checkAuthentication')`
`131`		`- ->willThrowException(new BadCredentialsException())`
	`131`	`+ ->willThrowException(new CredentialsExpiredException())`
`132`	`132`	`;`
`133`	`133`
`134`	`134`	`$provider->authenticate($this->getSupportedToken());`