fix: org admins can manage team even-types#23953
Conversation
WalkthroughThe create handler for viewer event-types heavy path updates its authorization flow: PBAC evaluation is performed only when team membership exists, getResourcePermissions is called inside that branch, and falls back to role-based checks on failure. System-admin and organization-level checks remain; unauthorized logging now includes membership presence. Tests add a new end-to-end suite for an organization admin who is not a team member, covering creation, retrieval, update, and deletion across collective and managed types, with detailed access-control scenarios. Another test suite is updated to reflect an organization member who is a team admin, changing membership role and several expectations from 404 to 403. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
keithwillcode
added
core
dosubot
bot
added
event-types
![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 (2)
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts (1)
111-117: Prisma: select only needed fields from membership.Avoid returning full rows. You only use
roleand presence; select it explicitly.- const hasMembership = await ctx.prisma.membership.findFirst({ - where: { - userId, - teamId: teamId, - accepted: true, - }, - }); + const hasMembership = await ctx.prisma.membership.findFirst({ + where: { userId, teamId, accepted: true }, + select: { role: true }, + });apps/api/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.ts (1)
2251-2362: Replace schedulingType string literals and @ts-ignore with the enum; use MembershipRole for role checks.
- apps/api/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.ts — multiple schedulingType string-literals (lines: 237, 267, 308 [+// @ts-ignore], 404, 697, 1082 [+// @ts-ignore], 1182, 1431, 1461, 1502 [+// @ts-ignore], 1598, 1891, 2276 [+// @ts-ignore], 2376). Replace these with the repository's SchedulingType enum (or equivalent) and remove the corresponding ts-ignores.
- packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts — change hasCreatePermission = ["ADMIN","OWNER"].includes(hasMembership.role) (around lines 143–146) to use MembershipRole.ADMIN / MembershipRole.OWNER (or the repo's MembershipRole enum).
🧹 Nitpick comments (11)
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts (3)
121-147: Use enum, not string literals, for role fallback check.Prevents subtle mismatches if enum values change or differ by case.
- hasCreatePermission = ["ADMIN", "OWNER"].includes(hasMembership.role); + hasCreatePermission = [MembershipRole.ADMIN, MembershipRole.OWNER].includes(hasMembership.role);
90-93: Clarify and simplify “users” connect condition.Intent reads as “connect current user only for personal event types.” Use
teamIdas the source of truth and drop the misleading comment.- // Only connecting the current user for non-managed event types and non team event types - users: isManagedEventType || schedulingType ? undefined : { connect: { id: userId } }, + // Connect current user only for personal (non-team) event types + users: teamId ? undefined : { connect: { id: userId } },Please confirm no callers rely on
schedulingTypeabsence to imply “personal,” otherwise keep behavior and just update the comment.
150-154: Tighten log message details.Keep membership presence, but avoid leaking internal state when logs aggregate across tenants; include role only when membership exists.
- console.warn( - `User ${userId} does not have eventType.create permission for team ${teamId}. Membership found: ${!!hasMembership}` - ); + console.warn( + `User ${userId} lacks eventType.create for team ${teamId}. member=${!!hasMembership}${ + hasMembership ? ` role=${hasMembership.role}` : "" + }` + );apps/api/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.ts (8)
306-309: Remove ts-ignore; use enum for schedulingType.Prefer
SchedulingType.COLLECTIVEover string and drop the ts-ignore.- // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - schedulingType: "collective", + schedulingType: SchedulingType.COLLECTIVE,
404-405: Use enum for managed schedulingType.- schedulingType: "MANAGED", + schedulingType: SchedulingType.MANAGED,
444-457: Make host equality resilient to order.Response host order may vary; assert contents with length + arrayContaining.
- expect(responseTeamEvent?.hosts).toEqual([ + expect(responseTeamEvent?.hosts?.length).toEqual(2); + expect(responseTeamEvent?.hosts).toEqual(expect.arrayContaining([ { userId: teammate1.id, name: teammate1.name, username: teammate1.username, avatarUrl: teammate1.avatarUrl, }, { userId: teammate2.id, name: teammate2.name, username: teammate2.username, avatarUrl: teammate2.avatarUrl, }, - ]); + ]));
1081-1083: Remove ts-ignore; use enum for round robin.- // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - schedulingType: "roundRobin", + schedulingType: SchedulingType.ROUND_ROBIN,
1171-1218: Managed without hosts: use enum and assert no user event leaks.Use enum for schedulingType; rest looks solid.
- schedulingType: "MANAGED", + schedulingType: SchedulingType.MANAGED,
1500-1503: Remove ts-ignore; use enum in second suite as well.- // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - schedulingType: "collective", + schedulingType: SchedulingType.COLLECTIVE,
1598-1611: Use enum for managed in second suite.- schedulingType: "MANAGED", + schedulingType: SchedulingType.MANAGED,
1639-1651: Host array: use arrayContaining here too.- expect(responseTeamEvent?.hosts).toEqual([ + expect(responseTeamEvent?.hosts?.length).toEqual(2); + expect(responseTeamEvent?.hosts).toEqual(expect.arrayContaining([ { userId: teammate1.id, name: teammate1.name, username: teammate1.username, avatarUrl: teammate1.avatarUrl }, { userId: teammate2.id, name: teammate2.name, username: teammate2.username, avatarUrl: teammate2.avatarUrl }, - ]); + ]));
📜 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/api/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.ts(6 hunks)packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.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/trpc/server/routers/viewer/eventTypes/heavy/create.handler.tsapps/api/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.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/eventTypes/heavy/create.handler.tsapps/api/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.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/trpc/server/routers/viewer/eventTypes/heavy/create.handler.tsapps/api/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.ts
🧠 Learnings (5)
📓 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: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
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.
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.
📚 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/heavy/create.handler.ts
📚 Learning: 2025-08-26T20:23:28.396Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/features/calAIPhone/providers/retellAI/services/AgentService.ts:83-88
Timestamp: 2025-08-26T20:23:28.396Z
Learning: In calcom/cal.com PR #22995, the workflow update handler in packages/trpc/server/routers/viewer/workflows/update.handler.ts includes workflow-level authorization via isAuthorized(userWorkflow, ctx.user.id, "workflow.update") which validates the user can update the workflow before calling updateToolsFromAgentId (per maintainer Udit-takkar).
Applied to files:
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts
📚 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/heavy/create.handler.ts
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
apps/api/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.ts
🧬 Code graph analysis (2)
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts (2)
packages/lib/server/repository/membership.ts (1)
hasMembership(87-99)packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
apps/api/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.ts (7)
packages/platform/types/api.ts (1)
ApiSuccessResponse(8-8)packages/platform/types/event-types/event-types_2024_06_14/outputs/event-type.output.ts (2)
TeamEventTypeOutput_2024_06_14(486-529)EventTypeOutput_2024_06_14(475-484)packages/platform/types/event-types/event-types_2024_06_14/inputs/create-event-type.input.ts (2)
CreateTeamEventTypeInput_2024_06_14(535-593)Host(513-533)apps/api/v2/test/fixtures/repository/membership.repository.fixture.ts (1)
MembershipRepositoryFixture(8-44)apps/api/v2/test/fixtures/repository/profiles.repository.fixture.ts (1)
ProfileRepositoryFixture(7-31)apps/api/v2/test/fixtures/repository/event-types.repository.fixture.ts (1)
EventTypesRepositoryFixture(7-64)packages/platform/types/event-types/event-types_2024_06_14/inputs/update-event-type.input.ts (1)
UpdateTeamEventTypeInput_2024_06_14(453-488)
🔇 Additional comments (10)
apps/api/v2/src/modules/organizations/event-types/organizations-event-types.e2e-spec.ts (10)
175-186: Good: org membership downgraded to MEMBER while team membership is ADMIN.Accurately models “org member + team admin” for authorization coverage.
247-251: Status shift to 403 is correct for forbidden create outside org.Matches team-scoped admin expectations after guarded PBAC.
571-580: Status shift to 403 on wrong team update looks right.Consistent with the new “team admin or org permission” gating.
950-973: Nice coverage: delete scenarios (outside org, missing, success).Clear expectations across 403/404/200 paths.
974-1055: Default bookingFields assertions LGTM.Matches current platform defaults; good regression coverage.
1220-1224: Helper LGTM.Concise host comparator; good reuse across tests.
1416-1445: Second suite “outside org create”: expected 404 is consistent.Different actor model yields 404; matches access rules.
1706-1709: 404 on get outside org is appropriate in this actor model.
1765-1774: 404 on patch wrong team is consistent with second suite’s expectations.
2144-2166: Delete tests (outside org + success) look good in second suite.
ThyMinimalDev
force-pushed
the
fix-event-type-org-admin
branch
from
504673c to
34ff9a0
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
| import type { User, Team } from "@calcom/prisma/client"; | ||
|
|
||
| describe("Organizations Event Types Endpoints", () => { | ||
| describe("User Authentication - User is Org Admin but not team admin", () => { |
There was a problem hiding this comment.
here we test with an ORG ADMIN who is not a MEMBER of the team, but the team belongs to the org
| import type { User, Team } from "@calcom/prisma/client"; | ||
|
|
||
| describe("Organizations Event Types Endpoints", () => { | ||
| describe("User Authentication - User is Org Admin", () => { |
There was a problem hiding this comment.
in this file we now test with an ORG MEMBER who is TEAM ADMIN
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/api/v2/src/modules/organizations/event-types/organizations-member-team-admin-event-types.e2e-spec.ts (1)
1073-1075: Remove @ts-ignore for roundRobin scheduling typeSimilar to Line 300-301, use the proper enum value instead of suppressing TypeScript errors.
- // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - schedulingType: "roundRobin", + schedulingType: SchedulingType.ROUND_ROBIN,apps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.ts (2)
299-301: Remove @ts-ignore and use proper enum valueUse the SchedulingType enum directly instead of suppressing TypeScript errors.
- // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - schedulingType: "collective", + schedulingType: SchedulingType.COLLECTIVE,
1073-1075: Remove @ts-ignore for roundRobin scheduling typeUse the proper enum value instead of suppressing TypeScript errors.
- // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - schedulingType: "roundRobin", + schedulingType: SchedulingType.ROUND_ROBIN,
🧹 Nitpick comments (2)
apps/api/v2/src/modules/organizations/event-types/organizations-member-team-admin-event-types.e2e-spec.ts (2)
38-38: Test description doesn't match the membership configurationThe test description says "User is Org member and team admin", but Line 175 assigns the user an org-level "MEMBER" role while Line 183 assigns a team-level "ADMIN" role. Consider clarifying the description to specify both roles explicitly.
- describe("User Authentication - User is Org member and team admin", () => { + describe("User Authentication - User is Org member (not admin) and team admin", () => {
299-301: Remove @ts-ignore and use proper type castingInstead of suppressing TypeScript errors, use the already imported
SchedulingTypeenum directly since the Transform decorator handles the conversion.- // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - schedulingType: "collective", + schedulingType: SchedulingType.COLLECTIVE,
📜 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 (3)
apps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.ts(1 hunks)apps/api/v2/src/modules/organizations/event-types/organizations-member-team-admin-event-types.e2e-spec.ts(6 hunks)packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts
🧰 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/api/v2/src/modules/organizations/event-types/organizations-member-team-admin-event-types.e2e-spec.tsapps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.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/api/v2/src/modules/organizations/event-types/organizations-member-team-admin-event-types.e2e-spec.tsapps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.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/api/v2/src/modules/organizations/event-types/organizations-member-team-admin-event-types.e2e-spec.tsapps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.ts
🧠 Learnings (3)
📓 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.
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
apps/api/v2/src/modules/organizations/event-types/organizations-member-team-admin-event-types.e2e-spec.tsapps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.ts
📚 Learning: 2025-08-21T13:44:06.805Z
Learnt from: supalarry
PR: calcom/cal.com#23217
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts:93-94
Timestamp: 2025-08-21T13:44:06.805Z
Learning: In apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts, repository functions that use explicit Prisma select clauses (like getEventTypeWithSeats) are used for specific purposes and don't need to include all EventType fields like bookingRequiresAuthentication. These methods don't feed into the general OutputEventTypesService_2024_06_14 flow.
Applied to files:
apps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.ts
🧬 Code graph analysis (1)
apps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.ts (7)
apps/api/v2/test/fixtures/repository/membership.repository.fixture.ts (1)
MembershipRepositoryFixture(8-44)apps/api/v2/test/fixtures/repository/profiles.repository.fixture.ts (1)
ProfileRepositoryFixture(7-31)apps/api/v2/test/fixtures/repository/event-types.repository.fixture.ts (1)
EventTypesRepositoryFixture(7-64)packages/platform/types/event-types/event-types_2024_06_14/outputs/event-type.output.ts (2)
TeamEventTypeOutput_2024_06_14(486-529)EventTypeOutput_2024_06_14(475-484)apps/api/v2/src/app.ts (1)
bootstrap(27-89)packages/platform/types/event-types/event-types_2024_06_14/inputs/create-event-type.input.ts (1)
CreateTeamEventTypeInput_2024_06_14(535-593)packages/platform/types/api.ts (1)
ApiSuccessResponse(8-8)
⏰ 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). (6)
- GitHub Check: Production builds / Build API v1
- GitHub Check: Production builds / Build Web App
- GitHub Check: Type check / check-types
- GitHub Check: Production builds / Build API v2
- GitHub Check: Tests / Unit
- GitHub Check: Production builds / Build Atoms
🔇 Additional comments (7)
apps/api/v2/src/modules/organizations/event-types/organizations-member-team-admin-event-types.e2e-spec.ts (4)
222-251: Access control tests correctly expect 403 for out-of-org operationsThe test now expects a 403 (Forbidden) status when an org member who is a team admin attempts to create an event-type for a team outside their organization. This aligns with the intended PBAC logic where organization membership boundaries are enforced.
511-514: Test correctly expects 403 for accessing team event-types outside the orgThe change from 404 to 403 is appropriate here as it correctly distinguishes between "not found" and "forbidden access" when trying to get an event-type of a team outside the organization.
571-579: Incorrect expectation for out-of-org team updateThe test expects 403 when updating an event-type for an incorrect team (outside the org), maintaining consistency with the access control pattern established in create and read operations.
943-953: Test correctly expects 403 for deleting team event-types outside the orgThe change to expect 403 instead of 404 is consistent with the access control pattern for operations on resources outside the organization boundary.
apps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.ts (3)
215-244: Test correctly expects 404 for org admin accessing teams outside their orgThe test appropriately expects 404 (Not Found) when an org admin tries to create an event-type for a team outside their organization, which is the correct response since the team doesn't exist within their organizational context.
504-508: Consistent 404 responses for out-of-org team operationsThe test correctly expects 404 when an org admin attempts to access event-types of teams outside their organization, maintaining consistency with the create operation behavior.
38-38: Test description accurately reflects the scenario — verifieduserAdmin is created with role "ADMIN" on the organization (membershipsRepositoryFixture.create with team.connect id = org.id — apps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.ts lines ~174–178); teammate1/teammate2 are created as MEMBERS on the team (team.connect id = team.id). No membership for userAdmin on the team exists.
| it("should delete event-type not part of the team", async () => { | ||
| return request(app.getHttpServer()) | ||
| .delete(`/v2/organizations/${org.id}/teams/${team.id}/event-types/99999`) | ||
| .expect(404); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Verify: "should delete event-type not part of the team"
The test title says "should delete event-type not part of the team" but actually tests deletion of a non-existent event-type (ID: 99999). The expected 404 is correct for a non-existent resource.
Let me verify if this test case is testing the right scenario:
🏁 Script executed:
#!/bin/bash
# Check if there are any event-types created for falseTestTeam that could be used for cross-team deletion test
rg -n "falseTestTeam.*event-?type" apps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.tsLength of output: 513
Align test name and behavior: it currently tests a non-existent ID, not a cross-team deletion
The test in apps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.ts (lines 949-952) sends DELETE .../teams/${team.id}/event-types/99999 and expects 404 — that verifies a non-existent resource, not "event-type not part of the team". Either:
- Replace 99999 with an existing event-type owned by the other team (e.g., collectiveEventType.id from falseTestTeam) so the test verifies deletion is rejected for an event-type that belongs to a different team, or
- Rename the test to reflect that it asserts a 404 for a non-existent event-type.
🤖 Prompt for AI Agents
In
apps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.ts
around lines 949-952, the test name says it verifies deletion of an event-type
not part of the team but uses a non-existent ID (99999) so it only asserts "not
found"; to fix, either replace 99999 with an actual event-type ID owned by the
other team (e.g., use collectiveEventType.id from falseTestTeam or the
appropriate fixture variable) so the request targets a real cross-team
event-type and keeps the .expect(404) (or update expectation to the correct
status if your app returns a different code for forbidden cross-team deletion),
or rename the test to state it asserts "non-existent event-type returns 404".
Ensure the chosen event-type variable is in scope in this test before
substituting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.ts (3)
181-193: Missing team membership for test user.The test user (userAdmin) is not added as a member of the team, which is correct for testing the "org admin but not team member" scenario. However, consider adding a comment to make this intentional omission explicit.
accepted: true, }); + // Note: userAdmin is intentionally NOT added as a member of team + // to test org admin privileges without team membership + await membershipsRepositoryFixture.create({ role: "MEMBER", user: { connect: { id: teammate1.id } },
504-508: 404 responses for cross-org operations differ from member test.All operations return 404 instead of 403 for teams outside the organization, which differs from the member-team-admin test. This inconsistency should be addressed for uniform authorization behavior.
Consider standardizing the error responses:
- Use 403 (Forbidden) when the user is authenticated but lacks permission
- Use 404 (Not Found) only when the resource genuinely doesn't exist or to prevent information disclosure
The current implementation might be leaking information about resource existence based on user role.
Also applies to: 534-537, 943-946
1-1232: Consider adding test coverage for mixed permissions scenarios.The current tests cover "org admin not team member" and "org member who is team admin" scenarios. Consider adding tests for:
- User who is both org admin AND team admin
- User who is neither org admin nor team member (should always fail)
- Cross-team operations within the same org
Would you like me to generate additional test cases to cover these mixed permission scenarios to ensure comprehensive authorization testing?
📜 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 (3)
apps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.ts(1 hunks)apps/api/v2/src/modules/organizations/event-types/organizations-member-team-admin-event-types.e2e-spec.ts(6 hunks)packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts
🧰 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/api/v2/src/modules/organizations/event-types/organizations-member-team-admin-event-types.e2e-spec.tsapps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.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/api/v2/src/modules/organizations/event-types/organizations-member-team-admin-event-types.e2e-spec.tsapps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.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/api/v2/src/modules/organizations/event-types/organizations-member-team-admin-event-types.e2e-spec.tsapps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.ts
🧠 Learnings (1)
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
apps/api/v2/src/modules/organizations/event-types/organizations-member-team-admin-event-types.e2e-spec.tsapps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.ts
🧬 Code graph analysis (1)
apps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.ts (8)
apps/api/v2/test/fixtures/repository/membership.repository.fixture.ts (1)
MembershipRepositoryFixture(8-44)apps/api/v2/test/fixtures/repository/profiles.repository.fixture.ts (1)
ProfileRepositoryFixture(7-31)apps/api/v2/test/fixtures/repository/event-types.repository.fixture.ts (1)
EventTypesRepositoryFixture(7-64)packages/platform/types/event-types/event-types_2024_06_14/outputs/event-type.output.ts (2)
TeamEventTypeOutput_2024_06_14(486-529)EventTypeOutput_2024_06_14(475-484)apps/api/v2/src/app.ts (1)
bootstrap(27-89)packages/platform/types/event-types/event-types_2024_06_14/inputs/create-event-type.input.ts (1)
CreateTeamEventTypeInput_2024_06_14(535-593)packages/platform/types/api.ts (1)
ApiSuccessResponse(8-8)packages/platform/types/event-types/event-types_2024_06_14/inputs/update-event-type.input.ts (1)
UpdateTeamEventTypeInput_2024_06_14(453-488)
🔇 Additional comments (11)
apps/api/v2/src/modules/organizations/event-types/organizations-member-team-admin-event-types.e2e-spec.ts (5)
38-38: Test scenario description is accurate.The updated test description "User is Org member and team admin" correctly reflects the test setup where the user has MEMBER role in the organization and ADMIN role in the team.
174-179: LGTM! Role configuration change aligns with test intent.The role change from ADMIN to MEMBER for the org membership correctly tests the scenario where a user is a regular org member but has team admin privileges.
247-250: Good: Proper 403 response for team outside organization.The test correctly expects 403 (Forbidden) when an org member tries to create an event-type for a team outside their organization, which is the appropriate HTTP status code for authorization failures.
512-514: Consistent authorization behavior for team operations outside org.All operations (GET, PATCH, DELETE) correctly return 403 for teams outside the organization, maintaining consistent authorization enforcement.
Also applies to: 577-579, 952-953
1220-1237: Test cleanup is properly implemented.The afterAll hook correctly cleans up all test fixtures including users, teams, and organizations.
apps/api/v2/src/modules/organizations/event-types/organizations-admin-not-team-member-event-types.e2e-spec.ts (6)
38-38: Test scenario description accurately reflects the membership configuration.The description "User is Org Admin but not team admin" correctly describes the test setup.
174-179: LGTM! Org admin role properly configured.The user is correctly set up as an organization ADMIN without being added as a team member, testing the authorization for org-level privileges.
276-383: LGTM! Comprehensive test coverage for event-type creation.The tests thoroughly validate that org admins can create both collective and managed event-types for teams within their organization, with proper validation of all response fields.
Also applies to: 385-466
890-941: Good coverage of assignAllTeamMembers feature.The test properly validates that org admins can assign all team members to a managed event-type, including verification of the database state.
1163-1211: LGTM! Tests edge case for managed event-types without hosts.Good coverage of the scenario where a managed event-type is created without specifying hosts.
215-244: Consider validating authorization behavior difference.The test expects 404 when an org admin tries to create an event-type for a team outside their org. This differs from the 403 returned in the member-team-admin test. Consider whether this inconsistency is intentional or if both should return 403 for authorization failures.
E2E results are ready! |
* fix: org admins can manage team even-types * fix: org admins can manage team even-types
3 participants
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist