Only allow admins to delete tags

Signed-off-by: Joas Schilling <coding@schilljs.com>

apps/dav/lib/SystemTag/SystemTagNode.php

@@ -157,12 +157,13 @@ class SystemTagNode implements \Sabre\DAV\INode {
 
 	public function delete() {
 		try {
+			if (!$this->isAdmin) {
+				throw new Forbidden('No permission to delete tag ' . $this->tag->getId());
+			}
+
 			if (!$this->tagManager->canUserSeeTag($this->tag, $this->user)) {
 				throw new NotFound('Tag with id ' . $this->tag->getId() . ' not found');
 			}
-			if (!$this->tagManager->canUserAssignTag($this->tag, $this->user)) {
-				throw new Forbidden('No permission to delete tag ' . $this->tag->getId());
-			}
 
 			$this->tagManager->deleteTags($this->tag->getId());
 		} catch (TagNotFoundException $e) {

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

@@ -24,19 +24,17 @@
 
 namespace OCA\DAV\Tests\unit\SystemTag;
 
-use Sabre\DAV\Exception\NotFound;
-use Sabre\DAV\Exception\MethodNotAllowed;
-use Sabre\DAV\Exception\Conflict;
 
 use OC\SystemTag\SystemTag;
 use OCP\SystemTag\TagNotFoundException;
 use OCP\SystemTag\TagAlreadyExistsException;
 use OCP\SystemTag\ISystemTag;
+use Sabre\DAV\Exception\Forbidden;
 
 class SystemTagNodeTest extends \Test\TestCase {
 
 	/**
-	 * @var \OCP\SystemTag\ISystemTagManager
+	 * @var \OCP\SystemTag\ISystemTagManager|\PHPUnit_Framework_MockObject_MockObject
 	 */
 	private $tagManager;
 
@@ -113,7 +111,7 @@ class SystemTagNodeTest extends \Test\TestCase {
 	/**
 	 * @dataProvider tagNodeProvider
 	 */
-	public function testUpdateTag($isAdmin, $originalTag, $changedArgs) {
+	public function testUpdateTag($isAdmin, ISystemTag $originalTag, $changedArgs) {
 		$this->tagManager->expects($this->once())
 			->method('canUserSeeTag')
 			->with($originalTag)
@@ -173,7 +171,7 @@ class SystemTagNodeTest extends \Test\TestCase {
 	/**
 	 * @dataProvider tagNodeProviderPermissionException
 	 */
-	public function testUpdateTagPermissionException($originalTag, $changedArgs, $expectedException = null) {
+	public function testUpdateTagPermissionException(ISystemTag $originalTag, $changedArgs, $expectedException = null) {
 		$this->tagManager->expects($this->any())
 			->method('canUserSeeTag')
 			->with($originalTag)
@@ -242,17 +240,16 @@ class SystemTagNodeTest extends \Test\TestCase {
 	 */
 	public function testDeleteTag($isAdmin) {
 		$tag = new SystemTag(1, 'tag1', true, true);
-		$this->tagManager->expects($this->once())
+		$this->tagManager->expects($isAdmin ? $this->once() : $this->never())
 			->method('canUserSeeTag')
 			->with($tag)
 			->will($this->returnValue(true));
-		$this->tagManager->expects($this->once())
-			->method('canUserAssignTag')
-			->with($tag)
-			->will($this->returnValue(true));
-		$this->tagManager->expects($this->once())
+		$this->tagManager->expects($isAdmin ? $this->once() : $this->never())
 			->method('deleteTags')
 			->with('1');
+		if (!$isAdmin) {
+			$this->setExpectedException(Forbidden::class);
+		}
 		$this->getTagNode($isAdmin, $tag)->delete();
 	}
 
@@ -261,7 +258,7 @@ class SystemTagNodeTest extends \Test\TestCase {
 			[
 				// cannot delete invisible tag
 				new SystemTag(1, 'Original', false, true),
-				'Sabre\DAV\Exception\NotFound',
+				'Sabre\DAV\Exception\Forbidden',
 			],
 			[
 				// cannot delete non-assignable tag
@@ -279,20 +276,11 @@ class SystemTagNodeTest extends \Test\TestCase {
 			->method('canUserSeeTag')
 			->with($tag)
 			->will($this->returnValue($tag->isUserVisible()));
-		$this->tagManager->expects($this->any())
-			->method('canUserAssignTag')
-			->with($tag)
-			->will($this->returnValue($tag->isUserAssignable()));
 		$this->tagManager->expects($this->never())
 			->method('deleteTags');
 
-		try {
-			$this->getTagNode(false, $tag)->delete();
-		} catch (\Exception $e) {
-			$thrown = $e;
-		}
-
-		$this->assertInstanceOf($expectedException, $thrown);
+		$this->setExpectedException($expectedException);
+		$this->getTagNode(false, $tag)->delete();
 	}
 
 	/**
@@ -304,14 +292,10 @@ class SystemTagNodeTest extends \Test\TestCase {
 			->method('canUserSeeTag')
 			->with($tag)
 			->will($this->returnValue($tag->isUserVisible()));
-		$this->tagManager->expects($this->any())
-			->method('canUserAssignTag')
-			->with($tag)
-			->will($this->returnValue($tag->isUserAssignable()));
 		$this->tagManager->expects($this->once())
 			->method('deleteTags')
 			->with('1')
 			->will($this->throwException(new TagNotFoundException()));
-		$this->getTagNode(false, $tag)->delete();
+		$this->getTagNode(true, $tag)->delete();
 	}
 }

build/integration/features/tags.feature

@@ -70,12 +70,13 @@ Feature: tags
     When "user0" edits the tag with name "TagWithGroups" and sets its groups to "group1|group3"
     Then The response should have a status code "403"
 
-  Scenario: Deleting a normal tag as regular user should work
+  Scenario: Deleting a normal tag as regular user should fail
     Given user "user0" exists
     Given "admin" creates a "normal" tag with name "MySuperAwesomeTagName"
     When "user0" deletes the tag with name "MySuperAwesomeTagName"
-    Then The response should have a status code "204"
-    And "0" tags should exist for "admin"
+    Then The response should have a status code "403"
+    And The following tags should exist for "admin"
+      |MySuperAwesomeTagName|true|true|
 
   Scenario: Deleting a not user-assignable tag as regular user should fail
     Given user "user0" exists
@@ -93,6 +94,12 @@ Feature: tags
     And The following tags should exist for "admin"
       |MySuperAwesomeTagName|false|true|
 
+  Scenario: Deleting a normal tag as admin should work
+    Given "admin" creates a "normal" tag with name "MySuperAwesomeTagName"
+    When "admin" deletes the tag with name "MySuperAwesomeTagName"
+    Then The response should have a status code "204"
+    And "0" tags should exist for "admin"
+
   Scenario: Deleting a not user-assignable tag as admin should work
     Given "admin" creates a "not user-assignable" tag with name "MySuperAwesomeTagName"
     When "admin" deletes the tag with name "MySuperAwesomeTagName"

core/js/systemtags/systemtagsinputfield.js

@@ -40,7 +40,9 @@
 		'<form class="systemtags-rename-form">' +
 		'    <label class="hidden-visually" for="{{cid}}-rename-input">{{renameLabel}}</label>' +
 		'    <input id="{{cid}}-rename-input" type="text" value="{{name}}">' +
-		'    <a href="#" class="delete icon icon-delete" title="{{deleteTooltip}}"></a>' +
+		'    {{#if isAdmin}}' +
+		'      <a href="#" class="delete icon icon-delete" title="{{deleteTooltip}}"></a>' +
+		'    {{/if}}' +
 		'</form>';
 
 	/**
@@ -148,7 +150,8 @@
 				cid: this.cid,
 				name: oldName,
 				deleteTooltip: t('core', 'Delete'),
-				renameLabel: t('core', 'Rename')
+				renameLabel: t('core', 'Rename'),
+				isAdmin: this._isAdmin
 			}));
 			$item.find('.label').after($renameForm);
 			$item.find('.label, .systemtags-actions').addClass('hidden');