Page MenuHomePhorge
Files F117750051

D1402.1775181682.diff
No OneTemporary
Actions

Authored By
Unknown
Size
16 KB
Referenced Files
None
Subscribers
None

D1402.1775181682.diff
View Options

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,68 @@
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 with specified address already exists
+ // Allow assigning the same alias to a user in the same group account
+ if ($exists = User::emailExists($email, true, $alias_exists)) {
+ if (!$is_alias || !$alias_exists || $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
@@ -20,9 +20,14 @@
{
$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, $exists_and_is_alias)) {
+ if (
+ !$exists_and_is_alias
+ || $exists->wallet()->user_id != $alias->user->wallet()->user_id
+ ) {
+ \Log::error("Failed creating alias {$alias->alias}. Email address exists.");
+ return false;
+ }
}
return true;
@@ -37,7 +42,9 @@
*/
public function created(UserAlias $alias)
{
- \App\Jobs\UserUpdate::dispatch($alias->user);
+ if ($alias->user) {
+ \App\Jobs\UserUpdate::dispatch($alias->user);
+ }
}
/**
@@ -49,7 +56,9 @@
*/
public function updated(UserAlias $alias)
{
- \App\Jobs\UserUpdate::dispatch($alias->user);
+ if ($alias->user) {
+ \App\Jobs\UserUpdate::dispatch($alias->user);
+ }
}
/**
@@ -61,6 +70,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
@@ -335,12 +335,48 @@
}
}
+ /**
+ * 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 @@
+<?php
+
+use Illuminate\Support\Facades\Schema;
+use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Database\Migrations\Migration;
+
+class UpdateUserAliasesUniqueKey extends Migration
+{
+ /**
+ * Run the migrations.
+ *
+ * @return void
+ */
+ public function up()
+ {
+ Schema::table(
+ 'user_aliases',
+ function (Blueprint $table) {
+ $table->dropUnique(['alias']);
+ $table->unique(['alias', 'user_id']);
+ }
+ );
+ }
+
+ /**
+ * Reverse the migrations.
+ *
+ * @return void
+ */
+ public function down()
+ {
+ }
+}
diff --git a/src/database/seeds/UserSeeder.php b/src/database/seeds/UserSeeder.php
--- a/src/database/seeds/UserSeeder.php
+++ b/src/database/seeds/UserSeeder.php
@@ -124,6 +124,8 @@
$john->assignPackage($package_lite, $joe);
+ $joe->setAliases(['joe.monster@kolabnow.com']);
+
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,14 @@
// 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 alias of other user, to be an alias, user not in the same group account
+ ["joe.monster@kolabnow.com", $user, true, 'The specified alias is not available.'],
// existing user
["jack@kolab.org", $john, true, 'The specified alias is not available.'],
@@ -994,7 +1000,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
@@ -258,6 +258,14 @@
$this->assertFalse($domain->isDeleted());
}
+ /**
+ * Tests for User::emailExists()
+ */
+ public function testEmailExists(): void
+ {
+ $this->markTestIncomplete();
+ }
+
/**
* Tests for User::findByEmail()
*/
@@ -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@kolabnow.com']);
+ $result = User::findByEmail('joe.monster@kolabnow.com');
+ $this->assertNull($result);
+ $ned->setAliases([]);
+
// TODO: searching by external email (setting)
$this->markTestIncomplete();
}
@@ -353,6 +368,32 @@
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);
+
+ // cleanup
+ $ned->setAliases([]);
}
/**

File Metadata

Mime Type
text/plain
Expires
Fri, Apr 3, 2:01 AM (21 h, 33 m ago)
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
18822067
Default Alt Text
D1402.1775181682.diff (16 KB)

Event Timeline