Skip to content

Commit 46ce626

Browse files
pringelmannpringelmann
committed
fix(provisioning_api): cap group listings and throttle group creation
Signed-off-by: Peter Ringelmann <peter.ringelmann@nextcloud.com>
1 parent ea88d00 commit 46ce626

8 files changed

Lines changed: 89 additions & 44 deletions

File tree

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

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
use OCP\AppFramework\Http\Attribute\NoAdminRequired;
2020
use OCP\AppFramework\Http\Attribute\NoSubAdminRequired;
2121
use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired;
22+
use OCP\AppFramework\Http\Attribute\UserRateLimit;
2223
use OCP\AppFramework\Http\DataResponse;
2324
use OCP\AppFramework\OCS\OCSException;
2425
use OCP\AppFramework\OCS\OCSForbiddenException;
@@ -42,6 +43,8 @@
4243
* @psalm-import-type Provisioning_APIUserDetailsGroupDisplayname from ResponseDefinitions
4344
*/
4445
class GroupsController extends AUserDataOCSController {
46+
public const int MAX_SEARCH_RESULTS = 500;
47+
4548

4649
public function __construct(
4750
string $appName,
@@ -75,15 +78,15 @@ public function __construct(
7578
* Get a list of groups
7679
*
7780
* @param string $search Text to search for
78-
* @param ?int $limit Limit the amount of groups returned
81+
* @param ?int<1, 500> $limit Limit the amount of groups returned, defaults to 500
7982
* @param int $offset Offset for searching for groups
8083
* @return DataResponse<Http::STATUS_OK, array{groups: list<string>}, array{}>
8184
*
8285
* 200: Groups returned
8386
*/
8487
#[NoAdminRequired]
8588
public function getGroups(string $search = '', ?int $limit = null, int $offset = 0): DataResponse {
86-
$groups = $this->groupManager->search($search, $limit, $offset);
89+
$groups = $this->searchGroups($search, $limit, $offset);
8790
$groups = array_map(function ($group) {
8891
/** @var IGroup $group */
8992
return $group->getGID();
@@ -96,7 +99,7 @@ public function getGroups(string $search = '', ?int $limit = null, int $offset =
9699
* Get a list of groups details
97100
*
98101
* @param string $search Text to search for
99-
* @param ?int $limit Limit the amount of groups returned
102+
* @param ?int<1, 500> $limit Limit the amount of groups returned, defaults to 500
100103
* @param int $offset Offset for searching for groups
101104
* @return DataResponse<Http::STATUS_OK, array{groups: list<Provisioning_APIGroupDetails>}, array{}>
102105
*
@@ -106,7 +109,7 @@ public function getGroups(string $search = '', ?int $limit = null, int $offset =
106109
#[AuthorizedAdminSetting(settings: Sharing::class)]
107110
#[AuthorizedAdminSetting(settings: Users::class)]
108111
public function getGroupsDetails(string $search = '', ?int $limit = null, int $offset = 0): DataResponse {
109-
$groups = $this->groupManager->search($search, $limit, $offset);
112+
$groups = $this->searchGroups($search, $limit, $offset);
110113
$groups = array_map(function ($group) {
111114
/** @var IGroup $group */
112115
return [
@@ -122,6 +125,17 @@ public function getGroupsDetails(string $search = '', ?int $limit = null, int $o
122125
return new DataResponse(['groups' => $groups]);
123126
}
124127

128+
/**
129+
* @return list<IGroup>
130+
*/
131+
private function searchGroups(string $search, ?int $limit, int $offset): array {
132+
if ($limit === null || $limit <= 0 || $limit > self::MAX_SEARCH_RESULTS) {
133+
$limit = self::MAX_SEARCH_RESULTS;
134+
}
135+
136+
return $this->groupManager->search($search, $limit, $offset);
137+
}
138+
125139
/**
126140
* Get a list of users in the specified group
127141
*
@@ -254,6 +268,7 @@ public function getGroupUsersDetails(string $groupId, string $search = '', ?int
254268
* 200: Group created successfully
255269
*/
256270
#[AuthorizedAdminSetting(settings:Users::class)]
271+
#[UserRateLimit(limit: 50, period: 600)]
257272
#[PasswordConfirmationRequired]
258273
public function addGroup(string $groupid, string $displayname = ''): DataResponse {
259274
// Validate name

‎apps/provisioning_api/openapi-full.json‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1245,12 +1245,14 @@
12451245
{
12461246
"name": "limit",
12471247
"in": "query",
1248-
"description": "Limit the amount of groups returned",
1248+
"description": "Limit the amount of groups returned, defaults to 500",
12491249
"schema": {
12501250
"type": "integer",
12511251
"format": "int64",
12521252
"nullable": true,
1253-
"default": null
1253+
"default": null,
1254+
"minimum": 1,
1255+
"maximum": 500
12541256
}
12551257
},
12561258
{
@@ -3186,12 +3188,14 @@
31863188
{
31873189
"name": "limit",
31883190
"in": "query",
3189-
"description": "Limit the amount of groups returned",
3191+
"description": "Limit the amount of groups returned, defaults to 500",
31903192
"schema": {
31913193
"type": "integer",
31923194
"format": "int64",
31933195
"nullable": true,
3194-
"default": null
3196+
"default": null,
3197+
"minimum": 1,
3198+
"maximum": 500
31953199
}
31963200
},
31973201
{

‎apps/provisioning_api/openapi.json‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -456,12 +456,14 @@
456456
{
457457
"name": "limit",
458458
"in": "query",
459-
"description": "Limit the amount of groups returned",
459+
"description": "Limit the amount of groups returned, defaults to 500",
460460
"schema": {
461461
"type": "integer",
462462
"format": "int64",
463463
"nullable": true,
464-
"default": null
464+
"default": null,
465+
"minimum": 1,
466+
"maximum": 500
465467
}
466468
},
467469
{
@@ -586,12 +588,14 @@
586588
{
587589
"name": "limit",
588590
"in": "query",
589-
"description": "Limit the amount of groups returned",
591+
"description": "Limit the amount of groups returned, defaults to 500",
590592
"schema": {
591593
"type": "integer",
592594
"format": "int64",
593595
"nullable": true,
594-
"default": null
596+
"default": null,
597+
"minimum": 1,
598+
"maximum": 500
595599
}
596600
},
597601
{

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

Lines changed: 35 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -157,46 +157,43 @@ private function asSubAdminOfGroup($group) {
157157

158158
public static function dataGetGroups(): array {
159159
return [
160-
[null, 0, 0],
161-
['foo', 0, 0],
162-
[null, 1, 0],
163-
[null, 0, 2],
164-
['foo', 1, 2],
160+
// search, requested limit, offset, expected forwarded limit
161+
[null, null, 0, GroupsController::MAX_SEARCH_RESULTS],
162+
['foo', null, 0, GroupsController::MAX_SEARCH_RESULTS],
163+
[null, 0, 0, GroupsController::MAX_SEARCH_RESULTS],
164+
[null, 1, 0, 1],
165+
[null, 10, 2, 10],
166+
['foo', 1, 2, 1],
167+
[null, GroupsController::MAX_SEARCH_RESULTS + 1, 0, GroupsController::MAX_SEARCH_RESULTS],
165168
];
166169
}
167170

168171
#[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataGetGroups')]
169-
public function testGetGroups(?string $search, int $limit, int $offset): void {
172+
public function testGetGroups(?string $search, ?int $limit, int $offset, int $expectedLimit): void {
170173
$groups = [$this->createGroup('group1'), $this->createGroup('group2')];
171174

172175
$search = $search === null ? '' : $search;
173176

174177
$this->groupManager
175178
->expects($this->once())
176179
->method('search')
177-
->with($search, $limit, $offset)
180+
->with($search, $expectedLimit, $offset)
178181
->willReturn($groups);
179182

180183
$result = $this->api->getGroups($search, $limit, $offset);
181184
$this->assertEquals(['groups' => ['group1', 'group2']], $result->getData());
182185
}
183186

184-
/**
185-
*
186-
* @param string|null $search
187-
* @param int|null $limit
188-
* @param int|null $offset
189-
*/
190187
#[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataGetGroups')]
191-
public function testGetGroupsDetails($search, $limit, $offset): void {
188+
public function testGetGroupsDetails(?string $search, ?int $limit, int $offset, int $expectedLimit): void {
192189
$groups = [$this->createGroup('group1'), $this->createGroup('group2')];
193190

194191
$search = $search === null ? '' : $search;
195192

196193
$this->groupManager
197194
->expects($this->once())
198195
->method('search')
199-
->with($search, $limit, $offset)
196+
->with($search, $expectedLimit, $offset)
200197
->willReturn($groups);
201198

202199
$result = $this->api->getGroupsDetails($search, $limit, $offset);
@@ -220,6 +217,29 @@ public function testGetGroupsDetails($search, $limit, $offset): void {
220217
]], $result->getData());
221218
}
222219

220+
public static function dataCapsBackendOverflow(): array {
221+
return [['getGroups'], ['getGroupsDetails']];
222+
}
223+
224+
/**
225+
* Each backend is searched with the full limit, so the merge can exceed it.
226+
* Truncating here would drop the last backend's groups entirely, so the
227+
* overflow is returned as-is.
228+
*/
229+
#[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataCapsBackendOverflow')]
230+
public function testBackendOverflowIsNotTruncated(string $method): void {
231+
$groups = array_fill(0, GroupsController::MAX_SEARCH_RESULTS + 5, $this->createGroup('group'));
232+
233+
$this->groupManager
234+
->expects($this->once())
235+
->method('search')
236+
->with('', GroupsController::MAX_SEARCH_RESULTS, 0)
237+
->willReturn($groups);
238+
239+
$result = $this->api->$method('', null, 0);
240+
$this->assertCount(GroupsController::MAX_SEARCH_RESULTS + 5, $result->getData()['groups']);
241+
}
242+
223243
public function testGetGroupAsSubadmin(): void {
224244
$group = $this->createGroup('group');
225245
$this->asSubAdminOfGroup($group);

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

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -141,14 +141,6 @@ public function usersList(INavigationManager $navigationManager, ISubAdmin $subA
141141
$canChangePassword = $this->canAdminChangeUserPasswords();
142142

143143
/* GROUPS */
144-
$groupsInfo = new MetaData(
145-
$uid,
146-
$isAdmin,
147-
$isDelegatedAdmin,
148-
$this->groupManager,
149-
$this->userSession
150-
);
151-
152144
$adminGroup = $this->groupManager->get('admin');
153145
$adminGroupData = [
154146
'id' => $adminGroup->getGID(),
@@ -167,6 +159,7 @@ public function usersList(INavigationManager $navigationManager, ISubAdmin $subA
167159

168160
$disabledUsers = -1;
169161
$userCount = 0;
162+
$ownSubAdminGroups = ($isAdmin || $isDelegatedAdmin) ? [] : $subAdmin->getSubAdminsGroups($user);
170163

171164
if (!$isLDAPUsed) {
172165
if ($isAdmin || $isDelegatedAdmin) {
@@ -176,7 +169,7 @@ public function usersList(INavigationManager $navigationManager, ISubAdmin $subA
176169
}, 0);
177170
} else {
178171
// User is subadmin !
179-
[$userCount,$disabledUsers] = $this->userManager->countUsersAndDisabledUsersOfGroups($groupsInfo->getGroups(), self::COUNT_LIMIT_FOR_SUBADMINS);
172+
[$userCount,$disabledUsers] = $this->userManager->countUsersAndDisabledUsersOfGroups($ownSubAdminGroups, self::COUNT_LIMIT_FOR_SUBADMINS);
180173
}
181174

182175
if ($disabledUsers > 0) {
@@ -199,7 +192,7 @@ public function usersList(INavigationManager $navigationManager, ISubAdmin $subA
199192
if (!$isAdmin && !$isDelegatedAdmin) {
200193
$subAdminGroups = array_map(
201194
fn (IGroup $group) => ['id' => $group->getGID(), 'name' => $group->getDisplayName()],
202-
$subAdmin->getSubAdminsGroups($user),
195+
$ownSubAdminGroups,
203196
);
204197
$subAdminGroups = array_values($subAdminGroups);
205198
}

‎lib/private/Group/MetaData.php‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ class MetaData {
1818
public const SORT_USERCOUNT = 1; // May have performance issues on LDAP backends
1919
public const SORT_GROUPNAME = 2;
2020

21+
public const DEFAULT_GROUP_LIMIT = 500;
22+
2123
/** @var array */
2224
protected $metaData = [];
2325
/** @var int */
@@ -148,12 +150,13 @@ private function sort(array &$entries, array $sortKeys): void {
148150
}
149151

150152
/**
151-
* returns the available groups
153+
* returns the available groups, capped at {@see self::DEFAULT_GROUP_LIMIT}
154+
*
152155
* @return IGroup[]
153156
*/
154157
public function getGroups(string $search = ''): array {
155158
if ($this->isAdmin || $this->isDelegatedAdmin) {
156-
return $this->groupManager->search($search);
159+
return $this->groupManager->search($search, self::DEFAULT_GROUP_LIMIT);
157160
} else {
158161
$userObject = $this->userSession->getUser();
159162
if ($userObject !== null && $this->groupManager instanceof GroupManager) {

‎openapi.json‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31801,12 +31801,14 @@
3180131801
{
3180231802
"name": "limit",
3180331803
"in": "query",
31804-
"description": "Limit the amount of groups returned",
31804+
"description": "Limit the amount of groups returned, defaults to 500",
3180531805
"schema": {
3180631806
"type": "integer",
3180731807
"format": "int64",
3180831808
"nullable": true,
31809-
"default": null
31809+
"default": null,
31810+
"minimum": 1,
31811+
"maximum": 500
3181031812
}
3181131813
},
3181231814
{
@@ -33742,12 +33744,14 @@
3374233744
{
3374333745
"name": "limit",
3374433746
"in": "query",
33745-
"description": "Limit the amount of groups returned",
33747+
"description": "Limit the amount of groups returned, defaults to 500",
3374633748
"schema": {
3374733749
"type": "integer",
3374833750
"format": "int64",
3374933751
"nullable": true,
33750-
"default": null
33752+
"default": null,
33753+
"minimum": 1,
33754+
"maximum": 500
3375133755
}
3375233756
},
3375333757
{

‎tests/lib/Group/MetaDataTest.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,10 +112,12 @@ public function testGetGroupsAsAdmin(): void {
112112
$this->groupManager
113113
->expects($this->once())
114114
->method('search')
115-
->with('Foo')
115+
->with('Foo', MetaData::DEFAULT_GROUP_LIMIT)
116116
->willReturn(['DummyValue']);
117117

118118
$expected = ['DummyValue'];
119119
$this->assertSame($expected, $this->invokePrivate($this->groupMetadata, 'getGroups', ['Foo']));
120120
}
121+
122+
121123
}

0 commit comments

Comments
 (0)