8000 [11.x] Rehash user passwords when validating credentials (#48665) · laravel/framework@ceb8ed2 · GitHub
[go: up one dir, main page]
Skip to content

Commit ceb8ed2

Browse files
valorintaylorotwellchrysanthos
authored
[11.x] Rehash user passwords when validating credentials (#48665)
* Rehash user passwords when validating credentials * Fix style violations * Remove hardcoded password when it's changable * Shift rehashing into SessionGuard The Session guard's attempt() method is a better place to apply rehashing than the validateCredentials() method on the provider. The latter shouldn't have side-effects, as per it's name. * Fix style violation * Add config option to disable rehashing on login * Clean up rehash flag injection * Fix contract in DatabaseUserProvider * Fixing return type in the docblocks * Use hash_equals() for a secure string comparison * formatting * formatting, leverage method on logoutOtherDevices * Fix spelling of passwords Co-authored-by: Chrysanthos <48060191+chrysanthos@users.noreply.github.com> --------- Co-authored-by: Taylor Otwell <taylor@laravel.com> Co-authored-by: Chrysanthos <48060191+chrysanthos@users.noreply.github.com>
1 parent 6e302e8 commit ceb8ed2

12 files changed

+270
-14
lines changed

‎src/Illuminate/Auth/AuthManager.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ public function createSessionDriver($name, $config)
128128
$name,
129129
$provider,
130130
$this->app['session.store'],
131+
rehashOnLogin: $this->app['config']->get('hashing.rehash_on_login', true),
131132
);
132133

133134
// When using the remember me functionality of the authentication services we

‎src/Illuminate/Auth/Authenticatable.php

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,13 @@
44

55
trait Authenticatable
66
{
7+
/**
8+
* The column name of the password field using during authentication.
9+
*
10+
* @var string
11+
*/
12+
protected $authPasswordName = 'password';
13+
714
/**
815
* The column name of the "remember me" token.
916
*
@@ -41,14 +48,24 @@ public function getAuthIdentifierForBroadcasting()
4148
return $this->getAuthIdentifier();
4249
}
4350

51+
/**
52+
* Get the name of the password attribute for the user.
53+
*
54+
* @return string
55+
*/
56+
public function getAuthPasswordName()
57+
{
58+
return $this->authPasswordName;
59+
}
60+
4461
/**
4562
* Get the password for the user.
4663
*
4764
* @return string
4865
*/
4966
public function getAuthPassword()
5067
{
51-
return $this->password;
68+
return $this->{$this->getAuthPasswordName()};
5269
}
5370

5471
/**

‎src/Illuminate/Auth/DatabaseUserProvider.php

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,4 +158,23 @@ public function validateCredentials(UserContract $user, array $credentials)
158158
$credentials['password'], $user->getAuthPassword()
159159
);
160160
}
161+
162+
/**
163+
* Rehash the user's password if required and supported.
164+
*
165+
* @param \Illuminate\Contracts\Auth\Authenticatable $user
166+
* @param array $credentials
167+
* @param bool $force
168+
* @return void
169+
*/
170+
public function rehashPasswordIfRequired(UserContract $user, array $credentials, bool $force = false)
171+
{
172+
if (! $this->hasher->needsRehash($user->getAuthPassword()) && ! $force) {
173+
return;
174+
}
175+
176+
$this->connection->table($this->table)
177+
->where($user->getAuthIdentifierName(), $user->getAuthIdentifier())
178+
->update([$user->getAuthPasswordName() => $this->hasher->make($credentials['password'])]);
179+
}
161180
}

‎src/Illuminate/Auth/EloquentUserProvider.php

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,25 @@ public function validateCredentials(UserContract $user, array $credentials)
155155
return $this->hasher->check($plain, $user->getAuthPassword());
156156
}
157157

158+
/**
159+
* Rehash the user's password if required and supported.
160+
*
161+
* @param \Illuminate\Contracts\Auth\Authenticatable $user
162+
* @param array $credentials
163+
* @param bool $force
164+
* @return void
165+
*/
166+
public function rehashPasswordIfRequired(UserContract $user, array $credentials, bool $force = false)
167+
{
168+
if (! $this->hasher->needsRehash($user->getAuthPassword()) && ! $force) {
169+
return;
170+
}
171+
172+
$user->forceFill([
173+
$user->getAuthPasswordName() => $this->hasher->make($credentials['password']),
174+
])->save();
175+
}
176+
158177
/**
159178
* Get a new query builder for the model instance.
160179
*

‎src/Illuminate/Auth/GenericUser.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,16 @@ public function getAuthIdentifier()
4444
return $this->attributes[$this->getAuthIdentifierName()];
4545
}
4646

47+
/**
48+
* Get the name of the password attribute for the user.
49+
*
50+
* @return string
51+
*/
52+
public function getAuthPasswordName()
53+
{
54+
return 'password';
55+
}
56+
4757
/**
4858
* Get the password for the user.
4959
*

‎src/Illuminate/Auth/SessionGuard.php

Lines changed: 39 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,13 @@ class SessionGuard implements StatefulGuard, SupportsBasicAuth
9696
*/
9797
protected $timebox;
9898

99+
/**
100+
* Indicates if passwords should be rehashed on login if needed.
101+
*
102+
* @var bool
103+
*/
104+
protected $rehashOnLogin;
105+
99106
/**
100107
* Indicates if the logout method has been called.
101108
*
@@ -118,19 +125,22 @@ class SessionGuard implements StatefulGuard, SupportsBasicAuth
118125
* @param \Illuminate\Contracts\Session\Session $session
119126
* @param \Symfony\Component\HttpFoundation\Request|null $request
120127
* @param \Illuminate\Support\Timebox|null $timebox
128+
* @param bool $rehashOnLogin
121129
* @return void
122130
*/
123131
public function __construct($name,
124132
UserProvider $provider,
125133
Session $session,
126134
Request $request = null,
127-
Timebox $timebox = null)
135+
Timebox $timebox = null,
136+
bool $rehashOnLogin = true)
128137
{
129138
$this->name = $name;
130139
$this->session = $session;
131140
$this->request = $request;
132141
$this->provider = $provider;
133142
$this->timebox = $timebox ?: new Timebox;
143+
$this->rehashOnLogin = $rehashOnLogin;
134144
}
135145

136146
/**
@@ -384,6 +394,8 @@ public function attempt(array $credentials = [], $remember = false)
384394
// to validate the user against the given credentials, and if they are in
385395
// fact valid we'll log the users into the application and return true.
386396
if ($this->hasValidCredentials($user, $credentials)) {
397+
$this->rehashPasswordIfRequired($user, $credentials);
398+
387399
$this->login($user, $remember);
388400

389401
return true;
@@ -415,6 +427,8 @@ public function attemptWhen(array $credentials = [], $callbacks = null, $remembe
415427
// the user is retrieved and validated. If one of the callbacks returns falsy we do
416428
// not login the user. Instead, we will fail the specific authentication attempt.
417429
if ($this->hasValidCredentials($user, $credentials) && $this->shouldLogin($callbacks, $user)) {
430+
$this->rehashPasswordIfRequired($user, $credentials);
431+
418432
$this->login($user, $remember);
419433

420434
return true;
@@ -465,6 +479,20 @@ protected function shouldLogin($callbacks, AuthenticatableContract $user)
465479
return true;
466480
}
467481

482+
/**
483+
* Rehash the user's password if enabled and required.
484+
*
485+
* @param \Illuminate\Contracts\Auth\Authenticatable $user
486+
* @param array $credentials
487+
* @return void
488+
*/
489+
protected function rehashPasswordIfRequired(AuthenticatableContract $user, array $credentials)
490+
{
491+
if ($this->rehashOnLogin) {
492+
$this->provider->rehashPasswordIfRequired($user, $credentials);
493+
}
494+
}
495+
468496
/**
469497
* Log the given user ID into the application.
470498
*
@@ -656,18 +684,17 @@ protected function cycleRememberToken(AuthenticatableContract $user)
656684
* The application must be using the AuthenticateSession middleware.
657685
*
658686
* @param string $password
659-
* @param string $attribute
660687
* @return \Illuminate\Contracts\Auth\Authenticatable|null
661688
*
662689
* @throws \Illuminate\Auth\AuthenticationException
663690
*/
664-
public function logoutOtherDevices($password, $attribute = 'password')
691+
public function logoutOtherDevices($password)
665692
{
666693
if (! $this->user()) {
667694
return;
668695
}
669696

670-
$result = $this->rehashUserPassword($password, $attribute);
697+
$result = $this->rehashUserPasswordForDeviceLogout($password);
671698

672699
if ($this->recaller() ||
673700
$this->getCookieJar()->hasQueued($this->getRecallerName())) {
@@ -680,23 +707,24 @@ public function logoutOtherDevices($password, $attribute = 'password')
680707
}
681708

682709
/**
683-
* Rehash the current user's password.
710+
* Rehash the current user's password for logging out other devices via AuthenticateSession.
684711
*
685712
* @param string $password
686-
* @param string $attribute
687713
* @return \Illuminate\Contracts\Auth\Authenticatable|null
688714
*
689715
* @throws \InvalidArgumentException
690716
*/
691-
protected function rehashUserPassword($password, $attribute)
717+
protected function rehashUserPasswordForDeviceLogout($password)
692718
{
693-
if (! Hash::check($password, $this->user()->{$attribute})) {
719+
$user = $this->user();
720+
721+
if (! Hash::check($password, $user->getAuthPassword())) {
694722
throw new InvalidArgumentException('The given password does not match the current password.');
695723
}
696724

697-
return tap($this->user()->forceFill([
698-
$attribute => Hash::make($password),
699-
]))->save();
725+
$this->provider->rehashPasswordIfRequired(
726+
$user, ['password' => $password], force: true
727+
);
700728
}
701729

702730
/**

‎src/Illuminate/Contracts/Auth/Authenticatable.php

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,13 @@ public function getAuthIdentifierName();
1818
*/
1919
public function getAuthIdentifier();
2020

21+
/**
22+
* Get the name of the password attribute for the user.
23+
*
24+
* @return string
25+
*/
26+
public function getAuthPasswordName();
27+
2128
/**
2229
* Get the password for the user.
2330
*

‎src/Illuminate/Contracts/Auth/UserProvider.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,4 +46,14 @@ public function retrieveByCredentials(array $credentials);
4646
* @return bool
4747
*/
4848
public function validateCredentials(Authenticatable $user, array $credentials);
49+
50+
/**
51+
* Rehash the user's password if required and supported.
52+
*
53+
* @param \Illuminate\Contracts\Auth\Authenticatable $user
54+
* @param array $credentials
55+
* @param bool $force
56+
* @return void
57+
*/
58+
public function rehashPasswordIfRequired(Authenticatable $user, array $credentials, bool $force = false);
4959
}

‎src/Illuminate/Session/Middleware/AuthenticateSession.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ public function handle($request, Closure $next)
5151
if ($this->guard()->viaRemember()) {
5252
$passwordHash = explode('|', $request->cookies->get($this->guard()->getRecallerName()))[2] ?? null;
5353

54-
if (! $passwordHash || $passwordHash != $request->user()->getAuthPassword()) {
54+
if (! $passwordHash || ! hash_equals($request->user()->getAuthPassword(), $passwordHash)) {
5555
$this->logout($request);
5656
}
5757
}
@@ -60,7 +60,7 @@ public function handle($request, Closure $next)
6060
$this->storePasswordHashInSession($request);
6161
}
6262

63-
if ($request->session()->get('password_hash_'.$this->auth->getDefaultDriver()) !== $request->user()->getAuthPassword()) {
63+
if (! hash_equals($request->session()->get('password_hash_'.$this->auth->getDefaultDriver()), $request->user()->getAuthPassword())) {
6464
$this->logout($request);
6565
}
6666

‎tests/Auth/AuthDatabaseUserProviderTest.php

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use Illuminate\Contracts\Auth\Authenticatable;
88
use Illuminate\Contracts\Hashing\Hasher;
99
use Illuminate\Database\Connection;
10+
use Illuminate\Database\ConnectionInterface;
1011
use Mockery as m;
1112
use PHPUnit\Framework\TestCase;
1213
use stdClass;
@@ -160,4 +161,61 @@ public function testCredentialValidation()
160161

161162
$this->assertTrue($result);
162163
}
164+
165+
public function testCredentialValidationFailed()
166+
{
167+
$conn = m::mock(Connection::class);
168+
$hasher = m::mock(Hasher::class);
169+
$hasher->shouldReceive('check')->once()->with('plain', 'hash')->andReturn(false);
170+
$provider = new DatabaseUserProvider($conn, $hasher, 'foo');
171+
$user = m::mock(Authenticatable::class);
172+
$user->shouldReceive('getAuthPassword')->once()->andReturn('hash');
173+
$result = $provider->validateCredentials($user, ['password' => 'plain']);
174+
175+
$this->assertFalse($result);
176+
}
177+
178+
public function testRehashPasswordIfRequired()
179+
{
180+
$hasher = m::mock(Hasher::class);
181+
$hasher->shouldReceive('needsRehash')->once()->with('hash')->andReturn(true);
182+
$hasher->shouldReceive('make')->once()->with('plain')->andReturn('rehashed');
183+
184+
$conn = m::mock(Connection::class);
185+
$table = m::mock(ConnectionInterface::class);
186+
$conn->shouldReceive('table')->once()->with('foo')->andReturn($table);
187+
$table->shouldReceive('where')->once()->with('id', 1)->andReturnSelf();
188+
$table->shouldReceive('update')->once()->with(['password_attribute' => 'rehashed']);
189+
190+
$user = m::mock(Authenticatable::class);
191+
$user->shouldReceive('getAuthIdentifierName')->once()->andReturn('id');
192+
$user->shouldReceive('getAuthIdentifier')->once()->andReturn(1);
193+
$user->shouldReceive('getAuthPassword')->once()->andReturn('hash');
194+
$user->shouldReceive('getAuthPasswordName')->once()->andReturn('password_attribute');
195+
196+
$provider = new DatabaseUserProvider($conn, $hasher, 'foo');
197+
$provider->rehashPasswordIfRequired($user, ['password' => 'plain']);
198+
}
199+
200+
public function testDontRehashPasswordIfNotRequired()
201+
{
202+
$hasher = m::mock(Hasher::class);
203+
$hasher->shouldReceive('needsRehash')->once()->with('hash')->andReturn(false);
204+
$hasher->shouldNotReceive('make');
205+
206+
$conn = m::mock(Connection::class);
207+
$table = m::mock(ConnectionInterface::class);
208+
$conn->shouldNotReceive('table');
209+
$table->shouldNotReceive('where');
210+
$table->shouldNotReceive('update');
211+
212+
$user = m::mock(Authenticatable::class);
213+
$user->shouldReceive('getAuthPassword')->once()->andReturn('hash');
214+
$user->shouldNotReceive('getAuthIdentifierName');
215+
$user->shouldNotReceive('getAuthIdentifier');
216+
$user->shouldNotReceive('getAuthPasswordName');
217+
218+
$provider = new DatabaseUserProvider($conn, $hasher, 'foo');
219+
$provider->rehashPasswordIfRequired($user, ['password' => 'plain']);
220+
}
163221
}

0 commit comments

Comments
 (0)
0