From 6b2bd29579caa4e095c0cd855d8c1f0faddd5013 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 27 Jul 2026 17:49:09 +0200 Subject: [PATCH 1/2] fix: require admin permissions for all systemtag updates Signed-off-by: Robin Appelman --- apps/dav/lib/SystemTag/SystemTagNode.php | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/apps/dav/lib/SystemTag/SystemTagNode.php b/apps/dav/lib/SystemTag/SystemTagNode.php index 2341d4823baa8..d0fa18440210d 100644 --- a/apps/dav/lib/SystemTag/SystemTagNode.php +++ b/apps/dav/lib/SystemTag/SystemTagNode.php @@ -98,18 +98,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 From 96ff6243f47bfde80fbec61128af7b84e31e7527 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 28 Jul 2026 22:04:56 +0200 Subject: [PATCH 2/2] test: adjust tests to new tag rename rule Signed-off-by: Robin Appelman --- .../unit/SystemTag/SystemTagNodeTest.php | 35 +++++++++++-------- build/integration/files_features/tags.feature | 6 ++-- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/apps/dav/tests/unit/SystemTag/SystemTagNodeTest.php b/apps/dav/tests/unit/SystemTag/SystemTagNodeTest.php index 594b5e15db6cf..8eb8b671778b1 100644 --- a/apps/dav/tests/unit/SystemTag/SystemTagNodeTest.php +++ b/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('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]); } @@ -187,7 +194,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); } @@ -207,7 +214,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('adminFlagProvider')] diff --git a/build/integration/files_features/tags.feature b/build/integration/files_features/tags.feature index fef8068cbc809..f7da05edfcc67 100644 --- a/build/integration/files_features/tags.feature +++ b/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