Merge pull request #44299 from nextcloud/backport/44297/stable27

[stable27] Forbid tagging readonly files
nextcloud · Mar 21, 2024 · 0bd0b5c · 0bd0b5c
2 parents 968ca2b + 3b4d5ea
commit 0bd0b5c
Show file tree

Hide file tree

Showing 7 changed files with 128 additions and 214 deletions.
diff --git a/apps/dav/lib/SystemTag/SystemTagMappingNode.php b/apps/dav/lib/SystemTag/SystemTagMappingNode.php
@@ -37,62 +37,15 @@
  * 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,10 @@ public function delete() {
 			if (!$this->tagManager->canUserAssignTag($this->tag, $this->user)) {
 				throw new Forbidden('No permission to unassign tag ' . $this->tag->getId());
 			}
+			$writeAccessFunction = $this->childWriteAccessFunction;
+			if (!$writeAccessFunction($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

diff --git a/apps/dav/lib/SystemTag/SystemTagsObjectMappingCollection.php b/apps/dav/lib/SystemTag/SystemTagsObjectMappingCollection.php
@@ -40,56 +40,14 @@
  * 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,10 @@ public function createFile($name, $data = null) {
 			if (!$this->tagManager->canUserAssignTag($tag, $this->user)) {
 				throw new Forbidden('No permission to assign tag ' . $tagId);
 			}
-
+			$writeAccessFunction = $this->childWriteAccessFunction;
+			if (!$writeAccessFunction($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 +185,8 @@ private function makeNode(ISystemTag $tag) {
 			$this->objectType,
 			$this->user,
 			$this->tagManager,
-			$this->tagMapper
+			$this->tagMapper,
+			$this->childWriteAccessFunction,
 		);
 	}
 }
diff --git a/apps/dav/lib/SystemTag/SystemTagsObjectTypeCollection.php b/apps/dav/lib/SystemTag/SystemTagsObjectTypeCollection.php
@@ -38,61 +38,15 @@
  * 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 @@ public function getChild($objectName) {
 			$this->objectType,
 			$this->userSession->getUser(),
 			$this->tagManager,
-			$this->tagMapper
+			$this->tagMapper,
+			$this->childWriteAccessFunction,
 		);
 	}
 

diff --git a/apps/dav/lib/SystemTag/SystemTagsRelationsCollection.php b/apps/dav/lib/SystemTag/SystemTagsRelationsCollection.php
@@ -26,6 +26,7 @@
  */
 namespace OCA\DAV\SystemTag;
 
+use OCP\Constants;
 use OCP\IGroupManager;
 use OCP\IUserSession;
 use OCP\SystemTag\ISystemTagManager;
@@ -60,10 +61,19 @@ public function __construct(
 				$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;
+				},
 			),
 		];
 
@@ -77,7 +87,8 @@ function ($name) {
 				$tagMapper,
 				$userSession,
 				$groupManager,
-				$entityExistsFunction
+				$entityExistsFunction,
+				fn ($name) => true,
 			);
 		}
 

diff --git a/apps/dav/tests/unit/SystemTag/SystemTagMappingNodeTest.php b/apps/dav/tests/unit/SystemTag/SystemTagMappingNodeTest.php
@@ -33,21 +33,9 @@
 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 @@ protected function setUp(): void {
 			->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 @@ public function getMappingNode($tag = null) {
 			'files',
 			$this->user,
 			$this->tagManager,
-			$this->tagMapper
+			$this->tagMapper,
+			fn ($id): bool => in_array($id, $writableNodeIds),
 		);
 	}
 
@@ -84,7 +73,7 @@ public function testGetters(): void {
 	}
 
 	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 @@ public function testDeleteTag(): void {
 		$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 @@ public function testDeleteTagExpectedException(ISystemTag $tag, $expectedExcepti
 		$this->assertInstanceOf($expectedException, $thrown);
 	}
 
-	
+
 	public function testDeleteTagNotFound(): void {
 		$this->expectException(\Sabre\DAV\Exception\NotFound::class);
 
@@ -164,6 +172,6 @@ public function testDeleteTagNotFound(): void {
 			->with(123, 'files', 1)
 			->will($this->throwException(new TagNotFoundException()));
 
-		$this->getMappingNode($tag)->delete();
+		$this->getMappingNode($tag, [123])->delete();
 	}
 }