Merge pull request #21814 from owncloud/systemtags-backend-permissions

Fix DAV backend to respect system tags permissions

apps/dav/lib/rootcollection.php

@@ -58,12 +58,17 @@ class RootCollection extends SimpleCollection {
 		$caldavBackend = new CalDavBackend($db);
 		$calendarRoot = new CalendarRoot($userPrincipalBackend, $caldavBackend, 'principals/users');
 		$calendarRoot->disableListing = $disableListing;
+
 		$systemTagCollection = new SystemTag\SystemTagsByIdCollection(
-			\OC::$server->getSystemTagManager()
+			\OC::$server->getSystemTagManager(),
+			\OC::$server->getUserSession(),
+			\OC::$server->getGroupManager()
 		);
 		$systemTagRelationsCollection = new SystemTag\SystemTagsRelationsCollection(
 			\OC::$server->getSystemTagManager(),
-			\OC::$server->getSystemTagObjectMapper()
+			\OC::$server->getSystemTagObjectMapper(),
+			\OC::$server->getUserSession(),
+			\OC::$server->getGroupManager()
 		);
 
 		$usersCardDavBackend = new CardDavBackend($db, $userPrincipalBackend);

apps/dav/lib/systemtag/systemtagmappingnode.php

@@ -23,6 +23,7 @@
 namespace OCA\DAV\SystemTag;
 
 use Sabre\DAV\Exception\NotFound;
+use Sabre\DAV\Exception\Forbidden;
 
 use OCP\SystemTag\ISystemTag;
 use OCP\SystemTag\ISystemTagManager;
@@ -55,6 +56,7 @@ class SystemTagMappingNode extends SystemTagNode {
 	 * @param ISystemTag $tag system tag
 	 * @param string $objectId
 	 * @param string $objectType
+	 * @param bool $isAdmin whether to allow permissions for admin
 	 * @param ISystemTagManager $tagManager
 	 * @param ISystemTagObjectMapper $tagMapper
 	 */
@@ -62,13 +64,14 @@ class SystemTagMappingNode extends SystemTagNode {
 		ISystemTag $tag,
 		$objectId,
 		$objectType,
+		$isAdmin,
 		ISystemTagManager $tagManager,
 		ISystemTagObjectMapper $tagMapper
 	) {
 		$this->objectId = $objectId;
 		$this->objectType = $objectType;
 		$this->tagMapper = $tagMapper;
-		parent::__construct($tag, $tagManager);
+		parent::__construct($tag, $isAdmin, $tagManager);
 	}
 
 	/**
@@ -94,6 +97,14 @@ class SystemTagMappingNode extends SystemTagNode {
 	 */
 	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());
+				}
+			}
 			$this->tagMapper->unassignTags($this->objectId, $this->objectType, $this->tag->getId());
 		} catch (TagNotFoundException $e) {
 			// can happen if concurrent deletion occurred

apps/dav/lib/systemtag/systemtagnode.php

@@ -22,6 +22,7 @@
 
 namespace OCA\DAV\SystemTag;
 
+use Sabre\DAV\Exception\Forbidden;
 use Sabre\DAV\Exception\NotFound;
 use Sabre\DAV\Exception\MethodNotAllowed;
 use Sabre\DAV\Exception\Conflict;
@@ -46,14 +47,23 @@ class SystemTagNode implements \Sabre\DAV\INode {
 	 */
 	protected $tagManager;
 
+	/**
+	 * Whether to allow permissions for admins
+	 *
+	 * @var bool
+	 */
+	protected $isAdmin;
+
 	/**
 	 * Sets up the node, expects a full path name
 	 *
 	 * @param ISystemTag $tag system tag
+	 * @param bool $isAdmin whether to allow operations for admins
 	 * @param ISystemTagManager $tagManager
 	 */
-	public function __construct(ISystemTag $tag, ISystemTagManager $tagManager) {
+	public function __construct(ISystemTag $tag, $isAdmin, ISystemTagManager $tagManager) {
 		$this->tag = $tag;
+		$this->isAdmin = $isAdmin;
 		$this->tagManager = $tagManager;
 	}
 
@@ -97,6 +107,21 @@ 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());
+				}
+
+				// only renaming is allowed for regular users
+				if ($userVisible !== $this->tag->isUserVisible()
+					|| $userAssignable !== $this->tag->isUserAssignable()
+				) {
+					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');
@@ -118,6 +143,14 @@ 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());
+				}
+			}
 			$this->tagManager->deleteTags($this->tag->getId());
 		} catch (TagNotFoundException $e) {
 			// can happen if concurrent deletion occurred

apps/dav/lib/systemtag/systemtagsbyidcollection.php

@@ -31,6 +31,8 @@ use Sabre\DAV\ICollection;
 use OCP\SystemTag\ISystemTagManager;
 use OCP\SystemTag\ISystemTag;
 use OCP\SystemTag\TagNotFoundException;
+use OCP\IGroupManager;
+use OCP\IUserSession;
 
 class SystemTagsByIdCollection implements ICollection {
 
@@ -39,13 +41,42 @@ class SystemTagsByIdCollection implements ICollection {
 	 */
 	private $tagManager;
 
+	/**
+	 * @var IGroupManager
+	 */
+	private $groupManager;
+
+	/**
+	 * @var IUserSession
+	 */
+	private $userSession;
+
 	/**
 	 * SystemTagsByIdCollection constructor.
 	 *
 	 * @param ISystemTagManager $tagManager
+	 * @param IUserSession $userSession
+	 * @param IGroupManager $groupManager
 	 */
-	public function __construct($tagManager) {
+	public function __construct(
+		ISystemTagManager $tagManager,
+		IUserSession $userSession,
+		IGroupManager $groupManager
+	) {
 		$this->tagManager = $tagManager;
+		$this->userSession = $userSession;
+		$this->groupManager = $groupManager;
+	}
+
+	/**
+	 * 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;
 	}
 
 	/**
@@ -69,8 +100,12 @@ class SystemTagsByIdCollection implements ICollection {
 	 */
 	function getChild($name) {
 		try {
-			$tags = $this->tagManager->getTagsByIds([$name]);
+			$tag = $this->tagManager->getTagsByIds([$name]);
-			return $this->makeNode(current($tags));
+			$tag = current($tag);
+			if (!$this->isAdmin() && !$tag->isUserVisible()) {
+				throw new NotFound('Tag with id ' . $name . ' not found');
+			}
+			return $this->makeNode($tag);
 		} catch (\InvalidArgumentException $e) {
 			throw new BadRequest('Invalid tag id', 0, $e);
 		} catch (TagNotFoundException $e) {
@@ -79,7 +114,12 @@ class SystemTagsByIdCollection implements ICollection {
 	}
 
 	function getChildren() {
-		$tags = $this->tagManager->getAllTags(true);
+		$visibilityFilter = true;
+		if ($this->isAdmin()) {
+			$visibilityFilter = null;
+		}
+
+		$tags = $this->tagManager->getAllTags($visibilityFilter);
 		return array_map(function($tag) {
 			return $this->makeNode($tag);
 		}, $tags);
@@ -90,7 +130,11 @@ class SystemTagsByIdCollection implements ICollection {
 	 */
 	function childExists($name) {
 		try {
-			$this->tagManager->getTagsByIds([$name]);
+			$tag = $this->tagManager->getTagsByIds([$name]);
+			$tag = current($tag);
+			if (!$this->isAdmin() && !$tag->isUserVisible()) {
+				return false;
+			}
 			return true;
 		} catch (\InvalidArgumentException $e) {
 			throw new BadRequest('Invalid tag id', 0, $e);
@@ -128,6 +172,6 @@ class SystemTagsByIdCollection implements ICollection {
 	 * @return SystemTagNode
 	 */
 	private function makeNode(ISystemTag $tag) {
-		return new SystemTagNode($tag, $this->tagManager);
+		return new SystemTagNode($tag, $this->isAdmin(), $this->tagManager);
 	}
 }

apps/dav/lib/systemtag/systemtagsobjectmappingcollection.php

@@ -57,23 +57,43 @@ class SystemTagsObjectMappingCollection implements ICollection {
 	 */
 	private $tagMapper;
 
+	/**
+	 * Whether to return results only visible for admins
+	 *
+	 * @var bool
+	 */
+	private $isAdmin;
+
+
 	/**
 	 * Constructor
 	 *
 	 * @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
 	 */
-	public function __construct($objectId, $objectType, $tagManager, $tagMapper) {
+	public function __construct($objectId, $objectType, $isAdmin, $tagManager, $tagMapper) {
 		$this->tagManager = $tagManager;
 		$this->tagMapper = $tagMapper;
 		$this->objectId = $objectId;
 		$this->objectType = $objectType;
+		$this->isAdmin = $isAdmin;
 	}
 
 	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());
+				}
+			}
 			$this->tagMapper->assignTags($this->objectId, $this->objectType, $tagId);
 		} catch (TagNotFoundException $e) {
 			throw new PreconditionFailed('Tag with id ' . $tagId . ' does not exist, cannot assign');
@@ -88,7 +108,10 @@ class SystemTagsObjectMappingCollection implements ICollection {
 		try {
 			if ($this->tagMapper->haveTag([$this->objectId], $this->objectType, $tagId, true)) {
 				$tag = $this->tagManager->getTagsByIds([$tagId]);
-				return $this->makeNode(current($tag));
+				$tag = current($tag);
+				if ($this->isAdmin || $tag->isUserVisible()) {
+					return $this->makeNode($tag);
+				}
 			}
 			throw new NotFound('Tag with id ' . $tagId . ' not present for object ' . $this->objectId);
 		} catch (\InvalidArgumentException $e) {
@@ -104,6 +127,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();
+			});
+		}
 		return array_values(array_map(function($tag) {
 			return $this->makeNode($tag);
 		}, $tags));
@@ -111,7 +140,18 @@ class SystemTagsObjectMappingCollection implements ICollection {
 
 	function childExists($tagId) {
 		try {
-			return ($this->tagMapper->haveTag([$this->objectId], $this->objectType, $tagId, true));
+			$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;
+			}
+			return true;
 		} catch (\InvalidArgumentException $e) {
 			throw new BadRequest('Invalid tag id', 0, $e);
 		} catch (TagNotFoundException $e) {
@@ -153,6 +193,7 @@ class SystemTagsObjectMappingCollection implements ICollection {
 			$tag,
 			$this->objectId,
 			$this->objectType,
+			$this->isAdmin,
 			$this->tagManager,
 			$this->tagMapper
 		);

apps/dav/lib/systemtag/systemtagsobjecttypecollection.php

@@ -29,6 +29,8 @@ use Sabre\DAV\ICollection;
 
 use OCP\SystemTag\ISystemTagManager;
 use OCP\SystemTag\ISystemTagObjectMapper;
+use OCP\IUserSession;
+use OCP\IGroupManager;
 
 /**
  * Collection containing object ids by object type
@@ -50,17 +52,48 @@ class SystemTagsObjectTypeCollection implements ICollection {
 	 */
 	private $tagMapper;
 
+	/**
+	 * @var IGroupManager
+	 */
+	private $groupManager;
+
+	/**
+	 * @var IUserSession
+	 */
+	private $userSession;
+
 	/**
 	 * Constructor
 	 *
 	 * @param string $objectType object type
 	 * @param ISystemTagManager $tagManager
 	 * @param ISystemTagObjectMapper $tagMapper
+	 * @param IUserSession $userSession
+	 * @param IGroupManager $groupManager
 	 */
-	public function __construct($objectType, $tagManager, $tagMapper) {
+	public function __construct(
+		$objectType, 
+		ISystemTagManager $tagManager,
+		ISystemTagObjectMapper $tagMapper,
+		IUserSession $userSession,
+		IGroupManager $groupManager
+	) {
 		$this->tagManager = $tagManager;
 		$this->tagMapper = $tagMapper;
 		$this->objectType = $objectType;
+		$this->userSession = $userSession;
+		$this->groupManager = $groupManager;
+	}
+
+	/**
+	 * 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;
 	}
 
 	/**
@@ -86,6 +119,7 @@ class SystemTagsObjectTypeCollection implements ICollection {
 		return new SystemTagsObjectMappingCollection(
 			$objectId,
 			$this->objectType,
+			$this->isAdmin(),
 			$this->tagManager,
 			$this->tagMapper
 		);

apps/dav/lib/systemtag/systemtagsrelationscollection.php

@@ -34,10 +34,23 @@ class SystemTagsRelationsCollection extends SimpleCollection {
 	 *
 	 * @param ISystemTagManager $tagManager
 	 * @param ISystemTagObjectMapper $tagMapper
+	 * @param IUserSession $userSession
+	 * @param IGroupManager $groupManager
 	 */
-	public function __construct($tagManager, $tagMapper) {
+	public function __construct(
+		ISystemTagManager $tagManager,
+		ISystemTagObjectMapper $tagMapper,
+		IUserSession $userSession,
+		IGroupManager $groupManager
+	) {
 		$children = [
-			new SystemTagsObjectTypeCollection('files', $tagManager, $tagMapper),
+			new SystemTagsObjectTypeCollection(
+				'files',
+				$tagManager,
+				$tagMapper,
+				$userSession,
+				$groupManager
+			),
 		];
 
 		parent::__construct('root', $children);

apps/dav/tests/unit/systemtag/systemtagmappingnode.php

@@ -25,12 +25,7 @@ use Sabre\DAV\Exception\NotFound;
 use OC\SystemTag\SystemTag;
 use OCP\SystemTag\TagNotFoundException;
 
-class SystemTagMappingNode extends SystemTagNode {
+class SystemTagMappingNode extends \Test\TestCase {
-
-	/**
-	 * @var \OCA\DAV\SystemTag\SystemTagMappingNode
-	 */
-	private $node;
 
 	/**
 	 * @var \OCP\SystemTag\ISystemTagManager
@@ -42,41 +37,85 @@ class SystemTagMappingNode extends SystemTagNode {
 	 */
 	private $tagMapper;
 
-	/**
-	 * @var \OCP\SystemTag\ISystemTag
-	 */
-	private $tag;
-
 	protected function setUp() {
 		parent::setUp();
 
-		$this->tag = new SystemTag(1, 'Test', true, false);
 		$this->tagManager = $this->getMock('\OCP\SystemTag\ISystemTagManager');
 		$this->tagMapper = $this->getMock('\OCP\SystemTag\ISystemTagObjectMapper');
+	}
 
-		$this->node = new \OCA\DAV\SystemTag\SystemTagMappingNode(
+	public function getMappingNode($isAdmin = true, $tag = null) {
-			$this->tag,
+		if ($tag === null) {
+			$tag = new SystemTag(1, 'Test', true, true);
+		}
+		return new \OCA\DAV\SystemTag\SystemTagMappingNode(
+			$tag,
 			123,
 			'files',
+			$isAdmin,
 			$this->tagManager,
 			$this->tagMapper
 		);
 	}
 
 	public function testGetters() {
-		parent::testGetters();
+		$tag = new SystemTag(1, 'Test', true, false);
-		$this->assertEquals(123, $this->node->getObjectId());
+		$node = $this->getMappingNode(true, $tag);
-		$this->assertEquals('files', $this->node->getObjectType());
+		$this->assertEquals('1', $node->getName());
+		$this->assertEquals($tag, $node->getSystemTag());
+		$this->assertEquals(123, $node->getObjectId());
+		$this->assertEquals('files', $node->getObjectType());
 	}
 
-	public function testDeleteTag() {
+	public function adminFlagProvider() {
+		return [[true], [false]];
+	}
+
+	/**
+	 * @dataProvider adminFlagProvider
+	 */
+	public function testDeleteTag($isAdmin) {
 		$this->tagManager->expects($this->never())
 			->method('deleteTags');
 		$this->tagMapper->expects($this->once())
 			->method('unassignTags')
 			->with(123, 'files', 1);
 
-		$this->node->delete();
+		$this->getMappingNode($isAdmin)->delete();
+	}
+
+	public function tagNodeDeleteProviderPermissionException() {
+		return [
+			[
+				// cannot unassign invisible tag
+				new SystemTag(1, 'Original', false, true),
+				'Sabre\DAV\Exception\NotFound',
+			],
+			[
+				// cannot unassign non-assignable tag
+				new SystemTag(1, 'Original', true, false),
+				'Sabre\DAV\Exception\Forbidden',
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider tagNodeDeleteProviderPermissionException
+	 */
+	public function testDeleteTagExpectedException($tag, $expectedException) {
+		$this->tagManager->expects($this->never())
+			->method('deleteTags');
+		$this->tagMapper->expects($this->never())
+			->method('unassignTags');
+
+		$thrown = null;
+		try {
+			$this->getMappingNode(false, $tag)->delete();
+		} catch (\Exception $e) {
+			$thrown = $e;
+		}
+
+		$this->assertInstanceOf($expectedException, $thrown);
 	}
 
 	/**
@@ -88,6 +127,6 @@ class SystemTagMappingNode extends SystemTagNode {
 			->with(123, 'files', 1)
 			->will($this->throwException(new TagNotFoundException()));
 
-		$this->node->delete();
+		$this->getMappingNode()->delete();
 	}
 }

apps/dav/tests/unit/systemtag/systemtagnode.php

@@ -31,47 +31,141 @@ use OCP\SystemTag\TagAlreadyExistsException;
 
 class SystemTagNode extends \Test\TestCase {
 
-	/**
-	 * @var \OCA\DAV\SystemTag\SystemTagNode
-	 */
-	private $node;
-
 	/**
 	 * @var \OCP\SystemTag\ISystemTagManager
 	 */
 	private $tagManager;
 
-	/**
-	 * @var \OCP\SystemTag\ISystemTag
-	 */
-	private $tag;
-
 	protected function setUp() {
 		parent::setUp();
 
-		$this->tag = new SystemTag(1, 'Test', true, false);
 		$this->tagManager = $this->getMock('\OCP\SystemTag\ISystemTagManager');
-
-		$this->node = new \OCA\DAV\SystemTag\SystemTagNode($this->tag, $this->tagManager);
 	}
 
-	public function testGetters() {
+	protected function getTagNode($isAdmin = true, $tag = null) {
-		$this->assertEquals('1', $this->node->getName());
+		if ($tag === null) {
-		$this->assertEquals($this->tag, $this->node->getSystemTag());
+			$tag = new SystemTag(1, 'Test', true, true);
+		}
+		return new \OCA\DAV\SystemTag\SystemTagNode(
+			$tag,
+			$isAdmin,
+			$this->tagManager
+		);
+	}
+
+	public function adminFlagProvider() {
+		return [[true], [false]];
+	}
+
+	/**
+	 * @dataProvider adminFlagProvider
+	 */
+	public function testGetters($isAdmin) {
+		$tag = new SystemTag('1', 'Test', true, true);
+		$node = $this->getTagNode($isAdmin, $tag);
+		$this->assertEquals('1', $node->getName());
+		$this->assertEquals($tag, $node->getSystemTag());
 	}
 
 	/**
 	 * @expectedException Sabre\DAV\Exception\MethodNotAllowed
 	 */
 	public function testSetName() {
-		$this->node->setName('2');
+		$this->getTagNode()->setName('2');
 	}
 
-	public function testUpdateTag() {
+	public function tagNodeProvider() {
+		return [
+			// admin
+			[
+				true,
+				new SystemTag(1, 'Original', true, true),
+				['Renamed', true, true]
+			],
+			[
+				true,
+				new SystemTag(1, 'Original', true, true),
+				['Original', false, false]
+			],
+			// non-admin
+			[
+				// renaming allowed
+				false,
+				new SystemTag(1, 'Original', true, true),
+				['Rename', true, true]
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider tagNodeProvider
+	 */
+	public function testUpdateTag($isAdmin, $originalTag, $changedArgs) {
 		$this->tagManager->expects($this->once())
 			->method('updateTag')
-			->with(1, 'Renamed', false, true);
+			->with(1, $changedArgs[0], $changedArgs[1], $changedArgs[2]);
-		$this->node->update('Renamed', false, true);
+		$this->getTagNode($isAdmin, $originalTag)
+			->update($changedArgs[0], $changedArgs[1], $changedArgs[2]);
+	}
+
+	public function tagNodeProviderPermissionException() {
+		return [
+			[
+				// changing permissions not allowed
+				new SystemTag(1, 'Original', true, true),
+				['Original', false, true],
+				'Sabre\DAV\Exception\Forbidden',
+			],
+			[
+				// changing permissions not allowed
+				new SystemTag(1, 'Original', true, true),
+				['Original', true, false],
+				'Sabre\DAV\Exception\Forbidden',
+			],
+			[
+				// changing permissions not allowed
+				new SystemTag(1, 'Original', true, true),
+				['Original', false, false],
+				'Sabre\DAV\Exception\Forbidden',
+			],
+			[
+				// changing non-assignable not allowed
+				new SystemTag(1, 'Original', true, false),
+				['Rename', true, false],
+				'Sabre\DAV\Exception\Forbidden',
+			],
+			[
+				// changing non-assignable not allowed
+				new SystemTag(1, 'Original', true, false),
+				['Original', true, true],
+				'Sabre\DAV\Exception\Forbidden',
+			],
+			[
+				// invisible tag does not exist
+				new SystemTag(1, 'Original', false, false),
+				['Rename', false, false],
+				'Sabre\DAV\Exception\NotFound',
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider tagNodeProviderPermissionException
+	 */
+	public function testUpdateTagPermissionException($originalTag, $changedArgs, $expectedException = null) {
+		$this->tagManager->expects($this->never())
+			->method('updateTag');
+
+		$thrown = null;
+
+		try {
+			$this->getTagNode(false, $originalTag)
+				->update($changedArgs[0], $changedArgs[1], $changedArgs[2]);
+		} catch (\Exception $e) {
+			$thrown = $e;
+		}
+
+		$this->assertInstanceOf($expectedException, $thrown);
 	}
 
 	/**
@@ -82,7 +176,7 @@ class SystemTagNode extends \Test\TestCase {
 			->method('updateTag')
 			->with(1, 'Renamed', false, true)
 			->will($this->throwException(new TagAlreadyExistsException()));
-		$this->node->update('Renamed', false, true);
+		$this->getTagNode()->update('Renamed', false, true);
 	}
 
 	/**
@@ -93,14 +187,48 @@ class SystemTagNode extends \Test\TestCase {
 			->method('updateTag')
 			->with(1, 'Renamed', false, true)
 			->will($this->throwException(new TagNotFoundException()));
-		$this->node->update('Renamed', false, true);
+		$this->getTagNode()->update('Renamed', false, true);
 	}
 
-	public function testDeleteTag() {
+	/**
+	 * @dataProvider adminFlagProvider
+	 */
+	public function testDeleteTag($isAdmin) {
 		$this->tagManager->expects($this->once())
 			->method('deleteTags')
 			->with('1');
-		$this->node->delete();
+		$this->getTagNode($isAdmin)->delete();
+	}
+
+	public function tagNodeDeleteProviderPermissionException() {
+		return [
+			[
+				// cannot delete invisible tag
+				new SystemTag(1, 'Original', false, true),
+				'Sabre\DAV\Exception\NotFound',
+			],
+			[
+				// cannot delete non-assignable tag
+				new SystemTag(1, 'Original', true, false),
+				'Sabre\DAV\Exception\Forbidden',
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider tagNodeDeleteProviderPermissionException
+	 */
+	public function testDeleteTagPermissionException($tag, $expectedException) {
+		$this->tagManager->expects($this->never())
+			->method('deleteTags');
+
+		try {
+			$this->getTagNode(false, $tag)->delete();
+		} catch (\Exception $e) {
+			$thrown = $e;
+		}
+
+		$this->assertInstanceOf($expectedException, $thrown);
 	}
 
 	/**
@@ -111,6 +239,6 @@ class SystemTagNode extends \Test\TestCase {
 			->method('deleteTags')
 			->with('1')
 			->will($this->throwException(new TagNotFoundException()));
-		$this->node->delete();
+		$this->getTagNode()->delete();
 	}
 }

apps/dav/tests/unit/systemtag/systemtagsbyidcollection.php

@@ -27,11 +27,6 @@ use OCP\SystemTag\TagNotFoundException;
 
 class SystemTagsByIdCollection extends \Test\TestCase {
 
-	/**
-	 * @var \OCA\DAV\SystemTag\SystemTagsByIdCollection
-	 */
-	private $node;
-
 	/**
 	 * @var \OCP\SystemTag\ISystemTagManager
 	 */
@@ -41,33 +36,76 @@ class SystemTagsByIdCollection extends \Test\TestCase {
 		parent::setUp();
 
 		$this->tagManager = $this->getMock('\OCP\SystemTag\ISystemTagManager');
+	}
 
-		$this->node = new \OCA\DAV\SystemTag\SystemTagsByIdCollection($this->tagManager);
+	public function getNode($isAdmin = true) {
+		$user = $this->getMock('\OCP\IUser');
+		$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));
+		$groupManager = $this->getMock('\OCP\IGroupManager');
+		$groupManager->expects($this->any())
+			->method('isAdmin')
+			->with('testuser')
+			->will($this->returnValue($isAdmin));
+		return new \OCA\DAV\SystemTag\SystemTagsByIdCollection(
+			$this->tagManager,
+			$userSession,
+			$groupManager
+		);
+	}
+
+	public function adminFlagProvider() {
+		return [[true], [false]];
 	}
 
 	/**
 	 * @expectedException Sabre\DAV\Exception\Forbidden
 	 */
 	public function testForbiddenCreateFile() {
-		$this->node->createFile('555');
+		$this->getNode()->createFile('555');
 	}
 
 	/**
 	 * @expectedException Sabre\DAV\Exception\Forbidden
 	 */
 	public function testForbiddenCreateDirectory() {
-		$this->node->createDirectory('789');
+		$this->getNode()->createDirectory('789');
 	}
 
-	public function testGetChild() {
+	public function getChildProvider() {
-		$tag = new SystemTag(123, 'Test', true, false);
+		return [
+			[
+				true,
+				true,
+			],
+			[
+				true,
+				false,
+			],
+			[
+				false,
+				true,
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider getChildProvider
+	 */
+	public function testGetChild($isAdmin, $userVisible) {
+		$tag = new SystemTag(123, 'Test', $userVisible, false);
 
 		$this->tagManager->expects($this->once())
 			->method('getTagsByIds')
 			->with(['123'])
 			->will($this->returnValue([$tag]));
 
-		$childNode = $this->node->getChild('123');
+		$childNode = $this->getNode($isAdmin)->getChild('123');
 
 		$this->assertInstanceOf('\OCA\DAV\SystemTag\SystemTagNode', $childNode);
 		$this->assertEquals('123', $childNode->getName());
@@ -83,7 +121,7 @@ class SystemTagsByIdCollection extends \Test\TestCase {
 			->with(['invalid'])
 			->will($this->throwException(new \InvalidArgumentException()));
 
-		$this->node->getChild('invalid');
+		$this->getNode()->getChild('invalid');
 	}
 
 	/**
@@ -95,10 +133,43 @@ class SystemTagsByIdCollection extends \Test\TestCase {
 			->with(['444'])
 			->will($this->throwException(new TagNotFoundException()));
 
-		$this->node->getChild('444');
+		$this->getNode()->getChild('444');
 	}
 
-	public function testGetChildren() {
+	/**
+	 * @expectedException Sabre\DAV\Exception\NotFound
+	 */
+	public function testGetChildUserNotVisible() {
+		$tag = new SystemTag(123, 'Test', false, false);
+
+		$this->tagManager->expects($this->once())
+			->method('getTagsByIds')
+			->with(['123'])
+			->will($this->returnValue([$tag]));
+
+		$this->getNode(false)->getChild('123');
+	}
+
+	public function testGetChildrenAdmin() {
+		$tag1 = new SystemTag(123, 'One', true, false);
+		$tag2 = new SystemTag(456, 'Two', true, true);
+
+		$this->tagManager->expects($this->once())
+			->method('getAllTags')
+			->with(null)
+			->will($this->returnValue([$tag1, $tag2]));
+
+		$children = $this->getNode(true)->getChildren();
+
+		$this->assertCount(2, $children);
+
+		$this->assertInstanceOf('\OCA\DAV\SystemTag\SystemTagNode', $children[0]);
+		$this->assertInstanceOf('\OCA\DAV\SystemTag\SystemTagNode', $children[1]);
+		$this->assertEquals($tag1, $children[0]->getSystemTag());
+		$this->assertEquals($tag2, $children[1]->getSystemTag());
+	}
+
+	public function testGetChildrenNonAdmin() {
 		$tag1 = new SystemTag(123, 'One', true, false);
 		$tag2 = new SystemTag(456, 'Two', true, true);
 
@@ -107,7 +178,7 @@ class SystemTagsByIdCollection extends \Test\TestCase {
 			->with(true)
 			->will($this->returnValue([$tag1, $tag2]));
 
-		$children = $this->node->getChildren();
+		$children = $this->getNode(false)->getChildren();
 
 		$this->assertCount(2, $children);
 
@@ -120,20 +191,34 @@ class SystemTagsByIdCollection extends \Test\TestCase {
 	public function testGetChildrenEmpty() {
 		$this->tagManager->expects($this->once())
 			->method('getAllTags')
-			->with(true)
+			->with(null)
 			->will($this->returnValue([]));
-		$this->assertCount(0, $this->node->getChildren());
+		$this->assertCount(0, $this->getNode()->getChildren());
 	}
 
-	public function testChildExists() {
+	public function childExistsProvider() {
-		$tag = new SystemTag(123, 'One', true, false);
+		return [
+			// admins, always visible
+			[true, true, true],
+			[true, false, true],
+			// non-admins, depends on flag
+			[false, true, true],
+			[false, false, false],
+		];
+	}
+
+	/**
+	 * @dataProvider childExistsProvider
+	 */
+	public function testChildExists($isAdmin, $userVisible, $expectedResult) {
+		$tag = new SystemTag(123, 'One', $userVisible, false);
 
 		$this->tagManager->expects($this->once())
 			->method('getTagsByIds')
 			->with(['123'])
 			->will($this->returnValue([$tag]));
 
-		$this->assertTrue($this->node->childExists('123'));
+		$this->assertEquals($expectedResult, $this->getNode($isAdmin)->childExists('123'));
 	}
 
 	public function testChildExistsNotFound() {
@@ -142,7 +227,7 @@ class SystemTagsByIdCollection extends \Test\TestCase {
 			->with(['123'])
 			->will($this->throwException(new TagNotFoundException()));
 
-		$this->assertFalse($this->node->childExists('123'));
+		$this->assertFalse($this->getNode()->childExists('123'));
 	}
 
 	/**
@@ -154,6 +239,6 @@ class SystemTagsByIdCollection extends \Test\TestCase {
 			->with(['invalid'])
 			->will($this->throwException(new \InvalidArgumentException()));
 
-		$this->node->childExists('invalid');
+		$this->getNode()->childExists('invalid');
 	}
 }

apps/dav/tests/unit/systemtag/systemtagsobjectmappingcollection.php

@@ -27,11 +27,6 @@ use OCP\SystemTag\TagNotFoundException;
 
 class SystemTagsObjectMappingCollection extends \Test\TestCase {
 
-	/**
-	 * @var \OCA\DAV\SystemTag\SystemTagsObjectTypeCollection
-	 */
-	private $node;
-
 	/**
 	 * @var \OCP\SystemTag\ISystemTagManager
 	 */
@@ -47,21 +42,72 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase {
 
 		$this->tagManager = $this->getMock('\OCP\SystemTag\ISystemTagManager');
 		$this->tagMapper = $this->getMock('\OCP\SystemTag\ISystemTagObjectMapper');
+	}
 
-		$this->node = new \OCA\DAV\SystemTag\SystemTagsObjectMappingCollection (
+	public function getNode($isAdmin = true) {
+		return new \OCA\DAV\SystemTag\SystemTagsObjectMappingCollection (
 			111,
 			'files',
+			$isAdmin,
 			$this->tagManager,
 			$this->tagMapper
 		);
 	}
 
-	public function testAssignTag() {
+	public function testAssignTagAsAdmin() {
+		$this->tagManager->expects($this->never())
+			->method('getTagsByIds');
 		$this->tagMapper->expects($this->once())
 			->method('assignTags')
 			->with(111, 'files', '555');
 
-		$this->node->createFile('555');
+		$this->getNode(true)->createFile('555');
+	}
+
+	public function testAssignTagAsUser() {
+		$tag = new SystemTag('1', 'Test', true, true);
+
+		$this->tagManager->expects($this->once())
+			->method('getTagsByIds')
+			->with('555')
+			->will($this->returnValue([$tag]));
+		$this->tagMapper->expects($this->once())
+			->method('assignTags')
+			->with(111, 'files', '555');
+
+		$this->getNode(false)->createFile('555');
+	}
+
+	public function permissionsProvider() {
+		return [
+			// invisible, tag does not exist for user
+			[false, true, '\Sabre\DAV\Exception\PreconditionFailed'],
+			// visible but static, cannot assign tag
+			[true, false, '\Sabre\DAV\Exception\Forbidden'],
+		];
+	}
+
+	/**
+	 * @dataProvider permissionsProvider
+	 */
+	public function testAssignTagAsUserNoPermission($userVisible, $userAssignable, $expectedException) {
+		$tag = new SystemTag('1', 'Test', $userVisible, $userAssignable);
+
+		$this->tagManager->expects($this->once())
+			->method('getTagsByIds')
+			->with('555')
+			->will($this->returnValue([$tag]));
+		$this->tagMapper->expects($this->never())
+			->method('assignTags');
+
+		$thrown = null;
+		try {
+			$this->getNode(false)->createFile('555');
+		} catch (\Exception $e) {
+			$thrown = $e;
+		}
+
+		$this->assertInstanceOf($expectedException, $thrown);
 	}
 
 	/**
@@ -73,18 +119,38 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase {
 			->with(111, 'files', '555')
 			->will($this->throwException(new TagNotFoundException()));
 
-		$this->node->createFile('555');
+		$this->getNode()->createFile('555');
 	}
 
 	/**
 	 * @expectedException Sabre\DAV\Exception\Forbidden
 	 */
 	public function testForbiddenCreateDirectory() {
-		$this->node->createDirectory('789');
+		$this->getNode()->createDirectory('789');
 	}
 
-	public function testGetChild() {
+	public function getChildProvider() {
-		$tag = new SystemTag(555, 'TheTag', true, false);
+		return [
+			[
+				true,
+				true,
+			],
+			[
+				true,
+				false,
+			],
+			[
+				false,
+				true,
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider getChildProvider
+	 */
+	public function testGetChild($isAdmin, $userVisible) {
+		$tag = new SystemTag(555, 'TheTag', $userVisible, false);
 
 		$this->tagMapper->expects($this->once())
 			->method('haveTag')
@@ -96,12 +162,31 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase {
 			->with(['555'])
 			->will($this->returnValue(['555' => $tag]));
 
-		$childNode = $this->node->getChild('555');
+		$childNode = $this->getNode($isAdmin)->getChild('555');
 
 		$this->assertInstanceOf('\OCA\DAV\SystemTag\SystemTagNode', $childNode);
 		$this->assertEquals('555', $childNode->getName());
 	}
 
+	/**
+	 * @expectedException \Sabre\DAV\Exception\NotFound
+	 */
+	public function testGetChildUserNonVisible() {
+		$tag = new SystemTag(555, 'TheTag', false, false);
+
+		$this->tagMapper->expects($this->once())
+			->method('haveTag')
+			->with([111], 'files', '555', true)
+			->will($this->returnValue(true));
+
+		$this->tagManager->expects($this->once())
+			->method('getTagsByIds')
+			->with(['555'])
+			->will($this->returnValue(['555' => $tag]));
+
+		$this->getNode(false)->getChild('555');
+	}
+
 	/**
 	 * @expectedException Sabre\DAV\Exception\NotFound
 	 */
@@ -111,7 +196,7 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase {
 			->with([111], 'files', '777')
 			->will($this->returnValue(false));
 
-		$this->node->getChild('777');
+		$this->getNode()->getChild('777');
 	}
 
 	/**
@@ -123,7 +208,7 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase {
 			->with([111], 'files', 'badid')
 			->will($this->throwException(new \InvalidArgumentException()));
 
-		$this->node->getChild('badid');
+		$this->getNode()->getChild('badid');
 	}
 
 	/**
@@ -135,24 +220,61 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase {
 			->with([111], 'files', '777')
 			->will($this->throwException(new TagNotFoundException()));
 
-		$this->node->getChild('777');
+		$this->getNode()->getChild('777');
 	}
 
-	public function testGetChildren() {
+	public function testGetChildrenAsAdmin() {
 		$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']]));
+			->will($this->returnValue(['111' => ['555', '556', '557']]));
 
 		$this->tagManager->expects($this->once())
 			->method('getTagsByIds')
-			->with(['555', '556'])
+			->with(['555', '556', '557'])
-			->will($this->returnValue(['555' => $tag1, '666' => $tag2]));
+			->will($this->returnValue(['555' => $tag1, '556' => $tag2, '557' => $tag3]));
 
-		$children = $this->node->getChildren();
+		$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->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();
 
 		$this->assertCount(2, $children);
 
@@ -168,13 +290,45 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase {
 		$this->assertEquals($tag2, $children[1]->getSystemTag());
 	}
 
-	public function testChildExists() {
+	public function testChildExistsAsAdmin() {
 		$this->tagMapper->expects($this->once())
 			->method('haveTag')
 			->with([111], 'files', '555')
 			->will($this->returnValue(true));
 
-		$this->assertTrue($this->node->childExists('555'));
+		$this->assertTrue($this->getNode(true)->childExists('555'));
+	}
+
+	public function testChildExistsWithVisibleTagAsUser() {
+		$tag = new SystemTag(555, 'TagOne', true, false);
+
+		$this->tagMapper->expects($this->once())
+			->method('haveTag')
+			->with([111], 'files', '555')
+			->will($this->returnValue(true));
+
+		$this->tagManager->expects($this->once())
+			->method('getTagsByIds')
+			->with('555')
+			->will($this->returnValue([$tag]));
+
+		$this->assertTrue($this->getNode(false)->childExists('555'));
+	}
+
+	public function testChildExistsWithInvisibleTagAsUser() {
+		$tag = new SystemTag(555, 'TagOne', false, false);
+
+		$this->tagMapper->expects($this->once())
+			->method('haveTag')
+			->with([111], 'files', '555')
+			->will($this->returnValue(true));
+
+		$this->tagManager->expects($this->once())
+			->method('getTagsByIds')
+			->with('555')
+			->will($this->returnValue([$tag]));
+
+		$this->assertFalse($this->getNode(false)->childExists('555'));
 	}
 
 	public function testChildExistsNotFound() {
@@ -183,7 +337,7 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase {
 			->with([111], 'files', '555')
 			->will($this->returnValue(false));
 
-		$this->assertFalse($this->node->childExists('555'));
+		$this->assertFalse($this->getNode()->childExists('555'));
 	}
 
 	public function testChildExistsTagNotFound() {
@@ -192,7 +346,7 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase {
 			->with([111], 'files', '555')
 			->will($this->throwException(new TagNotFoundException()));
 
-		$this->assertFalse($this->node->childExists('555'));
+		$this->assertFalse($this->getNode()->childExists('555'));
 	}
 
 	/**
@@ -204,24 +358,24 @@ class SystemTagsObjectMappingCollection extends \Test\TestCase {
 			->with([111], 'files', '555')
 			->will($this->throwException(new \InvalidArgumentException()));
 
-		$this->node->childExists('555');
+		$this->getNode()->childExists('555');
 	}
 
 	/**
 	 * @expectedException Sabre\DAV\Exception\Forbidden
 	 */
 	public function testDelete() {
-		$this->node->delete();
+		$this->getNode()->delete();
 	}
 
 	/**
 	 * @expectedException Sabre\DAV\Exception\Forbidden
 	 */
 	public function testSetName() {
-		$this->node->setName('somethingelse');
+		$this->getNode()->setName('somethingelse');
 	}
 
 	public function testGetName() {
-		$this->assertEquals('111', $this->node->getName());
+		$this->assertEquals('111', $this->getNode()->getName());
 	}
 }

apps/dav/tests/unit/systemtag/systemtagsobjecttypecollection.php

@@ -44,10 +44,26 @@ class SystemTagsObjectTypeCollection extends \Test\TestCase {
 		$this->tagManager = $this->getMock('\OCP\SystemTag\ISystemTagManager');
 		$this->tagMapper = $this->getMock('\OCP\SystemTag\ISystemTagObjectMapper');
 
+		$user = $this->getMock('\OCP\IUser');
+		$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));
+		$groupManager = $this->getMock('\OCP\IGroupManager');
+		$groupManager->expects($this->any())
+			->method('isAdmin')
+			->with('testuser')
+			->will($this->returnValue(true));
+
 		$this->node = new \OCA\DAV\SystemTag\SystemTagsObjectTypeCollection(
 			'files',
 			$this->tagManager,
-			$this->tagMapper
+			$this->tagMapper,
+			$userSession,
+			$groupManager
 		);
 	}