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. <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/twentyhq/twenty/pull/23280?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
This commit is contained in:
+54
@@ -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<RoleEntity> &
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
+1
-1
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user