Add interface methods for permission check

Instead of checking for admin perm, use interface method
canUserAssignTag and canUserSeeTag to check for permissions.
Allows for more flexible implementation.

apps/dav/lib/SystemTag/SystemTagMappingNode.php

@@ -56,7 +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 string $userId user id
 	 * @param ISystemTagManager $tagManager
 	 * @param ISystemTagObjectMapper $tagMapper
 	 */
@@ -64,14 +64,14 @@ class SystemTagMappingNode extends SystemTagNode {
 		ISystemTag $tag,
 		$objectId,
 		$objectType,
-		$isAdmin,
+		$userId,
 		ISystemTagManager $tagManager,
 		ISystemTagObjectMapper $tagMapper
 	) {
 		$this->objectId = $objectId;
 		$this->objectType = $objectType;
 		$this->tagMapper = $tagMapper;
-		parent::__construct($tag, $isAdmin, $tagManager);
+		parent::__construct($tag, $userId, $tagManager);
 	}
 
 	/**
@@ -97,13 +97,11 @@ 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());
-				}
+			if (!$this->tagManager->canUserSeeTag($this->tag, $this->userId)) {
+				throw new NotFound('Tag with id ' . $this->tag->getId() . ' not found');
+			}
+			if (!$this->tagManager->canUserAssignTag($this->tag, $this->userId)) {
+				throw new Forbidden('No permission to unassign tag ' . $this->tag->getId());
 			}
 			$this->tagMapper->unassignTags($this->objectId, $this->objectType, $this->tag->getId());
 		} catch (TagNotFoundException $e) {

apps/dav/lib/SystemTag/SystemTagNode.php

@@ -49,22 +49,22 @@ class SystemTagNode implements \Sabre\DAV\INode {
 	protected $tagManager;
 
 	/**
-	 * Whether to allow permissions for admins
+	 * User id
 	 *
-	 * @var bool
+	 * @var string
 	 */
-	protected $isAdmin;
+	protected $userId;
 
 	/**
 	 * 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
+	 * @param string $userId user id
+	 * @param ISystemTagManager $tagManager tag manager
 	 */
-	public function __construct(ISystemTag $tag, $isAdmin, ISystemTagManager $tagManager) {
+	public function __construct(ISystemTag $tag, $userId, ISystemTagManager $tagManager) {
 		$this->tag = $tag;
-		$this->isAdmin = $isAdmin;
+		$this->userId = $userId;
 		$this->tagManager = $tagManager;
 	}
 
@@ -109,21 +109,22 @@ 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());
-				}
+			if (!$this->tagManager->canUserSeeTag($this->tag, $this->userId)) {
+				throw new NotFound('Tag with id ' . $this->tag->getId() . ' does not exist');
 			}
+			if (!$this->tagManager->canUserAssignTag($this->tag, $this->userId)) {
+				throw new Forbidden('No permission to update tag ' . $this->tag->getId());
+			}
+
+			// FIXME: admin should be able to change permissions still
+
+			// 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');
@@ -145,14 +146,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->userId)) {
+				throw new NotFound('Tag with id ' . $this->tag->getId() . ' not found');
+			}
+			if (!$this->tagManager->canUserAssignTag($this->tag, $this->userId)) {
+				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

@@ -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();
@@ -78,6 +81,21 @@ class SystemTagsByIdCollection implements ICollection {
 		return false;
 	}
 
+	/**
+	 * Returns the user id
+	 *
+	 * @return string user id
+	 *
+	 * @throws NoUserException if no user exists in the session
+	 */
+	private function getUserId() {
+		$user = $this->userSession->getUser();
+		if ($user !== null) {
+			return $user->getUID();
+		}
+		throw new NoUserException();
+	}
+
 	/**
 	 * @param string $name
 	 * @param resource|string $data Initial payload
@@ -101,7 +119,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->getUserId())) {
 				throw new NotFound('Tag with id ' . $name . ' not found');
 			}
 			return $this->makeNode($tag);
@@ -131,7 +149,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->getUserId())) {
 				return false;
 			}
 			return true;
@@ -171,6 +189,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->getUserId(), $this->tagManager);
 	}
 }

apps/dav/lib/SystemTag/SystemTagsObjectMappingCollection.php

@@ -58,11 +58,11 @@ class SystemTagsObjectMappingCollection implements ICollection {
 	private $tagMapper;
 
 	/**
-	 * Whether to return results only visible for admins
+	 * User id
 	 *
-	 * @var bool
+	 * @var string
 	 */
-	private $isAdmin;
+	private $userId;
 
 
 	/**
@@ -70,30 +70,29 @@ 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 string $userId user id
 	 * @param ISystemTagManager $tagManager
 	 * @param ISystemTagObjectMapper $tagMapper
 	 */
-	public function __construct($objectId, $objectType, $isAdmin, $tagManager, $tagMapper) {
+	public function __construct($objectId, $objectType, $userId, $tagManager, $tagMapper) {
 		$this->tagManager = $tagManager;
 		$this->tagMapper = $tagMapper;
 		$this->objectId = $objectId;
 		$this->objectType = $objectType;
-		$this->isAdmin = $isAdmin;
+		$this->userId = $userId;
 	}
 
 	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->userId)) {
+				throw new PreconditionFailed('Tag with id ' . $tagId . ' does not exist, cannot assign');
+			}
+			if (!$this->tagManager->canUserAssignTag($tag, $this->userId)) {
+				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 +108,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->userId)) {
 					return $this->makeNode($tag);
 				}
 			}
@@ -127,12 +126,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->userId);
+		});
+
 		return array_values(array_map(function($tag) {
 			return $this->makeNode($tag);
 		}, $tags));
@@ -141,17 +140,12 @@ 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()) {
+			if ($result && !$this->tagManager->canUserSeeTag($tagId, $this->userId)) {
 				return false;
 			}
-			return true;
+
+			return $result;
 		} catch (\InvalidArgumentException $e) {
 			throw new BadRequest('Invalid tag id', 0, $e);
 		} catch (TagNotFoundException $e) {
@@ -193,7 +187,7 @@ class SystemTagsObjectMappingCollection implements ICollection {
 			$tag,
 			$this->objectId,
 			$this->objectType,
-			$this->isAdmin,
+			$this->userId,
 			$this->tagManager,
 			$this->tagMapper
 		);

apps/dav/lib/SystemTag/SystemTagsObjectTypeCollection.php

@@ -95,14 +95,18 @@ class SystemTagsObjectTypeCollection implements ICollection {
 	}
 
 	/**
-	 * Returns whether the currently logged in user is an administrator
+	 * Returns the user id
+	 *
+	 * @return string user id
+	 *
+	 * @throws NoUserException if no user exists in the session
 	 */
-	private function isAdmin() {
+	private function getUserId() {
 		$user = $this->userSession->getUser();
 		if ($user !== null) {
-			return $this->groupManager->isAdmin($user->getUID());
+			return $user->getUID();
 		}
-		return false;
+		throw new NoUserException();
 	}
 
 	/**
@@ -132,7 +136,7 @@ class SystemTagsObjectTypeCollection implements ICollection {
 		return new SystemTagsObjectMappingCollection(
 			$objectId,
 			$this->objectType,
-			$this->isAdmin(),
+			$this->getUserId(),
 			$this->tagManager,
 			$this->tagMapper
 		);

lib/private/SystemTag/ManagerFactory.php

@@ -59,6 +59,8 @@ class ManagerFactory implements ISystemTagManagerFactory {
 	public function getManager() {
 		return new SystemTagManager(
 			$this->serverContainer->getDatabaseConnection(),
+			$this->serverContainer->getUserManager(),
+			$this->serverContainer->getGroupManager(),
 			$this->serverContainer->getEventDispatcher()
 		);
 	}

lib/private/SystemTag/SystemTagManager.php

@@ -30,7 +30,14 @@ 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\UserNotFoundException;
 
+/**
+ * Manager class for system tags
+ */
 class SystemTagManager implements ISystemTagManager {
 
 	const TAG_TABLE = 'systemtag';
@@ -41,6 +48,12 @@ class SystemTagManager implements ISystemTagManager {
 	/** @var EventDispatcherInterface */
 	protected $dispatcher;
 
+	/** @var IUserManager */
+	protected $userManager;
+
+	/** @var IGroupManager */
+	protected $groupManager;
+
 	/**
 	 * Prepared query for selecting tags directly
 	 *
@@ -54,8 +67,15 @@ class SystemTagManager implements ISystemTagManager {
 	 * @param IDBConnection $connection database connection
 	 * @param EventDispatcherInterface $dispatcher
 	 */
-	public function __construct(IDBConnection $connection, EventDispatcherInterface $dispatcher) {
+	public function __construct(
+		IDBConnection $connection,
+		IUserManager $userManager,
+		IGroupManager $groupManager,
+		EventDispatcherInterface $dispatcher
+	) {
 		$this->connection = $connection;
+		$this->userManager = $userManager;
+		$this->groupManager = $groupManager;
 		$this->dispatcher = $dispatcher;
 
 		$query = $this->connection->getQueryBuilder();
@@ -316,6 +336,58 @@ class SystemTagManager implements ISystemTagManager {
 		}
 	}
 
+	/**
+	 * {@inheritdoc}
+	 */
+	public function canUserAssignTag($tag, $userId) {
+		if (!$tag instanceof ISystemTag) {
+			$tags = $this->getTagsByIds([$tag]);
+			/** @var ISystemTag $tag */
+			$tag = current($tags);
+		}
+
+		if ($tag->isUserAssignable()) {
+			return true;
+		}
+
+		$user = $this->userManager->get($userId);
+		if ($user === null) {
+			throw new UserNotFoundException($userId);
+		}
+
+		if ($this->groupManager->isAdmin($userId)) {
+			return true;
+		}
+
+		return false;
+	}
+
+	/**
+	 * {@inheritdoc}
+	 */
+	public function canUserSeeTag($tag, $userId) {
+		if (!$tag instanceof ISystemTag) {
+			$tags = $this->getTagsByIds([$tag]);
+			/** @var ISystemTag $tag */
+			$tag = current($tags);
+		}
+
+		if ($tag->isUserVisible()) {
+			return true;
+		}
+
+		$user = $this->userManager->get($userId);
+		if ($user === null) {
+			throw new UserNotFoundException($userId);
+		}
+
+		if ($this->groupManager->isAdmin($userId)) {
+			return true;
+		}
+
+		return false;
+	}
+
 	private function createSystemTagFromRow($row) {
 		return new SystemTag((int)$row['id'], $row['name'], (bool)$row['visibility'], (bool)$row['editable']);
 	}

lib/public/SystemTag/ISystemTagManager.php

@@ -113,4 +113,37 @@ interface ISystemTagManager {
 	 */
 	public function deleteTags($tagIds);
 
+	/**
+	 * Checks whether the given user is allowed to assign/unassign the tag with the
+	 * given id.
+	 *
+	 * @param string|\OCP\SystemTag\ISystemTag $tag tag id or system tag
+	 * @param string $userId user id
+	 *
+	 * @return true if the user is allowed to assign/unassign the tag, false otherwise
+	 *
+	 * @throws \OCP\SystemTag\TagNotFoundException if tag with the given id does not exist
+	 * @throws \OCP\UserNotFoundException if the given user id does not exist
+	 * @throws \InvalidArgumentException if the tag id is invalid (string instead of integer, etc.)
+	 *
+	 * @since 9.1.0
+	 */
+	public function canUserAssignTag($tag, $userId);
+
+	/**
+	 * Checks whether the given user is allowed to see the tag with the given id.
+	 *
+	 * @param string|\OCP\SystemTag\ISystemTag $tag tag id or system tag
+	 * @param string $userId user id
+	 *
+	 * @return true if the user is allowed to assign/unassign the tag, false otherwise
+	 *
+	 * @throws \OCP\SystemTag\TagNotFoundException if tag with the given id does not exist
+	 * @throws \OCP\UserNotFoundException if the given user id does not exist
+	 * @throws \InvalidArgumentException if the tag id is invalid (string instead of integer, etc.)
+	 *
+	 * @since 9.1.0
+	 */
+	public function canUserSeeTag($tag, $userId);
+
 }

lib/public/UserNotFoundException.php

@@ -0,0 +1,62 @@
+<?php
+/**
+ * @author Vincent Petry <pvince81@owncloud.com>
+ *
+ * @copyright Copyright (c) 2016, ownCloud, Inc.
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace OCP;
+
+/**
+ * Exception when a user was not found
+ *
+ * @since 9.1.0
+ */
+class UserNotFoundException extends \RuntimeException {
+
+	/**
+	 * User id that was not found
+	 *
+	 * @var string
+	 */
+	private $userId;
+
+	/**
+	 * UserNotFoundException constructor.
+	 *
+	 * @param string $message message
+	 * @param int $code error code
+	 * @param \Exception $previous previous exception
+	 * @param string $userId user id
+	 *
+	 * @since 9.1.0
+	 */
+	public function __construct($message = '', $code = 0, \Exception $previous = null, $userId = null) {
+		parent::__construct($message, $code, $previous);
+		$this->userId = $userId;
+	}
+
+	/**
+	 * Returns the user id that was not found
+	 *
+	 * @return string
+	 * @since 9.1.0
+	 */
+	public function getUserId() {
+		return $this->userId;
+	}
+}

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
@@ -36,6 +38,16 @@ class SystemTagManagerTest extends TestCase {
 	 */
 	private $connection;
 
+	/**
+	 * @var IGroupManager
+	 */
+	private $groupManager;
+
+	/**
+	 * @var IUserManager
+	 */
+	private $userManager;
+
 	/**
 	 * @var EventDispatcherInterface
 	 */
@@ -49,8 +61,16 @@ class SystemTagManagerTest extends TestCase {
 		$this->dispatcher = $this->getMockBuilder('Symfony\Component\EventDispatcher\EventDispatcherInterface')
 			->getMock();
 
+		$this->userManager = $this->getMockBuilder('\OCP\IUserManager')->getMock();
+		$this->groupManager = $this->getMockBuilder('\OCP\IGroupManager')->getMock();
+		$this->groupManager->expects($this->any())
+			->method('isAdmin')
+			->will($this->returnValue(false));
+
 		$this->tagManager = new SystemTagManager(
 			$this->connection,
+			$this->userManager,
+			$this->groupManager,
 			$this->dispatcher
 		);
 		$this->pruneTagsTables();
@@ -410,6 +430,68 @@ 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) {
+		$userId = 'test';
+		$tag1 = $this->tagManager->createTag('one', $userVisible, $userAssignable);
+
+		$this->userManager->expects($this->once())
+			->method('get')
+			->with($userId)
+			->will($this->returnValue([]));
+		$this->groupManager->expects($this->once())
+			->method('isAdmin')
+			->with($userId)
+			->will($this->returnValue($isAdmin));
+
+		$this->assertEquals($expectedResult, $this->tagManager->canUserSeeTag($tag1, $userID));
+		$this->assertEquals($expectedResult, $this->tagManager->canUserSeeTag($tag1->getId(), $userID));
+	}
+
+	public function assignabilityCheckProvider() {
+		return [
+			[false, false, false, false],
+			[true, false, false, false],
+			[true, true, false, true],
+			[false, true, false, false],
+			[false, false, true, true],
+			[false, true, true, true],
+			[true, false, true, true],
+			[true, true, true, true],
+		];
+	}
+
+	/**
+	 * @dataProvider assignabilityCheckProvider
+	 */
+	public function testVisibilityCheck($userVisible, $userAssignable, $isAdmin, $expectedResult) {
+		$userId = 'test';
+		$tag1 = $this->tagManager->createTag('one', $userVisible, $userAssignable);
+
+		$this->userManager->expects($this->once())
+			->method('get')
+			->with($userId)
+			->will($this->returnValue([]));
+		$this->groupManager->expects($this->once())
+			->method('isAdmin')
+			->with($userId)
+			->will($this->returnValue($isAdmin));
+
+		$this->assertEquals($expectedResult, $this->tagManager->canUserAssignTag($tag1, $userID));
+		$this->assertEquals($expectedResult, $this->tagManager->canUserAssignTag($tag1->getId(), $userID));
+	}
+
 	/**
 	 * @param ISystemTag $tag1
 	 * @param ISystemTag $tag2