From dd5d75d86f142bd22b5c7ac724dca2127a2094ab Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 2 May 2024 16:54:34 +0200 Subject: [PATCH 1/2] fix: cleanup logic for getting the max reshare permissions Signed-off-by: Robin Appelman --- lib/private/Share20/Manager.php | 58 +++++---------------------------- 1 file changed, 8 insertions(+), 50 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index f9280d75887a2..76d696eb97990 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -45,7 +45,6 @@ use OC\KnownUser\KnownUserService; use OC\Share20\Exception\ProviderException; use OCA\Files_Sharing\AppInfo\Application; -use OCA\Files_Sharing\ISharedStorage; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\File; use OCP\Files\Folder; @@ -295,56 +294,15 @@ protected function generalCreateChecks(IShare $share, bool $isUpdate = false) { throw new \InvalidArgumentException('A share requires permissions'); } - $isFederatedShare = $share->getNode()->getStorage()->instanceOfStorage('\OCA\Files_Sharing\External\Storage'); $permissions = 0; - - $isReshare = $share->getNode()->getOwner() && $share->getNode()->getOwner()->getUID() !== $share->getSharedBy(); - if (!$isReshare && $isUpdate) { - // in case of update on owner-less filesystem, we use share owner to improve reshare detection - $isReshare = $share->getShareOwner() !== $share->getSharedBy(); - } - - if (!$isFederatedShare && $isReshare) { - $userMounts = array_filter($userFolder->getById($share->getNode()->getId()), function ($mount) { - // We need to filter since there might be other mountpoints that contain the file - // e.g. if the user has access to the same external storage that the file is originating from - return $mount->getStorage()->instanceOfStorage(ISharedStorage::class); - }); - $userMount = array_shift($userMounts); - if ($userMount === null) { - throw new GenericShareException('Could not get proper share mount for ' . $share->getNode()->getId() . '. Failing since else the next calls are called with null'); - } - $mount = $userMount->getMountPoint(); - // When it's a reshare use the parent share permissions as maximum - $userMountPointId = $mount->getStorageRootId(); - $userMountPoints = $userFolder->getById($userMountPointId); - $userMountPoint = array_shift($userMountPoints); - - if ($userMountPoint === null) { - throw new GenericShareException('Could not get proper user mount for ' . $userMountPointId . '. Failing since else the next calls are called with null'); - } - - /* Check if this is an incoming share */ - $incomingShares = $this->getSharedWith($share->getSharedBy(), IShare::TYPE_USER, $userMountPoint, -1, 0); - $incomingShares = array_merge($incomingShares, $this->getSharedWith($share->getSharedBy(), IShare::TYPE_GROUP, $userMountPoint, -1, 0)); - $incomingShares = array_merge($incomingShares, $this->getSharedWith($share->getSharedBy(), IShare::TYPE_CIRCLE, $userMountPoint, -1, 0)); - $incomingShares = array_merge($incomingShares, $this->getSharedWith($share->getSharedBy(), IShare::TYPE_ROOM, $userMountPoint, -1, 0)); - - /** @var IShare[] $incomingShares */ - if (!empty($incomingShares)) { - foreach ($incomingShares as $incomingShare) { - $permissions |= $incomingShare->getPermissions(); - } - } - } else { - /* - * Quick fix for #23536 - * Non moveable mount points do not have update and delete permissions - * while we 'most likely' do have that on the storage. - */ - $permissions = $share->getNode()->getPermissions(); - if (!($share->getNode()->getMountPoint() instanceof MoveableMount)) { - $permissions |= \OCP\Constants::PERMISSION_DELETE | \OCP\Constants::PERMISSION_UPDATE; + $nodesForUser = $userFolder->getById($share->getNodeId()); + foreach ($nodesForUser as $node) { + if ($node->getInternalPath() === '' && !$node->getMountPoint() instanceof MoveableMount) { + // for the root of non-movable mount, the permissions we see if limited by the mount itself, + // so we instead use the "raw" permissions from the storage + $permissions |= $node->getStorage()->getPermissions(''); + } else { + $permissions |= $node->getPermissions(); } } From d1d6e79375c971201bea8e9881329f52cc237323 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 3 May 2024 15:21:53 +0200 Subject: [PATCH 2/2] test: adjust test to new permissions check logic Signed-off-by: Robin Appelman --- tests/lib/Share20/ManagerTest.php | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index c2ca5fe981124..aeea1b5b863e4 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -612,7 +612,7 @@ public function testVerifyPasswordHookFails() { self::invokePrivate($this->manager, 'verifyPassword', ['password']); } - public function createShare($id, $type, $path, $sharedWith, $sharedBy, $shareOwner, + public function createShare($id, $type, $node, $sharedWith, $sharedBy, $shareOwner, $permissions, $expireDate = null, $password = null, $attributes = null) { $share = $this->createMock(IShare::class); @@ -620,7 +620,10 @@ public function createShare($id, $type, $path, $sharedWith, $sharedBy, $shareOwn $share->method('getSharedWith')->willReturn($sharedWith); $share->method('getSharedBy')->willReturn($sharedBy); $share->method('getShareOwner')->willReturn($shareOwner); - $share->method('getNode')->willReturn($path); + $share->method('getNode')->willReturn($node); + if ($node && $node->getId()) { + $share->method('getNodeId')->willReturn($node->getId()); + } $share->method('getPermissions')->willReturn($permissions); $share->method('getAttributes')->willReturn($attributes); $share->method('getExpirationDate')->willReturn($expireDate); @@ -645,8 +648,10 @@ public function dataGeneralChecks() { ->willReturn(false); $file->method('getStorage') ->willReturn($storage); + $file->method('getId')->willReturn(108); $node->method('getStorage') ->willReturn($storage); + $node->method('getId')->willReturn(108); $data = [ [$this->createShare(null, IShare::TYPE_USER, $file, null, $user0, $user0, 31, null, null), 'SharedWith is not a valid user', true], @@ -676,6 +681,7 @@ public function dataGeneralChecks() { ]; $nonShareAble = $this->createMock(Folder::class); + $nonShareAble->method('getId')->willReturn(108); $nonShareAble->method('isShareable')->willReturn(false); $nonShareAble->method('getPath')->willReturn('path'); $nonShareAble->method('getName')->willReturn('name'); @@ -711,16 +717,22 @@ public function dataGeneralChecks() { $data[] = [$this->createShare(null, IShare::TYPE_GROUP, $limitedPermssions, $group0, $user0, $user0, 17, null, null), 'Cannot increase permissions of path', true]; $data[] = [$this->createShare(null, IShare::TYPE_LINK, $limitedPermssions, null, $user0, $user0, 3, null, null), 'Cannot increase permissions of path', true]; + $nonMovableStorage = $this->createMock(Storage\IStorage::class); + $nonMovableStorage->method('instanceOfStorage') + ->with('\OCA\Files_Sharing\External\Storage') + ->willReturn(false); + $nonMovableStorage->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_ALL); $nonMoveableMountPermssions = $this->createMock(Folder::class); $nonMoveableMountPermssions->method('isShareable')->willReturn(true); $nonMoveableMountPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_READ); $nonMoveableMountPermssions->method('getId')->willReturn(108); $nonMoveableMountPermssions->method('getPath')->willReturn('path'); $nonMoveableMountPermssions->method('getName')->willReturn('name'); + $nonMoveableMountPermssions->method('getInternalPath')->willReturn(''); $nonMoveableMountPermssions->method('getOwner') ->willReturn($owner); $nonMoveableMountPermssions->method('getStorage') - ->willReturn($storage); + ->willReturn($nonMovableStorage); $data[] = [$this->createShare(null, IShare::TYPE_USER, $nonMoveableMountPermssions, $user2, $user0, $user0, 11, null, null), 'Cannot increase permissions of path', false]; $data[] = [$this->createShare(null, IShare::TYPE_GROUP, $nonMoveableMountPermssions, $group0, $user0, $user0, 11, null, null), 'Cannot increase permissions of path', false]; @@ -794,8 +806,7 @@ public function testGeneralChecks($share, $exceptionMessage, $exception) { ->method('getId') ->willReturn(42); // Id 108 is used in the data to refer to the node of the share. - $userFolder->expects($this->any()) - ->method('getById') + $userFolder->method('getById') ->with(108) ->willReturn([$share->getNode()]); $userFolder->expects($this->any())