-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: move permissions to shared, a new token proto #192
Changes from 6 commits
6a28c40
6d627ea
7b4ea73
8c26e25
b90a673
74a212b
0d8b5f1
53d1d79
41e206e
c55d389
1276e60
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
syntax = "proto3"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a diff in this file that we should be paying attention to, or is this just an unchanged migration to a separate file There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No diff, unchanged migration to a separate file |
||
|
||
option go_package = "github.com/momentohq/client-sdk-go;client_sdk_go"; | ||
option java_multiple_files = true; | ||
option java_package = "momento.shared.permissions"; | ||
|
||
package permissions; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we can avoid using a package here, that might be sufficient to keep the web sdk working. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that we need to test the web SDK but I don't know that I want to introduce a new inconsistency in our use (or lack thereof) of packages until we know there's a problem with the web SDK. I'd rather be consistent with our current conventions and see if we can solve it for Web SDK. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. The hack will almost definitely break though, because even if we remove |
||
|
||
// Aliases for categories of functionality. | ||
enum CacheRole { | ||
CachePermitNone = 0; | ||
// Restricts access to apis that read and write data from caches: No higher level resource description or modification. | ||
CacheReadWrite = 1; | ||
// Restricts access to apis that read from caches: No higher level resource description or modification. | ||
CacheReadOnly = 2; | ||
// Doesn't allow conditional write APIs (SetIfNotExists, IncreaseTTL etc) | ||
CacheWriteOnly = 3; | ||
} | ||
|
||
// Aliases for categories of functionality. | ||
enum TopicRole { | ||
TopicPermitNone = 0; | ||
// Restricts access to apis that read and write data from topics: No higher level resource description or modification. | ||
TopicReadWrite = 1; | ||
// Restricts access to apis that read from topics: No higher level resource description or modification. | ||
TopicReadOnly = 2; | ||
// Only publish allowed | ||
ben-kugler marked this conversation as resolved.
Show resolved
Hide resolved
|
||
TopicWriteOnly = 3; | ||
} | ||
|
||
enum SuperUserPermissions { | ||
SuperUser = 0; | ||
} | ||
|
||
message Permissions { | ||
oneof kind { | ||
SuperUserPermissions super_user = 1; | ||
ExplicitPermissions explicit = 2; | ||
} | ||
} | ||
|
||
message ExplicitPermissions { | ||
repeated PermissionsType permissions = 1; | ||
} | ||
|
||
message PermissionsType { | ||
oneof kind { | ||
CachePermissions cache_permissions = 1; | ||
TopicPermissions topic_permissions = 2; | ||
} | ||
|
||
message All {} | ||
|
||
message CacheSelector { | ||
oneof kind { | ||
string cache_name = 1; | ||
} | ||
} | ||
|
||
message CacheItemSelector { | ||
oneof kind { | ||
bytes key = 1; | ||
bytes key_prefix = 2; | ||
} | ||
} | ||
|
||
message CachePermissions { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These message definitions are shared between regular v1 auth and acorn, however we don't actually want to support CacheItemSelector for v1 auth, right? Also, do we want to allow an acorn to be used for topics? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For now, yes, the control-plane will reject generate v1 token request that include |
||
CacheRole role = 1; | ||
oneof cache { | ||
All all_caches = 2; | ||
CacheSelector cache_selector = 3; | ||
} | ||
oneof cache_item { | ||
All all_items = 4; | ||
CacheItemSelector item_selector = 5; | ||
} | ||
} | ||
|
||
message TopicSelector { | ||
oneof kind { | ||
string topic_name = 1; | ||
} | ||
} | ||
|
||
message TopicPermissions { | ||
TopicRole role = 1; | ||
oneof cache { | ||
All all_caches = 2; | ||
CacheSelector cache_selector = 3; | ||
} | ||
oneof topic { | ||
All all_topics = 4; | ||
TopicSelector topic_selector = 5; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,35 @@ | ||||||
syntax = "proto3"; | ||||||
|
||||||
import "permissions.proto"; | ||||||
|
||||||
option go_package = "github.com/momentohq/client-sdk-go;client_sdk_go"; | ||||||
option java_multiple_files = true; | ||||||
option java_package = "momento.auth"; | ||||||
|
||||||
package token; | ||||||
ben-kugler marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
service Token { | ||||||
rpc GenerateTempAuthToken (_GenerateTempAuthTokenRequest) returns (stream _GenerateTempAuthTokenResponse) {} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'll clean these up once name is settled There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think we aligned in slack that we are not going to switch to |
||||||
} | ||||||
|
||||||
message _GenerateTempAuthTokenRequest { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This still needs to be finalized, alternative suggestions
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For better or worse, we have landed on |
||||||
// generate a token that has an expiry | ||||||
ben-kugler marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
message Expires { | ||||||
// how many seconds do you want the api token to be valid for? | ||||||
uint32 valid_for_seconds = 1; | ||||||
} | ||||||
|
||||||
Expires expires = 1; | ||||||
|
||||||
permissions.Permissions permissions = 2; | ||||||
} | ||||||
|
||||||
message _GenerateTempAuthTokenResponse { | ||||||
// the new api key used for authentication against Momento backend | ||||||
string api_key = 1; | ||||||
// the Momento endpoint that this token is allowed to make requests against | ||||||
string endpoint = 2; | ||||||
// epoch seconds when the api token expires | ||||||
uint64 valid_until = 3; | ||||||
|
||||||
} |
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.
Can you try generating the protos for the web sdk and compiling the web sdk with them? we used some hacks there to get it to work with strictjs and I am worried that moving these messages to different packages is going to break it.
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.
@cprice404 FYI ^
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.
good callout, definitely worth testing.
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.
Got a successful build without
package permissions
, did have to change imports, here's thejavascript-web
changesmomentohq/client-sdk-javascript#729
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.
Okay, this was a real pain. For
Javascript-web
, I removed thepackage
declarations, but in doing so, rust builds will fail since tonic server seems to build protos into the out directory only if they're a package, couldn't find anything in the docs to help with this.Noticing that the
javascript-web
build script actually comments outpackage
(thank you @krispraws for the great comments), I thought I would keep the package declares and updatepermissions.Permissions
toPermissions
forjavascript-web
only. It ain't pretty.Got it working and auth test running in
javascript-web
,javascript-nodejs
I was having much worse luck with,@grpc/grpc-js
is shared between the packages, soChannelCredentials, Interceptor
had conflicts between the two different versions.Here's what the JavaScript changes will look like momentohq/client-sdk-javascript#731
Alternatively to avoid all of this, we could maintain two copies of the permissions object in
auth
andtoken
, but not a hug fan of this.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.
the javascript-web build is already a rube goldberg machine. Personally I am okay with pushing more horror and complexity there rather than letting it bleed into other languages/SDKs. If @krispraws is.
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.
dumb question - Do we need to put each proto file/each grpc service in its own package? Can tokens.proto, permissions.proto and auth.proto all live in the same
auth
package?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.
I agree with @cprice404 - we can push the complexity into javascript-web builds.