From 8f7fb03d127ced3f0fbbaa61b1549529955615cf Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 28 Nov 2024 19:08:27 +0100 Subject: [PATCH 1/5] fix: improve checks for moving shares/storages into other mounts Signed-off-by: Robin Appelman --- lib/private/Files/View.php | 66 ++++++++++++++++++++++++++++-------- tests/lib/Files/ViewTest.php | 46 ++++++++++++++++++++++--- 2 files changed, 92 insertions(+), 20 deletions(-) diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 738e1442b67cc..37842ce02aae9 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -24,6 +24,7 @@ use OCP\Files\InvalidCharacterInPathException; use OCP\Files\InvalidDirectoryException; use OCP\Files\InvalidPathException; +use OCP\Files\Mount\IMountManager; use OCP\Files\Mount\IMountPoint; use OCP\Files\NotFoundException; use OCP\Files\ReservedWordException; @@ -700,6 +701,9 @@ public function rename($source, $target) { throw new ForbiddenException("Moving a folder into a child folder is forbidden", false); } + /** @var IMountManager $mountManager */ + $mountManager = \OC::$server->get(IMountManager::class); + $targetParts = explode('/', $absolutePath2); $targetUser = $targetParts[1] ?? null; $result = false; @@ -757,24 +761,25 @@ public function rename($source, $target) { try { $this->changeLock($target, ILockingProvider::LOCK_EXCLUSIVE, true); + $movedMounts = $mountManager->findIn($this->getAbsolutePath($source)); + if ($internalPath1 === '') { - if ($mount1 instanceof MoveableMount) { - $sourceParentMount = $this->getMount(dirname($source)); - if ($sourceParentMount === $mount2 && $this->targetIsNotShared($targetUser, $absolutePath2)) { - /** - * @var \OC\Files\Mount\MountPoint | \OC\Files\Mount\MoveableMount $mount1 - */ - $sourceMountPoint = $mount1->getMountPoint(); - $result = $mount1->moveMount($absolutePath2); - $manager->moveMount($sourceMountPoint, $mount1->getMountPoint()); - } else { - $result = false; - } - } else { - $result = false; - } + $sourceParentMount = $this->getMount(dirname($source)); + $movedMounts[] = $mount1; + $this->validateMountMove($movedMounts, $sourceParentMount, $mount2, !$this->targetIsNotShared($targetUser, $absolutePath2)); + + /** + * @var \OC\Files\Mount\MountPoint | \OC\Files\Mount\MoveableMount $mount1 + */ + $sourceMountPoint = $mount1->getMountPoint(); + $result = $mount1->moveMount($absolutePath2); + $manager->moveMount($sourceMountPoint, $mount1->getMountPoint()); + // moving a file/folder within the same mount point } elseif ($storage1 === $storage2) { + if (count($movedMounts) > 0) { + $this->validateMountMove($movedMounts, $mount1, $mount2, !$this->targetIsNotShared($targetUser, $absolutePath2)); + } if ($storage1) { $result = $storage1->rename($internalPath1, $internalPath2); } else { @@ -782,6 +787,9 @@ public function rename($source, $target) { } // moving a file/folder between storages (from $storage1 to $storage2) } else { + if (count($movedMounts) > 0) { + $this->validateMountMove($movedMounts, $mount1, $mount2, !$this->targetIsNotShared($targetUser, $absolutePath2)); + } $result = $storage2->moveFromStorage($storage1, $internalPath1, $internalPath2); } @@ -831,6 +839,34 @@ public function rename($source, $target) { return $result; } + private function validateMountMove(array $mounts, IMountPoint $sourceMount, IMountPoint $targetMount, bool $targetIsShared): void { + $targetType = 'storage'; + if ($targetMount instanceof SharedMount) { + $targetType = 'share'; + } + $targetPath = rtrim($targetMount->getMountPoint(), '/'); + + foreach ($mounts as $mount) { + $sourcePath = rtrim($mount->getMountPoint(), '/'); + $sourceType = 'storage'; + if ($mount instanceof SharedMount) { + $sourceType = 'share'; + } + + if (!$mount instanceof MoveableMount) { + throw new ForbiddenException("Storage {$sourcePath} cannot be moved", false); + } + + if ($targetIsShared) { + throw new ForbiddenException("Moving a $sourceType ($sourcePath) into shared folder is not allowed", false); + } + + if ($sourceMount !== $targetMount) { + throw new ForbiddenException("Moving a $sourceType ($sourcePath) into another $targetType ($targetPath) is not allowed", false); + } + } + } + /** * Copy a file/folder from the source path to target path * diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php index 2dfbf61641ac0..f3292ec92a3eb 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -20,6 +20,7 @@ use OCP\Constants; use OCP\Files\Config\IMountProvider; use OCP\Files\FileInfo; +use OCP\Files\ForbiddenException; use OCP\Files\GenericFileException; use OCP\Files\Mount\IMountManager; use OCP\Files\Storage\IStorage; @@ -1658,8 +1659,28 @@ public function testMoveMountPointIntoAnother() { $view = new View('/' . $this->user . '/files/'); - $this->assertFalse($view->rename('mount1', 'mount2'), 'Cannot overwrite another mount point'); - $this->assertFalse($view->rename('mount1', 'mount2/sub'), 'Cannot move a mount point into another'); + $this->expectException(ForbiddenException::class); + $view->rename('mount1', 'mount2'); + } + + public function testMoveMountPointIntoMount(): void { + self::loginAsUser($this->user); + + [$mount1, $mount2] = $this->createTestMovableMountPoints([ + $this->user . '/files/mount1', + $this->user . '/files/mount2', + ]); + + $mount1->expects($this->never()) + ->method('moveMount'); + + $mount2->expects($this->never()) + ->method('moveMount'); + + $view = new View('/' . $this->user . '/files/'); + + $this->expectException(ForbiddenException::class); + $view->rename('mount1', 'mount2/sub'); } /** @@ -1701,9 +1722,24 @@ public function testMoveMountPointIntoSharedFolder() { ->setNode($shareDir); $shareManager->createShare($share); - $this->assertFalse($view->rename('mount1', 'shareddir'), 'Cannot overwrite shared folder'); - $this->assertFalse($view->rename('mount1', 'shareddir/sub'), 'Cannot move mount point into shared folder'); - $this->assertFalse($view->rename('mount1', 'shareddir/sub/sub2'), 'Cannot move mount point into shared subfolder'); + try { + $view->rename('mount1', 'shareddir'); + $this->fail('Cannot overwrite shared folder'); + } catch (ForbiddenException $e) { + + } + try { + $view->rename('mount1', 'shareddir/sub'); + $this->fail('Cannot move mount point into shared folder'); + } catch (ForbiddenException $e) { + + } + try { + $view->rename('mount1', 'shareddir/sub/sub2'); + $this->fail('Cannot move mount point into shared subfolder'); + } catch (ForbiddenException $e) { + + } $this->assertTrue($view->rename('mount2', 'shareddir notshared/sub'), 'Can move mount point into a similarly named but non-shared folder'); $shareManager->deleteShare($share); From fc0ec5fbe588f84d952a67edf74fcb61276f9d42 Mon Sep 17 00:00:00 2001 From: Christopher Ng Date: Mon, 2 Dec 2024 16:02:02 -0800 Subject: [PATCH 2/5] feat(files): Display meaningful error message on move failure Signed-off-by: Christopher Ng --- apps/files/src/actions/moveOrCopyAction.ts | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/apps/files/src/actions/moveOrCopyAction.ts b/apps/files/src/actions/moveOrCopyAction.ts index fa7b5d4345ae9..9fce135f68427 100644 --- a/apps/files/src/actions/moveOrCopyAction.ts +++ b/apps/files/src/actions/moveOrCopyAction.ts @@ -4,7 +4,7 @@ */ import type { Folder, Node, View } from '@nextcloud/files' import type { IFilePickerButton } from '@nextcloud/dialogs' -import type { FileStat, ResponseDataDetailed } from 'webdav' +import type { FileStat, ResponseDataDetailed, WebDAVClientError } from 'webdav' import type { MoveCopyResult } from './moveOrCopyActionUtils' import { isAxiosError } from '@nextcloud/axios' @@ -165,7 +165,18 @@ export const handleCopyMoveNodeTo = async (node: Node, destination: Folder, meth } // getting here means either no conflict, file was renamed to keep both files // in a conflict, or the selected file was chosen to be kept during the conflict - await client.moveFile(currentPath, join(destinationPath, node.basename)) + try { + await client.moveFile(currentPath, join(destinationPath, node.basename)) + } catch (error) { + const parser = new DOMParser() + const text = await (error as WebDAVClientError).response?.text() + const message = parser.parseFromString(text ?? '', 'text/xml') + .querySelector('message')?.textContent + if (message) { + showError(message) + } + throw error + } // Delete the node as it will be fetched again // when navigating to the destination folder emit('files:node:deleted', node) From 46620bf0e37de6017fe7bc0a241107b0b8d394c3 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 4 Dec 2024 19:11:53 +0100 Subject: [PATCH 3/5] fix: translate mount move error messages Signed-off-by: Robin Appelman --- lib/private/Files/View.php | 46 +++++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 37842ce02aae9..fb5548df8cb3d 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -28,6 +28,7 @@ use OCP\Files\Mount\IMountPoint; use OCP\Files\NotFoundException; use OCP\Files\ReservedWordException; +use OCP\IL10N; use OCP\IUser; use OCP\Lock\ILockingProvider; use OCP\Lock\LockedException; @@ -59,6 +60,7 @@ class View { private bool $updaterEnabled = true; private UserManager $userManager; private LoggerInterface $logger; + private IL10N $l10n; /** * @throws \Exception If $root contains an invalid path @@ -73,6 +75,7 @@ public function __construct(string $root = '') { $this->lockingEnabled = !($this->lockingProvider instanceof \OC\Lock\NoopLockingProvider); $this->userManager = \OC::$server->getUserManager(); $this->logger = \OC::$server->get(LoggerInterface::class); + $this->l10n = \OC::$server->get(IFactory::class)->get('files'); } /** @@ -839,30 +842,51 @@ public function rename($source, $target) { return $result; } + /** + * @throws ForbiddenException + */ private function validateMountMove(array $mounts, IMountPoint $sourceMount, IMountPoint $targetMount, bool $targetIsShared): void { - $targetType = 'storage'; - if ($targetMount instanceof SharedMount) { - $targetType = 'share'; + $targetPath = $this->getRelativePath($targetMount->getMountPoint()); + if ($targetPath) { + $targetPath = trim($targetPath, '/'); + } else { + $targetPath = $targetMount->getMountPoint(); } - $targetPath = rtrim($targetMount->getMountPoint(), '/'); foreach ($mounts as $mount) { - $sourcePath = rtrim($mount->getMountPoint(), '/'); - $sourceType = 'storage'; - if ($mount instanceof SharedMount) { - $sourceType = 'share'; + $sourcePath = $this->getRelativePath($mount->getMountPoint()); + if ($sourcePath) { + $sourcePath = trim($sourcePath, '/'); + } else { + $sourcePath = $mount->getMountPoint(); } if (!$mount instanceof MoveableMount) { - throw new ForbiddenException("Storage {$sourcePath} cannot be moved", false); + throw new ForbiddenException($this->l10n->t('Storage %s cannot be moved', [$sourcePath]), false); } if ($targetIsShared) { - throw new ForbiddenException("Moving a $sourceType ($sourcePath) into shared folder is not allowed", false); + if ($sourceMount instanceof SharedMount) { + throw new ForbiddenException($this->l10n->t('Moving a share (%s) into a shared folder is not allowed', [$sourcePath]), false); + } else { + throw new ForbiddenException($this->l10n->t('Moving a storage (%s) into a shared folder is not allowed', [$sourcePath]), false); + } } if ($sourceMount !== $targetMount) { - throw new ForbiddenException("Moving a $sourceType ($sourcePath) into another $targetType ($targetPath) is not allowed", false); + if ($sourceMount instanceof SharedMount) { + if ($targetMount instanceof SharedMount) { + throw new ForbiddenException($this->l10n->t('Moving a share (%s) into another share (%s) is not allowed', [$sourcePath, $targetPath]), false); + } else { + throw new ForbiddenException($this->l10n->t('Moving a share (%s) into another storage (%s) is not allowed', [$sourcePath, $targetPath]), false); + } + } else { + if ($targetMount instanceof SharedMount) { + throw new ForbiddenException($this->l10n->t('Moving a storage (%s) into a share (%s) is not allowed', [$sourcePath, $targetPath]), false); + } else { + throw new ForbiddenException($this->l10n->t('Moving a storage (%s) into another storage (%s) is not allowed', [$sourcePath, $targetPath]), false); + } + } } } } From 6e580289a0683dc32e0233323c100b45ffe2da4d Mon Sep 17 00:00:00 2001 From: nextcloud-command Date: Fri, 3 Jan 2025 14:57:48 +0000 Subject: [PATCH 4/5] chore(assets): Recompile assets Signed-off-by: nextcloud-command From 49a0996f9c1b46cf06c40f8ae175be7c4ec14764 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 3 Jan 2025 17:41:39 +0100 Subject: [PATCH 5/5] fix: explicitly ignore nested mounts when transfering ownership Signed-off-by: Robin Appelman [skip ci] --- .../dav/tests/unit/Connector/Sabre/DirectoryTest.php | 2 +- .../files_features/transfer-ownership.feature | 2 +- lib/private/Files/View.php | 12 +++++++++--- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php index d2be66c13f52f..0636a251dec7b 100644 --- a/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/DirectoryTest.php @@ -41,7 +41,7 @@ public function isDeletable($path) { return $this->deletables[$path]; } - public function rename($path1, $path2) { + public function rename($path1, $path2, array $options = []) { return $this->canRename; } diff --git a/build/integration/files_features/transfer-ownership.feature b/build/integration/files_features/transfer-ownership.feature index 34fed8b9efd3a..4e5407cadbb38 100644 --- a/build/integration/files_features/transfer-ownership.feature +++ b/build/integration/files_features/transfer-ownership.feature @@ -514,7 +514,7 @@ Feature: transfer-ownership And user "user2" accepts last share When transferring ownership of path "test" from "user0" to "user1" Then the command failed with exit code 1 - And the command output contains the text "Could not transfer files." + And the command error output contains the text "Moving a storage (user0/files/test) into another storage (user1) is not allowed" Scenario: transferring ownership does not transfer received shares Given user "user0" exists diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index fb5548df8cb3d..9e9fc3e186a04 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -692,11 +692,14 @@ public function deleteAll($directory) { * * @param string $source source path * @param string $target target path + * @param array $options * * @return bool|mixed * @throws LockedException */ - public function rename($source, $target) { + public function rename($source, $target, array $options = []) { + $checkSubMounts = $options['checkSubMounts'] ?? true; + $absolutePath1 = Filesystem::normalizePath($this->getAbsolutePath($source)); $absolutePath2 = Filesystem::normalizePath($this->getAbsolutePath($target)); @@ -764,13 +767,16 @@ public function rename($source, $target) { try { $this->changeLock($target, ILockingProvider::LOCK_EXCLUSIVE, true); - $movedMounts = $mountManager->findIn($this->getAbsolutePath($source)); + if ($checkSubMounts) { + $movedMounts = $mountManager->findIn($this->getAbsolutePath($source)); + } else { + $movedMounts = []; + } if ($internalPath1 === '') { $sourceParentMount = $this->getMount(dirname($source)); $movedMounts[] = $mount1; $this->validateMountMove($movedMounts, $sourceParentMount, $mount2, !$this->targetIsNotShared($targetUser, $absolutePath2)); - /** * @var \OC\Files\Mount\MountPoint | \OC\Files\Mount\MoveableMount $mount1 */