Various fixes for transfer ownership #16439

LEDfan · 2019-07-17T12:33:43Z

First of all this adds the --move option, this will move the data into the root of the target user, but only if this user doesn't have any files. This can be used as a workaround for renaming users (which is required in one setup when the UID of ldap changes).

The three other commits are improvements to the transfer-ownership command. During my tests on a real instance, there was some issue with group folders and also the check to prevent to move to users who never logged in didn't work.

The last commit adds a catch for PHP errors. Sometimes the owner of a share was null, and this would stop the transfer script.

I guess it's okay to have this one PR, if not let me now and I will split it up.

skjnldsv · 2019-09-28T15:32:37Z

Please rebase @LEDfan :)

blizzz

looks ok, but two nitpicks. didn't test yet.

blizzz · 2019-10-11T15:58:26Z

apps/files/lib/Command/TransferOwnership.php

+				$output->writeln('<error>Could not restore share with id ' . $share->getId() . ':' . $e->getMessage() . '\n' . $e->getTraceAsString() . '</error>');
+			} catch (\Error $e) {
+				$output->writeln('<error>Could not restore share with id ' . $share->getId() . ':' . $e->getMessage() . '\n' . $e->getTraceAsString() . '</error>');


use Throwable, it covers both Exception and Error.

blizzz · 2019-10-11T16:00:00Z

apps/files/lib/Command/TransferOwnership.php

@@ -144,6 +154,11 @@ protected function execute(InputInterface $input, OutputInterface $output) {
 			return 1;
 		}

+		if ($input->hasArgument('move') && (!$view->is_dir($this->finalTarget) || count($view->getDirectoryContent($this->finalTarget)) > 0)) {
+			$output->writeln("<error>Destination path does not exists or is not empty</error>");
+			return 1;


return a different error code

blizzz · 2019-10-31T18:21:47Z

During my tests on a real instance, there was some issue with group folders and also the check to prevent to move to users who never logged in didn't work.

Can you elaborate on that?

We shouldn't have direct references to code from other apps in here. Maybe we need a fix in the app, or a more general rule in this code?

blizzz · 2019-10-31T18:22:48Z

(ah, btw, there is no notification issued with only new commits, but no comment)

ChristophWurst · 2019-12-13T11:46:33Z

@LEDfan could you have another look, please? Since your PR we've extracted the code into a reusable service #18025 because it's not only used by the CLI command in Nextcloud 18. Thus some of the changes have to be applied there.

wiswedel · 2020-01-02T10:45:46Z

@LEDfan could you have another look, please? Since your PR we've extracted the code into a reusable service #18025 because it's not only used by the CLI command in Nextcloud 18. Thus some of the changes have to be applied there.

Could someone please speed this up so we can start digging into nextcloud/guests#223 (which I need for a customer)?

Because Group folders don't have owners, use the same system to check the permissions of the node. Signed-off-by: Tobia De Koninck <[email protected]>

Signed-off-by: Tobia De Koninck <[email protected]>

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 <[email protected]>

Signed-off-by: Tobia De Koninck <[email protected]>

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 <[email protected]>

LEDfan · 2020-01-03T08:12:30Z

@ChristophWurst @wiswedel I applied the changes to the new code and quickly tested it. I'm currently doing my master thesis, so don't have much time to thoroughly test it.

During my tests on a real instance, there was some issue with group folders and also the check to prevent to move to users who never logged in didn't work.

Can you elaborate on that?
We shouldn't have direct references to code from other apps in here. Maybe we need a fix in the app, or a more general rule in this code?

I understand. IIRC the problem with the group folders was something like:

assume there is a group folder GF_test shared to the everyone group. This group has some user s user1-user4
user2 creates a folder in GF_test
user2 shares this folder (in the GF) again with user3
try do transfer from user3 to user4

On the instance I was speaking of, this throws exceptions and fails the transfer.
I just tried to reproduce this on the current master. However, the shared folder with user3 does not appear in the home folder of user3. In the section Shared with you there is also no folder, but there is the text 1 folder. The transfer command also now works without the change.

Maybe something has changed in NC18 regarding to re-sharing folders inside groupfolders?
So, I guess it may the safest option to remove the commit related to groupfolders.

wiswedel · 2020-01-03T12:22:55Z

So, I guess it may the safest option to remove the commit related to groupfolders.

I agree. Group folders adds a hell lot of complexity to the transfer operation. I vote for skipping group folder shares for the time being and mark that as an open issue. Better this way covering all "normal" shares than having a rushed implementation with hidden non-tested corner cases

ChristophWurst

Code looks good!

ChristophWurst · 2020-01-07T16:05:34Z

apps/files/lib/Command/TransferOwnership.php

@@ -99,7 +104,8 @@ protected function execute(InputInterface $input, OutputInterface $output) {
 				$sourceUserObject,
 				$destinationUserObject,
 				ltrim($input->getOption('path'), '/'),
-				$output
+				$output,
+				$input->hasArgument('move')


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

rullzer · 2020-01-08T09:33:20Z

So, I guess it may the safest option to remove the commit related to groupfolders.

I agree. Group folders adds a hell lot of complexity to the transfer operation. I vote for skipping group folder shares for the time being and mark that as an open issue. Better this way covering all "normal" shares than having a rushed implementation with hidden non-tested corner cases

Group folders should not fall in the transfer ownership transfer at all. Since the user doesn't own them ;)

rullzer · 2020-01-08T09:36:25Z

apps/files/lib/Service/OwnershipTransferService.php

@@ -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);


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

ChristophWurst · 2021-06-25T12:04:18Z

No update in a long time and conflicts badly. Let's do this from scratch if it's still relevant. If I'm not mistaken we added some of the changes in other PRs.

LEDfan force-pushed the fixes_transfer_ownership branch from b72154a to 07eb517 Compare July 18, 2019 08:54

skjnldsv added the 3. to review Waiting for reviews label Sep 28, 2019

skjnldsv requested review from nickvergessen, MorrisJobke, sharidas, rullzer and blizzz September 28, 2019 15:31

skjnldsv added bug feature: users and groups labels Sep 28, 2019

skjnldsv added this to the Nextcloud 18 milestone Sep 28, 2019

blizzz requested a review from icewind1991 October 11, 2019 15:56

blizzz reviewed Oct 11, 2019

View reviewed changes

LEDfan force-pushed the fixes_transfer_ownership branch 2 times, most recently from 4be34f9 to 0fd8a31 Compare October 12, 2019 14:55

skjnldsv requested a review from blizzz October 13, 2019 07:16

This was referenced Dec 11, 2019

18 Beta 2 #18343

Merged

18 beta 3 #18370

Merged

ChristophWurst added 0. Needs triage Pending check for reproducibility or if it fits our roadmap and removed 3. to review Waiting for reviews labels Dec 13, 2019

ChristophWurst removed this from the Nextcloud 18 milestone Dec 13, 2019

wiswedel mentioned this pull request Dec 13, 2019

Use transfer --move on guest conversion nextcloud/guests#223

Closed

1 task

LEDfan added 4 commits January 3, 2020 08:53

Fix transfer-ownership for Group Folders

5fb6e9e

Because Group folders don't have owners, use the same system to check the permissions of the node. Signed-off-by: Tobia De Koninck <[email protected]>

Fix tests

061a919

Signed-off-by: Tobia De Koninck <[email protected]>

Add option to transfer-ownership to move data

e1505ac

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 <[email protected]>

Prevent transferring data to user which never loggedin

9c42e85

Signed-off-by: Tobia De Koninck <[email protected]>

Catch \Error in Transfer::restoreShares

d89d987

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 <[email protected]>

LEDfan force-pushed the fixes_transfer_ownership branch from 0fd8a31 to d89d987 Compare January 3, 2020 07:56

ChristophWurst approved these changes Jan 7, 2020

View reviewed changes

rullzer reviewed Jan 8, 2020

View reviewed changes

ChristophWurst mentioned this pull request Mar 16, 2021

Only perform login check during ownership transfer for encryption #26149

Merged

ChristophWurst closed this Jun 25, 2021

ChristophWurst deleted the fixes_transfer_ownership branch June 25, 2021 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various fixes for transfer ownership #16439

Various fixes for transfer ownership #16439

LEDfan commented Jul 17, 2019

skjnldsv commented Sep 28, 2019

blizzz left a comment

blizzz Oct 11, 2019

blizzz Oct 11, 2019

blizzz commented Oct 31, 2019

blizzz commented Oct 31, 2019

ChristophWurst commented Dec 13, 2019

wiswedel commented Jan 2, 2020

LEDfan commented Jan 3, 2020

wiswedel commented Jan 3, 2020

ChristophWurst left a comment

ChristophWurst Jan 7, 2020

rullzer commented Jan 8, 2020

rullzer Jan 8, 2020

ChristophWurst commented Jun 25, 2021

Various fixes for transfer ownership #16439

Various fixes for transfer ownership #16439

Conversation

LEDfan commented Jul 17, 2019

skjnldsv commented Sep 28, 2019

blizzz left a comment

Choose a reason for hiding this comment

blizzz Oct 11, 2019

Choose a reason for hiding this comment

blizzz Oct 11, 2019

Choose a reason for hiding this comment

blizzz commented Oct 31, 2019

blizzz commented Oct 31, 2019

ChristophWurst commented Dec 13, 2019

wiswedel commented Jan 2, 2020

LEDfan commented Jan 3, 2020

wiswedel commented Jan 3, 2020

ChristophWurst left a comment

Choose a reason for hiding this comment

ChristophWurst Jan 7, 2020

Choose a reason for hiding this comment

rullzer commented Jan 8, 2020

rullzer Jan 8, 2020

Choose a reason for hiding this comment

ChristophWurst commented Jun 25, 2021