Skip to content

Commit fb8935e

Browse files
nicolas-grekasnicolas-grekas
committed
bug #39621 [Security] Fix event propagation for globally registered security events (scheb)
This PR was squashed before being merged into the 5.1 branch. Discussion ---------- [Security] Fix event propagation for globally registered security events | Q | A | ------------- | --- | Branch? | 5.1 | Bug fix? | yes | New feature? | no | Deprecations? | no | License | MIT When new authenticator security is enabled, the `AuthenticatorManager` is using its own firewall-specific event dispatcher. To allow security events being listened to on the global level, `RegisterGlobalSecurityEventListenersPass` is there to automatically add globally registered event listeners to the firewall-specific event dispatchers. `RegisterGlobalSecurityEventListenersPass` contains a list of events that are propagated, but unfortunately this list is incomplete as there are other events in `AuthenticatorManager` that would need too be propagated. So I added the missing (older) security events. These older events may also be registered by their name, rather than the FQN of the class, so I've also added those. As this is targeting 5.1, I'll file another PR for the `AuthenticationTokenCreatedEvent` that was introduced in 5.2, as soon as this change was merged into 5.x. On a note, I feel this "whitelist" approach to propagate security events to the global dispatcher isn't that great, because it's prone to error. Additional security events may be added in the future and adding these to `RegisterGlobalSecurityEventListenersPass` can easily be missed. When I added `AuthenticationTokenCreatedEvent` in PR #37359 I wasn't aware of this propagation mechanic existed and also no one reviewing the PR noticed it. Additional changes: - Typo fix :) - The `array_uintersect` in `RegisterGlobalSecurityEventListenersPassTest` wasn't implemented correctly * \* That function's behavior is really odd and easy to be used in the wrong way. The callback function isn't intended to return true/false for matching items, but return -1/0/1 like sorting functions. The tests seemingly only worked by chance as returning true/false is doing pretty much the opposite of what the callback function is supposed to do. Commits ------- 1675864 [Security] Fix event propagation for globally registered security events
2 parents c1cb43e + 1675864 commit fb8935e

File tree

2 files changed

+32
-6
lines changed

2 files changed

+32
-6
lines changed

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

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,18 @@
1313

1414
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
1515
use Symfony\Component\DependencyInjection\ContainerBuilder;
16+
use Symfony\Component\Security\Core\AuthenticationEvents;
17+
use Symfony\Component\Security\Core\Event\AuthenticationSuccessEvent;
1618
use Symfony\Component\Security\Http\Event\CheckPassportEvent;
19+
use Symfony\Component\Security\Http\Event\InteractiveLoginEvent;
1720
use Symfony\Component\Security\Http\Event\LoginFailureEvent;
1821
use Symfony\Component\Security\Http\Event\LoginSuccessEvent;
1922
use Symfony\Component\Security\Http\Event\LogoutEvent;
23+
use Symfony\Component\Security\Http\SecurityEvents;
2024

2125
/**
2226
* Makes sure all event listeners on the global dispatcher are also listening
23-
* to events on the firewall-specific dipatchers.
27+
* to events on the firewall-specific dispatchers.
2428
*
2529
* This compiler pass must be run after RegisterListenersPass of the
2630
* EventDispatcher component.
@@ -31,7 +35,18 @@
3135
*/
3236
class RegisterGlobalSecurityEventListenersPass implements CompilerPassInterface
3337
{
34-
private static $eventBubblingEvents = [CheckPassportEvent::class, LoginFailureEvent::class, LoginSuccessEvent::class, LogoutEvent::class];
38+
private static $eventBubblingEvents = [
39+
CheckPassportEvent::class,
40+
LoginFailureEvent::class,
41+
LoginSuccessEvent::class,
42+
LogoutEvent::class,
43+
AuthenticationSuccessEvent::class,
44+
InteractiveLoginEvent::class,
45+
46+
// When events are registered by their name
47+
AuthenticationEvents::AUTHENTICATION_SUCCESS,
48+
SecurityEvents::INTERACTIVE_LOGIN,
49+
];
3550

3651
/**
3752
* {@inheritdoc}

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

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
use Symfony\Component\EventDispatcher\DependencyInjection\RegisterListenersPass;
2020
use Symfony\Component\EventDispatcher\EventDispatcher;
2121
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
22+
use Symfony\Component\Security\Core\AuthenticationEvents;
2223
use Symfony\Component\Security\Http\Event\CheckPassportEvent;
2324
use Symfony\Component\Security\Http\Event\LoginSuccessEvent;
2425
use Symfony\Component\Security\Http\Event\LogoutEvent;
@@ -53,13 +54,15 @@ public function testRegisterCustomListener()
5354

5455
$this->container->register('app.security_listener', \stdClass::class)
5556
->addTag('kernel.event_listener', ['method' => 'onLogout', 'event' => LogoutEvent::class])
56-
->addTag('kernel.event_listener', ['method' => 'onLoginSuccess', 'event' => LoginSuccessEvent::class, 'priority' => 20]);
57+
->addTag('kernel.event_listener', ['method' => 'onLoginSuccess', 'event' => LoginSuccessEvent::class, 'priority' => 20])
58+
->addTag('kernel.event_listener', ['method' => 'onAuthenticationSuccess', 'event' => AuthenticationEvents::AUTHENTICATION_SUCCESS]);
5759

5860
$this->container->compile();
5961

6062
$this->assertListeners([
6163
[LogoutEvent::class, ['app.security_listener', 'onLogout'], 0],
6264
[LoginSuccessEvent::class, ['app.security_listener', 'onLoginSuccess'], 20],
65+
[AuthenticationEvents::AUTHENTICATION_SUCCESS, ['app.security_listener', 'onAuthenticationSuccess'], 0],
6366
]);
6467
}
6568

@@ -79,6 +82,7 @@ public function testRegisterCustomSubscriber()
7982
[LogoutEvent::class, [TestSubscriber::class, 'onLogout'], -200],
8083
[CheckPassportEvent::class, [TestSubscriber::class, 'onCheckPassport'], 120],
8184
[LoginSuccessEvent::class, [TestSubscriber::class, 'onLoginSuccess'], 0],
85+
[AuthenticationEvents::AUTHENTICATION_SUCCESS, [TestSubscriber::class, 'onAuthenticationSuccess'], 0],
8286
]);
8387
}
8488

@@ -95,17 +99,20 @@ public function testMultipleFirewalls()
9599

96100
$this->container->register('app.security_listener', \stdClass::class)
97101
->addTag('kernel.event_listener', ['method' => 'onLogout', 'event' => LogoutEvent::class])
98-
->addTag('kernel.event_listener', ['method' => 'onLoginSuccess', 'event' => LoginSuccessEvent::class, 'priority' => 20]);
102+
->addTag('kernel.event_listener', ['method' => 'onLoginSuccess', 'event' => LoginSuccessEvent::class, 'priority' => 20])
103+
->addTag('kernel.event_listener', ['method' => 'onAuthenticationSuccess', 'event' => AuthenticationEvents::AUTHENTICATION_SUCCESS]);
99104

100105
$this->container->compile();
101106

102107
$this->assertListeners([
103108
[LogoutEvent::class, ['app.security_listener', 'onLogout'], 0],
104109
[LoginSuccessEvent::class, ['app.security_listener', 'onLoginSuccess'], 20],
110+
[AuthenticationEvents::AUTHENTICATION_SUCCESS, ['app.security_listener', 'onAuthenticationSuccess'], 0],
105111
], 'security.event_dispatcher.main');
106112
$this->assertListeners([
107113
[LogoutEvent::class, ['app.security_listener', 'onLogout'], 0],
108114
[LoginSuccessEvent::class, ['app.security_listener', 'onLoginSuccess'], 20],
115+
[AuthenticationEvents::AUTHENTICATION_SUCCESS, ['app.security_listener', 'onAuthenticationSuccess'], 0],
109116
], 'security.event_dispatcher.api');
110117
}
111118

@@ -122,13 +129,15 @@ public function testListenerAlreadySpecific()
122129

123130
$this->container->register('app.security_listener', \stdClass::class)
124131
->addTag('kernel.event_listener', ['method' => 'onLogout', 'event' => LogoutEvent::class, 'dispatcher' => 'security.event_dispatcher.main'])
125-
->addTag('kernel.event_listener', ['method' => 'onLoginSuccess', 'event' => LoginSuccessEvent::class, 'priority' => 20]);
132+
->addTag('kernel.event_listener', ['method' => 'onLoginSuccess', 'event' => LoginSuccessEvent::class, 'priority' => 20])
133+
->addTag('kernel.event_listener', ['method' => 'onAuthenticationSuccess', 'event' => AuthenticationEvents::AUTHENTICATION_SUCCESS]);
126134

127135
$this->container->compile();
128136

129137
$this->assertListeners([
130138
[LogoutEvent::class, ['app.security_listener', 'onLogout'], 0],
131139
[LoginSuccessEvent::class, ['app.security_listener', 'onLoginSuccess'], 20],
140+
[AuthenticationEvents::AUTHENTICATION_SUCCESS, ['app.security_listener', 'onAuthenticationSuccess'], 0],
132141
], 'security.event_dispatcher.main');
133142
}
134143

@@ -146,7 +155,8 @@ private function assertListeners(array $expectedListeners, string $dispatcherId
146155
}
147156

148157
$foundListeners = array_uintersect($expectedListeners, $actualListeners, function (array $a, array $b) {
149-
return $a === $b;
158+
// PHP internally sorts all the arrays first, so returning proper 1 / -1 values is crucial
159+
return $a <=> $b;
150160
});
151161

152162
$this->assertEquals($expectedListeners, $foundListeners);
@@ -161,6 +171,7 @@ public static function getSubscribedEvents(): array
161171
LogoutEvent::class => ['onLogout', -200],
162172
CheckPassportEvent::class => ['onCheckPassport', 120],
163173
LoginSuccessEvent::class => 'onLoginSuccess',
174+
AuthenticationEvents::AUTHENTICATION_SUCCESS => 'onAuthenticationSuccess',
164175
];
165176
}
166177
}

0 commit comments

Comments
 (0)