diff --git a/packages/features/pbac/domain/mappers/RoleMapper.ts b/packages/features/pbac/domain/mappers/RoleMapper.ts deleted file mode 100644 index e3703a72a7..0000000000 --- a/packages/features/pbac/domain/mappers/RoleMapper.ts +++ /dev/null @@ -1,60 +0,0 @@ -import type { RoleType } from "@calcom/kysely/types"; - -import type { Role, RolePermission, RoleType as DomainRoleType } from "../models/Role"; - -type KyselyRole = { - id: string; - name: string; - color: string | null; - description: string | null; - teamId: number | null; - type: RoleType; - createdAt: Date; - updatedAt: Date; - permissions: Array<{ - id: string; - resource: string; - action: string; - }>; -}; - -export class RoleMapper { - static toDomain(kyselyRole: KyselyRole): Role { - return { - id: kyselyRole.id, - name: kyselyRole.name, - description: kyselyRole.description || undefined, - color: kyselyRole.color || undefined, - teamId: kyselyRole.teamId || undefined, - type: kyselyRole.type as DomainRoleType, - permissions: kyselyRole.permissions.map((p) => ({ - id: p.id, - resource: p.resource, - action: p.action, - })), - createdAt: kyselyRole.createdAt, - updatedAt: kyselyRole.updatedAt, - }; - } - - static toKysely(role: Role): Omit { - return { - id: role.id, - name: role.name, - description: role.description || null, - color: role.color || null, - teamId: role.teamId || null, - type: role.type as DomainRoleType, - createdAt: role.createdAt, - updatedAt: role.updatedAt, - }; - } - - static permissionToKysely(roleId: string, permission: RolePermission) { - return { - roleId, - resource: permission.resource, - action: permission.action, - }; - } -} diff --git a/packages/features/pbac/domain/models/Role.ts b/packages/features/pbac/domain/models/Role.ts index dfd2f3d401..f7a1543360 100644 --- a/packages/features/pbac/domain/models/Role.ts +++ b/packages/features/pbac/domain/models/Role.ts @@ -9,8 +9,10 @@ export type RoleType = (typeof RoleType)[keyof typeof RoleType]; export interface RolePermission { id: string; + roleId: string; resource: string; action: string; + createdAt: Date; } export interface Role { @@ -43,3 +45,8 @@ export interface UpdateRolePermissionsData { name?: string; }; } + +export type PermissionChange = { + resource: string; + action: string; +}; diff --git a/packages/features/pbac/domain/repositories/IRoleRepository.ts b/packages/features/pbac/domain/repositories/IRoleRepository.ts index b282d50d3f..a52e5f0831 100644 --- a/packages/features/pbac/domain/repositories/IRoleRepository.ts +++ b/packages/features/pbac/domain/repositories/IRoleRepository.ts @@ -1,9 +1,4 @@ -import type { Transaction } from "kysely"; - -import type { DB } from "@calcom/kysely"; - -import type { Role, CreateRoleData } from "../models/Role"; -import type { PermissionString } from "../types/permission-registry"; +import type { Role, CreateRoleData, RolePermission, PermissionChange } from "../models/Role"; export interface IRoleRepository { findByName(name: string, teamId?: number): Promise; @@ -14,12 +9,15 @@ export interface IRoleRepository { delete(id: string): Promise; update( roleId: string, - permissions: PermissionString[], + permissionChanges: { + toAdd: PermissionChange[]; + toRemove: PermissionChange[]; + }, updates?: { color?: string; name?: string; description?: string; } ): Promise; - transaction(callback: (repository: IRoleRepository, trx: Transaction) => Promise): Promise; + getPermissions(roleId: string): Promise; } diff --git a/packages/features/pbac/infrastructure/mappers/RoleOutputMapper.ts b/packages/features/pbac/infrastructure/mappers/RoleOutputMapper.ts new file mode 100644 index 0000000000..32f565c82b --- /dev/null +++ b/packages/features/pbac/infrastructure/mappers/RoleOutputMapper.ts @@ -0,0 +1,33 @@ +import type { Role as PrismaRole, RolePermission as PrismaRolePermission } from "@calcom/prisma/client"; + +import type { Role, RolePermission } from "../../domain/models/Role"; + +export class RoleOutputMapper { + static toDomain(prismaRole: PrismaRole & { permissions: PrismaRolePermission[] }): Role { + return { + id: prismaRole.id, + name: prismaRole.name, + color: prismaRole.color || undefined, + description: prismaRole.description || undefined, + teamId: prismaRole.teamId || undefined, + type: prismaRole.type, + permissions: prismaRole.permissions.map(this.toDomainPermission), + createdAt: prismaRole.createdAt, + updatedAt: prismaRole.updatedAt, + }; + } + + static toDomainPermission(prismaPermission: PrismaRolePermission): RolePermission { + return { + id: prismaPermission.id, + roleId: prismaPermission.roleId, + resource: prismaPermission.resource, + action: prismaPermission.action, + createdAt: prismaPermission.createdAt, + }; + } + + static toDomainList(prismaRoles: (PrismaRole & { permissions: PrismaRolePermission[] })[]): Role[] { + return prismaRoles.map(this.toDomain); + } +} diff --git a/packages/features/pbac/infrastructure/repositories/PermissionRepository.ts b/packages/features/pbac/infrastructure/repositories/PermissionRepository.ts index 02e3e798b5..a65bbe38bc 100644 --- a/packages/features/pbac/infrastructure/repositories/PermissionRepository.ts +++ b/packages/features/pbac/infrastructure/repositories/PermissionRepository.ts @@ -1,4 +1,5 @@ -import kysely from "@calcom/kysely"; +import db from "@calcom/prisma"; +import type { PrismaClient as PrismaClientWithExtensions } from "@calcom/prisma"; import { PermissionMapper } from "../../domain/mappers/PermissionMapper"; import type { TeamPermissions } from "../../domain/models/Permission"; @@ -7,114 +8,101 @@ import type { CrudAction, CustomAction } from "../../domain/types/permission-reg import { Resource, type PermissionString } from "../../domain/types/permission-registry"; export class PermissionRepository implements IPermissionRepository { + private client: PrismaClientWithExtensions; + + constructor(client: PrismaClientWithExtensions = db) { + this.client = client; + } + async getUserMemberships(userId: number): Promise { - const memberships = await kysely - .selectFrom("Membership") - .innerJoin("Role", "Role.id", "Membership.customRoleId") - .leftJoin("RolePermission", "RolePermission.roleId", "Role.id") - .select(["Membership.teamId", "Role.id as roleId", "RolePermission.resource", "RolePermission.action"]) - .where("Membership.userId", "=", userId) - .execute(); - - // Group permissions by teamId and roleId - const membershipsWithPermissions = memberships.reduce((acc, membership) => { - const key = `${membership.teamId}-${membership.roleId}`; - if (!acc[key]) { - acc[key] = { - teamId: membership.teamId, - role: { - id: membership.roleId, - permissions: [], + const memberships = await this.client.membership.findMany({ + where: { userId }, + include: { + customRole: { + include: { + permissions: true, }, - }; - } - if (membership.resource && membership.action) { - acc[key].role.permissions.push({ - resource: membership.resource, - action: membership.action, - }); - } - return acc; - }, {} as Record); - - return PermissionMapper.toDomain(Object.values(membershipsWithPermissions)); + }, + }, + }); + // Map to expected structure for PermissionMapper + const mapped = memberships.map((membership) => ({ + teamId: membership.teamId, + role: membership.customRole + ? { + id: membership.customRole.id, + permissions: membership.customRole.permissions, + } + : null, + })); + return PermissionMapper.toDomain(mapped); } async getMembershipByMembershipId(membershipId: number) { - const result = await kysely - .selectFrom("Membership") - .leftJoin("Team", "Team.id", "Membership.teamId") - .select([ - "Membership.id", - "Membership.teamId", - "Membership.userId", - "Membership.customRoleId", - "Team.parentId as team_parentId", - ]) - .where("Membership.id", "=", membershipId) - .executeTakeFirst(); - - if (!result) return null; - - return { - ...result, - team_parentId: result.team_parentId || undefined, - }; + return this.client.membership.findUnique({ + where: { id: membershipId }, + select: { + id: true, + teamId: true, + userId: true, + customRoleId: true, + team: { + select: { + parentId: true, + }, + }, + }, + }); } async getMembershipByUserAndTeam(userId: number, teamId: number) { - const result = await kysely - .selectFrom("Membership") - .leftJoin("Team", "Team.id", "Membership.teamId") - .select([ - "Membership.id", - "Membership.teamId", - "Membership.userId", - "Membership.customRoleId", - "Team.parentId as team_parentId", - ]) - .where("Membership.userId", "=", userId) - .where("Membership.teamId", "=", teamId) - .executeTakeFirst(); - - if (!result) return null; - - return { - ...result, - team_parentId: result.team_parentId || undefined, - }; + return this.client.membership.findFirst({ + where: { + userId, + teamId, + }, + select: { + id: true, + teamId: true, + userId: true, + customRoleId: true, + team: { + select: { + parentId: true, + }, + }, + }, + }); } async getOrgMembership(userId: number, orgId: number) { - const result = await kysely - .selectFrom("Membership") - .select(["id", "teamId", "userId", "customRoleId"]) - .where("userId", "=", userId) - .where("teamId", "=", orgId) - .executeTakeFirst(); - - return result || null; + return this.client.membership.findFirst({ + where: { + userId, + teamId: orgId, + }, + select: { + id: true, + teamId: true, + userId: true, + customRoleId: true, + }, + }); } async checkRolePermission(roleId: string, permission: PermissionString): Promise { const [resource, action] = permission.split("."); - const hasPermission = await kysely - .selectFrom("RolePermission") - .select("id") - .where("roleId", "=", roleId) - .where((eb) => - eb.or([ - // Global wildcard - eb.and([eb("resource", "=", "*"), eb("action", "=", "*")]), - // Resource wildcard - eb.and([eb("resource", "=", "*"), eb("action", "=", action)]), - // Action wildcard - eb.and([eb("resource", "=", resource), eb("action", "=", "*")]), - // Exact match - eb.and([eb("resource", "=", resource), eb("action", "=", action)]), - ]) - ) - .executeTakeFirst(); + const hasPermission = await this.client.rolePermission.findFirst({ + where: { + roleId, + OR: [ + { resource: "*", action: "*" }, + { resource: "*", action }, + { resource, action: "*" }, + { resource, action }, + ], + }, + }); return !!hasPermission; } @@ -123,42 +111,38 @@ export class PermissionRepository implements IPermissionRepository { const [resource, action] = p.split("."); return { resource, action }; }); + const resourceActions = permissionPairs.map((p) => [p.resource, p.action]); + const resources = permissionPairs.map((p) => p.resource); + const actions = permissionPairs.map((p) => p.action); - const matchingPermissionsCount = await kysely - .selectFrom("RolePermission") - .select((eb) => eb.fn.countAll().as("count")) - .where("roleId", "=", roleId) - .where((eb) => - eb.or([ - // Global wildcard - eb.and([eb("resource", "=", "*"), eb("action", "=", "*")]), - // Resource wildcards - eb.and([ - eb("resource", "=", "*"), - eb( - "action", - "in", - permissionPairs.map((p) => p.action) - ), - ]), - // Action wildcards - eb.and([ - eb( - "resource", - "in", - permissionPairs.map((p) => p.resource) - ), - eb("action", "=", "*"), - ]), - // Exact matches - eb.or( - permissionPairs.map((p) => eb.and([eb("resource", "=", p.resource), eb("action", "=", p.action)])) - ), - ]) + const matchingPermissions = await this.client.$queryRaw<[{ count: bigint }]>` + WITH permission_checks AS ( + -- Universal permission (*,*) + SELECT 1 as match FROM "RolePermission" + WHERE "roleId" = ${roleId} AND "resource" = '*' AND "action" = '*' + + UNION ALL + + -- Wildcard resource with specific actions + SELECT 1 as match FROM "RolePermission" + WHERE "roleId" = ${roleId} AND "resource" = '*' AND "action" = ANY(${actions}) + + UNION ALL + + -- Specific resources with wildcard action + SELECT 1 as match FROM "RolePermission" + WHERE "roleId" = ${roleId} AND "action" = '*' AND "resource" = ANY(${resources}) + + UNION ALL + + -- Exact resource-action pairs + SELECT 1 as match FROM "RolePermission" + WHERE "roleId" = ${roleId} AND ("resource", "action") = ANY(${resourceActions}) ) - .executeTakeFirstOrThrow(); + SELECT COUNT(*) as count FROM permission_checks + `; - return Number(matchingPermissionsCount.count) >= permissions.length; + return Number(matchingPermissions[0].count) >= permissions.length; } async getResourcePermissions( @@ -167,22 +151,26 @@ export class PermissionRepository implements IPermissionRepository { resource: Resource ): Promise<(CrudAction | CustomAction)[]> { // Get team-level permissions - const membership = await kysely - .selectFrom("Membership") - .select("customRoleId") - .where("userId", "=", userId) - .where("teamId", "=", teamId) - .executeTakeFirst(); - + const membership = await this.client.membership.findFirst({ + where: { + userId, + teamId, + }, + select: { + customRoleId: true, + }, + }); if (!membership?.customRoleId) return []; - - const teamPermissions = await kysely - .selectFrom("RolePermission") - .select(["action", "resource"]) - .where("roleId", "=", membership.customRoleId) - .where((eb) => eb.or([eb("resource", "=", resource), eb("resource", "=", Resource.All)])) - .execute(); - + const teamPermissions = await this.client.rolePermission.findMany({ + where: { + roleId: membership.customRoleId, + OR: [{ resource }, { resource: Resource.All }], + }, + select: { + action: true, + resource: true, + }, + }); return teamPermissions.map((p) => p.action as CrudAction | CustomAction); } } diff --git a/packages/features/pbac/infrastructure/repositories/RoleRepository.ts b/packages/features/pbac/infrastructure/repositories/RoleRepository.ts index 9be2ce5206..f37037956c 100644 --- a/packages/features/pbac/infrastructure/repositories/RoleRepository.ts +++ b/packages/features/pbac/infrastructure/repositories/RoleRepository.ts @@ -1,208 +1,149 @@ -import type { Transaction } from "kysely"; -import { jsonArrayFrom } from "kysely/helpers/postgres"; import { v4 as uuidv4 } from "uuid"; -import type { DB } from "@calcom/kysely"; -import kysely from "@calcom/kysely"; +import type { PrismaClient as PrismaWithExtensions } from "@calcom/prisma"; +import db from "@calcom/prisma"; -import { RoleMapper } from "../../domain/mappers/RoleMapper"; +import type { Role, RolePermission, PermissionChange, CreateRoleData } from "../../domain/models/Role"; import { RoleType } from "../../domain/models/Role"; -import type { CreateRoleData } from "../../domain/models/Role"; -import type { IRoleRepository } from "../../domain/repositories/IRoleRepository"; -import type { PermissionString } from "../../domain/types/permission-registry"; +import { RoleOutputMapper } from "../mappers/RoleOutputMapper"; -type KyselyRole = { - id: string; - name: string; - color: string | null; - description: string | null; - teamId: number | null; - type: RoleType; - createdAt: Date; - updatedAt: Date; - permissions: Array<{ - id: string; - resource: string; - action: string; - }>; -}; +export class RoleRepository { + constructor(private readonly client: PrismaWithExtensions = db) {} -export class RoleRepository implements IRoleRepository { - private getRoleSelect() { - return [ - "Role.id", - "Role.name", - "Role.description", - "Role.teamId", - "Role.type", - "Role.color", - "Role.createdAt", - "Role.updatedAt", - (eb: any) => - jsonArrayFrom( - eb - .selectFrom("RolePermission") - .select(["id", "resource", "action"]) - .whereRef("RolePermission.roleId", "=", "Role.id") - ).as("permissions"), - ] as const; + async findByName(name: string, teamId?: number): Promise { + const role = await this.client.role.findFirst({ + where: { name, teamId: teamId ?? null }, + include: { permissions: true }, + }); + return role ? RoleOutputMapper.toDomain(role) : null; } - async findByName(name: string, teamId?: number) { - const role = await kysely - .selectFrom("Role") - .select(this.getRoleSelect()) - .where("name", "=", name) - .where((eb) => (teamId ? eb("teamId", "=", teamId) : eb("teamId", "is", null))) - .executeTakeFirst(); - - return role ? RoleMapper.toDomain(role as KyselyRole) : null; + async findByTeamId(teamId: number): Promise { + const roles = await this.client.role.findMany({ + where: { teamId }, + include: { permissions: true }, + }); + return RoleOutputMapper.toDomainList(roles); } - async findById(id: string) { - const role = await kysely - .selectFrom("Role") - .select(this.getRoleSelect()) - .where("id", "=", id) - .executeTakeFirst(); - - return role ? RoleMapper.toDomain(role as KyselyRole) : null; + async roleBelongsToTeam(roleId: string, teamId: number): Promise { + const role = await this.client.role.findUnique({ + where: { id: roleId }, + select: { teamId: true }, + }); + return role?.teamId === teamId; } - async findByTeamId(teamId: number) { - const roles = await kysely - .selectFrom("Role") - .select(this.getRoleSelect()) - .where((eb) => eb.or([eb("Role.teamId", "=", teamId), eb("Role.type", "=", RoleType.SYSTEM)])) - .execute(); - - return roles.map((role) => RoleMapper.toDomain(role as KyselyRole)); - } - - async create(data: CreateRoleData) { - const roleId = uuidv4(); - - await kysely.transaction().execute(async (trx) => { - // Create role - await trx - .insertInto("Role") - .values({ - id: roleId, + async create(data: CreateRoleData): Promise { + const roleId = await this.client.$transaction(async (trx) => { + const role = await trx.role.create({ + data: { + id: uuidv4(), name: data.name, - description: data.description || null, - teamId: data.teamId || null, - type: data.type || RoleType.CUSTOM, - color: data.color || null, - createdAt: new Date(), - updatedAt: new Date(), - }) - .execute(); + color: data.color ?? null, + description: data.description ?? null, + teamId: data.teamId ?? null, + type: data.type ?? RoleType.CUSTOM, + }, + }); - // Create permissions only if there are any if (data.permissions.length > 0) { const permissionData = data.permissions.map((permission) => { const [resource, action] = permission.split("."); return { id: uuidv4(), - roleId, + roleId: role.id, resource, action, }; }); - await trx.insertInto("RolePermission").values(permissionData).execute(); + await trx.rolePermission.createMany({ data: permissionData }); } - - return roleId; + return role.id; }); - // Fetch complete role with permissions const completeRole = await this.findById(roleId); - - // This should never happen if (!completeRole) throw new Error("Failed to create role"); return completeRole; } - async delete(id: string) { - await kysely.transaction().execute(async (trx) => { - await trx.deleteFrom("RolePermission").where("roleId", "=", id).execute(); - await trx.deleteFrom("Role").where("id", "=", id).execute(); - }); + async delete(id: string): Promise { + await this.client.$transaction([ + this.client.rolePermission.deleteMany({ where: { roleId: id } }), + this.client.role.delete({ where: { id } }), + ]); } async update( roleId: string, - permissions: PermissionString[], + permissionChanges: { + toAdd: PermissionChange[]; + toRemove: PermissionChange[]; + }, updates?: { color?: string; name?: string; description?: string; } ) { - await kysely.transaction().execute(async (trx) => { + await this.client.$transaction(async (trx) => { // Update role metadata if provided if (updates) { - const updateData: Partial> = {}; - - if (updates.color !== undefined) { - updateData.color = updates.color || null; - } - if (updates.name !== undefined) { - updateData.name = updates.name; - } - if (updates.description !== undefined) { - updateData.description = updates.description || null; - } - - await trx.updateTable("Role").set(updateData).where("id", "=", roleId).execute(); + const updateData: Record = {}; + if (updates.color !== undefined) updateData.color = updates.color || null; + if (updates.name !== undefined) updateData.name = updates.name; + if (updates.description !== undefined) updateData.description = updates.description || null; + await trx.role.update({ where: { id: roleId }, data: updateData }); } - // Delete existing permissions - await trx.deleteFrom("RolePermission").where("roleId", "=", roleId).execute(); + // Remove permissions that are no longer needed + if (permissionChanges.toRemove.length > 0) { + await trx.rolePermission.deleteMany({ + where: { + roleId, + AND: permissionChanges.toRemove.map((p) => ({ + resource: p.resource, + action: p.action, + })), + }, + }); + } - // Create new permissions - const permissionData = permissions.map((permission) => { - const [resource, action] = permission.split("."); - return { + // Add new permissions + if (permissionChanges.toAdd.length > 0) { + const permissionData = permissionChanges.toAdd.map((permission) => ({ id: uuidv4(), roleId, - resource, - action, - }; - }); + resource: permission.resource, + action: permission.action, + })); - await trx.insertInto("RolePermission").values(permissionData).execute(); - - return roleId; + await trx.rolePermission.createMany({ data: permissionData }); + } }); - // Fetch updated role - const updatedRole = await kysely - .selectFrom("Role") - .select(this.getRoleSelect()) - .where("id", "=", roleId) - .executeTakeFirst(); - - if (!updatedRole) throw new Error("Failed to update role permissions"); - - return RoleMapper.toDomain(updatedRole as KyselyRole); + // Fetch complete role with permissions + const completeRole = await this.findById(roleId); + if (!completeRole) throw new Error("Failed to update role"); + return completeRole; } - async transaction( - callback: (repository: IRoleRepository, trx: Transaction) => Promise - ): Promise { - return kysely.transaction().execute(async (trx) => { - // Create a new repository instance with the transaction connection - const transactionRepo = new RoleRepository(); - // Pass both the repository and the transaction connection - return callback(transactionRepo, trx); + async findById(roleId: string): Promise { + const role = await this.client.role.findUnique({ + where: { id: roleId }, + include: { + permissions: true, + }, }); + return role ? RoleOutputMapper.toDomain(role) : null; } - async roleBelongsToTeam(roleId: string, teamId: number) { - const role = await this.findById(roleId); - return role?.teamId === teamId; + async getPermissions(roleId: string): Promise { + const permissions = await this.client.rolePermission.findMany({ + where: { roleId }, + }); + return permissions.map(RoleOutputMapper.toDomainPermission); } } diff --git a/packages/features/pbac/services/__tests__/permission-diff.service.test.ts b/packages/features/pbac/services/__tests__/permission-diff.service.test.ts new file mode 100644 index 0000000000..69dd4d3dd8 --- /dev/null +++ b/packages/features/pbac/services/__tests__/permission-diff.service.test.ts @@ -0,0 +1,78 @@ +import type { RolePermission } from "@prisma/client"; +import { describe, it, expect } from "vitest"; + +import { PermissionDiffService } from "../permission-diff.service"; + +describe("PermissionDiffService", () => { + const service = new PermissionDiffService(); + + describe("calculateDiff", () => { + it("should calculate permissions to add and remove", () => { + const existingPermissions = [ + { id: "1", roleId: "role1", resource: "booking", action: "create" }, + { id: "2", roleId: "role1", resource: "booking", action: "read" }, + ] as RolePermission[]; + + const newPermissions = ["booking.read", "booking.update", "event.create"]; + + const result = service.calculateDiff(newPermissions, existingPermissions); + + expect(result.toAdd).toEqual([ + { resource: "booking", action: "update" }, + { resource: "event", action: "create" }, + ]); + + expect(result.toRemove).toEqual([{ id: "1", roleId: "role1", resource: "booking", action: "create" }]); + }); + + it("should handle empty new permissions", () => { + const existingPermissions = [ + { id: "1", roleId: "role1", resource: "booking", action: "create" }, + ] as RolePermission[]; + + const result = service.calculateDiff([], existingPermissions); + + expect(result.toAdd).toEqual([]); + expect(result.toRemove).toEqual(existingPermissions); + }); + + it("should handle empty existing permissions", () => { + const newPermissions = ["booking.create", "booking.read"]; + + const result = service.calculateDiff(newPermissions, []); + + expect(result.toAdd).toEqual([ + { resource: "booking", action: "create" }, + { resource: "booking", action: "read" }, + ]); + expect(result.toRemove).toEqual([]); + }); + + it("should filter out internal _resource permissions", () => { + const existingPermissions = [ + { id: "1", roleId: "role1", resource: "booking", action: "create" }, + ] as RolePermission[]; + + const newPermissions = ["booking.create", "booking._resource", "event.create"]; + + const result = service.calculateDiff(newPermissions, existingPermissions); + + expect(result.toAdd).toEqual([{ resource: "event", action: "create" }]); + expect(result.toRemove).toEqual([]); + }); + + it("should handle identical permissions with no changes needed", () => { + const existingPermissions = [ + { id: "1", roleId: "role1", resource: "booking", action: "create" }, + { id: "2", roleId: "role1", resource: "booking", action: "read" }, + ] as RolePermission[]; + + const newPermissions = ["booking.create", "booking.read"]; + + const result = service.calculateDiff(newPermissions, existingPermissions); + + expect(result.toAdd).toEqual([]); + expect(result.toRemove).toEqual([]); + }); + }); +}); diff --git a/packages/features/pbac/services/__tests__/role.service.test.ts b/packages/features/pbac/services/__tests__/role.service.test.ts index 800ccb18dd..4eeda76e0a 100644 --- a/packages/features/pbac/services/__tests__/role.service.test.ts +++ b/packages/features/pbac/services/__tests__/role.service.test.ts @@ -5,9 +5,19 @@ import { RoleType } from "@calcom/prisma/enums"; import type { Role } from "../../domain/models/Role"; import type { IRoleRepository } from "../../domain/repositories/IRoleRepository"; import type { PermissionString } from "../../domain/types/permission-registry"; +import type { PermissionDiffService } from "../permission-diff.service"; import { RoleService } from "../role.service"; vi.mock("../../infrastructure/repositories/RoleRepository"); +vi.mock("../permission-diff.service"); + +// Mock db.$transaction +vi.mock("@calcom/prisma", () => ({ + default: { + $transaction: vi.fn((cb) => cb({ membership: { update: vi.fn() } })), + membership: { update: vi.fn() }, + }, +})); type MockRepository = { [K in keyof IRoleRepository]: Mock; @@ -16,6 +26,7 @@ type MockRepository = { describe("RoleService", () => { let service: RoleService; let mockRepository: MockRepository; + let mockPermissionDiffService: { calculateDiff: Mock }; beforeEach(() => { vi.clearAllMocks(); @@ -26,10 +37,19 @@ describe("RoleService", () => { create: vi.fn(), delete: vi.fn(), update: vi.fn(), - transaction: vi.fn(), roleBelongsToTeam: vi.fn(), + getPermissions: vi.fn(), }; - service = new RoleService(mockRepository); + + mockPermissionDiffService = { + calculateDiff: vi.fn(), + }; + + service = new RoleService( + mockRepository, + undefined, // permissionService is not mocked as it's not relevant for these tests + mockPermissionDiffService as unknown as PermissionDiffService + ); }); describe("createRole", () => { @@ -153,9 +173,18 @@ describe("RoleService", () => { }); describe("update", () => { - it("should update role permissions", async () => { + it("should update role permissions using permission diff", async () => { const roleId = "role-id"; const permissions = ["eventType.create", "eventType.read"] as PermissionString[]; + const existingPermissions = [{ id: "1", roleId, resource: "eventType", action: "delete" }]; + + const permissionChanges = { + toAdd: [ + { resource: "eventType", action: "create" }, + { resource: "eventType", action: "read" }, + ], + toRemove: [{ id: "1", roleId, resource: "eventType", action: "delete" }], + }; const role: Role = { id: roleId, @@ -172,14 +201,18 @@ describe("RoleService", () => { }; mockRepository.findById.mockResolvedValueOnce(role); + mockRepository.getPermissions.mockResolvedValueOnce(existingPermissions); + mockPermissionDiffService.calculateDiff.mockReturnValueOnce(permissionChanges); mockRepository.update.mockResolvedValueOnce(role); const result = await service.update({ roleId, permissions }); - expect(result).toBeDefined(); - expect(mockRepository.update).toHaveBeenCalledWith(roleId, permissions, { + + expect(mockPermissionDiffService.calculateDiff).toHaveBeenCalledWith(permissions, existingPermissions); + expect(mockRepository.update).toHaveBeenCalledWith(roleId, permissionChanges, { color: undefined, name: undefined, }); + expect(result).toBeDefined(); }); it("should throw error if role does not exist", async () => { @@ -277,45 +310,18 @@ describe("RoleService", () => { updatedAt: new Date(), }; - const mockTrx = { - selectFrom: vi.fn().mockReturnThis(), - select: vi.fn().mockReturnThis(), - where: vi.fn().mockReturnThis(), - executeTakeFirst: vi.fn().mockResolvedValue({ id: roleId }), - updateTable: vi.fn().mockReturnThis(), - set: vi.fn().mockReturnThis(), - execute: vi.fn().mockResolvedValue(undefined), - }; - - mockRepository.transaction.mockImplementationOnce((callback) => callback(mockRepository, mockTrx)); + mockRepository.findById.mockResolvedValueOnce(role); const result = await service.assignRoleToMember(roleId, membershipId); - expect(result).toEqual({ id: roleId }); - expect(mockTrx.selectFrom).toHaveBeenCalledWith("Role"); - expect(mockTrx.select).toHaveBeenCalledWith("id"); - expect(mockTrx.where).toHaveBeenCalledWith("id", "=", roleId); - expect(mockTrx.executeTakeFirst).toHaveBeenCalled(); - expect(mockTrx.updateTable).toHaveBeenCalledWith("Membership"); - expect(mockTrx.set).toHaveBeenCalledWith({ customRoleId: roleId }); - expect(mockTrx.where).toHaveBeenCalledWith("id", "=", membershipId); - expect(mockTrx.execute).toHaveBeenCalled(); + expect(result).toEqual(role); // Check the full role object }); it("should throw error if role does not exist", async () => { - const mockTrx = { - selectFrom: vi.fn().mockReturnThis(), - select: vi.fn().mockReturnThis(), - where: vi.fn().mockReturnThis(), - executeTakeFirst: vi.fn().mockResolvedValue(null), - }; - - mockRepository.transaction.mockImplementationOnce((callback) => callback(mockRepository, mockTrx)); + mockRepository.findById.mockResolvedValueOnce(null); await expect(service.assignRoleToMember(roleId, membershipId)).rejects.toThrow("Role not found"); - expect(mockTrx.selectFrom).toHaveBeenCalledWith("Role"); - expect(mockTrx.select).toHaveBeenCalledWith("id"); - expect(mockTrx.where).toHaveBeenCalledWith("id", "=", roleId); - expect(mockTrx.executeTakeFirst).toHaveBeenCalled(); + expect(mockRepository.findById).toHaveBeenCalledWith(roleId); + // Do NOT expect transaction methods to be called, since the repository returns null }); }); }); diff --git a/packages/features/pbac/services/permission-diff.service.ts b/packages/features/pbac/services/permission-diff.service.ts new file mode 100644 index 0000000000..d7c13b367d --- /dev/null +++ b/packages/features/pbac/services/permission-diff.service.ts @@ -0,0 +1,48 @@ +import type { RolePermission } from "../domain/models/Role"; + +export type PermissionString = string; +export type ParsedPermission = { resource: string; action: string }; + +export class PermissionDiffService { + private parsePermission(permission: PermissionString): ParsedPermission { + const [resource, action] = permission.split("."); + return { resource, action }; + } + + private filterInternalPermissions(permissions: PermissionString[]): PermissionString[] { + return permissions.filter((permission) => { + const { action } = this.parsePermission(permission); + return action !== "_resource"; + }); + } + + private createPermissionKey(permission: ParsedPermission): string { + return `${permission.resource}:${permission.action}`; + } + + calculateDiff(newPermissions: PermissionString[], existingPermissions: RolePermission[]) { + const filteredPermissions = this.filterInternalPermissions(newPermissions); + + // Create sets for comparison + const existingSet = new Set(existingPermissions.map((p) => this.createPermissionKey(p))); + + const newSet = new Set( + filteredPermissions.map((p) => { + const parsed = this.parsePermission(p); + return this.createPermissionKey(parsed); + }) + ); + + // Calculate permissions to add and remove + const toAdd = filteredPermissions + .map((p) => this.parsePermission(p)) + .filter((p) => !existingSet.has(this.createPermissionKey(p))); + + const toRemove = existingPermissions.filter((p) => !newSet.has(this.createPermissionKey(p))); + + return { + toAdd, + toRemove, + }; + } +} diff --git a/packages/features/pbac/services/role.service.ts b/packages/features/pbac/services/role.service.ts index 6ec2bb45de..dd7313c306 100644 --- a/packages/features/pbac/services/role.service.ts +++ b/packages/features/pbac/services/role.service.ts @@ -1,4 +1,4 @@ -import kysely from "@calcom/kysely"; +import db from "@calcom/prisma"; import type { MembershipRole } from "@calcom/prisma/enums"; import { RoleType as DomainRoleType } from "../domain/models/Role"; @@ -6,14 +6,14 @@ import type { CreateRoleData, UpdateRolePermissionsData } from "../domain/models import type { IRoleRepository } from "../domain/repositories/IRoleRepository"; import { RoleRepository } from "../infrastructure/repositories/RoleRepository"; import { DEFAULT_ROLE_IDS } from "../lib/constants"; +import { PermissionDiffService } from "./permission-diff.service"; import { PermissionService } from "./permission.service"; -// These IDs must match the ones in the migration - export class RoleService { constructor( private readonly repository: IRoleRepository = new RoleRepository(), - private readonly permissionService: PermissionService = new PermissionService() + private readonly permissionService: PermissionService = new PermissionService(), + private readonly permissionDiffService: PermissionDiffService = new PermissionDiffService() ) {} async createRole(data: CreateRoleData) { @@ -37,19 +37,13 @@ export class RoleService { } async assignRoleToMember(roleId: string, membershipId: number) { - return this.repository.transaction(async (repo, trx) => { - const role = await trx.selectFrom("Role").select("id").where("id", "=", roleId).executeTakeFirst(); - if (!role) throw new Error("Role not found"); - - // TODO: Move this to a MembershipRepository - bit difficult due to the trx record here. - await trx - .updateTable("Membership") - .set({ customRoleId: roleId }) - .where("id", "=", membershipId) - .execute(); - - return role; + const role = await this.repository.findById(roleId); + if (!role) throw new Error("Role not found"); + await db.membership.update({ + where: { id: membershipId }, + data: { customRoleId: roleId }, }); + return role; } async getRolePermissions(roleId: string) { @@ -58,12 +52,10 @@ export class RoleService { } async removeRoleFromMember(membershipId: number) { - // TODO: Move this to a MembershipRepository - await kysely - .updateTable("Membership") - .set({ customRoleId: null }) - .where("id", "=", membershipId) - .execute(); + await db.membership.update({ + where: { id: membershipId }, + data: { customRoleId: null }, + }); } async getRole(roleId: string) { @@ -79,12 +71,10 @@ export class RoleService { if (!role) { throw new Error("Role not found"); } - // Don't allow deleting default roles if (role.type === DomainRoleType.SYSTEM) { throw new Error("Cannot delete default roles"); } - await this.repository.delete(roleId); } @@ -93,19 +83,20 @@ export class RoleService { if (!role) { throw new Error("Role not found"); } - // Don't allow updating default roles if (role.type === DomainRoleType.SYSTEM) { throw new Error("Cannot update default roles"); } - // Validate permissions const validationResult = this.permissionService.validatePermissions(data.permissions); if (!validationResult.isValid) { throw new Error(validationResult.error || "Invalid permissions provided"); } - return this.repository.update(data.roleId, data.permissions, { + const existingPermissions = await this.repository.getPermissions(data.roleId); + const permissionChanges = this.permissionDiffService.calculateDiff(data.permissions, existingPermissions); + + return this.repository.update(data.roleId, permissionChanges, { color: data.updates?.color, name: data.updates?.name, });