Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] Do not deauthenticate user when the first refreshed user has changed#28072
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedJul 28, 2018
what is the lowest branch where this bug exists? can you check 2.8 please? maybe 3.4 otherwise? |
chalasr left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Needs rebase on 3.4
| 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))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Becomes irrelevant in case another provider succeeds, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, I will move the log at the end of the process
| } | ||
| return$token; | ||
| return$newToken; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
better avoid returning a different instance if possible
| returnnull; | ||
| continue; | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
| $refreshedUser =$provider->refreshUser($user); | ||
| $token->setUser($refreshedUser); | ||
| $newToken =clone$token; | ||
| $newToken->setUser($refreshedUser); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
chalasr commentedJul 29, 2018
chalasr commentedJul 29, 2018
Can you rebase your PR on the 3.4 branch and change the PR target branch accordingly? |
watashi-djo commentedJul 29, 2018
@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 commentedJul 30, 2018
@gpekz No worries :) |
| } | ||
| } | ||
| if (isset($deauthenticationContext)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we prefer assigning tonull by default, checking forif ($deauthenticationContext) instead ofisset
watashi-djo commentedJul 30, 2018
@chalasr Thanks for reviewing :) |
| try { | ||
| $refreshedUser =$provider->refreshUser($user); | ||
| $token->setUser($refreshedUser); | ||
| $newToken =clone$token; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
clone is not always possible. I suppose that we won't have any issues here, but just wanted to point it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 callsetUser() on theoriginal token (which contains the original user) then we can't callisAuthenticated() and break the loop in case it returns true as done below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You are right here. I forgot that we need to check the token with the refreshed user.
But since theTokenInterface is serializable we could maybe stick to$newToken = unserialize(serialize($token)); instead. This should always be safe, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Probably more expensive in term of memory and CPU time but feels a bit more robust.
@nicolas-grekas any thought?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@gpekz I forgot to mention the signature change:public static function hasUserChanged(UserInterface $originalUser, UserInterface $user); so thatAbstractToken would pass$this->user as$originalUser for its internal use and we could use it externally without creating dummy token instances
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
actually we also need to move this parthttps://github.com/symfony/security-core/blob/master/Authentication/Token/AbstractToken.php#L87-L99 inhasUserChanged() and remove any UserInterface typehint from the signature in order to handle strings (and objects implementing__toString(), seehttps://github.com/symfony/security-core/blob/master/Authentication/Token/TokenInterface.php#L60)
watashi-djoSep 21, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But tokens that do not extendAbstractToken use their ownsetUser andisAuthenticated methods here. Doing this change will break it. Isn't it a problem ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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 differencehttps://blackfire.io/profiles/compare/5f7a76a9-7786-4bd0-b360-96e43ad07b1b/graph.
+1 for usingunserialize(serialize($newToken)) instead ofclone $newToken given the token is already safely unserialized beforehand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done :)
weaverryan left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I had to stare at it awhile, but it makes sense to me: don't callsetUser() 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 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 commentedSep 18, 2018
friendly ping @gpekz |
watashi-djo commentedSep 20, 2018
@nicolas-grekas Before continuing, I would like your opinion on the unresolve discussion. |
nicolas-grekas commentedSep 20, 2018
I'd need@chalasr@xabbuh@weaverryan to chime in :) |
| 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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would it make sense to collect these data from all providers that reach this point instead of overwriting it every time?
chalasr commentedOct 10, 2018
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
Uh oh!
There was an error while loading.Please reload this page.
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.