Merge pull request #2512 from nextcloud/cleanup-system-tag-usage

Only allow admins to delete tags

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())
+		$this->tagManager->expects($isAdmin ? $this->once() : $this->never())
-			->method('canUserAssignTag')
-			->with($tag)
-			->will($this->returnValue(true));
-		$this->tagManager->expects($this->once())
 			->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->setExpectedException($expectedException);
 		$this->getTagNode(false, $tag)->delete();
-		} catch (\Exception $e) {
-			$thrown = $e;
-		}
-
-		$this->assertInstanceOf($expectedException, $thrown);
 	}
 
 	/**
@@ -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"
+    Then The response should have a status code "403"
-    And "0" tags should exist for "admin"
+    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}}">' +
+		'    {{#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');

core/js/tests/specs/systemtags/systemtagsinputfieldSpec.js

@@ -267,20 +267,6 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() {
 
 				saveStub.restore();
 			});
-			it('deletes model and submits change when clicking delete', function() {
-				var destroyStub = sinon.stub(OC.SystemTags.SystemTagModel.prototype, 'destroy');
-
-				expect($dropdown.find('.delete').length).toEqual(0);
-				$dropdown.find('.rename').mouseup();
-				// delete button appears
-				expect($dropdown.find('.delete').length).toEqual(1);
-				$dropdown.find('.delete').mouseup();
-
-				expect(destroyStub.calledOnce).toEqual(true);
-				expect(destroyStub.calledOn(view.collection.get('1')));
-
-				destroyStub.restore();
-			});
 		});
 		describe('setting data', function() {
 			it('sets value when calling setValues', function() {
@@ -299,12 +285,18 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() {
 	});
 
 	describe('as admin', function() {
+		var $dropdown;
+
 		beforeEach(function() {
 			view = new OC.SystemTags.SystemTagsInputField({
 				isAdmin: true
 			});
-			view.render();
 			$('.testInputContainer').append(view.$el);
+			$dropdown = $('<div class="select2-dropdown"></div>');
+			select2Stub.withArgs('dropdown').returns($dropdown);
+			$('#testArea').append($dropdown);
+
+			view.render();
 		});
 		it('formatResult renders tag name with visibility', function() {
 			var opts = select2Stub.getCall(0).args[0];
@@ -431,15 +423,50 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() {
 				]);
 			});
 		});
+		describe('tag actions', function() {
+			var opts;
+
+			beforeEach(function() {
+
+				opts = select2Stub.getCall(0).args[0];
+
+				view.collection.add([
+					new OC.SystemTags.SystemTagModel({id: '1', name: 'abc'}),
+				]);
+
+				$dropdown.append(opts.formatResult(view.collection.get('1').toJSON()));
+
+			});
+			it('deletes model and submits change when clicking delete', function() {
+				var destroyStub = sinon.stub(OC.SystemTags.SystemTagModel.prototype, 'destroy');
+
+				expect($dropdown.find('.delete').length).toEqual(0);
+				$dropdown.find('.rename').mouseup();
+				// delete button appears
+				expect($dropdown.find('.delete').length).toEqual(1);
+				$dropdown.find('.delete').mouseup();
+
+				expect(destroyStub.calledOnce).toEqual(true);
+				expect(destroyStub.calledOn(view.collection.get('1')));
+
+				destroyStub.restore();
+			});
+		});
 	});
 
 	describe('as user', function() {
+		var $dropdown;
+
 		beforeEach(function() {
 			view = new OC.SystemTags.SystemTagsInputField({
 				isAdmin: false
 			});
-			view.render();
 			$('.testInputContainer').append(view.$el);
+			$dropdown = $('<div class="select2-dropdown"></div>');
+			select2Stub.withArgs('dropdown').returns($dropdown);
+			$('#testArea').append($dropdown);
+
+			view.render();
 		});
 		it('formatResult renders tag name only', function() {
 			var opts = select2Stub.getCall(0).args[0];
@@ -570,5 +597,33 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() {
 				]);
 			});
 		});
+		describe('tag actions', function() {
+			var opts;
+
+			beforeEach(function() {
+
+				opts = select2Stub.getCall(0).args[0];
+
+				view.collection.add([
+					new OC.SystemTags.SystemTagModel({id: '1', name: 'abc'}),
+				]);
+
+				$dropdown.append(opts.formatResult(view.collection.get('1').toJSON()));
+
+			});
+			it('deletes model and submits change when clicking delete', function() {
+				var destroyStub = sinon.stub(OC.SystemTags.SystemTagModel.prototype, 'destroy');
+
+				expect($dropdown.find('.delete').length).toEqual(0);
+				$dropdown.find('.rename').mouseup();
+				// delete button appears only for admins
+				expect($dropdown.find('.delete').length).toEqual(0);
+				$dropdown.find('.delete').mouseup();
+
+				expect(destroyStub.notCalled).toEqual(true);
+
+				destroyStub.restore();
+			});
+		});
 	});
 });