From 5023fa61894da4ecd297499d049a66ab35fe7925 Mon Sep 17 00:00:00 2001 From: Kevin Papst Date: Wed, 25 Aug 2021 15:18:46 +0200 Subject: [PATCH] prevent email or username from being non-unique (#2730) --- src/Repository/UserRepository.php | 5 ++ src/User/UserService.php | 2 +- src/Validator/Constraints/User.php | 4 ++ src/Validator/Constraints/UserValidator.php | 47 ++++++++++++------- .../Constraints/UserValidatorTest.php | 15 ++++-- translations/validators.de.xlf | 8 ++++ translations/validators.en.xlf | 8 ++++ 7 files changed, 67 insertions(+), 22 deletions(-) diff --git a/src/Repository/UserRepository.php b/src/Repository/UserRepository.php index 9d667010..c34aa52b 100644 --- a/src/Repository/UserRepository.php +++ b/src/Repository/UserRepository.php @@ -82,6 +82,11 @@ class UserRepository extends EntityRepository implements UserLoaderInterface return parent::findOneBy($criteria, $orderBy); } + public function findByUsername($username): ?User + { + return parent::findOneBy(['username' => $username]); + } + public function countUser(?bool $enabled = null): int { if (null !== $enabled) { diff --git a/src/User/UserService.php b/src/User/UserService.php index a979032a..0237eb35 100644 --- a/src/User/UserService.php +++ b/src/User/UserService.php @@ -127,7 +127,7 @@ class UserService public function findUserByName(string $name): ?User { - return $this->repository->findOneBy(['username' => $name]); + return $this->repository->findByUsername($name); } public function findUserByConfirmationToken(string $token): ?User diff --git a/src/Validator/Constraints/User.php b/src/Validator/Constraints/User.php index 77cd5fe8..142bb908 100644 --- a/src/Validator/Constraints/User.php +++ b/src/Validator/Constraints/User.php @@ -20,10 +20,14 @@ class User extends Constraint { public const USER_EXISTING_EMAIL = 'kimai-user-00'; public const USER_EXISTING_NAME = 'kimai-user-01'; + public const USER_EXISTING_EMAIL_AS_NAME = 'kimai-user-02'; + public const USER_EXISTING_NAME_AS_EMAIL = 'kimai-user-03'; protected static $errorNames = [ self::USER_EXISTING_EMAIL => 'The email is already used.', self::USER_EXISTING_NAME => 'The username is already used.', + self::USER_EXISTING_EMAIL_AS_NAME => 'An equal username is already used.', + self::USER_EXISTING_NAME_AS_EMAIL => 'An equal email is already used.', ]; public $message = 'The user has invalid settings.'; diff --git a/src/Validator/Constraints/UserValidator.php b/src/Validator/Constraints/UserValidator.php index fa764b9a..f86b57ff 100644 --- a/src/Validator/Constraints/UserValidator.php +++ b/src/Validator/Constraints/UserValidator.php @@ -44,28 +44,41 @@ class UserValidator extends ConstraintValidator protected function validateUser(UserEntity $user, ExecutionContextInterface $context) { + $matchedEmail = false; if ($user->getEmail() !== null) { - $existingByEmail = $this->userService->findUserByEmail($user->getEmail()); - - if (null !== $existingByEmail && $user->getId() !== $existingByEmail->getId()) { - $context->buildViolation(User::getErrorName(User::USER_EXISTING_EMAIL)) - ->atPath('email') - ->setTranslationDomain('validators') - ->setCode(User::USER_EXISTING_EMAIL) - ->addViolation(); - } + $this->validateEmailExists($user->getId(), $user->getEmail(), 'email', User::USER_EXISTING_EMAIL, $context); + $this->validateEmailExists($user->getId(), $user->getUsername(), 'username', User::USER_EXISTING_NAME_AS_EMAIL, $context); } if ($user->getUsername() !== null) { - $existingByName = $this->userService->findUserByName($user->getUsername()); + $this->validateUsernameExists($user->getId(), $user->getUsername(), 'username', User::USER_EXISTING_NAME, $context); + $this->validateUsernameExists($user->getId(), $user->getEmail(), 'email', User::USER_EXISTING_EMAIL_AS_NAME, $context); + } + } - if (null !== $existingByName && $user->getId() !== $existingByName->getId()) { - $context->buildViolation(User::getErrorName(User::USER_EXISTING_NAME)) - ->atPath('username') - ->setTranslationDomain('validators') - ->setCode(User::USER_EXISTING_NAME) - ->addViolation(); - } + private function validateEmailExists(?int $userId, string $email, string $path, string $code, ExecutionContextInterface $context): void + { + $existingByEmail = $this->userService->findUserByEmail($email); + + if (null !== $existingByEmail && $userId !== $existingByEmail->getId()) { + $context->buildViolation(User::getErrorName($code)) + ->atPath($path) + ->setTranslationDomain('validators') + ->setCode($code) + ->addViolation(); + } + } + + private function validateUsernameExists(?int $userId, string $username, string $path, string $code, ExecutionContextInterface $context): void + { + $existingByName = $this->userService->findUserByName($username); + + if (null !== $existingByName && $userId !== $existingByName->getId()) { + $context->buildViolation(User::getErrorName($code)) + ->atPath($path) + ->setTranslationDomain('validators') + ->setCode($code) + ->addViolation(); } } } diff --git a/tests/Validator/Constraints/UserValidatorTest.php b/tests/Validator/Constraints/UserValidatorTest.php index 432e59f3..6bb3504f 100644 --- a/tests/Validator/Constraints/UserValidatorTest.php +++ b/tests/Validator/Constraints/UserValidatorTest.php @@ -72,11 +72,11 @@ class UserValidatorTest extends ConstraintValidatorTestCase public function testUserIsInvalidWithRepository() { $existing = $this->createMock(UserEntity::class); - $existing->expects($this->exactly(2))->method('getId')->willReturn(123); + $existing->expects($this->exactly(4))->method('getId')->willReturn(123); $userService = $this->createMock(UserService::class); - $userService->expects($this->once())->method('findUserByEmail')->willReturn($existing); - $userService->expects($this->once())->method('findUserByName')->willReturn($existing); + $userService->expects($this->exactly(2))->method('findUserByEmail')->willReturn($existing); + $userService->expects($this->exactly(2))->method('findUserByName')->willReturn($existing); $this->validator = new UserValidator($userService); $this->validator->initialize($this->context); @@ -87,12 +87,19 @@ class UserValidatorTest extends ConstraintValidatorTestCase $this->validator->validate($user, new User()); - $this->buildViolation('The email is already used.') + $this + ->buildViolation('The email is already used.') ->atPath('property.path.email') ->setCode(User::USER_EXISTING_EMAIL) + ->buildNextViolation('An equal email is already used.') + ->atPath('property.path.username') + ->setCode(User::USER_EXISTING_NAME_AS_EMAIL) ->buildNextViolation('The username is already used.') ->atPath('property.path.username') ->setCode(User::USER_EXISTING_NAME) + ->buildNextViolation('An equal username is already used.') + ->atPath('property.path.email') + ->setCode(User::USER_EXISTING_EMAIL_AS_NAME) ->assertRaised(); } } diff --git a/translations/validators.de.xlf b/translations/validators.de.xlf index d32b8647..b13dbeaa 100644 --- a/translations/validators.de.xlf +++ b/translations/validators.de.xlf @@ -18,6 +18,14 @@ The username is already used. Dieser Benutzername wird bereits verwendet. + + An equal username is already used. + Eine gleichlautender Benutzername wird bereits verwendet. + + + An equal email is already used. + Eine gleichlautende E-Mail-Adresse wird bereits verwendet. + This value is not a valid role. Dieser Wert ist keine gültige Rolle. diff --git a/translations/validators.en.xlf b/translations/validators.en.xlf index bd79b263..60b30965 100644 --- a/translations/validators.en.xlf +++ b/translations/validators.en.xlf @@ -18,6 +18,14 @@ The username is already used. The username is already used. + + An equal username is already used. + An equal username is already used. + + + An equal email is already used. + An equal email is already used. + This value is not a valid role. This value is not a valid role.