diff --git a/apps/dav/lib/SystemTag/SystemTagMappingNode.php b/apps/dav/lib/SystemTag/SystemTagMappingNode.php index 9762b6e1db931..113c8d8283693 100644 --- 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 index 4d73c17d7dd19..b45ef6c3f71bc 100644 --- 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 index 3fa40278cdbfb..f1d5fc06c99e8 100644 --- 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 index 6911d40e208a3..989c4640a61d9 100644 --- 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\EventDispatcher\IEventDispatcher; use OCP\Files\IRootFolder; use OCP\IGroupManager; @@ -60,7 +61,21 @@ function (string $name) use ($rootFolder, $userSession): bool { } else { return false; } - } + }, + function (string $name) use ($rootFolder, $userSession): bool { + $user = $userSession->getUser(); + if ($user) { + $nodes = $rootFolder->getUserFolder($user->getUID())->getById((int)$name); + foreach ($nodes as $node) { + if (($node->getPermissions() & Constants::PERMISSION_UPDATE) === Constants::PERMISSION_UPDATE) { + return true; + } + } + return false; + } else { + return false; + } + }, ), ]; @@ -75,7 +90,8 @@ function (string $name) use ($rootFolder, $userSession): bool { $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 index 6599c574dd9e8..97020cf7fa165 100644 --- 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(); } } diff --git a/apps/dav/tests/unit/SystemTag/SystemTagsObjectMappingCollectionTest.php b/apps/dav/tests/unit/SystemTag/SystemTagsObjectMappingCollectionTest.php index 22f2e69b74091..d0703229ea777 100644 --- a/apps/dav/tests/unit/SystemTag/SystemTagsObjectMappingCollectionTest.php +++ b/apps/dav/tests/unit/SystemTag/SystemTagsObjectMappingCollectionTest.php @@ -32,21 +32,9 @@ 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 @@ protected function setUp(): void { ->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 @@ public function testAssignTag(): void { ->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 @@ public function testAssignTagNoPermission($userVisible, $userAssignable, $expect $this->assertInstanceOf($expectedException, $thrown); } - + public function testAssignTagNotFound(): void { $this->expectException(\Sabre\DAV\Exception\PreconditionFailed::class); @@ -144,7 +155,7 @@ public function testAssignTagNotFound(): void { $this->getNode()->createFile('555'); } - + public function testForbiddenCreateDirectory(): void { $this->expectException(\Sabre\DAV\Exception\Forbidden::class); @@ -174,7 +185,7 @@ public function testGetChild(): void { $this->assertEquals('555', $childNode->getName()); } - + public function testGetChildNonVisible(): void { $this->expectException(\Sabre\DAV\Exception\NotFound::class); @@ -197,7 +208,7 @@ public function testGetChildNonVisible(): void { $this->getNode()->getChild('555'); } - + public function testGetChildRelationNotFound(): void { $this->expectException(\Sabre\DAV\Exception\NotFound::class); @@ -209,7 +220,7 @@ public function testGetChildRelationNotFound(): void { $this->getNode()->getChild('777'); } - + public function testGetChildInvalidId(): void { $this->expectException(\Sabre\DAV\Exception\BadRequest::class); @@ -221,7 +232,7 @@ public function testGetChildInvalidId(): void { $this->getNode()->getChild('badid'); } - + public function testGetChildTagDoesNotExist(): void { $this->expectException(\Sabre\DAV\Exception\NotFound::class); @@ -325,7 +336,7 @@ public function testChildExistsTagNotFound(): void { $this->assertFalse($this->getNode()->childExists('555')); } - + public function testChildExistsInvalidId(): void { $this->expectException(\Sabre\DAV\Exception\BadRequest::class); @@ -337,14 +348,14 @@ public function testChildExistsInvalidId(): void { $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); diff --git a/apps/dav/tests/unit/SystemTag/SystemTagsObjectTypeCollectionTest.php b/apps/dav/tests/unit/SystemTag/SystemTagsObjectTypeCollectionTest.php index 143d598fd2dcf..780b16858f106 100644 --- a/apps/dav/tests/unit/SystemTag/SystemTagsObjectTypeCollectionTest.php +++ b/apps/dav/tests/unit/SystemTag/SystemTagsObjectTypeCollectionTest.php @@ -87,6 +87,15 @@ protected function setUp(): void { $node = $userFolder->getFirstNodeById(intval($name)); return $node !== null; }; + $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,7 +103,8 @@ protected function setUp(): void { $this->tagMapper, $userSession, $groupManager, - $closure + $closure, + $writeAccessClosure, ); }