Skip to content

Commit

Permalink
Require the correct password to rehash it
Browse files Browse the repository at this point in the history
  • Loading branch information
mboynes committed Mar 15, 2021
1 parent c02e325 commit 580fb32
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 4 deletions.
24 changes: 21 additions & 3 deletions src/Illuminate/Auth/SessionGuard.php
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,26 @@ protected function cycleRememberToken(AuthenticatableContract $user)
$this->provider->updateRememberToken($user, $token);
}

/**
* Rehash the user's password.
*
* @param string $password
* @param string $attribute
* @return bool|null
*
* @throws AuthenticationException If the password is invalid.
*/
protected function rehashUserPassword($password, $attribute)
{
if (! Hash::check($password, $this->user()->$attribute)) {

This comment has been minimized.

Copy link
@cgaube

cgaube May 31, 2021

Hello - Just to point out that this will never work if the Authenticable object returned by $this->user() does not use attribute to retrieve the password .

Ideally we should use getAuthPassword instead

if (! Hash::check($password, $this->user()->getAuthPassword())) {

For example in my application my Authenticable is not an Eloquent model at all.

(i did have to create a forceFill function in order for this function to work though)

This comment has been minimized.

Copy link
@cgaube

cgaube May 31, 2021

This lead to another comment : we should probably no use forceFill either - but use a new function defined by the
Illuminate\Contracts\Auth\Authenticatable instead

throw new AuthenticationException('Password mismatch.');
}

return tap($this->user()->forceFill([
$attribute => Hash::make($password),
]))->save();
}

/**
* Invalidate other sessions for the current user.
*
Expand All @@ -588,9 +608,7 @@ public function logoutOtherDevices($password, $attribute = 'password')
return;
}

$result = tap($this->user()->forceFill([
$attribute => Hash::make($password),
]))->save();
$result = $this->rehashUserPassword($password, $attribute);

if ($this->recaller() ||
$this->getCookieJar()->hasQueued($this->getRecallerName())) {
Expand Down
17 changes: 16 additions & 1 deletion tests/Integration/Auth/AuthenticationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Illuminate\Tests\Integration\Auth;

use Illuminate\Auth\AuthenticationException;
use Illuminate\Auth\EloquentUserProvider;
use Illuminate\Auth\Events\Attempting;
use Illuminate\Auth\Events\Authenticated;
Expand Down Expand Up @@ -211,7 +212,7 @@ public function testLoggingOutOtherDevices()

$this->assertEquals(1, $user->id);

$this->app['auth']->logoutOtherDevices('adifferentpassword');
$this->app['auth']->logoutOtherDevices('password');
$this->assertEquals(1, $user->id);

Event::assertDispatched(OtherDeviceLogout::class, function ($event) {
Expand All @@ -222,6 +223,20 @@ public function testLoggingOutOtherDevices()
});
}

public function testPasswordMustBeValidToLogOutOtherDevices()
{
$this->expectException(AuthenticationException::class);
$this->expectExceptionMessage('Password mismatch.');

$this->app['auth']->loginUsingId(1);

$user = $this->app['auth']->user();

$this->assertEquals(1, $user->id);

$this->app['auth']->logoutOtherDevices('adifferentpassword');
}

public function testLoggingInOutViaAttemptRemembering()
{
$this->assertTrue(
Expand Down

0 comments on commit 580fb32

Please sign in to comment.