Adding All Token Claims to Access Token and IDToken#149
Open
srinivaskarre-sk wants to merge 5 commits into
Open
Adding All Token Claims to Access Token and IDToken#149srinivaskarre-sk wants to merge 5 commits into
srinivaskarre-sk wants to merge 5 commits into
Conversation
Adding All Token Claims to Access Token and IDToken
WalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Scalekit as ScalekitClient
participant JWKS as JWKS/KeySource
participant JWTLib as jwtVerify
Client->>Scalekit: getTokenClaims(token, options?)
Scalekit->>JWKS: fetch/resolve signing keys (if needed)
Scalekit->>JWTLib: jwtVerify(token, { jwks, ...options })
JWTLib-->>Scalekit: decoded payload (Record<string, any>)
Scalekit-->>Scalekit: attach shallow copy -> payload.claims = { ...payload }
Scalekit-->>Client: return payload augmented with claims
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/scalekit.test.ts (1)
113-121: Consider extracting token acquisition to reduce duplication.The token fetch logic is repeated in two tests. Extracting it to a helper function or using a
beforeAllhook would improve maintainability.♻️ Suggested refactor
// Add helper at the top of the describe block or in test utils async function fetchClientAccessToken(): Promise<string> { const envUrl = process.env.SCALEKIT_ENVIRONMENT_URL!; const clientId = process.env.SCALEKIT_CLIENT_ID!; const clientSecret = process.env.SCALEKIT_CLIENT_SECRET!; const tokenResponse = await axios.post( `${envUrl}/oauth/token`, new URLSearchParams({ grant_type: 'client_credentials', client_id: clientId, client_secret: clientSecret, }).toString(), { headers: { 'Content-Type': 'application/x-www-form-urlencoded' } } ); return tokenResponse.data.access_token; }Then use in tests:
const accessToken = await fetchClientAccessToken();Also applies to: 144-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/scalekit.test.ts` around lines 113 - 121, Extract the repeated token-acquisition logic into a helper (e.g., async function fetchClientAccessToken()) or run it once in a beforeAll hook and reuse the returned access token in tests; locate the axios POST block that builds new URLSearchParams with grant_type/client_id/client_secret (currently inside the tests referencing envUrl/clientId/clientSecret) and move that logic into fetchClientAccessToken(), returning tokenResponse.data.access_token, then replace the duplicated axios calls in the tests with await fetchClientAccessToken() (or assign the result to a variable in beforeAll and reference that variable in each test).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/scalekit.test.ts`:
- Around line 113-121: Extract the repeated token-acquisition logic into a
helper (e.g., async function fetchClientAccessToken()) or run it once in a
beforeAll hook and reuse the returned access token in tests; locate the axios
POST block that builds new URLSearchParams with
grant_type/client_id/client_secret (currently inside the tests referencing
envUrl/clientId/clientSecret) and move that logic into fetchClientAccessToken(),
returning tokenResponse.data.access_token, then replace the duplicated axios
calls in the tests with await fetchClientAccessToken() (or assign the result to
a variable in beforeAll and reference that variable in each test).
refactor and tc
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/scalekit.test.ts (2)
109-171: Guard live-token tests when credentials are missing.These tests hit a live
/oauth/tokenendpoint and rely on SCALEKIT_* env vars. Without creds they’ll fail early, so consider skipping when env vars are absent.♻️ Suggested guard for missing env vars
- describe('getTokenClaims', () => { + const hasClientCreds = + !!process.env.SCALEKIT_ENVIRONMENT_URL && + !!process.env.SCALEKIT_CLIENT_ID && + !!process.env.SCALEKIT_CLIENT_SECRET; + const describeWithCreds = hasClientCreds ? describe : describe.skip; + + describeWithCreds('getTokenClaims', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/scalekit.test.ts` around lines 109 - 171, Guard the live-token tests by checking for SCALEKIT_ENVIRONMENT_URL, SCALEKIT_CLIENT_ID, and SCALEKIT_CLIENT_SECRET before running the getTokenClaims test suite: in the describe('getTokenClaims') block, detect missing env vars and call test.skip or return early (e.g., use beforeAll that calls jest.skip or throws a specific Skip) so the three tests that post to /oauth/token and call client.getTokenClaims are skipped when credentials are absent; update references to the live-token logic (the axios.post calls and client.getTokenClaims<AccessTokenClaims>) to only run when all required env vars are present.
179-250: Make the unit tests fully self‑contained.This suite is labeled “unit” but still depends on SCALEKIT_ENVIRONMENT_URL. A fixed issuer fallback keeps it runnable without env configuration.
♻️ Suggested issuer fallback
- describe('validateToken (unit)', () => { + describe('validateToken (unit)', () => { + const UNIT_ISSUER = process.env.SCALEKIT_ENVIRONMENT_URL ?? 'https://unit.test'; ... - const envUrl = process.env.SCALEKIT_ENVIRONMENT_URL!; + const envUrl = UNIT_ISSUER; ... - const token = await new SignJWT({ sub: SUBJECT, iss: envUrl, custom: 'hello' }) + const token = await new SignJWT({ sub: SUBJECT, iss: UNIT_ISSUER, custom: 'hello' }) ... - const token = await new SignJWT({ sub: SUBJECT, iss: envUrl }) + const token = await new SignJWT({ sub: SUBJECT, iss: UNIT_ISSUER })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/scalekit.test.ts` around lines 179 - 250, The tests rely on process.env.SCALEKIT_ENVIRONMENT_URL; change the suite to use a local fixed fallback (e.g. const envUrl = process.env.SCALEKIT_ENVIRONMENT_URL || 'https://scalekit.local') and reference that single envUrl value in beforeAll and all tests (the SignJWT calls, validateToken and getTokenClaims invocations) so the unit tests are self-contained and do not require external env configuration; update the beforeAll where ScalekitClient is constructed and where tokens are signed to use this fallback envUrl.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/scalekit.test.ts`:
- Around line 109-171: Guard the live-token tests by checking for
SCALEKIT_ENVIRONMENT_URL, SCALEKIT_CLIENT_ID, and SCALEKIT_CLIENT_SECRET before
running the getTokenClaims test suite: in the describe('getTokenClaims') block,
detect missing env vars and call test.skip or return early (e.g., use beforeAll
that calls jest.skip or throws a specific Skip) so the three tests that post to
/oauth/token and call client.getTokenClaims are skipped when credentials are
absent; update references to the live-token logic (the axios.post calls and
client.getTokenClaims<AccessTokenClaims>) to only run when all required env vars
are present.
- Around line 179-250: The tests rely on process.env.SCALEKIT_ENVIRONMENT_URL;
change the suite to use a local fixed fallback (e.g. const envUrl =
process.env.SCALEKIT_ENVIRONMENT_URL || 'https://scalekit.local') and reference
that single envUrl value in beforeAll and all tests (the SignJWT calls,
validateToken and getTokenClaims invocations) so the unit tests are
self-contained and do not require external env configuration; update the
beforeAll where ScalekitClient is constructed and where tokens are signed to use
this fallback envUrl.
changing aud type to string[]
update with main and apply prettier config
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/scalekit.test.ts (1)
120-184: Make livegetTokenClaimstests conditional and DRY the token fetch path.Lines 122-124 and 154-156 hard-require env credentials and duplicate the same token request logic. Consider guarding this block when env vars are missing and centralizing token acquisition in one helper.
♻️ Suggested refactor
+const hasLiveTokenEnv = Boolean( + process.env.SCALEKIT_ENVIRONMENT_URL && + process.env.SCALEKIT_CLIENT_ID && + process.env.SCALEKIT_CLIENT_SECRET +); +const describeLive = hasLiveTokenEnv ? describe : describe.skip; + -describe('getTokenClaims', () => { +describeLive('getTokenClaims (integration)', () => { + const fetchClientCredentialsToken = async (): Promise<string> => { + const envUrl = process.env.SCALEKIT_ENVIRONMENT_URL!; + const clientId = process.env.SCALEKIT_CLIENT_ID!; + const clientSecret = process.env.SCALEKIT_CLIENT_SECRET!; + const tokenResponse = await axios.post( + `${envUrl}/oauth/token`, + new URLSearchParams({ + grant_type: 'client_credentials', + client_id: clientId, + client_secret: clientSecret, + }).toString(), + { headers: { 'Content-Type': 'application/x-www-form-urlencoded' } } + ); + return tokenResponse.data.access_token; + }; + it('should fetch client access token and validate claims via getTokenClaims', async () => { - // ... duplicate oauth/token request ... - const accessToken = tokenResponse.data.access_token; + const accessToken = await fetchClientCredentialsToken(); // ... });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/scalekit.test.ts` around lines 120 - 184, The tests for getTokenClaims duplicate the same axios.post token fetch and unconditionally require SCALEKIT_* env vars; refactor by extracting the token acquisition into a single helper (e.g., a getClientAccessToken helper used by the tests or a beforeAll that sets a shared accessToken) and guard the live tests by checking process.env.SCALEKIT_ENVIRONMENT_URL / SCALEKIT_CLIENT_ID / SCALEKIT_CLIENT_SECRET before running (skip or mark the describe/its when missing); update the tests that call client.getTokenClaims<AccessTokenClaims>(...) and the axios.post usage to use the centralized helper and make the live-suite conditional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/types/auth.ts`:
- Line 67: Update the AccessTokenClaims type so its aud property accepts both a
single string and an array of strings per RFC 7519: change the aud declaration
in the AccessTokenClaims interface/type (in src/types/auth.ts) from aud:
string[] to aud: string | string[] (or aud?: string | string[] if it should be
optional) so JWTs with a single audience string are accepted; make this change
on the AccessTokenClaims type definition referenced in the file.
---
Nitpick comments:
In `@tests/scalekit.test.ts`:
- Around line 120-184: The tests for getTokenClaims duplicate the same
axios.post token fetch and unconditionally require SCALEKIT_* env vars; refactor
by extracting the token acquisition into a single helper (e.g., a
getClientAccessToken helper used by the tests or a beforeAll that sets a shared
accessToken) and guard the live tests by checking
process.env.SCALEKIT_ENVIRONMENT_URL / SCALEKIT_CLIENT_ID /
SCALEKIT_CLIENT_SECRET before running (skip or mark the describe/its when
missing); update the tests that call
client.getTokenClaims<AccessTokenClaims>(...) and the axios.post usage to use
the centralized helper and make the live-suite conditional.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/constants/user.tssrc/scalekit.tssrc/types/auth.tstests/scalekit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/constants/user.ts
| export type AccessTokenClaims = { | ||
| sub: string; | ||
| iss: string; | ||
| aud: string[]; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
RFC 7519 JWT aud claim single string or array specification
💡 Result:
RFC 7519 (JWT) §4.1.3 specifies that the aud (Audience) claim can be either:
- Generally: a JSON array of case-sensitive strings (each a
StringOrURI) identifying intended recipients. If the receiver’s identifier is not present, the JWT MUST be rejected. [1] - Special case (single audience):
audMAY be a single case-sensitive string (StringOrURI) instead of an array. [1]
So: both a single string and an array are valid per RFC 7519; implementations should accept either form. [1]
Sources:
[1] RFC 7519, Section 4.1.3 (“aud” Claim) https://www.rfc-editor.org/rfc/rfc7519
🌐 Web query:
jose TypeScript library JWTPayload aud claim type definition
💡 Result:
In the jose TypeScript types, the JWT payload interface defines the aud (audience) claim as optional and typed as:
aud?: string | string[]This is shown in the generated type docs, which point to jose/dist/types/types.d.ts (where JWTPayload is declared). [1]
🏁 Script executed:
cat -n src/types/auth.ts | sed -n '60,75p'Repository: scalekit-inc/scalekit-sdk-node
Length of output: 496
AccessTokenClaims.aud must accept both string and string[] per RFC 7519.
The current type aud: string[] rejects valid JWTs with a single audience string. RFC 7519 §4.1.3 explicitly allows aud to be either a single string or an array of strings. The jose library's JWTPayload type reflects this with aud?: string | string[].
Proposed fix
export type AccessTokenClaims = {
sub: string;
iss: string;
- aud: string[];
+ aud: string | string[];
iat: number | undefined;
exp: number | undefined;
claims: Record<string, any>;
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types/auth.ts` at line 67, Update the AccessTokenClaims type so its aud
property accepts both a single string and an array of strings per RFC 7519:
change the aud declaration in the AccessTokenClaims interface/type (in
src/types/auth.ts) from aud: string[] to aud: string | string[] (or aud?: string
| string[] if it should be optional) so JWTs with a single audience string are
accepted; make this change on the AccessTokenClaims type definition referenced
in the file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adding All Token Claims to Access Token and IDToken
Summary by CodeRabbit
New Features
Chores
Tests & Docs