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

CHANGELOG.md

@@ -14,6 +14,7 @@ CHANGELOG
  * Deprecated returning a non-boolean value when implementing `Guard\AuthenticatorInterface::checkCredentials()`.
  * Deprecated passing more than one attribute to `AccessDecisionManager::decide()` and `AuthorizationChecker::isGranted()`
  * Added new `argon2id` encoder, undeprecated the `bcrypt` and `argon2i` ones (using `auto` is still recommended by default.)
+ * Added `AbstractListener` which replaces the deprecated `ListenerInterface`
 
 4.3.0
 -----

Guard/Firewall/GuardAuthenticationListener.php

@@ -21,6 +21,7 @@
 use Symfony\Component\Security\Guard\AuthenticatorInterface;
 use Symfony\Component\Security\Guard\GuardAuthenticatorHandler;
 use Symfony\Component\Security\Guard\Token\PreAuthenticationGuardToken;
+use Symfony\Component\Security\Http\Firewall\AbstractListener;
 use Symfony\Component\Security\Http\Firewall\LegacyListenerTrait;
 use Symfony\Component\Security\Http\Firewall\ListenerInterface;
 use Symfony\Component\Security\Http\RememberMe\RememberMeServicesInterface;
@@ -33,7 +34,7 @@
  *
  * @final since Symfony 4.3
  */
-class GuardAuthenticationListener implements ListenerInterface
+class GuardAuthenticationListener extends AbstractListener implements ListenerInterface
 {
     use LegacyListenerTrait;
 
@@ -62,9 +63,9 @@ public function __construct(GuardAuthenticatorHandler $guardHandler, Authenticat
     }
 
     /**
-     * Iterates over each authenticator to see if each wants to authenticate the request.
+     * {@inheritdoc}
      */
-    public function __invoke(RequestEvent $event)
+    public function supports(Request $request): ?bool
     {
         if (null !== $this->logger) {
             $context = ['firewall_key' => $this->providerKey];
@@ -76,7 +77,39 @@ public function __invoke(RequestEvent $event)
             $this->logger->debug('Checking for guard authentication credentials.', $context);
         }
 
+        $guardAuthenticators = [];
+
         foreach ($this->guardAuthenticators as $key => $guardAuthenticator) {
+            if (null !== $this->logger) {
+                $this->logger->debug('Checking support on guard authenticator.', ['firewall_key' => $this->providerKey, 'authenticator' => \get_class($guardAuthenticator)]);
+            }
+
+            if ($guardAuthenticator->supports($request)) {
+                $guardAuthenticators[$key] = $guardAuthenticator;
+            } elseif (null !== $this->logger) {
+                $this->logger->debug('Guard authenticator does not support the request.', ['firewall_key' => $this->providerKey, 'authenticator' => \get_class($guardAuthenticator)]);
+            }
+        }
+
+        if (!$guardAuthenticators) {
+            return false;
+        }
+
+        $request->attributes->set('_guard_authenticators', $guardAuthenticators);
+
+        return true;
+    }
+
+    /**
+     * Iterates over each authenticator to see if each wants to authenticate the request.
+     */
+    public function authenticate(RequestEvent $event)
+    {
+        $request = $event->getRequest();
+        $guardAuthenticators = $request->attributes->get('_guard_authenticators');
+        $request->attributes->remove('_guard_authenticators');
+
+        foreach ($guardAuthenticators as $key => $guardAuthenticator) {
             // get a key that's unique to *this* guard authenticator
             // this MUST be the same as GuardAuthenticationProvider
             $uniqueGuardKey = $this->providerKey.'_'.$key;
@@ -97,19 +130,6 @@ private function executeGuardAuthenticator(string $uniqueGuardKey, Authenticator
     {
         $request = $event->getRequest();
         try {
-            if (null !== $this->logger) {
-                $this->logger->debug('Checking support on guard authenticator.', ['firewall_key' => $this->providerKey, 'authenticator' => \get_class($guardAuthenticator)]);
-            }
-
-            // abort the execution of the authenticator if it doesn't support the request
-            if (!$guardAuthenticator->supports($request)) {
-                if (null !== $this->logger) {
-                    $this->logger->debug('Guard authenticator does not support the request.', ['firewall_key' => $this->providerKey, 'authenticator' => \get_class($guardAuthenticator)]);
-                }
-
-                return;
-            }
-
             if (null !== $this->logger) {
                 $this->logger->debug('Calling getCredentials() on guard authenticator.', ['firewall_key' => $this->providerKey, 'authenticator' => \get_class($guardAuthenticator)]);
             }

Guard/composer.json

@@ -18,7 +18,7 @@
     "require": {
         "php": "^7.1.3",
         "symfony/security-core": "^3.4.22|^4.2.3|^5.0",
-        "symfony/security-http": "^4.3"
+        "symfony/security-http": "^4.4.1"
     },
     "require-dev": {
         "psr/log": "~1.0"

Http/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);
             }
 

Http/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.');
         }

Http/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);
+}

Http/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()]);
         }

Http/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;
         }
 

Http/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;

Http/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();