validating rereshare permission when creating share using API make sure to check most permissive mountpoint

[mrow4a]

Description

0. there is a file simple-folder/simple-inner-folder/test.txt (user test1)
1. user test1 shares simple-folder with permission all to test2
2. user test2 reshares simple-folder with permission all to grp1 (has test1,test2,test3 users)
3. user test3 reshares simple-folder/simple-inner-folder with permission read to grp2 (has test1,test2,test3 users)
4. user test2 tries to reshare simple-folder/simple-inner-folder/test.txt with test4 and fails, besides it is reshare from folder with permissions all!!

What happens:

1. user receives folder simple-folder from reshare of simple-folder and grp1, and can find test.txt in simple-folder/simple-inner-folder/test.txt
2. user receives folder simple-inner-folder from reshare of simple-folder/simple-inner-folder with lower permission, and can additionally now find test.txt in /simple-inner-folder/test.txt (but with lower permission)
3. mount logic works correctly, thus all the permissions are restricted for respective mountpoints (see screenshots below)
4. However there is bug in createShare() function of Manager while validating permissions. This is due to the fact that OCS API /share is not aware of from which "folder/subfolder" is share reshared, and thus "suspected" mountpoint is returned as first to validate permissions
5. we should check permissions of "parent mountpoint" that is original share, and not some subfolder reshares that by design contain lower permissions. User CAN create share for that NODE in his user folder with permissions up to max permission that is allowed

fixes: https://github.com/owncloud/enterprise/issues/4497

How Has This Been Tested?

Integration Tests, Manually

Screenshots (if appropriate):

BACKGROUND:

BEFORE:

AFTER:

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Acceptance tests added
• Changelog item, see TEMPLATE

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
No Duplication information

[jvillafanez]

I think I'd vote for a won't fix without major changes.

For me, each share has its own permissions, which are independent of each other share. At least, I think it was designed that way. This means that accessing the file via "simple-folder/simple-inner-folder/test.txt" has a different set of permissions than accessing via "simple-inner-folder/test.txt", and as such, you can do different things depending where you come from.
Evaluating other share permissions would break this assumption of independence. It doesn't matter whether you check the parent or the children.

If we're going to change things, I think the current approach could make sense. Evaluating the inner permissions usually takes precedence over the parent permissions. I guess this is what is happening, which could be fine. The problem is that we still think that, since we're accessing through the parent share, it's the parent share's permissions the ones that should be applied, which is wrong.
In this case, I think it's a visualization problem what we should solve. Merging both shares into one and show the right permissions should help in this case. Basically, the user would have only the "simple-folder/simple-inner-folder/test.txt" path, "simple-folder" permissions could be A (according to the share), "simple-folder/simple-inner-folder" permissions could be B (according to the inner share) and "simple-folder/simpler-inner-folder/test.txt" permissions would also be B (according to the inner share).
There is no great change in the behaviour since this is what we're doing now (apparently)

Evaluating the parent permissions seems unnatural.
You're thinking about the parent share having greater permissions than the inner share, and as such you should be able to use those permissions over the inner share. However, this will also be confusing when the parent share has lower permissions because you won't be able to perform action from the inner share even though it has greater permissions.
The other option is to evaluate all the permissions and keep the more open ones. However, this will be a problem if what we're looking for is to reduce the permissions.

---

For me there are 2 solutions:

• Provide a way to know where you're accessing the share from. This should help with the share independence outlined above, but it seems a pain both to implement and to use.
• Show only the parent share and adjust the permissions of the contents based on any inner share that could be inside.

As said, for me the second option seems more natural, so we could evaluate the changes we need to do in order to decide if it's worthy or not.

[mrow4a]

@jvillafanez the problem is in validation permissions, not in mount creation logic! What you mentioned is solved as part of shared mount logic. Here the problem is that where this code is executed you have no idea from which exactly folder you share. You only know ORIGINAL node id, share-by and share-owner. Thus idea is to find most permissive while validating permissions.

[jvillafanez]

I guess I'll ping @pmaier1 to decide. For me, the PR brings a change in the behaviour that needs to be discussed because it will make impossible to restrict permissions.

Case 1:

1. "some-folder" shared with groupA with full permissions
2. "some-folder/inner-folder" shared with user1 (also member of groupA) with just read permissions

What are the permissions that user1 should have over "some-folder/inner-folder/test.txt"? Note that user1 can access to the file via "some-folder/inner-folder/test.txt" and "inner-folder/test.txt"

• Full permissions due to the share on "some-folder"
• Only read permissions due to the share on "some-folder/inner-folder"
• Full permissions due to the least restrictive of both shares
• Depends on where the users access from.

---

Case 2:

1. "some-folder" shared with groupA with read permissions
2. "some-folder/inner-folder" shared with user1 (also member of groupA) with full permissions

What are the permissions that user1 should have over "some-folder/inner-folder/test.txt"? Note that user1 can access to the file via "some-folder/inner-folder/test.txt" and "inner-folder/test.txt"

• Only read permissions due to the share on "some-folder"
• Full permissions due to the share on "some-folder/inner-folder"
• Full permissions due to the least restrictive of both shares
• Depends on where the users access from.

---

The problem of choosing the least restrictive option is that we won't be able to restrict permissions, which I think is a valid use case.

My point here is that the current behaviour the code has is the second option for both cases (correct me if I'm wrong), but the user expects the fourth option because they have 2 different shares. For me, the problem is this mismatch between the user's expectations and what the code is doing, so for me it's ok to treat the problem as a visualization issue and change the user's expectations (by showing only one share) but not the code behaviour.

Here the problem is that where this code is executed you have no idea from which exactly folder you share. You only know ORIGINAL node id, share-by and share-owner. Thus idea is to find most permissive while validating permissions.

Yes, I know. That's why I'm saying that showing 2 different shares there is obsolete and doesn't match the current behaviour. Bringing that back properly isn't possible without major changes because we have to know the path the user is using to access to the share, which isn't possible at the moment.

[mrow4a]

@jvillafanez what about screenshare session ? The problem is really that permissions are validated WHEN CREATING SHARE for wrong mountpoint for THE SAME FILE (node), that just got reshared via e.g. subfolder again. In screenshare I could demo a problem looking with debug in codelines. There is no permission escalation, and UI looks better. Note you cannot reproduce with current implementation UI behaviour translatong to API (up to my understanding)

[pmaier1]

If I'm not mistaken, my expectation is aligned with what @jvillafanez points out in here. Correct me if I'm wrong but we never "merged" shares or permissions in the past. Each share is independent in terms of it's mount point and permissions.

While I see benefits of merging shares and permissions, I see this as a major change that we should not make at this time in oC 10. Also, I don't see the relation to the original support issue (resharing a received share with a group).

@micbar what do you think?

[jvillafanez]

Correct me if I'm wrong but we never "merged" shares or permissions in the past. Each share is independent in terms of it's mount point and permissions.

I guess that was the idea, but it isn't working like that.
Assuming you have full permissions on "some-folder" and read permissions on "some-folder/inner-folder", what you have is that "some-folder/inner-folder/test.txt" has read permission even though you're accessing through the "some-folder" share. This means that those shares aren't independent.

Bringing back true share independence would require major changes because we don't know where the user is accessing to the share from, and this is a requirement to fix the problem this way.
Since this will require big changes, we're looking for alternatives with smaller code changes:

• Merging the shares (in this case) wouldn't change the current behaviour, but it's true that the user might be confused due to the change in the expectations. This would need to be evaluated since the amount of changes is unknown
• Choosing the least restrictive permissions will change the behaviour. It also makes impossible to reduce the permissions

To clarify a bit my position, I'm for keeping the current behaviour and merge both shares into one. Maybe the easiest option would be to skip the inner share from showing up, this way the user would only see the share on "some-folder" but the permissions would be based on the containing share, which is what we have now.

[mrow4a]

@jvillafanez @pmaier1 updated description with screenshots and clearer explanation of the issue.

[jvillafanez]

I'm not sure if this is a different problem or not...

1. Create user1, user2 and user3, all of them members of group1, group2 and group3
2. user1 create "/folder1/folder12/folder13/test.txt"
3. user1 shares "/folder1" with group1 with all permissions
4. user1 shares "/folder1/folder12/" with group2 with no permissions -> just read-only
5. user1 shares "/folder1/folder12/folder13" with group3 with "can share", "can edit" and "delete" permission
6. user2 tries to share "/folder1/folder12/folder13/test.txt" with user00 (random user not 1, 2 or 3) -> only "can share", "can edit" and "change" options are available (?)
7. user2 tries to share "/folder12/folder13/test.txt" with user00, but it isn't allowed (this is ok)
8. user2 tries to share "/folder13/test.txt" with user00 -> only "can share" option is available (?)

@mrow4a shouldn't steps 6 and 8 show more permissions? I'm not sure if this is a problem in the web UI or in the server... It's possible for user2 to change the permissions shown.

[mrow4a]

@jvillafanez the issue that I try to fix is related to failure while createShare. What you describe are problems with share mount creation, completely not related to this issue. Still valid point and we probably should target it, as it could cause similar problem to the one described here.

[jvillafanez]

return \strcmp($first->getPath(), $second->getPath()) ? Might need a comment, but ok

[jvillafanez]

Still valid point and we probably should target it, as it could cause similar problem to the one described here.

Should we fix it here or in another PR?

[mrow4a]

@jvillafanez another PR. The issue you found is is mounting logic, not in share creation while validating permissions.

[jvillafanez]

Check if #38625 (review) is worthy. Other than that, I think this is good enough.