Skip to content

[SecurityBundle] Improve profiler’s data #57425

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

MatTheCat
Copy link
Contributor

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:

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 #57369 is merged).

Before
After

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.

Before
After

Now, let’s add a global access control so that the AccessListener can do its job:

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 AccessDeniedExceptions. 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).

Before
After

(Other listeners are hidden on this screenshot but they would be displayed in the profiler.)

@MatTheCat MatTheCat requested a review from chalasr as a code owner June 17, 2024 09:09
@carsonbot carsonbot added this to the 7.2 milestone Jun 17, 2024
@MatTheCat MatTheCat changed the title [SecurityBundle] Improve profiler2019 [SecurityBundle] Improve profiler’s data Jun 17, 2024
@fabpot fabpot force-pushed the security_profiler_lazy branch from 0c51184 to 33bf1cb Compare June 19, 2024 06:18
@fabpot
Copy link
Member

fabpot commented Jun 19, 2024

Thank you @MatTheCat.

@fabpot fabpot merged commit c0e30bb into symfony:7.2 Jun 19, 2024
8 of 10 checks passed
@MatTheCat MatTheCat deleted the security_profiler_lazy branch June 19, 2024 07:44
@fabpot fabpot mentioned this pull request Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants