diff --git a/src/app/Console/Commands/UserAddAlias.php b/src/app/Console/Commands/UserAddAlias.php --- a/src/app/Console/Commands/UserAddAlias.php +++ b/src/app/Console/Commands/UserAddAlias.php @@ -55,7 +55,7 @@ $controller = $user->wallet()->owner; // Validate the alias - $error = UsersController::validateEmail($alias, $controller, true); + $error = UsersController::validateAlias($alias, $controller); if ($error) { if (!$this->option('force')) { diff --git a/src/app/Http/Controllers/API/SignupController.php b/src/app/Http/Controllers/API/SignupController.php --- a/src/app/Http/Controllers/API/SignupController.php +++ b/src/app/Http/Controllers/API/SignupController.php @@ -371,7 +371,7 @@ // Check if user with specified login already exists $email = $login . '@' . $domain; - if (User::emailExists($email)) { + if (User::emailExists($email) || User::aliasExists($email)) { return ['login' => \trans('validation.loginexists')]; } diff --git a/src/app/Http/Controllers/API/V4/UsersController.php b/src/app/Http/Controllers/API/V4/UsersController.php --- a/src/app/Http/Controllers/API/V4/UsersController.php +++ b/src/app/Http/Controllers/API/V4/UsersController.php @@ -554,7 +554,7 @@ if (empty($email)) { $errors['email'] = \trans('validation.required', ['attribute' => 'email']); - } elseif ($error = self::validateEmail($email, $controller, false)) { + } elseif ($error = self::validateEmail($email, $controller)) { $errors['email'] = $error; } } @@ -574,7 +574,7 @@ // validate new aliases if ( !in_array($alias, $existing_aliases) - && ($error = self::validateEmail($alias, $controller, true)) + && ($error = self::validateAlias($alias, $controller)) ) { if (!isset($errors['aliases'])) { $errors['aliases'] = []; @@ -655,29 +655,81 @@ } /** - * Email address (login or alias) validation + * Email address validation for use as a user mailbox (login). * - * @param string $email Email address - * @param \App\User $user The account owner - * @param bool $is_alias The email is an alias + * @param string $email Email address + * @param \App\User $user The account owner * - * @return string Error message on validation error + * @return ?string Error message on validation error */ - public static function validateEmail( - string $email, - \App\User $user, - bool $is_alias = false - ): ?string { - $attribute = $is_alias ? 'alias' : 'email'; + public static function validateEmail(string $email, \App\User $user): ?string + { + if (strpos($email, '@') === false) { + return \trans('validation.entryinvalid', ['attribute' => 'email']); + } + + list($login, $domain) = explode('@', Str::lower($email)); + + if (strlen($login) === 0 || strlen($domain) === 0) { + return \trans('validation.entryinvalid', ['attribute' => 'email']); + } + + // Check if domain exists + $domain = Domain::where('namespace', $domain)->first(); + + if (empty($domain)) { + return \trans('validation.domaininvalid'); + } + + // Validate login part alone + $v = Validator::make( + ['email' => $login], + ['email' => ['required', new UserEmailLocal(!$domain->isPublic())]] + ); + + if ($v->fails()) { + return $v->errors()->toArray()['email'][0]; + } + // Check if it is one of domains available to the user + $domains = \collect($user->domains())->pluck('namespace')->all(); + + if (!in_array($domain->namespace, $domains)) { + return \trans('validation.entryexists', ['attribute' => 'domain']); + } + + // Check if a user with specified address already exists + if (User::emailExists($email)) { + // TODO: Allow force-delete if this is a deleted user in the same custom domain + return \trans('validation.entryexists', ['attribute' => 'email']); + } + + // Check if an alias with specified address already exists. + if (User::aliasExists($email)) { + return \trans('validation.entryexists', ['attribute' => 'email']); + } + + return null; + } + + /** + * Email address validation for use as an alias. + * + * @param string $email Email address + * @param \App\User $user The account owner + * + * @return ?string Error message on validation error + */ + public static function validateAlias(string $email, \App\User $user): ?string + { if (strpos($email, '@') === false) { - return \trans('validation.entryinvalid', ['attribute' => $attribute]); + return \trans('validation.entryinvalid', ['attribute' => 'alias']); } list($login, $domain) = explode('@', Str::lower($email)); if (strlen($login) === 0 || strlen($domain) === 0) { - return \trans('validation.entryinvalid', ['attribute' => $attribute]); + return \trans('validation.entryinvalid', ['attribute' => 'alias']); } // Check if domain exists @@ -689,12 +741,12 @@ // Validate login part alone $v = Validator::make( - [$attribute => $login], - [$attribute => ['required', new UserEmailLocal(!$domain->isPublic())]] + ['alias' => $login], + ['alias' => ['required', new UserEmailLocal(!$domain->isPublic())]] ); if ($v->fails()) { - return $v->errors()->toArray()[$attribute][0]; + return $v->errors()->toArray()['alias'][0]; } // Check if it is one of domains available to the user @@ -704,18 +756,20 @@ return \trans('validation.entryexists', ['attribute' => 'domain']); } - // Check if a user/alias with specified address already exists - // Allow assigning the same alias to a user in the same group account, - // but only for non-public domains - // Allow an alias in a custom domain to an address that was a user before - if ($exists = User::emailExists($email, true, $alias_exists, $is_alias && !$domain->isPublic())) { - if ( - !$is_alias - || !$alias_exists - || $domain->isPublic() - || $exists->wallet()->user_id != $user->id - ) { - return \trans('validation.entryexists', ['attribute' => $attribute]); + // Check if a user with specified address already exists + if ($existing_user = User::emailExists($email, true)) { + // Allow an alias in a custom domain to an address that was a user before + if ($domain->isPublic() || !$existing_user->trashed()) { + return \trans('validation.entryexists', ['attribute' => 'alias']); + } + } + + // Check if an alias with specified address already exists + if (User::aliasExists($email)) { + // Allow assigning the same alias to a user in the same group account, + // but only for non-public domains + if ($domain->isPublic()) { + return \trans('validation.entryexists', ['attribute' => 'alias']); } } diff --git a/src/app/Observers/UserAliasObserver.php b/src/app/Observers/UserAliasObserver.php --- a/src/app/Observers/UserAliasObserver.php +++ b/src/app/Observers/UserAliasObserver.php @@ -29,24 +29,7 @@ \Log::error("Failed creating alias {$alias->alias}. Domain does not exist."); return false; } -/* - if ($exists = User::emailExists($alias->alias, true, $alias_exists, !$domain->isPublic())) { - if (!$alias_exists) { - \Log::error("Failed creating alias {$alias->alias}. Email address exists."); - return false; - } - if ($domain->isPublic()) { - \Log::error("Failed creating alias {$alias->alias}. Alias exists in public domain."); - return false; - } - - if ($exists->wallet()->user_id != $alias->user->wallet()->user_id) { - \Log::error("Failed creating alias {$alias->alias}. Alias exists in another account."); - return false; - } - } -*/ return true; } diff --git a/src/app/Traits/UserAliasesTrait.php b/src/app/Traits/UserAliasesTrait.php --- a/src/app/Traits/UserAliasesTrait.php +++ b/src/app/Traits/UserAliasesTrait.php @@ -4,6 +4,27 @@ trait UserAliasesTrait { + /** + * Find whether an email address exists as an alias + * (including aliases of deleted users). + * + * @param string $email Email address + * + * @return bool True if found, False otherwise + */ + public static function aliasExists(string $email): bool + { + if (strpos($email, '@') === false) { + return false; + } + + $email = \strtolower($email); + + $count = \App\UserAlias::where('alias', $email)->count(); + + return $count > 0; + } + /** * A helper to update user aliases list. * diff --git a/src/app/User.php b/src/app/User.php --- a/src/app/User.php +++ b/src/app/User.php @@ -350,17 +350,14 @@ } /** - * Find whether an email address exists (user or alias). - * Note: This will also find deleted users. + * Find whether an email address exists as a user (including deleted users). * * @param string $email Email address * @param bool $return_user Return User instance instead of boolean - * @param bool $is_alias Set to True if the existing email is an alias - * @param bool $existing Ignore deleted users * * @return \App\User|bool True or User model object if found, False otherwise */ - public static function emailExists(string $email, bool $return_user = false, &$is_alias = false, $existing = false) + public static function emailExists(string $email, bool $return_user = false) { if (strpos($email, '@') === false) { return false; @@ -368,30 +365,12 @@ $email = \strtolower($email); - if ($existing) { - $user = self::where('email', $email)->first(); - } else { - $user = self::withTrashed()->where('email', $email)->first(); - } + $user = self::withTrashed()->where('email', $email)->first(); if ($user) { return $return_user ? $user : true; } - $aliases = UserAlias::where('alias', $email); - - if ($existing) { - $aliases = $aliases->join('users', 'user_id', '=', 'users.id') - ->whereNull('users.deleted_at'); - } - - $alias = $aliases->first(); - - if ($alias) { - $is_alias = true; - return $return_user ? self::withTrashed()->find($alias->user_id) : true; - } - return false; } diff --git a/src/tests/Feature/Controller/UsersTest.php b/src/tests/Feature/Controller/UsersTest.php --- a/src/tests/Feature/Controller/UsersTest.php +++ b/src/tests/Feature/Controller/UsersTest.php @@ -1027,7 +1027,7 @@ } /** - * List of alias validation cases for testValidateEmail() + * List of email address validation cases for testValidateEmail() * * @return array Arguments for testValidateEmail() */ @@ -1043,61 +1043,113 @@ return [ // Invalid format - ["$domain", $john, true, 'The specified alias is invalid.'], - [".@$domain", $john, true, 'The specified alias is invalid.'], - ["test123456@localhost", $john, true, 'The specified domain is invalid.'], - ["test123456@unknown-domain.org", $john, true, 'The specified domain is invalid.'], + ["$domain", $john, 'The specified email is invalid.'], + [".@$domain", $john, 'The specified email is invalid.'], + ["test123456@localhost", $john, 'The specified domain is invalid.'], + ["test123456@unknown-domain.org", $john, 'The specified domain is invalid.'], - ["$domain", $john, false, 'The specified email is invalid.'], - [".@$domain", $john, false, 'The specified email is invalid.'], + ["$domain", $john, 'The specified email is invalid.'], + [".@$domain", $john, 'The specified email is invalid.'], // forbidden local part on public domains - ["admin@$domain", $john, true, 'The specified alias is not available.'], - ["administrator@$domain", $john, true, 'The specified alias is not available.'], + ["admin@$domain", $john, 'The specified email is not available.'], + ["administrator@$domain", $john, 'The specified email is not available.'], // forbidden (other user's domain) - ["testtest@kolab.org", $user, true, 'The specified domain is not available.'], + ["testtest@kolab.org", $user, 'The specified domain is not available.'], // existing alias of other user, to be a user email - ["jack.daniels@kolab.org", $john, false, 'The specified email is not available.'], + ["jack.daniels@kolab.org", $john, 'The specified email is not available.'], + + // valid (user domain) + ["admin@kolab.org", $john, null], + + // valid (public domain) + ["test.test@$domain", $john, null], + ]; + } + + /** + * User email address validation. + * + * Note: Technically these include unit tests, but let's keep it here for now. + * FIXME: Shall we do a http request for each case? + * + * @dataProvider dataValidateEmail + */ + public function testValidateEmail($email, $user, $expected_result): void + { + $result = UsersController::validateEmail($email, $user); + $this->assertSame($expected_result, $result); + } + + /** + * List of alias validation cases for testValidateAlias() + * + * @return array Arguments for testValidateAlias() + */ + public function dataValidateAlias(): array + { + $this->refreshApplication(); + $public_domains = Domain::getPublicDomains(); + $domain = reset($public_domains); + + $john = $this->getTestUser('john@kolab.org'); + $jack = $this->getTestUser('jack@kolab.org'); + $user = $this->getTestUser('UsersControllerTest1@userscontroller.com'); + + return [ + // Invalid format + ["$domain", $john, 'The specified alias is invalid.'], + [".@$domain", $john, 'The specified alias is invalid.'], + ["test123456@localhost", $john, 'The specified domain is invalid.'], + ["test123456@unknown-domain.org", $john, 'The specified domain is invalid.'], + + ["$domain", $john, 'The specified alias is invalid.'], + [".@$domain", $john, 'The specified alias is invalid.'], + + // forbidden local part on public domains + ["admin@$domain", $john, 'The specified alias is not available.'], + ["administrator@$domain", $john, 'The specified alias is not available.'], + + // forbidden (other user's domain) + ["testtest@kolab.org", $user, 'The specified domain is not available.'], // existing alias of other user, to be an alias, user in the same group account - ["jack.daniels@kolab.org", $john, true, null], + ["jack.daniels@kolab.org", $john, null], // existing user - ["jack@kolab.org", $john, true, 'The specified alias is not available.'], + ["jack@kolab.org", $john, 'The specified alias is not available.'], // valid (user domain) - ["admin@kolab.org", $john, true, null], + ["admin@kolab.org", $john, null], // valid (public domain) - ["test.test@$domain", $john, true, null], + ["test.test@$domain", $john, null], ]; } /** - * User email/alias validation. + * User email alias validation. * * Note: Technically these include unit tests, but let's keep it here for now. * FIXME: Shall we do a http request for each case? * - * @dataProvider dataValidateEmail + * @dataProvider dataValidateAlias */ - public function testValidateEmail($alias, $user, $is_alias, $expected_result): void + public function testValidateAlias($alias, $user, $expected_result): void { - $args = [$alias, $user, $is_alias]; - $result = $this->invokeMethod(new UsersController(), 'validateEmail', $args); - + $result = UsersController::validateAlias($alias, $user); $this->assertSame($expected_result, $result); } /** - * User email/alias validation - more cases. + * User alias validation - more cases. * * Note: Technically these include unit tests, but let's keep it here for now. * FIXME: Shall we do a http request for each case? */ - public function testValidateEmail2(): void + public function testValidateAlias2(): void { Queue::fake(); @@ -1112,16 +1164,16 @@ $deleted_pub->delete(); // An alias that was a user email before is allowed, but only for custom domains - $result = UsersController::validateEmail('deleted@kolab.org', $john, true); + $result = UsersController::validateAlias('deleted@kolab.org', $john); $this->assertSame(null, $result); - $result = UsersController::validateEmail('deleted-alias@kolab.org', $john, true); + $result = UsersController::validateAlias('deleted-alias@kolab.org', $john); $this->assertSame(null, $result); - $result = UsersController::validateEmail('deleted@kolabnow.com', $john, true); + $result = UsersController::validateAlias('deleted@kolabnow.com', $john); $this->assertSame('The specified alias is not available.', $result); - $result = UsersController::validateEmail('deleted-alias@kolabnow.com', $john, true); + $result = UsersController::validateAlias('deleted-alias@kolabnow.com', $john); $this->assertSame('The specified alias is not available.', $result); } } diff --git a/src/tests/Feature/UserTest.php b/src/tests/Feature/UserTest.php --- a/src/tests/Feature/UserTest.php +++ b/src/tests/Feature/UserTest.php @@ -280,12 +280,28 @@ $this->assertFalse($domain->isDeleted()); } + /** + * Tests for User::aliasExists() + */ + public function testAliasExists(): void + { + $this->assertTrue(User::aliasExists('jack.daniels@kolab.org')); + + $this->assertFalse(User::aliasExists('j.daniels@kolab.org')); + $this->assertFalse(User::aliasExists('john@kolab.org')); + } + /** * Tests for User::emailExists() */ public function testEmailExists(): void { - $this->markTestIncomplete(); + $this->assertFalse(User::emailExists('jack.daniels@kolab.org')); + $this->assertFalse(User::emailExists('j.daniels@kolab.org')); + + $this->assertTrue(User::emailExists('john@kolab.org')); + $user = User::emailExists('john@kolab.org', true); + $this->assertSame('john@kolab.org', $user->email); } /**