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); }); });