[Web] Groups: limit changes of domain admins to their domains

data/web/inc/lib/mailcow/MailboxGroup/Model/Group.php

@@ -42,6 +42,8 @@ class Group
     public array $sendAs = [];
     /** @var array[] {owner, rights, subscribe, state, last_error}: members may open these mailboxes (ACL group=mcg_<id>) */
     public array $imapGrants = [];
+    /** @var string[] owners of removed IMAP grants whose ACL Dovecot still has, not part of the grants to write */
+    public array $removingImapOwners = [];
     /** @var array[] {owner, type, folder, access, subscribe, state, last_error}: SOGo folders shared with the members */
     public array $sogoGrants = [];
     /** @var array[] warnings of the last write (msg values), not stored */

data/web/inc/lib/mailcow/MailboxGroup/Repository/GroupRepository.php

@@ -501,6 +501,8 @@ class GroupRepository extends Repository
         $group->imapGrants = array_map(fn(array $grant): array => array_merge($grant, ['subscribe' => (int)$grant['subscribe']]),
             $this->query("SELECT `owner`, `rights`, `subscribe`, `state`, `last_error` FROM `mailbox_group_imap_grant`
               WHERE `group_id` = :id AND `state` != 'removing' ORDER BY `owner`", $id)->fetchAll(PDO::FETCH_ASSOC));
+        $group->removingImapOwners = $this->query("SELECT `owner` FROM `mailbox_group_imap_grant` WHERE `group_id` = :id AND `state` = 'removing'",
+            $id)->fetchAll(PDO::FETCH_COLUMN);
         $group->sogoGrants = array_map(fn(array $grant): array => array_merge($grant, ['subscribe' => (int)$grant['subscribe']]),
             $this->query("SELECT `owner`, `folder_type` AS `type`, `folder`, `access`, `subscribe`, `state`, `last_error`
               FROM `mailbox_group_sogo_grant` WHERE `group_id` = :id ORDER BY `owner`, `folder_type`, `folder`", $id)->fetchAll(PDO::FETCH_ASSOC));

data/web/inc/lib/mailcow/MailboxGroup/Service/GroupService.php

@@ -79,6 +79,12 @@ class GroupService
                 return false;
             }
         }
+        // A removed grant still opens the mailbox until Dovecot dropped its ACL
+        foreach ($group->removingImapOwners as $owner) {
+            if (!$this->context->canAccessMailbox((string)$owner)) {
+                return false;
+            }
+        }
         return true;
     }
 
@@ -113,7 +119,7 @@ class GroupService
         $current = $this->editable($id);
         $group = clone $current;
         $this->apply($group, $input, $current);
-        $snapshot = $this->effects->snapshot(array_merge($current->usernames(), $group->usernames()));
+        $snapshot = $this->effects->snapshot($this->inScope(array_merge($current->usernames(), $group->usernames())));
         $sogo = $this->repo->update($group);
         $group->warnings = array_merge($this->sizeWarnings($group), $this->effects->apply($sogo, $snapshot),
             $this->sharing->applyImap((int)$group->id), $this->sharing->applySogo((int)$group->id));
@@ -132,7 +138,7 @@ class GroupService
     {
         $this->requireEditor();
         $group = $this->editable($id);
-        $snapshot = $this->effects->snapshot($group->usernames());
+        $snapshot = $this->effects->snapshot($this->inScope($group->usernames()));
         $this->sharing->removeImap($group);
         $sogo = $this->repo->delete($id);
         $group->warnings = $this->effects->apply($sogo, $snapshot);
@@ -150,13 +156,30 @@ class GroupService
         if (!$own && !($this->canRead() && $this->context->canAccessMailbox($username))) {
             return [];
         }
+        $groups = $this->lookup->membershipsOf($username);
+        // Domain admins see the mailbox's groups in their domains, not those of other domains
+        if (!$own && !$this->context->hasAdminRights()) {
+            $groups = array_values(array_filter($groups, fn(array $group): bool => $this->context->canAccessDomain((string)$group['domain'])));
+        }
         return [
-            'groups' => $this->lookup->membershipsOf($username),
+            'groups' => $groups,
             'template' => $this->templates->effective([$username])[$username] ?? null,
             'send_as' => $this->lookup->sendAsOf($username),
         ];
     }
 
+    /**
+     * Members whose templates a change of the caller may apply: a domain admin never changes mailboxes outside
+     * his domains, their new effective template applies with the next admin action or IdP login.
+     */
+    private function inScope(array $usernames): array
+    {
+        if ($this->context->hasAdminRights()) {
+            return $usernames;
+        }
+        return array_values(array_filter($usernames, fn(string $username): bool => $this->context->canAccessMailbox($username)));
+    }
+
     /** @throws AccessDenied when the caller cannot see or edit the group */
     private function editable(int $id): Group
     {

tests/php/MailboxGroup/Service/GroupServiceTest.php

@@ -301,6 +301,31 @@ class GroupServiceTest extends TestCase
         $this->assertTrue($this->service(self::admin())->canEdit($group));
     }
 
+    public function testRemovedGrantsCountUntilDovecotDroppedThem(): void
+    {
+        $group = self::stored();
+        $group->removingImapOwners = ['c@other.org'];
+
+        $this->assertFalse($this->service(self::domainAdmin())->canEdit($group));
+        $this->assertTrue($this->service(self::admin())->canEdit($group));
+    }
+
+    public function testDomainAdminChangesOnlyApplyTemplatesInHisDomains(): void
+    {
+        $this->repo->method('findVisible')->willReturn(self::stored([], ['a@example.org' => 'manual', 'c@other.org' => 'manual']));
+        $this->repo->method('update')->willReturn([]);
+        $this->repo->method('delete')->willReturn([]);
+        $effects = $this->createMock(ChangeEffects::class);
+        $effects->method('apply')->willReturn([]);
+        // before and after lists of update() may repeat a member, only the set counts
+        $effects->expects($this->exactly(2))->method('snapshot')
+            ->with($this->callback(fn(array $usernames): bool => array_values(array_unique($usernames)) === ['a@example.org']))->willReturn([]);
+        $service = new GroupService($this->repo, $this->lookup, $this->templates, $effects, $this->sharing, self::domainAdmin());
+
+        $service->update(7, ['active' => 0]);
+        $service->delete(7);
+    }
+
     public function testDeleteRemovesTheImapAclsFirst(): void
     {
         $group = self::stored();
@@ -346,13 +371,16 @@ class GroupServiceTest extends TestCase
 
     public function testMembershipsAreVisibleToTheMailboxAndItsManagers(): void
     {
-        $this->lookup->method('membershipsOf')->willReturn([['id' => 7, 'name' => 'Sales']]);
+        $sales = ['id' => 7, 'domain' => 'example.org', 'name' => 'Sales'];
+        $foreign = ['id' => 8, 'domain' => 'other.org', 'name' => 'Other'];
+        $this->lookup->method('membershipsOf')->willReturn([$sales, $foreign]);
         $this->lookup->method('sendAsOf')->willReturn(['sales@example.org']);
         $this->templates->method('effective')->willReturn(['a@example.org' => 'Staff']);
-        $expected = ['groups' => [['id' => 7, 'name' => 'Sales']], 'template' => 'Staff', 'send_as' => ['sales@example.org']];
+        $expected = ['groups' => [$sales, $foreign], 'template' => 'Staff', 'send_as' => ['sales@example.org']];
 
         $this->assertSame($expected, $this->service(FakeContext::user('a@example.org', []))->membershipsOf('A@example.org'));
-        $this->assertSame($expected, $this->service(self::domainAdmin())->membershipsOf('a@example.org'));
+        // Groups of other domains stay hidden from a domain admin
+        $this->assertSame(['groups' => [$sales]] + $expected, $this->service(self::domainAdmin())->membershipsOf('a@example.org'));
         $this->assertSame([], $this->service(FakeContext::user('b@example.org', []))->membershipsOf('a@example.org'));
         $this->assertSame([], $this->service(self::domainAdmin())->membershipsOf('c@other.org'));
     }