Merge pull request nextcloud#62810 from nextcloud/systemtag-update-permissions

fix: require admin permissions for all systemtag updates

Author: AndyScherzinger

apps/dav/lib/SystemTag/SystemTagNode.php

@@ -101,18 +101,10 @@ public function update($name, $userVisible, $userAssignable, $color): void {
 			if (!$this->tagManager->canUserSeeTag($this->tag, $this->user)) {
 				throw new NotFound('Tag with id ' . $this->tag->getId() . ' does not exist');
 			}
-			if (!$this->tagManager->canUserAssignTag($this->tag, $this->user)) {
-				throw new Forbidden('No permission to update tag ' . $this->tag->getId());
-			}
 
-			// only admin is able to change permissions, regular users can only rename
+			// only admin is able to update system tags
 			if (!$this->isAdmin) {
-				// only renaming is allowed for regular users
-				if ($userVisible !== $this->tag->isUserVisible()
-					|| $userAssignable !== $this->tag->isUserAssignable()
-				) {
-					throw new Forbidden('No permission to update permissions for tag ' . $this->tag->getId());
-				}
+				throw new Forbidden('No permission to update tag ' . $this->tag->getId());
 			}
 
 			// Make sure color is a proper hex

apps/dav/tests/unit/SystemTag/SystemTagNodeTest.php

@@ -70,36 +70,43 @@ public static function tagNodeProvider(): array {
 			[
 				true,
 				new SystemTag('1', 'Original', true, true),
-				['Renamed', true, true, null]
+				['Renamed', true, true, null],
+				true,
 			],
 			[
 				true,
 				new SystemTag('1', 'Original', true, true),
-				['Original', false, false, null]
+				['Original', false, false, null],
+				true,
 			],
 			// non-admin
 			[
-				// renaming allowed
+				// renaming not allowed
 				false,
 				new SystemTag('1', 'Original', true, true),
-				['Rename', true, true, '0082c9']
+				['Renamed', true, true, null],
+				false,
 			],
 		];
 	}
 
 	#[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'tagNodeProvider')]
-	public function testUpdateTag(bool $isAdmin, ISystemTag $originalTag, array $changedArgs): void {
-		$this->tagManager->expects($this->once())
-			->method('canUserSeeTag')
+	public function testUpdateTag(bool $isAdmin, ISystemTag $originalTag, $changedArgs, $allowed): void {
+		$this->tagManager->method('canUserSeeTag')
 			->with($originalTag)
 			->willReturn($originalTag->isUserVisible() || $isAdmin);
-		$this->tagManager->expects($this->once())
-			->method('canUserAssignTag')
+		$this->tagManager->method('canUserAssignTag')
 			->with($originalTag)
 			->willReturn($originalTag->isUserAssignable() || $isAdmin);
-		$this->tagManager->expects($this->once())
-			->method('updateTag')
-			->with(1, $changedArgs[0], $changedArgs[1], $changedArgs[2], $changedArgs[3]);
+		if ($allowed) {
+			$this->tagManager->expects($this->once())
+				->method('updateTag')
+				->with(1, $changedArgs[0], $changedArgs[1], $changedArgs[2], $changedArgs[3]);
+		} else {
+			$this->expectException(\Sabre\DAV\Exception\Forbidden::class);
+			$this->tagManager->expects($this->never())
+				->method('updateTag');
+		}
 		$this->getTagNode($isAdmin, $originalTag)
 			->update($changedArgs[0], $changedArgs[1], $changedArgs[2], $changedArgs[3]);
 	}
@@ -186,7 +193,7 @@ public function testUpdateTagAlreadyExists(): void {
 			->method('updateTag')
 			->with(1, 'Renamed', true, true)
 			->willThrowException(new TagAlreadyExistsException());
-		$this->getTagNode(false, $tag)->update('Renamed', true, true, null);
+		$this->getTagNode(true, $tag)->update('Renamed', true, true, null);
 	}
 
 	public function testUpdateTagNotFound(): void {
@@ -205,7 +212,7 @@ public function testUpdateTagNotFound(): void {
 			->method('updateTag')
 			->with(1, 'Renamed', true, true)
 			->willThrowException(new TagNotFoundException());
-		$this->getTagNode(false, $tag)->update('Renamed', true, true, null);
+		$this->getTagNode(true, $tag)->update('Renamed', true, true, null);
 	}
 
 	#[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'adminFlagProvider')]

build/integration/files_features/tags.feature

@@ -36,13 +36,13 @@ Feature: tags
     Then The response should have a status code "400"
     And "0" tags should exist for "user0"
 
-  Scenario: Renaming a normal tag as regular user should work
+  Scenario: Renaming a normal tag as regular user should fail
     Given user "user0" exists
     Given "admin" creates a "normal" tag with name "MySuperAwesomeTagName"
     When "user0" edits the tag with name "MySuperAwesomeTagName" and sets its name to "AnotherTagName"
-    Then The response should have a status code "207"
+    Then The response should have a status code "403"
     And The following tags should exist for "admin"
-      |AnotherTagName|true|true|
+      |MySuperAwesomeTagName|true|true|
 
   Scenario: Renaming a not user-assignable tag as regular user should fail
     Given user "user0" exists