feature #37337 [Security] Configurable execution order for firewall listeners (scheb)

This PR was merged into the 5.2-dev branch.

Discussion
----------

[Security] Configurable execution order for firewall listeners

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| License       | MIT
| Doc PR        | n/a

Hello there, I'm the author of `scheb/two-factor-bundle`, which extends Symfony's security layer with two-factor authentication. I've been closely following the recent changes by @wouterj to rework the security layer with "authenticators" (great work!). While I managed to make my bundle work with authenticators, I see some limitations in the security layer that I'd like to address to make such extensions easier to implement.

In #37336 I've submitted a draft to let security factories add their own authentication listeners to the firewall. This PR is intended to address the issue of execution order. If you look at the `Firewall` class

https://github.com/symfony/symfony/blob/f64f59a9c0d92fdd65f9de3e44b612402b224aaf/src/Symfony/Component/Security/Http/Firewall.php#L62-L82

authentication listeners are executed in the order of their creation. Additionally, there's hardcoded logic to execute `Symfony\Component\Security\Http\Firewall\AccessListener` always last and the logout listener second to last. I'd like to have a more flexible approach, to remove the hardcoded order and give authentication listeners the ability to determine their execution order.

I've added an optional interface to provide a priority to sort all registered authenitication listeners. Sorting is done in a compiler pass, so no time is wasted at runtime.

This is a draft, so I'd like to hear your opinion on this :)

Commits
-------

91388e871b Add ability to prioritize firewall listeners

Author: fabpot

DependencyInjection/Compiler/SortFirewallListenersPass.php

@@ -0,0 +1,80 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * 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 <[email protected]>
+ */
+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;
+    }
+}

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;
@@ -74,6 +75,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(array_merge(
             AuthenticationEvents::ALIASES,

Tests/DependencyInjection/Compiler/SortFirewallListenersPassTest.php

@@ -0,0 +1,82 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * 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;
+    }
+}

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];
     }