Skip to content

fix(teams): add organization scope to membership operations #26964

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
pedroccastro wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

fix(teams): add organization scope to membership operations #26964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3fb62b5
refactor(teams): improve membership validation for org admins
pedroccastro Jan 17, 2026
e4f7e90
test(teams): align removeMember tests with updated interface
pedroccastro Jan 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions packages/trpc/server/routers/viewer/teams/removeMember.handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ type RemoveMemberOptions = {
ctx: {
user: {
id: number;
organizationId: number | null;
organization?: {
id: number | null;
isOrgAdmin: boolean;
};
};
Expand All @@ -19,16 +21,17 @@ type RemoveMemberOptions = {

export const removeMemberHandler = async ({
ctx: {
user: { id: userId, organization },
user: { id: userId, organizationId, organization },
},
input,
}: RemoveMemberOptions) => {
}: RemoveMemberOptions): Promise<void> => {
await checkRateLimitAndThrowError({
identifier: `removeMember.${userId}`,
});

const { memberIds, teamIds, isOrg } = input;
const isOrgAdmin = organization?.isOrgAdmin ?? false;
const userOrgId = organizationId ?? organization?.id ?? null;

// Note: This assumes that all teams in the request have the same PBAC setting 9999% chance they do.
const primaryTeamId = teamIds[0];
Expand All @@ -45,6 +48,7 @@ export const removeMemberHandler = async ({
const { hasPermission } = await service.checkRemovePermissions({
userId,
isOrgAdmin,
organizationId: userOrgId,
memberIds,
teamIds,
isOrg,
Expand All @@ -58,6 +62,7 @@ export const removeMemberHandler = async ({
{
userId,
isOrgAdmin,
organizationId: userOrgId,
memberIds,
teamIds,
isOrg,
Expand Down
1 change: 1 addition & 0 deletions packages/trpc/server/routers/viewer/teams/removeMember/IRemoveMemberService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type { MembershipRole } from "@calcom/prisma/enums";
export interface RemoveMemberContext {
userId: number;
isOrgAdmin: boolean;
organizationId: number | null;
memberIds: number[];
teamIds: number[];
isOrg: boolean;
Expand Down
29 changes: 26 additions & 3 deletions packages/trpc/server/routers/viewer/teams/removeMember/LegacyRemoveMemberService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,35 @@ import { BaseRemoveMemberService } from "./BaseRemoveMemberService";
import type { RemoveMemberContext, RemoveMemberPermissionResult } from "./IRemoveMemberService";

export class LegacyRemoveMemberService extends BaseRemoveMemberService {
private async validateTeamsBelongToOrganization(
teamIds: number[],
organizationId: number
): Promise<boolean> {
const teams = await prisma.team.findMany({
where: {
id: { in: teamIds },
OR: [{ id: organizationId }, { parentId: organizationId }],
},
select: { id: true },
});

const validTeamIds = new Set(teams.map((t) => t.id));
return teamIds.every((id) => validTeamIds.has(id));
}

async checkRemovePermissions(context: RemoveMemberContext): Promise<RemoveMemberPermissionResult> {
const { userId, isOrgAdmin, teamIds } = context;
const { userId, isOrgAdmin, organizationId, teamIds } = context;

// Org admins have full permission
if (isOrgAdmin) {
// Return admin role for all teams
if (!organizationId) {
return { hasPermission: false };
}

const teamsValid = await this.validateTeamsBelongToOrganization(teamIds, organizationId);
if (!teamsValid) {
return { hasPermission: false };
}

const userRoles = new Map<number, MembershipRole | null>();
teamIds.forEach((teamId) => {
userRoles.set(teamId, MembershipRole.ADMIN);
Expand Down
94 changes: 92 additions & 2 deletions ...trpc/server/routers/viewer/teams/removeMember/__tests__/LegacyRemoveMemberService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ vi.mock("@calcom/prisma", () => ({
membership: {
findMany: vi.fn(),
},
team: {
findMany: vi.fn(),
},
},
}));

Expand All @@ -31,31 +34,46 @@ describe("LegacyRemoveMemberService", () => {
it("should allow org admin to remove members from teams they are NOT part of", async () => {
const userId = 1;
const isOrgAdmin = true;
const organizationId = 10;
const teamIds = [100, 200]; // Teams the org admin is not part of
const memberIds = [2, 3];

// Mock teams belong to the organization
vi.mocked(prisma.team.findMany).mockResolvedValue(
teamIds.map((id) => ({ id })) as { id: number }[]
);

const result = await service.checkRemovePermissions({
userId,
isOrgAdmin,
organizationId,
memberIds,
teamIds,
isOrg: true,
});

expect(result.hasPermission).toBe(true);
// Should not query database for org admin
// Should query team.findMany for org validation, but not membership.findMany
expect(prisma.team.findMany).toHaveBeenCalled();
expect(prisma.membership.findMany).not.toHaveBeenCalled();
});

it("should bypass membership checks for org admins", async () => {
const userId = 1;
const isOrgAdmin = true;
const organizationId = 10;
const teamIds = [1, 2, 3];
const memberIds = [4, 5, 6];

// Mock teams belong to the organization
vi.mocked(prisma.team.findMany).mockResolvedValue(
teamIds.map((id) => ({ id })) as { id: number }[]
);

const result = await service.checkRemovePermissions({
userId,
isOrgAdmin,
organizationId,
memberIds,
teamIds,
isOrg: true,
Expand All @@ -69,19 +87,26 @@ describe("LegacyRemoveMemberService", () => {
expect(result.userRoles?.get(teamId)).toBe(MembershipRole.ADMIN);
});

// Should not query database
// Should not query membership database for org admin
expect(prisma.membership.findMany).not.toHaveBeenCalled();
});

it("should allow org admin to remove from multiple teams at once", async () => {
const userId = 1;
const isOrgAdmin = true;
const organizationId = 10;
const teamIds = [1, 2, 3, 4, 5];
const memberIds = [10, 20];

// Mock teams belong to the organization
vi.mocked(prisma.team.findMany).mockResolvedValue(
teamIds.map((id) => ({ id })) as { id: number }[]
);

const result = await service.checkRemovePermissions({
userId,
isOrgAdmin,
organizationId,
memberIds,
teamIds,
isOrg: true,
Expand All @@ -94,12 +119,19 @@ describe("LegacyRemoveMemberService", () => {
it("should work for org admin even with isOrg=false", async () => {
const userId = 1;
const isOrgAdmin = true;
const organizationId = 10;
const teamIds = [1];
const memberIds = [2];

// Mock teams belong to the organization
vi.mocked(prisma.team.findMany).mockResolvedValue(
teamIds.map((id) => ({ id })) as { id: number }[]
);

const result = await service.checkRemovePermissions({
userId,
isOrgAdmin,
organizationId,
memberIds,
teamIds,
isOrg: false, // Note: isOrg is false
Expand All @@ -108,6 +140,49 @@ describe("LegacyRemoveMemberService", () => {
expect(result.hasPermission).toBe(true);
expect(prisma.membership.findMany).not.toHaveBeenCalled();
});

it("should deny org admin when teams do not belong to their organization", async () => {
const userId = 1;
const isOrgAdmin = true;
const organizationId = 10;
const teamIds = [100, 200]; // Teams from another organization
const memberIds = [2, 3];

// Mock: no teams returned (none belong to the organization)
vi.mocked(prisma.team.findMany).mockResolvedValue([]);

const result = await service.checkRemovePermissions({
userId,
isOrgAdmin,
organizationId,
memberIds,
teamIds,
isOrg: true,
});

expect(result.hasPermission).toBe(false);
});

it("should deny org admin when organizationId is null", async () => {
const userId = 1;
const isOrgAdmin = true;
const organizationId = null;
const teamIds = [1];
const memberIds = [2];

const result = await service.checkRemovePermissions({
userId,
isOrgAdmin,
organizationId,
memberIds,
teamIds,
isOrg: true,
});

expect(result.hasPermission).toBe(false);
// Should not query database when organizationId is null
expect(prisma.team.findMany).not.toHaveBeenCalled();
});
});

describe("Regular User Scenarios", () => {
Expand All @@ -124,6 +199,7 @@ describe("LegacyRemoveMemberService", () => {
const result = await service.checkRemovePermissions({
userId,
isOrgAdmin: false,
organizationId: null,
memberIds,
teamIds,
isOrg: false,
Expand All @@ -146,6 +222,7 @@ describe("LegacyRemoveMemberService", () => {
const result = await service.checkRemovePermissions({
userId,
isOrgAdmin: false,
organizationId: null,
memberIds,
teamIds,
isOrg: false,
Expand All @@ -167,6 +244,7 @@ describe("LegacyRemoveMemberService", () => {
const result = await service.checkRemovePermissions({
userId,
isOrgAdmin: false,
organizationId: null,
memberIds,
teamIds,
isOrg: false,
Expand All @@ -185,6 +263,7 @@ describe("LegacyRemoveMemberService", () => {
const result = await service.checkRemovePermissions({
userId,
isOrgAdmin: false,
organizationId: null,
memberIds,
teamIds,
isOrg: false,
Expand All @@ -207,6 +286,7 @@ describe("LegacyRemoveMemberService", () => {
const result = await service.checkRemovePermissions({
userId,
isOrgAdmin: false,
organizationId: null,
memberIds,
teamIds,
isOrg: false,
Expand All @@ -229,6 +309,7 @@ describe("LegacyRemoveMemberService", () => {
const result = await service.checkRemovePermissions({
userId,
isOrgAdmin: false,
organizationId: null,
memberIds,
teamIds,
isOrg: false,
Expand Down Expand Up @@ -257,6 +338,7 @@ describe("LegacyRemoveMemberService", () => {
{
userId,
isOrgAdmin: false,
organizationId: null,
memberIds,
teamIds,
isOrg: false,
Expand Down Expand Up @@ -284,6 +366,7 @@ describe("LegacyRemoveMemberService", () => {
{
userId,
isOrgAdmin: false,
organizationId: null,
memberIds,
teamIds,
isOrg: false,
Expand All @@ -307,6 +390,7 @@ describe("LegacyRemoveMemberService", () => {
{
userId,
isOrgAdmin: true, // Org admin
organizationId: 10,
memberIds,
teamIds,
isOrg: true,
Expand Down Expand Up @@ -334,6 +418,7 @@ describe("LegacyRemoveMemberService", () => {
{
userId,
isOrgAdmin: false,
organizationId: null,
memberIds,
teamIds,
isOrg: false,
Expand Down Expand Up @@ -361,6 +446,7 @@ describe("LegacyRemoveMemberService", () => {
{
userId,
isOrgAdmin: false,
organizationId: null,
memberIds,
teamIds,
isOrg: false,
Expand All @@ -383,6 +469,7 @@ describe("LegacyRemoveMemberService", () => {
{
userId,
isOrgAdmin: false,
organizationId: null,
memberIds,
teamIds,
isOrg: false,
Expand Down Expand Up @@ -413,6 +500,7 @@ describe("LegacyRemoveMemberService", () => {
{
userId,
isOrgAdmin: false,
organizationId: null,
memberIds,
teamIds,
isOrg: false,
Expand Down Expand Up @@ -469,6 +557,7 @@ describe("LegacyRemoveMemberService", () => {
const result = await service.checkRemovePermissions({
userId,
isOrgAdmin: false,
organizationId: null,
memberIds,
teamIds,
isOrg: false,
Expand All @@ -488,6 +577,7 @@ describe("LegacyRemoveMemberService", () => {
const result = await service.checkRemovePermissions({
userId,
isOrgAdmin: false,
organizationId: null,
memberIds,
teamIds,
isOrg: false,
Expand Down
Loading