8000 feature #37337 [Security] Configurable execution order for firewall l… · symfony/symfony@afdb97e · GitHub
[go: up one dir, main page]
Skip to content

Commit afdb97e

Browse files
fabpotfabpot
committed
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 ------- 91388e8 Add ability to prioritize firewall listeners
2 parents dad4e95 + 91388e8 commit afdb97e

File tree

9 files changed

+230
-11
lines changed

9 files changed

+230
-11
lines changed

‎src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/SortFirewallListenersPass.php‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler;
13+
14+
use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
15+
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
16+
use Symfony\Component\DependencyInjection\ContainerBuilder;
17+
use Symfony\Component\DependencyInjection\Definition;
18+
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
19+
use Symfony\Component\DependencyInjection\Reference;
20+
use Symfony\Component\Security\Http\Firewall\FirewallListenerInterface;
21+
22+
/**
23+
* Sorts firewall listeners based on the execution order provided by FirewallListenerInterface::getPriority().
24+
*
25+
* @author Christian Scheb <me@christianscheb.de>
26+
*/
27+
class SortFirewallListenersPass implements CompilerPassInterface
28+
{
29+
public function process(ContainerBuilder $container): void
30+
{
31+
if (!$container->hasParameter('security.firewalls')) {
32+
return;
33+
}
34+
35+
foreach ($container->getParameter('security.firewalls') as $firewallName) {
36+
$firewallContextDefinition = $container->getDefinition('security.firewall.map.context.'.$firewallName);
37+
$this->sortFirewallContextListeners($firewallContextDefinition, $container);
38+
}
39+
}
40+
41+
private function sortFirewallContextListeners(Definition $definition, ContainerBuilder $container): void
42+
{
43+
/** @var IteratorArgument $listenerIteratorArgument */
44+
$listenerIteratorArgument = $definition->getArgument(0);
45+
$prioritiesByServiceId = $this->getListenerPriorities($listenerIteratorArgument, $container);
46+
47+
$listeners = $listenerIteratorArgument->getValues();
48+
usort($listeners, function (Reference $a, Reference $b) use ($prioritiesByServiceId) {
49+
return $prioritiesByServiceId[(string) $b] <=> $prioritiesByServiceId[(string) $a];
50+
});
51+
52+
$listenerIteratorArgument->setValues(array_values($listeners));
53+
}
DC35
54+
55+
private function getListenerPriorities(IteratorArgument $listeners, ContainerBuilder $container): array
56+
{
57+
$priorities = [];
58+
59+
foreach ($listeners->getValues() as $reference) {
60+
$id = (string) $reference;
61+
$def = $container->getDefinition($id);
62+
63+
// We must assume that the class value has been correctly filled, even if the service is created by a factory
64+
$class = $def->getClass();
65+
66+
if (!$r = $container->getReflectionClass($class)) {
67+
throw new InvalidArgumentException(sprintf('Class "%s" used for service "%s" cannot be found.', $class, $id));
68+
}
69+
70+
$priority = 0;
71+
if ($r->isSubclassOf(FirewallListenerInterface::class)) {
72+
$priority = $r->getMethod('getPriority')->invoke(null);
73+
}
74+
75+
$priorities[$id] = $priority;
76+
}
77+
78+
return $priorities;
79+
}
80+
}

‎src/Symfony/Bundle/SecurityBundle/SecurityBundle.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\RegisterGlobalSecurityEventListenersPass;
1919
use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\RegisterLdapLocatorPass;
2020
use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\RegisterTokenUsageTrackingPass;
21+
use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\SortFirewallListenersPass;
2122
use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\AnonymousFactory;
2223
use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\CustomAuthenticatorFactory;
2324
use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\FormLoginFactory;
@@ -74,6 +75,8 @@ public function build(ContainerBuilder $container)
7475
$container->addCompilerPass(new RegisterLdapLocatorPass());
7576
// must be registered after RegisterListenersPass (in the FrameworkBundle)
7677
$container->addCompilerPass(new RegisterGlobalSecurityEventListenersPass(), PassConfig::TYPE_BEFORE_REMOVING, -200);
78+
// execute after ResolveChildDefinitionsPass optimization pass, to ensure class names are set
79+
$container->addCompilerPass(new SortFirewallListenersPass(), PassConfig::TYPE_BEFORE_REMOVING);
7780

7881
$container->addCompilerPass(new AddEventAliasesPass(array_merge(
7982
AuthenticationEvents::ALIASES,

‎src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/SortFirewallListenersPassTest.php‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Bundle\SecurityBundle\Tests\DependencyInjection\Compiler;
13+
14+
use PHPUnit\Framework\TestCase;
15+
use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\SortFirewallListenersPass;
16+
use Symfony\Bundle\SecurityBundle\Security\FirewallContext;
17+
use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
18+
use Symfony\Component\DependencyInjection\ContainerBuilder;
19+
use Symfony\Component\DependencyInjection\Reference;
20+
use Symfony\Component\Security\Http\Firewall\FirewallListenerInterface;
21+
22+
class SortFirewallListenersPassTest extends TestCase
23+
{
24+
public function testSortFirewallListeners()
25+
{
26+
$container = new ContainerBuilder();
27+
$container->setParameter('security.firewalls', ['main']);
28+
29+
$container->register('listener_priority_minus1', FirewallListenerPriorityMinus1::class);
30+
$container->register('listener_priority_1', FirewallListenerPriority1::class);
31+
$container->register('listener_priority_2', FirewallListenerPriority2::class);
32+
$container->register('listener_interface_not_implemented', \stdClass::class);
33+
34+
$firewallContext = $container->register('security.firewall.map.context.main', FirewallContext::class);
35+
$firewallContext->addTag('security.firewall_map_context');
36+
37+
$listeners = new IteratorArgument([
< DC35 /td>
38+
new Reference('listener_priority_minus1'),
39+
new Reference('listener_priority_1'),
40+
new Reference('listener_priority_2'),
41+
new Reference('listener_interface_not_implemented'),
42+
]);
43+
44+
$firewallContext->setArgument(0, $listeners);
45+
46+
$compilerPass = new SortFirewallListenersPass();
47+
$compilerPass->process($container);
48+
49+
$sortedListeners = $firewallContext->getArgument(0);
50+
$expectedSortedlisteners = [
51+
new Reference('listener_priority_2'),
52+
new Reference('listener_priority_1'),
53+
new Reference('listener_interface_not_implemented'),
54+
new Reference('listener_priority_minus1'),
55+
];
56+
$this->assertEquals($expectedSortedlisteners, $sortedListeners->getValues());
57+
}
58+
}
59+
60+
class FirewallListenerPriorityMinus1 implements FirewallListenerInterface
61+
{
62+
public static function getPriority(): int
63+
{
64+
return -1;
65+
}
66+
}
67+
68+
class FirewallListenerPriority1 implements FirewallListenerInterface
69+
{
70+
public static function getPriority(): int
71+
{
72+
return 1;
73+
}
74+
}
75+
76+
class FirewallListenerPriority2 implements FirewallListenerInterface
77+
{
78+
public static function getPriority(): int
79+
{
80+
return 2;
81+
}
82+
}

‎src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/SecurityExtensionTest.php‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
use Symfony\Component\Config\Definition\Builder\NodeDefinition;
2424
use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
2525
use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
26+
use Symfony\Component\DependencyInjection\Compiler\ResolveChildDefinitionsPass;
2627
use Symfony\Component\DependencyInjection\ContainerBuilder;
2728
use Symfony\Component\DependencyInjection\Reference;
2829
use Symfony\Component\ExpressionLanguage\Expression;
@@ -671,7 +672,7 @@ protected function getRawContainer()
671672
$bundle = new SecurityBundle();
672673
$bundle->build($container);
673674

674-
$container->getCompilerPassConfig()->setOptimizationPasses([]);
675+
$container->getCompilerPassConfig()->setOptimizationPasses([new ResolveChildDefinitionsPass()]);
675676
$container->getCompilerPassConfig()->setRemovingPasses([]);
676677
$container->getCompilerPassConfig()->setAfterRemovingPasses([]);
677678

@@ -764,11 +765,16 @@ class TestFirewallListenerFactory implements SecurityFactoryInterface, FirewallL
764765
{
765766
public function createListeners(ContainerBuilder $container, string $firewallName, array $config): array
766767
{
768+
$container->register('custom_firewall_listener_id', \stdClass::class);
769+
767770
return ['custom_firewall_listener_id'];
768771
}
769772

770773
public function create(ContainerBuilder $container, string $id, array $config, string $userProvider, ?string $defaultEntryPoint)
771774
{
775+
$container->register('provider_id', \stdClass::class);
776+
$container->register('listener_id', \stdClass::class);
777+
772778
return ['provider_id', 'listener_id', $defaultEntryPoint];
773779
}
774780

‎src/Symfony/Component/Security/Http/Firewall.php‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
use Symfony\Component\HttpKernel\Event\FinishRequestEvent;
1616
use Symfony\Component\HttpKernel\Event\RequestEvent;
1717
use Symfony\Component\HttpKernel\KernelEvents;
18-
use Symfony\Component\Security\Http\Firewall\AccessListener;
18+
use Symfony\Component\Security\Http\Firewall\FirewallListenerInterface;
1919
use Symfony\Contracts\EventDispatcher\EventDispatcherInterface;
2020

2121
/**
@@ -59,26 +59,28 @@ public function onKernelRequest(RequestEvent $event)
5959
$exceptionListener->register($this->dispatcher);
6060
}
6161

62+
// Authentication listeners are pre-sorted by SortFirewallListenersPass
6263
$authenticationListeners = function () use ($authenticationListeners, $logoutListener) {
63-
$accessListener = null;
64+
if (null !== $logoutListener) {
65+
$logoutListenerPriority = $this->getListenerPriority($logoutListener);
66+
}
6467

6568
foreach ($authenticationListeners as $listener) {
66-
if ($listener instanceof AccessListener) {
67-
$accessListener = $listener;
69+
$listenerPriority = $this->getListenerPriority($listener);
6870

69-
continue;
71+
// Yielding the LogoutListener at the correct position
72+
if (null !== $logoutListener && $listenerPriority < $logoutListenerPriority) {
73+
yield $logoutListener;
74+
$logoutListener = null;
7075
}
7176

7277
yield $listener;
7378
}
7479

80+
// When LogoutListener has the lowest priority of all listeners
7581
if (null !== $logoutListener) {
7682
yield $logoutListener;
7783
}
78-
79-
if (null !== $accessListener) {
80-
yield $accessListener;
81-
}
8284
};
8385

8486
$this->callListeners($event, $authenticationListeners());
@@ -115,4 +117,9 @@ protected function callListeners(RequestEvent $event, iterable $listeners)
115117
}
116118
}
117119
}
120+
121+
private function getListenerPriority(object $logoutListener): int
122+
{
123+
return $logoutListener instanceof FirewallListenerInterface ? $logoutListener->getPriority() : 0;
124+
}
118125
}

‎src/Symfony/Component/Security/Http/Firewall/AbstractListener.php‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
*
2020
* @author Nicolas Grekas <p@tchwork.com>
2121
*/
22-
abstract class AbstractListener
22+
abstract class AbstractListener implements FirewallListenerInterface
2323
{
2424
final public function __invoke(RequestEvent $event)
2525
{
@@ -39,4 +39,9 @@ abstract public function supports(Request $request): ?bool;
3939
* Does whatever is required to authenticate the request, typically calling $event->setResponse() internally.
4040
*/
4141
abstract public function authenticate(RequestEvent $event);
42+
43+
public static function getPriority(): int
44+
{
45+
return 0; // Default
46+
}
4247
}

‎src/Symfony/Component/Security/Http/Firewall/AccessListener.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,4 +109,9 @@ private function createAccessDeniedException(Request $request, array $attributes
109109

110110
return $exception;
111111
}
112+
113+
public static function getPriority(): int
114+
{
115+
return -255;
116+
}
112117
}

‎src/Symfony/Component/Security/Http/Firewall/FirewallListenerInterface.php‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\Security\Http\Firewall;
13+
14+
/**
15+
* Can be implemented by firewall listeners to define their priority in execution.
16+
*
17+
* @author Christian Scheb <me@christianscheb.de>
18+
*/
19+
interface FirewallListenerInterface
20+
{
21+
/**
22+
* Defines the priority of the listener.
23+
* The higher the number, the earlier a listener is executed.
24+
*/
25+
public static function getPriority(): int;
26+
}

‎src/Symfony/Component/Security/Http/Firewall/LogoutListener.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,4 +142,9 @@ protected function requiresLogout(Request $request): bool
142142
{
143143
return isset($this->options['logout_path']) && $this->httpUtils->checkRequestPath($request, $this->options['logout_path']);
144144
}
145+
146+
public static function getPriority(): int
147+
{
148+
return -127;
149+
}
145150
}

0 commit comments

Comments
 (0)
0