owncloud / core

:cloud: ownCloud web server core (Files, DAV, etc.)
https://owncloud.com
GNU Affero General Public License v3.0
8.33k stars 2.06k forks source link

resource based share management has broken corner cases #39347

Open butonic opened 2 years ago

butonic commented 2 years ago

Expected:

Background: 
  Given the admin has disabled auto accepting of shares

Scenario: accepting a new share on a resource that has already been mounted by accepting another share for the same resource
Given user demo shares 'foo' with group 'admin' read only
And user admin accepted 'foo'
And user admin renamed `/foo` to `/foo from demo`
When user demo shares 'foo' with user 'admin' read write
Then user admin should have write permission to `/foo from demo`
And user admin should have accepted the new share automatically

Actual:

After demo creates the second share User admin has to accept the new share before he can write to /foo from demo

Furthermore, when he accepts the new share, the mount point is renamed to /foo (/foo from demo no longer exists)

Finally, if admin declines the new share the original share is declined as well and /foo vanishes.

Explanation

There is a mixup between resource based and share based share management: when demo creates the two shares before admin accepts either of them, the accept action actually accepts both shares. The concept here is resource based sharing. That is the reason why the 'list shares with me' UI only shows a single entry (in oc10 classic). AFAICT ocis web not merging two shares for the same resource is a bug.

When the user has accepted (aka mounted) a resource any subsequent shares on the same resource for the same recipient should be effective immediately. No additional accept needed, no additional mounting needed.

In contrast to a resource based share management share based share management would allow creating two distinct mount points for the two shares. One would be read only, the other would be read write. They would have different names and the desktop client would duplicate the resources. This is undesired, as it causes confusion for end users, which is why the classic ui merges all shares for the same resource into one.

AFAICT we need to fix the above corner cases to properly have resource based share management in ownCloud 10 and OCIS.

butonic commented 2 years ago

cc @micbar @aduffeck @pmaier1 This is what we are seeing. I hope the steps are clear. For OCIS we agreed to implement this properly, as in auto accepting subsequent shares on the same resource for the same recipient.

@phil-davis this is related to https://github.com/owncloud/core/issues/38851 AFAICT

phil-davis commented 2 years ago

It seems strange the Alice would share folder with Brian read-only, then make another read-write share of folder with Brian.

IMO Alice would edit the existing share to Brian and add write permission.

For each resource that Alice has, she should only be able to share the resource once to each sharee (user or group).

But maybe that restriction causes problems if Alice wants to give the extra write permission to Brian only for a specified time-period. In that case, Alice shares the resource read-only to Brian with no expiration. Then Alice also shares the resource read-write to Brian with an expiration date. Brian should see a single view of the resource, and lose write access after the expiration date. I Anyway, Alice could share read-only to Brian, and read-write to "Group1", at different times. If Brian is a member of "Group1", and Brian has already accepted the first read-only share from Alice, then "something reasonable" has to happen when Brian "suddenly" gains an extra way of seeing the resources from Alice. "something reasonable" is probably that it "just happens" and Brian does not have to accept the "new share", and it works even if Brian had renamed the original share.

phil-davis commented 2 years ago

The test expectations have been adjusted a bit in #39917