Move redirect check to protected route file in analytics platform #2530
Conversation
📝 WalkthroughWalkthroughThe changes enhance the authentication flow by improving the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
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
🧹 Nitpick comments (3)
src/platform/src/core/utils/protectedRoute.js (3)
5-6: Consider security implications of storing tokens.
ImportingCookiesandjwt_decodecan be convenient, but the subsequent use in local storage (lines 67–68) might expose tokens to XSS attacks. Consider using httpOnly cookies or a secure session storage mechanism.
23-33: Implement exponential backoff in retry logic.
Currently,retryWithDelayuses a fixed delay of 1 second. A growing or exponential backoff is often more robust against ongoing 429 rate-limit issues.- const RETRY_DELAY = 1000; ... - await new Promise((resolve) => setTimeout(resolve, RETRY_DELAY)); + const RETRY_BASE_DELAY = 1000; ... + const attempt = MAX_RETRIES - retries + 1; + await new Promise((resolve) => + setTimeout(resolve, RETRY_BASE_DELAY * 2 ** (attempt - 1)) + );
86-86: Review dependency array to avoid unexpected useEffect triggers.
retryWithDelayin the dependency array can trigger re-renders whenever it changes (even if functionally stable). Consider memoizing or extracting it outside the component if unnecessary for re-renders.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/platform/src/core/utils/protectedRoute.js(2 hunks)src/platform/src/pages/Home/index.jsx(0 hunks)
💤 Files with no reviewable changes (1)
- src/platform/src/pages/Home/index.jsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-push-deploy-analytics-platform-preview
🔇 Additional comments (1)
src/platform/src/core/utils/protectedRoute.js (1)
63-76: Ensure consistent handling of Google authentication flow.
This flow retrieves tokens, decodes them, and sets session details. Confirm that downstream logic (e.g., UI or data calls) does not break if the Google auth cookie is missing or expired.
| const setupUserSession = async (user) => { | ||
| if (!user.groups[0]?.grp_title) { | ||
| throw new Error( | ||
| 'Server error. Contact support to add you to the AirQo Organisation', | ||
| ); | ||
| } | ||
|
|
||
| localStorage.setItem('loggedUser', JSON.stringify(user)); | ||
|
|
||
| const preferencesResponse = await retryWithDelay(() => | ||
| dispatch(getIndividualUserPreferences({ identifier: user._id })), | ||
| ); | ||
| if (preferencesResponse.payload.success) { | ||
| const preferences = preferencesResponse.payload.preferences; | ||
| const activeGroup = preferences[0]?.group_id | ||
| ? user.groups.find((group) => group._id === preferences[0].group_id) | ||
| : user.groups.find((group) => group.grp_title === 'airqo'); | ||
| localStorage.setItem('activeGroup', JSON.stringify(activeGroup)); | ||
| } | ||
|
|
||
| dispatch(setUserInfo(user)); | ||
| dispatch(setSuccess(true)); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle preference-fetch failures and validate group selection.
When getIndividualUserPreferences fails or returns no preferences, the user session lacks a fallback group. Consider providing a default group or clearly warning the user. Additionally, storing the entire user object in local storage (line 42) may risk exposing sensitive data.
|
New next-platform changes available for preview here |
Codebmk
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/platform/src/core/utils/protectedRoute.js (1)
39-61: 🛠️ Refactor suggestionHandle preference-fetch failures and validate group selection.
When
getIndividualUserPreferencesfails or returns no preferences, the user session lacks a fallback group. Consider providing a default group or clearly warning the user. Additionally, storing the entire user object in local storage (line 46) may risk exposing sensitive data.I recommend implementing a more robust fallback mechanism and filtering sensitive data:
const setupUserSession = async (user) => { if (!user.groups[0]?.grp_title) { throw new Error( 'Server error. Contact support to add you to the AirQo Organisation', ); } - localStorage.setItem('loggedUser', JSON.stringify(user)); + // Filter sensitive data before storing in localStorage + const safeUserData = { + _id: user._id, + name: user.name, + email: user.email, + groups: user.groups.map(g => ({ _id: g._id, grp_title: g.grp_title })) + }; + localStorage.setItem('loggedUser', JSON.stringify(safeUserData)); const preferencesResponse = await retryWithDelay(() => dispatch(getIndividualUserPreferences({ identifier: user._id })), ); - if (preferencesResponse.payload.success) { + let activeGroup = null; + try { + if (preferencesResponse.payload.success) { const preferences = preferencesResponse.payload.preferences; - const activeGroup = preferences[0]?.group_id + activeGroup = preferences[0]?.group_id ? user.groups.find((group) => group._id === preferences[0].group_id) : user.groups.find((group) => group.grp_title === 'airqo'); - localStorage.setItem('activeGroup', JSON.stringify(activeGroup)); - } + } + } catch (err) { + console.error('Error processing preferences:', err); + } + + // Ensure we have an active group, defaulting to the first one if needed + if (!activeGroup && user.groups.length > 0) { + activeGroup = user.groups[0]; + console.warn('No active group found in preferences, defaulting to:', activeGroup.grp_title); + } + + if (activeGroup) { + localStorage.setItem('activeGroup', JSON.stringify(activeGroup)); + } else { + throw new Error('No valid group found for user'); + } dispatch(setUserInfo(user)); dispatch(setSuccess(true)); };
🧹 Nitpick comments (3)
src/platform/src/core/utils/protectedRoute.js (3)
27-37: The retry mechanism is well-implemented but lacks logging.The
retryWithDelayfunction properly handles rate limiting (429) errors with exponential backoff, but consider adding logging before retries to help with debugging.const retryWithDelay = async (fn, retries = MAX_RETRIES) => { try { return await fn(); } catch (error) { if (retries > 0 && error.response?.status === 429) { + console.log(`Rate limited. Retrying in ${RETRY_DELAY}ms. Attempts remaining: ${retries - 1}`); await new Promise((resolve) => setTimeout(resolve, RETRY_DELAY)); return retryWithDelay(fn, retries - 1); } throw error; } };
94-94: Consider memoizing functions to optimize useEffect dependencies.Including
retryWithDelayin the dependency array could cause unnecessary re-renders as it's recreated on each render. Consider usinguseCallbackfor these functions.+const memoizedRetryWithDelay = React.useCallback( + async (fn, retries = MAX_RETRIES) => { + try { + return await fn(); + } catch (error) { + if (retries > 0 && error.response?.status === 429) { + await new Promise((resolve) => setTimeout(resolve, RETRY_DELAY)); + return memoizedRetryWithDelay(fn, retries - 1); + } + throw error; + } + }, + [] +); // Later in the useEffect dependencies array: -}, [userCredentials, dispatch, router, retryWithDelay, isRedirecting]); +}, [userCredentials, dispatch, router, memoizedRetryWithDelay, isRedirecting]);
66-94: Add timeout for Google authentication to prevent indefinite spinner.If the authentication or API calls hang, users might be stuck on the loading spinner. Consider adding a timeout mechanism.
if (router.query.success === 'google') { + // Set a timeout to prevent users from being stuck on the spinner + const timeoutId = setTimeout(() => { + if (isRedirecting) { + console.error('Google authentication timed out'); + setIsRedirecting(false); + router.push('/account/login'); + } + }, 15000); // 15 seconds timeout + const token = Cookies.get('access_token'); if (token) { localStorage.setItem('token', token); const decoded = jwt_decode(token); retryWithDelay(() => getUserDetails(decoded._id, token)) .then((response) => setupUserSession(response.users[0])) .catch((error) => { console.error('Google auth error:', error); setIsRedirecting(false); router.push('/account/login'); + clearTimeout(timeoutId); }); } else { setIsRedirecting(false); router.push('/account/login'); + clearTimeout(timeoutId); } return; // Exit early to prevent further checks until redirect is resolved }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/platform/src/core/utils/protectedRoute.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-push-deploy-analytics-platform-preview
🔇 Additional comments (5)
src/platform/src/core/utils/protectedRoute.js (5)
5-13: The imports align well with the updated authentication flow.The addition of authentication-related imports like
Cookies,jwt_decode, and Redux actions is consistent with moving the Google authentication logic from the Home component to this higher-order component.
15-16: Good use of constants for configuration values.Defining
MAX_RETRIESandRETRY_DELAYas constants improves maintainability and makes the retry behavior easily configurable.
23-25: Appropriate state tracking for Google authentication redirect.The
isRedirectingstate is initialized based on the URL query parameter, which is a clean approach to detecting and handling Google authentication redirects.
66-83: Google authentication flow is well-structured.The Google redirect handling is organized logically with proper error handling and redirection. The early return prevents further execution during redirection, which is a good practice.
97-99: Good UX handling during redirection.Showing a spinner while processing the Google authentication redirect provides appropriate visual feedback to users.
|
New next-platform changes available for preview here |
This pull request focuses on refactoring and improving the authentication logic, particularly around handling Google redirects and user session setup. The main changes include moving the authentication logic from
src/platform/src/pages/Home/index.jsxto a higher-order component insrc/platform/src/core/utils/protectedRoute.js, and removing redundant code from theHomecomponent.Authentication Refactoring:
src/platform/src/core/utils/protectedRoute.js: Introduced new functionsretryWithDelayandsetupUserSessionto handle retries and user session setup, respectively. Added logic to handle Google redirects and set up user sessions. [1] [2]Code Cleanup:
src/platform/src/pages/Home/index.jsx: Removed the authentication-related logic and redundant imports, includingCookies,jwt_decode,useRouter, and session setup functions. This simplifies theHomecomponent by delegating authentication responsibilities to the higher-order component. [1] [2] [3]Summary by CodeRabbit