bug #34627 [Security/Http] call auth listeners/guards eagerly when th…

…ey "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
-------

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

UPGRADE-4.4.md

@@ -222,6 +222,7 @@ Security
  * The `LdapUserProvider` class has been deprecated, use `Symfony\Component\Ldap\Security\LdapUserProvider` instead.
  * Implementations of `PasswordEncoderInterface` and `UserPasswordEncoderInterface` should add a new `needsRehash()` method
  * Deprecated returning a non-boolean value when implementing `Guard\AuthenticatorInterface::checkCredentials()`. Please explicitly return `false` to indicate invalid credentials.
+ * The `ListenerInterface` is deprecated, extend `AbstractListener` instead.
  * Deprecated passing more than one attribute to `AccessDecisionManager::decide()` and `AuthorizationChecker::isGranted()` (and indirectly the `is_granted()` Twig and ExpressionLanguage function)
 
    **Before**

UPGRADE-5.0.md

@@ -437,7 +437,7 @@ Security
  * `SimpleAuthenticatorInterface`, `SimpleFormAuthenticatorInterface`, `SimplePreAuthenticatorInterface`,
    `SimpleAuthenticationProvider`, `SimpleAuthenticationHandler`, `SimpleFormAuthenticationListener` and
    `SimplePreAuthenticationListener` have been removed. Use Guard instead.
- * The `ListenerInterface` has been removed, turn your listeners into callables instead.
+ * The `ListenerInterface` has been removed, extend `AbstractListener` instead.
  * The `Firewall::handleRequest()` method has been removed, use `Firewall::callListeners()` instead.
  * `\Serializable` interface has been removed from `AbstractToken` and `AuthenticationException`,
    thus `serialize()` and `unserialize()` aren't available.

src/Symfony/Bundle/SecurityBundle/Debug/WrappedListener.php

@@ -50,7 +50,7 @@ public function __invoke(RequestEvent $event)
         if (\is_callable($this->listener)) {
             ($this->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($this->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($this->listener), AbstractListener::class), E_USER_DEPRECATED);
             $this->listener->handle($event);
         }
         $this->time = microtime(true) - $startTime;

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

@@ -409,9 +409,7 @@ private function createFirewall(ContainerBuilder $container, string $id, array $
         }
 
         // Access listener
-        if ($firewall['stateless'] || empty($firewall['anonymous']['lazy'])) {
-            $listeners[] = new Reference('security.access_listener');
-        }
+        $listeners[] = new Reference('security.access_listener');
 
         // Exception listener
         $exceptionListener = new Reference($this->createExceptionListener($container, $firewall, $id, $configuredEntryPoint ?: $defaultEntryPoint, $firewall['stateless']));

src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml

@@ -156,9 +156,7 @@
             <argument type="service" id="security.exception_listener" />
             <argument />  <!-- LogoutListener -->
             <argument />  <!-- FirewallConfig -->
-            <argument type="service" id="security.access_listener" />
             <argument type="service" id="security.untracked_token_storage" />
-            <argument type="service" id="security.access_map" />
         </service>
 
         <service id="security.firewall.config" class="Symfony\Bundle\SecurityBundle\Security\FirewallConfig" abstract="true">

src/Symfony/Bundle/SecurityBundle/Security/LazyFirewallContext.php

@@ -13,11 +13,8 @@
 
 use Symfony\Component\HttpKernel\Event\RequestEvent;
 use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage;
-use Symfony\Component\Security\Core\Authorization\Voter\AuthenticatedVoter;
-use Symfony\Component\Security\Core\Exception\LazyResponseException;
-use Symfony\Component\Security\Http\AccessMapInterface;
 use Symfony\Component\Security\Http\Event\LazyResponseEvent;
-use Symfony\Component\Security\Http\Firewall\AccessListener;
+use Symfony\Component\Security\Http\Firewall\AbstractListener;
 use Symfony\Component\Security\Http\Firewall\ExceptionListener;
 use Symfony\Component\Security\Http\Firewall\LogoutListener;
 
@@ -28,17 +25,13 @@
  */
 class LazyFirewallContext extends FirewallContext
 {
-    private $accessListener;
     private $tokenStorage;
-    private $map;
 
-    public function __construct(iterable $listeners, ?ExceptionListener $exceptionListener, ?LogoutListener $logoutListener, ?FirewallConfig $config, AccessListener $accessListener, TokenStorage $tokenStorage, AccessMapInterface $map)
+    public function __construct(iterable $listeners, ?ExceptionListener $exceptionListener, ?LogoutListener $logoutListener, ?FirewallConfig $config, TokenStorage $tokenStorage)
     {
         parent::__construct($listeners, $exceptionListener, $logoutListener, $config);
 
-        $this->accessListener = $accessListener;
         $this->tokenStorage = $tokenStorage;
-        $this->map = $map;
     }
 
     public function getListeners(): iterable
@@ -48,26 +41,41 @@ public function getListeners(): iterable
 
     public function __invoke(RequestEvent $event)
     {
-        $this->tokenStorage->setInitializer(function () use ($event) {
-            $event = new LazyResponseEvent($event);
-            foreach (parent::getListeners() as $listener) {
-                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);
-                    $listener->handle($event);
-                }
+        $listeners = [];
+        $request = $event->getRequest();
+        $lazy = $request->isMethodCacheable();
+
+        foreach (parent::getListeners() as $listener) {
+            if (!\is_callable($listener)) {
+                @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);
+                $listeners[] = [$listener, 'handle'];
+                $lazy = false;
+            } elseif (!$lazy || !$listener instanceof AbstractListener) {
+                $listeners[] = $listener;
+                $lazy = $lazy && $listener instanceof AbstractListener;
+            } elseif (false !== $supports = $listener->supports($request)) {
+                $listeners[] = [$listener, 'authenticate'];
+                $lazy = null === $supports;
             }
-        });
+        }
 
-        try {
-            [$attributes] = $this->map->getPatterns($event->getRequest());
+        if (!$lazy) {
+            foreach ($listeners as $listener) {
+                $listener($event);
 
-            if ($attributes && [AuthenticatedVoter::IS_AUTHENTICATED_ANONYMOUSLY] !== $attributes) {
-                ($this->accessListener)($event);
+                if ($event->hasResponse()) {
+                    return;
+                }
             }
-        } catch (LazyResponseException $e) {
-            $event->setResponse($e->getResponse());
+
+            return;
         }
+
+        $this->tokenStorage->setInitializer(function () use ($event, $listeners) {
+            $event = new LazyResponseEvent($event);
+            foreach ($listeners as $listener) {
+                $listener($event);
+            }
+        });
     }
 }

src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/GuardedBundle/AppCustomAuthenticator.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\Bundle\SecurityBundle\Tests\Functional\Bundle\GuardedBundle;
+
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
+use Symfony\Component\Security\Core\Exception\AuthenticationException;
+use Symfony\Component\Security\Core\User\UserInterface;
+use Symfony\Component\Security\Core\User\UserProviderInterface;
+use Symfony\Component\Security\Guard\AbstractGuardAuthenticator;
+
+class AppCustomAuthenticator extends AbstractGuardAuthenticator
+{
+    public function supports(Request $request)
+    {
+        return true;
+    }
+
+    public function getCredentials(Request $request)
+    {
+        throw new AuthenticationException('This should be hit');
+    }
+
+    public function getUser($credentials, UserProviderInterface $userProvider)
+    {
+    }
+
+    public function checkCredentials($credentials, UserInterface $user)
+    {
+    }
+
+    public function onAuthenticationFailure(Request $request, AuthenticationException $exception)
+    {
+        return new Response('', 418);
+    }
+
+    public function onAuthenticationSuccess(Request $request, TokenInterface $token, $providerKey)
+    {
+    }
+
+    public function start(Request $request, AuthenticationException $authException = null)
+    {
+        return new Response($authException->getMessage(), Response::HTTP_UNAUTHORIZED);
+    }
+
+    public function supportsRememberMe()
+    {
+    }
+}

src/Symfony/Bundle/SecurityBundle/Tests/Functional/GuardedTest.php

@@ -0,0 +1,24 @@
+<?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\Bundle\SecurityBundle\Tests\Functional;
+
+class GuardedTest extends AbstractWebTestCase
+{
+    public function testGuarded()
+    {
+        $client = $this->createClient(['test_case' => 'Guarded', 'root_config' => 'config.yml']);
+
+        $client->request('GET', '/');
+
+        $this->assertSame(418, $client->getResponse()->getStatusCode());
+    }
+}

src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/Guarded/bundles.php

@@ -0,0 +1,15 @@
+<?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.
+ */
+
+return [
+    new Symfony\Bundle\FrameworkBundle\FrameworkBundle(),
+    new Symfony\Bundle\SecurityBundle\SecurityBundle(),
+];

src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/Guarded/config.yml

@@ -0,0 +1,22 @@
+framework:
+    secret: test
+    router: { resource: "%kernel.project_dir%/%kernel.test_case%/routing.yml" }
+    test: ~
+    default_locale: en
+    profiler: false
+    session:
+        storage_id: session.storage.mock_file
+
+services:
+    logger: { class: Psr\Log\NullLogger }
+    Symfony\Bundle\SecurityBundle\Tests\Functional\Bundle\GuardedBundle\AppCustomAuthenticator: ~
+
+security:
+    firewalls:
+        secure:
+            pattern: ^/
+            anonymous: lazy
+            stateless: false
+            guard:
+                authenticators:
+                    - Symfony\Bundle\SecurityBundle\Tests\Functional\Bundle\GuardedBundle\AppCustomAuthenticator

src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/Guarded/routing.yml

@@ -0,0 +1,5 @@
+main:
+    path: /
+    defaults:
+        _controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::urlRedirectAction
+        path: /app

src/Symfony/Bundle/SecurityBundle/composer.json

@@ -24,7 +24,7 @@
         "symfony/security-core": "^4.4",
         "symfony/security-csrf": "^4.2|^5.0",
         "symfony/security-guard": "^4.2|^5.0",
-        "symfony/security-http": "^4.4"
+        "symfony/security-http": "^4.4.1"
     },
     "require-dev": {
         "doctrine/doctrine-bundle": "^1.5|^2.0",

src/Symfony/Component/Security/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
 -----

src/Symfony/Component/Security/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)]);
             }

src/Symfony/Component/Security/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"

src/Symfony/Component/Security/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);
             }