From 4c300d79d21c9569242d323836093ce1157990f2 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/private/Group/Database.php | 20 ++++++++++++++++---- lib/private/Group/Manager.php | 4 ++++ tests/lib/Group/DatabaseTest.php | 16 ++++++++++++---- tests/lib/Group/ManagerTest.php | 24 ++++++++++++++++++++++++ 4 files changed, 56 insertions(+), 8 deletions(-) diff --git a/lib/private/Group/Database.php b/lib/private/Group/Database.php index c4915eefeae1a..865d45541eb1a 100644 --- a/lib/private/Group/Database.php +++ b/lib/private/Group/Database.php @@ -88,21 +88,22 @@ private function fixDI() { /** * Try to create a new group - * @param string $gid The name of the group to create + * @param string $displayName 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 $displayName): bool { $this->fixDI(); + $gid = $this->computeGid($displayName); 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($displayName)) ->execute(); } catch (UniqueConstraintViolationException $e) { $result = 0; @@ -111,7 +112,7 @@ public function createGroup(string $gid): bool { // Add to cache $this->groupCache[$gid] = [ 'gid' => $gid, - 'displayname' => $gid + 'displayname' => $displayName ]; return $result === 1; @@ -521,6 +522,8 @@ public function getDisplayName(string $gid): string { } public function getGroupDetails(string $gid): array { + // Makes sure we have a GID (useful on group creation) + $gid = $this->computeGid($gid); $displayName = $this->getDisplayName($gid); if ($displayName !== '') { return ['displayName' => $displayName]; @@ -595,4 +598,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 2b6eb70502b45..01c0f4d1644d0 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -89,6 +89,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,6 +283,8 @@ 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]); diff --git a/tests/lib/Group/DatabaseTest.php b/tests/lib/Group/DatabaseTest.php index 586d77e0ec0b5..36218fdde1a01 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,7 +55,7 @@ protected function tearDown(): void { parent::tearDown(); } - public function testAddDoubleNoCache() { + public function testAddDoubleNoCache(): void { $group = $this->getGroupName(); $this->backend->createGroup($group); @@ -65,4 +63,14 @@ public function testAddDoubleNoCache() { $backend = new \OC\Group\Database(); $this->assertFalse($backend->createGroup($group)); } + + public function testAddLongGroupName(): void { + $groupName = $this->getUniqueID('test_', 100); + + $created = $this->backend->createGroup($groupName); + $this->assertTrue($created); + + $group = $this->backend->getGroupDetails($groupName); + $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();