diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 35166c37ee3..a84481730c4 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -217,6 +217,17 @@ class Manager implements IManager { throw new \InvalidArgumentException($this->l->t('Valid permissions are required for sharing')); } + // Permissions must be valid + if ($share->getPermissions() < 0 || $share->getPermissions() > \OCP\Constants::PERMISSION_ALL) { + throw new \InvalidArgumentException($this->l->t('Valid permissions are required for sharing')); + } + + // Single file shares should never have delete or create permissions + if (($share->getNode() instanceof File) + && (($share->getPermissions() & (\OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_DELETE)) !== 0)) { + throw new \InvalidArgumentException($this->l->t('File shares cannot have create or delete permissions')); + } + $permissions = 0; $nodesForUser = $userFolder->getById($share->getNodeId()); foreach ($nodesForUser as $node) { diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 6df97423989..8b8d6ea4e65 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -893,10 +893,9 @@ class ManagerTest extends \Test\TestCase { $mount = $this->createMock(MoveableMount::class); $limitedPermssions->method('getMountPoint')->willReturn($mount); - - $data[] = [$this->createShare(null, IShare::TYPE_USER, $limitedPermssions, $user2, $user0, $user0, 31, null, null), 'Cannot increase permissions of path', true]; + // increase permissions of a re-share $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]; + $data[] = [$this->createShare(null, IShare::TYPE_USER, $limitedPermssions, $user2, $user0, $user0, 3, null, null), 'Cannot increase permissions of path', true]; $nonMovableStorage = $this->createMock(IStorage::class); $nonMovableStorage->method('instanceOfStorage') @@ -927,6 +926,20 @@ class ManagerTest extends \Test\TestCase { $data[] = [$this->createShare(null, IShare::TYPE_GROUP, $rootFolder, $group0, $user0, $user0, 2, null, null), 'You cannot share your root folder', true]; $data[] = [$this->createShare(null, IShare::TYPE_LINK, $rootFolder, null, $user0, $user0, 16, null, null), 'You cannot share your root folder', true]; + $allPermssionsFiles = $this->createMock(File::class); + $allPermssionsFiles->method('isShareable')->willReturn(true); + $allPermssionsFiles->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_ALL); + $allPermssionsFiles->method('getId')->willReturn(187); + $allPermssionsFiles->method('getOwner') + ->willReturn($owner); + $allPermssionsFiles->method('getStorage') + ->willReturn($storage); + + // test invalid CREATE or DELETE permissions + $data[] = [$this->createShare(null, IShare::TYPE_USER, $allPermssionsFiles, $user2, $user0, $user0, \OCP\Constants::PERMISSION_ALL, null, null), 'File shares cannot have create or delete permissions', true]; + $data[] = [$this->createShare(null, IShare::TYPE_GROUP, $allPermssionsFiles, $group0, $user0, $user0, \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE, null, null), 'File shares cannot have create or delete permissions', true]; + $data[] = [$this->createShare(null, IShare::TYPE_LINK, $allPermssionsFiles, null, $user0, $user0, \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_DELETE, null, null), 'File shares cannot have create or delete permissions', true]; + $allPermssions = $this->createMock(Folder::class); $allPermssions->method('isShareable')->willReturn(true); $allPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_ALL); @@ -939,6 +952,12 @@ class ManagerTest extends \Test\TestCase { $data[] = [$this->createShare(null, IShare::TYPE_USER, $allPermssions, $user2, $user0, $user0, 30, null, null), 'Shares need at least read permissions', true]; $data[] = [$this->createShare(null, IShare::TYPE_GROUP, $allPermssions, $group0, $user0, $user0, 2, null, null), 'Shares need at least read permissions', true]; + // test invalid permissions + $data[] = [$this->createShare(null, IShare::TYPE_USER, $allPermssions, $user2, $user0, $user0, 32, null, null), 'Valid permissions are required for sharing', true]; + $data[] = [$this->createShare(null, IShare::TYPE_GROUP, $allPermssions, $group0, $user0, $user0, 63, null, null), 'Valid permissions are required for sharing', true]; + $data[] = [$this->createShare(null, IShare::TYPE_LINK, $allPermssions, null, $user0, $user0, -1, null, null), 'Valid permissions are required for sharing', true]; + + // working shares $data[] = [$this->createShare(null, IShare::TYPE_USER, $allPermssions, $user2, $user0, $user0, 31, null, null), null, false]; $data[] = [$this->createShare(null, IShare::TYPE_GROUP, $allPermssions, $group0, $user0, $user0, 3, null, null), null, false]; $data[] = [$this->createShare(null, IShare::TYPE_LINK, $allPermssions, null, $user0, $user0, 17, null, null), null, false]; @@ -2406,8 +2425,9 @@ class ManagerTest extends \Test\TestCase { } public function testCreateShareUser(): void { + /** @var Manager&MockObject $manager */ $manager = $this->createManagerMock() - ->setMethods(['canShare', 'generalCreateChecks', 'userCreateChecks', 'pathCreateChecks']) + ->onlyMethods(['canShare', 'generalCreateChecks', 'userCreateChecks', 'pathCreateChecks']) ->getMock(); $shareOwner = $this->createMock(IUser::class);