Skip to content

Comments

refactor: use permission check service for isTeamAdmin#24026

Merged
hbjORbj merged 23 commits intomainfrom
refactor/pbac-for-isTeamAdmin
Sep 25, 2025
Merged

refactor: use permission check service for isTeamAdmin#24026
hbjORbj merged 23 commits intomainfrom
refactor/pbac-for-isTeamAdmin

Conversation

@hbjORbj
Copy link
Contributor

@hbjORbj hbjORbj commented Sep 24, 2025

What does this PR do?

Impact

  • Gets rid of isTeamAdmin util and replacing all its usages with PermissionCheckService
Screenshot 2025-09-24 at 5 32 32 PM

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • N/A - I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • To be tested

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 24, 2025

Walkthrough

This PR replaces legacy admin checks with PBAC via PermissionCheckService across multiple handlers. The function canAccess is renamed to canAccessOrganization, and all call sites updated. Team/org permissions now use explicit strings (e.g., organization.read, organization.invite, team.invite, team.update, eventType.update, booking.update) with fallback roles OWNER/ADMIN. isTeamAdmin is removed; role-management.factory now queries memberships directly. DSYNC and SSO handlers switch to canAccessOrganization. Invite, publish, update, and booking handlers now gate actions through PermissionCheckService. Tests are updated to mock PermissionCheckService.

Possibly related PRs

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title concisely summarizes the primary change by indicating that the refactor replaces the isTeamAdmin utility with the PermissionCheckService, uses a conventional “refactor:” prefix, and clearly communicates the main intent without extraneous detail.
Description Check ✅ Passed The description clearly states that the PR removes the isTeamAdmin util in favor of PermissionCheckService, includes confirmation of self-review, documentation impact, and test coverage, and thus directly relates to the code changes without introducing unrelated information.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/pbac-for-isTeamAdmin

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@graphite-app graphite-app bot requested review from a team September 24, 2025 08:12
@hbjORbj hbjORbj changed the title refactor: pbac for isteamadmin refactor: use permission check service for isTeamAdmin Sep 24, 2025
@hbjORbj hbjORbj marked this pull request as draft September 24, 2025 08:12
@keithwillcode keithwillcode added core area: core, team members only foundation labels Sep 24, 2025
@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 24, 2025
@vercel
Copy link

vercel bot commented Sep 24, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
cal Ignored Ignored Sep 25, 2025 2:00am
cal-eu Ignored Ignored Sep 25, 2025 2:00am

@hbjORbj hbjORbj marked this pull request as ready for review September 24, 2025 12:33
@dosubot dosubot bot added the teams area: teams, round robin, collective, managed event-types label Sep 24, 2025
@hbjORbj hbjORbj requested a review from sean-brydon September 24, 2025 12:37
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (8)
packages/trpc/server/routers/viewer/teams/removeHostsFromEventTypes.handler.ts (1)

31-40: Scope delete to teamId to prevent cross‑team host removal.
Currently, a user with permission on team A could pass eventTypeIds from team B and delete hosts there. Constrain by teamId.

   return await prisma.host.deleteMany({
     where: {
       eventTypeId: {
         in: eventTypeIds,
       },
       userId: {
         in: userIds,
       },
+      // Ensure we only affect event types belonging to the provided team
+      eventType: {
+        teamId,
+      },
     },
   });

If Prisma relational filters aren’t supported for deleteMany in your version, first resolve allowed IDs, then delete:

const allowed = await prisma.eventType.findMany({
  where: { id: { in: eventTypeIds }, teamId },
  select: { id: true },
});
const allowedIds = allowed.map((e) => e.id);
if (!allowedIds.length) return { count: 0 };
return prisma.host.deleteMany({
  where: { eventTypeId: { in: allowedIds }, userId: { in: userIds } },
});
packages/trpc/server/routers/viewer/teams/updateInternalNotesPresets.handler.ts (2)

53-94: Make updates atomic and return only necessary fields (Prisma best practices)

  • Wrap delete + upserts in a single transaction to avoid partial state on failure.
  • Select only fields needed by the client; avoid returning entire records.

Apply:

-// Delete removed presets
-if (idsToDelete.length > 0) {
-  await prisma.internalNotePreset.deleteMany({
-    where: {
-      id: {
-        in: idsToDelete,
-      },
-      teamId: input.teamId,
-    },
-  });
-}
-
-// Update or create presets
-const updatedPresets = await Promise.all(
-  input.presets.map((preset) => {
-    if (preset.id && preset.id !== -1) {
-      // Update existing preset
-      return prisma.internalNotePreset.update({
-        where: {
-          id: preset.id,
-          teamId: input.teamId,
-        },
-        data: {
-          name: preset.name,
-          cancellationReason: preset.cancellationReason,
-        },
-      });
-    } else {
-      // Create new preset
-      return prisma.internalNotePreset.create({
-        data: {
-          name: preset.name,
-          cancellationReason: preset.cancellationReason,
-          teamId: input.teamId,
-        },
-      });
-    }
-  })
-);
-
-return updatedPresets;
+const updatedPresets = await prisma.$transaction(async (tx) => {
+  if (idsToDelete.length > 0) {
+    await tx.internalNotePreset.deleteMany({
+      where: {
+        id: { in: idsToDelete },
+        teamId: input.teamId,
+      },
+    });
+  }
+
+  const results = await Promise.all(
+    input.presets.map((preset) => {
+      if (preset.id && preset.id !== -1) {
+        return tx.internalNotePreset.update({
+          where: {
+            id: preset.id,
+            teamId: input.teamId,
+          },
+          data: {
+            name: preset.name,
+            cancellationReason: preset.cancellationReason,
+          },
+          select: {
+            id: true,
+            name: true,
+            cancellationReason: true,
+          },
+        });
+      } else {
+        return tx.internalNotePreset.create({
+          data: {
+            name: preset.name,
+            cancellationReason: preset.cancellationReason,
+            teamId: input.teamId,
+          },
+          select: {
+            id: true,
+            name: true,
+            cancellationReason: true,
+          },
+        });
+      }
+    })
+  );
+
+  return results;
+});
+
+return updatedPresets;

96-96: Don't remove the default export without updating its import

packages/trpc/server/routers/viewer/teams/_router.tsx:188 currently does:
const { default: handler } = await import("./updateInternalNotesPresets.handler");
Either keep the default export in packages/trpc/server/routers/viewer/teams/updateInternalNotesPresets.handler.ts, or remove it and update the dynamic import to use the named export:
const { updateInternalNotesPresetsHandler: handler } = await import("./updateInternalNotesPresets.handler");
If removing default, also delete export default updateInternalNotesPresetsHandler; (handler file, ~line 96).

packages/trpc/server/routers/viewer/teams/addMembersToEventTypes.handler.ts (1)

32-38: Enforce that event types belong to the target team

Without validating ownership, a user could modify event types in another team by passing arbitrary IDs.

Apply this diff to validate and prevent cross-team modifications:

   const data: Prisma.HostCreateManyInput[] = eventTypeIds.flatMap((eventId) =>
     userIds.map((userId) => ({
       eventTypeId: eventId,
       userId: userId,
       priority: 2, // Default medium priority
     }))
   );
+
+  // Ensure all event types belong to the specified team
+  const validEventTypes = await prisma.eventType.findMany({
+    where: { id: { in: eventTypeIds }, teamId },
+    select: { id: true },
+  });
+  const validIds = new Set(validEventTypes.map((e) => e.id));
+  if (validIds.size !== eventTypeIds.length) {
+    throw new TRPCError({ code: "UNAUTHORIZED", message: "Invalid event types for team" });
+  }
packages/trpc/server/routers/viewer/dsync/delete.handler.ts (1)

21-35: Critical: Missing organizationId validation can delete all DSYNC records.

When input.organizationId is null on self-hosted (HOSTED_CAL_FEATURES=false), canAccessOrganization may allow access, and deleteMany receives { organizationId: undefined } which Prisma treats as omitted, leading to deleteMany({ where: {} }) → mass delete.

Apply this fix to require organizationId and to avoid passing undefined:

 export const deleteHandler = async ({ ctx, input }: Options) => {
   const { dsyncController } = await jackson();

-  const { message, access } = await canAccessOrganization(ctx.user, input.organizationId);
+  if (!input.organizationId) {
+    throw new TRPCError({
+      code: "BAD_REQUEST",
+      message: "Organization ID is required",
+    });
+  }
+
+  const { message, access } = await canAccessOrganization(ctx.user, input.organizationId);

   if (!access) {
     throw new TRPCError({
       code: "BAD_REQUEST",
       message,
     });
   }

   await prisma.dSyncData.deleteMany({
     where: {
-      organizationId: input.organizationId || undefined,
+      organizationId: input.organizationId,
     },
   });
   await dsyncController.directories.delete(input.directoryId);

   return null;
 };
packages/trpc/server/routers/viewer/sso/update.handler.ts (1)

26-33: Require management-level permission for SSO/DSync writes — change canAccessOrganization to accept a required permission and enforce it.

canAccessOrganization currently calls PermissionCheckService.checkPermission(..., permission: "organization.read") in packages/features/ee/sso/lib/saml.ts; create/update/delete handlers call it unchanged, so write operations are gated only by a read permission. Change canAccessOrganization signature to accept a permission (default "organization.read") and call it with "organization.update" or a dedicated "organization.sso.manage" for SSO/DSync create/update/delete.

Files to update:

  • packages/features/ee/sso/lib/saml.ts (canAccessOrganization)
  • packages/trpc/server/routers/viewer/sso/update.handler.ts
  • packages/trpc/server/routers/viewer/sso/updateOIDC.handler.ts
  • packages/trpc/server/routers/viewer/sso/delete.handler.ts
  • packages/trpc/server/routers/viewer/dsync/create.handler.ts
  • packages/trpc/server/routers/viewer/dsync/delete.handler.ts
  • packages/features/ee/dsync/lib/server/userCanCreateTeamGroupMapping.ts
packages/trpc/server/routers/viewer/sso/updateOIDC.handler.ts (1)

24-31: Require stronger permission for SSO mutating handlers — make canAccessOrganization accept a requiredPermission and enforce it.

canAccessOrganization (packages/features/ee/sso/lib/saml.ts) currently checks "organization.read" but updateOIDC.handler.ts (packages/trpc/server/routers/viewer/sso/updateOIDC.handler.ts — calls createOIDCConnection) is a mutating operation. Change canAccessOrganization to accept a requiredPermission parameter and update mutating callers to pass "organization.update" (or a dedicated "organization.manageSSO") — e.g. updateOIDC.handler.ts, sso/update.handler.ts, sso/delete.handler.ts, dsync/create.handler.ts, dsync/delete.handler.ts.

packages/trpc/server/routers/viewer/teams/inviteMember/utils.ts (1)

94-106: Prisma: replace include with select (project only required fields).

Our guideline forbids include. Select the exact fields used later (team.name/id/slug/isOrganization/parentId/metadata and nested organizationSettings and parent).

Apply this diff:

-  const team = await prisma.team.findUnique({
-    where: {
-      id: teamId,
-    },
-    include: {
-      organizationSettings: true,
-      parent: {
-        include: {
-          organizationSettings: true,
-        },
-      },
-    },
-  });
+  const team = await prisma.team.findUnique({
+    where: { id: teamId },
+    select: {
+      id: true,
+      name: true,
+      slug: true,
+      isOrganization: true,
+      parentId: true,
+      metadata: true,
+      organizationSettings: {
+        select: {
+          isOrganizationVerified: true,
+          isOrganizationConfigured: true,
+          orgAutoAcceptEmail: true,
+        },
+      },
+      parent: {
+        select: {
+          id: true,
+          name: true,
+          slug: true,
+          organizationSettings: {
+            select: {
+              isOrganizationVerified: true,
+              isOrganizationConfigured: true,
+              orgAutoAcceptEmail: true,
+            },
+          },
+        },
+      },
+    },
+  });
🧹 Nitpick comments (33)
packages/trpc/server/routers/viewer/teams/removeHostsFromEventTypes.handler.ts (3)

20-21: Hoist PermissionCheckService instantiation to module scope.
Avoid per-request instantiation and ease stubbing; keep a module-level instance.

Apply within this file:

-  const permissionCheckService = new PermissionCheckService();

Add near the imports (top-level):

const permissionCheckService = new PermissionCheckService();

28-29: Prefer FORBIDDEN over UNAUTHORIZED for permission denials.
403 better reflects “authenticated but not allowed.” Align with repo conventions if they use FORBIDDEN elsewhere.

-  if (!hasEventTypeUpdatePermission) throw new TRPCError({ code: "UNAUTHORIZED" });
+  if (!hasEventTypeUpdatePermission) throw new TRPCError({ code: "FORBIDDEN" });

43-43: Avoid default export; prefer named export only.
Reduces import ambiguity and improves refactors.

-export default removeHostsFromEventTypesHandler;
packages/trpc/server/routers/viewer/teams/updateInternalNotesPresets.handler.ts (1)

21-31: Remove unsafe userId fallback and avoid per-call service instantiation

  • user is NonNullable, so ctx.user?.id || 0 is unnecessary and can hide bugs. Use ctx.user.id.
  • Instantiate PermissionCheckService once (module-scope or via ctx) instead of per request.

Apply this diff within these lines:

-    const permissionCheckService = new PermissionCheckService();
-    const hasTeamUpdatePermission = await permissionCheckService.checkPermission({
-      userId: ctx.user?.id || 0,
+    const hasTeamUpdatePermission = await permissionCheckService.checkPermission({
+      userId: ctx.user.id,
       teamId: input.teamId,
       permission: "team.update",
       fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN],
     });

Add this once at module scope (outside the handler) to avoid per-call construction:

// Place under imports
const permissionCheckService = new PermissionCheckService();

Also confirm the business rule: Org admins bypass PBAC here. Is that intended for team-level internal note presets?

packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (3)

37-42: Don’t load unused sensitive fields; switch to select and drop credentials.

booking.user.credentials isn’t used here; avoid loading it and prefer select per our Prisma guideline.

Apply this diff:

-      user: {
-        include: {
-          destinationCalendar: true,
-          credentials: true,
-        },
-      },
+      user: {
+        select: {
+          destinationCalendar: true,
+        },
+      },

106-108: Select only the attendee fields you need.

Reduce payload and align with our Prisma “select-only” guideline.

Apply this diff:

-    include: {
-      attendees: true,
-    },
+    include: {
+      attendees: {
+        select: {
+          name: true,
+          email: true,
+          timeZone: true,
+          locale: true,
+        },
+      },
+    },

51-60: Skip PBAC check when user is organizer or attendee

Avoid an extra DB call — only invoke PermissionCheckService when the user is not the organizer and not an attendee (and booking.eventType?.teamId exists).

File: packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts

-  let hasBookingUpdatePermission = false;
-  if (booking.eventType?.teamId) {
-    const permissionCheckService = new PermissionCheckService();
-    hasBookingUpdatePermission = await permissionCheckService.checkPermission({
-      userId: user.id,
-      teamId: booking.eventType.teamId,
-      permission: "booking.update",
-      fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN],
-    });
-  }
+  let hasBookingUpdatePermission = false;
+  if (!isOrganizer && !isAttendee && booking.eventType?.teamId) {
+    const permissionCheckService = new PermissionCheckService();
+    hasBookingUpdatePermission = await permissionCheckService.checkPermission({
+      userId: user.id,
+      teamId: booking.eventType.teamId,
+      permission: "booking.update",
+      fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN],
+    });
+  }
packages/features/ee/dsync/lib/server/userCanCreateTeamGroupMapping.ts (2)

19-25: Use UNAUTHORIZED for failed access checks

Returning BAD_REQUEST for permission failures is misleading; align with other handlers using UNAUTHORIZED.

Apply this diff:

-  if (!access) {
-    throw new TRPCError({
-      code: "BAD_REQUEST",
-      message,
-    });
-  }
+  if (!access) {
+    throw new TRPCError({
+      code: "UNAUTHORIZED",
+      message,
+    });
+  }

49-49: Prefer named export over default export

Improves tree-shaking and consistency with codebase guidance.

Apply this diff:

-export default userCanCreateTeamGroupMapping;
+export { userCanCreateTeamGroupMapping };

Note: update imports at call sites accordingly.

packages/trpc/server/routers/viewer/teams/publish.handler.ts (1)

94-94: Avoid default export; keep the named export only

Improves tree-shaking and refactorability.

Apply this diff:

-export default publishHandler;
+// Prefer named export only

Update imports where needed.

packages/trpc/server/routers/viewer/teams/addMembersToEventTypes.handler.ts (1)

47-47: Drop default export

Prefer named export only per project guidance.

Apply this diff:

-export default addMembersToEventTypesHandler;
+// Prefer named export only
packages/features/pbac/services/role-management.factory.ts (1)

99-113: Use enums and select minimal fields in Prisma query

  • Use MembershipRole enums instead of string literals.
  • Add select to avoid fetching unnecessary columns.

Apply this diff:

-      const team = await prisma.membership.findFirst({
-        where: {
-          userId,
-          teamId: targetId,
-          accepted: true,
-          OR: [{ role: "ADMIN" }, { role: "OWNER" }],
-        },
-      });
+      const team = await prisma.membership.findFirst({
+        where: {
+          userId,
+          teamId: targetId,
+          accepted: true,
+          OR: [{ role: MembershipRole.ADMIN }, { role: MembershipRole.OWNER }],
+        },
+        select: { id: true },
+      });
packages/trpc/server/routers/viewer/sso/get.handler.ts (1)

26-33: Use UNAUTHORIZED on access failure

Align with other handlers and clearer semantics for authorization errors.

Apply this diff:

-  if (!access) {
-    throw new TRPCError({
-      code: "BAD_REQUEST",
-      message,
-    });
-  }
+  if (!access) {
+    throw new TRPCError({
+      code: "UNAUTHORIZED",
+      message,
+    });
+  }
packages/trpc/server/routers/viewer/dsync/create.handler.ts (2)

35-42: Verify tenant format consistency for DSYNC.

Other SSO/connection handlers use tenantPrefix + teamId, while this uses ${organization.slug}-${organization.id}. Please confirm BoxyHQ tenant mapping remains consistent across create/get/delete flows, or centralize tenant construction in a shared util to avoid drift.


60-60: Prefer named exports over default exports.

Improves tree-shaking and refactorability in the codebase.

-export default createHandler;
+// Prefer importing the named export where used:
+// import { createHandler } from './create.handler';
packages/trpc/server/routers/viewer/sso/delete.handler.ts (1)

26-33: Consider UNAUTHORIZED/FORBIDDEN over BAD_REQUEST for auth failures.

If client contracts permit, using UNAUTHORIZED (or FORBIDDEN) would better reflect semantics than BAD_REQUEST.

packages/trpc/server/routers/viewer/dsync/delete.handler.ts (1)

40-40: Prefer named exports over default exports.

Align with repository guidelines for clearer imports and better tree-shaking.

-export default deleteHandler;
+// Prefer importing the named export where used:
+// import { deleteHandler } from './delete.handler';
packages/trpc/server/routers/viewer/sso/updateOIDC.handler.ts (1)

35-44: Guard against missing NEXT_PUBLIC_WEBAPP_URL.

If unset, defaultRedirectUrl/redirectUrl become invalid. Consider validating and failing fast.

packages/trpc/server/routers/viewer/dsync/get.handler.ts (1)

56-56: Prefer named exports over default exports.

Follow repo guidance for named exports.

-export default getHandler;
+// Prefer importing the named export where used:
+// import { getHandler } from './get.handler';
packages/trpc/server/routers/viewer/teams/update.handler.ts (5)

28-35: PBAC adoption looks good; minor cleanup.

ctx.user is NonNullable; you can safely pass ctx.user.id without a fallback 0.

-      userId: ctx.user?.id || 0,
+      userId: ctx.user.id,

42-47: Limit Prisma selection to required fields.

Only id is needed for the slug conflict check.

   if (input.slug) {
-    const userConflict = await prisma.team.findMany({
-      where: {
-        slug: input.slug,
-      },
-    });
+    const userConflict = await prisma.team.findMany({
+      where: { slug: input.slug },
+      select: { id: true },
+    });
     if (userConflict.some((t) => t.id !== input.id)) return;
   }

50-55: Select only needed fields for prevTeam.

You later use slug, metadata, and rrTimestampBasis; fetch only those.

-  const prevTeam = await prisma.team.findUnique({
-    where: {
-      id: input.id,
-    },
-  });
+  const prevTeam = await prisma.team.findUnique({
+    where: { id: input.id },
+    select: { slug: true, metadata: true, rrTimestampBasis: true, parentId: true },
+  });

114-117: Select only returned fields on update.

Reduce payload and ensure we never leak extra fields.

-  const updatedTeam = await prisma.team.update({
-    where: { id: input.id },
-    data,
-  });
+  const updatedTeam = await prisma.team.update({
+    where: { id: input.id },
+    data,
+    select: {
+      logoUrl: true,
+      name: true,
+      bio: true,
+      slug: true,
+      theme: true,
+      brandColor: true,
+      darkBrandColor: true,
+      bookingLimits: true,
+      includeManagedEventsInLimits: true,
+      rrResetInterval: true,
+      rrTimestampBasis: true,
+      parentId: true, // used later to compute redirects
+    },
+  });

184-184: Prefer named exports over default exports.

Matches repository guidance.

-export default updateHandler;
+// Prefer importing the named export where used:
+// import { updateHandler } from './update.handler';
packages/features/ee/sso/lib/saml.ts (1)

32-32: Naming nit: teamId parameter actually represents an organization/team scope.

Consider renaming to organizationId (or scopeId) for clarity across call sites in a follow-up.

packages/trpc/server/routers/viewer/teams/deleteInvite.handler.ts (1)

51-51: Prefer named exports; drop the default export.

Default exports hurt tree-shaking and refactors. Keep the named export only.

Apply this diff:

-export default deleteInviteHandler;
packages/trpc/server/routers/viewer/teams/inviteMember/utils.ts (2)

68-81: PBAC migration looks good; keep permission derivation consistent across handlers.

This uses team.invite vs organization.invite based on isOrg. Ensure deleteInvite/setInviteExpiration use the same rule for sub-teams to avoid authorization drift.

Would you like a small shared helper (e.g., deriveInvitePermission({ isOrganization })) to reuse across handlers and tests?


71-71: Track the TODO with an issue.

Invite-to-different-org gap is noted. Suggest creating a follow-up issue to avoid it getting lost.

I can open a draft patch for this check if helpful.

packages/trpc/server/routers/viewer/teams/createInvite.handler.ts (2)

30-41: Confirm permission scope for sub-teams and align all handlers.

Here, sub-teams use "team.invite". In deleteInvite/setInviteExpiration, sub-teams currently resolve to "organization.invite". Please pick one rule (recommended: only org teams use "organization.invite") and make all three handlers consistent.

Consider extracting a small helper to derive the permission string to prevent future drift.


75-75: Prefer named exports; drop the default export.

Stay consistent with named exports across server handlers.

Apply this diff:

-export default createInviteHandler;
packages/trpc/server/routers/viewer/teams/setInviteExpiration.handler.ts (1)

61-61: Prefer named exports; drop the default export.

Default export not needed; keep the named export.

Apply this diff:

-export default setInviteExpirationHandler;
packages/trpc/server/routers/viewer/teams/inviteMember/inviteMemberUtils.test.ts (2)

106-125: Add test for team-level permission path (isOrg: false).

We only assert organization.invite. Add a case verifying permission: "team.invite" when isOrg is false to safeguard future regressions.

I can add a minimal test mirroring these two cases with isOrg: false if you’d like.

Also applies to: 128-146


27-33: Remove obsolete mocks.

isOrganisationAdmin/isOrganisationOwner are no longer used; this mock can be deleted.

Apply this diff:

-vi.mock("@calcom/lib/server/queries/organisations", () => {
-  return {
-    isOrganisationAdmin: vi.fn(),
-    isOrganisationOwner: vi.fn(),
-  };
-});
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 788fbdb and 91be568.

📒 Files selected for processing (22)
  • packages/features/ee/dsync/lib/server/userCanCreateTeamGroupMapping.ts (2 hunks)
  • packages/features/ee/sso/lib/saml.ts (3 hunks)
  • packages/features/ee/teams/lib/queries.ts (0 hunks)
  • packages/features/pbac/services/role-management.factory.ts (1 hunks)
  • packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (2 hunks)
  • packages/trpc/server/routers/viewer/dsync/create.handler.ts (2 hunks)
  • packages/trpc/server/routers/viewer/dsync/delete.handler.ts (2 hunks)
  • packages/trpc/server/routers/viewer/dsync/get.handler.ts (2 hunks)
  • packages/trpc/server/routers/viewer/sso/delete.handler.ts (2 hunks)
  • packages/trpc/server/routers/viewer/sso/get.handler.ts (2 hunks)
  • packages/trpc/server/routers/viewer/sso/update.handler.ts (2 hunks)
  • packages/trpc/server/routers/viewer/sso/updateOIDC.handler.ts (2 hunks)
  • packages/trpc/server/routers/viewer/teams/addMembersToEventTypes.handler.ts (2 hunks)
  • packages/trpc/server/routers/viewer/teams/createInvite.handler.ts (2 hunks)
  • packages/trpc/server/routers/viewer/teams/deleteInvite.handler.ts (2 hunks)
  • packages/trpc/server/routers/viewer/teams/inviteMember/inviteMemberUtils.test.ts (3 hunks)
  • packages/trpc/server/routers/viewer/teams/inviteMember/utils.ts (2 hunks)
  • packages/trpc/server/routers/viewer/teams/publish.handler.ts (2 hunks)
  • packages/trpc/server/routers/viewer/teams/removeHostsFromEventTypes.handler.ts (2 hunks)
  • packages/trpc/server/routers/viewer/teams/setInviteExpiration.handler.ts (2 hunks)
  • packages/trpc/server/routers/viewer/teams/update.handler.ts (2 hunks)
  • packages/trpc/server/routers/viewer/teams/updateInternalNotesPresets.handler.ts (2 hunks)
💤 Files with no reviewable changes (1)
  • packages/features/ee/teams/lib/queries.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 use include, always use select
Ensure the credential.key field is never returned from tRPC endpoints or APIs

Files:

  • packages/trpc/server/routers/viewer/teams/updateInternalNotesPresets.handler.ts
  • packages/trpc/server/routers/viewer/sso/get.handler.ts
  • packages/trpc/server/routers/viewer/dsync/create.handler.ts
  • packages/trpc/server/routers/viewer/sso/update.handler.ts
  • packages/trpc/server/routers/viewer/sso/delete.handler.ts
  • packages/trpc/server/routers/viewer/teams/addMembersToEventTypes.handler.ts
  • packages/trpc/server/routers/viewer/dsync/delete.handler.ts
  • packages/trpc/server/routers/viewer/teams/setInviteExpiration.handler.ts
  • packages/trpc/server/routers/viewer/teams/removeHostsFromEventTypes.handler.ts
  • packages/trpc/server/routers/viewer/sso/updateOIDC.handler.ts
  • packages/trpc/server/routers/viewer/teams/createInvite.handler.ts
  • packages/features/ee/dsync/lib/server/userCanCreateTeamGroupMapping.ts
  • packages/trpc/server/routers/viewer/teams/inviteMember/inviteMemberUtils.test.ts
  • packages/trpc/server/routers/viewer/dsync/get.handler.ts
  • packages/trpc/server/routers/viewer/teams/update.handler.ts
  • packages/features/ee/sso/lib/saml.ts
  • packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
  • packages/trpc/server/routers/viewer/teams/inviteMember/utils.ts
  • packages/trpc/server/routers/viewer/teams/publish.handler.ts
  • packages/trpc/server/routers/viewer/teams/deleteInvite.handler.ts
  • packages/features/pbac/services/role-management.factory.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/updateInternalNotesPresets.handler.ts
  • packages/trpc/server/routers/viewer/sso/get.handler.ts
  • packages/trpc/server/routers/viewer/dsync/create.handler.ts
  • packages/trpc/server/routers/viewer/sso/update.handler.ts
  • packages/trpc/server/routers/viewer/sso/delete.handler.ts
  • packages/trpc/server/routers/viewer/teams/addMembersToEventTypes.handler.ts
  • packages/trpc/server/routers/viewer/dsync/delete.handler.ts
  • packages/trpc/server/routers/viewer/teams/setInviteExpiration.handler.ts
  • packages/trpc/server/routers/viewer/teams/removeHostsFromEventTypes.handler.ts
  • packages/trpc/server/routers/viewer/sso/updateOIDC.handler.ts
  • packages/trpc/server/routers/viewer/teams/createInvite.handler.ts
  • packages/features/ee/dsync/lib/server/userCanCreateTeamGroupMapping.ts
  • packages/trpc/server/routers/viewer/teams/inviteMember/inviteMemberUtils.test.ts
  • packages/trpc/server/routers/viewer/dsync/get.handler.ts
  • packages/trpc/server/routers/viewer/teams/update.handler.ts
  • packages/features/ee/sso/lib/saml.ts
  • packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
  • packages/trpc/server/routers/viewer/teams/inviteMember/utils.ts
  • packages/trpc/server/routers/viewer/teams/publish.handler.ts
  • packages/trpc/server/routers/viewer/teams/deleteInvite.handler.ts
  • packages/features/pbac/services/role-management.factory.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/teams/updateInternalNotesPresets.handler.ts
  • packages/trpc/server/routers/viewer/sso/get.handler.ts
  • packages/trpc/server/routers/viewer/dsync/create.handler.ts
  • packages/trpc/server/routers/viewer/sso/update.handler.ts
  • packages/trpc/server/routers/viewer/sso/delete.handler.ts
  • packages/trpc/server/routers/viewer/teams/addMembersToEventTypes.handler.ts
  • packages/trpc/server/routers/viewer/dsync/delete.handler.ts
  • packages/trpc/server/routers/viewer/teams/setInviteExpiration.handler.ts
  • packages/trpc/server/routers/viewer/teams/removeHostsFromEventTypes.handler.ts
  • packages/trpc/server/routers/viewer/sso/updateOIDC.handler.ts
  • packages/trpc/server/routers/viewer/teams/createInvite.handler.ts
  • packages/features/ee/dsync/lib/server/userCanCreateTeamGroupMapping.ts
  • packages/trpc/server/routers/viewer/teams/inviteMember/inviteMemberUtils.test.ts
  • packages/trpc/server/routers/viewer/dsync/get.handler.ts
  • packages/trpc/server/routers/viewer/teams/update.handler.ts
  • packages/features/ee/sso/lib/saml.ts
  • packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
  • packages/trpc/server/routers/viewer/teams/inviteMember/utils.ts
  • packages/trpc/server/routers/viewer/teams/publish.handler.ts
  • packages/trpc/server/routers/viewer/teams/deleteInvite.handler.ts
  • packages/features/pbac/services/role-management.factory.ts
🧠 Learnings (7)
📚 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/teams/updateInternalNotesPresets.handler.ts
  • packages/trpc/server/routers/viewer/sso/update.handler.ts
  • packages/trpc/server/routers/viewer/teams/update.handler.ts
  • packages/trpc/server/routers/viewer/teams/inviteMember/utils.ts
  • packages/trpc/server/routers/viewer/teams/publish.handler.ts
📚 Learning: 2025-08-17T22:00:16.329Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/aiVoiceAgent/_router.ts:117-126
Timestamp: 2025-08-17T22:00:16.329Z
Learning: In calcom/cal.com PR #22995, packages/trpc/server/routers/viewer/aiVoiceAgent/_router.ts, the enabled input parameter in the update endpoint is intentionally not forwarded to aiService.updateAgentConfiguration() as the enabled/disabled agent functionality is not required at the moment (per maintainer Udit-takkar). Future reviews should not flag this as missing functionality unless requirements change.

Applied to files:

  • packages/trpc/server/routers/viewer/teams/updateInternalNotesPresets.handler.ts
  • packages/trpc/server/routers/viewer/teams/update.handler.ts
📚 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
  • packages/trpc/server/routers/viewer/teams/removeHostsFromEventTypes.handler.ts
📚 Learning: 2025-08-27T16:39:38.192Z
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.

Applied to files:

  • packages/trpc/server/routers/viewer/teams/setInviteExpiration.handler.ts
  • packages/trpc/server/routers/viewer/teams/createInvite.handler.ts
  • packages/trpc/server/routers/viewer/teams/inviteMember/utils.ts
📚 Learning: 2025-08-27T12:15:43.830Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/aiVoiceAgent/testCall.handler.ts:41-44
Timestamp: 2025-08-27T12:15:43.830Z
Learning: In calcom/cal.com, the AgentService.getAgent() method in packages/features/calAIPhone/providers/retellAI/services/AgentService.ts does NOT include authorization checks - it only validates the agentId parameter and directly calls the repository without verifying user/team access. This contrasts with other methods like getAgentWithDetails() which properly use findByIdWithUserAccessAndDetails() for authorization. When reviewing updateToolsFromAgentId() calls, always verify both agent ownership and eventType ownership are checked.

Applied to files:

  • packages/trpc/server/routers/viewer/teams/removeHostsFromEventTypes.handler.ts
  • packages/trpc/server/routers/viewer/teams/update.handler.ts
  • packages/trpc/server/routers/viewer/teams/inviteMember/utils.ts
  • packages/features/pbac/services/role-management.factory.ts
📚 Learning: 2025-09-16T08:59:45.884Z
Learnt from: anglerfishlyy
PR: calcom/cal.com#0
File: :0-0
Timestamp: 2025-09-16T08:59:45.884Z
Learning: anglerfishlyy's PR #23467 demonstrates excellent software architecture with proper email validation (regex + zod), secure token generation using randomBytes, comprehensive host type system using discriminated unions (userId OR email), and smart reuse of existing team invitation infrastructure. The implementation includes robust error handling, type safety, and maintains backward compatibility while adding CreatableSelect-based email invitation functionality.

Applied to files:

  • packages/trpc/server/routers/viewer/teams/createInvite.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:

  • packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
🧬 Code graph analysis (12)
packages/trpc/server/routers/viewer/teams/updateInternalNotesPresets.handler.ts (2)
packages/features/pbac/services/permission-check.service.ts (1)
  • PermissionCheckService (19-290)
packages/platform/libraries/index.ts (1)
  • MembershipRole (34-34)
packages/trpc/server/routers/viewer/teams/addMembersToEventTypes.handler.ts (2)
packages/features/pbac/services/permission-check.service.ts (1)
  • PermissionCheckService (19-290)
packages/platform/libraries/index.ts (2)
  • MembershipRole (34-34)
  • TRPCError (66-66)
packages/trpc/server/routers/viewer/teams/setInviteExpiration.handler.ts (1)
packages/features/pbac/services/permission-check.service.ts (1)
  • PermissionCheckService (19-290)
packages/trpc/server/routers/viewer/teams/removeHostsFromEventTypes.handler.ts (1)
packages/features/pbac/services/permission-check.service.ts (1)
  • PermissionCheckService (19-290)
packages/trpc/server/routers/viewer/teams/createInvite.handler.ts (1)
packages/features/pbac/services/permission-check.service.ts (1)
  • PermissionCheckService (19-290)
packages/trpc/server/routers/viewer/teams/inviteMember/inviteMemberUtils.test.ts (2)
packages/features/pbac/services/permission-check.service.ts (1)
  • PermissionCheckService (19-290)
packages/trpc/server/routers/viewer/teams/inviteMember/utils.ts (1)
  • ensureAtleastAdminPermissions (59-83)
packages/trpc/server/routers/viewer/teams/update.handler.ts (2)
packages/features/pbac/services/permission-check.service.ts (1)
  • PermissionCheckService (19-290)
packages/platform/libraries/index.ts (1)
  • MembershipRole (34-34)
packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (2)
packages/features/pbac/services/permission-check.service.ts (1)
  • PermissionCheckService (19-290)
packages/platform/libraries/index.ts (1)
  • MembershipRole (34-34)
packages/trpc/server/routers/viewer/teams/inviteMember/utils.ts (2)
packages/features/pbac/services/permission-check.service.ts (1)
  • PermissionCheckService (19-290)
packages/platform/libraries/index.ts (2)
  • MembershipRole (34-34)
  • TRPCError (66-66)
packages/trpc/server/routers/viewer/teams/publish.handler.ts (1)
packages/features/pbac/services/permission-check.service.ts (1)
  • PermissionCheckService (19-290)
packages/trpc/server/routers/viewer/teams/deleteInvite.handler.ts (1)
packages/features/pbac/services/permission-check.service.ts (1)
  • PermissionCheckService (19-290)
packages/features/pbac/services/role-management.factory.ts (2)
packages/features/pbac/services/permission-check.service.ts (1)
  • hasPermission (183-201)
packages/lib/server/queries/organisations/index.ts (1)
  • isOrganisationAdmin (7-17)
🔇 Additional comments (22)
packages/trpc/server/routers/viewer/teams/removeHostsFromEventTypes.handler.ts (2)

1-4: PBAC imports and role enum usage look correct.


20-29: PBAC migration looks good; confirm permission string.
Using eventType.update with OWNER/ADMIN fallback aligns with the goal. Please confirm that removing hosts is governed by eventType.update (and not a more specific permission).

packages/trpc/server/routers/viewer/teams/updateInternalNotesPresets.handler.ts (3)

1-1: PBAC refactor import looks good

Using PermissionCheckService here aligns with the PR objective and is consistent with the new authorization path.


3-3: Correct use of fallback roles

Importing MembershipRole for fallback roles is appropriate.


32-33: Use FORBIDDEN for permission denials (consistency/semantics)

Repo shows mixed usage of UNAUTHORIZED and FORBIDDEN; prefer FORBIDDEN for authenticated-but-not-authorized cases — change this throw to FORBIDDEN and run a sweep to align other handlers (e.g., workflows/getAllActiveWorkflows.handler.ts uses UNAUTHORIZED; workflows/util.ts and teams/create.handler.ts use FORBIDDEN).

packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts (3)

4-4: PBAC service import: good move.

Switching to PermissionCheckService aligns with the PBAC refactor.


9-9: Fallback roles import: appropriate.

Using MembershipRole for OWNER/ADMIN parity is correct.


62-64: Authorization condition: looks correct.

Combines PBAC with organizer/attendee checks as intended.

packages/features/ee/dsync/lib/server/userCanCreateTeamGroupMapping.ts (1)

1-1: Import rename to canAccessOrganization looks good

Change aligns with the refactor and keeps behavior intact.

packages/trpc/server/routers/viewer/teams/publish.handler.ts (2)

2-2: Good move to PermissionCheckService

Centralizing permission checks is consistent with PBAC rollout.


6-7: Type import additions look fine

Prisma types and MembershipRole usage are appropriate.

packages/trpc/server/routers/viewer/teams/addMembersToEventTypes.handler.ts (2)

21-27: PBAC permission check looks correct

Using eventType.update with OWNER/ADMIN fallback is consistent with the service contract.


29-31: LGTM on UNAUTHORIZED for permission failure

Consistent with other handlers.

packages/features/pbac/services/role-management.factory.ts (1)

115-121: Error semantics OK

Keep RoleManagementError.UNAUTHORIZED here to preserve legacy path behavior when PBAC is disabled.

packages/trpc/server/routers/viewer/sso/get.handler.ts (1)

4-9: Importing canAccessOrganization is correct

Matches the refactor across SSO handlers.

packages/trpc/server/routers/viewer/dsync/create.handler.ts (1)

24-31: Good switch to organization-scoped access helper.

Migration to canAccessOrganization is consistent with the PR direction.

packages/trpc/server/routers/viewer/sso/delete.handler.ts (1)

2-7: Import/usage alignment looks good.

Moving to canAccessOrganization here matches the broader refactor.

packages/trpc/server/routers/viewer/sso/update.handler.ts (1)

36-42: Confirm SAML redirect path correctness.

defaultRedirectUrl here uses “/auth/saml-idp” while other handlers use “/api/auth/saml/idp”. Please verify this endpoint is intended.

packages/trpc/server/routers/viewer/dsync/get.handler.ts (2)

21-28: LGTM on access check.

Consistent use of canAccessOrganization and clear error messaging.


38-42: organizationId in DSyncData is unique — findUnique is correct.

packages/prisma/schema.prisma (model DSyncData) defines organizationId Int? @unique, so prisma.dSyncData.findUnique({ where: { organizationId: input.organizationId } }) is valid.

packages/features/ee/sso/lib/saml.ts (1)

42-64: Using "organization.read" to gate write operations is too permissive.

This helper gates create/update/delete flows in multiple handlers. Checking only organization.read enables mutation by read-only users when PBAC is enabled.

Proposed change: allow callers to specify the required permission (defaulting to "organization.read" to preserve existing behavior). Then update write call sites to pass an update/manage permission.

-export const canAccessOrganization = async (user: { id: number; email: string }, teamId: number | null) => {
+export const canAccessOrganization = async (
+  user: { id: number; email: string },
+  teamId: number | null,
+  requiredPermission = "organization.read"
+) => {
@@
-    const permissionCheckService = new PermissionCheckService();
-    const hasPermission = await permissionCheckService.checkPermission({
-      userId,
-      teamId,
-      permission: "organization.read",
-      fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN],
-    });
+    const permissionCheckService = new PermissionCheckService();
+    const hasPermission = await permissionCheckService.checkPermission({
+      userId,
+      teamId,
+      permission: requiredPermission,
+      fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN],
+    });

Then, in mutating handlers (SSO/DSYNC create/update/delete), pass a stronger permission, e.g., "organization.update". See inline suggestions in those files.

I can update all call sites in this PR if you confirm the correct permission string (e.g., "organization.update" vs an SSO-specific permission like "sso.manage").

packages/trpc/server/routers/viewer/teams/inviteMember/inviteMemberUtils.test.ts (1)

34-38: Mock looks good.

PermissionCheckService is properly mocked and injected; assertions target the right payload.

Comment on lines 55 to 71
async function checkPermissions({ ctx, input }: PublishOptions) {
const { profile } = ctx.user;
if (profile?.organizationId && !isOrganisationAdmin(ctx.user.id, profile.organizationId))
throw new TRPCError({ code: "UNAUTHORIZED" });
if (!profile?.organizationId && !(await isTeamAdmin(ctx.user.id, input.teamId)))
throw new TRPCError({ code: "UNAUTHORIZED" });
const permissionCheckService = new PermissionCheckService();

const isOrg = !!profile?.organizationId;
const permission = isOrg ? "organization.update" : "team.update";
const teamId = isOrg ? profile.organizationId : input.teamId;

const hasUpdatePermission = await permissionCheckService.checkPermission({
userId: ctx.user.id,
teamId,
permission,
fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN],
});

if (!hasUpdatePermission) throw new TRPCError({ code: "UNAUTHORIZED" });
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Bug: Permission context derived from user profile, not target team

Using profile.organizationId to decide permission scope can authorize/deny incorrectly when publishing a child team. Derive scope from the target teamId instead.

Apply this diff to resolve context from the team being published:

 async function checkPermissions({ ctx, input }: PublishOptions) {
-  const { profile } = ctx.user;
-  const permissionCheckService = new PermissionCheckService();
-
-  const isOrg = !!profile?.organizationId;
-  const permission = isOrg ? "organization.update" : "team.update";
-  const teamId = isOrg ? profile.organizationId : input.teamId;
-
-  const hasUpdatePermission = await permissionCheckService.checkPermission({
-    userId: ctx.user.id,
-    teamId,
-    permission,
-    fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN],
-  });
-
-  if (!hasUpdatePermission) throw new TRPCError({ code: "UNAUTHORIZED" });
+  const permissionCheckService = new PermissionCheckService();
+  const team = await prisma.team.findUnique({
+    where: { id: input.teamId },
+    select: { id: true, parentId: true },
+  });
+  if (!team) throw new TRPCError({ code: "NOT_FOUND", message: "Team not found" });
+  const isOrg = team.parentId === null;
+  const permission = isOrg ? "organization.update" : "team.update";
+
+  const hasUpdatePermission = await permissionCheckService.checkPermission({
+    userId: ctx.user.id,
+    teamId: team.id,
+    permission,
+    fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN],
+  });
+
+  if (!hasUpdatePermission) throw new TRPCError({ code: "UNAUTHORIZED" });
 }

Add the missing prisma import at the top of the file:

+import prisma from "@calcom/prisma";
📝 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.

Suggested change
async function checkPermissions({ ctx, input }: PublishOptions) {
const { profile } = ctx.user;
if (profile?.organizationId && !isOrganisationAdmin(ctx.user.id, profile.organizationId))
throw new TRPCError({ code: "UNAUTHORIZED" });
if (!profile?.organizationId && !(await isTeamAdmin(ctx.user.id, input.teamId)))
throw new TRPCError({ code: "UNAUTHORIZED" });
const permissionCheckService = new PermissionCheckService();
const isOrg = !!profile?.organizationId;
const permission = isOrg ? "organization.update" : "team.update";
const teamId = isOrg ? profile.organizationId : input.teamId;
const hasUpdatePermission = await permissionCheckService.checkPermission({
userId: ctx.user.id,
teamId,
permission,
fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN],
});
if (!hasUpdatePermission) throw new TRPCError({ code: "UNAUTHORIZED" });
}
import prisma from "@calcom/prisma";
async function checkPermissions({ ctx, input }: PublishOptions) {
const permissionCheckService = new PermissionCheckService();
const team = await prisma.team.findUnique({
where: { id: input.teamId },
select: { id: true, parentId: true },
});
if (!team) throw new TRPCError({ code: "NOT_FOUND", message: "Team not found" });
const isOrg = team.parentId === null;
const permission = isOrg ? "organization.update" : "team.update";
const hasUpdatePermission = await permissionCheckService.checkPermission({
userId: ctx.user.id,
teamId: team.id,
permission,
fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN],
});
if (!hasUpdatePermission) throw new TRPCError({ code: "UNAUTHORIZED" });
}

@hbjORbj hbjORbj marked this pull request as draft September 24, 2025 12:59
@hbjORbj hbjORbj marked this pull request as ready for review September 25, 2025 01:57
@hbjORbj hbjORbj enabled auto-merge (squash) September 25, 2025 01:57
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/teams/deleteInvite.handler.ts (1)

51-51: Drop the default export to stay with named-exports-only guidance.

We generally stick to named exports across the codebase; please remove the new default export (and adjust any consumers if needed).

-export default deleteInviteHandler;
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 91be568 and d8dfa92.

📒 Files selected for processing (2)
  • packages/trpc/server/routers/viewer/teams/deleteInvite.handler.ts (2 hunks)
  • packages/trpc/server/routers/viewer/teams/setInviteExpiration.handler.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/trpc/server/routers/viewer/teams/setInviteExpiration.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 use include, always use select
Ensure the credential.key field is never returned from tRPC endpoints or APIs

Files:

  • packages/trpc/server/routers/viewer/teams/deleteInvite.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/deleteInvite.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/teams/deleteInvite.handler.ts
🧬 Code graph analysis (1)
packages/trpc/server/routers/viewer/teams/deleteInvite.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). (8)
  • GitHub Check: Production builds / Build Atoms
  • GitHub Check: Production builds / Build Web App
  • GitHub Check: Production builds / Build API v2
  • GitHub Check: Tests / Unit
  • GitHub Check: Linters / lint
  • GitHub Check: Type check / check-types
  • GitHub Check: Production builds / Build API v1
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
packages/trpc/server/routers/viewer/teams/deleteInvite.handler.ts (1)

36-44: Nice alignment with createInvite permissions.

Switching to PermissionCheckService with the org/team split mirrors the create-flow logic and keeps the PBAC + fallback story consistent. 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alishaz-polymath @joeauyeung do we allow SAML setup outside of orgs? Im not 100% sure

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, either self-hosting or Orgs only

Copy link
Member

@sean-brydon sean-brydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything LGTM but i want a answer on this https://github.com/calcom/cal.com/pull/24026/files#r2378093333 as im not sure if this is just dedicated to orgs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core area: core, team members only foundation ready-for-e2e 💻 refactor size/L teams area: teams, round robin, collective, managed event-types

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants