bring back CRUDS permissions for federated shares #23918

schiessle · 2016-04-11T14:14:18Z

bring back CRUDS permissions for federated shares

Display all permissions in share dialog
update webdav permissions (oc:permission are set correctly, it always contain the delete permission because delete === unshare should always be allowed)
update oc:share-permissions (here we can also show the "correct" delete permissions for shared folders) (needs always return the complete permissions the file was shared with #23945)
recipients of federated shares should respect the permissions
update file cache if propfind returns new permission
storage->getPermissions() should also do a propfind

Open Issues:

as soon as permission was set to 31, shareinfo is no longer called -> no information if permission change again (bring back CRUDS permissions for federated shares #23918 (comment))
fall back if server doesn't provide share-permissions
move share-permissions namespace once Move share-permissions property namespace #24017 is merged

mention-bot · 2016-04-11T14:14:21Z

By analyzing the blame information on this pull request, we identified @blizzz, @PVince81 and @rullzer to be potential reviewers

schiessle · 2016-04-12T10:24:40Z

@icewind1991 I have a question regarding the last commit in this PR (51643f4)

In order to make the remote server respect the share permissions I updated the master/index.php/apps/files_sharing/shareinfo?t=<some_token> call to return the right permissions.
This works great. The remote server which received the share calls the shareinfo every time and gets the updated permissions, except one case. If the received permission was 31 the server stops calling shareinfo. The share info call happens in two places:

https://github.com/owncloud/core/blob/master/apps/files_sharing/lib/external/scanner.php#L86
https://github.com/owncloud/core/blob/master/apps/files_sharing/lib/external/storage.php#L184

Do you have a idea why we call it on every request as long as the permission was < 31 and stop calling it as soon as the permission was set to 31? I couldn't find the reason.

schiessle · 2016-04-14T16:46:58Z

code is ready and works... please start to review it... I might add additional unit tests in the mean time.

cc @DeepDiver1975 would be great if you could have a look at the webdav related changes. Thanks!

rullzer · 2016-04-15T07:39:06Z

apps/files_sharing/ajax/shareinfo.php

@@ -62,29 +62,37 @@
 $rootInfo = \OC\Files\Filesystem::getFileInfo($path);


Nothing for now... but we should really get rid of this ajax file and move it to a proper controller...

yes, was also my thought... 😉

rullzer · 2016-04-15T07:41:14Z

You probabaly need to rebase as that share permissions fix is already in master.

schiessle · 2016-04-15T09:02:04Z

You probabaly need to rebase as that share permissions fix is already in master.

Was already up-to-date enough to contain the fix... but I rebased it once more to be on latest master.

schiessle · 2016-04-18T10:15:21Z

permission updated according to #24017
@rullzer, @DeepDiver1975... please review. Thanks!

rullzer · 2016-04-18T14:30:07Z

https://github.com/owncloud/core/pull/23918/files#diff-2e1f33af47835fc7dab530c048c95f9aL157 <-- shouldn't that go as well?

Currently federated shares are shared by default without sharing permissions and without delete permissions. I get that this was the old behaviour. But now it looks weird.

rullzer · 2016-04-18T14:37:43Z

apps/dav/lib/connector/sabre/node.php

+	 * @param string $user
 	 * @return int
 	 */
-	public function getSharePermissions() {


user is actually a bit misleading here...

The problem is that at the calling end the token is stored in the "user"-field. So it is always somehow misleading, either at the calling end or at the receiving end.

... but maybe it is better to call it "token" at this place to make it less confusing for the method as such

schiessle · 2016-04-18T14:39:40Z

Currently federated shares are shared by default without sharing permissions and without delete permissions. I get that this was the old behaviour. But now it looks weird.

Can you elaborate on it? What do you think looks wired. The point of the commit you refer to is that if a server receives a propfind from a user who mounted a federated share we need to return the permissions the file was shared with. This doesn't change the default permissions.

rullzer · 2016-04-18T14:45:33Z

@schiesbn when you initially share the file. Permissions send to the OCS Share API are 7.

It is weird from a user point I think. Because if I share a folder locally it is shared by default with permissions 31. And federated defaults to 7.

schiessle · 2016-04-18T14:47:51Z

It is weird from a user point I think. Because if I share a folder locally it is shared by default with permissions 31. And federated defaults to 7.

Of course we could change the default share permissions to match the local shares.

schiessle · 2016-04-18T15:16:33Z

@rullzer see my last commit... I removed the special handling for remote shares regarding the default permissions

rullzer · 2016-04-18T18:22:30Z

👍

schiessle · 2016-04-19T08:15:35Z

need a second reviewer... @PVince81 @DeepDiver1975 @icewind1991 ? Thanks!

icewind1991 · 2016-04-19T11:38:12Z

apps/dav/lib/connector/sabre/filesplugin.php

-				return $node->getSharePermissions();
+			$propFind->handle(self::SHARE_PERMISSIONS_PROPERTYNAME, function() use ($node, $httpRequest) {
+				return $node->getSharePermissions(
+					$httpRequest->getRawServerValue('PHP_AUTH_USER')


I'm fairly sure this will only work for some server setups

Can you elaborate on it? What could be used instead?

@LukasReschke since you are working on some auth related stuff for 9.1... do you see any issues with it?

I believe this should work considering the fact that OC::handleAuthHeaders does some mapping magic.

schiessle · 2016-04-20T14:05:19Z

@icewind1991 please have another look, I hope you like my last update 😉

icewind1991 · 2016-04-20T15:15:12Z

lib/private/files/storage/dav.php

Since we always set the share permission to a default value in propfind I would expect this to always be true

good point, see my last changes: e906796

…heck if the file has changed or not

…emote shares

icewind1991 · 2016-04-21T11:02:16Z

apps/files_sharing/lib/external/storage.php

+		$response = $this->propfind($path);
+		if (isset($response['{http://open-collaboration-services.org/ns}share-permissions'])) {
+			$permissions = $response['{http://open-collaboration-services.org/ns}share-permissions'];
+		} else {


Would it make sense to also check for the regular permissions field in the propfind result for older oc remotes

For old federated shares we set the permissions always to 31 and just let the recipient end for example decide to accept a write request or not. That's exactly what I do now in the else block.

icewind1991 · 2016-04-21T11:05:03Z

Do federated share correctly handle the owner losing permissions?

(Fed. reshare an internal share and then the owner of the internal share removing permissions)

schiessle · 2016-04-21T14:22:10Z

Do federated share correctly handle the owner losing permissions?
(Fed. reshare an internal share and then the owner of the internal share removing permissions)

Good question. I just tested it.
Since we now have a flat re-share list this is not really a issue.
If internalUser1 shares a file with internaleUser2 and then internalUser2 re-shares it with remoteUser1, the owner of the remote share will be internalUser1. So he can add/remove permissions for both shares (internal and federated) independent from each other. Same behavior as for internal re-shares.

schiessle · 2016-04-22T11:50:03Z

@icewind1991 I think all comments are addressed... Would be great to get this merged to move forward.

icewind1991 · 2016-04-22T12:41:35Z

👍

lock · 2019-08-06T07:02:24Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

schiessle added 2 - Developing feature:federated-cloud-sharing labels Apr 11, 2016

schiessle added this to the 9.1-current milestone Apr 11, 2016

schiessle force-pushed the cruds-for-federated-shares branch from 51a09d9 to 351d0bd Compare April 12, 2016 10:16

schiessle mentioned this pull request Apr 12, 2016

always return the complete permissions the file was shared with #23945

Merged

schiessle force-pushed the cruds-for-federated-shares branch 6 times, most recently from f0d9fae to 624195d Compare April 14, 2016 16:44

schiessle added 3 - To Review and removed 2 - Developing labels Apr 14, 2016

rullzer reviewed Apr 15, 2016
View reviewed changes

schiessle force-pushed the cruds-for-federated-shares branch 2 times, most recently from a9757f7 to 254f64d Compare April 15, 2016 09:00

schiessle force-pushed the cruds-for-federated-shares branch 2 times, most recently from 1358eef to 29c5af9 Compare April 15, 2016 11:30

Bjoern Schiessle added 3 commits April 18, 2016 12:02

bring back CRUDS permissions for federated shares

4ab02c0

set correct permissions in shareinfo

d1d80b4

return correct share permissions on propfind

52669d0

schiessle force-pushed the cruds-for-federated-shares branch from 29c5af9 to 52644eb Compare April 18, 2016 10:14

rullzer reviewed Apr 18, 2016
View reviewed changes

rullzer mentioned this pull request Apr 19, 2016

Federated sharing: volatile edit permission #24032

Closed

icewind1991 reviewed Apr 19, 2016
View reviewed changes

schiessle force-pushed the cruds-for-federated-shares branch from 87f4e2a to df58cdf Compare April 20, 2016 14:04

schiessle force-pushed the cruds-for-federated-shares branch from df58cdf to fa221ce Compare April 20, 2016 14:51

icewind1991 reviewed Apr 20, 2016
View reviewed changes

Björn Schießle added 3 commits April 20, 2016 17:47

ask for share-permissions on propfind

e906796

if share-permissions exists we use this information as indicator to c…

2edbb7d

…heck if the file has changed or not

always share with same default permissions, no special handling for r…

2a6a336

…emote shares

schiessle force-pushed the cruds-for-federated-shares branch from fa221ce to 2a6a336 Compare April 20, 2016 15:47

This was referenced Apr 20, 2016

Allow setting can edit permissions on federated shares in webUI #24108

Merged

Federated Sharing: Edit permission with/without delete #24128

Closed

icewind1991 reviewed Apr 21, 2016
View reviewed changes

schiessle merged commit 606b756 into master Apr 22, 2016

schiessle deleted the cruds-for-federated-shares branch April 22, 2016 12:50

ckamm mentioned this pull request Jun 22, 2016

[FedShare] Can share permission are not granted for folders by default in case of FedShare owncloud/client#4996

Closed

SamuAlfageme mentioned this pull request Apr 9, 2018

"Public link" share context menu shortcut owncloud/client#6356

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019

		@@ -62,29 +62,37 @@
		$rootInfo = \OC\Files\Filesystem::getFileInfo($path);

bring back CRUDS permissions for federated shares #23918

bring back CRUDS permissions for federated shares #23918

Uh oh!

Conversation

schiessle commented Apr 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Apr 11, 2016

Uh oh!

schiessle commented Apr 12, 2016

Uh oh!

schiessle commented Apr 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rullzer commented Apr 15, 2016

Uh oh!

schiessle commented Apr 15, 2016

Uh oh!

schiessle commented Apr 18, 2016

Uh oh!

rullzer commented Apr 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schiessle commented Apr 18, 2016

Uh oh!

rullzer commented Apr 18, 2016

Uh oh!

schiessle commented Apr 18, 2016

Uh oh!

schiessle commented Apr 18, 2016

Uh oh!

rullzer commented Apr 18, 2016

Uh oh!

schiessle commented Apr 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schiessle commented Apr 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icewind1991 commented Apr 21, 2016

Uh oh!

schiessle commented Apr 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schiessle commented Apr 22, 2016

Uh oh!

icewind1991 commented Apr 22, 2016

Uh oh!

lock bot commented Aug 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

schiessle commented Apr 11, 2016 •

edited

Loading

schiessle commented Apr 21, 2016 •

edited

Loading