fix: always check team.parentID even if no membership#23976
fix: always check team.parentID even if no membership#23976sean-brydon merged 4 commits intomainfrom
Conversation
WalkthroughAdds Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ 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 |
![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
🧹 Nitpick comments (1)
packages/features/pbac/services/permission-check.service.ts (1)
236-246: Verify potential null reference on membership.userId.At line 239, when
membershipexists and has ateam.parentId, the code accessesmembership.userId. However, whenmembershipis null (lines 240-245), the fallback correctly usesquery.userId.While the current implementation appears correct since
membership.userIdwould matchquery.userIdwhen the membership was fetched using the userId, consider usingquery.userIdconsistently for clarity:// Get org membership either through the team membership or directly from teamId if (membership?.team.parentId) { // User has team membership, check org through that - orgMembership = await this.repository.getOrgMembership(membership.userId, membership.team.parentId); + orgMembership = await this.repository.getOrgMembership(query.userId!, membership.team.parentId); } else if (query.userId && query.teamId) {
📜 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)
packages/features/pbac/domain/repositories/IPermissionRepository.ts(1 hunks)packages/features/pbac/infrastructure/repositories/PermissionRepository.ts(1 hunks)packages/features/pbac/services/__tests__/permission-check.service.test.ts(10 hunks)packages/features/pbac/services/permission-check.service.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*Repository.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Repository files must include
Repositorysuffix, prefix with technology if applicable (e.g.,PrismaAppRepository.ts), and use PascalCase matching the exported class
Files:
packages/features/pbac/infrastructure/repositories/PermissionRepository.tspackages/features/pbac/domain/repositories/IPermissionRepository.ts
**/*.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/infrastructure/repositories/PermissionRepository.tspackages/features/pbac/domain/repositories/IPermissionRepository.tspackages/features/pbac/services/permission-check.service.tspackages/features/pbac/services/__tests__/permission-check.service.test.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/infrastructure/repositories/PermissionRepository.tspackages/features/pbac/domain/repositories/IPermissionRepository.tspackages/features/pbac/services/permission-check.service.tspackages/features/pbac/services/__tests__/permission-check.service.test.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/pbac/infrastructure/repositories/PermissionRepository.tspackages/features/pbac/domain/repositories/IPermissionRepository.tspackages/features/pbac/services/permission-check.service.tspackages/features/pbac/services/__tests__/permission-check.service.test.ts
**/*.{service,repository}.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Avoid dot-suffixes like
.service.tsor.repository.tsfor new files; reserve.test.ts,.spec.ts,.types.tsfor their specific purposes
Files:
packages/features/pbac/services/permission-check.service.ts
🧬 Code graph analysis (1)
packages/features/pbac/services/permission-check.service.ts (1)
packages/platform/libraries/repositories.ts (1)
MembershipRepository(11-11)
⏰ 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: Detect changes
🔇 Additional comments (10)
packages/features/pbac/infrastructure/repositories/PermissionRepository.ts (1)
97-105: LGTM! Clean implementation of team data retrieval.The new
getTeamByIdmethod correctly implements the interface contract, usingselectto fetch only the required fields (id and parentId) as per the coding guidelines.packages/features/pbac/domain/repositories/IPermissionRepository.ts (1)
40-44: LGTM! Well-defined interface addition.The new method signature is properly typed and clearly indicates its purpose for retrieving team hierarchy information.
packages/features/pbac/services/__tests__/permission-check.service.test.ts (4)
40-40: LGTM! Mock repository properly updated.The test mock correctly includes the new
getTeamByIdmethod to support the updated test scenarios.
140-164: LGTM! Comprehensive test for org-level fallback.Excellent test coverage for the new org-level permission check behavior when team membership is absent. The test properly verifies the call sequence: attempt team membership → fetch team details → check org membership → validate org permissions.
276-303: LGTM! Consistent test coverage for bulk permissions.The test properly validates that
checkPermissionsfollows the same org-level fallback pattern ascheckPermission, ensuring consistency across permission check methods.
403-429: LGTM! Thorough resource permissions test.Good test coverage for
getResourcePermissionswith org-level fallback, properly verifying that permissions are correctly resolved through the organization when team membership is absent.packages/features/pbac/services/permission-check.service.ts (4)
74-81: LGTM! Clear comment update reflecting new behavior.The comment accurately describes that org-level permissions now work without requiring team membership, which aligns with the PR's objective.
117-118: Cleaner permission check flow with unified method.Good refactoring to use the internal
hasPermissionmethod, reducing code duplication and ensuring consistent permission evaluation logic.
121-127: LGTM! Proper fallback handling for non-PBAC teams.The fallback correctly ensures that team membership is required when PBAC is disabled, maintaining backward compatibility.
162-163: Consistent implementation for bulk permission checks.Good consistency with the single permission check, using the same unified
hasPermissionsmethod.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
| userId, | ||
| teamId: ctx.user.organizationId, | ||
| permission: "eventType.create", | ||
| fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER], |
There was a problem hiding this comment.
These are the roles we fallback to checking if PBAC is disabled.
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 (1)
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts (1)
79-81: Fix user linkage guard — use teamId (not schedulingType)Gating
usersonisManagedEventType || schedulingTypeis incorrect: any set schedulingType will skip linking the creator. Gate on teamId instead.File: packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts (users line ~75–81)
- // Only connecting the current user for non-managed event types and non team event types - users: isManagedEventType || schedulingType ? undefined : { connect: { id: userId } }, + // Only connect the current user for non-managed, non-team event types + users: isManagedEventType || !!teamId ? undefined : { connect: { id: userId } },Verify test coverage for managed vs non-managed × with/without teamId.
🧹 Nitpick comments (3)
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts (3)
57-57: Inject request-scoped dependencies into PermissionCheckService.
Constructing with defaults binds to the global Prisma instance; prefer request-scoped repositories/services (e.g., using ctx.prisma) to preserve transactions/region affinity.Would you like me to draft a follow-up change to accept ctx.prisma (or a repository instance) here and wire it through the service?
114-119: Good: team-level permission check now runs even without team membership.
This enables the new parentId fallback path in the service.The comments above (Lines 110–114) are now stale—consider updating/removing them.
100-106: Use Prisma select to minimize data fetched.
You only use membership as a boolean; fetch just an id.Apply this diff:
- const hasMembership = await ctx.prisma.membership.findFirst({ + const hasMembership = await ctx.prisma.membership.findFirst({ where: { userId, teamId: teamId, accepted: true, }, + select: { id: true }, });
📜 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 (1)
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts(3 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.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.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.ts
🧬 Code graph analysis (1)
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts (2)
packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(19-290)packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
🔇 Additional comments (1)
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts (1)
5-5: Centralizing permission checks via PermissionCheckService looks good.
This aligns the handler with the new PBAC flow.
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 (2)
80-80: Bug:usersconnect condition usesschedulingType, effectively disabling user connect.
TCreateInputSchema.schedulingTypeis always truthy (enum), so this prevents connecting the creating user on personal (non-team, non-managed) event types.- users: isManagedEventType || schedulingType ? undefined : { connect: { id: userId } }, + users: isManagedEventType || teamId ? undefined : { connect: { id: userId } },
146-153: Guard against nullprofile.idbefore create.
profile.idis typed asnumber | null. Creating with a nullprofileIdwill fail at runtime.const profile = ctx.user.profile; + if (!profile?.id) { + throw new TRPCError({ code: "BAD_REQUEST", message: "Profile not found for user." }); + } try { const eventTypeRepo = new EventTypeRepository(ctx.prisma); const eventType = await eventTypeRepo.create({ ...data, profileId: profile.id,
🧹 Nitpick comments (3)
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts (3)
99-109: Tighten condition and name for clarity.
schedulingTypeis always truthy; the extra check is redundant. Also consider a clearer variable name.- if (teamId && schedulingType) { + if (teamId) { @@ - const hasCreatePermission = await permissionService.checkPermission({ + const hasTeamEventTypeCreatePermission = await permissionService.checkPermission({ userId, teamId, permission: "eventType.create", fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER], }); @@ - if (!isSystemAdmin && !hasOrgEventTypeCreatePermission && !hasCreatePermission) { + if (!isSystemAdmin && !hasOrgEventTypeCreatePermission && !hasTeamEventTypeCreatePermission) {
57-57: Instantiate PermissionCheckService once (inject or hoist) to reduce per-request churn.Not a blocker, but constructing per-call adds overhead and complicates testing.
Outside this function, add:
// at module scope const permissionCheckService = new PermissionCheckService();Then inside the handler use
permissionCheckService.
110-115: Minor: augment log context.Consider adding
organizationIdto aid triage while keeping PII minimal.
📜 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 (1)
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts(3 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.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.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.ts
🧠 Learnings (1)
📚 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
🧬 Code graph analysis (1)
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts (1)
packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(19-290)
⏰ 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: Tests / Unit
- GitHub Check: Type check / check-types
🔇 Additional comments (2)
packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts (2)
128-135: Good Prisma usage: only selecting required fields.Adheres to the guideline to avoid
includeand select only what's needed.
61-68: Don't overwrite org-admin fallback; OR-in the PBAC result.Overwriting drops existing
isOrgAdmintruthiness if PBAC returns false. Preserve any prior truthy value.- if (ctx.user.organizationId) { - hasOrgEventTypeCreatePermission = await permissionService.checkPermission({ - userId, - teamId: ctx.user.organizationId, - permission: "eventType.create", - fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER], - }); - } + if (ctx.user.organizationId) { + const orgPBAC = await permissionService.checkPermission({ + userId, + teamId: ctx.user.organizationId, + permission: "eventType.create", + fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER], + }); + hasOrgEventTypeCreatePermission = hasOrgEventTypeCreatePermission || orgPBAC; + }Please reconfirm that
ctx.user.organizationIdis the org “teamId” used by PBAC (was previously verified via seed scripts).
E2E results are ready! |
* fix: always check team.parentID even if no membership * refactor permission handling in create event type handler * remove hasMembership --------- Co-authored-by: Morgan <33722304+ThyMinimalDev@users.noreply.github.com>
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