Skip to content

Commit 0531074

Browse files
pringelmannpringelmann
authored
Merge pull request #63534 from nextcloud/backport/63307/stable34
[stable34] fix(provisioning_api): let sub-admins change group memberships
2 parents 5d10e90 + 096b222 commit 0531074

2 files changed

Lines changed: 162 additions & 2 deletions

File tree

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

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -966,6 +966,18 @@ public function editUserMultiField(
966966
throw new OCSForbiddenException('Insufficient permissions to edit this user');
967967
}
968968

969+
// Sub-admins are limited to the groups they administer, so their group changes are
970+
// checked against that list instead of the blanket admin one. Both lists are read
971+
// once here and reused by the validation and the apply phase below.
972+
$canChangeAllGroups = $isAdmin || $isDelegatedAdmin;
973+
$currentGroupIds = $groups === null ? [] : $this->groupManager->getUserGroupIds($targetUser);
974+
$subAdminGids = $groups === null || $canChangeAllGroups
975+
? []
976+
: array_map(
977+
fn (IGroup $group): string => $group->getGID(),
978+
$subAdminManager->getSubAdminsGroups($currentLoggedInUser),
979+
);
980+
969981
// Validate all submitted fields — collect errors before applying anything
970982
$errors = [];
971983

@@ -1014,14 +1026,29 @@ public function editUserMultiField(
10141026
}
10151027

10161028
if ($groups !== null) {
1017-
if (!$isAdmin && !$isDelegatedAdmin) {
1029+
if (!$canChangeAllGroups && !$isSubAdminAccessible) {
10181030
$errors['groups'] = $this->l10n->t('Insufficient permissions to change groups');
10191031
} else {
1032+
// Only the added groups are checked against the caller's sub-admin groups:
1033+
// the request repeats the memberships it did not touch, and those may well
1034+
// be in groups the caller does not administer.
1035+
$addedGids = $canChangeAllGroups ? [] : array_diff($groups, $currentGroupIds);
1036+
10201037
foreach ($groups as $gid) {
10211038
if (!$this->groupManager->groupExists($gid)) {
10221039
$errors['groups'] = $this->l10n->t('Group %s does not exist', [$gid]);
10231040
break;
10241041
}
1042+
if (in_array($gid, $addedGids, true) && !in_array($gid, $subAdminGids, true)) {
1043+
$errors['groups'] = $this->l10n->t('Insufficient privileges for group %1$s', [$gid]);
1044+
break;
1045+
}
1046+
}
1047+
1048+
// The account has to stay in at least one group the caller administers,
1049+
// otherwise the sub-admin loses access to it (same rule as removeFromGroup).
1050+
if (!$canChangeAllGroups && !isset($errors['groups']) && array_intersect($groups, $subAdminGids) === []) {
1051+
$errors['groups'] = $this->l10n->t('Not viable to remove user from the last group you are sub-admin of');
10251052
}
10261053
}
10271054
}
@@ -1082,8 +1109,12 @@ public function editUserMultiField(
10821109
}
10831110

10841111
if ($groups !== null) {
1085-
$currentGroupIds = $this->groupManager->getUserGroupIds($targetUser);
10861112
foreach (array_diff($currentGroupIds, $groups) as $gid) {
1113+
// A sub-admin only gets to see part of the group list, so a group missing
1114+
// from their request is not an intent to remove it.
1115+
if (!$canChangeAllGroups && !in_array($gid, $subAdminGids, true)) {
1116+
continue;
1117+
}
10871118
$this->groupManager->get($gid)?->removeUser($targetUser);
10881119
}
10891120
foreach (array_diff($groups, $currentGroupIds) as $gid) {

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

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2927,6 +2927,135 @@ public function testUpdateUserDelegatedAdminCannotAddToAdminGroup(): void {
29272927
$this->assertSame(Http::STATUS_OK, $result->getStatus());
29282928
}
29292929

2930+
/**
2931+
* Wire up a sub-admin ('subadmin') editing an accessible account ('targetuser').
2932+
*
2933+
* @param list<string> $gids Every group of the scenario, mocked and returned by ID
2934+
* @param list<string> $subAdminGids Groups the caller is sub-admin of
2935+
* @param list<string> $memberGids Groups the account is currently a member of
2936+
* @return array{0: IUser&MockObject, 1: ISubAdmin&MockObject, 2: array<string, IGroup&MockObject>}
2937+
*/
2938+
private function mockSubAdminEditing(array $gids, array $subAdminGids, array $memberGids): array {
2939+
$currentUser = $this->createMock(IUser::class);
2940+
$currentUser->method('getUID')->willReturn('subadmin');
2941+
$this->userSession->method('getUser')->willReturn($currentUser);
2942+
2943+
$targetUser = $this->createMock(IUser::class);
2944+
$targetUser->method('getUID')->willReturn('targetuser');
2945+
$targetUser->method('getBackend')->willReturn($this->createMock(UserInterface::class));
2946+
$this->userManager->method('get')->with('targetuser')->willReturn($targetUser);
2947+
2948+
$this->groupManager->method('isAdmin')->willReturn(false);
2949+
$this->groupManager->method('isDelegatedAdmin')->willReturn(false);
2950+
$this->groupManager->method('groupExists')->willReturn(true);
2951+
$this->groupManager->method('getUserGroupIds')->willReturn($memberGids);
2952+
2953+
$groups = [];
2954+
foreach ($gids as $gid) {
2955+
$group = $this->createMock(IGroup::class);
2956+
$group->method('getGID')->willReturn($gid);
2957+
$groups[$gid] = $group;
2958+
}
2959+
$this->groupManager->method('get')
2960+
->willReturnMap(array_map(fn (string $gid): array => [$gid, $groups[$gid]], $gids));
2961+
2962+
$subAdmin = $this->createMock(ISubAdmin::class);
2963+
$subAdmin->method('isUserAccessible')->with($currentUser, $targetUser)->willReturn(true);
2964+
$subAdmin->method('getSubAdminsGroups')
2965+
->willReturn(array_map(fn (string $gid): IGroup => $groups[$gid], $subAdminGids));
2966+
$this->groupManager->method('getSubAdmin')->willReturn($subAdmin);
2967+
2968+
return [$targetUser, $subAdmin, $groups];
2969+
}
2970+
2971+
public function testUpdateUserSubAdminCanAddToOwnGroup(): void {
2972+
[$targetUser, , $groups] = $this->mockSubAdminEditing(
2973+
gids: ['staff', 'marketing'],
2974+
subAdminGids: ['staff'],
2975+
memberGids: ['marketing'],
2976+
);
2977+
2978+
$groups['staff']->expects($this->once())->method('addUser')->with($targetUser);
2979+
// The membership the sub-admin cannot administer is repeated, not changed
2980+
$groups['marketing']->expects($this->never())->method('addUser');
2981+
$groups['marketing']->expects($this->never())->method('removeUser');
2982+
2983+
$result = $this->api->editUserMultiField('targetuser', groups: ['marketing', 'staff']);
2984+
$this->assertSame(Http::STATUS_OK, $result->getStatus());
2985+
}
2986+
2987+
public function testUpdateUserSubAdminCannotAddToForeignGroup(): void {
2988+
[, , $groups] = $this->mockSubAdminEditing(
2989+
gids: ['staff', 'secret'],
2990+
subAdminGids: ['staff'],
2991+
memberGids: ['staff'],
2992+
);
2993+
2994+
$groups['secret']->expects($this->never())->method('addUser');
2995+
2996+
$result = $this->api->editUserMultiField('targetuser', groups: ['staff', 'secret']);
2997+
$this->assertSame(Http::STATUS_UNPROCESSABLE_ENTITY, $result->getStatus());
2998+
$this->assertSame('Insufficient privileges for group secret', $result->getData()['errors']['groups']);
2999+
}
3000+
3001+
public function testUpdateUserSubAdminCanRemoveFromOwnGroup(): void {
3002+
[$targetUser, , $groups] = $this->mockSubAdminEditing(
3003+
gids: ['staff', 'sales'],
3004+
subAdminGids: ['staff', 'sales'],
3005+
memberGids: ['staff', 'sales'],
3006+
);
3007+
3008+
$groups['staff']->expects($this->once())->method('removeUser')->with($targetUser);
3009+
$groups['sales']->expects($this->never())->method('removeUser');
3010+
3011+
$result = $this->api->editUserMultiField('targetuser', groups: ['sales']);
3012+
$this->assertSame(Http::STATUS_OK, $result->getStatus());
3013+
}
3014+
3015+
public function testUpdateUserSubAdminCannotRemoveLastGroupTheyAdminister(): void {
3016+
[, , $groups] = $this->mockSubAdminEditing(
3017+
gids: ['staff'],
3018+
subAdminGids: ['staff'],
3019+
memberGids: ['staff'],
3020+
);
3021+
3022+
$groups['staff']->expects($this->never())->method('removeUser');
3023+
3024+
$result = $this->api->editUserMultiField('targetuser', groups: []);
3025+
$this->assertSame(Http::STATUS_UNPROCESSABLE_ENTITY, $result->getStatus());
3026+
$this->assertArrayHasKey('groups', $result->getData()['errors']);
3027+
}
3028+
3029+
public function testUpdateUserSubAdminKeepsGroupsOutsideTheirScope(): void {
3030+
[, , $groups] = $this->mockSubAdminEditing(
3031+
gids: ['staff', 'marketing'],
3032+
subAdminGids: ['staff'],
3033+
memberGids: ['staff', 'marketing'],
3034+
);
3035+
3036+
// The sub-admin UI only offers the groups they administer, so 'marketing' is
3037+
// absent from the request without the sub-admin ever asking to remove it
3038+
$groups['marketing']->expects($this->never())->method('removeUser');
3039+
$groups['staff']->expects($this->never())->method('removeUser');
3040+
3041+
$result = $this->api->editUserMultiField('targetuser', groups: ['staff']);
3042+
$this->assertSame(Http::STATUS_OK, $result->getStatus());
3043+
}
3044+
3045+
public function testUpdateUserSubAdminCannotChangeSubAdminGroups(): void {
3046+
[, $subAdmin, ] = $this->mockSubAdminEditing(
3047+
gids: ['staff'],
3048+
subAdminGids: ['staff'],
3049+
memberGids: ['staff'],
3050+
);
3051+
3052+
$subAdmin->expects($this->never())->method('createSubAdmin');
3053+
3054+
$result = $this->api->editUserMultiField('targetuser', subadminGroups: ['staff']);
3055+
$this->assertSame(Http::STATUS_UNPROCESSABLE_ENTITY, $result->getStatus());
3056+
$this->assertArrayHasKey('subadminGroups', $result->getData()['errors']);
3057+
}
3058+
29303059
public function testUpdateUserCannotCreateSubAdminOfAdminGroup(): void {
29313060
$currentUser = $this->createMock(IUser::class);
29323061
$currentUser->method('getUID')->willReturn('admin');

0 commit comments

Comments
 (0)