Skip to content

Commit ca5cfdf

Browse files
AltahrimAltahrim
committed
fix(groups): allows to save group names with more than 64 characters
Mimic behaviour from LDAP users and add a hard limit to 255 characters Signed-off-by: Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
1 parent d3b5db8 commit ca5cfdf

File tree

9 files changed

+115
-22
lines changed

9 files changed

+115
-22
lines changed

‎lib/composer/composer/autoload_classmap.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -445,6 +445,7 @@
445445
'OCP\\Group\\Backend\\ICountDisabledInGroup' => $baseDir . '/lib/public/Group/Backend/ICountDisabledInGroup.php',
446446
'OCP\\Group\\Backend\\ICountUsersBackend' => $baseDir . '/lib/public/Group/Backend/ICountUsersBackend.php',
447447
'OCP\\Group\\Backend\\ICreateGroupBackend' => $baseDir . '/lib/public/Group/Backend/ICreateGroupBackend.php',
448+
'OCP\\Group\\Backend\\ICreateNamedGroupBackend' => $baseDir . '/lib/public/Group/Backend/ICreateNamedGroupBackend.php',
448449
'OCP\\Group\\Backend\\IDeleteGroupBackend' => $baseDir . '/lib/public/Group/Backend/IDeleteGroupBackend.php',
449450
'OCP\\Group\\Backend\\IGetDisplayNameBackend' => $baseDir . '/lib/public/Group/Backend/IGetDisplayNameBackend.php',
450451
'OCP\\Group\\Backend\\IGroupDetailsBackend' => $baseDir . '/lib/public/Group/Backend/IGroupDetailsBackend.php',

‎lib/composer/composer/autoload_static.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -478,6 +478,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
478478
'OCP\\Group\\Backend\\ICountDisabledInGroup' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ICountDisabledInGroup.php',
479479
'OCP\\Group\\Backend\\ICountUsersBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ICountUsersBackend.php',
480480
'OCP\\Group\\Backend\\ICreateGroupBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ICreateGroupBackend.php',
481+
'OCP\\Group\\Backend\\ICreateNamedGroupBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ICreateNamedGroupBackend.php',
481482
'OCP\\Group\\Backend\\IDeleteGroupBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/IDeleteGroupBackend.php',
482483
'OCP\\Group\\Backend\\IGetDisplayNameBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/IGetDisplayNameBackend.php',
483484
'OCP\\Group\\Backend\\IGroupDetailsBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/IGroupDetailsBackend.php',

‎lib/private/Group/Database.php‎

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@
3737
use OCP\Group\Backend\IBatchMethodsBackend;
3838
use OCP\Group\Backend\ICountDisabledInGroup;
3939
use OCP\Group\Backend\ICountUsersBackend;
40-
use OCP\Group\Backend\ICreateGroupBackend;
40+
use OCP\Group\Backend\ICreateNamedGroupBackend;
4141
use OCP\Group\Backend\IDeleteGroupBackend;
4242
use OCP\Group\Backend\IGetDisplayNameBackend;
4343
use OCP\Group\Backend\IGroupDetailsBackend;
@@ -55,7 +55,7 @@ class Database extends ABackend implements
5555
IAddToGroupBackend,
5656
ICountDisabledInGroup,
5757
ICountUsersBackend,
58-
ICreateGroupBackend,
58+
ICreateNamedGroupBackend,
5959
IDeleteGroupBackend,
6060
IGetDisplayNameBackend,
6161
IGroupDetailsBackend,
@@ -86,35 +86,28 @@ private function fixDI() {
8686
}
8787
}
8888

89-
/**
90-
* Try to create a new group
91-
* @param string $gid The name of the group to create
92-
* @return bool
93-
*
94-
* Tries to create a new group. If the group name already exists, false will
95-
* be returned.
96-
*/
97-
public function createGroup(string $gid): bool {
89+
public function createGroup(string $name): ?string {
9890
$this->fixDI();
9991

92+
$gid = $this->computeGid($name);
10093
try {
10194
// Add group
10295
$builder = $this->dbConn->getQueryBuilder();
10396
$result = $builder->insert('groups')
10497
->setValue('gid', $builder->createNamedParameter($gid))
105-
->setValue('displayname', $builder->createNamedParameter($gid))
98+
->setValue('displayname', $builder->createNamedParameter($name))
10699
->execute();
107100
} catch (UniqueConstraintViolationException $e) {
108-
$result = 0;
101+
return null;
109102
}
110103

111104
// Add to cache
112105
$this->groupCache[$gid] = [
113106
'gid' => $gid,
114-
'displayname' => $gid
107+
'displayname' => $name
115108
];
116109

117-
return $result === 1;
110+
return $gid;
118111
}
119112

120113
/**
@@ -595,4 +588,13 @@ public function setDisplayName(string $gid, string $displayName): bool {
595588
public function getBackendName(): string {
596589
return 'Database';
597590
}
591+
592+
/**
593+
* Compute group ID from display name (GIDs are limited to 64 characters in database)
594+
*/
595+
private function computeGid(string $displayName): string {
596+
return mb_strlen($displayName) > 64
597+
? hash('sha256', $displayName)
598+
: $displayName;
599+
}
598600
}

‎lib/private/Group/Manager.php‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
use OC\Hooks\PublicEmitter;
4444
use OCP\EventDispatcher\IEventDispatcher;
4545
use OCP\Group\Backend\IBatchMethodsBackend;
46+
use OCP\Group\Backend\ICreateNamedGroupBackend;
4647
use OCP\Group\Backend\IGroupDetailsBackend;
4748
use OCP\Group\Events\BeforeGroupCreatedEvent;
4849
use OCP\Group\Events\GroupCreatedEvent;
@@ -89,6 +90,8 @@ class Manager extends PublicEmitter implements IGroupManager {
8990

9091
private DisplayNameCache $displayNameCache;
9192

93+
private const MAX_GROUP_LENGTH = 255;
94+
9295
public function __construct(\OC\User\Manager $userManager,
9396
IEventDispatcher $dispatcher,
9497
LoggerInterface $logger,
@@ -281,12 +284,22 @@ public function createGroup($gid) {
281284
return null;
282285
} elseif ($group = $this->get($gid)) {
283286
return $group;
287+
} elseif (mb_strlen($gid) > self::MAX_GROUP_LENGTH) {
288+
throw new \Exception('Group name is limited to '. self::MAX_GROUP_LENGTH.' characters');
284289
} else {
285290
$this->dispatcher->dispatchTyped(new BeforeGroupCreatedEvent($gid));
286291
$this->emit('\OC\Group', 'preCreate', [$gid]);
287292
foreach ($this->backends as $backend) {
288293
if ($backend->implementsActions(Backend::CREATE_GROUP)) {
289-
if ($backend->createGroup($gid)) {
294+
if ($backend instanceof ICreateNamedGroupBackend) {
295+
$groupName = $gid;
296+
if (($gid = $backend->createGroup($groupName)) !== null) {
297+
$group = $this->getGroupObject($gid);
298+
$this->dispatcher->dispatchTyped(new GroupCreatedEvent($group));
299+
$this->emit('\OC\Group', 'postCreate', [$group]);
300+
return $group;
301+
}
302+
} elseif ($backend->createGroup($gid)) {
290303
$group = $this->getGroupObject($gid);
291304
$this->dispatcher->dispatchTyped(new GroupCreatedEvent($group));
292305
$this->emit('\OC\Group', 'postCreate', [$group]);

‎lib/public/Group/Backend/ABackend.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ public function implementsActions($actions): bool {
4848
if ($this instanceof ICountUsersBackend) {
4949
$implements |= GroupInterface::COUNT_USERS;
5050
}
51-
if ($this instanceof ICreateGroupBackend) {
51+
if ($this instanceof ICreateGroupBackend || $this instanceof ICreateNamedGroupBackend) {
5252
$implements |= GroupInterface::CREATE_GROUP;
5353
}
5454
if ($this instanceof IDeleteGroupBackend) {

‎lib/public/Group/Backend/ICreateGroupBackend.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727

2828
/**
2929
* @since 14.0.0
30+
* @deprecated 30.0.0 Use ICreateNamedGroupBackend instead
3031
*/
3132
interface ICreateGroupBackend {
3233
/**

‎lib/public/Group/Backend/ICreateNamedGroupBackend.php‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* @copyright Copyright (c) 2024 Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
7+
*
8+
* @author Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
9+
*
10+
* @license GNU AGPL version 3 or any later version
11+
*
12+
* This program is free software: you can redistribute it and/or modify
13+
* it under the terms of the GNU Affero General Public License as
14+
* published by the Free Software Foundation, either version 3 of the
15+
* License, or (at your option) any later version.
16+
*
17+
* This program is distributed in the hope that it will be useful,
18+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
19+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
20+
* GNU Affero General Public License for more details.
21+
*
22+
* You should have received a copy of the GNU Affero General Public License
23+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
24+
*
25+
*/
26+
namespace OCP\Group\Backend;
27+
28+
/**
29+
* @since 30.0.0
30+
*/
31+
interface ICreateNamedGroupBackend {
32+
/**
33+
* Tries to create a group from its name.
34+
*
35+
* If group name already exists, null is returned.
36+
* Otherwise, new group ID is returned.
37+
*
38+
* @param string $name Group name
39+
* @return ?string Group ID in case of success, null in case of failure
40+
* @since 30.0.0
41+
*/
42+
public function createGroup(string $name): ?string;
43+
}

‎tests/lib/Group/DatabaseTest.php‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,8 @@ class DatabaseTest extends Backend {
3636
/**
3737
* get a new unique group name
3838
* test cases can override this in order to clean up created groups
39-
*
40-
* @return string
4139
*/
42-
public function getGroupName($name = null) {
40+
public function getGroupName($name = null): string {
4341
$name = parent::getGroupName($name);
4442
$this->groups[] = $name;
4543
return $name;
@@ -57,12 +55,22 @@ protected function tearDown(): void {
5755
parent::tearDown();
5856
}
5957

60-
public function testAddDoubleNoCache() {
58+
public function testAddDoubleNoCache(): void {
6159
$group = $this->getGroupName();
6260

6361
$this->backend->createGroup($group);
6462

6563
$backend = new \OC\Group\Database();
66-
$this->assertFalse($backend->createGroup($group));
64+
$this->assertNull($backend->createGroup($group));
65+
}
66+
67+
public function testAddLongGroupName(): void {
68+
$groupName = $this->getUniqueID('test_', 100);
69+
70+
$gidCreated = $this->backend->createGroup($groupName);
71+
$this->assertEquals(64, strlen($gidCreated));
72+
73+
$group = $this->backend->getGroupDetails($gidCreated);
74+
$this->assertEquals(['displayName' => $groupName], $group);
6775
}
6876
}

‎tests/lib/Group/ManagerTest.php‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,30 @@ public function testCreateFailure() {
241241
$this->assertEquals(null, $group);
242242
}
243243

244+
public function testCreateTooLong() {
245+
/**@var \PHPUnit\Framework\MockObject\MockObject|\OC\Group\Backend $backend */
246+
$backendGroupCreated = false;
247+
$backend = $this->getTestBackend(
248+
GroupInterface::ADD_TO_GROUP |
249+
GroupInterface::REMOVE_FROM_GOUP |
250+
GroupInterface::COUNT_USERS |
251+
GroupInterface::CREATE_GROUP |
252+
GroupInterface::DELETE_GROUP |
253+
GroupInterface::GROUP_DETAILS
254+
);
255+
$groupName = str_repeat('x', 256);
256+
$backend->expects($this->any())
257+
->method('groupExists')
258+
->with($groupName)
259+
->willReturn(false);
260+
261+
$manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache);
262+
$manager->addBackend($backend);
263+
264+
$this->expectException(\Exception::class);
265+
$group = $manager->createGroup($groupName);
266+
}
267+
244268
public function testCreateExists() {
245269
/** @var \PHPUnit\Framework\MockObject\MockObject|\OC\Group\Backend $backend */
246270
$backend = $this->getTestBackend();

0 commit comments

Comments
 (0)