diff --git a/packages/features/pbac/domain/repositories/IPermissionRepository.ts b/packages/features/pbac/domain/repositories/IPermissionRepository.ts index 0ecc6f7c0d8535..7c32bf944fb590 100644 --- a/packages/features/pbac/domain/repositories/IPermissionRepository.ts +++ b/packages/features/pbac/domain/repositories/IPermissionRepository.ts @@ -37,6 +37,11 @@ export interface IPermissionRepository { customRoleId: string | null; } | null>; + getTeamById(teamId: number): Promise<{ + id: number; + parentId: number | null; + } | null>; + checkRolePermission(roleId: string, permission: PermissionString): Promise; checkRolePermissions(roleId: string, permissions: PermissionString[]): Promise; diff --git a/packages/features/pbac/infrastructure/repositories/PermissionRepository.ts b/packages/features/pbac/infrastructure/repositories/PermissionRepository.ts index 5fe5435131f62f..425d38bd4f0609 100644 --- a/packages/features/pbac/infrastructure/repositories/PermissionRepository.ts +++ b/packages/features/pbac/infrastructure/repositories/PermissionRepository.ts @@ -94,6 +94,16 @@ export class PermissionRepository implements IPermissionRepository { }); } + async getTeamById(teamId: number) { + return this.client.team.findUnique({ + where: { id: teamId }, + select: { + id: true, + parentId: true, + }, + }); + } + async checkRolePermission(roleId: string, permission: PermissionString): Promise { const { resource, action } = parsePermissionString(permission); const hasPermission = await this.client.rolePermission.findFirst({ diff --git a/packages/features/pbac/services/__tests__/permission-check.service.test.ts b/packages/features/pbac/services/__tests__/permission-check.service.test.ts index 8b1dc954ee4a5c..64bdc5cce579b7 100644 --- a/packages/features/pbac/services/__tests__/permission-check.service.test.ts +++ b/packages/features/pbac/services/__tests__/permission-check.service.test.ts @@ -37,6 +37,7 @@ describe("PermissionCheckService", () => { getMembershipByMembershipId: vi.fn(), getMembershipByUserAndTeam: vi.fn(), getOrgMembership: vi.fn(), + getTeamById: vi.fn(), getUserMemberships: vi.fn(), checkRolePermission: vi.fn(), checkRolePermissions: vi.fn(), @@ -67,28 +68,14 @@ describe("PermissionCheckService", () => { describe("checkPermission", () => { it("should check permission with PBAC enabled", async () => { - const membership = { + mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true); + mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({ id: 1, teamId: 1, userId: 1, - accepted: true, - role: "ADMIN" as MembershipRole, customRoleId: "admin_role", - disableImpersonation: false, - createdAt: new Date(), - updatedAt: new Date(), - }; - - (MembershipRepository.findUniqueByUserIdAndTeamId as Mock).mockResolvedValueOnce(membership); - mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true); - mockRepository.getMembershipByMembershipId.mockResolvedValueOnce({ - id: membership.id, - teamId: membership.teamId, - userId: membership.userId, - customRoleId: membership.customRoleId, team: { parentId: null }, }); - mockRepository.getOrgMembership.mockResolvedValueOnce(null); mockRepository.checkRolePermission.mockResolvedValueOnce(true); const result = await service.checkPermission({ @@ -99,12 +86,8 @@ describe("PermissionCheckService", () => { }); expect(result).toBe(true); - expect(MembershipRepository.findUniqueByUserIdAndTeamId).toHaveBeenCalledWith({ - userId: 1, - teamId: 1, - }); expect(mockFeaturesRepository.checkIfTeamHasFeature).toHaveBeenCalledWith(1, "pbac"); - expect(mockRepository.getMembershipByMembershipId).toHaveBeenCalledWith(1); + expect(mockRepository.getMembershipByUserAndTeam).toHaveBeenCalledWith(1, 1); expect(mockRepository.checkRolePermission).toHaveBeenCalledWith("admin_role", "eventType.create"); }); @@ -140,7 +123,8 @@ describe("PermissionCheckService", () => { expect(mockRepository.checkRolePermission).not.toHaveBeenCalled(); }); - it("should return false if membership not found", async () => { + it("should return false if membership not found when PBAC disabled", async () => { + mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(false); (MembershipRepository.findUniqueByUserIdAndTeamId as Mock).mockResolvedValueOnce(null); const result = await service.checkPermission({ @@ -153,21 +137,47 @@ describe("PermissionCheckService", () => { expect(result).toBe(false); }); - it("should return false if PBAC enabled but no customRoleId", async () => { - const membership = { + it("should check org-level permissions when user has no team membership but PBAC is enabled", async () => { + mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true); + mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce(null); + mockRepository.getTeamById.mockResolvedValueOnce({ id: 1, parentId: 2 }); + mockRepository.getOrgMembership.mockResolvedValueOnce({ + id: 100, + teamId: 2, + userId: 1, + customRoleId: "org_admin_role", + }); + mockRepository.checkRolePermission.mockResolvedValueOnce(true); + + const result = await service.checkPermission({ + userId: 1, + teamId: 1, + permission: "eventType.create", + fallbackRoles: ["ADMIN", "OWNER"], + }); + + expect(result).toBe(true); + expect(mockRepository.getMembershipByUserAndTeam).toHaveBeenCalledWith(1, 1); + expect(mockRepository.getTeamById).toHaveBeenCalledWith(1); + expect(mockRepository.getOrgMembership).toHaveBeenCalledWith(1, 2); + expect(mockRepository.checkRolePermission).toHaveBeenCalledWith("org_admin_role", "eventType.create"); + }); + + it("should return false if PBAC enabled but no customRoleId on team or org", async () => { + mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true); + mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({ id: 1, teamId: 1, userId: 1, - accepted: true, - role: "ADMIN" as MembershipRole, customRoleId: null, - disableImpersonation: false, - createdAt: new Date(), - updatedAt: new Date(), - }; - - (MembershipRepository.findUniqueByUserIdAndTeamId as Mock).mockResolvedValueOnce(membership); - mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true); + team: { parentId: 2 }, + }); + mockRepository.getOrgMembership.mockResolvedValueOnce({ + id: 100, + teamId: 2, + userId: 1, + customRoleId: null, + }); const result = await service.checkPermission({ userId: 1, @@ -182,28 +192,14 @@ describe("PermissionCheckService", () => { describe("checkPermissions", () => { it("should check multiple permissions with PBAC enabled", async () => { - const membership = { + mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true); + mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({ id: 1, teamId: 1, userId: 1, - accepted: true, - role: "ADMIN" as MembershipRole, customRoleId: "admin_role", - disableImpersonation: false, - createdAt: new Date(), - updatedAt: new Date(), - }; - - (MembershipRepository.findUniqueByUserIdAndTeamId as Mock).mockResolvedValueOnce(membership); - mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true); - mockRepository.getMembershipByMembershipId.mockResolvedValueOnce({ - id: membership.id, - teamId: membership.teamId, - userId: membership.userId, - customRoleId: membership.customRoleId, team: { parentId: null }, }); - mockRepository.getOrgMembership.mockResolvedValueOnce(null); mockRepository.checkRolePermissions.mockResolvedValueOnce(true); const result = await service.checkPermissions({ @@ -214,12 +210,8 @@ describe("PermissionCheckService", () => { }); expect(result).toBe(true); - expect(MembershipRepository.findUniqueByUserIdAndTeamId).toHaveBeenCalledWith({ - userId: 1, - teamId: 1, - }); expect(mockFeaturesRepository.checkIfTeamHasFeature).toHaveBeenCalledWith(1, "pbac"); - expect(mockRepository.getMembershipByMembershipId).toHaveBeenCalledWith(1); + expect(mockRepository.getMembershipByUserAndTeam).toHaveBeenCalledWith(1, 1); expect(mockRepository.checkRolePermissions).toHaveBeenCalledWith("admin_role", [ "eventType.create", "team.invite", @@ -259,25 +251,12 @@ describe("PermissionCheckService", () => { }); it("should return false when permissions array is empty", async () => { - const membership = { + mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true); + mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({ id: 1, teamId: 1, userId: 1, - accepted: true, - role: "MEMBER" as MembershipRole, // Change to MEMBER so fallback also fails customRoleId: "admin_role", - disableImpersonation: false, - createdAt: new Date(), - updatedAt: new Date(), - }; - - (MembershipRepository.findUniqueByUserIdAndTeamId as Mock).mockResolvedValueOnce(membership); - mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true); - mockRepository.getMembershipByMembershipId.mockResolvedValueOnce({ - id: membership.id, - teamId: membership.teamId, - userId: membership.userId, - customRoleId: membership.customRoleId, team: { parentId: null }, }); mockRepository.getOrgMembership.mockResolvedValueOnce(null); @@ -293,6 +272,35 @@ describe("PermissionCheckService", () => { expect(result).toBe(false); expect(mockRepository.checkRolePermissions).toHaveBeenCalledWith("admin_role", []); }); + + it("should check org-level permissions when user has no team membership with checkPermissions", async () => { + mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true); + mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce(null); + mockRepository.getTeamById.mockResolvedValueOnce({ id: 1, parentId: 2 }); + mockRepository.getOrgMembership.mockResolvedValueOnce({ + id: 100, + teamId: 2, + userId: 1, + customRoleId: "org_admin_role", + }); + mockRepository.checkRolePermissions.mockResolvedValueOnce(true); + + const result = await service.checkPermissions({ + userId: 1, + teamId: 1, + permissions: ["eventType.create", "team.invite"], + fallbackRoles: ["ADMIN", "OWNER"], + }); + + expect(result).toBe(true); + expect(mockRepository.getMembershipByUserAndTeam).toHaveBeenCalledWith(1, 1); + expect(mockRepository.getTeamById).toHaveBeenCalledWith(1); + expect(mockRepository.getOrgMembership).toHaveBeenCalledWith(1, 2); + expect(mockRepository.checkRolePermissions).toHaveBeenCalledWith("org_admin_role", [ + "eventType.create", + "team.invite", + ]); + }); }); describe("getUserPermissions", () => { @@ -392,6 +400,33 @@ describe("PermissionCheckService", () => { }); describe("getResourcePermissions", () => { + it("should return org permissions when user has no team membership but has org membership", async () => { + mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true); + mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce(null); + mockRepository.getTeamById.mockResolvedValueOnce({ id: 1, parentId: 2 }); + mockRepository.getOrgMembership.mockResolvedValueOnce({ + id: 100, + teamId: 2, + userId: 1, + customRoleId: "org_role", + }); + mockRepository.getResourcePermissionsByRoleId.mockResolvedValueOnce(["create", "read", "update"]); + + const result = await service.getResourcePermissions({ + userId: 1, + teamId: 1, + resource: Resource.EventType, + }); + + expect(result).toEqual(["eventType.create", "eventType.read", "eventType.update"]); + expect(mockRepository.getMembershipByUserAndTeam).toHaveBeenCalledWith(1, 1); + expect(mockRepository.getTeamById).toHaveBeenCalledWith(1); + expect(mockRepository.getOrgMembership).toHaveBeenCalledWith(1, 2); + expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenCalledWith( + "org_role", + Resource.EventType + ); + }); it("should return empty array when PBAC is disabled", async () => { mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(false); diff --git a/packages/features/pbac/services/permission-check.service.ts b/packages/features/pbac/services/permission-check.service.ts index 8b9c67d2e54409..4e0d4986eac3a1 100644 --- a/packages/features/pbac/services/permission-check.service.ts +++ b/packages/features/pbac/services/permission-check.service.ts @@ -71,8 +71,8 @@ export class PermissionCheckService { teamActions.forEach((action) => actions.add(action)); } - // Get org-level permissions as fallback - if (membership?.team?.parentId && orgMembership?.customRoleId) { + // Get org-level permissions (works even without team membership) + if (orgMembership?.customRoleId) { const orgActions = await this.repository.getResourcePermissionsByRoleId( orgMembership.customRoleId, resource @@ -108,27 +108,23 @@ export class PermissionCheckService { return false; } - const membership = await MembershipRepository.findUniqueByUserIdAndTeamId({ - userId, - teamId, - }); - - if (!membership) return false; - const isPBACEnabled = await this.featuresRepository.checkIfTeamHasFeature( teamId, this.PBAC_FEATURE_FLAG ); if (isPBACEnabled) { - if (!membership.customRoleId) { - this.logger.info(`PBAC is enabled for ${teamId} but no custom role is set on membership relation`); - return false; - } - - return this.hasPermission({ membershipId: membership.id }, permission); + // Check if user has permission through team or org membership + return this.hasPermission({ userId, teamId }, permission); } + // Fallback to role-based check only if user has team membership + const membership = await MembershipRepository.findUniqueByUserIdAndTeamId({ + userId, + teamId, + }); + + if (!membership) return false; return this.checkFallbackRoles(membership.role, fallbackRoles); } catch (error) { this.logger.error(error); @@ -157,27 +153,23 @@ export class PermissionCheckService { return false; } - const membership = await MembershipRepository.findUniqueByUserIdAndTeamId({ - userId, - teamId, - }); - - if (!membership) return false; - const isPBACEnabled = await this.featuresRepository.checkIfTeamHasFeature( teamId, this.PBAC_FEATURE_FLAG ); if (isPBACEnabled) { - if (!membership.customRoleId) { - this.logger.info(`PBAC is enabled for ${teamId} but no custom role is set on membership relation`); - return false; - } - - return this.hasPermissions({ membershipId: membership.id }, permissions); + // Check if user has permissions through team or org membership + return this.hasPermissions({ userId, teamId }, permissions); } + // Fallback to role-based check only if user has team membership + const membership = await MembershipRepository.findUniqueByUserIdAndTeamId({ + userId, + teamId, + }); + + if (!membership) return false; return this.checkFallbackRoles(membership.role, fallbackRoles); } catch (error) { this.logger.error(error); @@ -241,8 +233,16 @@ export class PermissionCheckService { membership = await this.repository.getMembershipByUserAndTeam(query.userId, query.teamId); } + // Get org membership either through the team membership or directly from teamId if (membership?.team.parentId) { + // User has team membership, check org through that orgMembership = await this.repository.getOrgMembership(membership.userId, membership.team.parentId); + } else if (query.userId && query.teamId) { + // No team membership, but check if team belongs to an org + const team = await this.repository.getTeamById(query.teamId); + if (team?.parentId) { + orgMembership = await this.repository.getOrgMembership(query.userId, team.parentId); + } } return { membership, orgMembership }; diff --git a/packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts b/packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts index 5350f04b02218e..d1ab682fc2a23a 100644 --- a/packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts +++ b/packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts @@ -2,8 +2,7 @@ import type { z } from "zod"; import { getDefaultLocations } from "@calcom/app-store/_utils/getDefaultLocations"; import { DailyLocationType } from "@calcom/app-store/constants"; -import { Resource } from "@calcom/features/pbac/domain/types/permission-registry"; -import { getResourcePermissions } from "@calcom/features/pbac/lib/resource-permissions"; +import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service"; import { EventTypeRepository } from "@calcom/lib/server/repository/eventTypeRepository"; import type { PrismaClient } from "@calcom/prisma"; import { Prisma } from "@calcom/prisma/client"; @@ -55,27 +54,17 @@ export const createHandler = async ({ ctx, input }: CreateOptions) => { const isManagedEventType = schedulingType === SchedulingType.MANAGED; const isOrgAdmin = !!ctx.user?.organization?.isOrgAdmin; + const permissionService = new PermissionCheckService(); // Check if user has organization-level eventType.create permission (equivalent to org admin for event types) let hasOrgEventTypeCreatePermission = isOrgAdmin; // Default fallback if (ctx.user.organizationId) { - try { - const orgPermissions = await getResourcePermissions({ - userId, - teamId: ctx.user.organizationId, - resource: Resource.EventType, - userRole: isOrgAdmin ? MembershipRole.ADMIN : MembershipRole.MEMBER, - fallbackRoles: { - create: { - roles: [MembershipRole.ADMIN, MembershipRole.OWNER], - }, - }, - }); - hasOrgEventTypeCreatePermission = orgPermissions.canCreate; - } catch (error) { - // If PBAC check fails, fall back to isOrgAdmin - hasOrgEventTypeCreatePermission = isOrgAdmin; - } + hasOrgEventTypeCreatePermission = await permissionService.checkPermission({ + userId, + teamId: ctx.user.organizationId, + permission: "eventType.create", + fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER], + }); } const locations: EventTypeLocation[] = @@ -108,50 +97,20 @@ export const createHandler = async ({ ctx, input }: CreateOptions) => { } if (teamId && schedulingType) { - const hasMembership = await ctx.prisma.membership.findFirst({ - where: { - userId, - teamId: teamId, - accepted: true, - }, - }); - const isSystemAdmin = ctx.user.role === "ADMIN"; - // Initialize team-level permission to false. - // We will only try to set this to true if the user is a member. - let hasCreatePermission = false; - - // Only check for team-level permissions if the user is actually a member of the team. - if (hasMembership) { - try { - const permissions = await getResourcePermissions({ - userId, - teamId, - resource: Resource.EventType, - userRole: hasMembership.role, - fallbackRoles: { - create: { - roles: [MembershipRole.ADMIN, MembershipRole.OWNER], - }, - }, - }); - hasCreatePermission = permissions.canCreate; - } catch (error) { - console.warn( - `PBAC check failed for user ${userId} on team ${teamId}, falling back to role check:`, - error - ); - hasCreatePermission = ["ADMIN", "OWNER"].includes(hasMembership.role); - } - } + // Only check for team-level permissions - this will also check for membership + const hasCreatePermission = await permissionService.checkPermission({ + userId, + teamId, + permission: "eventType.create", + fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER], + }); if (!isSystemAdmin && !hasOrgEventTypeCreatePermission && !hasCreatePermission) { // If none of the above conditions are met, the user is unauthorized. // which means the user is not admin of the team nor the org. - console.warn( - `User ${userId} does not have eventType.create permission for team ${teamId}. Membership found: ${!!hasMembership}` - ); + console.warn(`User ${userId} does not have eventType.create permission for team ${teamId}`); throw new TRPCError({ code: "UNAUTHORIZED" }); }