From 22b81ac1e446cde2edbade75d03d99fe94f82638 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 29 Jul 2019 15:00:07 +0200 Subject: [PATCH] Fix permission check on incoming federated shares MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since federated shares have their permissions set on the node, we do not need to check for parent share permissions. Otherwise reshares of incoming federated have no permission variable defined and creating them will fail Signed-off-by: Julius Härtl --- lib/private/Share20/Manager.php | 5 +++-- tests/lib/Share20/ManagerTest.php | 33 +++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index bd17406977..2a2c64cf38 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -290,8 +290,10 @@ class Manager implements IManager { throw new \InvalidArgumentException('A share requires permissions'); } + $isFederatedShare = $share->getNode()->getStorage()->instanceOfStorage('\OCA\Files_Sharing\External\Storage'); + $permissions = 0; $mount = $share->getNode()->getMountPoint(); - if ($share->getNode()->getOwner()->getUID() !== $share->getSharedBy()) { + if (!$isFederatedShare && $share->getNode()->getOwner()->getUID() !== $share->getSharedBy()) { // When it's a reshare use the parent share permissions as maximum $userMountPointId = $mount->getStorageRootId(); $userMountPoints = $userFolder->getById($userMountPointId); @@ -304,7 +306,6 @@ class Manager implements IManager { /** @var \OCP\Share\IShare[] $incomingShares */ if (!empty($incomingShares)) { - $permissions = 0; foreach ($incomingShares as $incomingShare) { $permissions |= $incomingShare->getPermissions(); } diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 10db23b2d9..6f46d69d8d 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -551,6 +551,14 @@ class ManagerTest extends \Test\TestCase { $file = $this->createMock(File::class); $node = $this->createMock(Node::class); + $storage = $this->createMock(Storage\IStorage::class); + $storage->method('instanceOfStorage') + ->with('\OCA\Files_Sharing\External\Storage') + ->willReturn(false); + $file->method('getStorage') + ->willReturn($storage); + $node->method('getStorage') + ->willReturn($storage); $data = [ [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $file, null, $user0, $user0, 31, null, null), 'SharedWith is not a valid user', true], @@ -584,6 +592,8 @@ class ManagerTest extends \Test\TestCase { $nonShareAble->method('getPath')->willReturn('path'); $nonShareAble->method('getOwner') ->willReturn($owner); + $nonShareAble->method('getStorage') + ->willReturn($storage); $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $nonShareAble, $user2, $user0, $user0, 31, null, null), 'You are not allowed to share path', true]; $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $nonShareAble, $group0, $user0, $user0, 31, null, null), 'You are not allowed to share path', true]; @@ -595,6 +605,8 @@ class ManagerTest extends \Test\TestCase { $limitedPermssions->method('getPath')->willReturn('path'); $limitedPermssions->method('getOwner') ->willReturn($owner); + $limitedPermssions->method('getStorage') + ->willReturn($storage); $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $limitedPermssions, $user2, $user0, $user0, null, null, null), 'A share requires permissions', true]; $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $limitedPermssions, $group0, $user0, $user0, null, null, null), 'A share requires permissions', true]; @@ -603,6 +615,7 @@ class ManagerTest extends \Test\TestCase { $mount = $this->createMock(MoveableMount::class); $limitedPermssions->method('getMountPoint')->willReturn($mount); + $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $limitedPermssions, $user2, $user0, $user0, 31, null, null), 'Can’t increase permissions of path', true]; $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $limitedPermssions, $group0, $user0, $user0, 17, null, null), 'Can’t increase permissions of path', true]; $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_LINK, $limitedPermssions, null, $user0, $user0, 3, null, null), 'Can’t increase permissions of path', true]; @@ -613,6 +626,8 @@ class ManagerTest extends \Test\TestCase { $nonMoveableMountPermssions->method('getPath')->willReturn('path'); $nonMoveableMountPermssions->method('getOwner') ->willReturn($owner); + $nonMoveableMountPermssions->method('getStorage') + ->willReturn($storage); $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $nonMoveableMountPermssions, $user2, $user0, $user0, 11, null, null), 'Can’t increase permissions of path', false]; $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $nonMoveableMountPermssions, $group0, $user0, $user0, 11, null, null), 'Can’t increase permissions of path', false]; @@ -631,6 +646,8 @@ class ManagerTest extends \Test\TestCase { $allPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_ALL); $allPermssions->method('getOwner') ->willReturn($owner); + $allPermssions->method('getStorage') + ->willReturn($storage); $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $allPermssions, $user2, $user0, $user0, 30, null, null), 'Shares need at least read permissions', true]; $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $allPermssions, $group0, $user0, $user0, 2, null, null), 'Shares need at least read permissions', true]; @@ -639,6 +656,22 @@ class ManagerTest extends \Test\TestCase { $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $allPermssions, $group0, $user0, $user0, 3, null, null), null, false]; $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_LINK, $allPermssions, null, $user0, $user0, 17, null, null), null, false]; + + $remoteStorage = $this->createMock(Storage\IStorage::class); + $remoteStorage->method('instanceOfStorage') + ->with('\OCA\Files_Sharing\External\Storage') + ->willReturn(true); + $remoteFile = $this->createMock(Folder::class); + $remoteFile->method('isShareable')->willReturn(true); + $remoteFile->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_READ ^ \OCP\Constants::PERMISSION_UPDATE); + $remoteFile->method('getOwner') + ->willReturn($owner); + $remoteFile->method('getStorage') + ->willReturn($storage); + $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_REMOTE, $remoteFile, $user2, $user0, $user0, 1, null, null), null, false]; + $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_REMOTE, $remoteFile, $user2, $user0, $user0, 3, null, null), null, false]; + $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_REMOTE, $remoteFile, $user2, $user0, $user0, 31, null, null), 'Can’t increase permissions of ', true]; + return $data; }