Skip to content
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

refactor: remove supportOldDangerousPassword #976

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions UPGRADING.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
# Upgrade Guide

## Version 1.0.0-beta.8 to 1.0.0

## Removed Deprecated Items

The [$supportOldDangerousPassword](#if-you-want-to-allow-login-with-existing-passwords)
feature for backward compatiblity has been removed. The old passwords saved in
Shield v1.0.0-beta.3 or earlier are no longer supported.

## Version 1.0.0-beta.7 to 1.0.0-beta.8

### Mandatory Config Changes
Expand Down
2 changes: 1 addition & 1 deletion phpstan-baseline.php
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@
];
$ignoreErrors[] = [
'message' => '#^Call to method PHPUnit\\\\Framework\\\\Assert\\:\\:assertInstanceOf\\(\\) with \'CodeIgniter\\\\\\\\Shield\\\\\\\\Result\' and CodeIgniter\\\\Shield\\\\Result will always evaluate to true\\.$#',
'count' => 9,
'count' => 8,
'path' => __DIR__ . '/tests/Authentication/Authenticators/SessionAuthenticatorTest.php',
];
$ignoreErrors[] = [
Expand Down
45 changes: 17 additions & 28 deletions src/Authentication/Authenticators/Session.php
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ private function checkSecurityConfig(): void
if ($securityConfig->csrfProtection === 'cookie') {
throw new SecurityException(
'Config\Security::$csrfProtection is set to \'cookie\'.'
. ' Same-site attackers may bypass the CSRF protection.'
. ' Please set it to \'session\'.'
. ' Same-site attackers may bypass the CSRF protection.'
. ' Please set it to \'session\'.'
);
}
}
Expand Down Expand Up @@ -343,30 +343,19 @@ public function check(array $credentials): Result
/** @var Passwords $passwords */
$passwords = service('passwords');

// This is only for supportOldDangerousPassword.
$needsRehash = false;

// Now, try matching the passwords.
if (! $passwords->verify($givenPassword, $user->password_hash)) {
if (
! setting('Auth.supportOldDangerousPassword')
|| ! $passwords->verifyDanger($givenPassword, $user->password_hash) // @phpstan-ignore-line
) {
return new Result([
'success' => false,
'reason' => lang('Auth.invalidPassword'),
]);
}

// Passed with old dangerous password.
$needsRehash = true;
return new Result([
'success' => false,
'reason' => lang('Auth.invalidPassword'),
]);
}

// Check to see if the password needs to be rehashed.
// This would be due to the hash algorithm or hash
// cost changing since the last time that a user
// logged in.
if ($passwords->needsRehash($user->password_hash) || $needsRehash) {
if ($passwords->needsRehash($user->password_hash)) {
$user->password_hash = $passwords->hash($givenPassword);
$this->provider->save($user);
}
Expand Down Expand Up @@ -661,10 +650,10 @@ public function startLogin(User $user): void
if ($userId !== null) {
throw new LogicException(
'The user has User Info in Session, so already logged in or in pending login state.'
. ' If a logged in user logs in again with other account, the session data of the previous'
. ' user will be used as the new user.'
. ' Fix your code to prevent users from logging in without logging out or delete the session data.'
. ' user_id: ' . $userId
. ' If a logged in user logs in again with other account, the session data of the previous'
. ' user will be used as the new user.'
. ' Fix your code to prevent users from logging in without logging out or delete the session data.'
. ' user_id: ' . $userId
);
}

Expand Down Expand Up @@ -749,18 +738,18 @@ public function login(User $user): void
if ($this->getIdentitiesForAction($user) !== []) {
throw new LogicException(
'The user has identities for action, so cannot complete login.'
. ' If you want to start to login with auth action, use startLogin() instead.'
. ' Or delete identities for action in database.'
. ' user_id: ' . $user->id
. ' If you want to start to login with auth action, use startLogin() instead.'
. ' Or delete identities for action in database.'
. ' user_id: ' . $user->id
);
}
// Check auth_action in Session
if ($this->getSessionKey('auth_action')) {
throw new LogicException(
'The user has auth action in session, so cannot complete login.'
. ' If you want to start to login with auth action, use startLogin() instead.'
. ' Or delete `auth_action` and `auth_action_message` in session data.'
. ' user_id: ' . $user->id
. ' If you want to start to login with auth action, use startLogin() instead.'
. ' Or delete `auth_action` and `auth_action_message` in session data.'
. ' user_id: ' . $user->id
);
}

Expand Down
15 changes: 0 additions & 15 deletions src/Authentication/Passwords.php
Original file line number Diff line number Diff line change
Expand Up @@ -90,21 +90,6 @@ public function verify(string $password, string $hash): bool
return password_verify($password, $hash);
}

/**
* Verifies a password against a previously hashed password.
*
* @param string $password The password we're checking
* @param string $hash The previously hashed password
*
* @deprecated This is only for backward compatibility.
*/
public function verifyDanger(string $password, string $hash): bool
{
return password_verify(base64_encode(
hash('sha384', $password, true)
), $hash);
}

/**
* Checks to see if a password should be rehashed.
*/
Expand Down
10 changes: 0 additions & 10 deletions src/Config/Auth.php
Original file line number Diff line number Diff line change
Expand Up @@ -374,16 +374,6 @@ class Auth extends BaseConfig
*/
public int $hashCost = 12;

/**
* If you need to support passwords saved in versions prior to Shield v1.0.0-beta.4.
* set this to true.
*
* See https://github.com/codeigniter4/shield/security/advisories/GHSA-c5vj-f36q-p9vg
*
* @deprecated This is only for backward compatibility.
*/
public bool $supportOldDangerousPassword = false;

/**
* ////////////////////////////////////////////////////////////////////
* OTHER SETTINGS
Expand Down
29 changes: 0 additions & 29 deletions tests/Authentication/Authenticators/SessionAuthenticatorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
use CodeIgniter\Shield\Entities\User;
use CodeIgniter\Shield\Exceptions\LogicException;
use CodeIgniter\Shield\Models\RememberModel;
use CodeIgniter\Shield\Models\UserIdentityModel;
use CodeIgniter\Shield\Models\UserModel;
use CodeIgniter\Shield\Result;
use CodeIgniter\Test\Mock\MockEvents;
Expand Down Expand Up @@ -313,34 +312,6 @@ public function testCheckSuccess(): void
$this->assertSame($this->user->id, $foundUser->id);
}

public function testCheckSuccessOldDangerousPassword(): void
{
/** @var Auth $config */
$config = config('Auth');
$config->supportOldDangerousPassword = true; // @phpstan-ignore-line

fake(
UserIdentityModel::class,
[
'user_id' => $this->user->id,
'type' => Session::ID_TYPE_EMAIL_PASSWORD,
'secret' => '[email protected]',
'secret2' => '$2y$10$WswjNNcR24cJvsXvBc5TveVVVQ9/EYC0eq.Ad9e/2cVnmeSEYBOEm',
]
);

$result = $this->auth->check([
'email' => '[email protected]',
'password' => 'passw0rd!',
]);

$this->assertInstanceOf(Result::class, $result);
$this->assertTrue($result->isOK());

$foundUser = $result->extraInfo();
$this->assertSame($this->user->id, $foundUser->id);
}

public function testAttemptCannotFindUser(): void
{
$result = $this->auth->attempt([
Expand Down
Loading