Conversation
|
Caution Review failedThe pull request is closed. WalkthroughReplaces per-team PBAC and inline permission logic in the remove-member flow with a service-oriented pipeline. The handler now destructures ctx.user into userId and organization, enforces at least one primaryTeamId, and delegates permission checking, validation, and removal to a service created by RemoveMemberServiceFactory. Adds IRemoveMemberService interfaces, a BaseRemoveMemberService with a concrete removeMembers delegation to TeamService.removeMembers, and two implementations (LegacyRemoveMemberService and PBACRemoveMemberService) plus tests and a factory that selects implementation by feature flag. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (11)
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
| // Check if user is an admin or owner of the organization | ||
| const isOrgAdmin = ctx.user.organization.isOrgAdmin; | ||
|
|
||
| if (!membership && !isOrgAdmin) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Little verbose but wanted to keep it clear what's happening here
dosubot
bot
added
teams
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/teams/removeMember.handler.ts (1)
78-122: Owner-protection logic incorrectly skips for mixed PBAC states (security).Current check runs owner rules only if PBAC is disabled for all teams. In mixed inputs (some teams PBAC ON, some OFF), owner rules are skipped for PBAC‑OFF teams, allowing unauthorized owner removals/self-removal.
Apply per-team enforcement for PBAC‑OFF teams:
// TODO(SEAN): Remove this after PBAC is rolled out. // Check if any team has PBAC enabled const featuresRepository = new FeaturesRepository(prisma); - const pbacEnabledForTeams = await Promise.all( - teamIds.map(async (teamId) => await featuresRepository.checkIfTeamHasFeature(teamId, "pbac")) - ); - const isAnyTeamPBACEnabled = pbacEnabledForTeams.some((enabled) => enabled); + const pbacStatusEntries = await Promise.all( + teamIds.map(async (teamId) => [teamId, await featuresRepository.checkIfTeamHasFeature(teamId, "pbac")] as const) + ); + const pbacEnabledByTeam = Object.fromEntries(pbacStatusEntries); + const teamsWithPBACDisabled = teamIds.filter((teamId) => !pbacEnabledByTeam[teamId]); - // Only apply traditional owner-based logic if PBAC is not enabled for any teams - if (!isAnyTeamPBACEnabled) { + // Apply traditional owner-based logic only for PBAC-disabled teams + if (teamsWithPBACDisabled.length > 0) { // Only a team owner can remove another team owner. - const isAnyMemberOwnerAndCurrentUserNotOwner = await Promise.all( - memberIds.map(async (memberId) => { - const isAnyTeamOwnerAndCurrentUserNotOwner = await Promise.all( - teamIds.map(async (teamId) => { - return (await isTeamOwner(memberId, teamId)) && !(await isTeamOwner(ctx.user.id, teamId)); - }) - ).then((results) => results.some((result) => result)); - - return isAnyTeamOwnerAndCurrentUserNotOwner; - }) - ).then((results) => results.some((result) => result)); + const isAnyMemberOwnerAndCurrentUserNotOwner = await Promise.all( + memberIds.map(async (memberId) => { + const perTeam = await Promise.all( + teamsWithPBACDisabled.map(async (teamId) => { + const targetIsOwner = await isTeamOwner(memberId, teamId); + if (!targetIsOwner) return false; + const currentUserIsOwner = await isTeamOwner(ctx.user.id, teamId); + return !currentUserIsOwner; + }) + ); + return perTeam.some(Boolean); + }) + ).then((r) => r.some(Boolean)); if (isAnyMemberOwnerAndCurrentUserNotOwner) { throw new TRPCError({ code: "UNAUTHORIZED", message: "Only a team owner can remove another team owner.", }); } // Check if user is trying to remove themselves from a team they own (prevent this) - if (isRemovingSelf && hasRemovePermission) { - // Additional check: ensure they're not an owner trying to remove themselves - const isOwnerOfAnyTeam = await Promise.all( - teamIds.map(async (teamId) => await isTeamOwner(ctx.user.id, teamId)) - ).then((results) => results.some((result) => result)); + if (isRemovingSelf) { + // Ensure owners cannot remove themselves from PBAC-disabled teams + const isOwnerOfAnyDisabledTeam = await Promise.all( + teamsWithPBACDisabled.map(async (teamId) => await isTeamOwner(ctx.user.id, teamId)) + ).then((results) => results.some(Boolean)); - if (isOwnerOfAnyTeam) { + if (isOwnerOfAnyDisabledTeam) { throw new TRPCError({ code: "FORBIDDEN", message: "You can not remove yourself from a team you own.", }); } } }
🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (1)
127-127: Prefer named export over default export.Aligns with repo guideline for clearer imports and better tree-shaking.
-export default removeMemberHandler; +export { removeMemberHandler };Please update import sites accordingly.
📜 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/teams/removeMember.handler.ts(2 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/teams/removeMember.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/removeMember.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/removeMember.handler.ts
🧬 Code graph analysis (1)
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (1)
packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)
⏰ 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). (10)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (2)
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (2)
1-1: Import looks good.
isTeamOwneris used below; path resolves consistently across EE code.
51-58: Org admin override doesn’t apply when PBAC is enabled. Verify intent.
getSpecificPermissionsignoresuserRolewhen PBAC is ON, so org admins not in the team will still be denied. If org admins should be able to remove in PBAC-enabled teams, we need a PBAC-side rule/override.Can you confirm the expected behavior for PBAC-enabled teams? If desired, I can propose either:
- a handler-level allow-list for org admins, or
- a PBAC rule change to honor org-admins for this action.
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts
Outdated
Show resolved
Hide resolved
E2E results are ready! |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (2)
44-48: Short‑circuit allow for org admins to reduce DB calls and avoid PBAC drift.You can return early for org admins and skip the PBAC call entirely. It matches the PR intent and saves a query per team.
Apply this diff:
- const isOrgAdmin = ctx.user.organization?.isOrgAdmin ?? false; - - if (!membership && !isOrgAdmin) { - return false; - } + const isOrgAdmin = ctx.user.organization?.isOrgAdmin ?? false; + // Org admins are allowed to remove members regardless of their team membership. + if (isOrgAdmin) { + return true; + } + // Non-admins must be members of the team to proceed. + if (!membership) { + return false; + }
126-126: Prefer named exports; avoid default export.Cal.com convention discourages default exports for clearer imports and better tree-shaking.
Apply this diff:
-export default removeMemberHandler; +// Prefer named export onlyNote: This is a non-functional refactor and would require updating import sites; consider a separate PR.
📜 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/teams/removeMember.handler.ts(2 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/teams/removeMember.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/removeMember.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/removeMember.handler.ts
🧠 Learnings (2)
📓 Common learnings
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).
📚 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/removeMember.handler.ts
🧬 Code graph analysis (1)
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (1)
packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)
🔇 Additional comments (3)
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (3)
1-1: Import usage looks correct.
isTeamOwneris used below; import path is fine.
44-48: Org-admin elevation + null-safe org check is correct.This unblocks org admins removing non-member users and avoids NPEs.
55-56: Verify PBAC mapping grants Organization.Remove to org admins.In PBAC-enabled teams,
userRoleis ignored bygetSpecificPermissions. EnsureResource.OrganizationincludesCustomAction.Removefor org admins; otherwise admins might still be denied despite this change.Run:
If Organization.Remove isn’t granted to org admins, either update the PBAC registry or keep the early-return for org admins (suggested above) to guarantee behavior from the org members table.
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/teams/removeMember.handler.ts (1)
69-76: Self-removal bypass ignores PBAC and allows owner self-removal when PBAC is enabled.
isRemovingSelfcurrently bypasses permissions unconditionally; the owner self-removal block only runs when PBAC is disabled. This allows owners to remove themselves on PBAC-enabled teams.Suggested fix (apply regardless of PBAC):
// After computing isRemovingSelf if (isRemovingSelf) { const isOwnerOfAnyTeam = await Promise.all( teamIds.map(async (teamId) => await isTeamOwner(ctx.user.id, teamId)) ).then((results) => results.some(Boolean)); if (isOwnerOfAnyTeam) { throw new TRPCError({ code: "FORBIDDEN", message: "You can not remove yourself from a team you own.", }); } } else if (!hasRemovePermission) { throw new TRPCError({ code: "UNAUTHORIZED" }); }Then remove the later owner self-removal block (Lines 107-120) to avoid duplicate checks.
Also applies to: 107-120
🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (1)
52-57: Explicitly reflect elevation in userRole for readability (no behavior change).Passing
userRole: elevateAsOrgAdmin ? MembershipRole.ADMIN : membership.rolemakes the intent clear without relying on the synthetic membership detail.Example:
userRole: (elevateAsOrgAdmin ? MembershipRole.ADMIN : membership.role),
📜 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/teams/removeMember.handler.ts(2 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/teams/removeMember.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/removeMember.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/removeMember.handler.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: sean-brydon
PR: calcom/cal.com#23614
File: packages/lib/server/repository/webhook.ts:115-127
Timestamp: 2025-09-09T07:26:55.843Z
Learning: In Cal.com, organizations are represented as teams in the database schema, so queries like `user.teams` naturally include both regular team memberships and organization memberships. No special handling is needed for organization-level webhooks as they appear as team webhooks in the permission system.
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (2)
48-50: Membership guard is correct.Returning
falsewhen no role is available prevents accidental permission grants. Safe default.
126-126: Export removeMemberHandler as a named exportImproves tree-shaking and refactor safety.
File: packages/trpc/server/routers/viewer/teams/removeMember.handler.ts
Proposed change:
-export default removeMemberHandler; +export { removeMemberHandler };Update import sites accordingly — automated search for import locations failed (ripgrep errors), verify and update all imports that currently rely on the default export.
| const isOrgAdmin = ctx.user.organization?.isOrgAdmin ?? false; | ||
| // if isOrgAdmin, we return early with a role; disabling the db lookup. | ||
| const membership: { role: MembershipRole } | null = isOrgAdmin | ||
| ? { role: MembershipRole.ADMIN } | ||
| : await prisma.membership.findUnique({ |
There was a problem hiding this comment.
🧩 Analysis chain
Constrain org-admin elevation to org context to avoid privilege bleed.
Right now any org admin is elevated for all removals, even when isOrg is false (team context). Gate elevation by the org context at minimum to prevent unintended cross-team escalation.
Apply this diff:
- const isOrgAdmin = ctx.user.organization?.isOrgAdmin ?? false;
- // if isOrgAdmin, we return early with a role; disabling the db lookup.
- const membership: { role: MembershipRole } | null = isOrgAdmin
+ const elevateAsOrgAdmin = (ctx.user.organization?.isOrgAdmin ?? false) && isOrg;
+ // If elevating as org admin, return early with ADMIN role; skip DB lookup.
+ const membership: { role: MembershipRole } | null = elevateAsOrgAdmin
? { role: MembershipRole.ADMIN }
: await prisma.membership.findUnique({Additionally, ensure elevation only applies to teams within the admin’s organization (server-side). If not already enforced upstream, validate that each teamId belongs to ctx.user.organization. I can help wire this once we confirm the Team schema.
🏁 Script executed:
#!/bin/bash
# Discover Team schema fields to validate org ownership of a team
fd -H --strip-cwd-prefix 'schema.prisma'
rg -nC2 -g '!**/node_modules/**' -g '!**/dist/**' -P '(?s)model\s+Team\s*\{.*?\}' $(fd -H 'schema.prisma') || true
rg -n -C2 -P 'Team.*(parentId|organizationId|orgId|parentTeamId|isOrganization)' || true
# Locate org↔team relationship helpers we can reuse (e.g., ownership/parent lookups)
rg -n -C2 -P '\bisTeamOwner\b|\bisOrganization\b|\borganizationId\b|\bparentId\b' --type=ts || trueLength of output: 281647
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- show file: removeMember.handler.ts (first 300 lines) ----"
sed -n '1,300p' packages/trpc/server/routers/viewer/teams/removeMember.handler.ts || true
echo
echo "---- grep for isOrgAdmin occurrences ----"
rg -n --hidden --no-ignore -S "isOrgAdmin" || true
echo
echo "---- show lines around membership lookup (if present) ----"
rg -n --hidden --no-ignore -S "membership.findUnique|prisma.membership.findUnique|membership:" packages/trpc/server/routers/viewer/teams/removeMember.handler.ts -n -C3 || trueLength of output: 19310
Constrain org-admin elevation to org context to avoid privilege bleed.
File: packages/trpc/server/routers/viewer/teams/removeMember.handler.ts — around lines 33–37
Apply this diff:
- const isOrgAdmin = ctx.user.organization?.isOrgAdmin ?? false;
- // if isOrgAdmin, we return early with a role; disabling the db lookup.
- const membership: { role: MembershipRole } | null = isOrgAdmin
+ const elevateAsOrgAdmin = (ctx.user.organization?.isOrgAdmin ?? false) && isOrg;
+ // If elevating as org admin, return early with ADMIN role; skip DB lookup.
+ const membership: { role: MembershipRole } | null = elevateAsOrgAdmin
? { role: MembershipRole.ADMIN }
: await prisma.membership.findUnique({Also enforce server-side ownership: before trusting org-admin elevation, fetch the teams (select id, parentId, isOrganization) and require each team to belong to ctx.user.organizationId (team.isOrganization ? team.id === ctx.user.organizationId : team.parentId === ctx.user.organizationId); otherwise reject the request.
📝 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.
| const isOrgAdmin = ctx.user.organization?.isOrgAdmin ?? false; | |
| // if isOrgAdmin, we return early with a role; disabling the db lookup. | |
| const membership: { role: MembershipRole } | null = isOrgAdmin | |
| ? { role: MembershipRole.ADMIN } | |
| : await prisma.membership.findUnique({ | |
| const elevateAsOrgAdmin = (ctx.user.organization?.isOrgAdmin ?? false) && isOrg; | |
| // If elevating as org admin, return early with ADMIN role; skip DB lookup. | |
| const membership: { role: MembershipRole } | null = elevateAsOrgAdmin | |
| ? { role: MembershipRole.ADMIN } | |
| : await prisma.membership.findUnique({ |
🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/teams/removeMember.handler.ts around
lines 33 to 37, constrain the current "isOrgAdmin" shortcut so it only elevates
to ADMIN for memberships when the target team(s) are actually within the same
organization as ctx.user; to do this, remove blind early elevation and instead
fetch the relevant teams from the DB (select id, parentId, isOrganization) for
the teams being acted on, then compute ownership by checking for each team: if
team.isOrganization then require team.id === ctx.user.organizationId else
require team.parentId === ctx.user.organizationId; only allow org-admin
elevation when all teams satisfy this check, otherwise reject the request
(throw/return unauthorized) and fall back to the normal membership DB lookup for
role resolution when not allowed.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (3)
72-94: Optional: batch permission checks for perf.The per-team permission checks are sequential. For many teams, do them in parallel and then
every(Boolean). Keep short-circuiting if call volume is small.- for (let i = 0; i < entries.length; i++) { - const [teamId, userRole] = entries[i]; - if (!userRole) { - hasRemovePermissionForAll = false; - break; - } - const permissions = await getSpecificPermissions({ - userId, - teamId, - resource, - userRole, - actions: [CustomAction.Remove], - fallbackRoles: { - [CustomAction.Remove]: { - roles: [MembershipRole.ADMIN, MembershipRole.OWNER], - }, - }, - }); - if (!permissions[CustomAction.Remove]) { - hasRemovePermissionForAll = false; - break; - } - } + const permissionResults = await Promise.all( + entries.map(async ([teamId, userRole]) => { + if (!userRole) return false; + const permissions = await getSpecificPermissions({ + userId, + teamId, + resource, + userRole, + actions: [CustomAction.Remove], + fallbackRoles: { [CustomAction.Remove]: { roles: [MembershipRole.ADMIN, MembershipRole.OWNER] } }, + }); + return !!permissions[CustomAction.Remove]; + }) + ); + hasRemovePermissionForAll = permissionResults.every(Boolean);
104-111: Avoid duplicate PBAC feature checks.You already query PBAC inside
getSpecificPermissions. This second pass repeats those lookups. Consider threading PBAC flags/results to avoid repeated DB reads.
153-154: Prefer named export over default export.Consistent with repo guidance, switch to a named export to improve tree-shaking and refactors.
-export default removeMemberHandler; +export { removeMemberHandler };If this file is imported via default elsewhere, migrate those imports in a follow-up PR.
📜 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/teams/removeMember.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/teams/removeMember.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/removeMember.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/removeMember.handler.ts
🧠 Learnings (2)
📚 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/removeMember.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/teams/removeMember.handler.ts
🧬 Code graph analysis (1)
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (2)
packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)packages/features/ee/teams/lib/queries.ts (1)
isTeamOwner(411-420)
🔇 Additional comments (3)
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (3)
96-101: Policy check: self-removal bypasses PBAC.Self-removal is allowed even when PBAC denies the action. Confirm this is intended under PBAC; otherwise, gate self-removal on PBAC as well.
37-38: Constrain org-admin elevation to org-context.Avoid privilege bleed in non-org contexts.
- const isOrgAdmin = organization?.isOrgAdmin ?? false; + const isOrgAdmin = organization?.isOrgAdmin ?? false; + const elevateAsOrgAdmin = isOrg && isOrgAdmin;
37-45: Server-side guard: ensure target teams belong to the admin’s organization.Right now an org-admin could act on teams outside their org if they know IDs. Validate org ownership of each target team before granting elevated rights.
const isOrgAdmin = organization?.isOrgAdmin ?? false; const elevateAsOrgAdmin = isOrg && isOrgAdmin; + // Ensure elevated org-admin actions only target teams within org(s) the user admins + if (elevateAsOrgAdmin) { + const adminOrgTeams = await prisma.membership.findMany({ + where: { + userId, + accepted: true, + role: MembershipRole.ADMIN, + team: { isOrganization: true }, + }, + select: { teamId: true }, + }); + const orgIds = new Set(adminOrgTeams.map((m) => m.teamId)); + if (orgIds.size === 0) { + throw new TRPCError({ code: "UNAUTHORIZED" }); + } + const teams = await prisma.team.findMany({ + where: { id: { in: teamIds } }, + select: { id: true, parentId: true, isOrganization: true }, + }); + const outsideOrg = teams.some((t) => + t.isOrganization ? !orgIds.has(t.id) : !t.parentId || !orgIds.has(t.parentId) + ); + if (outsideOrg) { + throw new TRPCError({ code: "UNAUTHORIZED" }); + } + }I can adapt this check if your Team schema differs (e.g., organizationId instead of parentId/isOrganization).
| const membershipRoles = await prisma.membership.findMany({ | ||
| where: { | ||
| userId, | ||
| teamId: { | ||
| in: teamIds, | ||
| }, | ||
| ...(isOrgAdmin | ||
| ? { | ||
| role: { | ||
| not: MembershipRole.ADMIN, | ||
| }, | ||
| } | ||
| : {}), | ||
| }, | ||
| select: { | ||
| role: true, | ||
| teamId: true, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Add accepted: true and avoid downgrading org-admin via the query.
Pending invites shouldn’t confer permissions. Also, when elevating org-admins, fetching non-ADMIN memberships and then overriding the default ADMIN can downgrade them to MEMBER. Limit the query to OWNER when elevating and require accepted memberships.
- const membershipRoles = await prisma.membership.findMany({
+ const membershipRoles = await prisma.membership.findMany({
where: {
userId,
teamId: {
in: teamIds,
},
- ...(isOrgAdmin
- ? {
- role: {
- not: MembershipRole.ADMIN,
- },
- }
- : {}),
+ accepted: true,
+ ...(elevateAsOrgAdmin
+ ? { role: MembershipRole.OWNER } // only allow upgrade to OWNER; don't fetch MEMBER
+ : {}),
},
select: {
role: true,
teamId: true,
},
});📝 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.
| const membershipRoles = await prisma.membership.findMany({ | |
| where: { | |
| userId, | |
| teamId: { | |
| in: teamIds, | |
| }, | |
| ...(isOrgAdmin | |
| ? { | |
| role: { | |
| not: MembershipRole.ADMIN, | |
| }, | |
| } | |
| : {}), | |
| }, | |
| select: { | |
| role: true, | |
| teamId: true, | |
| }, | |
| }); | |
| const membershipRoles = await prisma.membership.findMany({ | |
| where: { | |
| userId, | |
| teamId: { | |
| in: teamIds, | |
| }, | |
| accepted: true, | |
| ...(elevateAsOrgAdmin | |
| ? { role: MembershipRole.OWNER } // only allow upgrade to OWNER; don't fetch MEMBER | |
| : {}), | |
| }, | |
| select: { | |
| role: true, | |
| teamId: true, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/teams/removeMember.handler.ts around
lines 39 to 57, the membership query must only return accepted memberships and
must not accidentally include non-OWNER memberships when handling org-admin
elevation: add accepted: true to the where clause so pending invites are
excluded, and change the isOrgAdmin branch so it restricts role to OWNER
(instead of excluding ADMIN) to avoid fetching non-OWNER roles that could be
later overridden and downgrade admins.
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts
Outdated
Show resolved
Hide resolved
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (2)
151-151: Prefer named exports over default exportsDefault exports hinder tree-shaking and refactors. Suggest converting to a named export in a follow-up PR to avoid broad churn.
110-146: Optional: Reduce N×M owner checksThe nested isTeamOwner calls can be O(|members|×|teams|). You can prefetch roles with a single query for both the target members and current user on pbac-disabled teams to cut DB round-trips.
📜 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/teams/removeMember.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/teams/removeMember.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/removeMember.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/removeMember.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/teams/removeMember.handler.ts
🧬 Code graph analysis (1)
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (2)
packages/features/pbac/lib/resource-permissions.ts (1)
getSpecificPermissions(105-142)packages/features/ee/teams/lib/queries.ts (1)
isTeamOwner(411-420)
⏰ 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). (3)
- GitHub Check: Tests / Unit
- GitHub Check: Production builds / Build Web App
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts (3)
148-149: LGTM on final service callRemoval wiring remains unchanged and correct given upstream checks.
57-63: Preserve elevation semantics; don’t default ADMIN unless elevation is allowedDefaulting ADMIN on any org admin (even outside org scope) is a privilege escalation. Use the scoped elevation flag.
- const userRoles = new Map<number, MembershipRole | null>( - teamIds.map((teamId) => [teamId, isOrgAdmin ? MembershipRole.ADMIN : null]) - ); + const userRoles = new Map<number, MembershipRole | null>( + teamIds.map((teamId) => [teamId, elevateAsOrgAdmin ? MembershipRole.ADMIN : null]) + ); membershipRoles.forEach((m) => { userRoles.set(m.teamId, m.role); });
37-55: Constrain org‑admin elevation to org context, validate team ownership, and exclude pending invites
- Elevation currently applies regardless of org context and without verifying the teams belong to the admin’s org.
- Pending (unaccepted) memberships are considered; they must be excluded.
- const isOrgAdmin = organization?.isOrgAdmin ?? false; + const isOrgAdmin = organization?.isOrgAdmin ?? false; + const orgId = organization?.id ?? null; + + // Validate that all target teams belong to the admin's organization + const teams = await prisma.team.findMany({ + where: { id: { in: teamIds } }, + select: { id: true, isOrganization: true, parentId: true }, + }); + const allTeamsBelongToOrg = + isOrg && + !!orgId && + teams.every((t) => (t.isOrganization ? t.id === orgId : t.parentId === orgId)); + + // Elevate only when acting in org scope and all teams belong to admin's org + const elevateAsOrgAdmin = isOrgAdmin && allTeamsBelongToOrg; + + if (isOrg && isOrgAdmin && !allTeamsBelongToOrg) { + throw new TRPCError({ code: "UNAUTHORIZED" }); + } const membershipRoles = await prisma.membership.findMany({ where: { userId, teamId: { in: teamIds, }, - ...(isOrgAdmin - ? { - role: MembershipRole.OWNER, - } - : {}), + accepted: true, + ...(elevateAsOrgAdmin ? { role: MembershipRole.OWNER } : {}), }, select: { role: true, teamId: true, }, });
packages/trpc/server/routers/viewer/teams/removeMember.handler.ts
Outdated
Show resolved
Hide resolved
emrysal
left a comment
emrysal
left a comment
There was a problem hiding this comment.
I'm still not happy with this; made a few improvements but my main concern is that I'd like to prevent double querying memberships to get permissions, right now this does a membership check, then PBAC does the same.
Taking a look at how we can improve this now @emrysal |
| const teamsWithPermission = await this.permissionService.getTeamIdsWithPermission( | ||
| userId, | ||
| removePermission | ||
| ); |
There was a problem hiding this comment.
This is now O(1) instead of O(n+1) as we are not calling checkPermissions in a loop
There was a problem hiding this comment.
Moved all of the logic out of the handler into a service factory that picks what service to run based on teamId
if we use legacy or PBAC service.
* Fallback to org admin * Prevent accidental privilege escalation as code changes in the future * When org admin, we don't actually need to do the db query * Use findMany and Map to drill down permission adjustments * Exclude .MEMBER from overriding role, we likely don't want to demote * refactor logic * Add tests for services/factories + removeHandler * fix type check --------- Co-authored-by: Alex van Andel <me@alexvanandel.com> Co-authored-by: Sean Brydon <sean@cal.com>
4 participants
What does this PR do?
Currently org admins are getting an unauthorized error when removing members from teams they are not a part of in the org members table.
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?