fix(systemtags): Forbid tagging of readonly files

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>

apps/dav/lib/SystemTag/SystemTagMappingNode.php

@@ -37,62 +37,15 @@ use Sabre\DAV\Exception\NotFound;
  * Mapping node for system tag to object id
  */
 class SystemTagMappingNode implements \Sabre\DAV\INode {
-	/**
-	 * @var ISystemTag
-	 */
-	protected $tag;
-
-	/**
-	 * @var string
-	 */
-	private $objectId;
-
-	/**
-	 * @var string
-	 */
-	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 IUser $user user
-	 * @param ISystemTagManager $tagManager
-	 * @param ISystemTagObjectMapper $tagMapper
-	 */
 	public function __construct(
-		ISystemTag $tag,
-		$objectId,
-		$objectType,
-		IUser $user,
-		ISystemTagManager $tagManager,
-		ISystemTagObjectMapper $tagMapper
+		private ISystemTag $tag,
+		private string $objectId,
+		private string $objectType,
+		private IUser $user,
+		private ISystemTagManager $tagManager,
+		private ISystemTagObjectMapper $tagMapper,
+		private \Closure $childWriteAccessFunction,
 	) {
-		$this->tag = $tag;
-		$this->objectId = $objectId;
-		$this->objectType = $objectType;
-		$this->user = $user;
-		$this->tagManager = $tagManager;
-		$this->tagMapper = $tagMapper;
 	}
 
 	/**
@@ -166,6 +119,9 @@ class SystemTagMappingNode implements \Sabre\DAV\INode {
 			if (!$this->tagManager->canUserAssignTag($this->tag, $this->user)) {
 				throw new Forbidden('No permission to unassign tag ' . $this->tag->getId());
 			}
+			if (!($this->childWriteAccessFunction)($this->objectId)) {
+				throw new Forbidden('No permission to unassign tag to ' . $this->objectId);
+			}
 			$this->tagMapper->unassignTags($this->objectId, $this->objectType, $this->tag->getId());
 		} catch (TagNotFoundException $e) {
 			// can happen if concurrent deletion occurred

apps/dav/lib/SystemTag/SystemTagsObjectMappingCollection.php

@@ -40,56 +40,14 @@ use Sabre\DAV\ICollection;
  * Collection containing tags by object id
  */
 class SystemTagsObjectMappingCollection implements ICollection {
-
-	/**
-	 * @var string
-	 */
-	private $objectId;
-
-	/**
-	 * @var string
-	 */
-	private $objectType;
-
-	/**
-	 * @var ISystemTagManager
-	 */
-	private $tagManager;
-
-	/**
-	 * @var ISystemTagObjectMapper
-	 */
-	private $tagMapper;
-
-	/**
-	 * User
-	 *
-	 * @var IUser
-	 */
-	private $user;
-
-
-	/**
-	 * Constructor
-	 *
-	 * @param string $objectId object id
-	 * @param string $objectType object type
-	 * @param IUser $user user
-	 * @param ISystemTagManager $tagManager tag manager
-	 * @param ISystemTagObjectMapper $tagMapper tag mapper
-	 */
 	public function __construct(
-		$objectId,
-		$objectType,
-		IUser $user,
-		ISystemTagManager $tagManager,
-		ISystemTagObjectMapper $tagMapper
+		private string $objectId,
+		private string $objectType,
+		private IUser $user,
+		private ISystemTagManager $tagManager,
+		private ISystemTagObjectMapper $tagMapper,
+		protected \Closure $childWriteAccessFunction,
 	) {
-		$this->tagManager = $tagManager;
-		$this->tagMapper = $tagMapper;
-		$this->objectId = $objectId;
-		$this->objectType = $objectType;
-		$this->user = $user;
 	}
 
 	/**
@@ -106,7 +64,9 @@ class SystemTagsObjectMappingCollection implements ICollection {
 			if (!$this->tagManager->canUserAssignTag($tag, $this->user)) {
 				throw new Forbidden('No permission to assign tag ' . $tagId);
 			}
-
+			if (!($this->childWriteAccessFunction)($this->objectId)) {
+				throw new Forbidden('No permission to assign tag to ' . $this->objectId);
+			}
 			$this->tagMapper->assignTags($this->objectId, $this->objectType, $tagId);
 		} catch (TagNotFoundException $e) {
 			throw new PreconditionFailed('Tag with id ' . $tagId . ' does not exist, cannot assign');
@@ -224,7 +184,8 @@ class SystemTagsObjectMappingCollection implements ICollection {
 			$this->objectType,
 			$this->user,
 			$this->tagManager,
-			$this->tagMapper
+			$this->tagMapper,
+			$this->childWriteAccessFunction,
 		);
 	}
 }

apps/dav/lib/SystemTag/SystemTagsObjectTypeCollection.php

@@ -38,61 +38,15 @@ use Sabre\DAV\ICollection;
  * Collection containing object ids by object type
  */
 class SystemTagsObjectTypeCollection implements ICollection {
-
-	/**
-	 * @var string
-	 */
-	private $objectType;
-
-	/**
-	 * @var ISystemTagManager
-	 */
-	private $tagManager;
-
-	/**
-	 * @var ISystemTagObjectMapper
-	 */
-	private $tagMapper;
-
-	/**
-	 * @var IGroupManager
-	 */
-	private $groupManager;
-
-	/**
-	 * @var IUserSession
-	 */
-	private $userSession;
-
-	/**
-	 * @var \Closure
-	 **/
-	protected $childExistsFunction;
-
-	/**
-	 * Constructor
-	 *
-	 * @param string $objectType object type
-	 * @param ISystemTagManager $tagManager
-	 * @param ISystemTagObjectMapper $tagMapper
-	 * @param IUserSession $userSession
-	 * @param IGroupManager $groupManager
-	 * @param \Closure $childExistsFunction
-	 */
 	public function __construct(
-		$objectType,
-		ISystemTagManager $tagManager,
-		ISystemTagObjectMapper $tagMapper,
-		IUserSession $userSession,
-		IGroupManager $groupManager,
-		\Closure $childExistsFunction
+		private string $objectType,
+		private ISystemTagManager $tagManager,
+		private ISystemTagObjectMapper $tagMapper,
+		private IUserSession $userSession,
+		private IGroupManager $groupManager,
+		protected \Closure $childExistsFunction,
+		protected \Closure $childWriteAccessFunction,
 	) {
-		$this->tagManager = $tagManager;
-		$this->tagMapper = $tagMapper;
-		$this->objectType = $objectType;
-		$this->userSession = $userSession;
-		$this->groupManager = $groupManager;
-		$this->childExistsFunction = $childExistsFunction;
 	}
 
 	/**
@@ -134,7 +88,8 @@ class SystemTagsObjectTypeCollection implements ICollection {
 			$this->objectType,
 			$this->userSession->getUser(),
 			$this->tagManager,
-			$this->tagMapper
+			$this->tagMapper,
+			$this->childWriteAccessFunction,
 		);
 	}
 

apps/dav/lib/SystemTag/SystemTagsRelationsCollection.php

@@ -26,6 +26,7 @@
  */
 namespace OCA\DAV\SystemTag;
 
+use OCP\Constants;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\IGroupManager;
 use OCP\IUserSession;
@@ -50,10 +51,19 @@ class SystemTagsRelationsCollection extends SimpleCollection {
 				$tagMapper,
 				$userSession,
 				$groupManager,
-				function ($name) {
+				function ($name): bool {
 					$nodes = \OC::$server->getUserFolder()->getById((int)$name);
 					return !empty($nodes);
-				}
+				},
+				function ($name): bool {
+					$nodes = \OC::$server->getUserFolder()->getById((int)$name);
+					foreach ($nodes as $node) {
+						if (($node->getPermissions() & Constants::PERMISSION_UPDATE) === Constants::PERMISSION_UPDATE) {
+							return true;
+						}
+					}
+					return false;
+				},
 			),
 		];
 
@@ -68,7 +78,8 @@ class SystemTagsRelationsCollection extends SimpleCollection {
 				$tagMapper,
 				$userSession,
 				$groupManager,
-				$entityExistsFunction
+				$entityExistsFunction,
+				fn ($name) => true,
 			);
 		}
 

apps/dav/tests/unit/SystemTag/SystemTagMappingNodeTest.php

@@ -33,21 +33,9 @@ use OCP\SystemTag\ISystemTagObjectMapper;
 use OCP\SystemTag\TagNotFoundException;
 
 class SystemTagMappingNodeTest extends \Test\TestCase {
-
-	/**
-	 * @var \OCP\SystemTag\ISystemTagManager
-	 */
-	private $tagManager;
-
-	/**
-	 * @var \OCP\SystemTag\ISystemTagObjectMapper
-	 */
-	private $tagMapper;
-
-	/**
-	 * @var \OCP\IUser
-	 */
-	private $user;
+	private ISystemTagManager $tagManager;
+	private ISystemTagObjectMapper $tagMapper;
+	private IUser $user;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -60,7 +48,7 @@ class SystemTagMappingNodeTest extends \Test\TestCase {
 			->getMock();
 	}
 
-	public function getMappingNode($tag = null) {
+	public function getMappingNode($tag = null, array $writableNodeIds = []) {
 		if ($tag === null) {
 			$tag = new SystemTag(1, 'Test', true, true);
 		}
@@ -70,7 +58,8 @@ class SystemTagMappingNodeTest extends \Test\TestCase {
 			'files',
 			$this->user,
 			$this->tagManager,
-			$this->tagMapper
+			$this->tagMapper,
+			fn ($id): bool => in_array($id, $writableNodeIds),
 		);
 	}
 
@@ -84,7 +73,7 @@ class SystemTagMappingNodeTest extends \Test\TestCase {
 	}
 
 	public function testDeleteTag(): void {
-		$node = $this->getMappingNode();
+		$node = $this->getMappingNode(null, [123]);
 		$this->tagManager->expects($this->once())
 			->method('canUserSeeTag')
 			->with($node->getSystemTag())
@@ -102,6 +91,25 @@ class SystemTagMappingNodeTest extends \Test\TestCase {
 		$node->delete();
 	}
 
+	public function testDeleteTagForbidden(): void {
+		$node = $this->getMappingNode();
+		$this->tagManager->expects($this->once())
+			->method('canUserSeeTag')
+			->with($node->getSystemTag())
+			->willReturn(true);
+		$this->tagManager->expects($this->once())
+			->method('canUserAssignTag')
+			->with($node->getSystemTag())
+			->willReturn(true);
+		$this->tagManager->expects($this->never())
+			->method('deleteTags');
+		$this->tagMapper->expects($this->never())
+			->method('unassignTags');
+
+		$this->expectException(\Sabre\DAV\Exception\Forbidden::class);
+		$node->delete();
+	}
+
 	public function tagNodeDeleteProviderPermissionException() {
 		return [
 			[
@@ -144,7 +152,7 @@ class SystemTagMappingNodeTest extends \Test\TestCase {
 		$this->assertInstanceOf($expectedException, $thrown);
 	}
 
-	
+
 	public function testDeleteTagNotFound(): void {
 		$this->expectException(\Sabre\DAV\Exception\NotFound::class);
 
@@ -164,6 +172,6 @@ class SystemTagMappingNodeTest extends \Test\TestCase {
 			->with(123, 'files', 1)
 			->will($this->throwException(new TagNotFoundException()));
 
-		$this->getMappingNode($tag)->delete();
+		$this->getMappingNode($tag, [123])->delete();
 	}
 }

apps/dav/tests/unit/SystemTag/SystemTagsObjectMappingCollectionTest.php

@@ -32,21 +32,9 @@ use OCP\SystemTag\ISystemTagObjectMapper;
 use OCP\SystemTag\TagNotFoundException;
 
 class SystemTagsObjectMappingCollectionTest extends \Test\TestCase {
-
-	/**
-	 * @var \OCP\SystemTag\ISystemTagManager
-	 */
-	private $tagManager;
-
-	/**
-	 * @var \OCP\SystemTag\ISystemTagObjectMapper
-	 */
-	private $tagMapper;
-
-	/**
-	 * @var \OCP\IUser
-	 */
-	private $user;
+	private ISystemTagManager $tagManager;
+	private ISystemTagObjectMapper $tagMapper;
+	private IUser $user;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -60,13 +48,14 @@ class SystemTagsObjectMappingCollectionTest extends \Test\TestCase {
 			->getMock();
 	}
 
-	public function getNode() {
+	public function getNode(array $writableNodeIds = []) {
 		return new \OCA\DAV\SystemTag\SystemTagsObjectMappingCollection(
 			111,
 			'files',
 			$this->user,
 			$this->tagManager,
-			$this->tagMapper
+			$this->tagMapper,
+			fn ($id): bool => in_array($id, $writableNodeIds),
 		);
 	}
 
@@ -89,6 +78,28 @@ class SystemTagsObjectMappingCollectionTest extends \Test\TestCase {
 			->method('assignTags')
 			->with(111, 'files', '555');
 
+		$this->getNode([111])->createFile('555');
+	}
+
+	public function testAssignTagForbidden(): void {
+		$tag = new SystemTag('1', 'Test', true, true);
+		$this->tagManager->expects($this->once())
+			->method('canUserSeeTag')
+			->with($tag)
+			->willReturn(true);
+		$this->tagManager->expects($this->once())
+			->method('canUserAssignTag')
+			->with($tag)
+			->willReturn(true);
+
+		$this->tagManager->expects($this->once())
+			->method('getTagsByIds')
+			->with(['555'])
+			->willReturn([$tag]);
+		$this->tagMapper->expects($this->never())
+			->method('assignTags');
+
+		$this->expectException(\Sabre\DAV\Exception\Forbidden::class);
 		$this->getNode()->createFile('555');
 	}
 
@@ -132,7 +143,7 @@ class SystemTagsObjectMappingCollectionTest extends \Test\TestCase {
 		$this->assertInstanceOf($expectedException, $thrown);
 	}
 
-	
+
 	public function testAssignTagNotFound(): void {
 		$this->expectException(\Sabre\DAV\Exception\PreconditionFailed::class);
 
@@ -144,7 +155,7 @@ class SystemTagsObjectMappingCollectionTest extends \Test\TestCase {
 		$this->getNode()->createFile('555');
 	}
 
-	
+
 	public function testForbiddenCreateDirectory(): void {
 		$this->expectException(\Sabre\DAV\Exception\Forbidden::class);
 
@@ -174,7 +185,7 @@ class SystemTagsObjectMappingCollectionTest extends \Test\TestCase {
 		$this->assertEquals('555', $childNode->getName());
 	}
 
-	
+
 	public function testGetChildNonVisible(): void {
 		$this->expectException(\Sabre\DAV\Exception\NotFound::class);
 
@@ -197,7 +208,7 @@ class SystemTagsObjectMappingCollectionTest extends \Test\TestCase {
 		$this->getNode()->getChild('555');
 	}
 
-	
+
 	public function testGetChildRelationNotFound(): void {
 		$this->expectException(\Sabre\DAV\Exception\NotFound::class);
 
@@ -209,7 +220,7 @@ class SystemTagsObjectMappingCollectionTest extends \Test\TestCase {
 		$this->getNode()->getChild('777');
 	}
 
-	
+
 	public function testGetChildInvalidId(): void {
 		$this->expectException(\Sabre\DAV\Exception\BadRequest::class);
 
@@ -221,7 +232,7 @@ class SystemTagsObjectMappingCollectionTest extends \Test\TestCase {
 		$this->getNode()->getChild('badid');
 	}
 
-	
+
 	public function testGetChildTagDoesNotExist(): void {
 		$this->expectException(\Sabre\DAV\Exception\NotFound::class);
 
@@ -325,7 +336,7 @@ class SystemTagsObjectMappingCollectionTest extends \Test\TestCase {
 		$this->assertFalse($this->getNode()->childExists('555'));
 	}
 
-	
+
 	public function testChildExistsInvalidId(): void {
 		$this->expectException(\Sabre\DAV\Exception\BadRequest::class);
 
@@ -337,14 +348,14 @@ class SystemTagsObjectMappingCollectionTest extends \Test\TestCase {
 		$this->getNode()->childExists('555');
 	}
 
-	
+
 	public function testDelete(): void {
 		$this->expectException(\Sabre\DAV\Exception\Forbidden::class);
 
 		$this->getNode()->delete();
 	}
 
-	
+
 	public function testSetName(): void {
 		$this->expectException(\Sabre\DAV\Exception\Forbidden::class);
 

apps/dav/tests/unit/SystemTag/SystemTagsObjectTypeCollectionTest.php

@@ -87,6 +87,15 @@ class SystemTagsObjectTypeCollectionTest extends \Test\TestCase {
 			$nodes = $userFolder->getById(intval($name));
 			return !empty($nodes);
 		};
+		$writeAccessClosure = function ($name) use ($userFolder) {
+			$nodes = $userFolder->getById((int)$name);
+			foreach ($nodes as $node) {
+				if (($node->getPermissions() & Constants::PERMISSION_UPDATE) === Constants::PERMISSION_UPDATE) {
+					return true;
+				}
+			}
+			return false;
+		};
 
 		$this->node = new \OCA\DAV\SystemTag\SystemTagsObjectTypeCollection(
 			'files',
@@ -94,18 +103,19 @@ class SystemTagsObjectTypeCollectionTest extends \Test\TestCase {
 			$this->tagMapper,
 			$userSession,
 			$groupManager,
-			$closure
+			$closure,
+			$writeAccessClosure,
 		);
 	}
 
-	
+
 	public function testForbiddenCreateFile(): void {
 		$this->expectException(\Sabre\DAV\Exception\Forbidden::class);
 
 		$this->node->createFile('555');
 	}
 
-	
+
 	public function testForbiddenCreateDirectory(): void {
 		$this->expectException(\Sabre\DAV\Exception\Forbidden::class);
 
@@ -123,7 +133,7 @@ class SystemTagsObjectTypeCollectionTest extends \Test\TestCase {
 		$this->assertEquals('555', $childNode->getName());
 	}
 
-	
+
 	public function testGetChildWithoutAccess(): void {
 		$this->expectException(\Sabre\DAV\Exception\NotFound::class);
 
@@ -134,7 +144,7 @@ class SystemTagsObjectTypeCollectionTest extends \Test\TestCase {
 		$this->node->getChild('555');
 	}
 
-	
+
 	public function testGetChildren(): void {
 		$this->expectException(\Sabre\DAV\Exception\MethodNotAllowed::class);
 
@@ -157,14 +167,14 @@ class SystemTagsObjectTypeCollectionTest extends \Test\TestCase {
 		$this->assertFalse($this->node->childExists('555'));
 	}
 
-	
+
 	public function testDelete(): void {
 		$this->expectException(\Sabre\DAV\Exception\Forbidden::class);
 
 		$this->node->delete();
 	}
 
-	
+
 	public function testSetName(): void {
 		$this->expectException(\Sabre\DAV\Exception\Forbidden::class);