[Security] Deprecated not being logged out after user change #23882

linaori · 2017-08-14T08:33:17Z

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

This PR is an alternative approach to #19033. Due to a behavioral change that could break a lot of applications and websites, I've decided to trigger a deprecation instead of actually changing the behavior as that can be done for 4.0.

Whenever a user object is considered changed (AbstractToken::hasUserChanged) when setting a new user object after refreshing, it will now throw a deprecation, paving the way for a behavioral change in 4.0. The idea is that in 4.0 Symfony will simply trigger a logout when this case is encountered.

linaori · 2017-08-14T08:58:17Z

Test failure not related to this PR it seems

weaverryan · 2017-08-16T13:25:21Z

Woohoo! I'll try to review this tonight. Simple patch... but I'd like to try it in a few situations to check for side effects.

About deprecating this in 3.4 and actually changing it in 4.0, I think I agree. It's a potentially impactful change... and a non-obvious one (if you upgraded to 3.4 and the new code logged you out, you might not notice this at first - as it may only affect an edge-case situation). Due to the fact that this may only happen in some situations, the deprecation also isn't a perfect notifier, but it'll give people a better chance.

weaverryan · 2017-08-16T13:35:02Z

Actually, on second thought, the deprecation is not quite right: there is no action the developer could take on 3.4 to stop hitting the deprecation. Instead, it's just a warning that says "hey! This code path will change in the future". Then, on 4.0, they will have different results than 3.4. That's a problem, since it's not how we intend for deprecations to work.

@iltar wdyt about this?

linaori · 2017-08-16T14:13:18Z

@weaverryan it's a default behavior change that can break a lot of things, so I feel a deprecation is in place. However, you're right about this being kind of passive. What about a temporary parameter that you can turn on to trigger an exception here? Could be done in a similar fashion to several changes in 2.8 and 3.0, like swapping the choice keys/values, where the option gets removed for 5.0.

I'm not entirely sure on how to pass a message to the front-end though.

nicolas-grekas · 2017-08-16T16:03:25Z

src/Symfony/Component/Security/Http/Firewall/ContextListener.php

+                        $this->logger->debug('Token was deauthenticated after trying to refresh it.', array('username' => $refreshedUser->getUsername(), 'provider' => get_class($provider)));
+                    }
+
+                    @trigger_error('Refreshing a deauthenticated user is deprecated as of 3.4 and will trigger a logout in 4.0.', E_USER_DEPRECATED);


a changelog+upgrade entries would be great

I will add them as soon as I know how to proceed with the actual logging out users, as this might need more additions to the changelog and upgrade entries 👍

weaverryan · 2017-08-16T22:40:56Z

Yea, it seems to me, that we indeed do need to do something similar to the choice keys/values thing. Perhaps someone else has an idea but I think it's a good start. So, we could add the following config:

security:
    firewall:
        main:
            logout_on_user_change: true

If this new config key is NOT set, then we'll trigger the deprecation warning. This would require everyone to set this value explicitly (to true or false)... which is kind of annoying, but super explicit. In 4.0, this option would still exist, but would throw an exception if not set. Then, if we wanted, in 5.0, it could default to true (so that you don't need to explicitly configure it). The ContextListener would use this to know if it should log the user out or not.

This is the most conservative option I can think of: it absolutely doesn't break BC... but it's also kind of annoying. If you can't think of a better idea, let's do this first - it's easier to get a +1 or -1 from people if we're looking at real code.

chalasr · 2017-09-03T11:15:40Z

Such key should not be mandatory forstateless firewalls.

chalasr · 2017-09-20T19:16:28Z

@iltar Would you have time to finish this? I can try to help otherwise, feature freeze is coming.

linaori · 2017-09-21T06:16:07Z

@chalasr I'll put it on my todo list for upcoming Friday

linaori · 2017-09-21T09:38:34Z

I've updated the code to actually return null in the case where the context is set to true. I have not yet added this config to the firewall, I first wanted to know if this is okay. It doesn't trigger a message for the user after it was unset though, not sure if that's possible to add in this scenario.

linaori · 2017-09-22T14:24:49Z

Not sure if the test failures are something I can fix, seems to be an issue because it pulls in a different version than what my PR provides

ThePeterMick · 2017-09-22T14:31:32Z

@iltar - thanks a lot for looking into this! Very much appreciated.

In the meantime should we update the page at https://symfony.com/doc/current/security/entity_provider.html#understanding-serialize-and-how-a-user-is-saved-in-the-session, especially the line of "For example, if the username on the 2 User objects doesn't match for some reason, then the user will be logged out for security reasons".

At the moment it sounds like the appropriate description there should be "it checks the properties to determine if the roles should be reloaded"

Just an example right here how people follow this guidance and accept answers (hope they test all that, to find out it is not working): https://stackoverflow.com/questions/42877881/symfony-check-if-users-email-changed-while-logged-in

Also, when you currently change behind the scenes what the isEnabled() function returns between requests, it will log the user out (it is just other properties for which it does not log out the user), do you know where this behvaiour is triggered?

Finally, is there any workaround at the moment for this - to logout the user if any serialized properties change?

Cheers!

linaori · 2017-09-22T16:00:45Z

Finally, is there any workaround at the moment for this - to logout the user if any serialized properties change?

I guess you can reproduce a similar result by overriding some of the pre-defined classes and change the behavior in the authenticators, not 100% sure on that as it's quite a complicated change.

eXtreme · 2017-09-22T18:05:20Z

UPGRADE-3.4.md

@@ -269,6 +269,11 @@ SecurityBundle
    as first argument. Not passing it is deprecated and will throw a `TypeError`
    in 4.0.

+ * Added `logout_on_user_change` to the firewall options. This config item will
+   trigger a logout when the user has changed. should be set to true to avoid


should -> Should

eXtreme · 2017-09-22T18:05:30Z

src/Symfony/Bundle/SecurityBundle/CHANGELOG.md

@@ -13,6 +13,10 @@ CHANGELOG
 * `SetAclCommand::__construct()` now takes an instance of
   `Symfony\Component\Security\Acl\Model\MutableAclProviderInterfaceConnection`
   as first argument
+ * Added `logout_on_user_change` to the firewall options. This config item will
+   trigger a logout when the user has changed. should be set to true to avoid


should -> Should

linaori · 2017-09-25T06:59:40Z

status: Needs Review

ogizanagi · 2017-09-25T07:29:01Z

src/Symfony/Component/Security/Http/Firewall/ContextListener.php

@@ -198,7 +220,7 @@ protected function refreshUser(TokenInterface $token)
        }

        if ($userNotFoundByProvider) {
-            return;
+            return null;


To be reverted as it should be applied to the 2.7 branch. But it is worth a dedicated PR on the whole codebase. Related to #17201.

linaori · 2017-09-26T06:32:18Z

In that case, I've added a note here. It feels a bit weird having a non-deprecated method that does nothing in 4.0, but it's even weirder to have a fresh installation of 4.0 cause a deprecation.

eXtreme · 2017-09-26T08:15:19Z

Great to see that finally being taken care of 👍

chalasr · 2017-09-26T09:27:07Z

src/Symfony/Component/Security/Http/Firewall/ContextListener.php

@@ -61,14 +61,23 @@ public function __construct(TokenStorageInterface $tokenStorage, $userProviders,

        $this->tokenStorage = $tokenStorage;
        $this->userProviders = $userProviders;
-        $this->contextKey = $contextKey;


I think we need to deprecate the constructor argument if the class doesn't need it anymore. That can be done in another PR, could you revert this change (with the property removal)?

It's still used for the sessionKey below. Only the property is removed because it's not used. (However again, it could have been done on the 2.7 branch ^^)

Correct, I've merely removed the property because the property wasn't used anymore. If desired, I can change this in a lower branch instead.

@iltar that would be nice

@chalasr

…tar) This PR was merged into the 2.7 branch. Discussion ---------- Added null as explicit return type (?TokenInterface) | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23882 (comment) | License | MIT | Doc PR | ~ This fixes the returntype in the `ContextListener` so it can be merged upwards. /cc @chalasr Commits ------- 1ba4dd9 Added null as explicit return type (?TokenInterface)

…iltar) This PR was merged into the 2.7 branch. Discussion ---------- Removed unused private property in the ContextListener | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23882 (comment) | License | MIT | Doc PR | ~ The context property is unused, the local variable (constructor argument) is only concatenated to a string inside (session key). Commits ------- 6522c05 Removed unused private property

chalasr · 2017-09-26T11:05:50Z

Thank you @iltar.

…change (iltar) This PR was squashed before being merged into the 3.4 branch (closes #23882). Discussion ---------- [Security] Deprecated not being logged out after user change | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #17023 | License | MIT | Doc PR | ~ This PR is an alternative approach to #19033. Due to a behavioral change that could break a lot of applications and websites, I've decided to trigger a deprecation instead of actually changing the behavior as that can be done for 4.0. Whenever a user object is considered changed (`AbstractToken::hasUserChanged`) when setting a new user object after refreshing, it will now throw a deprecation, paving the way for a behavioral change in 4.0. The idea is that in 4.0 Symfony will simply trigger a logout when this case is encountered. Commits ------- 22f525b [Security] Deprecated not being logged out after user change

weaverryan · 2017-09-26T13:24:14Z

Woohoo! Thank you @iltar!

ThePeterMick · 2017-09-26T14:13:14Z

Thank you very much @iltar!

kbond · 2017-10-11T19:44:46Z

This works as expected for the session but when remember_me is used, the user remains logged in (and authenticated). How can I address this?

chalasr · 2017-10-11T19:52:51Z

@kbond would you mind opening an issue so we can keep track of this bug and discuss the possible solutions if needed?

kbond · 2017-10-11T20:21:09Z

Ok, opened issue #24525

eXtreme · 2017-10-11T21:16:40Z

oh, remember_me, hmm I wonder whether Guard is also affected by this? And if not, how?

kbond · 2017-10-11T22:09:17Z

@eXtreme guard is affected.

see symfony/symfony#23882

carsonbot added Status: Needs Review Security Bug Deprecation labels Aug 14, 2017

nicolas-grekas added this to the 3.4 milestone Aug 14, 2017

nicolas-grekas reviewed Aug 16, 2017

View reviewed changes

chalasr added Status: Needs Work and removed Status: Needs Review labels Sep 10, 2017

chalasr mentioned this pull request Sep 20, 2017

Automatic logout after changing password on another session #19033

Closed

Deprecated not being logged out after user change

908db1e

chalasr self-requested a review September 21, 2017 15:56

eXtreme reviewed Sep 22, 2017

View reviewed changes

carsonbot added Status: Needs Review and removed Status: Needs Work labels Sep 25, 2017

ogizanagi approved these changes Sep 25, 2017

View reviewed changes

linaori mentioned this pull request Sep 25, 2017

Added null as explicit return type (?TokenInterface) #24316

Merged

chalasr approved these changes Sep 25, 2017

View reviewed changes

Added configuration changes for setLogoutOnUserChange

83cff7c

fabpot approved these changes Sep 26, 2017

View reviewed changes

chalasr reviewed Sep 26, 2017

View reviewed changes

Added 4.0 changelog

268e922

linaori mentioned this pull request Sep 26, 2017

Removed unused private property in the ContextListener #24329

Merged

chalasr closed this Sep 26, 2017

xabbuh mentioned this pull request Sep 26, 2017

Document the logout_on_user_change option symfony/symfony-docs#8428

Closed

kbond mentioned this pull request Oct 11, 2017

[Security] logout remember_me user that has changed #24525

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 3, 2017

New Symfony Guard doesn't support user/token refresh/reload #23660

Closed

bmanolov added a commit to chitanka/biblioman that referenced this pull request Nov 11, 2017

add 'logout_on_user_change: true' to default firewall

a2932cf

see symfony/symfony#23882

chalasr mentioned this pull request Dec 15, 2017

[SecurityBundle] add missing logout_on_user_change options #25461

Merged

linaori mentioned this pull request Feb 16, 2018

Invalid form data for user are stored in token #26194

Closed

teohhanhui mentioned this pull request Apr 11, 2018

Deprecation: logout_on_user_change must be set to true on firewall config Sylius/Sylius#9327

Closed

linaori deleted the feature/token-deauthentication branch February 8, 2019 13:38

wouterj mentioned this pull request Jan 8, 2021

[Security] Refresh user roles without having to log out #39763

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Deprecated not being logged out after user change #23882

[Security] Deprecated not being logged out after user change #23882

linaori commented Aug 14, 2017

linaori commented Aug 14, 2017

weaverryan commented Aug 16, 2017

weaverryan commented Aug 16, 2017

linaori commented Aug 16, 2017

nicolas-grekas Aug 16, 2017

linaori Aug 16, 2017

weaverryan commented Aug 16, 2017

chalasr commented Sep 3, 2017

chalasr commented Sep 20, 2017

linaori commented Sep 21, 2017

linaori commented Sep 21, 2017

linaori commented Sep 22, 2017

ThePeterMick commented Sep 22, 2017

linaori commented Sep 22, 2017

eXtreme Sep 22, 2017

eXtreme Sep 22, 2017

linaori commented Sep 25, 2017

ogizanagi Sep 25, 2017 •

edited

Loading

linaori Sep 25, 2017

linaori commented Sep 26, 2017

eXtreme commented Sep 26, 2017

chalasr Sep 26, 2017

ogizanagi Sep 26, 2017 •

edited

Loading

linaori Sep 26, 2017

chalasr Sep 26, 2017

linaori Sep 26, 2017

chalasr commented Sep 26, 2017

weaverryan commented Sep 26, 2017

ThePeterMick commented Sep 26, 2017

kbond commented Oct 11, 2017 •

edited

Loading

chalasr commented Oct 11, 2017

kbond commented Oct 11, 2017

eXtreme commented Oct 11, 2017

kbond commented Oct 11, 2017

[Security] Deprecated not being logged out after user change #23882

[Security] Deprecated not being logged out after user change #23882

Conversation

linaori commented Aug 14, 2017

linaori commented Aug 14, 2017

weaverryan commented Aug 16, 2017

weaverryan commented Aug 16, 2017

linaori commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Aug 16, 2017

chalasr commented Sep 3, 2017

chalasr commented Sep 20, 2017

linaori commented Sep 21, 2017

linaori commented Sep 21, 2017

linaori commented Sep 22, 2017

ThePeterMick commented Sep 22, 2017

linaori commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linaori commented Sep 25, 2017

ogizanagi Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linaori commented Sep 26, 2017

eXtreme commented Sep 26, 2017

Choose a reason for hiding this comment

ogizanagi Sep 26, 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

chalasr commented Sep 26, 2017

weaverryan commented Sep 26, 2017

ThePeterMick commented Sep 26, 2017

kbond commented Oct 11, 2017 • edited Loading

chalasr commented Oct 11, 2017

kbond commented Oct 11, 2017

eXtreme commented Oct 11, 2017

kbond commented Oct 11, 2017

ogizanagi Sep 25, 2017 •

edited

Loading

ogizanagi Sep 26, 2017 •

edited

Loading

kbond commented Oct 11, 2017 •

edited

Loading