From 88740f035d733d170317d147e15dfceec0f3282f Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 11 May 2016 16:47:33 +0200 Subject: [PATCH] Act on effective system tag canAssign permission Whenever the server returns true for the can-assign Webdav property of a system tag, it means the current user is allowed to assign, regardless of the value of user-assignable. This commit brings the proper logic to the web UI to make it possible for users to assign when they have the permission. --- apps/dav/lib/SystemTag/SystemTagPlugin.php | 9 ++- apps/systemtags/js/systemtagsinfoview.js | 2 +- .../tests/js/systemtagsinfoviewSpec.js | 43 ++++++++--- core/js/systemtags/systemtagmodel.js | 10 ++- core/js/systemtags/systemtagsinputfield.js | 8 ++- .../systemtags/systemtagsinputfieldSpec.js | 71 ++++++++++++++----- lib/private/SystemTag/SystemTagManager.php | 2 +- tests/lib/SystemTag/SystemTagManagerTest.php | 1 + 8 files changed, 109 insertions(+), 37 deletions(-) diff --git a/apps/dav/lib/SystemTag/SystemTagPlugin.php b/apps/dav/lib/SystemTag/SystemTagPlugin.php index 0b44287371..3943d37af8 100644 --- a/apps/dav/lib/SystemTag/SystemTagPlugin.php +++ b/apps/dav/lib/SystemTag/SystemTagPlugin.php @@ -53,6 +53,7 @@ class SystemTagPlugin extends \Sabre\DAV\ServerPlugin { const DISPLAYNAME_PROPERTYNAME = '{http://owncloud.org/ns}display-name'; const USERVISIBLE_PROPERTYNAME = '{http://owncloud.org/ns}user-visible'; const USERASSIGNABLE_PROPERTYNAME = '{http://owncloud.org/ns}user-assignable'; + const CANASSIGN_PROPERTYNAME = '{http://owncloud.org/ns}can-assign'; /** * @var \Sabre\DAV\Server $server @@ -206,7 +207,7 @@ class SystemTagPlugin extends \Sabre\DAV\ServerPlugin { PropFind $propFind, \Sabre\DAV\INode $node ) { - if (!($node instanceof SystemTagNode)) { + if (!($node instanceof SystemTagNode) && !($node instanceof SystemTagMappingNode)) { return; } @@ -223,8 +224,14 @@ class SystemTagPlugin extends \Sabre\DAV\ServerPlugin { }); $propFind->handle(self::USERASSIGNABLE_PROPERTYNAME, function() use ($node) { + // this is the tag's inherent property "is user assignable" return $node->getSystemTag()->isUserAssignable() ? 'true' : 'false'; }); + + $propFind->handle(self::CANASSIGN_PROPERTYNAME, function() use ($node) { + // this is the effective permission for the current user + return $this->tagManager->canUserAssignTag($node->getSystemTag(), $this->userSession->getUser()) ? 'true' : 'false'; + }); } /** diff --git a/apps/systemtags/js/systemtagsinfoview.js b/apps/systemtags/js/systemtagsinfoview.js index 2f01ca7db3..a7320a3956 100644 --- a/apps/systemtags/js/systemtagsinfoview.js +++ b/apps/systemtags/js/systemtagsinfoview.js @@ -12,7 +12,7 @@ function modelToSelection(model) { var data = model.toJSON(); - if (!OC.isUserAdmin() && !data.userAssignable) { + if (!OC.isUserAdmin() && !data.canAssign) { data.locked = true; } return data; diff --git a/apps/systemtags/tests/js/systemtagsinfoviewSpec.js b/apps/systemtags/tests/js/systemtagsinfoviewSpec.js index 27724822c2..449dfd859d 100644 --- a/apps/systemtags/tests/js/systemtagsinfoviewSpec.js +++ b/apps/systemtags/tests/js/systemtagsinfoviewSpec.js @@ -62,9 +62,9 @@ describe('OCA.SystemTags.SystemTagsInfoView tests', function() { fetchStub.yieldTo('success', view.selectedTagsCollection); expect(setDataStub.calledOnce).toEqual(true); expect(setDataStub.getCall(0).args[0]).toEqual([{ - id: '1', name: 'test1', userVisible: true, userAssignable: true + id: '1', name: 'test1', userVisible: true, userAssignable: true, canAssign: true }, { - id: '3', name: 'test3', userVisible: true, userAssignable: true + id: '3', name: 'test3', userVisible: true, userAssignable: true, canAssign: true }]); expect(view.$el.hasClass('hidden')).toEqual(false); @@ -79,7 +79,7 @@ describe('OCA.SystemTags.SystemTagsInfoView tests', function() { view = new OCA.SystemTags.SystemTagsInfoView(); view.selectedTagsCollection.add([ {id: '1', name: 'test1'}, - {id: '3', name: 'test3', userVisible: false, userAssignable: false} + {id: '3', name: 'test3', userVisible: false, userAssignable: false, canAssign: false} ]); var callback = sinon.stub(); @@ -87,9 +87,9 @@ describe('OCA.SystemTags.SystemTagsInfoView tests', function() { expect(callback.calledOnce).toEqual(true); expect(callback.getCall(0).args[0]).toEqual([{ - id: '1', name: 'test1', userVisible: true, userAssignable: true + id: '1', name: 'test1', userVisible: true, userAssignable: true, canAssign: true }, { - id: '3', name: 'test3', userVisible: false, userAssignable: false + id: '3', name: 'test3', userVisible: false, userAssignable: false, canAssign: false }]); inputViewSpy.restore(); @@ -103,7 +103,7 @@ describe('OCA.SystemTags.SystemTagsInfoView tests', function() { view = new OCA.SystemTags.SystemTagsInfoView(); view.selectedTagsCollection.add([ {id: '1', name: 'test1'}, - {id: '3', name: 'test3', userAssignable: false} + {id: '3', name: 'test3', userAssignable: false, canAssign: false} ]); var callback = sinon.stub(); @@ -111,9 +111,33 @@ describe('OCA.SystemTags.SystemTagsInfoView tests', function() { expect(callback.calledOnce).toEqual(true); expect(callback.getCall(0).args[0]).toEqual([{ - id: '1', name: 'test1', userVisible: true, userAssignable: true + id: '1', name: 'test1', userVisible: true, userAssignable: true, canAssign: true }, { - id: '3', name: 'test3', userVisible: true, userAssignable: false, locked: true + id: '3', name: 'test3', userVisible: true, userAssignable: false, canAssign: false, locked: true + }]); + + inputViewSpy.restore(); + }); + it('does not set locked flag on non-assignable tags when canAssign overrides it with true', function() { + isAdminStub.returns(false); + + var inputViewSpy = sinon.spy(OC.SystemTags, 'SystemTagsInputField'); + var element = $(''); + view.remove(); + view = new OCA.SystemTags.SystemTagsInfoView(); + view.selectedTagsCollection.add([ + {id: '1', name: 'test1'}, + {id: '4', name: 'test4', userAssignable: false, canAssign: true} + ]); + + var callback = sinon.stub(); + inputViewSpy.getCall(0).args[0].initSelection(element, callback); + + expect(callback.calledOnce).toEqual(true); + expect(callback.getCall(0).args[0]).toEqual([{ + id: '1', name: 'test1', userVisible: true, userAssignable: true, canAssign: true + }, { + id: '4', name: 'test4', userVisible: true, userAssignable: false, canAssign: true }]); inputViewSpy.restore(); @@ -152,7 +176,8 @@ describe('OCA.SystemTags.SystemTagsInfoView tests', function() { id: '2', name: 'test2', userVisible: true, - userAssignable: true + userAssignable: true, + canAssign: true }); createStub.restore(); diff --git a/core/js/systemtags/systemtagmodel.js b/core/js/systemtags/systemtagmodel.js index b41fbdde61..89728357e2 100644 --- a/core/js/systemtags/systemtagmodel.js +++ b/core/js/systemtags/systemtagmodel.js @@ -23,14 +23,17 @@ defaults: { userVisible: true, - userAssignable: true + userAssignable: true, + canAssign: true }, davProperties: { 'id': '{' + NS_OWNCLOUD + '}id', 'name': '{' + NS_OWNCLOUD + '}display-name', 'userVisible': '{' + NS_OWNCLOUD + '}user-visible', - 'userAssignable': '{' + NS_OWNCLOUD + '}user-assignable' + 'userAssignable': '{' + NS_OWNCLOUD + '}user-assignable', + // read-only, effective permissions computed by the server, + 'canAssign': '{' + NS_OWNCLOUD + '}can-assign' }, parse: function(data) { @@ -38,7 +41,8 @@ id: data.id, name: data.name, userVisible: data.userVisible === true || data.userVisible === 'true', - userAssignable: data.userAssignable === true || data.userAssignable === 'true' + userAssignable: data.userAssignable === true || data.userAssignable === 'true', + canAssign: data.canAssign === true || data.canAssign === 'true' }; } }); diff --git a/core/js/systemtags/systemtagsinputfield.js b/core/js/systemtags/systemtagsinputfield.js index 45dc5b7b03..2eb8d0a44c 100644 --- a/core/js/systemtags/systemtagsinputfield.js +++ b/core/js/systemtags/systemtagsinputfield.js @@ -206,7 +206,8 @@ tag = this.collection.create({ name: e.object.name.trim(), userVisible: true, - userAssignable: true + userAssignable: true, + canAssign: true }, { success: function(model) { self._addToSelect2Selection(model.toJSON()); @@ -263,7 +264,7 @@ var tagModels = collection.filterByName(query.term.trim()); if (!self._isAdmin) { tagModels = _.filter(tagModels, function(tagModel) { - return tagModel.get('userAssignable'); + return tagModel.get('canAssign'); }); } query.callback({ @@ -331,6 +332,7 @@ name: term, userAssignable: true, userVisible: true, + canAssign: true, isNew: true }; } else { @@ -346,7 +348,7 @@ function modelToSelection(model) { var data = model.toJSON(); - if (!self._isAdmin && !data.userAssignable) { + if (!self._isAdmin && !data.canAssign) { // lock static tags for non-admins data.locked = true; } diff --git a/core/js/tests/specs/systemtags/systemtagsinputfieldSpec.js b/core/js/tests/specs/systemtags/systemtagsinputfieldSpec.js index 22bf0d2c82..cafc6e7842 100644 --- a/core/js/tests/specs/systemtags/systemtagsinputfieldSpec.js +++ b/core/js/tests/specs/systemtags/systemtagsinputfieldSpec.js @@ -68,7 +68,7 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { view.collection.add([ new OC.SystemTags.SystemTagModel({id: '1', name: 'abc'}), new OC.SystemTags.SystemTagModel({id: '2', name: 'def'}), - new OC.SystemTags.SystemTagModel({id: '3', name: 'abd', userAssignable: false}), + new OC.SystemTags.SystemTagModel({id: '3', name: 'abd', userAssignable: false, canAssign: false}), ]); }); it('does not create dummy tag when user types non-matching name', function() { @@ -84,6 +84,7 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { expect(result.isNew).toEqual(true); expect(result.userVisible).toEqual(true); expect(result.userAssignable).toEqual(true); + expect(result.canAssign).toEqual(true); }); it('creates dummy tag when user types non-matching name even with prefix of existing tag', function() { var opts = select2Stub.getCall(0).args[0]; @@ -93,6 +94,7 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { expect(result.isNew).toEqual(true); expect(result.userVisible).toEqual(true); expect(result.userAssignable).toEqual(true); + expect(result.canAssign).toEqual(true); }); it('creates the real tag and fires select event after user selects the dummy tag', function() { var selectHandler = sinon.stub(); @@ -110,14 +112,16 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { expect(createStub.getCall(0).args[0]).toEqual({ name: 'newname', userVisible: true, - userAssignable: true + userAssignable: true, + canAssign: true }); var newModel = new OC.SystemTags.SystemTagModel({ id: '123', name: 'newname', userVisible: true, - userAssignable: true + userAssignable: true, + canAssign: true }); // not called yet @@ -186,7 +190,8 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { expect(createStub.getCall(0).args[0]).toEqual({ name: 'newname', userVisible: true, - userAssignable: true + userAssignable: true, + canAssign: true }); var newModel = new OC.SystemTags.SystemTagModel({ @@ -320,7 +325,8 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { testTags = [ new OC.SystemTags.SystemTagModel({id: '1', name: 'test1'}), new OC.SystemTags.SystemTagModel({id: '2', name: 'test2'}), - new OC.SystemTags.SystemTagModel({id: '3', name: 'test3', userAssignable: false}), + new OC.SystemTags.SystemTagModel({id: '3', name: 'test3', userAssignable: false, canAssign: false}), + new OC.SystemTags.SystemTagModel({id: '4', name: 'test4', userAssignable: false, canAssign: true}) ]; }); afterEach(function() { @@ -328,7 +334,7 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { }); it('grabs values from the full collection', function() { var $el = view.$el.find('input'); - $el.val('1,3'); + $el.val('1,3,4'); var opts = select2Stub.getCall(0).args[0]; var callback = sinon.stub(); opts.initSelection($el, callback); @@ -339,11 +345,16 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { expect(callback.calledOnce).toEqual(true); var models = callback.getCall(0).args[0]; - expect(models.length).toEqual(2); + expect(models.length).toEqual(3); expect(models[0].id).toEqual('1'); expect(models[0].name).toEqual('test1'); + expect(models[0].locked).toBeFalsy(); expect(models[1].id).toEqual('3'); expect(models[1].name).toEqual('test3'); + expect(models[1].locked).toBeFalsy(); + expect(models[2].id).toEqual('4'); + expect(models[2].name).toEqual('test4'); + expect(models[2].locked).toBeFalsy(); }); }); describe('autocomplete', function() { @@ -356,7 +367,7 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { view.collection.add([ new OC.SystemTags.SystemTagModel({id: '1', name: 'abc'}), new OC.SystemTags.SystemTagModel({id: '2', name: 'def'}), - new OC.SystemTags.SystemTagModel({id: '3', name: 'abd', userAssignable: false}), + new OC.SystemTags.SystemTagModel({id: '3', name: 'abd', userAssignable: false, canAssign: false}), new OC.SystemTags.SystemTagModel({id: '4', name: 'Deg'}), ]); }); @@ -379,13 +390,15 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { id: '1', name: 'abc', userVisible: true, - userAssignable: true + userAssignable: true, + canAssign: true }, { id: '3', name: 'abd', userVisible: true, - userAssignable: false + userAssignable: false, + canAssign: false } ]); }); @@ -405,13 +418,15 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { id: '2', name: 'def', userVisible: true, - userAssignable: true + userAssignable: true, + canAssign: true }, { id: '4', name: 'Deg', userVisible: true, - userAssignable: true + userAssignable: true, + canAssign: true } ]); }); @@ -445,7 +460,8 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { testTags = [ new OC.SystemTags.SystemTagModel({id: '1', name: 'test1'}), new OC.SystemTags.SystemTagModel({id: '2', name: 'test2'}), - new OC.SystemTags.SystemTagModel({id: '3', name: 'test3', userAssignable: false}), + new OC.SystemTags.SystemTagModel({id: '3', name: 'test3', userAssignable: false, canAssign: false}), + new OC.SystemTags.SystemTagModel({id: '4', name: 'test4', userAssignable: false, canAssign: true}) ]; view.render(); }); @@ -454,7 +470,7 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { }); it('grabs values from the full collection', function() { var $el = view.$el.find('input'); - $el.val('1,3'); + $el.val('1,3,4'); var opts = select2Stub.getCall(0).args[0]; var callback = sinon.stub(); opts.initSelection($el, callback); @@ -465,11 +481,17 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { expect(callback.calledOnce).toEqual(true); var models = callback.getCall(0).args[0]; - expect(models.length).toEqual(2); + expect(models.length).toEqual(3); expect(models[0].id).toEqual('1'); expect(models[0].name).toEqual('test1'); + expect(models[0].locked).toBeFalsy(); expect(models[1].id).toEqual('3'); expect(models[1].name).toEqual('test3'); + // not assignable / cannot assign locks the entry + expect(models[1].locked).toEqual(true); + expect(models[2].id).toEqual('4'); + expect(models[2].name).toEqual('test4'); + expect(models[2].locked).toBeFalsy(); }); }); describe('autocomplete', function() { @@ -483,8 +505,9 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { view.collection.add([ new OC.SystemTags.SystemTagModel({id: '1', name: 'abc'}), new OC.SystemTags.SystemTagModel({id: '2', name: 'def'}), - new OC.SystemTags.SystemTagModel({id: '3', name: 'abd', userAssignable: false}), + new OC.SystemTags.SystemTagModel({id: '3', name: 'abd', userAssignable: false, canAssign: false}), new OC.SystemTags.SystemTagModel({id: '4', name: 'Deg'}), + new OC.SystemTags.SystemTagModel({id: '5', name: 'abe', userAssignable: false, canAssign: true}) ]); }); afterEach(function() { @@ -506,7 +529,15 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { id: '1', name: 'abc', userVisible: true, - userAssignable: true + userAssignable: true, + canAssign: true + }, + { + id: '5', + name: 'abe', + userVisible: true, + userAssignable: false, + canAssign: true } ]); }); @@ -526,13 +557,15 @@ describe('OC.SystemTags.SystemTagsInputField tests', function() { id: '2', name: 'def', userVisible: true, - userAssignable: true + userAssignable: true, + canAssign: true }, { id: '4', name: 'Deg', userVisible: true, - userAssignable: true + userAssignable: true, + canAssign: true } ]); }); diff --git a/lib/private/SystemTag/SystemTagManager.php b/lib/private/SystemTag/SystemTagManager.php index 832afc2a11..e0bd83603d 100644 --- a/lib/private/SystemTag/SystemTagManager.php +++ b/lib/private/SystemTag/SystemTagManager.php @@ -350,7 +350,7 @@ class SystemTagManager implements ISystemTagManager { return false; } - $groupIds = $this->groupManager->getUserGroupIds($user->getUID()); + $groupIds = $this->groupManager->getUserGroupIds($user); if (!empty($groupIds)) { $matchingGroups = array_intersect($groupIds, $this->getTagGroups($tag)); if (!empty($matchingGroups)) { diff --git a/tests/lib/SystemTag/SystemTagManagerTest.php b/tests/lib/SystemTag/SystemTagManagerTest.php index 04f49eff96..e697e37346 100644 --- a/tests/lib/SystemTag/SystemTagManagerTest.php +++ b/tests/lib/SystemTag/SystemTagManagerTest.php @@ -493,6 +493,7 @@ class SystemTagManagerTest extends TestCase { ->will($this->returnValue($isAdmin)); $this->groupManager->expects($this->any()) ->method('getUserGroupIds') + ->with($user) ->will($this->returnValue($userGroupIds)); $this->assertEquals($expectedResult, $this->tagManager->canUserAssignTag($tag1, $user));