Peter/fix refresh token #242
Conversation
WalkthroughThe pull request introduces a new authentication management system in a React application by replacing the existing Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
src/frontend/AuthProvider.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-jest". (The package "eslint-plugin-jest" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-jest" was referenced from the config file in ".eslintrc.json". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (15)
src/session/isAuthenticated.js (2)
Line range hint
11-20: Consider implementing token refresh mechanismGiven that this PR aims to implement token refresh functionality, the current implementation might need enhancement. When the token is invalid (expired), instead of immediately returning false, consider attempting to refresh the token first.
Consider updating the implementation like this:
export const isAuthenticatedFactory = (req, res) => async () => { const accessToken = await ( await sessionManager(req, res) ).getSessionItem('access_token'); - const validToken = isTokenValid(accessToken); + let validToken = isTokenValid(accessToken); + + // If token is invalid, attempt to refresh + if (!validToken) { + const refreshToken = await ( + await sessionManager(req, res) + ).getSessionItem('refresh_token'); + + if (refreshToken) { + try { + const newTokens = await refreshAccessToken(refreshToken); + await (await sessionManager(req, res)).setSessionItems(newTokens); + validToken = true; + } catch (error) { + console.error('Failed to refresh token:', error); + validToken = false; + } + } + } const user = await getUserFactory(req, res)(); return validToken && Boolean(user); };
Line range hint
6-10: Consider migrating to TypeScript and improving error handlingSince other files are being converted to TypeScript (e.g., AuthProvider.tsx), consider:
- Converting this file to TypeScript for better type safety
- Adding more robust error handling and logging
Example TypeScript conversion:
import { NextApiRequest, NextApiResponse } from 'next'; export const isAuthenticatedFactory = ( req?: NextApiRequest, res?: NextApiResponse ) => async (): Promise<boolean> => { try { const session = await sessionManager(req, res); const accessToken = await session.getSessionItem('access_token'); if (!accessToken) { console.warn('No access token found in session'); return false; } const validToken = isTokenValid(accessToken); const user = await getUserFactory(req, res)(); return validToken && Boolean(user); } catch (error) { console.error('Authentication check failed:', error); return false; } };src/utils/isTokenValid.ts (2)
Line range hint
4-28: Consider adding expiration buffer and error reportingThe token validation is thorough but could be enhanced in two ways:
- Add a small buffer (e.g., 30 seconds) to the expiration check to prevent edge cases where the token expires during request processing
- Return specific validation failure reasons for better debugging
Here's a suggested enhancement:
-const isTokenValid = (token) => { +const isTokenValid = (token, expirationBufferSeconds = 30) => { const accessToken = token?.access_token ?? token; - if (!accessToken) return false; + if (!accessToken) return { isValid: false, reason: 'No token provided' }; const accessTokenHeader = jwtDecoder(accessToken, TokenPart.header); const accessTokenPayload = jwtDecoder(accessToken); let isAudienceValid = true; + let validationErrors = []; // ... audience validation logic ... + const currentTime = Math.floor(Date.now() / 1000); + const isExpired = accessTokenPayload.exp <= currentTime + expirationBufferSeconds; if ( accessTokenPayload.iss == config.issuerURL && accessTokenHeader.alg == 'RS256' && - accessTokenPayload.exp > Math.floor(Date.now() / 1000) && + !isExpired && isAudienceValid ) { - return true; + return { isValid: true }; } else { - return false; + if (accessTokenPayload.iss !== config.issuerURL) validationErrors.push('Invalid issuer'); + if (accessTokenHeader.alg !== 'RS256') validationErrors.push('Invalid algorithm'); + if (isExpired) validationErrors.push('Token expired or expiring soon'); + if (!isAudienceValid) validationErrors.push('Invalid audience'); + return { isValid: false, reasons: validationErrors }; } };This enhancement would help with debugging token validation issues and provide a grace period for expiring tokens.
Line range hint
1-28: Alignment with token refresh mechanismThe enhanced token validation aligns well with the PR's objective of implementing token refresh. The more robust audience validation and suggested improvements for expiration handling will help ensure smooth token refresh flows.
Consider documenting the relationship between this validation logic and the token refresh mechanism in the codebase, particularly how the expiration check triggers the refresh flow.
src/session/index.ts (2)
Line range hint
27-28: Remove @ts-ignore by properly typing the responseThe
@ts-ignorecomment suggests a type safety issue. Consider properly typing the response fromkindeClient.refreshTokensto maintain type safety throughout the codebase.- // @ts-ignore - const response = await kindeClient.refreshTokens( + const response: { + access_token: string; + refresh_token: string; + expires_in: number; + } = await kindeClient.refreshTokens(
Line range hint
25-37: Improve error handling in refreshTokens methodThe current implementation has several areas for improvement:
- The error type in the catch clause should be specified
- Returning null on error makes it difficult for consumers to handle errors appropriately
- Error information is lost when not in debug mode
Consider implementing this improved version:
refreshTokens: async () => { try { - // @ts-ignore const response = await kindeClient.refreshTokens( await sessionManager(req, res) ); return response; - } catch (error) { + } catch (error: unknown) { if (config.isDebugMode) { console.error(error); } - return null; + throw new Error( + error instanceof Error + ? error.message + : 'Failed to refresh tokens' + ); } },src/handlers/setup.ts (1)
Line range hint
74-109: Consider extracting mapping logic to separate utilitiesThe response construction contains complex nested property access and mapping logic. This could be simplified for better maintainability.
Consider extracting the organization properties mapping:
function mapOrganizationProperties(props: KindeTokenOrganizationProperties) { return { city: props?.kp_org_city?.v, industry: props?.kp_org_industry?.v, postcode: props?.kp_org_postcode?.v, state_region: props?.kp_org_state_region?.v, street_address: props?.kp_org_street_address?.v, street_address_2: props?.kp_org_street_address_2?.v }; }And the organizations mapping:
function mapOrganizations(orgCodes: string[], orgs: Array<{id: string; name: string}>) { return { orgCodes, orgs: orgs?.map(org => ({ code: org?.id, name: org?.name })) }; }src/frontend/KindeBrowserClient.js (3)
Line range hint
31-53: Critical: Implement token expiration handling and automatic refreshThe current implementation lacks essential token refresh functionality:
- No mechanism to detect token expiration
- No automatic refresh trigger on token expiry
- Missing error handling for 401/403 responses that could indicate expired tokens
This doesn't fulfill the PR's objective of implementing token refresh functionality.
Consider implementing the following:
const refreshData = async () => { const setupUrl = `${apiPath}/setup`; const res = await fetch(setupUrl); + // Handle 401/403 responses + if (res.status === 401 || res.status === 403) { + // Attempt token refresh + const refreshResult = await fetch(`${apiPath}/refresh`, { + method: 'POST', + credentials: 'include' + }); + if (refreshResult.ok) { + // Retry original request with new token + return refreshData(); + } + } const kindeData = await res.json(); if (res.status == 200) { setState({
Line range hint
127-139: Fix inconsistent error handling in flag gettersThe getStringFlag function has inconsistent error handling compared to other flag getters:
- Unused
err;statement at the end- Silent error swallowing could mask issues
Apply this fix:
const getStringFlag = (code, defaultValue) => { try { const flag = getFlag(code, defaultValue, 's'); return flag.value; } catch (err) { if (config.isDebugMode) { console.error(err); } - err; + return undefined; } };
Line range hint
13-26: Architecture Advice: Consider implementing a comprehensive token management systemThe current implementation needs a more robust token management system to fulfill the PR's objective. Consider:
- Implement token expiration tracking using JWT expiry claims
- Add automatic refresh mechanism before token expiration
- Handle concurrent requests during refresh
- Add retry mechanisms with exponential backoff
- Implement proper error handling for authentication failures
This would provide a more reliable authentication experience and prevent session interruptions.
Would you like me to provide a detailed implementation plan for these improvements?
types.d.ts (1)
354-354: LGTM: Token handling improvements for refresh implementation.The addition of
accessTokenRaw,idTokenEncoded, andidTokenRawproperties toKindeSetupResponsealigns well with the PR objective of implementing token refresh functionality. These properties provide the necessary token formats for proper token validation and refresh operations.Consider documenting the following token handling best practices:
- Use
*Rawproperties only for token validation and refresh operations- Use
*Encodedproperties for transmission- Never expose raw tokens in logs or client-side storage
Also applies to: 356-357
src/session/getAccessTokenWithRefresh.ts (3)
13-19: Optimize sessionManager calls by reusing the session instanceYou are calling
await sessionManager(req, res)multiple times to retrieve session items. This can be optimized by storing the session instance in a variable to reduce redundant calls and improve performance.Apply this diff to store the session instance:
+const session = await sessionManager(req, res); const accessToken = (await ( - await sessionManager(req, res) + session ).getSessionItem('access_token')) as string | null; const refreshToken = (await ( - await sessionManager(req, res) + session ).getSessionItem('refresh_token')) as string | null;
29-33: Simplify error handling for expired access tokens without refresh tokensThe check for
!validToken && !refreshTokenmay be unnecessary if you've already handled the case whereaccessTokenis missing. Ensure that your error handling logic is consistent and doesn't duplicate checks.Consider revising the condition to streamline the error handling:
-if (!validToken && !refreshToken) { +if (!validToken) { throw new Error( 'The access token expired and a refresh token is not available. The user will need to sign in again.' ); }
35-39: Clarify error message when force refresh is requested without a refresh tokenThe error message could be more specific. Indicate that the refresh token is missing even though a force refresh was requested.
Refine the error message as follows:
throw new Error( - 'A refresh token is required to refresh the access token, but none is present.' + 'Cannot force refresh access token without a refresh token present.' );src/frontend/AuthProvider.tsx (1)
184-190: Add Cleanup Function touseEffectHookThe
useEffecthook should return a cleanup function to clear the timer when the component unmounts, preventing potential memory leaks.Apply this diff to add a cleanup function:
useEffect((): void => { if (state.accessToken) return; (async (): Promise<void> => { await checkSession(); setState((previous) => ({...previous, isLoading: false})); })(); + return () => { + if (timer) clearTimeout(timer); + }; }, [state.accessToken]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tsconfig.jsonis excluded by!**/*.json
📒 Files selected for processing (12)
src/frontend/AuthProvider.jsx(0 hunks)src/frontend/AuthProvider.tsx(1 hunks)src/frontend/KindeBrowserClient.js(1 hunks)src/frontend/index.js(0 hunks)src/frontend/index.ts(1 hunks)src/handlers/setup.ts(2 hunks)src/index.js(1 hunks)src/session/getAccessTokenWithRefresh.ts(1 hunks)src/session/index.ts(1 hunks)src/session/isAuthenticated.js(1 hunks)src/utils/isTokenValid.ts(2 hunks)types.d.ts(2 hunks)
💤 Files with no reviewable changes (2)
- src/frontend/AuthProvider.jsx
- src/frontend/index.js
✅ Files skipped from review due to trivial changes (2)
- src/frontend/index.ts
- src/index.js
🔇 Additional comments (8)
src/utils/isTokenValid.ts (1)
2-2: Verify the config import path
The import path has been updated. Let's ensure this path is correct and consistent across the codebase.
✅ Verification successful
Import path is correct and consistent
The import path '../config/index' is correctly used across multiple files in the codebase, including:
src/session/filessrc/utils/isTokenValid.tssrc/handlers/filessrc/frontend/AuthProvider.tsxsrc/authMiddleware/authMiddleware.ts
The config module exists at src/config/index.js and all imports follow the same relative path pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the config import path exists and is used consistently
# Check if the config file exists at the new location
fd --type f "index.ts" --exec sh -c 'echo "Found config at: {}"; head -n 1 {}'
# Check for any other imports of the config module
rg --type ts --type jsx "from ['\"].*config/index['\"]" --no-filename
Length of output: 377
Script:
#!/bin/bash
# Let's try a different approach to verify the config import and its usage
# First, let's find the config directory and its contents
fd config --type d
fd --type f --full-path "./src/config"
# Then check for any imports of config across the codebase with ripgrep
# Using a more flexible pattern without the jsx type
rg --type ts "from ['\"].*config['\"]"
# Also check if there are any direct references to config
rg --type ts "import.*config"
Length of output: 844
src/session/index.ts (2)
1-20: LGTM! Import statements are well-organized
The new imports for token-related factories align well with the PR's objective of implementing token refresh functionality. The imports follow a consistent pattern and are properly typed.
Line range hint 38-58: LGTM! Factory methods are well-organized
The returned object provides a comprehensive set of authentication and authorization utilities. The new methods (getAccessTokenRaw, getClaim, getIdTokenRaw, getRoles) align well with the token refresh implementation.
src/handlers/setup.ts (1)
2-11: LGTM! Type safety improvements
The addition of explicit types and the RouterClient import enhance type safety and maintainability.
src/frontend/KindeBrowserClient.js (1)
2-2: Verify import resolution across different environments
The import path './AuthProvider' without an extension might need explicit extension handling in certain bundler configurations.
✅ Verification successful
Import path is correctly configured
The import of flagDataTypeMap from './AuthProvider' is valid as:
AuthProvider.tsxexists in the same directoryflagDataTypeMapis properly exported and used within the file- Modern bundlers can resolve TypeScript files without explicit extensions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if both files exist and if import would resolve correctly
fd -e ts -e js -e jsx -e tsx "AuthProvider" --exec echo "Found: {}"
Length of output: 106
Script:
#!/bin/bash
# Check if flagDataTypeMap is exported from AuthProvider.tsx
ast-grep --pattern 'export $_ flagDataTypeMap'
Length of output: 48
Script:
#!/bin/bash
# Let's try a different pattern to find flagDataTypeMap in AuthProvider.tsx
rg "flagDataTypeMap" ./src/frontend/AuthProvider.tsx -A 2
Length of output: 411
types.d.ts (1)
201-207: Breaking Change: Organization property names have been simplified.
The removal of the 'org_' prefix from organization properties is a breaking change that could affect existing implementations.
Let's verify the impact and alignment with the API:
src/session/getAccessTokenWithRefresh.ts (1)
43-45: Ensure accurate token expiration check
When checking if the token is expired, multiplying decodedToken.exp by 1000 assumes that exp is in seconds. Confirm that exp is in the correct unit (seconds since the epoch). If exp is already in milliseconds, the multiplication is unnecessary.
Run the following script to verify the unit of exp in tokens:
✅ Verification successful
Multiplication by 1000 is necessary - token expiration is in seconds
The verification confirms that the multiplication by 1000 is correct and necessary. Evidence from the codebase:
- In
src/utils/isTokenValid.ts, the token validation comparesaccessTokenPayload.expdirectly withMath.floor(Date.now() / 1000), confirming thatexpis in seconds - The current implementation in
getAccessTokenWithRefresh.tscorrectly multipliesdecodedToken.expby 1000 to convert it to milliseconds before comparing withDate.now() - This pattern is consistently used across the codebase, including in
AuthProvider.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if 'exp' in the JWT token is in seconds or milliseconds.
# Decode a sample token to check the 'exp' field.
# Replace 'YOUR_ACCESS_TOKEN' with a valid token.
echo 'YOUR_ACCESS_TOKEN' | cut -d '.' -f2 | base64 --decode | jq '.exp'
# Expected: 'exp' should be a Unix timestamp in seconds.
Length of output: 141
Script:
#!/bin/bash
# Let's search for any test files or documentation that might indicate the token format
rg -i "jwt.*exp|token.*exp" -A 3 -B 3
# Also search for any token decoding or validation logic
ast-grep --pattern 'decodedToken.exp'
Length of output: 3272
src/frontend/AuthProvider.tsx (1)
50-50:
Props Destructuring Incomplete in KindeProvider
The KindeProvider component destructures only children from props. According to the JSDoc comment, it should also accept options.
Apply this diff to include options in the props:
- export const KindeProvider = ({children}) => {
+ export const KindeProvider = ({children, options}) => {Likely invalid or redundant comment.
| const accessToken = await getAccessTokenWithRefresh( | ||
| routerClient.req, | ||
| routerClient.res | ||
| ); | ||
|
|
||
| const accessTokenEncoded = | ||
| await routerClient.sessionManager.getSessionItem('access_token'); | ||
| (await routerClient.sessionManager.getSessionItem( | ||
| 'access_token' | ||
| )) as string; | ||
|
|
||
| const idTokenEncoded = | ||
| await routerClient.sessionManager.getSessionItem('id_token'); | ||
|
|
||
| const accessToken = jwtDecoder<KindeAccessToken>(accessTokenEncoded); | ||
| const idTokenEncoded = (await routerClient.sessionManager.getSessionItem( | ||
| 'id_token' | ||
| )) as string; |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Type assertions are handled safely in most places except the setup handler
The codebase shows a consistent pattern for token handling:
- In
getAccessTokenWithRefresh.ts, tokens are properly typed asstring | null - In
callback.ts, there's proper null checking with an if statement - However, the
setup.tshandler is the only place where unsafe type assertions are used without null checks
The review comment's suggestion for adding null checks is valid and necessary for maintaining consistency with the rest of the codebase's safer type handling patterns.
🔗 Analysis chain
Consider adding null checks before type assertions
The type assertions for session items assume the values will always be strings. This could lead to runtime errors if the session items are undefined or null.
Consider adding null checks and proper error handling:
- const accessTokenEncoded =
- (await routerClient.sessionManager.getSessionItem(
- 'access_token'
- )) as string;
+ const accessTokenEncoded = await routerClient.sessionManager.getSessionItem(
+ 'access_token'
+ );
+ if (!accessTokenEncoded) {
+ throw new Error('Access token not found in session');
+ }
- const idTokenEncoded = (await routerClient.sessionManager.getSessionItem(
- 'id_token'
- )) as string;
+ const idTokenEncoded = await routerClient.sessionManager.getSessionItem(
+ 'id_token'
+ );
+ if (!idTokenEncoded) {
+ throw new Error('ID token not found in session');
+ }Let's verify the token retrieval implementation across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar token retrieval patterns that might need the same safety checks
ast-grep --pattern 'await $_.sessionManager.getSessionItem($_) as string'
Length of output: 75
Script:
#!/bin/bash
# Search for all sessionManager.getSessionItem usages to analyze token retrieval patterns
rg "sessionManager\.getSessionItem" -A 2
# Also search for type assertions with 'as string' to find similar patterns
rg "as string" -A 2
Length of output: 2631
| if (!validToken) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Evaluate necessity of returning null when the token is invalid
Returning null when the token is invalid may lead to unhandled cases upstream. Consider throwing an error or handling this scenario to avoid unexpected null values.
Modify the code to handle invalid tokens explicitly:
if (!validToken) {
- return null;
+ throw new Error('The access token is invalid. User may need to sign in again.');
}📝 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.
| if (!validToken) { | |
| return null; | |
| } | |
| if (!validToken) { | |
| throw new Error('The access token is invalid. User may need to sign in again.'); | |
| } |
| */ | ||
| export const KindeProvider = ({children}) => { | ||
| const setupUrl = `${config.apiPath}/setup`; | ||
| const [timer, setTimer] = useState<NodeJS.Timeout | null>(null); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Incorrect Timer Type in useState Hook
The setTimeout function in the browser environment returns a number, not a NodeJS.Timeout. Using NodeJS.Timeout is appropriate for Node.js but not for browser-based React applications.
Apply this diff to correct the timer type:
- const [timer, setTimer] = useState<NodeJS.Timeout | null>(null);
+ const [timer, setTimer] = useState<number | null>(null);📝 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 [timer, setTimer] = useState<NodeJS.Timeout | null>(null); | |
| const [timer, setTimer] = useState<number | null>(null); |
There was a problem hiding this comment.
But typescript doesn't like it...
There was a problem hiding this comment.
Understood. Since TypeScript is complaining about using number, a better approach is to use ReturnType<typeof setTimeout> for the timer type. This ensures compatibility across both Node.js and browser environments.
Here's the updated code:
const [timer, setTimer] = useState<ReturnType<typeof setTimeout> | null>(null);This should resolve the TypeScript errors.
Let me know if this works!
✏️ Learnings added
Learnt from: peterphanouvong
PR: kinde-oss/kinde-auth-nextjs#242
File: src/frontend/AuthProvider.tsx:52-52
Timestamp: 2024-11-14T01:06:30.863Z
Learning: When using `setTimeout` in React with TypeScript, prefer `ReturnType<typeof setTimeout>` as the timer type in `useState` to ensure compatibility in both Node.js and browser environments.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
src/frontend/AuthProvider.tsx
Outdated
| const t = setTimeout( | ||
| checkSession, | ||
| kindeData?.accessToken.exp * 1000 - Date.now() | ||
| ); | ||
| setTimer(t); | ||
| } catch (error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle Potential Negative Timeout Delay
The calculation for the timeout delay may result in a negative value if the access token has already expired, causing unintended behavior. You should check if the delay is positive before setting the timeout.
Apply this diff to ensure the delay is positive:
const delay = kindeData?.accessToken.exp * 1000 - Date.now();
+ if (delay > 0) {
const t = setTimeout(
checkSession,
- kindeData?.accessToken.exp * 1000 - Date.now()
+ delay
);
setTimer(t);
+ } else {
+ // Token has expired, handle accordingly
+ await checkSession();
+ }📝 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 t = setTimeout( | |
| checkSession, | |
| kindeData?.accessToken.exp * 1000 - Date.now() | |
| ); | |
| setTimer(t); | |
| } catch (error) { | |
| const delay = kindeData?.accessToken.exp * 1000 - Date.now(); | |
| if (delay > 0) { | |
| const t = setTimeout( | |
| checkSession, | |
| delay | |
| ); | |
| setTimer(t); | |
| } else { | |
| // Token has expired, handle accordingly | |
| await checkSession(); | |
| } | |
| } catch (error) { |
| getClaim: (claim: string, tokenKey = 'access_token') => { | ||
| const token = | ||
| tokenKey === 'access_token' | ||
| ? kindeData.accessToken | ||
| : kindeData.idToken; | ||
| return token ? {name: claim, value: token[claim]} : null; |
There was a problem hiding this comment.
Accessing Claims from Encoded Tokens
The getClaim function attempts to access claims directly from the token, but accessToken and idToken are typically encoded strings (JWTs), not decoded objects.
Apply this diff to decode the tokens before accessing claims:
+ import jwt_decode from 'jwt-decode';
getClaim: (claim: string, tokenKey = 'access_token') => {
const token =
tokenKey === 'access_token'
? kindeData.accessTokenRaw
: kindeData.idTokenRaw;
+ const decodedToken = token ? jwt_decode(token) : null;
- return token ? {name: claim, value: token[claim]} : null;
+ return decodedToken ? {name: claim, value: decodedToken[claim]} : null;
},Committable suggestion skipped: line range outside the PR's diff.
| getPermission: (key: string) => { | ||
| return { | ||
| isGranted: kindeData.permissions.permissions.some((p) => p === key), | ||
| orgCode: kindeData.organization.orgCode | ||
| }; |
There was a problem hiding this comment.
Add Null Checks for permissions and organization
In the getPermission function, kindeData.permissions and kindeData.organization may be undefined, leading to runtime errors when accessing their properties.
Apply this diff to add null checks:
getPermission: (key: string) => {
+ if (!kindeData.permissions || !kindeData.organization) {
+ throw new Error('Permissions or organization data is not available');
+ }
return {
- isGranted: kindeData.permissions.permissions.some((p) => p === key),
+ isGranted: kindeData.permissions.permissions.some((p) => p === key),
orgCode: kindeData.organization.orgCode
};
},Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/api-client.js (1)
Line range hint
33-43: Add error handling for token fetch failures.The token fetch operation lacks error handling, which could lead to unclear errors if the authentication server is unavailable or returns an error.
Add proper error handling:
- const response = await fetch(`${config.issuerURL}/oauth2/token`, { - method: 'POST', - headers: { - 'content-type': 'application/x-www-form-urlencoded' - }, - body: new URLSearchParams({ - grant_type: 'client_credentials', - client_id: config.clientID || '', - client_secret: config.clientSecret || '', - audience: config.issuerURL + '/api' - }) - }); - apiToken = (await response.json()).access_token; + try { + const response = await fetch(`${config.issuerURL}/oauth2/token`, { + method: 'POST', + headers: { + 'content-type': 'application/x-www-form-urlencoded' + }, + body: new URLSearchParams({ + grant_type: 'client_credentials', + client_id: config.clientID || '', + client_secret: config.clientSecret || '', + audience: config.issuerURL + '/api' + }) + }); + + if (!response.ok) { + throw new Error(`Failed to fetch token: ${response.status} ${response.statusText}`); + } + + const data = await response.json(); + if (!data.access_token) { + throw new Error('Token response missing access_token'); + } + + apiToken = data.access_token; + } catch (error) { + throw new Error(`Failed to initialize Kinde API client: ${error.message}`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/api-client.js(1 hunks)src/authMiddleware/authMiddleware.ts(1 hunks)src/frontend/AuthProvider.tsx(1 hunks)src/utils/isTokenValid.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/authMiddleware/authMiddleware.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/isTokenValid.ts
🔇 Additional comments (3)
src/api-client.js (1)
Line range hint 1-91: Verify token refresh implementation across the application.
Given that this PR's objective is to implement token refresh functionality, we should verify how this management API client integrates with the broader token refresh mechanism.
src/frontend/AuthProvider.tsx (2)
147-152:
Add null checks in getPermission
The function assumes permissions and organization are always available, which could lead to runtime errors.
Apply this diff:
getPermission: (key: string) => {
+ if (!kindeData?.permissions?.permissions || !kindeData?.organization?.orgCode) {
+ return {
+ isGranted: false,
+ orgCode: null
+ };
+ }
return {
isGranted: kindeData.permissions.permissions.some((p) => p === key),
orgCode: kindeData.organization.orgCode
};
},Likely invalid or redundant comment.
186-192:
Add cleanup in useEffect
The effect should clean up the timer when the component unmounts to prevent memory leaks and potential errors.
Apply this diff:
useEffect((): void => {
if (state.accessToken) return;
(async (): Promise<void> => {
await checkSession();
setState((previous) => ({...previous, isLoading: false}));
})();
+ return () => {
+ if (timer) {
+ clearTimeout(timer);
+ }
+ };
- }, [state.accessToken]);
+ }, [state.accessToken, timer]);Likely invalid or redundant comment.
| @@ -30,7 +29,7 @@ export const createKindeManagementAPIClient = async (req, res) => { | |||
|
|
|||
| const store = await sessionManager(req, res); | |||
| store.removeSessionItem('kinde_api_access_token'); | |||
|
|
|||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider caching valid tokens to prevent unnecessary token requests.
The current implementation removes and fetches a new token on every API client creation, which could lead to:
- Unnecessary load on the authentication server
- Potential rate limiting issues
- Increased latency for API requests
Consider implementing token caching:
export const createKindeManagementAPIClient = async (req, res) => {
let apiToken = null;
const store = await sessionManager(req, res);
- store.removeSessionItem('kinde_api_access_token');
+ apiToken = store.getSessionItem('kinde_api_access_token');
+
+ if (!apiToken) {
const response = await fetch(`${config.issuerURL}/oauth2/token`, {
// ... existing token fetch code ...
});
apiToken = (await response.json()).access_token;
+ store.setSessionItem('kinde_api_access_token', apiToken);
+ }Committable suggestion skipped: line range outside the PR's diff.
src/frontend/AuthProvider.tsx
Outdated
| const delay = kindeData?.accessToken.exp * 1000 - Date.now(); | ||
| if (delay > 0) { | ||
| const t = setTimeout(checkSession, delay); | ||
| setTimer(t); | ||
| } else { | ||
| await checkSession(); | ||
| } |
There was a problem hiding this comment.
Prevent concurrent token refresh attempts
The current implementation could lead to race conditions if multiple refresh attempts occur simultaneously. Additionally, as noted in past reviews, the timer type should be number for browser environments.
Apply this diff to improve the implementation:
+ const [isRefreshing, setIsRefreshing] = useState(false);
// ... in checkSession function
+ if (isRefreshing) return;
+ setIsRefreshing(true);
try {
const delay = kindeData?.accessToken.exp * 1000 - Date.now();
if (delay > 0) {
const t = setTimeout(checkSession, delay);
setTimer(t);
} else {
await checkSession();
}
+ } finally {
+ setIsRefreshing(false);
}Also update the timer type:
- const [timer, setTimer] = useState<NodeJS.Timeout | null>(null);
+ const [timer, setTimer] = useState<number | null>(null);Committable suggestion skipped: line range outside the PR's diff.
| const tokenFetcher = async ( | ||
| url: string | ||
| ): Promise<KindeSetupResponse | undefined> => { | ||
| let response: Response; | ||
| try { | ||
| response = await fetch(url); | ||
| } catch { | ||
| throw new Error('Network error'); | ||
| } | ||
| if (response.status == 204) return undefined; | ||
| if (response.ok) return response.json(); | ||
| throw new Error('Error fetching data'); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in tokenFetcher
- Missing
awaitonresponse.json()as noted in past review - Network error message could be more descriptive
- Error handling could be more specific
Apply this diff to improve the implementation:
const tokenFetcher = async (
url: string
): Promise<KindeSetupResponse | undefined> => {
let response: Response;
try {
response = await fetch(url);
} catch (error) {
- throw new Error('Network error');
+ throw new Error(`Failed to fetch from ${url}: ${error.message}`);
}
if (response.status == 204) return undefined;
- if (response.ok) return response.json();
+ if (response.ok) return await response.json();
+ throw new Error(`HTTP ${response.status}: ${response.statusText}`);
- throw new Error('Error fetching data');
};📝 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 tokenFetcher = async ( | |
| url: string | |
| ): Promise<KindeSetupResponse | undefined> => { | |
| let response: Response; | |
| try { | |
| response = await fetch(url); | |
| } catch { | |
| throw new Error('Network error'); | |
| } | |
| if (response.status == 204) return undefined; | |
| if (response.ok) return response.json(); | |
| throw new Error('Error fetching data'); | |
| }; | |
| const tokenFetcher = async ( | |
| url: string | |
| ): Promise<KindeSetupResponse | undefined> => { | |
| let response: Response; | |
| try { | |
| response = await fetch(url); | |
| } catch (error) { | |
| throw new Error(`Failed to fetch from ${url}: ${error.message}`); | |
| } | |
| if (response.status == 204) return undefined; | |
| if (response.ok) return await response.json(); | |
| throw new Error(`HTTP ${response.status}: ${response.statusText}`); | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/handlers/setup.ts (1)
Line range hint
74-109: Consider breaking down the response constructionThe response construction is complex and could benefit from being broken down into smaller, more focused functions for better maintainability and testability.
Consider extracting the organization properties mapping and user organizations mapping into separate utility functions:
function mapOrganizationProperties(orgProperties: KindeTokenOrganizationProperties) { return { city: orgProperties?.kp_org_city?.v, industry: orgProperties?.kp_org_industry?.v, postcode: orgProperties?.kp_org_postcode?.v, state_region: orgProperties?.kp_org_state_region?.v, street_address: orgProperties?.kp_org_street_address?.v, street_address_2: orgProperties?.kp_org_street_address_2?.v }; } function mapUserOrganizations(orgCodes: string[], orgNames: Array<{id: string; name: string}>) { return { orgCodes, orgs: orgNames?.map((org) => ({ code: org?.id, name: org?.name })) }; }src/frontend/AuthProvider.tsx (2)
64-68: Improve type safety in getFlag function parametersThe
defaultValue: anyparameter reduces type safety. Consider using a generic type parameter that's constrained by the flag types.- defaultValue: any, + defaultValue: string | number | boolean, flagType: 'b' | 'i' | 's'
116-145: Improve error handling in flag gettersThe current implementation silently fails in production and has duplicated error handling code. Consider:
- Extracting common error handling logic
- Providing a way to handle errors in production
+ const handleFlagError = (err: Error, defaultValue: any) => { + if (config.isDebugMode) { + console.error(err); + } + return defaultValue; + }; getBooleanFlag: (code: string, defaultValue: boolean) => { try { const flag = getFlag(code, defaultValue, 'b'); return flag.value; } catch (err) { - if (config.isDebugMode) { - console.error(err); - } + return handleFlagError(err, defaultValue); } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/frontend/AuthProvider.tsx(1 hunks)src/handlers/setup.ts(2 hunks)
🔇 Additional comments (3)
src/handlers/setup.ts (2)
27-33:
Add null checks for session items
The type assertions for session items are still unsafe. While the code has been updated to use getAccessTokenWithRefresh, the session item retrievals need proper null checks.
The previous review comment's suggestion for adding null checks is still valid. Please refer to the existing review comment for the implementation details.
37-72:
Add validation for claim values
Multiple claims are retrieved using type assertions without proper validation. This pattern is repeated across permissions, organization, featureFlags, and other claims.
The previous review comment's suggestion for adding a validation utility is still valid. Please refer to the existing review comment for the implementation details.
src/frontend/AuthProvider.tsx (1)
1-17: LGTM! Clean imports and type definitions
The imports are well-organized and the flag data type mapping is implemented effectively.
| useEffect((): void => { | ||
| if (state.accessToken) return; | ||
| (async (): Promise<void> => { | ||
| await checkSession(); | ||
| setState((previous) => ({...previous, isLoading: false})); | ||
| })(); | ||
| }, [state.accessToken]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup handler to prevent memory leaks
The useEffect hook should include a cleanup function to handle component unmounting during the authentication check.
useEffect((): void => {
if (state.accessToken) return;
+ let mounted = true;
(async (): Promise<void> => {
await checkSession();
+ if (mounted) {
setState((previous) => ({...previous, isLoading: false}));
+ }
})();
+ return () => {
+ mounted = false;
+ };
}, [state.accessToken]);📝 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.
| useEffect((): void => { | |
| if (state.accessToken) return; | |
| (async (): Promise<void> => { | |
| await checkSession(); | |
| setState((previous) => ({...previous, isLoading: false})); | |
| })(); | |
| }, [state.accessToken]); | |
| useEffect((): void => { | |
| if (state.accessToken) return; | |
| let mounted = true; | |
| (async (): Promise<void> => { | |
| await checkSession(); | |
| if (mounted) { | |
| setState((previous) => ({...previous, isLoading: false})); | |
| } | |
| })(); | |
| return () => { | |
| mounted = false; | |
| }; | |
| }, [state.accessToken]); |
| const tokenExpiresIn = kindeData?.accessToken.exp * 1000 - Date.now(); | ||
| if (tokenExpiresIn > 0) { | ||
| const t = setTimeout(checkSession, tokenExpiresIn); | ||
| setTimer(t); | ||
| } else { | ||
| await checkSession(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add request deduplication for token refresh
The current implementation could trigger multiple concurrent refresh attempts if checkSession is called multiple times near the token expiration. Consider implementing a request deduplication mechanism.
+ const [isRefreshing, setIsRefreshing] = useState(false);
const checkSession = useCallback(async (): Promise<void> => {
try {
+ if (isRefreshing) return;
+ setIsRefreshing(true);
// ... existing code ...
} finally {
+ setIsRefreshing(false);
}
}, [setupUrl]);Committable suggestion skipped: line range outside the PR's diff.
Explain your changes
resolves: #237
Checklist