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

Transfer ownership cannot increase permissions for file shares with invalid perms #26541

Closed
PVince81 opened this Issue Nov 3, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@PVince81
Member

PVince81 commented Nov 3, 2016

Steps

  1. Setup OC 8.2.8 (or any OC version older than 9)
  2. Create three users "user1" and "user2" and "user3"
  3. Login as "user1"
  4. Create a file "welcome.txt" (if it doesn't exist)
  5. Using the OCS Share API, create a file share with permissions 23 with the user "user2": `curl -u user1:test http://localhost/owncloud/ocs/v1.php/apps/files_sharing/api/v1/shares\?format\=json -X POST --data "path=/welcome.txt&permissions=23&shareType=0&shareWith=user2".
  6. Check "oc_share" and see that the permissions of this share is 23 (not 19)
  7. Update up to at least OC 9.0.5 or later
  8. occ files:transfer-ownership user1 user3

Expected result

Transfer of welcome.txt to user2 succeeded.

Actual result

± % occ files:transfer-ownership user1 user3
Analysing files of user1 ...
    1 [============================]
Collecting all share information for files and folder of user1 ...
    1 [============================]
Transferring files to user3/files/transferred from user1 on 2016-11-03T13:56:18+00:00 ...
Restoring shares ...

                                                                                                               
  [OCP\Share\Exceptions\GenericShareException]                                                                 
  Cannot increase permissions of /user3/files/transferred from user1 on 2016-11-03T13:56:18+00:00/welcome.txt  

Versions

Observed on v9.0.5 and possibly present in later versions.

This situation is possible to create only from OC < 9 because back then the OCS Share API allows to create file shares with permissions > 19 while the max permissions is actually 19 for file shares.
Note that the web UI created such shares properly but shares created with other clients might have sent higher permissions.

Possible fixes:

  • Fix the share managers to always reduce the permissions to 19 for file shares at read time even if the DB contains a higher value. This would make it possible to cover more code paths than just transfer file ownership
    and/or
  • Fix the share manager to not do a check "can user change permissions" when reducing permissions from > 19 to 19 for file shares

@PVince81 PVince81 added this to the 9.0.7 milestone Nov 3, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 3, 2016

Member

@owncloud/qa @DeepDiver1975

@davivel @nasli I remember that there were some misunderstandings about sharing permissions in the mobile apps so it is likely that some older versions allowed setting permissions higher than 19 and create this kind of situatoin.

Member

PVince81 commented Nov 3, 2016

@owncloud/qa @DeepDiver1975

@davivel @nasli I remember that there were some misunderstandings about sharing permissions in the mobile apps so it is likely that some older versions allowed setting permissions higher than 19 and create this kind of situatoin.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 3, 2016

Member

Fix is here #26542

Member

PVince81 commented Nov 3, 2016

Fix is here #26542

@nasli

This comment has been minimized.

Show comment
Hide comment
@nasli

nasli Nov 4, 2016

Member

@PVince81 It was mostly about federated shares, default permissions and allow give share permissions or not.
Max file permission for files I think always has been 19 and 31 for folders

Member

nasli commented Nov 4, 2016

@PVince81 It was mostly about federated shares, default permissions and allow give share permissions or not.
Max file permission for files I think always has been 19 and 31 for folders

@PVince81 PVince81 referenced this issue Nov 7, 2016

Merged

Add repair step to fix file share permissions #26562

5 of 11 tasks complete
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 7, 2016

Member

A better fix here using a repair step: #26562

Member

PVince81 commented Nov 7, 2016

A better fix here using a repair step: #26562

@PVince81 PVince81 self-assigned this Nov 21, 2016

@PVince81 PVince81 closed this Nov 21, 2016

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