Skip to content

Commit

Permalink
EZP-30983: Prevented users from reusing old password
Browse files Browse the repository at this point in the history
  • Loading branch information
adamwojs committed Oct 2, 2019
1 parent 17c2678 commit aa6e6af
Show file tree
Hide file tree
Showing 6 changed files with 117 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,10 @@ public function getValidatorSchema()
'type' => 'int',
'default' => null,
],
'requireNewPassword' => [
'type' => 'int',
'default' => null,
],
'minLength' => [
'type' => 'int',
'default' => 10,
Expand All @@ -123,6 +127,7 @@ public function getValidValidatorConfiguration()
'requireAtLeastOneLowerCaseCharacter' => true,
'requireAtLeastOneNumericCharacter' => true,
'requireAtLeastOneNonAlphanumericCharacter' => false,
'requireNewPassword' => false,
'minLength' => 10,
],
];
Expand Down
29 changes: 27 additions & 2 deletions eZ/Publish/API/Repository/Tests/UserServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2860,7 +2860,7 @@ public function testValidatePasswordWithDefaultContext()
* @covers \eZ\Publish\API\Repository\UserService::validatePassword()
* @dataProvider dataProviderForValidatePassword
*/
public function testValidatePassword(string $password, array $expectedErrorr)
public function testValidatePassword(string $password, array $expectedErrors)
{
$userService = $this->getRepository()->getUserService();
$contentType = $this->createUserContentTypeWithStrongPassword();
Expand All @@ -2873,7 +2873,31 @@ public function testValidatePassword(string $password, array $expectedErrorr)
$actualErrors = $userService->validatePassword($password, $context);
/* END: Use Case */

$this->assertEquals($expectedErrorr, $actualErrors);
$this->assertEquals($expectedErrors, $actualErrors);
}

public function testValidatePasswordReturnsErrorWhenOldPasswordIsReused(): void
{
$password = 'P@blish123!';

$userService = $this->getRepository()->getUserService();
// Password expiration needs to be enabled
$contentType = $this->createUserContentTypeWithPasswordExpirationDate();

$user = $this->createTestUserWithPassword($password, $contentType);

/* BEGIN: Use Case */
$context = new PasswordValidationContext([
'contentType' => $contentType,
'user' => $user,
]);

$actualErrors = $userService->validatePassword($password, $context);
/* END: Use Case */

$this->assertEquals([
new ValidationError('New password cannot be the same as old password', null, [], 'password'),
], $actualErrors);
}

/**
Expand Down Expand Up @@ -3021,6 +3045,7 @@ private function createUserContentTypeWithStrongPassword(): ContentType
'requireAtLeastOneLowerCaseCharacter' => 1,
'requireAtLeastOneNumericCharacter' => 1,
'requireAtLeastOneNonAlphanumericCharacter' => 1,
'requireNewPassword' => 1,
'minLength' => 8,
],
]);
Expand Down
4 changes: 4 additions & 0 deletions eZ/Publish/Core/FieldType/Tests/UserTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ protected function getValidatorConfigurationSchemaExpectation()
'type' => 'int',
'default' => null,
],
'requireNewPassword' => [
'type' => 'int',
'default' => null,
],
'minLength' => [
'type' => 'int',
'default' => 10,
Expand Down
4 changes: 4 additions & 0 deletions eZ/Publish/Core/FieldType/User/Type.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ class Type extends FieldType
'type' => 'int',
'default' => null,
],
'requireNewPassword' => [
'type' => 'int',
'default' => null,
],
'minLength' => [
'type' => 'int',
'default' => 10,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class UserConverter implements Converter
private const REQUIRE_AT_LEAST_ONE_LOWER_CASE_CHAR = 2;
private const REQUIRE_AT_LEAST_ONE_NUMERIC_CHAR = 4;
private const REQUIRE_AT_LEAST_ONE_NON_ALPHANUMERIC_CHAR = 8;
private const REQUIRE_NEW_PASSWORD = 16;

/**
* {@inheritdoc}
Expand Down Expand Up @@ -56,6 +57,7 @@ public function toStorageFieldDefinition(FieldDefinition $fieldDef, StorageField
'requireAtLeastOneLowerCaseCharacter' => self::REQUIRE_AT_LEAST_ONE_LOWER_CASE_CHAR,
'requireAtLeastOneNumericCharacter' => self::REQUIRE_AT_LEAST_ONE_NUMERIC_CHAR,
'requireAtLeastOneNonAlphanumericCharacter' => self::REQUIRE_AT_LEAST_ONE_NON_ALPHANUMERIC_CHAR,
'requireNewPassword' => self::REQUIRE_NEW_PASSWORD,
];

$storageDef->dataInt1 = 0;
Expand Down Expand Up @@ -88,6 +90,7 @@ public function toFieldDefinition(StorageFieldDefinition $storageDef, FieldDefin
self::REQUIRE_AT_LEAST_ONE_LOWER_CASE_CHAR => 'requireAtLeastOneLowerCaseCharacter',
self::REQUIRE_AT_LEAST_ONE_NUMERIC_CHAR => 'requireAtLeastOneNumericCharacter',
self::REQUIRE_AT_LEAST_ONE_NON_ALPHANUMERIC_CHAR => 'requireAtLeastOneNonAlphanumericCharacter',
self::REQUIRE_NEW_PASSWORD => 'requireNewPassword',
];

foreach ($rules as $flag => $rule) {
Expand Down
86 changes: 74 additions & 12 deletions eZ/Publish/Core/Repository/UserService.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
use eZ\Publish\Core\Base\Exceptions\UserPasswordValidationException;
use eZ\Publish\Core\FieldType\User\Value as UserValue;
use eZ\Publish\Core\FieldType\User\Type as UserType;
use eZ\Publish\Core\FieldType\ValidationError;
use eZ\Publish\Core\Repository\Validator\UserPasswordValidator;
use eZ\Publish\Core\Repository\Values\User\User;
use eZ\Publish\Core\Repository\Values\User\UserCreateStruct;
Expand Down Expand Up @@ -605,7 +606,7 @@ public function loadUserByCredentials($login, $password, array $prioritizedLangu
}

$spiUser = $this->userHandler->loadByLogin($login);
if (!$this->verifyPassword($login, $password, $spiUser)) {
if (!$this->verifyPasswordForSPIUser($login, $password, $spiUser)) {
throw new NotFoundException('user', $login);
}

Expand Down Expand Up @@ -1274,6 +1275,8 @@ public function newUserGroupUpdateStruct()
*/
public function validatePassword(string $password, PasswordValidationContext $context = null): array
{
$errors = [];

if ($context === null) {
$contentType = $this->repository->getContentTypeService()->loadContentType(
$this->settings['userClassID']
Expand All @@ -1298,11 +1301,22 @@ public function validatePassword(string $password, PasswordValidationContext $co
}

$configuration = $userFieldDefinition->getValidatorConfiguration();
if (!isset($configuration['PasswordValueValidator'])) {
return [];
if (isset($configuration['PasswordValueValidator'])) {
$errors = (new UserPasswordValidator($configuration['PasswordValueValidator']))->validate($password);
}

return (new UserPasswordValidator($configuration['PasswordValueValidator']))->validate($password);
if ($context->user !== null) {
$isPasswordTTLEnabled = $this->getPasswordInfo($context->user)->hasExpirationDate();
$isNewPasswordRequired = $configuration['PasswordValueValidator']['requireNewPassword'] ?? false;

if (($isPasswordTTLEnabled || $isNewPasswordRequired) &&
$this->verifyPasswordForAPIUser($context->user->login, $password, $context->user)
) {
$errors[] = new ValidationError('New password cannot be the same as old password', null, [], 'password');
}
}

return $errors;
}

/**
Expand Down Expand Up @@ -1409,9 +1423,39 @@ private function getUserFieldDefinition(ContentType $contentType): ?FieldDefinit
return null;
}

/**
* Verifies if the provided login and password are valid for eZ\Publish\SPI\Persistence\User.
*
* @param string $login User login
* @param string $password User password
* @param \eZ\Publish\SPI\Persistence\User $spiUser Loaded user handler
*
* @return bool return true if the login and password are sucessfully validate and false, if not.
*/
protected function verifyPasswordForSPIUser(string $login, string $password, SPIUser $spiUser): bool
{
return $this->doVerifyPassword($login, $password, $spiUser->passwordHash, $spiUser->hashAlgorithm);
}

/**
* Verifies if the provided login and password are valid for eZ\Publish\API\Repository\Values\User\User.
*
* @param string $login User login
* @param string $password User password
* @param \eZ\Publish\API\Repository\Values\User\User $apiUser Loaded user
*
* @return bool return true if the login and password are sucessfully validate and false, if not.
*/
protected function verifyPasswordForAPIUser(string $login, string $password, APIUser $apiUser): bool
{
return $this->doVerifyPassword($login, $password, $apiUser->passwordHash, $apiUser->hashAlgorithm);
}

/**
* Verifies if the provided login and password are valid.
*
* @deprecated since v7.5.5 in favour of verifyPasswordForSPIUser
*
* @param string $login User login
* @param string $password User password
* @param \eZ\Publish\SPI\Persistence\User $spiUser Loaded user handler
Expand All @@ -1421,23 +1465,41 @@ private function getUserFieldDefinition(ContentType $contentType): ?FieldDefinit
*/
protected function verifyPassword($login, $password, $spiUser)
{
return $this->verifyPasswordForSPIUser($login, $password, $spiUser);
}

/**
* Verifies if the provided login and password are valid against given password hash and hash type.
*
* @param string $login User login
* @param string $plainPassword User password
* @param string $passwordHash User password hash
* @param int $hashAlgorithm Hash type
*
* @return bool return true if the login and password are sucessfully validate and false, if not.
*/
private function doVerifyPassword(
string $login,
string $plainPassword,
string $passwordHash,
int $hashAlgorithm
): bool {
// In case of bcrypt let php's password functionality do it's magic
if ($spiUser->hashAlgorithm === APIUser::PASSWORD_HASH_BCRYPT ||
$spiUser->hashAlgorithm === APIUser::PASSWORD_HASH_PHP_DEFAULT) {
return password_verify($password, $spiUser->passwordHash);
if ($hashAlgorithm === APIUser::PASSWORD_HASH_BCRYPT ||
$hashAlgorithm === APIUser::PASSWORD_HASH_PHP_DEFAULT
) {
return password_verify($plainPassword, $passwordHash);
}

// Randomize login time to protect against timing attacks
usleep(random_int(0, 30000));

$passwordHash = $this->createPasswordHash(
return $passwordHash === $this->createPasswordHash(
$login,
$password,
$plainPassword,
$this->settings['siteName'],
$spiUser->hashAlgorithm
$hashAlgorithm
);

return $passwordHash === $spiUser->passwordHash;
}

/**
Expand Down

0 comments on commit aa6e6af

Please sign in to comment.