diff options
author | Vincent Petry <pvince81@owncloud.com> | 2016-05-21 19:06:54 +0200 |
---|---|---|
committer | Vincent Petry <pvince81@owncloud.com> | 2016-05-21 19:06:54 +0200 |
commit | 693484008360b219848d4fbcdce9b7f26987850b (patch) | |
tree | be128b41a2717b4fe9814c08f40d63d7913dca92 | |
parent | bca7586574dc3daf6a890688fdf7634f6fa58bab (diff) | |
parent | b40c0bad9685873bbcd61f310717c0da0c9faf75 (diff) | |
download | nextcloud-server-693484008360b219848d4fbcdce9b7f26987850b.tar.gz nextcloud-server-693484008360b219848d4fbcdce9b7f26987850b.zip |
Merge pull request #24307 from owncloud/systemtags-perminterface
Add interface methods for permission check for system tags
30 files changed, 1305 insertions, 414 deletions
diff --git a/apps/dav/lib/SystemTag/SystemTagMappingNode.php b/apps/dav/lib/SystemTag/SystemTagMappingNode.php index bb2936c13dc..09a3c79c757 100644 --- a/apps/dav/lib/SystemTag/SystemTagMappingNode.php +++ b/apps/dav/lib/SystemTag/SystemTagMappingNode.php @@ -24,21 +24,22 @@ namespace OCA\DAV\SystemTag; use Sabre\DAV\Exception\NotFound; use Sabre\DAV\Exception\Forbidden; +use Sabre\DAV\Exception\MethodNotAllowed; use OCP\SystemTag\ISystemTag; use OCP\SystemTag\ISystemTagManager; use OCP\SystemTag\ISystemTagObjectMapper; use OCP\SystemTag\TagNotFoundException; +use OCP\IUser; /** * Mapping node for system tag to object id */ -class SystemTagMappingNode extends SystemTagNode { - +class SystemTagMappingNode implements \Sabre\DAV\INode { /** - * @var ISystemTagObjectMapper + * @var ISystemTag */ - private $tagMapper; + protected $tag; /** * @var string @@ -51,12 +52,29 @@ class SystemTagMappingNode extends SystemTagNode { private $objectType; /** + * User + * + * @var IUser + */ + protected $user; + + /** + * @var ISystemTagManager + */ + protected $tagManager; + + /** + * @var ISystemTagObjectMapper + */ + private $tagMapper; + + /** * Sets up the node, expects a full path name * * @param ISystemTag $tag system tag * @param string $objectId * @param string $objectType - * @param bool $isAdmin whether to allow permissions for admin + * @param IUser $user user * @param ISystemTagManager $tagManager * @param ISystemTagObjectMapper $tagMapper */ @@ -64,14 +82,16 @@ class SystemTagMappingNode extends SystemTagNode { ISystemTag $tag, $objectId, $objectType, - $isAdmin, + IUser $user, ISystemTagManager $tagManager, ISystemTagObjectMapper $tagMapper ) { + $this->tag = $tag; $this->objectId = $objectId; $this->objectType = $objectType; + $this->user = $user; + $this->tagManager = $tagManager; $this->tagMapper = $tagMapper; - parent::__construct($tag, $isAdmin, $tagManager); } /** @@ -93,17 +113,52 @@ class SystemTagMappingNode extends SystemTagNode { } /** + * Returns the system tag represented by this node + * + * @return ISystemTag system tag + */ + public function getSystemTag() { + return $this->tag; + } + + /** + * Returns the id of the tag + * + * @return string + */ + public function getName() { + return $this->tag->getId(); + } + + /** + * Renames the node + * + * @param string $name The new name + * + * @throws MethodNotAllowed not allowed to rename node + */ + public function setName($name) { + throw new MethodNotAllowed(); + } + + /** + * Returns null, not supported + * + */ + public function getLastModified() { + return null; + } + + /** * Delete tag to object association */ public function delete() { try { - if (!$this->isAdmin) { - if (!$this->tag->isUserVisible()) { - throw new NotFound('Tag with id ' . $this->tag->getId() . ' not found'); - } - if (!$this->tag->isUserAssignable()) { - throw new Forbidden('No permission to unassign 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 unassign tag ' . $this->tag->getId()); } $this->tagMapper->unassignTags($this->objectId, $this->objectType, $this->tag->getId()); } catch (TagNotFoundException $e) { diff --git a/apps/dav/lib/SystemTag/SystemTagNode.php b/apps/dav/lib/SystemTag/SystemTagNode.php index 500e1a3adea..f139a3335c8 100644 --- a/apps/dav/lib/SystemTag/SystemTagNode.php +++ b/apps/dav/lib/SystemTag/SystemTagNode.php @@ -32,6 +32,7 @@ use OCP\SystemTag\ISystemTag; use OCP\SystemTag\ISystemTagManager; use OCP\SystemTag\TagNotFoundException; use OCP\SystemTag\TagAlreadyExistsException; +use OCP\IUser; /** * DAV node representing a system tag, with the name being the tag id. @@ -49,6 +50,13 @@ class SystemTagNode implements \Sabre\DAV\INode { protected $tagManager; /** + * User + * + * @var IUser + */ + protected $user; + + /** * Whether to allow permissions for admins * * @var bool @@ -59,11 +67,13 @@ class SystemTagNode implements \Sabre\DAV\INode { * Sets up the node, expects a full path name * * @param ISystemTag $tag system tag + * @param IUser $user user * @param bool $isAdmin whether to allow operations for admins - * @param ISystemTagManager $tagManager + * @param ISystemTagManager $tagManager tag manager */ - public function __construct(ISystemTag $tag, $isAdmin, ISystemTagManager $tagManager) { + public function __construct(ISystemTag $tag, IUser $user, $isAdmin, ISystemTagManager $tagManager) { $this->tag = $tag; + $this->user = $user; $this->isAdmin = $isAdmin; $this->tagManager = $tagManager; } @@ -109,14 +119,15 @@ class SystemTagNode implements \Sabre\DAV\INode { */ public function update($name, $userVisible, $userAssignable) { try { - if (!$this->isAdmin) { - if (!$this->tag->isUserVisible()) { - throw new NotFound('Tag with id ' . $this->tag->getId() . ' does not exist'); - } - if (!$this->tag->isUserAssignable()) { - throw new Forbidden('No permission to update tag ' . $this->tag->getId()); - } + 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 + if (!$this->isAdmin) { // only renaming is allowed for regular users if ($userVisible !== $this->tag->isUserVisible() || $userAssignable !== $this->tag->isUserAssignable() @@ -124,6 +135,7 @@ class SystemTagNode implements \Sabre\DAV\INode { throw new Forbidden('No permission to update permissions for tag ' . $this->tag->getId()); } } + $this->tagManager->updateTag($this->tag->getId(), $name, $userVisible, $userAssignable); } catch (TagNotFoundException $e) { throw new NotFound('Tag with id ' . $this->tag->getId() . ' does not exist'); @@ -145,14 +157,13 @@ class SystemTagNode implements \Sabre\DAV\INode { public function delete() { try { - if (!$this->isAdmin) { - if (!$this->tag->isUserVisible()) { - throw new NotFound('Tag with id ' . $this->tag->getId() . ' not found'); - } - if (!$this->tag->isUserAssignable()) { - 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) { // can happen if concurrent deletion occurred diff --git a/apps/dav/lib/SystemTag/SystemTagPlugin.php b/apps/dav/lib/SystemTag/SystemTagPlugin.php index a266b4a1214..af683954b87 100644 --- a/apps/dav/lib/SystemTag/SystemTagPlugin.php +++ b/apps/dav/lib/SystemTag/SystemTagPlugin.php @@ -24,18 +24,20 @@ namespace OCA\DAV\SystemTag; use OCP\IGroupManager; use OCP\IUserSession; -use Sabre\DAV\Exception\NotFound; use Sabre\DAV\PropFind; use Sabre\DAV\PropPatch; use Sabre\DAV\Exception\BadRequest; -use Sabre\DAV\Exception\UnsupportedMediaType; use Sabre\DAV\Exception\Conflict; +use Sabre\DAV\Exception\Forbidden; +use Sabre\DAV\Exception\NotFound; +use Sabre\DAV\Exception\UnsupportedMediaType; use OCP\SystemTag\ISystemTag; use OCP\SystemTag\ISystemTagManager; use OCP\SystemTag\TagAlreadyExistsException; use Sabre\HTTP\RequestInterface; use Sabre\HTTP\ResponseInterface; +use OCA\DAV\SystemTag\SystemTagMappingNode; /** * Sabre plugin to handle system tags: @@ -52,6 +54,8 @@ 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 GROUPS_PROPERTYNAME = '{http://owncloud.org/ns}groups'; + const CANASSIGN_PROPERTYNAME = '{http://owncloud.org/ns}can-assign'; /** * @var \Sabre\DAV\Server $server @@ -181,14 +185,26 @@ class SystemTagPlugin extends \Sabre\DAV\ServerPlugin { $userAssignable = (bool)$data['userAssignable']; } - if($userVisible === false || $userAssignable === false) { + $groups = []; + if (isset($data['groups'])) { + $groups = $data['groups']; + if (is_string($groups)) { + $groups = explode('|', $groups); + } + } + + if($userVisible === false || $userAssignable === false || !empty($groups)) { if(!$this->userSession->isLoggedIn() || !$this->groupManager->isAdmin($this->userSession->getUser()->getUID())) { throw new BadRequest('Not sufficient permissions'); } } try { - return $this->tagManager->createTag($tagName, $userVisible, $userAssignable); + $tag = $this->tagManager->createTag($tagName, $userVisible, $userAssignable); + if (!empty($groups)) { + $this->tagManager->setTagGroups($tag, $groups); + } + return $tag; } catch (TagAlreadyExistsException $e) { throw new Conflict('Tag already exists', 0, $e); } @@ -205,7 +221,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; } @@ -222,8 +238,27 @@ 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'; + }); + + $propFind->handle(self::GROUPS_PROPERTYNAME, function() use ($node) { + if (!$this->groupManager->isAdmin($this->userSession->getUser()->getUID())) { + // property only available for admins + throw new Forbidden(); + } + $groups = []; + // no need to retrieve groups for namespaces that don't qualify + if ($node->getSystemTag()->isUserVisible() && !$node->getSystemTag()->isUserAssignable()) { + $groups = $this->tagManager->getTagGroups($node->getSystemTag()); + } + return implode('|', $groups); + }); } /** @@ -239,6 +274,7 @@ class SystemTagPlugin extends \Sabre\DAV\ServerPlugin { self::DISPLAYNAME_PROPERTYNAME, self::USERVISIBLE_PROPERTYNAME, self::USERASSIGNABLE_PROPERTYNAME, + self::GROUPS_PROPERTYNAME, ], function($props) use ($path) { $node = $this->server->tree->getNodeForPath($path); if (!($node instanceof SystemTagNode)) { @@ -250,22 +286,42 @@ class SystemTagPlugin extends \Sabre\DAV\ServerPlugin { $userVisible = $tag->isUserVisible(); $userAssignable = $tag->isUserAssignable(); + $updateTag = false; + if (isset($props[self::DISPLAYNAME_PROPERTYNAME])) { $name = $props[self::DISPLAYNAME_PROPERTYNAME]; + $updateTag = true; } if (isset($props[self::USERVISIBLE_PROPERTYNAME])) { $propValue = $props[self::USERVISIBLE_PROPERTYNAME]; $userVisible = ($propValue !== 'false' && $propValue !== '0'); + $updateTag = true; } if (isset($props[self::USERASSIGNABLE_PROPERTYNAME])) { $propValue = $props[self::USERASSIGNABLE_PROPERTYNAME]; $userAssignable = ($propValue !== 'false' && $propValue !== '0'); + $updateTag = true; + } + + if (isset($props[self::GROUPS_PROPERTYNAME])) { + if (!$this->groupManager->isAdmin($this->userSession->getUser()->getUID())) { + // property only available for admins + throw new Forbidden(); + } + + $propValue = $props[self::GROUPS_PROPERTYNAME]; + $groupIds = explode('|', $propValue); + $this->tagManager->setTagGroups($tag, $groupIds); + } + + if ($updateTag) { + $node->update($name, $userVisible, $userAssignable); } - $node->update($name, $userVisible, $userAssignable); return true; }); + } } diff --git a/apps/dav/lib/SystemTag/SystemTagsByIdCollection.php b/apps/dav/lib/SystemTag/SystemTagsByIdCollection.php index 298902501ab..2b24bce9f35 100644 --- a/apps/dav/lib/SystemTag/SystemTagsByIdCollection.php +++ b/apps/dav/lib/SystemTag/SystemTagsByIdCollection.php @@ -32,6 +32,7 @@ use OCP\SystemTag\ISystemTag; use OCP\SystemTag\TagNotFoundException; use OCP\IGroupManager; use OCP\IUserSession; +use OC\User\NoUserException; class SystemTagsByIdCollection implements ICollection { @@ -69,6 +70,8 @@ class SystemTagsByIdCollection implements ICollection { /** * Returns whether the currently logged in user is an administrator + * + * @return bool true if the user is an admin */ private function isAdmin() { $user = $this->userSession->getUser(); @@ -101,7 +104,7 @@ class SystemTagsByIdCollection implements ICollection { try { $tag = $this->tagManager->getTagsByIds([$name]); $tag = current($tag); - if (!$this->isAdmin() && !$tag->isUserVisible()) { + if (!$this->tagManager->canUserSeeTag($tag, $this->userSession->getUser())) { throw new NotFound('Tag with id ' . $name . ' not found'); } return $this->makeNode($tag); @@ -131,7 +134,7 @@ class SystemTagsByIdCollection implements ICollection { try { $tag = $this->tagManager->getTagsByIds([$name]); $tag = current($tag); - if (!$this->isAdmin() && !$tag->isUserVisible()) { + if (!$this->tagManager->canUserSeeTag($tag, $this->userSession->getUser())) { return false; } return true; @@ -171,6 +174,6 @@ class SystemTagsByIdCollection implements ICollection { * @return SystemTagNode */ private function makeNode(ISystemTag $tag) { - return new SystemTagNode($tag, $this->isAdmin(), $this->tagManager); + return new SystemTagNode($tag, $this->userSession->getUser(), $this->isAdmin(), $this->tagManager); } } diff --git a/apps/dav/lib/SystemTag/SystemTagsObjectMappingCollection.php b/apps/dav/lib/SystemTag/SystemTagsObjectMappingCollection.php index eb75ed06393..c4c45dc76d6 100644 --- a/apps/dav/lib/SystemTag/SystemTagsObjectMappingCollection.php +++ b/apps/dav/lib/SystemTag/SystemTagsObjectMappingCollection.php @@ -31,6 +31,7 @@ use OCP\SystemTag\ISystemTagManager; use OCP\SystemTag\ISystemTagObjectMapper; use OCP\SystemTag\ISystemTag; use OCP\SystemTag\TagNotFoundException; +use OCP\IUser; /** * Collection containing tags by object id @@ -58,11 +59,11 @@ class SystemTagsObjectMappingCollection implements ICollection { private $tagMapper; /** - * Whether to return results only visible for admins + * User * - * @var bool + * @var IUser */ - private $isAdmin; + private $user; /** @@ -70,30 +71,35 @@ class SystemTagsObjectMappingCollection implements ICollection { * * @param string $objectId object id * @param string $objectType object type - * @param bool $isAdmin whether to return results visible only for admins - * @param ISystemTagManager $tagManager - * @param ISystemTagObjectMapper $tagMapper + * @param IUser $user user + * @param ISystemTagManager $tagManager tag manager + * @param ISystemTagObjectMapper $tagMapper tag mapper */ - public function __construct($objectId, $objectType, $isAdmin, $tagManager, $tagMapper) { + public function __construct( + $objectId, + $objectType, + IUser $user, + ISystemTagManager $tagManager, + ISystemTagObjectMapper $tagMapper + ) { $this->tagManager = $tagManager; $this->tagMapper = $tagMapper; $this->objectId = $objectId; $this->objectType = $objectType; - $this->isAdmin = $isAdmin; + $this->user = $user; } function createFile($tagId, $data = null) { try { - if (!$this->isAdmin) { - $tag = $this->tagManager->getTagsByIds($tagId); - $tag = current($tag); - if (!$tag->isUserVisible()) { - throw new PreconditionFailed('Tag with id ' . $tagId . ' does not exist, cannot assign'); - } - if (!$tag->isUserAssignable()) { - throw new Forbidden('No permission to assign tag ' . $tag->getId()); - } + $tags = $this->tagManager->getTagsByIds([$tagId]); + $tag = current($tags); + if (!$this->tagManager->canUserSeeTag($tag, $this->user)) { + throw new PreconditionFailed('Tag with id ' . $tagId . ' does not exist, cannot assign'); } + if (!$this->tagManager->canUserAssignTag($tag, $this->user)) { + throw new Forbidden('No permission to assign tag ' . $tagId); + } + $this->tagMapper->assignTags($this->objectId, $this->objectType, $tagId); } catch (TagNotFoundException $e) { throw new PreconditionFailed('Tag with id ' . $tagId . ' does not exist, cannot assign'); @@ -109,7 +115,7 @@ class SystemTagsObjectMappingCollection implements ICollection { if ($this->tagMapper->haveTag([$this->objectId], $this->objectType, $tagId, true)) { $tag = $this->tagManager->getTagsByIds([$tagId]); $tag = current($tag); - if ($this->isAdmin || $tag->isUserVisible()) { + if ($this->tagManager->canUserSeeTag($tag, $this->user)) { return $this->makeNode($tag); } } @@ -127,12 +133,12 @@ class SystemTagsObjectMappingCollection implements ICollection { return []; } $tags = $this->tagManager->getTagsByIds($tagIds); - if (!$this->isAdmin) { - // filter out non-visible tags - $tags = array_filter($tags, function($tag) { - return $tag->isUserVisible(); - }); - } + + // filter out non-visible tags + $tags = array_filter($tags, function($tag) { + return $this->tagManager->canUserSeeTag($tag, $this->user); + }); + return array_values(array_map(function($tag) { return $this->makeNode($tag); }, $tags)); @@ -141,17 +147,16 @@ class SystemTagsObjectMappingCollection implements ICollection { function childExists($tagId) { try { $result = ($this->tagMapper->haveTag([$this->objectId], $this->objectType, $tagId, true)); - if ($this->isAdmin || !$result) { - return $result; - } - // verify if user is allowed to see this tag - $tag = $this->tagManager->getTagsByIds($tagId); - $tag = current($tag); - if (!$tag->isUserVisible()) { - return false; + if ($result) { + $tags = $this->tagManager->getTagsByIds([$tagId]); + $tag = current($tags); + if (!$this->tagManager->canUserSeeTag($tag, $this->user)) { + return false; + } } - return true; + + return $result; } catch (\InvalidArgumentException $e) { throw new BadRequest('Invalid tag id', 0, $e); } catch (TagNotFoundException $e) { @@ -193,7 +198,7 @@ class SystemTagsObjectMappingCollection implements ICollection { $tag, $this->objectId, $this->objectType, - $this->isAdmin, + $this->user, $this->tagManager, $this->tagMapper ); diff --git a/apps/dav/lib/SystemTag/SystemTagsObjectTypeCollection.php b/apps/dav/lib/SystemTag/SystemTagsObjectTypeCollection.php index bdbc73c4e32..ae4b9d51a1b 100644 --- a/apps/dav/lib/SystemTag/SystemTagsObjectTypeCollection.php +++ b/apps/dav/lib/SystemTag/SystemTagsObjectTypeCollection.php @@ -95,17 +95,6 @@ class SystemTagsObjectTypeCollection implements ICollection { } /** - * Returns whether the currently logged in user is an administrator - */ - private function isAdmin() { - $user = $this->userSession->getUser(); - if ($user !== null) { - return $this->groupManager->isAdmin($user->getUID()); - } - return false; - } - - /** * @param string $name * @param resource|string $data Initial payload * @throws Forbidden @@ -132,7 +121,7 @@ class SystemTagsObjectTypeCollection implements ICollection { return new SystemTagsObjectMappingCollection( $objectId, $this->objectType, - $this->isAdmin(), + $this->userSession->getUser(), $this->tagManager, $this->tagMapper ); diff --git a/apps/dav/tests/unit/systemtag/systemtagmappingnode.php b/apps/dav/tests/unit/systemtag/systemtagmappingnode.php index 7f2ff7d6616..f0e1c3bc567 100644 --- a/apps/dav/tests/unit/systemtag/systemtagmappingnode.php +++ b/apps/dav/tests/unit/systemtag/systemtagmappingnode.php @@ -24,6 +24,7 @@ namespace OCA\DAV\Tests\Unit\SystemTag; use Sabre\DAV\Exception\NotFound; use OC\SystemTag\SystemTag; use OCP\SystemTag\TagNotFoundException; +use OCP\SystemTag\ISystemTag; class SystemTagMappingNode extends \Test\TestCase { @@ -37,14 +38,20 @@ class SystemTagMappingNode extends \Test\TestCase { */ private $tagMapper; + /** + * @var \OCP\IUser + */ + private $user; + protected function setUp() { parent::setUp(); $this->tagManager = $this->getMock('\OCP\SystemTag\ISystemTagManager'); $this->tagMapper = $this->getMock('\OCP\SystemTag\ISystemTagObjectMapper'); + $this->user = $this->getMock('\OCP\IUser'); } - public function getMappingNode($isAdmin = true, $tag = null) { + public function getMappingNode($tag = null) { if ($tag === null) { $tag = new SystemTag(1, 'Test', true, true); } @@ -52,7 +59,7 @@ class SystemTagMappingNode extends \Test\TestCase { $tag, 123, 'files', - $isAdmin, + $this->user, $this->tagManager, $this->tagMapper ); @@ -60,28 +67,30 @@ class SystemTagMappingNode extends \Test\TestCase { public function testGetters() { $tag = new SystemTag(1, 'Test', true, false); - $node = $this->getMappingNode(true, $tag); + $node = $this->getMappingNode($tag); $this->assertEquals('1', $node->getName()); $this->assertEquals($tag, $node->getSystemTag()); $this->assertEquals(123, $node->getObjectId()); $this->assertEquals('files', $node->getObjectType()); } - public function adminFlagProvider() { - return [[true], [false]]; - } - - /** - * @dataProvider adminFlagProvider - */ - public function testDeleteTag($isAdmin) { + public function testDeleteTag() { + $node = $this->getMappingNode(); + $this->tagManager->expects($this->once()) + ->method('canUserSeeTag') + ->with($node->getSystemTag()) + ->will($this->returnValue(true)); + $this->tagManager->expects($this->once()) + ->method('canUserAssignTag') + ->with($node->getSystemTag()) + ->will($this->returnValue(true)); $this->tagManager->expects($this->never()) ->method('deleteTags'); $this->tagMapper->expects($this->once()) ->method('unassignTags') ->with(123, 'files', 1); - $this->getMappingNode($isAdmin)->delete(); + $node->delete(); } public function tagNodeDeleteProviderPermissionException() { @@ -102,7 +111,15 @@ class SystemTagMappingNode extends \Test\TestCase { /** * @dataProvider tagNodeDeleteProviderPermissionException */ - public function testDeleteTagExpectedException($tag, $expectedException) { + public function testDeleteTagExpectedException(ISystemTag $tag, $expectedException) { + $this->tagManager->expects($this->any()) + ->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'); $this->tagMapper->expects($this->never()) @@ -110,7 +127,7 @@ class SystemTagMappingNode extends \Test\TestCase { $thrown = null; try { - $this->getMappingNode(false, $tag)->delete(); + $this->getMappingNode($tag)->delete(); } catch (\Exception $e) { $thrown = $e; } @@ -122,11 +139,22 @@ class SystemTagMappingNode extends \Test\TestCase { * @expectedException Sabre\DAV\Exception\NotFound */ public function testDeleteTagNotFound() { + // assuming the tag existed at the time the node was created, + // but got deleted concurrently in the database + $tag = new SystemTag(1, 'Test', true, true); + $this->tagManager->expects($this->once()) + ->method('canUserSeeTag') + ->with($tag) + ->will($this->returnValue($tag->isUserVisible())); + $this->tagManager->expects($this->once()) + ->method('canUserAssignTag') + ->with($tag) + ->will($this->returnValue($tag->isUserAssignable())); $this->tagMapper->expects($this->once()) ->method('unassignTags') ->with(123, 'files', 1) ->will($this->throwException(new TagNotFoundException())); - $this->getMappingNode()->delete(); + $this->getMappingNode($tag)->delete(); } } diff --git a/apps/dav/tests/unit/systemtag/systemtagnode.php b/apps/dav/tests/unit/systemtag/systemtagnode.php index 5184b74e5c8..d9e088a7d90 100644 --- a/apps/dav/tests/unit/systemtag/systemtagnode.php +++ b/apps/dav/tests/unit/systemtag/systemtagnode.php @@ -28,6 +28,7 @@ use Sabre\DAV\Exception\Conflict; use OC\SystemTag\SystemTag; use OCP\SystemTag\TagNotFoundException; use OCP\SystemTag\TagAlreadyExistsException; +use OCP\SystemTag\ISystemTag; class SystemTagNode extends \Test\TestCase { @@ -36,10 +37,16 @@ class SystemTagNode extends \Test\TestCase { */ private $tagManager; + /** + * @var \OCP\IUser + */ + private $user; + protected function setUp() { parent::setUp(); $this->tagManager = $this->getMock('\OCP\SystemTag\ISystemTagManager'); + $this->user = $this->getMock('\OCP\IUser'); } protected function getTagNode($isAdmin = true, $tag = null) { @@ -48,6 +55,7 @@ class SystemTagNode extends \Test\TestCase { } return new \OCA\DAV\SystemTag\SystemTagNode( $tag, + $this->user, $isAdmin, $this->tagManager ); @@ -102,6 +110,14 @@ class SystemTagNode extends \Test\TestCase { */ public function testUpdateTag($isAdmin, $originalTag, $changedArgs) { $this->tagManager->expects($this->once()) + ->method('canUserSeeTag') + ->with($originalTag) + ->will($this->returnValue($originalTag->isUserVisible() || $isAdmin)); + $this->tagManager->expects($this->once()) + ->method('canUserAssignTag') + ->with($originalTag) + ->will($this->returnValue($originalTag->isUserAssignable() || $isAdmin)); + $this->tagManager->expects($this->once()) ->method('updateTag') ->with(1, $changedArgs[0], $changedArgs[1], $changedArgs[2]); $this->getTagNode($isAdmin, $originalTag) @@ -153,6 +169,14 @@ class SystemTagNode extends \Test\TestCase { * @dataProvider tagNodeProviderPermissionException */ public function testUpdateTagPermissionException($originalTag, $changedArgs, $expectedException = null) { + $this->tagManager->expects($this->any()) + ->method('canUserSeeTag') + ->with($originalTag) + ->will($this->returnValue($originalTag->isUserVisible())); + $this->tagManager->expects($this->any()) + ->method('canUserAssignTag') + ->with($originalTag) + ->will($this->returnValue($originalTag->isUserAssignable())); $this->tagManager->expects($this->never()) ->method('updateTag'); @@ -172,32 +196,59 @@ class SystemTagNode extends \Test\TestCase { * @expectedException Sabre\DAV\Exception\Conflict */ public function testUpdateTagAlreadyExists() { + $tag = new SystemTag(1, 'tag1', true, true); + $this->tagManager->expects($this->any()) + ->method('canUserSeeTag') + ->with($tag) + ->will($this->returnValue(true)); + $this->tagManager->expects($this->any()) + ->method('canUserAssignTag') + ->with($tag) + ->will($this->returnValue(true)); $this->tagManager->expects($this->once()) ->method('updateTag') - ->with(1, 'Renamed', false, true) + ->with(1, 'Renamed', true, true) ->will($this->throwException(new TagAlreadyExistsException())); - $this->getTagNode()->update('Renamed', false, true); + $this->getTagNode(false, $tag)->update('Renamed', true, true); } /** * @expectedException Sabre\DAV\Exception\NotFound */ public function testUpdateTagNotFound() { + $tag = new SystemTag(1, 'tag1', true, true); + $this->tagManager->expects($this->any()) + ->method('canUserSeeTag') + ->with($tag) + ->will($this->returnValue(true)); + $this->tagManager->expects($this->any()) + ->method('canUserAssignTag') + ->with($tag) + ->will($this->returnValue(true)); $this->tagManager->expects($this->once()) ->method('updateTag') - ->with(1, 'Renamed', false, true) + ->with(1, 'Renamed', true, true) ->will($this->throwException(new TagNotFoundException())); - $this->getTagNode()->update('Renamed', false, true); + $this->getTagNode(false, $tag)->update('Renamed', true, true); } /** * @dataProvider adminFlagProvider */ public function testDeleteTag($isAdmin) { + $tag = new SystemTag(1, 'tag1', true, true); + $this->tagManager->expects($this->once()) + ->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()) ->method('deleteTags') ->with('1'); - $this->getTagNode($isAdmin)->delete(); + $this->getTagNode($isAdmin, $tag)->delete(); } public function tagNodeDeleteProviderPermissionException() { @@ -218,7 +269,15 @@ class SystemTagNode extends \Test\TestCase { /** * @dataProvider tagNodeDeleteProviderPermissionException */ - public function testDeleteTagPermissionException($tag, $expectedException) { + public function testDeleteTagPermissionException(ISystemTag $tag, $expectedException) { + $this->tagManager->expects($this->any()) + ->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'); @@ -235,10 +294,19 @@ class SystemTagNode extends \Test\TestCase { * @expectedException Sabre\DAV\Exception\NotFound */ public function testDeleteTagNotFound() { + $tag = new SystemTag(1, 'tag1', true, true); + $this->tagManager->expects($this->any()) + ->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()->delete(); + $this->getTagNode(false, $tag)->delete(); } } diff --git a/apps/dav/tests/unit/systemtag/systemtagplugin.php b/apps/dav/tests/unit/systemtag/systemtagplugin.php index 4466533f1e0..da82bc8904a 100644 --- a/apps/dav/tests/unit/systemtag/systemtagplugin.php +++ b/apps/dav/tests/unit/systemtag/systemtagplugin.php @@ -27,6 +27,8 @@ use OC\SystemTag\SystemTag; use OCP\IGroupManager; use OCP\IUserSession; use OCP\SystemTag\TagAlreadyExistsException; +use OCP\IUser; +use OCP\SystemTag\ISystemTag; class SystemTagPlugin extends \Test\TestCase { @@ -34,6 +36,8 @@ class SystemTagPlugin extends \Test\TestCase { const DISPLAYNAME_PROPERTYNAME = \OCA\DAV\SystemTag\SystemTagPlugin::DISPLAYNAME_PROPERTYNAME; const USERVISIBLE_PROPERTYNAME = \OCA\DAV\SystemTag\SystemTagPlugin::USERVISIBLE_PROPERTYNAME; const USERASSIGNABLE_PROPERTYNAME = \OCA\DAV\SystemTag\SystemTagPlugin::USERASSIGNABLE_PROPERTYNAME; + const CANASSIGN_PROPERTYNAME = \OCA\DAV\SystemTag\SystemTagPlugin::CANASSIGN_PROPERTYNAME; + const GROUPS_PROPERTYNAME = \OCA\DAV\SystemTag\SystemTagPlugin::GROUPS_PROPERTYNAME; /** * @var \Sabre\DAV\Server @@ -61,6 +65,11 @@ class SystemTagPlugin extends \Test\TestCase { private $userSession; /** + * @var IUser + */ + private $user; + + /** * @var \OCA\DAV\SystemTag\SystemTagPlugin */ private $plugin; @@ -75,7 +84,16 @@ class SystemTagPlugin extends \Test\TestCase { $this->tagManager = $this->getMock('\OCP\SystemTag\ISystemTagManager'); $this->groupManager = $this->getMock('\OCP\IGroupManager'); + $this->user = $this->getMock('\OCP\IUser'); $this->userSession = $this->getMock('\OCP\IUserSession'); + $this->userSession + ->expects($this->any()) + ->method('getUser') + ->willReturn($this->user); + $this->userSession + ->expects($this->any()) + ->method('isLoggedIn') + ->willReturn(true); $this->plugin = new \OCA\DAV\SystemTag\SystemTagPlugin( $this->tagManager, @@ -85,22 +103,84 @@ class SystemTagPlugin extends \Test\TestCase { $this->plugin->initialize($this->server); } - public function testGetProperties() { - $systemTag = new SystemTag(1, 'Test', true, true); - $requestedProperties = [ - self::ID_PROPERTYNAME, - self::DISPLAYNAME_PROPERTYNAME, - self::USERVISIBLE_PROPERTYNAME, - self::USERASSIGNABLE_PROPERTYNAME - ]; - $expectedProperties = [ - 200 => [ - self::ID_PROPERTYNAME => '1', - self::DISPLAYNAME_PROPERTYNAME => 'Test', - self::USERVISIBLE_PROPERTYNAME => 'true', - self::USERASSIGNABLE_PROPERTYNAME => 'true', - ] + public function getPropertiesDataProvider() { + return [ + [ + new SystemTag(1, 'Test', true, true), + [], + [ + self::ID_PROPERTYNAME, + self::DISPLAYNAME_PROPERTYNAME, + self::USERVISIBLE_PROPERTYNAME, + self::USERASSIGNABLE_PROPERTYNAME, + self::CANASSIGN_PROPERTYNAME, + ], + [ + self::ID_PROPERTYNAME => '1', + self::DISPLAYNAME_PROPERTYNAME => 'Test', + self::USERVISIBLE_PROPERTYNAME => 'true', + self::USERASSIGNABLE_PROPERTYNAME => 'true', + self::CANASSIGN_PROPERTYNAME => 'true', + ] + ], + [ + new SystemTag(1, 'Test', true, false), + [], + [ + self::ID_PROPERTYNAME, + self::DISPLAYNAME_PROPERTYNAME, + self::USERVISIBLE_PROPERTYNAME, + self::USERASSIGNABLE_PROPERTYNAME, + self::CANASSIGN_PROPERTYNAME, + ], + [ + self::ID_PROPERTYNAME => '1', + self::DISPLAYNAME_PROPERTYNAME => 'Test', + self::USERVISIBLE_PROPERTYNAME => 'true', + self::USERASSIGNABLE_PROPERTYNAME => 'false', + self::CANASSIGN_PROPERTYNAME => 'false', + ] + ], + [ + new SystemTag(1, 'Test', true, false), + ['group1', 'group2'], + [ + self::ID_PROPERTYNAME, + self::GROUPS_PROPERTYNAME, + ], + [ + self::ID_PROPERTYNAME => '1', + self::GROUPS_PROPERTYNAME => 'group1|group2', + ] + ], + [ + new SystemTag(1, 'Test', true, true), + ['group1', 'group2'], + [ + self::ID_PROPERTYNAME, + self::GROUPS_PROPERTYNAME, + ], + [ + self::ID_PROPERTYNAME => '1', + // groups only returned when userAssignable is false + self::GROUPS_PROPERTYNAME => '', + ] + ], ]; + } + + /** + * @dataProvider getPropertiesDataProvider + */ + public function testGetProperties(ISystemTag $systemTag, $groups, $requestedProperties, $expectedProperties) { + $this->user->expects($this->any()) + ->method('getUID') + ->willReturn('admin'); + $this->groupManager + ->expects($this->any()) + ->method('isAdmin') + ->with('admin') + ->willReturn(true); $node = $this->getMockBuilder('\OCA\DAV\SystemTag\SystemTagNode') ->disableOriginalConstructor() @@ -109,6 +189,14 @@ class SystemTagPlugin extends \Test\TestCase { ->method('getSystemTag') ->will($this->returnValue($systemTag)); + $this->tagManager->expects($this->any()) + ->method('canUserAssignTag') + ->will($this->returnValue($systemTag->isUserAssignable())); + + $this->tagManager->expects($this->any()) + ->method('getTagGroups') + ->will($this->returnValue($groups)); + $this->tree->expects($this->any()) ->method('getNodeForPath') ->with('/systemtag/1') @@ -128,12 +216,62 @@ class SystemTagPlugin extends \Test\TestCase { $result = $propFind->getResultForMultiStatus(); $this->assertEmpty($result[404]); - unset($result[404]); - $this->assertEquals($expectedProperties, $result); + $this->assertEquals($expectedProperties, $result[200]); } - public function testUpdateProperties() { + /** + * @expectedException \Sabre\DAV\Exception\Forbidden + */ + public function testGetPropertiesForbidden() { $systemTag = new SystemTag(1, 'Test', true, false); + $requestedProperties = [ + self::ID_PROPERTYNAME, + self::GROUPS_PROPERTYNAME, + ]; + $this->user->expects($this->once()) + ->method('getUID') + ->willReturn('admin'); + $this->groupManager + ->expects($this->once()) + ->method('isAdmin') + ->with('admin') + ->willReturn(false); + + $node = $this->getMockBuilder('\OCA\DAV\SystemTag\SystemTagNode') + ->disableOriginalConstructor() + ->getMock(); + $node->expects($this->any()) + ->method('getSystemTag') + ->will($this->returnValue($systemTag)); + + $this->tree->expects($this->any()) + ->method('getNodeForPath') + ->with('/systemtag/1') + ->will($this->returnValue($node)); + + $propFind = new \Sabre\DAV\PropFind( + '/systemtag/1', + $requestedProperties, + 0 + ); + + $this->plugin->handleGetProperties( + $propFind, + $node + ); + } + + public function testUpdatePropertiesAdmin() { + $systemTag = new SystemTag(1, 'Test', true, false); + $this->user->expects($this->any()) + ->method('getUID') + ->willReturn('admin'); + $this->groupManager + ->expects($this->any()) + ->method('isAdmin') + ->with('admin') + ->willReturn(true); + $node = $this->getMockBuilder('\OCA\DAV\SystemTag\SystemTagNode') ->disableOriginalConstructor() ->getMock(); @@ -150,11 +288,16 @@ class SystemTagPlugin extends \Test\TestCase { ->method('update') ->with('Test changed', false, true); + $this->tagManager->expects($this->once()) + ->method('setTagGroups') + ->with($systemTag, ['group1', 'group2']); + // properties to set $propPatch = new \Sabre\DAV\PropPatch(array( self::DISPLAYNAME_PROPERTYNAME => 'Test changed', self::USERVISIBLE_PROPERTYNAME => 'false', self::USERASSIGNABLE_PROPERTYNAME => 'true', + self::GROUPS_PROPERTYNAME => 'group1|group2', )); $this->plugin->handleUpdateProperties( @@ -174,102 +317,89 @@ class SystemTagPlugin extends \Test\TestCase { } /** - * @expectedException \Sabre\DAV\Exception\BadRequest - * @expectedExceptionMessage Not sufficient permissions + * @expectedException \Sabre\DAV\Exception\Forbidden */ - public function testCreateNotAssignableTagAsRegularUser() { - $user = $this->getMock('\OCP\IUser'); - $user->expects($this->once()) + public function testUpdatePropertiesForbidden() { + $systemTag = new SystemTag(1, 'Test', true, false); + $this->user->expects($this->any()) ->method('getUID') ->willReturn('admin'); - $this->userSession - ->expects($this->once()) - ->method('isLoggedIn') - ->willReturn(true); - $this->userSession - ->expects($this->once()) - ->method('getUser') - ->willReturn($user); $this->groupManager - ->expects($this->once()) + ->expects($this->any()) ->method('isAdmin') ->with('admin') ->willReturn(false); - $requestData = json_encode([ - 'name' => 'Test', - 'userVisible' => true, - 'userAssignable' => false, - ]); - - $node = $this->getMockBuilder('\OCA\DAV\SystemTag\SystemTagsByIdCollection') + $node = $this->getMockBuilder('\OCA\DAV\SystemTag\SystemTagNode') ->disableOriginalConstructor() ->getMock(); - $this->tagManager->expects($this->never()) - ->method('createTag'); + $node->expects($this->any()) + ->method('getSystemTag') + ->will($this->returnValue($systemTag)); $this->tree->expects($this->any()) ->method('getNodeForPath') - ->with('/systemtags') + ->with('/systemtag/1') ->will($this->returnValue($node)); - $request = $this->getMockBuilder('Sabre\HTTP\RequestInterface') - ->disableOriginalConstructor() - ->getMock(); - $response = $this->getMockBuilder('Sabre\HTTP\ResponseInterface') - ->disableOriginalConstructor() - ->getMock(); + $node->expects($this->never()) + ->method('update'); - $request->expects($this->once()) - ->method('getPath') - ->will($this->returnValue('/systemtags')); + $this->tagManager->expects($this->never()) + ->method('setTagGroups'); - $request->expects($this->once()) - ->method('getBodyAsString') - ->will($this->returnValue($requestData)); + // properties to set + $propPatch = new \Sabre\DAV\PropPatch(array( + self::GROUPS_PROPERTYNAME => 'group1|group2', + )); - $request->expects($this->once()) - ->method('getHeader') - ->with('Content-Type') - ->will($this->returnValue('application/json')); + $this->plugin->handleUpdateProperties( + '/systemtag/1', + $propPatch + ); - $this->plugin->httpPost($request, $response); + $propPatch->commit(); } + public function createTagInsufficientPermissionsProvider() { + return [ + [true, false, ''], + [false, true, ''], + [true, true, 'group1|group2'], + ]; + } /** + * @dataProvider createTagInsufficientPermissionsProvider * @expectedException \Sabre\DAV\Exception\BadRequest * @expectedExceptionMessage Not sufficient permissions */ - public function testCreateInvisibleTagAsRegularUser() { - $user = $this->getMock('\OCP\IUser'); - $user->expects($this->once()) + public function testCreateNotAssignableTagAsRegularUser($userVisible, $userAssignable, $groups) { + $this->user->expects($this->once()) ->method('getUID') ->willReturn('admin'); - $this->userSession - ->expects($this->once()) - ->method('isLoggedIn') - ->willReturn(true); - $this->userSession - ->expects($this->once()) - ->method('getUser') - ->willReturn($user); $this->groupManager ->expects($this->once()) ->method('isAdmin') ->with('admin') ->willReturn(false); - $requestData = json_encode([ + $requestData = [ 'name' => 'Test', - 'userVisible' => false, - 'userAssignable' => true, - ]); + 'userVisible' => $userVisible, + 'userAssignable' => $userAssignable, + ]; + if (!empty($groups)) { + $requestData['groups'] = $groups; + } + $requestData = json_encode($requestData); $node = $this->getMockBuilder('\OCA\DAV\SystemTag\SystemTagsByIdCollection') ->disableOriginalConstructor() ->getMock(); $this->tagManager->expects($this->never()) ->method('createTag'); + $this->tagManager->expects($this->never()) + ->method('setTagGroups'); $this->tree->expects($this->any()) ->method('getNodeForPath') @@ -352,19 +482,21 @@ class SystemTagPlugin extends \Test\TestCase { $this->plugin->httpPost($request, $response); } - public function testCreateTagInByIdCollection() { - $user = $this->getMock('\OCP\IUser'); - $user->expects($this->once()) + public function createTagProvider() { + return [ + [true, false, ''], + [false, false, ''], + [true, false, 'group1|group2'], + ]; + } + + /** + * @dataProvider createTagProvider + */ + public function testCreateTagInByIdCollection($userVisible, $userAssignable, $groups) { + $this->user->expects($this->once()) ->method('getUID') ->willReturn('admin'); - $this->userSession - ->expects($this->once()) - ->method('isLoggedIn') - ->willReturn(true); - $this->userSession - ->expects($this->once()) - ->method('getUser') - ->willReturn($user); $this->groupManager ->expects($this->once()) ->method('isAdmin') @@ -373,19 +505,33 @@ class SystemTagPlugin extends \Test\TestCase { $systemTag = new SystemTag(1, 'Test', true, false); - $requestData = json_encode([ + $requestData = [ 'name' => 'Test', - 'userVisible' => true, - 'userAssignable' => false, - ]); + 'userVisible' => $userVisible, + 'userAssignable' => $userAssignable, + ]; + if (!empty($groups)) { + $requestData['groups'] = $groups; + } + $requestData = json_encode($requestData); $node = $this->getMockBuilder('\OCA\DAV\SystemTag\SystemTagsByIdCollection') ->disableOriginalConstructor() ->getMock(); $this->tagManager->expects($this->once()) ->method('createTag') - ->with('Test', true, false) + ->with('Test', $userVisible, $userAssignable) ->will($this->returnValue($systemTag)); + + if (!empty($groups)) { + $this->tagManager->expects($this->once()) + ->method('setTagGroups') + ->with($systemTag, explode('|', $groups)) + ->will($this->returnValue($systemTag)); + } else { + $this->tagManager->expects($this->never()) + ->method('setTagGroups'); + } $this->tree->expects($this->any()) ->method('getNodeForPath') @@ -431,18 +577,9 @@ class SystemTagPlugin extends \Test\TestCase { } public function testCreateTagInMappingCollection() { - $user = $this->getMock('\OCP\IUser'); - $user->expects($this->once()) + $this->user->expects($this->once()) ->method('getUID') ->willReturn('admin'); - $this->userSession - ->expects($this->once()) - ->method('isLoggedIn') - ->willReturn(true); - $this->userSession - ->expects($this->once()) - ->method('getUser') - ->willReturn($user); $this->groupManager ->expects($this->once()) ->method('isAdmin') @@ -510,8 +647,6 @@ class SystemTagPlugin extends \Test\TestCase { * @expectedException \Sabre\DAV\Exception\NotFound */ public function testCreateTagToUnknownNode() { - $systemTag = new SystemTag(1, 'Test', true, false); - $node = $this->getMockBuilder('\OCA\DAV\SystemTag\SystemTagsObjectMappingCollection') ->disableOriginalConstructor() ->getMock(); @@ -545,18 +680,9 @@ class SystemTagPlugin extends \Test\TestCase { * @expectedException \Sabre\DAV\Exception\Conflict */ public function testCreateTagConflict($nodeClass) { - $user = $this->getMock('\OCP\IUser'); - $user->expects($this->once()) + $this->user->expects($this->once()) ->method('getUID') ->willReturn('admin'); - $this->userSession - ->expects($this->once()) - ->method('isLoggedIn') - ->willReturn(true); - $this->userSession - ->expects($this->once()) - ->method('getUser') - ->willReturn($user); $this->groupManager ->expects($this->once()) ->method('isAdmin') diff --git a/apps/dav/tests/unit/systemtag/systemtagsbyidcollection.php b/apps/dav/tests/unit/systemtag/systemtagsbyidcollection.php index a2bf571ab68..5aa28d1a254 100644 --- a/apps/dav/tests/unit/systemtag/systemtagsbyidcollection.php +++ b/apps/dav/tests/unit/systemtag/systemtagsbyidcollection.php @@ -32,6 +32,11 @@ class SystemTagsByIdCollection extends \Test\TestCase { */ private $tagManager; + /** + * @var \OCP\IUser + */ + private $user; + protected function setUp() { parent::setUp(); @@ -39,14 +44,14 @@ class SystemTagsByIdCollection extends \Test\TestCase { } public function getNode($isAdmin = true) { - $user = $this->getMock('\OCP\IUser'); - $user->expects($this->any()) + $this->user = $this->getMock('\OCP\IUser'); + $this->user->expects($this->any()) ->method('getUID') ->will($this->returnValue('testuser')); $userSession = $this->getMock('\OCP\IUserSession'); $userSession->expects($this->any()) ->method('getUser') - ->will($this->returnValue($user)); + ->will($this->returnValue($this->user)); $groupManager = $this->getMock('\OCP\IGroupManager'); $groupManager->expects($this->any()) ->method('isAdmin') @@ -77,35 +82,19 @@ class SystemTagsByIdCollection extends \Test\TestCase { $this->getNode()->createDirectory('789'); } - public function getChildProvider() { - return [ - [ - true, - true, - ], - [ - true, - false, - ], - [ - false, - true, - ], - ]; - } - - /** - * @dataProvider getChildProvider - */ - public function testGetChild($isAdmin, $userVisible) { - $tag = new SystemTag(123, 'Test', $userVisible, false); + public function testGetChild() { + $tag = new SystemTag(123, 'Test', true, false); + $this->tagManager->expects($this->once()) + ->method('canUserSeeTag') + ->with($tag) + ->will($this->returnValue(true)); $this->tagManager->expects($this->once()) ->method('getTagsByIds') ->with(['123']) ->will($this->returnValue([$tag])); - $childNode = $this->getNode($isAdmin)->getChild('123'); + $childNode = $this->getNode()->getChild('123'); $this->assertInstanceOf('\OCA\DAV\SystemTag\SystemTagNode', $childNode); $this->assertEquals('123', $childNode->getName()); @@ -198,27 +187,27 @@ class SystemTagsByIdCollection extends \Test\TestCase { public function childExistsProvider() { return [ - // admins, always visible - [true, true, true], - [true, false, true], - // non-admins, depends on flag - [false, true, true], - [false, false, false], + [true, true], + [false, false], ]; } /** * @dataProvider childExistsProvider */ - public function testChildExists($isAdmin, $userVisible, $expectedResult) { + public function testChildExists($userVisible, $expectedResult) { $tag = new SystemTag(123, 'One', $userVisible, false); + $this->tagManager->expects($this->once()) + ->method('canUserSeeTag') + ->with($tag) + ->will($this->returnValue($userVisible)); $this->tagManager->expects($this->once()) ->method('getTagsByIds') ->with(['123']) ->will($this->returnValue([$tag])); - $this->assertEquals($expectedResult, $this->getNode($isAdmin)->childExists('123')); + $this->assertEquals($expectedResult, $this->getNode()->childExists('123')); } public function testChildExistsNotFound() { diff --git a/apps/dav/tests/unit/systemtag/systemtagsobjectmappingcollection.php b/apps/dav/tests/unit/systemtag/systemtagsobjectmappingcollection.php index df97acd846b..9adc5b88c41 100644 --- a/apps/dav/tests/unit/systemtag/systemtagsobjectmappingcollection.php +++ b/apps/dav/tests/unit/systemtag/systemtagsobjectmappingcollection.php @@ -37,45 +37,50 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase { */ private $tagMapper; + /** + * @var \OCP\IUser + */ + private $user; + protected function setUp() { parent::setUp(); $this->tagManager = $this->getMock('\OCP\SystemTag\ISystemTagManager'); $this->tagMapper = $this->getMock('\OCP\SystemTag\ISystemTagObjectMapper'); + + $this->user = $this->getMock('\OCP\IUser'); } - public function getNode($isAdmin = true) { + public function getNode() { return new \OCA\DAV\SystemTag\SystemTagsObjectMappingCollection ( 111, 'files', - $isAdmin, + $this->user, $this->tagManager, $this->tagMapper ); } - public function testAssignTagAsAdmin() { - $this->tagManager->expects($this->never()) - ->method('getTagsByIds'); - $this->tagMapper->expects($this->once()) - ->method('assignTags') - ->with(111, 'files', '555'); - - $this->getNode(true)->createFile('555'); - } - - public function testAssignTagAsUser() { + public function testAssignTag() { $tag = new SystemTag('1', 'Test', true, true); + $this->tagManager->expects($this->once()) + ->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()) ->method('getTagsByIds') - ->with('555') + ->with(['555']) ->will($this->returnValue([$tag])); $this->tagMapper->expects($this->once()) ->method('assignTags') ->with(111, 'files', '555'); - $this->getNode(false)->createFile('555'); + $this->getNode()->createFile('555'); } public function permissionsProvider() { @@ -90,19 +95,27 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase { /** * @dataProvider permissionsProvider */ - public function testAssignTagAsUserNoPermission($userVisible, $userAssignable, $expectedException) { + public function testAssignTagNoPermission($userVisible, $userAssignable, $expectedException) { $tag = new SystemTag('1', 'Test', $userVisible, $userAssignable); + $this->tagManager->expects($this->once()) + ->method('canUserSeeTag') + ->with($tag) + ->will($this->returnValue($userVisible)); + $this->tagManager->expects($this->any()) + ->method('canUserAssignTag') + ->with($tag) + ->will($this->returnValue($userAssignable)); $this->tagManager->expects($this->once()) ->method('getTagsByIds') - ->with('555') + ->with(['555']) ->will($this->returnValue([$tag])); $this->tagMapper->expects($this->never()) ->method('assignTags'); $thrown = null; try { - $this->getNode(false)->createFile('555'); + $this->getNode()->createFile('555'); } catch (\Exception $e) { $thrown = $e; } @@ -114,9 +127,9 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase { * @expectedException Sabre\DAV\Exception\PreconditionFailed */ public function testAssignTagNotFound() { - $this->tagMapper->expects($this->once()) - ->method('assignTags') - ->with(111, 'files', '555') + $this->tagManager->expects($this->once()) + ->method('getTagsByIds') + ->with(['555']) ->will($this->throwException(new TagNotFoundException())); $this->getNode()->createFile('555'); @@ -129,28 +142,12 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase { $this->getNode()->createDirectory('789'); } - public function getChildProvider() { - return [ - [ - true, - true, - ], - [ - true, - false, - ], - [ - false, - true, - ], - ]; - } - - /** - * @dataProvider getChildProvider - */ - public function testGetChild($isAdmin, $userVisible) { - $tag = new SystemTag(555, 'TheTag', $userVisible, false); + public function testGetChild() { + $tag = new SystemTag(555, 'TheTag', true, false); + $this->tagManager->expects($this->once()) + ->method('canUserSeeTag') + ->with($tag) + ->will($this->returnValue(true)); $this->tagMapper->expects($this->once()) ->method('haveTag') @@ -162,17 +159,21 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase { ->with(['555']) ->will($this->returnValue(['555' => $tag])); - $childNode = $this->getNode($isAdmin)->getChild('555'); + $childNode = $this->getNode()->getChild('555'); - $this->assertInstanceOf('\OCA\DAV\SystemTag\SystemTagNode', $childNode); + $this->assertInstanceOf('\OCA\DAV\SystemTag\SystemTagMappingNode', $childNode); $this->assertEquals('555', $childNode->getName()); } /** * @expectedException \Sabre\DAV\Exception\NotFound */ - public function testGetChildUserNonVisible() { + public function testGetChildNonVisible() { $tag = new SystemTag(555, 'TheTag', false, false); + $this->tagManager->expects($this->once()) + ->method('canUserSeeTag') + ->with($tag) + ->will($this->returnValue(false)); $this->tagMapper->expects($this->once()) ->method('haveTag') @@ -184,7 +185,7 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase { ->with(['555']) ->will($this->returnValue(['555' => $tag])); - $this->getNode(false)->getChild('555'); + $this->getNode()->getChild('555'); } /** @@ -223,7 +224,7 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase { $this->getNode()->getChild('777'); } - public function testGetChildrenAsAdmin() { + public function testGetChildren() { $tag1 = new SystemTag(555, 'TagOne', true, false); $tag2 = new SystemTag(556, 'TagTwo', true, true); $tag3 = new SystemTag(557, 'InvisibleTag', false, true); @@ -238,43 +239,13 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase { ->with(['555', '556', '557']) ->will($this->returnValue(['555' => $tag1, '556' => $tag2, '557' => $tag3])); - $children = $this->getNode(true)->getChildren(); - - $this->assertCount(3, $children); - - $this->assertInstanceOf('\OCA\DAV\SystemTag\SystemTagMappingNode', $children[0]); - $this->assertInstanceOf('\OCA\DAV\SystemTag\SystemTagMappingNode', $children[1]); - $this->assertInstanceOf('\OCA\DAV\SystemTag\SystemTagMappingNode', $children[2]); - - $this->assertEquals(111, $children[0]->getObjectId()); - $this->assertEquals('files', $children[0]->getObjectType()); - $this->assertEquals($tag1, $children[0]->getSystemTag()); - - $this->assertEquals(111, $children[1]->getObjectId()); - $this->assertEquals('files', $children[1]->getObjectType()); - $this->assertEquals($tag2, $children[1]->getSystemTag()); + $this->tagManager->expects($this->exactly(3)) + ->method('canUserSeeTag') + ->will($this->returnCallback(function($tag) { + return $tag->isUserVisible(); + })); - $this->assertEquals(111, $children[2]->getObjectId()); - $this->assertEquals('files', $children[2]->getObjectType()); - $this->assertEquals($tag3, $children[2]->getSystemTag()); - } - - public function testGetChildrenAsUser() { - $tag1 = new SystemTag(555, 'TagOne', true, false); - $tag2 = new SystemTag(556, 'TagTwo', true, true); - $tag3 = new SystemTag(557, 'InvisibleTag', false, true); - - $this->tagMapper->expects($this->once()) - ->method('getTagIdsForObjects') - ->with([111], 'files') - ->will($this->returnValue(['111' => ['555', '556', '557']])); - - $this->tagManager->expects($this->once()) - ->method('getTagsByIds') - ->with(['555', '556', '557']) - ->will($this->returnValue(['555' => $tag1, '556' => $tag2, '557' => $tag3])); - - $children = $this->getNode(false)->getChildren(); + $children = $this->getNode()->getChildren(); $this->assertCount(2, $children); @@ -290,16 +261,7 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase { $this->assertEquals($tag2, $children[1]->getSystemTag()); } - public function testChildExistsAsAdmin() { - $this->tagMapper->expects($this->once()) - ->method('haveTag') - ->with([111], 'files', '555') - ->will($this->returnValue(true)); - - $this->assertTrue($this->getNode(true)->childExists('555')); - } - - public function testChildExistsWithVisibleTagAsUser() { + public function testChildExistsWithVisibleTag() { $tag = new SystemTag(555, 'TagOne', true, false); $this->tagMapper->expects($this->once()) @@ -308,14 +270,19 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase { ->will($this->returnValue(true)); $this->tagManager->expects($this->once()) + ->method('canUserSeeTag') + ->with($tag) + ->will($this->returnValue(true)); + + $this->tagManager->expects($this->once()) ->method('getTagsByIds') - ->with('555') + ->with(['555']) ->will($this->returnValue([$tag])); - $this->assertTrue($this->getNode(false)->childExists('555')); + $this->assertTrue($this->getNode()->childExists('555')); } - public function testChildExistsWithInvisibleTagAsUser() { + public function testChildExistsWithInvisibleTag() { $tag = new SystemTag(555, 'TagOne', false, false); $this->tagMapper->expects($this->once()) @@ -325,10 +292,10 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase { $this->tagManager->expects($this->once()) ->method('getTagsByIds') - ->with('555') + ->with(['555']) ->will($this->returnValue([$tag])); - $this->assertFalse($this->getNode(false)->childExists('555')); + $this->assertFalse($this->getNode()->childExists('555')); } public function testChildExistsNotFound() { diff --git a/apps/systemtags/js/systemtagsinfoview.js b/apps/systemtags/js/systemtagsinfoview.js index 2f01ca7db34..a7320a3956f 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/lib/Activity/Extension.php b/apps/systemtags/lib/Activity/Extension.php index 8c101a6f550..6bb83e37cfc 100644 --- a/apps/systemtags/lib/Activity/Extension.php +++ b/apps/systemtags/lib/Activity/Extension.php @@ -324,7 +324,7 @@ class Extension implements IExtension { case 'assignable': return '<parameter>' . $matches[1] . '</parameter>'; case 'not-assignable': - return '<parameter>' . $l->t('%s (not-assignable)', $matches[1]) . '</parameter>'; + return '<parameter>' . $l->t('%s (restricted)', $matches[1]) . '</parameter>'; case 'invisible': return '<parameter>' . $l->t('%s (invisible)', $matches[1]) . '</parameter>'; } diff --git a/apps/systemtags/tests/js/systemtagsinfoviewSpec.js b/apps/systemtags/tests/js/systemtagsinfoviewSpec.js index 27724822c2e..449dfd859d7 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 = $('<input type="hidden" val="1,4"/>'); + 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/build/integration/features/bootstrap/TagsContext.php b/build/integration/features/bootstrap/TagsContext.php index 10d0b9ae545..882f5d9e13a 100644 --- a/build/integration/features/bootstrap/TagsContext.php +++ b/build/integration/features/bootstrap/TagsContext.php @@ -97,28 +97,36 @@ class TagsContext implements \Behat\Behat\Context\Context { } /** - * @When :user creates a :type tag with name :name * @param string $user * @param string $type * @param string $name - * @throws \Exception + * @param string $groups */ - public function createsATagWithName($user, $type, $name) { - $userVisible = 'true'; - $userAssignable = 'true'; + private function createTag($user, $type, $name, $groups = null) { + $userVisible = true; + $userAssignable = true; switch ($type) { case 'normal': break; case 'not user-assignable': - $userAssignable = 'false'; + $userAssignable = false; break; case 'not user-visible': - $userVisible = 'false'; + $userVisible = false; break; default: throw new \Exception('Unsupported type'); } + $body = [ + 'name' => $name, + 'userVisible' => $userVisible, + 'userAssignable' => $userAssignable, + ]; + if ($groups !== null) { + $body['groups'] = $groups; + } + try { $this->response = $this->client->post( $this->baseUrl . '/remote.php/dav/systemtags/', @@ -130,15 +138,38 @@ class TagsContext implements \Behat\Behat\Context\Context { 'headers' => [ 'Content-Type' => 'application/json', ], - 'body' => '{"name":"'.$name.'","userVisible":'.$userVisible.',"userAssignable":'.$userAssignable.'}', + 'body' => json_encode($body) ] ); - } catch (\GuzzleHttp\Exception\ClientException $e){ + } catch (\GuzzleHttp\Exception\ClientException $e) { $this->response = $e->getResponse(); } } /** + * @When :user creates a :type tag with name :name + * @param string $user + * @param string $type + * @param string $name + * @throws \Exception + */ + public function createsATagWithName($user, $type, $name) { + $this->createTag($user, $type, $name); + } + + /** + * @When :user creates a :type tag with name :name and groups :groups + * @param string $user + * @param string $type + * @param string $name + * @param string $groups + * @throws \Exception + */ + public function createsATagWithNameAndGroups($user, $type, $name, $groups) { + $this->createTag($user, $type, $name, $groups); + } + + /** * @Then The response should have a status code :statusCode * @param int $statusCode * @throws \Exception @@ -155,21 +186,30 @@ class TagsContext implements \Behat\Behat\Context\Context { * @param string $user * @return array */ - private function requestTagsForUser($user) { + private function requestTagsForUser($user, $withGroups = false) { try { - $request = $this->client->createRequest( - 'PROPFIND', - $this->baseUrl . '/remote.php/dav/systemtags/', - [ - 'body' => '<?xml version="1.0"?> + $body = '<?xml version="1.0"?> <d:propfind xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns"> <d:prop> <oc:id /> <oc:display-name /> <oc:user-visible /> - <oc:user-assignable /> + <oc:user-assignable /> + <oc:can-assign /> +'; + + if ($withGroups) { + $body .= '<oc:groups />'; + } + + $body .= ' </d:prop> -</d:propfind>', +</d:propfind>'; + $request = $this->client->createRequest( + 'PROPFIND', + $this->baseUrl . '/remote.php/dav/systemtags/', + [ + 'body' => $body, 'auth' => [ $user, $this->getPasswordForUser($user), @@ -193,11 +233,16 @@ class TagsContext implements \Behat\Behat\Context\Context { continue; } + // FIXME: use actual property names instead of guessing index position $tags[$singleEntry[0]['value']] = [ 'display-name' => $singleEntry[1]['value'], 'user-visible' => $singleEntry[2]['value'], 'user-assignable' => $singleEntry[3]['value'], + 'can-assign' => $singleEntry[4]['value'], ]; + if (isset($singleEntry[5])) { + $tags[$singleEntry[0]['value']]['groups'] = $singleEntry[5]['value']; + } } return $tags; @@ -239,6 +284,42 @@ class TagsContext implements \Behat\Behat\Context\Context { } /** + * @Then the user :user :can assign The :type tag with name :tagName + */ + public function theUserCanAssignTheTag($user, $can, $type, $tagName) { + $foundTag = $this->findTag($type, $tagName, $user); + if ($foundTag === null) { + throw new \Exception('No matching tag found'); + } + + if ($can === 'can') { + $expected = 'true'; + } else if ($can === 'cannot') { + $expected = 'false'; + } else { + throw new \Exception('Invalid condition, must be "can" or "cannot"'); + } + + if ($foundTag['can-assign'] !== $expected) { + throw new \Exception('Tag cannot be assigned by user'); + } + } + + /** + * @Then The :type tag with name :tagName has the groups :groups + */ + public function theTagHasGroup($type, $tagName, $groups) { + $foundTag = $this->findTag($type, $tagName, 'admin', true); + if ($foundTag === null) { + throw new \Exception('No matching tag found'); + } + + if ($foundTag['groups'] !== $groups) { + throw new \Exception('Tag has groups "' . $foundTag['group'] . '" instead of the expected "' . $groups . '"'); + } + } + + /** * @Then :count tags should exist for :user * @param int $count * @param string $user @@ -251,6 +332,45 @@ class TagsContext implements \Behat\Behat\Context\Context { } /** + * Find tag by type and name + * + * @param string $type tag type + * @param string $tagName tag name + * @param string $user retrieved from which user + * @param bool $withGroups whether to also query the tag's groups + * + * @return array tag values or null if not found + */ + private function findTag($type, $tagName, $user = 'admin', $withGroups = false) { + $tags = $this->requestTagsForUser($user, $withGroups); + $userAssignable = 'true'; + $userVisible = 'true'; + switch ($type) { + case 'normal': + break; + case 'not user-assignable': + $userAssignable = 'false'; + break; + case 'not user-visible': + $userVisible = 'false'; + break; + default: + throw new \Exception('Unsupported type'); + } + + $foundTag = null; + foreach ($tags as $tag) { + if ($tag['display-name'] === $tagName + && $tag['user-visible'] === $userVisible + && $tag['user-assignable'] === $userAssignable) { + $foundTag = $tag; + break; + } + } + return $foundTag; + } + + /** * @param string $name * @return int */ @@ -305,6 +425,44 @@ class TagsContext implements \Behat\Behat\Context\Context { } /** + * @When :user edits the tag with name :oldNmae and sets its groups to :groups + * @param string $user + * @param string $oldName + * @param string $groups + * @throws \Exception + */ + public function editsTheTagWithNameAndSetsItsGroupsTo($user, $oldName, $groups) { + $tagId = $this->findTagIdByName($oldName); + if($tagId === 0) { + throw new \Exception('Could not find tag to rename'); + } + + try { + $request = $this->client->createRequest( + 'PROPPATCH', + $this->baseUrl . '/remote.php/dav/systemtags/' . $tagId, + [ + 'body' => '<?xml version="1.0"?> +<d:propertyupdate xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns"> + <d:set> + <d:prop> + <oc:groups>' . $groups . '</oc:groups> + </d:prop> + </d:set> +</d:propertyupdate>', + 'auth' => [ + $user, + $this->getPasswordForUser($user), + ], + ] + ); + $this->response = $this->client->send($request); + } catch (\GuzzleHttp\Exception\ClientException $e) { + $this->response = $e->getResponse(); + } + } + + /** * @When :user deletes the tag with name :name * @param string $user * @param string $name diff --git a/build/integration/features/tags.feature b/build/integration/features/tags.feature index 286fb62bf42..d793c0d3c61 100644 --- a/build/integration/features/tags.feature +++ b/build/integration/features/tags.feature @@ -21,6 +21,18 @@ Feature: tags Then The response should have a status code "400" And "0" tags should exist for "admin" + Scenario: Creating a not user-assignable tag with groups as admin should work + Given user "user0" exists + When "admin" creates a "not user-assignable" tag with name "TagWithGroups" and groups "group1|group2" + Then The response should have a status code "201" + And The "not user-assignable" tag with name "TagWithGroups" has the groups "group1|group2" + + Scenario: Creating a normal tag with groups as regular user should fail + Given user "user0" exists + When "user0" creates a "normal" tag with name "MySuperAwesomeTagName" and groups "group1|group2" + 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 Given user "user0" exists Given "admin" creates a "normal" tag with name "MySuperAwesomeTagName" @@ -45,6 +57,19 @@ Feature: tags And The following tags should exist for "admin" |MySuperAwesomeTagName|false|true| + Scenario: Editing tag groups as admin should work + Given user "user0" exists + Given "admin" creates a "not user-assignable" tag with name "TagWithGroups" and groups "group1|group2" + When "admin" edits the tag with name "TagWithGroups" and sets its groups to "group1|group3" + Then The response should have a status code "207" + And The "not user-assignable" tag with name "TagWithGroups" has the groups "group1|group3" + + Scenario: Editing tag groups as regular user should fail + Given user "user0" exists + Given "admin" creates a "not user-assignable" tag with name "TagWithGroups" + 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 Given user "user0" exists Given "admin" creates a "normal" tag with name "MySuperAwesomeTagName" @@ -122,6 +147,23 @@ Feature: tags And "/myFileToTag.txt" shared by "user0" has the following tags |MyFirstTag| + Scenario: Assigning a not user-assignable tag to a file shared by someone else as regular user belongs to tag's groups should work + Given user "user0" exists + Given user "user1" exists + Given group "group1" exists + Given user "user1" belongs to group "group1" + Given "admin" creates a "not user-assignable" tag with name "MySuperAwesomeTagName" and groups "group1" + Given user "user0" uploads file "data/textfile.txt" to "/myFileToTag.txt" + Given As "user0" sending "POST" to "/apps/files_sharing/api/v1/shares" with + | path | myFileToTag.txt | + | shareWith | user1 | + | shareType | 0 | + When "user1" adds the tag "MySuperAwesomeTagName" to "/myFileToTag.txt" shared by "user0" + Then The response should have a status code "201" + And "/myFileToTag.txt" shared by "user0" has the following tags + |MySuperAwesomeTagName| + + Scenario: Assigning a not user-visible tag to a file shared by someone else as regular user should fail Given user "user0" exists Given user "user1" exists @@ -368,3 +410,18 @@ Feature: tags And "/myFileToTag.txt" shared by "user0" has the following tags for "user1" || And The response should have a status code "404" + + Scenario: User can assign tags when in the tag's groups + Given user "user0" exists + Given group "group1" exists + Given user "user0" belongs to group "group1" + When "admin" creates a "not user-assignable" tag with name "TagWithGroups" and groups "group1|group2" + Then The response should have a status code "201" + And the user "user0" can assign the "not user-assignable" tag with name "TagWithGroups" + + Scenario: User cannot assign tags when not in the tag's groups + Given user "user0" exists + When "admin" creates a "not user-assignable" tag with name "TagWithGroups" and groups "group1|group2" + Then The response should have a status code "201" + And the user "user0" cannot assign the "not user-assignable" tag with name "TagWithGroups" + diff --git a/build/jsdocs9.tar.bz2 b/build/jsdocs9.tar.bz2 Binary files differnew file mode 100644 index 00000000000..dfab675733d --- /dev/null +++ b/build/jsdocs9.tar.bz2 diff --git a/core/js/systemtags/systemtagmodel.js b/core/js/systemtags/systemtagmodel.js index b41fbdde61e..89728357e25 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/systemtags.js b/core/js/systemtags/systemtags.js index 042f49bb8ed..05ead6f3dcd 100644 --- a/core/js/systemtags/systemtags.js +++ b/core/js/systemtags/systemtags.js @@ -36,7 +36,7 @@ var scope; if (!tag.userAssignable) { - scope = t('core', 'not assignable'); + scope = t('core', 'restricted'); } if (!tag.userVisible) { // invisible also implicitly means not assignable diff --git a/core/js/systemtags/systemtagsinputfield.js b/core/js/systemtags/systemtagsinputfield.js index 45dc5b7b03e..2eb8d0a44cb 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/systemtagsSpec.js b/core/js/tests/specs/systemtags/systemtagsSpec.js index 515b75258a0..f6d99e62a3c 100644 --- a/core/js/tests/specs/systemtags/systemtagsSpec.js +++ b/core/js/tests/specs/systemtags/systemtagsSpec.js @@ -64,6 +64,6 @@ describe('OC.SystemTags tests', function() { testScope(true, true, 'Fourty Two'); testScope(false, true, 'Fourty Two (invisible)'); testScope(false, false, 'Fourty Two (invisible)'); - testScope(true, false, 'Fourty Two (not assignable)'); + testScope(true, false, 'Fourty Two (restricted)'); }); }); diff --git a/core/js/tests/specs/systemtags/systemtagsinputfieldSpec.js b/core/js/tests/specs/systemtags/systemtagsinputfieldSpec.js index 22bf0d2c82a..503bef7cf2b 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'); + // restricted / 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/core/shipped.json b/core/shipped.json index e33269dad75..a3abe22d8d5 100644 --- a/core/shipped.json +++ b/core/shipped.json @@ -30,6 +30,7 @@ "provisioning_api", "sharepoint", "systemtags", + "systemtags_management", "templateeditor", "updatenotification", "user_external", diff --git a/db_structure.xml b/db_structure.xml index 6e57b003fcf..640edfbfd80 100644 --- a/db_structure.xml +++ b/db_structure.xml @@ -1383,6 +1383,49 @@ <table> <!-- + System tag to group mapping + --> + <name>*dbprefix*systemtag_group</name> + + <declaration> + + <!-- Foreign Key systemtag::id --> + <field> + <name>systemtagid</name> + <type>integer</type> + <default>0</default> + <notnull>true</notnull> + <unsigned>true</unsigned> + <length>4</length> + </field> + + <field> + <name>gid</name> + <type>string</type> + <notnull>true</notnull> + </field> + + <index> + <name>systemtag_group</name> + <primary>true</primary> + <unique>true</unique> + <field> + <name>gid</name> + <sorting>ascending</sorting> + </field> + <field> + <name>systemtagid</name> + <sorting>ascending</sorting> + </field> + </index> + + </declaration> + + </table> + + <table> + + <!-- Namespaced Key-Value Store for arbitrary data. - Keys are namespaced per userid and appid. - E.g. (admin, files, foo) -> bar diff --git a/lib/private/SystemTag/ManagerFactory.php b/lib/private/SystemTag/ManagerFactory.php index d9acf327f8a..6b238e3c428 100644 --- a/lib/private/SystemTag/ManagerFactory.php +++ b/lib/private/SystemTag/ManagerFactory.php @@ -59,6 +59,7 @@ class ManagerFactory implements ISystemTagManagerFactory { public function getManager() { return new SystemTagManager( $this->serverContainer->getDatabaseConnection(), + $this->serverContainer->getGroupManager(), $this->serverContainer->getEventDispatcher() ); } diff --git a/lib/private/SystemTag/SystemTagManager.php b/lib/private/SystemTag/SystemTagManager.php index 76a60a91328..2b0ef03e471 100644 --- a/lib/private/SystemTag/SystemTagManager.php +++ b/lib/private/SystemTag/SystemTagManager.php @@ -30,10 +30,18 @@ use OCP\SystemTag\ManagerEvent; use OCP\SystemTag\TagAlreadyExistsException; use OCP\SystemTag\TagNotFoundException; use Symfony\Component\EventDispatcher\EventDispatcherInterface; +use OCP\IUserManager; +use OCP\IGroupManager; +use OCP\SystemTag\ISystemTag; +use OCP\IUser; +/** + * Manager class for system tags + */ class SystemTagManager implements ISystemTagManager { const TAG_TABLE = 'systemtag'; + const TAG_GROUP_TABLE = 'systemtag_group'; /** @var IDBConnection */ protected $connection; @@ -41,6 +49,9 @@ class SystemTagManager implements ISystemTagManager { /** @var EventDispatcherInterface */ protected $dispatcher; + /** @var IGroupManager */ + protected $groupManager; + /** * Prepared query for selecting tags directly * @@ -54,8 +65,13 @@ class SystemTagManager implements ISystemTagManager { * @param IDBConnection $connection database connection * @param EventDispatcherInterface $dispatcher */ - public function __construct(IDBConnection $connection, EventDispatcherInterface $dispatcher) { + public function __construct( + IDBConnection $connection, + IGroupManager $groupManager, + EventDispatcherInterface $dispatcher + ) { $this->connection = $connection; + $this->groupManager = $groupManager; $this->dispatcher = $dispatcher; $query = $this->connection->getQueryBuilder(); @@ -316,7 +332,102 @@ class SystemTagManager implements ISystemTagManager { } } + /** + * {@inheritdoc} + */ + public function canUserAssignTag(ISystemTag $tag, IUser $user) { + // early check to avoid unneeded group lookups + if ($tag->isUserAssignable() && $tag->isUserVisible()) { + return true; + } + + if ($this->groupManager->isAdmin($user->getUID())) { + return true; + } + + if (!$tag->isUserVisible()) { + return false; + } + + $groupIds = $this->groupManager->getUserGroupIds($user); + if (!empty($groupIds)) { + $matchingGroups = array_intersect($groupIds, $this->getTagGroups($tag)); + if (!empty($matchingGroups)) { + return true; + } + } + + return false; + } + + /** + * {@inheritdoc} + */ + public function canUserSeeTag(ISystemTag $tag, IUser $user) { + if ($tag->isUserVisible()) { + return true; + } + + if ($this->groupManager->isAdmin($user->getUID())) { + return true; + } + + return false; + } + private function createSystemTagFromRow($row) { return new SystemTag((int)$row['id'], $row['name'], (bool)$row['visibility'], (bool)$row['editable']); } + + /** + * {@inheritdoc} + */ + public function setTagGroups(ISystemTag $tag, $groupIds) { + // delete relationships first + $this->connection->beginTransaction(); + try { + $query = $this->connection->getQueryBuilder(); + $query->delete(self::TAG_GROUP_TABLE) + ->where($query->expr()->eq('systemtagid', $query->createNamedParameter($tag->getId()))) + ->execute(); + + // add each group id + $query = $this->connection->getQueryBuilder(); + $query->insert(self::TAG_GROUP_TABLE) + ->values([ + 'systemtagid' => $query->createNamedParameter($tag->getId()), + 'gid' => $query->createParameter('gid'), + ]); + foreach ($groupIds as $groupId) { + $query->setParameter('gid', $groupId); + $query->execute(); + } + + $this->connection->commit(); + } catch (\Exception $e) { + $this->connection->rollback(); + throw $e; + } + } + + /** + * {@inheritdoc} + */ + public function getTagGroups(ISystemTag $tag) { + $groupIds = []; + $query = $this->connection->getQueryBuilder(); + $query->select('gid') + ->from(self::TAG_GROUP_TABLE) + ->where($query->expr()->eq('systemtagid', $query->createNamedParameter($tag->getId()))) + ->orderBy('gid'); + + $result = $query->execute(); + while ($row = $result->fetch()) { + $groupIds[] = $row['gid']; + } + + $result->closeCursor(); + + return $groupIds; + } } diff --git a/lib/public/SystemTag/ISystemTagManager.php b/lib/public/SystemTag/ISystemTagManager.php index 983bfd636ce..35447b05fdc 100644 --- a/lib/public/SystemTag/ISystemTagManager.php +++ b/lib/public/SystemTag/ISystemTagManager.php @@ -22,6 +22,9 @@ namespace OCP\SystemTag; +use OCP\IUser; +use OCP\SystemTag\ISystemTag; + /** * Public interface to access and manage system-wide tags. * @@ -113,4 +116,49 @@ interface ISystemTagManager { */ public function deleteTags($tagIds); + /** + * Checks whether the given user is allowed to assign/unassign the tag with the + * given id. + * + * @param ISystemTag $tag tag to check permission for + * @param IUser $user user to check permission for + * + * @return true if the user is allowed to assign/unassign the tag, false otherwise + * + * @since 9.1.0 + */ + public function canUserAssignTag(ISystemTag $tag, IUser $user); + + /** + * Checks whether the given user is allowed to see the tag with the given id. + * + * @param ISystemTag $tag tag to check permission for + * @param IUser $user user to check permission for + * + * @return true if the user can see the tag, false otherwise + * + * @since 9.1.0 + */ + public function canUserSeeTag(ISystemTag $tag, IUser $userId); + + /** + * Set groups that can assign a given tag. + * + * @param ISystemTag $tag tag for group assignment + * @param string[] $groupIds group ids of groups that can assign/unassign the tag + * + * @since 9.1.0 + */ + public function setTagGroups(ISystemTag $tag, $groupIds); + + /** + * Get groups that can assign a given tag. + * + * @param ISystemTag $tag tag for group assignment + * + * @return string[] group ids of groups that can assign/unassign the tag + * + * @since 9.1.0 + */ + public function getTagGroups(ISystemTag $tag); } diff --git a/settings/js/settings.js b/settings/js/settings.js index fcbe328b76f..5a2ba4bcec7 100644 --- a/settings/js/settings.js +++ b/settings/js/settings.js @@ -16,10 +16,13 @@ OC.Settings = _.extend(OC.Settings, { * for groups) * * @param $elements jQuery element (hidden input) to setup select2 on - * @param [extraOptions] extra options hash to pass to select2 + * @param {Array} [extraOptions] extra options hash to pass to select2 + * @param {Array} [options] extra options + * @param {Array} [options.excludeAdmins=false] flag whether to exclude admin groups */ - setupGroupsSelect: function($elements, extraOptions) { + setupGroupsSelect: function($elements, extraOptions, options) { var self = this; + options = options || {}; if ($elements.length > 0) { // note: settings are saved through a "change" event registered // on all input fields @@ -48,9 +51,11 @@ OC.Settings = _.extend(OC.Settings, { var results = []; // add groups - $.each(data.data.adminGroups, function(i, group) { - results.push({id:group.id, displayname:group.name}); - }); + if (!options.excludeAdmins) { + $.each(data.data.adminGroups, function(i, group) { + results.push({id:group.id, displayname:group.name}); + }); + } $.each(data.data.groups, function(i, group) { results.push({id:group.id, displayname:group.name}); }); diff --git a/tests/lib/SystemTag/SystemTagManagerTest.php b/tests/lib/SystemTag/SystemTagManagerTest.php index 1afb147f08a..e697e373465 100644 --- a/tests/lib/SystemTag/SystemTagManagerTest.php +++ b/tests/lib/SystemTag/SystemTagManagerTest.php @@ -17,6 +17,8 @@ use OCP\SystemTag\ISystemTag; use OCP\SystemTag\ISystemTagManager; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Test\TestCase; +use OCP\IUserManager; +use OCP\IGroupManager; /** * Class TestSystemTagManager @@ -37,6 +39,11 @@ class SystemTagManagerTest extends TestCase { private $connection; /** + * @var IGroupManager + */ + private $groupManager; + + /** * @var EventDispatcherInterface */ private $dispatcher; @@ -49,8 +56,11 @@ class SystemTagManagerTest extends TestCase { $this->dispatcher = $this->getMockBuilder('Symfony\Component\EventDispatcher\EventDispatcherInterface') ->getMock(); + $this->groupManager = $this->getMockBuilder('\OCP\IGroupManager')->getMock(); + $this->tagManager = new SystemTagManager( $this->connection, + $this->groupManager, $this->dispatcher ); $this->pruneTagsTables(); @@ -410,6 +420,102 @@ class SystemTagManagerTest extends TestCase { ], $tagIdMapping); } + public function visibilityCheckProvider() { + return [ + [false, false, false, false], + [true, false, false, true], + [false, false, true, true], + [true, false, true, true], + ]; + } + + /** + * @dataProvider visibilityCheckProvider + */ + public function testVisibilityCheck($userVisible, $userAssignable, $isAdmin, $expectedResult) { + $user = $this->getMockBuilder('\OCP\IUser')->getMock(); + $user->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('test')); + $tag1 = $this->tagManager->createTag('one', $userVisible, $userAssignable); + + $this->groupManager->expects($this->any()) + ->method('isAdmin') + ->with('test') + ->will($this->returnValue($isAdmin)); + + $this->assertEquals($expectedResult, $this->tagManager->canUserSeeTag($tag1, $user)); + } + + public function assignabilityCheckProvider() { + return [ + // no groups + [false, false, false, false], + [true, false, false, false], + [true, true, false, true], + [false, true, false, false], + // admin rulez + [false, false, true, true], + [false, true, true, true], + [true, false, true, true], + [true, true, true, true], + // ignored groups + [false, false, false, false, ['group1'], ['group1']], + [true, true, false, true, ['group1'], ['group1']], + [true, true, false, true, ['group1'], ['anothergroup']], + [false, true, false, false, ['group1'], ['group1']], + // admin has precedence over groups + [false, false, true, true, ['group1'], ['anothergroup']], + [false, true, true, true, ['group1'], ['anothergroup']], + [true, false, true, true, ['group1'], ['anothergroup']], + [true, true, true, true, ['group1'], ['anothergroup']], + // groups only checked when visible and user non-assignable and non-admin + [true, false, false, false, ['group1'], ['anothergroup1']], + [true, false, false, true, ['group1'], ['group1']], + [true, false, false, true, ['group1', 'group2'], ['group2', 'group3']], + ]; + } + + /** + * @dataProvider assignabilityCheckProvider + */ + public function testAssignabilityCheck($userVisible, $userAssignable, $isAdmin, $expectedResult, $userGroupIds = [], $tagGroupIds = []) { + $user = $this->getMockBuilder('\OCP\IUser')->getMock(); + $user->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('test')); + $tag1 = $this->tagManager->createTag('one', $userVisible, $userAssignable); + $this->tagManager->setTagGroups($tag1, $tagGroupIds); + + $this->groupManager->expects($this->any()) + ->method('isAdmin') + ->with('test') + ->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)); + } + + public function testTagGroups() { + $tag1 = $this->tagManager->createTag('tag1', true, false); + $tag2 = $this->tagManager->createTag('tag2', true, false); + $this->tagManager->setTagGroups($tag1, ['group1', 'group2']); + $this->tagManager->setTagGroups($tag2, ['group2', 'group3']); + + $this->assertEquals(['group1', 'group2'], $this->tagManager->getTagGroups($tag1)); + $this->assertEquals(['group2', 'group3'], $this->tagManager->getTagGroups($tag2)); + + // change groups + $this->tagManager->setTagGroups($tag1, ['group3', 'group4']); + $this->tagManager->setTagGroups($tag2, []); + + $this->assertEquals(['group3', 'group4'], $this->tagManager->getTagGroups($tag1)); + $this->assertEquals([], $this->tagManager->getTagGroups($tag2)); + } + /** * @param ISystemTag $tag1 * @param ISystemTag $tag2 diff --git a/version.php b/version.php index d9e1ca1df1c..a4f1c4dbce0 100644 --- a/version.php +++ b/version.php @@ -26,7 +26,7 @@ // We only can count up. The 4. digit is only for the internal patchlevel to trigger DB upgrades // between betas, final and RCs. This is _not_ the public version number. Reset minor/patchlevel // when updating major/minor version number. -$OC_Version = array(9, 1, 0, 2); +$OC_Version = array(9, 1, 0, 3); // The human readable string $OC_VersionString = '9.1.0 pre alpha'; |