-
Notifications
You must be signed in to change notification settings - Fork 29
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 token scope field to gateway authenticate requests #123
Conversation
7ab7db2
to
f3b250f
Compare
This API makes complete sense within the file sync and shares context, which is the scope of the CS3 APIs; however 🤔 I recently started an issue about dealing with roles in a decentralized manner, and was wondering how would this API design choice affect it. On my issue I mention let the IDP define the roles, but IDP's are not subjected to the CS3 APIs. Are this changes the predecessor of a sort of Roles Service? Because a service that enforces this roles would make total sense (this is what we do with the Settings service [Settings is a complete missnomer...]) |
Hi @refs. Yes, going by owncloud/ocis#1971, it serves a similar purpose. Primarily, it is used to mint tokens specifically for cases where we impersonate other users without allowing the full scope of what the actual user has access to. We can discuss how to incorporate both the things in a single design. I wanted to finish coding it so that I could verify its correctness. We can change the design according to what needs we want it to suffice. Here's a diagram detailing how the tokens are minted and additional scopes added https://codimd.web.cern.ch/XTib-1TzTyqx2IZJJOq5pA (you might have to zoom in a bit) |
Also, this serves as a better approach compared to cs3org/reva#1549. Thanks to @labkode for the idea! |
@ishank011 ah:
I see, it makes total sense |
// REQUIRED. | ||
// The resource embedded in the request of a particular method. It depends on | ||
// the method, hence is left as opaque. | ||
cs3.types.v1beta1.OpaqueEntry resource = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that resource
here is an opaque object, not a storage reference. That's because we need to allow a variety of requests, not just limited to the storage provider. Eg, GetPublicShare ref:<token:"XRKWRTefdZPPVVo">
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, a valid concern. Thanks for pointing it out 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes make sense, I think this is good to go. The only piece of criticism I find is the definition of roles at the API level, since they are almost correlated to a "files" app. But as a way to verify a proof of correctness is good to go! 🚀
// REQUIRED. | ||
// The resource embedded in the request of a particular method. It depends on | ||
// the method, hence is left as opaque. | ||
cs3.types.v1beta1.OpaqueEntry resource = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, a valid concern. Thanks for pointing it out 👍
We mint the JWT tokens in Gateway/Authenticate. Adding an optional
token_scope
field would allow us to ensure that access is restricted to only these scopes. To be used in cases when we impersonate other users, including public shares, OCM shares and guest accounts.