Skip to content

Conversation

@ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Mar 16, 2021

With most simple storage we actually want data to be transferable to new users, even before the target user has logged in for the first time. Hence I'm scoping the login check to only be run when encryption is actually in use.

The check was added in #16439. @LEDfan did you use encryption in your tests back then?

@blizzz
Copy link
Member

blizzz commented Mar 16, 2021

What's missing is that the file system is set up for the target users when they did not log in yet – then its not there and moving fails, with or without --move flag.

@ChristophWurst ChristophWurst force-pushed the fix/ownership-transfer-ready-encryption branch from 16c7794 to e68c4a4 Compare March 19, 2021 10:26
@blizzz
Copy link
Member

blizzz commented Mar 19, 2021

The method OC\Server::getUserFolder has been marked as deprecated

@ChristophWurst
Copy link
Member Author

The method OC\Server::getUserFolder has been marked as deprecated

Yeah … there is no easy replacement. I deprecated that one to slim down the public API.

I'll fix it in another PR.

@blizzz
Copy link
Member

blizzz commented Mar 19, 2021

Yeah … there is no easy replacement. I deprecated that one to slim down the public API.

get → IRootFolder → getUserFolder?

I'll fix it in another PR.

okidoki

of course

if ($destinationUser->getLastLogin() === 0 || !$this->encryptionManager->isReadyForUser($destinationUid)) {
// If encryption is on we have to ensure the user has logged in before and that all encryption modules are ready
if (($this->encryptionManager->isEnabled() && $destinationUser->getLastLogin() === 0)
|| !$this->encryptionManager->isReadyForUser($destinationUid)) {
Copy link
Member

Choose a reason for hiding this comment

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

That is a method that's not in the Interface. Do we want to put it there or declare that $this->encryptionManager is the implementation and not the interface even if it is in an app (it's still the files app which will always be shipped, but still)

Copy link
Member

Choose a reason for hiding this comment

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

Or add it to the baseline 🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah let's inject the implementation

Copy link
Member

Choose a reason for hiding this comment

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

This wasn't taken care of and Psalm will complain 🤯

Copy link
Member Author

Choose a reason for hiding this comment

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

I know 🤷

@rullzer rullzer merged commit 90909ab into master Mar 22, 2021
@rullzer rullzer deleted the fix/ownership-transfer-ready-encryption branch March 22, 2021 19:28
@blizzz
Copy link
Member

blizzz commented May 3, 2021

/backport to stable21

@blizzz
Copy link
Member

blizzz commented May 3, 2021

/backport to stable20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants