fix: Remove team members as org admin (#24020)
* 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>
This commit is contained in:
Joe Au-Yeung
co-authored by
Alex van Andel
Sean Brydon
Alex van Andel
Sean Brydon
parent
77c61abef8
commit
b2239374bc
@@ -1,121 +1,72 @@
|
||||
import { FeaturesRepository } from "@calcom/features/flags/features.repository";
|
||||
import { Resource, CustomAction } from "@calcom/features/pbac/domain/types/permission-registry";
|
||||
import { getSpecificPermissions } from "@calcom/features/pbac/lib/resource-permissions";
|
||||
import { checkRateLimitAndThrowError } from "@calcom/lib/checkRateLimitAndThrowError";
|
||||
import { isTeamOwner } from "@calcom/features/ee/teams/lib/queries";
|
||||
import { TeamService } from "@calcom/lib/server/service/teamService";
|
||||
import { prisma } from "@calcom/prisma";
|
||||
import { MembershipRole } from "@calcom/prisma/enums";
|
||||
import type { TrpcSessionUser } from "@calcom/trpc/server/types";
|
||||
|
||||
import { TRPCError } from "@trpc/server";
|
||||
|
||||
import type { TRemoveMemberInputSchema } from "./removeMember.schema";
|
||||
import { RemoveMemberServiceFactory } from "./removeMember/RemoveMemberServiceFactory";
|
||||
|
||||
type RemoveMemberOptions = {
|
||||
ctx: {
|
||||
user: NonNullable<TrpcSessionUser>;
|
||||
sourceIp?: string;
|
||||
user: {
|
||||
id: number;
|
||||
organization?: {
|
||||
isOrgAdmin: boolean;
|
||||
};
|
||||
};
|
||||
};
|
||||
input: TRemoveMemberInputSchema;
|
||||
};
|
||||
|
||||
export const removeMemberHandler = async ({ ctx, input }: RemoveMemberOptions) => {
|
||||
export const removeMemberHandler = async ({
|
||||
ctx: {
|
||||
user: { id: userId, organization },
|
||||
},
|
||||
input,
|
||||
}: RemoveMemberOptions) => {
|
||||
await checkRateLimitAndThrowError({
|
||||
identifier: `removeMember.${ctx.user.id}`,
|
||||
identifier: `removeMember.${userId}`,
|
||||
});
|
||||
|
||||
const { memberIds, teamIds, isOrg } = input;
|
||||
const isOrgAdmin = organization?.isOrgAdmin ?? false;
|
||||
|
||||
// Check PBAC permissions for each team
|
||||
const hasRemovePermission = await Promise.all(
|
||||
teamIds.map(async (teamId) => {
|
||||
// Get user's membership role in this team
|
||||
const membership = await prisma.membership.findFirst({
|
||||
where: {
|
||||
userId: ctx.user.id,
|
||||
teamId: teamId,
|
||||
},
|
||||
select: {
|
||||
role: true,
|
||||
},
|
||||
});
|
||||
// Note: This assumes that all teams in the request have the same PBAC setting 9999% chance they do.
|
||||
const primaryTeamId = teamIds[0];
|
||||
if (!primaryTeamId) {
|
||||
throw new TRPCError({
|
||||
code: "BAD_REQUEST",
|
||||
message: "At least one team ID must be provided",
|
||||
});
|
||||
}
|
||||
|
||||
if (!membership) return false;
|
||||
// Get the appropriate service based on feature flag
|
||||
const service = await RemoveMemberServiceFactory.create(primaryTeamId);
|
||||
|
||||
// Check PBAC permissions for removing team members
|
||||
const permissions = await getSpecificPermissions({
|
||||
userId: ctx.user.id,
|
||||
teamId: teamId,
|
||||
resource: isOrg ? Resource.Organization : Resource.Team,
|
||||
userRole: membership.role,
|
||||
actions: [CustomAction.Remove],
|
||||
fallbackRoles: {
|
||||
[CustomAction.Remove]: {
|
||||
roles: [MembershipRole.ADMIN, MembershipRole.OWNER],
|
||||
},
|
||||
},
|
||||
});
|
||||
const { hasPermission } = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg,
|
||||
});
|
||||
|
||||
return permissions[CustomAction.Remove];
|
||||
})
|
||||
).then((results) => results.every((result) => result));
|
||||
|
||||
// Check if user is trying to remove themselves (allowed for non-owners)
|
||||
const isRemovingSelf = memberIds.length === 1 && memberIds[0] === ctx.user.id;
|
||||
|
||||
// Allow if user has remove permission OR if they're removing themselves
|
||||
if (!hasRemovePermission && !isRemovingSelf) {
|
||||
if (!hasPermission) {
|
||||
throw new TRPCError({ code: "UNAUTHORIZED" });
|
||||
}
|
||||
|
||||
// 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"))
|
||||
await service.validateRemoval(
|
||||
{
|
||||
userId,
|
||||
isOrgAdmin,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg,
|
||||
},
|
||||
hasPermission
|
||||
);
|
||||
const isAnyTeamPBACEnabled = pbacEnabledForTeams.some((enabled) => enabled);
|
||||
|
||||
// Only apply traditional owner-based logic if PBAC is not enabled for any teams
|
||||
if (!isAnyTeamPBACEnabled) {
|
||||
// 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));
|
||||
|
||||
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 (isOwnerOfAnyTeam) {
|
||||
throw new TRPCError({
|
||||
code: "FORBIDDEN",
|
||||
message: "You can not remove yourself from a team you own.",
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
await TeamService.removeMembers({ teamIds, userIds: memberIds, isOrg });
|
||||
// Perform the removal
|
||||
await service.removeMembers(memberIds, teamIds, isOrg);
|
||||
};
|
||||
|
||||
export default removeMemberHandler;
|
||||
|
||||
@@ -0,0 +1,21 @@
|
||||
import { TeamService } from "@calcom/lib/server/service/teamService";
|
||||
|
||||
import type {
|
||||
IRemoveMemberService,
|
||||
RemoveMemberContext,
|
||||
RemoveMemberPermissionResult,
|
||||
} from "./IRemoveMemberService";
|
||||
|
||||
/**
|
||||
* Base abstract class for remove member services
|
||||
* Provides common functionality and defines abstract methods for specific implementations
|
||||
*/
|
||||
export abstract class BaseRemoveMemberService implements IRemoveMemberService {
|
||||
abstract checkRemovePermissions(context: RemoveMemberContext): Promise<RemoveMemberPermissionResult>;
|
||||
|
||||
abstract validateRemoval(context: RemoveMemberContext, hasPermission: boolean): Promise<void>;
|
||||
|
||||
async removeMembers(memberIds: number[], teamIds: number[], isOrg: boolean): Promise<void> {
|
||||
await TeamService.removeMembers({ teamIds, userIds: memberIds, isOrg });
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,31 @@
|
||||
import type { MembershipRole } from "@calcom/prisma/enums";
|
||||
|
||||
export interface RemoveMemberContext {
|
||||
userId: number;
|
||||
isOrgAdmin: boolean;
|
||||
memberIds: number[];
|
||||
teamIds: number[];
|
||||
isOrg: boolean;
|
||||
}
|
||||
|
||||
export interface RemoveMemberPermissionResult {
|
||||
hasPermission: boolean;
|
||||
userRoles?: Map<number, MembershipRole | null>;
|
||||
}
|
||||
|
||||
export interface IRemoveMemberService {
|
||||
/**
|
||||
* Checks if the user has permission to remove members from teams
|
||||
*/
|
||||
checkRemovePermissions(context: RemoveMemberContext): Promise<RemoveMemberPermissionResult>;
|
||||
|
||||
/**
|
||||
* Validates that the removal can proceed (e.g., owner checks)
|
||||
*/
|
||||
validateRemoval(context: RemoveMemberContext, hasPermission: boolean): Promise<void>;
|
||||
|
||||
/**
|
||||
* Performs the actual removal of members from teams
|
||||
*/
|
||||
removeMembers(memberIds: number[], teamIds: number[], isOrg: boolean): Promise<void>;
|
||||
}
|
||||
@@ -0,0 +1,108 @@
|
||||
import * as teamQueries from "@calcom/features/ee/teams/lib/queries";
|
||||
import { prisma } from "@calcom/prisma";
|
||||
import { MembershipRole } from "@calcom/prisma/enums";
|
||||
|
||||
import { TRPCError } from "@trpc/server";
|
||||
|
||||
import { BaseRemoveMemberService } from "./BaseRemoveMemberService";
|
||||
import type { RemoveMemberContext, RemoveMemberPermissionResult } from "./IRemoveMemberService";
|
||||
|
||||
export class LegacyRemoveMemberService extends BaseRemoveMemberService {
|
||||
async checkRemovePermissions(context: RemoveMemberContext): Promise<RemoveMemberPermissionResult> {
|
||||
const { userId, isOrgAdmin, teamIds } = context;
|
||||
|
||||
// Org admins have full permission
|
||||
if (isOrgAdmin) {
|
||||
// Return admin role for all teams
|
||||
const userRoles = new Map<number, MembershipRole | null>();
|
||||
teamIds.forEach((teamId) => {
|
||||
userRoles.set(teamId, MembershipRole.ADMIN);
|
||||
});
|
||||
return {
|
||||
hasPermission: true,
|
||||
userRoles,
|
||||
};
|
||||
}
|
||||
|
||||
// Get all memberships in a single query
|
||||
const membershipRoles = await prisma.membership.findMany({
|
||||
where: {
|
||||
userId,
|
||||
teamId: {
|
||||
in: teamIds,
|
||||
},
|
||||
},
|
||||
select: {
|
||||
role: true,
|
||||
teamId: true,
|
||||
},
|
||||
});
|
||||
|
||||
// Create a map for O(1) lookup
|
||||
const userRoles = new Map<number, MembershipRole | null>();
|
||||
membershipRoles.forEach((m) => {
|
||||
userRoles.set(m.teamId, m.role);
|
||||
});
|
||||
|
||||
// Check if user has admin or owner role in all teams
|
||||
const allowedRoles: MembershipRole[] = [MembershipRole.ADMIN, MembershipRole.OWNER];
|
||||
let hasPermission = true;
|
||||
|
||||
for (const teamId of teamIds) {
|
||||
const userRole = userRoles.get(teamId);
|
||||
if (!userRole || !allowedRoles.includes(userRole)) {
|
||||
hasPermission = false;
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
return {
|
||||
hasPermission,
|
||||
userRoles,
|
||||
};
|
||||
}
|
||||
|
||||
async validateRemoval(context: RemoveMemberContext, hasPermission: boolean): Promise<void> {
|
||||
const { userId, memberIds, teamIds, isOrgAdmin } = context;
|
||||
const isRemovingSelf = memberIds.length === 1 && memberIds[0] === userId;
|
||||
|
||||
// Only a team owner can remove another team owner (org admins are exempt)
|
||||
if (!isOrgAdmin) {
|
||||
const isAnyMemberOwnerAndCurrentUserNotOwner = await Promise.all(
|
||||
memberIds.map(async (memberId) => {
|
||||
const isAnyTeamOwnerAndCurrentUserNotOwner = await Promise.all(
|
||||
teamIds.map(async (teamId) => {
|
||||
return (
|
||||
(await teamQueries.isTeamOwner(memberId, teamId)) &&
|
||||
!(await teamQueries.isTeamOwner(userId, teamId))
|
||||
);
|
||||
})
|
||||
).then((results) => results.some((result) => result));
|
||||
|
||||
return isAnyTeamOwnerAndCurrentUserNotOwner;
|
||||
})
|
||||
).then((results) => results.some((result) => result));
|
||||
|
||||
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 && hasPermission) {
|
||||
const isOwnerOfAnyTeam = await Promise.all(
|
||||
teamIds.map(async (teamId) => await teamQueries.isTeamOwner(userId, teamId))
|
||||
).then((results) => results.some((result) => result));
|
||||
|
||||
if (isOwnerOfAnyTeam) {
|
||||
throw new TRPCError({
|
||||
code: "FORBIDDEN",
|
||||
message: "You can not remove yourself from a team you own.",
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,60 @@
|
||||
import * as teamQueries from "@calcom/features/ee/teams/lib/queries";
|
||||
import { PermissionMapper } from "@calcom/features/pbac/domain/mappers/PermissionMapper";
|
||||
import { Resource, CustomAction } from "@calcom/features/pbac/domain/types/permission-registry";
|
||||
import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service";
|
||||
|
||||
import { TRPCError } from "@trpc/server";
|
||||
|
||||
import { BaseRemoveMemberService } from "./BaseRemoveMemberService";
|
||||
import type { RemoveMemberContext, RemoveMemberPermissionResult } from "./IRemoveMemberService";
|
||||
|
||||
export class PBACRemoveMemberService extends BaseRemoveMemberService {
|
||||
private permissionService = new PermissionCheckService();
|
||||
|
||||
async checkRemovePermissions(context: RemoveMemberContext): Promise<RemoveMemberPermissionResult> {
|
||||
const { userId, teamIds, isOrg } = context;
|
||||
|
||||
const resource = isOrg ? Resource.Organization : Resource.Team;
|
||||
const removePermission = PermissionMapper.toPermissionString({
|
||||
resource,
|
||||
action: CustomAction.Remove,
|
||||
});
|
||||
|
||||
const teamsWithPermission = await this.permissionService.getTeamIdsWithPermission(
|
||||
userId,
|
||||
removePermission
|
||||
);
|
||||
|
||||
// Convert to Set for O(1) lookup
|
||||
const teamsWithPermissionSet = new Set(teamsWithPermission);
|
||||
|
||||
// Check if user has permission for ALL requested teams
|
||||
const hasPermission = teamIds.every((teamId) => teamsWithPermissionSet.has(teamId));
|
||||
|
||||
return {
|
||||
hasPermission,
|
||||
};
|
||||
}
|
||||
|
||||
async validateRemoval(context: RemoveMemberContext, hasPermission: boolean): Promise<void> {
|
||||
const { userId, memberIds, teamIds } = context;
|
||||
const isRemovingSelf = memberIds.length === 1 && memberIds[0] === userId;
|
||||
|
||||
/**
|
||||
* TODO: Figure out the best way to prevent someone bricking a team
|
||||
* by removing all people with updateRole permissions
|
||||
*/
|
||||
if (isRemovingSelf && hasPermission) {
|
||||
const isOwnerOfAnyTeam = await Promise.all(
|
||||
teamIds.map(async (teamId) => await teamQueries.isTeamOwner(userId, teamId))
|
||||
).then((results) => results.some((result) => result));
|
||||
|
||||
if (isOwnerOfAnyTeam) {
|
||||
throw new TRPCError({
|
||||
code: "FORBIDDEN",
|
||||
message: "You can not remove yourself from a team you own.",
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,21 @@
|
||||
import { FeaturesRepository } from "@calcom/features/flags/features.repository";
|
||||
import { prisma } from "@calcom/prisma";
|
||||
|
||||
import type { IRemoveMemberService } from "./IRemoveMemberService";
|
||||
import { LegacyRemoveMemberService } from "./LegacyRemoveMemberService";
|
||||
import { PBACRemoveMemberService } from "./PBACRemoveMemberService";
|
||||
|
||||
export class RemoveMemberServiceFactory {
|
||||
/**
|
||||
* Creates the appropriate RemoveMemberService based on whether PBAC is enabled
|
||||
* Caches the service per team/org to avoid repeated feature flag checks
|
||||
*/
|
||||
static async create(teamId: number): Promise<IRemoveMemberService> {
|
||||
const featuresRepository = new FeaturesRepository(prisma);
|
||||
const isPBACEnabled = await featuresRepository.checkIfTeamHasFeature(teamId, "pbac");
|
||||
|
||||
const service = isPBACEnabled ? new PBACRemoveMemberService() : new LegacyRemoveMemberService();
|
||||
|
||||
return service;
|
||||
}
|
||||
}
|
||||
+501
@@ -0,0 +1,501 @@
|
||||
import { describe, expect, it, vi, beforeEach } from "vitest";
|
||||
|
||||
import * as teamQueries from "@calcom/features/ee/teams/lib/queries";
|
||||
import { TeamService } from "@calcom/lib/server/service/teamService";
|
||||
import { prisma } from "@calcom/prisma";
|
||||
import { MembershipRole } from "@calcom/prisma/enums";
|
||||
|
||||
import { TRPCError } from "@trpc/server";
|
||||
|
||||
import { LegacyRemoveMemberService } from "../LegacyRemoveMemberService";
|
||||
|
||||
vi.mock("@calcom/prisma", () => ({
|
||||
prisma: {
|
||||
membership: {
|
||||
findMany: vi.fn(),
|
||||
},
|
||||
},
|
||||
}));
|
||||
|
||||
vi.mock("@calcom/lib/server/service/teamService");
|
||||
vi.mock("@calcom/features/ee/teams/lib/queries");
|
||||
|
||||
describe("LegacyRemoveMemberService", () => {
|
||||
let service: LegacyRemoveMemberService;
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
service = new LegacyRemoveMemberService();
|
||||
});
|
||||
|
||||
describe("checkRemovePermissions", () => {
|
||||
describe("Org Admin Scenarios", () => {
|
||||
it("should allow org admin to remove members from teams they are NOT part of", async () => {
|
||||
const userId = 1;
|
||||
const isOrgAdmin = true;
|
||||
const teamIds = [100, 200]; // Teams the org admin is not part of
|
||||
const memberIds = [2, 3];
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: true,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(true);
|
||||
// Should not query database for org admin
|
||||
expect(prisma.membership.findMany).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("should bypass membership checks for org admins", async () => {
|
||||
const userId = 1;
|
||||
const isOrgAdmin = true;
|
||||
const teamIds = [1, 2, 3];
|
||||
const memberIds = [4, 5, 6];
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: true,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(true);
|
||||
expect(result.userRoles).toBeInstanceOf(Map);
|
||||
|
||||
// Org admin should have ADMIN role for all teams
|
||||
teamIds.forEach((teamId) => {
|
||||
expect(result.userRoles?.get(teamId)).toBe(MembershipRole.ADMIN);
|
||||
});
|
||||
|
||||
// Should not query database
|
||||
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 teamIds = [1, 2, 3, 4, 5];
|
||||
const memberIds = [10, 20];
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: true,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(true);
|
||||
expect(prisma.membership.findMany).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("should work for org admin even with isOrg=false", async () => {
|
||||
const userId = 1;
|
||||
const isOrgAdmin = true;
|
||||
const teamIds = [1];
|
||||
const memberIds = [2];
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false, // Note: isOrg is false
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(true);
|
||||
expect(prisma.membership.findMany).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("Regular User Scenarios", () => {
|
||||
it("should allow ADMIN to remove members", async () => {
|
||||
const userId = 1;
|
||||
const teamIds = [1, 2];
|
||||
const memberIds = [3];
|
||||
|
||||
vi.mocked(prisma.membership.findMany).mockResolvedValue([
|
||||
{ id: 1, userId, teamId: 1, role: MembershipRole.ADMIN } as any,
|
||||
{ id: 2, userId, teamId: 2, role: MembershipRole.ADMIN } as any,
|
||||
]);
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(true);
|
||||
expect(result.userRoles?.get(1)).toBe(MembershipRole.ADMIN);
|
||||
expect(result.userRoles?.get(2)).toBe(MembershipRole.ADMIN);
|
||||
});
|
||||
|
||||
it("should allow OWNER to remove members", async () => {
|
||||
const userId = 1;
|
||||
const teamIds = [1];
|
||||
const memberIds = [2];
|
||||
|
||||
vi.mocked(prisma.membership.findMany).mockResolvedValue([
|
||||
{ id: 1, userId, teamId: 1, role: MembershipRole.OWNER } as any,
|
||||
]);
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(true);
|
||||
expect(result.userRoles?.get(1)).toBe(MembershipRole.OWNER);
|
||||
});
|
||||
|
||||
it("should deny MEMBER from removing members", async () => {
|
||||
const userId = 1;
|
||||
const teamIds = [1];
|
||||
const memberIds = [2];
|
||||
|
||||
vi.mocked(prisma.membership.findMany).mockResolvedValue([
|
||||
{ id: 1, userId, teamId: 1, role: MembershipRole.MEMBER } as any,
|
||||
]);
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(false);
|
||||
});
|
||||
|
||||
it("should deny non-member from removing members", async () => {
|
||||
const userId = 1;
|
||||
const teamIds = [1];
|
||||
const memberIds = [2];
|
||||
|
||||
vi.mocked(prisma.membership.findMany).mockResolvedValue([]);
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(false);
|
||||
});
|
||||
|
||||
it("should require ADMIN/OWNER role in ALL teams for multi-team removal", async () => {
|
||||
const userId = 1;
|
||||
const teamIds = [1, 2, 3];
|
||||
const memberIds = [4];
|
||||
|
||||
vi.mocked(prisma.membership.findMany).mockResolvedValue([
|
||||
{ id: 1, userId, teamId: 1, role: MembershipRole.ADMIN } as any,
|
||||
{ id: 2, userId, teamId: 2, role: MembershipRole.MEMBER } as any, // Not admin/owner
|
||||
{ id: 3, userId, teamId: 3, role: MembershipRole.OWNER } as any,
|
||||
]);
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(false);
|
||||
});
|
||||
|
||||
it("should allow when user has ADMIN/OWNER in all teams", async () => {
|
||||
const userId = 1;
|
||||
const teamIds = [1, 2, 3];
|
||||
const memberIds = [4, 5];
|
||||
|
||||
vi.mocked(prisma.membership.findMany).mockResolvedValue([
|
||||
{ id: 1, userId, teamId: 1, role: MembershipRole.ADMIN } as any,
|
||||
{ id: 2, userId, teamId: 2, role: MembershipRole.OWNER } as any,
|
||||
{ id: 3, userId, teamId: 3, role: MembershipRole.ADMIN } as any,
|
||||
]);
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(true);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("validateRemoval", () => {
|
||||
describe("Owner Protection", () => {
|
||||
it("should prevent non-owner from removing owner", async () => {
|
||||
const userId = 1;
|
||||
const memberIds = [2];
|
||||
const teamIds = [1];
|
||||
const userRoles = new Map([[1, MembershipRole.ADMIN]]);
|
||||
|
||||
// Member 2 is owner, but userId 1 is not owner
|
||||
vi.mocked(teamQueries.isTeamOwner)
|
||||
.mockResolvedValueOnce(true) // isTeamOwner(2, 1) - member is owner
|
||||
.mockResolvedValueOnce(false); // isTeamOwner(1, 1) - current user is not owner
|
||||
|
||||
await expect(
|
||||
service.validateRemoval(
|
||||
{
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
},
|
||||
true
|
||||
)
|
||||
).rejects.toThrow(
|
||||
expect.objectContaining({
|
||||
code: "UNAUTHORIZED",
|
||||
message: "Only a team owner can remove another team owner.",
|
||||
})
|
||||
);
|
||||
});
|
||||
|
||||
it("should allow owner to remove another owner", async () => {
|
||||
const userId = 1;
|
||||
const memberIds = [2];
|
||||
const teamIds = [1];
|
||||
const userRoles = new Map([[1, MembershipRole.OWNER]]);
|
||||
|
||||
vi.mocked(teamQueries.isTeamOwner).mockResolvedValue(true);
|
||||
|
||||
await expect(
|
||||
service.validateRemoval(
|
||||
{
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
},
|
||||
true
|
||||
)
|
||||
).resolves.not.toThrow();
|
||||
});
|
||||
|
||||
it("should allow org admin to remove owner", async () => {
|
||||
const userId = 1;
|
||||
const memberIds = [2];
|
||||
const teamIds = [1];
|
||||
const userRoles = new Map([[1, MembershipRole.ADMIN]]);
|
||||
|
||||
vi.mocked(teamQueries.isTeamOwner).mockResolvedValue(true);
|
||||
|
||||
// Org admin can remove owner
|
||||
await expect(
|
||||
service.validateRemoval(
|
||||
{
|
||||
userId,
|
||||
isOrgAdmin: true, // Org admin
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: true,
|
||||
},
|
||||
{ hasPermission: true, userRoles }
|
||||
)
|
||||
).resolves.not.toThrow();
|
||||
|
||||
// isTeamOwner should not be called for org admins
|
||||
expect(teamQueries.isTeamOwner).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("Self-Removal Prevention", () => {
|
||||
it("should prevent owner from removing themselves", async () => {
|
||||
const userId = 1;
|
||||
const memberIds = [1]; // Same as userId
|
||||
const teamIds = [1];
|
||||
const userRoles = new Map([[1, MembershipRole.OWNER]]);
|
||||
|
||||
vi.mocked(teamQueries.isTeamOwner).mockResolvedValue(true); // User is owner
|
||||
|
||||
await expect(
|
||||
service.validateRemoval(
|
||||
{
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
},
|
||||
true // hasPermission - this should be boolean, not object
|
||||
)
|
||||
).rejects.toThrow(
|
||||
expect.objectContaining({
|
||||
code: "FORBIDDEN",
|
||||
message: "You can not remove yourself from a team you own.",
|
||||
})
|
||||
);
|
||||
});
|
||||
|
||||
it("should allow admin to remove themselves", async () => {
|
||||
const userId = 1;
|
||||
const memberIds = [1]; // Same as userId
|
||||
const teamIds = [1];
|
||||
const userRoles = new Map([[1, MembershipRole.ADMIN]]);
|
||||
|
||||
vi.mocked(teamQueries.isTeamOwner).mockResolvedValue(false);
|
||||
|
||||
await expect(
|
||||
service.validateRemoval(
|
||||
{
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
},
|
||||
{ hasPermission: true, userRoles }
|
||||
)
|
||||
).resolves.not.toThrow();
|
||||
});
|
||||
|
||||
it("should allow member to remove themselves", async () => {
|
||||
const userId = 1;
|
||||
const memberIds = [1]; // Same as userId
|
||||
const teamIds = [1];
|
||||
const userRoles = new Map([[1, MembershipRole.MEMBER]]);
|
||||
|
||||
vi.mocked(teamQueries.isTeamOwner).mockResolvedValue(false);
|
||||
|
||||
await expect(
|
||||
service.validateRemoval(
|
||||
{
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
},
|
||||
{ hasPermission: true, userRoles }
|
||||
)
|
||||
).resolves.not.toThrow();
|
||||
});
|
||||
});
|
||||
|
||||
describe("Multi-Member Validation", () => {
|
||||
it("should validate each member independently", async () => {
|
||||
const userId = 1;
|
||||
const memberIds = [2, 3, 4];
|
||||
const teamIds = [1];
|
||||
const userRoles = new Map([[1, MembershipRole.ADMIN]]);
|
||||
|
||||
// Member 2 is not owner, member 3 is owner, member 4 is not owner
|
||||
// Current user (1) is not owner
|
||||
vi.mocked(teamQueries.isTeamOwner)
|
||||
.mockResolvedValueOnce(false) // isTeamOwner(2, 1) - member 2 is not owner
|
||||
.mockResolvedValueOnce(true) // isTeamOwner(3, 1) - member 3 is owner
|
||||
.mockResolvedValueOnce(false) // isTeamOwner(1, 1) - current user is not owner
|
||||
.mockResolvedValueOnce(false); // isTeamOwner(4, 1) - member 4 is not owner (if reached)
|
||||
|
||||
await expect(
|
||||
service.validateRemoval(
|
||||
{
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
},
|
||||
{ hasPermission: true, userRoles }
|
||||
)
|
||||
).rejects.toThrow(
|
||||
expect.objectContaining({
|
||||
code: "UNAUTHORIZED",
|
||||
message: "Only a team owner can remove another team owner.",
|
||||
})
|
||||
);
|
||||
|
||||
expect(teamQueries.isTeamOwner).toHaveBeenCalledTimes(4); // Checks member 2, user 1, member 3 (owner), user 1
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("removeMembers", () => {
|
||||
it("should call TeamService.removeMembers with correct parameters", async () => {
|
||||
const memberIds = [1, 2, 3];
|
||||
const teamIds = [4, 5];
|
||||
const isOrg = true;
|
||||
|
||||
vi.mocked(TeamService.removeMembers).mockResolvedValue(undefined);
|
||||
|
||||
await service.removeMembers(memberIds, teamIds, isOrg);
|
||||
|
||||
expect(TeamService.removeMembers).toHaveBeenCalledWith({
|
||||
userIds: memberIds,
|
||||
teamIds,
|
||||
isOrg,
|
||||
});
|
||||
});
|
||||
|
||||
it("should propagate errors from TeamService", async () => {
|
||||
const memberIds = [1];
|
||||
const teamIds = [2];
|
||||
const isOrg = false;
|
||||
|
||||
const error = new Error("Database error");
|
||||
vi.mocked(TeamService.removeMembers).mockRejectedValue(error);
|
||||
|
||||
await expect(service.removeMembers(memberIds, teamIds, isOrg)).rejects.toThrow("Database error");
|
||||
});
|
||||
});
|
||||
|
||||
describe("Edge Cases", () => {
|
||||
it("should handle empty memberIds array", async () => {
|
||||
const userId = 1;
|
||||
const memberIds: number[] = [];
|
||||
const teamIds = [1];
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
});
|
||||
|
||||
// Should still check permissions even with no members to remove
|
||||
expect(prisma.membership.findMany).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("should handle permission check with no teams in database", async () => {
|
||||
const userId = 1;
|
||||
const teamIds = [999]; // Non-existent team
|
||||
const memberIds = [2];
|
||||
|
||||
vi.mocked(prisma.membership.findMany).mockResolvedValue([]);
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(false);
|
||||
});
|
||||
});
|
||||
});
|
||||
+474
@@ -0,0 +1,474 @@
|
||||
import { describe, expect, it, vi, beforeEach, type Mock } from "vitest";
|
||||
|
||||
import * as teamQueries from "@calcom/features/ee/teams/lib/queries";
|
||||
import { PermissionMapper } from "@calcom/features/pbac/domain/mappers/PermissionMapper";
|
||||
import { Resource, CustomAction } from "@calcom/features/pbac/domain/types/permission-registry";
|
||||
import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service";
|
||||
import { TeamService } from "@calcom/lib/server/service/teamService";
|
||||
import { prisma } from "@calcom/prisma";
|
||||
import { MembershipRole } from "@calcom/prisma/enums";
|
||||
|
||||
import { PBACRemoveMemberService } from "../PBACRemoveMemberService";
|
||||
|
||||
vi.mock("@calcom/prisma", () => ({
|
||||
prisma: {
|
||||
membership: {
|
||||
findMany: vi.fn(),
|
||||
},
|
||||
},
|
||||
}));
|
||||
|
||||
vi.mock("@calcom/lib/server/service/teamService");
|
||||
vi.mock("@calcom/features/ee/teams/lib/queries");
|
||||
vi.mock("@calcom/features/pbac/services/permission-check.service");
|
||||
vi.mock("@calcom/features/pbac/domain/mappers/PermissionMapper");
|
||||
|
||||
describe("PBACRemoveMemberService", () => {
|
||||
let service: PBACRemoveMemberService;
|
||||
let mockPermissionCheckService: {
|
||||
getTeamIdsWithPermission: Mock;
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
|
||||
mockPermissionCheckService = {
|
||||
getTeamIdsWithPermission: vi.fn(),
|
||||
};
|
||||
|
||||
vi.mocked(PermissionCheckService).mockImplementation(() => mockPermissionCheckService as any);
|
||||
|
||||
service = new PBACRemoveMemberService();
|
||||
});
|
||||
|
||||
describe("checkRemovePermissions", () => {
|
||||
describe("PBAC Permission Checks", () => {
|
||||
it("should check team.remove permission for team context", async () => {
|
||||
const userId = 1;
|
||||
const teamIds = [1, 2];
|
||||
const memberIds = [3];
|
||||
const isOrg = false;
|
||||
|
||||
const removePermission = "team.remove";
|
||||
vi.mocked(PermissionMapper.toPermissionString).mockReturnValue(removePermission);
|
||||
mockPermissionCheckService.getTeamIdsWithPermission.mockResolvedValue([1, 2]);
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(true);
|
||||
|
||||
expect(PermissionMapper.toPermissionString).toHaveBeenCalledWith({
|
||||
resource: Resource.Team,
|
||||
action: CustomAction.Remove,
|
||||
});
|
||||
|
||||
expect(mockPermissionCheckService.getTeamIdsWithPermission).toHaveBeenCalledWith(
|
||||
userId,
|
||||
removePermission
|
||||
);
|
||||
});
|
||||
|
||||
it("should check organization.remove permission for org context", async () => {
|
||||
const userId = 1;
|
||||
const teamIds = [1, 2];
|
||||
const memberIds = [3];
|
||||
const isOrg = true;
|
||||
|
||||
const removePermission = "organization.remove";
|
||||
vi.mocked(PermissionMapper.toPermissionString).mockReturnValue(removePermission);
|
||||
mockPermissionCheckService.getTeamIdsWithPermission.mockResolvedValue([1, 2]);
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(true);
|
||||
|
||||
expect(PermissionMapper.toPermissionString).toHaveBeenCalledWith({
|
||||
resource: Resource.Organization,
|
||||
action: CustomAction.Remove,
|
||||
});
|
||||
});
|
||||
|
||||
it("should deny when user lacks permission for some teams", async () => {
|
||||
const userId = 1;
|
||||
const teamIds = [1, 2, 3];
|
||||
const memberIds = [4];
|
||||
const isOrg = false;
|
||||
|
||||
const removePermission = "team.remove";
|
||||
vi.mocked(PermissionMapper.toPermissionString).mockReturnValue(removePermission);
|
||||
// User only has permission for teams 1 and 2, not 3
|
||||
mockPermissionCheckService.getTeamIdsWithPermission.mockResolvedValue([1, 2]);
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(false);
|
||||
});
|
||||
|
||||
it("should allow when user has permission for all teams", async () => {
|
||||
const userId = 1;
|
||||
const teamIds = [1, 2, 3];
|
||||
const memberIds = [4, 5];
|
||||
const isOrg = false;
|
||||
|
||||
const removePermission = "team.remove";
|
||||
vi.mocked(PermissionMapper.toPermissionString).mockReturnValue(removePermission);
|
||||
mockPermissionCheckService.getTeamIdsWithPermission.mockResolvedValue([1, 2, 3, 4]); // Has more permissions
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(true);
|
||||
});
|
||||
|
||||
it("should deny when user has no permissions", async () => {
|
||||
const userId = 1;
|
||||
const teamIds = [1];
|
||||
const memberIds = [2];
|
||||
const isOrg = false;
|
||||
|
||||
const removePermission = "team.remove";
|
||||
vi.mocked(PermissionMapper.toPermissionString).mockReturnValue(removePermission);
|
||||
mockPermissionCheckService.getTeamIdsWithPermission.mockResolvedValue([]);
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("Multi-team Operations", () => {
|
||||
it("should require permission for ALL teams in request", async () => {
|
||||
const userId = 1;
|
||||
const teamIds = [10, 20, 30, 40];
|
||||
const memberIds = [100];
|
||||
const isOrg = false;
|
||||
|
||||
const removePermission = "team.remove";
|
||||
vi.mocked(PermissionMapper.toPermissionString).mockReturnValue(removePermission);
|
||||
// Missing permission for team 30
|
||||
mockPermissionCheckService.getTeamIdsWithPermission.mockResolvedValue([10, 20, 40]);
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(false);
|
||||
});
|
||||
|
||||
it("should handle large team lists efficiently", async () => {
|
||||
const userId = 1;
|
||||
const teamIds = Array.from({ length: 100 }, (_, i) => i + 1);
|
||||
const memberIds = [999];
|
||||
const isOrg = true;
|
||||
|
||||
const removePermission = "organization.remove";
|
||||
vi.mocked(PermissionMapper.toPermissionString).mockReturnValue(removePermission);
|
||||
mockPermissionCheckService.getTeamIdsWithPermission.mockResolvedValue(teamIds);
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(true);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("validateRemoval", () => {
|
||||
describe("Owner Protection", () => {
|
||||
it("should allow removing owners with PBAC permissions", async () => {
|
||||
const userId = 1;
|
||||
const memberIds = [2];
|
||||
const teamIds = [1];
|
||||
|
||||
vi.mocked(teamQueries.isTeamOwner).mockResolvedValue(true); // Member 2 is owner
|
||||
|
||||
// Get user's role
|
||||
vi.mocked(prisma.membership.findMany).mockResolvedValue([
|
||||
{ id: 1, userId, teamId: 1, role: MembershipRole.ADMIN } as any,
|
||||
]);
|
||||
|
||||
// PBAC service doesn't have owner-to-owner protection logic
|
||||
// If user has PBAC remove permission, they can remove owners (unlike Legacy service)
|
||||
await expect(
|
||||
service.validateRemoval(
|
||||
{
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
},
|
||||
true // hasPermission
|
||||
)
|
||||
).resolves.not.toThrow();
|
||||
});
|
||||
|
||||
it("should allow owner to remove another owner with PBAC", async () => {
|
||||
const userId = 1;
|
||||
const memberIds = [2];
|
||||
const teamIds = [1];
|
||||
|
||||
vi.mocked(teamQueries.isTeamOwner).mockResolvedValue(true); // Member 2 is owner
|
||||
|
||||
// User is owner
|
||||
vi.mocked(prisma.membership.findMany).mockResolvedValue([
|
||||
{ id: 1, userId, teamId: 1, role: MembershipRole.OWNER } as any,
|
||||
]);
|
||||
|
||||
await expect(
|
||||
service.validateRemoval(
|
||||
{
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
},
|
||||
true
|
||||
)
|
||||
).resolves.not.toThrow();
|
||||
});
|
||||
});
|
||||
|
||||
describe("Self-Removal Prevention", () => {
|
||||
it("should prevent owner from removing themselves", async () => {
|
||||
const userId = 1;
|
||||
const memberIds = [1]; // Same as userId
|
||||
const teamIds = [1];
|
||||
|
||||
vi.mocked(teamQueries.isTeamOwner).mockResolvedValue(true); // User is owner of the team
|
||||
|
||||
vi.mocked(prisma.membership.findMany).mockResolvedValue([
|
||||
{ id: 1, userId, teamId: 1, role: MembershipRole.OWNER } as any,
|
||||
]);
|
||||
|
||||
await expect(
|
||||
service.validateRemoval(
|
||||
{
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
},
|
||||
true
|
||||
)
|
||||
).rejects.toThrow(
|
||||
expect.objectContaining({
|
||||
code: "FORBIDDEN",
|
||||
message: "You can not remove yourself from a team you own.",
|
||||
})
|
||||
);
|
||||
});
|
||||
|
||||
it("should allow admin to remove themselves", async () => {
|
||||
const userId = 1;
|
||||
const memberIds = [1];
|
||||
const teamIds = [1];
|
||||
|
||||
vi.mocked(teamQueries.isTeamOwner).mockResolvedValue(false);
|
||||
|
||||
vi.mocked(prisma.membership.findMany).mockResolvedValue([
|
||||
{ id: 1, userId, teamId: 1, role: MembershipRole.ADMIN } as any,
|
||||
]);
|
||||
|
||||
await expect(
|
||||
service.validateRemoval(
|
||||
{
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
},
|
||||
true
|
||||
)
|
||||
).resolves.not.toThrow();
|
||||
});
|
||||
});
|
||||
|
||||
describe("Multi-Member Validation", () => {
|
||||
it("should allow removing multiple members including owners", async () => {
|
||||
const userId = 1;
|
||||
const memberIds = [2, 3, 4];
|
||||
const teamIds = [1];
|
||||
|
||||
// User is admin
|
||||
vi.mocked(prisma.membership.findMany).mockResolvedValue([
|
||||
{ id: 1, userId, teamId: 1, role: MembershipRole.ADMIN } as any,
|
||||
]);
|
||||
|
||||
// Member 3 is owner
|
||||
vi.mocked(teamQueries.isTeamOwner)
|
||||
.mockResolvedValueOnce(false) // member 2
|
||||
.mockResolvedValueOnce(true) // member 3 is owner
|
||||
.mockResolvedValueOnce(false); // member 4
|
||||
|
||||
// PBAC service doesn't validate owner-to-owner removal
|
||||
// It only prevents self-removal by owners
|
||||
await expect(
|
||||
service.validateRemoval(
|
||||
{
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
},
|
||||
true
|
||||
)
|
||||
).resolves.not.toThrow();
|
||||
});
|
||||
});
|
||||
|
||||
describe("Edge Cases", () => {
|
||||
it("should handle validation when hasPermission is false", async () => {
|
||||
const userId = 1;
|
||||
const memberIds = [2];
|
||||
const teamIds = [1];
|
||||
|
||||
// Should not perform validation if no permission
|
||||
await expect(
|
||||
service.validateRemoval(
|
||||
{
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg: false,
|
||||
},
|
||||
false // No permission
|
||||
)
|
||||
).resolves.not.toThrow();
|
||||
|
||||
// Should not check ownership or roles
|
||||
expect(teamQueries.isTeamOwner).not.toHaveBeenCalled();
|
||||
expect(prisma.membership.findMany).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("removeMembers", () => {
|
||||
it("should call TeamService.removeMembers with correct parameters", async () => {
|
||||
const memberIds = [1, 2, 3];
|
||||
const teamIds = [4, 5];
|
||||
const isOrg = true;
|
||||
|
||||
vi.mocked(TeamService.removeMembers).mockResolvedValue(undefined);
|
||||
|
||||
await service.removeMembers(memberIds, teamIds, isOrg);
|
||||
|
||||
expect(TeamService.removeMembers).toHaveBeenCalledWith({
|
||||
userIds: memberIds,
|
||||
teamIds,
|
||||
isOrg,
|
||||
});
|
||||
});
|
||||
|
||||
it("should propagate errors from TeamService", async () => {
|
||||
const memberIds = [1];
|
||||
const teamIds = [2];
|
||||
const isOrg = false;
|
||||
|
||||
const error = new Error("Database connection failed");
|
||||
vi.mocked(TeamService.removeMembers).mockRejectedValue(error);
|
||||
|
||||
await expect(service.removeMembers(memberIds, teamIds, isOrg)).rejects.toThrow(
|
||||
"Database connection failed"
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("Service Initialization", () => {
|
||||
it("should create PermissionCheckService on instantiation", () => {
|
||||
const newService = new PBACRemoveMemberService();
|
||||
|
||||
expect(PermissionCheckService).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("Permission Service Integration", () => {
|
||||
it("should handle permission service errors gracefully", async () => {
|
||||
const userId = 1;
|
||||
const teamIds = [1];
|
||||
const memberIds = [2];
|
||||
const isOrg = false;
|
||||
|
||||
const removePermission = "team.remove";
|
||||
vi.mocked(PermissionMapper.toPermissionString).mockReturnValue(removePermission);
|
||||
|
||||
mockPermissionCheckService.getTeamIdsWithPermission.mockRejectedValue(
|
||||
new Error("Permission service unavailable")
|
||||
);
|
||||
|
||||
await expect(
|
||||
service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg,
|
||||
})
|
||||
).rejects.toThrow("Permission service unavailable");
|
||||
});
|
||||
|
||||
it("should handle empty permission results", async () => {
|
||||
const userId = 1;
|
||||
const teamIds = [1, 2];
|
||||
const memberIds = [3];
|
||||
const isOrg = false;
|
||||
|
||||
const removePermission = "team.remove";
|
||||
vi.mocked(PermissionMapper.toPermissionString).mockReturnValue(removePermission);
|
||||
mockPermissionCheckService.getTeamIdsWithPermission.mockResolvedValue(null as any);
|
||||
|
||||
const result = await service.checkRemovePermissions({
|
||||
userId,
|
||||
isOrgAdmin: false,
|
||||
memberIds,
|
||||
teamIds,
|
||||
isOrg,
|
||||
});
|
||||
|
||||
expect(result.hasPermission).toBe(false);
|
||||
});
|
||||
});
|
||||
});
|
||||
+105
@@ -0,0 +1,105 @@
|
||||
import { describe, expect, it, vi, beforeEach } from "vitest";
|
||||
|
||||
import { FeaturesRepository } from "@calcom/features/flags/features.repository";
|
||||
|
||||
import { LegacyRemoveMemberService } from "../LegacyRemoveMemberService";
|
||||
import { PBACRemoveMemberService } from "../PBACRemoveMemberService";
|
||||
import { RemoveMemberServiceFactory } from "../RemoveMemberServiceFactory";
|
||||
|
||||
vi.mock("@calcom/features/flags/features.repository");
|
||||
vi.mock("../LegacyRemoveMemberService");
|
||||
vi.mock("../PBACRemoveMemberService");
|
||||
|
||||
describe("RemoveMemberServiceFactory", () => {
|
||||
let mockFeaturesRepository: {
|
||||
checkIfTeamHasFeature: vi.Mock;
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
|
||||
mockFeaturesRepository = {
|
||||
checkIfTeamHasFeature: vi.fn(),
|
||||
};
|
||||
|
||||
vi.mocked(FeaturesRepository).mockImplementation(() => mockFeaturesRepository as any);
|
||||
});
|
||||
|
||||
describe("Service Creation", () => {
|
||||
it("should create LegacyRemoveMemberService when PBAC is disabled", async () => {
|
||||
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValue(false);
|
||||
|
||||
const teamId = 1;
|
||||
const service = await RemoveMemberServiceFactory.create(teamId);
|
||||
|
||||
expect(mockFeaturesRepository.checkIfTeamHasFeature).toHaveBeenCalledWith(teamId, "pbac");
|
||||
expect(service).toBeInstanceOf(LegacyRemoveMemberService);
|
||||
});
|
||||
|
||||
it("should create PBACRemoveMemberService when PBAC is enabled", async () => {
|
||||
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValue(true);
|
||||
|
||||
const teamId = 1;
|
||||
const service = await RemoveMemberServiceFactory.create(teamId);
|
||||
|
||||
expect(mockFeaturesRepository.checkIfTeamHasFeature).toHaveBeenCalledWith(teamId, "pbac");
|
||||
expect(service).toBeInstanceOf(PBACRemoveMemberService);
|
||||
});
|
||||
});
|
||||
|
||||
describe("Service Creation for Different Teams", () => {
|
||||
it("should create different services for different teams", async () => {
|
||||
// Team 1 has PBAC disabled
|
||||
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(false);
|
||||
// Team 2 has PBAC enabled
|
||||
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
|
||||
|
||||
const service1 = await RemoveMemberServiceFactory.create(1);
|
||||
const service2 = await RemoveMemberServiceFactory.create(2);
|
||||
|
||||
expect(service1).toBeInstanceOf(LegacyRemoveMemberService);
|
||||
expect(service2).toBeInstanceOf(PBACRemoveMemberService);
|
||||
expect(service1).not.toBe(service2);
|
||||
|
||||
expect(mockFeaturesRepository.checkIfTeamHasFeature).toHaveBeenCalledTimes(2);
|
||||
expect(mockFeaturesRepository.checkIfTeamHasFeature).toHaveBeenCalledWith(1, "pbac");
|
||||
expect(mockFeaturesRepository.checkIfTeamHasFeature).toHaveBeenCalledWith(2, "pbac");
|
||||
});
|
||||
|
||||
it("should create service for each call (no caching in current implementation)", async () => {
|
||||
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValue(true);
|
||||
|
||||
const teamId = 1;
|
||||
|
||||
// Make multiple calls
|
||||
const service1 = await RemoveMemberServiceFactory.create(teamId);
|
||||
const service2 = await RemoveMemberServiceFactory.create(teamId);
|
||||
const service3 = await RemoveMemberServiceFactory.create(teamId);
|
||||
|
||||
// All should be different instances (no caching currently)
|
||||
expect(service1).not.toBe(service2);
|
||||
expect(service2).not.toBe(service3);
|
||||
|
||||
// Feature flag should be called each time
|
||||
expect(mockFeaturesRepository.checkIfTeamHasFeature).toHaveBeenCalledTimes(3);
|
||||
});
|
||||
});
|
||||
|
||||
describe("Error Handling", () => {
|
||||
it("should propagate errors from features repository", async () => {
|
||||
const error = new Error("Features repository error");
|
||||
mockFeaturesRepository.checkIfTeamHasFeature.mockRejectedValue(error);
|
||||
|
||||
await expect(RemoveMemberServiceFactory.create(1)).rejects.toThrow("Features repository error");
|
||||
});
|
||||
|
||||
it("should handle null/undefined feature flag gracefully", async () => {
|
||||
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValue(null as any);
|
||||
|
||||
const service = await RemoveMemberServiceFactory.create(1);
|
||||
|
||||
// Should default to Legacy when feature flag is falsy
|
||||
expect(service).toBeInstanceOf(LegacyRemoveMemberService);
|
||||
});
|
||||
});
|
||||
});
|
||||
+308
@@ -0,0 +1,308 @@
|
||||
import { describe, expect, it, vi, beforeEach } from "vitest";
|
||||
|
||||
import { checkRateLimitAndThrowError } from "@calcom/lib/checkRateLimitAndThrowError";
|
||||
|
||||
import { TRPCError } from "@trpc/server";
|
||||
|
||||
import { removeMemberHandler } from "../../removeMember.handler";
|
||||
import type { IRemoveMemberService } from "../IRemoveMemberService";
|
||||
import { RemoveMemberServiceFactory } from "../RemoveMemberServiceFactory";
|
||||
|
||||
vi.mock("@calcom/lib/checkRateLimitAndThrowError");
|
||||
vi.mock("../RemoveMemberServiceFactory");
|
||||
|
||||
describe("removeMemberHandler", () => {
|
||||
const mockService: IRemoveMemberService = {
|
||||
checkRemovePermissions: vi.fn(),
|
||||
validateRemoval: vi.fn(),
|
||||
removeMembers: vi.fn(),
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
vi.mocked(checkRateLimitAndThrowError).mockResolvedValue({
|
||||
success: true,
|
||||
remaining: 99,
|
||||
limit: 100,
|
||||
reset: new Date().getTime() + 60 * 1000,
|
||||
});
|
||||
vi.mocked(RemoveMemberServiceFactory.create).mockResolvedValue(mockService);
|
||||
vi.mocked(mockService.checkRemovePermissions).mockResolvedValue({
|
||||
hasPermission: true,
|
||||
userRoles: new Map(),
|
||||
});
|
||||
});
|
||||
|
||||
describe("Rate Limiting", () => {
|
||||
it("should check rate limit before processing", async () => {
|
||||
const userId = 1;
|
||||
const input = {
|
||||
teamIds: [1],
|
||||
memberIds: [2],
|
||||
isOrg: false,
|
||||
};
|
||||
|
||||
await removeMemberHandler({
|
||||
ctx: { user: { id: userId } },
|
||||
input,
|
||||
});
|
||||
|
||||
expect(checkRateLimitAndThrowError).toHaveBeenCalledWith({
|
||||
identifier: `removeMember.${userId}`,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("Input Validation", () => {
|
||||
it("should throw BAD_REQUEST when no team IDs provided", async () => {
|
||||
const input = {
|
||||
teamIds: [],
|
||||
memberIds: [2],
|
||||
isOrg: false,
|
||||
};
|
||||
|
||||
await expect(
|
||||
removeMemberHandler({
|
||||
ctx: { user: { id: 1 } },
|
||||
input,
|
||||
})
|
||||
).rejects.toThrow(
|
||||
expect.objectContaining({
|
||||
code: "BAD_REQUEST",
|
||||
message: "At least one team ID must be provided",
|
||||
})
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("Service Factory Integration", () => {
|
||||
it("should create service using primary team ID", async () => {
|
||||
const primaryTeamId = 1;
|
||||
const input = {
|
||||
teamIds: [primaryTeamId, 2, 3],
|
||||
memberIds: [4, 5],
|
||||
isOrg: false,
|
||||
};
|
||||
|
||||
mockService.checkRemovePermissions = vi.fn().mockResolvedValue({ hasPermission: true });
|
||||
|
||||
await removeMemberHandler({
|
||||
ctx: { user: { id: 1 } },
|
||||
input,
|
||||
});
|
||||
|
||||
expect(RemoveMemberServiceFactory.create).toHaveBeenCalledWith(primaryTeamId);
|
||||
});
|
||||
});
|
||||
|
||||
describe("Permission Checking", () => {
|
||||
it("should pass org admin status to permission check", async () => {
|
||||
const userId = 1;
|
||||
const isOrgAdmin = true;
|
||||
const input = {
|
||||
teamIds: [1],
|
||||
memberIds: [2],
|
||||
isOrg: true,
|
||||
};
|
||||
|
||||
mockService.checkRemovePermissions = vi.fn().mockResolvedValue({ hasPermission: true });
|
||||
|
||||
await removeMemberHandler({
|
||||
ctx: {
|
||||
user: {
|
||||
id: userId,
|
||||
organization: { isOrgAdmin },
|
||||
},
|
||||
},
|
||||
input,
|
||||
});
|
||||
|
||||
expect(mockService.checkRemovePermissions).toHaveBeenCalledWith({
|
||||
userId,
|
||||
isOrgAdmin,
|
||||
memberIds: input.memberIds,
|
||||
teamIds: input.teamIds,
|
||||
isOrg: input.isOrg,
|
||||
});
|
||||
});
|
||||
|
||||
it("should default isOrgAdmin to false when not provided", async () => {
|
||||
const userId = 1;
|
||||
const input = {
|
||||
teamIds: [1],
|
||||
memberIds: [2],
|
||||
isOrg: false,
|
||||
};
|
||||
|
||||
mockService.checkRemovePermissions = vi.fn().mockResolvedValue({ hasPermission: true });
|
||||
|
||||
await removeMemberHandler({
|
||||
ctx: { user: { id: userId } },
|
||||
input,
|
||||
});
|
||||
|
||||
expect(mockService.checkRemovePermissions).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
isOrgAdmin: false,
|
||||
})
|
||||
);
|
||||
});
|
||||
|
||||
it("should throw UNAUTHORIZED when user lacks permission", async () => {
|
||||
const input = {
|
||||
teamIds: [1],
|
||||
memberIds: [2],
|
||||
isOrg: false,
|
||||
};
|
||||
|
||||
mockService.checkRemovePermissions = vi.fn().mockResolvedValue({ hasPermission: false });
|
||||
|
||||
await expect(
|
||||
removeMemberHandler({
|
||||
ctx: { user: { id: 1 } },
|
||||
input,
|
||||
})
|
||||
).rejects.toThrow(
|
||||
expect.objectContaining({
|
||||
code: "UNAUTHORIZED",
|
||||
})
|
||||
);
|
||||
|
||||
expect(mockService.validateRemoval).not.toHaveBeenCalled();
|
||||
expect(mockService.removeMembers).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("Removal Flow", () => {
|
||||
it("should complete full removal flow when user has permission", async () => {
|
||||
const userId = 1;
|
||||
const isOrgAdmin = false;
|
||||
const input = {
|
||||
teamIds: [1, 2],
|
||||
memberIds: [3, 4],
|
||||
isOrg: false,
|
||||
};
|
||||
|
||||
const hasPermissionResult = {
|
||||
hasPermission: true,
|
||||
userRoles: new Map([
|
||||
[1, "ADMIN"],
|
||||
[2, "OWNER"],
|
||||
]),
|
||||
};
|
||||
|
||||
mockService.checkRemovePermissions = vi.fn().mockResolvedValue(hasPermissionResult);
|
||||
mockService.validateRemoval = vi.fn().mockResolvedValue(undefined);
|
||||
mockService.removeMembers = vi.fn().mockResolvedValue(undefined);
|
||||
|
||||
await removeMemberHandler({
|
||||
ctx: { user: { id: userId } },
|
||||
input,
|
||||
});
|
||||
|
||||
// Verify service calls in order
|
||||
expect(mockService.checkRemovePermissions).toHaveBeenCalledWith({
|
||||
userId,
|
||||
isOrgAdmin,
|
||||
memberIds: input.memberIds,
|
||||
teamIds: input.teamIds,
|
||||
isOrg: input.isOrg,
|
||||
});
|
||||
|
||||
expect(mockService.validateRemoval).toHaveBeenCalledWith(
|
||||
{
|
||||
userId,
|
||||
isOrgAdmin,
|
||||
memberIds: input.memberIds,
|
||||
teamIds: input.teamIds,
|
||||
isOrg: input.isOrg,
|
||||
},
|
||||
hasPermissionResult.hasPermission
|
||||
);
|
||||
|
||||
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 () => {
|
||||
const userId = 1;
|
||||
const isOrgAdmin = true;
|
||||
const input = {
|
||||
teamIds: [100, 200], // Teams the org admin is not part of
|
||||
memberIds: [3, 4],
|
||||
isOrg: true,
|
||||
};
|
||||
|
||||
const hasPermissionResult = { hasPermission: true };
|
||||
|
||||
mockService.checkRemovePermissions = vi.fn().mockResolvedValue(hasPermissionResult);
|
||||
mockService.validateRemoval = vi.fn().mockResolvedValue(undefined);
|
||||
mockService.removeMembers = vi.fn().mockResolvedValue(undefined);
|
||||
|
||||
await removeMemberHandler({
|
||||
ctx: {
|
||||
user: {
|
||||
id: userId,
|
||||
organization: { isOrgAdmin },
|
||||
},
|
||||
},
|
||||
input,
|
||||
});
|
||||
|
||||
expect(mockService.checkRemovePermissions).toHaveBeenCalledWith({
|
||||
userId,
|
||||
isOrgAdmin: true,
|
||||
memberIds: input.memberIds,
|
||||
teamIds: input.teamIds,
|
||||
isOrg: input.isOrg,
|
||||
});
|
||||
|
||||
expect(mockService.removeMembers).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("Error Handling", () => {
|
||||
it("should propagate validation errors", async () => {
|
||||
const input = {
|
||||
teamIds: [1],
|
||||
memberIds: [2],
|
||||
isOrg: false,
|
||||
};
|
||||
|
||||
mockService.checkRemovePermissions = vi.fn().mockResolvedValue({ hasPermission: true });
|
||||
mockService.validateRemoval = vi
|
||||
.fn()
|
||||
.mockRejectedValue(new TRPCError({ code: "BAD_REQUEST", message: "Cannot remove owner" }));
|
||||
|
||||
await expect(
|
||||
removeMemberHandler({
|
||||
ctx: { user: { id: 1 } },
|
||||
input,
|
||||
})
|
||||
).rejects.toThrow(
|
||||
expect.objectContaining({
|
||||
code: "BAD_REQUEST",
|
||||
message: "Cannot remove owner",
|
||||
})
|
||||
);
|
||||
});
|
||||
|
||||
it("should propagate service removal errors", async () => {
|
||||
const input = {
|
||||
teamIds: [1],
|
||||
memberIds: [2],
|
||||
isOrg: false,
|
||||
};
|
||||
|
||||
mockService.checkRemovePermissions = vi.fn().mockResolvedValue({ hasPermission: true });
|
||||
mockService.validateRemoval = vi.fn().mockResolvedValue(undefined);
|
||||
mockService.removeMembers = vi.fn().mockRejectedValue(new Error("Database error"));
|
||||
|
||||
await expect(
|
||||
removeMemberHandler({
|
||||
ctx: { user: { id: 1 } },
|
||||
input,
|
||||
})
|
||||
).rejects.toThrow("Database error");
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,9 @@
|
||||
export { RemoveMemberServiceFactory } from "./RemoveMemberServiceFactory";
|
||||
export type {
|
||||
IRemoveMemberService,
|
||||
RemoveMemberContext,
|
||||
RemoveMemberPermissionResult,
|
||||
} from "./IRemoveMemberService";
|
||||
export { BaseRemoveMemberService } from "./BaseRemoveMemberService";
|
||||
export { PBACRemoveMemberService } from "./PBACRemoveMemberService";
|
||||
export { LegacyRemoveMemberService } from "./LegacyRemoveMemberService";
|
||||
Reference in New Issue
Block a user