feat: add token id optional field to disposable tokens #968

pratik151192 · 2023-10-16T18:27:11Z

Added token id as optional field (within an optional props) to disposable tokens API. The object DisposableTokenScope is encapsulated with all kinds of permission policies that we can provide, and it didn't seem clean to add a tokenID field there. Instead, added as a new props for that API.

PR #1 for https://github.com/momentohq/dev-eco-issue-tracker/issues/511

pratik151192 · 2023-10-17T00:27:27Z

@cprice404 I notice that the method generateAPIKey too accepts a tokenID but I didn't club it in this PR as the ticket was scoped to the disposableToken API. LMK if you want me to club here though.

pratik151192 · 2023-10-17T00:29:22Z

packages/core/src/internal/utils/validators.ts

+  if (tokenID.length > 64) {
+    throw new InvalidArgumentError(
+      'TokenID must be less than or equal to 64 characters.'
+    );
+  }


I had thought to encode the string and check the byte length in UTF-8 or something so lmk if this seems less potent that that.

I think this is fine

anitarua · 2023-10-17T16:24:12Z

packages/client-sdk-nodejs/src/internal/internal-auth-client.ts

    const request = new token.token._GenerateDisposableTokenRequest({
      expires: expires,
      auth_token: this.creds.getAuthToken(),
      permissions: permissions,
+      token_id: tokenID,


can the proto accept an undefined tokenID?

yes

constructor(data?: any[] | { expires?: _GenerateDisposableTokenRequest.Expires; auth_token?: string; permissions?: dependency_1.permission_messages.Permissions; token_id?: string; });

anitarua

lgtm!

pratik151192 · 2023-10-17T20:30:33Z

@anitarua @cprice404 unless am seeing it wrong, looks like the last 3 CIs failed on one or more tests unrelated to the PR. This particularly started happening after I added 2 new auth tests which don't seem to be related to integ tests. But would love another eye; I am afraid if this might be another Azure tipping point for something

cprice404 · 2023-10-17T20:48:56Z

@anitarua @cprice404 unless am seeing it wrong, looks like the last 3 CIs failed on one or more tests unrelated to the PR. This particularly started happening after I added 2 new auth tests which don't seem to be related to integ tests. But would love another eye; I am afraid if this might be another Azure tipping point for something

this seems really different from prev failures in that it's data plane (afaik all the azure things happened on control plane), plus if this is running on osx i'm not sure it's even using azure. Did you spot check the server metrics?

cprice404 · 2023-10-18T16:31:17Z

@anitarua @cprice404 unless am seeing it wrong, looks like the last 3 CIs failed on one or more tests unrelated to the PR. This particularly started happening after I added 2 new auth tests which don't seem to be related to integ tests. But would love another eye; I am afraid if this might be another Azure tipping point for something

this seems really different from prev failures in that it's data plane (afaik all the azure things happened on control plane), plus if this is running on osx i'm not sure it's even using azure. Did you spot check the server metrics?

I'm gonna kick the failed CI jobs one more time just to rule out something weird going on yesterday

cprice404

just a few nits!

cprice404 · 2023-10-18T16:33:14Z

packages/client-sdk-nodejs/src/internal/internal-auth-client.ts

@@ -208,10 +211,22 @@ export class InternalAuthClient implements IAuthClient {
      return new GenerateDisposableToken.Error(normalizeSdkError(err as Error));
    }

+    const tokenID = disposableTokenProps?.tokenID;
+    if (tokenID) {


javascript has very liberal definitions for "truthiness", so it's always better to be very explicit about the condition you are checking for. In this case I think it would be !== undefined.

cprice404 · 2023-10-18T16:34:03Z

packages/client-sdk-web/src/internal/auth-client.ts

@@ -213,6 +216,18 @@ export class InternalWebGrpcAuthClient<

    request.setPermissions(permissions);

+    const tokenID = disposableTokenProps?.tokenID;
+    if (tokenID) {


same nit about explicit condition check here

cprice404 · 2023-10-18T16:35:42Z

packages/client-sdk-nodejs/src/internal/internal-auth-client.ts

@@ -208,10 +211,22 @@ export class InternalAuthClient implements IAuthClient {
      return new GenerateDisposableToken.Error(normalizeSdkError(err as Error));
    }

+    const tokenID = disposableTokenProps?.tokenID;


supernit: I think the idiomatic casing for this variable name in TS/JS would be tokenId. I don't really care about it here for a local var, but we want to be deliberate in the props object since that is effectively public API.

cprice404 · 2023-10-18T16:37:02Z

packages/core/src/auth/tokens/disposable-token-scope.ts

@@ -77,6 +77,10 @@ export type DisposableTokenScope =
  | PredefinedScope
  | DisposableTokenCachePermissions;

+export interface DisposableTokenProps {
+  tokenID?: string;


here - I think we want tokenId for this public interface unless you have some style reference that suggests otherwise

cprice404 · 2023-10-18T16:38:36Z

packages/core/src/internal/utils/validators.ts

+  if (tokenID.length > 64) {
+    throw new InvalidArgumentError(
+      'TokenID must be less than or equal to 64 characters.'
+    );
+  }


I think this is fine

cprice404 · 2023-10-18T16:46:09Z

@anitarua @cprice404 unless am seeing it wrong, looks like the last 3 CIs failed on one or more tests unrelated to the PR. This particularly started happening after I added 2 new auth tests which don't seem to be related to integ tests. But would love another eye; I am afraid if this might be another Azure tipping point for something

this seems really different from prev failures in that it's data plane (afaik all the azure things happened on control plane), plus if this is running on osx i'm not sure it's even using azure. Did you spot check the server metrics?

I'm gonna kick the failed CI jobs one more time just to rule out something weird going on yesterday

the latest CI failure looks more like a bug than a transient issue; i don't see any timeouts or anything

anitarua

lgtm!

pratik151192 added 4 commits October 16, 2023 15:26

feat: add token id optional field to disposable tokens

488a6f8

chore: use generic props object for API signature

fd50c4d

chore: add tests for tokenID changes

bf72647

chore: add try-catch during validation

47a8200

pratik151192 requested review from cprice404 and anitarua October 17, 2023 00:26

pratik151192 marked this pull request as ready for review October 17, 2023 00:26

pratik151192 commented Oct 17, 2023

View reviewed changes

anitarua reviewed Oct 17, 2023

View reviewed changes

anitarua previously approved these changes Oct 17, 2023

View reviewed changes

cprice404 reviewed Oct 18, 2023

View reviewed changes

chore: addressing comments for style

c1101d7

pratik151192 dismissed anitarua’s stale review via c1101d7 October 18, 2023 17:24

pratik151192 requested review from anitarua and cprice404 October 18, 2023 21:11

anitarua approved these changes Oct 18, 2023

View reviewed changes

cprice404 approved these changes Oct 19, 2023

View reviewed changes

pratik151192 merged commit b7ed3bd into main Oct 19, 2023
13 checks passed

pratik151192 deleted the token-id-disposable-tokens branch October 19, 2023 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add token id optional field to disposable tokens #968

feat: add token id optional field to disposable tokens #968

pratik151192 commented Oct 16, 2023 •

edited

Loading

pratik151192 commented Oct 17, 2023

pratik151192 Oct 17, 2023

cprice404 Oct 18, 2023

anitarua Oct 17, 2023

pratik151192 Oct 17, 2023

anitarua left a comment

pratik151192 commented Oct 17, 2023

cprice404 commented Oct 17, 2023

cprice404 commented Oct 18, 2023

cprice404 left a comment

cprice404 Oct 18, 2023

cprice404 Oct 18, 2023

cprice404 Oct 18, 2023

cprice404 Oct 18, 2023

cprice404 Oct 18, 2023

cprice404 commented Oct 18, 2023

anitarua left a comment

feat: add token id optional field to disposable tokens #968

feat: add token id optional field to disposable tokens #968

Conversation

pratik151192 commented Oct 16, 2023 • edited Loading

pratik151192 commented Oct 17, 2023

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

anitarua left a comment

Choose a reason for hiding this comment

pratik151192 commented Oct 17, 2023

cprice404 commented Oct 17, 2023

cprice404 commented Oct 18, 2023

cprice404 left a 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

cprice404 commented Oct 18, 2023

anitarua left a comment

Choose a reason for hiding this comment

pratik151192 commented Oct 16, 2023 •

edited

Loading