bug #34627 [Security/Http] call auth listeners/guards eagerly when they "support" the request (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[Security/Http] call auth listeners/guards eagerly when they "support" the request

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #34614, Fix #34679
| License       | MIT
| Doc PR        | -

This fixes the form authenticator linked to #34614.
Since laziness is here to provide compatibility with HTTP caching, it should be disabled when the request cannot be cached.

Tests don't pass yet, but I'm on the path to something here.

The PR now introduces a new `AbstractListener` that splits the handling logic in two:
- `supports(Request): ?bool` is always called eagerly and tells whether the listener matches the request for an earger call or a lazy call
- `authenticate(RequestEvent)` does the rest of the job when `supports()` allows so - lazily or not depending on the return value of `supports()`.

Of course, this remains compatible with non-lazy logics, see `AbstractListener::__invoke()`.

Commits
-------

b20ebe6b90 [Security/Http] call auth listeners/guards eagerly when they "support" the request

Author: fabpot

Firewall.php

@@ -138,7 +138,7 @@ protected function handleRequest(GetResponseEvent $event, $listeners)
             if (\is_callable($listener)) {
                 $listener($event);
             } else {
-                @trigger_error(sprintf('Calling the "%s::handle()" method from the firewall is deprecated since Symfony 4.3, implement "__invoke()" instead.', \get_class($listener)), E_USER_DEPRECATED);
+                @trigger_error(sprintf('Calling the "%s::handle()" method from the firewall is deprecated since Symfony 4.3, extend "%s" instead.', \get_class($listener), AbstractListener::class), E_USER_DEPRECATED);
                 $listener->handle($event);
             }
 

Firewall/AbstractAuthenticationListener.php

@@ -49,7 +49,7 @@
  * @author Fabien Potencier <fabien@symfony.com>
  * @author Johannes M. Schmitt <schmittjoh@gmail.com>
  */
-abstract class AbstractAuthenticationListener implements ListenerInterface
+abstract class AbstractAuthenticationListener extends AbstractListener implements ListenerInterface
 {
     use LegacyListenerTrait;
 
@@ -105,20 +105,24 @@ public function setRememberMeServices(RememberMeServicesInterface $rememberMeSer
         $this->rememberMeServices = $rememberMeServices;
     }
 
+    /**
+     * {@inheritdoc}
+     */
+    public function supports(Request $request): ?bool
+    {
+        return $this->requiresAuthentication($request);
+    }
+
     /**
      * Handles form based authentication.
      *
      * @throws \RuntimeException
      * @throws SessionUnavailableException
      */
-    public function __invoke(RequestEvent $event)
+    public function authenticate(RequestEvent $event)
     {
         $request = $event->getRequest();
 
-        if (!$this->requiresAuthentication($request)) {
-            return;
-        }
-
         if (!$request->hasSession()) {
             throw new \RuntimeException('This authentication method requires a session.');
         }

Firewall/AbstractListener.php

@@ -0,0 +1,42 @@
+<?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\Firewall;
+
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\Event\RequestEvent;
+
+/**
+ * A base class for listeners that can tell whether they should authenticate incoming requests.
+ *
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+abstract class AbstractListener
+{
+    final public function __invoke(RequestEvent $event)
+    {
+        if (false !== $this->supports($event->getRequest())) {
+            $this->authenticate($event);
+        }
+    }
+
+    /**
+     * Tells whether the authenticate() method should be called or not depending on the incoming request.
+     *
+     * Returning null means authenticate() can be called lazily when accessing the token storage.
+     */
+    abstract public function supports(Request $request): ?bool;
+
+    /**
+     * Does whatever is required to authenticate the request, typically calling $event->setResponse() internally.
+     */
+    abstract public function authenticate(RequestEvent $event);
+}

Firewall/AbstractPreAuthenticatedListener.php

@@ -35,7 +35,7 @@
  *
  * @internal since Symfony 4.3
  */
-abstract class AbstractPreAuthenticatedListener implements ListenerInterface
+abstract class AbstractPreAuthenticatedListener extends AbstractListener implements ListenerInterface
 {
     use LegacyListenerTrait;
 
@@ -56,20 +56,31 @@ public function __construct(TokenStorageInterface $tokenStorage, AuthenticationM
     }
 
     /**
-     * Handles pre-authentication.
+     * {@inheritdoc}
      */
-    public function __invoke(RequestEvent $event)
+    public function supports(Request $request): ?bool
     {
-        $request = $event->getRequest();
-
         try {
-            list($user, $credentials) = $this->getPreAuthenticatedData($request);
+            $request->attributes->set('_pre_authenticated_data', $this->getPreAuthenticatedData($request));
         } catch (BadCredentialsException $e) {
             $this->clearToken($e);
 
-            return;
+            return false;
         }
 
+        return true;
+    }
+
+    /**
+     * Handles pre-authentication.
+     */
+    public function authenticate(RequestEvent $event)
+    {
+        $request = $event->getRequest();
+
+        [$user, $credentials] = $request->attributes->get('_pre_authenticated_data');
+        $request->attributes->remove('_pre_authenticated_data');
+
         if (null !== $this->logger) {
             $this->logger->debug('Checking current security token.', ['token' => (string) $this->tokenStorage->getToken()]);
         }

Firewall/AccessListener.php

@@ -11,10 +11,12 @@
 
 namespace Symfony\Component\Security\Http\Firewall;
 
+use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Event\RequestEvent;
 use Symfony\Component\Security\Core\Authentication\AuthenticationManagerInterface;
 use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
 use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface;
+use Symfony\Component\Security\Core\Authorization\Voter\AuthenticatedVoter;
 use Symfony\Component\Security\Core\Exception\AccessDeniedException;
 use Symfony\Component\Security\Core\Exception\AuthenticationCredentialsNotFoundException;
 use Symfony\Component\Security\Http\AccessMapInterface;
@@ -27,7 +29,7 @@
  *
  * @final since Symfony 4.3
  */
-class AccessListener implements ListenerInterface
+class AccessListener extends AbstractListener implements ListenerInterface
 {
     use LegacyListenerTrait;
 
@@ -44,23 +46,35 @@ public function __construct(TokenStorageInterface $tokenStorage, AccessDecisionM
         $this->authManager = $authManager;
     }
 
+    /**
+     * {@inheritdoc}
+     */
+    public function supports(Request $request): ?bool
+    {
+        [$attributes] = $this->map->getPatterns($request);
+        $request->attributes->set('_access_control_attributes', $attributes);
+
+        return $attributes && [AuthenticatedVoter::IS_AUTHENTICATED_ANONYMOUSLY] !== $attributes ? true : null;
+    }
+
     /**
      * Handles access authorization.
      *
      * @throws AccessDeniedException
      * @throws AuthenticationCredentialsNotFoundException
      */
-    public function __invoke(RequestEvent $event)
+    public function authenticate(RequestEvent $event)
     {
         if (!$event instanceof LazyResponseEvent && null === $token = $this->tokenStorage->getToken()) {
             throw new AuthenticationCredentialsNotFoundException('A Token was not found in the TokenStorage.');
         }
 
         $request = $event->getRequest();
 
-        list($attributes) = $this->map->getPatterns($request);
+        $attributes = $request->attributes->get('_access_control_attributes');
+        $request->attributes->remove('_access_control_attributes');
 
-        if (!$attributes) {
+        if (!$attributes || ([AuthenticatedVoter::IS_AUTHENTICATED_ANONYMOUSLY] === $attributes && $event instanceof LazyResponseEvent)) {
             return;
         }
 

Firewall/AnonymousAuthenticationListener.php

@@ -12,6 +12,7 @@
 namespace Symfony\Component\Security\Http\Firewall;
 
 use Psr\Log\LoggerInterface;
+use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Event\RequestEvent;
 use Symfony\Component\Security\Core\Authentication\AuthenticationManagerInterface;
 use Symfony\Component\Security\Core\Authentication\Token\AnonymousToken;
@@ -26,7 +27,7 @@
  *
  * @final since Symfony 4.3
  */
-class AnonymousAuthenticationListener implements ListenerInterface
+class AnonymousAuthenticationListener extends AbstractListener implements ListenerInterface
 {
     use LegacyListenerTrait;
 
@@ -43,10 +44,18 @@ public function __construct(TokenStorageInterface $tokenStorage, string $secret,
         $this->logger = $logger;
     }
 
+    /**
+     * {@inheritdoc}
+     */
+    public function supports(Request $request): ?bool
+    {
+        return null; // always run authenticate() lazily with lazy firewalls
+    }
+
     /**
      * Handles anonymous authentication.
      */
-    public function __invoke(RequestEvent $event)
+    public function authenticate(RequestEvent $event)
     {
         if (null !== $this->tokenStorage->getToken()) {
             return;

Firewall/BasicAuthenticationListener.php

@@ -29,7 +29,7 @@
  *
  * @final since Symfony 4.3
  */
-class BasicAuthenticationListener implements ListenerInterface
+class BasicAuthenticationListener extends AbstractListener implements ListenerInterface
 {
     use LegacyListenerTrait;
 
@@ -55,10 +55,18 @@ public function __construct(TokenStorageInterface $tokenStorage, AuthenticationM
         $this->ignoreFailure = false;
     }
 
+    /**
+     * {@inheritdoc}
+     */
+    public function supports(Request $request): ?bool
+    {
+        return null !== $request->headers->get('PHP_AUTH_USER');
+    }
+
     /**
      * Handles basic authentication.
      */
-    public function __invoke(RequestEvent $event)
+    public function authenticate(RequestEvent $event)
     {
         $request = $event->getRequest();
 

Firewall/ChannelListener.php

@@ -12,6 +12,7 @@
 namespace Symfony\Component\Security\Http\Firewall;
 
 use Psr\Log\LoggerInterface;
+use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Event\RequestEvent;
 use Symfony\Component\Security\Http\AccessMapInterface;
 use Symfony\Component\Security\Http\EntryPoint\AuthenticationEntryPointInterface;
@@ -24,7 +25,7 @@
  *
  * @final since Symfony 4.3
  */
-class ChannelListener implements ListenerInterface
+class ChannelListener extends AbstractListener implements ListenerInterface
 {
     use LegacyListenerTrait;
 
@@ -42,10 +43,8 @@ public function __construct(AccessMapInterface $map, AuthenticationEntryPointInt
     /**
      * Handles channel management.
      */
-    public function __invoke(RequestEvent $event)
+    public function supports(Request $request): ?bool
     {
-        $request = $event->getRequest();
-
         list(, $channel) = $this->map->getPatterns($request);
 
         if ('https' === $channel && !$request->isSecure()) {
@@ -59,21 +58,26 @@ public function __invoke(RequestEvent $event)
                 }
             }
 
-            $response = $this->authenticationEntryPoint->start($request);
-
-            $event->setResponse($response);
-
-            return;
+            return true;
         }
 
         if ('http' === $channel && $request->isSecure()) {
             if (null !== $this->logger) {
                 $this->logger->info('Redirecting to HTTP.');
             }
 
-            $response = $this->authenticationEntryPoint->start($request);
-
-            $event->setResponse($response);
+            return true;
         }
+
+        return false;
+    }
+
+    public function authenticate(RequestEvent $event)
+    {
+        $request = $event->getRequest();
+
+        $response = $this->authenticationEntryPoint->start($request);
+
+        $event->setResponse($response);
     }
 }

Firewall/ContextListener.php

@@ -41,7 +41,7 @@
  *
  * @final since Symfony 4.3
  */
-class ContextListener implements ListenerInterface
+class ContextListener extends AbstractListener implements ListenerInterface
 {
     use LegacyListenerTrait;
 
@@ -84,10 +84,18 @@ public function setLogoutOnUserChange($logoutOnUserChange)
         @trigger_error(sprintf('The "%s()" method is deprecated since Symfony 4.1.', __METHOD__), E_USER_DEPRECATED);
     }
 
+    /**
+     * {@inheritdoc}
+     */
+    public function supports(Request $request): ?bool
+    {
+        return null; // always run authenticate() lazily with lazy firewalls
+    }
+
     /**
      * Reads the Security Token from the session.
      */
-    public function __invoke(RequestEvent $event)
+    public function authenticate(RequestEvent $event)
     {
         if (!$this->registered && null !== $this->dispatcher && $event->isMasterRequest()) {
             $this->dispatcher->addListener(KernelEvents::RESPONSE, [$this, 'onKernelResponse']);

Firewall/LogoutListener.php

@@ -30,7 +30,7 @@
  *
  * @final since Symfony 4.3
  */
-class LogoutListener implements ListenerInterface
+class LogoutListener extends AbstractListener implements ListenerInterface
 {
     use LegacyListenerTrait;
 
@@ -63,6 +63,14 @@ public function addHandler(LogoutHandlerInterface $handler)
         $this->handlers[] = $handler;
     }
 
+    /**
+     * {@inheritdoc}
+     */
+    public function supports(Request $request): ?bool
+    {
+        return $this->requiresLogout($request);
+    }
+
     /**
      * Performs the logout if requested.
      *
@@ -72,14 +80,10 @@ public function addHandler(LogoutHandlerInterface $handler)
      * @throws LogoutException   if the CSRF token is invalid
      * @throws \RuntimeException if the LogoutSuccessHandlerInterface instance does not return a response
      */
-    public function __invoke(RequestEvent $event)
+    public function authenticate(RequestEvent $event)
     {
         $request = $event->getRequest();
 
-        if (!$this->requiresLogout($request)) {
-            return;
-        }
-
         if (null !== $this->csrfTokenManager) {
             $csrfToken = ParameterBagUtils::getRequestParameterValue($request, $this->options['csrf_parameter']);