From 4708ff34c72403953be601b6fdbaa71c19bb89f6 Mon Sep 17 00:00:00 2001 From: Marie <51697796+ijreilly@users.noreply.github.com> Date: Fri, 8 Aug 2025 10:27:53 +0200 Subject: [PATCH] [permissions] Fix update of relation field permissions (#13755) In this PR, we add some validation logic and rules in both FE and BE to ensure field permissions are handled correctly for relation fields. - (BE) Only one field permission per fieldMetadata is accepted per input. This is already guaranteed in the FE. It was added to help guarantee that, when looking within the field permissions input for a potential field permission on a relationTargetFieldMetadataId for a relation field, there can only be 0 or 1. - (FE) Only field permission with new values are sent to save, to avoid sending contradictory field permissions for related fields. E.g. let's say I have an existing field permission restricting read permission on company's people field. By definition I also have one on person's company field. If I update this field permission to enable the read permission by updating company's people field, in the previous logic I was also going to send for upsert the existing obsolete field permission on person's company. Thus the server does not know which is the right value so we should only send the new value. - (BE) If the server receives two contradictory field permissions on two related fields, e.g. on company's people with canRead = null and person's company with canRead = false, it throws an error. --- .../roles/role/hooks/useSaveDraftRoleToDB.ts | 11 ++++- .../utils/newFieldPermissionsFilter.util.ts | 23 ++++++++++ .../field-permission.service.ts | 44 ++++++++++++++++--- 3 files changed, 71 insertions(+), 7 deletions(-) create mode 100644 packages/twenty-front/src/modules/settings/roles/role/hooks/utils/newFieldPermissionsFilter.util.ts diff --git a/packages/twenty-front/src/modules/settings/roles/role/hooks/useSaveDraftRoleToDB.ts b/packages/twenty-front/src/modules/settings/roles/role/hooks/useSaveDraftRoleToDB.ts index e133b1715d..e3cca310d7 100644 --- a/packages/twenty-front/src/modules/settings/roles/role/hooks/useSaveDraftRoleToDB.ts +++ b/packages/twenty-front/src/modules/settings/roles/role/hooks/useSaveDraftRoleToDB.ts @@ -1,6 +1,7 @@ import { GET_ROLES } from '@/settings/roles/graphql/queries/getRolesQuery'; import { useUpdateWorkspaceMemberRole } from '@/settings/roles/hooks/useUpdateWorkspaceMemberRole'; import { useRemoveFieldPermissionInDraftRole } from '@/settings/roles/role-permissions/object-level-permissions/field-permissions/hooks/useRemoveFieldPermissionInDraftRole'; +import { newFieldPermissionsFilter } from '@/settings/roles/role/hooks/utils/newFieldPermissionsFilter.util'; import { settingsDraftRoleFamilyState } from '@/settings/roles/states/settingsDraftRoleFamilyState'; import { settingsPersistedRoleFamilyState } from '@/settings/roles/states/settingsPersistedRoleFamilyState'; import { SettingsPath } from '@/types/SettingsPath'; @@ -72,7 +73,7 @@ export const useSaveDraftRoleToDB = ({ ); }); - const fieldPermissionsToUpsert = + const onlyMeaningfulFieldPermissions = dirtyFields.fieldPermissions?.filter( (dirtyFieldPermissionToFilter) => !fieldPermissionsThatShouldntBeCreatedBecauseTheyAreUseless?.some( @@ -82,6 +83,14 @@ export const useSaveDraftRoleToDB = ({ ), ) ?? []; + const fieldPermissionsToUpsert = onlyMeaningfulFieldPermissions.filter( + (dirtyFieldPermission) => + newFieldPermissionsFilter( + dirtyFieldPermission, + settingsPersistedRole?.fieldPermissions, + ), + ); + const { removeFieldPermissionInDraftRole } = useRemoveFieldPermissionInDraftRole(); diff --git a/packages/twenty-front/src/modules/settings/roles/role/hooks/utils/newFieldPermissionsFilter.util.ts b/packages/twenty-front/src/modules/settings/roles/role/hooks/utils/newFieldPermissionsFilter.util.ts new file mode 100644 index 0000000000..794a8da6ca --- /dev/null +++ b/packages/twenty-front/src/modules/settings/roles/role/hooks/utils/newFieldPermissionsFilter.util.ts @@ -0,0 +1,23 @@ +import { FieldPermission } from '~/generated/graphql'; + +export const newFieldPermissionsFilter = ( + dirtyFieldPermission: FieldPermission, + existingFieldPermissions?: FieldPermission[] | null, +) => { + const existingFieldPermission = existingFieldPermissions?.find( + (persistedFieldPermission) => + persistedFieldPermission.fieldMetadataId === + dirtyFieldPermission.fieldMetadataId, + ); + + if (!existingFieldPermission) { + return true; + } + + return ( + dirtyFieldPermission.canReadFieldValue !== + existingFieldPermission.canReadFieldValue || + dirtyFieldPermission.canUpdateFieldValue !== + existingFieldPermission.canUpdateFieldValue + ); +}; 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 5f6d434eae..b00cd86edb 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 @@ -7,7 +7,10 @@ import { In, Repository } from 'typeorm'; import { RelationType } from 'src/engine/metadata-modules/field-metadata/interfaces/relation-type.interface'; -import { InternalServerError } from 'src/engine/core-modules/graphql/utils/graphql-errors.util'; +import { + InternalServerError, + UserInputError, +} from 'src/engine/core-modules/graphql/utils/graphql-errors.util'; import { FieldMetadataEntity } from 'src/engine/metadata-modules/field-metadata/field-metadata.entity'; import { isFieldMetadataTypeRelation } from 'src/engine/metadata-modules/field-metadata/utils/is-field-metadata-type-relation.util'; import { type UpsertFieldPermissionsInput } from 'src/engine/metadata-modules/object-permission/dtos/upsert-field-permissions.input'; @@ -74,6 +77,7 @@ export class FieldPermissionService { input.fieldPermissions.forEach((fieldPermission) => { this.validateFieldPermission({ + allFieldPermissions: input.fieldPermissions, fieldPermission, objectMetadataMapsById, rolesPermissions, @@ -169,16 +173,28 @@ export class FieldPermissionService { } private validateFieldPermission({ + allFieldPermissions, fieldPermission, objectMetadataMapsById, rolesPermissions, role, }: { + allFieldPermissions: UpsertFieldPermissionsInput['fieldPermissions']; fieldPermission: UpsertFieldPermissionsInput['fieldPermissions'][0]; objectMetadataMapsById: ObjectMetadataMaps['byId']; rolesPermissions: ObjectsPermissionsByRoleIdDeprecated; role: RoleEntity; }) { + const duplicateFieldPermissions = allFieldPermissions.filter( + (permission) => + permission.fieldMetadataId === fieldPermission.fieldMetadataId, + ); + + if (duplicateFieldPermissions.length > 1) { + throw new UserInputError( + `Cannot accept more than one fieldPermission for field ${fieldPermission.fieldMetadataId} in input.`, + ); + } if ( ('canUpdateFieldValue' in fieldPermission && fieldPermission.canUpdateFieldValue !== null && @@ -402,16 +418,32 @@ export class FieldPermissionService { fieldMetadata.settings?.relationType === RelationType.ONE_TO_MANY || fieldMetadata.settings?.relationType === RelationType.MANY_TO_ONE ) { - const fieldPermissionInputHasFieldPermissionOnRelationTargetFieldMetadata = + const fieldPermissionsOnRelationTargetField = fieldPermissions.filter( (fieldPermissionInput) => fieldPermissionInput.fieldMetadataId === fieldMetadata.relationTargetFieldMetadataId, - ).length > 0; + ); + + if (fieldPermissionsOnRelationTargetField.length > 0) { + const firstFieldPermission = + fieldPermissionsOnRelationTargetField[0]; // validation rules guarantee there can only be one + + const hasConflictingPermissions = + fieldPermission.canReadFieldValue !== + firstFieldPermission.canReadFieldValue || + fieldPermission.canUpdateFieldValue !== + firstFieldPermission.canUpdateFieldValue; + + if (hasConflictingPermissions) { + throw new UserInputError( + 'Conflicting field permissions found for relation target field', + { + userFriendlyMessage: `Contradicting field permissions have been detected on a relation field (${fieldMetadata.name}).`, + }, + ); + } - if ( - fieldPermissionInputHasFieldPermissionOnRelationTargetFieldMetadata - ) { return; }