Resharing edgecases cause unaccessible shares

[micbar]

Describe the bug

A clear and concise description of what the bug is.

Steps to reproduce

Steps to reproduce the behavior:

Resharing a file (or folder) with different permission can lead to unaccessible shares. Follow the steps to reproduce

As admin share a file to einstein with viewer permissions
As admin share same file to marie with editor permissions
As marie reshare the file with einstein with editor permissions
--> an error message will appear saying grpc error
--> the share will still be created
As einstein go to shared withme page and accept the share
--> Note AE avatar appears twice in Shared with column
As einstein reshare the file to katherine with editor permission
--> an error message will appear saying grpc error
--> the share will NOT be created
As admin check the shares on the file
--> It contains two shares to einstein as viewer and editor
As admin delete the viewer share to einstein
--> einstein cannot access the file any more, the editor share is not considered

Expected behavior

• Proper error handling without breakin the user experiance
• Shares should not be unaccessible

Actual behavior

A clear and concise description of what happened.

Setup

Please describe how you started the server and provide a list of relevant environment variables or configuration files.

OCIS_XXX=somevalue
OCIS_YYY=somevalue
PROXY_XXX=somevalue

Additional context

Add any other context about the problem here.

[butonic]

What should conceptionally happen?
While the grants on the storage can just be updated the share manager needs to be clarified:

• When the admin creates two shares he expects to see them in his shared with others list
• When marie updates a share she can do that by
  • navigating to the shared with me page in web
  • clicking the actions button and choosing the 'share' action
  • the sidebar opens and she does not see that the file is already shared with einstein (because she is not a space manager?)
  • she can try to share with einstein
  • but she will get the grpc error

The reason currently is that we send a CreateShare request because marie is not aware of the existing share in the share manager. We have an explicit check if a share already exists.

The real question is what should happen in the web ui? Why does Marie not see the existing share for einstein (works as designed because she is not a space member)? Should she, as she has the sharing permission set?

We could keep two shares for the same resource in the share manager. but then we need to recalculate the grant that is set on the storage, whenever a share is deleted. Which in effect then makes the grants on the storage only a projection of all shares in the share manager.

But again the more fundamental question is what is expected to happen from the end users perspective? @tbsbdr @micbar

[micbar]

My solution would be

As admin share a file to einstein with viewer permissions
As admin share same file to marie with editor permissions
As marie reshare the file with einstein with editor permissions
--> an error message will appear saying grpc error
--> the share will still be created

The reshare from marie back to the owner or anybody else which already has the share should be forbidden with a clear error message.

[micbar]

@pmaier1 ^

[pmaier1]

The reshare from marie back to the owner or anybody else which already has the share should be forbidden with a clear error message.

Agreed, that would also be my expectation.

[ScharfViktor]

The reshare from marie back to the owner or anybody else which already has the share should be forbidden with a clear error message.

why we prohibit maria to reshare with editor permission? She has rights do it and previosly she could do it. and even if we forbid her to do so, she can re-share to "sailing-lovers" group (einstein is member) and up einsteins permissions

[phil-davis]

Sharing with individual users can be restricted like this - we can detect if a particular user is the original "owner" of the share (the share comes from that user's personal space) or if that particular user already has the proposed access anyway. But groups are more of a challenge for trying to stop "circular resharing". The group to be shared with can have many members, some of which may already have some access to the resources. Then what do we do? Allow the reshare, or not?

[butonic]

The real question is what should happen in the web ui? Why does Marie not see the existing share for einstein (works as designed because she is not a space member)? Should she, as she has the sharing permission set?

[micbar]

The real question is what should happen in the web ui? Why does Marie not see the existing share for einstein (works as designed because she is not a space member)? Should she, as she has the sharing permission set?

That has always been the oc sharing policy that only the share owner/creator/receiver can see the share. A share received should not see other shares on the same resource where he is not the receiver.

Group sharing complicates that.

@kobergj what was the quick fix on the experimental branch?

[kobergj]

I did mix up things a little (too many master branches).
So: We have done a quick fix, but not on experimental it was going directly to edge. Here is the corresponding PR: cs3org/reva#3176

The solution was to simply forbid creating shares to users that already have a share on this resource. That is the current behavior of ocis.

Group shares were not considered.

[butonic]

Some technical background: when navigating through the tree and selecting a single resource the share manager will only return the shares on that resource that you created or received. When you are a manager of the space the resource is contained in yo can see all shares in that space.

So far, so clean.

So, as a share receiver you will not see shares of other users, leading to this resharing problem: you cannot see other users shares and it is still not clear what should happen when - as a share recipient with oc10 reshare permission - you create a share for a user that already has a share (but you are unaware of it). Several options:

1. you just get an error saying the user already has a share. Currently, only the storageprovider does this as you can only have one ACL per user and group.

• make the share manager return an already exists error
• what should the ui do? AFAIK it cannot list existing shares and is not supposed to by product requirements, which I herby challenge. It is utterly confusing for end users and when they try to create a share the can still find out if a user has access. Furthermore, what about group shares? You want to be able to create a readonly group share and the add some write shares for specific users.

2. we try to aggregate permissions on the storage and change the ACL.

• when the existing ACL contains all permissions the user wants to grant nothing needs to be done. Unless the resharing user tries to remove permissions ... he is not aware of ...
• when the new grant adds more permissions, we could use the new permissions.
• so we always write the grant with more permissions? what if some permissions are taken away and others are added? can resharing only add grants?
• a resharer would then not be able to remove permissions.
• well the share manager could calculate the new grant because it is aware of other shares for the user and then use UpdateGrant

In any case, what does a clear error message look like? Albert already has access to the file/folder you are trying to share? That is btw a breaking change between oc10 and ocis. In oc10 you can just create multiple shares and it will logically AND them as the permissions on the resource.

For ocis/reva it would mean the share manager calculates the grant that is persisted on the storage.

• add a AggregatedGrant property to the CreateShareResponse so the gateway can update the grant accordingly

[butonic]

After lunch, I'll go with preventing the reshare in the share manager if a share already exists ... less sucky approach for now

[kobergj]

@butonic We previously blocked it in ocs service. The idea was that share manager shouldn't care about such logic. It is also a valid option to aggregate shares (as you already stated). Therefore blocking it in ocs service is imo cleaner.

[butonic]

hm that makes sense...

[butonic]

@kobergj following the steps above on edge ocs does not even see the share in the ListShares call. When the GranteeEqual comparison iterates over the shares in https://github.com/cs3org/reva/blob/7e300c41608401c96515cd317a371941c688ff58/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go#L1482-L1486 it does not get a chance to stop the request.

That is by design of the jsoncs3 implementation, because marie neither created the share nor is she a member of the space.

I'll see if I can make the jsoncs3 driver return an appropriate error ...

[kobergj]

I see. Looks like we were working in different directions. Before we were trying to keep share manager free from business logic, hence it would return all shares on ocs service filters them by need.
If we switch direction we need to replicate the logic in all share managers.

[butonic]

@micbar I cannot reproduce the last step in

As admin share a file to einstein with viewer permissions
As admin share same file to marie with editor permissions
As marie reshare the file with einstein with editor permissions
--> an error message will appear saying grpc error
--> the share will still be created

In cs3org/reva#3287 I made OCS return a forbidden error, but the logic in the gateway already prevents the creation of a storage grant. And the jsoncs3 driver already returns an already exists error ...

Are you using cs3 or jsoncs3?