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

…" the request
symfony · Nov 30, 2019 · b20ebe6 · b20ebe6
1 parent 59b6cfe
commit b20ebe6
Show file tree

Hide file tree

Showing 32 changed files with 462 additions and 243 deletions.
diff --git a/UPGRADE-4.4.md b/UPGRADE-4.4.md
@@ -219,6 +219,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**

diff --git a/UPGRADE-5.0.md b/UPGRADE-5.0.md
@@ -434,7 +434,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.

diff --git a/src/Symfony/Bundle/SecurityBundle/Debug/WrappedListener.php b/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;

diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php b/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']));

diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml b/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">

diff --git a/src/Symfony/Bundle/SecurityBundle/Security/LazyFirewallContext.php b/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);
+            }
+        });
     }
 }
diff --git a/...ny/Bundle/SecurityBundle/Tests/Functional/Bundle/GuardedBundle/AppCustomAuthenticator.php b/...ny/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()
+    {
+    }
+}
diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/GuardedTest.php b/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());
+    }
+}
diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/Guarded/bundles.php b/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(),
+];
diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/Guarded/config.yml b/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
diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/Guarded/routing.yml b/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
diff --git a/src/Symfony/Bundle/SecurityBundle/composer.json b/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",

diff --git a/src/Symfony/Component/Security/CHANGELOG.md b/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
 -----

diff --git a/src/Symfony/Component/Security/Guard/Firewall/GuardAuthenticationListener.php b/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)]);
             }

diff --git a/src/Symfony/Component/Security/Guard/composer.json b/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"

diff --git a/src/Symfony/Component/Security/Http/Firewall.php b/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);
             }