Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Various fixes for transfer ownership #16439

Closed
wants to merge 5 commits into from
Closed

Various fixes for transfer ownership #16439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5fb6e9e
Fix transfer-ownership for Group Folders
LEDfan Jul 17, 2019
061a919
Fix tests
LEDfan Jul 18, 2019
e1505ac
Add option to transfer-ownership to move data
LEDfan Jan 3, 2020
9c42e85
Prevent transferring data to user which never loggedin
LEDfan Jan 3, 2020
d89d987
Catch \Error in Transfer::restoreShares
LEDfan Jan 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions apps/files/lib/Command/TransferOwnership.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -99,7 +104,8 @@ protected function execute(InputInterface $input, OutputInterface $output) {
$sourceUserObject,
$destinationUserObject,
ltrim($input->getOption('path'), '/'),
$output
$output,
$input->hasArgument('move')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC this always returns true as the parameter exists. You can use getOption('move') !== null

);
} catch (TransferOwnershipException $e) {
$output->writeln("<error>" . $e->getMessage() . "</error>");
Expand Down
23 changes: 18 additions & 5 deletions apps/files/lib/Service/OwnershipTransferService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,24 +71,32 @@ 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();
$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);
}

$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);
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this fail in 99% of the cases?
As the example files are put into place?

}


// analyse source folder
$this->analyse(
$sourceUid,
Expand Down Expand Up @@ -273,7 +286,7 @@ private function restoreShares(string $sourceUid,
}
} catch (\OCP\Files\NotFoundException $e) {
$output->writeln('<error>Share with id ' . $share->getId() . ' points at deleted file, skipping</error>');
} catch (\Exception $e) {
} catch (\Throwable $e) {
$output->writeln('<error>Could not restore share with id ' . $share->getId() . ':' . $e->getTraceAsString() . '</error>');
}
$progress->advance();
Expand Down
2 changes: 1 addition & 1 deletion lib/private/Share20/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
10 changes: 7 additions & 3 deletions tests/lib/Share20/ManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

namespace Test\Share20;

use OC\Files\Mount\MoveableMount;
use OC\HintException;
use OC\Share20\DefaultShareProvider;
use OC\Share20\Exception;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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];
Expand All @@ -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];
Expand All @@ -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];
Expand Down