[Security] Do not deauthenticate user when the first refreshed user has changed #28072

watashi-djo · 2018-07-26T22:12:36Z

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

Currently the token is deauthenticated when the first refreshed user has changed. In theory, a second user provider could find a user that is the same than the user stored in the token.

Also, the deauthentication is currently affected by the order of the user providers in the security.yaml and IMHO it does not make sense.

nicolas-grekas · 2018-07-28T15:56:59Z

what is the lowest branch where this bug exists? can you check 2.8 please? maybe 3.4 otherwise?

chalasr

Needs rebase on 3.4

chalasr · 2018-07-28T20:00:07Z

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


                // tokens can be deauthenticated if the user has been changed.
-                if (!$token->isAuthenticated()) {
+                if (!$newToken->isAuthenticated()) {
                    if (null !== $this->logger) {
                        $this->logger->debug('Token was deauthenticated after trying to refresh it.', array('username' => $refreshedUser->getUsername(), 'provider' => \get_class($provider)));


Becomes irrelevant in case another provider succeeds, right?

Yes, I will move the log at the end of the process

chalasr · 2018-07-28T20:02:31Z

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

@@ -193,7 +196,7 @@ protected function refreshUser(TokenInterface $token)
                    $this->logger->debug('User was reloaded from a user provider.', $context);
                }

-                return $token;
+                return $newToken;


better avoid returning a different instance if possible

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


-                    return null;
+                    continue;
                }


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

@@ -169,15 +169,14 @@ protected function refreshUser(TokenInterface $token)

            try {
                $refreshedUser = $provider->refreshUser($user);
-                $token->setUser($refreshedUser);
+                $newToken = clone $token;
+                $newToken->setUser($refreshedUser);


chalasr · 2018-07-29T10:55:48Z

ping @iltar @weaverryan

chalasr · 2018-07-29T12:44:37Z

Can you rebase your PR on the 3.4 branch and change the PR target branch accordingly?

watashi-djo · 2018-07-29T20:05:46Z

@chalasr Rebase is done. I made a mistake during this one, it removed some of your comments and automatically requested a review of dunglas. Sorry for this !

chalasr · 2018-07-30T09:05:44Z

@gpekz No worries :)

chalasr · 2018-07-30T09:08:27Z

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

@@ -209,6 +210,14 @@ protected function refreshUser(TokenInterface $token)
            }
        }

+        if (isset($deauthenticationContext)) {


we prefer assigning to null by default, checking for if ($deauthenticationContext) instead of isset

watashi-djo · 2018-07-30T13:11:04Z

@chalasr Thanks for reviewing :)

fabpot · 2018-08-02T09:40:14Z

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

@@ -169,21 +170,22 @@ protected function refreshUser(TokenInterface $token)

            try {
                $refreshedUser = $provider->refreshUser($user);
-                $token->setUser($refreshedUser);
+                $newToken = clone $token;


clone is not always possible. I suppose that we won't have any issues here, but just wanted to point it out.

I think we should not clone here. We can just keep the refreshed user and replace the one in the token after all user providers have been processed.

I'm not sure to understand what you mean.
Are you suggesting to call $token->setUser() only once with the user returned by the last user provider of the loop? The right ("unchanged") user could be in the middle so we need to compare them all, right?
I mean, the "authenticated" state is part of the token. If we don't call setUser() on the original token (which contains the original user) then we can't call isAuthenticated() and break the loop in case it returns true as done below.

You are right here. I forgot that we need to check the token with the refreshed user.

But since the TokenInterface is serializable we could maybe stick to $newToken = unserialize(serialize($token)); instead. This should always be safe, right?

Probably more expensive in term of memory and CPU time but feels a bit more robust.
@nicolas-grekas any thought?

@gpekz I forgot to mention the signature change:
public static function hasUserChanged(UserInterface $originalUser, UserInterface $user); so that AbstractToken would pass $this->user as $originalUser for its internal use and we could use it externally without creating dummy token instances

actually we also need to move this part https://github.com/symfony/security-core/blob/master/Authentication/Token/AbstractToken.php#L87-L99 in hasUserChanged() and remove any UserInterface typehint from the signature in order to handle strings (and objects implementing __toString(), see https://github.com/symfony/security-core/blob/master/Authentication/Token/TokenInterface.php#L60)

But tokens that do not extend AbstractToken use their own setUser and isAuthenticated methods here. Doing this change will break it. Isn't it a problem ?

@gpekz I think you're right, it's not good to rely on hasUserChanged as it's AbstractToken specific.
Compared clone to serialize on 10 providers with 9/10 failing to refresh and got no significative difference https://blackfire.io/profiles/compare/5f7a76a9-7786-4bd0-b360-96e43ad07b1b/graph.

+1 for using unserialize(serialize($newToken)) instead of clone $newToken given the token is already safely unserialized beforehand.

weaverryan

I had to stare at it awhile, but it makes sense to me: don't call setUser() on the token until you actually successfully refresh the user. And allow subsequent user providers an opportunity to refresh, using the original, unmodified token.

And yes, looks like 3.4 is the right branch - the "logging out on user change" wasn't in 2.8.

nicolas-grekas

We can just keep the refreshed user and replace the one in the token after all user providers have been processed.

Possible? Putting a "changes required" review to not merge before resolving :)

nicolas-grekas · 2018-09-18T12:20:50Z

friendly ping @gpekz

watashi-djo · 2018-09-20T14:09:37Z

friendly ping @gpekz

@nicolas-grekas Before continuing, I would like your opinion on the unresolve discussion.

nicolas-grekas · 2018-09-20T14:37:34Z

I'd need @chalasr @xabbuh @weaverryan to chime in :)

xabbuh · 2018-10-04T18:20:45Z

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

-                        if (null !== $this->logger) {
-                            $this->logger->debug('Token was deauthenticated after trying to refresh it.', array('username' => $refreshedUser->getUsername(), 'provider' => \get_class($provider)));
-                        }
+                        $deauthenticationContext = array('username' => $refreshedUser->getUsername(), 'provider' => \get_class($provider));


Would it make sense to collect these data from all providers that reach this point instead of overwriting it every time?

…as changed

chalasr · 2018-10-10T08:06:09Z

Nice first contribution @gpekz, thank you.

…shed user has changed (gpekz) This PR was squashed before being merged into the 3.4 branch (closes #28072). Discussion ---------- [Security] Do not deauthenticate user when the first refreshed user has changed | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Currently the token is deauthenticated when the first refreshed user has changed. In theory, a second user provider could find a user that is the same than the user stored in the token. Also, the deauthentication is currently affected by the order of the user providers in the security.yaml and IMHO it does not make sense. Commits ------- 95dce67 [Security] Do not deauthenticate user when the first refreshed user has changed

carsonbot added Status: Needs Review Security Bug BC Break labels Jul 26, 2018

chalasr reviewed Jul 28, 2018

View reviewed changes

chalasr added this to the 3.4 milestone Jul 28, 2018

chalasr reviewed Jul 29, 2018

View reviewed changes

chalasr removed the BC Break label Jul 29, 2018

watashi-djo requested a review from dunglas as a code owner July 29, 2018 18:46

watashi-djo changed the base branch from master to 3.4 July 29, 2018 18:46

watashi-djo closed this Jul 29, 2018

watashi-djo reopened this Jul 29, 2018

chalasr reviewed Jul 30, 2018

View reviewed changes

chalasr approved these changes Jul 30, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jul 30, 2018

fabpot reviewed Aug 2, 2018

View reviewed changes

weaverryan approved these changes Aug 8, 2018

View reviewed changes

nicolas-grekas requested changes Aug 10, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Aug 10, 2018

carsonbot removed the Status: Needs Work label Oct 1, 2018

chalasr approved these changes Oct 1, 2018

View reviewed changes

carsonbot added the Status: Reviewed label Oct 1, 2018

xabbuh reviewed Oct 4, 2018

View reviewed changes

[Security] Do not deauthenticate user when the first refreshed user h…

95dce67

…as changed

chalasr merged commit 95dce67 into symfony:3.4 Oct 10, 2018

This was referenced Nov 3, 2018

Release v3.4.18 #29070

Merged

Release v4.1.7 #29071

Merged

javiereguiluz mentioned this pull request Dec 15, 2018

Possible issue with unserialize in PHP 7.3 #29459

Closed

[Security] Do not deauthenticate user when the first refreshed user has changed #28072

[Security] Do not deauthenticate user when the first refreshed user has changed #28072

Conversation

watashi-djo commented Jul 26, 2018 • edited by chalasr Loading

nicolas-grekas commented Jul 28, 2018

chalasr left a comment

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

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

chalasr commented Jul 29, 2018

chalasr commented Jul 29, 2018

watashi-djo commented Jul 29, 2018

chalasr commented Jul 30, 2018

Choose a reason for hiding this comment

watashi-djo commented Jul 30, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

watashi-djo Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan left a comment • edited Loading

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Sep 18, 2018

watashi-djo commented Sep 20, 2018

nicolas-grekas commented Sep 20, 2018

Choose a reason for hiding this comment

chalasr commented Oct 10, 2018

watashi-djo commented Jul 26, 2018 •

edited by chalasr

Loading

chalasr Sep 21, 2018 •

edited

Loading

watashi-djo Sep 21, 2018 •

edited

Loading

weaverryan left a comment •

edited

Loading