From aa6e6afeadb44cd1a97c90bebd194a17ea78aa14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20W=C3=B3js?= Date: Mon, 30 Sep 2019 09:55:51 +0200 Subject: [PATCH] EZP-30983: Prevented users from reusing old password --- .../Tests/FieldType/UserIntegrationTest.php | 5 ++ .../API/Repository/Tests/UserServiceTest.php | 29 ++++++- eZ/Publish/Core/FieldType/Tests/UserTest.php | 4 + eZ/Publish/Core/FieldType/User/Type.php | 4 + .../FieldValue/Converter/UserConverter.php | 3 + eZ/Publish/Core/Repository/UserService.php | 86 ++++++++++++++++--- 6 files changed, 117 insertions(+), 14 deletions(-) diff --git a/eZ/Publish/API/Repository/Tests/FieldType/UserIntegrationTest.php b/eZ/Publish/API/Repository/Tests/FieldType/UserIntegrationTest.php index 5b262ff70b6..7266a327b72 100644 --- a/eZ/Publish/API/Repository/Tests/FieldType/UserIntegrationTest.php +++ b/eZ/Publish/API/Repository/Tests/FieldType/UserIntegrationTest.php @@ -102,6 +102,10 @@ public function getValidatorSchema() 'type' => 'int', 'default' => null, ], + 'requireNewPassword' => [ + 'type' => 'int', + 'default' => null, + ], 'minLength' => [ 'type' => 'int', 'default' => 10, @@ -123,6 +127,7 @@ public function getValidValidatorConfiguration() 'requireAtLeastOneLowerCaseCharacter' => true, 'requireAtLeastOneNumericCharacter' => true, 'requireAtLeastOneNonAlphanumericCharacter' => false, + 'requireNewPassword' => false, 'minLength' => 10, ], ]; diff --git a/eZ/Publish/API/Repository/Tests/UserServiceTest.php b/eZ/Publish/API/Repository/Tests/UserServiceTest.php index 4eb12760216..772235a3903 100644 --- a/eZ/Publish/API/Repository/Tests/UserServiceTest.php +++ b/eZ/Publish/API/Repository/Tests/UserServiceTest.php @@ -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(); @@ -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); } /** @@ -3021,6 +3045,7 @@ private function createUserContentTypeWithStrongPassword(): ContentType 'requireAtLeastOneLowerCaseCharacter' => 1, 'requireAtLeastOneNumericCharacter' => 1, 'requireAtLeastOneNonAlphanumericCharacter' => 1, + 'requireNewPassword' => 1, 'minLength' => 8, ], ]); diff --git a/eZ/Publish/Core/FieldType/Tests/UserTest.php b/eZ/Publish/Core/FieldType/Tests/UserTest.php index 8fa90a0b5ea..c9cb3ecd7ac 100644 --- a/eZ/Publish/Core/FieldType/Tests/UserTest.php +++ b/eZ/Publish/Core/FieldType/Tests/UserTest.php @@ -66,6 +66,10 @@ protected function getValidatorConfigurationSchemaExpectation() 'type' => 'int', 'default' => null, ], + 'requireNewPassword' => [ + 'type' => 'int', + 'default' => null, + ], 'minLength' => [ 'type' => 'int', 'default' => 10, diff --git a/eZ/Publish/Core/FieldType/User/Type.php b/eZ/Publish/Core/FieldType/User/Type.php index dc7031caadc..f68032ef2cd 100644 --- a/eZ/Publish/Core/FieldType/User/Type.php +++ b/eZ/Publish/Core/FieldType/User/Type.php @@ -63,6 +63,10 @@ class Type extends FieldType 'type' => 'int', 'default' => null, ], + 'requireNewPassword' => [ + 'type' => 'int', + 'default' => null, + ], 'minLength' => [ 'type' => 'int', 'default' => 10, diff --git a/eZ/Publish/Core/Persistence/Legacy/Content/FieldValue/Converter/UserConverter.php b/eZ/Publish/Core/Persistence/Legacy/Content/FieldValue/Converter/UserConverter.php index 822db5fa380..96d69ff5270 100644 --- a/eZ/Publish/Core/Persistence/Legacy/Content/FieldValue/Converter/UserConverter.php +++ b/eZ/Publish/Core/Persistence/Legacy/Content/FieldValue/Converter/UserConverter.php @@ -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} @@ -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; @@ -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) { diff --git a/eZ/Publish/Core/Repository/UserService.php b/eZ/Publish/Core/Repository/UserService.php index 959f8422c50..b3f66b8ad90 100644 --- a/eZ/Publish/Core/Repository/UserService.php +++ b/eZ/Publish/Core/Repository/UserService.php @@ -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; @@ -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); } @@ -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'] @@ -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; } /** @@ -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 @@ -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; } /**