From 6ae5900ac9ee8e757ba708514feed0ad515fda61 Mon Sep 17 00:00:00 2001 From: Weiko Date: Thu, 2 Apr 2026 15:15:14 +0200 Subject: [PATCH] Fix field permission validation rejecting undefined optional fields (#19243) The backend validation for field permissions was using !== null to check canReadFieldValue and canUpdateFieldValue, but these optional GraphQL fields can also be undefined when omitted from the input. This caused the validation to incorrectly reject legitimate requests (e.g., restricting only "update" without specifying "read") with the error "Field permissions can only be used to restrict access, not to grant additional permissions." Replaced !== null checks with isDefined() so that both null and undefined are treated as "no opinion" on that permission. To reproduce: - Create a single FieldPermission on an object with canEdit: false without any other rule on that same object --- .../field-permission.service.ts | 29 ++++++--- .../read-permissions.integration-spec.ts | 4 +- .../update-permissions.integration-spec.ts | 62 +++++-------------- 3 files changed, 37 insertions(+), 58 deletions(-) diff --git a/packages/twenty-server/src/engine/metadata-modules/object-permission/field-permission/field-permission.service.ts b/packages/twenty-server/src/engine/metadata-modules/object-permission/field-permission/field-permission.service.ts index f319101700..c9747dd682 100644 --- a/packages/twenty-server/src/engine/metadata-modules/object-permission/field-permission/field-permission.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/object-permission/field-permission/field-permission.service.ts @@ -130,8 +130,8 @@ export class FieldPermissionService { { objectMetadataId: fieldPermission.objectMetadataId, fieldMetadataId: fieldPermission.fieldMetadataId, - canReadFieldValue: fieldPermission.canReadFieldValue ?? undefined, - canUpdateFieldValue: fieldPermission.canUpdateFieldValue ?? undefined, + canReadFieldValue: fieldPermission.canReadFieldValue, + canUpdateFieldValue: fieldPermission.canUpdateFieldValue, }, ); } @@ -183,9 +183,13 @@ export class FieldPermissionService { ); } else { const effectiveCanRead = - desired.canReadFieldValue ?? current.canReadFieldValue; + desired.canReadFieldValue !== undefined + ? desired.canReadFieldValue + : current.canReadFieldValue; const effectiveCanUpdate = - desired.canUpdateFieldValue ?? current.canUpdateFieldValue; + desired.canUpdateFieldValue !== undefined + ? desired.canUpdateFieldValue + : current.canUpdateFieldValue; const changed = effectiveCanRead !== current.canReadFieldValue || effectiveCanUpdate !== current.canUpdateFieldValue; @@ -201,8 +205,8 @@ export class FieldPermissionService { current.objectMetadataUniversalIdentifier, fieldMetadataUniversalIdentifier: current.fieldMetadataUniversalIdentifier, - canReadFieldValue: effectiveCanRead ?? undefined, - canUpdateFieldValue: effectiveCanUpdate ?? undefined, + canReadFieldValue: effectiveCanRead, + canUpdateFieldValue: effectiveCanUpdate, createdAt: current.createdAt, updatedAt: now, }); @@ -210,9 +214,16 @@ export class FieldPermissionService { } } + const inputFieldKeys = new Set( + input.fieldPermissions.map((fp) => + keyFrom(fp.objectMetadataId, fp.fieldMetadataId), + ), + ); + for (const current of currentFieldPermissionsForRole) { const key = keyFrom(current.objectMetadataId, current.fieldMetadataId); - if (!desiredMap.has(key)) { + + if (inputFieldKeys.has(key) && !desiredMap.has(key)) { flatEntityToDelete.push({ universalIdentifier: current.universalIdentifier, applicationUniversalIdentifier: @@ -324,9 +335,9 @@ export class FieldPermissionService { } if ( - (fieldPermission.canUpdateFieldValue !== null && + (isDefined(fieldPermission.canUpdateFieldValue) && fieldPermission.canUpdateFieldValue !== false) || - (fieldPermission.canReadFieldValue !== null && + (isDefined(fieldPermission.canReadFieldValue) && fieldPermission.canReadFieldValue !== false) ) { throw new PermissionsException( diff --git a/packages/twenty-server/test/integration/graphql/suites/object-records-permissions/fields-permissions/read-permissions.integration-spec.ts b/packages/twenty-server/test/integration/graphql/suites/object-records-permissions/fields-permissions/read-permissions.integration-spec.ts index df7e7dac0b..342385ac25 100644 --- a/packages/twenty-server/test/integration/graphql/suites/object-records-permissions/fields-permissions/read-permissions.integration-spec.ts +++ b/packages/twenty-server/test/integration/graphql/suites/object-records-permissions/fields-permissions/read-permissions.integration-spec.ts @@ -117,7 +117,7 @@ describe('Field permissions restrictions', () => { { objectMetadataId: companyObjectId, fieldMetadataId: restrictedCompanyFieldId, - canReadFieldValue: false, + canUpdateFieldValue: false, }, ], }); @@ -134,7 +134,7 @@ describe('Field permissions restrictions', () => { { objectMetadataId: personObjectId, fieldMetadataId: restrictedPersonFieldId, - canReadFieldValue: false, + canUpdateFieldValue: false, }, ], }); diff --git a/packages/twenty-server/test/integration/graphql/suites/object-records-permissions/fields-permissions/update-permissions.integration-spec.ts b/packages/twenty-server/test/integration/graphql/suites/object-records-permissions/fields-permissions/update-permissions.integration-spec.ts index 375703229f..3a28e53db2 100644 --- a/packages/twenty-server/test/integration/graphql/suites/object-records-permissions/fields-permissions/update-permissions.integration-spec.ts +++ b/packages/twenty-server/test/integration/graphql/suites/object-records-permissions/fields-permissions/update-permissions.integration-spec.ts @@ -14,6 +14,8 @@ import { updateWorkspaceMemberRole } from 'test/integration/graphql/utils/update import { upsertFieldPermissions } from 'test/integration/graphql/utils/upsert-field-permissions.util'; import { makeMetadataAPIRequest } from 'test/integration/metadata/suites/utils/make-metadata-api-request.util'; +import { ErrorCode } from 'src/engine/core-modules/graphql/utils/graphql-errors.util'; +import { PermissionsExceptionMessage } from 'src/engine/metadata-modules/permissions/permissions.exception'; import { WORKSPACE_MEMBER_DATA_SEED_IDS } from 'src/engine/workspace-manager/dev-seeder/data/constants/workspace-member-data-seeds.constant'; const client = request(`http://localhost:${APP_PORT}`); @@ -28,33 +30,13 @@ const COMPANY_GQL_FIELDS_WITHOUT_EMPLOYEES = ` id name `; -const expectEmployeesIsAccessible = ({ - response, - operationName, - expectedEmployees, -}: { - response: any; - operationName: - | 'createCompanies' - | 'createCompany' - | 'updateCompanies' - | 'updateCompany'; - expectedEmployees?: number; -}) => { - expect(response.body.errors).toBeUndefined(); - expect(response.body.data).toBeDefined(); - - const result = - operationName === 'createCompany' || operationName === 'updateCompany' - ? response.body.data[operationName] - : response.body.data[operationName]?.[0]; - - expect(result).toBeDefined(); - if (typeof expectedEmployees === 'number') { - expect(result.employees).toBe(expectedEmployees); - } else { - expect(typeof result.employees).toBe('number'); - } +const expectPermissionDeniedError = (response: any) => { + expect(response.body.errors).toBeDefined(); + expect(response.body.errors.length).toBeGreaterThan(0); + expect(response.body.errors[0].message).toBe( + PermissionsExceptionMessage.PERMISSION_DENIED, + ); + expect(response.body.errors[0].extensions.code).toBe(ErrorCode.FORBIDDEN); }; describe('Field update permissions restrictions', () => { @@ -314,7 +296,7 @@ describe('Field update permissions restrictions', () => { // }); // }); - describe('should allow employees field when creating', () => { + describe('should block creating with update-restricted field in data', () => { beforeEach(async () => { await restrictUpdateAccessToCompanyEmployee( customRoleId, @@ -337,11 +319,7 @@ describe('Field update permissions restrictions', () => { const response = await makeGraphqlAPIRequestWithMemberRole(graphqlOperation); - expectEmployeesIsAccessible({ - response, - operationName: 'createCompanies', - expectedEmployees: 15, - }); + expectPermissionDeniedError(response); }); it('2. createOne with restricted field', async () => { @@ -354,14 +332,10 @@ describe('Field update permissions restrictions', () => { const response = await makeGraphqlAPIRequestWithMemberRole(graphqlOperation); - expectEmployeesIsAccessible({ - response, - operationName: 'createCompany', - expectedEmployees: 25, - }); + expectPermissionDeniedError(response); }); }); - describe('should allow employees field in update operation responses', () => { + describe('should block read-restricted field in update operation responses', () => { beforeEach(async () => { await restrictReadAccessToCompanyEmployee( customRoleId, @@ -382,10 +356,7 @@ describe('Field update permissions restrictions', () => { const response = await makeGraphqlAPIRequestWithMemberRole(graphqlOperation); - expectEmployeesIsAccessible({ - response, - operationName: 'updateCompanies', - }); + expectPermissionDeniedError(response); }); it('2. updateOne requesting restricted field in response', async () => { @@ -399,10 +370,7 @@ describe('Field update permissions restrictions', () => { const response = await makeGraphqlAPIRequestWithMemberRole(graphqlOperation); - expectEmployeesIsAccessible({ - response, - operationName: 'updateCompany', - }); + expectPermissionDeniedError(response); }); });