diff --git a/apps/dav/lib/SystemTag/SystemTagNode.php b/apps/dav/lib/SystemTag/SystemTagNode.php index 36fddcd824..bd21082f78 100644 --- a/apps/dav/lib/SystemTag/SystemTagNode.php +++ b/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) { diff --git a/apps/dav/tests/unit/SystemTag/SystemTagNodeTest.php b/apps/dav/tests/unit/SystemTag/SystemTagNodeTest.php index 43674f4b79..3722bd9d25 100644 --- a/apps/dav/tests/unit/SystemTag/SystemTagNodeTest.php +++ b/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(); } } diff --git a/build/integration/features/tags.feature b/build/integration/features/tags.feature index 3578441908..0c6cd06f9f 100644 --- a/build/integration/features/tags.feature +++ b/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" diff --git a/core/js/systemtags/systemtagsinputfield.js b/core/js/systemtags/systemtagsinputfield.js index 690525c0eb..d5f6bd5f97 100644 --- a/core/js/systemtags/systemtagsinputfield.js +++ b/core/js/systemtags/systemtagsinputfield.js @@ -40,7 +40,9 @@ '
' + ' ' + ' ' + - ' ' + + ' {{#if isAdmin}}' + + ' ' + + ' {{/if}}' + '
'; /** @@ -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'); diff --git a/core/js/tests/specs/systemtags/systemtagsinputfieldSpec.js b/core/js/tests/specs/systemtags/systemtagsinputfieldSpec.js index 503bef7cf2..8d3f67bfa0 100644 --- a/core/js/tests/specs/systemtags/systemtagsinputfieldSpec.js +++ b/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 = $('
'); + 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 = $('
'); + 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(); + }); + }); }); });