diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 7cf5a235892c8..5a9317b772cc7 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -23,6 +23,7 @@ use OCA\Files_Sharing\ResponseDefinitions; use OCA\Files_Sharing\SharedStorage; use OCA\Files_Sharing\TokenScopes as SharingTokenScopes; +use OCA\Files_Sharing\ViewOnly; use OCA\GlobalSiteSelector\Service\SlaveService; use OCP\App\IAppManager; use OCP\AppFramework\Http; @@ -2112,14 +2113,13 @@ private function checkInheritedAttributes(IShare $share): void { /** @var SharedStorage $storage */ $originalShare = $storage->getShare(); - $inheritedAttributes = $originalShare->getAttributes(); // For federated shares: users can only be MORE restrictive, never LESS restrictive // If parent has hideDownload=true, child MUST have hideDownload=true $parentHidesDownload = $originalShare->getHideDownload(); // Check if download permission is available from parent - $parentAllowsDownload = $inheritedAttributes === null || $inheritedAttributes->getAttribute('permissions', 'download') !== false; + $parentAllowsDownload = ViewOnly::canDownloadThroughMount($node); // Apply inheritance rules: // 1. If parent hides download, child must hide download diff --git a/apps/files_sharing/lib/Updater.php b/apps/files_sharing/lib/Updater.php index 7831d5892665f..122c31c42a154 100644 --- a/apps/files_sharing/lib/Updater.php +++ b/apps/files_sharing/lib/Updater.php @@ -11,6 +11,7 @@ use OC\Files\Cache\FileAccess; use OC\Files\Filesystem; use OC\Files\Mount\MountPoint; +use OC\Share20\Manager; use OCP\Constants; use OCP\Files\Folder; use OCP\Files\IRootFolder; @@ -66,6 +67,10 @@ private static function moveShareInOrOutOfShare($path): void { $shareManager = Server::get(\OCP\Share\IManager::class); + if ($shareManager instanceof Manager) { + $shareManager->restrictDownloadOfReshares($src); + } + // We intentionally include invalid shares, as they have been automatically invalidated due to the node no longer // being accessible for the user. Only in this case where we adjust the share after it was moved we want to ignore // this to be able to still adjust it. diff --git a/apps/files_sharing/lib/ViewOnly.php b/apps/files_sharing/lib/ViewOnly.php index 7c39ad5de1326..20477d53542fe 100644 --- a/apps/files_sharing/lib/ViewOnly.php +++ b/apps/files_sharing/lib/ViewOnly.php @@ -12,6 +12,7 @@ use OCP\Files\Folder; use OCP\Files\Node; use OCP\Files\NotFoundException; +use OCP\Share\IShare; /** * Handles restricting for download of files @@ -84,30 +85,34 @@ private function dirRecursiveCheck(Folder $dirInfo): bool { return true; } - /** - * @param Node $fileInfo - * @return bool - */ public function isDownloadable(Node $fileInfo): bool { - // Restrict view-only to nodes which are shared try { - $storage = $fileInfo->getStorage(); + return self::canDownloadThroughMount($fileInfo); } catch (NotFoundException) { return true; } + } - if (!$storage->instanceOfStorage(SharedStorage::class)) { + /** + * Whether the node can be downloaded through the mount it is accessed by, + * which for share mounts requires one of the received shares to allow it. + * + * @param ?callable(IShare): bool $allowsDownload whether a received share allows downloads, defaults to its download attribute + * @throws NotFoundException + */ + public static function canDownloadThroughMount(Node $node, ?callable $allowsDownload = null): bool { + $mount = $node->getMountPoint(); + if (!$mount instanceof SharedMount) { return true; } - // Extract extra permissions - /** @var SharedStorage $storage */ - $share = $storage->getShare(); - - // Check whether download-permission was denied (granted if not set) - $attributes = $share->getAttributes(); - $canDownload = $attributes?->getAttribute('permissions', 'download'); + $allowsDownload ??= static fn (IShare $share): bool => $share->getAttributes()?->getAttribute('permissions', 'download') !== false; + foreach ($mount->getGroupedShares() as $share) { + if ($allowsDownload($share)) { + return true; + } + } - return $canDownload !== false; + return false; } } diff --git a/apps/files_sharing/tests/ApplicationTest.php b/apps/files_sharing/tests/ApplicationTest.php index 1454f5ba3e9ed..a4c7ccbad7811 100644 --- a/apps/files_sharing/tests/ApplicationTest.php +++ b/apps/files_sharing/tests/ApplicationTest.php @@ -12,7 +12,7 @@ use OCA\Files_Sharing\AppInfo\Application; use OCA\Files_Sharing\Listener\BeforeDirectFileDownloadListener; use OCA\Files_Sharing\Listener\BeforeZipCreatedListener; -use OCA\Files_Sharing\SharedStorage; +use OCA\Files_Sharing\SharedMount; use OCA\Files_Sharing\ViewOnly; use OCP\Files\Events\BeforeDirectFileDownloadEvent; use OCP\Files\Events\BeforeZipCreatedEvent; @@ -20,7 +20,7 @@ use OCP\Files\Folder; use OCP\Files\IRootFolder; use OCP\Files\IUserFolder; -use OCP\Files\Storage\IStorage; +use OCP\Files\Mount\IMountPoint; use OCP\IUser; use OCP\IUserSession; use OCP\Share\IAttributes; @@ -47,13 +47,13 @@ protected function setUp(): void { public static function providesDataForCanGet(): array { return [ 'normal file (sender)' => [ - '/bar.txt', IStorage::class, false, null, true + '/bar.txt', null, true ], 'shared file (receiver) with attribute secure-view-enabled set false' => [ - '/share-bar.txt', SharedStorage::class, true, true, true + '/share-bar.txt', true, true ], 'shared file (receiver) with attribute secure-view-enabled set true' => [ - '/secure-share-bar.txt', SharedStorage::class, true, false, false + '/secure-share-bar.txt', false, false ], ]; } @@ -61,24 +61,23 @@ public static function providesDataForCanGet(): array { #[DataProvider(methodName: 'providesDataForCanGet')] public function testCheckDirectCanBeDownloaded( string $path, - string $storageClass, - bool $instanceOfSharedStorage, - ?bool $storageShareDownloadPermission, + ?bool $shareDownloadPermission, bool $canDownloadDirectly, ): void { - $fileStorage = $this->createMock($storageClass); - $fileStorage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn($instanceOfSharedStorage); - if ($storageShareDownloadPermission !== null) { + if ($shareDownloadPermission === null) { + $mount = $this->createMock(IMountPoint::class); + } else { $fileShareAttributes = $this->createMock(IAttributes::class); - $fileShareAttributes->method('getAttribute')->with('permissions', 'download')->willReturn($storageShareDownloadPermission); + $fileShareAttributes->method('getAttribute')->with('permissions', 'download')->willReturn($shareDownloadPermission); $fileShare = $this->createMock(IShare::class); $fileShare->method('getAttributes')->willReturn($fileShareAttributes); - $fileStorage->method('getShare')->willReturn($fileShare); + $mount = $this->createMock(SharedMount::class); + $mount->method('getGroupedShares')->willReturn([$fileShare]); } $file = $this->createMock(File::class); - $file->method('getStorage')->willReturn($fileStorage); + $file->method('getMountPoint')->willReturn($mount); $userFolder = $this->createMock(IUserFolder::class); $userFolder->method('get')->willReturn($file); @@ -105,16 +104,16 @@ public function testCheckDirectCanBeDownloaded( public static function providesDataForCanZip(): array { return [ 'can download zipped 2 non-shared files inside non-shared folder' => [ - '/folder', ['bar1.txt', 'bar2.txt'], 'nonSharedStorage', ['nonSharedStorage','nonSharedStorage'], true + '/folder', ['bar1.txt', 'bar2.txt'], 'nonSharedMount', ['nonSharedMount','nonSharedMount'], true ], 'can download zipped non-shared folder' => [ - '/', ['folder'], 'nonSharedStorage', ['nonSharedStorage','nonSharedStorage'], true + '/', ['folder'], 'nonSharedMount', ['nonSharedMount','nonSharedMount'], true ], 'cannot download zipped 1 non-shared file and 1 secure-shared inside non-shared folder' => [ - '/folder', ['secured-bar1.txt', 'bar2.txt'], 'nonSharedStorage', ['nonSharedStorage','secureSharedStorage'], false, + '/folder', ['secured-bar1.txt', 'bar2.txt'], 'nonSharedMount', ['nonSharedMount','secureSharedMount'], false, ], 'cannot download zipped secure-shared folder' => [ - '/', ['secured-folder'], 'secureSharedStorage', [], false, + '/', ['secured-folder'], 'secureSharedMount', [], false, ], ]; } @@ -123,42 +122,40 @@ public static function providesDataForCanZip(): array { public function testCheckZipCanBeDownloaded( string $dir, array $files, - string $folderStorage, + string $folderMount, array $directoryListing, bool $downloadSuccessful, ): void { - // Mock: Normal file/folder storage - $nonSharedStorage = $this->createMock(IStorage::class); - $nonSharedStorage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(false); + // Mock: Normal file/folder mount + $nonSharedMount = $this->createMock(IMountPoint::class); // Mock: Secure-view file/folder shared storage $secureReceiverFileShareAttributes = $this->createMock(IAttributes::class); $secureReceiverFileShareAttributes->method('getAttribute')->with('permissions', 'download')->willReturn(false); $secureReceiverFileShare = $this->createMock(IShare::class); $secureReceiverFileShare->method('getAttributes')->willReturn($secureReceiverFileShareAttributes); - $secureSharedStorage = $this->createMock(SharedStorage::class); - $secureSharedStorage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true); - $secureSharedStorage->method('getShare')->willReturn($secureReceiverFileShare); + $secureSharedMount = $this->createMock(SharedMount::class); + $secureSharedMount->method('getGroupedShares')->willReturn([$secureReceiverFileShare]); $folder = $this->createMock(Folder::class); $folder->method('getPath')->willReturn($dir); - if ($folderStorage === 'nonSharedStorage') { - $folder->method('getStorage')->willReturn($nonSharedStorage); - } elseif ($folderStorage === 'secureSharedStorage') { - $folder->method('getStorage')->willReturn($secureSharedStorage); + if ($folderMount === 'nonSharedMount') { + $folder->method('getMountPoint')->willReturn($nonSharedMount); + } elseif ($folderMount === 'secureSharedMount') { + $folder->method('getMountPoint')->willReturn($secureSharedMount); } else { - throw new \Exception('Unknown storage ' . $folderStorage); + throw new \Exception('Unknown mount ' . $folderMount); } if (count($directoryListing) > 0) { $directoryListing = array_map( - function (string $fileStorage) use ($nonSharedStorage, $secureSharedStorage) { + function (string $fileMount) use ($nonSharedMount, $secureSharedMount) { $file = $this->createMock(File::class); - if ($fileStorage === 'nonSharedStorage') { - $file->method('getStorage')->willReturn($nonSharedStorage); - } elseif ($fileStorage === 'secureSharedStorage') { - $file->method('getStorage')->willReturn($secureSharedStorage); + if ($fileMount === 'nonSharedMount') { + $file->method('getMountPoint')->willReturn($nonSharedMount); + } elseif ($fileMount === 'secureSharedMount') { + $file->method('getMountPoint')->willReturn($secureSharedMount); } else { - throw new \Exception('Unknown storage ' . $fileStorage); + throw new \Exception('Unknown mount ' . $fileMount); } return $file; }, @@ -177,13 +174,13 @@ function (string $fileStorage) use ($nonSharedStorage, $secureSharedStorage) { // If the folder contains any secure-shared files, make it appear as a secure-shared folder // so that ViewOnly::isDownloadable() will return false - $containsSecureSharedFiles = in_array('secureSharedStorage', $directoryListing); - if ($containsSecureSharedFiles && $folderStorage === 'nonSharedStorage') { - $folder->method('getStorage')->willReturn($secureSharedStorage); + $containsSecureSharedFiles = in_array('secureSharedMount', $directoryListing); + if ($containsSecureSharedFiles && $folderMount === 'nonSharedMount') { + $folder->method('getMountPoint')->willReturn($secureSharedMount); } $rootFolder = $this->createMock(Folder::class); - $rootFolder->method('getStorage')->willReturn($nonSharedStorage); + $rootFolder->method('getMountPoint')->willReturn($nonSharedMount); $rootFolder->method('getDirectoryListing')->willReturn([$folder]); $userFolder = $this->createMock(IUserFolder::class); diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index 57366ded86c8c..11bf9c50abf83 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -14,6 +14,7 @@ use OCA\Federation\TrustedServers; use OCA\Files_Sharing\Controller\ShareAPIController; use OCA\Files_Sharing\External\Storage; +use OCA\Files_Sharing\SharedMount; use OCA\Files_Sharing\SharedStorage; use OCP\App\IAppManager; use OCP\AppFramework\Http\DataResponse; @@ -4654,6 +4655,12 @@ public function testOwnerCanAlwaysDownload(): void { $this->invokePrivate($ocs, 'checkInheritedAttributes', [$share]); } + private function createSharedMount(IShare ...$shares): SharedMount&MockObject { + $mount = $this->createMock(SharedMount::class); + $mount->method('getGroupedShares')->willReturn($shares); + return $mount; + } + public function testParentHideDownloadEnforcedOnChild(): void { $ocs = $this->mockFormatShare(); @@ -4673,6 +4680,7 @@ public function testParentHideDownloadEnforcedOnChild(): void { $storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true); $storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage); $storage->method('getShare')->willReturn($originalShare); + $node->method('getMountPoint')->willReturn($this->createSharedMount($originalShare)); $originalShare->method('getHideDownload')->willReturn(true); // Parent hides download $originalShare->method('getAttributes')->willReturn(null); @@ -4704,6 +4712,7 @@ public function testUserCanHideWhenParentAllows(): void { $storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true); $storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage); $storage->method('getShare')->willReturn($originalShare); + $node->method('getMountPoint')->willReturn($this->createSharedMount($originalShare)); $originalShare->method('getHideDownload')->willReturn(false); // Parent allows download $originalShare->method('getAttributes')->willReturn(null); @@ -4739,6 +4748,7 @@ public function testParentDownloadAttributeInherited(): void { $storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true); $storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage); $storage->method('getShare')->willReturn($originalShare); + $node->method('getMountPoint')->willReturn($this->createSharedMount($originalShare)); $originalShare->method('getHideDownload')->willReturn(false); $originalShare->method('getAttributes')->willReturn($attributes); $attributes->method('getAttribute')->with('permissions', 'download')->willReturn(false); // Parent forbids download @@ -4802,6 +4812,7 @@ public function testUserAllowsDownloadWhenParentPermits(): void { $storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true); $storage->method('getInstanceOfStorage')->with(SharedStorage::class)->willReturn($storage); $storage->method('getShare')->willReturn($originalShare); + $node->method('getMountPoint')->willReturn($this->createSharedMount($originalShare)); $originalShare->method('getHideDownload')->willReturn(false); // Parent allows download $originalShare->method('getAttributes')->willReturn(null); diff --git a/apps/files_sharing/tests/ReshareAttributesTest.php b/apps/files_sharing/tests/ReshareAttributesTest.php new file mode 100644 index 0000000000000..87a98852d8ab7 --- /dev/null +++ b/apps/files_sharing/tests/ReshareAttributesTest.php @@ -0,0 +1,250 @@ +view->mkdir('folder'); + $this->view->mkdir('folder/sub'); + $this->view->file_put_contents('folder/sub/file.txt', $this->data); + } + + private function createShare(int $type, string $path, string $initiator, ?string $recipient = null): IShare { + $this->loginHelper($initiator); + if ($recipient !== null) { + return $this->share($type, $path, $initiator, $recipient, Constants::PERMISSION_ALL); + } + + $share = $this->shareManager->newShare() + ->setShareType($type) + ->setSharedBy($initiator) + ->setNode($this->rootFolder->getUserFolder($initiator)->get($path)) + ->setPermissions(Constants::PERMISSION_READ); + return $this->shareManager->createShare($share); + } + + private function setDownload(IShare $share, bool $canDownload): void { + $this->loginHelper($share->getSharedBy()); + $share = $this->shareManager->getShareById($share->getFullId()); + $attributes = $share->getAttributes() ?? $share->newAttributes(); + $attributes->setAttribute('permissions', 'download', $canDownload); + $share->setAttributes($attributes); + $this->shareManager->updateShare($share); + } + + private function canDownload(IShare $share): bool { + return $this->shareManager->getShareById($share->getFullId())->canDownload(); + } + + /** + * Moving between storages on object store primary storage creates new + * file ids, so the shares of the moved node no longer resolve. + */ + private function assertCannotServeDownloads(IShare $share): void { + $share = $this->shareManager->getShareById($share->getFullId()); + try { + $share->getNode(); + $nodeExists = true; + } catch (NotFoundException) { + $nodeExists = false; + } + $this->assertFalse($nodeExists && $share->canDownload()); + } + + public function testRestrictingDownloadRestrictsReshareChain(): void { + $share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2); + $reshare = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER3); + $link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2); + $fileLink = $this->createShare(IShare::TYPE_LINK, 'folder/sub/file.txt', self::TEST_FILES_SHARING_API_USER2); + $chainedLink = $this->createShare(IShare::TYPE_LINK, 'folder/sub', self::TEST_FILES_SHARING_API_USER3); + + $this->setDownload($share, false); + + $this->assertFalse($this->canDownload($reshare)); + $this->assertFalse($this->canDownload($link)); + $this->assertFalse($this->canDownload($fileLink)); + $this->assertFalse($this->canDownload($chainedLink)); + } + + public function testRestrictingDownloadKeepsSharesOfTheOwner(): void { + $share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2); + $ownerShare = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER3); + $ownerLink = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER1); + + $this->setDownload($share, false); + + $this->assertTrue($this->canDownload($ownerShare)); + $this->assertTrue($this->canDownload($ownerLink)); + } + + public function testRestrictingDownloadKeepsResharesWithAnotherAllowingShare(): void { + $share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2); + $groupShare = $this->createShare(IShare::TYPE_GROUP, 'folder', self::TEST_FILES_SHARING_API_USER1, 'group1'); + $link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2); + + $this->setDownload($share, false); + $this->assertTrue($this->canDownload($link)); + + $this->setDownload($groupShare, false); + $this->assertFalse($this->canDownload($link)); + } + + public function testReceivedNodeIsDownloadableIfAnyReceivedShareAllowsIt(): void { + $share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2); + $this->setDownload($share, false); + $this->createShare(IShare::TYPE_GROUP, 'folder', self::TEST_FILES_SHARING_API_USER1, 'group1'); + + $this->loginHelper(self::TEST_FILES_SHARING_API_USER2); + $node = $this->rootFolder->getUserFolder(self::TEST_FILES_SHARING_API_USER2)->get('folder/sub/file.txt'); + $this->assertTrue((new ViewOnly(null))->isDownloadable($node)); + } + + public function testRestrictingDownloadRestrictsCyclicReshares(): void { + $share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2); + $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER3); + $cyclicShare = $this->createShare(IShare::TYPE_GROUP, 'folder', self::TEST_FILES_SHARING_API_USER3, 'group1'); + $link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2); + + $this->setDownload($share, false); + + $this->assertFalse($this->canDownload($cyclicShare)); + $this->assertFalse($this->canDownload($link)); + } + + public function testRestrictingDownloadKeepsCyclicResharesReachableFromTheOwner(): void { + $share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2); + $otherShare = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER3); + $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER4); + $this->createShare(IShare::TYPE_GROUP, 'folder', self::TEST_FILES_SHARING_API_USER3, 'group1'); + $link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2); + + $this->setDownload($share, false); + $this->assertTrue($this->canDownload($link)); + + $this->setDownload($otherShare, false); + $this->assertFalse($this->canDownload($link)); + } + + public function testRestrictingDownloadOfReshareRestrictsItsReshares(): void { + $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2); + $reshare = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER3); + $link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2); + $chainedLink = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER3); + + $this->setDownload($reshare, false); + + $this->assertTrue($this->canDownload($link)); + $this->assertFalse($this->canDownload($chainedLink)); + } + + public function testAllowingDownloadAgainKeepsResharesRestricted(): void { + $share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2); + $link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2); + + $this->setDownload($share, false); + $this->setDownload($share, true); + + $this->assertFalse($this->canDownload($link)); + } + + public function testUpdatingPermissionsKeepsResharesUnchanged(): void { + $share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2); + $link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2); + + $this->loginHelper(self::TEST_FILES_SHARING_API_USER1); + $share = $this->shareManager->getShareById($share->getFullId()); + $share->setPermissions(Constants::PERMISSION_READ | Constants::PERMISSION_SHARE); + $this->shareManager->updateShare($share); + + $this->assertNull($this->shareManager->getShareById($link->getFullId())->getAttributes()); + } + + public function testRestrictingDownloadRestrictsResharesOfUnresolvableSharers(): void { + $share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2); + $link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2); + + $qb = Server::get(IDBConnection::class)->getQueryBuilder(); + $qb->update('share') + ->set('uid_initiator', $qb->createNamedParameter('non-existing-user')) + ->where($qb->expr()->eq('id', $qb->createNamedParameter($link->getId()))) + ->executeStatement(); + + $this->setDownload($share, false); + + $this->assertFalse($this->canDownload($link)); + } + + public function testMovingIntoRestrictedShareRestrictsReshares(): void { + $share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2); + $this->setDownload($share, false); + + $this->loginHelper(self::TEST_FILES_SHARING_API_USER2); + $view = new View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files'); + $view->mkdir('own'); + $view->file_put_contents('own/file.txt', $this->data); + $link = $this->createShare(IShare::TYPE_LINK, 'own', self::TEST_FILES_SHARING_API_USER2); + $reshare = $this->createShare(IShare::TYPE_USER, 'own', self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER3); + $chainedLink = $this->createShare(IShare::TYPE_LINK, 'own', self::TEST_FILES_SHARING_API_USER3); + + $this->loginHelper(self::TEST_FILES_SHARING_API_USER2); + $view->rename('own', 'folder/own'); + + $this->assertCannotServeDownloads($link); + $this->assertCannotServeDownloads($reshare); + $this->assertCannotServeDownloads($chainedLink); + } + + public function testMovingByOwnerIntoRestrictedShareRestrictsReshares(): void { + $this->view->mkdir('open'); + $this->view->mkdir('open/moved'); + $share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2); + $this->createShare(IShare::TYPE_USER, 'open', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2); + $this->setDownload($share, false); + $link = $this->createShare(IShare::TYPE_LINK, 'open/moved', self::TEST_FILES_SHARING_API_USER2); + $this->assertTrue($this->canDownload($link)); + + $this->loginHelper(self::TEST_FILES_SHARING_API_USER1); + $this->view->rename('open/moved', 'folder/moved'); + + $this->assertFalse($this->canDownload($link)); + } + + public function testMovingIntoAllowingShareKeepsReshares(): void { + $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2); + + $this->loginHelper(self::TEST_FILES_SHARING_API_USER2); + $view = new View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files'); + $view->mkdir('own'); + $link = $this->createShare(IShare::TYPE_LINK, 'own', self::TEST_FILES_SHARING_API_USER2); + + $this->loginHelper(self::TEST_FILES_SHARING_API_USER2); + $view->rename('own', 'folder/own'); + + $this->assertTrue($this->canDownload($link)); + } +} diff --git a/apps/files_sharing/tests/unit/Listener/BeforeZipCreatedListenerTest.php b/apps/files_sharing/tests/unit/Listener/BeforeZipCreatedListenerTest.php index 77ee69c42023a..496c998ca233c 100644 --- a/apps/files_sharing/tests/unit/Listener/BeforeZipCreatedListenerTest.php +++ b/apps/files_sharing/tests/unit/Listener/BeforeZipCreatedListenerTest.php @@ -10,7 +10,7 @@ namespace OCA\Files_Sharing\Tests\unit\Listener; use OCA\Files_Sharing\Listener\BeforeZipCreatedListener; -use OCA\Files_Sharing\SharedStorage; +use OCA\Files_Sharing\SharedMount; use OCP\Files\Events\BeforeZipCreatedEvent; use OCP\Files\File; use OCP\Files\Folder; @@ -270,7 +270,7 @@ public function testHandle( private function createSharedFile(bool $downloadable, string $path): File&MockObject { $file = $this->createMock(File::class); - $file->method('getStorage')->willReturn($this->createSharedStorage($downloadable)); + $file->method('getMountPoint')->willReturn($this->createSharedMount($downloadable)); $file->method('getPath')->willReturn($path); $file->method('getName')->willReturn(basename($path)); @@ -282,7 +282,7 @@ private function createSharedFile(bool $downloadable, string $path): File&MockOb */ private function createSharedFolder(bool $downloadable, string $path, array $children = []): Folder&MockObject { $folder = $this->createMock(Folder::class); - $folder->method('getStorage')->willReturn($this->createSharedStorage($downloadable)); + $folder->method('getMountPoint')->willReturn($this->createSharedMount($downloadable)); $folder->method('getDirectoryListing')->willReturn($children); $folder->method('getPath')->willReturn($path); $folder->method('getName')->willReturn(basename($path)); @@ -290,20 +290,16 @@ private function createSharedFolder(bool $downloadable, string $path, array $chi return $folder; } - private function createSharedStorage(bool $downloadable): SharedStorage&MockObject { + private function createSharedMount(bool $downloadable): SharedMount&MockObject { $attributes = $this->createMock(IAttributes::class); $attributes->method('getAttribute')->with('permissions', 'download')->willReturn($downloadable); $share = $this->createMock(IShare::class); $share->method('getAttributes')->willReturn($attributes); - $storage = $this->getMockBuilder(SharedStorage::class) - ->disableOriginalConstructor() - ->onlyMethods(['instanceOfStorage', 'getShare']) - ->getMock(); - $storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true); - $storage->method('getShare')->willReturn($share); + $mount = $this->createMock(SharedMount::class); + $mount->method('getGroupedShares')->willReturn([$share]); - return $storage; + return $mount; } } diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index adfe982313626..fba15c4cc741f 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -1893,32 +1893,29 @@ private function targetIsNotShared(string $user, string $targetPath): bool { $providers = [ IShare::TYPE_USER, IShare::TYPE_GROUP, + IShare::TYPE_LINK, IShare::TYPE_EMAIL, IShare::TYPE_CIRCLE, IShare::TYPE_ROOM, IShare::TYPE_DECK, ]; $shareManager = Server::get(IManager::class); - /** @var IShare[] $shares */ - $shares = array_merge(...array_map(function (int $type) use ($shareManager, $user) { - return $shareManager->getSharesBy($user, $type); - }, $providers)); + $rootFolder = Server::get(IRootFolder::class); + $userFolderPath = '/' . $user . '/files'; - foreach ($shares as $share) { + for ($path = $targetPath; str_starts_with($path, $userFolderPath . '/'); $path = dirname($path)) { try { - $sharedPath = $share->getNode()->getPath(); - } catch (NotFoundException $e) { - // node is not found, ignoring - $this->logger->debug( - 'Could not find the node linked to a share', - ['app' => 'files', 'exception' => $e]); + $node = $rootFolder->get($path); + } catch (NotFoundException) { continue; } - if ($targetPath === $sharedPath || str_starts_with($targetPath, $sharedPath . '/')) { - $this->logger->debug( - 'It is not allowed to move one mount point into a shared folder', - ['app' => 'files']); - return false; + foreach ($providers as $type) { + if ($shareManager->getSharesBy($user, $type, $node, false, 1) !== []) { + $this->logger->debug( + 'It is not allowed to move one mount point into a shared folder', + ['app' => 'files']); + return false; + } } } diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 60c750fe66cdb..4e4a0611dea3b 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -19,6 +19,7 @@ use OCA\Circles\Api\v1\Circles; use OCA\Files_Sharing\AppInfo\Application; use OCA\Files_Sharing\SharedStorage; +use OCA\Files_Sharing\ViewOnly; use OCA\ShareByMail\ShareByMailProvider; use OCP\Constants; use OCP\DB\QueryBuilder\IQueryBuilder; @@ -769,6 +770,12 @@ public function updateShare(IShare $share, bool $onlyValid = true): IShare { $share = $provider->update($share); } + if (in_array($share->getShareType(), [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_CIRCLE, IShare::TYPE_ROOM, IShare::TYPE_DECK], true) + && $originalShare->getAttributes()?->getAttribute('permissions', 'download') !== false + && $share->getAttributes()?->getAttribute('permissions', 'download') === false) { + $this->restrictDownloadOfReshares($share->getNode(), [$share->getFullId() => true]); + } + if ($expirationDateUpdated === true) { \OC_Hook::emit(Share::class, 'post_set_expiration_date', [ 'itemType' => $share->getNode() instanceof File ? 'file' : 'folder', @@ -852,6 +859,113 @@ private function formatShareAttributes(IShare $share): string { )); } + /** + * Restrict downloads of the reshares of the given node and its descendants + * whose sharer cannot download them through a chain of received shares + * that allow downloads, starting at the owner. + * + * @param array $restrictedShareIds full ids of received shares to treat as restricted + */ + public function restrictDownloadOfReshares(Node $node, array $restrictedShareIds = []): void { + $reshares = $this->getResharesOf($node); + + // Grow the allowed reshares from received shares outside of them, so that + // reshares which only allow each other (cycles) end up restricted. + $allowed = []; + $allowsDownload = function (IShare $receivedShare) use ($restrictedShareIds, $reshares, &$allowed): bool { + $id = $receivedShare->getFullId(); + return !isset($restrictedShareIds[$id]) + && $receivedShare->getAttributes()?->getAttribute('permissions', 'download') !== false + && (!isset($reshares[$id]) || isset($allowed[$id])); + }; + do { + $changed = false; + foreach ($reshares as $id => $reshare) { + if (isset($allowed[$id]) || $reshare->getAttributes()?->getAttribute('permissions', 'download') === false) { + continue; + } + if ($this->canDownloadThroughReceivedShares($reshare->getSharedBy(), $reshare->getNodeId(), $allowsDownload)) { + $allowed[$id] = true; + $changed = true; + } + } + } while ($changed); + + foreach ($reshares as $id => $reshare) { + if (isset($allowed[$id]) || $reshare->getAttributes()?->getAttribute('permissions', 'download') === false) { + continue; + } + $attributes = $reshare->getAttributes() ?? $reshare->newAttributes(); + $attributes->setAttribute('permissions', 'download', false); + $reshare->setAttributes($attributes); + $this->factory->getProviderForType($reshare->getShareType())->update($reshare); + } + } + + /** + * Get the shares of the node and its descendants that were not created by the node owner. + * + * @return array shares indexed by their full id + */ + private function getResharesOf(Node $node): array { + $internalPath = $node->getInternalPath(); + + $qb = $this->connection->getQueryBuilder(); + $qb->select('s.id', 's.share_type') + ->from('share', 's') + ->innerJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid')) + ->where($qb->expr()->eq('f.storage', $qb->createNamedParameter($node->getStorage()->getCache()->getNumericStorageId(), IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->in('s.item_type', $qb->createNamedParameter(['file', 'folder'], IQueryBuilder::PARAM_STR_ARRAY))) + ->andWhere($qb->expr()->neq('s.share_type', $qb->createNamedParameter(IShare::TYPE_USERGROUP, IQueryBuilder::PARAM_INT))); + + $owner = $node->getOwner()?->getUID(); + if ($owner !== null) { + $qb->andWhere($qb->expr()->neq('s.uid_initiator', $qb->createNamedParameter($owner))); + } + if ($internalPath !== '') { + $qb->andWhere($qb->expr()->orX( + $qb->expr()->eq('f.fileid', $qb->createNamedParameter($node->getId(), IQueryBuilder::PARAM_INT)), + $qb->expr()->like('f.path', $qb->createNamedParameter($this->connection->escapeLikeParameter($internalPath) . '/%')), + )); + } + + $reshares = []; + $result = $qb->executeQuery(); + while ($row = $result->fetchAssociative()) { + try { + $reshare = $this->factory->getProviderForType((int)$row['share_type'])->getShareById((string)$row['id']); + $reshares[$reshare->getFullId()] = $reshare; + } catch (ProviderException|ShareNotFound) { + } + } + $result->closeCursor(); + + return $reshares; + } + + /** + * Whether the user can download the node through their own storage or + * through one of the shares they received it from. + * + * @param callable(IShare): bool $allowsDownload whether a received share allows downloads + */ + private function canDownloadThroughReceivedShares(string $userId, int $nodeId, callable $allowsDownload): bool { + try { + $nodes = $this->rootFolder->getUserFolder($userId)->getById($nodeId); + } catch (\Exception $e) { + $this->logger->warning('Could not resolve the received shares of ' . $userId . ', restricting downloads of their reshares', ['exception' => $e]); + return false; + } + + foreach ($nodes as $node) { + if (ViewOnly::canDownloadThroughMount($node, $allowsDownload)) { + return true; + } + } + + return false; + } + #[Override] public function acceptShare(IShare $share, string $recipientId): IShare { [$providerId,] = $this->splitFullId($share->getFullId()); diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php index 22a73b46b56ad..dc0e025dc1b90 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -32,6 +32,7 @@ use OCP\Files\InvalidPathException; use OCP\Files\IRootFolder; use OCP\Files\Mount\IMountManager; +use OCP\Files\Node; use OCP\Files\NotFoundException; use OCP\Files\Storage\IStorage; use OCP\Files\Storage\IStorageFactory; @@ -1782,6 +1783,46 @@ public function testMoveMountPointIntoSharedFolder(): void { $userObject->delete(); } + public function testMoveMountPointIntoLinkSharedFolder(): void { + self::loginAsUser(self::$user); + + [$mount1] = $this->createTestMovableMountPoints([ + self::$user . '/files/mount1', + ]); + $mount1->expects($this->never()) + ->method('moveMount'); + + $view = new View('/' . self::$user . '/files/'); + $userFolder = Server::get(IRootFolder::class)->getUserFolder(self::$user); + $shareManager = Server::get(IShareManager::class); + $shares = []; + // More links than a single page of shares, created before the shared target + for ($i = 0; $i <= 50; $i++) { + $view->mkdir('other' . $i); + $shares[] = $this->createLinkShare($shareManager, $userFolder->get('other' . $i)); + } + $view->mkdir('linkdir'); + $shares[] = $this->createLinkShare($shareManager, $userFolder->get('linkdir')); + + try { + $this->expectException(ForbiddenException::class); + $view->rename('mount1', 'linkdir/mount1'); + } finally { + foreach ($shares as $share) { + $shareManager->deleteShare($share); + } + } + } + + private function createLinkShare(IShareManager $shareManager, Node $node): IShare { + $share = $shareManager->newShare(); + $share->setSharedBy(self::$user) + ->setShareType(IShare::TYPE_LINK) + ->setPermissions(Constants::PERMISSION_READ) + ->setNode($node); + return $shareManager->createShare($share); + } + public static function basicOperationProviderForLocks(): array { return [ // --- write hook ----