Feature : Added server side verification for all routes . #3491
Conversation
WalkthroughThis pull request introduces a new GraphQL query, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
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
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
PurnenduMIshra129th
changed the title
![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 (2)
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx (1)
30-46: Avoid loose equality checks for roles.
Use triple-equals comparisons (===) to minimize type-coercion risks and maintain consistency.- if (role == 'user') { + if (role === 'user') {src/components/SecuredRoute/SecuredRoute.tsx (1)
19-25: Gracefully handle a missing or invalid token.
Similar toSecuredRouteForUser, ensure you handle the case whengetItem('token')returns null or an empty string to avoid potential 401/403 backend errors.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/docs/auto-docs/GraphQl/Queries/Queries/variables/VERIFY_ROLE.md(1 hunks)docs/docs/auto-docs/components/SecuredRoute/SecuredRoute/functions/default.md(1 hunks)docs/docs/auto-docs/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser/functions/default.md(1 hunks)src/GraphQl/Queries/Queries.ts(1 hunks)src/components/SecuredRoute/SecuredRoute.tsx(2 hunks)src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- docs/docs/auto-docs/components/SecuredRoute/SecuredRoute/functions/default.md
- docs/docs/auto-docs/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser/functions/default.md
- docs/docs/auto-docs/GraphQl/Queries/Queries/variables/VERIFY_ROLE.md
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (6)
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx (3)
26-28: Check for potential infinite refetch loops.
UsinggetItem('token')directly inside the dependency array can cause re-renders if the returned value is a new object or if local storage changes frequently. Consider memoizing or storing the token in a state variable to avoid rapid re-fetch calls.
19-25: 🛠️ Refactor suggestionEnsure fallback behavior when no token is available.
IfgetItem('token')returnsnullor an empty value,Authorization: Bearer nullmay cause backend errors. You may want to gracefully handle missing tokens before passing the headers.+ const token = getItem('token'); const { data, loading, error, refetch } = useQuery(VERIFY_ROLE, { context: { headers: { - Authorization: `Bearer ${getItem('token')}`, + ...(token ? { Authorization: `Bearer ${token}` } : {}), }, }, });
1-2: Use consistent import paths and naming conventions.
Make sure that the import path'GraphQl/Queries/Queries'is spelled consistently across the codebase (GraphQlvs.GraphQL). Also, confirm that you keep a consistent naming pattern for these imports (e.g.import { VERIFY_ROLE } from 'src/GraphQl...').As a follow-up, you can search the codebase to confirm correct usage:
src/components/SecuredRoute/SecuredRoute.tsx (2)
1-2: Confirm consistent import usage and file casing.
Same as inSecuredRouteForUser, ensure'GraphQl/Queries/Queries'is spelled with consistent folder-case to avoid cross-platform issues.
26-28: Check for repeated refetch triggers.
RetrievinggetItem('token')in the dependency array can lead to frequent re-fetch if the return value changes unexpectedly. Consider using a state variable.src/GraphQl/Queries/Queries.ts (1)
867-874: Verify alignment with backend schema.
TheverifyRolequery fields (isAuthorizedandrole) must match exactly the backend’s resolver fields. Double-check the spelling and data types to prevent runtime errors.
|
@palisadoes cheack once this Pr. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3491 +/- ##
=======================================
Coverage 3.68% 3.69%
=======================================
Files 315 315
Lines 8301 8319 +18
Branches 1882 1887 +5
=======================================
+ Hits 306 307 +1
- Misses 7957 7974 +17
Partials 38 38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Please fix the failing tests, make code rabbit approve your PR and add tests to cover the code modifications |
PurnenduMIshra129th
force-pushed
the
feature/VerifyRoleRoute
branch
from
21478ec to
23ebe30
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.spec.tsx
Show resolved
Hide resolved
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.spec.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/components/SecuredRoute/SecuredRoute.tsx (1)
Line range hint
55-89: Refactor session timeout logic into a custom hook.The current implementation uses global variables and direct DOM manipulation, which are anti-patterns in React. Consider moving this logic into a custom hook.
// useSessionTimeout.ts export const useSessionTimeout = (timeoutMinutes: number, onTimeout: () => void) => { useEffect(() => { let lastActive = Date.now(); const timeoutMilliseconds = timeoutMinutes * 60 * 1000; const handleActivity = () => { lastActive = Date.now(); }; const checkInactivity = () => { const timeSinceLastActive = Date.now() - lastActive; if (timeSinceLastActive > timeoutMilliseconds) { onTimeout(); } }; window.addEventListener('mousemove', handleActivity); const interval = setInterval(checkInactivity, 60000); return () => { window.removeEventListener('mousemove', handleActivity); clearInterval(interval); }; }, [timeoutMinutes, onTimeout]); }; // Usage in SecuredRoute const SecuredRoute = (): JSX.Element => { useSessionTimeout(15, () => { toast.warn('Kindly relogin as session has expired'); setItem('IsLoggedIn', 'FALSE'); window.location.href = '/'; }); // ... rest of the component };
🧹 Nitpick comments (6)
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.spec.tsx (4)
14-17: Consider enhancing the mock setup with type safety.The mock setup is correct but could benefit from TypeScript types for better type safety and IDE support.
vi.mock('utils/useLocalstorage', () => ({ __esModule: true, - default: vi.fn(), + default: vi.fn<typeof useLocalStorage>(), }));
24-39: Enhance mockQuery helper with type safety and flexibility.The helper function could benefit from TypeScript types and a more flexible token parameter.
-const mockQuery = (role: string, isAuthorized: boolean): MockedResponse => ({ +type Role = 'user' | 'admin' | 'superadmin' | ''; +const mockQuery = (role: Role, isAuthorized: boolean, token = 'mock_token'): MockedResponse => ({ request: { query: VERIFY_ROLE, context: { - headers: { Authorization: `Bearer mock_token` }, + headers: { Authorization: `Bearer ${token}` }, }, }, result: { data: { verifyRole: { role, isAuthorized, }, }, }, });
41-69: Enhance test description and assertions for user role verification.The test case could be more explicit about what it's verifying and include additional assertions.
-it('renders the route when the user is logged in as a user', async () => { +it('should render organizations component when authenticated user has user role', async () => { // ... existing code ... await waitFor(() => { expect(screen.getByTestId('organizations-content')).toBeInTheDocument(); + expect(screen.queryByText(/404/i)).not.toBeInTheDocument(); + expect(screen.queryByText(/Error/i)).not.toBeInTheDocument(); }); });
152-188: Enhance error handling test coverage.The error handling test could be more comprehensive by testing different error scenarios.
Add test cases for different error scenarios:
it('should handle network errors', async () => { (useLocalStorage as Mock).mockReturnValue({ getItem: vi.fn(() => 'mock_token'), setItem: vi.fn(), }); render( <MockedProvider mocks={[ { request: { query: VERIFY_ROLE, context: { headers: { Authorization: `Bearer mock_token` }, }, }, error: new Error('Network error'), }, ]} addTypename={false} > <MemoryRouter initialEntries={['/user/organizations']}> <Routes> <Route element={<SecuredRouteForUser />}> <Route path="/user/organizations" element={<div>Test Page</div>} /> </Route> </Routes> </MemoryRouter> </MockedProvider>, ); await waitFor(() => { expect(screen.getByText(/Error During Routing/i)).toBeInTheDocument(); expect(screen.getByText(/Network error/i)).toBeInTheDocument(); }); }); it('should handle unauthorized responses', async () => { (useLocalStorage as Mock).mockReturnValue({ getItem: vi.fn(() => 'mock_token'), setItem: vi.fn(), }); render( <MockedProvider mocks={[ { request: { query: VERIFY_ROLE, context: { headers: { Authorization: `Bearer mock_token` }, }, }, result: { errors: [{ message: 'Unauthorized' }], }, }, ]} addTypename={false} > <MemoryRouter initialEntries={['/user/organizations']}> <Routes> <Route element={<SecuredRouteForUser />}> <Route path="/user/organizations" element={<div>Test Page</div>} /> </Route> </Routes> </MemoryRouter> </MockedProvider>, ); await waitFor(() => { expect(screen.getByText(/Error During Routing/i)).toBeInTheDocument(); expect(screen.getByText(/Unauthorized/i)).toBeInTheDocument(); }); });src/components/SecuredRoute/SecuredRoute.tsx (2)
31-35: Improve error handling and loading states.The current error handling shows a generic message. Consider:
- Logging the error for debugging
- Showing more specific error messages
- Adding a retry mechanism
if (loading) { - return <div> Loading.....</div>; + return <div>Verifying access...</div>; } else if (error) { + console.error('Route verification failed:', error); - return <div>Error During Routing ...</div>; + return ( + <div> + <p>Failed to verify access. Please try again.</p> + <button onClick={() => refetch()}>Retry</button> + </div> + ); }
40-42: Use strict equality comparison for role checks.Replace loose equality (==) with strict equality (===) for type-safe comparisons.
-if (role == 'superAdmin') { +if (role === 'superAdmin') { return <Outlet />; -} else if (role == 'admin') { +} else if (role === 'admin') {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/docs/auto-docs/screens/MemberDetail/MemberDetail/functions/getLanguageName.md(1 hunks)docs/docs/auto-docs/screens/MemberDetail/MemberDetail/functions/prettyDate.md(1 hunks)src/components/SecuredRoute/SecuredRoute.tsx(2 hunks)src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.spec.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/docs/auto-docs/screens/MemberDetail/MemberDetail/functions/prettyDate.md
- docs/docs/auto-docs/screens/MemberDetail/MemberDetail/functions/getLanguageName.md
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (2)
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.spec.tsx (1)
103-126: 🛠️ Refactor suggestionAdd test coverage for superadmin role.
The test suite covers user and admin roles but is missing coverage for the superadmin role.
Add a test case for superadmin role:
it('should render PageNotFound when user is a superadmin', async () => { (useLocalStorage as Mock).mockReturnValue({ getItem: vi.fn((key) => (key === 'token' ? 'mock_token' : 'TRUE')), setItem: vi.fn(), }); render( <MockedProvider mocks={[mockQuery('superadmin', true)]} addTypename={false}> <MemoryRouter initialEntries={['/user/organizations']}> <Routes> <Route element={<SecuredRouteForUser />}> <Route path="/user/organizations" element={<PageNotFound />} /> </Route> </Routes> </MemoryRouter> </MockedProvider>, ); await waitFor(() => { expect(screen.getByText(/404/i)).toBeInTheDocument(); expect(screen.getByText(/notFoundMsg/i)).toBeInTheDocument(); }); });src/components/SecuredRoute/SecuredRoute.tsx (1)
38-38: Move restricted routes to a configuration file.The
restrictedRoutesForAdminarray should be moved to a configuration file for better maintainability.
line 20 Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
line 30 Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
src/screens/PageNotFound/PageNotFound.tsx (1)
28-29: Consider replacing local storage check with server-side verification.The PR aims to enhance security by implementing server-side verification for all routes. However, this component still relies on local storage for admin status checks, which could potentially bypass the new security measures.
Consider using the new
VERIFY_ROLEGraphQL query mentioned in the PR summary to verify the admin status server-side. This would align with the PR's objective of replacing local storage checks with server-side verification.Example approach:
import { useQuery } from '@apollo/client'; import { VERIFY_ROLE } from 'graphql/queries/auth'; // Replace local storage check with GraphQL query const { data: roleData } = useQuery(VERIFY_ROLE); const isAdmin = roleData?.verifyRole?.isAdmin; // Use isAdmin instead of adminFor in the componentsrc/components/SecuredRoute/SecuredRoute.tsx (1)
66-92: Improve session timeout implementation.The current session timeout implementation has potential issues:
- Global variables for session management
- No cleanup of event listeners
- Potential memory leaks from setInterval
Consider moving this logic to a custom hook:
// hooks/useSessionTimeout.ts import { useEffect } from 'react'; import { toast } from 'react-toastify'; import useLocalStorage from 'utils/useLocalstorage'; export const useSessionTimeout = (timeoutMinutes = 15) => { const { setItem } = useLocalStorage(); useEffect(() => { let lastActive = Date.now(); const timeoutMs = timeoutMinutes * 60 * 1000; const handleActivity = () => { lastActive = Date.now(); }; const checkTimeout = () => { const timeSinceLastActive = Date.now() - lastActive; if (timeSinceLastActive > timeoutMs) { toast.warn('Session expired. Please login again.'); setItem('IsLoggedIn', 'FALSE'); window.location.href = '/'; } }; // Add multiple event listeners for better activity tracking const events = ['mousemove', 'keydown', 'click', 'touchstart']; events.forEach(event => document.addEventListener(event, handleActivity)); const interval = setInterval(checkTimeout, 60000); // Check every minute return () => { events.forEach(event => document.removeEventListener(event, handleActivity)); clearInterval(interval); }; }, [timeoutMinutes, setItem]); };src/App.spec.tsx (1)
74-97: Add test cases for role-based routing.The current test only verifies navigation to '/orglist'. Add tests for role-based access control.
it('should restrict access based on user role', async () => { const restrictedRoutes = ['/member', '/users', '/communityProfile']; for (const route of restrictedRoutes) { window.history.pushState({}, '', route); await wait(); // For non-admin users, should show PageNotFound expect(screen.getByTestId('page-not-found')).toBeTruthy(); } }); it('should allow superAdmin access to all routes', async () => { // Use superAdmin mock data const routes = ['/orglist', '/member', '/users']; for (const route of routes) { window.history.pushState({}, '', route); await wait(); // Should not show PageNotFound expect(screen.queryByTestId('page-not-found')).toBeNull(); } });
🧹 Nitpick comments (2)
src/components/SecuredRoute/SecuredRoute.tsx (2)
20-27: Enhance error handling for GraphQL query.The GraphQL query setup looks good, but could benefit from more robust error handling.
const { data, loading, error, refetch } = useQuery(VERIFY_ROLE, { skip: !getItem('token'), context: { headers: { Authorization: `Bearer ${getItem('token')}`, }, }, + onError: (error) => { + console.error('Role verification failed:', error); + toast.error('Authentication error. Please try logging in again.'); + } });
41-45: Improve user experience during loading and error states.The current loading and error messages are too basic. Consider using a proper loading spinner and error component.
- if (loading) { - return <div> Loading.....</div>; - } else if (error) { - return <div>Error During Routing ...</div>; - } else { + if (loading) { + return <LoadingSpinner message="Verifying access..." />; + } else if (error) { + return <ErrorComponent message="Access verification failed. Please try again." />; + } else {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/docs/auto-docs/screens/PageNotFound/PageNotFound/functions/default.md(1 hunks)src/App.spec.tsx(1 hunks)src/components/SecuredRoute/SecuredRoute.tsx(2 hunks)src/screens/PageNotFound/PageNotFound.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/docs/auto-docs/screens/PageNotFound/PageNotFound/functions/default.md
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Check Python Code Style
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (2)
src/screens/PageNotFound/PageNotFound.tsx (2)
8-8: LGTM!The SignOut component import aligns with the PR's security enhancements.
33-35: LGTM!The SignOut component is well-positioned and provides a clear way for users to sign out when they reach a 404 page.
…Admin have user Account also access to login through User section
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx (2)
19-25: Add error handling and retry policy for the GraphQL query.While the query setup is good, consider these improvements for better error handling:
- Add token validation before making the query
- Implement a retry policy for network failures
const { data, loading, error, refetch } = useQuery(VERIFY_ROLE, { context: { headers: { Authorization: `Bearer ${getItem('token')}`, }, }, + onError: (error) => { + console.error('Route verification failed:', error); + }, + retry: 2, + retryDelay: (attemptIndex) => Math.min(1000 * Math.pow(2, attemptIndex), 30000), });
30-34: Enhance loading and error states for better UX.The current implementation uses basic text messages. Consider:
- Using a proper loading component (e.g., spinner)
- Providing more informative error messages
if (loading) { - return <div> Loading.....</div>; + return <LoadingSpinner message="Verifying access..." />; } else if (error) { - return <div>Error During Routing ...</div>; + return <ErrorMessage + title="Access Verification Failed" + message="Unable to verify your access permissions. Please try refreshing the page." + />; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/App.spec.tsx(3 hunks)src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/App.spec.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (1)
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx (1)
1-3: LGTM! Appropriate imports for server-side verification.The imports correctly include the necessary Apollo Client and GraphQL dependencies for implementing server-side route verification.
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx
Outdated
Show resolved
Hide resolved
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/SecuredRoute/SecuredRoute.tsx (1)
66-92: Improve session timeout implementation.The current session timeout implementation has several issues that should be addressed:
- Global event listeners could cause memory leaks
- Direct DOM manipulation bypasses React
- Hard-coded timeout values reduce maintainability
+// Move to src/config/sessionConfig.ts +export const SESSION_CONFIG = { + timeoutMinutes: 15, + inactiveIntervalMinutes: 1 +}; +// Create a new hook: src/hooks/useSessionTimeout.ts +import { useEffect } from 'react'; +import { useNavigate } from 'react-router-dom'; +import { toast } from 'react-toastify'; +import { SESSION_CONFIG } from 'config/sessionConfig'; + +export const useSessionTimeout = () => { + const navigate = useNavigate(); + + useEffect(() => { + let lastActive = Date.now(); + + const handleActivity = () => { + lastActive = Date.now(); + }; + + const checkInactivity = () => { + const currentTime = Date.now(); + const timeSinceLastActive = currentTime - lastActive; + + if (timeSinceLastActive > SESSION_CONFIG.timeoutMinutes * 60 * 1000) { + toast.warn('Session expired. Please log in again.'); + setItem('IsLoggedIn', 'FALSE'); + navigate('/', { replace: true }); + } + }; + + // Add event listeners through React + window.addEventListener('mousemove', handleActivity); + window.addEventListener('keypress', handleActivity); + + const interval = setInterval( + checkInactivity, + SESSION_CONFIG.inactiveIntervalMinutes * 60 * 1000 + ); + + // Cleanup + return () => { + window.removeEventListener('mousemove', handleActivity); + window.removeEventListener('keypress', handleActivity); + clearInterval(interval); + }; + }, [navigate]); +}; +// In SecuredRoute component +const SecuredRoute = (): JSX.Element => { + useSessionTimeout(); // ... rest of the component }; - -// Remove the global timeout logic -const timeoutMinutes = 15; -const timeoutMilliseconds = timeoutMinutes * 60 * 1000; -// ... remove the rest of the timeout logic
♻️ Duplicate comments (1)
src/components/SecuredRoute/SecuredRoute.tsx (1)
46-63: 🛠️ Refactor suggestionCentralize and enhance role-based access control.
The current implementation with hard-coded routes and nested conditions is difficult to maintain. Consider implementing the previously suggested ROUTE_PERMISSIONS configuration.
+// Move to a separate file: src/config/routePermissions.ts +export const ROUTE_PERMISSIONS = { + superAdmin: ['*'], + admin: ['/orglist', '/dashboard', '/events'], + user: ['/dashboard', '/profile'] +}; +// In SecuredRoute.tsx -const restrictedRoutesForAdmin = ['/member', '/users', '/communityProfile']; +import { ROUTE_PERMISSIONS } from 'config/routePermissions'; const role = data?.verifyRole?.role || ''; const isLoggedIn = data?.verifyRole?.isAuthorized ?? false; -if (isLoggedIn) { - if (role == 'superAdmin') { - return <Outlet />; - } else if (role == 'admin') { - if (restrictedRoutesForAdmin.includes(location.pathname)) { - return <PageNotFound />; - } - return <Outlet />; - } else { +if (!isLoggedIn) { + return <Navigate to="/" replace />; +} + +const allowedRoutes = ROUTE_PERMISSIONS[role] || []; +const hasAccess = allowedRoutes.includes('*') || + allowedRoutes.includes(location.pathname); + +if (!hasAccess) { + console.warn(`Access denied for ${role} to ${location.pathname}`); + if (process.env.NODE_ENV === 'development') { + console.debug('Allowed routes:', allowedRoutes); + } + return <PageNotFound />; +} + +return <Outlet />; - return <PageNotFound />; - } -} else { - return <Navigate to="/" replace />; -}
🧹 Nitpick comments (4)
src/components/SecuredRoute/SecuredRoute.tsx (2)
19-39: Enhance error handling for token management.While the token management implementation is good, consider adding error boundaries and logging for token-related issues.
const { data, loading, error, refetch } = useQuery(VERIFY_ROLE, { skip: !getItem('token'), context: { headers: { Authorization: `Bearer ${getItem('token')}`, }, }, + onError: (error) => { + console.error('Role verification failed:', error); + // Consider implementing a retry mechanism + if (error.message.includes('jwt expired')) { + // Handle expired token + setItem('IsLoggedIn', 'FALSE'); + window.location.href = '/'; + } + } });
41-45: Improve loading and error state UI.The current loading and error messages are too basic. Consider using a proper loading spinner and more informative error messages.
if (loading) { - return <div> Loading.....</div>; + return ( + <div className="loading-container"> + <LoadingSpinner /> + <p>Verifying your access...</p> + </div> + ); } else if (error) { - return <div>Error During Routing ...</div>; + return ( + <div className="error-container"> + <ErrorIcon /> + <p>Unable to verify your access. Please try refreshing the page.</p> + {process.env.NODE_ENV === 'development' && ( + <pre>{error.message}</pre> + )} + </div> + ); }src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx (2)
8-15: Update JSDoc comments to reflect the new implementation.The component's documentation still references the old local storage based checks (
AdminFor). Update it to reflect the new GraphQL-based role verification system./** * A component that guards routes by checking if the user is logged in. - * If the user is logged in and does not have 'AdminFor' set, the child routes are rendered. + * If the user is logged in and has an appropriate role (user/admin/superAdmin), the child routes are rendered. * If the user is not logged in, they are redirected to the homepage. - * If the user is logged in but has 'AdminFor' set, a 404 page is shown. + * If the user is logged in but has an invalid role, a 404 page is shown. * * @returns JSX.Element - Rendered component based on user authentication and role. */
30-34: Enhance loading and error states.The current loading and error messages could be more informative and user-friendly.
if (loading) { - return <div> Loading.....</div>; + return <div className="loading-spinner">Verifying your access...</div>; } else if (error) { - return <div>Error During Routing ...</div>; + return ( + <div className="error-message"> + Unable to verify your access. Please try refreshing the page or contact support if the issue persists. + </div> + ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/App.spec.tsx(3 hunks)src/components/SecuredRoute/SecuredRoute.tsx(2 hunks)src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/App.spec.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (3)
src/components/SecuredRoute/SecuredRoute.tsx (1)
1-8: LGTM! Proper imports for GraphQL and routing.The imports are well-organized and include all necessary dependencies for server-side verification.
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx (2)
26-28: Optimize the useEffect dependency array.The current implementation could cause unnecessary refetches because
getItem('token')creates a new reference on every render.
35-47: 🛠️ Refactor suggestionImprove role verification logic.
The current implementation could be more robust with strict equality and type-safe role enumeration.
+ enum UserRole { + USER = 'user', + ADMIN = 'admin', + SUPER_ADMIN = 'superAdmin' + } - const role = data?.verifyRole?.role || ''; - const isLoggedIn = data?.verifyRole?.isAuthorized ?? false; + const role = data?.verifyRole?.role as UserRole; + const isAuthorized = data?.verifyRole?.isAuthorized ?? false; - if (isLoggedIn) { + if (isAuthorized) { - if (role == 'user' || role == 'admin' || role == 'superAdmin') { + if (Object.values(UserRole).includes(role)) { return <Outlet />; } else { return <PageNotFound />; } } else { return <Navigate to="/" replace />; }Likely invalid or redundant comment.
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx
Outdated
Show resolved
Hide resolved
|
@palisadoes Testing of frontend application takes more than 1 hours.Is it is issue or what?And for testing a individual component also takes more than 6-7 minutes |
|
|
@palisadoes not even the component i have developed except that component where mine will not affect will also takes time individually.Yes today i will fix all these failing test cases. |
|
@palisadoes i got clarity . why some component taking time not all are taking much time. Just like app.tsx which is render all child component when the test runs it renders all the child component i think that's why this type of component are taking time. |
|
@palisadoes i am facing this issue of screen flickering . I does not know exact issue why it is making . But guesing it is due mis manage of state or something inside a useEffect and may be refreshToken which is causing infinte re render . 2025-02-05.01-31-11.1.mp4 |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
src/screens/PageNotFound/PageNotFound.spec.tsx (1)
54-71: Update test strategy to use GraphQL responses.The test still relies on local storage (
setItem('AdminFor', ...)) for admin verification. Since this PR implements server-side verification, the test should use mocked GraphQL responses instead.Here's how you can update the test:
it('should render properly for ADMIN or SUPERADMIN', () => { - setItem('AdminFor', [ - { - _id: '6537904485008f171cf29924', - __typename: 'Organization', - }, - ]); + // Mock the VERIFY_ROLE query response for admin + const mockLink = new ApolloLink((operation, forward) => { + if (operation.operationName === 'VERIFY_ROLE') { + return new Observable((observer) => { + observer.next({ + data: { + verifyRole: { + isAuthorized: true, + role: 'ADMIN' + } + } + }); + observer.complete(); + }); + } + return forward(operation); + }); + + const testClient = new ApolloClient({ + cache: new InMemoryCache(), + link: mockLink + }); render( - <ApolloProvider client={client}> + <ApolloProvider client={testClient}>src/components/SecuredRoute/SecuredRoute.tsx (2)
80-82: Prevent memory leaks from event listeners.The mousemove event listener is added but never removed, which could cause memory leaks.
Wrap the event listener in a useEffect hook:
-document.addEventListener('mousemove', () => { - lastActive = Date.now(); -}); +useEffect(() => { + const updateLastActive = () => { + lastActive = Date.now(); + }; + + document.addEventListener('mousemove', updateLastActive); + return () => { + document.removeEventListener('mousemove', updateLastActive); + }; +}, []);
85-96: Improve session timeout implementation.The current implementation using setInterval could cause performance issues and might not clean up properly.
Use React's useEffect for session management:
+const useSessionTimeout = (timeoutMinutes: number, inactiveIntervalMin: number) => { + useEffect(() => { + let timeoutId: NodeJS.Timeout; + + const checkInactivity = () => { + const currentTime = Date.now(); + const timeSinceLastActive = currentTime - lastActive; + + if (timeSinceLastActive > timeoutMinutes * 60 * 1000) { + toast.warn('Your session has expired. Please log in again.'); + setItem('IsLoggedIn', 'FALSE'); + window.location.href = '/'; + } + }; + + timeoutId = setInterval(checkInactivity, inactiveIntervalMin * 60 * 1000); + + return () => { + clearInterval(timeoutId); + }; + }, [timeoutMinutes, inactiveIntervalMin]); +}; -setInterval(() => { - const currentTime = Date.now(); - const timeSinceLastActive = currentTime - lastActive; - - if (timeSinceLastActive > timeoutMilliseconds) { - toast.warn('Kindly relogin as sessison has expired'); - window.location.href = '/'; - setItem('IsLoggedIn', 'FALSE'); - } -}, inactiveIntervalMilsec); +useSessionTimeout(timeoutMinutes, inactiveIntervalMin);
🧹 Nitpick comments (4)
src/screens/PageNotFound/PageNotFound.spec.tsx (1)
34-35: Remove unnecessary whitespace and comment.The whitespace character and inline comment don't add value and should be removed.
- {' '} - {/* ✅ Add ApolloProvider */}src/components/CheckIn/types.ts (2)
45-49: Add JSDoc comments for the UserRole enum.The enum is well-structured but lacks documentation explaining the purpose and usage of each role.
Add JSDoc comments:
+/** + * Enum representing user roles in the system. + * @enum {string} + */ export enum UserRole { + /** Regular user with basic privileges */ USER = 'user', + /** Administrator with elevated privileges */ ADMIN = 'admin', + /** Super administrator with full system access */ SUPER_ADMIN = 'superAdmin', }
51-56: Add JSDoc comments for the VerifyRoleResponse type.The type definition would benefit from documentation explaining its purpose and structure.
Add JSDoc comments:
+/** + * Type representing the response from role verification. + * @typedef {Object} VerifyRoleResponse + */ export type VerifyRoleResponse = { + /** Object containing authorization details */ verifyRole: { + /** Whether the user is authorized */ isAuthorized: boolean; + /** The user's role in the system */ role: UserRole; }; };src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx (1)
45-49: Enhance loading and error states.The current loading and error states could be more informative and user-friendly.
Consider using a loading spinner and more descriptive error messages:
if (loading) { - return <div> Loading.....</div>; + return <div className="loading-spinner">Verifying your access...</div>; } else if (error) { - return <div>Error During Routing ...</div>; + return <div className="error-message"> + Unable to verify your access. Please try refreshing the page. + {error.message && <p>Error: {error.message}</p>} + </div>; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/docs/auto-docs/components/CheckIn/types/enumerations/UserRole.md(1 hunks)docs/docs/auto-docs/components/CheckIn/types/type-aliases/VerifyRoleResponse.md(1 hunks)docs/docs/auto-docs/components/SecuredRoute/SecuredRoute/functions/default.md(1 hunks)docs/docs/auto-docs/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser/functions/default.md(1 hunks)src/components/CheckIn/types.ts(1 hunks)src/components/SecuredRoute/SecuredRoute.tsx(2 hunks)src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx(2 hunks)src/screens/PageNotFound/PageNotFound.spec.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/docs/auto-docs/components/CheckIn/types/type-aliases/VerifyRoleResponse.md
- docs/docs/auto-docs/components/CheckIn/types/enumerations/UserRole.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/docs/auto-docs/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser/functions/default.md
- docs/docs/auto-docs/components/SecuredRoute/SecuredRoute/functions/default.md
🧰 Additional context used
📓 Learnings (2)
src/components/SecuredRoute/SecuredRoute.tsx (1)
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-admin#3491
File: src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx:19-25
Timestamp: 2025-02-04T19:16:17.807Z
Learning: In React components using Apollo Client, avoid defining explicit TypeScript types for GraphQL query responses as they are automatically generated based on the GraphQL schema. Focus on query-specific parameters like `skip` for optimization.
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx (1)
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-admin#3491
File: src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx:19-25
Timestamp: 2025-02-04T19:16:17.807Z
Learning: In React components using Apollo Client, avoid defining explicit TypeScript types for GraphQL query responses as they are automatically generated based on the GraphQL schema. Focus on query-specific parameters like `skip` for optimization.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Application
- GitHub Check: Check Python Code Style
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (2)
src/screens/PageNotFound/PageNotFound.spec.tsx (1)
12-18: LGTM! Apollo Client imports are well-organized.The imports are properly structured with type imports separated from component imports.
src/components/SecuredRoute/SecuredRoute.tsx (1)
52-63: 🛠️ Refactor suggestionCentralize route permissions configuration.
The route restrictions are hardcoded in the component, making them difficult to maintain.
Move route permissions to a configuration file:
+// src/config/routePermissions.ts +export const ROUTE_PERMISSIONS = { + [UserRole.SUPER_ADMIN]: ['*'], // Access all routes + [UserRole.ADMIN]: ['/orglist', '/events'], // Restricted access + [UserRole.USER]: ['/dashboard', '/profile'] // User-specific routes +}; -const restrictedRoutesForAdmin = ['/member', '/users', '/communityProfile']; +import { ROUTE_PERMISSIONS } from 'config/routePermissions'; if (isAuthorized) { if (role == UserRole.SUPER_ADMIN) { return <Outlet />; } else if (role == UserRole.ADMIN) { - if (restrictedRoutesForAdmin.includes(location.pathname)) { + if (!ROUTE_PERMISSIONS[UserRole.ADMIN].includes(location.pathname)) { return <PageNotFound />; } return <Outlet />; }Likely invalid or redundant comment.
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx
Outdated
Show resolved
Hide resolved
…ser.tsx line 57 Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx (3)
2-3: Consider relocating shared types to a common directory.The
VerifyRoleResponsetype andUserRoleenum are imported fromcomponents/CheckIn/typesbut are used across multiple components. Consider moving these to a shared types directory (e.g.,src/typesorsrc/shared/types) to improve maintainability.
21-31: Remove explicit type parameter from useQuery.As per best practices with Apollo Client, the TypeScript types for GraphQL query responses are automatically generated based on the GraphQL schema. The explicit type parameter
VerifyRoleResponseis unnecessary.- const { data, loading, error, refetch } = useQuery<VerifyRoleResponse>( + const { data, loading, error, refetch } = useQuery(
45-49: Enhance loading and error handling.The current implementation uses basic text messages. Consider:
- Using a proper loading component for better UX
- Providing more informative error messages to help users understand and resolve issues
if (loading) { - return <div> Loading.....</div>; + return <LoadingSpinner message="Verifying access..." />; } else if (error) { - return <div>Error During Routing ...</div>; + return <ErrorMessage + message="Failed to verify access permissions" + details={error.message} + action={() => refetch()} + />; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx (1)
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-admin#3491
File: src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx:19-25
Timestamp: 2025-02-04T19:16:17.807Z
Learning: In React components using Apollo Client, avoid defining explicit TypeScript types for GraphQL query responses as they are automatically generated based on the GraphQL schema. Focus on query-specific parameters like `skip` for optimization.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx
Outdated
Show resolved
Hide resolved
…awa-admin into feature/VerifyRoleRoute
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/SecuredRoute/SecuredRoute.tsx (1)
74-100: Refactor session timeout logic into a custom hook.The current implementation has several issues:
- Global event listeners and intervals can cause memory leaks
- Hard-coded timeout values should be configurable
- Session timeout logic should be encapsulated
Create a new custom hook:
// src/hooks/useSessionTimeout.ts import { useEffect } from 'react'; import { toast } from 'react-toastify'; import useLocalStorage from 'utils/useLocalstorage'; interface SessionTimeoutConfig { timeoutMinutes: number; checkIntervalMinutes: number; } export const useSessionTimeout = ({ timeoutMinutes = 15, checkIntervalMinutes = 1 }: SessionTimeoutConfig) => { const { setItem } = useLocalStorage(); useEffect(() => { let lastActive = Date.now(); const timeoutMs = timeoutMinutes * 60 * 1000; const intervalMs = checkIntervalMinutes * 60 * 1000; const handleActivity = () => { lastActive = Date.now(); }; const checkTimeout = setInterval(() => { const timeSinceLastActive = Date.now() - lastActive; if (timeSinceLastActive > timeoutMs) { toast.warn('Kindly relogin as session has expired'); window.location.href = '/'; setItem('IsLoggedIn', 'FALSE'); } }, intervalMs); document.addEventListener('mousemove', handleActivity); return () => { document.removeEventListener('mousemove', handleActivity); clearInterval(checkTimeout); }; }, [timeoutMinutes, checkIntervalMinutes]); };Then use it in the component:
-// Time constants for session timeout and inactivity interval -const timeoutMinutes = 15; -const timeoutMilliseconds = timeoutMinutes * 60 * 1000; - -const inactiveIntervalMin = 1; -const inactiveIntervalMilsec = inactiveIntervalMin * 60 * 1000; - -let lastActive: number = Date.now(); - -// Update lastActive timestamp on mouse movement -document.addEventListener('mousemove', () => { - lastActive = Date.now(); -}); - -// Check for inactivity and handle session timeout -setInterval(() => { - const currentTime = Date.now(); - const timeSinceLastActive = currentTime - lastActive; - - // If inactive for longer than the timeout period, show a warning and log out - if (timeSinceLastActive > timeoutMilliseconds) { - toast.warn('Kindly relogin as sessison has expired'); - - window.location.href = '/'; - setItem('IsLoggedIn', 'FALSE'); - } -}, inactiveIntervalMilsec); +const SecuredRoute = (): JSX.Element => { + useSessionTimeout({ + timeoutMinutes: 15, + checkIntervalMinutes: 1 + }); + // ... rest of the component
🧹 Nitpick comments (3)
src/components/SecuredRoute/SecuredRoute.tsx (3)
50-50: Remove console.log statement.Console.log statements should not be present in production code.
- console.log('Mocked Data:', data);
58-60: Use strict equality operator for role comparison.Use
===instead of==for type-safe comparisons.- if (role == UserRole.SUPER_ADMIN) { + if (role === UserRole.SUPER_ADMIN) {
56-56: Move restricted routes to a configuration file.Hard-coded routes should be moved to a configuration file for better maintainability.
Create a new file
src/config/routes.ts:export const RESTRICTED_ROUTES = { ADMIN: ['/member', '/users', '/communityProfile'] };Then import and use it:
- const restrictedRoutesForAdmin = ['/member', '/users', '/communityProfile']; + import { RESTRICTED_ROUTES } from 'config/routes'; + // ... + if (RESTRICTED_ROUTES.ADMIN.includes(location.pathname)) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/App.spec.tsx(3 hunks)src/components/SecuredRoute/SecuredRoute.tsx(2 hunks)src/screens/PageNotFound/PageNotFound.spec.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/App.spec.tsx
- src/screens/PageNotFound/PageNotFound.spec.tsx
🧰 Additional context used
📓 Learnings (1)
src/components/SecuredRoute/SecuredRoute.tsx (2)
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-admin#3491
File: src/components/SecuredRoute/SecuredRoute.tsx:0-0
Timestamp: 2025-02-04T20:30:07.119Z
Learning: In the talawa-admin project, role-based routing is implemented using separate routes for admin and user roles, with explicit role checks and a list of restricted routes for admins, rather than using a centralized route permissions configuration.
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-admin#3491
File: src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx:19-25
Timestamp: 2025-02-04T19:16:17.807Z
Learning: In React components using Apollo Client, avoid defining explicit TypeScript types for GraphQL query responses as they are automatically generated based on the GraphQL schema. Focus on query-specific parameters like `skip` for optimization.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (1)
src/components/SecuredRoute/SecuredRoute.tsx (1)
1-9: LGTM! Imports and type definitions are well organized.The imports are properly structured, including necessary dependencies for GraphQL integration and type definitions.
…ser.tsx line number 50 Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx (1)
10-17: Update component documentation to reflect current implementation.The JSDoc comments mention checking for 'AdminFor' in local storage, but the component now uses server-side role verification. Update the documentation to accurately describe the current behavior:
/** * A component that guards routes by checking if the user is logged in. - * If the user is logged in and does not have 'AdminFor' set, the child routes are rendered. - * If the user is not logged in, they are redirected to the homepage. - * If the user is logged in but has 'AdminFor' set, a 404 page is shown. + * Uses server-side verification to check user's role and authorization status. + * If the user is authorized with a valid role (USER, ADMIN, or SUPER_ADMIN), + * the child routes are rendered. Otherwise, they are redirected to the homepage + * or shown a 404 page. * * @returns JSX.Element - Rendered component based on user authentication and role. */
🧹 Nitpick comments (2)
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx (2)
2-3: Consider relocating shared types to a common types directory.The
VerifyRoleResponsetype andUserRoleenum are currently imported fromCheckIn/types, but they seem to be shared across different components. Consider moving these types to a dedicated types directory (e.g.,src/typesorsrc/shared/types) for better organization and reusability.
53-53: Fix code formatting.Remove extra whitespace on line 53 to resolve the Prettier formatting issue.
🧰 Tools
🪛 ESLint
[error] 53-53: Delete
····(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx (1)
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-admin#3491
File: src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx:19-25
Timestamp: 2025-02-04T19:16:17.807Z
Learning: In React components using Apollo Client, avoid defining explicit TypeScript types for GraphQL query responses as they are automatically generated based on the GraphQL schema. Focus on query-specific parameters like `skip` for optimization.
🪛 ESLint
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx
[error] 53-53: Delete ····
(prettier/prettier)
🪛 GitHub Actions: PR Workflow
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx
[warning] 1-1: Code style issues found in the above file. Run Prettier with --write to fix.
🔇 Additional comments (1)
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx (1)
45-69: Well-implemented routing logic with proper error handling.The implementation successfully:
- Handles loading and error states
- Verifies user authorization on the server side
- Uses strict equality comparison for role verification
- Properly handles undefined data
- Implements secure routing based on user roles
This aligns well with the PR objectives of enhancing route security.
🧰 Tools
🪛 ESLint
[error] 53-53: Delete
····(prettier/prettier)
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/SecuredRoute/SecuredRoute.tsx (1)
73-99: Fix potential memory leaks in session timeout implementation.The current implementation has several issues:
- Event listener is never removed
- setInterval is never cleared
- Global variables could cause issues in testing
Move the session timeout logic into the component:
const SecuredRoute = (): JSX.Element => { + useEffect(() => { + let lastActive = Date.now(); + let intervalId: NodeJS.Timeout; + + const updateLastActive = () => { + lastActive = Date.now(); + }; + + const checkInactivity = () => { + const currentTime = Date.now(); + const timeSinceLastActive = currentTime - lastActive; + + if (timeSinceLastActive > timeoutMilliseconds) { + toast.warn('Kindly relogin as session has expired'); + window.location.href = '/'; + setItem('IsLoggedIn', 'FALSE'); + } + }; + + document.addEventListener('mousemove', updateLastActive); + intervalId = setInterval(checkInactivity, inactiveIntervalMilsec); + + return () => { + document.removeEventListener('mousemove', updateLastActive); + clearInterval(intervalId); + }; + }, []); // ... rest of the component }; - -let lastActive: number = Date.now(); - -document.addEventListener('mousemove', () => { - lastActive = Date.now(); -}); - -setInterval(() => { - const currentTime = Date.now(); - const timeSinceLastActive = currentTime - lastActive; - - if (timeSinceLastActive > timeoutMilliseconds) { - toast.warn('Kindly relogin as sessison has expired'); - window.location.href = '/'; - setItem('IsLoggedIn', 'FALSE'); - } -}, inactiveIntervalMilsec);
🧹 Nitpick comments (5)
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx (2)
10-17: Update component documentation to reflect new role verification logic.The JSDoc comments still reference the old 'AdminFor' check which has been replaced with server-side role verification.
/** * A component that guards routes by checking if the user is logged in. - * If the user is logged in and does not have 'AdminFor' set, the child routes are rendered. + * If the user is logged in and has an authorized role (USER, ADMIN, or SUPER_ADMIN), + * the child routes are rendered. * If the user is not logged in, they are redirected to the homepage. - * If the user is logged in but has 'AdminFor' set, a 404 page is shown. + * If the user is logged in but has an unauthorized role, a 404 page is shown. * * @returns JSX.Element - Rendered component based on user authentication and role. */
45-49: Enhance error handling messages.Consider providing more informative loading and error states to improve user experience.
if (loading) { - return <div> Loading.....</div>; + return <div>Verifying user access...</div>; } else if (error) { - return <div>Error During Routing ...</div>; + return <div>Unable to verify access. Please try again or contact support.</div>; } else {src/components/SecuredRoute/SecuredRoute.tsx (3)
20-43: Enhance error handling for the GraphQL query.While the token management is well-implemented, consider adding error handling for network failures and invalid tokens.
const { data, loading, error, refetch } = useQuery<VerifyRoleResponse>( VERIFY_ROLE, { skip: !getItem('token'), context: { headers: { Authorization: `Bearer ${getItem('token')}`, }, }, + onError: (error) => { + if (error.message.includes('invalid token')) { + setItem('IsLoggedIn', 'FALSE'); + window.location.href = '/'; + } + toast.error('Authentication error. Please try again.'); + } }, );
45-49: Improve error message for better user experience.The current error message is too technical. Consider providing a more user-friendly message.
- return <div>Error During Routing ...</div>; + return <div>Unable to verify your access. Please try logging in again.</div>;
55-55: Extract restricted routes as a constant.Move the restricted routes array outside the component to prevent recreation on each render.
+const RESTRICTED_ROUTES_FOR_ADMIN = ['/member', '/users', '/communityProfile'] as const; + const SecuredRoute = (): JSX.Element => { // ... - const restrictedRoutesForAdmin = ['/member', '/users', '/communityProfile']; + const restrictedRoutesForAdmin = RESTRICTED_ROUTES_FOR_ADMIN;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/SecuredRoute/SecuredRoute.tsx(2 hunks)src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx(2 hunks)src/screens/PageNotFound/PageNotFound.spec.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screens/PageNotFound/PageNotFound.spec.tsx
🧰 Additional context used
📓 Learnings (2)
src/components/SecuredRoute/SecuredRoute.tsx (3)
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-admin#3491
File: src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx:34-43
Timestamp: 2025-02-06T20:56:05.378Z
Learning: In SecuredRouteForUser component, token updates between role switches (e.g., user to superAdmin) are better handled using separate useEffect hooks - one for initial token setup (empty deps) and another for refetch on token changes, rather than combining them into a single effect.
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-admin#3491
File: src/components/SecuredRoute/SecuredRoute.tsx:0-0
Timestamp: 2025-02-04T20:30:07.119Z
Learning: In the talawa-admin project, role-based routing is implemented using separate routes for admin and user roles, with explicit role checks and a list of restricted routes for admins, rather than using a centralized route permissions configuration.
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-admin#3491
File: src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx:19-25
Timestamp: 2025-02-04T19:16:17.807Z
Learning: In React components using Apollo Client, avoid defining explicit TypeScript types for GraphQL query responses as they are automatically generated based on the GraphQL schema. Focus on query-specific parameters like `skip` for optimization.
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx (2)
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-admin#3491
File: src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx:34-43
Timestamp: 2025-02-06T20:56:05.378Z
Learning: In SecuredRouteForUser component, token updates between role switches (e.g., user to superAdmin) are better handled using separate useEffect hooks - one for initial token setup (empty deps) and another for refetch on token changes, rather than combining them into a single effect.
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-admin#3491
File: src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx:19-25
Timestamp: 2025-02-04T19:16:17.807Z
Learning: In React components using Apollo Client, avoid defining explicit TypeScript types for GraphQL query responses as they are automatically generated based on the GraphQL schema. Focus on query-specific parameters like `skip` for optimization.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (5)
src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsx (4)
1-8: LGTM! Well-organized imports.The imports are properly organized with types separated from runtime dependencies.
21-31: LGTM! Well-configured GraphQL query.The query setup follows best practices with:
- Proper skip condition to prevent unnecessary queries
- Authorization header configuration
- Use of auto-generated types from the GraphQL schema
32-43: LGTM! Token management optimized for role switches.The separate useEffect hooks effectively handle token updates during role switches (e.g., user to superAdmin).
50-69: LGTM! Secure role verification implementation.The role verification logic is well-implemented with:
- Proper null checks
- Strict equality comparisons
- Clear handling of different authorization states
src/components/SecuredRoute/SecuredRoute.tsx (1)
1-9: LGTM! Clean import structure.The imports are well-organized and all dependencies are properly utilized.
palisadoes
merged commit 47e7c60
into
PalisadoesFoundation:develop
What kind of change does this PR introduce?
Feature
Issue Number:
Fixes #1839
Snapshots/Videos:
here a user is trying to access superAdmin page
results after adding serverSide verification
here if admin also trying to access superAdmin pages also he will not be able to access
results after
here we are whitelisting the pages which pages is common between admin and superAdmin so that admin will have limited access to superAdmin pages like' /orglist ' this page can be accessed to admin and superAdmin both.
here see the screenShot
If relevant, did you update the documentation?
No
Summary
Does this PR introduce a breaking change?
No
Checklist
CodeRabbit AI Review
Test Coverage
Other information
-It is more secure than our conventional way.
Have you read the contributing guide?
Yes
Summary by CodeRabbit
VERIFY_ROLE,UserRole, andVerifyRoleResponse.