Skip to content

Commit c0e30bb

Browse files
committed
feature symfony#57425 [SecurityBundle] Improve profiler’s data (MatTheCat)
This PR was squashed before being merged into the 7.2 branch. Discussion ---------- [SecurityBundle] Improve profiler’s data | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | N/A | License | MIT Let’s display the profiler for a request matching a lone lazy firewall: ```yaml firewalls: main: lazy: true ``` Since no channel is forced, we know the `ChannelListener` did not run. To make it more obvious, this PR displays `(none)` instead of `0.00 ms` when the duration is null (which will happen once symfony#57369 is merged). <details> <summary>Before</summary> <img src="https://github.com/MatTheCat/symfony/assets/1898254/d8168db1-2a6d-46d9-8fd2-597182327f2f" alt=""> </details> <details> <summary>After</summary> <img src="https://github.com/MatTheCat/symfony/assets/1898254/8bf2345a-5e33-4674-863e-e3a40d30f15a" alt=""> </details> But what about the `ContextListener`? Since the firewall is stateful we know it ran, yet its displayed duration also is `0.00 ms`. Turns out that because the firewall is lazy, the `ContextListener` ran way past the moment the `TraceableFirewallListener` stored its data. In fact, it may be the `SecurityDataCollector` itself which trigger it by accessing the security token. This PR makes the `TraceableFirewallListener` fetch data only when needed, so that they’re up-to-date when the `SecurityDataCollector` asks for them. <details> <summary>Before</summary> <img src="https://github.com/MatTheCat/symfony/assets/1898254/312ee207-7607-4f15-9126-4adc1c14a6bf" alt=""> </details> <details> <summary>After</summary> <img src="https://github.com/MatTheCat/symfony/assets/1898254/0e141294-365c-4ae7-8dc7-d0c390ab700f" alt=""> </details> Now, let’s add a global access control so that the `AccessListener` can do its job: ```yaml access_control: - { roles: ROLE_USER } ``` The profiler then says no security listeners have been recorded 🤔 This is because the `AccessListener` let the `ExceptionListener` work out a response by throwing `AccessDeniedException`s. When this happens, the `TraceableFirewallListener` is cut short before it can store the data it needs (note that it also impacts non-lazy firewalls, but past listeners would then be recorded). This PR stores these data before listeners are called, so that they are available even if one of them throws (this includes authenticators’ data which suffer from the same issue). <details> <summary>Before</summary> <img src="https://github.com/MatTheCat/symfony/assets/1898254/4846ad06-007c-4ff0-ae69-4841bfdf90a2" alt=""> </details> <details> <summary>After</summary> <img src="https://github.com/MatTheCat/symfony/assets/1898254/405dabed-e6aa-4252-8c5f-48d00174f7fd" alt=""> <p>(Other listeners are hidden on this screenshot but they would be displayed in the profiler.)</p> </details> Commits ------- 33bf1cb [SecurityBundle] Improve profiler’s data
2 parents 65e88c8 + 33bf1cb commit c0e30bb

File tree

2 files changed

+38
-44
lines changed

2 files changed

+38
-44
lines changed

src/Symfony/Bundle/SecurityBundle/Debug/TraceableFirewallListener.php

Lines changed: 36 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -27,78 +27,72 @@
2727
final class TraceableFirewallListener extends FirewallListener implements ResetInterface
2828
{
2929
private array $wrappedListeners = [];
30-
private array $authenticatorsInfo = [];
30+
private ?TraceableAuthenticatorManagerListener $authenticatorManagerListener = null;
3131

3232
public function getWrappedListeners(): array
3333
{
34-
return $this->wrappedListeners;
34+
return array_map(
35+
static fn (WrappedListener|WrappedLazyListener $listener) => $listener->getInfo(),
36+
$this->wrappedListeners
37+
);
3538
}
3639

3740
public function getAuthenticatorsInfo(): array
3841
{
39-
return $this->authenticatorsInfo;
42+
return $this->authenticatorManagerListener?->getAuthenticatorsInfo() ?? [];
4043
}
4144

4245
public function reset(): void
4346
{
4447
$this->wrappedListeners = [];
45-
$this->authenticatorsInfo = [];
48+
$this->authenticatorManagerListener = null;
4649
}
4750

4851
protected function callListeners(RequestEvent $event, iterable $listeners): void
4952
{
50-
$wrappedListeners = [];
51-
$wrappedLazyListeners = [];
52-
$authenticatorManagerListener = null;
53-
53+
$requestListeners = [];
5454
foreach ($listeners as $listener) {
5555
if ($listener instanceof LazyFirewallContext) {
56-
\Closure::bind(function () use (&$wrappedLazyListeners, &$wrappedListeners, &$authenticatorManagerListener) {
57-
$listeners = [];
56+
$contextWrappedListeners = [];
57+
$contextAuthenticatorManagerListener = null;
58+
59+
\Closure::bind(function () use (&$contextWrappedListeners, &$contextAuthenticatorManagerListener) {
5860
foreach ($this->listeners as $listener) {
59-
if (!$authenticatorManagerListener && $listener instanceof TraceableAuthenticatorManagerListener) {
60-
$authenticatorManagerListener = $listener;
61-
}
62-
if ($listener instanceof FirewallListenerInterface) {
63-
$listener = new WrappedLazyListener($listener);
64-
$listeners[] = $listener;
65-
$wrappedLazyListeners[] = $listener;
66-
} else {
67-
$listeners[] = function (RequestEvent $event) use ($listener, &$wrappedListeners) {
68-
$wrappedListener = new WrappedListener($listener);
69-
$wrappedListener($event);
70-
$wrappedListeners[] = $wrappedListener->getInfo();
71-
};
61+
if ($listener instanceof TraceableAuthenticatorManagerListener) {
62+
$contextAuthenticatorManagerListener ??= $listener;
7263
}
64+
$contextWrappedListeners[] = $listener instanceof FirewallListenerInterface
65+
? new WrappedLazyListener($listener)
66+
: new WrappedListener($listener)
67+
;
7368
}
74-
$this->listeners = $listeners;
69+
$this->listeners = $contextWrappedListeners;
7570
}, $listener, FirewallContext::class)();
7671

77-
$listener($event);
72+
$this->authenticatorManagerListener ??= $contextAuthenticatorManagerListener;
73+
$this->wrappedListeners = array_merge($this->wrappedListeners, $contextWrappedListeners);
74+
75+
$requestListeners[] = $listener;
7876
} else {
79-
$wrappedListener = $listener instanceof FirewallListenerInterface ? new WrappedLazyListener($listener) : new WrappedListener($listener);
80-
$wrappedListener($event);
81-
$wrappedListeners[] = $wrappedListener->getInfo();
82-
if (!$authenticatorManagerListener && $listener instanceof TraceableAuthenticatorManagerListener) {
83-
$authenticatorManagerListener = $listener;
77+
if ($listener instanceof TraceableAuthenticatorManagerListener) {
78+
$this->authenticatorManagerListener ??= $listener;
8479
}
85-
}
80+
$wrappedListener = $listener instanceof FirewallListenerInterface
81+
? new WrappedLazyListener($listener)
82+
: new WrappedListener($listener)
83+
;
84+
$this->wrappedListeners[] = $wrappedListener;
8685

87-
if ($event->hasResponse()) {
88-
break;
86+
$requestListeners[] = $wrappedListener;
8987
}
9088
}
9189

92-
if ($wrappedLazyListeners) {
93-
foreach ($wrappedLazyListeners as $lazyListener) {
94-
$this->wrappedListeners[] = $lazyListener->getInfo();
95-
}
96-
}
97-
98-
$this->wrappedListeners = array_merge($this->wrappedListeners, $wrappedListeners);
90+
foreach ($requestListeners as $listener) {
91+
$listener($event);
9992

100-
if ($authenticatorManagerListener) {
101-
$this->authenticatorsInfo = $authenticatorManagerListener->getAuthenticatorsInfo();
93+
if ($event->hasResponse()) {
94+
break;
95+
}
10296
}
10397
}
10498
}

src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -318,7 +318,7 @@
318318

319319
<tr>
320320
<td class="font-normal">{{ profiler_dump(listener.stub) }}</td>
321-
<td class="no-wrap">{{ '%0.2f'|format(listener.time * 1000) }} ms</td>
321+
<td class="no-wrap">{{ listener.time is null ? '(none)' : '%0.2f ms'|format(listener.time * 1000) }}</td>
322322
<td class="font-normal">{{ listener.response ? profiler_dump(listener.response) : '(none)' }}</td>
323323
</tr>
324324

@@ -362,7 +362,7 @@
362362
<td class="font-normal">{{ profiler_dump(authenticator.stub) }}</td>
363363
<td class="no-wrap">{{ source('@WebProfiler/Icon/' ~ (authenticator.supports ? 'yes' : 'no') ~ '.svg') }}</td>
364364
<td class="no-wrap">{{ authenticator.authenticated is not null ? source('@WebProfiler/Icon/' ~ (authenticator.authenticated ? 'yes' : 'no') ~ '.svg') : '' }}</td>
365-
<td class="no-wrap">{{ '%0.2f'|format(authenticator.duration * 1000) }} ms</td>
365+
<td class="no-wrap">{{ authenticator.duration is null ? '(none)' : '%0.2f ms'|format(authenticator.duration * 1000) }}</td>
366366
<td class="font-normal">{{ authenticator.passport ? profiler_dump(authenticator.passport) : '(none)' }}</td>
367367
<td class="font-normal">
368368
{% for badge in authenticator.badges ?? [] %}

0 commit comments

Comments
 (0)