From d6c186a71e3e7e627279ea4e8fbcfaea36b1da52 Mon Sep 17 00:00:00 2001 From: Thomas Trompette Date: Fri, 24 Jul 2026 17:49:52 +0200 Subject: [PATCH] Fix system objects bypassing role object permission overrides (#23280) ## Bug Fixes #23062 (security). A workspace member whose role denies all object access could still read/mutate system-object records (messages, calendar events, and related system objects). System objects bypassed explicit role-level object permissions. ## Root cause In `workspace-roles-permissions-cache.service.ts`, the per-object permission helper resolved values as: ```ts (isSystem ? true : (overrideValue ?? defaultValue)) ``` For every non-workflow, non-workspace-member system object this forced `read`/`update`/`softDelete`/`destroy` to `true`, so an explicit deny override on the role was never consulted. ## Fix Flip the precedence so an explicit role override wins, and the `isSystem` default only applies when the role provides no override: ```ts overrideValue ?? (isSystem ? true : defaultValue) ``` Because the override fields are `boolean | undefined`, `??` correctly honors an explicit `false` while still falling back to the system default (`true`) when the role has no override row for that object. Workflow objects (settings-gated via the `WORKFLOWS` flag) and workspace-member objects (settings-gated, always readable) are handled in separate branches and are unchanged, so their intended defaults do not regress. ## Testing - `nx lint:diff-with-main twenty-server` passes. - Typecheck: no new errors from this change (pre-existing unrelated failures in `twenty-shared` date-filter utils only). - Manually verified on a local instance that a deny-all role no longer has read access to system objects. Review in cubic --- ...ce-roles-permissions-cache.service.spec.ts | 54 +++++++++++++++++++ ...rkspace-roles-permissions-cache.service.ts | 2 +- 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/packages/twenty-server/src/engine/metadata-modules/role/services/__tests__/workspace-roles-permissions-cache.service.spec.ts b/packages/twenty-server/src/engine/metadata-modules/role/services/__tests__/workspace-roles-permissions-cache.service.spec.ts index 8ada548fb5..99bcd76e56 100644 --- a/packages/twenty-server/src/engine/metadata-modules/role/services/__tests__/workspace-roles-permissions-cache.service.spec.ts +++ b/packages/twenty-server/src/engine/metadata-modules/role/services/__tests__/workspace-roles-permissions-cache.service.spec.ts @@ -25,6 +25,7 @@ const WORKSPACE_MEMBER_OBJECT_METADATA_ID = '22222222-2222-4222-8222-222222222222'; const WORKFLOW_OBJECT_METADATA_ID = '33333333-3333-4333-8333-333333333333'; const PERSON_OBJECT_METADATA_ID = '44444444-4444-4444-8444-444444444444'; +const MESSAGE_OBJECT_METADATA_ID = '55555555-5555-4555-8555-555555555555'; const createBaseRole = ( overrides: Partial & @@ -86,6 +87,12 @@ describe('WorkspaceRolesPermissionsCacheService', () => { universalIdentifier: STANDARD_OBJECTS.person.universalIdentifier, labelIdentifierFieldMetadataId: null, } as ObjectMetadataEntity, + { + id: MESSAGE_OBJECT_METADATA_ID, + isSystem: true, + universalIdentifier: STANDARD_OBJECTS.message.universalIdentifier, + labelIdentifierFieldMetadataId: null, + } as ObjectMetadataEntity, ]; beforeEach(async () => { @@ -316,4 +323,51 @@ describe('WorkspaceRolesPermissionsCacheService', () => { expect(personPermissions.canDestroyObjectRecords).toBe(true); }); }); + + describe('system object (message)', () => { + it('should default to full access when no object permission override exists', async () => { + roleRepository.find.mockResolvedValue([ + createBaseRole({ + rolePermissionFlags: [], + objectPermissions: [], + }), + ]); + + const result = await service.computeForCache(WORKSPACE_ID); + const messagePermissions = result[ROLE_ID][MESSAGE_OBJECT_METADATA_ID]; + + expect(messagePermissions.canReadObjectRecords).toBe(true); + expect(messagePermissions.canUpdateObjectRecords).toBe(true); + expect(messagePermissions.canSoftDeleteObjectRecords).toBe(true); + expect(messagePermissions.canDestroyObjectRecords).toBe(true); + }); + + it('should honor an explicit deny override instead of forcing system default', async () => { + objectPermissionRepository.find.mockResolvedValue([ + { + roleId: ROLE_ID, + objectMetadataId: MESSAGE_OBJECT_METADATA_ID, + canReadObjectRecords: false, + canUpdateObjectRecords: false, + canSoftDeleteObjectRecords: false, + canDestroyObjectRecords: false, + } as ObjectPermissionEntity, + ]); + + roleRepository.find.mockResolvedValue([ + createBaseRole({ + rolePermissionFlags: [], + objectPermissions: [], + }), + ]); + + const result = await service.computeForCache(WORKSPACE_ID); + const messagePermissions = result[ROLE_ID][MESSAGE_OBJECT_METADATA_ID]; + + expect(messagePermissions.canReadObjectRecords).toBe(false); + expect(messagePermissions.canUpdateObjectRecords).toBe(false); + expect(messagePermissions.canSoftDeleteObjectRecords).toBe(false); + expect(messagePermissions.canDestroyObjectRecords).toBe(false); + }); + }); }); diff --git a/packages/twenty-server/src/engine/metadata-modules/role/services/workspace-roles-permissions-cache.service.ts b/packages/twenty-server/src/engine/metadata-modules/role/services/workspace-roles-permissions-cache.service.ts index 1406482e02..feea6e3d98 100644 --- a/packages/twenty-server/src/engine/metadata-modules/role/services/workspace-roles-permissions-cache.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/role/services/workspace-roles-permissions-cache.service.ts @@ -181,7 +181,7 @@ export class WorkspaceRolesPermissionsCacheService extends WorkspaceCacheProvide const getPermissionValue = ( overrideValue: boolean | undefined, defaultValue: boolean, - ) => (isSystem ? true : (overrideValue ?? defaultValue)); + ) => overrideValue ?? (isSystem ? true : defaultValue); canRead = getPermissionValue( objectRecordPermissionsOverride?.canReadObjectRecords,