nextcloud · pringelmann · Aug 17, 2026
@@ -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) {

@@ -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<string> $gids Every group of the scenario, mocked and returned by ID
+	 * @param list<string> $subAdminGids Groups the caller is sub-admin of
+	 * @param list<string> $memberGids Groups the account is currently a member of
+	 * @return array{0: IUser&MockObject, 1: ISubAdmin&MockObject, 2: array<string, IGroup&MockObject>}
+	 */
+	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');