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

ogizanagi · 2016-11-22T12:36:45Z

Q	A
Branch?	3.2
Bug fix?	no
New feature?	yes (but rework a new 3.2 feature)
BC breaks?	no
Deprecations?	no (removes one)
Tests pass?	yes
Fixed tickets	20585#issuecomment-262085751
License	MIT
Doc PR	N/A

This PR reworks some parts of #19398 for solving issues like #20585 about accessing the firewall configs in different ways.
The main entry point for getting firewall configs becomes the security.firewall.config_registry service.
FirewallMap & FirewallContext stay unchanged, as they are more about the security execution context than getting the configuration.

It also removes the deprecation introduced in FirewallContext (and even in case this one is not merged, I'd like to challenge this deprecation which seems superfluous instead of simply allowing null)

However, if the only real uses-cases are only to access the config at runtime from the firewall name, we may not need that "complexity" and refactoring (see #20592 instead).

WDYT?

cc @chalasr

ogizanagi · 2016-11-22T17:36:03Z

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

+     */
+    private function getRequestMatcher(FirewallConfig $config)
+    {
+        if (empty($config->getRequestMatcher())) {


Note: this would be replaced by if (null === $config->getRequestMatcher()) { if #20589 is merged

In case of this one is considered before #20589, I think it should be if (!$config->getRequestMatcher())

I prefer empty which is semantically better IMHO.

chalasr · 2016-11-22T19:15:54Z

Imho, adding this registry makes sense as soon as (and only if) we need some more accessors for the FirewallConfig.

The current() method makes sense, but you can get the same result with the current implementation using FirewallMap::getFirewallConfig($requestStack->getCurrentRequest()) which is exactly what the fromRequest() method added here does, and I do think using FirewallMap is good enough for that.

Given I can't find valid use cases forall() and inContext() (maybe I missed one?) and the need reported in #20585 (corresponding to get() added here) can be easily solved by #20592 instead (making the service public), I would say we don't need such a service right now.

Of course that's just my opinion and I may be wrong but I would favor #20592 instead of this one, even if your implementation looks very good.

edit: considering the use cases, maybe it's worth it.

ogizanagi · 2016-11-22T19:34:45Z

inContext() was motivated by the need to identify firewalls sharing a same context in #20516, but we've injected the context along with the logout listener informations instead.

all() may be used to collect informations about the application, to complete the profiler for instance (firewalls order, request matching informations, ...).

Despite I prefer having this clear entrypoint separated from the security execution context, I do think identifying proper use cases to legitimate this approach is required. Maybe @julienfalque could give us some hints?

chalasr · 2016-11-22T17:49:10Z

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

+            }
+        }
+
+        return $configs;


Maybe

- $configs = array(); - foreach ($this->firewallConfigs as $config) { - if ($config->getContext() === $context) { - $configs[] = $config; - } - } - - return $configs; + return array_filter($this->firewallConfigs, function (FirewallConfig $config) { + return $context === config->getContext(); + });

?

chalasr · 2016-11-22T17:52:06Z

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

+     */
+    private function getRequestMatcher(FirewallConfig $config)
+    {
+        if (empty($config->getRequestMatcher())) {


In case of this one is considered before #20589, I think it should be if (!$config->getRequestMatcher())

…oint for firewall configs

julienfalque · 2016-11-22T20:20:33Z

I need to be able to retrieve a FirewallConfig by name for #20426. This would allow me to retrieve the config of a Firewall and use matching field names (actually with more work to retrieve firewall options, but this will probably be another PR).

Making the FirewallConfig public could be a solution but, as I will know the firewall name at runtime (form type option), it would require to either inject the container or all the FirewallConfig instances using tags. I think this dedicated FirewallConfigRegistry service is an easier and more elegant solution.

nicolas-grekas · 2016-11-23T13:33:51Z

This one is way too late in the 3.2 release cycle IMHO. Is it superseded by #20592?

ogizanagi · 2016-11-23T14:11:49Z

Of course I suspected this. The time left before releasing 3.2 is probably too short for such a change, but If we want to do it later, we fall in the deprecation cycle. So I had to at least try.

#20592 would be enough for use-cases I can imagine right now. But as you said, this is actually solvable by defining aliases on userland.

nicolas-grekas · 2016-11-23T14:13:00Z

So maybe close this one until someone comes with a use case?

chalasr · 2016-11-23T14:16:31Z

@nicolas-grekas Apparently there is one use case at least (#20591 (comment)) for retrieving firewall config by name at runtime without having the whole container injected, so maybe this one is good to keep on 3.3.

Another alternative could be to add the logic needed by #20591 (comment) in the FirewallMap, like FirewallMap::getFirewallConfig($name); which makes sense imho, but it would be for 3.3 anyway.

ogizanagi · 2016-11-23T14:23:06Z

As a side note: WDYT about removing the deprecation in FirewallContext ?

Having the configuration object on the FirewallContext is a bonus and isn't required for the security flow to work.
If this PR is legitimated for 3.3, we'll deprecate this argument from the constructor anyway in order to remove it in 4.0.

chalasr · 2016-11-23T14:29:38Z

👍 for removing the deprecation on 3.2.

nicolas-grekas · 2016-11-23T14:33:09Z

@ogizanagi can you update #20589 to remove the deprecation?

ogizanagi · 2016-11-23T14:39:06Z

Done. Closing this one for now.

julienfalque · 2016-11-23T17:53:21Z

So... Is this totally abandoned or do we still want it on 3.3? I don't think using public aliases will help for the use case I mentioned.

ogizanagi · 2016-11-23T18:17:52Z

@julienfalque : We must have enough arguments for this to happen in 3.3, because it'll require extra work to deprecate paths introduced in 3.2 and ensuring BC until 4.0. That's why I thought we may rework it before it's too late and 3.2 is released...but it's already too late 😅

Note that #20585 may still happen in 3.3 as is if we don't mind mixing responsibilities between the security flow and getting the config objects.

@chalasr

…izanagi) This PR was squashed before being merged into the 3.2 branch (closes #20589). Discussion ---------- [SecurityBundle] Fix FirewallConfig nullable arguments | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no, but removes one | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A Nullable arguments were replaced by empty strings by the DIC config if values weren't replaced in the extension. Another solution (looking safer to me) is to apply the following changes in the DIC configuration: ```diff diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml b/src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml index 54e5734..499dd5e 100644 --- a/src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml +++ b/src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml @@ -116,15 +116,15 @@ <service id="security.firewall.config" class="Symfony\Bundle\SecurityBundle\Security\FirewallConfig" abstract="true" public="false"> <argument />  - <argument />  - <argument />  - <argument />  - <argument />  - <argument />  - <argument />  - <argument />  - <argument />  - <argument />  + <argument>null</argument>  + <argument>null</argument>  + <argument>true</argument>  + <argument>false</argument>  + <argument>null</argument>  + <argument>null</argument>  + <argument>null</argument>  + <argument>null</argument>  + <argument>null</argument>  <argument type="collection" />  </service> ``` But it doesn't seem to be the way we deal with arguments meant to be replaced by extensions in the Symfony core. cc @chalasr This PR also removes the FirewallContext mandatory `FirewallConfig` argument deprecation for reasons expressed in #20591 (comment) Commits ------- 79ef474 [SecurityBundle] Remove FirewallContext mandatory FirewallConfig argument deprecation f09ccf4 [SecurityBundle] Fix FirewallConfig nullable arguments

carsonbot added Status: Needs Review SecurityBundle Feature labels Nov 22, 2016

This was referenced Nov 22, 2016

[SecurityBundle] Make FirewallConfig services public #20592

Closed

[SecurityBundle] Method to retrieve firewall config by name #20585

Closed

ogizanagi commented Nov 22, 2016

View reviewed changes

chalasr reviewed Nov 22, 2016

View reviewed changes

[SecurityBundle] Add a FirewallConfigRegistry service as main entry p…

755399a

…oint for firewall configs

ogizanagi mentioned this pull request Nov 23, 2016

[SecurityBundle] Fix FirewallConfig nullable arguments #20589

Merged

ogizanagi closed this Nov 23, 2016

ogizanagi deleted the firewall_config_registry branch November 23, 2016 14:39

ogizanagi mentioned this pull request Nov 25, 2016

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

Closed

julienfalque mentioned this pull request Nov 26, 2016

[SecurityBundle] Add a login form Type #20426

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ogizanagi commented Nov 22, 2016 •

edited

Loading

ogizanagi Nov 22, 2016

chalasr Nov 22, 2016

ogizanagi Nov 22, 2016

chalasr commented Nov 22, 2016 •

edited

Loading

ogizanagi commented Nov 22, 2016 •

edited

Loading

chalasr Nov 22, 2016

ogizanagi Nov 22, 2016

chalasr Nov 22, 2016

julienfalque commented Nov 22, 2016

nicolas-grekas commented Nov 23, 2016

ogizanagi commented Nov 23, 2016 •

edited

Loading

nicolas-grekas commented Nov 23, 2016

chalasr commented Nov 23, 2016

ogizanagi commented Nov 23, 2016

chalasr commented Nov 23, 2016

nicolas-grekas commented Nov 23, 2016

ogizanagi commented Nov 23, 2016

julienfalque commented Nov 23, 2016

ogizanagi commented Nov 23, 2016

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

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

Conversation

ogizanagi commented Nov 22, 2016 • edited Loading

ogizanagi Nov 22, 2016

Choose a reason for hiding this comment

chalasr Nov 22, 2016

Choose a reason for hiding this comment

ogizanagi Nov 22, 2016

Choose a reason for hiding this comment

chalasr commented Nov 22, 2016 • edited Loading

ogizanagi commented Nov 22, 2016 • edited Loading

chalasr Nov 22, 2016

Choose a reason for hiding this comment

ogizanagi Nov 22, 2016

Choose a reason for hiding this comment

chalasr Nov 22, 2016

Choose a reason for hiding this comment

julienfalque commented Nov 22, 2016

nicolas-grekas commented Nov 23, 2016

ogizanagi commented Nov 23, 2016 • edited Loading

nicolas-grekas commented Nov 23, 2016

chalasr commented Nov 23, 2016

ogizanagi commented Nov 23, 2016

chalasr commented Nov 23, 2016

nicolas-grekas commented Nov 23, 2016

ogizanagi commented Nov 23, 2016

julienfalque commented Nov 23, 2016

ogizanagi commented Nov 23, 2016

ogizanagi commented Nov 22, 2016 •

edited

Loading

chalasr commented Nov 22, 2016 •

edited

Loading

ogizanagi commented Nov 22, 2016 •

edited

Loading

ogizanagi commented Nov 23, 2016 •

edited

Loading