refactor: migrate TeamEventTypeForm to use PBAC instead of isTeamAdminOrOwner#24034
refactor: migrate TeamEventTypeForm to use PBAC instead of isTeamAdminOrOwner#24034eunjae-lee merged 12 commits intomainfrom
Conversation
…nOrOwner - Replace isTeamAdminOrOwner prop with permissions.canCreateEventType - Move permission checks to server-side using PermissionCheckService - Use eventType.create permission string as specified in PBAC guide - Update all parent components: CreateEventTypeDialog, event-types-view, CreateEventTypePlatformWrapper - Follow existing PBAC patterns from event-types-listing-view.tsx - Maintain backward compatibility with role-based fallback 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:
|
WalkthroughAdds server-side session validation and params parsing to the team event-type page (redirects to login or returns 404 on invalid/missing data). Integrates PermissionCheckService to compute per-team canCreateEventType permissions, exposes those permissions from the backend (getUserEventGroups → teamPermissions), and threads them through UI: profileOptions now include permissions, CreateEventTypeDialog is a named export with a ProfileOption type, CreateTeamEventType and TeamEventTypeForm accept a permissions prop, and page/component signatures/exports were updated accordingly. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (8)
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 |
- Add eventType.create permission checks to TRPC teams.get handler
- Add PBAC permission checks to platform /organizations/{orgId}/teams/me endpoint
- Update all three components to use server-side permission data instead of client-side async calls
- Add canCreateEventTypes property to platform team types
- Maintain backward compatibility with role-based fallbacks
- Remove unused imports and variables
Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
- Update event-types-view.tsx to use team.canCreateEventTypes from server - Update CreateEventTypeDialog.tsx to use team.canCreateEventTypes with fallback - Update CreateEventTypePlatformWrapper.tsx to use team.canCreateEventTypes with fallback - Remove hardcoded permission values and role-based checks - Maintain backward compatibility with existing role-based logic as fallback Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![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/modules/event-types/views/event-types-listing-view.tsx (1)
947-985: Guard teamPermissions access, unify permission prop naming, and update server transform/tests
- In
apps/web/modules/event-types/views/event-types-listing-view.tsx, change the filter/map to use
userEventGroupsData.teamPermissions?.[profile.teamId]?.canCreateEventTypewith a fallback to role-based logic, and always return a{ canCreateEventType: boolean }object.- In
packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts, rename the output fieldcanCreateEventTypes→canCreateEventType(and similarly for update/delete), and adjust all references accordingly.- Update tests in
packages/trpc/server/routers/viewer/eventTypes/__tests__/getUserEventGroups.test.tsto expect the singularcanCreateEventTypeproperty.apps/web/modules/settings/teams/[id]/event-types-view.tsx (1)
46-49: Fix: Query enable condition can fire for invalid teamId (-1).!!teamId is true for -1. This calls the query with an invalid id. Use a positive check.
- const { data: team } = trpc.viewer.teams.get.useQuery( - { teamId: teamId ?? -1, isOrg: false }, - { enabled: !!teamId } - ); + const { data: team } = trpc.viewer.teams.get.useQuery( + { teamId: teamId ?? -1, isOrg: false }, + { enabled: Number.isFinite(teamId) && teamId > 0 } + );
🧹 Nitpick comments (7)
packages/features/eventtypes/components/CreateEventTypeDialog.tsx (1)
79-80: Align fallback permissions with role-based logic to avoid accidental denials.If server PBAC is missing for a team, defaulting to canCreateEventType: false may unnecessarily restrict authorized admins/owners. Mirror the filter’s role-based fallback here.
- const permissions = teamProfile?.permissions ?? { canCreateEventType: false }; + const permissions = + teamProfile?.permissions ?? + { + canCreateEventType: + teamProfile?.membershipRole === MembershipRole.ADMIN || + teamProfile?.membershipRole === MembershipRole.OWNER, + };Additionally ensure MembershipRole is a value import (not type-only) to use it at runtime:
- import type { MembershipRole } from "@calcom/prisma/enums"; + import { MembershipRole } from "@calcom/prisma/enums";packages/trpc/server/routers/viewer/eventTypes/getUserEventGroups.handler.ts (2)
64-82: De-dupe teamIds to avoid redundant permission checks (N+1 pattern).filteredEventTypeGroups can contain multiple groups per team. This performs duplicate checkPermission calls and wastes queries. De‑duplicate teamIds before Promise.all.
Apply this diff:
- const teamPermissionsArray = await Promise.all( - filteredEventTypeGroups - .map((group) => group.teamId) - .filter((teamId): teamId is number => teamId !== null && teamId !== undefined) - .map(async (teamId) => { - const canCreateEventType = await permissionCheckService.checkPermission({ - userId: user.id, - teamId: teamId, - permission: "eventType.create", - fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN], - }); - return [teamId, { canCreateEventType }] as const; - }) - ); + const teamIds = Array.from( + new Set( + filteredEventTypeGroups + .map((group) => group.teamId) + .filter((teamId): teamId is number => teamId != null) + ) + ); + + const teamPermissionsArray = await Promise.all( + teamIds.map(async (teamId) => { + const canCreateEventType = await permissionCheckService.checkPermission({ + userId: user.id, + teamId, + permission: "eventType.create", + fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN], + }); + return [teamId, { canCreateEventType }] as const; + }) + );
64-82: Optional: Batch permission check to one query.PermissionCheckService exposes getTeamIdsWithPermission(). You can fetch all allowed teamIds once and derive the map, reducing DB roundtrips.
Example replacement for this block:
const teamIds = Array.from(new Set(/* as above */)); const allowedTeamIds = await permissionCheckService.getTeamIdsWithPermission(user.id, "eventType.create"); const allowedSet = new Set(allowedTeamIds); const teamPermissions = Object.fromEntries( teamIds.map((id) => [id, { canCreateEventType: allowedSet.has(id) }]) );Then remove teamPermissionsArray and Object.fromEntries usage.
packages/features/ee/teams/components/TeamEventTypeForm.tsx (2)
71-71: Localize the “Slug” label.Frontend code should use t() for text. Replace the hardcoded "Slug" with a localized key.
- label={isPlatform ? "Slug" : `${t("url")}: ${urlPrefix}`} + label={isPlatform ? t("slug") : `${t("url")}: ${urlPrefix}`}- label={isPlatform ? "Slug" : t("url")} + label={isPlatform ? t("slug") : t("url")}Also applies to: 95-95
16-28: Nit: Use PascalCase for type name.Type aliases should be PascalCase for consistency.
-type props = { +type Props = { permissions: { canCreateEventType: boolean; }; // ... }; export const TeamEventTypeForm = ({ permissions, // ... -}: props) => { +}: Props) => {Also applies to: 29-39
packages/platform/atoms/event-types/wrappers/CreateEventTypePlatformWrapper.tsx (1)
80-82: Use enum for role checks to prevent string drift.Compare against MembershipRole to avoid typos and improve type safety.
+import { MembershipRole } from "@calcom/prisma/enums"; ... - const permissions = { - canCreateEventType: team?.role === "ADMIN" || team?.role === "OWNER", - }; + const permissions = { + canCreateEventType: [MembershipRole.ADMIN, MembershipRole.OWNER].includes( + (team?.role as MembershipRole) ?? ("" as MembershipRole) + ), + };apps/web/modules/settings/teams/[id]/event-types-view.tsx (1)
23-24: Harden teamId parsing to avoid NaN.If id is present but non-numeric, Number(...) yields NaN and propagates. Add a safe fallback.
- const teamId = searchParams?.get("id") ? Number(searchParams.get("id")) : -1; + const idParam = searchParams?.get("id"); + const parsedId = idParam != null ? Number(idParam) : -1; + const teamId = Number.isFinite(parsedId) ? parsedId : -1;
📜 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 (7)
apps/web/app/(use-page-wrapper)/settings/teams/[id]/event-type/page.tsx(2 hunks)apps/web/modules/event-types/views/event-types-listing-view.tsx(4 hunks)apps/web/modules/settings/teams/[id]/event-types-view.tsx(2 hunks)packages/features/ee/teams/components/TeamEventTypeForm.tsx(4 hunks)packages/features/eventtypes/components/CreateEventTypeDialog.tsx(4 hunks)packages/platform/atoms/event-types/wrappers/CreateEventTypePlatformWrapper.tsx(1 hunks)packages/trpc/server/routers/viewer/eventTypes/getUserEventGroups.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:
packages/platform/atoms/event-types/wrappers/CreateEventTypePlatformWrapper.tsxapps/web/modules/settings/teams/[id]/event-types-view.tsxpackages/features/ee/teams/components/TeamEventTypeForm.tsxapps/web/modules/event-types/views/event-types-listing-view.tsxpackages/features/eventtypes/components/CreateEventTypeDialog.tsxapps/web/app/(use-page-wrapper)/settings/teams/[id]/event-type/page.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:
packages/platform/atoms/event-types/wrappers/CreateEventTypePlatformWrapper.tsxpackages/trpc/server/routers/viewer/eventTypes/getUserEventGroups.handler.tsapps/web/modules/settings/teams/[id]/event-types-view.tsxpackages/features/ee/teams/components/TeamEventTypeForm.tsxapps/web/modules/event-types/views/event-types-listing-view.tsxpackages/features/eventtypes/components/CreateEventTypeDialog.tsxapps/web/app/(use-page-wrapper)/settings/teams/[id]/event-type/page.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/platform/atoms/event-types/wrappers/CreateEventTypePlatformWrapper.tsxpackages/trpc/server/routers/viewer/eventTypes/getUserEventGroups.handler.tsapps/web/modules/settings/teams/[id]/event-types-view.tsxpackages/features/ee/teams/components/TeamEventTypeForm.tsxapps/web/modules/event-types/views/event-types-listing-view.tsxpackages/features/eventtypes/components/CreateEventTypeDialog.tsxapps/web/app/(use-page-wrapper)/settings/teams/[id]/event-type/page.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/eventTypes/getUserEventGroups.handler.ts
🧠 Learnings (4)
📓 Common learnings
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/eventTypes/getUserEventGroups.handler.tsapps/web/app/(use-page-wrapper)/settings/teams/[id]/event-type/page.tsx
📚 Learning: 2025-08-05T07:42:06.335Z
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's getUserEventGroups handler refactor (PR #22618), the membershipCount field for team event groups is intentionally set to 0 in the new createTeamEventGroup function, as confirmed by sean-brydon (PR author). This preserves the same behavior as the old implementation that was being refactored, maintaining backward compatibility. While other parts of the codebase may use actual member counts, this specific implementation maintains the previous behavior.
Applied to files:
packages/trpc/server/routers/viewer/eventTypes/getUserEventGroups.handler.ts
📚 Learning: 2025-09-08T09:34:46.661Z
Learnt from: CarinaWolli
PR: calcom/cal.com#23421
File: packages/features/ee/workflows/components/AddActionDialog.tsx:171-174
Timestamp: 2025-09-08T09:34:46.661Z
Learning: In packages/features/ee/workflows/components/AddActionDialog.tsx, the actionOptions prop is guaranteed to never be empty according to maintainer CarinaWolli, so defensive checks for empty arrays are not necessary.
Applied to files:
packages/features/eventtypes/components/CreateEventTypeDialog.tsx
🧬 Code graph analysis (5)
packages/platform/atoms/event-types/wrappers/CreateEventTypePlatformWrapper.tsx (1)
packages/features/ee/teams/components/TeamEventTypeForm.tsx (1)
TeamEventTypeForm(29-164)
packages/trpc/server/routers/viewer/eventTypes/getUserEventGroups.handler.ts (2)
packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(19-290)packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
apps/web/modules/event-types/views/event-types-listing-view.tsx (1)
packages/features/eventtypes/components/CreateEventTypeDialog.tsx (1)
ProfileOption(30-39)
packages/features/eventtypes/components/CreateEventTypeDialog.tsx (1)
packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
apps/web/app/(use-page-wrapper)/settings/teams/[id]/event-type/page.tsx (5)
packages/features/auth/lib/getServerSession.ts (1)
getServerSession(32-136)apps/web/lib/buildLegacyCtx.ts (1)
buildLegacyRequest(47-49)packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(19-290)packages/platform/libraries/index.ts (1)
MembershipRole(34-34)apps/web/modules/settings/teams/[id]/event-types-view.tsx (2)
LayoutWrapper(73-88)CreateTeamEventType(19-71)
⏰ 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: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (10)
apps/web/modules/event-types/views/event-types-listing-view.tsx (2)
15-18: Good switch to named export and shared type.Importing CreateEventTypeDialog as a named export and reusing the shared ProfileOption type is aligned with our guidelines and improves API clarity.
852-864: CTA prop shape update looks correct.The CreateButton integration matches the new ProfileOption[] and passes it through to the dialog as expected.
packages/features/eventtypes/components/CreateEventTypeDialog.tsx (3)
30-39: Public ProfileOption type LGTM.Fields align with consumers, and the permissions object is explicit and minimal.
68-68: Signature update to accept ProfileOption[] is correct.Matches upstream consumers and removes ad-hoc typing.
128-129: Prop change to pass permissions into TeamEventTypeForm looks correct.This completes the PBAC prop plumbing for team creation.
packages/trpc/server/routers/viewer/eventTypes/getUserEventGroups.handler.ts (1)
1-1: API extension looks good; ensure client types updated.Importing PermissionCheckService/MembershipRole and returning teamPermissions aligns with the PBAC prep and UI needs.
Confirm all API consumers expect { eventTypeGroups, profiles, teamPermissions }. If you use a shared type for this handler’s return shape, update it accordingly to avoid implicit any.
Also applies to: 6-6, 86-86
packages/features/ee/teams/components/TeamEventTypeForm.tsx (2)
16-19: Prop migration to permissions is consistent.Switching from isTeamAdminOrOwner to permissions.canCreateEventType is clear and aligns with the PR goal.
Also applies to: 30-31, 45-45
130-136: Managed option gating via permissions looks correct.Conditional layout/classes and rendering based on canCreateEventType read well and keep UX consistent.
Please verify e2e/visual tests that assert visibility of the Managed option now use the new permissions prop.
Also applies to: 142-144, 147-157
packages/platform/atoms/event-types/wrappers/CreateEventTypePlatformWrapper.tsx (1)
87-91: Prop pass-through to TeamEventTypeForm is correct.New permissions prop is wired correctly.
apps/web/modules/settings/teams/[id]/event-types-view.tsx (1)
15-17: Props and wiring to TeamEventTypeForm look good.New CreateTeamEventType signature and permissions passthrough are consistent with the PBAC migration.
Confirm upstream caller(s) now pass the new permissions prop to CreateTeamEventType.
Also applies to: 19-20, 56-69, 58-61
E2E results are ready! |
| const canCreateEventType = await permissionCheckService.checkPermission({ | ||
| userId: user.id, | ||
| teamId: teamId, | ||
| permission: "eventType.create", |
There was a problem hiding this comment.
I think we should use constants here
const CREATE_EVENT_TYPE_PERMISSION = "eventType.create";
There was a problem hiding this comment.
This is already fully typed, so if you put evnetType.create, TypeScript will throw an error :)
| const teamPermissionsArray = await Promise.all( | ||
| filteredEventTypeGroups | ||
| .map((group) => group.teamId) | ||
| .filter((teamId): teamId is number => teamId !== null && teamId !== undefined) |
There was a problem hiding this comment.
Just more readable and easily understandable
const teamIdsToCheck = [...new Set(
filteredEventTypeGroups
.map(group => group.teamId)
.filter((id): id is number => id != null)
)];
const teamPermissionChecks = teamIdsToCheck.map(async (teamId) => {
const hasPermission = await permissionCheckService.checkPermission({
userId: user.id,
teamId,
permission: CREATE_EVENT_TYPE_PERMISSION,
fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN],
});
return {
teamId,
permissions: { canCreateEventType: hasPermission }
};
});
const permissionResults = await Promise.all(teamPermissionChecks);
Udit-takkar
left a comment
Udit-takkar
left a comment
There was a problem hiding this comment.
Can you add some automated tests?
In this case, the existing tests cover this refactoring. |
4 participants
What does this PR do?
This PR refactors the
TeamEventTypeFormcomponent to prepare for Permission-Based Access Control (PBAC) migration by replacing theisTeamAdminOrOwnerprop with apermissionsobject structure. This is preparatory work that updates the component interface while maintaining backward compatibility with existing role-based logic.Key Changes:
isTeamAdminOrOwner: booleanprop withpermissions: { canCreateEventType: boolean }permissions.canCreateEventTypeinstead ofisTeamAdminOrOwnerCreateEventTypeDialog,event-types-view,CreateEventTypePlatformWrapper) to pass permissions objectPermissionCheckServiceandeventType.createpermission strings is not yet implemented. The underlying permission logic remains role-based for now.Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Functional Testing: Verify that team event type creation still works as before:
Permission Validation:
event-types-view.tsx(currently hardcoded totrue)No Breaking Changes: Ensure no regressions in existing event type creation workflows
Human Review Checklist
Critical Items to Review:
event-types-view.tsxhascanCreateEventType: truehardcoded - is this intentional?PermissionCheckServiceintegration is planned for future PRLink to Devin run: https://app.devin.ai/sessions/255f23adbea242aca3357be92000df37
Requested by: @eunjae-lee