Merge pull request #62920 from nextcloud/backport/62810/stable30

[stable30] fix: require admin permissions for all systemtag updates

Author: sorbaugh

apps/dav/lib/SystemTag/SystemTagNode.php

@@ -112,18 +112,10 @@ public function update($name, $userVisible, $userAssignable): 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());
 			}
 
 			$this->tagManager->updateTag($this->tag->getId(), $name, $userVisible, $userAssignable);

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

@@ -75,38 +75,45 @@ public function tagNodeProvider() {
 			[
 				true,
 				new SystemTag(1, 'Original', true, true),
-				['Renamed', true, true]
+				['Renamed', true, true],
+				true,
 			],
 			[
 				true,
 				new SystemTag(1, 'Original', true, true),
-				['Original', false, false]
+				['Original', false, false],
+				true,
 			],
 			// non-admin
 			[
-				// renaming allowed
+				// renaming not allowed
 				false,
 				new SystemTag(1, 'Original', true, true),
-				['Rename', true, true]
+				['Renamed', true, true],
+				false,
 			],
 		];
 	}
 
 	/**
 	 * @dataProvider tagNodeProvider
 	 */
-	public function testUpdateTag($isAdmin, ISystemTag $originalTag, $changedArgs): void {
-		$this->tagManager->expects($this->once())
-			->method('canUserSeeTag')
+	public function testUpdateTag($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]);
+		if ($allowed) {
+			$this->tagManager->expects($this->once())
+				->method('updateTag')
+				->with(1, $changedArgs[0], $changedArgs[1], $changedArgs[2]);
+		} 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]);
 	}
@@ -196,7 +203,7 @@ public function testUpdateTagAlreadyExists(): void {
 			->method('updateTag')
 			->with(1, 'Renamed', true, true)
 			->will($this->throwException(new TagAlreadyExistsException()));
-		$this->getTagNode(false, $tag)->update('Renamed', true, true);
+		$this->getTagNode(true, $tag)->update('Renamed', true, true);
 	}
 
 
@@ -216,7 +223,7 @@ public function testUpdateTagNotFound(): void {
 			->method('updateTag')
 			->with(1, 'Renamed', true, true)
 			->will($this->throwException(new TagNotFoundException()));
-		$this->getTagNode(false, $tag)->update('Renamed', true, true);
+		$this->getTagNode(true, $tag)->update('Renamed', true, true);
 	}
 
 	/**

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