diff --git a/packages/twenty-server/src/engine/metadata-modules/ai/ai-agent-role/agent-role.service.spec.ts b/packages/twenty-server/src/engine/metadata-modules/ai/ai-agent-role/agent-role.service.spec.ts index 9c6bf6725a..a53aeb6f74 100644 --- a/packages/twenty-server/src/engine/metadata-modules/ai/ai-agent-role/agent-role.service.spec.ts +++ b/packages/twenty-server/src/engine/metadata-modules/ai/ai-agent-role/agent-role.service.spec.ts @@ -12,6 +12,7 @@ import { RoleTargetService } from 'src/engine/metadata-modules/role-target/servi import { RoleEntity } from 'src/engine/metadata-modules/role/role.entity'; import { getWorkspaceScopedRepositoryToken } from 'src/engine/twenty-orm/workspace-scoped-repository/get-workspace-scoped-repository-token.util'; import { type WorkspaceScopedRepository } from 'src/engine/twenty-orm/workspace-scoped-repository/workspace-scoped-repository'; +import { WorkspaceCacheService } from 'src/engine/workspace-cache/services/workspace-cache.service'; import { AiAgentRoleService } from './ai-agent-role.service'; describe('AiAgentRoleService', () => { @@ -20,6 +21,7 @@ describe('AiAgentRoleService', () => { let roleRepository: WorkspaceScopedRepository; let roleTargetRepository: WorkspaceScopedRepository; let roleTargetService: RoleTargetService; + let workspaceCacheService: WorkspaceCacheService; const testWorkspaceId = 'test-workspace-id'; let testAgent: AgentEntity; @@ -43,6 +45,7 @@ describe('AiAgentRoleService', () => { useValue: { findOne: jest.fn(), save: jest.fn(), + delete: jest.fn(), }, }, { @@ -62,6 +65,12 @@ describe('AiAgentRoleService', () => { delete: jest.fn(), }, }, + { + provide: WorkspaceCacheService, + useValue: { + invalidateAndRecompute: jest.fn(), + }, + }, ], }).compile(); @@ -76,6 +85,9 @@ describe('AiAgentRoleService', () => { WorkspaceScopedRepository >(getWorkspaceScopedRepositoryToken(RoleTargetEntity)); roleTargetService = module.get(RoleTargetService); + workspaceCacheService = module.get( + WorkspaceCacheService, + ); // Setup test data testAgent = { @@ -372,4 +384,67 @@ describe('AiAgentRoleService', () => { }); }); }); + + describe('deleteAgentOnlyRoleIfUnused', () => { + it('invalidates cached role permissions after deleting the role', async () => { + const agentOnlyRole = { + ...testRole, + canBeAssignedToAgents: true, + canBeAssignedToUsers: false, + canBeAssignedToApiKeys: false, + } as RoleEntity; + + jest.spyOn(roleRepository, 'findOne').mockResolvedValue(agentOnlyRole); + jest.spyOn(roleTargetRepository, 'count').mockResolvedValue(0); + jest + .spyOn(roleRepository, 'delete') + .mockResolvedValue({ raw: [], generatedMaps: [] }); + jest + .spyOn(workspaceCacheService, 'invalidateAndRecompute') + .mockResolvedValue(); + + await service.deleteAgentOnlyRoleIfUnused({ + roleId: agentOnlyRole.id, + roleTargetId: 'deleted-role-target-id', + workspaceId: testWorkspaceId, + }); + + expect(roleRepository.delete).toHaveBeenCalledWith(testWorkspaceId, { + id: agentOnlyRole.id, + }); + expect(workspaceCacheService.invalidateAndRecompute).toHaveBeenCalledWith( + testWorkspaceId, + ['flatRoleMaps', 'flatRolePermissionFlagMaps'], + ); + expect( + (roleRepository.delete as jest.Mock).mock.invocationCallOrder[0], + ).toBeLessThan( + (workspaceCacheService.invalidateAndRecompute as jest.Mock).mock + .invocationCallOrder[0], + ); + }); + + it('keeps the cache unchanged while the role is still assigned', async () => { + const agentOnlyRole = { + ...testRole, + canBeAssignedToAgents: true, + canBeAssignedToUsers: false, + canBeAssignedToApiKeys: false, + } as RoleEntity; + + jest.spyOn(roleRepository, 'findOne').mockResolvedValue(agentOnlyRole); + jest.spyOn(roleTargetRepository, 'count').mockResolvedValue(1); + + await service.deleteAgentOnlyRoleIfUnused({ + roleId: agentOnlyRole.id, + roleTargetId: 'deleted-role-target-id', + workspaceId: testWorkspaceId, + }); + + expect(roleRepository.delete).not.toHaveBeenCalled(); + expect( + workspaceCacheService.invalidateAndRecompute, + ).not.toHaveBeenCalled(); + }); + }); }); diff --git a/packages/twenty-server/src/engine/metadata-modules/ai/ai-agent-role/ai-agent-role.module.ts b/packages/twenty-server/src/engine/metadata-modules/ai/ai-agent-role/ai-agent-role.module.ts index 878e0ba0ca..95f5456dc5 100644 --- a/packages/twenty-server/src/engine/metadata-modules/ai/ai-agent-role/ai-agent-role.module.ts +++ b/packages/twenty-server/src/engine/metadata-modules/ai/ai-agent-role/ai-agent-role.module.ts @@ -6,12 +6,14 @@ import { RoleTargetEntity } from 'src/engine/metadata-modules/role-target/role-t import { RoleTargetModule } from 'src/engine/metadata-modules/role-target/role-target.module'; import { RoleEntity } from 'src/engine/metadata-modules/role/role.entity'; import { provideWorkspaceScopedRepository } from 'src/engine/twenty-orm/workspace-scoped-repository/provide-workspace-scoped-repository'; +import { WorkspaceCacheModule } from 'src/engine/workspace-cache/workspace-cache.module'; import { AiAgentRoleService } from './ai-agent-role.service'; @Module({ imports: [ TypeOrmModule.forFeature([AgentEntity, RoleEntity, RoleTargetEntity]), RoleTargetModule, + WorkspaceCacheModule, ], providers: [ AiAgentRoleService, diff --git a/packages/twenty-server/src/engine/metadata-modules/ai/ai-agent-role/ai-agent-role.service.ts b/packages/twenty-server/src/engine/metadata-modules/ai/ai-agent-role/ai-agent-role.service.ts index 7315150d28..37210218e8 100644 --- a/packages/twenty-server/src/engine/metadata-modules/ai/ai-agent-role/ai-agent-role.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/ai/ai-agent-role/ai-agent-role.service.ts @@ -13,6 +13,7 @@ import { RoleTargetService } from 'src/engine/metadata-modules/role-target/servi import { RoleEntity } from 'src/engine/metadata-modules/role/role.entity'; import { InjectWorkspaceScopedRepository } from 'src/engine/twenty-orm/workspace-scoped-repository/inject-workspace-scoped-repository.decorator'; import { WorkspaceScopedRepository } from 'src/engine/twenty-orm/workspace-scoped-repository/workspace-scoped-repository'; +import { WorkspaceCacheService } from 'src/engine/workspace-cache/services/workspace-cache.service'; @Injectable() export class AiAgentRoleService { constructor( @@ -23,6 +24,7 @@ export class AiAgentRoleService { @InjectWorkspaceScopedRepository(RoleTargetEntity) private readonly roleTargetRepository: WorkspaceScopedRepository, private readonly roleTargetService: RoleTargetService, + private readonly workspaceCacheService: WorkspaceCacheService, ) {} public async assignRoleToAgent({ @@ -196,6 +198,10 @@ export class AiAgentRoleService { if (remainingAssignments === 0) { await this.roleRepository.delete(workspaceId, { id: roleId }); + await this.workspaceCacheService.invalidateAndRecompute(workspaceId, [ + 'flatRoleMaps', + 'flatRolePermissionFlagMaps', + ]); } } } diff --git a/packages/twenty-server/src/engine/metadata-modules/flat-role/utils/flat-role-has-permission-flag.util.ts b/packages/twenty-server/src/engine/metadata-modules/flat-role/utils/flat-role-has-permission-flag.util.ts new file mode 100644 index 0000000000..0c8cc30c32 --- /dev/null +++ b/packages/twenty-server/src/engine/metadata-modules/flat-role/utils/flat-role-has-permission-flag.util.ts @@ -0,0 +1,36 @@ +import { + type PermissionFlagType, + SystemPermissionFlag, +} from 'twenty-shared/constants'; +import { isDefined } from 'twenty-shared/utils'; + +import { type FlatRolePermissionFlagMaps } from 'src/engine/metadata-modules/flat-role-permission-flag/types/flat-role-permission-flag-maps.type'; +import { type FlatRole } from 'src/engine/metadata-modules/flat-role/types/flat-role.type'; + +export const flatRoleHasPermissionFlag = ({ + flatRole, + permissionFlag, + flatRolePermissionFlagMaps, +}: { + flatRole: FlatRole; + permissionFlag: PermissionFlagType; + flatRolePermissionFlagMaps: FlatRolePermissionFlagMaps; +}): boolean => { + const permissionFlagUniversalIdentifier = + SystemPermissionFlag[permissionFlag]; + + return flatRole.rolePermissionFlagIds.some((rolePermissionFlagId) => { + const rolePermissionFlagUniversalIdentifier = + flatRolePermissionFlagMaps.universalIdentifierById[rolePermissionFlagId]; + + if (!isDefined(rolePermissionFlagUniversalIdentifier)) { + return false; + } + + return ( + flatRolePermissionFlagMaps.byUniversalIdentifier[ + rolePermissionFlagUniversalIdentifier + ]?.permissionFlagUniversalIdentifier === permissionFlagUniversalIdentifier + ); + }); +}; diff --git a/packages/twenty-server/src/engine/metadata-modules/permissions/__tests__/permissions.service.spec.ts b/packages/twenty-server/src/engine/metadata-modules/permissions/__tests__/permissions.service.spec.ts index 24df69949d..bb8d21c209 100644 --- a/packages/twenty-server/src/engine/metadata-modules/permissions/__tests__/permissions.service.spec.ts +++ b/packages/twenty-server/src/engine/metadata-modules/permissions/__tests__/permissions.service.spec.ts @@ -8,22 +8,50 @@ import { import { ApiKeyRoleService } from 'src/engine/core-modules/api-key/services/api-key-role.service'; import { ApplicationEntity } from 'src/engine/core-modules/application/application.entity'; +import { createEmptyFlatEntityMaps } from 'src/engine/metadata-modules/flat-entity/constant/create-empty-flat-entity-maps.constant'; +import { type SyncableFlatEntity } from 'src/engine/metadata-modules/flat-entity/types/flat-entity-from.type'; +import { type FlatEntityMaps } from 'src/engine/metadata-modules/flat-entity/types/flat-entity-maps.type'; +import { addFlatEntityToFlatEntityMapsOrThrow } from 'src/engine/metadata-modules/flat-entity/utils/add-flat-entity-to-flat-entity-maps-or-throw.util'; +import { type FlatRolePermissionFlag } from 'src/engine/metadata-modules/flat-role-permission-flag/types/flat-role-permission-flag.type'; +import { type FlatRole } from 'src/engine/metadata-modules/flat-role/types/flat-role.type'; import { PermissionsService } from 'src/engine/metadata-modules/permissions/permissions.service'; import { RoleEntity } from 'src/engine/metadata-modules/role/role.entity'; import { UserRoleService } from 'src/engine/metadata-modules/user-role/user-role.service'; +import { type RolePermissionConfig } from 'src/engine/twenty-orm/types/role-permission-config'; import { getWorkspaceScopedRepositoryToken } from 'src/engine/twenty-orm/workspace-scoped-repository/get-workspace-scoped-repository-token.util'; import { WorkspaceCacheService } from 'src/engine/workspace-cache/services/workspace-cache.service'; +const buildFlatEntityMaps = ( + entities: T[], +): FlatEntityMaps => + entities.reduce( + (maps, entity) => + addFlatEntityToFlatEntityMapsOrThrow({ + flatEntity: entity, + flatEntityMaps: maps, + }), + createEmptyFlatEntityMaps() as FlatEntityMaps, + ); + describe('PermissionsService', () => { let service: PermissionsService; + let roleRepository: { find: jest.Mock }; + let workspaceCacheService: { getOrRecompute: jest.Mock }; beforeEach(async () => { + roleRepository = { + find: jest.fn(), + }; + workspaceCacheService = { + getOrRecompute: jest.fn(), + }; + const module: TestingModule = await Test.createTestingModule({ providers: [ PermissionsService, { provide: getWorkspaceScopedRepositoryToken(RoleEntity), - useValue: {}, + useValue: roleRepository, }, { provide: ApiKeyRoleService, @@ -35,7 +63,7 @@ describe('PermissionsService', () => { }, { provide: WorkspaceCacheService, - useValue: {}, + useValue: workspaceCacheService, }, { provide: getRepositoryToken(ApplicationEntity), @@ -450,4 +478,163 @@ describe('PermissionsService', () => { }); }); }); + + describe.each([ + { + evaluator: 'checkRolesPermissions' as const, + permissionFlag: PermissionFlagType.DATA_MODEL, + basePermission: 'canUpdateAllSettings' as const, + }, + { + evaluator: 'hasToolPermission' as const, + permissionFlag: PermissionFlagType.HTTP_REQUEST_TOOL, + basePermission: 'canAccessAllTools' as const, + }, + ])( + '$evaluator from workspace cache', + ({ evaluator, permissionFlag, basePermission }) => { + const workspaceId = 'test-workspace-id'; + const createFlatRole = ({ + id, + hasBasePermission = false, + rolePermissionFlagIds = [], + }: { + id: string; + hasBasePermission?: boolean; + rolePermissionFlagIds?: string[]; + }): FlatRole => + ({ + id, + universalIdentifier: `${id}-universal-identifier`, + canAccessAllTools: false, + canUpdateAllSettings: false, + rolePermissionFlagIds, + [basePermission]: hasBasePermission, + }) as FlatRole; + const createFlatRolePermissionFlag = ({ + id, + permissionFlagUniversalIdentifier, + }: { + id: string; + permissionFlagUniversalIdentifier: string; + }): FlatRolePermissionFlag => + ({ + id, + universalIdentifier: `${id}-universal-identifier`, + permissionFlagUniversalIdentifier, + }) as FlatRolePermissionFlag; + const mockCachedPermissions = ({ + roles, + rolePermissionFlags = [], + }: { + roles: FlatRole[]; + rolePermissionFlags?: FlatRolePermissionFlag[]; + }) => { + workspaceCacheService.getOrRecompute.mockResolvedValue({ + flatRoleMaps: buildFlatEntityMaps(roles), + flatRolePermissionFlagMaps: buildFlatEntityMaps(rolePermissionFlags), + }); + }; + const evaluate = (rolePermissionConfig: RolePermissionConfig) => + service[evaluator](rolePermissionConfig, workspaceId, permissionFlag); + + afterEach(() => { + expect(roleRepository.find).not.toHaveBeenCalled(); + }); + + it('preserves union and intersection semantics', async () => { + const grantingRole = createFlatRole({ + id: 'granting-role-id', + hasBasePermission: true, + }); + const denyingRole = createFlatRole({ + id: 'denying-role-id', + }); + + mockCachedPermissions({ + roles: [grantingRole, denyingRole], + }); + + await expect( + evaluate({ unionOf: [grantingRole.id, denyingRole.id] }), + ).resolves.toBe(true); + await expect( + evaluate({ intersectionOf: [grantingRole.id, denyingRole.id] }), + ).resolves.toBe(false); + expect(workspaceCacheService.getOrRecompute).toHaveBeenCalledWith( + workspaceId, + ['flatRoleMaps', 'flatRolePermissionFlagMaps'], + ); + }); + + it('grants an explicitly assigned permission flag', async () => { + const rolePermissionFlag = createFlatRolePermissionFlag({ + id: 'role-permission-flag-id', + permissionFlagUniversalIdentifier: + SystemPermissionFlag[permissionFlag], + }); + const role = createFlatRole({ + id: 'role-id', + rolePermissionFlagIds: [rolePermissionFlag.id], + }); + + mockCachedPermissions({ + roles: [role], + rolePermissionFlags: [rolePermissionFlag], + }); + + await expect(evaluate({ unionOf: [role.id] })).resolves.toBe(true); + }); + + it('denies an unrelated permission flag', async () => { + const rolePermissionFlag = createFlatRolePermissionFlag({ + id: 'role-permission-flag-id', + permissionFlagUniversalIdentifier: SystemPermissionFlag.WORKSPACE, + }); + const role = createFlatRole({ + id: 'role-id', + rolePermissionFlagIds: [rolePermissionFlag.id], + }); + + mockCachedPermissions({ + roles: [role], + rolePermissionFlags: [rolePermissionFlag], + }); + + await expect(evaluate({ unionOf: [role.id] })).resolves.toBe(false); + }); + + it('bypasses checks without loading the cache', async () => { + await expect( + evaluate({ shouldBypassPermissionChecks: true }), + ).resolves.toBe(true); + expect(workspaceCacheService.getOrRecompute).not.toHaveBeenCalled(); + }); + + it('fails closed for invalid or missing roles', async () => { + const role = createFlatRole({ + id: 'role-id', + hasBasePermission: true, + }); + + mockCachedPermissions({ roles: [role] }); + + await expect(evaluate({ unionOf: [] })).resolves.toBe(false); + await expect(evaluate({ unionOf: [role.id, role.id] })).resolves.toBe( + false, + ); + await expect( + evaluate({ unionOf: [role.id, 'missing-role-id'] }), + ).resolves.toBe(false); + }); + + it('fails closed when the workspace cache is unavailable', async () => { + workspaceCacheService.getOrRecompute.mockRejectedValue( + new Error('Cache unavailable'), + ); + + await expect(evaluate({ unionOf: ['role-id'] })).resolves.toBe(false); + }); + }, + ); }); diff --git a/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.service.ts b/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.service.ts index ec78c4c647..8134a82aa8 100644 --- a/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.service.ts @@ -7,7 +7,7 @@ import { SystemPermissionFlag, } from 'twenty-shared/constants'; import { isDefined } from 'twenty-shared/utils'; -import { In, Repository } from 'typeorm'; +import { Repository } from 'typeorm'; import { ApiKeyRoleService } from 'src/engine/core-modules/api-key/services/api-key-role.service'; import { ApplicationEntity } from 'src/engine/core-modules/application/application.entity'; @@ -15,6 +15,9 @@ import { ApplicationException, ApplicationExceptionCode, } from 'src/engine/core-modules/application/application.exception'; +import { type FlatRolePermissionFlagMaps } from 'src/engine/metadata-modules/flat-role-permission-flag/types/flat-role-permission-flag-maps.type'; +import { type FlatRole } from 'src/engine/metadata-modules/flat-role/types/flat-role.type'; +import { flatRoleHasPermissionFlag } from 'src/engine/metadata-modules/flat-role/utils/flat-role-has-permission-flag.util'; import { TOOL_PERMISSION_FLAGS } from 'src/engine/metadata-modules/permissions/constants/tool-permission-flags'; import { PermissionsException, @@ -29,6 +32,12 @@ import { InjectWorkspaceScopedRepository } from 'src/engine/twenty-orm/workspace import { WorkspaceScopedRepository } from 'src/engine/twenty-orm/workspace-scoped-repository/workspace-scoped-repository'; import { WorkspaceCacheService } from 'src/engine/workspace-cache/services/workspace-cache.service'; +type CachedRolesFromPermissionConfig = { + roles: FlatRole[]; + useIntersection: boolean; + flatRolePermissionFlagMaps: FlatRolePermissionFlagMaps; +} | null; + @Injectable() export class PermissionsService { constructor( @@ -271,8 +280,7 @@ export class PermissionsService { private async getRolesFromPermissionConfig( rolePermissionConfig: RolePermissionConfig, workspaceId: string, - relations: string[] = [], - ): Promise<{ roles: RoleEntity[]; useIntersection: boolean } | null> { + ): Promise { if ('shouldBypassPermissionChecks' in rolePermissionConfig) { return null; } @@ -292,16 +300,50 @@ export class PermissionsService { throw new Error('No role IDs provided'); } - const roles = await this.roleRepository.find(workspaceId, { - where: { id: In(roleIds) }, - relations, - }); + if (new Set(roleIds).size !== roleIds.length) { + throw new Error('Duplicate role IDs provided'); + } + + const { flatRoleMaps, flatRolePermissionFlagMaps } = + await this.workspaceCacheService.getOrRecompute(workspaceId, [ + 'flatRoleMaps', + 'flatRolePermissionFlagMaps', + ]); + const roles = roleIds + .map((roleId) => { + const roleUniversalIdentifier = + flatRoleMaps.universalIdentifierById[roleId]; + + return isDefined(roleUniversalIdentifier) + ? flatRoleMaps.byUniversalIdentifier[roleUniversalIdentifier] + : undefined; + }) + .filter(isDefined); if (roles.length !== roleIds.length) { throw new Error('Some roles not found'); } - return { roles, useIntersection }; + return { roles, useIntersection, flatRolePermissionFlagMaps }; + } + + private checkFlatRolePermissions( + role: FlatRole, + setting: PermissionFlagType, + flatRolePermissionFlagMaps: FlatRolePermissionFlagMaps, + ): boolean { + const hasBasePermission = this.isToolPermission(setting) + ? role.canAccessAllTools + : role.canUpdateAllSettings; + + return ( + hasBasePermission === true || + flatRoleHasPermissionFlag({ + flatRole: role, + permissionFlag: setting, + flatRolePermissionFlagMaps, + }) + ); } public async checkRolesPermissions( @@ -313,18 +355,23 @@ export class PermissionsService { const result = await this.getRolesFromPermissionConfig( rolePermissionConfig, workspaceId, - ['rolePermissionFlags', 'rolePermissionFlags.permissionFlag'], ); if (result === null) { return true; } - const { roles, useIntersection } = result; + const { roles, useIntersection, flatRolePermissionFlagMaps } = result; + const checkRoleHasPermission = (role: FlatRole) => + this.checkFlatRolePermissions( + role, + setting, + flatRolePermissionFlagMaps, + ); return useIntersection - ? roles.every((role) => this.checkRolePermissions(role, setting)) - : roles.some((role) => this.checkRolePermissions(role, setting)); + ? roles.every(checkRoleHasPermission) + : roles.some(checkRoleHasPermission); } catch { return false; } @@ -339,21 +386,24 @@ export class PermissionsService { const result = await this.getRolesFromPermissionConfig( rolePermissionConfig, workspaceId, - ['rolePermissionFlags', 'rolePermissionFlags.permissionFlag'], ); if (result === null) { return true; } - const { roles, useIntersection } = result; + const { roles, useIntersection, flatRolePermissionFlagMaps } = result; - const checkRoleHasPermission = (role: RoleEntity) => { + const checkRoleHasPermission = (role: FlatRole) => { if (role.canAccessAllTools === true) { return true; } - return this.roleHasPermissionFlag(role, flag); + return flatRoleHasPermissionFlag({ + flatRole: role, + permissionFlag: flag, + flatRolePermissionFlagMaps, + }); }; return useIntersection