From 46ce62660eeb322a056144bba33eec7c34d4d6ae Mon Sep 17 00:00:00 2001 From: Peter Ringelmann Date: Wed, 26 Aug 2026 13:02:59 +0200 Subject: [PATCH] fix(provisioning_api): cap group listings and throttle group creation Signed-off-by: Peter Ringelmann --- .../lib/Controller/GroupsController.php | 23 +++++++-- apps/provisioning_api/openapi-full.json | 12 +++-- apps/provisioning_api/openapi.json | 12 +++-- .../tests/Controller/GroupsControllerTest.php | 50 +++++++++++++------ .../lib/Controller/UsersController.php | 13 ++--- lib/private/Group/MetaData.php | 7 ++- openapi.json | 12 +++-- tests/lib/Group/MetaDataTest.php | 4 +- 8 files changed, 89 insertions(+), 44 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/GroupsController.php b/apps/provisioning_api/lib/Controller/GroupsController.php index 1c1be04c88ded..ca676a6b735f5 100644 --- a/apps/provisioning_api/lib/Controller/GroupsController.php +++ b/apps/provisioning_api/lib/Controller/GroupsController.php @@ -19,6 +19,7 @@ use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoSubAdminRequired; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; +use OCP\AppFramework\Http\Attribute\UserRateLimit; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCS\OCSException; use OCP\AppFramework\OCS\OCSForbiddenException; @@ -42,6 +43,8 @@ * @psalm-import-type Provisioning_APIUserDetailsGroupDisplayname from ResponseDefinitions */ class GroupsController extends AUserDataOCSController { + public const int MAX_SEARCH_RESULTS = 500; + public function __construct( string $appName, @@ -75,7 +78,7 @@ public function __construct( * Get a list of groups * * @param string $search Text to search for - * @param ?int $limit Limit the amount of groups returned + * @param ?int<1, 500> $limit Limit the amount of groups returned, defaults to 500 * @param int $offset Offset for searching for groups * @return DataResponse}, array{}> * @@ -83,7 +86,7 @@ public function __construct( */ #[NoAdminRequired] public function getGroups(string $search = '', ?int $limit = null, int $offset = 0): DataResponse { - $groups = $this->groupManager->search($search, $limit, $offset); + $groups = $this->searchGroups($search, $limit, $offset); $groups = array_map(function ($group) { /** @var IGroup $group */ return $group->getGID(); @@ -96,7 +99,7 @@ public function getGroups(string $search = '', ?int $limit = null, int $offset = * Get a list of groups details * * @param string $search Text to search for - * @param ?int $limit Limit the amount of groups returned + * @param ?int<1, 500> $limit Limit the amount of groups returned, defaults to 500 * @param int $offset Offset for searching for groups * @return DataResponse}, array{}> * @@ -106,7 +109,7 @@ public function getGroups(string $search = '', ?int $limit = null, int $offset = #[AuthorizedAdminSetting(settings: Sharing::class)] #[AuthorizedAdminSetting(settings: Users::class)] public function getGroupsDetails(string $search = '', ?int $limit = null, int $offset = 0): DataResponse { - $groups = $this->groupManager->search($search, $limit, $offset); + $groups = $this->searchGroups($search, $limit, $offset); $groups = array_map(function ($group) { /** @var IGroup $group */ return [ @@ -122,6 +125,17 @@ public function getGroupsDetails(string $search = '', ?int $limit = null, int $o return new DataResponse(['groups' => $groups]); } + /** + * @return list + */ + private function searchGroups(string $search, ?int $limit, int $offset): array { + if ($limit === null || $limit <= 0 || $limit > self::MAX_SEARCH_RESULTS) { + $limit = self::MAX_SEARCH_RESULTS; + } + + return $this->groupManager->search($search, $limit, $offset); + } + /** * Get a list of users in the specified group * @@ -254,6 +268,7 @@ public function getGroupUsersDetails(string $groupId, string $search = '', ?int * 200: Group created successfully */ #[AuthorizedAdminSetting(settings:Users::class)] + #[UserRateLimit(limit: 50, period: 600)] #[PasswordConfirmationRequired] public function addGroup(string $groupid, string $displayname = ''): DataResponse { // Validate name diff --git a/apps/provisioning_api/openapi-full.json b/apps/provisioning_api/openapi-full.json index ebba71a0259dc..7b8dcf819f64b 100644 --- a/apps/provisioning_api/openapi-full.json +++ b/apps/provisioning_api/openapi-full.json @@ -1245,12 +1245,14 @@ { "name": "limit", "in": "query", - "description": "Limit the amount of groups returned", + "description": "Limit the amount of groups returned, defaults to 500", "schema": { "type": "integer", "format": "int64", "nullable": true, - "default": null + "default": null, + "minimum": 1, + "maximum": 500 } }, { @@ -3186,12 +3188,14 @@ { "name": "limit", "in": "query", - "description": "Limit the amount of groups returned", + "description": "Limit the amount of groups returned, defaults to 500", "schema": { "type": "integer", "format": "int64", "nullable": true, - "default": null + "default": null, + "minimum": 1, + "maximum": 500 } }, { diff --git a/apps/provisioning_api/openapi.json b/apps/provisioning_api/openapi.json index 4550e551a02e6..2faaec0f3c19a 100644 --- a/apps/provisioning_api/openapi.json +++ b/apps/provisioning_api/openapi.json @@ -456,12 +456,14 @@ { "name": "limit", "in": "query", - "description": "Limit the amount of groups returned", + "description": "Limit the amount of groups returned, defaults to 500", "schema": { "type": "integer", "format": "int64", "nullable": true, - "default": null + "default": null, + "minimum": 1, + "maximum": 500 } }, { @@ -586,12 +588,14 @@ { "name": "limit", "in": "query", - "description": "Limit the amount of groups returned", + "description": "Limit the amount of groups returned, defaults to 500", "schema": { "type": "integer", "format": "int64", "nullable": true, - "default": null + "default": null, + "minimum": 1, + "maximum": 500 } }, { diff --git a/apps/provisioning_api/tests/Controller/GroupsControllerTest.php b/apps/provisioning_api/tests/Controller/GroupsControllerTest.php index 72fc57421e59c..8b6438b6cfa30 100644 --- a/apps/provisioning_api/tests/Controller/GroupsControllerTest.php +++ b/apps/provisioning_api/tests/Controller/GroupsControllerTest.php @@ -157,16 +157,19 @@ private function asSubAdminOfGroup($group) { public static function dataGetGroups(): array { return [ - [null, 0, 0], - ['foo', 0, 0], - [null, 1, 0], - [null, 0, 2], - ['foo', 1, 2], + // search, requested limit, offset, expected forwarded limit + [null, null, 0, GroupsController::MAX_SEARCH_RESULTS], + ['foo', null, 0, GroupsController::MAX_SEARCH_RESULTS], + [null, 0, 0, GroupsController::MAX_SEARCH_RESULTS], + [null, 1, 0, 1], + [null, 10, 2, 10], + ['foo', 1, 2, 1], + [null, GroupsController::MAX_SEARCH_RESULTS + 1, 0, GroupsController::MAX_SEARCH_RESULTS], ]; } #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataGetGroups')] - public function testGetGroups(?string $search, int $limit, int $offset): void { + public function testGetGroups(?string $search, ?int $limit, int $offset, int $expectedLimit): void { $groups = [$this->createGroup('group1'), $this->createGroup('group2')]; $search = $search === null ? '' : $search; @@ -174,21 +177,15 @@ public function testGetGroups(?string $search, int $limit, int $offset): void { $this->groupManager ->expects($this->once()) ->method('search') - ->with($search, $limit, $offset) + ->with($search, $expectedLimit, $offset) ->willReturn($groups); $result = $this->api->getGroups($search, $limit, $offset); $this->assertEquals(['groups' => ['group1', 'group2']], $result->getData()); } - /** - * - * @param string|null $search - * @param int|null $limit - * @param int|null $offset - */ #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataGetGroups')] - public function testGetGroupsDetails($search, $limit, $offset): void { + public function testGetGroupsDetails(?string $search, ?int $limit, int $offset, int $expectedLimit): void { $groups = [$this->createGroup('group1'), $this->createGroup('group2')]; $search = $search === null ? '' : $search; @@ -196,7 +193,7 @@ public function testGetGroupsDetails($search, $limit, $offset): void { $this->groupManager ->expects($this->once()) ->method('search') - ->with($search, $limit, $offset) + ->with($search, $expectedLimit, $offset) ->willReturn($groups); $result = $this->api->getGroupsDetails($search, $limit, $offset); @@ -220,6 +217,29 @@ public function testGetGroupsDetails($search, $limit, $offset): void { ]], $result->getData()); } + public static function dataCapsBackendOverflow(): array { + return [['getGroups'], ['getGroupsDetails']]; + } + + /** + * Each backend is searched with the full limit, so the merge can exceed it. + * Truncating here would drop the last backend's groups entirely, so the + * overflow is returned as-is. + */ + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataCapsBackendOverflow')] + public function testBackendOverflowIsNotTruncated(string $method): void { + $groups = array_fill(0, GroupsController::MAX_SEARCH_RESULTS + 5, $this->createGroup('group')); + + $this->groupManager + ->expects($this->once()) + ->method('search') + ->with('', GroupsController::MAX_SEARCH_RESULTS, 0) + ->willReturn($groups); + + $result = $this->api->$method('', null, 0); + $this->assertCount(GroupsController::MAX_SEARCH_RESULTS + 5, $result->getData()['groups']); + } + public function testGetGroupAsSubadmin(): void { $group = $this->createGroup('group'); $this->asSubAdminOfGroup($group); diff --git a/apps/settings/lib/Controller/UsersController.php b/apps/settings/lib/Controller/UsersController.php index 88656772b825c..7e0db09519920 100644 --- a/apps/settings/lib/Controller/UsersController.php +++ b/apps/settings/lib/Controller/UsersController.php @@ -141,14 +141,6 @@ public function usersList(INavigationManager $navigationManager, ISubAdmin $subA $canChangePassword = $this->canAdminChangeUserPasswords(); /* GROUPS */ - $groupsInfo = new MetaData( - $uid, - $isAdmin, - $isDelegatedAdmin, - $this->groupManager, - $this->userSession - ); - $adminGroup = $this->groupManager->get('admin'); $adminGroupData = [ 'id' => $adminGroup->getGID(), @@ -167,6 +159,7 @@ public function usersList(INavigationManager $navigationManager, ISubAdmin $subA $disabledUsers = -1; $userCount = 0; + $ownSubAdminGroups = ($isAdmin || $isDelegatedAdmin) ? [] : $subAdmin->getSubAdminsGroups($user); if (!$isLDAPUsed) { if ($isAdmin || $isDelegatedAdmin) { @@ -176,7 +169,7 @@ public function usersList(INavigationManager $navigationManager, ISubAdmin $subA }, 0); } else { // User is subadmin ! - [$userCount,$disabledUsers] = $this->userManager->countUsersAndDisabledUsersOfGroups($groupsInfo->getGroups(), self::COUNT_LIMIT_FOR_SUBADMINS); + [$userCount,$disabledUsers] = $this->userManager->countUsersAndDisabledUsersOfGroups($ownSubAdminGroups, self::COUNT_LIMIT_FOR_SUBADMINS); } if ($disabledUsers > 0) { @@ -199,7 +192,7 @@ public function usersList(INavigationManager $navigationManager, ISubAdmin $subA if (!$isAdmin && !$isDelegatedAdmin) { $subAdminGroups = array_map( fn (IGroup $group) => ['id' => $group->getGID(), 'name' => $group->getDisplayName()], - $subAdmin->getSubAdminsGroups($user), + $ownSubAdminGroups, ); $subAdminGroups = array_values($subAdminGroups); } diff --git a/lib/private/Group/MetaData.php b/lib/private/Group/MetaData.php index 3f91f3c388790..bd08c66a20833 100644 --- a/lib/private/Group/MetaData.php +++ b/lib/private/Group/MetaData.php @@ -18,6 +18,8 @@ class MetaData { public const SORT_USERCOUNT = 1; // May have performance issues on LDAP backends public const SORT_GROUPNAME = 2; + public const DEFAULT_GROUP_LIMIT = 500; + /** @var array */ protected $metaData = []; /** @var int */ @@ -148,12 +150,13 @@ private function sort(array &$entries, array $sortKeys): void { } /** - * returns the available groups + * returns the available groups, capped at {@see self::DEFAULT_GROUP_LIMIT} + * * @return IGroup[] */ public function getGroups(string $search = ''): array { if ($this->isAdmin || $this->isDelegatedAdmin) { - return $this->groupManager->search($search); + return $this->groupManager->search($search, self::DEFAULT_GROUP_LIMIT); } else { $userObject = $this->userSession->getUser(); if ($userObject !== null && $this->groupManager instanceof GroupManager) { diff --git a/openapi.json b/openapi.json index e8b59f417a805..fb730df1df5ed 100644 --- a/openapi.json +++ b/openapi.json @@ -31801,12 +31801,14 @@ { "name": "limit", "in": "query", - "description": "Limit the amount of groups returned", + "description": "Limit the amount of groups returned, defaults to 500", "schema": { "type": "integer", "format": "int64", "nullable": true, - "default": null + "default": null, + "minimum": 1, + "maximum": 500 } }, { @@ -33742,12 +33744,14 @@ { "name": "limit", "in": "query", - "description": "Limit the amount of groups returned", + "description": "Limit the amount of groups returned, defaults to 500", "schema": { "type": "integer", "format": "int64", "nullable": true, - "default": null + "default": null, + "minimum": 1, + "maximum": 500 } }, { diff --git a/tests/lib/Group/MetaDataTest.php b/tests/lib/Group/MetaDataTest.php index 8151ff92e186c..ff276f2d3e19a 100644 --- a/tests/lib/Group/MetaDataTest.php +++ b/tests/lib/Group/MetaDataTest.php @@ -112,10 +112,12 @@ public function testGetGroupsAsAdmin(): void { $this->groupManager ->expects($this->once()) ->method('search') - ->with('Foo') + ->with('Foo', MetaData::DEFAULT_GROUP_LIMIT) ->willReturn(['DummyValue']); $expected = ['DummyValue']; $this->assertSame($expected, $this->invokePrivate($this->groupMetadata, 'getGroups', ['Foo'])); } + + }