From 7c73199bdefb204aaf0c49c6ed40379633b539b8 Mon Sep 17 00:00:00 2001 From: FreddleSpl0it <75116288+FreddleSpl0it@users.noreply.github.com> Date: Mon, 2 Nov 2026 10:01:43 +0100 Subject: [PATCH] [Web] Groups: limit changes of domain admins to their domains --- .../lib/mailcow/MailboxGroup/Model/Group.php | 2 ++ .../Repository/GroupRepository.php | 2 ++ .../MailboxGroup/Service/GroupService.php | 29 ++++++++++++++-- .../MailboxGroup/Service/GroupServiceTest.php | 34 +++++++++++++++++-- 4 files changed, 61 insertions(+), 6 deletions(-) diff --git a/data/web/inc/lib/mailcow/MailboxGroup/Model/Group.php b/data/web/inc/lib/mailcow/MailboxGroup/Model/Group.php index 90a5b54e7..bfb5cd9ef 100644 --- a/data/web/inc/lib/mailcow/MailboxGroup/Model/Group.php +++ b/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_) */ 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 */ diff --git a/data/web/inc/lib/mailcow/MailboxGroup/Repository/GroupRepository.php b/data/web/inc/lib/mailcow/MailboxGroup/Repository/GroupRepository.php index f414ea2cd..59b3518a1 100644 --- a/data/web/inc/lib/mailcow/MailboxGroup/Repository/GroupRepository.php +++ b/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)); diff --git a/data/web/inc/lib/mailcow/MailboxGroup/Service/GroupService.php b/data/web/inc/lib/mailcow/MailboxGroup/Service/GroupService.php index 3780a1b40..9fdf0aa9f 100644 --- a/data/web/inc/lib/mailcow/MailboxGroup/Service/GroupService.php +++ b/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 { diff --git a/tests/php/MailboxGroup/Service/GroupServiceTest.php b/tests/php/MailboxGroup/Service/GroupServiceTest.php index 731e656cc..a9816989e 100644 --- a/tests/php/MailboxGroup/Service/GroupServiceTest.php +++ b/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')); }