Repository navigation
refactor(auth): unify pbac logic and remove hasAdminAccess - #33
Conversation
- Implement granular utility with wildcard support - Update to delegates to - Add permission to - Replace with check in Login/Signup - Remove legacy function
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughReplaces a hard-coded admin check with a generic hasPermission(permissions, resource, action) evaluator; updates call sites (login/signup), normalizes frontend resource/action mapping in AccessControlProvider, and expands RBAC seeds to finer-grained resource:action permissions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Login/Signup Page
participant Auth as AuthProvider/Session
participant ACP as AccessControlProvider
participant PermUtil as hasPermission
participant Identity as Identity Adapter/DB
Client->>Auth: request current user + permissions[]
Auth-->>Client: returns user + permissions[]
Client->>ACP: ask for route decision (resource, action)
ACP->>Auth: ensure permissions available (if needed)
ACP-->>ACP: normalize resource & map action
ACP->>PermUtil: hasPermission(permissions, resource, action)
PermUtil->>PermUtil: evaluate patterns (resource:action, resource:*, *:action, *)
PermUtil-->>ACP: boolean allowed/denied
ACP-->>Client: return decision (navigate /admin or /dashboard)
Note over Identity,Auth: Identity seeds provide granular permissions read by Auth/Adapter
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/identity/src/scripts/seed-rbac.ts (1)
59-73: Consider: Redundant permission assignments for owner role.The owner role is assigned both
users:manageand the individual CRUD actions (users:create,users:update,users:delete). The same applies to tenants. Ifmanageis intended to imply full access, the individual CRUD permissions are redundant.However, if explicit CRUD permissions are needed for granular checks elsewhere, this is acceptable. Please clarify the intent—either:
- Remove individual CRUD if
manageimplies all, or- Document that both are needed for specific check scenarios.
🤖 Fix all issues with AI agents
In `@apps/web/src/providers/access-control-provider.ts`:
- Around line 9-20: The resourceMap mapping for access control (variables:
resource, targetResource, resourceMap) only handles "users" and "tenants" and
will misclassify new frontend resources; update the logic to be defensive by
either expanding resourceMap with expected entries (e.g., "admin/settings" ->
"settings", "admin/dashboard" -> "dashboard") or normalize incoming resource
strings by stripping a leading "admin/" prefix and applying a fallback (e.g.,
use resource.replace(/^admin\//, "") or default to resource) before lookup, and
add a short comment or a console/processLogger warning when an unmapped resource
is encountered so future additions are evident.
- Around line 22-26: The access control check uses targetAction and calls
hasPermission(permissions, targetResource, targetAction) but doesn't normalize
frontend actions like "edit" to the backend's "update", causing useCan({ action:
"edit" }) to fail; update the logic that sets targetAction (the variable
computed from action || "manage") to map "edit" => "update" (and preserve other
actions), then pass the normalized action into hasPermission so checks match the
seeded permissions; reference targetAction and hasPermission in your change and
adjust any callers relying on the previous value (e.g., useCan/TenantList
usage).
In `@packages/identity/src/scripts/seed-rbac.ts`:
- Around line 25-26: The seeded permission with id "dashboard:view" is not
assigned to any role, so update the roleMap in seed-rbac.ts to include
"dashboard:view" for the owner and admin roles (add the permission id to the
arrays for the "owner" and "admin" entries in the roleMap). Ensure the roleMap
keys "owner" and "admin" reference the exact permission string "dashboard:view"
so hasPermission(user.permissions, 'dashboard', 'view') returns true for those
roles.
- access-control-provider: normalize 'admin/' prefix from resources and map frontend actions (edit->update) - seed-rbac: assign 'dashboard:view' to owner and admin roles
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/web/src/providers/access-control-provider.ts`:
- Around line 9-24: The resourceMap entries ("admin/users", "admin/tenants") are
unreachable because targetResource.startsWith("admin/") is handled first; remove
the redundant entries from resourceMap or move the resourceMap lookup before the
startsWith branch if you need non-prefix mappings. Update the normalization
logic in access-control-provider.ts (variables/functions: targetResource,
resourceMap, startsWith, replace) so either (A) delete the admin/* keys from
resourceMap and keep the startsWith replacement, or (B) consult
resourceMap[targetResource] before the startsWith check to allow explicit
mapping, ensuring no dead code remains.
In `@packages/identity/src/scripts/seed-rbac.ts`:
- Around line 59-74: The owner role in roleMap currently lists both broad
permissions like "users:manage" and each CRUD permission; replace the explicit
CRUD entries with resource wildcards (e.g., use "users:*", "tenants:*",
"settings:*" in roleMap under owner) to remove redundancy and rely on
hasPermission wildcard support, and update the seeding logic that creates
permissions (if any) to ensure wildcard permissions ("users:*", "tenants:*",
"settings:*") are created/stored so role assignment remains valid; adjust
references to roleMap and any permission creation functions accordingly.
| const roleMap: Record<string, string[]> = { | ||
| owner: [ | ||
| "dashboard:view", | ||
| "users:manage", | ||
| "users:read", | ||
| "users:create", | ||
| "users:update", | ||
| "users:delete", | ||
| "tenants:manage", | ||
| "tenants:read", | ||
| "tenants:create", | ||
| "tenants:update", | ||
| "tenants:delete", | ||
| "settings:manage", | ||
| "settings:read", | ||
| ], |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using resource wildcards to reduce redundancy.
The owner role lists both users:manage and all individual CRUD permissions (users:read, users:create, users:update, users:delete). Since hasPermission supports resource wildcards (users:*), you could simplify this to avoid maintaining duplicate entries.
This is optional since the explicit listing is correct and provides clear audit trails.
♻️ Alternative using wildcards
owner: [
"dashboard:view",
- "users:manage",
- "users:read",
- "users:create",
- "users:update",
- "users:delete",
- "tenants:manage",
- "tenants:read",
- "tenants:create",
- "tenants:update",
- "tenants:delete",
- "settings:manage",
- "settings:read",
+ "users:*",
+ "tenants:*",
+ "settings:*",
],Note: This requires seeding users:*, tenants:*, and settings:* as permissions if you want them stored in the database.
🤖 Prompt for AI Agents
In `@packages/identity/src/scripts/seed-rbac.ts` around lines 59 - 74, The owner
role in roleMap currently lists both broad permissions like "users:manage" and
each CRUD permission; replace the explicit CRUD entries with resource wildcards
(e.g., use "users:*", "tenants:*", "settings:*" in roleMap under owner) to
remove redundancy and rely on hasPermission wildcard support, and update the
seeding logic that creates permissions (if any) to ensure wildcard permissions
("users:*", "tenants:*", "settings:*") are created/stored so role assignment
remains valid; adjust references to roleMap and any permission creation
functions accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/web/src/providers/access-control-provider.ts`:
- Around line 9-24: The resource normalization currently strips the "admin/"
prefix before consulting resourceMap, causing explicit overrides like
"admin/users" in resourceMap to be ignored; change the logic in
access-control-provider so you consult resourceMap first (check
resourceMap[targetResource] and use that mapping if present), and only if there
is no explicit mapping then apply the prefix normalization (use
targetResource.replace(/^admin\//, "")). Also remove redundant entries from
resourceMap or keep them if you want to preserve explicit overrides for specific
keys (refer to variables/function names targetResource and resourceMap in the
diff).
In `@packages/identity/src/scripts/seed-rbac.ts`:
- Around line 25-26: Replace the inconsistent dashboard permission entry { id:
"dashboard:view", action: "view", resource: "dashboard" } with a consistent
read-based permission (e.g., id "dashboard:read" and action "read") so it
matches other resources like "users:read" and "settings:read"; update any
references to "dashboard:view" in permission checks or tests to "dashboard:read"
if present.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/identity/src/scripts/seed-rbac.ts (1)
89-100: Consider adding type safety for permission IDs.The
roleMapuses string literals for permission IDs without compile-time validation againststandardPerms. A typo in a permission ID would only be caught at runtime when the database insert fails due to foreign key constraints.♻️ Optional improvement for type safety
+ // Extract permission IDs as a type for compile-time safety + const permissionIds = standardPerms.map(p => p.id) as const; + type PermissionId = typeof standardPerms[number]['id']; + - const roleMap: Record<string, string[]> = { + const roleMap: Record<string, PermissionId[]> = { owner: [ "dashboard:read", // ... rest of permissions ], // ... };Alternatively, derive the type from
standardPermsto ensure permission strings inroleMapare validated at compile time.
🤖 Fix all issues with AI agents
In `@apps/web/src/providers/access-control-provider.ts`:
- Around line 14-35: resourceMap and actionMap are being recreated inside the
can() flow on every call; hoist these static maps to module scope to avoid
per-call allocation and centralize normalization config. Move the const
resourceMap and const actionMap declarations out of the can() function (or
whichever function contains this logic) into top-level module scope in
access-control-provider.ts, keep their keys/values unchanged, and update can()
to reference these top-level variables (ensure no local shadowing of
targetResource or action variables).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/providers/access-control-provider.ts`:
- Around line 13-21: ACTION_MAP contains redundant identity entries ("create":
"create" and "delete": "delete") that are no-ops because the code already falls
back to rawAction; remove those two entries from ACTION_MAP to simplify the map
(or alternatively leave them if you prefer explicit documentation), and ensure
any logic that uses ACTION_MAP (referencing ACTION_MAP and the fallback
rawAction) still works unchanged after removing them.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.