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

feat: move permissions to shared, a new token proto #192

Merged
merged 11 commits into from
Aug 18, 2023
Binary file not shown.
93 changes: 3 additions & 90 deletions proto/auth.proto
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
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";
Expand Down Expand Up @@ -69,98 +71,9 @@ message _GenerateApiTokenRequest {
Expires expires = 2;
}

// 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
TopicWriteOnly = 3;
}

string auth_token = 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 {
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;
}
}
}

Permissions permissions = 4;
permissions.Permissions permissions = 4;
Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cprice404 FYI ^

Copy link
Collaborator

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.

Copy link
Contributor Author

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 the javascript-web changes
momentohq/client-sdk-javascript#729

image

Copy link
Contributor Author

@ben-kugler ben-kugler Aug 16, 2023

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 the package 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 out package (thank you @krispraws for the great comments), I thought I would keep the package declares and update permissions.Permissions to Permissions for javascript-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, so ChannelCredentials, Interceptor had conflicts between the two different versions.
image

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 and token, but not a hug fan of this.

Copy link
Collaborator

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.

Copy link
Contributor

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?

Copy link
Contributor

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.

}

message _GenerateApiTokenResponse {
Expand Down
96 changes: 96 additions & 0 deletions proto/permissions.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
syntax = "proto3";
Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 package permissions temporarily, the types in it are still being referred to as permissions.* in the other protos.


// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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?
Perhaps it's clearer to have these restrictions in proto instead of mr2?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 CacheItemSelector, but if we do want to support CacheItemSelector in v1 tokens later, i think it will be easier to make validation and sdk changes to allow rather then proto changes. Yes, we want to allow acorn's for us in topics, I think one of the key use cases for acorn's was browser chats.
i do think you're right that it's probably would be more clear, but I think the only groups that will have troubles are the brave souls using our proto's directly.

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;
}
}
}
35 changes: 35 additions & 0 deletions proto/token.proto
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) {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rpc GenerateTempAuthToken (_GenerateTempAuthTokenRequest) returns (stream _GenerateTempAuthTokenResponse) {}
rpc GetTemporaryAuthToken (_GetTemporaryAuthTokenRequest) returns (_GetTemporaryAuthTokenResponse) {}
  • I think you can just use "get" instead of "generate" to mean the same thing.
  • We should prefer clear, full words like "temporary" over needless abbreviations like "temp."
  • If you think you need a server-side streaming response, let's chat on slack. I'm guessing that was just a mistaken copypaste?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I think you can just use "get" instead of "generate" to mean the same thing.
    • I like that
  • We should prefer clear, full words like "temporary" over needless abbreviations like "temp."
    • Yes, this name also happens to be temporary, this pr is in effort to get more of a discussion/decision on the API name
  • If you think you need a server-side streaming response, let's chat on slack. I'm guessing that was just a mistaken copypaste?
    • Yep, def a copypaste miss

I'll clean these up once name is settled

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 Get, we will keep Generate for consistency with existing API. Jury might still be out on Temporary vs other names

}

message _GenerateTempAuthTokenRequest {
Copy link
Contributor Author

@ben-kugler ben-kugler Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs to be finalized, alternative suggestions

  • GenerateEphemeralAuthToken
  • GenerateProvisionalAuthToken
  • GenerateBriefAuthToken
  • GeneratePerishableAuthToken
  • GenerateDecayingAuthToken
  • GenerateTransientAuthToken

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For better or worse, we have landed on GenerateAuthAcorn.

// 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;

}
8 changes: 8 additions & 0 deletions rust/momento-protos/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
pub mod permissions {
include!("permissions.rs");
}

pub mod auth {
include!("auth.rs");
}

pub mod token {
include!("token.rs");
}

pub mod cache_client {
include!("cache_client.rs");

Expand Down
2 changes: 2 additions & 0 deletions rust/proto_generator/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ fn main() {
.out_dir(out_dir)
.compile(
&[
format!("{proto_dir}/permissions.proto"),
ben-kugler marked this conversation as resolved.
Show resolved Hide resolved
format!("{proto_dir}/auth.proto"),
format!("{proto_dir}/token.proto"),
format!("{proto_dir}/cacheclient.proto"),
format!("{proto_dir}/cachepubsub.proto"),
format!("{proto_dir}/controlclient.proto"),
Expand Down
Loading