From f962c2606191aefa9e66fb4478719e49fd4bc3d2 Mon Sep 17 00:00:00 2001 From: Wouter de Jong Date: Sat, 20 Jun 2020 21:56:07 +0200 Subject: [PATCH] Resolve event bubbling logic in a compiler pass * This removes duplicate event dispatching logic on event bubbling, which probably improves performance. * It allows to still specify listener priorities while listening on a bubbled-up event (instead of a fix moment where the event bubbling occurs) --- ...gisterGlobalSecurityEventListenersPass.php | 70 ++++++++ .../DependencyInjection/SecurityExtension.php | 4 +- .../FirewallEventBubblingListener.php | 50 ------ .../Bundle/SecurityBundle/SecurityBundle.php | 3 + ...erGlobalSecurityEventListenersPassTest.php | 166 ++++++++++++++++++ 5 files changed, 241 insertions(+), 52 deletions(-) create mode 100644 src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/RegisterGlobalSecurityEventListenersPass.php delete mode 100644 src/Symfony/Bundle/SecurityBundle/EventListener/FirewallEventBubblingListener.php create mode 100644 src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/RegisterGlobalSecurityEventListenersPassTest.php diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/RegisterGlobalSecurityEventListenersPass.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/RegisterGlobalSecurityEventListenersPass.php new file mode 100644 index 0000000000000..dc99a1c2dd1f6 --- /dev/null +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/RegisterGlobalSecurityEventListenersPass.php @@ -0,0 +1,70 @@ + + * + * 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 + * + * @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); + } + } + } +} diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php index b1957efe3d207..198bf08a24d0c 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php @@ -236,6 +236,8 @@ private function createFirewalls(array $config, ContainerBuilder $container) $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 @@ private function createFirewall(ContainerBuilder $container, string $id, array $ // 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 = []; diff --git a/src/Symfony/Bundle/SecurityBundle/EventListener/FirewallEventBubblingListener.php b/src/Symfony/Bundle/SecurityBundle/EventListener/FirewallEventBubblingListener.php deleted file mode 100644 index cf302cd58f1ac..0000000000000 --- a/src/Symfony/Bundle/SecurityBundle/EventListener/FirewallEventBubblingListener.php +++ /dev/null @@ -1,50 +0,0 @@ - - * - * 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 - */ -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); - } -} diff --git a/src/Symfony/Bundle/SecurityBundle/SecurityBundle.php b/src/Symfony/Bundle/SecurityBundle/SecurityBundle.php index 2fe52fef57263..9388ec3331f14 100644 --- a/src/Symfony/Bundle/SecurityBundle/SecurityBundle.php +++ b/src/Symfony/Bundle/SecurityBundle/SecurityBundle.php @@ -15,6 +15,7 @@ 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 @@ public function build(ContainerBuilder $container) $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, diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/RegisterGlobalSecurityEventListenersPassTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/RegisterGlobalSecurityEventListenersPassTest.php new file mode 100644 index 0000000000000..8576d24c26bff --- /dev/null +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/RegisterGlobalSecurityEventListenersPassTest.php @@ -0,0 +1,166 @@ + + * + * 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', + ]; + } +}