feat(systemtags): add setting to block non admin to create system tags

Signed-off-by: Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>

9 files changed, 161 insertions, 28 deletions

diff --git a/apps/dav/lib/SystemTag/SystemTagPlugin.php b/apps/dav/lib/SystemTag/SystemTagPlugin.php
index 8305944cb0b..428e71d91f4 100644
--- a/apps/dav/lib/SystemTag/SystemTagPlugin.php
+++ b/apps/dav/lib/SystemTag/SystemTagPlugin.php
@@ -18,6 +18,7 @@ use OCP\SystemTag\ISystemTag;
 use OCP\SystemTag\ISystemTagManager;
 use OCP\SystemTag\ISystemTagObjectMapper;
 use OCP\SystemTag\TagAlreadyExistsException;
+use OCP\SystemTag\TagCreationForbiddenException;
 use OCP\Util;
 use Sabre\DAV\Exception\BadRequest;
 use Sabre\DAV\Exception\Conflict;
@@ -189,6 +190,8 @@ class SystemTagPlugin extends \Sabre\DAV\ServerPlugin {
 			return $tag;
 		} catch (TagAlreadyExistsException $e) {
 			throw new Conflict('Tag already exists', 0, $e);
+		} catch (TagCreationForbiddenException $e) {
+			throw new Forbidden('You don’t have right to create tags', 0, $e);
 		}
 	}
 
@@ -376,7 +379,7 @@ class SystemTagPlugin extends \Sabre\DAV\ServerPlugin {
 		if (!$node instanceof SystemTagNode && !$node instanceof SystemTagObjectType) {
 			return;
 		}
-		
+
 		$propPatch->handle([self::OBJECTIDS_PROPERTYNAME], function ($props) use ($node) {
 			if (!$node instanceof SystemTagObjectType) {
 				return false;
@@ -394,7 +397,7 @@ class SystemTagPlugin extends \Sabre\DAV\ServerPlugin {
 				if (count($objectTypes) !== 1 || $objectTypes[0] !== $node->getName()) {
 					throw new BadRequest('Invalid object-ids property. All object types must be of the same type: ' . $node->getName());
 				}
-				
+
 				$this->tagMapper->setObjectIdsForTag($node->getSystemTag()->getId(), $node->getName(), array_keys($objects));
 			}
 
diff --git a/apps/settings/tests/Settings/Admin/ServerTest.php b/apps/settings/tests/Settings/Admin/ServerTest.php
index aa33ff9df74..a49dcc98bf8 100644
--- a/apps/settings/tests/Settings/Admin/ServerTest.php
+++ b/apps/settings/tests/Settings/Admin/ServerTest.php
@@ -84,8 +84,7 @@ class ServerTest extends TestCase {
 		$this->appConfig
 			->expects($this->any())
 			->method('getValueString')
-			->with('core', 'backgroundjobs_mode', 'ajax')
-			->willReturn('ajax');
+			->willReturnCallback(fn ($a, $b, $default) => $default);
 		$this->profileManager
 			->expects($this->exactly(2))
 			->method('isProfileEnabled')
diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php
index 6014ce88691..b589690bc10 100644
--- a/lib/composer/composer/autoload_classmap.php
+++ b/lib/composer/composer/autoload_classmap.php
@@ -796,6 +796,7 @@ return array(
     'OCP\\SystemTag\\MapperEvent' => $baseDir . '/lib/public/SystemTag/MapperEvent.php',
     'OCP\\SystemTag\\SystemTagsEntityEvent' => $baseDir . '/lib/public/SystemTag/SystemTagsEntityEvent.php',
     'OCP\\SystemTag\\TagAlreadyExistsException' => $baseDir . '/lib/public/SystemTag/TagAlreadyExistsException.php',
+    'OCP\\SystemTag\\TagCreationForbiddenException' => $baseDir . '/lib/public/SystemTag/TagCreationForbiddenException.php',
     'OCP\\SystemTag\\TagNotFoundException' => $baseDir . '/lib/public/SystemTag/TagNotFoundException.php',
     'OCP\\Talk\\Exceptions\\NoBackendException' => $baseDir . '/lib/public/Talk/Exceptions/NoBackendException.php',
     'OCP\\Talk\\IBroker' => $baseDir . '/lib/public/Talk/IBroker.php',
diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php
index 01b54d1098a..65c8ae215dc 100644
--- a/lib/composer/composer/autoload_static.php
+++ b/lib/composer/composer/autoload_static.php
@@ -845,6 +845,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
         'OCP\\SystemTag\\MapperEvent' => __DIR__ . '/../../..' . '/lib/public/SystemTag/MapperEvent.php',
         'OCP\\SystemTag\\SystemTagsEntityEvent' => __DIR__ . '/../../..' . '/lib/public/SystemTag/SystemTagsEntityEvent.php',
         'OCP\\SystemTag\\TagAlreadyExistsException' => __DIR__ . '/../../..' . '/lib/public/SystemTag/TagAlreadyExistsException.php',
+        'OCP\\SystemTag\\TagCreationForbiddenException' => __DIR__ . '/../../..' . '/lib/public/SystemTag/TagCreationForbiddenException.php',
         'OCP\\SystemTag\\TagNotFoundException' => __DIR__ . '/../../..' . '/lib/public/SystemTag/TagNotFoundException.php',
         'OCP\\Talk\\Exceptions\\NoBackendException' => __DIR__ . '/../../..' . '/lib/public/Talk/Exceptions/NoBackendException.php',
         'OCP\\Talk\\IBroker' => __DIR__ . '/../../..' . '/lib/public/Talk/IBroker.php',
diff --git a/lib/private/SystemTag/ManagerFactory.php b/lib/private/SystemTag/ManagerFactory.php
index 0fd2ca5fd36..4d3a1396529 100644
--- a/lib/private/SystemTag/ManagerFactory.php
+++ b/lib/private/SystemTag/ManagerFactory.php
@@ -9,7 +9,11 @@ declare(strict_types=1);
 namespace OC\SystemTag;
 
 use OCP\EventDispatcher\IEventDispatcher;
+use OCP\IAppConfig;
+use OCP\IDBConnection;
+use OCP\IGroupManager;
 use OCP\IServerContainer;
+use OCP\IUserSession;
 use OCP\SystemTag\ISystemTagManager;
 use OCP\SystemTag\ISystemTagManagerFactory;
 use OCP\SystemTag\ISystemTagObjectMapper;
@@ -36,9 +40,11 @@ class ManagerFactory implements ISystemTagManagerFactory {
 	 */
 	public function getManager(): ISystemTagManager {
 		return new SystemTagManager(
-			$this->serverContainer->getDatabaseConnection(),
-			$this->serverContainer->getGroupManager(),
+			$this->serverContainer->get(IDBConnection::class),
+			$this->serverContainer->get(IGroupManager::class),
 			$this->serverContainer->get(IEventDispatcher::class),
+			$this->serverContainer->get(IUserSession::class),
+			$this->serverContainer->get(IAppConfig::class),
 		);
 	}
 
@@ -50,7 +56,7 @@ class ManagerFactory implements ISystemTagManagerFactory {
 	 */
 	public function getObjectMapper(): ISystemTagObjectMapper {
 		return new SystemTagObjectMapper(
-			$this->serverContainer->getDatabaseConnection(),
+			$this->serverContainer->get(IDBConnection::class),
 			$this->getManager(),
 			$this->serverContainer->get(IEventDispatcher::class),
 		);
diff --git a/lib/private/SystemTag/SystemTagManager.php b/lib/private/SystemTag/SystemTagManager.php
index 8bbd36c384f..7979b3916f1 100644
--- a/lib/private/SystemTag/SystemTagManager.php
+++ b/lib/private/SystemTag/SystemTagManager.php
@@ -11,13 +11,16 @@ namespace OC\SystemTag;
 use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\EventDispatcher\IEventDispatcher;
+use OCP\IAppConfig;
 use OCP\IDBConnection;
 use OCP\IGroupManager;
 use OCP\IUser;
+use OCP\IUserSession;
 use OCP\SystemTag\ISystemTag;
 use OCP\SystemTag\ISystemTagManager;
 use OCP\SystemTag\ManagerEvent;
 use OCP\SystemTag\TagAlreadyExistsException;
+use OCP\SystemTag\TagCreationForbiddenException;
 use OCP\SystemTag\TagNotFoundException;
 
 /**
@@ -36,6 +39,8 @@ class SystemTagManager implements ISystemTagManager {
 		protected IDBConnection $connection,
 		protected IGroupManager $groupManager,
 		protected IEventDispatcher $dispatcher,
+		private IUserSession $userSession,
+		private IAppConfig $appConfig,
 	) {
 		$query = $this->connection->getQueryBuilder();
 		$this->selectTagQuery = $query->select('*')
@@ -145,7 +150,10 @@ class SystemTagManager implements ISystemTagManager {
 	}
 
 	public function createTag(string $tagName, bool $userVisible, bool $userAssignable): ISystemTag {
-		$tagName = trim($tagName);
+		$user = $this->userSession->getUser();
+		if (!$this->canUserCreateTag($user)) {
+			throw new TagCreationForbiddenException('Tag creation forbidden');
+		}
 		// Length of name column is 64
 		$truncatedTagName = substr($tagName, 0, 64);
 		$query = $this->connection->getQueryBuilder();
@@ -321,6 +329,19 @@ class SystemTagManager implements ISystemTagManager {
 		return false;
 	}
 
+	public function canUserCreateTag(?IUser $user): bool {
+		if ($user === null) {
+			// If no user given, allows only calls from CLI
+			return \OC::$CLI;
+		}
+
+		if ($this->appConfig->getValueBool('systemtags', 'restrict_creation_to_admin', false) === false) {
+			return true;
+		}
+
+		return $this->groupManager->isAdmin($user->getUID());
+	}
+
 	public function canUserSeeTag(ISystemTag $tag, ?IUser $user): bool {
 		// If no user, then we only show public tags
 		if (!$user && $tag->getAccessLevel() === ISystemTag::ACCESS_LEVEL_PUBLIC) {
diff --git a/lib/public/SystemTag/ISystemTagManager.php b/lib/public/SystemTag/ISystemTagManager.php
index 04804798114..66206d677f9 100644
--- a/lib/public/SystemTag/ISystemTagManager.php
+++ b/lib/public/SystemTag/ISystemTagManager.php
@@ -57,8 +57,10 @@ interface ISystemTagManager {
 	 * @return ISystemTag system tag
 	 *
 	 * @throws TagAlreadyExistsException if tag already exists
+	 * @throws TagCreationForbiddenException if user doesn't have the right to create a new tag
 	 *
 	 * @since 9.0.0
+	 * @since 31.0.0 Can throw TagCreationForbiddenExceptionif user doesn't have the right to create a new tag
 	 */
 	public function createTag(string $tagName, bool $userVisible, bool $userAssignable): ISystemTag;
 
@@ -118,6 +120,16 @@ interface ISystemTagManager {
 	public function canUserAssignTag(ISystemTag $tag, ?IUser $user): bool;
 
 	/**
+	 * Checks whether the given user is allowed to create new tags
+	 *
+	 * @param IUser|null $user user to check permission for
+	 * @return bool true if the user is allowed to create a new tag, false otherwise
+	 *
+	 * @since 31.0.0
+	 */
+	public function canUserCreateTag(?IUser $user): bool;
+
+	/**
 	 * Checks whether the given user is allowed to see the tag with the given id.
 	 *
 	 * @param ISystemTag $tag tag to check permission for
diff --git a/lib/public/SystemTag/TagCreationForbiddenException.php b/lib/public/SystemTag/TagCreationForbiddenException.php
new file mode 100644
index 00000000000..eeae38e31f7
--- /dev/null
+++ b/lib/public/SystemTag/TagCreationForbiddenException.php
@@ -0,0 +1,18 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-only
+ */
+
+namespace OCP\SystemTag;
+
+/**
+ * Exception when a user doesn't have the right to create a tag
+ *
+ * @since 31.0.0
+ */
+class TagCreationForbiddenException extends \RuntimeException {
+}
diff --git a/tests/lib/SystemTag/SystemTagManagerTest.php b/tests/lib/SystemTag/SystemTagManagerTest.php
index 99f033cfd95..94103c52cb1 100644
--- a/tests/lib/SystemTag/SystemTagManagerTest.php
+++ b/tests/lib/SystemTag/SystemTagManagerTest.php
@@ -11,9 +11,11 @@ namespace Test\SystemTag;
 use OC\SystemTag\SystemTagManager;
 use OC\SystemTag\SystemTagObjectMapper;
 use OCP\EventDispatcher\IEventDispatcher;
+use OCP\IAppConfig;
 use OCP\IDBConnection;
 use OCP\IGroupManager;
 use OCP\IUser;
+use OCP\IUserSession;
 use OCP\SystemTag\ISystemTag;
 use OCP\SystemTag\ISystemTagManager;
 use Test\TestCase;
@@ -25,25 +27,12 @@ use Test\TestCase;
  * @package Test\SystemTag
  */
 class SystemTagManagerTest extends TestCase {
-	/**
-	 * @var ISystemTagManager
-	 **/
-	private $tagManager;
-
-	/**
-	 * @var IDBConnection
-	 */
-	private $connection;
-
-	/**
-	 * @var IGroupManager
-	 */
-	private $groupManager;
-
-	/**
-	 * @var IEventDispatcher
-	 */
-	private $dispatcher;
+	private ISystemTagManager $tagManager;
+	private IDBConnection $connection;
+	private IGroupManager $groupManager;
+	private IUserSession $userSession;
+	private IAppConfig $appConfig;
+	private IEventDispatcher $dispatcher;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -52,17 +41,22 @@ class SystemTagManagerTest extends TestCase {
 
 		$this->dispatcher = $this->createMock(IEventDispatcher::class);
 		$this->groupManager = $this->createMock(IGroupManager::class);
+		$this->userSession = $this->createMock(IUserSession::class);
+		$this->appConfig = $this->createMock(IAppConfig::class);
 
 		$this->tagManager = new SystemTagManager(
 			$this->connection,
 			$this->groupManager,
-			$this->dispatcher
+			$this->dispatcher,
+			$this->userSession,
+			$this->appConfig,
 		);
 		$this->pruneTagsTables();
 	}
 
 	protected function tearDown(): void {
 		$this->pruneTagsTables();
+		\OC::$CLI = true;
 		parent::tearDown();
 	}
 
@@ -535,6 +529,84 @@ class SystemTagManagerTest extends TestCase {
 		$this->assertEquals([], $this->tagManager->getTagGroups($tag1));
 	}
 
+	private function allowedToCreateProvider(): array {
+		return [
+			[true, null, true],
+			[true, null, false],
+			[false, true, true],
+			[false, true, false],
+			[false, false, false],
+		];
+	}
+
+	/**
+	 * @dataProvider allowedToCreateProvider
+	 */
+	public function testAllowedToCreateTag(bool $isCli, ?bool $isAdmin, bool $isRestricted): void {
+		$oldCli = \OC::$CLI;
+		\OC::$CLI = $isCli;
+
+		$user = $this->getMockBuilder(IUser::class)->getMock();
+		$user->expects($this->any())
+			->method('getUID')
+			->willReturn('test');
+		$this->userSession->expects($this->any())
+			->method('getUser')
+			->willReturn($isAdmin === null ? null : $user);
+		$this->groupManager->expects($this->any())
+			->method('isAdmin')
+			->with('test')
+			->willReturn($isAdmin);
+		$this->appConfig->expects($this->any())
+			->method('getValueBool')
+			->with('systemtags', 'restrict_creation_to_admin')
+			->willReturn($isRestricted);
+
+		$name = uniqid('tag_', true);
+		$tag = $this->tagManager->createTag($name, true, true);
+		$this->assertEquals($tag->getName(), $name);
+		$this->tagManager->deleteTags($tag->getId());
+
+		\OC::$CLI = $oldCli;
+	}
+
+	private function disallowedToCreateProvider(): array {
+		return [
+			[false],
+			[null],
+		];
+	}
+
+	/**
+	 * @dataProvider disallowedToCreateProvider
+	 */
+	public function testDisallowedToCreateTag(?bool $isAdmin): void {
+		$oldCli = \OC::$CLI;
+		\OC::$CLI = false;
+
+		$user = $this->getMockBuilder(IUser::class)->getMock();
+		$user->expects($this->any())
+			->method('getUID')
+			->willReturn('test');
+		$this->userSession->expects($this->any())
+			->method('getUser')
+			->willReturn($isAdmin === null ? null : $user);
+		$this->groupManager->expects($this->any())
+			->method('isAdmin')
+			->with('test')
+			->willReturn($isAdmin);
+		$this->appConfig->expects($this->any())
+			->method('getValueBool')
+			->with('systemtags', 'restrict_creation_to_admin')
+			->willReturn(true);
+
+		$this->expectException(\Exception::class);
+		$tag = $this->tagManager->createTag(uniqid('tag_', true), true, true);
+
+		\OC::$CLI = $oldCli;
+	}
+
+
 	/**
 	 * @param ISystemTag $tag1
 	 * @param ISystemTag $tag2