UN-2955 [FIX] Re-verify admin status on navigation to admin routes #1773
UN-2955 [FIX] Re-verify admin status on navigation to admin routes #1773hari-kuriakose merged 11 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces a synchronous admin check with an asynchronous verification in RequireAdmin: fetches user profile from the API, reconciles fetched Changes
Sequence DiagramsequenceDiagram
participant Browser as Browser/Component
participant RequireAdmin as RequireAdmin
participant API as API Server
participant Session as Session Storage
Browser->>RequireAdmin: Mount
RequireAdmin->>RequireAdmin: set isVerifying = true
RequireAdmin->>API: GET /api/v1/unstract/{orgId}/users/profile/ (X-CSRFToken)
activate API
API-->>RequireAdmin: user data (is_admin, role)
deactivate API
RequireAdmin->>Session: compare fetched is_admin/role with stored
alt Differences found
RequireAdmin->>Session: updateSessionDetails(...)
end
RequireAdmin->>RequireAdmin: set isVerifying = false
alt org is open source
RequireAdmin->>Browser: Render NotFound
else not admin
RequireAdmin->>Browser: Render Unauthorized
else admin
RequireAdmin->>Browser: Render Outlet
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kirtimanmishrazipstack
requested review from
jagadeeswaran-zipstack and
vishnuszipstack
![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
🤖 Fix all issues with AI agents
In `@frontend/src/components/helpers/auth/RequireAdmin.js`:
- Around line 17-49: The useEffect's verifyAdminStatus async call can update
state after unmount; fix by creating an AbortController inside the effect, pass
controller.signal into the axios.get call in verifyAdminStatus, and in the
effect cleanup call controller.abort() so the request is cancelled; also update
the catch inside verifyAdminStatus to ignore or handle AbortError (do not call
setIsVerifying or updateSessionDetails when aborted) and only call
setIsVerifying(false) in the finally when the request was not aborted to avoid
state updates on an unmounted component.
🧹 Nitpick comments (1)
frontend/src/components/helpers/auth/RequireAdmin.js (1)
37-38: Consider logging errors for debuggability.Silently swallowing all errors makes it difficult to diagnose issues (e.g., network failures, auth errors, API changes). Consider logging in non-production environments.
🔧 Suggested improvement
- } catch { + } catch (error) { + if (!axios.isCancel(error)) { + console.error("Failed to verify admin status:", error); + } // On error, keep current session state } finally {
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/helpers/auth/RequireAdmin.js`:
- Line 47: The effect in RequireAdmin.js references sessionDetails and calls
updateSessionDetails but only lists orgId in its dependency array, causing stale
closures; update the useEffect dependency array to include sessionDetails and
updateSessionDetails (even if updateSessionDetails is stable) so the effect
re-runs when sessionDetails changes and satisfies react-hooks/exhaustive-deps,
and verify the logic inside the effect that compares sessionDetails.role and
sessionDetails.isAdmin still behaves correctly when re-invoked.
🧹 Nitpick comments (1)
frontend/src/components/helpers/auth/RequireAdmin.js (1)
35-36: Consider logging verification errors for debuggability.Silently swallowing all errors makes it difficult to diagnose issues when the profile API fails. At minimum, consider logging in development mode.
♻️ Proposed improvement
} catch (error) { - // On error, keep current session state + // On error, keep current session state but log for debugging + console.error("Admin verification failed:", error); } finally {
…/unstract into UN-2955-BUG-User-role-update-UI
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/src/components/helpers/auth/RequireAdmin.js`:
- Around line 43-47: When orgId becomes available later we must reset the
verification flag so protected content isn't shown briefly; in the useEffect
that calls verifyAdminStatus, set isVerifying to true whenever orgId is
present/changes (call setIsVerifying(true) immediately before invoking
verifyAdminStatus) and keep the existing setIsVerifying(false) in the else
branch; reference the state variable isVerifying, the setter setIsVerifying, and
the verifyAdminStatus function in RequireAdmin.js.
- Around line 30-31: The current truthy check prevents setting an intentionally
cleared role (null/empty); change the condition that assigns updates.role so it
updates when the backend returned a value including null but not when the field
is absent. Replace the guard `if (currentRole && currentRole !==
sessionDetails?.role)` with a check that ensures currentRole is defined (not
undefined) and different from the existing role, e.g. `if (currentRole !==
undefined && currentRole !== sessionDetails?.role) updates.role = currentRole;`
so null/empty from the API will overwrite the session while truly missing fields
are ignored.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/helpers/auth/RequireAdmin.js`:
- Around line 36-38: The empty catch in the RequireAdmin component's async
verification silently swallows failures; update that catch to accept an error
parameter, log the error (e.g., console.error or app logger) for visibility, and
handle auth-specific cases such as a 401 by clearing the auth/session state
(e.g., call the component's logout or setSession(null) helper) and redirecting
to the login route (or set a flag that forces re-auth). Keep finally behavior
intact (e.g., setIsVerifying(false)). Ensure you reference the same
error-handling block inside RequireAdmin so you don't change the surrounding
verification flow.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
frontend/src/components/helpers/auth/RequireAdmin.js
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
5 participants
What
RequireAdminroute guard mounts.Why
admin → user), the demoted user's frontend kept showingadmin-only pages becauseRequireAdminonly checked the locally cachedsessionDetails.isAdmin, set once at login and never refreshed.How
useEffectinRequireAdminthat callsGET /api/v1/unstract/{orgId}/users/profile/on mount.is_adminandrolewith the cached session values.updateSessionDetails()to patch the Zustand store, which triggers a re-render showing theUnauthorizedpage immediately.nullto avoid a flash of the wrong state.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.