Skip to content

Commit c6da64e

Browse files
KiaraGrouwstraKiaraGrouwstra
committed
feat(groups): honor effective membership in access checks
Migrate Share20, AppManager, SystemTagManager, AuthorizedGroupMapper, ShareDisableChecker, and MandatoryTwoFactor to resolve group membership via getUserEffectiveGroupIds so nested-group edges flow through to share ACLs, app restrictions, tag visibility, settings delegation, and 2FA enforcement. MandatoryTwoFactor deliberately keeps the *excluded* groups list on direct membership: expanding it transitively would let an admin silently exempt an arbitrary population from 2FA by nesting groups under an excluded one, a one-way security weakening. Enforced groups are expanded (strictly more secure). Refs #36150. Signed-off-by: Kiara Grouwstra <cinereal@riseup.net>
1 parent a0a0f3c commit c6da64e

19 files changed

Lines changed: 425 additions & 108 deletions

File tree

‎apps/files_sharing/lib/Controller/ShareAPIController.php‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1532,11 +1532,14 @@ protected function canAccessShare(IShare $share, bool $checkGroups = true): bool
15321532
return true;
15331533
}
15341534

1535-
// If in the recipient group, you can see the share
1535+
// If in the recipient group (directly or via a nested group), you can see the share
15361536
if ($checkGroups && $share->getShareType() === IShare::TYPE_GROUP) {
1537-
$sharedWith = $this->groupManager->get($share->getSharedWith());
15381537
$user = $this->userManager->get($this->userId);
1539-
if ($user !== null && $sharedWith !== null && $sharedWith->inGroup($user)) {
1538+
if ($user !== null && in_array(
1539+
$share->getSharedWith(),
1540+
$this->groupManager->getUserEffectiveGroupIds($user),
1541+
true,
1542+
)) {
15401543
return true;
15411544
}
15421545
}
@@ -1662,11 +1665,14 @@ protected function canDeleteShareFromSelf(IShare $share): bool {
16621665
return false;
16631666
}
16641667

1665-
// If in the recipient group, you can delete the share from self
1668+
// If in the recipient group (directly or via a nested group), you can delete the share from self
16661669
if ($share->getShareType() === IShare::TYPE_GROUP) {
1667-
$sharedWith = $this->groupManager->get($share->getSharedWith());
16681670
$user = $this->userManager->get($this->userId);
1669-
if ($user !== null && $sharedWith !== null && $sharedWith->inGroup($user)) {
1671+
if ($user !== null && in_array(
1672+
$share->getSharedWith(),
1673+
$this->groupManager->getUserEffectiveGroupIds($user),
1674+
true,
1675+
)) {
16701676
return true;
16711677
}
16721678
}

‎apps/files_sharing/lib/External/Manager.php‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,13 @@ private function canAccessShare(ExternalShare $share, IUser $user): bool {
158158
$groupShare = $share;
159159
}
160160

161-
if ($this->groupManager->get($groupShare->getUser())->inGroup($user)) {
161+
// Honor nested-group membership: a user in a sub-group of the
162+
// target group is also an effective recipient.
163+
if (in_array(
164+
$groupShare->getUser(),
165+
$this->groupManager->getUserEffectiveGroupIds($user),
166+
true,
167+
)) {
162168
return true;
163169
}
164170
}

‎apps/files_sharing/lib/Listener/UserShareAcceptanceListener.php‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,23 @@ public function handle(Event $event): void {
4343
return;
4444
}
4545

46-
$users = $group->getUsers();
47-
foreach ($users as $user) {
48-
$this->handleAutoAccept($share, $user->getUID());
46+
// Walk descendants so effective members reached via nested-group
47+
// edges also get the share auto-accepted, matching what
48+
// `IManager::getSharedWith` returns for them.
49+
$seen = [];
50+
foreach ($this->groupManager->getGroupEffectiveDescendantIds($group) as $gid) {
51+
$descendant = $this->groupManager->get($gid);
52+
if ($descendant === null) {
53+
continue;
54+
}
55+
foreach ($descendant->getUsers() as $user) {
56+
$uid = $user->getUID();
57+
if (isset($seen[$uid])) {
58+
continue;
59+
}
60+
$seen[$uid] = true;
61+
$this->handleAutoAccept($share, $uid);
62+
}
4963
}
5064
}
5165
}

‎apps/files_sharing/lib/Notification/Notifier.php‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,8 +164,13 @@ protected function parseShareInvitation(IShare $share, INotification $notificati
164164
throw new AlreadyProcessedException();
165165
}
166166

167-
$group = $this->groupManager->get($share->getSharedWith());
168-
if ($group === null || !$group->inGroup($user)) {
167+
// Honor nested-group membership so a user in a sub-group of the
168+
// recipient group still receives the pending-share notification.
169+
if (!in_array(
170+
$share->getSharedWith(),
171+
$this->groupManager->getUserEffectiveGroupIds($user),
172+
true,
173+
)) {
169174
throw new AlreadyProcessedException();
170175
}
171176

‎apps/files_sharing/tests/Controller/ShareAPIControllerTest.php‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -438,9 +438,9 @@ public function testDeleteSharedWithMyGroup(): void {
438438
->method('get')
439439
->with($this->currentUser)
440440
->willReturn($user);
441-
$group->method('inGroup')
441+
$this->groupManager->method('getUserEffectiveGroupIds')
442442
->with($user)
443-
->willReturn(true);
443+
->willReturn(['group']);
444444

445445
$node->expects($this->once())
446446
->method('lock')
@@ -1780,16 +1780,17 @@ public function testCanAccessShareAsGroupMember(string $group, bool $expected):
17801780
->with($this->currentUser)
17811781
->willReturn($user);
17821782

1783-
$group = $this->createMock(IGroup::class);
1784-
$group->method('inGroup')->with($user)->willReturn(true);
1785-
$group2 = $this->createMock(IGroup::class);
1786-
$group2->method('inGroup')->with($user)->willReturn(false);
1787-
1783+
$groupMock = $this->createMock(IGroup::class);
1784+
$group2Mock = $this->createMock(IGroup::class);
17881785
$this->groupManager->method('get')->willReturnMap([
1789-
['group', $group],
1790-
['group2', $group2],
1786+
['group', $groupMock],
1787+
['group2', $group2Mock],
17911788
['group-null', null],
17921789
]);
1790+
// Only "group" contains the current user (directly or via nesting).
1791+
$this->groupManager->method('getUserEffectiveGroupIds')
1792+
->with($user)
1793+
->willReturn(['group']);
17931794

17941795
if ($expected) {
17951796
$this->assertTrue($this->invokePrivate($this->ocs, 'canAccessShare', [$share]));

‎apps/provisioning_api/lib/Controller/GroupsController.php‎

Lines changed: 38 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -165,13 +165,20 @@ public function getGroupUsers(string $groupId): DataResponse {
165165
$isAdmin = $this->groupManager->isAdmin($user->getUID());
166166
$isDelegatedAdmin = $this->groupManager->isDelegatedAdmin($user->getUID());
167167
if ($isAdmin || $isDelegatedAdmin || $isSubadminOfGroup || $isMember) {
168-
$users = $this->groupManager->get($groupId)->getUsers();
169-
$users = array_map(function ($user) {
170-
/** @var IUser $user */
171-
return $user->getUID();
172-
}, $users);
168+
// Honor nested-group edges: return the union of direct members
169+
// and every descendant group's direct members.
170+
$users = [];
171+
foreach ($this->groupManager->getGroupEffectiveDescendantIds($group) as $gid) {
172+
$descendant = $this->groupManager->get($gid);
173+
if ($descendant === null) {
174+
continue;
175+
}
176+
foreach ($descendant->getUsers() as $member) {
177+
$users[$member->getUID()] = true;
178+
}
179+
}
173180
/** @var list<string> $users */
174-
$users = array_values($users);
181+
$users = array_values(array_keys($users));
175182
return new DataResponse(['users' => $users]);
176183
}
177184

@@ -208,7 +215,31 @@ public function getGroupUsersDetails(string $groupId, string $search = '', ?int
208215
$isAdmin = $this->groupManager->isAdmin($currentUser->getUID());
209216
$isDelegatedAdmin = $this->groupManager->isDelegatedAdmin($currentUser->getUID());
210217
if ($isAdmin || $isDelegatedAdmin || $isSubadminOfGroup) {
211-
$users = $group->searchUsers($search, $limit, $offset);
218+
// Honor nested-group edges: the effective user set of a parent
219+
// group is the union of its own direct members and the members
220+
// of every descendant group.
221+
$users = [];
222+
$seen = [];
223+
foreach ($this->groupManager->getGroupEffectiveDescendantIds($group) as $gid) {
224+
$descendant = $this->groupManager->get($gid);
225+
if ($descendant === null) {
226+
continue;
227+
}
228+
foreach ($descendant->searchUsers($search) as $user) {
229+
$uid = $user->getUID();
230+
if (isset($seen[$uid])) {
231+
continue;
232+
}
233+
$seen[$uid] = true;
234+
$users[] = $user;
235+
}
236+
}
237+
if ($offset > 0) {
238+
$users = array_slice($users, $offset);
239+
}
240+
if ($limit !== null && $limit >= 0) {
241+
$users = array_slice($users, 0, $limit);
242+
}
212243

213244
// Extract required number
214245
$usersDetails = [];

‎apps/provisioning_api/tests/Controller/GroupsControllerTest.php‎

Lines changed: 90 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,10 @@ public function testGetGroupAsSubadmin(): void {
228228
->method('groupExists')
229229
->with('group')
230230
->willReturn(true);
231+
$this->groupManager
232+
->method('getGroupEffectiveDescendantIds')
233+
->with($group)
234+
->willReturn(['group']);
231235
$group
232236
->method('getUsers')
233237
->willReturn([
@@ -273,6 +277,10 @@ public function testGetGroupAsAdmin(): void {
273277
->method('groupExists')
274278
->with('group')
275279
->willReturn(true);
280+
$this->groupManager
281+
->method('getGroupEffectiveDescendantIds')
282+
->with($group)
283+
->willReturn(['group']);
276284
$group
277285
->method('getUsers')
278286
->willReturn([
@@ -478,13 +486,17 @@ public function testGetGroupUsersDetails(): void {
478486
$group = $this->createGroup($gid);
479487
$group->expects($this->once())
480488
->method('searchUsers')
481-
->with('', null, 0)
489+
->with('')
482490
->willReturn(array_values($users));
483491

484492
$this->groupManager
485493
->method('get')
486494
->with($gid)
487495
->willReturn($group);
496+
$this->groupManager
497+
->method('getGroupEffectiveDescendantIds')
498+
->with($group)
499+
->willReturn([$gid]);
488500
$this->groupManager->expects($this->any())
489501
->method('getUserGroups')
490502
->willReturn([$group]);
@@ -523,13 +535,17 @@ public function testGetGroupUsersDetailsEncoded(): void {
523535
$group = $this->createGroup($gid);
524536
$group->expects($this->once())
525537
->method('searchUsers')
526-
->with('', null, 0)
538+
->with('')
527539
->willReturn(array_values($users));
528540

529541
$this->groupManager
530542
->method('get')
531543
->with($gid)
532544
->willReturn($group);
545+
$this->groupManager
546+
->method('getGroupEffectiveDescendantIds')
547+
->with($group)
548+
->willReturn([$gid]);
533549
$this->groupManager->expects($this->any())
534550
->method('getUserGroups')
535551
->willReturn([$group]);
@@ -545,4 +561,76 @@ public function testGetGroupUsersDetailsEncoded(): void {
545561

546562
$this->api->getGroupUsersDetails(urlencode($gid));
547563
}
564+
565+
public function testGetGroupUsersUnionsNestedDescendants(): void {
566+
$parentGid = 'engineering';
567+
$childGid = 'backend';
568+
569+
$this->asAdmin();
570+
571+
$alice = $this->createUser('alice');
572+
$this->userManager->method('get')
573+
->willReturnCallback(fn (string $uid) => $uid === 'alice' ? $alice : null);
574+
575+
$parent = $this->createGroup($parentGid);
576+
$parent->method('getUsers')->willReturn([]);
577+
$child = $this->createGroup($childGid);
578+
$child->method('getUsers')->willReturn([$alice]);
579+
580+
$this->groupManager
581+
->method('get')
582+
->willReturnMap([
583+
[$parentGid, $parent],
584+
[$childGid, $child],
585+
]);
586+
$this->groupManager
587+
->method('getGroupEffectiveDescendantIds')
588+
->with($parent)
589+
->willReturn([$parentGid, $childGid]);
590+
591+
$result = $this->api->getGroupUsers($parentGid);
592+
593+
self::assertSame(['users' => ['alice']], $result->getData());
594+
}
595+
596+
public function testGetGroupUsersDetailsUnionsNestedDescendants(): void {
597+
$parentGid = 'engineering';
598+
$childGid = 'backend';
599+
600+
$this->asAdmin();
601+
602+
$alice = $this->createUser('alice');
603+
$this->userManager->method('get')
604+
->willReturnCallback(fn (string $uid) => $uid === 'alice' ? $alice : null);
605+
606+
$parent = $this->createGroup($parentGid);
607+
$parent->method('searchUsers')->with('')->willReturn([]);
608+
$child = $this->createGroup($childGid);
609+
$child->method('searchUsers')->with('')->willReturn([$alice]);
610+
611+
$this->groupManager
612+
->method('get')
613+
->willReturnMap([
614+
[$parentGid, $parent],
615+
[$childGid, $child],
616+
]);
617+
$this->groupManager
618+
->method('getGroupEffectiveDescendantIds')
619+
->with($parent)
620+
->willReturn([$parentGid, $childGid]);
621+
$this->groupManager->expects($this->any())
622+
->method('getUserGroups')
623+
->willReturn([$parent]);
624+
625+
$this->subAdminManager->expects($this->any())
626+
->method('isSubAdminOfGroup')
627+
->willReturn(false);
628+
$this->subAdminManager->expects($this->any())
629+
->method('getSubAdminsGroups')
630+
->willReturn([]);
631+
632+
$result = $this->api->getGroupUsersDetails($parentGid);
633+
634+
self::assertSame(['alice'], array_keys($result->getData()['users']));
635+
}
548636
}

‎lib/private/App/AppManager.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -386,7 +386,9 @@ private function checkAppForUser(string $enabled, ?IUser $user): bool {
386386
return false;
387387
}
388388

389-
$userGroups = $this->groupManager->getUserGroupIds($user);
389+
// Apps restricted to groups should also be enabled for members of
390+
// sub-groups reached via nested-group edges.
391+
$userGroups = $this->groupManager->getUserEffectiveGroupIds($user);
390392
foreach ($userGroups as $groupId) {
391393
if (in_array($groupId, $groupIds, true)) {
392394
return true;

‎lib/private/Authentication/TwoFactorAuth/MandatoryTwoFactor.php‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -55,29 +55,36 @@ public function isEnforcedFor(IUser $user): bool {
5555
if (!$state->isEnforced()) {
5656
return false;
5757
}
58-
$uid = $user->getUID();
58+
// Enforced groups are expanded along nested-group edges: if a group
59+
// is marked enforced and the user is in a sub-group of it, 2FA is
60+
// applied. This is strictly more secure (no user can hide behind
61+
// nesting to skip 2FA).
62+
//
63+
// Excluded groups are NOT expanded. Expanding them would let an
64+
// admin accidentally exempt a large population from 2FA by nesting
65+
// a group under an exempt one — a silent weakening of a security
66+
// boundary via hierarchy changes. Admins who want sub-groups
67+
// exempted must mark each exempt group explicitly.
68+
$effectiveGroups = $this->groupManager->getUserEffectiveGroupIds($user);
69+
$directGroups = $this->groupManager->getUserGroupIds($user);
5970

6071
/*
6172
* If there is a list of enforced groups, we only enforce 2FA for members of those groups.
6273
* For all the other users it is not enforced (overruling the excluded groups list).
6374
*/
6475
if (!empty($state->getEnforcedGroups())) {
65-
foreach ($state->getEnforcedGroups() as $group) {
66-
if ($this->groupManager->isInGroup($uid, $group)) {
67-
return true;
68-
}
76+
if (array_intersect($state->getEnforcedGroups(), $effectiveGroups) !== []) {
77+
return true;
6978
}
7079
// Not a member of any of these groups -> no 2FA enforced
7180
return false;
7281
}
7382

7483
/**
75-
* If the user is member of an excluded group, 2FA won't be enforced.
84+
* If the user is directly a member of an excluded group, 2FA won't be enforced.
7685
*/
77-
foreach ($state->getExcludedGroups() as $group) {
78-
if ($this->groupManager->isInGroup($uid, $group)) {
79-
return false;
80-
}
86+
if (array_intersect($state->getExcludedGroups(), $directGroups) !== []) {
87+
return false;
8188
}
8289

8390
/**

0 commit comments

Comments
 (0)