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>

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;
+	}
 }

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]);

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));
 	}
+
+	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);
+	}
 }

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();