From 5fb6e9e2a740a94a3a284a5862e4b93adc340500 Mon Sep 17 00:00:00 2001 From: Tobia De Koninck Date: Wed, 17 Jul 2019 14:23:17 +0200 Subject: [PATCH 1/5] Fix transfer-ownership for Group Folders Because Group folders don't have owners, use the same system to check the permissions of the node. Signed-off-by: Tobia De Koninck --- lib/private/Share20/Manager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 4c94cf26a4d29..36c491d5f4ba9 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -301,7 +301,7 @@ protected function generalCreateChecks(\OCP\Share\IShare $share) { $isFederatedShare = $share->getNode()->getStorage()->instanceOfStorage('\OCA\Files_Sharing\External\Storage'); $permissions = 0; $mount = $share->getNode()->getMountPoint(); - if (!$isFederatedShare && $share->getNode()->getOwner()->getUID() !== $share->getSharedBy()) { + if ($mount->getMountType() !== 'group' && !$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); From 061a919623e8c3f08cfc2686b41ca7bf3fa05e38 Mon Sep 17 00:00:00 2001 From: Tobia De Koninck Date: Thu, 18 Jul 2019 10:35:57 +0200 Subject: [PATCH 2/5] Fix tests Signed-off-by: Tobia De Koninck --- tests/lib/Share20/ManagerTest.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 02f779abf51ed..02851d192119d 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -21,7 +21,6 @@ namespace Test\Share20; -use OC\Files\Mount\MoveableMount; use OC\HintException; use OC\Share20\DefaultShareProvider; use OC\Share20\Exception; @@ -53,11 +52,13 @@ use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\IProviderFactory; use OCP\Share\IShare; +use OC\Files\Mount\MountPoint; use OCP\Share\IShareProvider; use PHPUnit\Framework\MockObject\MockBuilder; use PHPUnit\Framework\MockObject\MockObject; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\EventDispatcher\GenericEvent; +use Test\TestMoveableMountPoint; /** * Class ManagerTest @@ -624,8 +625,8 @@ public function dataGeneralChecks() { $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $limitedPermssions, $group0, $user0, $user0, null, null, null), 'A share requires permissions', true]; $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_LINK, $limitedPermssions, null, $user0, $user0, null, null, null), 'A share requires permissions', true]; - $mount = $this->createMock(MoveableMount::class); - $limitedPermssions->method('getMountPoint')->willReturn($mount); + $movableMount = $this->createMock(TestMoveableMountPoint::class); + $limitedPermssions->method('getMountPoint')->willReturn($movableMount); $data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $limitedPermssions, $user2, $user0, $user0, 31, null, null), 'Can’t increase permissions of path', true]; @@ -640,6 +641,8 @@ public function dataGeneralChecks() { ->willReturn($owner); $nonMoveableMountPermssions->method('getStorage') ->willReturn($storage); + $nonMoveableMountPoint = $this->createMock(MountPoint::class); + $nonMoveableMountPermssions->method('getMountPoint')->willReturn($nonMoveableMountPoint); $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]; @@ -660,6 +663,7 @@ public function dataGeneralChecks() { ->willReturn($owner); $allPermssions->method('getStorage') ->willReturn($storage); + $allPermssions->method('getMountPoint')->willReturn($movableMount); $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]; From e1505aca4f4ff538e4cb7060c53018c2413b2586 Mon Sep 17 00:00:00 2001 From: Tobia De Koninck Date: Fri, 3 Jan 2020 08:43:39 +0100 Subject: [PATCH 3/5] Add option to transfer-ownership to move data This will move the home folder of own user to another user. Only allowed if that other user's home folder is empty. Can be used as workaround to rename users. Signed-off-by: Tobia De Koninck --- apps/files/lib/Command/TransferOwnership.php | 10 ++++++++-- .../lib/Service/OwnershipTransferService.php | 19 ++++++++++++++++--- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/apps/files/lib/Command/TransferOwnership.php b/apps/files/lib/Command/TransferOwnership.php index cad1b1b46cbe9..cad007ffa7343 100644 --- a/apps/files/lib/Command/TransferOwnership.php +++ b/apps/files/lib/Command/TransferOwnership.php @@ -77,7 +77,12 @@ protected function configure() { InputOption::VALUE_REQUIRED, 'selectively provide the path to transfer. For example --path="folder_name"', '' - ); + )->addOption( + 'move', + null, + InputOption::VALUE_NONE, + 'move data from source user to root directory of destination user, which must be empty' + ); } protected function execute(InputInterface $input, OutputInterface $output) { @@ -99,7 +104,8 @@ protected function execute(InputInterface $input, OutputInterface $output) { $sourceUserObject, $destinationUserObject, ltrim($input->getOption('path'), '/'), - $output + $output, + $input->hasArgument('move') ); } catch (TransferOwnershipException $e) { $output->writeln("" . $e->getMessage() . ""); diff --git a/apps/files/lib/Service/OwnershipTransferService.php b/apps/files/lib/Service/OwnershipTransferService.php index 8530edd17b17f..8af894a01673a 100644 --- a/apps/files/lib/Service/OwnershipTransferService.php +++ b/apps/files/lib/Service/OwnershipTransferService.php @@ -71,12 +71,16 @@ public function __construct(IEncryptionManager $manager, * @param IUser $destinationUser * @param string $path * + * @param OutputInterface|null $output + * @param bool $move * @throws TransferOwnershipException + * @throws \OC\User\NoUserException */ public function transfer(IUser $sourceUser, IUser $destinationUser, string $path, - ?OutputInterface $output = null): void { + ?OutputInterface $output = null, + bool $move = false): void { $output = $output ?? new NullOutput(); $sourceUid = $sourceUser->getUID(); $destinationUid = $destinationUser->getUID(); @@ -87,8 +91,12 @@ public function transfer(IUser $sourceUser, throw new TransferOwnershipException("The target user is not ready to accept files. The user has at least to have logged in once.", 2); } - $date = date('Y-m-d H-i-s'); - $finalTarget = "$destinationUid/files/transferred from $sourceUid on $date"; + if ($move) { + $finalTarget = "$destinationUid/files/"; + } else { + $date = date('Y-m-d H-i-s'); + $finalTarget = "$destinationUid/files/transferred from $sourceUid on $date"; + } // setup filesystem Filesystem::initMountPoints($sourceUid); @@ -99,6 +107,11 @@ public function transfer(IUser $sourceUser, throw new TransferOwnershipException("Unknown path provided: $path", 1); } + if ($move && (!$view->is_dir($finalTarget) || count($view->getDirectoryContent($finalTarget)) > 0)) { + throw new TransferOwnershipException("Destination path does not exists or is not empty", 1); + } + + // analyse source folder $this->analyse( $sourceUid, From 9c42e85f91b15797256a10156d83569cdd9cbaec Mon Sep 17 00:00:00 2001 From: Tobia De Koninck Date: Fri, 3 Jan 2020 08:48:17 +0100 Subject: [PATCH 4/5] Prevent transferring data to user which never loggedin Signed-off-by: Tobia De Koninck --- apps/files/lib/Service/OwnershipTransferService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/files/lib/Service/OwnershipTransferService.php b/apps/files/lib/Service/OwnershipTransferService.php index 8af894a01673a..3dc55615c9525 100644 --- a/apps/files/lib/Service/OwnershipTransferService.php +++ b/apps/files/lib/Service/OwnershipTransferService.php @@ -87,7 +87,7 @@ public function transfer(IUser $sourceUser, $sourcePath = rtrim($sourceUid . '/files/' . $path, '/'); // target user has to be ready - if (!$this->encryptionManager->isReadyForUser($destinationUid)) { + if ($destinationUser->getLastLogin() === 0 || !$this->encryptionManager->isReadyForUser($destinationUid)) { throw new TransferOwnershipException("The target user is not ready to accept files. The user has at least to have logged in once.", 2); } From d89d9871bbe0fa15b36e30633013f3dbfd8cf4b6 Mon Sep 17 00:00:00 2001 From: Tobia De Koninck Date: Fri, 3 Jan 2020 08:50:37 +0100 Subject: [PATCH 5/5] Catch \Error in Transfer::restoreShares This makes the command more fault tolerant. An \Error can happen when e.g. the owner of a share is null. If we don't catch this, the restore process will stop in an unknown state. Signed-off-by: Tobia De Koninck --- apps/files/lib/Service/OwnershipTransferService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/files/lib/Service/OwnershipTransferService.php b/apps/files/lib/Service/OwnershipTransferService.php index 3dc55615c9525..b130910e25b31 100644 --- a/apps/files/lib/Service/OwnershipTransferService.php +++ b/apps/files/lib/Service/OwnershipTransferService.php @@ -286,7 +286,7 @@ private function restoreShares(string $sourceUid, } } catch (\OCP\Files\NotFoundException $e) { $output->writeln('Share with id ' . $share->getId() . ' points at deleted file, skipping'); - } catch (\Exception $e) { + } catch (\Throwable $e) { $output->writeln('Could not restore share with id ' . $share->getId() . ':' . $e->getTraceAsString() . ''); } $progress->advance();