Repository navigation
feat(web): migrate redirects and access control to granular permissions - #32
Conversation
📝 WalkthroughWalkthroughMoves UI access from role-based checks to permission/capability-based checks: adds permissions to AuthUser, surfaces permissions from authProvider, introduces an accessControlProvider, and replaces systemRole guards with useCan/permission checks across admin pages and lists. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
apps/web/src/modules/identity/users/UserList.tsx (1)
54-58: Consider handling the permission check loading state.While
canCreate?.cansafely handles undefined, theuseCanhook also returnsisLoading. During the brief loading period, the create button will be hidden. If this causes a noticeable UI shift, consider showing a skeleton or placeholder.♻️ Optional: Handle loading state
- const { data: canCreate } = useCan({ + const { data: canCreate, isLoading: isPermissionLoading } = useCan({ resource: "admin/users", action: "create", });Then conditionally render a placeholder during loading if needed.
apps/web/src/modules/identity/tenants/TenantList.tsx (1)
119-119: Consider hiding the Actions column header when actions are unavailable.The "Actions" header is always rendered, but the cell content is conditionally hidden based on
showActions. This leaves an empty column for users without permissions.♻️ Conditionally render Actions column
- <TableHead className="w-[80px] text-right">Actions</TableHead> + {showActions && <TableHead className="w-[80px] text-right">Actions</TableHead>}And update the corresponding
TableCellto also be conditional:- <TableCell className="text-right"> - {showActions && ( + {showActions && ( + <TableCell className="text-right"> ... - )} - </TableCell> + </TableCell> + )}Also update
colSpanin the empty state and expanded row accordingly.apps/web/src/providers/auth-provider.ts (1)
65-76: Note: Multiple session fetches may occur.Both
getIdentity()andgetPermissions()independently callauthClient.getSession(). When used together (e.g., in access control provider), this could result in duplicate requests.Consider whether
authClient.getSession()has internal caching. If not, a shared session fetch helper within this provider could reduce redundant calls:♻️ Optional: Share session fetch
// Helper to get cached session let cachedSession: { data: any; timestamp: number } | null = null; const SESSION_TTL = 5000; // 5 seconds async function getCachedSession() { const now = Date.now(); if (cachedSession && (now - cachedSession.timestamp) < SESSION_TTL) { return cachedSession.data; } const session = await authClient.getSession(); cachedSession = { data: session, timestamp: now }; return session; }apps/web/src/pages/auth/LoginPage.tsx (1)
66-82: Consider using login response data to determine redirect target.The current flow for admin users causes a double navigation:
- Login succeeds → navigates to
/dashboard(line 82)- Component re-renders with authenticated user →
useEffectredirects to/admin(line 33)The comment at lines 73-76 acknowledges this could be improved. Since
data.user.permissionsshould be available from the login response, consider checking it directly:♻️ Proposed improvement
const toParam = searchParams.get('to'); // Only allow relative paths to prevent open redirect attacks const isValidRedirect = toParam && toParam.startsWith('/') && !toParam.startsWith('//'); - const redirectUrl = isValidRedirect ? toParam : fallback; + + // Determine fallback based on permissions from login response + const adminPermissions = ['*', 'tenants:manage', 'tenants:read', 'users:manage']; + const hasAdminAccess = data.user?.permissions?.some( + (p: string) => adminPermissions.includes(p) || p.startsWith('system_') + ); + const smartFallback = hasAdminAccess ? '/admin' : '/dashboard'; + + const redirectUrl = isValidRedirect ? toParam : smartFallback; navigate(redirectUrl);apps/web/src/modules/identity/tenants/TenantList.spec.tsx (2)
171-226: Fix conflictinguseCanmocks and remove debug comments.This skipped test has multiple issues:
Conflicting mocks:
useCanis mocked twice with opposite values:
- Line 173:
{ can: false }- Line 195:
{ can: true }(overrides the previous)Debug comments (lines 197-209, 222-224) explain the mocking confusion but should be cleaned up.
To properly test the read-only scenario, remove the second mock and the debug comments:
🔧 Proposed fix
it.skip('shows read-only status badge for platform_user', () => { // eslint-disable-next-line `@typescript-eslint/no-explicit-any` vi.mocked(useCan).mockImplementation(() => ({ data: { can: false } } as any)); const mockUser = { // ... user setup }; const mockAuthValue = { // ... auth value setup }; - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - vi.mocked(useCan).mockImplementation(() => ({ data: { can: true } } as any)); - - // For this test, useCan should return FALSE. - // We need to override the mock. - // ... (remove all debug comments through line 209) const client = createQueryClient(); render( // ... render code ); - // If useCan returns true (default mock), this will fail. - // Check failure first. const statusButtons = screen.queryAllByRole('button', { name: /active|suspended/i }); expect(statusButtons.length).toBe(0); });
228-267: Same issue: conflictinguseCanmocks in skipped test.This test has the same problem as the previous one—
useCanis mocked withcan: falseat line 230, then overridden tocan: trueat line 251. Remove the second mock (line 250-251) to fix this test when it's unskipped.apps/web/src/layouts/AdminLayout.tsx (1)
140-158: Add permission check to AdminLayout before rendering admin content.The layout renders for any authenticated user but should also verify admin permissions. While some child components use
useCan()for UI control (UserList, UserShow), this is insufficient as non-admin users can still navigate to admin routes. AdminDashboardPage lacks any permission check and renders unconditionally. Add a permission guard in AdminLayout before rendering the layout outlet:const { data: canAccess } = useCan({ resource: "admin", action: "read", }); if (canAccess?.can === false) { return ( <div className="flex items-center justify-center h-screen bg-background text-muted-foreground"> Access denied. </div> ); }This ensures only users with admin permissions can access the entire admin panel, not just authenticated users.
🤖 Fix all issues with AI agents
In `@apps/web/src/layouts/AdminLayout.spec.tsx`:
- Around line 84-92: Remove the speculative inline comments inside the test
"renders content when user is platform_user"; update the test to be explicit by
either asserting the expected behavior for a user with systemRole
'platform_user' and permissions ['tenants:read'] or delete the comment lines
entirely. Locate the mock for useAuth in this test and the user fixture/object
being passed (user: { name: 'Staff', systemRole: 'platform_user', permissions:
['tenants:read'] }) and ensure the test contains a clear assertion about
rendered content or access rather than question-style comments.
In `@apps/web/src/pages/auth/LoginPage.tsx`:
- Around line 27-33: LoginPage and SignupPage use different admin-permission
checks causing inconsistent redirects; extract the logic into a shared utility
function (e.g., hasAdminAccess(permissions: string[] | undefined)) that returns
true if permissions include '*' or any of
['tenants:manage','tenants:read','users:manage'] or start with 'system_' or
'admin:'; then replace the inline checks in LoginPage (where hasAdminAccess is
currently computed) and in SignupPage with calls to this utility so both pages
use the exact same admin-detection logic.
In `@apps/web/src/pages/auth/SignupPage.tsx`:
- Line 94: Remove the unused systemRole property from the type assertion on
sessionData.user in SignupPage.tsx: replace the cast `sessionData.user as {
systemRole?: string; permissions?: string[] }` with a type that only includes
the used permissions field (e.g., `sessionData.user as { permissions?: string[]
}` or preferably the existing User type/interface if available), and update any
imports or references to use that proper user type so systemRole is no longer
present in the code.
- Around line 94-96: The admin-permission check is wrong: replace the
startsWith('admin:') fallback with a check that matches the actual permission
format "resource:action" (e.g., 'system_users:manage'). Update the
hasAdminAccess computation (where sessionData.user is read into user) to true if
user.permissions includes '*' OR any permission where
permission.split(':')[0].startsWith('system_') or matches a short list of system
admin permissions like 'system_users:manage' / 'system_tenants:manage'; adjust
the expression accordingly to iterate user.permissions and inspect the resource
portion before the ':'.
In `@apps/web/src/providers/access-control-provider.ts`:
- Around line 36-45: The hardcoded resourceMap and its assignment to
targetResource make adding new admin resources brittle; either document this
mapping with a clear comment above resourceMap explaining it must be updated
when new resources are added, or refactor to make it configurable (e.g., accept
a mapping argument or load from configuration/environment and fall back to the
current resourceMap). Update the code that references resourceMap and
targetResource so it reads from the external/ injected mapping (or retains the
documented constant) and add a short inline comment describing the update
process and where to change mappings.
| it('renders content when user is platform_user', () => { | ||
| (useAuth as unknown as Mock).mockReturnValue({ | ||
| isLoading: false, | ||
| isAuthenticated: true, | ||
| user: { name: 'Staff', systemRole: 'platform_user' }, | ||
| // platform_user with no admin permissions should see content but tailored? | ||
| // Actually AdminLayout protects the route. If platform_user is allowed, they must have some perms? | ||
| // Or maybe AdminLayout allows access but shows limited menu? | ||
| // Assuming platform_user is generic employee. | ||
| user: { name: 'Staff', systemRole: 'platform_user', permissions: ['tenants:read'] }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove speculative commentary in the test.
The inline comments read as uncertainty; prefer explicit assertions or remove to keep intent clear.
🤖 Prompt for AI Agents
In `@apps/web/src/layouts/AdminLayout.spec.tsx` around lines 84 - 92, Remove the
speculative inline comments inside the test "renders content when user is
platform_user"; update the test to be explicit by either asserting the expected
behavior for a user with systemRole 'platform_user' and permissions
['tenants:read'] or delete the comment lines entirely. Locate the mock for
useAuth in this test and the user fixture/object being passed (user: { name:
'Staff', systemRole: 'platform_user', permissions: ['tenants:read'] }) and
ensure the test contains a clear assertion about rendered content or access
rather than question-style comments.
| can: async ({ resource, action }: CanParams): Promise<CanReturnType> => { | ||
| // Fetch permissions fresh (or cached by AuthProvider/Client) | ||
| // Note: authProvider.getPermissions is implemented now. | ||
| // We cast to string[] strictly. | ||
| const permissions = (await authProvider.getPermissions?.()) as string[] ?? []; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the access-control-provider.ts file
fd -e ts "access-control-provider" 2>/dev/nullRepository: pramodnarayana/nexiom
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Read the full access-control-provider.ts file
cat -n apps/web/src/providers/access-control-provider.tsRepository: pramodnarayana/nexiom
Length of output: 3616
🏁 Script executed:
#!/bin/bash
# Find and examine the auth-provider
fd -e ts "auth-provider" 2>/dev/nullRepository: pramodnarayana/nexiom
Length of output: 251
🏁 Script executed:
#!/bin/bash
# Read the auth-provider.ts file
cat -n apps/web/src/providers/auth-provider.tsRepository: pramodnarayana/nexiom
Length of output: 3581
🏁 Script executed:
#!/bin/bash
# Find the auth-client implementation
fd -e ts -e js "auth-client" --type f 2>/dev/null | grep -v node_modules | grep -v ".spec"Repository: pramodnarayana/nexiom
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Read the auth-client.ts file
cat -n apps/web/src/lib/auth-client.tsRepository: pramodnarayana/nexiom
Length of output: 958
🏁 Script executed:
#!/bin/bash
# Search for better-auth documentation/package info and caching behavior
fd -e json "package.json" -type f 2>/dev/null | head -5 | xargs grep -l "better-auth" 2>/dev/nullRepository: pramodnarayana/nexiom
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Find package.json and check better-auth version
cat package.json | grep -A 2 -B 2 "better-auth" 2>/dev/null || echo "Checking all package.json files..."
find . -name "package.json" -type f 2>/dev/null | xargs cat 2>/dev/null | grep "better-auth"Repository: pramodnarayana/nexiom
Length of output: 319
🏁 Script executed:
#!/bin/bash
# Search for any caching, memoization, or session caching patterns in the codebase
echo "=== Searching for cache/memoization patterns ==="
rg -n "cache|memo|Cache|useMemo|useCallback|sessionCache" --type=ts --type=tsx -C 2 apps/web/src/providers/ 2>/dev/null
echo ""
echo "=== Checking if getSession is called multiple times ==="
rg -n "getSession|getPermissions" --type=ts --type=tsx apps/web/src/ 2>/dev/null | head -20Repository: pramodnarayana/nexiom
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Search more broadly for getSession and getPermissions usage
echo "=== Searching for all getSession calls ==="
rg -n "getSession" --type=ts --type=tsx 2>/dev/null
echo ""
echo "=== Searching for all getPermissions calls ==="
rg -n "getPermissions" --type=ts --type=tsx 2>/dev/null
echo ""
echo "=== Searching for useCan hook usage ==="
rg -n "useCan" --type=ts --type=tsx 2>/dev/nullRepository: pramodnarayana/nexiom
Length of output: 196
🏁 Script executed:
#!/bin/bash
# Try without type filters to find usages
echo "=== Searching for getSession ==="
rg "getSession" 2>/dev/null
echo ""
echo "=== Searching for getPermissions ==="
rg "getPermissions" 2>/dev/null
echo ""
echo "=== Searching for useCan ==="
rg "useCan" 2>/dev/null
echo ""
echo "=== Check access-control-provider usage ==="
rg "accessControlProvider" 2>/dev/nullRepository: pramodnarayana/nexiom
Length of output: 13934
🏁 Script executed:
#!/bin/bash
# Check the AuthProvider component to see if there's any caching
cat -n apps/web/src/lib/auth/AuthProvider.tsxRepository: pramodnarayana/nexiom
Length of output: 11551
🏁 Script executed:
#!/bin/bash
# Search for better-auth caching behavior documentation or implementation hints
echo "=== Checking for better-auth session caching in node_modules or imports ==="
rg "cache|memo|Cache|session.*cache" --type=ts 2>/dev/null | grep -i "better\|auth" | head -10
echo ""
echo "=== Checking how RefineProvider uses accessControlProvider ==="
cat -n apps/web/src/routes/AdminRoutes.tsx | head -50Repository: pramodnarayana/nexiom
Length of output: 2652
Consider caching permissions to prevent redundant getSession() calls.
getPermissions() is called on every can() invocation. With multiple useCan hooks rendered simultaneously (found in TenantListPage, UserShow, UserList, TenantList), this results in redundant HTTP requests. better-auth's createAuthClient() does not provide built-in caching for getSession() calls.
Implement memoization at the provider level or add a short TTL cache to authProvider.getPermissions() to avoid repeated session lookups during the same render cycle.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/pages/auth/LoginPage.tsx (1)
64-80: Post-login redirect may cause double navigation for admin users.After successful login, the code redirects to
/dashboard(fallback), then theuseEffectruns and may redirect admin users to/admin. This causes unnecessary navigation and potential UI flicker.Consider checking permissions from the login response (
data.user.permissions) to redirect correctly on the first navigation, similar to how SignupPage handles it.🔧 Suggested fix
// Call AuthProvider to set state setAuthState(data); - - - // Redirect logic: Respect ?to param, then Fallback + // Redirect logic: Respect ?to param, then permission-based fallback const searchParams = new URLSearchParams(window.location.search); - - // Default fallback - const fallback = '/dashboard'; - - // Note: systemRole check removed in favor of strict permissions. - // We rely on the useEffect above to redirect if they land on /login while authenticated. - // Or if we wanted to be fancy, we'd check data.user.permissions here if available. - // For now, simple fallback is safe. + + // Determine fallback based on user permissions + const userPermissions = data.user?.permissions as string[] | undefined; + const fallback = hasAdminAccess(userPermissions) ? '/admin' : '/dashboard'; const toParam = searchParams.get('to'); // Only allow relative paths to prevent open redirect attacks const isValidRedirect = toParam && toParam.startsWith('/') && !toParam.startsWith('//'); const redirectUrl = isValidRedirect ? toParam : fallback; navigate(redirectUrl);
🤖 Fix all issues with AI agents
In `@apps/web/src/lib/auth/utils.ts`:
- Around line 19-21: The current explicit systemPermissions array includes
'tenants:read', which—because the 'member' role is seeded with that
permission—accidentally grants members admin-dashboard access; fix by choosing
one of two actions: either remove 'tenants:read' from the systemPermissions
constant (leave only 'tenants:manage' and 'users:manage' in systemPermissions)
so read-only tenants access no longer implies admin access, or remove
'tenants:read' from the seeded 'member' role in the RBAC seed data so members
won't gain system-level admin access; update the symbol systemPermissions or the
seeded role definition for 'member' accordingly.
In `@apps/web/src/providers/access-control-provider.ts`:
- Around line 54-59: The custom-action permission check only tests for an exact
`${targetResource}:${action}` match; update that branch in
access-control-provider.ts (the block using variables targetResource, action,
and permissions) to also allow `${targetResource}:manage` as a fallback so users
with manage permission can perform custom actions (i.e., return { can: true } if
permissions includes either the exact `${targetResource}:${action}` or
`${targetResource}:manage`).
| // Explicit System Permissions | ||
| const systemPermissions = ['tenants:manage', 'tenants:read', 'users:manage']; | ||
| if (permissions.some(p => systemPermissions.includes(p))) return true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how tenants:read permission is used across the codebase
echo "=== Searching for tenants:read usage ==="
rg -n "tenants:read" --type=ts -C3
echo ""
echo "=== Check seed data for permission assignments ==="
rg -n "tenants:read|tenants:manage|users:manage" --type=ts -C2 | head -80Repository: pramodnarayana/nexiom
Length of output: 8895
Clarify the intended scope of tenants:read permission for admin access.
Including tenants:read in systemPermissions is documented as intentional (see line 7 comment), but the seed data reveals a design issue: the member role is assigned tenants:read (line 52 in seed-rbac.ts), which unintentionally grants members admin dashboard access. This doesn't align with typical RBAC practices where read-only permissions shouldn't enable admin-level features. Either:
- Remove
tenants:readfromsystemPermissionsand limit totenants:manageonly, or - Remove
tenants:readfrom thememberrole to prevent unintended admin access.
🤖 Prompt for AI Agents
In `@apps/web/src/lib/auth/utils.ts` around lines 19 - 21, The current explicit
systemPermissions array includes 'tenants:read', which—because the 'member' role
is seeded with that permission—accidentally grants members admin-dashboard
access; fix by choosing one of two actions: either remove 'tenants:read' from
the systemPermissions constant (leave only 'tenants:manage' and 'users:manage'
in systemPermissions) so read-only tenants access no longer implies admin
access, or remove 'tenants:read' from the seeded 'member' role in the RBAC seed
data so members won't gain system-level admin access; update the symbol
systemPermissions or the seeded role definition for 'member' accordingly.
| if (action !== "list" && action !== "show" && action !== "create" && action !== "edit" && action !== "delete") { | ||
| // Custom action (exact match) | ||
| if (permissions.includes(`${targetResource}:${action}`)) { | ||
| return { can: true }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Custom action check should also consider manage permission as fallback.
Custom actions (like invite) only check for exact match (system_users:invite), but if a user has system_users:manage, they should implicitly have permission for custom actions too. This is inconsistent with lines 70-72 where manage implies read.
🔧 Suggested fix
// Also allow if specific permission exists (e.g. system_users:invite)
// If action is custom, e.g. "invite", check "system_users:invite"
if (action !== "list" && action !== "show" && action !== "create" && action !== "edit" && action !== "delete") {
// Custom action (exact match)
if (permissions.includes(`${targetResource}:${action}`)) {
return { can: true };
}
+ // Allow if user has manage permission (implies all actions)
+ if (permissions.includes(`${targetResource}:manage`)) {
+ return { can: true };
+ }
}📝 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.
| if (action !== "list" && action !== "show" && action !== "create" && action !== "edit" && action !== "delete") { | |
| // Custom action (exact match) | |
| if (permissions.includes(`${targetResource}:${action}`)) { | |
| return { can: true }; | |
| } | |
| } | |
| if (action !== "list" && action !== "show" && action !== "create" && action !== "edit" && action !== "delete") { | |
| // Custom action (exact match) | |
| if (permissions.includes(`${targetResource}:${action}`)) { | |
| return { can: true }; | |
| } | |
| // Allow if user has manage permission (implies all actions) | |
| if (permissions.includes(`${targetResource}:manage`)) { | |
| return { can: true }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@apps/web/src/providers/access-control-provider.ts` around lines 54 - 59, The
custom-action permission check only tests for an exact
`${targetResource}:${action}` match; update that branch in
access-control-provider.ts (the block using variables targetResource, action,
and permissions) to also allow `${targetResource}:manage` as a fallback so users
with manage permission can perform custom actions (i.e., return { can: true } if
permissions includes either the exact `${targetResource}:${action}` or
`${targetResource}:manage`).
1 participant
Summary by CodeRabbit
New Features
Changes
✏️ Tip: You can customize this high-level summary in your review settings.