Skip to content

[10.x] Support rehashing user passwords with BC support #48673

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

[10.x] Support rehashing user passwords with BC support #48673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f4522a8
Support rehashing user passwords with BC support
valorin Oct 9, 2023
d24b3fe
Compare hashes as sanity check for valid attribute
valorin Oct 9, 2023
74a3642
Fix style violations
valorin Oct 9, 2023
a94f5e5
Use FQN
valorin Oct 9, 2023
1cc1e56
Remove redundant conditional
valorin Oct 9, 2023
adcb353
Update DatabaseUserProvider.php
taylorotwell Oct 9, 2023
57a3de9
Update EloquentUserProvider.php
taylorotwell Oct 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 36 additions & 3 deletions src/Illuminate/Auth/DatabaseUserProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,41 @@ protected function getGenericUser($user)
*/
public function validateCredentials(UserContract $user, array $credentials)
{
return $this->hasher->check(
$credentials['password'], $user->getAuthPassword()
);
if (is_null($plain = $credentials['password'])) {
return false;
}

if (! $this->hasher->check($plain, $hash = $user->getAuthPassword())) {
return false;
}

$this->rehashIfRequired($user, $hash, $plain);

return true;
}

/**
* Rehash the user's password if required.
*
* @param \Illuminate\Contracts\Auth\Authenticatable $user
* @param string $hash
* @param string $plain
* @return void
*/
protected function rehashIfRequired(UserContract $user, string $hash, string $plain)
{
$attribute = method_exists($user, 'getAuthPasswordName') ? $user->getAuthPasswordName() : 'password';

if (! isset($user->{$attribute}) || ! hash_equals($hash, $user->{$attribute})) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the hash gets checked twice (once in validateCredentials and here).

I guess this is done to be able to make this a public method so it won't rehash passwords that don't match the user's password. But the downside is double hash checking for every login attempt.

Would it make more sense to split this in two methods, one private without the hash check and one public that uses the private method after validating the hash?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hash_equals() is a basic string comparison function, I could have used ===, but it's best practice to use that for hashes.

You're thinking of password_verify().

return;
}

if (! $this->hasher->needsRehash($hash)) {
return;
}

$this->connection->table($this->table)
->where($user->getAuthIdentifierName(), $user->getAuthIdentifier())
->update([$attribute => $this->hasher->make($plain)]);
}
}
33 changes: 32 additions & 1 deletion src/Illuminate/Auth/EloquentUserProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,38 @@ public function validateCredentials(UserContract $user, array $credentials)
return false;
}

return $this->hasher->check($plain, $user->getAuthPassword());
if (! $this->hasher->check($plain, $hash = $user->getAuthPassword())) {
return false;
}

$this->rehashIfRequired($user, $hash, $plain);

return true;
}

/**
* Rehash the user's password if required.
*
* @param \Illuminate\Contracts\Auth\Authenticatable $user
* @param string $hash
* @param string $plain
* @return void
*/
protected function rehashIfRequired(UserContract $user, string $hash, string $plain)
{
$attribute = method_exists($user, 'getAuthPasswordName') ? $user->getAuthPasswordName() : 'password';

if (! isset($user->{$attribute}) || ! hash_equals($hash, $user->{$attribute})) {
return;
}

if (! $this->hasher->needsRehash($hash)) {
return;
}

$user->forceFill([
$attribute => $this->hasher->make($plain),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this not be a BC for those of us who use an Attribute mutator to set and hash the password?

protected function password(): Attribute
{
    return new Attribute(
        set: fn (string $value): string => Hash::make($value),
    );
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it probably would.

The built-in hash cast checks if it's hashed before hashing:

return $value !== null && ! Hash::isHashed($value) ? Hash::make($value) : $value;

But a custom cast like your example wouldn't do this. 🤔

])->save();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This potentially may trigger User model observer and maybe not be desirable in some scenarios.

}

/**
Expand Down
51 changes: 51 additions & 0 deletions tests/Auth/AuthDatabaseUserProviderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Illuminate\Contracts\Auth\Authenticatable;
use Illuminate\Contracts\Hashing\Hasher;
use Illuminate\Database\Connection;
use Illuminate\Database\ConnectionInterface;
use Mockery as m;
use PHPUnit\Framework\TestCase;
use stdClass;
Expand Down Expand Up @@ -160,4 +161,54 @@ public function testCredentialValidation()

$this->assertTrue($result);
}

public function testCredentialValidationWithPasswordAttributeWithoutRehashing()
{
$conn = m::mock(Connection::class);
$hasher = m::mock(Hasher::class);
$hasher->shouldReceive('check')->once()->with('plain', 'hash')->andReturn(true);
$hasher->shouldReceive('needsRehash')->once()->with('hash')->andReturn(false);
$provider = new DatabaseUserProvider($conn, $hasher, 'foo');
$user = m::mock(Authenticatable::class);
$user->password = 'hash';
$user->shouldReceive('getAuthPassword')->once()->andReturn('hash');
$result = $provider->validateCredentials($user, ['password' => 'plain']);

$this->assertTrue($result);
}

public function testCredentialValidationRequiresRehash()
{
$hasher = m::mock(Hasher::class);
$hasher->shouldReceive('check')->once()->with('plain', 'hash')->andReturn(true);
$hasher->shouldReceive('needsRehash')->once()->with('hash')->andReturn(true);
$hasher->shouldReceive('make')->once()->with('plain')->andReturn('rehashed');
$conn = m::mock(Connection::class);
$table = m::mock(ConnectionInterface::class);
$conn->shouldReceive('table')->once()->with('foo')->andReturn($table);
$table->shouldReceive('where')->once()->with('id', 1)->andReturnSelf();
$table->shouldReceive('update')->once()->with(['password' => 'rehashed']);
$provider = new DatabaseUserProvider($conn, $hasher, 'foo');
$user = m::mock(Authenticatable::class);
$user->password = 'hash';
$user->shouldReceive('getAuthIdentifierName')->once()->andReturn('id');
$user->shouldReceive('getAuthIdentifier')->once()->andReturn(1);
$user->shouldReceive('getAuthPassword')->once()->andReturn('hash');
$result = $provider->validateCredentials($user, ['password' => 'plain']);

$this->assertTrue($result);
}

public function testCredentialValidationFailed()
{
$conn = m::mock(Connection::class);
$hasher = m::mock(Hasher::class);
$hasher->shouldReceive('check')->once()->with('plain', 'hash')->andReturn(false);
$provider = new DatabaseUserProvider($conn, $hasher, 'foo');
$user = m::mock(Authenticatable::class);
$user->shouldReceive('getAuthPassword')->once()->andReturn('hash');
$result = $provider->validateCredentials($user, ['password' => 'plain']);

$this->assertFalse($result);
}
}
43 changes: 43 additions & 0 deletions tests/Auth/AuthEloquentUserProviderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,49 @@ public function testCredentialValidation()
$this->assertTrue($result);
}

public function testCredentialValidationWithPasswordAttributeWithoutRehashing()
{
$hasher = m::mock(Hasher::class);
$hasher->shouldReceive('check')->once()->with('plain', 'hash')->andReturn(true);
$hasher->shouldReceive('needsRehash')->once()->with('hash')->andReturn(false);
$provider = new EloquentUserProvider($hasher, 'foo');
$user = m::mock(Authenticatable::class);
$user->password = 'hash';
$user->shouldReceive('getAuthPassword')->once()->andReturn('hash');
$result = $provider->validateCredentials($user, ['password' => 'plain']);

$this->assertTrue($result);
}

public function testCredentialValidationRequiresRehash()
{
$hasher = m::mock(Hasher::class);
$hasher->shouldReceive('check')->once()->with('plain', 'hash')->andReturn(true);
$hasher->shouldReceive('needsRehash')->once()->with('hash')->andReturn(true);
$hasher->shouldReceive('make')->once()->with('plain')->andReturn('rehashed');
$provider = new EloquentUserProvider($hasher, 'foo');
$user = m::mock(Authenticatable::class);
$user->password = 'hash';
$user->shouldReceive('getAuthPassword')->once()->andReturn('hash');
$user->shouldReceive('forceFill')->once()->with(['password' => 'rehashed'])->andReturnSelf();
$user->shouldReceive('save')->once();
$result = $provider->validateCredentials($user, ['password' => 'plain']);

$this->assertTrue($result);
}

public function testCredentialValidationFailed()
{
$hasher = m::mock(Hasher::class);
$hasher->shouldReceive('check')->once()->with('plain', 'hash')->andReturn(false);
$provider = new EloquentUserProvider($hasher, 'foo');
$user = m::mock(Authenticatable::class);
$user->shouldReceive('getAuthPassword')->once()->andReturn('hash');
$result = $provider->validateCredentials($user, ['password' => 'plain']);

$this->assertFalse($result);
}

public function testModelsCanBeCreated()
{
$hasher = m::mock(Hasher::class);
Expand Down