From 1ea18e9ef9e95cead7fe91ad4a7cb3fa46b3dfd7 Mon Sep 17 00:00:00 2001 From: Guillaume Jacquart Date: Fri, 16 Oct 2026 08:04:25 +0000 Subject: [PATCH] feat(editor): Show policy-blocked items in the source control pull modal (#40604) Co-authored-by: Claude Opus 5.5 (1M context) --- .../content-import-policy-result.schema.ts | 6 +- .../schemas/source-controlled-file.schema.ts | 2 +- .../source-control-import.service.ee.test.ts | 176 +++++++++++++++ .../__tests__/source-control.service.test.ts | 40 ++++ .../source-control-import.service.ee.ts | 107 ++++++++- .../source-control.service.ee.ts | 10 +- .../source-control-import.service.test.ts | 55 +++++ .../frontend/@n8n/i18n/src/locales/en.json | 7 + .../composables/usePolicyViolationLabels.ts | 25 +++ .../composables/usePolicyViolationToast.ts | 26 +-- .../components/SourceControlPullModal.test.ts | 139 ++++++++++++ .../components/SourceControlPullModal.vue | 208 ++++++++++++++++-- .../sourceControl.utils.test.ts | 43 +++- .../sourceControl.ee/sourceControl.utils.ts | 25 ++- 14 files changed, 806 insertions(+), 63 deletions(-) create mode 100644 packages/frontend/editor-ui/src/app/composables/usePolicyViolationLabels.ts diff --git a/packages/@n8n/api-types/src/schemas/content-import-policy-result.schema.ts b/packages/@n8n/api-types/src/schemas/content-import-policy-result.schema.ts index 9b93eb3c79d..3ed823347f9 100644 --- a/packages/@n8n/api-types/src/schemas/content-import-policy-result.schema.ts +++ b/packages/@n8n/api-types/src/schemas/content-import-policy-result.schema.ts @@ -4,9 +4,9 @@ import { policyCheckFailureSchema } from './policy-check-failure.schema'; import { policyViolationSchema } from './policy-violation.schema'; /** - * Why the content-import policy blocked one artifact. The artifact was skipped; the rest of the - * import/pull still lands. `checkErrors` stays empty here: under `enforce` a check that cannot - * answer fails the whole import, so it never lands on a single artifact's result. + * Why the content-import policy blocks one artifact; the rest of the import still lands. + * `checkErrors` is empty on an import result, because a broken check fails the whole import. + * A pull preview only evaluates, so there it can list the checks that could not answer. */ export const contentImportPolicyResultSchema = z.object({ violations: z.array(policyViolationSchema), diff --git a/packages/@n8n/api-types/src/schemas/source-controlled-file.schema.ts b/packages/@n8n/api-types/src/schemas/source-controlled-file.schema.ts index 0c9e7a451c7..fe9e100cb0a 100644 --- a/packages/@n8n/api-types/src/schemas/source-controlled-file.schema.ts +++ b/packages/@n8n/api-types/src/schemas/source-controlled-file.schema.ts @@ -61,7 +61,7 @@ export const SourceControlledFileSchema = z.object({ owner: ResourceOwnerSchema.optional(), // Resource owner can be a personal email or team information publishingError: z.string().optional(), publishingErrorDetails: workflowPublishBlockedDetailsSchema.optional(), - /** Advisory only — never blocks the pull. */ + /** Pull status: what the policy preview found. Pull result: why the pull skipped the file. */ contentImportPolicy: contentImportPolicyResultSchema.optional(), }); diff --git a/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts b/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts index 6a377d51481..b17128a71c9 100644 --- a/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts +++ b/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts @@ -3002,6 +3002,182 @@ describe('SourceControlImportService', () => { }); }); + describe('previewContentImportPolicy', () => { + const userId = 'user-id-123'; + const personalProject = Object.assign(new Project(), { + id: 'personal-project-id', + type: 'personal', + }); + const nodes = [ + { + id: 'node-1', + name: 'Node 1', + type: 'n8n-nodes-base.noOp', + typeVersion: 1, + position: [0, 0], + parameters: {}, + }, + ]; + const violation: PolicyViolation = { + kind: 'node-type-unavailable', + checkId: 'test.check', + message: 'not allowed', + }; + + const file = (overrides: Partial): SourceControlledFile => ({ + file: '/mock/file.json', + id: 'id', + name: 'name', + type: 'workflow', + status: 'created', + location: 'remote', + conflict: false, + updatedAt: '', + ...overrides, + }); + + const remoteWorkflow = (fields: Record) => ({ + versionId: 'v1', + nodes, + connections: {}, + ...fields, + }); + + const givenFiles = (contents: Record) => { + fsReadFile.mockImplementation(async (path) => JSON.stringify(contents[path as string])); + }; + + beforeEach(() => { + projectRepository.getPersonalProjectForUserOrFail.mockResolvedValue(personalProject); + policyEnforcementService.evaluateContentImport.mockResolvedValue({ violations: [] }); + }); + + it('reads nothing when no content-import check is registered', async () => { + policyEnforcementService.hasChecksFor.mockReturnValueOnce(false); + + await service.previewContentImportPolicy([file({})], userId); + + expect(fsReadFile).not.toHaveBeenCalled(); + expect(policyEnforcementService.evaluateContentImport).not.toHaveBeenCalled(); + }); + + it('evaluates an incoming workflow with the same context the pull enforces', async () => { + givenFiles({ '/mock/wf.json': remoteWorkflow({ id: 'wf1', name: 'Workflow 1' }) }); + const files = [file({ file: '/mock/wf.json', id: 'wf1' })]; + + await service.previewContentImportPolicy(files, userId); + + expect(policyEnforcementService.evaluateContentImport).toHaveBeenCalledWith({ + workflow: { id: 'wf1', name: 'Workflow 1', nodes }, + projectId: personalProject.id, + transport: 'source-control', + }); + }); + + it('marks a blocked workflow and leaves an allowed one unmarked', async () => { + givenFiles({ + '/mock/blocked.json': remoteWorkflow({ id: 'blocked', name: 'Blocked' }), + '/mock/allowed.json': remoteWorkflow({ id: 'allowed', name: 'Allowed' }), + }); + policyEnforcementService.evaluateContentImport.mockImplementation(async (context) => + 'workflow' in context && context.workflow.id === 'blocked' + ? { violations: [violation] } + : { violations: [] }, + ); + const blocked = file({ file: '/mock/blocked.json', id: 'blocked' }); + const allowed = file({ file: '/mock/allowed.json', id: 'allowed' }); + + await service.previewContentImportPolicy([blocked, allowed], userId); + + expect(blocked.contentImportPolicy).toEqual({ violations: [violation], checkErrors: [] }); + expect(allowed.contentImportPolicy).toBeUndefined(); + }); + + it('skips deleted files and other resource types', async () => { + const files = [ + file({ id: 'deleted', status: 'deleted' }), + file({ id: 'tags', type: 'tags' }), + file({ id: 'variables', type: 'variables' }), + ]; + + await service.previewContentImportPolicy(files, userId); + + expect(fsReadFile).not.toHaveBeenCalled(); + expect(policyEnforcementService.evaluateContentImport).not.toHaveBeenCalled(); + }); + + it('skips a workflow file the import would reject as incomplete', async () => { + givenFiles({ + '/mock/wf.json': remoteWorkflow({ id: 'wf1', name: 'Workflow 1', versionId: undefined }), + }); + + await service.previewContentImportPolicy( + [file({ file: '/mock/wf.json', id: 'wf1' })], + userId, + ); + + expect(policyEnforcementService.evaluateContentImport).not.toHaveBeenCalled(); + }); + + it('targets a team project the pull has not created yet, without creating it', async () => { + projectRepository.findOne.mockResolvedValue(null); + givenFiles({ + '/mock/wf.json': remoteWorkflow({ + id: 'wf1', + name: 'Workflow 1', + owner: { type: 'team', teamId: 'new-team', teamName: 'New team' }, + }), + }); + + await service.previewContentImportPolicy( + [file({ file: '/mock/wf.json', id: 'wf1' })], + userId, + ); + + expect(policyEnforcementService.evaluateContentImport).toHaveBeenCalledWith( + expect.objectContaining({ projectId: 'new-team' }), + ); + expect(projectRepository.save).not.toHaveBeenCalled(); + }); + + it('evaluates an incoming credential and keeps the checks that could not answer', async () => { + const checkErrors = [{ checkId: 'test.check', correlationId: 'corr-1' }]; + policyEnforcementService.evaluateContentImport.mockResolvedValue({ + violations: [], + checkErrors, + }); + givenFiles({ '/mock/cred.json': { id: 'cred1', type: 'slackApi', ownedBy: null } }); + const credential = file({ file: '/mock/cred.json', id: 'cred1', type: 'credential' }); + + await service.previewContentImportPolicy([credential], userId); + + expect(policyEnforcementService.evaluateContentImport).toHaveBeenCalledWith({ + credential: { id: 'cred1', type: 'slackApi' }, + projectId: personalProject.id, + transport: 'source-control', + }); + expect(credential.contentImportPolicy).toEqual({ violations: [], checkErrors }); + }); + + it('resolves each owner once', async () => { + const owner = { type: 'team', teamId: 'team-1', teamName: 'Team 1' }; + projectRepository.findOne.mockResolvedValue( + Object.assign(new Project(), { id: 'team-1', type: 'team' }), + ); + givenFiles({ + '/mock/a.json': remoteWorkflow({ id: 'a', name: 'A', owner }), + '/mock/b.json': remoteWorkflow({ id: 'b', name: 'B', owner }), + }); + + await service.previewContentImportPolicy( + [file({ file: '/mock/a.json', id: 'a' }), file({ file: '/mock/b.json', id: 'b' })], + userId, + ); + + expect(projectRepository.findOne).toHaveBeenCalledTimes(1); + }); + }); + describe('getRemoteVariablesFromFile', () => { it('should parse variables file correctly', async () => { globMock.mockResolvedValue(['/mock/variables.json']); diff --git a/packages/cli/src/modules/source-control.ee/__tests__/source-control.service.test.ts b/packages/cli/src/modules/source-control.ee/__tests__/source-control.service.test.ts index 727654e6c27..ad1b0bee641 100644 --- a/packages/cli/src/modules/source-control.ee/__tests__/source-control.service.test.ts +++ b/packages/cli/src/modules/source-control.ee/__tests__/source-control.service.test.ts @@ -683,6 +683,18 @@ describe('SourceControlService', () => { }); }); + it('does not run the policy preview, because the import enforces on its own', async () => { + mockStatusService.getStatus.mockResolvedValueOnce([]); + sourceControlImportService.importWorkflowFromWorkFolder.mockResolvedValue([]); + + await sourceControlService.pullWorkfolder(mock({ id: 'user-1' }), { + force: true, + autoPublish: 'none', + }); + + expect(sourceControlImportService.previewContentImportPolicy).not.toHaveBeenCalled(); + }); + it('announces each pulled workflow, but not one skipped by the content policy', async () => { const user = mock({ id: 'user-1' }); mockStatusService.getStatus.mockResolvedValueOnce([ @@ -1113,6 +1125,34 @@ describe('SourceControlService', () => { expect(pushResult.find((i) => i.type === 'folders')).toHaveProperty('conflict', false); }); + it('previews the content-import policy on a pull status', async () => { + const files = [mock({ id: 'workflow-1', type: 'workflow' })]; + mockStatusService.getStatus.mockResolvedValue(files); + + await sourceControlService.getStatus(globalAdminUserWithId, { + direction: 'pull', + verbose: false, + preferLocalVersion: false, + }); + + expect(sourceControlImportService.previewContentImportPolicy).toHaveBeenCalledWith( + files, + globalAdminUserWithId.id, + ); + }); + + it('does not preview the content-import policy on a push status', async () => { + mockStatusService.getStatus.mockResolvedValue([]); + + await sourceControlService.getStatus(globalAdminUser, { + direction: 'push', + verbose: false, + preferLocalVersion: false, + }); + + expect(sourceControlImportService.previewContentImportPolicy).not.toHaveBeenCalled(); + }); + it('should throw `ForbiddenError` if direction is pull and user is not allowed to globally pull', async () => { // ARRANGE const user = globalMemberUser; diff --git a/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts b/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts index 4db03a049d5..420c7bbac70 100644 --- a/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts +++ b/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts @@ -18,7 +18,7 @@ import { WorkflowTagMapping, WorkflowTagMappingRepository, } from '@n8n/db'; -import type { PolicyCleared } from '@n8n/decorators'; +import type { ContentImportContext, PolicyCleared } from '@n8n/decorators'; import { Service } from '@n8n/di'; import { PROJECT_ADMIN_ROLE_SLUG } from '@n8n/permissions'; import { In, type DataSourceOptions, type EntityManager } from '@n8n/typeorm'; @@ -27,7 +27,7 @@ import { sleep } from '@n8n/utils/sleep'; import glob from 'fast-glob'; import isEqual from 'lodash/isEqual'; import { Credentials, ErrorReporter, InstanceSettings } from 'n8n-core'; -import type { AutoPublishMode } from 'n8n-workflow'; +import type { AutoPublishMode, INode } from 'n8n-workflow'; import { shouldAutoPublishWorkflow, jsonParse, @@ -75,6 +75,7 @@ import { SOURCE_CONTROL_WORKFLOW_EXPORT_FOLDER, } from './constants'; import { SourceControlContextFactory } from './source-control-context.factory'; +import { getNonDeletedResources } from './source-control-resource-helper'; import { getCredentialExportPath, getDataTableColumnKey, @@ -117,6 +118,25 @@ const toStatusOwner = (project: Project | undefined): StatusResourceOwner | unde return undefined; }; +/** Shared by the pull and its preview, so both evaluate the same input. */ +const workflowImportPolicyContext = ( + workflow: { id: string; name: string; nodes: INode[] }, + projectId: string, +): ContentImportContext => ({ + workflow: { id: workflow.id, name: workflow.name, nodes: workflow.nodes }, + projectId, + transport: 'source-control', +}); + +const credentialImportPolicyContext = ( + credential: { id: string | null; type: string }, + projectId: string, +): ContentImportContext => ({ + credential: { id: credential.id, type: credential.type }, + projectId, + transport: 'source-control', +}); + /** * How long a pull waits for one workflow's unpublish to settle before it gives * up on deleting it. The outbox consumer normally drains within a second. @@ -881,11 +901,10 @@ export class SourceControlImportService { let cleared: PolicyCleared<'contentImport'>; try { cleared = await this.policyEnforcementService.enforceContentImport( - { - workflow: { id, name: importedWorkflow.name, nodes }, - projectId: targetOwnerProject.id, - transport: 'source-control', - }, + workflowImportPolicyContext( + { id, name: importedWorkflow.name, nodes }, + targetOwnerProject.id, + ), { kind: 'user', user: { id: userId } }, ); } catch (error) { @@ -1075,6 +1094,58 @@ export class SourceControlImportService { } } + /** + * Marks each incoming workflow and credential that the pull would skip, by setting its + * `contentImportPolicy`. Evaluates only: it writes nothing and creates no project. + */ + async previewContentImportPolicy(files: SourceControlledFile[], userId: string): Promise { + if (!this.policyEnforcementService.hasChecksFor('contentImport')) return; + + const workflows = getNonDeletedResources(files, 'workflow'); + const credentials = getNonDeletedResources(files, 'credential'); + if (workflows.length === 0 && credentials.length === 0) return; + + const personalProject = await this.projectRepository.getPersonalProjectForUserOrFail(userId); + const projectIdByOwner = new Map>(); + const targetProjectId = async (owner: RemoteResourceOwner | null | undefined) => { + const key = JSON.stringify(owner ?? null); + let projectId = projectIdByOwner.get(key); + if (!projectId) { + projectId = this.findTargetOwnerProjectId(owner, personalProject.id); + projectIdByOwner.set(key, projectId); + } + return await projectId; + }; + + const evaluate = async (file: SourceControlledFile, context: ContentImportContext) => { + const { violations, checkErrors = [] } = + await this.policyEnforcementService.evaluateContentImport(context); + if (violations.length > 0 || checkErrors.length > 0) { + file.contentImportPolicy = { violations, checkErrors }; + } + }; + + await mapInBatches(workflows, SOURCE_CONTROL_READ_FILE_BATCH_SIZE, async (file) => { + const { id, versionId, name, nodes, connections, owner } = await this.parseWorkflowFromFile( + file.file, + ); + // The import skips a file missing these before the policy runs. + if (!id || !versionId || !nodes || !connections) return; + + const projectId = await targetProjectId(owner); + await evaluate(file, workflowImportPolicyContext({ id, name, nodes }, projectId)); + }); + + await mapInBatches(credentials, SOURCE_CONTROL_READ_FILE_BATCH_SIZE, async (file) => { + const { id, type, ownedBy } = jsonParse( + await fsReadFile(file.file, { encoding: 'utf8' }), + ); + + const projectId = await targetProjectId(ownedBy); + await evaluate(file, credentialImportPolicyContext({ id: id ?? null, type }, projectId)); + }); + } + async importCredentialsFromWorkFolder(candidates: SourceControlledFile[], userId: string) { const personalProject = await this.projectRepository.getPersonalProjectForUserOrFail(userId); const candidateIds = candidates.map((c) => c.id); @@ -1137,11 +1208,10 @@ export class SourceControlImportService { let cleared: PolicyCleared<'contentImport'>; try { cleared = await this.policyEnforcementService.enforceContentImport( - { - credential: { id: credential.id ?? null, type }, - projectId: targetOwnerProject.id, - transport: 'source-control', - }, + credentialImportPolicyContext( + { id: credential.id ?? null, type }, + targetOwnerProject.id, + ), { kind: 'user', user: { id: userId } }, ); } catch (error) { @@ -2157,6 +2227,19 @@ export class SourceControlImportService { return isSharedResource ? await this.createTeamProject(remoteOwner) : fallbackProject; } + /** Read-only twin of `resolveTargetOwnerProject`: the pull creates a missing team project with the team id. */ + private async findTargetOwnerProjectId( + remoteOwner: RemoteResourceOwner | null | undefined, + fallbackProjectId: string, + ): Promise { + const project = await this.findOwnerProjectInLocalDb(remoteOwner ?? undefined); + if (project) return project.id; + + const isSharedResource = + remoteOwner && typeof remoteOwner !== 'string' && remoteOwner.type === 'team'; + return isSharedResource ? remoteOwner.teamId : fallbackProjectId; + } + private async findOwnerProjectInLocalDb(owner: RemoteResourceOwner | IWorkflowToImport['owner']) { if (!owner) { return null; diff --git a/packages/cli/src/modules/source-control.ee/source-control.service.ee.ts b/packages/cli/src/modules/source-control.ee/source-control.service.ee.ts index ca002d18fa3..8f19866a6db 100644 --- a/packages/cli/src/modules/source-control.ee/source-control.service.ee.ts +++ b/packages/cli/src/modules/source-control.ee/source-control.service.ee.ts @@ -728,7 +728,15 @@ export class SourceControlService { async getStatus(user: User, options: SourceControlGetStatus) { return await this.workfolderMutex(async () => { await this.sanityCheck(); - return await this.sourceControlStatusService.getStatus(user, options); + const result = await this.sourceControlStatusService.getStatus(user, options); + + // Only the preview pays for this; the pull enforces on its own status call. + if (options.direction === 'pull') { + const files = Array.isArray(result) ? result : result.sourceControlledFiles; + await this.sourceControlImportService.previewContentImportPolicy(files, user.id); + } + + return result; }); } diff --git a/packages/cli/test/integration/environments/source-control-import.service.test.ts b/packages/cli/test/integration/environments/source-control-import.service.test.ts index 36be9cdbda2..8889f0d1e36 100644 --- a/packages/cli/test/integration/environments/source-control-import.service.test.ts +++ b/packages/cli/test/integration/environments/source-control-import.service.test.ts @@ -2249,6 +2249,61 @@ describe('SourceControlImportService', () => { ).resolves.toBeNull(); }); + describe('preview', () => { + const pullStatus = (id: string, file: string) => + mock({ id, file, type: 'workflow', status: 'modified' }); + + beforeEach(() => { + mockPolicyEnforcementService.evaluateContentImport.mockReset(); + mockPolicyEnforcementService.evaluateContentImport.mockResolvedValue({ violations: [] }); + }); + + it("evaluates against the remote owner's project, and writes nothing", async () => { + const importingUser = await getGlobalOwner(); + const team = await createTeamProject('Policy team'); + const workflow: IWorkflowToImport = { + ...makeWorkflowImport(), + owner: { type: 'team', teamId: team.id, teamName: team.name }, + }; + const status = pullStatus(workflow.id, putWorkflowFile(workflow.id, workflow)); + const violation = { kind: 'node-type-unavailable', checkId: 'c', message: 'no' }; + mockPolicyEnforcementService.evaluateContentImport.mockResolvedValue({ + violations: [violation], + }); + + await service.previewContentImportPolicy([status], importingUser.id); + + expect(mockPolicyEnforcementService.evaluateContentImport).toHaveBeenCalledWith( + expect.objectContaining({ projectId: team.id }), + ); + expect(status.contentImportPolicy).toEqual({ violations: [violation], checkErrors: [] }); + expect(mockPolicyEnforcementService.enforceContentImport).not.toHaveBeenCalled(); + await expect( + workflowRepository.findOne({ where: { id: workflow.id } }), + ).resolves.toBeNull(); + }); + + it('does not create the team project a pull would create', async () => { + const importingUser = await getGlobalOwner(); + const teamId = nanoid(); + const workflow: IWorkflowToImport = { + ...makeWorkflowImport(), + owner: { type: 'team', teamId, teamName: 'Not yet here' }, + }; + const file = putWorkflowFile(workflow.id, workflow); + + await service.previewContentImportPolicy( + [pullStatus(workflow.id, file)], + importingUser.id, + ); + + expect(mockPolicyEnforcementService.evaluateContentImport).toHaveBeenCalledWith( + expect.objectContaining({ projectId: teamId }), + ); + await expect(projectRepository.findOneBy({ id: teamId })).resolves.toBeNull(); + }); + }); + // A check that cannot answer is an infrastructure fault, not a property of one workflow. it('fails the pull when the policy layer errors', async () => { const importingUser = await getGlobalOwner(); diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index b530db9e741..4f69e277b05 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -4641,6 +4641,13 @@ "settings.sourceControl.modals.pull.buttons.cancel": "@:_reusableBaseText.cancel", "settings.sourceControl.modals.pull.buttons.save": "Pull and override", "settings.sourceControl.modals.pull.buttons.pull": "Pull", + "settings.sourceControl.modals.pull.buttons.pullCount": "Pull {count} item | Pull {count} items", + "settings.sourceControl.modals.pull.buttons.saveCount": "Pull and override {count} item | Pull and override {count} items", + "settings.sourceControl.modals.pull.policyBlocked": "Blocked by policy", + "settings.sourceControl.modals.pull.policyBlocked.tooltip": "A policy blocks this item. The pull will skip it.", + "settings.sourceControl.modals.pull.policyBlockedCallout": "{count} item is blocked by a policy and won't be pulled. | {count} items are blocked by a policy and won't be pulled.", + "settings.sourceControl.modals.pull.policyCheckFailedCallout": "We couldn't check {count} item against your policies. The pull may fail. Try again in a moment. | We couldn't check {count} items against your policies. The pull may fail. Try again in a moment.", + "settings.sourceControl.modals.pull.policyBlockedGroup": "Not pulled · blocked by policy ({count})", "settings.sourceControl.modals.pull.dataTablesWarning": "The following data table has schema changes. Column removal will cause data loss that cannot be recovered: | The following data table has schema changes. Column removal will cause data loss that cannot be recovered: | The following {count} data tables have schema changes. Column removal will cause data loss that cannot be recovered:", "settings.sourceControl.modals.pullResult.title": "Auto-publish results", "settings.sourceControl.modals.pullResult.tabs.failed": "Failed to publish", diff --git a/packages/frontend/editor-ui/src/app/composables/usePolicyViolationLabels.ts b/packages/frontend/editor-ui/src/app/composables/usePolicyViolationLabels.ts new file mode 100644 index 00000000000..b0efec7461b --- /dev/null +++ b/packages/frontend/editor-ui/src/app/composables/usePolicyViolationLabels.ts @@ -0,0 +1,25 @@ +import type { PolicyViolation } from '@n8n/api-types'; +import { useNodeTypesStore } from '@/app/stores/nodeTypes.store'; +import { useCredentialsStore } from '@/features/credentials/credentials.store'; + +export const NODE_TYPE_SUBJECT = 'nodeType'; +export const CREDENTIAL_TYPE_SUBJECT = 'credentialType'; + +export function usePolicyViolationLabels() { + const nodeTypesStore = useNodeTypesStore(); + const credentialsStore = useCredentialsStore(); + + /** The display name of the node or credential type a violation names, if known. */ + function labelOf({ subject, subjectType }: PolicyViolation): string | undefined { + if (subject === undefined) return undefined; + + if (subjectType === NODE_TYPE_SUBJECT) return nodeTypesStore.getNodeType(subject)?.displayName; + if (subjectType === CREDENTIAL_TYPE_SUBJECT) { + return credentialsStore.getCredentialTypeByName(subject)?.displayName; + } + + return undefined; + } + + return { labelOf }; +} diff --git a/packages/frontend/editor-ui/src/app/composables/usePolicyViolationToast.ts b/packages/frontend/editor-ui/src/app/composables/usePolicyViolationToast.ts index 3c62901a9c7..24f8385b0d6 100644 --- a/packages/frontend/editor-ui/src/app/composables/usePolicyViolationToast.ts +++ b/packages/frontend/editor-ui/src/app/composables/usePolicyViolationToast.ts @@ -4,8 +4,11 @@ import { useToast, type NotificationHandle } from '@n8n/composables/useToast'; import { useTelemetry } from '@n8n/composables/useTelemetry'; import { PolicyViolationList } from '@n8n/frontend-module-type-availability-policies'; import { canvasEventBus } from '@/features/workflows/canvas/canvas.eventBus'; -import { useNodeTypesStore } from '@/app/stores/nodeTypes.store'; -import { useCredentialsStore } from '@/features/credentials/credentials.store'; +import { + CREDENTIAL_TYPE_SUBJECT, + NODE_TYPE_SUBJECT, + usePolicyViolationLabels, +} from '@/app/composables/usePolicyViolationLabels'; import type { INodeUi } from '@/Interface'; import { hasNodeCredentialFilled } from '@/app/utils/nodes/nodeTransforms'; import { useWorkflowsStore } from '@/app/stores/workflows.store'; @@ -20,9 +23,6 @@ type PolicyRefusedAction = 'save' | 'publish' | 'execute'; let activeToast: { handle: NotificationHandle; refusedAction: PolicyRefusedAction } | undefined; -const NODE_TYPE_SUBJECT = 'nodeType'; -const CREDENTIAL_TYPE_SUBJECT = 'credentialType'; - const NODE_MATCHER: Record (node: INodeUi) => boolean) | undefined> = { [NODE_TYPE_SUBJECT]: (subject) => (node) => node.type === subject, @@ -33,20 +33,8 @@ export function usePolicyViolationToast() { const toast = useToast(); const telemetry = useTelemetry(); const workflowsStore = useWorkflowsStore(); - const nodeTypesStore = useNodeTypesStore(); - const credentialsStore = useCredentialsStore(); const uiStore = useUIStore(); - - function displayNameOf({ subject, subjectType }: PolicyViolation): string | undefined { - if (subject === undefined) return undefined; - - if (subjectType === NODE_TYPE_SUBJECT) return nodeTypesStore.getNodeType(subject)?.displayName; - if (subjectType === CREDENTIAL_TYPE_SUBJECT) { - return credentialsStore.getCredentialTypeByName(subject)?.displayName; - } - - return undefined; - } + const { labelOf } = usePolicyViolationLabels(); function isOpenOnCanvas(documentId: WorkflowDocumentId): boolean { return ( @@ -80,7 +68,7 @@ export function usePolicyViolationToast() { duration: 0, message: h(PolicyViolationList, { violations, - labelOf: displayNameOf, + labelOf, isJumpable: (violation: PolicyViolation) => isOpenOnCanvas(documentId) && nodeIdsFor(violation, documentId).length > 0, onJump: (violation: PolicyViolation) => { diff --git a/packages/frontend/editor-ui/src/features/integrations/sourceControl.ee/components/SourceControlPullModal.test.ts b/packages/frontend/editor-ui/src/features/integrations/sourceControl.ee/components/SourceControlPullModal.test.ts index 681c3e71e85..ca31e6ea818 100644 --- a/packages/frontend/editor-ui/src/features/integrations/sourceControl.ee/components/SourceControlPullModal.test.ts +++ b/packages/frontend/editor-ui/src/features/integrations/sourceControl.ee/components/SourceControlPullModal.test.ts @@ -963,4 +963,143 @@ describe('SourceControlPullModal', () => { expect(wrapper.getByText('Active Workflow')).toBeInTheDocument(); }); }); + + describe('policy-blocked items', () => { + const blockedWorkflow: SourceControlledFile = { + ...sampleFiles[0], + id: 'blocked-workflow', + name: 'Blocked Workflow', + status: 'created', + contentImportPolicy: { + violations: [ + { kind: 'node-type-unavailable', checkId: 'test.check', message: 'Not allowed' }, + ], + checkErrors: [], + }, + }; + + it('flags a blocked item and says it will not be pulled', () => { + const { getByTestId } = renderModal({ + pinia, + props: { data: { eventBus, status: [blockedWorkflow, sampleFiles[1]] } }, + }); + + expect(getByTestId('source-control-pull-policy-blocked')).toHaveTextContent( + 'Blocked by policy', + ); + expect(getByTestId('source-control-pull-policy-callout')).toHaveTextContent( + "1 item is blocked by a policy and won't be pulled.", + ); + }); + + it('lists blocked items in their own group after the pullable ones', () => { + const { getAllByTestId, getByTestId } = renderModal({ + pinia, + props: { data: { eventBus, status: [blockedWorkflow, sampleFiles[0]] } }, + }); + + const group = getByTestId('source-control-pull-policy-group'); + const [pullable, blocked] = getAllByTestId('pull-modal-item'); + + expect(group).toHaveTextContent('Not pulled · blocked by policy (1)'); + expect(pullable).toHaveTextContent(sampleFiles[0].name); + expect(blocked).toHaveTextContent('Blocked Workflow'); + expect( + group.compareDocumentPosition(pullable) & Node.DOCUMENT_POSITION_PRECEDING, + ).toBeTruthy(); + expect( + group.compareDocumentPosition(blocked) & Node.DOCUMENT_POSITION_FOLLOWING, + ).toBeTruthy(); + }); + + it('shows no policy flag when nothing is blocked', () => { + const { queryByTestId } = renderModal({ + pinia, + props: { data: { eventBus, status: sampleFiles } }, + }); + + expect(queryByTestId('source-control-pull-policy-blocked')).not.toBeInTheDocument(); + expect(queryByTestId('source-control-pull-policy-callout')).not.toBeInTheDocument(); + expect(queryByTestId('source-control-pull-policy-group')).not.toBeInTheDocument(); + expect( + queryByTestId('source-control-pull-policy-check-failed-callout'), + ).not.toBeInTheDocument(); + }); + + it('puts the number of pullable items on the button for a partial pull', () => { + const { getByTestId } = renderModal({ + pinia, + props: { data: { eventBus, status: [blockedWorkflow, ...sampleFiles] } }, + }); + + expect(getByTestId('force-pull')).toHaveTextContent('Pull 2 items'); + expect(getByTestId('force-pull')).toBeEnabled(); + }); + + it('keeps the plain button label when nothing is blocked', () => { + const { getByTestId } = renderModal({ + pinia, + props: { data: { eventBus, status: sampleFiles } }, + }); + + expect(getByTestId('force-pull')).toHaveTextContent(/^Pull$/); + }); + + it('ignores a blocked modified workflow when choosing the override label', () => { + const { getByTestId } = renderModal({ + pinia, + props: { + data: { + eventBus, + status: [{ ...blockedWorkflow, status: 'modified' }, sampleFiles[1]], + }, + }, + }); + + expect(getByTestId('force-pull')).toHaveTextContent('Pull 1 item'); + }); + + it('disables the button when every item is blocked', () => { + const { getByTestId } = renderModal({ + pinia, + props: { data: { eventBus, status: [blockedWorkflow] } }, + }); + + expect(getByTestId('force-pull')).toBeDisabled(); + }); + + it('warns separately when the policy could not check an item', () => { + const uncheckedWorkflow: SourceControlledFile = { + ...sampleFiles[0], + contentImportPolicy: { + violations: [], + checkErrors: [{ checkId: 'test.check', correlationId: 'corr-1' }], + }, + }; + + const { getByTestId, queryByTestId } = renderModal({ + pinia, + props: { data: { eventBus, status: [uncheckedWorkflow, sampleFiles[1]] } }, + }); + + expect(getByTestId('source-control-pull-policy-check-failed-callout')).toHaveTextContent( + "We couldn't check 1 item against your policies.", + ); + expect(queryByTestId('source-control-pull-policy-group')).not.toBeInTheDocument(); + expect(getByTestId('force-pull')).toBeEnabled(); + }); + + it('does not mark a blocked workflow for auto-publish', async () => { + const wrapper = renderModal({ + pinia, + props: { data: { eventBus, status: [blockedWorkflow] } }, + }); + + await userEvent.click(wrapper.getByTestId('auto-publish-select')); + await waitFor(() => expect(wrapper.getAllByText('On').length).toBeGreaterThan(0)); + await userEvent.click(wrapper.getAllByText('On')[0]); + + expect(wrapper.queryByText('Auto-publish')).not.toBeInTheDocument(); + }); + }); }); diff --git a/packages/frontend/editor-ui/src/features/integrations/sourceControl.ee/components/SourceControlPullModal.vue b/packages/frontend/editor-ui/src/features/integrations/sourceControl.ee/components/SourceControlPullModal.vue index 41e92747bbb..8200217a574 100644 --- a/packages/frontend/editor-ui/src/features/integrations/sourceControl.ee/components/SourceControlPullModal.vue +++ b/packages/frontend/editor-ui/src/features/integrations/sourceControl.ee/components/SourceControlPullModal.vue @@ -19,8 +19,12 @@ import { getPullPriorityByStatus, getStatusText, getStatusTheme, + isBlockedByPolicy, + isPolicyCheckFailed, notifyUserAboutPullWorkFolderOutcome, } from '../sourceControl.utils'; +import { usePolicyViolationLabels } from '@/app/composables/usePolicyViolationLabels'; +import { PolicyViolationList } from '@n8n/frontend-module-type-availability-policies'; import type { SourceControlTreeRow } from '../sourceControl.types'; import { useUIStore } from '@/app/stores/ui.store'; import { type SourceControlledFile, SOURCE_CONTROL_FILE_TYPE } from '@n8n/api-types'; @@ -38,6 +42,7 @@ import { N8nButton, N8nCallout, N8nHeading, + N8nHoverCard, N8nIcon, N8nIconButton, N8nInfoTip, @@ -53,6 +58,9 @@ type SourceControlledFileWithProject = SourceControlledFile & { project?: ProjectListItem; willBeAutoPublished?: boolean; }; +type PullModalRow = + | SourceControlTreeRow + | { id: string; type: 'policyGroup'; count: number; depth: 0 }; const props = defineProps<{ data: { eventBus: EventBus; status?: SourceControlledFile[] }; }>(); @@ -67,6 +75,7 @@ const route = useRoute(); const router = useRouter(); const settingsStore = useSettingsStore(); const uiStore = useUIStore(); +const { labelOf: policyViolationLabelOf } = usePolicyViolationLabels(); const isWorkflowDiffsEnabled = computed(() => settingsStore.settings.enterprise.workflowDiffs); @@ -107,9 +116,13 @@ const autoPublishOptions = computed(() => { ]; }); +// A blocked workflow is not overwritten, so it does not make the pull an override. const hasModifiedWorkflows = computed(() => { return status.value.some( - (f) => f.type === SOURCE_CONTROL_FILE_TYPE.workflow && f.status === 'modified', + (f) => + f.type === SOURCE_CONTROL_FILE_TYPE.workflow && + f.status === 'modified' && + !isBlockedByPolicy(f), ); }); @@ -193,6 +206,7 @@ const sortedWorkflows = computed(() => willBeAutoPublished: file.type === SOURCE_CONTROL_FILE_TYPE.workflow && file.status !== 'deleted' && + !isBlockedByPolicy(file) && shouldAutoPublishWorkflow({ isNewWorkflow: file.status === 'created', isLocalPublished: file.isLocalPublished ?? false, @@ -202,8 +216,12 @@ const sortedWorkflows = computed(() => })), ); +const pullableWorkflows = computed(() => + sortedWorkflows.value.filter((file) => !isBlockedByPolicy(file)), +); + const { visibleWorkflowRows, isFolderCollapsed, toggleFolderCollapse } = - useWorkflowTreeRows(sortedWorkflows); + useWorkflowTreeRows(pullableWorkflows); const sortedCredentials = useSourceControlFileList({ files: computed(() => groupedFilesByType.value[SOURCE_CONTROL_FILE_TYPE.credential] || []), @@ -236,6 +254,22 @@ const dataTableWarningMessage = computed(() => { }); }); +const policyBlockedCount = computed(() => status.value.filter(isBlockedByPolicy).length); +const policyCheckFailedCount = computed(() => status.value.filter(isPolicyCheckFailed).length); + +const LISTED_TYPES: SourceControlledFileType[] = [ + SOURCE_CONTROL_FILE_TYPE.workflow, + SOURCE_CONTROL_FILE_TYPE.credential, + SOURCE_CONTROL_FILE_TYPE.datatable, +]; + +/** Counts only the items listed in the tabs, so the number matches what the user sees. */ +const pullableListedCount = computed( + () => + status.value.filter((file) => LISTED_TYPES.includes(file.type) && !isBlockedByPolicy(file)) + .length, +); + // Active data source based on tab const activeDataSourceFiltered = computed(() => { if (activeTab.value === SOURCE_CONTROL_FILE_TYPE.workflow) { @@ -250,17 +284,29 @@ const activeDataSourceFiltered = computed(() => { return []; }); -const activeRows = computed>>(() => { - if (activeTab.value === SOURCE_CONTROL_FILE_TYPE.workflow) { - return visibleWorkflowRows.value; - } +const toFileRow = (file: SourceControlledFileWithProject): PullModalRow => ({ + id: `file:${file.id}`, + type: 'file', + file, + depth: 0, +}); - return activeDataSourceFiltered.value.map((file) => ({ - id: `file:${file.id}`, - type: 'file' as const, - file, - depth: 0, - })); +/** Blocked items go in their own group at the end, so they read as left out of the pull. */ +const activeRows = computed(() => { + const files = activeDataSourceFiltered.value; + const blocked = files.filter(isBlockedByPolicy); + const pullableRows = + activeTab.value === SOURCE_CONTROL_FILE_TYPE.workflow + ? visibleWorkflowRows.value + : files.filter((file) => !isBlockedByPolicy(file)).map(toFileRow); + + if (blocked.length === 0) return pullableRows; + + return [ + ...pullableRows, + { id: 'policy-blocked-group', type: 'policyGroup', count: blocked.length, depth: 0 }, + ...blocked.map(toFileRow), + ]; }); const filtersNoResultText = computed(() => { @@ -321,6 +367,32 @@ const otherFiles = computed(() => { return others; }); +const hasNothingToPull = computed( + () => policyBlockedCount.value > 0 && pullableListedCount.value === 0 && !otherFiles.value.length, +); + +// The count only shows on a partial pull, where it differs from the full list. +const pullButtonLabel = computed(() => { + const showCount = policyBlockedCount.value > 0 && pullableListedCount.value > 0; + const count = pullableListedCount.value; + + if (hasModifiedWorkflows.value) { + return showCount + ? i18n.baseText('settings.sourceControl.modals.pull.buttons.saveCount', { + adjustToNumber: count, + interpolate: { count: `${count}` }, + }) + : i18n.baseText('settings.sourceControl.modals.pull.buttons.save'); + } + + return showCount + ? i18n.baseText('settings.sourceControl.modals.pull.buttons.pullCount', { + adjustToNumber: count, + interpolate: { count: `${count}` }, + }) + : i18n.baseText('settings.sourceControl.modals.pull.buttons.pull'); +}); + const otherFilesText = computed(() => { const parts: string[] = []; @@ -371,6 +443,7 @@ async function pullWorkfolder() { const workflowResults = pullStatus.filter( (file) => file.type === SOURCE_CONTROL_FILE_TYPE.workflow && + !isBlockedByPolicy(file) && workflowsToAutoPublish.some((w) => w.id === file.id), ); @@ -453,6 +526,32 @@ onMounted(() => { @@ -885,4 +1040,17 @@ onMounted(() => { .rowNoBorder { border-bottom: 0; } + +.policyGroupRow { + padding: var(--spacing--xs) var(--spacing--sm) var(--spacing--2xs); + border-bottom: var(--border); + background-color: var(--color--background--light-2); +} + +.policyCard { + display: flex; + flex-direction: column; + gap: var(--spacing--2xs); + padding: var(--spacing--xs); +} diff --git a/packages/frontend/editor-ui/src/features/integrations/sourceControl.ee/sourceControl.utils.test.ts b/packages/frontend/editor-ui/src/features/integrations/sourceControl.ee/sourceControl.utils.test.ts index 94e6d21f2f6..e8d0337e58f 100644 --- a/packages/frontend/editor-ui/src/features/integrations/sourceControl.ee/sourceControl.utils.test.ts +++ b/packages/frontend/editor-ui/src/features/integrations/sourceControl.ee/sourceControl.utils.test.ts @@ -11,7 +11,7 @@ import { import type { useToast } from '@n8n/composables/useToast'; import type { Router } from 'vue-router'; -import { SOURCE_CONTROL_FILE_STATUS } from '@n8n/api-types'; +import { SOURCE_CONTROL_FILE_STATUS, type SourceControlledFile } from '@n8n/api-types'; describe('source control utils', () => { describe('getStatusText()', () => { @@ -429,5 +429,46 @@ describe('source control utils', () => { }), ); }); + + describe('with items skipped by a policy', () => { + const toast = { showToast: vi.fn() } as unknown as ReturnType; + const router = { + push: vi.fn(), + resolve: vi.fn().mockReturnValue({ href: '/test' }), + } as unknown as Router; + const workflow = (id: string, blocked: boolean): SourceControlledFile => ({ + id, + name: id, + type: 'workflow', + status: 'created', + location: 'remote', + conflict: false, + file: `/${id}.json`, + updatedAt: '2025-01-09T13:12:24.580Z', + ...(blocked && { + contentImportPolicy: { + violations: [{ kind: 'node-type-unavailable', checkId: 'c', message: 'no' }], + checkErrors: [], + }, + }), + }); + + beforeEach(() => { + vi.mocked(toast.showToast).mockClear(); + }); + + it('counts only the pulled items and adds no policy warning', async () => { + await notifyUserAboutPullWorkFolderOutcome( + [workflow('pulled', false), workflow('skipped', true)], + toast, + router, + ); + + expect(toast.showToast).toHaveBeenCalledTimes(1); + expect(toast.showToast).toHaveBeenCalledWith( + expect.objectContaining({ message: '1 Workflow was pulled' }), + ); + }); + }); }); }); diff --git a/packages/frontend/editor-ui/src/features/integrations/sourceControl.ee/sourceControl.utils.ts b/packages/frontend/editor-ui/src/features/integrations/sourceControl.ee/sourceControl.utils.ts index ed64a467f16..efc4cb7079a 100644 --- a/packages/frontend/editor-ui/src/features/integrations/sourceControl.ee/sourceControl.utils.ts +++ b/packages/frontend/editor-ui/src/features/integrations/sourceControl.ee/sourceControl.utils.ts @@ -28,6 +28,14 @@ export const getStatusTheme = (status: SourceControlledFileStatus) => { return statusToBadgeThemeMap[status]; }; +/** A content-import policy blocks this file, so the pull skips it. */ +export const isBlockedByPolicy = (file: SourceControlledFile) => + (file.contentImportPolicy?.violations.length ?? 0) > 0; + +/** A policy check failed on this file, so the pull is likely to fail as a whole. */ +export const isPolicyCheckFailed = (file: SourceControlledFile) => + !isBlockedByPolicy(file) && (file.contentImportPolicy?.checkErrors.length ?? 0) > 0; + type StatusPriority = Partial>; const pullStatusPriority: StatusPriority = { [SOURCE_CONTROL_FILE_STATUS.modified]: 2, @@ -265,19 +273,24 @@ export const notifyUserAboutPullWorkFolderOutcome = async ( return; } + // The pull dialog already showed what a policy blocks, so only what landed is reported. + const pulled = files.filter((file) => !isBlockedByPolicy(file)); + const { credential, tags, variables, datatable, workflow, folders, project } = groupBy( - files, + pulled, 'type', ); + const successToast = { + title: i18n.baseText('settings.sourceControl.pull.success.title'), + message: pullMessage({ credential, tags, variables, datatable, workflow, folders, project }), + type: 'success' as const, + }; + const toastMessages = [ ...(variables?.length ? [createVariablesToast(router)] : []), ...(credential?.length ? [createCredentialsToast(router)] : []), - { - title: i18n.baseText('settings.sourceControl.pull.success.title'), - message: pullMessage({ credential, tags, variables, datatable, workflow, folders, project }), - type: 'success' as const, - }, + ...(pulled.length ? [successToast] : []), ]; for (const message of toastMessages) {