Skip to content
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

add grant methods to the gateway #165

Closed
wants to merge 1 commit into from

Conversation

micbar
Copy link
Member

@micbar micbar commented Feb 27, 2022

Description

The methods to work with Grants are currently only available directly on the storageprovider.

This PR adds them to the Gateway too.

Background

We are using a stat cache in the gateway which also caches the grants. Currently it it completely unaware when something regarding the grants changes because these methods are currently called directly on the storage providers bypassing the gateway.

Implementation

I aim to implement this on master and edge branch because the change is relatively contained.

@micbar micbar requested a review from labkode as a code owner February 27, 2022 20:24
@micbar
Copy link
Member Author

micbar commented Feb 27, 2022

@kobergj @butonic @dragotin

@butonic
Copy link
Contributor

butonic commented Feb 28, 2022

The idea of Grants is that they are internal representations of permissions on storage providers. If and how they are created is an implementetion detail of the Share manager: it persist the share intent, independently of the storage providers / drivers. The gatewaycan be configured to forward sharing requests as Grants to the storage providers to give them a chance to also represent the share in whatever way is suitable, eg. ACLs.

@labkode correct me if the above is a misrepresentation from my side.

In cs3org/reva#2250 @C0rby had to bypass the gateway to add members to spaces. I remember having a discussion with him about how to proceed, but I don't remember the reasons. Maybe he can chime in.

I am saying that because I don't like to expose two sets of apis (Shares & Grants) that seemingly are intended for the same thing: managing access to resoucres. This would be confusing.

@labkode
Copy link
Member

labkode commented Feb 28, 2022

The idea of Grants is that they are internal representations of permissions on storage providers. If and how they are created is an implementetion detail of the Share manager: it persist the share intent, independently of the storage providers / drivers. The gatewaycan be configured to forward sharing requests as Grants to the storage providers to give them a chance to also represent the share in whatever way is suitable, eg. ACLs.

@labkode correct me if the above is a misrepresentation from my side.

You're right! The Sharing API should be the one exposed to clients to define share behaviour.
Exposing the Grants directly does not bring any benefit as this API is intended to be internal only.

@micbar
Copy link
Member Author

micbar commented Feb 28, 2022

IMO that does't make any sense. Bypassing the gateway makes it even worse.

@micbar
Copy link
Member Author

micbar commented Feb 28, 2022

We need a solution for this. And a pragmatic one. Because currently we expose the grants as Permissions to the clients.
Therefore we are caching them in the gateway. But the gateway is completely unaware of permission changes. That is because we are "hiding" these operations internally.

@micbar
Copy link
Member Author

micbar commented Feb 28, 2022

Ok, i thought you were talking about the OCS Shares HTTP API.

I was not aware of the CS3 Shares Api.

This fits better.

@micbar micbar closed this Feb 28, 2022
@butonic
Copy link
Contributor

butonic commented Feb 28, 2022

@micbar @C0rby I think it makes more sense to add a list of managers to the StorageSpace message: https://cs3org.github.io/cs3apis/#cs3.storage.provider.v1beta1.StorageSpace

That would allow using UpdateSpace to change the managers.

@C0rby
Copy link
Contributor

C0rby commented Feb 28, 2022

What about a list of Grants to represent space members. Then the roles of the members are not defined by the list and we could define more roles in the future based on the permissions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants