diff --git a/packages/features/ee/teams/repositories/TeamRepository.ts b/packages/features/ee/teams/repositories/TeamRepository.ts index 08a9a0888a..b33e05fdab 100644 --- a/packages/features/ee/teams/repositories/TeamRepository.ts +++ b/packages/features/ee/teams/repositories/TeamRepository.ts @@ -614,6 +614,16 @@ export class TeamRepository { }); } + async findByIdsAndOrgId({ teamIds, orgId }: { teamIds: number[]; orgId: number }) { + return await this.prismaClient.team.findMany({ + where: { + id: { in: teamIds }, + OR: [{ id: orgId }, { parentId: orgId }], + }, + select: { id: true }, + }); + } + async findTeamBySlugWithAdminRole(teamSlug: string, userId: number) { return this.prismaClient.team.findFirst({ select: { id: true }, diff --git a/packages/trpc/server/routers/viewer/teams/removeMember.handler.ts b/packages/trpc/server/routers/viewer/teams/removeMember.handler.ts index 41556a153f..085fee76b0 100644 --- a/packages/trpc/server/routers/viewer/teams/removeMember.handler.ts +++ b/packages/trpc/server/routers/viewer/teams/removeMember.handler.ts @@ -9,7 +9,9 @@ type RemoveMemberOptions = { ctx: { user: { id: number; + organizationId: number | null; organization?: { + id: number | null; isOrgAdmin: boolean; }; }; @@ -19,16 +21,17 @@ type RemoveMemberOptions = { export const removeMemberHandler = async ({ ctx: { - user: { id: userId, organization }, + user: { id: userId, organizationId, organization }, }, input, -}: RemoveMemberOptions) => { +}: RemoveMemberOptions): Promise => { 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]; @@ -45,6 +48,7 @@ export const removeMemberHandler = async ({ const { hasPermission } = await service.checkRemovePermissions({ userId, isOrgAdmin, + organizationId: userOrgId, memberIds, teamIds, isOrg, @@ -58,6 +62,7 @@ export const removeMemberHandler = async ({ { userId, isOrgAdmin, + organizationId: userOrgId, memberIds, teamIds, isOrg, diff --git a/packages/trpc/server/routers/viewer/teams/removeMember/IRemoveMemberService.ts b/packages/trpc/server/routers/viewer/teams/removeMember/IRemoveMemberService.ts index ca1305b485..5ce59a5287 100644 --- a/packages/trpc/server/routers/viewer/teams/removeMember/IRemoveMemberService.ts +++ b/packages/trpc/server/routers/viewer/teams/removeMember/IRemoveMemberService.ts @@ -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; diff --git a/packages/trpc/server/routers/viewer/teams/removeMember/LegacyRemoveMemberService.ts b/packages/trpc/server/routers/viewer/teams/removeMember/LegacyRemoveMemberService.ts index 4b5266d1a6..5df4d31379 100644 --- a/packages/trpc/server/routers/viewer/teams/removeMember/LegacyRemoveMemberService.ts +++ b/packages/trpc/server/routers/viewer/teams/removeMember/LegacyRemoveMemberService.ts @@ -1,4 +1,5 @@ import * as teamQueries from "@calcom/features/ee/teams/lib/queries"; +import type { TeamRepository } from "@calcom/features/ee/teams/repositories/TeamRepository"; import { prisma } from "@calcom/prisma"; import { MembershipRole } from "@calcom/prisma/enums"; @@ -8,12 +9,32 @@ import { BaseRemoveMemberService } from "./BaseRemoveMemberService"; import type { RemoveMemberContext, RemoveMemberPermissionResult } from "./IRemoveMemberService"; export class LegacyRemoveMemberService extends BaseRemoveMemberService { + constructor(private teamRepository: TeamRepository) { + super(); + } + + private async validateTeamsBelongToOrganization( + teamIds: number[], + organizationId: number + ): Promise { + const teams = await this.teamRepository.findByIdsAndOrgId({ teamIds, orgId: organizationId }); + const validTeamIds = new Set(teams.map((t) => t.id)); + return teamIds.every((id) => validTeamIds.has(id)); + } + async checkRemovePermissions(context: RemoveMemberContext): Promise { - 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(); teamIds.forEach((teamId) => { userRoles.set(teamId, MembershipRole.ADMIN); diff --git a/packages/trpc/server/routers/viewer/teams/removeMember/RemoveMemberServiceFactory.ts b/packages/trpc/server/routers/viewer/teams/removeMember/RemoveMemberServiceFactory.ts index 50ffd2a94d..bcd0aedb4f 100644 --- a/packages/trpc/server/routers/viewer/teams/removeMember/RemoveMemberServiceFactory.ts +++ b/packages/trpc/server/routers/viewer/teams/removeMember/RemoveMemberServiceFactory.ts @@ -1,4 +1,5 @@ import { FeaturesRepository } from "@calcom/features/flags/features.repository"; +import { TeamRepository } from "@calcom/features/ee/teams/repositories/TeamRepository"; import { prisma } from "@calcom/prisma"; import type { IRemoveMemberService } from "./IRemoveMemberService"; @@ -14,7 +15,8 @@ export class RemoveMemberServiceFactory { const featuresRepository = new FeaturesRepository(prisma); const isPBACEnabled = await featuresRepository.checkIfTeamHasFeature(teamId, "pbac"); - const service = isPBACEnabled ? new PBACRemoveMemberService() : new LegacyRemoveMemberService(); + const teamRepository = new TeamRepository(prisma); + const service = isPBACEnabled ? new PBACRemoveMemberService() : new LegacyRemoveMemberService(teamRepository); return service; } diff --git a/packages/trpc/server/routers/viewer/teams/removeMember/__tests__/LegacyRemoveMemberService.test.ts b/packages/trpc/server/routers/viewer/teams/removeMember/__tests__/LegacyRemoveMemberService.test.ts index d35d6d74ce..00b5be5cf2 100644 --- a/packages/trpc/server/routers/viewer/teams/removeMember/__tests__/LegacyRemoveMemberService.test.ts +++ b/packages/trpc/server/routers/viewer/teams/removeMember/__tests__/LegacyRemoveMemberService.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi, beforeEach } from "vitest"; import * as teamQueries from "@calcom/features/ee/teams/lib/queries"; +import type { TeamRepository } from "@calcom/features/ee/teams/repositories/TeamRepository"; import { TeamService } from "@calcom/features/ee/teams/services/teamService"; import { prisma } from "@calcom/prisma"; import { MembershipRole } from "@calcom/prisma/enums"; @@ -20,10 +21,14 @@ vi.mock("@calcom/features/ee/teams/lib/queries"); describe("LegacyRemoveMemberService", () => { let service: LegacyRemoveMemberService; + let mockTeamRepository: { findByIdsAndOrgId: ReturnType }; beforeEach(() => { vi.clearAllMocks(); - service = new LegacyRemoveMemberService(); + mockTeamRepository = { + findByIdsAndOrgId: vi.fn(), + }; + service = new LegacyRemoveMemberService(mockTeamRepository as unknown as TeamRepository); }); describe("checkRemovePermissions", () => { @@ -31,31 +36,39 @@ 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]; + mockTeamRepository.findByIdsAndOrgId.mockResolvedValue(teamIds.map((id) => ({ id }))); + const result = await service.checkRemovePermissions({ userId, isOrgAdmin, + organizationId, memberIds, teamIds, isOrg: true, }); expect(result.hasPermission).toBe(true); - // Should not query database for org admin + expect(mockTeamRepository.findByIdsAndOrgId).toHaveBeenCalledWith({ teamIds, orgId: organizationId }); 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]; + mockTeamRepository.findByIdsAndOrgId.mockResolvedValue(teamIds.map((id) => ({ id }))); + const result = await service.checkRemovePermissions({ userId, isOrgAdmin, + organizationId, memberIds, teamIds, isOrg: true, @@ -69,19 +82,23 @@ 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]; + mockTeamRepository.findByIdsAndOrgId.mockResolvedValue(teamIds.map((id) => ({ id }))); + const result = await service.checkRemovePermissions({ userId, isOrgAdmin, + organizationId, memberIds, teamIds, isOrg: true, @@ -94,12 +111,16 @@ 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]; + mockTeamRepository.findByIdsAndOrgId.mockResolvedValue(teamIds.map((id) => ({ id }))); + const result = await service.checkRemovePermissions({ userId, isOrgAdmin, + organizationId, memberIds, teamIds, isOrg: false, // Note: isOrg is false @@ -108,6 +129,47 @@ 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]; + + mockTeamRepository.findByIdsAndOrgId.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); + expect(mockTeamRepository.findByIdsAndOrgId).not.toHaveBeenCalled(); + }); }); describe("Regular User Scenarios", () => { @@ -124,6 +186,7 @@ describe("LegacyRemoveMemberService", () => { const result = await service.checkRemovePermissions({ userId, isOrgAdmin: false, + organizationId: null, memberIds, teamIds, isOrg: false, @@ -146,6 +209,7 @@ describe("LegacyRemoveMemberService", () => { const result = await service.checkRemovePermissions({ userId, isOrgAdmin: false, + organizationId: null, memberIds, teamIds, isOrg: false, @@ -167,6 +231,7 @@ describe("LegacyRemoveMemberService", () => { const result = await service.checkRemovePermissions({ userId, isOrgAdmin: false, + organizationId: null, memberIds, teamIds, isOrg: false, @@ -185,6 +250,7 @@ describe("LegacyRemoveMemberService", () => { const result = await service.checkRemovePermissions({ userId, isOrgAdmin: false, + organizationId: null, memberIds, teamIds, isOrg: false, @@ -207,6 +273,7 @@ describe("LegacyRemoveMemberService", () => { const result = await service.checkRemovePermissions({ userId, isOrgAdmin: false, + organizationId: null, memberIds, teamIds, isOrg: false, @@ -229,6 +296,7 @@ describe("LegacyRemoveMemberService", () => { const result = await service.checkRemovePermissions({ userId, isOrgAdmin: false, + organizationId: null, memberIds, teamIds, isOrg: false, @@ -257,6 +325,7 @@ describe("LegacyRemoveMemberService", () => { { userId, isOrgAdmin: false, + organizationId: null, memberIds, teamIds, isOrg: false, @@ -284,6 +353,7 @@ describe("LegacyRemoveMemberService", () => { { userId, isOrgAdmin: false, + organizationId: null, memberIds, teamIds, isOrg: false, @@ -307,6 +377,7 @@ describe("LegacyRemoveMemberService", () => { { userId, isOrgAdmin: true, // Org admin + organizationId: 10, memberIds, teamIds, isOrg: true, @@ -334,6 +405,7 @@ describe("LegacyRemoveMemberService", () => { { userId, isOrgAdmin: false, + organizationId: null, memberIds, teamIds, isOrg: false, @@ -361,6 +433,7 @@ describe("LegacyRemoveMemberService", () => { { userId, isOrgAdmin: false, + organizationId: null, memberIds, teamIds, isOrg: false, @@ -383,6 +456,7 @@ describe("LegacyRemoveMemberService", () => { { userId, isOrgAdmin: false, + organizationId: null, memberIds, teamIds, isOrg: false, @@ -413,6 +487,7 @@ describe("LegacyRemoveMemberService", () => { { userId, isOrgAdmin: false, + organizationId: null, memberIds, teamIds, isOrg: false, @@ -469,6 +544,7 @@ describe("LegacyRemoveMemberService", () => { const result = await service.checkRemovePermissions({ userId, isOrgAdmin: false, + organizationId: null, memberIds, teamIds, isOrg: false, @@ -488,6 +564,7 @@ describe("LegacyRemoveMemberService", () => { const result = await service.checkRemovePermissions({ userId, isOrgAdmin: false, + organizationId: null, memberIds, teamIds, isOrg: false, diff --git a/packages/trpc/server/routers/viewer/teams/removeMember/__tests__/removeMember.handler.test.ts b/packages/trpc/server/routers/viewer/teams/removeMember/__tests__/removeMember.handler.test.ts index c8a3574391..d5f92ce667 100644 --- a/packages/trpc/server/routers/viewer/teams/removeMember/__tests__/removeMember.handler.test.ts +++ b/packages/trpc/server/routers/viewer/teams/removeMember/__tests__/removeMember.handler.test.ts @@ -24,7 +24,7 @@ describe("removeMemberHandler", () => { success: true, remaining: 99, limit: 100, - reset: new Date().getTime() + 60 * 1000, + reset: Date.now() + 60 * 1000, }); vi.mocked(RemoveMemberServiceFactory.create).mockResolvedValue(mockService); vi.mocked(mockService.checkRemovePermissions).mockResolvedValue({ @@ -43,7 +43,7 @@ describe("removeMemberHandler", () => { }; await removeMemberHandler({ - ctx: { user: { id: userId } }, + ctx: { user: { id: userId, organizationId: null } }, input, }); @@ -63,7 +63,7 @@ describe("removeMemberHandler", () => { await expect( removeMemberHandler({ - ctx: { user: { id: 1 } }, + ctx: { user: { id: 1, organizationId: null } }, input, }) ).rejects.toThrow( @@ -87,7 +87,7 @@ describe("removeMemberHandler", () => { mockService.checkRemovePermissions = vi.fn().mockResolvedValue({ hasPermission: true }); await removeMemberHandler({ - ctx: { user: { id: 1 } }, + ctx: { user: { id: 1, organizationId: null } }, input, }); @@ -99,6 +99,7 @@ describe("removeMemberHandler", () => { it("should pass org admin status to permission check", async () => { const userId = 1; const isOrgAdmin = true; + const organizationId = 10; const input = { teamIds: [1], memberIds: [2], @@ -111,7 +112,8 @@ describe("removeMemberHandler", () => { ctx: { user: { id: userId, - organization: { isOrgAdmin }, + organizationId, + organization: { id: organizationId, isOrgAdmin }, }, }, input, @@ -120,6 +122,7 @@ describe("removeMemberHandler", () => { expect(mockService.checkRemovePermissions).toHaveBeenCalledWith({ userId, isOrgAdmin, + organizationId, memberIds: input.memberIds, teamIds: input.teamIds, isOrg: input.isOrg, @@ -137,13 +140,14 @@ describe("removeMemberHandler", () => { mockService.checkRemovePermissions = vi.fn().mockResolvedValue({ hasPermission: true }); await removeMemberHandler({ - ctx: { user: { id: userId } }, + ctx: { user: { id: userId, organizationId: null } }, input, }); expect(mockService.checkRemovePermissions).toHaveBeenCalledWith( expect.objectContaining({ isOrgAdmin: false, + organizationId: null, }) ); }); @@ -159,7 +163,7 @@ describe("removeMemberHandler", () => { await expect( removeMemberHandler({ - ctx: { user: { id: 1 } }, + ctx: { user: { id: 1, organizationId: null } }, input, }) ).rejects.toThrow( @@ -177,6 +181,7 @@ describe("removeMemberHandler", () => { it("should complete full removal flow when user has permission", async () => { const userId = 1; const isOrgAdmin = false; + const organizationId = null; const input = { teamIds: [1, 2], memberIds: [3, 4], @@ -196,7 +201,7 @@ describe("removeMemberHandler", () => { mockService.removeMembers = vi.fn().mockResolvedValue(undefined); await removeMemberHandler({ - ctx: { user: { id: userId } }, + ctx: { user: { id: userId, organizationId } }, input, }); @@ -204,6 +209,7 @@ describe("removeMemberHandler", () => { expect(mockService.checkRemovePermissions).toHaveBeenCalledWith({ userId, isOrgAdmin, + organizationId, memberIds: input.memberIds, teamIds: input.teamIds, isOrg: input.isOrg, @@ -213,6 +219,7 @@ describe("removeMemberHandler", () => { { userId, isOrgAdmin, + organizationId, memberIds: input.memberIds, teamIds: input.teamIds, isOrg: input.isOrg, @@ -223,11 +230,12 @@ describe("removeMemberHandler", () => { expect(mockService.removeMembers).toHaveBeenCalledWith(input.memberIds, input.teamIds, input.isOrg); }); - it("should handle org admin removing members from teams they are not part of", async () => { + it("should handle org admin removing members from teams within their organization", async () => { const userId = 1; const isOrgAdmin = true; + const organizationId = 50; const input = { - teamIds: [100, 200], // Teams the org admin is not part of + teamIds: [100, 200], // Teams within the org admin's organization memberIds: [3, 4], isOrg: true, }; @@ -242,7 +250,8 @@ describe("removeMemberHandler", () => { ctx: { user: { id: userId, - organization: { isOrgAdmin }, + organizationId, + organization: { id: organizationId, isOrgAdmin }, }, }, input, @@ -251,6 +260,7 @@ describe("removeMemberHandler", () => { expect(mockService.checkRemovePermissions).toHaveBeenCalledWith({ userId, isOrgAdmin: true, + organizationId, memberIds: input.memberIds, teamIds: input.teamIds, isOrg: input.isOrg, @@ -275,7 +285,7 @@ describe("removeMemberHandler", () => { await expect( removeMemberHandler({ - ctx: { user: { id: 1 } }, + ctx: { user: { id: 1, organizationId: null } }, input, }) ).rejects.toThrow( @@ -299,7 +309,7 @@ describe("removeMemberHandler", () => { await expect( removeMemberHandler({ - ctx: { user: { id: 1 } }, + ctx: { user: { id: 1, organizationId: null } }, input, }) ).rejects.toThrow("Database error");