feat(core): support multiple authorization types in refreshToken config#224
feat(core): support multiple authorization types in refreshToken config#224halvaradop wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughOAuth token schemas and payload transformation now support refresh-token expiry and configurable authorization. Refresh requests handle basic or credentials authentication, callbacks create provider-scoped JWT cookies, and related tests and configuration are updated. ChangesOAuth token flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OAuthProvider
participant OAuthCallback
participant jwtManager
participant Browser
OAuthProvider->>OAuthCallback: accessToken response
OAuthCallback->>jwtManager: createToken(token payload)
jwtManager-->>OAuthCallback: providerToken
OAuthCallback->>Browser: Set-Cookie provider-scoped access token
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/core/test/actions/tokens/tokens.test.ts (1)
196-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock returns
OAuthTokenPayload(camelCase) instead ofOAuthAccessTokenResponseType(snake_case).Both the basic-auth and credentials-auth tests use
json: async () => oauthTokensas the fetch mock.oauthTokenshas camelCase properties (accessToken,expiresAt,refreshToken), butrefreshProviderTokenreads snake_case fields (data.access_token,data.expires_in,data.refresh_token). Since those are allundefined, every value falls back topayload.*. The tests pass but never exercise the response-parsing logic — a field-name regression would go undetected.♻️ Proposed fix: use a snake_case mock response
const mockFetch = vi.fn().mockResolvedValueOnce({ ok: true, - json: async () => oauthTokens, + json: async () => ({ + access_token: "new-access-token", + expires_in: 7200, + refresh_token: "new-refresh-token", + refresh_token_expires_in: 86400, + token_type: "Bearer", + scope: "scope1 scope2", + id_token: undefined, + }), })Then update the response assertion to expect the transformed values rather than
...oauthTokenswith fallbacks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/test/actions/tokens/tokens.test.ts` around lines 196 - 248, Update the fetch mocks in the credentials-auth and basic-auth refresh tests to return an OAuthAccessTokenResponseType-shaped object with snake_case fields (access_token, expires_in, refresh_token) instead of oauthTokens. Adjust the response assertions to verify the values transformed from those snake_case fields, ensuring refreshProviderToken exercises response parsing rather than payload fallbacks.packages/core/src/shared/utils.ts (1)
239-253: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCapture
Date.now()once for timestamp consistency.
Date.now()is called three separate times (lines 243, 246, 251). While the difference is typically negligible,expiresAt,refreshTokenExpiresAt, andissuedAtcould have slightly different base timestamps under edge conditions. Extract a singleconst now = Math.floor(Date.now() / 1000)and reuse it.♻️ Proposed refactor
export const transformToTokenPayload = (tokens: OAuthAccessTokenResponseType & { id_token?: string }) => { + const now = Math.floor(Date.now() / 1000) return { accessToken: tokens.access_token, - expiresAt: tokens.expires_in ? Math.floor(Date.now() / 1000) + tokens.expires_in : undefined, + expiresAt: tokens.expires_in ? now + tokens.expires_in : undefined, refreshToken: tokens.refresh_token, refreshTokenExpiresAt: tokens.refresh_token_expires_in - ? Math.floor(Date.now() / 1000) + tokens.refresh_token_expires_in + ? now + tokens.refresh_token_expires_in : undefined, idToken: tokens.id_token, tokenType: tokens.token_type, scopes: isString(tokens.scope) ? [tokens.scope] : Array.isArray(tokens.scope) ? tokens.scope : [], - issuedAt: Math.floor(Date.now() / 1000), + issuedAt: now, } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/shared/utils.ts` around lines 239 - 253, Update transformToTokenPayload to compute a single const now = Math.floor(Date.now() / 1000) before constructing the payload, then reuse now for expiresAt, refreshTokenExpiresAt, and issuedAt instead of calling Date.now() repeatedly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/actions/callback/callback.ts`:
- Around line 135-136: Default the token type to "Bearer" when constructing the
provider token in the callback flow: ensure the payload returned by
transformToTokenPayload has tokenType set to "Bearer" whenever the OAuth
response omits token_type, before passing it to context.jwtManager.createToken.
This keeps it valid for OAuthTokenPayloadSchema and parseOAuthTokens.
In `@packages/core/src/api/getProviderTokens.ts`:
- Around line 29-39: In the provider token request logic, avoid calling
createBasicAuthHeader unconditionally before determining the authorization mode.
Compute isCredentialsAuth first, then create the Basic auth header only when it
is needed, while preserving the existing credentials-auth behavior and request
headers.
- Line 23: Fix the refresh-token validation in getProviderTokens by explicitly
rejecting object-shaped refreshToken values that lack a url property; do not
rely on isRefreshTokenObject for this check because it already requires url.
Preserve support for string URLs and valid refresh-token objects, and ensure
invalid objects are rejected before fetchAsync receives them.
---
Nitpick comments:
In `@packages/core/src/shared/utils.ts`:
- Around line 239-253: Update transformToTokenPayload to compute a single const
now = Math.floor(Date.now() / 1000) before constructing the payload, then reuse
now for expiresAt, refreshTokenExpiresAt, and issuedAt instead of calling
Date.now() repeatedly.
In `@packages/core/test/actions/tokens/tokens.test.ts`:
- Around line 196-248: Update the fetch mocks in the credentials-auth and
basic-auth refresh tests to return an OAuthAccessTokenResponseType-shaped object
with snake_case fields (access_token, expires_in, refresh_token) instead of
oauthTokens. Adjust the response assertions to verify the values transformed
from those snake_case fields, ensuring refreshProviderToken exercises response
parsing rather than payload fallbacks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22101bc6-c2dd-44d8-a912-094decb7f5e3
📒 Files selected for processing (12)
apps/nextjs/app-router/src/lib/auth.tspackages/core/src/@types/oauth.tspackages/core/src/actions/callback/callback.tspackages/core/src/api/getProviderTokens.tspackages/core/src/oauth/atlassian.tspackages/core/src/schemas.tspackages/core/src/shared/assert.tspackages/core/src/shared/utils.tspackages/core/test/actions/tokens/tokens.test.tspackages/core/test/api/getAccessToken.test.tspackages/core/test/api/getProviderTokens.test.tspackages/react/package.json
| const tokenPayload = transformToTokenPayload(accessToken) | ||
| const providerToken = await context.jwtManager.createToken(tokenPayload) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the createToken implementation to check if it validates the payload
rg -n 'createToken' packages/core/src --type=ts -C3Repository: aura-stack-ts/auth
Length of output: 4578
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== transformToTokenPayload / OAuthTokenPayloadSchema ==\n'
rg -n 'transformToTokenPayload|OAuthTokenPayloadSchema|tokenType|token_type' packages/core/src --type=ts -C4
printf '\n== JWTManager createToken implementation / typings ==\n'
rg -n 'createToken\\s*:' packages/core/src --type=ts -C4
rg -n 'encodeJWT|signJWS|encryptJWE|jose-manager' packages/core/src --type=ts -C4
printf '\n== parseOAuthTokens / getProviderTokens path ==\n'
rg -n 'parseOAuthTokens|getProviderTokens|refreshProviderToken' packages/core/src --type=ts -C4Repository: aura-stack-ts/auth
Length of output: 14222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== jose-manager ==\n'
sed -n '1,120p' packages/core/src/session/jose-manager.ts
printf '\n== validator registry oauth section ==\n'
sed -n '160,215p' packages/core/src/validator/registry.ts
printf '\n== getProviderTokens ==\n'
sed -n '1,180p' packages/core/src/api/getProviderTokens.ts
printf '\n== parseOAuthTokens search ==\n'
rg -n 'parseOAuthTokens' packages/core/src -C4Repository: aura-stack-ts/auth
Length of output: 11583
Default tokenType when building provider tokens.
transformToTokenPayload can emit tokenType: undefined because OAuthAccessTokenResponse.token_type is optional, but OAuthTokenPayloadSchema.tokenType is required. That means the cookie can be written successfully and then fail when parseOAuthTokens reads it back. Defaulting to "Bearer" here avoids the mismatch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/actions/callback/callback.ts` around lines 135 - 136,
Default the token type to "Bearer" when constructing the provider token in the
callback flow: ensure the payload returned by transformToTokenPayload has
tokenType set to "Bearer" whenever the OAuth response omits token_type, before
passing it to context.jwtManager.createToken. This keeps it valid for
OAuthTokenPayloadSchema and parseOAuthTokens.
| provider: RuntimeOAuthProvider | ||
| ): Promise<OAuthTokenPayload> => { | ||
| if (!provider.refreshToken || (typeof provider.refreshToken === "object" && !("url" in provider.refreshToken))) { | ||
| if (!provider.refreshToken || (isRefreshTokenObject(provider.refreshToken) && !("url" in provider.refreshToken))) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Dead validation: object without url is never caught.
isRefreshTokenObject already checks "url" in value, so isRefreshTokenObject(provider.refreshToken) && !("url" in provider.refreshToken) is always false. An object-shaped refreshToken missing url passes validation, then line 29 falls to the else branch and passes the raw object as the URL to fetchAsync, causing a runtime failure.
🐛 Proposed fix
- if (!provider.refreshToken || (isRefreshTokenObject(provider.refreshToken) && !("url" in provider.refreshToken))) {
+ if (!provider.refreshToken || (typeof provider.refreshToken === "object" && !isRefreshTokenObject(provider.refreshToken)) {
throw new AuraAuthError({ code: "OAUTH_INVALID_REFRESH_TOKEN_CONFIG" })
}This throws when refreshToken is an object that lacks url, while still allowing string URLs and valid objects with url.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/api/getProviderTokens.ts` at line 23, Fix the refresh-token
validation in getProviderTokens by explicitly rejecting object-shaped
refreshToken values that lack a url property; do not rely on
isRefreshTokenObject for this check because it already requires url. Preserve
support for string URLs and valid refresh-token objects, and ensure invalid
objects are rejected before fetchAsync receives them.
| const url = isRefreshTokenObject(provider.refreshToken) ? provider.refreshToken.url : provider.refreshToken | ||
| const basicAuth = createBasicAuthHeader(provider.clientId!, provider.clientSecret!) | ||
|
|
||
| const isCredentialsAuth = isRefreshTokenObject(provider.refreshToken) | ||
| ? provider.refreshToken.authorization?.type === "credentials" | ||
| : false | ||
| const response = await fetchAsync(url, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/x-www-form-urlencoded", | ||
| ...(isCredentialsAuth ? {} : { Authorization: basicAuth }), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
createBasicAuthHeader called unconditionally — can throw in credentials-auth mode.
createBasicAuthHeader is invoked on line 30 before isCredentialsAuth is computed. When authorization.type === "credentials", the Basic header is never used (line 39 spreads {}), yet the call still executes. If clientId or clientSecret is missing, it throws AUTH_BASIC_CREDENTIALS_INVALID — a misleading error for a code path that sends credentials in the body, not as a Basic header.
🐛 Proposed fix: defer basic-auth creation until needed
const url = isRefreshTokenObject(provider.refreshToken) ? provider.refreshToken.url : provider.refreshToken
- const basicAuth = createBasicAuthHeader(provider.clientId!, provider.clientSecret!)
const isCredentialsAuth = isRefreshTokenObject(provider.refreshToken)
? provider.refreshToken.authorization?.type === "credentials"
: false
const response = await fetchAsync(url, {
method: "POST",
headers: {
"Content-Type": "application/x-www-form-urlencoded",
- ...(isCredentialsAuth ? {} : { Authorization: basicAuth }),
+ ...(isCredentialsAuth ? {} : { Authorization: createBasicAuthHeader(provider.clientId!, provider.clientSecret!) }),
...(typeof provider.refreshToken === "object" && provider.refreshToken.headers ? provider.refreshToken.headers : {}),
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const url = isRefreshTokenObject(provider.refreshToken) ? provider.refreshToken.url : provider.refreshToken | |
| const basicAuth = createBasicAuthHeader(provider.clientId!, provider.clientSecret!) | |
| const isCredentialsAuth = isRefreshTokenObject(provider.refreshToken) | |
| ? provider.refreshToken.authorization?.type === "credentials" | |
| : false | |
| const response = await fetchAsync(url, { | |
| method: "POST", | |
| headers: { | |
| "Content-Type": "application/x-www-form-urlencoded", | |
| ...(isCredentialsAuth ? {} : { Authorization: basicAuth }), | |
| const url = isRefreshTokenObject(provider.refreshToken) ? provider.refreshToken.url : provider.refreshToken | |
| const isCredentialsAuth = isRefreshTokenObject(provider.refreshToken) | |
| ? provider.refreshToken.authorization?.type === "credentials" | |
| : false | |
| const response = await fetchAsync(url, { | |
| method: "POST", | |
| headers: { | |
| "Content-Type": "application/x-www-form-urlencoded", | |
| ...(isCredentialsAuth ? {} : { Authorization: createBasicAuthHeader(provider.clientId!, provider.clientSecret!) }), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/api/getProviderTokens.ts` around lines 29 - 39, In the
provider token request logic, avoid calling createBasicAuthHeader
unconditionally before determining the authorization mode. Compute
isCredentialsAuth first, then create the Basic auth header only when it is
needed, while preserving the existing credentials-auth behavior and request
headers.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes