feat: (PBAC) Introduce depends on permission registery #23440
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (2)
Walkthrough
Possibly related PRs
Tip You can customize the tone of the review comments and chat replies.Set the ✨ Finishing Touches
🧪 Generate unit tests❌ Error creating Unit Test PR.
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts (2)
126-127: Fix permission parsing for dotted resources (e.g., "organization.attributes.read").Splitting on the first dot breaks hierarchical resource ids. Use lastIndexOf(".") to split once at the final dot.
Apply this diff:
- const [resource, action] = permission.split("."); + const lastDot = permission.lastIndexOf("."); + if (lastDot === -1) return newPermissions; // defensive + const resource = permission.slice(0, lastDot) as keyof typeof PERMISSION_REGISTRY; + const action = permission.slice(lastDot + 1) as CrudAction | CustomAction;And import CustomAction:
-import { CrudAction } from "@calcom/features/pbac/domain/types/permission-registry"; +import { CrudAction, CustomAction } from "@calcom/features/pbac/domain/types/permission-registry";
1-3: Add minimal helpers and memoized reverse index for O(k) toggles.Computing closure each toggle via full scans is O(N*M). Add small helpers with a cached reverse graph.
Add the following near the top of the module (outside the function):
type Registry = typeof PERMISSION_REGISTRY; type Permission = `${string}.${string}`; let reverseIndexCache: Map<Permission, Set<Permission>> | null = null; function buildReverseIndex(registry: Registry): Map<Permission, Set<Permission>> { const map = new Map<Permission, Set<Permission>>(); Object.entries(registry).forEach(([res, config]) => { Object.entries(config).forEach(([act, details]) => { if (act.startsWith("_")) return; const perm = `${res}.${act}` as Permission; const deps = (details as { dependsOn?: Permission[] })?.dependsOn ?? []; deps.forEach((d) => { if (!map.has(d)) map.set(d, new Set()); map.get(d)!.add(perm); }); // ensure key exists if (!map.has(perm)) map.set(perm, new Set()); }); }); return map; } function getReverseIndex(registry: Registry) { if (!reverseIndexCache) reverseIndexCache = buildReverseIndex(registry); return reverseIndexCache; } export function getTransitiveDependencies(perm: Permission, registry: Registry): Set<Permission> { // forward crawl const [res, act] = [perm.slice(0, perm.lastIndexOf(".")), perm.slice(perm.lastIndexOf(".")+1)]; const details = (registry as any)[res]?.[act] as { dependsOn?: Permission[] } | undefined; const visited = new Set<Permission>(); const stack = [...(details?.dependsOn ?? [])]; while (stack.length) { const d = stack.pop()!; if (visited.has(d)) continue; visited.add(d); const [dr, da] = [d.slice(0, d.lastIndexOf(".")), d.slice(d.lastIndexOf(".")+1)]; const dd = (registry as any)[dr]?.[da] as { dependsOn?: Permission[] } | undefined; if (dd?.dependsOn) stack.push(...dd.dependsOn); } return visited; } export function getTransitiveDependents(perm: Permission, registry: Registry): Set<Permission> { const rev = getReverseIndex(registry); const visited = new Set<Permission>(); const stack = [...(rev.get(perm) ?? [])]; while (stack.length) { const d = stack.pop()!; if (visited.has(d)) continue; visited.add(d); (rev.get(d) ?? []).forEach((n) => stack.push(n)); } return visited; }Note: invalidate reverseIndexCache if PERMISSION_REGISTRY is hot-reloaded.
Also applies to: 117-121
🧹 Nitpick comments (9)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts (5)
145-151: Keep or drop the read auto-enable? Prefer relying on registry for consistency.If registry encodes Create/Update/Delete → Read (it does), this block is redundant and risks drift. Either remove it or guard behind a feature flag for backward compatibility.
Apply this diff to remove redundancy:
- // If enabling create, update, or delete, automatically enable read permission (backward compatibility) - if (action === CrudAction.Create || action === CrudAction.Update || action === CrudAction.Delete) { - const readPermission = `${resource}.${CrudAction.Read}`; - if (!newPermissions.includes(readPermission)) { - newPermissions.push(readPermission); - } - }
14-15: Type tightening and safety nits.
- Prefer PermissionString over string for APIs (toggleSinglePermission, hasAllPermissions) to reduce invalid inputs.
- Use keyof typeof resourceConfig for action indexing.
Apply this diff:
- hasAllPermissions: (permissions: string[]) => boolean; + hasAllPermissions: (permissions: PermissionString[]) => boolean; @@ - toggleSinglePermission: (permission: string, enabled: boolean, currentPermissions: string[]) => string[]; + toggleSinglePermission: (permission: PermissionString, enabled: boolean, currentPermissions: PermissionString[]) => PermissionString[];And within toggleSinglePermission, replace:
- if (resourceConfig && resourceConfig[action as CrudAction]) { - const permissionDetails = resourceConfig[action as CrudAction]; + if (resourceConfig && (action in resourceConfig)) { + const permissionDetails = (resourceConfig as Record<string, any>)[action] as { dependsOn?: PermissionString[] } | undefined;Also applies to: 66-115, 179-185
72-81: Global “select all” correctness check.When selecting All (“”), you add both "." and all individual perms. If you rely on "." elsewhere for checks, consider returning only ".*" here to keep state minimal; recompute individuals when needed.
Apply this diff if desired:
- const allPossiblePerms = getAllPossiblePermissions(); - newPermissions = ["*.*", ...allPossiblePerms]; + newPermissions = ["*.*"];
179-185: Avoid re-adding duplicate ".".Edge case: if hasAllPermissions returns true while "." already exists (future refactors), guard before push.
Apply this diff:
- if (hasAllPermissions(newPermissions)) { - newPermissions.push("*.*"); - } + if (hasAllPermissions(newPermissions) && !newPermissions.includes("*.*")) { + newPermissions.push("*.*"); + }
117-151: Add tests for the exact scenarios called out in the PR.
- Enabling organization.changeMemberRole should auto-enable organization.listMembers, organization.read, and role.read.
- Disabling organization.read should remove listMembers and changeMemberRole.
I can add unit tests for toggleSinglePermission covering transitive enable/disable and dotted resources. Want me to open a follow-up PR with tests?
Also applies to: 153-177
packages/features/pbac/domain/types/permission-registry.ts (4)
44-45: Interface extension LGTM; optional docs on transitivity.dependsOn field addition is good. Document that dependencies are acyclic permission strings and intended to be transitively closed by consumers.
Apply this diff:
- dependsOn?: PermissionString[]; // Dependencies that must be enabled when this permission is enabled + dependsOn?: PermissionString[]; // Direct prerequisites (acyclic). Consumers should resolve transitively.
121-147: Data looks consistent; ensure it remains cycle-free and referentially valid.Great coverage of CRUD→read and custom actions. Given UI now relies on this, add a dev-only invariant to:
- assert each dependsOn points to an existing permission key
- detect cycles
I can add a small validation that runs in dev/test. Example helper:
export function validatePermissionRegistry(reg: PermissionRegistry) { const perms = new Set<string>(); const edges = new Map<string, string[]>(); Object.entries(reg).forEach(([res, cfg]) => { Object.entries(cfg).forEach(([act, d]) => { if (act.startsWith("_")) return; const id = `${res}.${act}`; perms.add(id); const deps = (d as PermissionDetails).dependsOn ?? []; edges.set(id, deps); }); }); // referential integrity for (const [id, deps] of edges) { for (const dep of deps) { if (!perms.has(dep)) throw new Error(`PBAC: "${id}" depends on missing permission "${dep}"`); } } // cycle detection const seen = new Set<string>(), stack = new Set<string>(); const dfs = (n: string) => { if (stack.has(n)) throw new Error(`PBAC: dependency cycle detected at "${n}"`); if (seen.has(n)) return; seen.add(n); stack.add(n); (edges.get(n) ?? []).forEach(dfs); stack.delete(n); }; perms.forEach(dfs); }Call once in non-production.
Also applies to: 153-180, 185-234, 253-301, 306-342, 358-385, 390-417, 422-449
286-293: Copy text nit: “organization” vs “team”.Organization.ChangeMemberRole description says “team members”. Prefer “organization members”.
Apply this diff:
- description: "Change role of team members", + description: "Change role of organization members",
254-260: Optional: include transitive deps explicitly if UI doesn’t compute closure.If you decide not to implement transitive resolution in the hook, add organization.read to invite/remove/changeMemberRole (they already depend on listMembers which depends on read). Otherwise keep current minimal form.
Would you prefer registry to be transitively complete or keep it minimal and let the hook resolve transitively?
Also applies to: 262-276, 278-285, 286-293, 294-301
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts(1 hunks)packages/features/pbac/domain/types/permission-registry.ts(14 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.tspackages/features/pbac/domain/types/permission-registry.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.tspackages/features/pbac/domain/types/permission-registry.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.tspackages/features/pbac/domain/types/permission-registry.ts
🧬 Code graph analysis (1)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts (1)
packages/features/pbac/domain/types/permission-registry.ts (1)
PERMISSION_REGISTRY(104-450)
...e-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts
Outdated
Show resolved
Hide resolved
...e-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts
Outdated
Show resolved
Hide resolved
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Co-authored-by: Eunjae Lee <[email protected]>
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 (2)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts (2)
86-90: Don’t use startsWith for resource filtering; it drops sibling resources (critical).
organization.*filtering will also removeorganization.attributes.*because of the shared prefix.Apply this diff:
- newPermissions = newPermissions.filter((p) => !p.startsWith(`${resource}.`)); + newPermissions = newPermissions.filter((p) => { + const i = p.lastIndexOf("."); + if (i === -1) return true; + const pResource = p.slice(0, i); + return pResource !== resource; + });
104-109: Add transitive dependencies when setting a resource to “all”.Some actions depend on other resources (e.g., organization.changeMemberRole → role.read). Current code doesn’t add cross-resource deps.
Apply this diff:
- allResourcePerms = Object.keys(resourceConfig) + allResourcePerms = Object.keys(resourceConfig) .filter((action) => !action.startsWith("_")) .map((action) => `${resource}.${action}`); newPermissions.push(...allResourcePerms); + // Add transitive dependencies for all added actions + const depSet = new Set<string>(); + allResourcePerms.forEach((p) => { + getTransitiveDependencies(p).forEach((d) => depSet.add(d)); + }); + depSet.forEach((d) => { + if (!newPermissions.includes(d)) newPermissions.push(d); + });
🧹 Nitpick comments (5)
packages/features/pbac/utils/permissionTraversal.ts (2)
9-12: Tighten types: use PermissionString and removeany.Strengthen the API and avoid unsafe casts.
Apply this diff:
-import { CrudAction, type CustomAction, PERMISSION_REGISTRY } from "../domain/types/permission-registry"; +import { CrudAction, type CustomAction, PERMISSION_REGISTRY, type PermissionString } from "../domain/types/permission-registry"; @@ -export const traversePermissions = ( - startPermission: string, +export const traversePermissions = ( + startPermission: PermissionString, @@ -export const getTransitiveDependencies = (permission: string): string[] => { +export const getTransitiveDependencies = (permission: PermissionString): string[] => { return traversePermissions(permission, "dependencies"); }; @@ -export const getTransitiveDependents = (permission: string): string[] => { +export const getTransitiveDependents = (permission: PermissionString): string[] => { return traversePermissions(permission, "dependents"); };Also consider replacing
as anyon Line 56 with a precise type derived from the registry to prevent accidental shape drift.Also applies to: 92-95, 101-103
52-80: Optional: build a reverse index for dependents to avoid O(N) scans per BFS step.Precompute
dependsOn -> [dependents]once (module scope) and reuse in traversal.I can sketch a small index builder if helpful.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts (3)
129-135: Remove unused destructuring and avoid duplicates when enabling.
resource/actionare unused, and pushing the same permission repeatedly causes churn.Apply this diff:
- // Parse the permission to get resource and action - const [resource, action] = permission.split("."); - if (enabled) { // Add the requested permission - newPermissions.push(permission); + if (!newPermissions.includes(permission)) { + newPermissions.push(permission); + }
36-43: Optional: memoize hasAllPermissions for large registries.This runs often in the UI; a simple memo keyed by the permission set hash can help.
70-83: Cache all permissions for “all resources” toggling
getAllPossiblePermissions is invoked on every toggle whenresource === "*". Move its result into a module-level constant (e.g.ALL_PERMISSIONS) to avoid rebuilding the array each time, and verify it contains the correct count of unique entries.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts(2 hunks)packages/features/pbac/utils/permissionTraversal.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/pbac/utils/permissionTraversal.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/pbac/utils/permissionTraversal.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/pbac/utils/permissionTraversal.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts
🧬 Code graph analysis (2)
packages/features/pbac/utils/permissionTraversal.ts (1)
packages/features/pbac/domain/types/permission-registry.ts (1)
PERMISSION_REGISTRY(104-450)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts (1)
packages/features/pbac/utils/permissionTraversal.ts (2)
getTransitiveDependencies(92-94)getTransitiveDependents(101-103)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (2)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts (2)
136-143: Good switch to transitive closure.Enabling via
getTransitiveDependenciesand removing viagetTransitiveDependentsaddresses multi-hop chains and keeps state consistent.Also applies to: 144-152
1-7: Imports look correct; prefer named utilities and keep traversal local to features.No default exports and feature-local utils are aligned with guidelines.
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
E2E results are ready! |
What does this PR do?
Fixes the issue @CarinaWolli mentioned here where its possible to have a confusing logic of permissions. So we are introducing a dependsOn registery item.
#23004 (comment)
Video Demo (if applicable):
How should this be tested?
Run yarn seed-pbac
Create a new role.
Toggle ChangeMember Role
Notice the permissions in dependsOn get toggled