[SecurityBundle][Profiler] Give info about called security listeners in profiler #23105

chalasr · 2017-06-08T12:48:10Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#11134
License	MIT
Doc PR	n/a

Currently the profiler gives no info about security listeners (see fixed ticket), this displays each called listener with the time spent at calling it and its response if any.

nicolas-grekas · 2017-06-08T13:06:35Z

src/Symfony/Bundle/SecurityBundle/EventListener/FirewallListener.php

@@ -56,4 +56,12 @@ public function onKernelFinishRequest(FinishRequestEvent $event)

        parent::onKernelFinishRequest($event);
    }
+
+    /**
+     * @internal


I think it's OK to not atg this as internal (same below)

ogizanagi · 2017-06-08T13:25:50Z

src/Symfony/Bundle/SecurityBundle/DataCollector/SecurityDataCollector.php

     */
-    public function __construct(TokenStorageInterface $tokenStorage = null, RoleHierarchyInterface $roleHierarchy = null, LogoutUrlGenerator $logoutUrlGenerator = null, AccessDecisionManagerInterface $accessDecisionManager = null, FirewallMapInterface $firewallMap = null)
+    public function __construct(TokenStorageInterface $tokenStorage = null, RoleHierarchyInterface $roleHierarchy = null, LogoutUrlGenerator $logoutUrlGenerator = null, AccessDecisionManagerInterface $accessDecisionManager = null, TraceableFirewallListener $firewall = null)


This is a BC break, right? What about removing the typehint for now with proper deprecation and BC support, and re-add it on 4.0?

Seems like. We could also inject both FirewallMap and TraceableFirewallListener distinctively, removing the getter added here. WDYT?

Looks good to me to inject both and remove the getter, yes.

ogizanagi · 2017-06-08T13:29:55Z

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

+
+    public function getInfo()
+    {
+        $pretty = get_class($this->listener);


- $pretty + $class

? (I spotted this comes from Symfony\Component\EventDispatcher\Debug\WrappedListener, but there isn't the same treatment here. Let's be more explicit). Do you even need it ?

I started suffixing by ::handle() for finally removing it, ok for $class

I don't need it :) good catch

chalasr · 2017-06-09T08:08:54Z

Comments addressed. Build failure on deps=high is normal because the security component is updated here.

ogizanagi · 2017-06-09T08:26:08Z

src/Symfony/Bundle/SecurityBundle/Resources/config/collectors.xml

@@ -14,6 +14,7 @@
            <argument type="service" id="security.logout_url_generator" />
            <argument type="service" id="security.access.decision_manager" />
            <argument type="service" id="security.firewall.map" />
+            <argument type="service" id="security.firewall" />


If you don't inject explicitly debug.security.firewall here, you can't typehint TraceableFirewallListener, but should add an instanceof check in the collector, otherwise it'll break in non debug mode when the TraceableFirewallListener does not decorates security.firewall (I still don't get why we would have collectors in non-debug mode though...).

IMHO, it's better to keep your collector as you did, but inject debug.security.firewall with on-invalid="null" here.

updated to inject debug.security.firewall or null as suggested

chalasr · 2017-06-11T11:31:00Z

This is ready.

fabpot · 2017-06-11T16:29:11Z

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

+ *
+ * @author Robin Chalas <robin.chalas@gmail.com>
+ *
+ * @final since version 3.4


Why are you using the tag here instead of marking the class as being final directly?

To ease maintenance while keeping extensibility: one could add more info per listener and if a day we don't need this method for dumping the object, the breaking change would be free

I don't get it. This class could even be marked as being internal. I would definitely mark it as final.

fabpot · 2017-06-11T16:29:18Z

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

+ *
+ * @author Robin Chalas <robin.chalas@gmail.com>
+ *
+ * @final since version 3.4


fabpot · 2017-06-11T16:32:09Z

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

+ */
+class WrappedListener implements ListenerInterface
+{
+    public $response;


Why is it public?

It is not anymore

stof · 2017-06-12T15:31:56Z

src/Symfony/Bundle/SecurityBundle/Resources/config/security_debug.xml

@@ -10,5 +10,11 @@
        <service id="debug.security.access.decision_manager" class="Symfony\Component\Security\Core\Authorization\TraceableAccessDecisionManager" decorates="security.access.decision_manager">
            <argument type="service" id="debug.security.access.decision_manager.inner" />
        </service>
+
+        <service id="debug.security.firewall" class="Symfony\Bundle\SecurityBundle\Debug\TraceableFirewallListener" decorates="security.firewall" public="true">


this service should not be public. There is no reason to do a `$container->get('debug.security.firewall')

done but that means the bundle needs a conflict for the dispatcher as event subscribers must be public before 3.3

stof · 2017-06-12T15:33:21Z

src/Symfony/Bundle/SecurityBundle/Resources/config/security_debug.xml

@@ -10,5 +10,11 @@
        <service id="debug.security.access.decision_manager" class="Symfony\Component\Security\Core\Authorization\TraceableAccessDecisionManager" decorates="security.access.decision_manager">
            <argument type="service" id="debug.security.access.decision_manager.inner" />
        </service>
+
+        <service id="debug.security.firewall" class="Symfony\Bundle\SecurityBundle\Debug\TraceableFirewallListener" decorates="security.firewall" public="true">


Using a decorator is weird here, as it does not perform decoration actually (it never uses the inner service).

chalasr · 2017-06-13T08:51:19Z

rebased

fabpot · 2017-06-13T16:07:17Z

Thank you @chalasr.

…rity listeners in profiler (chalasr) This PR was merged into the 3.4 branch. Discussion ---------- [SecurityBundle][Profiler] Give info about called security listeners in profiler | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #11134 | License | MIT | Doc PR | n/a Currently the profiler gives no info about security listeners (see fixed ticket), this displays each called listener with the time spent at calling it and its response if any. ![preview](https://image.prntscr.com/image/Wx-n-Ni_RQK5JGTdTZsdGw.png) Commits ------- 369f19f Give info about called security listeners in profiler

carsonbot added Status: Needs Review SecurityBundle WebProfilerBundle Feature labels Jun 8, 2017

chalasr force-pushed the secu-record-listeners branch from f633d9e to eb6f6b8 Compare June 8, 2017 12:49

chalasr mentioned this pull request Jun 8, 2017

[Feature][Security] Add a TraceableFirewall #11134

Closed

chalasr force-pushed the secu-record-listeners branch from eb6f6b8 to 0450bd4 Compare June 8, 2017 13:00

nicolas-grekas reviewed Jun 8, 2017

View reviewed changes

chalasr force-pushed the secu-record-listeners branch 2 times, most recently from 493097e to 813ce0a Compare June 8, 2017 13:23

ogizanagi reviewed Jun 8, 2017

View reviewed changes

chalasr force-pushed the secu-record-listeners branch 3 times, most recently from 48f5e96 to 1f36aa8 Compare June 8, 2017 21:37

ogizanagi reviewed Jun 9, 2017

View reviewed changes

nicolas-grekas added this to the 3.4 milestone Jun 9, 2017

chalasr force-pushed the secu-record-listeners branch 3 times, most recently from b549295 to 6065367 Compare June 9, 2017 13:08

chalasr force-pushed the secu-record-listeners branch 2 times, most recently from f4be835 to aef1b6f Compare June 11, 2017 12:32

fabpot requested changes Jun 11, 2017

View reviewed changes

chalasr force-pushed the secu-record-listeners branch from aef1b6f to a7514fc Compare June 12, 2017 08:10

fabpot approved these changes Jun 12, 2017

View reviewed changes

stof requested changes Jun 12, 2017

View reviewed changes

chalasr force-pushed the secu-record-listeners branch 3 times, most recently from c72a273 to 3ff46f1 Compare June 13, 2017 00:18

chalasr force-pushed the secu-record-listeners branch from 3ff46f1 to e767231 Compare June 13, 2017 00:24

Give info about called security listeners in profiler

369f19f

chalasr force-pushed the secu-record-listeners branch from e767231 to 369f19f Compare June 13, 2017 08:31

fabpot merged commit 369f19f into symfony:3.4 Jun 13, 2017

chalasr deleted the secu-record-listeners branch June 13, 2017 16:29

yceruto mentioned this pull request Jun 21, 2017

[SecurityBundle] Add user impersonation info and exit action to the profiler #23026

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

chalasr mentioned this pull request Nov 15, 2017

Improve performance of TraceableFirewallListener #24838

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SecurityBundle][Profiler] Give info about called security listeners in profiler #23105

[SecurityBundle][Profiler] Give info about called security listeners in profiler #23105

chalasr commented Jun 8, 2017

nicolas-grekas Jun 8, 2017

ogizanagi Jun 8, 2017

chalasr Jun 8, 2017 •

edited

Loading

ogizanagi Jun 8, 2017

ogizanagi Jun 8, 2017

chalasr Jun 8, 2017

chalasr Jun 8, 2017

chalasr commented Jun 9, 2017

ogizanagi Jun 9, 2017

chalasr Jun 9, 2017

chalasr commented Jun 11, 2017

fabpot Jun 11, 2017

chalasr Jun 11, 2017

fabpot Jun 12, 2017

chalasr Jun 12, 2017

fabpot Jun 11, 2017

fabpot Jun 11, 2017

chalasr Jun 12, 2017

stof Jun 12, 2017

chalasr Jun 13, 2017

stof Jun 12, 2017

chalasr Jun 13, 2017

chalasr commented Jun 13, 2017

fabpot commented Jun 13, 2017

[SecurityBundle][Profiler] Give info about called security listeners in profiler #23105

[SecurityBundle][Profiler] Give info about called security listeners in profiler #23105

Conversation

chalasr commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr commented Jun 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr commented Jun 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr commented Jun 13, 2017

fabpot commented Jun 13, 2017

chalasr Jun 8, 2017 •

edited

Loading