Skip to content

Commit b223937

Browse files
joeauyeungemrysalsean-brydon
authored
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 <[email protected]> Co-authored-by: Sean Brydon <[email protected]>
1 parent 77c61ab commit b223937

11 files changed

Lines changed: 1683 additions & 94 deletions

‎packages/trpc/server/routers/viewer/teams/removeMember.handler.ts‎

Lines changed: 45 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -1,121 +1,72 @@
1-
import { FeaturesRepository } from "@calcom/features/flags/features.repository";
2-
import { Resource, CustomAction } from "@calcom/features/pbac/domain/types/permission-registry";
3-
import { getSpecificPermissions } from "@calcom/features/pbac/lib/resource-permissions";
41
import { checkRateLimitAndThrowError } from "@calcom/lib/checkRateLimitAndThrowError";
5-
import { isTeamOwner } from "@calcom/features/ee/teams/lib/queries";
6-
import { TeamService } from "@calcom/lib/server/service/teamService";
7-
import { prisma } from "@calcom/prisma";
8-
import { MembershipRole } from "@calcom/prisma/enums";
9-
import type { TrpcSessionUser } from "@calcom/trpc/server/types";
102

113
import { TRPCError } from "@trpc/server";
124

135
import type { TRemoveMemberInputSchema } from "./removeMember.schema";
6+
import { RemoveMemberServiceFactory } from "./removeMember/RemoveMemberServiceFactory";
147

158
type RemoveMemberOptions = {
169
ctx: {
17-
user: NonNullable<TrpcSessionUser>;
18-
sourceIp?: string;
10+
user: {
11+
id: number;
12+
organization?: {
13+
isOrgAdmin: boolean;
14+
};
15+
};
1916
};
2017
input: TRemoveMemberInputSchema;
2118
};
2219

23-
export const removeMemberHandler = async ({ ctx, input }: RemoveMemberOptions) => {
20+
export const removeMemberHandler = async ({
21+
ctx: {
22+
user: { id: userId, organization },
23+
},
24+
input,
25+
}: RemoveMemberOptions) => {
2426
await checkRateLimitAndThrowError({
25-
identifier: `removeMember.${ctx.user.id}`,
27+
identifier: `removeMember.${userId}`,
2628
});
2729

2830
const { memberIds, teamIds, isOrg } = input;
31+
const isOrgAdmin = organization?.isOrgAdmin ?? false;
32+
33+
// Note: This assumes that all teams in the request have the same PBAC setting 9999% chance they do.
34+
const primaryTeamId = teamIds[0];
35+
if (!primaryTeamId) {
36+
throw new TRPCError({
37+
code: "BAD_REQUEST",
38+
message: "At least one team ID must be provided",
39+
});
40+
}
2941

30-
// Check PBAC permissions for each team
31-
const hasRemovePermission = await Promise.all(
32-
teamIds.map(async (teamId) => {
33-
// Get user's membership role in this team
34-
const membership = await prisma.membership.findFirst({
35-
where: {
36-
userId: ctx.user.id,
37-
teamId: teamId,
38-
},
39-
select: {
40-
role: true,
41-
},
42-
});
43-
44-
if (!membership) return false;
45-
46-
// Check PBAC permissions for removing team members
47-
const permissions = await getSpecificPermissions({
48-
userId: ctx.user.id,
49-
teamId: teamId,
50-
resource: isOrg ? Resource.Organization : Resource.Team,
51-
userRole: membership.role,
52-
actions: [CustomAction.Remove],
53-
fallbackRoles: {
54-
[CustomAction.Remove]: {
55-
roles: [MembershipRole.ADMIN, MembershipRole.OWNER],
56-
},
57-
},
58-
});
59-
60-
return permissions[CustomAction.Remove];
61-
})
62-
).then((results) => results.every((result) => result));
42+
// Get the appropriate service based on feature flag
43+
const service = await RemoveMemberServiceFactory.create(primaryTeamId);
6344

64-
// Check if user is trying to remove themselves (allowed for non-owners)
65-
const isRemovingSelf = memberIds.length === 1 && memberIds[0] === ctx.user.id;
45+
const { hasPermission } = await service.checkRemovePermissions({
46+
userId,
47+
isOrgAdmin,
48+
memberIds,
49+
teamIds,
50+
isOrg,
51+
});
6652

67-
// Allow if user has remove permission OR if they're removing themselves
68-
if (!hasRemovePermission && !isRemovingSelf) {
53+
if (!hasPermission) {
6954
throw new TRPCError({ code: "UNAUTHORIZED" });
7055
}
7156

72-
// TODO(SEAN): Remove this after PBAC is rolled out.
73-
// Check if any team has PBAC enabled
74-
const featuresRepository = new FeaturesRepository(prisma);
75-
const pbacEnabledForTeams = await Promise.all(
76-
teamIds.map(async (teamId) => await featuresRepository.checkIfTeamHasFeature(teamId, "pbac"))
57+
await service.validateRemoval(
58+
{
59+
userId,
60+
isOrgAdmin,
61+
memberIds,
62+
teamIds,
63+
isOrg,
64+
},
65+
hasPermission
7766
);
78-
const isAnyTeamPBACEnabled = pbacEnabledForTeams.some((enabled) => enabled);
79-
80-
// Only apply traditional owner-based logic if PBAC is not enabled for any teams
81-
if (!isAnyTeamPBACEnabled) {
82-
// Only a team owner can remove another team owner.
83-
const isAnyMemberOwnerAndCurrentUserNotOwner = await Promise.all(
84-
memberIds.map(async (memberId) => {
85-
const isAnyTeamOwnerAndCurrentUserNotOwner = await Promise.all(
86-
teamIds.map(async (teamId) => {
87-
return (await isTeamOwner(memberId, teamId)) && !(await isTeamOwner(ctx.user.id, teamId));
88-
})
89-
).then((results) => results.some((result) => result));
90-
91-
return isAnyTeamOwnerAndCurrentUserNotOwner;
92-
})
93-
).then((results) => results.some((result) => result));
94-
95-
if (isAnyMemberOwnerAndCurrentUserNotOwner) {
96-
throw new TRPCError({
97-
code: "UNAUTHORIZED",
98-
message: "Only a team owner can remove another team owner.",
99-
});
100-
}
101-
102-
// Check if user is trying to remove themselves from a team they own (prevent this)
103-
if (isRemovingSelf && hasRemovePermission) {
104-
// Additional check: ensure they're not an owner trying to remove themselves
105-
const isOwnerOfAnyTeam = await Promise.all(
106-
teamIds.map(async (teamId) => await isTeamOwner(ctx.user.id, teamId))
107-
).then((results) => results.some((result) => result));
108-
109-
if (isOwnerOfAnyTeam) {
110-
throw new TRPCError({
111-
code: "FORBIDDEN",
112-
message: "You can not remove yourself from a team you own.",
113-
});
114-
}
115-
}
116-
}
11767

118-
await TeamService.removeMembers({ teamIds, userIds: memberIds, isOrg });
68+
// Perform the removal
69+
await service.removeMembers(memberIds, teamIds, isOrg);
11970
};
12071

12172
export default removeMemberHandler;

‎packages/trpc/server/routers/viewer/teams/removeMember/BaseRemoveMemberService.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
import { TeamService } from "@calcom/lib/server/service/teamService";
2+
3+
import type {
4+
IRemoveMemberService,
5+
RemoveMemberContext,
6+
RemoveMemberPermissionResult,
7+
} from "./IRemoveMemberService";
8+
9+
/**
10+
* Base abstract class for remove member services
11+
* Provides common functionality and defines abstract methods for specific implementations
12+
*/
13+
export abstract class BaseRemoveMemberService implements IRemoveMemberService {
14+
abstract checkRemovePermissions(context: RemoveMemberContext): Promise<RemoveMemberPermissionResult>;
15+
16+
abstract validateRemoval(context: RemoveMemberContext, hasPermission: boolean): Promise<void>;
17+
18+
async removeMembers(memberIds: number[], teamIds: number[], isOrg: boolean): Promise<void> {
19+
await TeamService.removeMembers({ teamIds, userIds: memberIds, isOrg });
20+
}
21+
}

‎packages/trpc/server/routers/viewer/teams/removeMember/IRemoveMemberService.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
import type { MembershipRole } from "@calcom/prisma/enums";
2+
3+
export interface RemoveMemberContext {
4+
userId: number;
5+
isOrgAdmin: boolean;
6+
memberIds: number[];
7+
teamIds: number[];
8+
isOrg: boolean;
9+
}
10+
11+
export interface RemoveMemberPermissionResult {
12+
hasPermission: boolean;
13+
userRoles?: Map<number, MembershipRole | null>;
14+
}
15+
16+
export interface IRemoveMemberService {
17+
/**
18+
* Checks if the user has permission to remove members from teams
19+
*/
20+
checkRemovePermissions(context: RemoveMemberContext): Promise<RemoveMemberPermissionResult>;
21+
22+
/**
23+
* Validates that the removal can proceed (e.g., owner checks)
24+
*/
25+
validateRemoval(context: RemoveMemberContext, hasPermission: boolean): Promise<void>;
26+
27+
/**
28+
* Performs the actual removal of members from teams
29+
*/
30+
removeMembers(memberIds: number[], teamIds: number[], isOrg: boolean): Promise<void>;
31+
}

‎packages/trpc/server/routers/viewer/teams/removeMember/LegacyRemoveMemberService.ts‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
import * as teamQueries from "@calcom/features/ee/teams/lib/queries";
2+
import { prisma } from "@calcom/prisma";
3+
import { MembershipRole } from "@calcom/prisma/enums";
4+
5+
import { TRPCError } from "@trpc/server";
6+
7+
import { BaseRemoveMemberService } from "./BaseRemoveMemberService";
8+
import type { RemoveMemberContext, RemoveMemberPermissionResult } from "./IRemoveMemberService";
9+
10+
export class LegacyRemoveMemberService extends BaseRemoveMemberService {
11+
async checkRemovePermissions(context: RemoveMemberContext): Promise<RemoveMemberPermissionResult> {
12+
const { userId, isOrgAdmin, teamIds } = context;
13+
14+
// Org admins have full permission
15+
if (isOrgAdmin) {
16+
// Return admin role for all teams
17+
const userRoles = new Map<number, MembershipRole | null>();
18+
teamIds.forEach((teamId) => {
19+
userRoles.set(teamId, MembershipRole.ADMIN);
20+
});
21+
return {
22+
hasPermission: true,
23+
userRoles,
24+
};
25+
}
26+
27+
// Get all memberships in a single query
28+
const membershipRoles = await prisma.membership.findMany({
29+
where: {
30+
userId,
31+
teamId: {
32+
in: teamIds,
33+
},
34+
},
35+
select: {
36+
role: true,
37+
teamId: true,
38+
},
39+
});
40+
41+
// Create a map for O(1) lookup
42+
const userRoles = new Map<number, MembershipRole | null>();
43+
membershipRoles.forEach((m) => {
44+
userRoles.set(m.teamId, m.role);
45+
});
46+
47+
// Check if user has admin or owner role in all teams
48+
const allowedRoles: MembershipRole[] = [MembershipRole.ADMIN, MembershipRole.OWNER];
49+
let hasPermission = true;
50+
51+
for (const teamId of teamIds) {
52+
const userRole = userRoles.get(teamId);
53+
if (!userRole || !allowedRoles.includes(userRole)) {
54+
hasPermission = false;
55+
break;
56+
}
57+
}
58+
59+
return {
60+
hasPermission,
61+
userRoles,
62+
};
63+
}
64+
65+
async validateRemoval(context: RemoveMemberContext, hasPermission: boolean): Promise<void> {
66+
const { userId, memberIds, teamIds, isOrgAdmin } = context;
67+
const isRemovingSelf = memberIds.length === 1 && memberIds[0] === userId;
68+
69+
// Only a team owner can remove another team owner (org admins are exempt)
70+
if (!isOrgAdmin) {
71+
const isAnyMemberOwnerAndCurrentUserNotOwner = await Promise.all(
72+
memberIds.map(async (memberId) => {
73+
const isAnyTeamOwnerAndCurrentUserNotOwner = await Promise.all(
74+
teamIds.map(async (teamId) => {
75+
return (
76+
(await teamQueries.isTeamOwner(memberId, teamId)) &&
77+
!(await teamQueries.isTeamOwner(userId, teamId))
78+
);
79+
})
80+
).then((results) => results.some((result) => result));
81+
82+
return isAnyTeamOwnerAndCurrentUserNotOwner;
83+
})
84+
).then((results) => results.some((result) => result));
85+
86+
if (isAnyMemberOwnerAndCurrentUserNotOwner) {
87+
throw new TRPCError({
88+
code: "UNAUTHORIZED",
89+
message: "Only a team owner can remove another team owner.",
90+
});
91+
}
92+
}
93+
94+
// Check if user is trying to remove themselves from a team they own (prevent this)
95+
if (isRemovingSelf && hasPermission) {
96+
const isOwnerOfAnyTeam = await Promise.all(
97+
teamIds.map(async (teamId) => await teamQueries.isTeamOwner(userId, teamId))
98+
).then((results) => results.some((result) => result));
99+
100+
if (isOwnerOfAnyTeam) {
101+
throw new TRPCError({
102+
code: "FORBIDDEN",
103+
message: "You can not remove yourself from a team you own.",
104+
});
105+
}
106+
}
107+
}
108+
}

‎packages/trpc/server/routers/viewer/teams/removeMember/PBACRemoveMemberService.ts‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
import * as teamQueries from "@calcom/features/ee/teams/lib/queries";
2+
import { PermissionMapper } from "@calcom/features/pbac/domain/mappers/PermissionMapper";
3+
import { Resource, CustomAction } from "@calcom/features/pbac/domain/types/permission-registry";
4+
import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service";
5+
6+
import { TRPCError } from "@trpc/server";
7+
8+
import { BaseRemoveMemberService } from "./BaseRemoveMemberService";
9+
import type { RemoveMemberContext, RemoveMemberPermissionResult } from "./IRemoveMemberService";
10+
11+
export class PBACRemoveMemberService extends BaseRemoveMemberService {
12+
private permissionService = new PermissionCheckService();
13+
14+
async checkRemovePermissions(context: RemoveMemberContext): Promise<RemoveMemberPermissionResult> {
15+
const { userId, teamIds, isOrg } = context;
16+
17+
const resource = isOrg ? Resource.Organization : Resource.Team;
18+
const removePermission = PermissionMapper.toPermissionString({
19+
resource,
20+
action: CustomAction.Remove,
21+
});
22+
23+
const teamsWithPermission = await this.permissionService.getTeamIdsWithPermission(
24+
userId,
25+
removePermission
26+
);
27+
28+
// Convert to Set for O(1) lookup
29+
const teamsWithPermissionSet = new Set(teamsWithPermission);
30+
31+
// Check if user has permission for ALL requested teams
32+
const hasPermission = teamIds.every((teamId) => teamsWithPermissionSet.has(teamId));
33+
34+
return {
35+
hasPermission,
36+
};
37+
}
38+
39+
async validateRemoval(context: RemoveMemberContext, hasPermission: boolean): Promise<void> {
40+
const { userId, memberIds, teamIds } = context;
41+
const isRemovingSelf = memberIds.length === 1 && memberIds[0] === userId;
42+
43+
/**
44+
* TODO: Figure out the best way to prevent someone bricking a team
45+
* by removing all people with updateRole permissions
46+
*/
47+
if (isRemovingSelf && hasPermission) {
48+
const isOwnerOfAnyTeam = await Promise.all(
49+
teamIds.map(async (teamId) => await teamQueries.isTeamOwner(userId, teamId))
50+
).then((results) => results.some((result) => result));
51+
52+
if (isOwnerOfAnyTeam) {
53+
throw new TRPCError({
54+
code: "FORBIDDEN",
55+
message: "You can not remove yourself from a team you own.",
56+
});
57+
}
58+
}
59+
}
60+
}

0 commit comments

Comments
 (0)