Merge pull request #44763 from nextcloud/fix/group_name_length_db

9 files changed, 115 insertions, 22 deletions

diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php
index 16054f71a73..cb57d44ea60 100644
--- a/lib/composer/composer/autoload_classmap.php
+++ b/lib/composer/composer/autoload_classmap.php
@@ -462,6 +462,7 @@ return array(
     '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 3e97a6d3f18..0a4e9571049 100644
--- a/lib/composer/composer/autoload_static.php
+++ b/lib/composer/composer/autoload_static.php
@@ -503,6 +503,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 c4915eefeae..7c9bcdda859 100644
--- a/lib/private/Group/Database.php
+++ b/lib/private/Group/Database.php
@@ -37,7 +37,7 @@ use OCP\Group\Backend\IAddToGroupBackend;
 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 @@ class Database extends ABackend implements
 		}
 	}
 
-	/**
-	 * 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 @@ class Database extends ABackend implements
 	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 30f55107a96..694b6de30c4 100644
--- a/lib/private/Group/Manager.php
+++ b/lib/private/Group/Manager.php
@@ -43,6 +43,7 @@ namespace OC\Group;
 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,
@@ -270,12 +273,22 @@ class Manager extends PublicEmitter implements IGroupManager {
 			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 274b98655e4..ec456ed8d81 100644
--- a/lib/public/Group/Backend/ABackend.php
+++ b/lib/public/Group/Backend/ABackend.php
@@ -48,7 +48,7 @@ abstract class ABackend implements GroupInterface, IBatchMethodsBackend {
 		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 1c90eae1149..ff60bfe257d 100644
--- a/lib/public/Group/Backend/ICreateGroupBackend.php
+++ b/lib/public/Group/Backend/ICreateGroupBackend.php
@@ -27,6 +27,7 @@ namespace OCP\Group\Backend;
 
 /**
  * @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 00000000000..985dc2bee2e
--- /dev/null
+++ b/lib/public/Group/Backend/ICreateNamedGroupBackend.php
@@ -0,0 +1,43 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * @copyright Copyright (c) 2024 Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
+ *
+ * @author Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
+ *
+ * @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 <http://www.gnu.org/licenses/>.
+ *
+ */
+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 6a59ed92be3..593fbe60bf9 100644
--- a/tests/lib/Group/DatabaseTest.php
+++ b/tests/lib/Group/DatabaseTest.php
@@ -18,10 +18,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;
@@ -39,12 +37,22 @@ class DatabaseTest extends Backend {
 		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 bcf9071db5f..81fd77cc78a 100644
--- a/tests/lib/Group/ManagerTest.php
+++ b/tests/lib/Group/ManagerTest.php
@@ -226,6 +226,30 @@ class ManagerTest extends TestCase {
 		$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();