Fix resharing of federated shares that were created out of links by juliusknorr · Pull Request #19793 · nextcloud/server

juliusknorr · 2020-03-05T15:03:52Z

This basically fixes two seaparte issues that caused the issue description from below:

When converting a link share into a federated share, the share type LINK was kept in the oc_share entry
Link shares have reshare permissions by default now if outgoing server2server sharing is enabled

Steps to reproduce

Create 3 different test users (User A, User B, User C)
User A shares a folder with the name “FOLDER” by link (with write perms) and sends this link to User B
User B takes this link and adds the shared folder to his Nextcloud space. User B can now access this shared folder as if it was his own folder.
User B shares the folder to User C as a “native” share (by searching for “User C” in the share dialog)
User C now sees the folder “FOLDER” in his personal Nextcloud space. As soon as User C clicks the folder it disappears and an error message pops up („Operation not permitted“). The folder is not listed under the normal files any more, but still shows up as “Shared with you”.

Caveat

One unfixed part remains. Since before this patch accessing the reshare was possible for the first time and only failed on continuous requests. This is mainly caused by the super share being initialized first

server/apps/files_sharing/lib/SharedStorage.php

Line 90 in 5bf3d1b

$this->superShare = $arguments['superShare'];

and not being updated after the external share is being scanned and the permission of the node is updated

server/lib/private/Files/Cache/Watcher.php

Line 103 in 5bf3d1b

public function update($path, $cachedData) {

nickvergessen · 2020-03-05T15:41:31Z

apps/files_sharing/lib/Controller/ShareAPIController.php

-				$share->setPermissions(Constants::PERMISSION_READ);
+				$permissions = Constants::PERMISSION_READ;
+			}
+			// TODO: It might make sense to have a dedicated setting to allow/deny converting link shares into federated ones


I think the reason is, that you can still mount the dav endpoint as external storage

Right, so in that case turning of share permissions on a link would be kind of hiding it only anyway.

ChristophWurst

Code makes sense, though I'm not a sharing expert

rullzer

Looks good

rullzer · 2020-03-24T19:34:46Z

But ci says no

skjnldsv · 2020-04-11T06:29:51Z

CI still says no

1) OCA\Files_Sharing\Tests\ApiTest::testCreateShareLink
128 | Failed asserting that 17 matches expected 1.
129 |  
130 | /drone/src/apps/files_sharing/tests/ApiTest.php:209
131 |  
132 | 2) OCA\Files_Sharing\Tests\ApiTest::testCreateShareLinkPublicUpload
133 | Failed asserting that 31 matches expected 15.
134 |  
135 | /drone/src/apps/files_sharing/tests/ApiTest.php:235

Signed-off-by: Julius Härtl <jus@bitgrid.net>

…enabled Signed-off-by: Julius Härtl <jus@bitgrid.net>

Signed-off-by: Julius Härtl <jus@bitgrid.net>

juliusknorr · 2020-04-23T11:59:35Z

Ok, some sharing integration tests also need fixes here, I'll have another look.

Signed-off-by: Julius Härtl <jus@bitgrid.net>

juliusknorr · 2020-04-23T17:01:53Z

Fine now. The integration-sharees-* ones are different issues failing on master as well.

skjnldsv · 2020-04-29T14:02:20Z

apps/files_sharing/lib/Controller/ShareAPIController.php

+			}
+			// TODO: It might make sense to have a dedicated setting to allow/deny converting link shares into federated ones
+			if ($this->shareManager->outgoingServer2ServerSharesAllowed()) {
+				$permissions |= Constants::PERMISSION_SHARE;


So, this actually breaks the UI.
Because suddenly we have SHARE+READ (17) instead of just READ (1).
The front doesn't understand it as it's not part of the available options.

@juliushaertl

server/apps/files_sharing/src/components/SharingEntryLink.vue

Lines 352 to 355 in 699cbeb

publicUploadRWValue: OC.PERMISSION_UPDATE | OC.PERMISSION_CREATE | OC.PERMISSION_READ | OC.PERMISSION_DELETE,

publicUploadRValue: OC.PERMISSION_READ,

publicUploadWValue: OC.PERMISSION_CREATE,

So it's even worse, we add the Share permission on create
But we always remove it on update. 🤔

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

Lines 992 to 993 in cb05782

$newPermissions = (int) $permissions;

$newPermissions = $newPermissions & ~Constants::PERMISSION_SHARE;

Fix in progress in #20726

skjnldsv · 2020-04-29T14:59:48Z

Also, needs backport, no ?

skjnldsv · 2020-04-29T15:12:34Z

/backport to stable18

backportbot-nextcloud · 2020-04-29T15:14:13Z

The backport to stable18 failed. Please do this backport manually.

MorrisJobke · 2020-05-25T19:14:02Z

Also, needs backport, no ?

@juliushaertl 🏓

juliusknorr added bug 3. to review Waiting for reviews labels Mar 5, 2020

juliusknorr added this to the Nextcloud 19 milestone Mar 5, 2020

juliusknorr requested review from ChristophWurst, icewind1991, kesselb, nickvergessen, rullzer and schiessle March 5, 2020 15:03

nickvergessen reviewed Mar 5, 2020

View reviewed changes

ChristophWurst approved these changes Mar 9, 2020

View reviewed changes

rullzer approved these changes Mar 24, 2020

View reviewed changes

juliusknorr added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 3, 2020

rullzer mentioned this pull request Apr 4, 2020

19 beta 2 #20303

Merged

80 tasks

juliusknorr force-pushed the bugfix/noid/link-to-federated-reshare branch from ea47311 to 2ac680c Compare April 9, 2020 09:36

juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 9, 2020

rullzer mentioned this pull request Apr 9, 2020

19 beta 3 #20386

Merged

59 tasks

skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 11, 2020

This was referenced Apr 15, 2020

19 beta 4 #20506

Merged

19 Beta 5 #20510

Merged

juliusknorr added 3 commits April 21, 2020 14:35

Set proper share type when converting link shares to federated shares

45c3ee7

Signed-off-by: Julius Härtl <jus@bitgrid.net>

Link shares have reshare permission if outgoing federated shares are …

5646f7c

…enabled Signed-off-by: Julius Härtl <jus@bitgrid.net>

Remove unneeded test since links have resharing permissions by default

397ab1c

Signed-off-by: Julius Härtl <jus@bitgrid.net>

juliusknorr force-pushed the bugfix/noid/link-to-federated-reshare branch from 2ac680c to 397ab1c Compare April 21, 2020 12:35

juliusknorr removed the 2. developing Work in progress label Apr 21, 2020

juliusknorr added the 4. to release Ready to be released and/or waiting for tests to finish label Apr 21, 2020

rullzer mentioned this pull request Apr 23, 2020

19 beta 6 #20611

Merged

11 tasks

juliusknorr added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Apr 23, 2020

Adjust integration tests to new permissions

7a9469f

Signed-off-by: Julius Härtl <jus@bitgrid.net>

juliusknorr added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Apr 23, 2020

MorrisJobke approved these changes Apr 27, 2020

View reviewed changes

MorrisJobke merged commit 1738e17 into master Apr 27, 2020

MorrisJobke deleted the bugfix/noid/link-to-federated-reshare branch April 27, 2020 09:05

skjnldsv reviewed Apr 29, 2020

View reviewed changes

skjnldsv mentioned this pull request Apr 29, 2020

Fix federated link sharing permissions #20726

Merged

backportbot-nextcloud bot added the backport-request label Apr 29, 2020

This was referenced May 26, 2020

[stable18] Fix resharing of federated shares that were created out of links #21109

Merged

[stable17] Fix resharing of federated shares that were created out of links #21110

Merged

juliusknorr removed the backport-request label May 26, 2020

joshtrichards mentioned this pull request Aug 19, 2024

Create Share API not honoring "permissions" parameter #17504

Closed

	publicUploadRWValue: OC.PERMISSION_UPDATE \| OC.PERMISSION_CREATE \| OC.PERMISSION_READ \| OC.PERMISSION_DELETE,
	publicUploadRValue: OC.PERMISSION_READ,
	publicUploadWValue: OC.PERMISSION_CREATE,

	$newPermissions = (int) $permissions;
	$newPermissions = $newPermissions & ~Constants::PERMISSION_SHARE;

Uh oh!

Conversation

juliusknorr commented Mar 5, 2020

Steps to reproduce

Caveat

Uh oh!

nickvergessen Mar 5, 2020

Choose a reason for hiding this comment

Uh oh!

juliusknorr Mar 5, 2020

Choose a reason for hiding this comment

Uh oh!

ChristophWurst left a comment

Choose a reason for hiding this comment

Uh oh!

rullzer left a comment

Choose a reason for hiding this comment

Uh oh!

rullzer commented Mar 24, 2020

Uh oh!

skjnldsv commented Apr 11, 2020

Uh oh!

juliusknorr commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juliusknorr commented Apr 23, 2020

Uh oh!

skjnldsv Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

skjnldsv Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skjnldsv Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

skjnldsv Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

skjnldsv commented Apr 29, 2020

Uh oh!

skjnldsv commented Apr 29, 2020

Uh oh!

backportbot-nextcloud bot commented Apr 29, 2020

Uh oh!

MorrisJobke commented May 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

juliusknorr commented Apr 23, 2020 •

edited

Loading

skjnldsv Apr 29, 2020 •

edited

Loading