Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allign WebDAV's <oc:permissions> with ocs permissions on PROPFIND #20426

Closed
ckamm opened this issue Nov 10, 2015 · 20 comments
Closed

Allign WebDAV's <oc:permissions> with ocs permissions on PROPFIND #20426

ckamm opened this issue Nov 10, 2015 · 20 comments

Comments

@ckamm
Copy link

ckamm commented Nov 10, 2015

With 8.2.0:

  • Have two accounts A, B
  • Share a file <A>/file to <B>/file with all permissions
  • Look at the permissions returned for /file in PROPFIND of <B>/ and <B>/file. Both should be SRDNVW.
  • Remove the edit permissions from the share.
  • Look at the permissions again. The PROPFIND for <B>/file returns SRDNV (correct), but the <B>/file entry in the directory propfind continues to say SRDNVW.

This has the practical implication that the desktop sync client will not have up to date information about the file permissions and will behave incorrectly. owncloud/client#3244

@DeepDiver1975 @PVince81

@rullzer
Copy link
Contributor

rullzer commented Nov 17, 2015

Ah permission mess :)
The trouble is that we here try to map two different permission sets in 1 field. On the one hand we have the share permissions and on the other hand we have the oc filesystem permissions.

If you propfind on the root it will see hey there is a mountpoint /file. And it will retrieve the permissions correctly. However because it is a mountpoint in / it will say. Well it is a share so the receiver can always rename (move mountpoint) or delete the file (unshare).

Now if you do a propfind directly on /file it won't do all this mountpoint magic (since you profind just a file). And thus return the correct permissions.

So actually this is 2 bugs.

  1. Different permissions
  2. No way to get the proper share permissions

@PVince81
Copy link
Contributor

From my understanding the sync client only needs the permissions to find out whether or not to attempt a server call. If the permissions are too permissive, it isn't that bad if the sync client needs to make a server call to see it fail afterwards. So it's just a way to save server calls. Correct me if I'm wrong @ckamm 😄

With that assumption, we could make it so that the "PROPFIND /file" returns the same permissions as the sub-result from "PROPFIND /".

@rullzer
Copy link
Contributor

rullzer commented Nov 18, 2015

@PVince81 not exactly. The clients wants to mark files that it can't edit as read only. So the uers will get notified on their system that they can't save the file. Right now they can edit the file just fine. Save it. And then the client will tell them there is a problem when uploading.

@PVince81
Copy link
Contributor

@icewind1991 can you comment on this ? Was this behavior originally on purpose ?

@PVince81
Copy link
Contributor

This also means that when we make the web UI use Webdav we might encounter similar issues.

@PVince81
Copy link
Contributor

Looks like this behavior exists since OC 7 already.

@PVince81
Copy link
Contributor

I see some perms manipulation here in getDirectoryContents(): https://github.com/owncloud/core/blob/v8.2.0/lib/private/files/view.php#L1362
That's used by PROPFIND with Depth: 1.

@rullzer
Copy link
Contributor

rullzer commented Nov 18, 2015

I see some perms manipulation here in getDirectoryContents(): https://github.com/owncloud/core/blob/master/lib/private/files/view.php#L1370
That's used by PROPFIND with Depth: 1.

YES!. When I ran with XDebug yesterday that was the point. Permissions are fine till that point.
We add those there because it is a mount point (moveable mount point). So we can always move and delete it.

@icewind1991
Copy link
Contributor

Then we should also add those permissions to the storage root when doing getFileInfo

@MorrisJobke
Copy link
Contributor

@rullzer @icewind1991 Can we fix this soon? Is there a PR ready to review? Can we backport this then afterwards? Thanks

@MorrisJobke
Copy link
Contributor

@rullzer @icewind1991 Can we fix this soon? Is there a PR ready to review? Can we backport this then afterwards? Thanks

And keep in mind, that the instance this was noticed is on 7.0.x. Could there also be some fixes in the more recent versions involved?

@PVince81
Copy link
Contributor

I'm not aware of any changes in that part.

Note that the fix suggested by @icewind1991 might also influence the display of permissions in the web UI, so might need to properly retest that part.

@rullzer
Copy link
Contributor

rullzer commented Nov 23, 2015

As far as I know this has been broken for a long long time. (Also when looking trought he git log).

What I think would be "best". Is have the filed return a consitent group of permissions (from the fs point of view).

So the permissions should be the same when looking at a file as when looking at the parent folder.

Then we add a new field that can indicate if it is read only. Altough I have no idea how we can properly backport a fix here. Since one way or the other we change behaviour pretty drasticly...

@PVince81
Copy link
Contributor

@rullzer how about the quickfix of putting the same permission changing logic in getFileInfo as suggested by @icewind1991 ? At least it would return consistent results.

@PVince81
Copy link
Contributor

Potential fix here to make the permissions more consistent: #20685

@MorrisJobke
Copy link
Contributor

The original blue issue was solved by an upgrade to 7.0.11

@PVince81 PVince81 removed this from the 9.1.3 milestone Nov 30, 2016
@PVince81 PVince81 modified the milestones: 9.1.5, 9.1.4 Feb 6, 2017
@PVince81 PVince81 modified the milestones: 9.1.6, 9.1.5 Apr 13, 2017
@PVince81 PVince81 modified the milestones: 9.1.7, 9.1.6 May 29, 2017
@PVince81 PVince81 modified the milestones: triage, 9.1.7 Jun 29, 2017
@ownclouders
Copy link
Contributor

Hey, this issue has been closed because the label status/STALE is set and there were no updates for 7 days. Feel free to reopen this issue if you deem it appropriate.

(This is an automated comment from GitMate.io.)

@SamuAlfageme
Copy link

See for reference & discussion owncloud/client#6346 (comment)

The oc:permissions of a "shared with me; cannot edit but can reshare" :

<oc:permissions>SRDNVW</oc:permissions>

... this is returned in both WebDAV endpoints (old & new). Based on client mappings:

https://github.com/owncloud/client/blob/cedf72825bc0db8e3c147958a9382ee5b411d9ea/src/common/remotepermissions.h#L42-L54

This is not consistent with the semantics of "Shared with me; can reshare but nothing else".

For everything to be sound with the files backend behavior, the permissions should be:

<oc:permissions>SRDV</oc:permissions>

... since Rename and Delete operations are also allowed for this file. And to be aligned with the response of of http://localhost/ocs/v2.php/apps/files_sharing/api/v1/shares?format=json&path=/test&shared_with_me=true:

{
   "ocs":{
      "meta":{
         "status":"ok",
         "statuscode":200,
         "message":null
      },
      "data":[
         {
            "id":"2",
            "share_type":0,
            "uid_owner":"demo",
            "displayname_owner":"demo",
            "permissions":17,

@SamuAlfageme SamuAlfageme changed the title Stale shared-file permissions returned on directory PROPFIND Allign WebDAV's <oc:permissions> with ocs permissions on PROPFIND Feb 6, 2018
@PVince81 PVince81 modified the milestones: triage, maybe some day May 22, 2018
@PVince81 PVince81 removed their assignment Oct 15, 2018
@stale
Copy link

stale bot commented Sep 21, 2021

This issue has been automatically closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants