Fix issue with personal mount points and sharing

[RobinMcCorkell]

An issue existed where readData used OCP\User::getUser() to get the user for personal mount points, which worked in all situations apart from when a personal mount point was used for sharing, so the return from getUser() is not the user that owns the share. As such, any personal mount points would not work correctly when shared.

readData and writeData have been changed from using a $isPersonal boolean to using a $user string|null. $isPersonal = false can now be written as $user = NULL (or left out in the case of readData), and $isPersonal = true can be written as $user = OCP\User::getUser().

This PR fixes the sharing issues encountered in #8167.

[scrutinizer-notifier]

The inspection completed: 1 new issues, 8 updated code elements

[ghost]

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4379/

[karlitschek]

@schiesbn

[schiessle]

@Xenopathic can you tell me the steps you performed to test it?

I tried following:

• mount a ftp storage to /ftp
• share /ftp to user2

I run into multiple problem, to be fair they seems to be not directly connected to your changes:

1. At the first try I couldn't share the /ftp folder at all. Because the check if the file exists, introduced by @icewind1991 (Verify that a file exists before we share it #7780) fails for external storage mount point. Probably the external storage implementation for file_exists returns false for the root? Needs to be checked and fixed.

( I removed the check to be able to test this branch and shared the /ftp folder successfully)

1. Login as user2 and go to the Shared folder: The shared folder has no name, I only see the folder icon and I can't navigate into the folder.

Both problems exists on master and on your branch, so I can't test your changes but I also wonder how it worked for you?

Can you look into this issues, also if they are not directly related to your PR?

[RobinMcCorkell]

@schiesbn The problem can be replicated as follows:

1. Create a personal mount point (needs to be valid)
2. Share a directory from inside that share to another user
3. Attempt to view the directory as that user

You shouldn't be able to access that folder, and owncloud.log will contain errors relating to calling getMountPoint() on a non-object (errors fixed by #8285, that branch instead pushes the user back to their home view and logs a useful error). This is caused by the personal mount point not getting mounted, since the code to get personal mount points is hardcoded to use OCP\User::getUser() which always returns the logged in user instead of the shared directory owner.

[icewind1991]

At the first try I couldn't share the /ftp folder at all. Because the check if the file exists, introduced by @icewind1991 (#7780) fails for external storage mount point. Probably the external storage implementation for file_exists returns false for the root? Needs to be checked and fixed.

I'll have a look at this

[PVince81]

@Xenopathic I tried your steps:
On master: the shared dir doesn't appear for user2 which is the bug in question.
On this branch alone (old Shared dir): the shared dir appears for user2 and its contents can be viewed.
On this branch, but with master merged into it (removed Shared dir): also works.

You might want to rebase this branch onto master to get the latest state of the sharing code. The "Shared" folder has been removed and shares can now be moved freely 😄

👍

[RobinMcCorkell]

@PVince81 so this PR fixes the issue, even with the new 'free' shared directories? I don't see any reason to rebase if this is the case, since this only touches code in files_external and not files_sharing.

@schiesbn any luck testing this out?

[PVince81]

Yes it does work with the new approach.

If you don't rebase then I suggest that the potential testers use the "merge master into this branch" locally to test, to make sure it will work with the latest work. Recently I decided to always go this route to make sure that merged code will also work on master.

[PVince81]

@icewind1991 @schiesbn can you review this as well ? Thanks.

[icewind1991]

Code looks good 👍

[PVince81]

Thanks, but you haven't seen how good the unit test will look 😉

[RobinMcCorkell]

@PVince81 Do you want me to do the unit test or are you doing it? I wasn't aware this needed one...

[PVince81]

Sorry, I commented in the wrong ticket... didn't have ☕ this morning.

[PVince81]

@karlitschek @craigpg we need to backport this to OC 6 to fix #9535

That issue on OC 6 has been introduced by the backport of the password obfuscation code which brought in additional required changes.

[PVince81]

Backport PR is here: #9635