From 607d9ee6e5d73e0d10b88c8ac6b33b0685f1d8b6 Mon Sep 17 00:00:00 2001 From: Weiko Date: Wed, 17 Jun 2026 17:44:01 +0200 Subject: [PATCH] fix(server): allow app-defined permission flags to be referenced by a role in the same sync (#21742) ## Context When an application defined custom permission flags and a role referencing them in the same sync, installation failed, first at validation (Permission flag not found) and then at execution (Migration action 'create' for 'rolePermissionFlag' failed). Root cause: both the migration builder order and the runner execution order processed rolePermissionFlag before permissionFlag, so the role's flag assignments were validated/inserted before the flags they reference existed. ## Changes - Builder order: run the permissionFlag builder before rolePermissionFlag so newly created flags are visible in the optimistic maps when assignments are validated. - Execution order: order the permission-flag actions so definitions are created before assignments, and assignments deleted before definitions, keeping the FK satisfied in both directions. - In-use check: move the "flag still assigned to a role" guard out of the per-entity deletion validator (order-dependent, false-positived when a flag and its assignments were deleted together) into a new order-independent validatePermissionFlagNotInUseCrossEntity (aligned with existing validateObjectMetadataCrossEntity, validateViewFieldLabelIdentifierCrossEntity, ...), run after all builders against the migration's final state. This fixes both the create path (define flag + reference it in one sync) and the teardown path (delete flag + its assignments in one sync). Review in cubic --- ...-flag-not-in-use-cross-entity.util.spec.ts | 99 +++++++++++++++++++ ...ssion-flag-not-in-use-cross-entity.util.ts | 66 +++++++++++++ ...ce-migration-build-orchestrator.service.ts | 8 +- .../compute-ordered-migration-actions.util.ts | 9 +- ...ross-entity-transversal-validation.util.ts | 8 ++ ...-permission-flag-validator.service.spec.ts | 55 ----------- .../flat-permission-flag-validator.service.ts | 18 ---- 7 files changed, 180 insertions(+), 83 deletions(-) create mode 100644 packages/twenty-server/src/engine/metadata-modules/flat-permission-flag/validators/utils/__tests__/validate-permission-flag-not-in-use-cross-entity.util.spec.ts create mode 100644 packages/twenty-server/src/engine/metadata-modules/flat-permission-flag/validators/utils/validate-permission-flag-not-in-use-cross-entity.util.ts diff --git a/packages/twenty-server/src/engine/metadata-modules/flat-permission-flag/validators/utils/__tests__/validate-permission-flag-not-in-use-cross-entity.util.spec.ts b/packages/twenty-server/src/engine/metadata-modules/flat-permission-flag/validators/utils/__tests__/validate-permission-flag-not-in-use-cross-entity.util.spec.ts new file mode 100644 index 0000000000..f354f9aa8a --- /dev/null +++ b/packages/twenty-server/src/engine/metadata-modules/flat-permission-flag/validators/utils/__tests__/validate-permission-flag-not-in-use-cross-entity.util.spec.ts @@ -0,0 +1,99 @@ +import { createEmptyFlatEntityMaps } from 'src/engine/metadata-modules/flat-entity/constant/create-empty-flat-entity-maps.constant'; +import { type FlatEntityMaps } from 'src/engine/metadata-modules/flat-entity/types/flat-entity-maps.type'; +import { type FlatRolePermissionFlag } from 'src/engine/metadata-modules/flat-role-permission-flag/types/flat-role-permission-flag.type'; +import { validatePermissionFlagNotInUseCrossEntity } from 'src/engine/metadata-modules/flat-permission-flag/validators/utils/validate-permission-flag-not-in-use-cross-entity.util'; +import { PermissionFlagExceptionCode } from 'src/engine/metadata-modules/permission-flag/permission-flag.exception'; + +const FLAG_UNIVERSAL_IDENTIFIER = '00000000-0000-0000-0000-000000000001'; + +const buildRolePermissionFlag = ( + overrides: Partial = {}, +): FlatRolePermissionFlag => + ({ + id: '00000000-0000-0000-0000-000000000101', + universalIdentifier: '00000000-0000-0000-0000-000000000101', + permissionFlagId: FLAG_UNIVERSAL_IDENTIFIER, + permissionFlagUniversalIdentifier: FLAG_UNIVERSAL_IDENTIFIER, + roleUniversalIdentifier: '00000000-0000-0000-0000-000000000201', + workspaceId: 'workspace-id', + applicationId: '00000000-0000-0000-0000-000000000aaa', + applicationUniversalIdentifier: '00000000-0000-0000-0000-000000000aaa', + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + ...overrides, + }) as unknown as FlatRolePermissionFlag; + +const buildRolePermissionFlagMaps = ( + rolePermissionFlags: FlatRolePermissionFlag[] = [], +): FlatEntityMaps => { + const maps = + createEmptyFlatEntityMaps() as unknown as FlatEntityMaps; + + for (const rolePermissionFlag of rolePermissionFlags) { + maps.byUniversalIdentifier[rolePermissionFlag.universalIdentifier] = + rolePermissionFlag; + } + + return maps; +}; + +const buildArgs = (args: { + rolePermissionFlags: FlatRolePermissionFlag[]; + deletedFlagUniversalIdentifiers: string[]; +}) => + ({ + optimisticUniversalFlatMaps: { + flatRolePermissionFlagMaps: buildRolePermissionFlagMaps( + args.rolePermissionFlags, + ), + }, + deletedPermissionFlagActions: args.deletedFlagUniversalIdentifiers.map( + (universalIdentifier) => ({ + type: 'delete', + metadataName: 'permissionFlag', + universalIdentifier, + flatEntity: { universalIdentifier, key: 'MANAGE_INVOICES' }, + }), + ), + }) as unknown as Parameters< + typeof validatePermissionFlagNotInUseCrossEntity + >[0]; + +describe('validatePermissionFlagNotInUseCrossEntity', () => { + it('returns no error when no permission flag is being deleted', () => { + const result = validatePermissionFlagNotInUseCrossEntity( + buildArgs({ + rolePermissionFlags: [buildRolePermissionFlag()], + deletedFlagUniversalIdentifiers: [], + }), + ); + + expect(result.permissionFlag).toHaveLength(0); + }); + + it('rejects deleting a flag still referenced by a role in the target state', () => { + const result = validatePermissionFlagNotInUseCrossEntity( + buildArgs({ + rolePermissionFlags: [buildRolePermissionFlag()], + deletedFlagUniversalIdentifiers: [FLAG_UNIVERSAL_IDENTIFIER], + }), + ); + + expect( + result.permissionFlag.flatMap((failure) => + failure.errors.map((error) => error.code), + ), + ).toEqual([PermissionFlagExceptionCode.PERMISSION_FLAG_IN_USE]); + }); + + it('allows deleting a flag whose assignments are deleted in the same migration', () => { + const result = validatePermissionFlagNotInUseCrossEntity( + buildArgs({ + rolePermissionFlags: [], + deletedFlagUniversalIdentifiers: [FLAG_UNIVERSAL_IDENTIFIER], + }), + ); + + expect(result.permissionFlag).toHaveLength(0); + }); +}); diff --git a/packages/twenty-server/src/engine/metadata-modules/flat-permission-flag/validators/utils/validate-permission-flag-not-in-use-cross-entity.util.ts b/packages/twenty-server/src/engine/metadata-modules/flat-permission-flag/validators/utils/validate-permission-flag-not-in-use-cross-entity.util.ts new file mode 100644 index 0000000000..a1f9ba0237 --- /dev/null +++ b/packages/twenty-server/src/engine/metadata-modules/flat-permission-flag/validators/utils/validate-permission-flag-not-in-use-cross-entity.util.ts @@ -0,0 +1,66 @@ +import { msg, t } from '@lingui/core/macro'; +import { isDefined } from 'twenty-shared/utils'; + +import { PermissionFlagExceptionCode } from 'src/engine/metadata-modules/permission-flag/permission-flag.exception'; +import { type OrchestratorFailureReport } from 'src/engine/workspace-manager/workspace-migration/types/workspace-migration-orchestrator.type'; +import { type AllUniversalFlatEntityMaps } from 'src/engine/workspace-manager/workspace-migration/universal-flat-entity/types/all-universal-flat-entity-maps.type'; +import { type UniversalDeletePermissionFlagAction } from 'src/engine/workspace-manager/workspace-migration/workspace-migration-builder/builders/permission-flag/types/workspace-migration-permission-flag-action.type'; +import { getEmptyFlatEntityValidationError } from 'src/engine/workspace-manager/workspace-migration/workspace-migration-builder/builders/utils/get-flat-entity-validation-error.util'; + +export const validatePermissionFlagNotInUseCrossEntity = ({ + optimisticUniversalFlatMaps, + deletedPermissionFlagActions, +}: { + optimisticUniversalFlatMaps: Pick< + AllUniversalFlatEntityMaps, + 'flatRolePermissionFlagMaps' + >; + deletedPermissionFlagActions: UniversalDeletePermissionFlagAction[]; +}): Pick => { + const validationErrors: Pick = { + permissionFlag: [], + }; + + if (deletedPermissionFlagActions.length === 0) { + return validationErrors; + } + + const survivingRolePermissionFlags = Object.values( + optimisticUniversalFlatMaps.flatRolePermissionFlagMaps + .byUniversalIdentifier, + ).filter(isDefined); + + for (const deleteAction of deletedPermissionFlagActions) { + const isStillReferenced = survivingRolePermissionFlags.some( + (rolePermissionFlag) => + rolePermissionFlag.permissionFlagUniversalIdentifier === + deleteAction.universalIdentifier, + ); + + if (!isStillReferenced) { + continue; + } + + const flagKey = + deleteAction.flatEntity?.key ?? deleteAction.universalIdentifier; + + const failedValidation = getEmptyFlatEntityValidationError({ + flatEntityMinimalInformation: { + universalIdentifier: deleteAction.universalIdentifier, + key: deleteAction.flatEntity?.key, + }, + metadataName: 'permissionFlag', + type: 'delete', + }); + + failedValidation.errors.push({ + code: PermissionFlagExceptionCode.PERMISSION_FLAG_IN_USE, + message: t`Permission flag definition with key ${flagKey} is still assigned to a role`, + userFriendlyMessage: msg`Remove this permission from all roles before deleting it`, + }); + + validationErrors.permissionFlag.push(failedValidation); + } + + return validationErrors; +}; diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/services/workspace-migration-build-orchestrator.service.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/services/workspace-migration-build-orchestrator.service.ts index 5521191eb2..6a66039ba4 100644 --- a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/services/workspace-migration-build-orchestrator.service.ts +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/services/workspace-migration-build-orchestrator.service.ts @@ -228,14 +228,14 @@ export class WorkspaceMigrationBuildOrchestratorService { ALL_METADATA_NAME.fieldPermission, workspaceMigrationFieldPermissionActionsBuilderService, ), - createEntityActionsBuilderTask( - ALL_METADATA_NAME.rolePermissionFlag, - workspaceMigrationRolePermissionFlagActionsBuilderService, - ), createEntityActionsBuilderTask( ALL_METADATA_NAME.permissionFlag, workspaceMigrationPermissionFlagActionsBuilderService, ), + createEntityActionsBuilderTask( + ALL_METADATA_NAME.rolePermissionFlag, + workspaceMigrationRolePermissionFlagActionsBuilderService, + ), createEntityActionsBuilderTask( ALL_METADATA_NAME.roleTarget, workspaceMigrationRoleTargetActionsBuilderService, diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/compute-ordered-migration-actions.util.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/compute-ordered-migration-actions.util.ts index eee0f08c91..234f1cb873 100644 --- a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/compute-ordered-migration-actions.util.ts +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/compute-ordered-migration-actions.util.ts @@ -71,16 +71,13 @@ export const computeOrderedMigrationActions = ( ...aggregatedOrchestratorActionsReport.fieldPermission.update, /// - // Permission flags + // Permission flag definitions and their role assignments. ...aggregatedOrchestratorActionsReport.rolePermissionFlag.delete, - ...aggregatedOrchestratorActionsReport.rolePermissionFlag.create, - ...aggregatedOrchestratorActionsReport.rolePermissionFlag.update, - /// - - // Permission flag definitions ...aggregatedOrchestratorActionsReport.permissionFlag.delete, ...aggregatedOrchestratorActionsReport.permissionFlag.create, + ...aggregatedOrchestratorActionsReport.rolePermissionFlag.create, ...aggregatedOrchestratorActionsReport.permissionFlag.update, + ...aggregatedOrchestratorActionsReport.rolePermissionFlag.update, /// // Agents diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/cross-entity-transversal-validation.util.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/cross-entity-transversal-validation.util.ts index 60d5d1766f..3f9934b197 100644 --- a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/cross-entity-transversal-validation.util.ts +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/utils/cross-entity-transversal-validation.util.ts @@ -1,4 +1,5 @@ import { validateObjectMetadataCrossEntity } from 'src/engine/metadata-modules/flat-object-metadata/validators/utils/validate-object-metadata-cross-entity.util'; +import { validatePermissionFlagNotInUseCrossEntity } from 'src/engine/metadata-modules/flat-permission-flag/validators/utils/validate-permission-flag-not-in-use-cross-entity.util'; import { validateViewFieldLabelIdentifierCrossEntity } from 'src/engine/metadata-modules/flat-view-field/validators/utils/validate-view-field-label-identifier-cross-entity.util'; import { type OrchestratorActionsReport, @@ -32,8 +33,15 @@ export const crossEntityTransversalValidation = ({ preDeletionFlatViewFieldMaps, }); + const { permissionFlag } = validatePermissionFlagNotInUseCrossEntity({ + optimisticUniversalFlatMaps, + deletedPermissionFlagActions: + orchestratorActionsReport.permissionFlag.delete, + }); + crossEntityFailureReport.objectMetadata.push(...objectMetadata); crossEntityFailureReport.viewField.push(...viewField); + crossEntityFailureReport.permissionFlag.push(...permissionFlag); validateUniversalIdentifierCrossEntityUniquenessThroughReportMutation({ optimisticUniversalFlatMaps, diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/validators/services/__tests__/flat-permission-flag-validator.service.spec.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/validators/services/__tests__/flat-permission-flag-validator.service.spec.ts index 8fa457234a..227a787e63 100644 --- a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/validators/services/__tests__/flat-permission-flag-validator.service.spec.ts +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/validators/services/__tests__/flat-permission-flag-validator.service.spec.ts @@ -1,6 +1,5 @@ import { Test, type TestingModule } from '@nestjs/testing'; -import { PermissionFlagType } from 'twenty-shared/constants'; import { ALL_METADATA_NAME } from 'twenty-shared/metadata'; import { createEmptyFlatEntityMaps } from 'src/engine/metadata-modules/flat-entity/constant/create-empty-flat-entity-maps.constant'; @@ -38,23 +37,6 @@ const buildEmptyRolePermissionFlagMaps = (): FlatEntityMaps => createEmptyFlatEntityMaps() as unknown as FlatEntityMaps; -const buildFlatRolePermissionFlag = ( - overrides: Partial = {}, -): FlatRolePermissionFlag => - ({ - id: '00000000-0000-0000-0000-000000000101', - universalIdentifier: '00000000-0000-0000-0000-000000000101', - permissionFlagId: '00000000-0000-0000-0000-000000000001', - permissionFlagUniversalIdentifier: '00000000-0000-0000-0000-000000000001', - roleUniversalIdentifier: '00000000-0000-0000-0000-000000000201', - workspaceId: 'workspace-id', - applicationId: '00000000-0000-0000-0000-000000000aaa', - applicationUniversalIdentifier: '00000000-0000-0000-0000-000000000aaa', - createdAt: new Date().toISOString(), - updatedAt: new Date().toISOString(), - ...overrides, - }) as unknown as FlatRolePermissionFlag; - const buildArgs = ( flatEntityToValidate: FlatPermissionFlag, optimisticMaps: FlatEntityMaps = buildEmptyMaps(), @@ -335,43 +317,6 @@ describe('FlatPermissionFlagValidatorService', () => { PermissionFlagExceptionCode.PERMISSION_FLAG_IS_STANDARD, ]); }); - - it('rejects deleting a definition while roles still grant its key', () => { - const existing = buildFlatDefinition({ - key: PermissionFlagType.WORKSPACE, - }); - const optimisticMaps = buildEmptyMaps(); - optimisticMaps.byUniversalIdentifier[existing.universalIdentifier] = - existing; - - const rolePermissionFlagMaps = buildEmptyRolePermissionFlagMaps(); - const rolePermissionFlag = buildFlatRolePermissionFlag({ - permissionFlagId: existing.id, - permissionFlagUniversalIdentifier: existing.universalIdentifier, - }); - rolePermissionFlagMaps.byUniversalIdentifier[ - rolePermissionFlag.universalIdentifier - ] = rolePermissionFlag; - - const result = service.validateFlatPermissionFlagDeletion({ - flatEntityToValidate: existing, - optimisticFlatEntityMapsAndRelatedFlatEntityMaps: { - flatPermissionFlagMaps: optimisticMaps, - flatRolePermissionFlagMaps: rolePermissionFlagMaps, - }, - buildOptions: { - isSystemBuild: false, - applicationUniversalIdentifier: - '00000000-0000-0000-0000-000000000aaa', - }, - } as unknown as Parameters< - FlatPermissionFlagValidatorService['validateFlatPermissionFlagDeletion'] - >[0]); - - expect(result.errors.map((error) => error.code)).toEqual([ - PermissionFlagExceptionCode.PERMISSION_FLAG_IN_USE, - ]); - }); }); // ALL_METADATA_NAME is referenced as a sanity check the metadata name exists diff --git a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/validators/services/flat-permission-flag-validator.service.ts b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/validators/services/flat-permission-flag-validator.service.ts index 26c43fc8a9..821cbf7f61 100644 --- a/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/validators/services/flat-permission-flag-validator.service.ts +++ b/packages/twenty-server/src/engine/workspace-manager/workspace-migration/workspace-migration-builder/validators/services/flat-permission-flag-validator.service.ts @@ -166,7 +166,6 @@ export class FlatPermissionFlagValidatorService { flatEntityToValidate: { universalIdentifier }, optimisticFlatEntityMapsAndRelatedFlatEntityMaps: { flatPermissionFlagMaps: optimisticFlatPermissionFlagMaps, - flatRolePermissionFlagMaps: optimisticFlatRolePermissionFlagMaps, }, buildOptions, }: UniversalFlatEntityValidationArgs< @@ -209,23 +208,6 @@ export class FlatPermissionFlagValidatorService { }); } - const isPermissionFlagInUse = Object.values( - optimisticFlatRolePermissionFlagMaps.byUniversalIdentifier, - ).some( - (rolePermissionFlag) => - isDefined(rolePermissionFlag) && - rolePermissionFlag.permissionFlagUniversalIdentifier === - existing.universalIdentifier, - ); - - if (isPermissionFlagInUse) { - validationResult.errors.push({ - code: PermissionFlagExceptionCode.PERMISSION_FLAG_IN_USE, - message: t`Permission flag definition with key ${existing.key} is still assigned to a role`, - userFriendlyMessage: msg`Remove this permission from all roles before deleting it`, - }); - } - return validationResult; } }