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

ben-kugler · 2023-08-15T20:23:43Z

Moved permissions to a new shared folder
GenerateAuthToken contains no new changes, only moving of Permissions message
New GenerateTempAuthToken request and response, is the same as GenerateAuthToken however it doesn't include a expiration with option of Never and response will never include a refreshToken

This reverts commit 6d627ea.

ben-kugler · 2023-08-15T21:20:33Z

proto/token.proto

+  rpc GenerateTempAuthToken (_GenerateTempAuthTokenRequest) returns (stream _GenerateTempAuthTokenResponse) {}
+}
+
+message _GenerateTempAuthTokenRequest {


This still needs to be finalized, alternative suggestions

GenerateEphemeralAuthToken

GenerateProvisionalAuthToken

GenerateBriefAuthToken

GeneratePerishableAuthToken

GenerateDecayingAuthToken

GenerateTransientAuthToken

For better or worse, we have landed on GenerateAuthAcorn.

kvcache · 2023-08-15T21:18:22Z

proto/token.proto

+package token;
+
+service Token {
+  rpc GenerateTempAuthToken (_GenerateTempAuthTokenRequest) returns (stream _GenerateTempAuthTokenResponse) {}


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?

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

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

proto/token.proto

krispraws · 2023-08-15T23:48:22Z

proto/auth.proto

-    }
-
-    Permissions permissions = 4;
+  permissions.Permissions permissions = 4;


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.

@cprice404 FYI ^

good callout, definitely worth testing.

Got a successful build without package permissions, did have to change imports, here's the javascript-web changes
momentohq/client-sdk-javascript#729

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.

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.

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.

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?

I agree with @cprice404 - we can push the complexity into javascript-web builds.

proto/permissions.proto

krispraws · 2023-08-15T23:52:13Z

proto/permissions.proto

+option java_multiple_files = true;
+option java_package = "momento.shared.permissions";
+
+package permissions;


if we can avoid using a package here, that might be sufficient to keep the web sdk working.

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.

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.

proto/token.proto

rust/proto_generator/build.rs

cprice404 · 2023-08-16T01:42:40Z

proto/permissions.proto

@@ -0,0 +1,96 @@
+syntax = "proto3";


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

No diff, unchanged migration to a separate file

cprice404 · 2023-08-16T01:44:04Z

proto/token.proto

+package token;
+
+service Token {
+  rpc GenerateTempAuthToken (_GenerateTempAuthTokenRequest) returns (stream _GenerateTempAuthTokenResponse) {}


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

proto/token.proto

rust/proto_generator/build.rs

wandaitzuchen · 2023-08-17T21:03:45Z

proto/permissions.proto

+    }
+  }
+
+  message CachePermissions {


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?

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.

ben-kugler added 5 commits August 15, 2023 13:23

feat: move permissions to shared, a new token proto

6a28c40

remove local testing file

6d627ea

Revert "remove local testing file"

7b4ea73

This reverts commit 6d627ea.

correct java/rust(maybe) build

8c26e25

missing permissions mode declare

b90a673

ben-kugler requested review from cprice404, danielamiao, ksmotiv8, kvcache, krispraws and wandaitzuchen August 15, 2023 21:13

Remove extra auth_token

74a212b

ben-kugler commented Aug 15, 2023

View reviewed changes

kvcache reviewed Aug 15, 2023

View reviewed changes

krispraws reviewed Aug 15, 2023

View reviewed changes

proto/token.proto Show resolved Hide resolved

krispraws reviewed Aug 15, 2023

View reviewed changes

rust/proto_generator/build.rs Show resolved Hide resolved

cprice404 reviewed Aug 16, 2023

View reviewed changes

ben-kugler added 5 commits August 16, 2023 12:05

remove package from permissions, add rest of build files

0d8b5f1

missing rust import, don't use momento npm

53d1d79

comment correction, add auth_token

41e206e

add help message, don't use package ext in javascript web

c55d389

we've landed on a name, GenerateAuthAcorn

1276e60

ben-kugler requested review from cprice404 and krispraws August 17, 2023 18:20

wandaitzuchen reviewed Aug 17, 2023

View reviewed changes

wandaitzuchen approved these changes Aug 17, 2023

View reviewed changes

ben-kugler merged commit ce843d6 into main Aug 18, 2023
7 checks passed

ben-kugler deleted the feat/permissions-to-shared+token-proto branch August 18, 2023 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ben-kugler commented Aug 15, 2023 •

edited

Loading

ben-kugler Aug 15, 2023 •

edited

Loading

krispraws Aug 17, 2023

kvcache Aug 15, 2023

ben-kugler Aug 15, 2023

cprice404 Aug 16, 2023

krispraws Aug 15, 2023

krispraws Aug 15, 2023

cprice404 Aug 16, 2023

ben-kugler Aug 16, 2023

ben-kugler Aug 16, 2023 •

edited

Loading

cprice404 Aug 16, 2023

krispraws Aug 17, 2023

krispraws Aug 17, 2023

krispraws Aug 15, 2023

cprice404 Aug 16, 2023

krispraws Aug 16, 2023

cprice404 Aug 16, 2023

ben-kugler Aug 16, 2023

cprice404 Aug 16, 2023

wandaitzuchen Aug 17, 2023

ben-kugler Aug 17, 2023

	rpc GenerateTempAuthToken (_GenerateTempAuthTokenRequest) returns (stream _GenerateTempAuthTokenResponse) {}
	rpc GetTemporaryAuthToken (_GetTemporaryAuthTokenRequest) returns (_GetTemporaryAuthTokenResponse) {}

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

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

Conversation

ben-kugler commented Aug 15, 2023 • edited Loading

ben-kugler Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-kugler Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-kugler commented Aug 15, 2023 •

edited

Loading

ben-kugler Aug 15, 2023 •

edited

Loading

ben-kugler Aug 16, 2023 •

edited

Loading