bug #39389 [Security] Move the handleAuthenticationSuccess logic outside try/catch block (jderusse)

This PR was merged into the 5.2 branch.

Discussion
----------

[Security]  Move the handleAuthenticationSuccess logic outside try/catch block

| Q             | A
| ------------- | ---
| Branch?       | 5.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

The current implementation of `AuthenticationManager` handle the `handleAuthenticationSuccess` logic in a try/catch block which triggers the `handleAuthenticationFailure` in case of failure.

Which could leads to inconsistency and unexpected behavior. The authentication is either successfully or failure, but can't be both in the same request.

Commits
-------

da5c39ec2e Move AuthenticationSuccessEvent outside try/catch block

Author: jderusse

Authentication/AuthenticatorManager.php

@@ -185,18 +185,6 @@ private function executeAuthenticator(AuthenticatorInterface $authenticator, Req
             if (null !== $this->logger) {
                 $this->logger->info('Authenticator successful!', ['token' => $authenticatedToken, 'authenticator' => \get_class($authenticator)]);
             }
-
-            // success! (sets the token on the token storage, etc)
-            $response = $this->handleAuthenticationSuccess($authenticatedToken, $passport, $request, $authenticator);
-            if ($response instanceof Response) {
-                return $response;
-            }
-
-            if (null !== $this->logger) {
-                $this->logger->debug('Authenticator set no success response: request continues.', ['authenticator' => \get_class($authenticator)]);
-            }
-
-            return null;
         } catch (AuthenticationException $e) {
             // oh no! Authentication failed!
             $response = $this->handleAuthenticationFailure($e, $request, $authenticator, $passport);
@@ -206,6 +194,18 @@ private function executeAuthenticator(AuthenticatorInterface $authenticator, Req
 
             return null;
         }
+
+        // success! (sets the token on the token storage, etc)
+        $response = $this->handleAuthenticationSuccess($authenticatedToken, $passport, $request, $authenticator);
+        if ($response instanceof Response) {
+            return $response;
+        }
+
+        if (null !== $this->logger) {
+            $this->logger->debug('Authenticator set no success response: request continues.', ['authenticator' => \get_class($authenticator)]);
+        }
+
+        return null;
     }
 
     private function handleAuthenticationSuccess(TokenInterface $authenticatedToken, PassportInterface $passport, Request $request, AuthenticatorInterface $authenticator): ?Response

EventListener/UserCheckerListener.php

@@ -12,11 +12,12 @@
 namespace Symfony\Component\Security\Http\EventListener;
 
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\Security\Core\Event\AuthenticationSuccessEvent;
 use Symfony\Component\Security\Core\User\UserCheckerInterface;
+use Symfony\Component\Security\Core\User\UserInterface;
 use Symfony\Component\Security\Http\Authenticator\Passport\Badge\PreAuthenticatedUserBadge;
 use Symfony\Component\Security\Http\Authenticator\Passport\UserPassportInterface;
 use Symfony\Component\Security\Http\Event\CheckPassportEvent;
-use Symfony\Component\Security\Http\Event\LoginSuccessEvent;
 
 /**
  * @author Wouter de Jong <[email protected]>
@@ -43,21 +44,21 @@ public function preCheckCredentials(CheckPassportEvent $event): void
         $this->userChecker->checkPreAuth($passport->getUser());
     }
 
-    public function postCheckCredentials(LoginSuccessEvent $event): void
+    public function postCheckCredentials(AuthenticationSuccessEvent $event): void
     {
-        $passport = $event->getPassport();
-        if (!$passport instanceof UserPassportInterface || null === $passport->getUser()) {
+        $user = $event->getAuthenticationToken()->getUser();
+        if (!$user instanceof UserInterface) {
             return;
         }
 
-        $this->userChecker->checkPostAuth($passport->getUser());
+        $this->userChecker->checkPostAuth($user);
     }
 
     public static function getSubscribedEvents(): array
     {
         return [
             CheckPassportEvent::class => ['preCheckCredentials', 256],
-            LoginSuccessEvent::class => ['postCheckCredentials', 256],
+            AuthenticationSuccessEvent::class => ['postCheckCredentials', 256],
         ];
     }
 }

Tests/EventListener/UserCheckerListenerTest.php

@@ -12,17 +12,17 @@
 namespace Symfony\Component\Security\Http\Tests\EventListener;
 
 use PHPUnit\Framework\TestCase;
-use Symfony\Component\HttpFoundation\Request;
-use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
+use Symfony\Component\Security\Core\Authentication\Token\PreAuthenticatedToken;
+use Symfony\Component\Security\Core\Event\AuthenticationSuccessEvent;
 use Symfony\Component\Security\Core\User\User;
 use Symfony\Component\Security\Core\User\UserCheckerInterface;
 use Symfony\Component\Security\Http\Authenticator\AuthenticatorInterface;
 use Symfony\Component\Security\Http\Authenticator\Passport\Badge\PreAuthenticatedUserBadge;
 use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
 use Symfony\Component\Security\Http\Authenticator\Passport\PassportInterface;
 use Symfony\Component\Security\Http\Authenticator\Passport\SelfValidatingPassport;
+use Symfony\Component\Security\Http\Authenticator\Token\PostAuthenticationToken;
 use Symfony\Component\Security\Http\Event\CheckPassportEvent;
-use Symfony\Component\Security\Http\Event\LoginSuccessEvent;
 use Symfony\Component\Security\Http\EventListener\UserCheckerListener;
 
 class UserCheckerListenerTest extends TestCase
@@ -63,14 +63,14 @@ public function testPostAuthValidCredentials()
     {
         $this->userChecker->expects($this->once())->method('checkPostAuth')->with($this->user);
 
-        $this->listener->postCheckCredentials($this->createLoginSuccessEvent());
+        $this->listener->postCheckCredentials(new AuthenticationSuccessEvent(new PostAuthenticationToken($this->user, 'main', [])));
     }
 
     public function testPostAuthNoUser()
     {
         $this->userChecker->expects($this->never())->method('checkPostAuth');
 
-        $this->listener->postCheckCredentials($this->createLoginSuccessEvent($this->createMock(PassportInterface::class)));
+        $this->listener->postCheckCredentials(new AuthenticationSuccessEvent(new PreAuthenticatedToken('nobody', null, 'main')));
     }
 
     private function createCheckPassportEvent($passport = null)
@@ -82,12 +82,8 @@ private function createCheckPassportEvent($passport = null)
         return new CheckPassportEvent($this->createMock(AuthenticatorInterface::class), $passport);
     }
 
-    private function createLoginSuccessEvent($passport = null)
+    private function createAuthenticationSuccessEvent()
     {
-        if (null === $passport) {
-            $passport = new SelfValidatingPassport(new UserBadge('test', function () { return $this->user; }));
-        }
-
-        return new LoginSuccessEvent($this->createMock(AuthenticatorInterface::class), $passport, $this->createMock(TokenInterface::class), new Request(), null, 'main');
+        return new AuthenticationSuccessEvent(new PostAuthenticationToken($this->user, 'main', []));
     }
 }