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::findByEmail($email)) { + if (User::emailExists($email)) { return ['login' => \trans('validation.loginexists')]; } diff --git a/src/app/Http/Controllers/API/V4/Admin/UsersController.php b/src/app/Http/Controllers/API/V4/Admin/UsersController.php --- a/src/app/Http/Controllers/API/V4/Admin/UsersController.php +++ b/src/app/Http/Controllers/API/V4/Admin/UsersController.php @@ -4,6 +4,7 @@ use App\Domain; use App\User; +use App\UserAlias; use App\UserSetting; use Illuminate\Http\Request; use Illuminate\Support\Facades\Validator; @@ -27,16 +28,21 @@ } } elseif (strpos($search, '@')) { // Search by email - if ($user = User::findByEmail($search, false)) { + $user = User::where('email', $search)->first(); + if ($user) { $result->push($user); } else { - // Search by an external email - // TODO: This is not optimal (external email should be in users table) - $user_ids = UserSetting::where('key', 'external_email')->where('value', $search) - ->get()->pluck('user_id'); + // Search by an alias + $user_ids = UserAlias::where('alias', $search)->get()->pluck('user_id'); + if ($user_ids->isEmpty()) { + // Search by an external email + $user_ids = UserSetting::where('key', 'external_email') + ->where('value', $search)->get()->pluck('user_id'); + } - // TODO: Sort order - $result = User::find($user_ids); + if (!$user_ids->isEmpty()) { + $result = User::whereIn('id', $user_ids)->orderBy('email')->get(); + } } } elseif (is_numeric($search)) { // Search by user ID 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 @@ -514,7 +514,7 @@ if (empty($email)) { $errors['email'] = \trans('validation.required', ['attribute' => 'email']); - } elseif ($error = \App\Utils::validateEmail($email, $controller, false)) { + } elseif ($error = self::validateEmail($email, $controller, false)) { $errors['email'] = $error; } } @@ -534,7 +534,7 @@ // validate new aliases if ( !in_array($alias, $existing_aliases) - && ($error = \App\Utils::validateEmail($alias, $controller, true)) + && ($error = self::validateEmail($alias, $controller, true)) ) { if (!isset($errors['aliases'])) { $errors['aliases'] = []; @@ -598,4 +598,74 @@ return false; } + + /** + * Email address (login or alias) validation + * + * @param string $email Email address + * @param \App\User $user The account owner + * @param bool $is_alias The email is an alias + * + * @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'; + + if (strpos($email, '@') === false) { + return \trans('validation.entryinvalid', ['attribute' => $attribute]); + } + + list($login, $domain) = explode('@', $email); + + // Check if domain exists + $domain = Domain::where('namespace', Str::lower($domain))->first(); + + if (empty($domain)) { + return \trans('validation.domaininvalid'); + } + + // Validate login part alone + $v = Validator::make( + [$attribute => $login], + [$attribute => ['required', new UserEmailLocal(!$domain->isPublic())]] + ); + + if ($v->fails()) { + return $v->errors()->toArray()[$attribute][0]; + } + + // Check if it is one of domains available to the user + // TODO: We should have a helper that returns "flat" array with domain names + // I guess we could use pluck() somehow + $domains = array_map( + function ($domain) { + return $domain->namespace; + }, + $user->domains() + ); + + if (!in_array($domain->namespace, $domains)) { + 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 + if ($exists = User::emailExists($email, true, $alias_exists)) { + if ( + !$is_alias + || !$alias_exists + || $domain->isPublic() + || $exists->wallet()->user_id != $user->id + ) { + return \trans('validation.entryexists', ['attribute' => $attribute]); + } + } + + return null; + } } 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 @@ -2,6 +2,7 @@ namespace App\Observers; +use App\Domain; use App\User; use App\UserAlias; @@ -20,9 +21,25 @@ { $alias->alias = \strtolower($alias->alias); - if (User::where('email', $alias->alias)->first()) { - \Log::error("Failed creating alias {$alias->alias}. User exists."); - return false; + if ($exists = User::emailExists($alias->alias, true, $alias_exists)) { + if (!$alias_exists) { + \Log::error("Failed creating alias {$alias->alias}. Email address exists."); + return false; + } + + list($login, $domain) = explode('@', $alias->alias); + + $domain = Domain::where('namespace', $domain)->first(); + + if (!$domain || $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; @@ -37,7 +54,9 @@ */ public function created(UserAlias $alias) { - \App\Jobs\UserUpdate::dispatch($alias->user); + if ($alias->user) { + \App\Jobs\UserUpdate::dispatch($alias->user); + } } /** @@ -49,7 +68,9 @@ */ public function updated(UserAlias $alias) { - \App\Jobs\UserUpdate::dispatch($alias->user); + if ($alias->user) { + \App\Jobs\UserUpdate::dispatch($alias->user); + } } /** @@ -61,6 +82,8 @@ */ public function deleted(UserAlias $alias) { - \App\Jobs\UserUpdate::dispatch($alias->user); + if ($alias->user) { + \App\Jobs\UserUpdate::dispatch($alias->user); + } } } diff --git a/src/app/User.php b/src/app/User.php --- a/src/app/User.php +++ b/src/app/User.php @@ -336,11 +336,47 @@ } /** + * Find whether an email address exists (user or alias). + * Note: This will also find 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 + * + * @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) + { + if (strpos($email, '@') === false) { + return false; + } + + $email = \strtolower($email); + + $user = self::withTrashed()->where('email', $email)->first(); + + if ($user) { + return $return_user ? $user : true; + } + + $alias = UserAlias::where('alias', $email)->first(); + + if ($alias) { + $is_alias = true; + return $return_user ? User::withTrashed()->find($alias->user_id) : true; + } + + return false; + } + + /** * Helper to find user by email address, whether it is - * main email address, alias or external email + * main email address, alias or an external email. + * + * If there's more than one alias NULL will be returned. * * @param string $email Email address - * @param bool $external Search also by an external email + * @param bool $external Search also for an external email * * @return \App\User User model object if found */ @@ -358,10 +394,10 @@ return $user; } - $alias = UserAlias::where('alias', $email)->first(); + $aliases = UserAlias::where('alias', $email)->get(); - if ($alias) { - return $alias->user; + if (count($aliases) == 1) { + return $aliases->first()->user; } // TODO: External email diff --git a/src/app/Utils.php b/src/app/Utils.php --- a/src/app/Utils.php +++ b/src/app/Utils.php @@ -2,11 +2,8 @@ namespace App; -use App\Rules\UserEmailLocal; use Carbon\Carbon; use Illuminate\Support\Facades\Auth; -use Illuminate\Support\Facades\Validator; -use Illuminate\Support\Str; use Ramsey\Uuid\Uuid; /** @@ -155,65 +152,4 @@ return $env; } - - /** - * Email address (login or alias) validation - * - * @param string $email Email address - * @param \App\User $user The account owner - * @param bool $is_alias The email is an alias - * - * @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'; - - if (strpos($email, '@') === false) { - return \trans('validation.entryinvalid', ['attribute' => $attribute]); - } - - list($login, $domain) = explode('@', $email); - - // Check if domain exists - $domain = Domain::where('namespace', Str::lower($domain))->first(); - - if (empty($domain)) { - return \trans('validation.domaininvalid'); - } - - // Validate login part alone - $v = Validator::make( - [$attribute => $login], - [$attribute => ['required', new UserEmailLocal(!$domain->isPublic())]] - ); - - if ($v->fails()) { - return $v->errors()->toArray()[$attribute][0]; - } - - // Check if it is one of domains available to the user - // TODO: We should have a helper that returns "flat" array with domain names - // I guess we could use pluck() somehow - $domains = array_map( - function ($domain) { - return $domain->namespace; - }, - $user->domains() - ); - - if (!in_array($domain->namespace, $domains)) { - return \trans('validation.entryexists', ['attribute' => 'domain']); - } - - // Check if user with specified address already exists - if (User::findByEmail($email)) { - return \trans('validation.entryexists', ['attribute' => $attribute]); - } - - return null; - } } diff --git a/src/database/migrations/2020_06_18_150000_update_user_aliases_unique_key.php b/src/database/migrations/2020_06_18_150000_update_user_aliases_unique_key.php new file mode 100644 --- /dev/null +++ b/src/database/migrations/2020_06_18_150000_update_user_aliases_unique_key.php @@ -0,0 +1,33 @@ +dropUnique(['alias']); + $table->unique(['alias', 'user_id']); + } + ); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + } +} diff --git a/src/database/seeds/local/UserSeeder.php b/src/database/seeds/local/UserSeeder.php --- a/src/database/seeds/local/UserSeeder.php +++ b/src/database/seeds/local/UserSeeder.php @@ -125,6 +125,8 @@ $john->assignPackage($package_lite, $joe); + $joe->setAliases(['joe.monster@kolab.org']); + factory(User::class, 10)->create(); $jeroen = User::create( 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 @@ -970,8 +970,11 @@ // forbidden (other user's domain) ["testtest@kolab.org", $user, true, 'The specified domain is not available.'], - // existing alias of other user - ["jack.daniels@kolab.org", $john, true, 'The specified alias 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.'], + + // existing alias of other user, to be an alias, user in the same group account + ["jack.daniels@kolab.org", $john, true, null], // existing user ["jack@kolab.org", $john, true, 'The specified alias is not available.'], @@ -994,7 +997,8 @@ */ public function testValidateEmail($alias, $user, $is_alias, $expected_result): void { - $result = $this->invokeMethod(new \App\Utils(), 'validateEmail', [$alias, $user, $is_alias]); + $args = [$alias, $user, $is_alias]; + $result = $this->invokeMethod(new UsersController(), 'validateEmail', $args); $this->assertSame($expected_result, $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 @@ -259,6 +259,14 @@ } /** + * Tests for User::emailExists() + */ + public function testEmailExists(): void + { + $this->markTestIncomplete(); + } + + /** * Tests for User::findByEmail() */ public function testFindByEmail(): void @@ -280,6 +288,13 @@ $this->assertInstanceOf(User::class, $result); $this->assertSame($user->id, $result->id); + // A case where two users have the same alias + $ned = $this->getTestUser('ned@kolab.org'); + $ned->setAliases(['joe.monster@kolab.org']); + $result = User::findByEmail('joe.monster@kolab.org'); + $this->assertNull($result); + $ned->setAliases([]); + // TODO: searching by external email (setting) $this->markTestIncomplete(); } @@ -353,6 +368,37 @@ Queue::assertPushed(\App\Jobs\UserUpdate::class, 4); $this->assertCount(0, $user->aliases()->get()); + + // Test sanity checks in UserAliasObserver + Queue::fake(); + + // Existing user + $user->setAliases(['john@kolab.org']); + $this->assertCount(0, $user->aliases()->get()); + + // Existing alias (in another account) + $user->setAliases(['john.doe@kolab.org']); + $this->assertCount(0, $user->aliases()->get()); + + Queue::assertNothingPushed(); + + // Existing user (in the same group account) + $ned = $this->getTestUser('ned@kolab.org'); + $ned->setAliases(['john@kolab.org']); + $this->assertCount(0, $ned->aliases()->get()); + + // Existing alias (in the same group account) + $ned = $this->getTestUser('ned@kolab.org'); + $ned->setAliases(['john.doe@kolab.org']); + $this->assertSame('john.doe@kolab.org', $ned->aliases()->first()->alias); + + // Existing alias (in another account, public domain) + $user->setAliases(['alias@kolabnow.com']); + $ned->setAliases(['alias@kolabnow.com']); + $this->assertCount(0, $ned->aliases()->get()); + + // cleanup + $ned->setAliases([]); } /**