Shares are displayed to users with resharing rights

[ArtificialOwl]

Signed-off-by: Maxence Lange maxence@artificial-owl.com

[ArtificialOwl]

This is a proof of concept.
The idea is to provide the list of current shares on a file not just to owner but to users with re-sharing rights.

missing:

• Circles
• Check if it is working for every client
• tests

[MorrisJobke]

cc @nextcloud/sharing

[ArtificialOwl]

The circles app needs to be upgraded to https://github.com/nextcloud/circles/tree/compat15

[ArtificialOwl]

julius made some change in the UI so that there is no more possible interaction (with errors) when user have resharing rights, but is not owner.

I would like to have someone with better knowledge on the sharing stuff, while we check with the new tests.

@rullzer maybe ?

[rullzer]

I think you can just get the node the share points to here and check if you have share permissions. Then the whole backend works for you to figure out if you have sharing rights.

[rullzer]

I think the basis are good.

However I'm not sure if the provider changes are enough.
basically we'd just need to fetch all shares for a given file and then later on we can filter (I do not think we can do this on a DB level).

because if the file is owned by me. (So i'm the owner). Or the file is reshared by me.
However consider

1. userA shares a file with userB with reshare permissions
2. userB shares the same file to userC without reshare permissions
3. userA shares the same file to userD with reshare permisions
4. userD shares the same file to userE with reshare permissions.

Now if userA, userB, userD or userE query the endpoint they should all get the complete list of people with share access right?
They cannot edit all (there is still the inheritance right?)

Only userC should not get the list.

I do not think this change accomplisches this yet right?

[ArtificialOwl]

@rullzer so I made a bunch of manual test based on your list, and it work as expected. Last work from @juliushaertl improve a lot UX.

• People who can edit, can edit their files.
• People who cannot edit but can reshare, the subpeople they reshare to cannot edit.
• People who cannot reshare but can edit, can edit but cannot reshare.
• People can only delete share they have generated.

[ArtificialOwl]

However, tests needs to be checked !

[juliusknorr]

Failing unit tests don't look related:

1) OCA\Comments\Tests\Unit\Notification\NotifierTest::testPrepareSuccess
OCP\Notification\INotification::getUser() was not expected to be called more than once.

/drone/src/github.com/nextcloud/server/apps/comments/lib/Notification/Notifier.php:109
/drone/src/github.com/nextcloud/server/apps/comments/tests/Unit/Notification/NotifierTest.php:186

2) OCA\Comments\Tests\Unit\Notification\NotifierTest::testPrepareSuccessDeletedUser
OCP\Notification\INotification::getUser() was not expected to be called more than once.

/drone/src/github.com/nextcloud/server/apps/comments/lib/Notification/Notifier.php:109
/drone/src/github.com/nextcloud/server/apps/comments/tests/Unit/Notification/NotifierTest.php:273

Edit: They are unrelated #12009 (comment)

[juliusknorr]

@daita As far as i can see, there is just a phan error really failing due to this PR.

+ ./lib/composer/phan/phan/phan -k build/.phan/config.php
apps/files_sharing/lib/Controller/ShareAPIController.php:1140 PhanUndeclaredClassMethod Call to method getMember from undeclared class \OCA\Circles\Api\v1\Circles

🙈

[ArtificialOwl]

wtf, all tests are green !?

[MorrisJobke]

@nextcloud/sharing Please review this one

[juliusknorr]

Rebased to resolve conflicts.

[MorrisJobke]

@juliushaertl Do you mind to also review it?

[rullzer]

So this will require client changes.

Can you tell me exactly when a client has the rights to edit an entry?
Should we add an extra field to the response?

[ArtificialOwl]

There is no change in the edition of an entry, we only providing more information (shares list) to people with resharing rights.

Meaning, you can edit an entry when you are the owner of the share, or owner of the file.

Note that the listing contains all shares and is complete: the API will send the current username of the viewer, so this should be hidden by the client.

[rullzer]

ok so it indeed works as adversited

However we will need to extend the client to show the same info.
cc: @tobiasKaminsky @marinofaggiana @juliushaertl @camilasan @rullzer

I'm just pinging you all here so somebody reminds me/us that we need to fix that.

[rullzer]

LGTM merged now and fix issues we find later.

[tobiasKaminsky]

However we will need to extend the client to show the same info.
cc: @tobiasKaminsky @marinofaggiana @juliushaertl @camilasan @rullzer

Can you give as info, how where and what we should display? :-)
On android I would not do this on file list, as there is not much space, but on file/folders details view?

[tobiasKaminsky]

I got it now:

• create file/folder
• share it with multiple person/link and user A
  

as user A:

• log in
• see details of file/folder
  

So this is on user A's side a readonly view.

[tobiasKaminsky]

http://localhost/nc/ocs/v2.php/apps/files_sharing/api/v1/shares?path=%2Freshare&reshares=true
gives me

<?xml version="1.0"?>
<ocs>
	<meta>
		<status>ok</status>
		<statuscode>200</statuscode>
		<message>OK</message>
	</meta>
	<data>
		<element>
			<id>369</id>
			<share_type>0</share_type>
			<uid_owner>admin</uid_owner>
			<displayname_owner>admin</displayname_owner>
			<permissions>31</permissions>
			<stime>1541401350</stime>
			<parent/>
			<expiration/>
			<token/>
			<uid_file_owner>admin</uid_file_owner>
			<note></note>
			<label/>
			<displayname_file_owner>admin</displayname_file_owner>
			<path>/reshare</path>
			<item_type>folder</item_type>
			<mimetype>httpd/unix-directory</mimetype>
			<storage_id>shared::/reshare</storage_id>
			<storage>1</storage>
			<item_source>72222</item_source>
			<file_source>72222</file_source>
			<file_parent>103</file_parent>
			<file_target>/reshare</file_target>
			<share_with>tobi</share_with>
			<share_with_displayname>Kaminsky Tobi</share_with_displayname>
			<mail_send>0</mail_send>
			<hide_download>0</hide_download>
		</element>
		<element>
			<id>372</id>
			<share_type>0</share_type>
			<uid_owner>admin</uid_owner>
			<displayname_owner>admin</displayname_owner>
			<permissions>31</permissions>
			<stime>1541401424</stime>
			<parent/>
			<expiration/>
			<token/>
			<uid_file_owner>admin</uid_file_owner>
			<note></note>
			<label/>
			<displayname_file_owner>admin</displayname_file_owner>
			<path>/reshare</path>
			<item_type>folder</item_type>
			<mimetype>httpd/unix-directory</mimetype>
			<storage_id>shared::/reshare</storage_id>
			<storage>1</storage>
			<item_source>72222</item_source>
			<file_source>72222</file_source>
			<file_parent>103</file_parent>
			<file_target>/reshare</file_target>
			<share_with>test</share_with>
			<share_with_displayname>test</share_with_displayname>
			<mail_send>0</mail_send>
			<hide_download>0</hide_download>
		</element>
		<element>
			<id>371</id>
			<share_type>3</share_type>
			<uid_owner>admin</uid_owner>
			<displayname_owner>admin</displayname_owner>
			<permissions>1</permissions>
			<stime>1541401417</stime>
			<parent/>
			<expiration/>
			<token>qQ47qMLWrnfK6yw</token>
			<uid_file_owner>admin</uid_file_owner>
			<note></note>
			<label></label>
			<displayname_file_owner>admin</displayname_file_owner>
			<path>/reshare</path>
			<item_type>folder</item_type>
			<mimetype>httpd/unix-directory</mimetype>
			<storage_id>shared::/reshare</storage_id>
			<storage>1</storage>
			<item_source>72222</item_source>
			<file_source>72222</file_source>
			<file_parent>103</file_parent>
			<file_target>/reshare</file_target>
			<share_with/>
			<share_with_displayname/>
			<send_password_by_talk></send_password_by_talk>
			<url>http://localhost/nc/index.php/s/qQ47qMLWrnfK6yw</url>
			<mail_send>0</mail_send>
			<hide_download>0</hide_download>
		</element>
	</data>
</ocs>

How do I see that this is a reshare and current user is not allowed to edit it?
Just because uid_owner is different?

[jospoortvliet]

@daita nice improvement! 👍

[skjnldsv]

@daita there are some weird stuff going on here :)
Can anyone reproduce this:

1. xxx share with yyy with reshare perms
2. xxx share with zzz
3. yyy sees zzz in the sharees list
4. xxx remove reshare rights to yyy
5. yyy does NOT sees zzz anymore

This is supposed to be the correct behaviour, right?
I personally cannot have this, it does't matter if I have the resharing rights or not, I always see zzz! @nextcloud/sharing

[skjnldsv]

@daita yep, found the issue.

server/apps/files_sharing/lib/Controller/ShareAPIController.php

Lines 770 to 772 in 85a80b0

	if (!$resharingRight && $this->shareProviderResharingRights($this->currentUser, $share, $path)) {
	$resharingRight = true;
	}

Once resharingRight we never check for shareProviderResharingRights again.
That means all of the other shares in the array will be set as resharing = true.

@rullzer, @blizzz, @nickvergessen, @MorrisJobke I fail to really grasp how this miniFormatted variable works there, but it seems like we have a bug there.

[ArtificialOwl]

yyy sees the shares to zzz as he is the creator of the reshare.

if xxx remove the reshare from yyy to zzz and create the same reshare to zzz (*xxx is the author of the new reshare), yyy should only be able to see this reshare if he have resharing rights.

Now, the $resharingRight is a small trick I am doing as I am already browsing the list of the shares (for one file) to format the shares, I am using the same loop to check if one of the shares from the list gives to the current viewer resharing rights on the file.

As you can see, I generate 2 lists: $formatted and $miniFormatted $formatted is the default one, but if none of the shares from the list provides resharing rights to the viewer, we're using $miniFormatted

So in the end, after some talk, it was an issue not related to the existing code.

[skjnldsv]

All good btw, we had a late night call with Maxence and figured things out.
A pr is here, but the workflow is allright currently.
#16761