Hotfix/token api break #2320
Hotfix/token api break #2320
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes introduce a new state variable Changes
Suggested labels
Suggested reviewers
Poem
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
npm warn config production Use 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Outside diff range and nitpick comments (4)
src/features/common/Layout/UserPropsContext.tsx (4)
Line range hint
116-118: Enhance error handling for 403 statusThe current implementation only removes impersonation data on 403 status without any user feedback or recovery action.
Consider this enhancement:
} else if (res.status === 403) { localStorage.removeItem('impersonationData'); + setIsImpersonationModeOn(false); + // Notify user about the permission issue + throw new Error('Permission denied. Impersonation session ended.'); } else {
Line range hint
119-122: Implement proper error handling for the catch-all else caseThe current implementation silently ignores other error cases.
Consider this enhancement:
} else { - //any other error + // Handle unexpected errors + setUser(null); + setToken(null); + throw new Error(`Unexpected error occurred: ${res.status}`); }
Line range hint
66-122: Consider implementing a centralized error handling strategyWhile the current changes address the immediate token API issues, consider implementing a more comprehensive error handling strategy:
- Create a centralized error handling utility for consistent error management
- Implement proper error tracking/monitoring
- Add user-friendly error messages and recovery flows
- Consider adding retry mechanisms for transient failures
Would you like assistance in designing a centralized error handling solution?
Error handling needed for impersonation API response
The
getAccountInfofunction inapi.tscurrently returns the raw response without proper error handling, while other authenticated requests in the same file properly handle various error scenarios. Specifically:
- The function should handle API errors using the
APIErrorclass like other authenticated requests- Token validation and logout handling should be consistent with other authenticated endpoints
- The impersonation-specific error cases should be properly propagated
Key locations that need attention:
src/utils/apiRequests/api.ts: Add error handling ingetAccountInfosimilar togetAuthenticatedRequestsrc/features/user/Settings/ImpersonateUser/ImpersonateUserForm.tsx: Improve error handling inhandleImpersonationto handle specific API error cases🔗 Analysis chain
Line range hint
91-93: Address the TODO comment regarding error handling for impersonationThe comment indicates missing error handling for the impersonated email scenario in
getAccountInfo.Let's check the implementation of
getAccountInfo:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for getAccountInfo implementation to understand error handling needs rg -A 10 "export const getAccountInfo"Length of output: 40
Script:
#!/bin/bash # Let's try different search patterns to locate getAccountInfo rg -l "getAccountInfo" # Also search for similar function names in case it was renamed rg -l "AccountInfo" # Search for API calls related to account information ast-grep --pattern 'fetch($$$"/api/account"$$$)'Length of output: 381
Script:
#!/bin/bash # Let's examine the implementation of getAccountInfo in the API utils cat src/utils/apiRequests/api.ts # Also check how it's used in the impersonation form cat src/features/user/Settings/ImpersonateUser/ImpersonateUserForm.tsxLength of output: 14940
| try { | ||
| const accessToken = await getAccessTokenSilently(); | ||
| setToken(accessToken); | ||
| } catch (error) { | ||
| console.error('Error fetching access token:', error); | ||
| loginWithRedirect({ | ||
| redirectUri: `${window.location.origin}/login`, | ||
| ui_locales: localStorage.getItem('language') || 'en', | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling robustness and security
While the try-catch block addresses the immediate token retrieval issue, there are several improvements needed:
- Avoid logging sensitive information to console in production
- Different error types (like
invalid_grantvs network errors) might need different handling - Consider implementing a redirect counter to prevent infinite loops
Consider this enhanced implementation:
try {
const accessToken = await getAccessTokenSilently();
setToken(accessToken);
} catch (error) {
- console.error('Error fetching access token:', error);
+ // Only log in development
+ if (process.env.NODE_ENV === 'development') {
+ console.error('Error fetching access token:', error);
+ }
+ // Check if we've already redirected to prevent loops
+ const redirectCount = parseInt(sessionStorage.getItem('auth_redirect_count') || '0');
+ if (redirectCount > 3) {
+ // Consider showing a user-friendly error message instead of redirecting
+ console.error('Multiple authentication failures');
+ return;
+ }
+ sessionStorage.setItem('auth_redirect_count', (redirectCount + 1).toString());
loginWithRedirect({
redirectUri: `${window.location.origin}/login`,
ui_locales: localStorage.getItem('language') || 'en',
});
}📝 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.
| try { | |
| const accessToken = await getAccessTokenSilently(); | |
| setToken(accessToken); | |
| } catch (error) { | |
| console.error('Error fetching access token:', error); | |
| loginWithRedirect({ | |
| redirectUri: `${window.location.origin}/login`, | |
| ui_locales: localStorage.getItem('language') || 'en', | |
| }); | |
| } | |
| try { | |
| const accessToken = await getAccessTokenSilently(); | |
| setToken(accessToken); | |
| } catch (error) { | |
| // Only log in development | |
| if (process.env.NODE_ENV === 'development') { | |
| console.error('Error fetching access token:', error); | |
| } | |
| // Check if we've already redirected to prevent loops | |
| const redirectCount = parseInt(sessionStorage.getItem('auth_redirect_count') || '0'); | |
| if (redirectCount > 3) { | |
| // Consider showing a user-friendly error message instead of redirecting | |
| console.error('Multiple authentication failures'); | |
| return; | |
| } | |
| sessionStorage.setItem('auth_redirect_count', (redirectCount + 1).toString()); | |
| loginWithRedirect({ | |
| redirectUri: `${window.location.origin}/login`, | |
| ui_locales: localStorage.getItem('language') || 'en', | |
| }); | |
| } |
- log errors only in development to prevent exposing sensitive information in production. - Implement redirect attempt counter to limit the number of redirects and prevent infinite loops.
Fixed the invalid_grant (403 Forbidden) error by implementing a
try-catchblock in the useEffect for token retrieval.If an error occurs while fetching the access token, the user is redirected to the login page to resolve the invalid or expired
refresh token.