symfony · fabpot · Sep 2, 2020 · Jun 17, 2020
diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/SortFirewallListenersPass.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/SortFirewallListenersPass.php
@@ -0,0 +1,80 @@
+<?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\DependencyInjection\Compiler;
+
+use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
+use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
+use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\Security\Http\Firewall\FirewallListenerInterface;
+
+/**
+ * Sorts firewall listeners based on the execution order provided by FirewallListenerInterface::getPriority().
+ *
+ * @author Christian Scheb <me@christianscheb.de>
+ */
+class SortFirewallListenersPass implements CompilerPassInterface
+{
+    public function process(ContainerBuilder $container): void
+    {
+        if (!$container->hasParameter('security.firewalls')) {
+            return;
+        }
+
+        foreach ($container->getParameter('security.firewalls') as $firewallName) {
+            $firewallContextDefinition = $container->getDefinition('security.firewall.map.context.'.$firewallName);
+            $this->sortFirewallContextListeners($firewallContextDefinition, $container);
+        }
+    }
+
+    private function sortFirewallContextListeners(Definition $definition, ContainerBuilder $container): void
+    {
+        /** @var IteratorArgument $listenerIteratorArgument */
+        $listenerIteratorArgument = $definition->getArgument(0);
+        $prioritiesByServiceId = $this->getListenerPriorities($listenerIteratorArgument, $container);
+
+        $listeners = $listenerIteratorArgument->getValues();
+        usort($listeners, function (Reference $a, Reference $b) use ($prioritiesByServiceId) {
+            return $prioritiesByServiceId[(string) $b] <=> $prioritiesByServiceId[(string) $a];
+        });
+
+        $listenerIteratorArgument->setValues(array_values($listeners));
+    }
+
+    private function getListenerPriorities(IteratorArgument $listeners, ContainerBuilder $container): array
+    {
+        $priorities = [];
+
+        foreach ($listeners->getValues() as $reference) {
+            $id = (string) $reference;
+            $def = $container->getDefinition($id);
+
+            // We must assume that the class value has been correctly filled, even if the service is created by a factory
+            $class = $def->getClass();
+
+            if (!$r = $container->getReflectionClass($class)) {
+                throw new InvalidArgumentException(sprintf('Class "%s" used for service "%s" cannot be found.', $class, $id));
+            }
+
+            $priority = 0;
+            if ($r->isSubclassOf(FirewallListenerInterface::class)) {
+                $priority = $r->getMethod('getPriority')->invoke(null);
+            }
+
+            $priorities[$id] = $priority;
+        }
+
+        return $priorities;
+    }
+}
diff --git a/src/Symfony/Bundle/SecurityBundle/SecurityBundle.php b/src/Symfony/Bundle/SecurityBundle/SecurityBundle.php
@@ -18,6 +18,7 @@
 use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\RegisterGlobalSecurityEventListenersPass;
 use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\RegisterLdapLocatorPass;
 use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\RegisterTokenUsageTrackingPass;
+use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\SortFirewallListenersPass;
 use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\AnonymousFactory;
 use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\CustomAuthenticatorFactory;
 use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\FormLoginFactory;
@@ -78,6 +79,8 @@ public function build(ContainerBuilder $container)
         $container->addCompilerPass(new RegisterLdapLocatorPass());
         // must be registered after RegisterListenersPass (in the FrameworkBundle)
         $container->addCompilerPass(new RegisterGlobalSecurityEventListenersPass(), PassConfig::TYPE_BEFORE_REMOVING, -200);
+        // execute after ResolveChildDefinitionsPass optimization pass, to ensure class names are set
+        $container->addCompilerPass(new SortFirewallListenersPass(), PassConfig::TYPE_BEFORE_REMOVING);
 
         $container->addCompilerPass(new AddEventAliasesPass([
             AuthenticationSuccessEvent::class => AuthenticationEvents::AUTHENTICATION_SUCCESS,

diff --git a/...undle/SecurityBundle/Tests/DependencyInjection/Compiler/SortFirewallListenersPassTest.php b/...undle/SecurityBundle/Tests/DependencyInjection/Compiler/SortFirewallListenersPassTest.php
@@ -0,0 +1,82 @@
+<?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\DependencyInjection\Compiler;
+
+use PHPUnit\Framework\TestCase;
+use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\SortFirewallListenersPass;
+use Symfony\Bundle\SecurityBundle\Security\FirewallContext;
+use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\Security\Http\Firewall\FirewallListenerInterface;
+
+class SortFirewallListenersPassTest extends TestCase
+{
+    public function testSortFirewallListeners()
+    {
+        $container = new ContainerBuilder();
+        $container->setParameter('security.firewalls', ['main']);
+
+        $container->register('listener_priority_minus1', FirewallListenerPriorityMinus1::class);
+        $container->register('listener_priority_1', FirewallListenerPriority1::class);
+        $container->register('listener_priority_2', FirewallListenerPriority2::class);
+        $container->register('listener_interface_not_implemented', \stdClass::class);
+
+        $firewallContext = $container->register('security.firewall.map.context.main', FirewallContext::class);
+        $firewallContext->addTag('security.firewall_map_context');
+
+        $listeners = new IteratorArgument([
+            new Reference('listener_priority_minus1'),
+            new Reference('listener_priority_1'),
+            new Reference('listener_priority_2'),
+            new Reference('listener_interface_not_implemented'),
+        ]);
+
+        $firewallContext->setArgument(0, $listeners);
+
+        $compilerPass = new SortFirewallListenersPass();
+        $compilerPass->process($container);
+
+        $sortedListeners = $firewallContext->getArgument(0);
+        $expectedSortedlisteners = [
+            new Reference('listener_priority_2'),
+            new Reference('listener_priority_1'),
+            new Reference('listener_interface_not_implemented'),
+            new Reference('listener_priority_minus1'),
+        ];
+        $this->assertEquals($expectedSortedlisteners, $sortedListeners->getValues());
+    }
+}
+
+class FirewallListenerPriorityMinus1 implements FirewallListenerInterface
+{
+    public static function getPriority(): int
+    {
+        return -1;
+    }
+}
+
+class FirewallListenerPriority1 implements FirewallListenerInterface
+{
+    public static function getPriority(): int
+    {
+        return 1;
+    }
+}
+
+class FirewallListenerPriority2 implements FirewallListenerInterface
+{
+    public static function getPriority(): int
+    {
+        return 2;
+    }
+}
diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/SecurityExtensionTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/SecurityExtensionTest.php
@@ -23,6 +23,7 @@
 use Symfony\Component\Config\Definition\Builder\NodeDefinition;
 use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
 use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
+use Symfony\Component\DependencyInjection\Compiler\ResolveChildDefinitionsPass;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\ExpressionLanguage\Expression;
@@ -671,7 +672,7 @@ protected function getRawContainer()
         $bundle = new SecurityBundle();
         $bundle->build($container);
 
-        $container->getCompilerPassConfig()->setOptimizationPasses([]);
+        $container->getCompilerPassConfig()->setOptimizationPasses([new ResolveChildDefinitionsPass()]);
         $container->getCompilerPassConfig()->setRemovingPasses([]);
         $container->getCompilerPassConfig()->setAfterRemovingPasses([]);
 
@@ -764,11 +765,16 @@ class TestFirewallListenerFactory implements SecurityFactoryInterface, FirewallL
 {
     public function createListeners(ContainerBuilder $container, string $firewallName, array $config): array
     {
+        $container->register('custom_firewall_listener_id', \stdClass::class);
+
         return ['custom_firewall_listener_id'];
     }
 
     public function create(ContainerBuilder $container, string $id, array $config, string $userProvider, ?string $defaultEntryPoint)
     {
+        $container->register('provider_id', \stdClass::class);
+        $container->register('listener_id', \stdClass::class);
+
         return ['provider_id', 'listener_id', $defaultEntryPoint];
     }
 

diff --git a/src/Symfony/Component/Security/Http/Firewall.php b/src/Symfony/Component/Security/Http/Firewall.php
@@ -15,7 +15,7 @@
 use Symfony\Component\HttpKernel\Event\FinishRequestEvent;
 use Symfony\Component\HttpKernel\Event\RequestEvent;
 use Symfony\Component\HttpKernel\KernelEvents;
-use Symfony\Component\Security\Http\Firewall\AccessListener;
+use Symfony\Component\Security\Http\Firewall\FirewallListenerInterface;
 use Symfony\Contracts\EventDispatcher\EventDispatcherInterface;
 
 /**
@@ -59,26 +59,28 @@ public function onKernelRequest(RequestEvent $event)
             $exceptionListener->register($this->dispatcher);
         }
 
+        // Authentication listeners are pre-sorted by SortFirewallListenersPass
         $authenticationListeners = function () use ($authenticationListeners, $logoutListener) {
-            $accessListener = null;
+            if (null !== $logoutListener) {
+                $logoutListenerPriority = $this->getListenerPriority($logoutListener);
+            }
 
             foreach ($authenticationListeners as $listener) {
-                if ($listener instanceof AccessListener) {
-                    $accessListener = $listener;
+                $listenerPriority = $this->getListenerPriority($listener);
 
-                    continue;
+                // Yielding the LogoutListener at the correct position
+                if (null !== $logoutListener && $listenerPriority < $logoutListenerPriority) {
+                    yield $logoutListener;
+                    $logoutListener = null;
                 }
 
                 yield $listener;
             }
 
+            // When LogoutListener has the lowest priority of all listeners
             if (null !== $logoutListener) {
                 yield $logoutListener;
             }
-
-            if (null !== $accessListener) {
-                yield $accessListener;
-            }
         };
 
         $this->callListeners($event, $authenticationListeners());
@@ -115,4 +117,9 @@ protected function callListeners(RequestEvent $event, iterable $listeners)
             }
         }
     }
+
+    private function getListenerPriority(object $logoutListener): int
+    {
+        return $logoutListener instanceof FirewallListenerInterface ? $logoutListener->getPriority() : 0;
+    }
 }
diff --git a/src/Symfony/Component/Security/Http/Firewall/AbstractListener.php b/src/Symfony/Component/Security/Http/Firewall/AbstractListener.php
@@ -19,7 +19,7 @@
  *
  * @author Nicolas Grekas <p@tchwork.com>
  */
-abstract class AbstractListener
+abstract class AbstractListener implements FirewallListenerInterface
 {
     final public function __invoke(RequestEvent $event)
     {
@@ -39,4 +39,9 @@ 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);
+
+    public static function getPriority(): int
+    {
+        return 0; // Default
+    }
 }
diff --git a/src/Symfony/Component/Security/Http/Firewall/AccessListener.php b/src/Symfony/Component/Security/Http/Firewall/AccessListener.php
@@ -122,4 +122,9 @@ private function createAccessDeniedException(Request $request, array $attributes
 
         return $exception;
     }
+
+    public static function getPriority(): int
+    {
+        return -255;
+    }
 }
diff --git a/src/Symfony/Component/Security/Http/Firewall/FirewallListenerInterface.php b/src/Symfony/Component/Security/Http/Firewall/FirewallListenerInterface.php
@@ -0,0 +1,26 @@
+<?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;
+
+/**
+ * Can be implemented by firewall listeners to define their priority in execution.
+ *
+ * @author Christian Scheb <me@christianscheb.de>
+ */
+interface FirewallListenerInterface
+{
+    /**
+     * Defines the priority of the listener.
+     * The higher the number, the earlier a listener is executed.
+     */
+    public static function getPriority(): int;
+}
diff --git a/src/Symfony/Component/Security/Http/Firewall/LogoutListener.php b/src/Symfony/Component/Security/Http/Firewall/LogoutListener.php
@@ -142,4 +142,9 @@ protected function requiresLogout(Request $request): bool
     {
         return isset($this->options['logout_path']) && $this->httpUtils->checkRequestPath($request, $this->options['logout_path']);
     }
+
+    public static function getPriority(): int
+    {
+        return -127;
+    }
 }