Skip to content

[11.x] Rehash user passwords when validating credentials #48665

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

Merged
merged 13 commits into from
Dec 10, 2023
Merged
1 change: 1 addition & 0 deletions src/Illuminate/Auth/AuthManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ public function createSessionDriver($name, $config)
$name,
$provider,
$this->app['session.store'],
rehashOnLogin: $this->app['hash']->rehashOnLogin(),
);

// When using the remember me functionality of the authentication services we
Expand Down
19 changes: 18 additions & 1 deletion src/Illuminate/Auth/Authenticatable.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,13 @@ trait Authenticatable
*/
protected $rememberTokenName = 'remember_token';

/**
* The column name of the auth password field.
*
* @var string
*/
protected $authPasswordName = 'password';

/**
* Get the name of the unique identifier for the user.
*
Expand Down Expand Up @@ -41,14 +48,24 @@ public function getAuthIdentifierForBroadcasting()
return $this->getAuthIdentifier();
}

/**
* Get the name of the password attribute for the user.
*
* @return string
*/
public function getAuthPasswordName()
{
return $this->authPasswordName;
}

/**
* Get the password for the user.
*
* @return string
*/
public function getAuthPassword()
{
return $this->password;
return $this->{$this->getAuthPasswordName()};
}

/**
Expand Down
18 changes: 18 additions & 0 deletions src/Illuminate/Auth/DatabaseUserProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -158,4 +158,22 @@ public function validateCredentials(UserContract $user, array $credentials)
$credentials['password'], $user->getAuthPassword()
);
}

/**
* Rehash the user's password if required and supported.
*
* @param \Illuminate\Contracts\Auth\Authenticatable $user
* @param array $credentials
* @return string|null
*/
public function rehashPasswordIfRequired(UserContract $user, array $credentials)
{
if (! $this->hasher->needsRehash($user->getAuthPassword())) {
return;
}

$this->connection->table($this->table)
->where($user->getAuthIdentifierName(), $user->getAuthIdentifier())
->update([$user->getAuthPasswordName() => $this->hasher->make($credentials['password'])]);
}
}
18 changes: 18 additions & 0 deletions src/Illuminate/Auth/EloquentUserProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,24 @@ public function validateCredentials(UserContract $user, array $credentials)
return $this->hasher->check($plain, $user->getAuthPassword());
}

/**
* Rehash the user's password if required and supported.
*
* @param \Illuminate\Contracts\Auth\Authenticatable $user
* @param array $credentials
* @return string|null
Copy link

Choose a reason for hiding this comment

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

need to recheck the return type docblocks on the three concrete versions of this method, the contract one is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! Fixed. 👍

*/
public function rehashPasswordIfRequired(UserContract $user, array $credentials)
{
if (! $this->hasher->needsRehash($user->getAuthPassword())) {
return;
}

$user->forceFill([
$user->getAuthPasswordName() => $this->hasher->make($credentials['password']),
])->save();
}

/**
* Get a new query builder for the model instance.
*
Expand Down
10 changes: 10 additions & 0 deletions src/Illuminate/Auth/GenericUser.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@ public function getAuthIdentifier()
return $this->attributes[$this->getAuthIdentifierName()];
}

/**
* Get the name of the password attribute for the user.
*
* @return string
*/
public function getAuthPasswordName()
{
return 'password';
}

/**
* Get the password for the user.
*
Expand Down
27 changes: 26 additions & 1 deletion src/Illuminate/Auth/SessionGuard.php
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,13 @@ class SessionGuard implements StatefulGuard, SupportsBasicAuth
*/
protected $timebox;

/**
* Rehash passwords during login.
*
* @var bool
*/
protected $rehashOnLogin;

/**
* Indicates if the logout method has been called.
*
Expand Down Expand Up @@ -124,13 +131,15 @@ public function __construct($name,
UserProvider $provider,
Session $session,
Request $request = null,
Timebox $timebox = null)
Timebox $timebox = null,
bool $rehashOnLogin = true)
{
$this->name = $name;
$this->session = $session;
$this->request = $request;
$this->provider = $provider;
$this->timebox = $timebox ?: new Timebox;
$this->rehashOnLogin = $rehashOnLogin;
}

/**
Expand Down Expand Up @@ -384,6 +393,7 @@ public function attempt(array $credentials = [], $remember = false)
// to validate the user against the given credentials, and if they are in
// fact valid we'll log the users into the application and return true.
if ($this->hasValidCredentials($user, $credentials)) {
$this->rehashPasswordIfRequired($user, $credentials);
$this->login($user, $remember);

return true;
Expand Down Expand Up @@ -415,6 +425,7 @@ public function attemptWhen(array $credentials = [], $callbacks = null, $remembe
// the user is retrieved and validated. If one of the callbacks returns falsy we do
// not login the user. Instead, we will fail the specific authentication attempt.
if ($this->hasValidCredentials($user, $credentials) && $this->shouldLogin($callbacks, $user)) {
$this->rehashPasswordIfRequired($user, $credentials);
$this->login($user, $remember);

return true;
Expand Down Expand Up @@ -447,6 +458,20 @@ protected function hasValidCredentials($user, $credentials)
}, 200 * 1000);
}

/**
* Rehash the user's password if enabled and required.
*
* @param \Illuminate\Contracts\Auth\Authenticatable $user
* @param array $credentials
* @return bool
*/
protected function rehashPasswordIfRequired(AuthenticatableContract $user, array $credentials)
{
if ($this->rehashOnLogin) {
$this->provider->rehashPasswordIfRequired($user, $credentials);
}
}

/**
* Determine if the user should login by executing the given callbacks.
*
Expand Down
7 changes: 7 additions & 0 deletions src/Illuminate/Contracts/Auth/Authenticatable.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ public function getAuthIdentifierName();
*/
public function getAuthIdentifier();

/**
* Get the name of the password attribute for the user.
*
* @return string
*/
public function getAuthPasswordName();

/**
* Get the password for the user.
*
Expand Down
9 changes: 9 additions & 0 deletions src/Illuminate/Contracts/Auth/UserProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,13 @@ public function retrieveByCredentials(array $credentials);
* @return bool
*/
public function validateCredentials(Authenticatable $user, array $credentials);

/**
* Rehash the user's password if required and supported.
*
* @param \Illuminate\Contracts\Auth\Authenticatable $user
* @param array $credentials
* @return void
*/
public function rehashPasswordIfRequired(Authenticatable $user, array $credentials);
}
10 changes: 10 additions & 0 deletions src/Illuminate/Hashing/HashManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,16 @@ public function isHashed($value)
return password_get_info($value)['algo'] !== null;
}

/**
* Determine if rehashing should be performed during login.
*
* @return bool
*/
public function rehashOnLogin()
{
return $this->config->get('hashing.rehash_on_login', true);
}

/**
* Get the default driver name.
*
Expand Down
58 changes: 58 additions & 0 deletions tests/Auth/AuthDatabaseUserProviderTest.php
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,61 @@ public function testCredentialValidation()

$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);
}

public function testRehashPasswordIfRequired()
{
$hasher = m::mock(Hasher::class);
$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_attribute' => 'rehashed']);

$user = m::mock(Authenticatable::class);
$user->shouldReceive('getAuthIdentifierName')->once()->andReturn('id');
$user->shouldReceive('getAuthIdentifier')->once()->andReturn(1);
$user->shouldReceive('getAuthPassword')->once()->andReturn('hash');
$user->shouldReceive('getAuthPasswordName')->once()->andReturn('password_attribute');

$provider = new DatabaseUserProvider($conn, $hasher, 'foo');
$provider->rehashPasswordIfRequired($user, ['password' => 'plain']);
}

public function testDontRehashPasswordIfNotRequired()
{
$hasher = m::mock(Hasher::class);
$hasher->shouldReceive('needsRehash')->once()->with('hash')->andReturn(false);
$hasher->shouldNotReceive('make');

$conn = m::mock(Connection::class);
$table = m::mock(ConnectionInterface::class);
$conn->shouldNotReceive('table');
$table->shouldNotReceive('where');
$table->shouldNotReceive('update');

$user = m::mock(Authenticatable::class);
$user->shouldReceive('getAuthPassword')->once()->andReturn('hash');
$user->shouldNotReceive('getAuthIdentifierName');
$user->shouldNotReceive('getAuthIdentifier');
$user->shouldNotReceive('getAuthPasswordName');

$provider = new DatabaseUserProvider($conn, $hasher, 'foo');
$provider->rehashPasswordIfRequired($user, ['password' => 'plain']);
}
}
44 changes: 44 additions & 0 deletions tests/Auth/AuthEloquentUserProviderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,50 @@ public function testCredentialValidation()
$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 testRehashPasswordIfRequired()
{
$hasher = m::mock(Hasher::class);
$hasher->shouldReceive('needsRehash')->once()->with('hash')->andReturn(true);
$hasher->shouldReceive('make')->once()->with('plain')->andReturn('rehashed');

$user = m::mock(Authenticatable::class);
$user->shouldReceive('getAuthPassword')->once()->andReturn('hash');
$user->shouldReceive('getAuthPasswordName')->once()->andReturn('password_attribute');
$user->shouldReceive('forceFill')->once()->with(['password_attribute' => 'rehashed'])->andReturnSelf();
$user->shouldReceive('save')->once();

$provider = new EloquentUserProvider($hasher, 'foo');
$provider->rehashPasswordIfRequired($user, ['password' => 'plain']);
}

public function testDontRehashPasswordIfNotRequired()
{
$hasher = m::mock(Hasher::class);
$hasher->shouldReceive('needsRehash')->once()->with('hash')->andReturn(false);
$hasher->shouldNotReceive('make');

$user = m::mock(Authenticatable::class);
$user->shouldReceive('getAuthPassword')->once()->andReturn('hash');
$user->shouldNotReceive('getAuthPasswordName');
$user->shouldNotReceive('forceFill');
$user->shouldNotReceive('save');

$provider = new EloquentUserProvider($hasher, 'foo');
$provider->rehashPasswordIfRequired($user, ['password' => 'plain']);
}

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