\OC\Share\Share::getUsersSharingFile does not work properly with renamed shares and groups

[LukasReschke]

\OC\Share\Share::getUsersSharingFile is not properly working in combination with renamed shares and groups on stable8 (didn't test master yet – suspect it to be broken as well), this can be reproduced by:

1. Setup instance on stable8 as user "admin"
2. Create a user "test"
3. Add user "test" to group "users"
4. Create a folder "shared" as user "admin" and share with "users"
5. Upload a file "test.txt" to shared as user admin
6. Login as user "test"
7. Rename folder "shared" to folder "foo"
8. Login as user "admin" again

Create a file with the following content and access it as admin:

<?php
require_once('./lib/base.php');

var_dump(\OC\Share\Share::getUsersSharingFile('shared/test.txt', 'admin', true, true));

Return will be:

array (size=2)
  'admin' => string '/shared/test.txt' (length=16)
  'test' => string '/shared/test.txt' (length=16)

Obviously it should resolve to foo instead of shared for the user "test".

cc @icewind1991

[LukasReschke]

cc @PVince81 I guess that was what I was stumbled about…

[LukasReschke]

This works properly if directly sharing to the user.

[LukasReschke]

… I wonder what this may all potentially break in core …

[PVince81]

But strangely encryption works fine, and it uses that.
The key part was to make sure to get the $ownerPath first.

But let me try the steps, maybe this is yet another nuance.

[PVince81]

also CC @schiesbn

[PVince81]

After running the steps, I put the contents into "test.php" and open it in the browser:

• as "admin":

array (size=2)
  'admin' => string 'shared/test.txt' (length=15)
  'test' => string '/sharedtest.txt' (length=15)

• as user "test":

array (size=1)
  'admin' => string 'shared/test.txt' (length=15)

Hmm, but as the admin I'd expect to have "test" there too.

[PVince81]

Wait what ? "/sharedtest.txt" ? Looks like a missing slash.
But even then, it should be called "foo"

[PVince81]

Ok, forgot to say that my file is called "test8.txt" not "test.txt". But still, might be another bug ?! getUsersSharingFile() should not return results for non-existing files.

[PVince81]

Okay now, when pointing the API call at the correct file:

• as "admin":

array (size=2)
  'admin' => string '/shared/test8.txt' (length=17)
  'test' => string '/shared/test8.txt' (length=17)

• as "test":

array (size=1)
  'admin' => string '/shared/test8.txt' (length=17)

At least now the result is consistent with @LukasReschke.

Now if that is really broken, it means encryption (share keys) should break too.
But when I tried yesterday it worked fine.
So either we're doing the call wrong (missing setupFS?) or encryption is doing something else.

Trying encryption now...

[PVince81]

Now I also wonder, are we supposed to call getUsersSharingFile() directly on the file ?
Let's try calling it on the shared folder instead.

[PVince81]

When calling it like this:
var_dump(\OC\Share\Share::getUsersSharingFile('shared', 'admin', true, true));

I get:

• as admin:

array (size=2)
  'admin' => string '/shared' (length=7)
  'test' => string '/shared' (length=7)

• as test:

array (size=1)
  'admin' => string '/shared' (length=7)

Nope 😦

Trying encryption now.

[PVince81]

Bad news, encryption breaks too, here are the steps (on stable8):

1. Setup stable8 with admin user "admin"
2. Login as "admin"
3. Enable encryption
4. Log out then log in again
5. Create a user "test" in a group "users"
6. Create another user "test2" in the group "users"
7. Create a folder "shared"
8. Share "shared" with the group "users"
9. Login as "test"
10. Rename "shared" to "foo"
11. Upload a file "test.txt" into "foo"
12. Login as "test2"
13. Download "shared/test.txt"

Expected: file can be downloaded
Actual: file broken

It is likely that the shared keys for the second user weren't created properly because of the incomplete result returned by getUsersSharingFile()

[schiessle]

@PVince81 I can't reproduce it. I started with a fresh installation of stable8 and followed exactly your steps. after step 11 I checked the keys in data/admin/files_encryption. Share-keys exists for "admin", "test" and "test2", so all shares were resolved correctly. If I log-in as "test2" I can open the file.

[PVince81]

Ok, let me double check the steps then and add as many details as possible.
@LukasReschke and I had a similar case two days ago and it worked.
But now it's broken again, so maybe there's a subtle nuance.

[PVince81]

@schiesbn you're right, executing my exact steps do work... damn. I'll try and find out how I triggered it.

I still don't understand how encryption manages to get the correct users when calling getUsersSharingFile("shared/somefile.txt") when logged in as recipient.
Maybe the difference with @LukasReschke's snippet is that we call getUidAndFilename() which calls getOwner() which might do some setupFS/initMountPoints magic that are missing from the snippet ?

[PVince81]

Okay, I got something: if I add setupFS and getUidAndFilename() to @LukasReschke's snippet, it will now return the correct list of users:

<?php
require_once('./lib/base.php');

\OC_Util::setupFS();

$file = '/foo/test.txt';

//list($owner, $ownerPath) = \OCA\Files_Encryption\Util::getUidAndFilename($file);
list($owner, $ownerPath) = \OCA\Files_Versions\Storage::getUidAndFilename($file);

var_dump($owner);
var_dump($ownerPath);

var_dump(\OC\Share\Share::getUsersSharingFile($ownerPath, $owner, true, true));

When run as "test", I get this:

string 'admin' (length=5)
string '/shared/test.txt' (length=16)
array (size=3)
  'admin' => string '/shared/test.txt' (length=16)
  'test' => string '/shared/test.txt' (length=16)
  'test2' => string '/shared/test.txt' (length=16)

But if I comment out setupFS and the other one, I only get the admin.

At least it makes sense...

Now to the original issue: the path of "test" is wrong and should be "foo" instead of "shared".
However, I grepped the code and did not find any location core where these paths are used.
So this means this bug shouldn't break anything.

[schiessle]

Maybe the difference with @LukasReschke's snippet is that we call getUidAndFilename() which calls getOwner() which might do some setupFS/initMountPoints magic that are missing from the snippet ?

Yes, encryption first resolve the owner and the owner path and then check the shares from the owners perspective. Encryption also only needs the list of owners, we don't care about the path. So the issue described here shouldn't have any influence on encryption.

[PVince81]

And it seems that the resolution itself somehow affects the state of the FS statically, which makes it work...

[PVince81]

The code that builds the paths is here: https://github.com/owncloud/core/blob/stable8/lib/private/share/share.php#L162
It just gets a list of all users and then uses the same "file_target" for all of them instead of getting each one's individual entries.

[PVince81]

I'm writing a unit test first, in the spirit of "test first"

[PVince81]

PR here: #15961 (only contains a unit test to reproduce the issue)

[PVince81]

@nickvergessen has fixed this here #17330