Feature : Added server side verification for all routes . #3491
Conversation
WalkthroughThe pull request introduces significant enhancements to the authentication and authorization mechanisms for routes. It replaces local storage-based checks with server-side GraphQL queries in components such as 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 8317 +16
Branches 1882 1887 +5
=======================================
+ Hits 306 307 +1
- Misses 7957 7972 +15
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.
| const link = new ApolloLink((operation, forward) => { | ||
| return forward(operation); | ||
| }); | ||
|
|
||
| const client: ApolloClient<NormalizedCacheObject> = new ApolloClient({ | ||
| cache: new InMemoryCache(), | ||
| link: ApolloLink.from([link]), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add mock responses for authorization scenarios.
The current Apollo Client setup uses a no-op link. Consider adding mock responses to test the component's behavior under different authorization states, especially since this PR implements server-side verification.
Here's how you can enhance the setup:
const link = new ApolloLink((operation, forward) => {
+ // Mock responses based on the operation
+ if (operation.operationName === 'VERIFY_ROLE') {
+ return new Observable((observer) => {
+ // Simulate different authorization scenarios
+ observer.next({
+ data: {
+ verifyRole: {
+ isAuthorized: true, // or false for different test cases
+ role: 'ADMIN' // or other roles
+ }
+ }
+ });
+ observer.complete();
+ });
+ }
return forward(operation);
});📝 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 link = new ApolloLink((operation, forward) => { | |
| return forward(operation); | |
| }); | |
| const client: ApolloClient<NormalizedCacheObject> = new ApolloClient({ | |
| cache: new InMemoryCache(), | |
| link: ApolloLink.from([link]), | |
| }); | |
| const link = new ApolloLink((operation, forward) => { | |
| // Mock responses based on the operation | |
| if (operation.operationName === 'VERIFY_ROLE') { | |
| return new Observable((observer) => { | |
| // Simulate different authorization scenarios | |
| observer.next({ | |
| data: { | |
| verifyRole: { | |
| isAuthorized: true, // or false for different test cases | |
| role: 'ADMIN' // or other roles | |
| } | |
| } | |
| }); | |
| observer.complete(); | |
| }); | |
| } | |
| return forward(operation); | |
| }); | |
| const client: ApolloClient<NormalizedCacheObject> = new ApolloClient({ | |
| cache: new InMemoryCache(), | |
| link: ApolloLink.from([link]), | |
| }); |
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)
| const { isAuthorized = false, role = '' } = (data as VerifyRoleResponse) | ||
| .verifyRole; | ||
|
|
||
| if (isAuthorized) { | ||
| if ( | ||
| role == UserRole.USER || | ||
| role == UserRole.ADMIN || | ||
| role == UserRole.SUPER_ADMIN | ||
| ) { | ||
| return <Outlet />; | ||
| } else { | ||
| return <PageNotFound />; | ||
| } | ||
| } else { | ||
| return <Navigate to="/" replace />; | ||
| } | ||
| } |
There was a problem hiding this comment.
Strengthen role verification logic.
Several improvements can enhance the security and reliability of the role verification:
- Use strict equality comparison
- Remove type assertion by properly typing the query response
- Consider handling undefined data explicitly
- const { isAuthorized = false, role = '' } = (data as VerifyRoleResponse)
- .verifyRole;
+ if (!data?.verifyRole) {
+ return <Navigate to="/" replace />;
+ }
+
+ const { isAuthorized, role } = data.verifyRole;
if (isAuthorized) {
if (
- role == UserRole.USER ||
- role == UserRole.ADMIN ||
- role == UserRole.SUPER_ADMIN
+ role === UserRole.USER ||
+ role === UserRole.ADMIN ||
+ role === UserRole.SUPER_ADMIN
) {📝 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 { isAuthorized = false, role = '' } = (data as VerifyRoleResponse) | |
| .verifyRole; | |
| if (isAuthorized) { | |
| if ( | |
| role == UserRole.USER || | |
| role == UserRole.ADMIN || | |
| role == UserRole.SUPER_ADMIN | |
| ) { | |
| return <Outlet />; | |
| } else { | |
| return <PageNotFound />; | |
| } | |
| } else { | |
| return <Navigate to="/" replace />; | |
| } | |
| } | |
| if (!data?.verifyRole) { | |
| return <Navigate to="/" replace />; | |
| } | |
| const { isAuthorized, role } = data.verifyRole; | |
| if (isAuthorized) { | |
| if ( | |
| role === UserRole.USER || | |
| role === UserRole.ADMIN || | |
| role === UserRole.SUPER_ADMIN | |
| ) { | |
| return <Outlet />; | |
| } else { | |
| return <PageNotFound />; | |
| } | |
| } else { | |
| return <Navigate to="/" replace />; | |
| } | |
| } |
| useEffect(() => { | ||
| const newToken = getItem('token'); | ||
| if (newToken !== token) { | ||
| setToken(newToken); | ||
| } | ||
| }, []); | ||
|
|
||
| // Conditional rendering based on authentication status and role | ||
| return isLoggedIn === 'TRUE' ? ( | ||
| <>{adminFor == undefined ? <Outlet /> : <PageNotFound />}</> | ||
| ) : ( | ||
| <Navigate to="/" replace /> | ||
| ); | ||
| useEffect(() => { | ||
| refetch(); // Refetch when token updates | ||
| }, [token]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve token state management.
The current implementation has two potential issues:
- The first useEffect with empty dependencies won't react to token changes in localStorage
- Using separate effects for token updates and refetch could lead to race conditions
Consider combining the effects:
- useEffect(() => {
- const newToken = getItem('token');
- if (newToken !== token) {
- setToken(newToken);
- }
- }, []);
-
- useEffect(() => {
- refetch(); // Refetch when token updates
- }, [token]);
+ useEffect(() => {
+ const newToken = getItem('token');
+ if (newToken !== token) {
+ setToken(newToken);
+ refetch();
+ }
+ }, [getItem('token')]);Committable suggestion skipped: line range outside the PR's diff.
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
Release Notes
New Features
VERIFY_ROLEto verify user authorization and role.UserRoleenumeration andVerifyRoleResponsetype for better role management.Documentation
VERIFY_ROLEquery and theUserRoleenumeration.default()functions in theSecuredRoute,SecuredRouteForUser, andPageNotFoundcomponents.getLanguageNameandprettyDatefunctions.Improvements
SecuredRouteandSecuredRouteForUsercomponents.Bug Fixes
Tests
SecuredRouteForUsercomponent to include loading and error state handling.Appcomponent for better clarity and functionality.PageNotFoundcomponent.