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

* refactor(teams): improve membership validation for org admins - Add organization scope validation to removeMember handler - Ensure team operations are scoped to users organization context - Update related tests * test(teams): align removeMember tests with updated interface - Add organizationId to test contexts - Add team.findMany mock for org validation - Add test cases for org scope validation * refactor(teams): use TeamRepository for org validation in LegacyRemoveMemberService - Add findByIdsAndOrgId method to TeamRepository - Inject TeamRepository into LegacyRemoveMemberService via constructor - Update RemoveMemberServiceFactory to instantiate and inject TeamRepository - Update tests to mock TeamRepository instead of direct prisma calls Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: keith@cal.com <keithwillcode@gmail.com> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
2026-01-19 11:26:05 -03:00
parent 62216f2db6
commit ea66a9093c
7 changed files with 148 additions and 22 deletions
@@ -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 },
@@ -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<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];
@@ -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,
@@ -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;
@@ -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<boolean> {
+    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<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);
@@ -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;
  }
@@ -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<typeof vi.fn> };

  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,
@@ -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");