[10.3.0 alpha] Owner cannot edit group's permissions

[HanaGemela]

Works with 2.6.0-daily20190820 (build 12341), macOS 10.14.6, server 10.2
Broken on 2.6.0-daily20190820 (build 12341), macOS 10.14.6, server 10.3.0 alpha
Broken on 2.5.4, Windows 10, server 10.3.0 alpha

Steps to recreate:

1. User1 shares a file to User2 with only Share permission
2. User2 shares a file to group friends (User1 and User2 are both members of this group) without any permissions
3. User1 adds Edit permissions to group friends

Actual result: The entry gets greyed out and cannot be edited again
Expected result: User1 can again immediately remove the Edit permissions

[micbar]

@karakayasemi Hana told me that you will investigate.
Please add your Core PR / Ticket to the Release Project that we can track the progress.

[karakayasemi]

The server-side is throwing Cannot increase permission exception. The error message can be seen in web UI with same recreate steps.

When a share owner edits reshare permissions, the maximum permission must be calculated based on the rights of the share owner. In this case, the server is using re-sharer user's permission and, not letting increase.
There is a problem with this condition: https://github.com/owncloud/core/blob/master/lib/private/Share20/Manager.php#L322 . For detecting re-share, instead of the current condition, we can simply compare the session user UID with the share owner UID.

@micbar For this ticket nothing to do in here, we can transfer this ticket to core repo,

[individual-it]

@phil-davis you have done a lot of sharing + permission tests, is that situation covered?

[PVince81]

rating p2 as this is a regression

[karakayasemi]

The PR which brings back the old behavior before share permission refactoring is open in here: #36144 .

On the other hand, I have doubts about this expected behavior. I am not sure from set the permission of the share to a higher value than the permission of the sharer.

[phil-davis]

I need to clarify the steps to recreate. We think it means:

Steps to recreate:

1. User1 shares file.txt with User2 with only Share permission (and they get read permission also, because, for example, clients and the webUI do not really do "only share permission")
2. User2 reshares file.txt to group friends (User1 and User2 are both members of this group) without any permissions (i.e. the group gets read, but not Share permission)
3. User1 can see both their original share to User2, and the reshare by User2 to group friends
4. User1 adds Edit permissions to group friends

Note: actually it does not matter if User1 or User2 are members of group friends - User1 still sees that reshare to group friends because they are the top-level owner of the file/original share.

We do not have sharing permissions tests that cover editing of reshares by the top-level share owner. (and a thousand other combinations of things that can happen when there are reshares of reshares... and permissions get edited at all points in the resharing "tree")

[phil-davis]

Some combinations of permission editing need the requirements to be stated.

1. a higher-level file/share owner can edit the permissions of a lower-level reshare to be less than or equal to the permissions given in the original share. (seems not contentious)
2. a higher-level file/share owner can edit the permissions of a lower-level reshare to be greater than the permissions given in the original share - even though the original share still has the lower permissions. (contentious - IMO they should have to increase the permissions of the original share first, then they can increase the permissions of the reshare by the other user)
3. a higher-level file/share owner can reduce the permissions of the original share and:
   a) the permissions of any reshare will be reduced as necessary so that they do not exceed the new permissions of the original share, or;
   b) the permissions of any reshare will not be changed (and might now be more permissions than currently given in the original share)
4. the higher-level file/share owner can delete the original share, and all reshares under that get deleted. (seems not contentious)
5. the higher-level file/share owner can delete any reshare of the original share. (seems not contentious)
6. The rules apply in a cascading way down the "sharing tree", so if you have received an original share or reshare and reshare it onwards, then you have "control" of your reshare and all reshares underneath in a similar way.
7. Lower-level share receivers cannot change permissions in the sharing-resharing tree above them.
8. Share receivers can reshare sub-folders/files, and the permission changing rules for those "sub-reshares" work in a similar way to when the whole received folder is reshared.
9. When sharing of folders to sub-folders to sub-folders to... goes around in circles then...?

[hodyroff]

2. IMHO this is fine and intented.
3. same here, is intented.
4. is not understandable, please ellaborate. A circle is prevented normally.

[micbar]

@phil-davis
Could you elaborate on 9. too?

[phil-davis]

@phil-davis
Could you elaborate on 9. too?

user1 has folders with sub-folders folder1/folder2/folder3/folder4

If user1 shares folder1 with user2. Then user2 reshares folder1/folder2 with user3, then user3 reshares folder2/folder3 with user4, then user4 reshares folder3/folder4 back to user2. user2 receives back a folder4 (which is actually inside their folder2 from user1). Then user2 reshares that folder4 to user5 and...

If any of those try to reshare back to user1 then there is a message Can't share with the share owner - so that is good. If I just share folder1 around in a circle of user2-user3-user4-user2, then it does prevent making the circle with a message Path already shared with this user

The scenario above seems to work OK. But there will be combinations approaching infinity of weird ways that users can receive their (sub)folders back again. "some" combinations should be in acceptance test scenarios. The challenge is to decide how far to go with it. It certainly needs some test scenarios that generate those messages above.

Maybe point 9 is fine, and just needs more test coverage to be sure.

[phil-davis]

4 is not understandable, please ellaborate

If the share owner user1 has shared f1 to user2 and user2 has reshared f1 to user3 and user3 has reshared f1 to user4 then:

1. If user1 deletes the share, then all the reshares get deleted
2. If user2 deletes their reshare, then the "sub" share from user3 to user4 gets deleted (user3 has lost access to f1 and so has lost the right to reshare f1)

[hodyroff]

4 is not understandable, please ellaborate

If the share owner user1 has shared f1 to user2 and user2 has reshared f1 to user3 and user3 has reshared f1 to user4 then:

1. If `user1` deletes the share, then all the reshares get deleted

2. If `user2` deletes their reshare, then the "sub" share from `user3` to `user4` gets deleted (`user3` has lost access to `f1` and so has lost the right to reshare `f1`)

Yes, that is expected and fine.

[hodyroff]

@phil-davis
Could you elaborate on 9. too?

user1 has folders with sub-folders folder1/folder2/folder3/folder4

If user1 shares folder1 with user2. Then user2 reshares folder1/folder2 with user3, then user3 reshares folder2/folder3 with user4, then user4 reshares folder3/folder4 back to user2. user2 receives back a folder4 (which is actually inside their folder2 from user1). Then user2 reshares that folder4 to user5 and...

If any of those try to reshare back to user1 then there is a message Can't share with the share owner - so that is good. If I just share folder1 around in a circle of user2-user3-user4-user2, then it does prevent making the circle with a message Path already shared with this user

The scenario above seems to work OK. But there will be combinations approaching infinity of weird ways that users can receive their (sub)folders back again. "some" combinations should be in acceptance test scenarios. The challenge is to decide how far to go with it. It certainly needs some test scenarios that generate those messages above.

Maybe point 9 is fine, and just needs more test coverage to be sure.

That is fine, yes.

[micbar]

@phil-davis
See the new PR from @karakayasemi
Looks very promising.

Maybe we could add some more scenarios, now that the requirements are more clear.

[karakayasemi]

There are two open PR that resolves this issue:#36159 and #36166. I couldn't decide which one to continue with.

Lastly, #36159 considered risky in terms of detecting the original node owner. But, in my investigation which is explained in here: #36159 (comment), I could not find any problematic case.

In #36166, adding an optional parameter to an interface method is a little disturbing me. But, it has not any unpredictable result. If we agree to continue with #36166, let's close #36159 and review #36166.

@PVince81, @jvillafanez, @micbar your thoughts can help me to solve this dilemma. Thanks.

[jvillafanez]

My concern with #36159 is that you want to check for the user in the session, but instead, you check for the root folder's owner assuming it will be the same than the session user.
At first sight, it seems you choose to check for the root folder's owner, but you could have chosen any other random method instead of the "straightforward" session user. This will be confusing in the long run.

The problem with #36166 is that we need to change the interface, which I don't think it's a good idea at this stage.

I think the best compromise is to go with #36159 (including a comment with a fixme or todo), and change the interface for the next version, more inline with #36166. Note that we should keep consistency with the rest of the operations in #36166, which is something not done yet.

[karakayasemi]

@jvillafanez I included a comment to #36159 which explains the confusion. Also, I will raise a ticket for necessary interface change, if we merge #36159.

Note that we should keep consistency with the rest of the operations in #36166, which is something not done yet.

Actually, since we just added an optional parameter and initiated it with null in #36166, no other change is necessary. Rest of the operations will continue to work like before. The goal of the change is IMHO very specific use-case: To give the original shareowner a privilege that allows him to act like a normal share on his re-shares.

[micbar]

@jvillafanez @karakayasemi
I talked with @PVince81
We agreed to go with #36166 with some requested changes.