diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index f46084a98741e..86858dfe8f5a0 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -959,6 +959,18 @@ public function editUserMultiField( throw new OCSForbiddenException('Insufficient permissions to edit this user'); } + // Sub-admins are limited to the groups they administer, so their group changes are + // checked against that list instead of the blanket admin one. Both lists are read + // once here and reused by the validation and the apply phase below. + $canChangeAllGroups = $isAdmin || $isDelegatedAdmin; + $currentGroupIds = $groups === null ? [] : $this->groupManager->getUserGroupIds($targetUser); + $subAdminGids = $groups === null || $canChangeAllGroups + ? [] + : array_map( + fn (IGroup $group): string => $group->getGID(), + $subAdminManager->getSubAdminsGroups($currentLoggedInUser), + ); + // Validate all submitted fields — collect errors before applying anything $errors = []; @@ -1007,14 +1019,29 @@ public function editUserMultiField( } if ($groups !== null) { - if (!$isAdmin && !$isDelegatedAdmin) { + if (!$canChangeAllGroups && !$isSubAdminAccessible) { $errors['groups'] = $this->l10n->t('Insufficient permissions to change groups'); } else { + // Only the added groups are checked against the caller's sub-admin groups: + // the request repeats the memberships it did not touch, and those may well + // be in groups the caller does not administer. + $addedGids = $canChangeAllGroups ? [] : array_diff($groups, $currentGroupIds); + foreach ($groups as $gid) { if (!$this->groupManager->groupExists($gid)) { $errors['groups'] = $this->l10n->t('Group %s does not exist', [$gid]); break; } + if (in_array($gid, $addedGids, true) && !in_array($gid, $subAdminGids, true)) { + $errors['groups'] = $this->l10n->t('Insufficient privileges for group %1$s', [$gid]); + break; + } + } + + // The account has to stay in at least one group the caller administers, + // otherwise the sub-admin loses access to it (same rule as removeFromGroup). + if (!$canChangeAllGroups && !isset($errors['groups']) && array_intersect($groups, $subAdminGids) === []) { + $errors['groups'] = $this->l10n->t('Not viable to remove user from the last group you are sub-admin of'); } } } @@ -1075,8 +1102,12 @@ public function editUserMultiField( } if ($groups !== null) { - $currentGroupIds = $this->groupManager->getUserGroupIds($targetUser); foreach (array_diff($currentGroupIds, $groups) as $gid) { + // A sub-admin only gets to see part of the group list, so a group missing + // from their request is not an intent to remove it. + if (!$canChangeAllGroups && !in_array($gid, $subAdminGids, true)) { + continue; + } $this->groupManager->get($gid)?->removeUser($targetUser); } foreach (array_diff($groups, $currentGroupIds) as $gid) { diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index 08355a5cb1804..4ad35b35b79e6 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -2908,6 +2908,135 @@ public function testUpdateUserDelegatedAdminCannotAddToAdminGroup(): void { $this->assertSame(Http::STATUS_OK, $result->getStatus()); } + /** + * Wire up a sub-admin ('subadmin') editing an accessible account ('targetuser'). + * + * @param list $gids Every group of the scenario, mocked and returned by ID + * @param list $subAdminGids Groups the caller is sub-admin of + * @param list $memberGids Groups the account is currently a member of + * @return array{0: IUser&MockObject, 1: ISubAdmin&MockObject, 2: array} + */ + private function mockSubAdminEditing(array $gids, array $subAdminGids, array $memberGids): array { + $currentUser = $this->createMock(IUser::class); + $currentUser->method('getUID')->willReturn('subadmin'); + $this->userSession->method('getUser')->willReturn($currentUser); + + $targetUser = $this->createMock(IUser::class); + $targetUser->method('getUID')->willReturn('targetuser'); + $targetUser->method('getBackend')->willReturn($this->createMock(UserInterface::class)); + $this->userManager->method('get')->with('targetuser')->willReturn($targetUser); + + $this->groupManager->method('isAdmin')->willReturn(false); + $this->groupManager->method('isDelegatedAdmin')->willReturn(false); + $this->groupManager->method('groupExists')->willReturn(true); + $this->groupManager->method('getUserGroupIds')->willReturn($memberGids); + + $groups = []; + foreach ($gids as $gid) { + $group = $this->createMock(IGroup::class); + $group->method('getGID')->willReturn($gid); + $groups[$gid] = $group; + } + $this->groupManager->method('get') + ->willReturnMap(array_map(fn (string $gid): array => [$gid, $groups[$gid]], $gids)); + + $subAdmin = $this->createMock(ISubAdmin::class); + $subAdmin->method('isUserAccessible')->with($currentUser, $targetUser)->willReturn(true); + $subAdmin->method('getSubAdminsGroups') + ->willReturn(array_map(fn (string $gid): IGroup => $groups[$gid], $subAdminGids)); + $this->groupManager->method('getSubAdmin')->willReturn($subAdmin); + + return [$targetUser, $subAdmin, $groups]; + } + + public function testUpdateUserSubAdminCanAddToOwnGroup(): void { + [$targetUser, , $groups] = $this->mockSubAdminEditing( + gids: ['staff', 'marketing'], + subAdminGids: ['staff'], + memberGids: ['marketing'], + ); + + $groups['staff']->expects($this->once())->method('addUser')->with($targetUser); + // The membership the sub-admin cannot administer is repeated, not changed + $groups['marketing']->expects($this->never())->method('addUser'); + $groups['marketing']->expects($this->never())->method('removeUser'); + + $result = $this->api->editUserMultiField('targetuser', groups: ['marketing', 'staff']); + $this->assertSame(Http::STATUS_OK, $result->getStatus()); + } + + public function testUpdateUserSubAdminCannotAddToForeignGroup(): void { + [, , $groups] = $this->mockSubAdminEditing( + gids: ['staff', 'secret'], + subAdminGids: ['staff'], + memberGids: ['staff'], + ); + + $groups['secret']->expects($this->never())->method('addUser'); + + $result = $this->api->editUserMultiField('targetuser', groups: ['staff', 'secret']); + $this->assertSame(Http::STATUS_UNPROCESSABLE_ENTITY, $result->getStatus()); + $this->assertSame('Insufficient privileges for group secret', $result->getData()['errors']['groups']); + } + + public function testUpdateUserSubAdminCanRemoveFromOwnGroup(): void { + [$targetUser, , $groups] = $this->mockSubAdminEditing( + gids: ['staff', 'sales'], + subAdminGids: ['staff', 'sales'], + memberGids: ['staff', 'sales'], + ); + + $groups['staff']->expects($this->once())->method('removeUser')->with($targetUser); + $groups['sales']->expects($this->never())->method('removeUser'); + + $result = $this->api->editUserMultiField('targetuser', groups: ['sales']); + $this->assertSame(Http::STATUS_OK, $result->getStatus()); + } + + public function testUpdateUserSubAdminCannotRemoveLastGroupTheyAdminister(): void { + [, , $groups] = $this->mockSubAdminEditing( + gids: ['staff'], + subAdminGids: ['staff'], + memberGids: ['staff'], + ); + + $groups['staff']->expects($this->never())->method('removeUser'); + + $result = $this->api->editUserMultiField('targetuser', groups: []); + $this->assertSame(Http::STATUS_UNPROCESSABLE_ENTITY, $result->getStatus()); + $this->assertArrayHasKey('groups', $result->getData()['errors']); + } + + public function testUpdateUserSubAdminKeepsGroupsOutsideTheirScope(): void { + [, , $groups] = $this->mockSubAdminEditing( + gids: ['staff', 'marketing'], + subAdminGids: ['staff'], + memberGids: ['staff', 'marketing'], + ); + + // The sub-admin UI only offers the groups they administer, so 'marketing' is + // absent from the request without the sub-admin ever asking to remove it + $groups['marketing']->expects($this->never())->method('removeUser'); + $groups['staff']->expects($this->never())->method('removeUser'); + + $result = $this->api->editUserMultiField('targetuser', groups: ['staff']); + $this->assertSame(Http::STATUS_OK, $result->getStatus()); + } + + public function testUpdateUserSubAdminCannotChangeSubAdminGroups(): void { + [, $subAdmin, ] = $this->mockSubAdminEditing( + gids: ['staff'], + subAdminGids: ['staff'], + memberGids: ['staff'], + ); + + $subAdmin->expects($this->never())->method('createSubAdmin'); + + $result = $this->api->editUserMultiField('targetuser', subadminGroups: ['staff']); + $this->assertSame(Http::STATUS_UNPROCESSABLE_ENTITY, $result->getStatus()); + $this->assertArrayHasKey('subadminGroups', $result->getData()['errors']); + } + public function testUpdateUserCannotCreateSubAdminOfAdminGroup(): void { $currentUser = $this->createMock(IUser::class); $currentUser->method('getUID')->willReturn('admin');