prevent email or username from being non-unique (#2730)

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) {

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

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.';

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());
+            $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 (null !== $existingByEmail && $user->getId() !== $existingByEmail->getId()) {
-                $context->buildViolation(User::getErrorName(User::USER_EXISTING_EMAIL))
-                    ->atPath('email')
-                    ->setTranslationDomain('validators')
-                    ->setCode(User::USER_EXISTING_EMAIL)
-                    ->addViolation();
-            }
         }
 
         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()) {
+    private function validateEmailExists(?int $userId, string $email, string $path, string $code, ExecutionContextInterface $context): void
-                $context->buildViolation(User::getErrorName(User::USER_EXISTING_NAME))
+    {
-                    ->atPath('username')
+        $existingByEmail = $this->userService->findUserByEmail($email);
-                    ->setTranslationDomain('validators')
+
-                    ->setCode(User::USER_EXISTING_NAME)
+        if (null !== $existingByEmail && $userId !== $existingByEmail->getId()) {
-                    ->addViolation();
+            $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();
         }
     }
 }

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->exactly(2))->method('findUserByEmail')->willReturn($existing);
-        $userService->expects($this->once())->method('findUserByName')->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();
     }
 }

translations/validators.de.xlf

@@ -18,6 +18,14 @@
                 <source>The username is already used.</source>
                 <target>Dieser Benutzername wird bereits verwendet.</target>
             </trans-unit>
+            <trans-unit id="An equal username is already used.">
+                <source>An equal username is already used.</source>
+                <target>Eine gleichlautender Benutzername wird bereits verwendet.</target>
+            </trans-unit>
+            <trans-unit id="An equal email is already used.">
+                <source>An equal email is already used.</source>
+                <target>Eine gleichlautende E-Mail-Adresse wird bereits verwendet.</target>
+            </trans-unit>
             <trans-unit id="This value is not a valid role.">
                 <source>This value is not a valid role.</source>
                 <target>Dieser Wert ist keine gültige Rolle.</target>

translations/validators.en.xlf

@@ -18,6 +18,14 @@
                 <source>The username is already used.</source>
                 <target>The username is already used.</target>
             </trans-unit>
+            <trans-unit id="An equal username is already used.">
+                <source>An equal username is already used.</source>
+                <target>An equal username is already used.</target>
+            </trans-unit>
+            <trans-unit id="An equal email is already used.">
+                <source>An equal email is already used.</source>
+                <target>An equal email is already used.</target>
+            </trans-unit>
             <trans-unit id="This value is not a valid role.">
                 <source>This value is not a valid role.</source>
                 <target>This value is not a valid role.</target>