refactor: replace isTeamAdminOrOwner with PBAC permissions#24037
refactor: replace isTeamAdminOrOwner with PBAC permissions#24037eunjae-lee merged 5 commits intomainfrom
Conversation
- Remove isTeamAdminOrOwner from team-members-view.tsx, rely on server-side permissions - Replace role checks in addMembersToEventTypes.handler.ts with eventType.update permission - Follow PBAC refactoring guide patterns for consistent permission checking - Fix TypeScript any type usage and unused variable warnings Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Walkthrough
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. |
…hub.com:calcom/cal.com into devin/1758707522-pbac-refactor-team-permissions
dosubot
bot
added
platform
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/trpc/server/routers/viewer/teams/addMembersToEventTypes.handler.ts (1)
32-44: Authorize the target resources: validate eventTypeIds belong to teamId and userIds are team members.Current code allows a user with rights in team A to modify event types in team B by passing their IDs. Also allows adding non‑members as hosts by raw userId. Enforce scoping before createMany.
const { eventTypeIds, userIds, teamId } = input; const permissionCheckService = new PermissionCheckService(); @@ - // check if user has eventType.update permission - if (!hasPermission) throw new TRPCError({ code: "UNAUTHORIZED" }); + // check if user has eventType.update permission + if (!hasPermission) throw new TRPCError({ code: "FORBIDDEN" }); + + // Ensure all eventTypes belong to the provided team + const eventTypes = await prisma.eventType.findMany({ + where: { id: { in: eventTypeIds } }, + select: { id: true, teamId: true }, + }); + const validEventTypeIds = new Set(eventTypes.filter((et) => et.teamId === teamId).map((et) => et.id)); + if (validEventTypeIds.size !== eventTypeIds.length) { + throw new TRPCError({ code: "FORBIDDEN", message: "One or more event types are not in the specified team." }); + } + + // Ensure all users are members of the team + const memberships = await prisma.membership.findMany({ + where: { teamId, userId: { in: userIds } }, + select: { userId: true }, + }); + const teamMemberUserIds = new Set(memberships.map((m) => m.userId)); + if (teamMemberUserIds.size !== userIds.length) { + throw new TRPCError({ code: "FORBIDDEN", message: "One or more users are not members of the team." }); + } @@ - const data: Prisma.HostCreateManyInput[] = eventTypeIds.flatMap((eventId) => - userIds.map((userId) => ({ + const data: Prisma.HostCreateManyInput[] = eventTypeIds.flatMap((eventId) => + userIds.map((userId) => ({ eventTypeId: eventId, userId: userId, priority: 2, // Default medium priority })) ); + + if (data.length === 0) { + // Align with Prisma.BatchPayload return type + return { count: 0 }; + }
🧹 Nitpick comments (6)
packages/trpc/server/routers/viewer/teams/addMembersToEventTypes.handler.ts (3)
29-30: Return FORBIDDEN on insufficient permissions (not UNAUTHORIZED).User is authenticated but lacks permission; FORBIDDEN is semantically correct and aligns with typical HTTP/TRPC semantics.
- if (!hasPermission) throw new TRPCError({ code: "UNAUTHORIZED" }); + if (!hasPermission) throw new TRPCError({ code: "FORBIDDEN" });
47-47: Avoid default exports; keep only the named export.Project guidelines favor named exports for clearer APIs and tree-shaking. Remove the default export.
-export default addMembersToEventTypesHandler;
21-27: Use enum constants for the permission key
ImportResourceandCrudActionfrom the PBAC registry and replace the string literal"eventType.update"with${Resource.EventType}.${CrudAction.Update}to ensure correctness and type safety.apps/web/modules/teams/team-members-view.tsx (3)
25-31: Remove unused prop attributes or wire it up.attributes is declared on TeamMembersViewProps but never used. Drop it to keep the surface minimal, or use it where intended.
- attributes?: { - id: string; - name: string; - options: { - value: string; - }[]; - }[];
38-38: Dead state: _showInviteLinkSettingsModal is never read.Either remove the state and the onSettingsOpen handler below, or wire it to an InviteLinkSettings modal if that exists.
If removing:
- const [_showInviteLinkSettingsModal, setShowInviteLinkSettingsModal] = useState(false);And at Line 68:
- onSettingsOpen={() => setShowInviteLinkSettingsModal(true)} + // onSettingsOpen removed: no settings modal in this view
59-60: Update copy to PBAC terminology.The message says “Only admin can see…”, which conflicts with PBAC (non-admins may have canListMembers). Suggest a permission-based copy key (e.g., "no_permission_to_view_team_members").
If an appropriate translation key exists, apply:
- <h2 className="text-default">{t("only_admin_can_see_members_of_team")}</h2> + <h2 className="text-default">{t("no_permission_to_view_team_members")}</h2>If not, consider adding it to i18n resources.
📜 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 (2)
apps/web/modules/teams/team-members-view.tsx(1 hunks)packages/trpc/server/routers/viewer/teams/addMembersToEventTypes.handler.ts(2 hunks)
🧰 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/trpc/server/routers/viewer/teams/addMembersToEventTypes.handler.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/trpc/server/routers/viewer/teams/addMembersToEventTypes.handler.tsapps/web/modules/teams/team-members-view.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/trpc/server/routers/viewer/teams/addMembersToEventTypes.handler.tsapps/web/modules/teams/team-members-view.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/modules/teams/team-members-view.tsx
🧠 Learnings (3)
📓 Common learnings
Learnt from: eunjae-lee
PR: calcom/cal.com#24006
File: apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx:37-48
Timestamp: 2025-09-24T11:53:40.162Z
Learning: In bookings listing views, when checking permissions for the member filter UI, use "booking.read" permission rather than "team.listMembers" because the filter's purpose is to read bookings of other team members, not just to list the members themselves. The permission check should align with the actual capability being granted.
Learnt from: anglerfishlyy
PR: calcom/cal.com#0
File: :0-0
Timestamp: 2025-08-27T16:39:38.192Z
Learning: anglerfishlyy successfully implemented CAL-3076 email invitation feature for Cal.com team event-types in PR #23312. The feature allows inviting people via email directly from assignment flow, with automatic team invitation if email doesn't belong to existing team member. Implementation includes Host type modifications (userId?: number, email?: string, isPending?: boolean), CheckedTeamSelect component updates with CreatableSelect, TRPC schema validation with zod email validation, and integration with existing teamInvite system.
Learnt from: SinghaAnirban005
PR: calcom/cal.com#23343
File: packages/features/insights/server/trpc-router.ts:1080-1101
Timestamp: 2025-08-26T08:08:23.395Z
Learning: In packages/features/insights/server/trpc-router.ts, when filtering personal event types (userId provided, no teamId, not isAll), the query correctly uses user.id (authenticated user) instead of the input userId parameter for security reasons. This prevents users from accessing other users' personal event types by passing arbitrary user IDs.
📚 Learning: 2025-08-26T08:08:23.395Z
Learnt from: SinghaAnirban005
PR: calcom/cal.com#23343
File: packages/features/insights/server/trpc-router.ts:1080-1101
Timestamp: 2025-08-26T08:08:23.395Z
Learning: In packages/features/insights/server/trpc-router.ts, when filtering personal event types (userId provided, no teamId, not isAll), the query correctly uses user.id (authenticated user) instead of the input userId parameter for security reasons. This prevents users from accessing other users' personal event types by passing arbitrary user IDs.
Applied to files:
packages/trpc/server/routers/viewer/teams/addMembersToEventTypes.handler.ts
📚 Learning: 2025-09-24T11:53:40.162Z
Learnt from: eunjae-lee
PR: calcom/cal.com#24006
File: apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx:37-48
Timestamp: 2025-09-24T11:53:40.162Z
Learning: In bookings listing views, when checking permissions for the member filter UI, use "booking.read" permission rather than "team.listMembers" because the filter's purpose is to read bookings of other team members, not just to list the members themselves. The permission check should align with the actual capability being granted.
Applied to files:
apps/web/modules/teams/team-members-view.tsx
🧬 Code graph analysis (2)
packages/trpc/server/routers/viewer/teams/addMembersToEventTypes.handler.ts (1)
packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(19-290)hasPermission(183-201)
apps/web/modules/teams/team-members-view.tsx (1)
packages/features/users/components/UserTable/types.ts (1)
MemberPermissions(56-62)
⏰ 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/modules/teams/team-members-view.tsx (2)
40-42: Ensure parent passes PBAC permissions; fallback is now strict false.This change is intentionally more restrictive — verify every TeamMembersView usage passes permissions.canListMembers to avoid regressions where members disappear. Automated search failed (rg: "unrecognized file type: tsx"); manual verification or a successful project-wide search is required.
49-54: Deprecate isOrgAdminOrOwner prop in favor of PBAC-driven permissions.Passing a hardcoded isOrgAdminOrOwner={false} risks diverging logic. Prefer to have MemberList rely solely on the permissions prop and remove this flag from its API.
Run to see MemberList prop usage:
eunjae-lee
deleted the
devin/1758707522-pbac-refactor-team-permissions
branch
E2E results are ready! |
3 participants
What does this PR do?
This PR refactors team member permission checks to replace role-based access control (
isTeamAdminOrOwner) with Permission-Based Access Control (PBAC) system, following the established PBAC refactoring patterns in the codebase.Key Changes:
isTeamAdminOrOwnercheck and relies entirely on server-side PBAC permissions (team.listMembers)isTeamAdmin/isTeamOwnerchecks witheventType.updatepermission usingPermissionCheckServiceanytype usage in interface definitionLink to Devin run: https://app.devin.ai/sessions/cdd9503002d94ef3908f30b4f3e52dcc
Requested by: @eunjae-lee
How should this be tested?
Team Member Access Testing:
team.listMemberspermission is grantedEvent Type Member Management:
eventType.updatepermissionCritical Areas to Review:
eventType.updateis the correct permission (user requestedeventType.editbut permission registry showseventType.update)falseinstead of(!team.isPrivate || isTeamAdminOrOwner)- this could be more restrictiveMandatory Tasks (DO NOT REMOVE)
Checklist