-
Notifications
You must be signed in to change notification settings - Fork 11.3k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[5.5] Attempt to fix AuthenticateSession/remember_me issue #19843
Conversation
We'll probably really need to think through what happens when people upgrade from 5.4 to 5.5 if we are changing the handling of the recaller cookie. What will happen? |
All remember_me cookies will be considered invalid and users will be logged out. |
No, it doesn't. =) What is the issue? Do you want to logout a user from all devices they are remembered on when they change their password? |
@sisve yes, we do. |
@sisve sorry this was meant as explanation to Taylor and Adam since we were discussing the issue together, should have mentioned more context :) |
if we talking about it, does it possible in 5.5 to add a simple way of logging out a user from all his devices? |
@@ -461,7 +461,7 @@ protected function ensureRememberTokenIsSet(AuthenticatableContract $user) | |||
protected function queueRecallerCookie(AuthenticatableContract $user) | |||
{ | |||
$this->getCookieJar()->queue($this->createRecaller( | |||
$user->getAuthIdentifier().'|'.$user->getRememberToken() | |||
$user->getAuthIdentifier().'|'.$user->getRememberToken().'|'.$user->getAuthPassword() |
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.
Really sending password here? :/
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 also having trouble understanding how this can be safe, please advise.
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.
It's not the password, it's just the hash.
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.
Thanks. In that case, this was what confused me in Authenticatable.php:
public function getAuthPassword()
{
return $this->password;
}
I think this is important enough to be be mentioned in the Upgrade Guide. It took awhile to find the cause after the 5.5 upgrade. |
This PR explains the issue with the current AuthenticateSession when login is done via the remember_me token.
When the session expires,
$request->session()->has('password_hash')
will return false so it'll log the user out even if he's logged in vieremember_me
.To fix this we store the user password hash while logged in, we save that to the remember_me token, and in case the user is being logged in via remember_me later we'll use that hash to compare with the current hash and see if the user should be logged out.
Note: while upgrading from 5.4 to 5.5 all remember_me cookies will be considered invalid and users will be logged out.