From 09e89334c6c9c2b1076c448bcd3e18e1abce3a9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Malfait?= Date: Wed, 12 Nov 2025 11:51:54 +0100 Subject: [PATCH] Fix Tool permissionn guard issue (#15770) Fix a critical issue on the permission guard for tools which led to users being denied access when they should have had access --- .../snippets/translatable-card.mdx | 26 -- .../__tests__/permissions.service.spec.ts | 437 ++++++++++++++++++ .../permissions/permissions.service.ts | 6 +- 3 files changed, 442 insertions(+), 27 deletions(-) delete mode 100644 packages/twenty-docs/snippets/translatable-card.mdx create mode 100644 packages/twenty-server/src/engine/metadata-modules/permissions/__tests__/permissions.service.spec.ts diff --git a/packages/twenty-docs/snippets/translatable-card.mdx b/packages/twenty-docs/snippets/translatable-card.mdx deleted file mode 100644 index 5401eff3d4..0000000000 --- a/packages/twenty-docs/snippets/translatable-card.mdx +++ /dev/null @@ -1,26 +0,0 @@ -export const TranslatableCardTitle = ({ children }) => { - return <>{children}; -}; - -export const TranslatableCard = ({ children, ...props }) => { - const childrenArray = React.Children.toArray(children); - - let title = ''; - let content = []; - - childrenArray.forEach((child) => { - // Check if child is TranslatableCardTitle component - if (child?.type?.name === 'TranslatableCardTitle' || child?.props?.mdxType === 'TranslatableCardTitle') { - title = child.props.children; - } else { - content.push(child); - } - }); - - return ( - - {content} - - ); -}; - 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 new file mode 100644 index 0000000000..376bc071c7 --- /dev/null +++ b/packages/twenty-server/src/engine/metadata-modules/permissions/__tests__/permissions.service.spec.ts @@ -0,0 +1,437 @@ +import { Test, type TestingModule } from '@nestjs/testing'; +import { getRepositoryToken } from '@nestjs/typeorm'; + +import { PermissionFlagType } from 'src/engine/metadata-modules/permissions/constants/permission-flag-type.constants'; +import { PermissionsService } from 'src/engine/metadata-modules/permissions/permissions.service'; +import { RoleEntity } from 'src/engine/metadata-modules/role/role.entity'; +import { ApiKeyRoleService } from 'src/engine/core-modules/api-key/api-key-role.service'; +import { UserRoleService } from 'src/engine/metadata-modules/user-role/user-role.service'; +import { WorkspacePermissionsCacheService } from 'src/engine/metadata-modules/workspace-permissions-cache/workspace-permissions-cache.service'; + +describe('PermissionsService', () => { + let service: PermissionsService; + + beforeEach(async () => { + const module: TestingModule = await Test.createTestingModule({ + providers: [ + PermissionsService, + { + provide: getRepositoryToken(RoleEntity), + useValue: {}, + }, + { + provide: ApiKeyRoleService, + useValue: {}, + }, + { + provide: UserRoleService, + useValue: {}, + }, + { + provide: WorkspacePermissionsCacheService, + useValue: {}, + }, + ], + }).compile(); + + service = module.get(PermissionsService); + }); + + describe('checkRolePermissions', () => { + describe('canAccessAllTools for tool permissions', () => { + it('should grant permission when canAccessAllTools is true for a tool permission', () => { + const roleWithAllTools: Partial = { + id: 'test-role-id', + label: 'Test Role', + description: 'Test role description', + icon: 'IconTest', + canAccessAllTools: true, + canUpdateAllSettings: false, + canReadAllObjectRecords: false, + canUpdateAllObjectRecords: false, + canSoftDeleteAllObjectRecords: false, + canDestroyAllObjectRecords: false, + canBeAssignedToUsers: true, + canBeAssignedToAgents: true, + canBeAssignedToApiKeys: true, + permissionFlags: [], + workspaceId: 'test-workspace-id', + createdAt: new Date(), + updatedAt: new Date(), + isEditable: true, + }; + + // Test all tool permissions + expect( + service.checkRolePermissions( + roleWithAllTools as RoleEntity, + PermissionFlagType.UPLOAD_FILE, + ), + ).toBe(true); + expect( + service.checkRolePermissions( + roleWithAllTools as RoleEntity, + PermissionFlagType.DOWNLOAD_FILE, + ), + ).toBe(true); + expect( + service.checkRolePermissions( + roleWithAllTools as RoleEntity, + PermissionFlagType.AI, + ), + ).toBe(true); + expect( + service.checkRolePermissions( + roleWithAllTools as RoleEntity, + PermissionFlagType.VIEWS, + ), + ).toBe(true); + expect( + service.checkRolePermissions( + roleWithAllTools as RoleEntity, + PermissionFlagType.SEND_EMAIL_TOOL, + ), + ).toBe(true); + expect( + service.checkRolePermissions( + roleWithAllTools as RoleEntity, + PermissionFlagType.IMPORT_CSV, + ), + ).toBe(true); + expect( + service.checkRolePermissions( + roleWithAllTools as RoleEntity, + PermissionFlagType.EXPORT_CSV, + ), + ).toBe(true); + expect( + service.checkRolePermissions( + roleWithAllTools as RoleEntity, + PermissionFlagType.CONNECTED_ACCOUNTS, + ), + ).toBe(true); + }); + + it('should NOT grant settings permissions when canAccessAllTools is true', () => { + const roleWithAllTools: Partial = { + id: 'test-role-id', + label: 'Test Role', + description: 'Test role description', + icon: 'IconTest', + canAccessAllTools: true, + canUpdateAllSettings: false, + canReadAllObjectRecords: false, + canUpdateAllObjectRecords: false, + canSoftDeleteAllObjectRecords: false, + canDestroyAllObjectRecords: false, + canBeAssignedToUsers: true, + canBeAssignedToAgents: true, + canBeAssignedToApiKeys: true, + permissionFlags: [], + workspaceId: 'test-workspace-id', + createdAt: new Date(), + updatedAt: new Date(), + isEditable: true, + }; + + // Test that settings permissions are NOT granted + expect( + service.checkRolePermissions( + roleWithAllTools as RoleEntity, + PermissionFlagType.ROLES, + ), + ).toBe(false); + expect( + service.checkRolePermissions( + roleWithAllTools as RoleEntity, + PermissionFlagType.WORKSPACE, + ), + ).toBe(false); + expect( + service.checkRolePermissions( + roleWithAllTools as RoleEntity, + PermissionFlagType.DATA_MODEL, + ), + ).toBe(false); + expect( + service.checkRolePermissions( + roleWithAllTools as RoleEntity, + PermissionFlagType.SECURITY, + ), + ).toBe(false); + }); + }); + + describe('canUpdateAllSettings for settings permissions', () => { + it('should grant permission when canUpdateAllSettings is true for a settings permission', () => { + const roleWithAllSettings: Partial = { + id: 'test-role-id', + label: 'Test Role', + description: 'Test role description', + icon: 'IconTest', + canAccessAllTools: false, + canUpdateAllSettings: true, + canReadAllObjectRecords: false, + canUpdateAllObjectRecords: false, + canSoftDeleteAllObjectRecords: false, + canDestroyAllObjectRecords: false, + canBeAssignedToUsers: true, + canBeAssignedToAgents: true, + canBeAssignedToApiKeys: true, + permissionFlags: [], + roleTargets: [], + objectPermissions: [], + fieldPermissions: [], + workspaceId: 'test-workspace-id', + createdAt: new Date(), + updatedAt: new Date(), + isEditable: true, + }; + + // Test all settings permissions + expect( + service.checkRolePermissions( + roleWithAllSettings as RoleEntity, + PermissionFlagType.ROLES, + ), + ).toBe(true); + expect( + service.checkRolePermissions( + roleWithAllSettings as RoleEntity, + PermissionFlagType.WORKSPACE, + ), + ).toBe(true); + expect( + service.checkRolePermissions( + roleWithAllSettings as RoleEntity, + PermissionFlagType.DATA_MODEL, + ), + ).toBe(true); + expect( + service.checkRolePermissions( + roleWithAllSettings as RoleEntity, + PermissionFlagType.SECURITY, + ), + ).toBe(true); + expect( + service.checkRolePermissions( + roleWithAllSettings as RoleEntity, + PermissionFlagType.WORKFLOWS, + ), + ).toBe(true); + expect( + service.checkRolePermissions( + roleWithAllSettings as RoleEntity, + PermissionFlagType.WORKSPACE_MEMBERS, + ), + ).toBe(true); + expect( + service.checkRolePermissions( + roleWithAllSettings as RoleEntity, + PermissionFlagType.API_KEYS_AND_WEBHOOKS, + ), + ).toBe(true); + }); + + it('should NOT grant tool permissions when canUpdateAllSettings is true', () => { + const roleWithAllSettings: Partial = { + id: 'test-role-id', + label: 'Test Role', + description: 'Test role description', + icon: 'IconTest', + canAccessAllTools: false, + canUpdateAllSettings: true, + canReadAllObjectRecords: false, + canUpdateAllObjectRecords: false, + canSoftDeleteAllObjectRecords: false, + canDestroyAllObjectRecords: false, + canBeAssignedToUsers: true, + canBeAssignedToAgents: true, + canBeAssignedToApiKeys: true, + permissionFlags: [], + roleTargets: [], + objectPermissions: [], + fieldPermissions: [], + workspaceId: 'test-workspace-id', + createdAt: new Date(), + updatedAt: new Date(), + isEditable: true, + }; + + // Test that tool permissions are NOT granted + expect( + service.checkRolePermissions( + roleWithAllSettings as RoleEntity, + PermissionFlagType.UPLOAD_FILE, + ), + ).toBe(false); + expect( + service.checkRolePermissions( + roleWithAllSettings as RoleEntity, + PermissionFlagType.DOWNLOAD_FILE, + ), + ).toBe(false); + expect( + service.checkRolePermissions( + roleWithAllSettings as RoleEntity, + PermissionFlagType.AI, + ), + ).toBe(false); + expect( + service.checkRolePermissions( + roleWithAllSettings as RoleEntity, + PermissionFlagType.VIEWS, + ), + ).toBe(false); + }); + }); + + describe('Granular permissions with permissionFlags', () => { + it('should grant specific tool permission when included in permissionFlags even if canAccessAllTools is false', () => { + const roleWithSpecificPermission: Partial = { + id: 'test-role-id', + label: 'Test Role', + description: 'Test role description', + icon: 'IconTest', + canAccessAllTools: false, + canUpdateAllSettings: false, + canReadAllObjectRecords: false, + canUpdateAllObjectRecords: false, + canSoftDeleteAllObjectRecords: false, + canDestroyAllObjectRecords: false, + canBeAssignedToUsers: true, + canBeAssignedToAgents: true, + canBeAssignedToApiKeys: true, + permissionFlags: [ + { + id: 'permission-1', + flag: PermissionFlagType.UPLOAD_FILE, + roleId: 'test-role-id', + workspaceId: 'test-workspace-id', + createdAt: new Date(), + updatedAt: new Date(), + }, + ] as any, + workspaceId: 'test-workspace-id', + createdAt: new Date(), + updatedAt: new Date(), + isEditable: true, + }; + + expect( + service.checkRolePermissions( + roleWithSpecificPermission as RoleEntity, + PermissionFlagType.UPLOAD_FILE, + ), + ).toBe(true); + expect( + service.checkRolePermissions( + roleWithSpecificPermission as RoleEntity, + PermissionFlagType.DOWNLOAD_FILE, + ), + ).toBe(false); + }); + + it('should grant specific settings permission when included in permissionFlags even if canUpdateAllSettings is false', () => { + const roleWithSpecificPermission: Partial = { + id: 'test-role-id', + label: 'Test Role', + description: 'Test role description', + icon: 'IconTest', + canAccessAllTools: false, + canUpdateAllSettings: false, + canReadAllObjectRecords: false, + canUpdateAllObjectRecords: false, + canSoftDeleteAllObjectRecords: false, + canDestroyAllObjectRecords: false, + canBeAssignedToUsers: true, + canBeAssignedToAgents: true, + canBeAssignedToApiKeys: true, + permissionFlags: [ + { + id: 'permission-1', + flag: PermissionFlagType.ROLES, + roleId: 'test-role-id', + workspaceId: 'test-workspace-id', + createdAt: new Date(), + updatedAt: new Date(), + }, + ] as any, + workspaceId: 'test-workspace-id', + createdAt: new Date(), + updatedAt: new Date(), + isEditable: true, + }; + + expect( + service.checkRolePermissions( + roleWithSpecificPermission as RoleEntity, + PermissionFlagType.ROLES, + ), + ).toBe(true); + expect( + service.checkRolePermissions( + roleWithSpecificPermission as RoleEntity, + PermissionFlagType.WORKSPACE, + ), + ).toBe(false); + }); + }); + + describe('No permissions', () => { + it('should deny all permissions when neither canAccessAllTools nor canUpdateAllSettings are true and no specific permissions', () => { + const roleWithNoPermissions: Partial = { + id: 'test-role-id', + label: 'Test Role', + description: 'Test role description', + icon: 'IconTest', + canAccessAllTools: false, + canUpdateAllSettings: false, + canReadAllObjectRecords: false, + canUpdateAllObjectRecords: false, + canSoftDeleteAllObjectRecords: false, + canDestroyAllObjectRecords: false, + canBeAssignedToUsers: true, + canBeAssignedToAgents: true, + canBeAssignedToApiKeys: true, + permissionFlags: [], + roleTargets: [], + objectPermissions: [], + fieldPermissions: [], + workspaceId: 'test-workspace-id', + createdAt: new Date(), + updatedAt: new Date(), + isEditable: true, + }; + + // Tool permissions should be denied + expect( + service.checkRolePermissions( + roleWithNoPermissions as RoleEntity, + PermissionFlagType.UPLOAD_FILE, + ), + ).toBe(false); + expect( + service.checkRolePermissions( + roleWithNoPermissions as RoleEntity, + PermissionFlagType.AI, + ), + ).toBe(false); + + // Settings permissions should be denied + expect( + service.checkRolePermissions( + roleWithNoPermissions as RoleEntity, + PermissionFlagType.ROLES, + ), + ).toBe(false); + expect( + service.checkRolePermissions( + roleWithNoPermissions as RoleEntity, + PermissionFlagType.WORKSPACE, + ), + ).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 b822194b48..751cd2ba16 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 @@ -187,7 +187,11 @@ export class PermissionsService { role: RoleEntity, setting: PermissionFlagType, ): boolean { - if (role.canUpdateAllSettings === true) { + const hasBasePermission = this.isToolPermission(setting) + ? role.canAccessAllTools + : role.canUpdateAllSettings; + + if (hasBasePermission === true) { return true; }