Conversation
WalkthroughIntroduces privacy-aware permissions and enforcement: adds CustomAction.ListMembersPrivate, extends PermissionDetails with visibleWhen, updates getPermissionsForScope to accept an optional isPrivate flag and filter by privacy, and updates PERMISSION_REGISTRY entries. UI components (RolesList, RoleSheet, AdvancedPermissionGroup, SimplePermissionItem) accept and propagate an isPrivate prop. Pages fetch/cache team/org isPrivate and pass it downstream. Member-listing handlers switch to PermissionCheckService and use privacy-specific permission strings/fallbacks. A SQL migration grants listMembersPrivate to admin_role and tests were updated for public/private cases. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
E2E results are ready! |
![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 (5)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/AdvancedPermissionGroup.tsx (1)
26-38: Prop added, but togglers aren’t privacy-aware (mismatch).You pass isPrivate into the component, but usePermissions(scope) still uses the non-privacy registry. Toggling “all/read” or single permissions can add hidden/irrelevant permissions (e.g., team.listMembers on private teams).
- Extend usePermissions to accept isPrivate and internally use getPermissionsForScopeAndPrivacy for:
- getAllPossiblePermissions
- hasAllPermissions
- getResourcePermissionLevel
- toggleResourcePermissionLevel
- toggleSinglePermission
- Pass isPrivate into usePermissions here.
Apply this diff:
- const { toggleSinglePermission, toggleResourcePermissionLevel } = usePermissions(scope); + const { toggleSinglePermission, toggleResourcePermissionLevel } = usePermissions(scope, isPrivate);And update usePermissions (outside this file) to accept isPrivate and to filter dependencies against the privacy-scoped registry before adding them. Example change (apps/.../usePermissions.ts):
export function usePermissions(scope: Scope = Scope.Organization, isPrivate = false): UsePermissionsReturn { const scopedRegistry = useMemo(() => getPermissionsForScopeAndPrivacy(scope, isPrivate), [scope, isPrivate]); const getAllPossiblePermissions = () => { const permissions: string[] = []; Object.entries(scopedRegistry).forEach(([resource, config]) => { if (resource !== "*") { Object.keys(config) .filter((action) => !action.startsWith("_")) .forEach((action) => permissions.push(`${resource}.${action}`)); } }); return permissions; }; const hasAllPermissions = (permissions: string[]) => Object.entries(scopedRegistry).every(([resource, config]) => { if (resource === "*") return true; return Object.keys(config) .filter((a) => !a.startsWith("_")) .every((a) => permissions.includes(`${resource}.${a}`)); }); const getResourcePermissionLevel = (resource: string, permissions: string[]): PermissionLevel => { if (resource === "*") return permissions.includes("*.*") ? "all" : "none"; const resourceConfig = scopedRegistry[resource as keyof typeof scopedRegistry]; if (!resourceConfig) return "none"; if (permissions.includes("*.*")) return "all"; const allResourcePerms = Object.keys(resourceConfig) .filter((a) => !a.startsWith("_")) .map((a) => `${resource}.${a}`); const hasAllPerms = allResourcePerms.every((p) => permissions.includes(p)); const hasReadPerm = permissions.includes(`${resource}.${CrudAction.Read}`); if (hasAllPerms) return "all"; if (hasReadPerm) return "read"; return "none"; }; const filterToScoped = (perms: string[]) => { const allowed = new Set<string>(); Object.entries(scopedRegistry).forEach(([res, cfg]) => { Object.keys(cfg) .filter((a) => !a.startsWith("_")) .forEach((a) => allowed.add(`${res}.${a}`)); }); return perms.filter((p) => allowed.has(p)); }; const toggleResourcePermissionLevel = (resource: string, level: PermissionLevel, current: string[]): string[] => { let newPermissions = current.filter((p) => p !== "*.*"); if (resource === "*") { if (level === "all") newPermissions = ["*.*", ...getAllPossiblePermissions()]; } else { newPermissions = newPermissions.filter((p) => !p.startsWith(`${resource}.`)); const resourceConfig = scopedRegistry[resource as keyof typeof scopedRegistry]; if (!resourceConfig) return current; switch (level) { case "read": newPermissions.push(`${resource}.${CrudAction.Read}`); break; case "all": { const allResourcePerms = Object.keys(resourceConfig) .filter((a) => !a.startsWith("_")) .map((a) => `${resource}.${a}`); newPermissions.push(...allResourcePerms); // ensure dependencies are also filtered by scoped registry allResourcePerms.forEach((perm) => { getTransitiveDependencies(perm, scope /* + isPrivate if you extend helpers */).forEach((dep) => { if (!newPermissions.includes(dep)) newPermissions.push(dep); }); }); newPermissions = filterToScoped(newPermissions); break; } case "none": default: break; } if (hasAllPermissions(newPermissions)) newPermissions.push("*.*"); } return filterToScoped(newPermissions); }; const toggleSinglePermission = (permission: string, enabled: boolean, current: string[]): string[] => { let newPermissions = current.filter((p) => p !== "*.*"); if (enabled) { newPermissions.push(permission); getTransitiveDependencies(permission, scope /* + isPrivate */).forEach((dep) => { if (!newPermissions.includes(dep)) newPermissions.push(dep); }); } else { newPermissions = newPermissions.filter((p) => p !== permission); getTransitiveDependents(permission, scope /* + isPrivate */).forEach((dep) => { newPermissions = newPermissions.filter((p) => p !== dep); }); } if (hasAllPermissions(newPermissions)) newPermissions.push("*.*"); return filterToScoped(newPermissions); }; return { hasAllPermissions, getResourcePermissionLevel, toggleResourcePermissionLevel, toggleSinglePermission }; }packages/features/pbac/domain/types/permission-registry.ts (1)
326-344: Dependencies don’t account for private/public variants.Several actions still depend on team.listMembers, but private teams expose team.listMembersPrivate. This causes:
- Hidden/unavailable dependencies in private contexts.
- Toggler logic adding the wrong permission.
Fix by including both variants and filtering by the privacy-scoped registry in the togglers (see usePermissions refactor comment).
Apply this diff:
[CustomAction.Invite]: { @@ - dependsOn: ["team.read", "team.listMembers", "role.read"], + dependsOn: ["team.read", "team.listMembers", "team.listMembersPrivate", "role.read"], }, @@ [CustomAction.Remove]: { @@ - dependsOn: ["team.read", "team.listMembers"], + dependsOn: ["team.read", "team.listMembers", "team.listMembersPrivate"], }, @@ [CustomAction.ChangeMemberRole]: { @@ - dependsOn: ["team.read", "team.listMembers", "role.read"], + dependsOn: ["team.read", "team.listMembers", "team.listMembersPrivate", "role.read"], }, @@ [CustomAction.Impersonate]: { @@ - dependsOn: ["team.read", "team.listMembers"], + dependsOn: ["team.read", "team.listMembers", "team.listMembersPrivate"], },This must be paired with updating usePermissions to filter dependencies to those present in getPermissionsForScopeAndPrivacy(scope, isPrivate). Based on learnings
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/SimplePermissionItem.tsx (1)
16-23: Prop added, but togglers aren’t privacy-aware (mismatch).Same mismatch as AdvancedPermissionGroup: usePermissions(scope) ignores privacy. Display uses privacy, value/toggling does not.
- Update usePermissions to accept isPrivate.
- Pass isPrivate into usePermissions here.
Apply this diff:
- const { getResourcePermissionLevel, toggleResourcePermissionLevel } = usePermissions(scope); + const { getResourcePermissionLevel, toggleResourcePermissionLevel } = usePermissions(scope, isPrivate);apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx (2)
158-176: Filter outgoing permissions by privacy-scoped registry.If a role contains permissions not visible for the current privacy (e.g., org/team switched from public↔private), users can’t see/remove them, and they’ll be resubmitted. Filter on submit to the allowed set for this privacy.
Apply this diff:
- const onSubmit = (values: FormValues) => { + const onSubmit = (values: FormValues) => { + // keep only permissions present in the privacy-scoped registry + const allowed = new Set<string>(); + Object.entries(scopedRegistry).forEach(([res, cfg]) => { + Object.keys(cfg) + .filter((a) => a !== "_resource") + .forEach((a) => allowed.add(`${res}.${a}`)); + }); + const filteredPerms = (values.permissions || []).filter((p) => allowed.has(p)); if (isEditing && role) { updateMutation.mutate({ teamId, roleId: role.id, name: values.name, - permissions: values.permissions as any, + permissions: filteredPerms as any, color: values.color, }); } else { createMutation.mutate({ teamId, name: values.name, description: values.description, color: values.color, - permissions: values.permissions as any, + permissions: filteredPerms as any, }); } };
1-293: Update permission traversal and hooks to be privacy-aware
- packages/features/pbac/utils/permissionTraversal.ts: change
getPermissionsForScope(scope)togetPermissionsForScopeAndPrivacy(scope, isPrivate)(addisPrivateparameter totraversePermissions,getTransitiveDependencies, andgetTransitiveDependents).- apps/web/app/.../usePermissions.ts: extend
usePermissionsto acceptisPrivate, replacegetPermissionsForScope(scope)withgetPermissionsForScopeAndPrivacy(scope, isPrivate), and passisPrivateinto allgetTransitiveDependencies/getTransitiveDependentscalls.
🧹 Nitpick comments (12)
packages/prisma/migrations/20250923085350_add_list_members_private_permissions/migration.sql (1)
3-15: Avoid hard dependency on UUID generation in SQL; rely on DB defaults.If "RolePermission".id and "createdAt" have defaults (common with Prisma), omit them to reduce extension dependencies and let the DB handle values.
Here’s an alternative insert:
-INSERT INTO "RolePermission" (id, "roleId", resource, action, "createdAt") -SELECT - gen_random_uuid(), 'admin_role', resource, action, NOW() +INSERT INTO "RolePermission" ("roleId", resource, action) SELECT - ('team', 'listMembersPrivate'), + 'admin_role', resource, action FROM ( VALUES -- Team listMembersPrivate permission ('team', 'listMembersPrivate'), -- Organization listMembersPrivate permission ('organization', 'listMembersPrivate') ) AS permissions(resource, action) ON CONFLICT ("roleId", resource, action) DO NOTHING;apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsx (2)
74-76: Prefer notFound() over throwing errors, and localize the message if user-visible.Throwing a generic error here can surface a 500. Use notFound() for 404 semantics.
Apply:
-import { redirect } from "next/navigation"; +import { notFound, redirect } from "next/navigation"; ... - if (!team) { - throw new Error("Team not found"); - } + if (!team) { + notFound(); + }As per coding guidelines
21-28: Fix params typing; remove unnecessary awaits.Next.js passes params as a plain object, not a Promise. Awaiting headers()/cookies() is also unnecessary.
Apply:
-export const generateMetadata = async ({ params }: { params: Promise<{ id: string }> }) => +export const generateMetadata = async ({ params }: { params: { id: string } }) => await _generateMetadata( (t) => t("team_members"), (t) => t("members_team_description"), undefined, undefined, - `/settings/teams/${(await params).id}/members` + `/settings/teams/${params.id}/members` ); ... -const Page = async ({ params }: { params: Promise<{ id: string }> }) => { +const Page = async ({ params }: { params: { id: string } }) => { const t = await getTranslate(); - const { id } = await params; + const { id } = params; const teamId = parseInt(id); ... - const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) }); + const session = await getServerSession({ req: buildLegacyRequest(headers(), cookies()) });As per coding guidelines
Also applies to: 60-64
packages/trpc/server/routers/viewer/organizations/listMembers.handler.test.ts (2)
26-27: Stale PBAC mock likely unnecessary.
getSpecificPermissionsisn’t used by the SUT after migrating to PermissionCheckService. Consider removing to reduce test setup noise.Apply this diff to simplify:
-vi.mock("@calcom/features/pbac/lib/resource-permissions", () => ({ - getSpecificPermissions: vi.fn().mockResolvedValue({ - listMembers: true, - listMembersPrivate: true, - }), -})); +// (removed) no longer needed after PermissionCheckService migration
78-80: Unnecessary team privacy mock in this test.
organizations/listMembers.handlerreadsctx.user.organization.isPrivate, notprisma.team.findUnique. This mock can be dropped.Apply this diff:
- // Mock team.findUnique to return only isPrivate field since that's what the handler selects - prisma.team.findUnique.mockResolvedValue({ isPrivate: false } as any); + // (removed) not used by organizations/listMembers.handlerpackages/trpc/server/routers/viewer/teams/listMembers.handler.ts (1)
167-180: Derive resource from team hierarchy, not user context.Using
teamId === ctx.user.organizationIdcan misclassify the resource if the user context and target team diverge. Prefer usingteam.parentId(org teams have no parent).Apply this diff:
- const isTargetingOrg = teamId === ctx.user.organizationId; + const isTargetingOrg = !team?.parentId; @@ - const resource = isTargetingOrg ? Resource.Organization : Resource.Team; - const targetAction = team.isPrivate ? CustomAction.ListMembersPrivate : CustomAction.ListMembers; - const permissionString = `${resource}.${targetAction}` as PermissionString; + const resource = isTargetingOrg ? Resource.Organization : Resource.Team; + const targetAction = team.isPrivate ? CustomAction.ListMembersPrivate : CustomAction.ListMembers; + const permissionString = `${resource}.${targetAction}` as PermissionString;Additional change outside the selected range (to support the above):
- Include
parentIdin theteamselect.- const team = await prisma.team.findUnique({ - where: { id: teamId }, - select: { isPrivate: true }, - }); + const team = await prisma.team.findUnique({ + where: { id: teamId }, + select: { isPrivate: true, parentId: true }, + });packages/trpc/server/routers/viewer/teams/legacyListMembers.handler.ts (1)
1-3: Remove unused imports.
ResourceandgetSpecificPermissionsare unused after the PBAC migration here.Apply this diff:
-import { Resource, CustomAction } from "@calcom/features/pbac/domain/types/permission-registry"; -import { getSpecificPermissions } from "@calcom/features/pbac/lib/resource-permissions"; -import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service"; +import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service";apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/AdvancedPermissionGroup.tsx (2)
9-11: Remove unused import.getPermissionsForScope is no longer used in this file.
Apply this diff:
-import { - Scope, - CrudAction, - getPermissionsForScope, - getPermissionsForScopeAndPrivacy, -} from "@calcom/features/pbac/domain/types/permission-registry"; +import { Scope, CrudAction, getPermissionsForScopeAndPrivacy } from "@calcom/features/pbac/domain/types/permission-registry";
41-43: Compute scoped registry once.Rebuilding the registry on every render is unnecessary. Memoize by [scope, isPrivate].
Apply this diff:
- const scopedRegistry = getPermissionsForScopeAndPrivacy(scope, isPrivate); + const scopedRegistry = useMemo( + () => getPermissionsForScopeAndPrivacy(scope, isPrivate), + [scope, isPrivate] + );apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/SimplePermissionItem.tsx (1)
35-38: Unnecessary fallback and missing memoization.getPermissionsForScopeAndPrivacy always returns a registry; the PERMISSION_REGISTRY fallback is redundant. Also memoize the registry.
Apply this diff:
- const scopedRegistry = getPermissionsForScopeAndPrivacy(scope, isPrivate); - - const registry = scopedRegistry || PERMISSION_REGISTRY; + const registry = useMemo( + () => getPermissionsForScopeAndPrivacy(scope, isPrivate), + [scope, isPrivate] + );And remove the PERMISSION_REGISTRY import:
-import { - Scope, - PERMISSION_REGISTRY, - getPermissionsForScope, - getPermissionsForScopeAndPrivacy, -} from "@calcom/features/pbac/domain/types/permission-registry"; +import { Scope, getPermissionsForScopeAndPrivacy } from "@calcom/features/pbac/domain/types/permission-registry";apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx (2)
119-132: Avoid recomputing registry on search keystrokes.searchQuery and t in the deps cause the registry to rebuild unnecessarily.
Apply this diff:
- const { filteredResources, scopedRegistry } = useMemo(() => { - // Use privacy-aware filtering if we have privacy information - const scopedRegistry = getPermissionsForScopeAndPrivacy(scope, isPrivate); - const filteredResources = Object.keys(scopedRegistry).filter((resource) => + const scopedRegistry = useMemo( + () => getPermissionsForScopeAndPrivacy(scope, isPrivate), + [scope, isPrivate] + ); + + const filteredResources = useMemo( + () => + Object.keys(scopedRegistry).filter((resource) => t( scopedRegistry[resource as Resource][CrudAction.All as keyof (typeof scopedRegistry)[Resource]] ?.i18nKey || "" ) .toLowerCase() .includes(searchQuery.toLowerCase()) - ); - return { filteredResources, scopedRegistry }; - }, [searchQuery, t, scope, isPrivate]); + ), + [searchQuery, t, scopedRegistry] + );
1-20: Remove unused getPermissionsForScope import.It appears unused after the switch.
Apply this diff:
-import { - CrudAction, - Scope, - getPermissionsForScope, - getPermissionsForScopeAndPrivacy, -} from "@calcom/features/pbac/domain/types/permission-registry"; +import { CrudAction, Scope, getPermissionsForScopeAndPrivacy } from "@calcom/features/pbac/domain/types/permission-registry";
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/AdvancedPermissionGroup.tsx(3 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx(6 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RolesList.tsx(3 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/SimplePermissionItem.tsx(3 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/page.tsx(3 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsx(2 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/roles/page.tsx(4 hunks)apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx(3 hunks)apps/web/lib/team/[slug]/getServerSideProps.tsx(1 hunks)packages/features/pbac/domain/types/permission-registry.ts(5 hunks)packages/prisma/migrations/20250923085350_add_list_members_private_permissions/migration.sql(1 hunks)packages/trpc/server/routers/viewer/organizations/getMembers.handler.ts(2 hunks)packages/trpc/server/routers/viewer/organizations/listMembers.handler.test.ts(4 hunks)packages/trpc/server/routers/viewer/organizations/listMembers.handler.ts(2 hunks)packages/trpc/server/routers/viewer/teams/legacyListMembers.handler.ts(3 hunks)packages/trpc/server/routers/viewer/teams/listMembers.handler.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/AdvancedPermissionGroup.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/SimplePermissionItem.tsxapps/web/lib/team/[slug]/getServerSideProps.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RolesList.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/roles/page.tsxapps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx
**/*.{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/AdvancedPermissionGroup.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/page.tsxpackages/trpc/server/routers/viewer/teams/legacyListMembers.handler.tspackages/trpc/server/routers/viewer/organizations/getMembers.handler.tspackages/trpc/server/routers/viewer/teams/listMembers.handler.tspackages/trpc/server/routers/viewer/organizations/listMembers.handler.test.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/SimplePermissionItem.tsxapps/web/lib/team/[slug]/getServerSideProps.tsxpackages/trpc/server/routers/viewer/organizations/listMembers.handler.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RolesList.tsxpackages/features/pbac/domain/types/permission-registry.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/roles/page.tsxapps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx
**/*.{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/AdvancedPermissionGroup.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/page.tsxpackages/trpc/server/routers/viewer/teams/legacyListMembers.handler.tspackages/trpc/server/routers/viewer/organizations/getMembers.handler.tspackages/trpc/server/routers/viewer/teams/listMembers.handler.tspackages/trpc/server/routers/viewer/organizations/listMembers.handler.test.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/SimplePermissionItem.tsxapps/web/lib/team/[slug]/getServerSideProps.tsxpackages/trpc/server/routers/viewer/organizations/listMembers.handler.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RolesList.tsxpackages/features/pbac/domain/types/permission-registry.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/roles/page.tsxapps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx
**/*.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/trpc/server/routers/viewer/teams/legacyListMembers.handler.tspackages/trpc/server/routers/viewer/organizations/getMembers.handler.tspackages/trpc/server/routers/viewer/teams/listMembers.handler.tspackages/trpc/server/routers/viewer/organizations/listMembers.handler.test.tspackages/trpc/server/routers/viewer/organizations/listMembers.handler.tspackages/features/pbac/domain/types/permission-registry.ts
🧬 Code graph analysis (10)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/AdvancedPermissionGroup.tsx (2)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts (1)
usePermissions(21-181)packages/features/pbac/domain/types/permission-registry.ts (1)
getPermissionsForScopeAndPrivacy(166-200)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/page.tsx (1)
packages/features/auth/lib/next-auth-options.ts (1)
session(746-771)
packages/trpc/server/routers/viewer/teams/legacyListMembers.handler.ts (2)
packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(19-306)hasPermission(183-201)packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
packages/trpc/server/routers/viewer/organizations/getMembers.handler.ts (2)
packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(19-306)packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
packages/trpc/server/routers/viewer/teams/listMembers.handler.ts (2)
packages/features/pbac/domain/types/permission-registry.ts (1)
PermissionString(67-67)packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(19-306)
packages/trpc/server/routers/viewer/organizations/listMembers.handler.test.ts (1)
packages/trpc/server/routers/viewer/organizations/listMembers.handler.ts (1)
listMembersHandler(39-369)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/SimplePermissionItem.tsx (2)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/usePermissions.ts (1)
usePermissions(21-181)packages/features/pbac/domain/types/permission-registry.ts (1)
getPermissionsForScopeAndPrivacy(166-200)
packages/trpc/server/routers/viewer/organizations/listMembers.handler.ts (1)
packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(19-306)hasPermission(183-201)
apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx (1)
scripts/prepare-local-for-delegation-credentials-testing.js (1)
org(42-44)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx (1)
packages/features/pbac/domain/types/permission-registry.ts (1)
getPermissionsForScopeAndPrivacy(166-200)
⏰ 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). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (34)
apps/web/lib/team/[slug]/getServerSideProps.tsx (1)
8-8: Consolidated import path looks good.Switching to the canonical
@calcom/features/ee/teams/lib/queriesexport removes duplication without functional impact. 👍apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/members/page.tsx (3)
105-107: Good: Added privacy-specific action to the permission request.Including CustomAction.ListMembersPrivate ensures correct resolution in private contexts.
124-126: Good: Fallback roles aligned for private/public cases.Using the same computed fallbackRolesCanListMembers keeps parity between ListMembers and ListMembersPrivate.
132-134: Good: Privacy-aware mapping of canListMembers.Switching based on team.isPrivate correctly selects the effective permission.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/page.tsx (3)
47-57: Good: Cached DB-backed privacy check using select.Efficient select of isPrivate with revalidate is appropriate for the page’s needs.
84-88: Nice: Fetch privacy, roles, and permissions concurrently.Keeps TTFB lower and maintains a single source of truth for isPrivate.
124-125: Propagating isPrivate downstream is aligned with PBAC changes.RolesList can now filter permission visibility based on privacy.
apps/web/app/(use-page-wrapper)/settings/organizations/(org-user-only)/members/page.tsx (2)
73-85: Good: Added privacy-specific action and fallback roles for organizations.This keeps org member listing consistent with private/public contexts.
103-105: Good: Privacy-aware resolution of canListMembers.Correctly selects ListMembersPrivate when org.isPrivate.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RolesList.tsx (2)
51-52: Propagating isPrivate via props is correct.This wires privacy to RoleSheet and downstream components.
118-119: Forwarding isPrivate to RoleSheet is aligned with the new permission flow.Ensures UI permission visibility honors privacy.
packages/trpc/server/routers/viewer/organizations/getMembers.handler.ts (2)
37-72: Prisma select usage looks good and avoids include.Selected only the needed fields; complies with guidelines.
As per coding guidelines
20-33: ctx.user.organization is guaranteed to be populated
The session middleware’s getUserFromSession always sets both organization and organizationId on the user return value, so accessing ctx.user.organization.isPrivate is safe and no additional lookup is needed.Likely an incorrect or invalid review comment.
packages/prisma/migrations/20250923085350_add_list_members_private_permissions/migration.sql (1)
14-14: Unique constraint verified for RolePermission(roleId, resource, action). The Prisma schema defines@@unique([roleId, resource, action]), so theON CONFLICTclause will not error at runtime.packages/trpc/server/routers/viewer/organizations/listMembers.handler.test.ts (4)
11-12: Good: default export for prisma mock.Ensures modules expecting a default export work seamlessly in tests.
30-36: Good: PermissionCheckService is properly mocked.Keeps tests focused on verifying permission inputs without coupling to service internals.
250-286: LGTM: private org permission check is asserted.Verifies
"organization.listMembersPrivate"and fallback["ADMIN","OWNER"].
288-316: LGTM: public org permission check is asserted.Verifies
"organization.listMembers"and fallback["MEMBER","ADMIN","OWNER"].packages/trpc/server/routers/viewer/teams/listMembers.handler.ts (1)
1-5: OK to import typed permission primitives.Keeps permission construction type-safe downstream.
packages/trpc/server/routers/viewer/organizations/listMembers.handler.ts (2)
91-102: Good migration to PermissionCheckService with privacy-aware fallback roles.Correctly selects
"organization.listMembersPrivate"for private orgs, and"organization.listMembers"otherwise, with appropriate fallback roles.
104-104: Early safe-return on denied access looks correct.Matches the endpoint’s existing response shape (
canUserGetMembers: false).packages/trpc/server/routers/viewer/teams/legacyListMembers.handler.ts (2)
24-34: Behavior change: new org-level PBAC gate. Verify consumers.Now returns empty results when the user lacks org list permission. Ensure callers and UI expect empty results instead of previous behavior.
If you want, I can add a focused unit test asserting the empty result when
checkPermissionresolves tofalse.
139-164: LGTM: centralized org permission gate.Uses org privacy to choose permission and appropriate fallback roles; Prisma query uses
selectonly.apps/web/app/(use-page-wrapper)/settings/(settings-layout)/teams/[id]/roles/page.tsx (2)
54-65: Good: cached team privacy helper.Efficiently fetches
isPrivatewithselectand caches it.
127-131: Good: plumbs isPrivate into RolesList.Enables privacy-aware permission filtering in the UI flow.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/AdvancedPermissionGroup.tsx (1)
10-11: Good: privacy-aware registry imported.Importing getPermissionsForScopeAndPrivacy is correct for private/public filtering.
packages/features/pbac/domain/types/permission-registry.ts (3)
30-36: New CustomAction is fine; ensure downstream checks cover it.Adding ListMembersPrivate is consistent with the new privacy model.
50-53: Visibility hook looks good.visibleWhen.teamPrivacy enables UI filtering without breaking type safety.
160-200: Privacy-aware filter function LGTM.Implementation correctly intersects scope and teamPrivacy.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/SimplePermissionItem.tsx (1)
8-9: Good: switched to privacy-aware registry.Importing getPermissionsForScopeAndPrivacy is correct.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx (4)
14-15: Good: privacy-aware registry imported.Correctly swaps to getPermissionsForScopeAndPrivacy.
60-70: Prop added and passed through — good.RoleSheet now takes isPrivate and propagates. This enables privacy-aware filtering downstream.
227-235: Downstream propagation is correct.Passing isPrivate to AdvancedPermissionGroup ensures consistent filtering in the advanced view.
251-260: Downstream propagation is correct (simple view).Passing isPrivate to SimplePermissionItem aligns both views.
| visibleWhen: { | ||
| teamPrivacy: "public", // Only show for public orgs | ||
| }, | ||
| }, | ||
| [CustomAction.ListMembersPrivate]: { | ||
| description: "List private organization members", | ||
| category: "org", | ||
| i18nKey: "pbac_action_list_members", // Same UI label as listMembers for consistency | ||
| descriptionI18nKey: "pbac_desc_list_organization_members", // Same description as listMembers | ||
| scope: [Scope.Organization], | ||
| dependsOn: ["organization.read"], | ||
| visibleWhen: { | ||
| teamPrivacy: "private", // Only show for private orgs | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Organization-level dependencies mirror the same issue.
Invite/Remove/ChangeMemberRole rely on organization.listMembers only; add organization.listMembersPrivate too.
Apply this diff:
[CustomAction.Invite]: {
@@
- dependsOn: ["organization.listMembers"],
+ dependsOn: ["organization.listMembers", "organization.listMembersPrivate"],
},
[CustomAction.Remove]: {
@@
- dependsOn: ["organization.listMembers"],
+ dependsOn: ["organization.listMembers", "organization.listMembersPrivate"],
},
[CustomAction.ChangeMemberRole]: {
@@
- dependsOn: ["organization.listMembers", "role.read"],
+ dependsOn: ["organization.listMembers", "organization.listMembersPrivate", "role.read"],
},In addition, ensure getTransitiveDependencies/getTransitiveDependents accept isPrivate and use the privacy-scoped registry. Based on learnings
📝 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.
| visibleWhen: { | |
| teamPrivacy: "public", // Only show for public orgs | |
| }, | |
| }, | |
| [CustomAction.ListMembersPrivate]: { | |
| description: "List private organization members", | |
| category: "org", | |
| i18nKey: "pbac_action_list_members", // Same UI label as listMembers for consistency | |
| descriptionI18nKey: "pbac_desc_list_organization_members", // Same description as listMembers | |
| scope: [Scope.Organization], | |
| dependsOn: ["organization.read"], | |
| visibleWhen: { | |
| teamPrivacy: "private", // Only show for private orgs | |
| }, | |
| }, | |
| [CustomAction.Invite]: { | |
| // … other props … | |
| dependsOn: ["organization.listMembers", "organization.listMembersPrivate"], | |
| }, | |
| [CustomAction.Remove]: { | |
| // … other props … | |
| dependsOn: ["organization.listMembers", "organization.listMembersPrivate"], | |
| }, | |
| [CustomAction.ChangeMemberRole]: { | |
| // … other props … | |
| dependsOn: ["organization.listMembers", "organization.listMembersPrivate", "role.read"], | |
| }, |
🤖 Prompt for AI Agents
In packages/features/pbac/domain/types/permission-registry.ts around lines 392
to 406, organization-level actions Invite/Remove/ChangeMemberRole currently only
depend on "organization.listMembers" but must also depend on
"organization.listMembersPrivate" for private orgs; update those actions'
dependsOn arrays to include "organization.listMembersPrivate". Also update the
getTransitiveDependencies and getTransitiveDependents functions to accept an
isPrivate boolean and use the privacy-scoped registry (choose private registry
when isPrivate is true, otherwise public) when resolving dependencies so
privacy-scoped permissions are considered in transitive calculations.
| INSERT INTO "RolePermission" (id, "roleId", resource, action, "createdAt") | ||
| SELECT | ||
| gen_random_uuid(), 'admin_role', resource, action, NOW() | ||
| FROM ( |
There was a problem hiding this comment.
Ensure pgcrypto extension is available before using gen_random_uuid().
Without pgcrypto, this migration will fail in environments where the extension isn’t installed.
Apply this diff to make the migration resilient:
+-- Required for gen_random_uuid()
+CREATE EXTENSION IF NOT EXISTS "pgcrypto";
+
INSERT INTO "RolePermission" (id, "roleId", resource, action, "createdAt")
SELECT
gen_random_uuid(), 'admin_role', resource, action, NOW()
FROM (🤖 Prompt for AI Agents
In
packages/prisma/migrations/20250923085350_add_list_members_private_permissions/migration.sql
around lines 3 to 6, the migration uses gen_random_uuid() but does not ensure
the pgcrypto extension exists; add a statement at the top of the migration to
create the extension if missing (e.g., CREATE EXTENSION IF NOT EXISTS pgcrypto;)
before any use of gen_random_uuid(), so the function is available in
environments where pgcrypto isn't installed.
packages/trpc/server/routers/viewer/organizations/getMembers.handler.ts
Outdated
Show resolved
Hide resolved
packages/trpc/server/routers/viewer/organizations/getMembers.handler.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Eunjae Lee <hey@eunjae.dev>
…eams' into feat/pbac-private-teams
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 (3)
packages/features/pbac/domain/types/permission-registry.ts (3)
399-406: Missing private permission dependency.The
ChangeMemberRoleaction depends only on the publiclistMemberspermission but should also includelistMembersPrivateto work correctly in private organizations.Apply this diff to include both dependencies:
[CustomAction.ChangeMemberRole]: { description: "Change role of team members", category: "org", i18nKey: "pbac_action_change_member_role", descriptionI18nKey: "pbac_desc_change_organization_member_role", scope: [Scope.Organization], - dependsOn: ["organization.listMembers", "role.read"], + dependsOn: ["organization.listMembers", "organization.listMembersPrivate", "role.read"], },
285-293: Team-level dependencies also need private permissions.The team actions
Invite,Remove,ChangeMemberRole, andImpersonatecurrently only depend on the publicteam.listMemberspermission, but should also includeteam.listMembersPrivatefor consistency with the organization-level fixes.Apply this diff to include private permissions in team dependencies:
[CustomAction.Invite]: { description: "Invite team members", category: "team", i18nKey: "pbac_action_invite", descriptionI18nKey: "pbac_desc_invite_team_members", - dependsOn: ["team.read", "team.listMembers", "role.read"], + dependsOn: ["team.read", "team.listMembers", "team.listMembersPrivate", "role.read"], }, [CustomAction.Remove]: { description: "Remove team members", category: "team", i18nKey: "pbac_action_remove", descriptionI18nKey: "pbac_desc_remove_team_members", - dependsOn: ["team.read", "team.listMembers"], + dependsOn: ["team.read", "team.listMembers", "team.listMembersPrivate"], }, [CustomAction.ChangeMemberRole]: { description: "Change role of team members", category: "team", i18nKey: "pbac_action_change_member_role", descriptionI18nKey: "pbac_desc_change_team_member_role", - dependsOn: ["team.read", "team.listMembers", "role.read"], + dependsOn: ["team.read", "team.listMembers", "team.listMembersPrivate", "role.read"], }, [CustomAction.Impersonate]: { description: "Impersonate team members", category: "team", i18nKey: "pbac_action_impersonate", descriptionI18nKey: "pbac_desc_impersonate_team_members", - dependsOn: ["team.read", "team.listMembers"], + dependsOn: ["team.read", "team.listMembers", "team.listMembersPrivate"], },Also applies to: 318-319, 325-325
134-168: Propagate isPrivate through permission traversal utilities
- Extend traversePermissions, getTransitiveDependencies and getTransitiveDependents (packages/features/pbac/utils/permissionTraversal.ts) to accept an optional isPrivate flag and forward it into getPermissionsForScope.
- Update all call sites in usePermissions.ts (e.g. lines 24–25, 38–39, 52–53, 90–92, 118–119, 150) to pass the same isPrivate parameter when invoking these helpers.
- Audit filterResourceConfig usage in permission.service.ts and replace with getPermissionsForScope(scope, isPrivate) if privacy should filter getAllPermissions or getPermissionsByResource.
🧹 Nitpick comments (1)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx (1)
59-60: Prop semantics: document optional privacy filteringAdd a brief JSDoc to clarify that when
isPrivateisundefined, privacy filtering should not apply; whentrue|false, it should. This helps prevent accidental over-filtering by callers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/AdvancedPermissionGroup.tsx(2 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx(5 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/SimplePermissionItem.tsx(2 hunks)packages/features/pbac/domain/types/permission-registry.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/AdvancedPermissionGroup.tsx
- apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/SimplePermissionItem.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/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:
packages/features/pbac/domain/types/permission-registry.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx
**/*.{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/domain/types/permission-registry.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx
🧬 Code graph analysis (1)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx (1)
packages/features/pbac/domain/types/permission-registry.ts (1)
getPermissionsForScope(134-168)
⏰ 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). (7)
- GitHub Check: Production builds / Build API v1
- GitHub Check: Tests / Unit
- GitHub Check: Production builds / Build API v2
- GitHub Check: Production builds / Build Web App
- GitHub Check: Production builds / Build Atoms
- GitHub Check: Type check / check-types
- GitHub Check: Linters / lint
🔇 Additional comments (8)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx (3)
118-121: Privacy-aware registry lookup and memo deps look goodUsing
getPermissionsForScope(scope, isPrivate)and addingisPrivateto deps correctly recalculates when privacy changes.Also applies to: 130-130
233-234: Propagating isPrivate to AdvancedPermissionGroup is correctKeeps child filtering consistent with parent.
258-259: Propagating isPrivate to SimplePermissionItem is correctEnsures simple view aligns with privacy filtering.
packages/features/pbac/domain/types/permission-registry.ts (5)
30-30: LGTM!The addition of
ListMembersPrivateenum member correctly extends the action types for privacy-aware permissions.
50-53: LGTM! Well-designed visibility control interface.The addition of the
visibleWhenproperty with team privacy filtering is well-structured and provides clear control over which permissions are shown based on privacy settings.
129-168: Function signature correctly implements optional parameter pattern.The implementation follows TypeScript best practices by placing the optional
isPrivateparameter after the requiredscopeparameter. The privacy filtering logic correctly handles three states: undefined (show all), true (private), and false (public).
299-312: LGTM! Proper privacy-aware permission structure.The team
ListMembersandListMembersPrivatepermissions are correctly configured with appropriate visibility conditions. The reuse of the same i18n keys for consistent UI labeling is a good design choice.
360-374: Organization-level dependencies still missing private permission.The organization's
ListMembersPrivatepermission is properly defined, but the dependency issue flagged in the past review remains unaddressed.Apply this diff to include the private permission in organization dependencies:
[CustomAction.Invite]: { description: "Invite organization members", category: "org", i18nKey: "pbac_action_invite", descriptionI18nKey: "pbac_desc_invite_organization_members", scope: [Scope.Organization], - dependsOn: ["organization.listMembers"], + dependsOn: ["organization.listMembers", "organization.listMembersPrivate"], }, [CustomAction.Remove]: { description: "Remove organization members", category: "org", i18nKey: "pbac_action_remove", descriptionI18nKey: "pbac_desc_remove_organization_members", scope: [Scope.Organization], - dependsOn: ["organization.listMembers"], + dependsOn: ["organization.listMembers", "organization.listMembersPrivate"], },
.../(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx
Show resolved
Hide resolved
eunjae-lee
left a comment
eunjae-lee
left a comment
There was a problem hiding this comment.
e2e is failing, but the change looks good !
CarinaWolli
left a comment
CarinaWolli
left a comment
There was a problem hiding this comment.
Nice work, everything is working as expected 🙌🏻
5 participants
What does this PR do?
This PR adds the ability to have a new permission "listMembersPrivate" -> This is a permission which controls the ability for users to see other users in the team when the team/org is private.
For now we do not allow this on custom roles only via the default admin/owner role.
Video: cap.so/s/wkmjz41yekcw32h
How to test