diff --git a/src/app/Backends/LDAP.php b/src/app/Backends/LDAP.php --- a/src/app/Backends/LDAP.php +++ b/src/app/Backends/LDAP.php @@ -62,10 +62,8 @@ $config = self::getConfig('admin'); $ldap = self::initLDAP($config); - $hostedRootDN = \config('ldap.hosted.root_dn'); $mgmtRootDN = \config('ldap.admin.root_dn'); - - $domainBaseDN = "ou={$domain->namespace},{$hostedRootDN}"; + $domainBaseDN = self::baseDN($domain->namespace); $aci = [ '(targetattr = "*")' @@ -101,14 +99,12 @@ self::setDomainAttributes($domain, $entry); if (!$ldap->get_entry($dn)) { - $result = $ldap->add_entry($dn, $entry); - - if (!$result) { - self::throwException( - $ldap, - "Failed to create domain {$domain->namespace} in LDAP (" . __LINE__ . ")" - ); - } + self::addEntry( + $ldap, + $dn, + $entry, + "Failed to create domain {$domain->namespace} in LDAP (" . __LINE__ . ")" + ); } // create ou, roles, ous @@ -153,62 +149,56 @@ ); if (!$ldap->get_entry($domainBaseDN)) { - $result = $ldap->add_entry($domainBaseDN, $entry); - - if (!$result) { - self::throwException( - $ldap, - "Failed to create domain {$domain->namespace} in LDAP (" . __LINE__ . ")" - ); - } + self::addEntry( + $ldap, + $domainBaseDN, + $entry, + "Failed to create domain {$domain->namespace} in LDAP (" . __LINE__ . ")" + ); } foreach (['Groups', 'People', 'Resources', 'Shared Folders'] as $item) { - if (!$ldap->get_entry("ou={$item},{$domainBaseDN}")) { - $result = $ldap->add_entry( - "ou={$item},{$domainBaseDN}", - [ - 'ou' => $item, - 'description' => $item, - 'objectclass' => [ - 'top', - 'organizationalunit' - ] + $itemDN = self::baseDN($domain->namespace, $item); + if (!$ldap->get_entry($itemDN)) { + $itemEntry = [ + 'ou' => $item, + 'description' => $item, + 'objectclass' => [ + 'top', + 'organizationalunit' ] - ); + ]; - if (!$result) { - self::throwException( - $ldap, - "Failed to create domain {$domain->namespace} in LDAP (" . __LINE__ . ")" - ); - } + self::addEntry( + $ldap, + $itemDN, + $itemEntry, + "Failed to create domain {$domain->namespace} in LDAP (" . __LINE__ . ")" + ); } } foreach (['kolab-admin'] as $item) { - if (!$ldap->get_entry("cn={$item},{$domainBaseDN}")) { - $result = $ldap->add_entry( - "cn={$item},{$domainBaseDN}", - [ - 'cn' => $item, - 'description' => "{$item} role", - 'objectclass' => [ - 'top', - 'ldapsubentry', - 'nsmanagedroledefinition', - 'nsroledefinition', - 'nssimpleroledefinition' - ] + $itemDN = "cn={$item},{$domainBaseDN}"; + if (!$ldap->get_entry($itemDN)) { + $itemEntry = [ + 'cn' => $item, + 'description' => "{$item} role", + 'objectclass' => [ + 'top', + 'ldapsubentry', + 'nsmanagedroledefinition', + 'nsroledefinition', + 'nssimpleroledefinition' ] - ); + ]; - if (!$result) { - self::throwException( - $ldap, - "Failed to create domain {$domain->namespace} in LDAP (" . __LINE__ . ")" - ); - } + self::addEntry( + $ldap, + $itemDN, + $itemEntry, + "Failed to create domain {$domain->namespace} in LDAP (" . __LINE__ . ")" + ); } } @@ -231,46 +221,27 @@ $config = self::getConfig('admin'); $ldap = self::initLDAP($config); - list($cn, $domainName) = explode('@', $group->email); - - $domain = $group->domain(); - - if (empty($domain)) { - self::throwException( - $ldap, - "Failed to create group {$group->email} in LDAP (" . __LINE__ . ")" - ); - } - - $hostedRootDN = \config('ldap.hosted.root_dn'); - - $domainBaseDN = "ou={$domain->namespace},{$hostedRootDN}"; - - $groupBaseDN = "ou=Groups,{$domainBaseDN}"; - - $dn = "cn={$cn},{$groupBaseDN}"; + $domainName = explode('@', $group->email, 2)[1]; + $cn = $ldap->quote_string($group->name); + $dn = "cn={$cn}," . self::baseDN($domainName, 'Groups'); $entry = [ - 'cn' => $cn, 'mail' => $group->email, 'objectclass' => [ 'top', 'groupofuniquenames', 'kolabgroupofuniquenames' ], - 'uniquemember' => [] ]; self::setGroupAttributes($ldap, $group, $entry); - $result = $ldap->add_entry($dn, $entry); - - if (!$result) { - self::throwException( - $ldap, - "Failed to create group {$group->email} in LDAP (" . __LINE__ . ")" - ); - } + self::addEntry( + $ldap, + $dn, + $entry, + "Failed to create group {$group->email} in LDAP (" . __LINE__ . ")" + ); if (empty(self::$ldap)) { $ldap->close(); @@ -325,14 +296,12 @@ self::setUserAttributes($user, $entry); - $result = $ldap->add_entry($dn, $entry); - - if (!$result) { - self::throwException( - $ldap, - "Failed to create user {$user->email} in LDAP (" . __LINE__ . ")" - ); - } + self::addEntry( + $ldap, + $dn, + $entry, + "Failed to create user {$user->email} in LDAP (" . __LINE__ . ")" + ); } if (empty(self::$ldap)) { @@ -352,10 +321,7 @@ $config = self::getConfig('admin'); $ldap = self::initLDAP($config); - $hostedRootDN = \config('ldap.hosted.root_dn'); - $mgmtRootDN = \config('ldap.admin.root_dn'); - - $domainBaseDN = "ou={$domain->namespace},{$hostedRootDN}"; + $domainBaseDN = self::baseDN($domain->namespace); if ($ldap->get_entry($domainBaseDN)) { $result = $ldap->delete_entry_recursive($domainBaseDN); @@ -568,41 +534,18 @@ $config = self::getConfig('admin'); $ldap = self::initLDAP($config); - list($cn, $domainName) = explode('@', $group->email); - - $domain = $group->domain(); + $newEntry = $oldEntry = self::getGroupEntry($ldap, $group->email, $dn); - if (empty($domain)) { + if (empty($oldEntry)) { self::throwException( $ldap, "Failed to update group {$group->email} in LDAP (group not found)" ); } - $hostedRootDN = \config('ldap.hosted.root_dn'); - - $domainBaseDN = "ou={$domain->namespace},{$hostedRootDN}"; - - $groupBaseDN = "ou=Groups,{$domainBaseDN}"; - - $dn = "cn={$cn},{$groupBaseDN}"; - - $entry = [ - 'cn' => $cn, - 'mail' => $group->email, - 'objectclass' => [ - 'top', - 'groupofuniquenames', - 'kolabgroupofuniquenames' - ], - 'uniquemember' => [] - ]; - - $oldEntry = $ldap->get_entry($dn); - - self::setGroupAttributes($ldap, $group, $entry); + self::setGroupAttributes($ldap, $group, $newEntry); - $result = $ldap->modify_entry($dn, $oldEntry, $entry); + $result = $ldap->modify_entry($dn, $oldEntry, $newEntry); if (!is_array($result)) { self::throwException( @@ -712,15 +655,13 @@ $settings = $group->getSettings(['sender_policy']); $entry['kolaballowsmtpsender'] = json_decode($settings['sender_policy'] ?: '[]', true); + $entry['cn'] = $group->name; + $entry['uniquemember'] = []; + $groupDomain = explode('@', $group->email, 2)[1]; + $domainBaseDN = self::baseDN($groupDomain); $validMembers = []; - $domain = $group->domain(); - - $hostedRootDN = \config('ldap.hosted.root_dn'); - - $domainBaseDN = "ou={$domain->namespace},{$hostedRootDN}"; - foreach ($group->members as $member) { list($local, $domainName) = explode('@', $member); @@ -728,7 +669,7 @@ $memberEntry = $ldap->get_entry($memberDN); // if the member is in the local domain but doesn't exist, drop it - if ($domainName == $domain->namespace && !$memberEntry) { + if ($domainName == $groupDomain && !$memberEntry) { continue; } @@ -755,10 +696,12 @@ // Update members in sql (some might have been removed), // skip model events to not invoke another update job - $group->members = $validMembers; - Group::withoutEvents(function () use ($group) { - $group->save(); - }); + if ($group->members !== $validMembers) { + $group->members = $validMembers; + Group::withoutEvents(function () use ($group) { + $group->save(); + }); + } } /** @@ -868,20 +811,26 @@ */ private static function getGroupEntry($ldap, $email, &$dn = null) { - list($_local, $_domain) = explode('@', $email, 2); + $domainName = explode('@', $email, 2)[1]; + $base_dn = self::baseDN($domainName, 'Groups'); - $domain = $ldap->find_domain($_domain); + $attrs = ['dn', 'cn', 'mail', 'uniquemember', 'objectclass', 'kolaballowsmtpsender']; - if (!$domain) { - return $domain; - } + // For groups we're using search() instead of get_entry() because + // a group name is not constant, so e.g. on update we might have + // the new name, but not the old one. Email address is constant. + $result = $ldap->search($base_dn, "(mail=$email)", "sub", $attrs); - $base_dn = $ldap->domain_root_dn($_domain); - $dn = "cn={$_local},ou=Groups,{$base_dn}"; + if ($result && $result->count() == 1) { + $entries = $result->entries(true); + $dn = key($entries); + $entry = $entries[$dn]; + $entry['dn'] = $dn; - $entry = $ldap->get_entry($dn); + return $entry; + } - return $entry ?: null; + return null; } /** @@ -892,20 +841,13 @@ * @param string $dn Reference to user DN * @param bool $full Get extra attributes, e.g. nsroledn * - * @return false|null|array User entry, False on error, NULL if not found + * @return ?array User entry, NULL if not found */ private static function getUserEntry($ldap, $email, &$dn = null, $full = false) { - list($_local, $_domain) = explode('@', $email, 2); - - $domain = $ldap->find_domain($_domain); + $domainName = explode('@', $email, 2)[1]; - if (!$domain) { - return $domain; - } - - $base_dn = $ldap->domain_root_dn($_domain); - $dn = "uid={$email},ou=People,{$base_dn}"; + $dn = "uid={$email}," . self::baseDN($domainName, 'People'); $entry = $ldap->get_entry($dn); @@ -976,6 +918,39 @@ } /** + * A wrapper for Net_LDAP3::add_entry() with error handler + * + * @param \Net_LDAP3 $ldap Ldap connection + * @param string $dn Entry DN + * @param array $entry Entry attributes + * @param ?string $errorMsg A message to throw as an exception on error + * + * @throws \Exception + */ + private static function addEntry($ldap, string $dn, array $entry, $errorMsg = null) + { + // try/catch because Laravel converts warnings into exceptions + // and we want more human-friendly error message than that + try { + $result = $ldap->add_entry($dn, $entry); + } catch (\Exception $e) { + $result = false; + } + + if (!$result) { + if (!$errorMsg) { + $errorMsg = "LDAP Error (" . __LINE__ . ")"; + } + + if (isset($e)) { + $errorMsg .= ": " . $e->getMessage(); + } + + self::throwException($ldap, $errorMsg); + } + } + + /** * Throw exception and close the connection when needed * * @param \Net_LDAP3 $ldap Ldap connection @@ -991,4 +966,25 @@ throw new \Exception($message); } + + /** + * Create a base DN string for specified object + * + * @param string $domainName Domain namespace + * @param ?string $ouName Optional name of the sub-tree (OU) + * + * @return string Full base DN + */ + private static function baseDN(string $domainName, string $ouName = null): string + { + $hostedRootDN = \config('ldap.hosted.root_dn'); + + $dn = "ou={$domainName},{$hostedRootDN}"; + + if ($ouName) { + $dn = "ou={$ouName},{$dn}"; + } + + return $dn; + } } diff --git a/src/app/Console/Commands/GroupsCommand.php b/src/app/Console/Commands/GroupsCommand.php new file mode 100644 --- /dev/null +++ b/src/app/Console/Commands/GroupsCommand.php @@ -0,0 +1,12 @@ +sortBy('namespace')->values(); + $result = $result->sortBy('name')->values(); } } elseif (!empty($search)) { if ($group = Group::where('email', $search)->first()) { @@ -41,6 +41,7 @@ $data = [ 'id' => $group->id, 'email' => $group->email, + 'name' => $group->name, ]; $data = array_merge($data, self::groupStatuses($group)); diff --git a/src/app/Http/Controllers/API/V4/GroupsController.php b/src/app/Http/Controllers/API/V4/GroupsController.php --- a/src/app/Http/Controllers/API/V4/GroupsController.php +++ b/src/app/Http/Controllers/API/V4/GroupsController.php @@ -5,6 +5,7 @@ use App\Http\Controllers\Controller; use App\Domain; use App\Group; +use App\Rules\GroupName; use App\User; use Carbon\Carbon; use Illuminate\Http\Request; @@ -73,11 +74,12 @@ { $user = $this->guard()->user(); - $result = $user->groups()->orderBy('email')->get() + $result = $user->groups()->orderBy('name')->orderBy('email')->get() ->map(function (Group $group) { $data = [ 'id' => $group->id, 'email' => $group->email, + 'name' => $group->name, ]; $data = array_merge($data, self::groupStatuses($group)); @@ -284,13 +286,26 @@ return $this->errorResponse(403); } - $email = request()->input('email'); - $members = request()->input('members'); + $email = $request->input('email'); + $members = $request->input('members'); $errors = []; + $rules = [ + 'name' => 'required|string|max:191', + ]; // Validate group address if ($error = GroupsController::validateGroupEmail($email, $owner)) { $errors['email'] = $error; + } else { + list(, $domainName) = explode('@', $email); + $rules['name'] = ['required', 'string', new GroupName($owner, $domainName)]; + } + + // Validate the group name + $v = Validator::make($request->all(), $rules); + + if ($v->fails()) { + $errors = array_merge($errors, $v->errors()->toArray()); } // Validate members' email addresses @@ -318,6 +333,7 @@ // Create the group $group = new Group(); + $group->name = $request->input('name'); $group->email = $email; $group->members = $members; $group->save(); @@ -355,11 +371,24 @@ } $owner = $group->wallet()->owner; - - // It is possible to update members property only for now - $members = request()->input('members'); + $name = $request->input('name'); + $members = $request->input('members'); $errors = []; + // Validate the group name + if ($name !== null && $name != $group->name) { + list(, $domainName) = explode('@', $group->email); + $rules = ['name' => ['required', 'string', new GroupName($owner, $domainName)]]; + + $v = Validator::make($request->all(), $rules); + + if ($v->fails()) { + $errors = array_merge($errors, $v->errors()->toArray()); + } else { + $group->name = $name; + } + } + // Validate members' email addresses if (empty($members) || !is_array($members)) { $errors['members'] = \trans('validation.listmembersrequired'); diff --git a/src/app/Http/Controllers/API/V4/Reseller/GroupsController.php b/src/app/Http/Controllers/API/V4/Reseller/GroupsController.php --- a/src/app/Http/Controllers/API/V4/Reseller/GroupsController.php +++ b/src/app/Http/Controllers/API/V4/Reseller/GroupsController.php @@ -27,7 +27,7 @@ }); } - $result = $result->sortBy('namespace')->values(); + $result = $result->sortBy('name')->values(); } } elseif (!empty($search)) { if ($group = Group::withSubjectTenantContext()->where('email', $search)->first()) { @@ -40,6 +40,7 @@ $data = [ 'id' => $group->id, 'email' => $group->email, + 'name' => $group->name, ]; $data = array_merge($data, self::groupStatuses($group)); diff --git a/src/app/Observers/GroupObserver.php b/src/app/Observers/GroupObserver.php --- a/src/app/Observers/GroupObserver.php +++ b/src/app/Observers/GroupObserver.php @@ -17,6 +17,10 @@ public function creating(Group $group): void { $group->status |= Group::STATUS_NEW | Group::STATUS_ACTIVE; + + if (!isset($group->name) && isset($group->email)) { + $group->name = explode('@', $group->email)[0]; + } } /** diff --git a/src/app/Rules/GroupName.php b/src/app/Rules/GroupName.php new file mode 100644 --- /dev/null +++ b/src/app/Rules/GroupName.php @@ -0,0 +1,72 @@ +owner = $owner; + $this->domain = Str::lower($domain); + } + + /** + * Determine if the validation rule passes. + * + * @param string $attribute Attribute name + * @param mixed $name The value to validate + * + * @return bool + */ + public function passes($attribute, $name): bool + { + if (empty($name) || !is_string($name)) { + $this->message = \trans('validation.nameinvalid'); + return false; + } + + // Check the max length, according to the database column length + if (strlen($name) > 191) { + $this->message = \trans('validation.nametoolong'); + return false; + } + + // Check if the name is unique in the domain + // FIXME: Maybe just using the whole groups table would be faster than groups()? + $exists = $this->owner->groups() + ->where('groups.name', $name) + ->where('groups.email', 'like', '%@' . $this->domain) + ->exists(); + + if ($exists) { + $this->message = \trans('validation.nameexists'); + return false; + } + + return true; + } + + /** + * Get the validation error message. + * + * @return string + */ + public function message(): ?string + { + return $this->message; + } +} diff --git a/src/config/ldap.php b/src/config/ldap.php --- a/src/config/ldap.php +++ b/src/config/ldap.php @@ -21,10 +21,7 @@ // probably proxy credentials? ], - 'user_base_dn' => env('LDAP_USER_BASE_DN', null), - 'base_dn' => env('LDAP_BASE_DN', null), 'root_dn' => env('LDAP_ROOT_DN', null), - 'unique_attribute' => env('LDAP_UNIQUE_ATTRIBUTE', 'nsuniqueid'), 'service_bind_dn' => env('LDAP_SERVICE_BIND_DN', null), 'service_bind_pw' => env('LDAP_SERVICE_BIND_PW', null), 'login_filter' => env('LDAP_LOGIN_FILTER', '(&(objectclass=kolabinetorgperson)(uid=%s))'), diff --git a/src/database/migrations/2021_11_10_100000_add_group_name_column.php b/src/database/migrations/2021_11_10_100000_add_group_name_column.php new file mode 100644 --- /dev/null +++ b/src/database/migrations/2021_11_10_100000_add_group_name_column.php @@ -0,0 +1,52 @@ +string('name')->nullable()->after('email'); + } + ); + + // Fill the name with the local part of the email address + DB::table('groups')->update([ + 'name' => DB::raw("SUBSTRING_INDEX(`email`, '@', 1)") + ]); + + Schema::table( + 'groups', + function (Blueprint $table) { + $table->string('name')->nullable(false)->change(); + } + ); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + Schema::table( + 'groups', + function (Blueprint $table) { + $table->dropColumn('name'); + } + ); + } +} diff --git a/src/resources/lang/en/ui.php b/src/resources/lang/en/ui.php --- a/src/resources/lang/en/ui.php +++ b/src/resources/lang/en/ui.php @@ -56,6 +56,7 @@ 'delete' => "Delete list", 'email' => "Email", 'list-empty' => "There are no distribution lists in this account.", + 'name' => "Name", 'new' => "New distribution list", 'recipients' => "Recipients", 'sender-policy' => "Sender Access List", diff --git a/src/resources/lang/en/validation.php b/src/resources/lang/en/validation.php --- a/src/resources/lang/en/validation.php +++ b/src/resources/lang/en/validation.php @@ -141,6 +141,9 @@ 'spf-entry-invalid' => 'The entry format is invalid. Expected a domain name starting with a dot.', 'sp-entry-invalid' => 'The entry format is invalid. Expected an email, domain, or part of it.', 'invalid-config-parameter' => 'The requested configuration parameter is not supported.', + 'nameexists' => 'The specified name is not available.', + 'nameinvalid' => 'The specified name is invalid.', + 'nametoolong' => 'The specified name is too long.', /* |-------------------------------------------------------------------------- diff --git a/src/resources/vue/Admin/Distlist.vue b/src/resources/vue/Admin/Distlist.vue --- a/src/resources/vue/Admin/Distlist.vue +++ b/src/resources/vue/Admin/Distlist.vue @@ -22,6 +22,12 @@
+ +
+ {{ list.name }} +
+
+
diff --git a/src/resources/vue/Admin/User.vue b/src/resources/vue/Admin/User.vue --- a/src/resources/vue/Admin/User.vue +++ b/src/resources/vue/Admin/User.vue @@ -290,6 +290,7 @@ + @@ -297,13 +298,16 @@ + - +
{{ $t('distlist.name') }} {{ $t('form.email') }}
+ {{ list.name }} + {{ list.email }}
{{ $t('user.distlists-none') }}{{ $t('user.distlists-none') }}
diff --git a/src/resources/vue/Distlist/Info.vue b/src/resources/vue/Distlist/Info.vue --- a/src/resources/vue/Distlist/Info.vue +++ b/src/resources/vue/Distlist/Info.vue @@ -34,6 +34,12 @@
+ +
+ +
+
+
@@ -50,10 +56,10 @@
-
+
- + {{ $t('distlist.sender-policy-text') }} @@ -100,6 +106,9 @@ .catch(this.$root.errorHandler) } }, + mounted() { + $('#name').focus() + }, methods: { deleteList() { axios.delete('/api/v4/groups/' + this.list_id) diff --git a/src/resources/vue/Distlist/List.vue b/src/resources/vue/Distlist/List.vue --- a/src/resources/vue/Distlist/List.vue +++ b/src/resources/vue/Distlist/List.vue @@ -12,6 +12,7 @@ + @@ -19,13 +20,16 @@ + - +
{{ $t('distlist.name') }} {{ $t('distlist.email') }}
+ {{ list.name }} + {{ list.email }}
{{ $t('distlist.list-empty') }}{{ $t('distlist.list-empty') }}
diff --git a/src/tests/Browser/Admin/DistlistTest.php b/src/tests/Browser/Admin/DistlistTest.php --- a/src/tests/Browser/Admin/DistlistTest.php +++ b/src/tests/Browser/Admin/DistlistTest.php @@ -59,7 +59,7 @@ $this->browse(function (Browser $browser) { $user = $this->getTestUser('john@kolab.org'); - $group = $this->getTestGroup('group-test@kolab.org'); + $group = $this->getTestGroup('group-test@kolab.org', ['name' => 'Test Group']); $group->assignToWallet($user->wallets->first()); $group->members = ['test1@gmail.com', 'test2@gmail.com']; $group->save(); @@ -80,14 +80,16 @@ ->on($distlist_page) ->assertSeeIn('@distlist-info .card-title', $group->email) ->with('@distlist-info form', function (Browser $browser) use ($group) { - $browser->assertElementsCount('.row', 3) + $browser->assertElementsCount('.row', 4) ->assertSeeIn('.row:nth-child(1) label', 'ID (Created)') ->assertSeeIn('.row:nth-child(1) #distlistid', "{$group->id} ({$group->created_at})") ->assertSeeIn('.row:nth-child(2) label', 'Status') ->assertSeeIn('.row:nth-child(2) #status.text-danger', 'Not Ready') - ->assertSeeIn('.row:nth-child(3) label', 'Recipients') - ->assertSeeIn('.row:nth-child(3) #members', $group->members[0]) - ->assertSeeIn('.row:nth-child(3) #members', $group->members[1]); + ->assertSeeIn('.row:nth-child(3) label', 'Name') + ->assertSeeIn('.row:nth-child(3) #name', $group->name) + ->assertSeeIn('.row:nth-child(4) label', 'Recipients') + ->assertSeeIn('.row:nth-child(4) #members', $group->members[0]) + ->assertSeeIn('.row:nth-child(4) #members', $group->members[1]); }) ->assertElementsCount('ul.nav-tabs', 1) ->assertSeeIn('ul.nav-tabs .nav-link', 'Settings') diff --git a/src/tests/Browser/Admin/UserTest.php b/src/tests/Browser/Admin/UserTest.php --- a/src/tests/Browser/Admin/UserTest.php +++ b/src/tests/Browser/Admin/UserTest.php @@ -195,7 +195,7 @@ $wallet->discount()->associate($discount); $wallet->debit(2010); $wallet->save(); - $group = $this->getTestGroup('group-test@kolab.org'); + $group = $this->getTestGroup('group-test@kolab.org', ['name' => 'Test Group']); $group->assignToWallet($john->wallets->first()); $john->setSetting('greylist_enabled', null); @@ -276,8 +276,9 @@ ->click('@nav #tab-distlists') ->with('@user-distlists table', function (Browser $browser) { $browser->assertElementsCount('tbody tr', 1) - ->assertSeeIn('tbody tr:nth-child(1) td:first-child a', 'group-test@kolab.org') + ->assertSeeIn('tbody tr:nth-child(1) td:first-child a', 'Test Group') ->assertVisible('tbody tr:nth-child(1) td:first-child svg.text-danger') + ->assertSeeIn('tbody tr:nth-child(1) td:last-child a', 'group-test@kolab.org') ->assertMissing('tfoot'); }); diff --git a/src/tests/Browser/DistlistTest.php b/src/tests/Browser/DistlistTest.php --- a/src/tests/Browser/DistlistTest.php +++ b/src/tests/Browser/DistlistTest.php @@ -82,7 +82,7 @@ // Create a single group, add beta+distlist entitlements $john = $this->getTestUser('john@kolab.org'); $this->addDistlistEntitlement($john); - $group = $this->getTestGroup('group-test@kolab.org'); + $group = $this->getTestGroup('group-test@kolab.org', ['name' => 'Test Group']); $group->assignToWallet($john->wallets->first()); // Test distribution lists page @@ -93,9 +93,12 @@ ->on(new DistlistList()) ->whenAvailable('@table', function (Browser $browser) { $browser->waitFor('tbody tr') + ->assertSeeIn('thead tr th:nth-child(1)', 'Name') + ->assertSeeIn('thead tr th:nth-child(2)', 'Email') ->assertElementsCount('tbody tr', 1) - ->assertSeeIn('tbody tr:nth-child(1) a', 'group-test@kolab.org') - ->assertText('tbody tr:nth-child(1) svg.text-danger title', 'Not Ready') + ->assertSeeIn('tbody tr:nth-child(1) td:nth-child(1) a', 'Test Group') + ->assertText('tbody tr:nth-child(1) td:nth-child(1) svg.text-danger title', 'Not Ready') + ->assertSeeIn('tbody tr:nth-child(1) td:nth-child(2) a', 'group-test@kolab.org') ->assertMissing('tfoot'); }); }); @@ -130,10 +133,13 @@ ->with('@general', function (Browser $browser) { // Assert form content $browser->assertMissing('#status') - ->assertSeeIn('div.row:nth-child(1) label', 'Email') + ->assertFocused('#name') + ->assertSeeIn('div.row:nth-child(1) label', 'Name') ->assertValue('div.row:nth-child(1) input[type=text]', '') - ->assertSeeIn('div.row:nth-child(2) label', 'Recipients') - ->assertVisible('div.row:nth-child(2) .list-input') + ->assertSeeIn('div.row:nth-child(2) label', 'Email') + ->assertValue('div.row:nth-child(2) input[type=text]', '') + ->assertSeeIn('div.row:nth-child(3) label', 'Recipients') + ->assertVisible('div.row:nth-child(3) .list-input') ->with(new ListInput('#members'), function (Browser $browser) { $browser->assertListInputValue([]) ->assertValue('@input', ''); @@ -141,15 +147,17 @@ ->assertSeeIn('button[type=submit]', 'Submit'); }) // Test error conditions + ->type('#name', str_repeat('A', 192)) ->type('#email', 'group-test@kolabnow.com') ->click('@general button[type=submit]') - ->waitFor('#email + .invalid-feedback') - ->assertSeeIn('#email + .invalid-feedback', 'The specified domain is not available.') - ->assertFocused('#email') ->waitFor('#members + .invalid-feedback') + ->assertSeeIn('#email + .invalid-feedback', 'The specified domain is not available.') + ->assertSeeIn('#name + .invalid-feedback', 'The name may not be greater than 191 characters.') ->assertSeeIn('#members + .invalid-feedback', 'At least one recipient is required.') + ->assertFocused('#name') ->assertToast(Toast::TYPE_ERROR, 'Form validation error') // Test successful group creation + ->type('#name', 'Test Group') ->type('#email', 'group-test@kolab.org') ->with(new ListInput('#members'), function (Browser $browser) { $browser->addListEntry('test1@gmail.com') @@ -161,17 +169,20 @@ ->assertElementsCount('@table tbody tr', 1); // Test group update - $browser->click('@table tr:nth-child(1) a') + $browser->click('@table tr:nth-child(1) td:first-child a') ->on(new DistlistInfo()) ->assertSeeIn('#distlist-info .card-title', 'Distribution list') ->with('@general', function (Browser $browser) { // Assert form content - $browser->assertSeeIn('div.row:nth-child(1) label', 'Status') + $browser->assertFocused('#name') + ->assertSeeIn('div.row:nth-child(1) label', 'Status') ->assertSeeIn('div.row:nth-child(1) span.text-danger', 'Not Ready') - ->assertSeeIn('div.row:nth-child(2) label', 'Email') - ->assertValue('div.row:nth-child(2) input[type=text]:disabled', 'group-test@kolab.org') - ->assertSeeIn('div.row:nth-child(3) label', 'Recipients') - ->assertVisible('div.row:nth-child(3) .list-input') + ->assertSeeIn('div.row:nth-child(2) label', 'Name') + ->assertValue('div.row:nth-child(2) input[type=text]', 'Test Group') + ->assertSeeIn('div.row:nth-child(3) label', 'Email') + ->assertValue('div.row:nth-child(3) input[type=text]:disabled', 'group-test@kolab.org') + ->assertSeeIn('div.row:nth-child(4) label', 'Recipients') + ->assertVisible('div.row:nth-child(4) .list-input') ->with(new ListInput('#members'), function (Browser $browser) { $browser->assertListInputValue(['test1@gmail.com', 'test2@gmail.com']) ->assertValue('@input', ''); @@ -201,7 +212,7 @@ $this->assertSame(['test1@gmail.com'], $group->members); // Test group deletion - $browser->click('@table tr:nth-child(1) a') + $browser->click('@table tr:nth-child(1) td:first-child a') ->on(new DistlistInfo()) ->assertSeeIn('button.button-delete', 'Delete list') ->click('button.button-delete') diff --git a/src/tests/Browser/Reseller/DistlistTest.php b/src/tests/Browser/Reseller/DistlistTest.php --- a/src/tests/Browser/Reseller/DistlistTest.php +++ b/src/tests/Browser/Reseller/DistlistTest.php @@ -59,7 +59,7 @@ $this->browse(function (Browser $browser) { $user = $this->getTestUser('john@kolab.org'); - $group = $this->getTestGroup('group-test@kolab.org'); + $group = $this->getTestGroup('group-test@kolab.org', ['name' => 'Test Group']); $group->assignToWallet($user->wallets->first()); $group->members = ['test1@gmail.com', 'test2@gmail.com']; $group->save(); @@ -80,14 +80,16 @@ ->on($distlist_page) ->assertSeeIn('@distlist-info .card-title', $group->email) ->with('@distlist-info form', function (Browser $browser) use ($group) { - $browser->assertElementsCount('.row', 3) + $browser->assertElementsCount('.row', 4) ->assertSeeIn('.row:nth-child(1) label', 'ID (Created)') ->assertSeeIn('.row:nth-child(1) #distlistid', "{$group->id} ({$group->created_at})") ->assertSeeIn('.row:nth-child(2) label', 'Status') ->assertSeeIn('.row:nth-child(2) #status.text-danger', 'Not Ready') - ->assertSeeIn('.row:nth-child(3) label', 'Recipients') - ->assertSeeIn('.row:nth-child(3) #members', $group->members[0]) - ->assertSeeIn('.row:nth-child(3) #members', $group->members[1]); + ->assertSeeIn('.row:nth-child(3) label', 'Name') + ->assertSeeIn('.row:nth-child(3) #name', $group->name) + ->assertSeeIn('.row:nth-child(4) label', 'Recipients') + ->assertSeeIn('.row:nth-child(4) #members', $group->members[0]) + ->assertSeeIn('.row:nth-child(4) #members', $group->members[1]); }) ->assertElementsCount('ul.nav-tabs', 1) ->assertSeeIn('ul.nav-tabs .nav-link', 'Settings') diff --git a/src/tests/Browser/Reseller/UserTest.php b/src/tests/Browser/Reseller/UserTest.php --- a/src/tests/Browser/Reseller/UserTest.php +++ b/src/tests/Browser/Reseller/UserTest.php @@ -183,7 +183,7 @@ $wallet->discount()->associate($discount); $wallet->debit(2010); $wallet->save(); - $group = $this->getTestGroup('group-test@kolab.org'); + $group = $this->getTestGroup('group-test@kolab.org', ['name' => 'Test Group']); $group->assignToWallet($john->wallets->first()); // Click the managed-by link on Jack's page @@ -263,8 +263,9 @@ ->click('@nav #tab-distlists') ->with('@user-distlists table', function (Browser $browser) { $browser->assertElementsCount('tbody tr', 1) - ->assertSeeIn('tbody tr:nth-child(1) td:first-child a', 'group-test@kolab.org') + ->assertSeeIn('tbody tr:nth-child(1) td:first-child a', 'Test Group') ->assertVisible('tbody tr:nth-child(1) td:first-child svg.text-danger') + ->assertSeeIn('tbody tr:nth-child(1) td:last-child a', 'group-test@kolab.org') ->assertMissing('tfoot'); }); diff --git a/src/tests/Feature/Backends/LDAPTest.php b/src/tests/Feature/Backends/LDAPTest.php --- a/src/tests/Feature/Backends/LDAPTest.php +++ b/src/tests/Feature/Backends/LDAPTest.php @@ -174,7 +174,9 @@ $this->assertSame(['member3@testldap.com'], $group->fresh()->members); // Update members (add non-existing local member, expect it to be aot-removed from the group) + // Update group name and sender_policy $group->members = ['member3@testldap.com', 'member-local@kolab.org']; + $group->name = 'Te(=ść)1'; $group->save(); $group->setSetting('sender_policy', null); @@ -183,6 +185,8 @@ // TODO: Should we force this to be always an array? $expected['uniquemember'] = 'uid=member3@testldap.com,ou=People,ou=kolab.org,' . $root_dn; $expected['kolaballowsmtpsender'] = null; + $expected['dn'] = 'cn=Te(\\3dść)1,ou=Groups,ou=kolab.org,' . $root_dn; + $expected['cn'] = 'Te(=ść)1'; $ldap_group = LDAP::getGroup($group->email); @@ -315,6 +319,25 @@ } /** + * Test handling errors on a group creation + * + * @group ldap + */ + public function testCreateGroupException(): void + { + $this->expectException(\Exception::class); + $this->expectExceptionMessageMatches('/Failed to create group/'); + + $group = new Group([ + 'name' => 'test', + 'email' => 'test@testldap.com', + 'status' => Group::STATUS_NEW | Group::STATUS_ACTIVE, + ]); + + LDAP::createGroup($group); + } + + /** * Test handling errors on user creation * * @group ldap @@ -362,6 +385,7 @@ $this->expectExceptionMessageMatches('/group not found/'); $group = new Group([ + 'name' => 'test', 'email' => 'test@testldap.com', 'status' => Group::STATUS_NEW | Group::STATUS_ACTIVE, ]); diff --git a/src/tests/Feature/Controller/Admin/GroupsTest.php b/src/tests/Feature/Controller/Admin/GroupsTest.php --- a/src/tests/Feature/Controller/Admin/GroupsTest.php +++ b/src/tests/Feature/Controller/Admin/GroupsTest.php @@ -80,6 +80,7 @@ $this->assertSame(1, $json['count']); $this->assertCount(1, $json['list']); $this->assertSame($group->email, $json['list'][0]['email']); + $this->assertSame($group->name, $json['list'][0]['name']); // Search by owner (Ned is a controller on John's wallets, // here we expect only domains assigned to Ned's wallet(s)) @@ -114,6 +115,7 @@ $this->assertEquals($group->id, $json['id']); $this->assertEquals($group->email, $json['email']); + $this->assertEquals($group->name, $json['name']); $this->assertEquals($group->status, $json['status']); } diff --git a/src/tests/Feature/Controller/GroupsTest.php b/src/tests/Feature/Controller/GroupsTest.php --- a/src/tests/Feature/Controller/GroupsTest.php +++ b/src/tests/Feature/Controller/GroupsTest.php @@ -18,6 +18,7 @@ parent::setUp(); $this->deleteTestGroup('group-test@kolab.org'); + $this->deleteTestGroup('group-test2@kolab.org'); } /** @@ -26,6 +27,7 @@ public function tearDown(): void { $this->deleteTestGroup('group-test@kolab.org'); + $this->deleteTestGroup('group-test2@kolab.org'); parent::tearDown(); } @@ -102,6 +104,7 @@ $this->assertCount(1, $json); $this->assertSame($group->id, $json[0]['id']); $this->assertSame($group->email, $json[0]['email']); + $this->assertSame($group->name, $json[0]['name']); $this->assertArrayHasKey('isDeleted', $json[0]); $this->assertArrayHasKey('isSuspended', $json[0]); $this->assertArrayHasKey('isActive', $json[0]); @@ -227,6 +230,7 @@ $this->assertSame($group->id, $json['id']); $this->assertSame($group->email, $json['email']); + $this->assertSame($group->name, $json['name']); $this->assertSame($group->members, $json['members']); $this->assertTrue(!empty($json['statusInfo'])); $this->assertArrayHasKey('isDeleted', $json); @@ -385,9 +389,12 @@ $this->assertSame('error', $json['status']); $this->assertSame("The email field is required.", $json['errors']['email']); + $this->assertSame("At least one recipient is required.", $json['errors']['members']); + $this->assertSame("The name field is required.", $json['errors']['name'][0]); $this->assertCount(2, $json); + $this->assertCount(3, $json['errors']); - // Test missing members + // Test missing members and name $post = ['email' => 'group-test@kolab.org']; $response = $this->actingAs($john)->post("/api/v4/groups", $post); $json = $response->json(); @@ -396,10 +403,12 @@ $this->assertSame('error', $json['status']); $this->assertSame("At least one recipient is required.", $json['errors']['members']); + $this->assertSame("The name field is required.", $json['errors']['name'][0]); $this->assertCount(2, $json); + $this->assertCount(2, $json['errors']); - // Test invalid email - $post = ['email' => 'invalid']; + // Test invalid email and too long name + $post = ['email' => 'invalid', 'name' => str_repeat('A', 192)]; $response = $this->actingAs($john)->post("/api/v4/groups", $post); $response->assertStatus(422); @@ -407,10 +416,13 @@ $this->assertSame('error', $json['status']); $this->assertCount(2, $json); - $this->assertSame('The specified email is invalid.', $json['errors']['email']); + $this->assertSame("The specified email is invalid.", $json['errors']['email']); + $this->assertSame("The name may not be greater than 191 characters.", $json['errors']['name'][0]); + $this->assertCount(3, $json['errors']); // Test successful group creation $post = [ + 'name' => 'Test Group', 'email' => 'group-test@kolab.org', 'members' => ['test1@domain.tld', 'test2@domain.tld'] ]; @@ -429,6 +441,19 @@ $this->assertSame($post['email'], $group->email); $this->assertSame($post['members'], $group->members); $this->assertTrue($john->groups()->get()->contains($group)); + + // Group name must be unique within a domain + $post['email'] = 'group-test2@kolab.org'; + $post['members'] = ['test1@domain.tld']; + + $response = $this->actingAs($john)->post("/api/v4/groups", $post); + $response->assertStatus(422); + $json = $response->json(); + + $this->assertSame('error', $json['status']); + $this->assertCount(2, $json); + $this->assertCount(1, $json['errors']); + $this->assertSame("The specified name is not available.", $json['errors']['name'][0]); } /** @@ -474,8 +499,9 @@ $this->assertCount(2, $json); $this->assertSame('The specified email address is invalid.', $json['errors']['members'][1]); - // Valid data - members changed + // Valid data - members and name changed $post = [ + 'name' => 'Test Gr', 'members' => ['member1@test.domain', 'member2@test.domain'] ]; @@ -487,7 +513,11 @@ $this->assertSame('success', $json['status']); $this->assertSame("Distribution list updated successfully.", $json['message']); $this->assertCount(2, $json); - $this->assertSame($group->fresh()->members, $post['members']); + + $group->refresh(); + + $this->assertSame($post['name'], $group->name); + $this->assertSame($post['members'], $group->members); } /** diff --git a/src/tests/Feature/Controller/Reseller/GroupsTest.php b/src/tests/Feature/Controller/Reseller/GroupsTest.php --- a/src/tests/Feature/Controller/Reseller/GroupsTest.php +++ b/src/tests/Feature/Controller/Reseller/GroupsTest.php @@ -86,6 +86,7 @@ $this->assertSame(1, $json['count']); $this->assertCount(1, $json['list']); $this->assertSame($group->email, $json['list'][0]['email']); + $this->assertSame($group->name, $json['list'][0]['name']); // Search by owner (Ned is a controller on John's wallets, // here we expect only domains assigned to Ned's wallet(s)) @@ -144,6 +145,7 @@ $this->assertEquals($group->id, $json['id']); $this->assertEquals($group->email, $json['email']); + $this->assertEquals($group->name, $json['name']); $this->assertEquals($group->status, $json['status']); } diff --git a/src/tests/Feature/GroupTest.php b/src/tests/Feature/GroupTest.php --- a/src/tests/Feature/GroupTest.php +++ b/src/tests/Feature/GroupTest.php @@ -65,65 +65,6 @@ $this->assertSame('["test","-"]', $group->getSetting('sender_policy')); $this->assertSame([], $result); } - - /** - * Test group status assignment and is*() methods - */ - public function testStatus(): void - { - $group = new Group(); - - $this->assertSame(false, $group->isNew()); - $this->assertSame(false, $group->isActive()); - $this->assertSame(false, $group->isDeleted()); - $this->assertSame(false, $group->isLdapReady()); - $this->assertSame(false, $group->isSuspended()); - - $group->status = Group::STATUS_NEW; - - $this->assertSame(true, $group->isNew()); - $this->assertSame(false, $group->isActive()); - $this->assertSame(false, $group->isDeleted()); - $this->assertSame(false, $group->isLdapReady()); - $this->assertSame(false, $group->isSuspended()); - - $group->status |= Group::STATUS_ACTIVE; - - $this->assertSame(true, $group->isNew()); - $this->assertSame(true, $group->isActive()); - $this->assertSame(false, $group->isDeleted()); - $this->assertSame(false, $group->isLdapReady()); - $this->assertSame(false, $group->isSuspended()); - - $group->status |= Group::STATUS_LDAP_READY; - - $this->assertSame(true, $group->isNew()); - $this->assertSame(true, $group->isActive()); - $this->assertSame(false, $group->isDeleted()); - $this->assertSame(true, $group->isLdapReady()); - $this->assertSame(false, $group->isSuspended()); - - $group->status |= Group::STATUS_DELETED; - - $this->assertSame(true, $group->isNew()); - $this->assertSame(true, $group->isActive()); - $this->assertSame(true, $group->isDeleted()); - $this->assertSame(true, $group->isLdapReady()); - $this->assertSame(false, $group->isSuspended()); - - $group->status |= Group::STATUS_SUSPENDED; - - $this->assertSame(true, $group->isNew()); - $this->assertSame(true, $group->isActive()); - $this->assertSame(true, $group->isDeleted()); - $this->assertSame(true, $group->isLdapReady()); - $this->assertSame(true, $group->isSuspended()); - - // Unknown status value - $this->expectException(\Exception::class); - $group->status = 111; - } - /** * Test creating a group */ @@ -134,6 +75,7 @@ $group = Group::create(['email' => 'GROUP-test@kolabnow.com']); $this->assertSame('group-test@kolabnow.com', $group->email); + $this->assertSame('group-test', $group->name); $this->assertMatchesRegularExpression('/^[0-9]{1,20}$/', $group->id); $this->assertSame([], $group->members); $this->assertTrue($group->isNew()); @@ -322,6 +264,64 @@ } /** + * Test group status assignment and is*() methods + */ + public function testStatus(): void + { + $group = new Group(); + + $this->assertSame(false, $group->isNew()); + $this->assertSame(false, $group->isActive()); + $this->assertSame(false, $group->isDeleted()); + $this->assertSame(false, $group->isLdapReady()); + $this->assertSame(false, $group->isSuspended()); + + $group->status = Group::STATUS_NEW; + + $this->assertSame(true, $group->isNew()); + $this->assertSame(false, $group->isActive()); + $this->assertSame(false, $group->isDeleted()); + $this->assertSame(false, $group->isLdapReady()); + $this->assertSame(false, $group->isSuspended()); + + $group->status |= Group::STATUS_ACTIVE; + + $this->assertSame(true, $group->isNew()); + $this->assertSame(true, $group->isActive()); + $this->assertSame(false, $group->isDeleted()); + $this->assertSame(false, $group->isLdapReady()); + $this->assertSame(false, $group->isSuspended()); + + $group->status |= Group::STATUS_LDAP_READY; + + $this->assertSame(true, $group->isNew()); + $this->assertSame(true, $group->isActive()); + $this->assertSame(false, $group->isDeleted()); + $this->assertSame(true, $group->isLdapReady()); + $this->assertSame(false, $group->isSuspended()); + + $group->status |= Group::STATUS_DELETED; + + $this->assertSame(true, $group->isNew()); + $this->assertSame(true, $group->isActive()); + $this->assertSame(true, $group->isDeleted()); + $this->assertSame(true, $group->isLdapReady()); + $this->assertSame(false, $group->isSuspended()); + + $group->status |= Group::STATUS_SUSPENDED; + + $this->assertSame(true, $group->isNew()); + $this->assertSame(true, $group->isActive()); + $this->assertSame(true, $group->isDeleted()); + $this->assertSame(true, $group->isLdapReady()); + $this->assertSame(true, $group->isSuspended()); + + // Unknown status value + $this->expectException(\Exception::class); + $group->status = 111; + } + + /** * Tests for Group::suspend() */ public function testSuspend(): void