bug #37368 [Security] Resolve event bubbling of logout + new events in a compiler pass (wouterj)

This PR was merged into the 5.1 branch. Discussion ---------- [Security] Resolve event bubbling of logout + new events in a compiler pass | Q | A | ------------- | --- | Branch? | 5.1 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #37292 | License | MIT | Doc PR | - This PR proposes to create a compiler pass that registers listeners on the main `event_dispatcher` on the firewall-specific event dispatcher during compile time. This allows to still specify listener priorities while listening on a bubbled-up event (instead of a fix moment where the event bubbling occurs). It probably also improves performance, as it doesn't use duplicated event dispatching logic to provide event bubbling. Nothing changes on the user side. I proposed this as a bugfix, as it fixes the bug mentioned in #37292 (not being able to use listener priorities). I did remove a class, which was introduced in 5.1 and is very internal. I think it's safe, but we can also keep it and remove in master. Commits ------- f962c26061 Resolve event bubbling logic in a compiler pass
2020-06-23 06:57:21 +02:00 · 2020-06-23 06:57:21 +02:00 · b83d250c53
commit b83d250c53
parent 0878dd4ac0 f962c26061
5 changed files with 241 additions and 52 deletions
--- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/RegisterGlobalSecurityEventListenersPass.php
+++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/RegisterGlobalSecurityEventListenersPass.php
@ -0,0 +1,70 @@
+<?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\Compiler\CompilerPassInterface;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\Security\Http\Event\CheckPassportEvent;
+use Symfony\Component\Security\Http\Event\LoginFailureEvent;
+use Symfony\Component\Security\Http\Event\LoginSuccessEvent;
+use Symfony\Component\Security\Http\Event\LogoutEvent;
+
+/**
+ * Makes sure all event listeners on the global dispatcher are also listening
+ * to events on the firewall-specific dipatchers.
+ *
+ * This compiler pass must be run after RegisterListenersPass of the
+ * EventDispatcher component.
+ *
+ * @author Wouter de Jong <wouter@wouterj.nl>
+ *
+ * @internal
+ */
+class RegisterGlobalSecurityEventListenersPass implements CompilerPassInterface
+{
+    private static $eventBubblingEvents = [CheckPassportEvent::class, LoginFailureEvent::class, LoginSuccessEvent::class, LogoutEvent::class];
+
+    /**
+     * {@inheritdoc}
+     */
+    public function process(ContainerBuilder $container)
+    {
+        if (!$container->has('event_dispatcher') || !$container->hasParameter('security.firewalls')) {
+            return;
+        }
+
+        $firewallDispatchers = [];
+        foreach ($container->getParameter('security.firewalls') as $firewallName) {
+            if (!$container->has('security.event_dispatcher.'.$firewallName)) {
+                continue;
+            }
+
+            $firewallDispatchers[] = $container->findDefinition('security.event_dispatcher.'.$firewallName);
+        }
+
+        $globalDispatcher = $container->findDefinition('event_dispatcher');
+        foreach ($globalDispatcher->getMethodCalls() as $methodCall) {
+            if ('addListener' !== $methodCall[0]) {
+                continue;
+            }
+
+            $methodCallArguments = $methodCall[1];
+            if (!\in_array($methodCallArguments[0], self::$eventBubblingEvents, true)) {
+                continue;
+            }
+
+            foreach ($firewallDispatchers as $firewallDispatcher) {
+                $firewallDispatcher->addMethodCall('addListener', $methodCallArguments);
+            }
+        }
+    }
+}
--- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php
+++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php
@ -236,6 +236,8 @@ class SecurityExtension extends Extension implements PrependExtensionInterface
        $firewalls = $config['firewalls'];
        $providerIds = $this->createUserProviders($config, $container);

+        $container->setParameter('security.firewalls', array_keys($firewalls));
+
        // make the ContextListener aware of the configured user providers
        $contextListenerDefinition = $container->getDefinition('security.context_listener');
        $arguments = $contextListenerDefinition->getArguments();
@ -348,8 +350,6 @@ class SecurityExtension extends Extension implements PrependExtensionInterface
        // Register Firewall-specific event dispatcher
        $firewallEventDispatcherId = 'security.event_dispatcher.'.$id;
        $container->register($firewallEventDispatcherId, EventDispatcher::class);
-        $container->setDefinition($firewallEventDispatcherId.'.event_bubbling_listener', new ChildDefinition('security.event_dispatcher.event_bubbling_listener'))
-            ->addTag('kernel.event_subscriber', ['dispatcher' => $firewallEventDispatcherId]);

        // Register listeners
        $listeners = [];
--- a/src/Symfony/Bundle/SecurityBundle/EventListener/FirewallEventBubblingListener.php
+++ b/src/Symfony/Bundle/SecurityBundle/EventListener/FirewallEventBubblingListener.php
@ -1,50 +0,0 @@
-<?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\EventListener;
-
-use Symfony\Component\EventDispatcher\EventSubscriberInterface;
-use Symfony\Component\Security\Http\Event\CheckPassportEvent;
-use Symfony\Component\Security\Http\Event\LoginFailureEvent;
-use Symfony\Component\Security\Http\Event\LoginSuccessEvent;
-use Symfony\Component\Security\Http\Event\LogoutEvent;
-use Symfony\Contracts\EventDispatcher\EventDispatcherInterface;
-
-/**
- * A listener that dispatches all security events from the firewall-specific
- * dispatcher on the global event dispatcher.
- *
- * @author Wouter de Jong <wouter@wouterj.nl>
- */
-class FirewallEventBubblingListener implements EventSubscriberInterface
-{
-    private $eventDispatcher;
-
-    public function __construct(EventDispatcherInterface $eventDispatcher)
-    {
-        $this->eventDispatcher = $eventDispatcher;
-    }
-
-    public static function getSubscribedEvents(): array
-    {
-        return [
-            LogoutEvent::class => 'bubbleEvent',
-            LoginFailureEvent::class => 'bubbleEvent',
-            LoginSuccessEvent::class => 'bubbleEvent',
-            CheckPassportEvent::class => 'bubbleEvent',
-        ];
-    }
-
-    public function bubbleEvent($event): void
-    {
-        $this->eventDispatcher->dispatch($event);
-    }
-}
--- a/src/Symfony/Bundle/SecurityBundle/SecurityBundle.php
+++ b/src/Symfony/Bundle/SecurityBundle/SecurityBundle.php
@ -15,6 +15,7 @@ use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\AddExpressionLang
 use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\AddSecurityVotersPass;
 use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\AddSessionDomainConstraintPass;
 use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\RegisterCsrfFeaturesPass;
+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\Security\Factory\AnonymousFactory;
@ -75,6 +76,8 @@ class SecurityBundle extends Bundle
        $container->addCompilerPass(new RegisterCsrfFeaturesPass());
        $container->addCompilerPass(new RegisterTokenUsageTrackingPass(), PassConfig::TYPE_BEFORE_OPTIMIZATION, 200);
        $container->addCompilerPass(new RegisterLdapLocatorPass());
+        // must be registered after RegisterListenersPass (in the FrameworkBundle)
+        $container->addCompilerPass(new RegisterGlobalSecurityEventListenersPass(), PassConfig::TYPE_BEFORE_REMOVING, -200);

        $container->addCompilerPass(new AddEventAliasesPass([
            AuthenticationSuccessEvent::class => AuthenticationEvents::AUTHENTICATION_SUCCESS,
--- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/RegisterGlobalSecurityEventListenersPassTest.php
+++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/RegisterGlobalSecurityEventListenersPassTest.php
@ -0,0 +1,166 @@
+<?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\SecurityExtension;
+use Symfony\Bundle\SecurityBundle\SecurityBundle;
+use Symfony\Component\DependencyInjection\Compiler\PassConfig;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\EventDispatcher\DependencyInjection\RegisterListenersPass;
+use Symfony\Component\EventDispatcher\EventDispatcher;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\Security\Http\Event\CheckPassportEvent;
+use Symfony\Component\Security\Http\Event\LoginSuccessEvent;
+use Symfony\Component\Security\Http\Event\LogoutEvent;
+
+class RegisterGlobalSecurtyEventListenersPassTest extends TestCase
+{
+    private $container;
+
+    protected function setUp(): void
+    {
+        $this->container = new ContainerBuilder();
+        $this->container->setParameter('kernel.debug', false);
+        $this->container->register('request_stack', \stdClass::class);
+        $this->container->register('event_dispatcher', EventDispatcher::class);
+
+        $this->container->registerExtension(new SecurityExtension());
+
+        $this->container->addCompilerPass(new RegisterListenersPass(), PassConfig::TYPE_BEFORE_REMOVING);
+        $this->container->getCompilerPassConfig()->setRemovingPasses([]);
+        $this->container->getCompilerPassConfig()->setAfterRemovingPasses([]);
+
+        $securityBundle = new SecurityBundle();
+        $securityBundle->build($this->container);
+    }
+
+    public function testRegisterCustomListener()
+    {
+        $this->container->loadFromExtension('security', [
+            'enable_authenticator_manager' => true,
+            'firewalls' => ['main' => ['pattern' => '/', 'http_basic' => true]],
+        ]);
+
+        $this->container->register('app.security_listener', \stdClass::class)
+            ->addTag('kernel.event_listener', ['method' => 'onLogout', 'event' => LogoutEvent::class])
+            ->addTag('kernel.event_listener', ['method' => 'onLoginSuccess', 'event' => LoginSuccessEvent::class, 'priority' => 20]);
+
+        $this->container->compile();
+
+        $this->assertListeners([
+            [LogoutEvent::class, ['app.security_listener', 'onLogout'], 0],
+            [LoginSuccessEvent::class, ['app.security_listener', 'onLoginSuccess'], 20],
+        ]);
+    }
+
+    public function testRegisterCustomSubscriber()
+    {
+        $this->container->loadFromExtension('security', [
+            'enable_authenticator_manager' => true,
+            'firewalls' => ['main' => ['pattern' => '/', 'http_basic' => true]],
+        ]);
+
+        $this->container->register(TestSubscriber::class)
+            ->addTag('kernel.event_subscriber');
+
+        $this->container->compile();
+
+        $this->assertListeners([
+            [LogoutEvent::class, [TestSubscriber::class, 'onLogout'], -200],
+            [CheckPassportEvent::class, [TestSubscriber::class, 'onCheckPassport'], 120],
+            [LoginSuccessEvent::class, [TestSubscriber::class, 'onLoginSuccess'], 0],
+        ]);
+    }
+
+    public function testMultipleFirewalls()
+    {
+        $this->container->loadFromExtension('security', [
+            'enable_authenticator_manager' => true,
+            'firewalls' => ['main' => ['pattern' => '/', 'http_basic' => true], 'api' => ['pattern' => '/api', 'http_basic' => true]],
+        ]);
+
+        $this->container->register('security.event_dispatcher.api', EventDispatcher::class)
+            ->addTag('security.event_dispatcher')
+            ->setPublic(true);
+
+        $this->container->register('app.security_listener', \stdClass::class)
+            ->addTag('kernel.event_listener', ['method' => 'onLogout', 'event' => LogoutEvent::class])
+            ->addTag('kernel.event_listener', ['method' => 'onLoginSuccess', 'event' => LoginSuccessEvent::class, 'priority' => 20]);
+
+        $this->container->compile();
+
+        $this->assertListeners([
+            [LogoutEvent::class, ['app.security_listener', 'onLogout'], 0],
+            [LoginSuccessEvent::class, ['app.security_listener', 'onLoginSuccess'], 20],
+        ], 'security.event_dispatcher.main');
+        $this->assertListeners([
+            [LogoutEvent::class, ['app.security_listener', 'onLogout'], 0],
+            [LoginSuccessEvent::class, ['app.security_listener', 'onLoginSuccess'], 20],
+        ], 'security.event_dispatcher.api');
+    }
+
+    public function testListenerAlreadySpecific()
+    {
+        $this->container->loadFromExtension('security', [
+            'enable_authenticator_manager' => true,
+            'firewalls' => ['main' => ['pattern' => '/', 'http_basic' => true]],
+        ]);
+
+        $this->container->register('security.event_dispatcher.api', EventDispatcher::class)
+            ->addTag('security.event_dispatcher')
+            ->setPublic(true);
+
+        $this->container->register('app.security_listener', \stdClass::class)
+            ->addTag('kernel.event_listener', ['method' => 'onLogout', 'event' => LogoutEvent::class, 'dispatcher' => 'security.event_dispatcher.main'])
+            ->addTag('kernel.event_listener', ['method' => 'onLoginSuccess', 'event' => LoginSuccessEvent::class, 'priority' => 20]);
+
+        $this->container->compile();
+
+        $this->assertListeners([
+            [LogoutEvent::class, ['app.security_listener', 'onLogout'], 0],
+            [LoginSuccessEvent::class, ['app.security_listener', 'onLoginSuccess'], 20],
+        ], 'security.event_dispatcher.main');
+    }
+
+    private function assertListeners(array $expectedListeners, string $dispatcherId = 'security.event_dispatcher.main')
+    {
+        $actualListeners = [];
+        foreach ($this->container->findDefinition($dispatcherId)->getMethodCalls() as $methodCall) {
+            [$method, $arguments] = $methodCall;
+            if ('addListener' !== $method) {
+                continue;
+            }
+
+            $arguments[1] = [(string) $arguments[1][0]->getValues()[0], $arguments[1][1]];
+            $actualListeners[] = $arguments;
+        }
+
+        $foundListeners = array_uintersect($expectedListeners, $actualListeners, function (array $a, array $b) {
+            return $a === $b;
+        });
+
+        $this->assertEquals($expectedListeners, $foundListeners);
+    }
+}
+
+class TestSubscriber implements EventSubscriberInterface
+{
+    public static function getSubscribedEvents(): array
+    {
+        return [
+            LogoutEvent::class => ['onLogout', -200],
+            CheckPassportEvent::class => ['onCheckPassport', 120],
+            LoginSuccessEvent::class => 'onLoginSuccess',
+        ];
+    }
+}