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

chalasr · 2016-07-20T23:27:07Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes but it should not have any impact in userland
Tests pass?	yes
Fixed tickets	#15294
License	MIT
Doc PR	todo

With this, the FirewallContext class now has a getConfig() method returning a FirewallConfig object representing the firewall configuration.

Also this adds a getFirewallConfig() method to the FirewallMap class of the SecurityBundle.
In a next time, this could be useful to display some firewall related informations to the Profiler, as pointed out in #15294.

Also, it can be useful to be able to access the current firewall configuration from an AuthenticationListener, especially for third party bundles (I can develop on demand).

chalasr · 2016-07-20T23:52:54Z

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

            <argument type="service" id="security.token_storage" on-invalid="null" />
        </service>

+        <service id="security.firewall.map.firewall" class="Symfony\Bundle\SecurityBundle\Security\Firewall" abstract="true">


Hope someone have a better id for this.

chalasr · 2016-07-21T11:01:25Z

Status: Needs work

chalasr · 2016-07-25T22:54:19Z

Status: needs review

HeahDude · 2016-08-03T05:30:13Z

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

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


Yeah I got what the message means, but why making it mandatory?

Is there a good reason to break BC here, this value object does not bring any logic, so keeping a default null in 4.0 does not look wrong to me, although I don't have any strong feeling about this :)

@HeahDude Sorry, I got it now.

There is no "good" reason to break BC here. But in the other hand, I can't find a good one to have FirewallContext::$config being null, as we shouldn't be able to get an instance of FirewallContext without configuring a firewall (it is already documented as "a wrapper around the current firewall configuration" 😄 ).

Also as you pointed out, this deprecation is not mandatory, I stay ready to remove it if your point comes to be shared.

The thing is that the firewall is not depending on this config class for being configured, it can simply live without it. This "aware" interface is more about sharing by exposing, and I guess it was not designed this way before because the less you expose in security the more it's safe, you've noticed that yourself with the listeners config. So this class does not really add any value unless it comes to be collected and displayed (i.e in the profiler).

Also, by removing null as default you should expect a php error instead of an exception unless you throw it yourself in 4.0, meaning keeping the if too to perform the check.

So actually the message should be something like "%s()" expects an instance of "%s" as third argument since version 3.2 and will trigger an error in 4.0.

I got your point. Nothing depends on this argument, so there is no class that can't live without it. I will remove the deprecation.

I won't be as much categorical, because it would mean any code relying on getting the firewall config from the FirewallContext::getConfig() method will have to keep checking the returned value is not null.
We use this method in the profiler, but as some issues were opened mentioning the use cases of getting the firewall name on their own side for their app, it means end users will use this path to retrieve the firewall name from the configuration.
So, if we can avoid this extra check in 4.0, why not doing it ? The FirewallContext class is almost tied to the SecurityBundle anyway. It's unlikely to be extended nor used anywhere else than in the security extension. So, IMHO, making this argument mandatory won't matter.

What @ogizanagi say is what I was trying to express, being not sure of what's the most important.
The FirewallConfig class is not used anywhere at the moment, excepted the work done in the profiler, that checks for the existence of the object for a matter of flexibility and consistency with other collected security-related data. But, the FirewallContext class being accessible through a public service, the primary goal of this PR is to make end users / third party / built-in tools or bundles able to use, expose and share the configuration of the configured firewalls. It can be helpful from a authentication listener to behave differently depending on this config for instance. In these edge cases, even they are not so many, I think it would be a pity to involve checking the existence of this object just for avoiding a (not that impacting) BC break in the future.
So I'm repeating myself but If a FirewallContext is accessible, its configuration should be too (as its service name ends with the firewall name), IMHO without having to worry about whether this one is defined or not (doing it already for the FirewallContext service itself).
I will keep it as is for now, staying ready to change it if a maintainer is opposed to this change.
I have much to learn about that kind of stuff so thanks for the discussion.

Fair enough :)

Deprecation message corrected, good catch @HeahDude

chalasr · 2016-09-11T09:37:17Z

Could we take a decision on this one? The value object added is mostly intended to be consumed by the profiler data collector (see #19490) as other existing ones and btw solve the issue of being able to get informations of the current firewall, adding some value to the FirewallContext class.

fabpot · 2016-09-21T23:59:06Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

-    private function createFirewall(ContainerBuilder $container, $id, $firewall, &$authenticationProviders, $providerIds)
+    private function createFirewall(ContainerBuilder $container, $id, $firewall, &$authenticationProviders, $providerIds, $configId)
    {
+        // FirewallConfig


does not seem useful

fabpot · 2016-09-22T00:04:00Z

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

+namespace Symfony\Bundle\SecurityBundle\Security;
+
+/**
+ * Object representation of a firewall configuration.


I think it can be removed as it does not add anything we don't already know :)

fabpot · 2016-09-22T00:04:09Z

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

+    private $listeners;
+
+    /**
+     * Constructor.


Comment should be removed.

Removed the whole docblock, getters should be enough as this is built internally

fabpot · 2016-09-22T00:04:49Z

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

+    }
+
+    /**
+     * Returns whether the security is enabled or not.


Any comment that is the straightforward translation of the method name can be removed.

These ones are from the base PR #19398. Let me know if I should close it for doing all in this one, otherwise the second commit is what this PR adds.

Sorry, my mistake.

fabpot · 2016-10-04T22:17:51Z

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;


I would return this return statement here.

I would remove this return statement :)

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

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

-use Symfony\Component\Security\Http\FirewallMapInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\DependencyInjection\ContainerInterface;
+use Symfony\Component\Security\Http\FirewallMapInterface;


This should be move up like it was before.

Add a FirewallConfig object, pass it to the FirewallContext Add FirewallContextTest & FirewallConfigTest Populate FirewallConfig definition from SecurityExtension Add missing anonymous listener in FirewallConfig::listenerConfigs Add a functional test Fabbot fixes Fix security option value Add ContextAwareFirewallMapInterface Remove bool casts from getters CS/Spelling Fixes Remove FirewallConfig::listenerConfigs in favor of FirewallConfig::listeners; Add FirewallConfig::allowAnonymous() Add allowAnonymous()/isSecurityEnabled, update comments Fabbot fixes Fix deprecation message Remove interface CS Fixes

fabpot · 2016-11-02T22:45:51Z

👍

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

Thank you @chalasr.

…accessible from FirewallContext (chalasr) This PR was merged into the 3.2-dev branch. Discussion ---------- [DX][SecurityBundle] Introduce a FirewallConfig class accessible from FirewallContext | Q | A | | --- | --- | | Branch? | master | | Bug fix? | no | | New feature? | yes | | BC breaks? | no | | Deprecations? | yes but it should not have any impact in userland | | Tests pass? | yes | | Fixed tickets | #15294 | | License | MIT | | Doc PR | todo | With this, the `FirewallContext` class now has a `getConfig()` method returning a `FirewallConfig` object representing the firewall configuration. Also this adds a `getContext()` method to the `FirewallMap` class of the `SecurityBundle`, to be able to retrieve the current context. In a next time, this could be useful to display some firewall related informations to the Profiler, as pointed out in #15294. Also, it can be useful to be able to access the current firewall configuration from an AuthenticationListener, especially for third party bundles (I can develop on demand). Commits ------- 52d25ed Introduce a FirewallConfig class

…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

This PR was merged into the 3.2-dev branch. Discussion ---------- [Security] improve some firewall config comments | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19398 | License | MIT | Doc PR | Commits ------- cb6c703 [Security] improve some firewall config comments

…chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [SecurityBundle] Rename FirewallContext#getContext() | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a As pointed out in #19398 (comment), the name of this method is misleading. Because a public service using this class is created for each defined firewall, I suggest to change it to `FirewallContext#getListeners()`, deprecating the current `getContext()` for removing it in 4.0. Commits ------- ee66b49 [SecurityBundle] Rename FirewallContext#getContext()

carsonbot added Status: Needs Review Security Feature Deprecation labels Jul 20, 2016

chalasr changed the title ~~[Security] Introduce a Firewall class and service~~ [SecurityBundle] Introduce a Firewall class and service Jul 20, 2016

chalasr force-pushed the firewall_service branch 7 times, most recently from 4a51436 to 3cb9d6c Compare July 20, 2016 23:52

chalasr reviewed Jul 20, 2016
View reviewed changes

chalasr force-pushed the firewall_service branch from 3cb9d6c to 83fc3a4 Compare July 20, 2016 23:53

chalasr mentioned this pull request Jul 21, 2016

[DX][Security] Introduce a class representing a/the current firewall #15294

Closed

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jul 21, 2016

chalasr changed the title ~~[SecurityBundle] Introduce a Firewall class and service~~ [SecurityBundle] Introduce a FirewallConfig class accessible from FirewallContext Jul 22, 2016

chalasr force-pushed the firewall_service branch 5 times, most recently from 3d64dd4 to 0dc120b Compare July 22, 2016 17:49

chalasr changed the title ~~[SecurityBundle] Introduce a FirewallConfig class accessible from FirewallContext~~ [DX][SecurityBundle] Introduce a FirewallConfig class accessible from FirewallContext Jul 22, 2016

chalasr force-pushed the firewall_service branch 4 times, most recently from b7d84be to 033d970 Compare July 25, 2016 22:31

HeahDude reviewed Aug 3, 2016
View reviewed changes

chalasr force-pushed the firewall_service branch 2 times, most recently from ab17be0 to 4aa2242 Compare August 6, 2016 09:00

chalasr force-pushed the firewall_service branch from 4aa2242 to f13bdda Compare September 21, 2016 21:47

fabpot reviewed Sep 22, 2016

View reviewed changes

chalasr force-pushed the firewall_service branch 2 times, most recently from ecc5e12 to 87e402d Compare September 22, 2016 00:23

chalasr force-pushed the firewall_service branch 4 times, most recently from d261970 to c647985 Compare October 12, 2016 09:09

fabpot reviewed Nov 2, 2016

View reviewed changes

chalasr force-pushed the firewall_service branch from c647985 to fc0b374 Compare November 2, 2016 22:26

fabpot reviewed Nov 2, 2016

View reviewed changes

chalasr force-pushed the firewall_service branch from fc0b374 to 52d25ed Compare November 2, 2016 22:44

fabpot merged commit 52d25ed into symfony:master Nov 2, 2016

chalasr deleted the firewall_service branch November 2, 2016 22:53

xabbuh mentioned this pull request Nov 4, 2016

[Security] improve some firewall config comments #20404

Merged

chalasr mentioned this pull request Nov 5, 2016

[SecurityBundle] Rename FirewallContext#getContext() #20417

Merged

fabpot mentioned this pull request Nov 17, 2016

Release v3.2.0-RC1 #20552

Merged

ogizanagi mentioned this pull request Nov 22, 2016

[SecurityBundle] Add a FirewallConfigRegistry service as main entry point #20591

Closed

javiereguiluz mentioned this pull request Nov 25, 2016

Add docs for the Firewall Config class symfony/symfony-docs#7164

Closed

Uh oh!

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

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

Uh oh!

Conversation

chalasr commented Jul 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr commented Jul 21, 2016

Uh oh!

chalasr commented Jul 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagi Aug 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr commented Sep 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Nov 2, 2016

Uh oh!

fabpot commented Nov 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chalasr commented Jul 20, 2016 •

edited

Loading

ogizanagi Aug 3, 2016 •

edited

Loading

chalasr Sep 22, 2016 •

edited

Loading

chalasr Sep 22, 2016 •

edited

Loading