[SecurityBundle] Integrate current firewall in Profiler #19490

chalasr · 2016-07-30T12:08:15Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT

Based on #19398.

This integrates current firewall information into the Profiler.

Toolbar

Panel

Examples:

Show config

main:
    pattern:   ^/
    anonymous: false
    stateless: true
    provider: in_memory
    access_denied_url: /access_denied
    http_basic: ~

Show config

main:
    pattern:   ^/
    anonymous: true
    stateless: false
    provider: in_memory
    context: dummy
    access_denied_url: /access_denied
    http_basic: ~

Show config

api:
    pattern:   ^/
    security: false

HeahDude · 2016-07-30T12:16:58Z

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

+            {% if collector.firewall|default([]) is not empty %}
+            <div class="sf-toolbar-info-piece">
+                <b>Firewall name</b>
+                <span>{{ collector.firewall.name }}</span>


Maybe you should use the default filter as below

Good catch!

HeahDude · 2016-07-30T12:30:08Z

I've left some minor comments but that's a really nice PR, thanks!

chalasr · 2016-07-30T12:36:04Z

Thank you @HeahDude, especially for your inputs about what it could be nice to see. I let you know when I made changes.

chalasr · 2016-07-30T14:23:30Z

@HeahDude I did the changes and updated the description accordingly.
Hope I didn't forget anything.

ogizanagi · 2016-07-30T18:34:04Z

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

+                $this->data['firewall'] = $firewallContext->getConfig();
+            }
+        } else {
+            $this->data['firewall'] = array();


So $this->data['firewall'] may either contain a FirewallConfig instance or an empty array if unavailable ? It's quite strange, isn't it ?
What about not setting it at all if unavailable, and returning false or null in getFirewall() ?

Twig checks will also make more sense:

- {% if collector.firewall|default([]) is not empty %} + {% if collector.firewall %}

What do you think ?

Totally agreed.

While my preference would be to set it to null, the twig check should already work with an empty array:

> var_dump((bool) []); bool(false)

I confirm it works, but let's do object or null.

chalasr · 2016-08-02T18:09:29Z

I made the change and updated the description accordingly (screenshots).
Thanks to reviewers!

chalasr · 2016-08-02T18:29:14Z

Tests have been updated.

ogizanagi · 2016-08-02T18:37:19Z

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

@@ -33,6 +33,12 @@
                <span>{{ collector.tokenClass|abbr_class }}</span>
            </div>
            {% endif %}
+            {% if collector.firewall|default([]) is not empty %}


- {% if collector.firewall|default([]) is not empty %} + {% if collector.firewall %}

chalasr · 2016-08-02T20:13:55Z

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

+        </div>
+    {% else %}
+        <div class="empty">
+            <p>The security component is disabled.</p>


I'm thinking to remove this else statement. As is, this message would appear in both Security Token and Security Firewall sections. Maybe should I move the existing one in a higher position to make it hide the two sections?

chalasr · 2016-08-15T10:25:06Z

This PR and the base one are good to merge (or close).

chalasr · 2016-09-21T23:28:44Z

Updated after #19614

fabpot · 2016-09-21T23:50:39Z

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

@@ -132,6 +137,16 @@ public function collect(Request $request, Response $response, \Exception $except
            $this->data['voter_strategy'] = 'unknown';
            $this->data['voters'] = array();
        }
+
+        // collect firewall context informations


information

chalasr · 2016-09-25T15:12:09Z

I made the changes for using profiler_dump() properly and updated the description accordingly.

chalasr · 2016-10-04T12:50:46Z

@fabpot I think this one is ready

@HeahDude

…ap (chalasr) This PR was merged into the 3.2-dev branch. Discussion ---------- [SecurityBundle] Cache contexts per request in FirewallMap | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19819 (comment) | License | MIT | Doc PR | n/a From @HeahDude in #19819 (comment), I propose to store and retrieve `Context` objects per `Request` using `SplObjectStorage`. At the moment, contexts are consumed by the `Symfony\Components\Security\Http\Firewall` class only, but they could be indirectly by end users if #19490 and/or #19819 come to be merged. Commits ------- ffacec1 [SecurityBundle] Cache contexts per request in FirewallMap

rvanlaak · 2016-10-25T07:40:47Z

This one would be great, because it also would allow to add a "user logout link" to the profiler. Before this PR the required firewall wasn't available. Also see #17037 for a poc.

chalasr · 2016-11-02T18:38:46Z

Imho Security is the most complex thing for beginners. The fact we don't provide informations about it (i.e. the current firewall and all the logic behind it) involves that any security issue requires to look deeper in the core, which leads to misunderstandings and lots of support issues (just answered one by saying to the user to change the order on which firewalls are defined, it would have been easy to debug with this in the profiler).

The lack of feedbacks is too bad here. Shall I close this?

javiereguiluz · 2016-11-02T18:44:51Z

@chalasr please don't close it! You are right about the poor contents of the security profiler panel and debug toolbar.

ro0NL · 2016-11-02T19:33:42Z

src/Symfony/Bundle/SecurityBundle/Security/FirewallConfig.php

+    private $userChecker;
+    private $listeners;
+
+    public function __construct($name, $requestMatcher, $securityEnabled = true, $stateless = false, $provider = null, $context = null, $entryPoint = null, $accessDeniedHandler = null, $accessDeniedUrl = null, $userChecker = null, $listeners = array())


So.. what about __construct(array $config)?

I started with exactly what you propose and came to this.
Having distinct arguments is imho more structuring and gives more visibility about what the class provides. Also it avoids lots of isset($key) ? $key : null, we would have to build $config from SecurityExtension anyway.

Also, please see #19398 for this part, this depends on :)

I see :):) Think you're right.. this represents a config so the arguments dont add much complexity anyway.

ro0NL · 2016-11-02T19:36:15Z

src/Symfony/Bundle/SecurityBundle/Security/FirewallContext.php

+        if (null === $config) {
+            @trigger_error(sprintf('"%s()" expects an instance of "%s" as third argument since version 3.2 and will trigger an error in 4.0 if not provided.', __METHOD__, FirewallConfig::class), E_USER_DEPRECATED);
+
+            return;


Can be removed. Perhaps put the deprecation on top.

fabpot · 2016-11-02T22:52:41Z

Can you rebase now that the related PR is merged?

chalasr · 2016-11-02T23:30:55Z

Rebased

fabpot · 2016-11-02T23:43:56Z

Thank you @chalasr.

…r (chalasr) This PR was merged into the 3.2-dev branch. Discussion ---------- [SecurityBundle] Integrate current firewall in Profiler | Q | A | | --- | --- | | Branch? | master | | Bug fix? | no | | New feature? | yes | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | n/a | | License | MIT | Based on #19398. This integrates current firewall information into the Profiler. **Toolbar** ![Profiler toolbar](http://image.prntscr.com/image/bedec39cea4945e994c8531b80241cf6.png) **Panel** ![Profiler panel](http://image.prntscr.com/image/3b656c1346844c6194a0db42cb8f9fdc.png) Examples: <details> <summary> Show config</summary> ``` yaml main: pattern: ^/ anonymous: false stateless: true provider: in_memory access_denied_url: /access_denied http_basic: ~ ``` </details> ![Panel](http://image.prntscr.com/image/057062a1da744f3c8e00c3c77ded46a8.png) <details> <summary> Show config</summary> ``` yaml main: pattern: ^/ anonymous: true stateless: false provider: in_memory context: dummy access_denied_url: /access_denied http_basic: ~ ``` </details> ![Panel](http://image.prntscr.com/image/a44e54cf018d4bc98c3e0ecf92c37416.png) <details> <summary> Show config</summary> ``` yaml api: pattern: ^/ security: false ``` </details> ![Panel](http://image.prntscr.com/image/c4ea3d7c792447b2ae2b18cd4e08d0dd.png) Commits ------- 75e208e Integrate current firewall in profiler

carsonbot added Status: Needs Review SecurityBundle Feature labels Jul 30, 2016

chalasr force-pushed the firewalls_profiler_infos branch from beb2180 to 9549901 Compare July 30, 2016 12:09

HeahDude reviewed Jul 30, 2016
View reviewed changes

chalasr force-pushed the firewalls_profiler_infos branch from fd9a5d2 to 6c706b7 Compare July 30, 2016 14:02

chalasr force-pushed the firewalls_profiler_infos branch from 6c706b7 to acd4cea Compare July 30, 2016 14:24

ogizanagi reviewed Jul 30, 2016
View reviewed changes

chalasr mentioned this pull request Aug 1, 2016

[DX][SecurityBundle] Introduce a FirewallConfig class accessible from FirewallContext #19398

Merged

chalasr force-pushed the firewalls_profiler_infos branch from acd4cea to 73fe755 Compare August 1, 2016 23:27

chalasr force-pushed the firewalls_profiler_infos branch from 73fe755 to 54a4ec3 Compare August 2, 2016 18:27

ogizanagi reviewed Aug 2, 2016
View reviewed changes

chalasr force-pushed the firewalls_profiler_infos branch from 54a4ec3 to bab3497 Compare August 2, 2016 18:55

chalasr reviewed Aug 2, 2016
View reviewed changes

chalasr force-pushed the firewalls_profiler_infos branch 3 times, most recently from 16a7d92 to da9bc66 Compare August 2, 2016 23:28

chalasr force-pushed the firewalls_profiler_infos branch 2 times, most recently from 3677070 to 0bcf5a2 Compare August 11, 2016 09:21

chalasr force-pushed the firewalls_profiler_infos branch 2 times, most recently from d1e1fb2 to 9a95c6e Compare September 21, 2016 23:27

fabpot reviewed Sep 21, 2016

View reviewed changes

chalasr force-pushed the firewalls_profiler_infos branch 2 times, most recently from cd6c6b7 to 35cd6cd Compare September 25, 2016 14:24

chalasr force-pushed the firewalls_profiler_infos branch from 35cd6cd to 5100137 Compare October 3, 2016 17:19

chalasr mentioned this pull request Oct 10, 2016

[SecurityBundle] Cache contexts per request in FirewallMap #20196

Merged

chalasr force-pushed the firewalls_profiler_infos branch 2 times, most recently from 83970b3 to fd82a18 Compare October 12, 2016 09:21

ro0NL reviewed Nov 2, 2016

View reviewed changes

chalasr force-pushed the firewalls_profiler_infos branch from fd82a18 to be2a55b Compare November 2, 2016 23:15

Integrate current firewall in profiler

75e208e

chalasr force-pushed the firewalls_profiler_infos branch from be2a55b to 75e208e Compare November 2, 2016 23:16

fabpot merged commit 75e208e into symfony:master Nov 2, 2016

chalasr deleted the firewalls_profiler_infos branch November 2, 2016 23:44

fabpot mentioned this pull request Nov 17, 2016

Release v3.2.0-RC1 #20552

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SecurityBundle] Integrate current firewall in Profiler #19490

[SecurityBundle] Integrate current firewall in Profiler #19490

chalasr commented Jul 30, 2016 •

edited

Loading

HeahDude Jul 30, 2016

chalasr Jul 30, 2016

HeahDude commented Jul 30, 2016 •

edited

Loading

chalasr commented Jul 30, 2016

chalasr commented Jul 30, 2016

ogizanagi Jul 30, 2016 •

edited

Loading

chalasr Jul 30, 2016

linaori Jul 31, 2016

chalasr Jul 31, 2016

chalasr commented Aug 2, 2016

chalasr commented Aug 2, 2016

ogizanagi Aug 2, 2016

chalasr Aug 2, 2016

chalasr Aug 2, 2016 •

edited

Loading

chalasr commented Aug 15, 2016

chalasr commented Sep 21, 2016 •

edited

Loading

fabpot Sep 21, 2016

chalasr Sep 22, 2016

chalasr commented Sep 25, 2016

chalasr commented Oct 4, 2016

rvanlaak commented Oct 25, 2016 •

edited

Loading

chalasr commented Nov 2, 2016 •

edited

Loading

javiereguiluz commented Nov 2, 2016

ro0NL Nov 2, 2016

chalasr Nov 2, 2016

ro0NL Nov 2, 2016

ro0NL Nov 2, 2016

fabpot commented Nov 2, 2016

chalasr commented Nov 2, 2016

fabpot commented Nov 2, 2016

[SecurityBundle] Integrate current firewall in Profiler #19490

[SecurityBundle] Integrate current firewall in Profiler #19490

Conversation

chalasr commented Jul 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude commented Jul 30, 2016 • edited Loading

chalasr commented Jul 30, 2016

chalasr commented Jul 30, 2016

ogizanagi Jul 30, 2016 • 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

chalasr commented Aug 2, 2016

chalasr commented Aug 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Aug 2, 2016 • edited Loading

Choose a reason for hiding this comment

chalasr commented Aug 15, 2016

chalasr commented Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr commented Sep 25, 2016

chalasr commented Oct 4, 2016

rvanlaak commented Oct 25, 2016 • edited Loading

chalasr commented Nov 2, 2016 • edited Loading

javiereguiluz commented Nov 2, 2016

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

fabpot commented Nov 2, 2016

chalasr commented Nov 2, 2016

fabpot commented Nov 2, 2016

chalasr commented Jul 30, 2016 •

edited

Loading

HeahDude commented Jul 30, 2016 •

edited

Loading

ogizanagi Jul 30, 2016 •

edited

Loading

chalasr Aug 2, 2016 •

edited

Loading

chalasr commented Sep 21, 2016 •

edited

Loading

rvanlaak commented Oct 25, 2016 •

edited

Loading

chalasr commented Nov 2, 2016 •

edited

Loading