[SecurityBundle] fix setLogoutOnUserChange calls for context listeners #25272

dmaicher · 2017-12-02T16:50:36Z

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#25267
License	MIT
Doc PR	-

As pointed out in #25267 the setLogoutOnUserChange method calls were added to the parent definition security.context_listener instead of the concrete child definitions security.context_listener.*.

ping @iltar @chalasr

fabpot · 2017-12-02T23:05:58Z

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

@@ -276,15 +276,19 @@ private function createFirewalls($config, ContainerBuilder $container)
                $customUserChecker = true;
            }

+            $configId = 'security.firewall.map.config.'.$name;
+
+            list($matcher, $listeners, $exceptionListener) = $this->createFirewall($container, $name, $firewall, $authenticationProviders, $providerIds, $configId);


Those 2 lines can bu moved back to where they were, right?

chalasr · 2017-12-03T10:02:24Z

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

            $configId = 'security.firewall.map.config.'.$name;

            list($matcher, $listeners, $exceptionListener) = $this->createFirewall($container, $name, $firewall, $authenticationProviders, $providerIds, $configId);

+            foreach ($listeners as $listener) {
+                if (0 === strpos($listenerId = (string) $listener, 'security.context_listener.')) {
+                    $container->getDefinition($listenerId)->addMethodCall('setLogoutOnUserChange', array($firewall['logout_on_user_change']));


Can be done in createContextListener by ~~passing the option value as argument to createFirewall first~~ (it has the whole config), or iterate over $this->contextListeners after the createFirewall loop.

@chalasr done 😉 Will make merging into higher branches a bit harder though 😋

Xymanek · 2017-12-03T14:36:10Z

How will this work if multiple firewalls share the same context?

dmaicher · 2017-12-03T14:39:19Z

@Xymanek hmm I think you are right. That case is still not handled properly 😢

I will look into it.

dmaicher · 2017-12-03T14:49:56Z

@iltar @chalasr any idea how to fix this?

Seems ContextListener::$logoutOnUserChange needs some rethinking as indeed the context instance can be shared amongst multiple firewalls.

Within the ContextListener I don't see any way of knowing which firewall(s) its currently handling? So how to decide $logoutOnUserChange per firewall then?

Or we enforce (with an error when validating the config?) that all firewalls that share a context have the same value for logout_on_user_change?

chalasr · 2017-12-03T16:57:04Z

@dmaicher I think it should throw indeed

dmaicher · 2017-12-03T17:55:04Z

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

+            }
+
+            if (isset($this->firewallsByContextKey[$contextKey]) && $this->firewallsByContextKey[$contextKey][1]['logout_on_user_change'] !== $firewall['logout_on_user_change']) {
+                throw new InvalidConfigurationException(sprintf('Firewalls "%s" and "%s" need to have the same value for option "logout_on_user_change" as they are sharing the context "%s"', $this->firewallsByContextKey[$contextKey][0], $id, $contextKey));


@chalasr done. There is a tiny chance though that this is a BC break if someone set the option already on some firewalls with a shared context.

But the behavior is wrong anyway with that config then 😋

Current behavior is buggy, fine to me

chalasr · 2017-12-04T02:15:16Z

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

+            }
+
+            $this->firewallsByContextKey[$contextKey] = array($id, $firewall);
+            $listeners[] = new Reference($this->createContextListener($container, $contextKey, $firewall['logout_on_user_change']));


must be reached only if the key is set on $firewall. Also I would store the option value only instead of the whole config

must be reached only if the key is set on $firewall.

Not sure I follow. That line was always reached before as well?

Also I would store the option value only instead of the whole config

👍

Now it's accessing $firewall['logout_on_user_change'] which might be not set.
In this case we should not register the firewall in $firewallsByContextKey, perhaps pass null to createContextListener() in order to not add the method call as well

But it did that also before 😉

https://github.com/symfony/symfony/blob/3.4/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php#L283

I actually think the array key will always exist and it cannot be null?

https://github.com/symfony/symfony/blob/3.4/src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php#L251

Forgot that it's a bool node, sorry for the noise!

No problem ;) will update the PR after work

chalasr · 2017-12-04T02:16:45Z

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

@@ -43,6 +43,7 @@ class SecurityExtension extends Extension
    private $factories = array();
    private $userProviderFactories = array();
    private $expressionLanguage;
+    private $firewallsByContextKey = array();


can you add a deprec docblock so that we don't forget to remove it in 5.0?

dmaicher · 2017-12-04T18:04:29Z

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

@@ -340,17 +340,6 @@ private function addFirewallsSection(ArrayNodeDefinition $rootNode, array $facto
                    return $firewall;
                })
            ->end()
-            ->validate()


@chalasr this is now actually not needed anymore since triggering the deprecation is moved and it's not triggered anymore at all if stateless = true or security = false.

good catch!

dmaicher · 2017-12-04T18:05:03Z

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

@@ -43,6 +43,7 @@ class SecurityExtension extends Extension
    private $factories = array();
    private $userProviderFactories = array();
    private $expressionLanguage;
+    private $logoutOnUserChangeByContextKey = array();


@chalasr should I add some deprecation info here? Did not see that for private member variables elsewhere?

Let's let it as is and keep it in mind, note for mergers: this prop should be removed when merging in 4.0 (I can take care of the merge)

chalasr · 2017-12-04T20:04:16Z

Thanks for fixing this bug @dmaicher.

@iltar

…xt listeners (dmaicher) This PR was squashed before being merged into the 3.4 branch (closes #25272). Discussion ---------- [SecurityBundle] fix setLogoutOnUserChange calls for context listeners | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25267 | License | MIT | Doc PR | - As pointed out in #25267 the `setLogoutOnUserChange` method calls were added to the parent definition `security.context_listener` instead of the concrete child definitions `security.context_listener.*`. ping @iltar @chalasr Commits ------- 4eff146 [SecurityBundle] fix setLogoutOnUserChange calls for context listeners

[SecurityBundle] fix setLogoutOnUserChange calls for context listeners

d4a6f6c

carsonbot added Status: Needs Review SecurityBundle Bug and removed Status: Needs Review labels Dec 2, 2017

fabpot requested changes Dec 2, 2017

View reviewed changes

carsonbot added the Status: Needs Work label Dec 2, 2017

review feedback

98abd1e

chalasr reviewed Dec 3, 2017

View reviewed changes

review feedback #2

fb42305

carsonbot added Status: Reviewed and removed Status: Needs Work labels Dec 3, 2017

chalasr approved these changes Dec 3, 2017

View reviewed changes

Simperfit approved these changes Dec 3, 2017

View reviewed changes

dmaicher added 3 commits December 3, 2017 18:51

throw if sharing context with different logout_on_user_change values

d9d7477

throw if sharing context with different logout_on_user_change values

3ece4c6

revert unrelated change

4657298

dmaicher commented Dec 3, 2017

View reviewed changes

carsonbot removed the Status: Reviewed label Dec 4, 2017

chalasr requested changes Dec 4, 2017

View reviewed changes

carsonbot added the Status: Needs Work label Dec 4, 2017

nicolas-grekas added this to the 3.4 milestone Dec 4, 2017

carsonbot added Status: Reviewed and removed Status: Needs Work labels Dec 4, 2017

chalasr approved these changes Dec 4, 2017

View reviewed changes

refactor + cleanup

8174c95

dmaicher commented Dec 4, 2017

View reviewed changes

nicolas-grekas approved these changes Dec 4, 2017

View reviewed changes

fabpot closed this Dec 4, 2017

This was referenced Dec 4, 2017

Release v3.4.1 #25321

Merged

Release v4.0.1 #25322

Merged

Uh oh!

[SecurityBundle] fix setLogoutOnUserChange calls for context listeners #25272

[SecurityBundle] fix setLogoutOnUserChange calls for context listeners #25272

Uh oh!

Conversation

dmaicher commented Dec 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr Dec 3, 2017 • 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!

Xymanek commented Dec 3, 2017

Uh oh!

dmaicher commented Dec 3, 2017

Uh oh!

dmaicher commented Dec 3, 2017

Uh oh!

chalasr commented Dec 3, 2017

Uh oh!

dmaicher Dec 3, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

dmaicher Dec 4, 2017 • 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!

dmaicher Dec 4, 2017 • 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 commented Dec 4, 2017

Uh oh!

Uh oh!

chalasr Dec 3, 2017 •

edited

Loading

dmaicher Dec 3, 2017 •

edited

Loading

dmaicher Dec 4, 2017 •

edited

Loading

dmaicher Dec 4, 2017 •

edited

Loading