From 81e370008346c5d9cece678772fd9c8e2c45e9d0 Mon Sep 17 00:00:00 2001 From: Benjamin Gaussorgues Date: Wed, 10 Apr 2024 11:00:51 +0200 Subject: [PATCH] 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 --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/private/Group/Database.php | 32 +++++++------- lib/private/Group/Manager.php | 15 ++++++- lib/public/Group/Backend/ABackend.php | 2 +- .../Group/Backend/ICreateGroupBackend.php | 1 + .../Backend/ICreateNamedGroupBackend.php | 43 +++++++++++++++++++ tests/lib/Group/DatabaseTest.php | 18 +++++--- tests/lib/Group/ManagerTest.php | 24 +++++++++++ 9 files changed, 115 insertions(+), 22 deletions(-) create mode 100644 lib/public/Group/Backend/ICreateNamedGroupBackend.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index beb09589003af..d976b5e7f3506 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -445,6 +445,7 @@ 'OCP\\Group\\Backend\\ICountDisabledInGroup' => $baseDir . '/lib/public/Group/Backend/ICountDisabledInGroup.php', 'OCP\\Group\\Backend\\ICountUsersBackend' => $baseDir . '/lib/public/Group/Backend/ICountUsersBackend.php', 'OCP\\Group\\Backend\\ICreateGroupBackend' => $baseDir . '/lib/public/Group/Backend/ICreateGroupBackend.php', + 'OCP\\Group\\Backend\\ICreateNamedGroupBackend' => $baseDir . '/lib/public/Group/Backend/ICreateNamedGroupBackend.php', 'OCP\\Group\\Backend\\IDeleteGroupBackend' => $baseDir . '/lib/public/Group/Backend/IDeleteGroupBackend.php', 'OCP\\Group\\Backend\\IGetDisplayNameBackend' => $baseDir . '/lib/public/Group/Backend/IGetDisplayNameBackend.php', 'OCP\\Group\\Backend\\IGroupDetailsBackend' => $baseDir . '/lib/public/Group/Backend/IGroupDetailsBackend.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 2d29f02c4b429..d362d4fdfd6c6 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -478,6 +478,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OCP\\Group\\Backend\\ICountDisabledInGroup' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ICountDisabledInGroup.php', 'OCP\\Group\\Backend\\ICountUsersBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ICountUsersBackend.php', 'OCP\\Group\\Backend\\ICreateGroupBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ICreateGroupBackend.php', + 'OCP\\Group\\Backend\\ICreateNamedGroupBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ICreateNamedGroupBackend.php', 'OCP\\Group\\Backend\\IDeleteGroupBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/IDeleteGroupBackend.php', 'OCP\\Group\\Backend\\IGetDisplayNameBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/IGetDisplayNameBackend.php', 'OCP\\Group\\Backend\\IGroupDetailsBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/IGroupDetailsBackend.php', diff --git a/lib/private/Group/Database.php b/lib/private/Group/Database.php index 13837eef55294..1e66b62ade3a6 100644 --- a/lib/private/Group/Database.php +++ b/lib/private/Group/Database.php @@ -37,7 +37,7 @@ use OCP\Group\Backend\IBatchMethodsBackend; use OCP\Group\Backend\ICountDisabledInGroup; use OCP\Group\Backend\ICountUsersBackend; -use OCP\Group\Backend\ICreateGroupBackend; +use OCP\Group\Backend\ICreateNamedGroupBackend; use OCP\Group\Backend\IDeleteGroupBackend; use OCP\Group\Backend\IGetDisplayNameBackend; use OCP\Group\Backend\IGroupDetailsBackend; @@ -55,7 +55,7 @@ class Database extends ABackend implements IAddToGroupBackend, ICountDisabledInGroup, ICountUsersBackend, - ICreateGroupBackend, + ICreateNamedGroupBackend, IDeleteGroupBackend, IGetDisplayNameBackend, IGroupDetailsBackend, @@ -86,35 +86,28 @@ private function fixDI() { } } - /** - * Try to create a new group - * @param string $gid The name of the group to create - * @return bool - * - * Tries to create a new group. If the group name already exists, false will - * be returned. - */ - public function createGroup(string $gid): bool { + public function createGroup(string $name): ?string { $this->fixDI(); + $gid = $this->computeGid($name); try { // Add group $builder = $this->dbConn->getQueryBuilder(); $result = $builder->insert('groups') ->setValue('gid', $builder->createNamedParameter($gid)) - ->setValue('displayname', $builder->createNamedParameter($gid)) + ->setValue('displayname', $builder->createNamedParameter($name)) ->execute(); } catch (UniqueConstraintViolationException $e) { - $result = 0; + return null; } // Add to cache $this->groupCache[$gid] = [ 'gid' => $gid, - 'displayname' => $gid + 'displayname' => $name ]; - return $result === 1; + return $gid; } /** @@ -595,4 +588,13 @@ public function setDisplayName(string $gid, string $displayName): bool { public function getBackendName(): string { return 'Database'; } + + /** + * Compute group ID from display name (GIDs are limited to 64 characters in database) + */ + private function computeGid(string $displayName): string { + return mb_strlen($displayName) > 64 + ? hash('sha256', $displayName) + : $displayName; + } } diff --git a/lib/private/Group/Manager.php b/lib/private/Group/Manager.php index dafbe4295a491..649de92f9fc57 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -43,6 +43,7 @@ use OC\Hooks\PublicEmitter; use OCP\EventDispatcher\IEventDispatcher; use OCP\Group\Backend\IBatchMethodsBackend; +use OCP\Group\Backend\ICreateNamedGroupBackend; use OCP\Group\Backend\IGroupDetailsBackend; use OCP\Group\Events\BeforeGroupCreatedEvent; use OCP\Group\Events\GroupCreatedEvent; @@ -89,6 +90,8 @@ class Manager extends PublicEmitter implements IGroupManager { private DisplayNameCache $displayNameCache; + private const MAX_GROUP_LENGTH = 255; + public function __construct(\OC\User\Manager $userManager, IEventDispatcher $dispatcher, LoggerInterface $logger, @@ -281,12 +284,22 @@ public function createGroup($gid) { return null; } elseif ($group = $this->get($gid)) { return $group; + } elseif (mb_strlen($gid) > self::MAX_GROUP_LENGTH) { + throw new \Exception('Group name is limited to '. self::MAX_GROUP_LENGTH.' characters'); } else { $this->dispatcher->dispatchTyped(new BeforeGroupCreatedEvent($gid)); $this->emit('\OC\Group', 'preCreate', [$gid]); foreach ($this->backends as $backend) { if ($backend->implementsActions(Backend::CREATE_GROUP)) { - if ($backend->createGroup($gid)) { + if ($backend instanceof ICreateNamedGroupBackend) { + $groupName = $gid; + if (($gid = $backend->createGroup($groupName)) !== null) { + $group = $this->getGroupObject($gid); + $this->dispatcher->dispatchTyped(new GroupCreatedEvent($group)); + $this->emit('\OC\Group', 'postCreate', [$group]); + return $group; + } + } elseif ($backend->createGroup($gid)) { $group = $this->getGroupObject($gid); $this->dispatcher->dispatchTyped(new GroupCreatedEvent($group)); $this->emit('\OC\Group', 'postCreate', [$group]); diff --git a/lib/public/Group/Backend/ABackend.php b/lib/public/Group/Backend/ABackend.php index 274b98655e465..ec456ed8d81f2 100644 --- a/lib/public/Group/Backend/ABackend.php +++ b/lib/public/Group/Backend/ABackend.php @@ -48,7 +48,7 @@ public function implementsActions($actions): bool { if ($this instanceof ICountUsersBackend) { $implements |= GroupInterface::COUNT_USERS; } - if ($this instanceof ICreateGroupBackend) { + if ($this instanceof ICreateGroupBackend || $this instanceof ICreateNamedGroupBackend) { $implements |= GroupInterface::CREATE_GROUP; } if ($this instanceof IDeleteGroupBackend) { diff --git a/lib/public/Group/Backend/ICreateGroupBackend.php b/lib/public/Group/Backend/ICreateGroupBackend.php index 1c90eae114915..ff60bfe257d7a 100644 --- a/lib/public/Group/Backend/ICreateGroupBackend.php +++ b/lib/public/Group/Backend/ICreateGroupBackend.php @@ -27,6 +27,7 @@ /** * @since 14.0.0 + * @deprecated 30.0.0 Use ICreateNamedGroupBackend instead */ interface ICreateGroupBackend { /** diff --git a/lib/public/Group/Backend/ICreateNamedGroupBackend.php b/lib/public/Group/Backend/ICreateNamedGroupBackend.php new file mode 100644 index 0000000000000..985dc2bee2e20 --- /dev/null +++ b/lib/public/Group/Backend/ICreateNamedGroupBackend.php @@ -0,0 +1,43 @@ + + * + * @author Benjamin Gaussorgues + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ +namespace OCP\Group\Backend; + +/** + * @since 30.0.0 + */ +interface ICreateNamedGroupBackend { + /** + * Tries to create a group from its name. + * + * If group name already exists, null is returned. + * Otherwise, new group ID is returned. + * + * @param string $name Group name + * @return ?string Group ID in case of success, null in case of failure + * @since 30.0.0 + */ + public function createGroup(string $name): ?string; +} diff --git a/tests/lib/Group/DatabaseTest.php b/tests/lib/Group/DatabaseTest.php index 586d77e0ec0b5..9d3b4d8b9fd63 100644 --- a/tests/lib/Group/DatabaseTest.php +++ b/tests/lib/Group/DatabaseTest.php @@ -36,10 +36,8 @@ class DatabaseTest extends Backend { /** * get a new unique group name * test cases can override this in order to clean up created groups - * - * @return string */ - public function getGroupName($name = null) { + public function getGroupName($name = null): string { $name = parent::getGroupName($name); $this->groups[] = $name; return $name; @@ -57,12 +55,22 @@ protected function tearDown(): void { parent::tearDown(); } - public function testAddDoubleNoCache() { + public function testAddDoubleNoCache(): void { $group = $this->getGroupName(); $this->backend->createGroup($group); $backend = new \OC\Group\Database(); - $this->assertFalse($backend->createGroup($group)); + $this->assertNull($backend->createGroup($group)); + } + + public function testAddLongGroupName(): void { + $groupName = $this->getUniqueID('test_', 100); + + $gidCreated = $this->backend->createGroup($groupName); + $this->assertEquals(64, strlen($gidCreated)); + + $group = $this->backend->getGroupDetails($gidCreated); + $this->assertEquals(['displayName' => $groupName], $group); } } diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index 142532a5f4feb..9a53bc8326c00 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -241,6 +241,30 @@ public function testCreateFailure() { $this->assertEquals(null, $group); } + public function testCreateTooLong() { + /**@var \PHPUnit\Framework\MockObject\MockObject|\OC\Group\Backend $backend */ + $backendGroupCreated = false; + $backend = $this->getTestBackend( + GroupInterface::ADD_TO_GROUP | + GroupInterface::REMOVE_FROM_GOUP | + GroupInterface::COUNT_USERS | + GroupInterface::CREATE_GROUP | + GroupInterface::DELETE_GROUP | + GroupInterface::GROUP_DETAILS + ); + $groupName = str_repeat('x', 256); + $backend->expects($this->any()) + ->method('groupExists') + ->with($groupName) + ->willReturn(false); + + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager->addBackend($backend); + + $this->expectException(\Exception::class); + $group = $manager->createGroup($groupName); + } + public function testCreateExists() { /** @var \PHPUnit\Framework\MockObject\MockObject|\OC\Group\Backend $backend */ $backend = $this->getTestBackend();