From 532e8ef93058af3f33eb0fef06c286b19639945e Mon Sep 17 00:00:00 2001 From: Guillaume Jacquart Date: Fri, 16 Oct 2026 14:48:56 +0000 Subject: [PATCH] fix(core): Save policy rules that name uninstalled packages (no-changelog) (#41210) Co-authored-by: Claude Opus 5.5 (1M context) Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> --- .../node-type-policy-public.dto.ts | 22 +- .../src/events/type-availability-policies.ts | 3 + .../__tests__/telemetry-event-relay.test.ts | 37 ++-- .../cli/src/events/maps/relay.event-map.ts | 3 +- .../events/relays/telemetry.event-relay.ts | 6 +- .../type-availability-policies/README.md | 11 +- .../__tests__/package-resolver.test.ts | 120 +++++++++++ .../__tests__/policy-rule-lint.test.ts | 98 +++++++++ .../__tests__/policy-shadow-lint.test.ts | 24 +-- .../type-availability-policy.service.test.ts | 192 ++++++++++++++---- .../package-resolver.ts | 33 ++- .../policy-rule-lint.ts | 48 +++++ .../policy-shadow-lint.ts | 3 +- .../type-availability-policy.service.ts | 125 ++++-------- ...al-type-policies.public.controller.test.ts | 2 +- ...de-type-policies.public.controller.test.ts | 2 +- ...DocumentWriteResultPublicDto.generated.yml | 34 +++- ...ffectiveWriteResultPublicDto.generated.yml | 34 +++- ...DocumentWriteResultPublicDto.generated.yml | 34 +++- ...ffectiveWriteResultPublicDto.generated.yml | 34 +++- .../credential-type-policy.extends.test.ts | 16 +- ...ability-policy-instance.controller.test.ts | 26 ++- .../credential-type-policies.test.ts | 11 +- .../public-api/node-type-policies.test.ts | 12 +- 24 files changed, 726 insertions(+), 204 deletions(-) create mode 100644 packages/cli/src/modules/type-availability-policies/__tests__/policy-rule-lint.test.ts create mode 100644 packages/cli/src/modules/type-availability-policies/policy-rule-lint.ts diff --git a/packages/@n8n/api-types/src/dto/type-availability-policies/node-type-policy-public.dto.ts b/packages/@n8n/api-types/src/dto/type-availability-policies/node-type-policy-public.dto.ts index afa1b99555c..caafed5d525 100644 --- a/packages/@n8n/api-types/src/dto/type-availability-policies/node-type-policy-public.dto.ts +++ b/packages/@n8n/api-types/src/dto/type-availability-policies/node-type-policy-public.dto.ts @@ -14,11 +14,15 @@ import { publicApiPaginationSchema } from '../pagination/pagination.dto'; * `PutCredentialTypeInstancePolicyDto`, …), so validation is identical on both surfaces. */ -/** A rule that never matches because an earlier rule in the same list already covers it. */ -const shadowWarningSchema = z.object({ - ruleId: z.string(), - shadowedByRuleId: z.string(), -}); +/** + * A saved rule worth a second look: `shadowed` when an earlier rule in the same list already + * covers it, so it never matches. `notLoaded` when it names a package or type this instance + * has not loaded, so nothing here uses it yet. `NODES_EXCLUDE` and `NODES_INCLUDE` count too. + */ +const ruleWarningSchema = z.discriminatedUnion('kind', [ + z.object({ kind: z.literal('shadowed'), ruleId: z.string(), shadowedByRuleId: z.string() }), + z.object({ kind: z.literal('notLoaded'), ruleId: z.string() }), +]); /** Plain rule arrays on purpose: a response never re-runs the duplicate-id refinement. */ function publicSchemasFor(rule: Rule) { @@ -52,9 +56,9 @@ function publicSchemasFor(rule: Rule) { return { effective: effectivePolicy.shape, - effectiveWriteResult: { ...effectivePolicy.shape, warnings: z.array(shadowWarningSchema) }, + effectiveWriteResult: { ...effectivePolicy.shape, warnings: z.array(ruleWarningSchema) }, document: policyDocument.shape, - documentWriteResult: { policy: policyDocument, warnings: z.array(shadowWarningSchema) }, + documentWriteResult: { policy: policyDocument, warnings: z.array(ruleWarningSchema) }, documentList: { data: z.array(policyDocument), nextCursor: z.string().nullable() }, // The scope's version after a replace, which always bumps it. attachments: { attachments: z.array(attachment), version: z.number().int().positive() }, @@ -67,13 +71,13 @@ const credentialType = publicSchemasFor(credentialTypePolicyRuleSchemas.rule); /** `GET` of a scope's composed node type policy. */ export class PolicyEffectivePublicDto extends Z.class(nodeType.effective) {} -/** `PUT` of a scope's composed node type policy: the new state plus shadowing warnings. */ +/** `PUT` of a scope's composed node type policy: the new state plus rule warnings. */ export class PolicyEffectiveWriteResultPublicDto extends Z.class(nodeType.effectiveWriteResult) {} /** One reusable node type policy document. */ export class PolicyDocumentPublicDto extends Z.class(nodeType.document) {} -/** Create or update of a node type policy document, plus shadowing warnings. */ +/** Create or update of a node type policy document, plus rule warnings. */ export class PolicyDocumentWriteResultPublicDto extends Z.class(nodeType.documentWriteResult) {} export class PolicyDocumentListPublicDto extends Z.class(nodeType.documentList) {} diff --git a/packages/@n8n/telemetry/src/events/type-availability-policies.ts b/packages/@n8n/telemetry/src/events/type-availability-policies.ts index 0b18c02d6b6..fd312994ca8 100644 --- a/packages/@n8n/telemetry/src/events/type-availability-policies.ts +++ b/packages/@n8n/telemetry/src/events/type-availability-policies.ts @@ -90,6 +90,9 @@ export const TYPE_AVAILABILITY_POLICIES_TELEMETRY = defineTelemetryEvents({ shadow_warning_count: z .number() .describe('Saved rules that can never match, because an earlier rule already covers them'), + not_loaded_warning_count: z + .number() + .describe('Saved rules that name a package or type this instance has not loaded'), version: z.number().describe('The scope version after the write'), }), }, diff --git a/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts b/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts index b47831baec2..68840f645c1 100644 --- a/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts +++ b/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts @@ -758,7 +758,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'allow', version: 1 }, rulesBefore: null, rulesAfter: [denyRule], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }); expect(telemetry.track).toHaveBeenCalledWith( @@ -780,7 +781,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'deny', version: 1 }, rulesBefore: null, rulesAfter: [denyRule, allowPackageRule, delegateRule], - warningCount: 2, + shadowWarningCount: 2, + notLoadedWarningCount: 1, }; eventService.emit('node-type-policy-saved', event); @@ -810,6 +812,7 @@ describe('TelemetryEventRelay', () => { allowed_types: ['n8n-nodes-base.code'], previous_rule_count: null, shadow_warning_count: 2, + not_loaded_warning_count: 1, version: 1, }, ); @@ -831,7 +834,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'allow', version: 1 }, rulesBefore: null, rulesAfter: [credentialDenyRule], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }; eventService.emit('node-type-policy-saved', event); @@ -871,7 +875,8 @@ describe('TelemetryEventRelay', () => { rulesAfter: [ { id: 'rule-1', action: 'deny', selector: { kind: 'extends', value: 'oAuth2Api' } }, ], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }); expect(telemetry.track).toHaveBeenCalledWith( @@ -900,7 +905,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'allow', version: 1 }, rulesBefore: null, rulesAfter: [credentialPackageDenyRule], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }; eventService.emit('node-type-policy-saved', event); @@ -927,7 +933,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'deny', version: 1 }, rulesBefore: null, rulesAfter: [], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }; eventService.emit('node-type-policy-saved', event); @@ -945,7 +952,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'deny', version: 5 }, rulesBefore: [denyRule], rulesAfter: [allowPackageRule], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }; eventService.emit('node-type-policy-saved', event); @@ -975,7 +983,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'deny', version: 1 }, rulesBefore: null, rulesAfter: [], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }; eventService.emit('node-type-policy-saved', event); @@ -1097,7 +1106,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'allow', version: 1 }, rulesBefore: null, rulesAfter: [denyRule], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }); expect(telemetry.track).toHaveBeenCalledWith( @@ -1121,7 +1131,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'allow', version: 1 }, rulesBefore: null, rulesAfter: [{ ...allowPackageRule, action: 'deny' }], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }); expect(telemetry.track).toHaveBeenCalledWith( @@ -1151,7 +1162,8 @@ describe('TelemetryEventRelay', () => { action: 'allow' as const, selector: { kind: 'name' as const, value: name }, })), - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }); const properties = vi.mocked(telemetry.track).mock.calls.at(-1)?.[1]; @@ -1174,7 +1186,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'deny', version: 1 }, rulesBefore: null, rulesAfter: [], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }); expect(telemetry.track).toHaveBeenCalledWith( diff --git a/packages/cli/src/events/maps/relay.event-map.ts b/packages/cli/src/events/maps/relay.event-map.ts index 6c96d23ada2..c72226d7603 100644 --- a/packages/cli/src/events/maps/relay.event-map.ts +++ b/packages/cli/src/events/maps/relay.event-map.ts @@ -1450,7 +1450,8 @@ export type RelayEventMap = { after: { defaultAction: PolicyAction; version: number }; rulesBefore: readonly PolicyRule[] | null; rulesAfter: readonly PolicyRule[]; - warningCount: number; + shadowWarningCount: number; + notLoadedWarningCount: number; }; 'node-type-policy-attachments-updated': { diff --git a/packages/cli/src/events/relays/telemetry.event-relay.ts b/packages/cli/src/events/relays/telemetry.event-relay.ts index 8930d2a6408..c0e7c33984e 100644 --- a/packages/cli/src/events/relays/telemetry.event-relay.ts +++ b/packages/cli/src/events/relays/telemetry.event-relay.ts @@ -595,7 +595,8 @@ export class TelemetryEventRelay extends EventRelay { after, rulesBefore, rulesAfter, - warningCount, + shadowWarningCount, + notLoadedWarningCount, }: RelayEventMap['node-type-policy-saved']) { if (!isPolicyKind(kind)) return; @@ -619,7 +620,8 @@ export class TelemetryEventRelay extends EventRelay { ...countSelectorKinds(rulesAfter), ...summarizeTypeAvailability(rulesAfter, after.defaultAction, types, resolvePackage), previous_rule_count: rulesBefore?.length ?? null, - shadow_warning_count: warningCount, + shadow_warning_count: shadowWarningCount, + not_loaded_warning_count: notLoadedWarningCount, version: after.version, }, ); diff --git a/packages/cli/src/modules/type-availability-policies/README.md b/packages/cli/src/modules/type-availability-policies/README.md index edff49d7cde..a1624018073 100644 --- a/packages/cli/src/modules/type-availability-policies/README.md +++ b/packages/cli/src/modules/type-availability-policies/README.md @@ -220,7 +220,15 @@ covers `googleOAuth2Api` and `googleSheetsOAuth2Api`. A `name` rule stays exact, `name oAuth2Api` blocks only the generic OAuth2 credential. Rule order decides as usual: an `allow name googleSheetsOAuth2Api` placed before `deny extends oAuth2Api` keeps Sheets usable, and placed after it, the shadow lint warns that it can never match. A write rejects an `extends` -rule in a node type policy, and one that names a credential type that is not installed. +rule in a node type policy. + +A rule can name a package or type that this instance has not loaded. The write saves it and +returns a `notLoaded` warning for it, next to the `shadowed` warnings. This applies to all +three selectors. Thus, when you uninstall a package, its rules do not block later writes to the +policy. An installed node that `NODES_EXCLUDE` or `NODES_INCLUDE` filters out is not loaded +either. The policy still enforces its rule, because evaluation matches type names, but a policy +cannot make a filtered-out node available. One exception: a write rejects a `package` rule on `n8n-creds-base`, because no package +can ever provide it. ### Reading it on the execution path @@ -324,6 +332,7 @@ through a sealed repository method, and the lint rule that guards that has no al | `credential-type-policy.check.ts` | Credential types: all eight points, including `credentialSave` | | `policy-evaluator.ts` | Pure evaluation: first match per scope, then the instance ∩ project composition | | `policy-shadow-lint.ts` | Warns at write time about rules an earlier rule already covers | +| `policy-rule-lint.ts` | Adds `notLoaded` warnings and merges all write-time warnings in rule order | | `package-resolver.ts` | Resolves a type's package per `kind`, for the `package` selector | | `type-availability-policy.service.ts` | Reads and writes the store, with versioning and row locks | | `type-availability-policy-instance.controller.ts` | Instance scope, documents and attachments, `node-types` | diff --git a/packages/cli/src/modules/type-availability-policies/__tests__/package-resolver.test.ts b/packages/cli/src/modules/type-availability-policies/__tests__/package-resolver.test.ts index 72940387314..4b5b8db1473 100644 --- a/packages/cli/src/modules/type-availability-policies/__tests__/package-resolver.test.ts +++ b/packages/cli/src/modules/type-availability-policies/__tests__/package-resolver.test.ts @@ -3,6 +3,7 @@ import type { NodeTypes } from '@/node-types'; import { CREDENTIAL_TYPES_KIND, NODE_TYPES_KIND } from '../constants'; import { + loadedCheckFor, isCredentialTypeKnown, isPackageInstalled, packageResolverFor, @@ -168,3 +169,122 @@ describe('isCredentialTypeKnown', () => { expect(isCredentialTypeKnown(registry, 'constructor')).toBe(false); }); }); + +describe('loadedCheckFor', () => { + /** Stands in for `NodeTypes.resolveBaseName`: `slackTool` is a synthetic variant of `slack`. */ + const nodeTypes = { + resolveBaseName: (name: string) => ({ + baseName: name === 'n8n-nodes-base.slackTool' ? 'n8n-nodes-base.slack' : name, + isSyntheticTool: name === 'n8n-nodes-base.slackTool', + }), + } as unknown as NodeTypes; + + /** A stand-in for `LoadNodesAndCredentials`, built from just the fields this check reads. */ + function makeFullRegistry(overrides: { + loaders?: LoadNodesAndCredentials['loaders']; + knownNodes?: LoadNodesAndCredentials['knownNodes']; + knownCredentials?: LoadNodesAndCredentials['knownCredentials']; + }): LoadNodesAndCredentials { + return { + loaders: overrides.loaders ?? {}, + knownNodes: overrides.knownNodes ?? {}, + knownCredentials: overrides.knownCredentials ?? {}, + } as unknown as LoadNodesAndCredentials; + } + + describe('package selector', () => { + it('is true for a package that has a registered loader', () => { + const registry = makeFullRegistry({ + loaders: { 'n8n-nodes-base': {} as LoadNodesAndCredentials['loaders'][string] }, + }); + const isLoaded = loadedCheckFor(NODE_TYPES_KIND, nodeTypes, registry); + + expect(isLoaded({ kind: 'package', value: 'n8n-nodes-base' })).toBe(true); + }); + + it('is false for a package with no registered loader', () => { + const isLoaded = loadedCheckFor(NODE_TYPES_KIND, nodeTypes, makeFullRegistry({})); + + expect(isLoaded({ kind: 'package', value: 'n8n-nodes-not-installed' })).toBe(false); + }); + + it('is false for a package named after an inherited Object.prototype property', () => { + const isLoaded = loadedCheckFor(NODE_TYPES_KIND, nodeTypes, makeFullRegistry({})); + + expect(isLoaded({ kind: 'package', value: 'constructor' })).toBe(false); + }); + }); + + describe('extends selector', () => { + it('is true for a known credential type, checked the same way for either kind', () => { + const registry = makeFullRegistry({ + knownCredentials: { oAuth2Api: { className: '', sourcePath: '' } }, + }); + const selector = { kind: 'extends', value: 'oAuth2Api' } as const; + + expect(loadedCheckFor(NODE_TYPES_KIND, nodeTypes, registry)(selector)).toBe(true); + expect(loadedCheckFor(CREDENTIAL_TYPES_KIND, {} as NodeTypes, registry)(selector)).toBe(true); + }); + + it('is false for an unknown credential type', () => { + const isLoaded = loadedCheckFor(CREDENTIAL_TYPES_KIND, {} as NodeTypes, makeFullRegistry({})); + + expect(isLoaded({ kind: 'extends', value: 'unknownApi' })).toBe(false); + }); + + it('is false for a value named after an inherited Object.prototype property', () => { + const isLoaded = loadedCheckFor(CREDENTIAL_TYPES_KIND, {} as NodeTypes, makeFullRegistry({})); + + expect(isLoaded({ kind: 'extends', value: 'constructor' })).toBe(false); + }); + }); + + describe('name selector, node-types kind', () => { + it('is true when the resolved base name is a known node', () => { + const registry = makeFullRegistry({ + knownNodes: { 'n8n-nodes-base.slack': { className: '', sourcePath: '' } }, + }); + const isLoaded = loadedCheckFor(NODE_TYPES_KIND, nodeTypes, registry); + + expect(isLoaded({ kind: 'name', value: 'n8n-nodes-base.slack' })).toBe(true); + }); + + it('is false when the resolved base name is not a known node', () => { + const isLoaded = loadedCheckFor(NODE_TYPES_KIND, nodeTypes, makeFullRegistry({})); + + expect(isLoaded({ kind: 'name', value: 'n8n-nodes-base.slack' })).toBe(false); + }); + + it('treats a synthetic tool variant as installed when its base node is known', () => { + const registry = makeFullRegistry({ + knownNodes: { 'n8n-nodes-base.slack': { className: '', sourcePath: '' } }, + }); + const isLoaded = loadedCheckFor(NODE_TYPES_KIND, nodeTypes, registry); + + expect(isLoaded({ kind: 'name', value: 'n8n-nodes-base.slackTool' })).toBe(true); + }); + + it('is false for a name resolving to an inherited Object.prototype property', () => { + const isLoaded = loadedCheckFor(NODE_TYPES_KIND, nodeTypes, makeFullRegistry({})); + + expect(isLoaded({ kind: 'name', value: 'constructor' })).toBe(false); + }); + }); + + describe('name selector, credential-types kind', () => { + it('is true when the value is a known credential type', () => { + const registry = makeFullRegistry({ + knownCredentials: { slackApi: { className: '', sourcePath: '' } }, + }); + const isLoaded = loadedCheckFor(CREDENTIAL_TYPES_KIND, {} as NodeTypes, registry); + + expect(isLoaded({ kind: 'name', value: 'slackApi' })).toBe(true); + }); + + it('is false when the value is not a known credential type', () => { + const isLoaded = loadedCheckFor(CREDENTIAL_TYPES_KIND, {} as NodeTypes, makeFullRegistry({})); + + expect(isLoaded({ kind: 'name', value: 'unknownApi' })).toBe(false); + }); + }); +}); diff --git a/packages/cli/src/modules/type-availability-policies/__tests__/policy-rule-lint.test.ts b/packages/cli/src/modules/type-availability-policies/__tests__/policy-rule-lint.test.ts new file mode 100644 index 00000000000..3a52ef67640 --- /dev/null +++ b/packages/cli/src/modules/type-availability-policies/__tests__/policy-rule-lint.test.ts @@ -0,0 +1,98 @@ +import { lintPolicyRules, lintRulesForMissingTypes } from '../policy-rule-lint'; +import type { PolicyRule, PolicySelector } from '../policy-rule.types'; + +type RuleOverrides = Partial & Pick; + +const rule = (overrides: RuleOverrides): PolicyRule => ({ + action: 'allow', + ...overrides, +}); + +describe('lintRulesForMissingTypes', () => { + it('returns no warnings when every rule is installed', () => { + const rules = [ + rule({ id: 'a', selector: { kind: 'name', value: 'typeA' } }), + rule({ id: 'b', selector: { kind: 'package', value: 'n8n-nodes-base' } }), + ]; + + expect(lintRulesForMissingTypes(rules, () => true)).toEqual([]); + }); + + it('flags each rule whose selector is not installed, carrying its id', () => { + const rules = [ + rule({ id: 'installed', selector: { kind: 'name', value: 'typeA' } }), + rule({ id: 'missing-one', selector: { kind: 'name', value: 'typeB' } }), + rule({ id: 'missing-two', selector: { kind: 'package', value: 'n8n-nodes-missing' } }), + ]; + const isLoaded = (selector: PolicySelector) => selector.value === 'typeA'; + + expect(lintRulesForMissingTypes(rules, isLoaded)).toEqual([ + { kind: 'notLoaded', ruleId: 'missing-one' }, + { kind: 'notLoaded', ruleId: 'missing-two' }, + ]); + }); + + it('returns no warnings for an empty rule list', () => { + expect(lintRulesForMissingTypes([], () => false)).toEqual([]); + }); +}); + +describe('lintPolicyRules', () => { + const policedType = (name: string) => ({ name, baseName: name }); + const resolvePackage = () => null; + + it('returns no warnings when every rule is installed and none are shadowed', () => { + const rules = [ + rule({ id: 'a', selector: { kind: 'name', value: 'typeA' } }), + rule({ id: 'b', selector: { kind: 'name', value: 'typeB' } }), + ]; + + expect(lintPolicyRules(rules, { resolvePackage, policedType, isLoaded: () => true })).toEqual( + [], + ); + }); + + it('flags not-installed rules that are not shadowed, in rule order', () => { + const rules = [ + rule({ id: 'a', selector: { kind: 'name', value: 'typeA' } }), + rule({ id: 'b', selector: { kind: 'name', value: 'typeB' } }), + ]; + const isLoaded = (selector: PolicySelector) => selector.value !== 'typeB'; + + expect(lintPolicyRules(rules, { resolvePackage, policedType, isLoaded })).toEqual([ + { kind: 'notLoaded', ruleId: 'b' }, + ]); + }); + + it('merges shadow and not-installed warnings, sorted by rule position rather than warning kind', () => { + const rules = [ + rule({ id: 'a', selector: { kind: 'name', value: 'typeA' } }), + rule({ id: 'b', selector: { kind: 'name', value: 'typeB' } }), + // identical selector to `a`, so shadowed by it + rule({ id: 'c', selector: { kind: 'name', value: 'typeA' } }), + ]; + const isLoaded = (selector: PolicySelector) => selector.value !== 'typeB'; + + expect(lintPolicyRules(rules, { resolvePackage, policedType, isLoaded })).toEqual([ + { kind: 'notLoaded', ruleId: 'b' }, + { kind: 'shadowed', ruleId: 'c', shadowedByRuleId: 'a' }, + ]); + }); + + it('lists the shadowed warning before the notLoaded warning for the same rule', () => { + const ruleX = rule({ id: 'x', selector: { kind: 'name', value: 'typeX' } }); + // identical selector to `ruleX`, so shadowed by it, and separately stubbed as not installed + const ruleY = rule({ id: 'y', selector: { kind: 'name', value: 'typeX' } }); + const notLoadedSelectors = new Set([ruleY.selector]); + const isLoaded = (selector: PolicySelector) => !notLoadedSelectors.has(selector); + + expect(lintPolicyRules([ruleX, ruleY], { resolvePackage, policedType, isLoaded })).toEqual([ + { kind: 'shadowed', ruleId: 'y', shadowedByRuleId: 'x' }, + { kind: 'notLoaded', ruleId: 'y' }, + ]); + }); + + it('returns no warnings for an empty rule list', () => { + expect(lintPolicyRules([], { resolvePackage, policedType, isLoaded: () => true })).toEqual([]); + }); +}); diff --git a/packages/cli/src/modules/type-availability-policies/__tests__/policy-shadow-lint.test.ts b/packages/cli/src/modules/type-availability-policies/__tests__/policy-shadow-lint.test.ts index 046ebead2ee..fa7eb81e590 100644 --- a/packages/cli/src/modules/type-availability-policies/__tests__/policy-shadow-lint.test.ts +++ b/packages/cli/src/modules/type-availability-policies/__tests__/policy-shadow-lint.test.ts @@ -26,7 +26,7 @@ describe('lintRulesForShadowing', () => { ]; expect(lintRulesForShadowing(rules)).toEqual([ - { ruleId: 'deny-slack', shadowedByRuleId: 'allow-package' }, + { kind: 'shadowed', ruleId: 'deny-slack', shadowedByRuleId: 'allow-package' }, ]); }); @@ -66,7 +66,7 @@ describe('lintRulesForShadowing', () => { ]; expect(lintRulesForShadowing(rules, undefined, policedType)).toEqual([ - { ruleId: 'deny-gmail-tool', shadowedByRuleId: 'allow-gmail' }, + { kind: 'shadowed', ruleId: 'deny-gmail-tool', shadowedByRuleId: 'allow-gmail' }, ]); }); @@ -109,7 +109,7 @@ describe('lintRulesForShadowing', () => { ]; expect(lintRulesForShadowing(rules)).toEqual([ - { ruleId: 'deny-slack', shadowedByRuleId: 'allow-slack' }, + { kind: 'shadowed', ruleId: 'deny-slack', shadowedByRuleId: 'allow-slack' }, ]); }); @@ -121,8 +121,8 @@ describe('lintRulesForShadowing', () => { ]; expect(lintRulesForShadowing(rules)).toEqual([ - { ruleId: 'second-package', shadowedByRuleId: 'first-package' }, - { ruleId: 'later-name', shadowedByRuleId: 'first-package' }, + { kind: 'shadowed', ruleId: 'second-package', shadowedByRuleId: 'first-package' }, + { kind: 'shadowed', ruleId: 'later-name', shadowedByRuleId: 'first-package' }, ]); }); @@ -137,7 +137,7 @@ describe('lintRulesForShadowing', () => { ]; expect(lintRulesForShadowing(rules, resolvePackage)).toEqual([ - { ruleId: 'deny-slack-api', shadowedByRuleId: 'allow-package' }, + { kind: 'shadowed', ruleId: 'deny-slack-api', shadowedByRuleId: 'allow-package' }, ]); }); @@ -162,7 +162,7 @@ describe('lintRulesForShadowing', () => { ]; expect(lintRulesForShadowing(nameFirst)).toEqual([ - { ruleId: 'later-name', shadowedByRuleId: 'earlier-name' }, + { kind: 'shadowed', ruleId: 'later-name', shadowedByRuleId: 'earlier-name' }, ]); const packageFirst = [ @@ -172,8 +172,8 @@ describe('lintRulesForShadowing', () => { ]; expect(lintRulesForShadowing(packageFirst)).toEqual([ - { ruleId: 'later-name-1', shadowedByRuleId: 'earlier-package' }, - { ruleId: 'later-name-2', shadowedByRuleId: 'earlier-package' }, + { kind: 'shadowed', ruleId: 'later-name-1', shadowedByRuleId: 'earlier-package' }, + { kind: 'shadowed', ruleId: 'later-name-2', shadowedByRuleId: 'earlier-package' }, ]); }); @@ -196,18 +196,18 @@ describe('lintRulesForShadowing', () => { it('flags a later name rule for a type built on an earlier family rule', () => { expect( lint([family('oauth2', 'oAuth2Api'), name('sheets', 'googleSheetsOAuth2Api')]), - ).toEqual([{ ruleId: 'sheets', shadowedByRuleId: 'oauth2' }]); + ).toEqual([{ kind: 'shadowed', ruleId: 'sheets', shadowedByRuleId: 'oauth2' }]); }); it('flags a later name rule for the family base itself', () => { expect(lint([family('oauth2', 'oAuth2Api'), name('generic', 'oAuth2Api')])).toEqual([ - { ruleId: 'generic', shadowedByRuleId: 'oauth2' }, + { kind: 'shadowed', ruleId: 'generic', shadowedByRuleId: 'oauth2' }, ]); }); it('flags a later family rule nested inside an earlier one', () => { expect(lint([family('oauth2', 'oAuth2Api'), family('google', 'googleOAuth2Api')])).toEqual([ - { ruleId: 'google', shadowedByRuleId: 'oauth2' }, + { kind: 'shadowed', ruleId: 'google', shadowedByRuleId: 'oauth2' }, ]); }); diff --git a/packages/cli/src/modules/type-availability-policies/__tests__/type-availability-policy.service.test.ts b/packages/cli/src/modules/type-availability-policies/__tests__/type-availability-policy.service.test.ts index 0b8252a6c30..da61853dc85 100644 --- a/packages/cli/src/modules/type-availability-policies/__tests__/type-availability-policy.service.test.ts +++ b/packages/cli/src/modules/type-availability-policies/__tests__/type-availability-policy.service.test.ts @@ -46,6 +46,8 @@ const RULE: PolicyRule = { selector: { kind: 'name', value: 'n8n-nodes-base.slack' }, }; +const RULE_2: PolicyRule = { ...RULE, id: 'r2' }; + const DELEGATE_RULE: PolicyRule = { id: 'r-delegate', action: 'delegate', @@ -113,6 +115,11 @@ describe('TypeAvailabilityPolicyService', () => { scopeRepository.findScopeKeysByIds.mockResolvedValue([]); // Every fixture rule names `n8n-nodes-base`, so it must resolve as an installed package. loadNodesAndCredentials.loaders = { 'n8n-nodes-base': makeLoader('n8n-nodes-base') }; + // Fixture `name` rules name Slack, so it must resolve as a loaded node. + Object.defineProperty(loadNodesAndCredentials, 'knownNodes', { + configurable: true, + value: { 'n8n-nodes-base.slack': { className: '', sourcePath: '' } }, + }); nodeTypes.resolveBaseName.mockImplementation((name) => ({ baseName: name, isSyntheticTool: false, @@ -315,21 +322,72 @@ describe('TypeAvailabilityPolicyService', () => { const { warnings } = await service.createPolicyDocument(KIND, [RULE, shadowed], 'user-1'); - expect(warnings).toEqual([{ ruleId: 'r2', shadowedByRuleId: 'r1' }]); + expect(warnings).toEqual([{ kind: 'shadowed', ruleId: 'r2', shadowedByRuleId: 'r1' }]); expect(policyRepository.createPolicy).toHaveBeenCalled(); }); - it('rejects a package rule naming a package that is not installed', async () => { + it('saves a package rule naming a package that is not installed, with a warning', async () => { + policyRepository.createPolicy.mockResolvedValue(makePolicy()); const rule: PolicyRule = { id: 'r1', action: 'deny', selector: { kind: 'package', value: 'n8n-nodes-not-installed' }, }; - await expect(service.createPolicyDocument(KIND, [rule], 'user-1')).rejects.toThrow( - 'Package rule names a package that is not installed: n8n-nodes-not-installed', + const { warnings } = await service.createPolicyDocument(KIND, [rule], 'user-1'); + + expect(warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r1' }]); + expect(policyRepository.createPolicy).toHaveBeenCalled(); + }); + + it('saves a name rule naming a node type that is not loaded, with a warning', async () => { + policyRepository.createPolicy.mockResolvedValue(makePolicy()); + const rule: PolicyRule = { + id: 'r1', + action: 'deny', + selector: { kind: 'name', value: 'n8n-nodes-base.notLoaded' }, + }; + + const { warnings } = await service.createPolicyDocument(KIND, [rule], 'user-1'); + + expect(warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r1' }]); + }); + + it('checks a synthetic tool variant rule against its base node', async () => { + policyRepository.createPolicy.mockResolvedValue(makePolicy()); + nodeTypes.resolveBaseName.mockImplementation((name) => ({ + baseName: name.replace(/Tool$/, ''), + isSyntheticTool: name.endsWith('Tool'), + })); + const rules: PolicyRule[] = [ + { id: 'r1', action: 'deny', selector: { kind: 'name', value: 'n8n-nodes-base.slackTool' } }, + { id: 'r2', action: 'deny', selector: { kind: 'name', value: 'n8n-nodes-base.fooTool' } }, + ]; + + const { warnings } = await service.createPolicyDocument(KIND, rules, 'user-1'); + + expect(warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r2' }]); + }); + + it('lists a shadowed warning before a notLoaded warning for the same rule, in rule order', async () => { + policyRepository.createPolicy.mockResolvedValue(makePolicy()); + const missing = (id: string): PolicyRule => ({ + id, + action: 'deny', + selector: { kind: 'name', value: 'n8n-nodes-base.notLoaded' }, + }); + + const { warnings } = await service.createPolicyDocument( + KIND, + [missing('r1'), RULE_2, missing('r3')], + 'user-1', ); - expect(policyRepository.createPolicy).not.toHaveBeenCalled(); + + expect(warnings).toEqual([ + { kind: 'notLoaded', ruleId: 'r1' }, + { kind: 'shadowed', ruleId: 'r3', shadowedByRuleId: 'r1' }, + { kind: 'notLoaded', ruleId: 'r3' }, + ]); }); describe('extends rules', () => { @@ -360,17 +418,37 @@ describe('TypeAvailabilityPolicyService', () => { expect(policyRepository.createPolicy).not.toHaveBeenCalled(); }); - it('rejects an extends rule naming a credential type that is not installed', async () => { - await expect( - service.createPolicyDocument( - CREDENTIAL_TYPES_KIND, - [familyRule('r1', 'notInstalledApi')], - 'user-1', - ), - ).rejects.toThrow( - 'Extends rule names a credential type that is not installed: notInstalledApi', + it('saves an extends rule naming a credential type that is not installed, with a warning', async () => { + policyRepository.createPolicy.mockResolvedValue( + makePolicy({ kind: CREDENTIAL_TYPES_KIND }), ); - expect(policyRepository.createPolicy).not.toHaveBeenCalled(); + + const { warnings } = await service.createPolicyDocument( + CREDENTIAL_TYPES_KIND, + [familyRule('r1', 'notLoadedApi')], + 'user-1', + ); + + expect(warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r1' }]); + }); + + it('saves a credential name rule naming a type that is not installed, with a warning', async () => { + policyRepository.createPolicy.mockResolvedValue( + makePolicy({ kind: CREDENTIAL_TYPES_KIND }), + ); + const rule: PolicyRule = { + id: 'r1', + action: 'deny', + selector: { kind: 'name', value: 'notLoadedApi' }, + }; + + const { warnings } = await service.createPolicyDocument( + CREDENTIAL_TYPES_KIND, + [rule], + 'user-1', + ); + + expect(warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r1' }]); }); it('warns about a derived-type rule placed after the family rule that covers it', async () => { @@ -389,7 +467,7 @@ describe('TypeAvailabilityPolicyService', () => { 'user-1', ); - expect(warnings).toEqual([{ ruleId: 'r2', shadowedByRuleId: 'r1' }]); + expect(warnings).toEqual([{ kind: 'shadowed', ruleId: 'r2', shadowedByRuleId: 'r1' }]); }); }); @@ -438,7 +516,7 @@ describe('TypeAvailabilityPolicyService', () => { }; await expect(service.createPolicyDocument(KIND, [rule], 'user-1')).rejects.toThrow( - 'Package rule names a package that is not installed: n8n-creds-base', + 'Package rule names "n8n-creds-base", which no package provides. Write a credential type rule instead.', ); expect(policyRepository.createPolicy).not.toHaveBeenCalled(); }); @@ -451,19 +529,29 @@ describe('TypeAvailabilityPolicyService', () => { scopeRepository.containsProjectScope.mockResolvedValue(false); }); - it('rejects a package rule naming a package that is not installed, before opening a transaction', async () => { - const rule: PolicyRule = { - id: 'r1', + it('keeps accepting edits to a document whose package rule names an uninstalled package', async () => { + const missingPackage: PolicyRule = { + id: 'r-missing', action: 'deny', - selector: { kind: 'package', value: 'n8n-nodes-not-installed' }, + selector: { kind: 'package', value: 'n8n-nodes-mcp' }, }; - - await expect( - service.updatePolicyDocument(KIND, 'policy-1', [rule], 1, 'user-1'), - ).rejects.toThrow( - 'Package rule names a package that is not installed: n8n-nodes-not-installed', + policyRepository.findByIdAndKind.mockResolvedValue( + makePolicy({ rules: [missingPackage], version: 1 }), ); - expect(transactionRunner.run).not.toHaveBeenCalled(); + policyRepository.updateRules.mockResolvedValue( + makePolicy({ rules: [missingPackage, RULE], version: 2 }), + ); + + const { warnings } = await service.updatePolicyDocument( + KIND, + 'policy-1', + [missingPackage, RULE], + 1, + 'user-1', + ); + + expect(warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r-missing' }]); + expect(policyRepository.updateRules).toHaveBeenCalled(); }); it('rejects a node type rule naming a credential-only node, before opening a transaction', async () => { @@ -868,11 +956,11 @@ describe('TypeAvailabilityPolicyService', () => { attachmentRepository.listScopeIdsAttachedToPolicy.mockResolvedValue(['scope-1']); }); - it('rejects a package rule naming a package that is not installed, before opening a transaction', async () => { + it('rejects a package rule on the credential-only prefix, before opening a transaction', async () => { const rule: PolicyRule = { id: 'r1', action: 'deny', - selector: { kind: 'package', value: 'n8n-nodes-not-installed' }, + selector: { kind: 'package', value: 'n8n-creds-base' }, }; await expect( @@ -883,9 +971,7 @@ describe('TypeAvailabilityPolicyService', () => { 0, 'user-1', ), - ).rejects.toThrow( - 'Package rule names a package that is not installed: n8n-nodes-not-installed', - ); + ).rejects.toThrow('Package rule names "n8n-creds-base"'); expect(transactionRunner.run).not.toHaveBeenCalled(); }); @@ -1137,7 +1223,8 @@ describe('TypeAvailabilityPolicyService', () => { after: { defaultAction: 'deny', version: 1 }, rulesBefore: null, rulesAfter: [RULE], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }); }); @@ -1188,10 +1275,47 @@ describe('TypeAvailabilityPolicyService', () => { after: { defaultAction: 'deny', version: 2 }, rulesBefore: [], rulesAfter: [RULE], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }); }); + it('saves a rule for an uninstalled package, returns its warning and counts it apart', async () => { + const missingPackage: PolicyRule = { + id: 'r-missing', + action: 'deny', + selector: { kind: 'package', value: 'n8n-nodes-mcp' }, + }; + const scope = makeScope({ defaultAction: 'allow', version: 1 }); + scopeRepository.findScopeByKindAndProject.mockResolvedValue(scope); + const existingPolicy = makePolicy({ rules: [missingPackage], version: 1 }); + attachmentRepository.listAttachmentsForScope.mockResolvedValue([ + { policyId: existingPolicy.id, rules: [missingPackage], priority: 0, isFloor: false }, + ]); + policyRepository.findByIdAndKind.mockResolvedValue(existingPolicy); + policyRepository.updateRules.mockResolvedValue( + makePolicy({ rules: [missingPackage, RULE, RULE_2], version: 2 }), + ); + scopeRepository.findScopeById.mockResolvedValue(makeScope({ version: 2 })); + + const result = await service.setEffectivePolicy( + KIND, + null, + { rules: [missingPackage, RULE, RULE_2], defaultAction: 'allow' }, + 1, + 'user-2', + ); + + expect(result.warnings).toEqual([ + { kind: 'notLoaded', ruleId: 'r-missing' }, + { kind: 'shadowed', ruleId: 'r2', shadowedByRuleId: 'r1' }, + ]); + expect(eventService.emit).toHaveBeenCalledWith( + 'node-type-policy-saved', + expect.objectContaining({ shadowWarningCount: 1, notLoadedWarningCount: 1 }), + ); + }); + it('throws NotFoundError when the document update unexpectedly finds no row', async () => { const scope = makeScope({ defaultAction: 'allow', version: 1 }); scopeRepository.findScopeByKindAndProject.mockResolvedValue(scope); diff --git a/packages/cli/src/modules/type-availability-policies/package-resolver.ts b/packages/cli/src/modules/type-availability-policies/package-resolver.ts index f0dfe5707a9..30f6f7dffce 100644 --- a/packages/cli/src/modules/type-availability-policies/package-resolver.ts +++ b/packages/cli/src/modules/type-availability-policies/package-resolver.ts @@ -7,6 +7,7 @@ import { type PackageResolver, type PolicedType, } from './policy-evaluator'; +import type { PolicySelector } from './policy-rule.types'; /** * A credential type name (e.g. `slackApi`) carries no package prefix, unlike a node type @@ -93,7 +94,7 @@ export function policedTypeFor( }); } -/** Whether a credential type is known, checked at write time for an `extends` rule's value. */ +/** Whether a credential type is known, checked at write time for a rule's value. */ export function isCredentialTypeKnown( loadNodesAndCredentials: LoadNodesAndCredentials, typeName: string, @@ -102,8 +103,7 @@ export function isCredentialTypeKnown( } /** - * Whether `packageName` names a package this instance actually has loaded — checked at write - * time so a `package` rule can never be saved unable to match anything. + * Whether `packageName` names a package this instance actually has loaded. * * `Object.hasOwn`, not `in`: `in` also matches an inherited property, so a rule naming * `toString` or `constructor` would pass this check through `Object.prototype` even though @@ -115,3 +115,30 @@ export function isPackageInstalled( ): boolean { return Object.hasOwn(loadNodesAndCredentials.loaders, packageName); } + +/** + * Whether a selector names something this instance has loaded, for the write-time + * `notLoaded` warning. A `name` rule on a synthetic tool variant checks its base node. + */ +export function loadedCheckFor( + kind: string, + nodeTypes: NodeTypes, + loadNodesAndCredentials: LoadNodesAndCredentials, +): (selector: PolicySelector) => boolean { + const policedType = policedTypeFor(kind, nodeTypes, loadNodesAndCredentials); + const isNameKnown = + kind === NODE_TYPES_KIND + ? (name: string) => Object.hasOwn(loadNodesAndCredentials.knownNodes, name) + : (name: string) => isCredentialTypeKnown(loadNodesAndCredentials, name); + + return (selector) => { + switch (selector.kind) { + case 'package': + return isPackageInstalled(loadNodesAndCredentials, selector.value); + case 'extends': + return isCredentialTypeKnown(loadNodesAndCredentials, selector.value); + case 'name': + return isNameKnown(policedType(selector.value).baseName); + } + }; +} diff --git a/packages/cli/src/modules/type-availability-policies/policy-rule-lint.ts b/packages/cli/src/modules/type-availability-policies/policy-rule-lint.ts new file mode 100644 index 00000000000..03d639e43f7 --- /dev/null +++ b/packages/cli/src/modules/type-availability-policies/policy-rule-lint.ts @@ -0,0 +1,48 @@ +import type { PackageResolver, PolicedType } from './policy-evaluator'; +import type { PolicyRule, PolicySelector } from './policy-rule.types'; +import { lintRulesForShadowing, type ShadowWarning } from './policy-shadow-lint'; + +/** + * `ruleId` names a package or type this instance has not loaded. The rule still applies by + * name, for example to an imported workflow. `NODES_EXCLUDE` and `NODES_INCLUDE` count too. + */ +export type NotLoadedWarning = { + readonly kind: 'notLoaded'; + readonly ruleId: string; +}; + +export type PolicyRuleWarning = ShadowWarning | NotLoadedWarning; + +/** + * Warns instead of rejecting: a package can be uninstalled after its rule is saved, and the + * rule must not block later, unrelated writes to the same policy. + */ +export function lintRulesForMissingTypes( + rules: readonly PolicyRule[], + isLoaded: (selector: PolicySelector) => boolean, +): NotLoadedWarning[] { + return rules + .filter((rule) => !isLoaded(rule.selector)) + .map((rule) => ({ kind: 'notLoaded', ruleId: rule.id })); +} + +/** + * Every warning for a rule list, in rule order. A rule with both kinds lists `shadowed` first, + * since the sort is stable. + */ +export function lintPolicyRules( + rules: readonly PolicyRule[], + resolvers: { + resolvePackage: PackageResolver; + policedType: (name: string) => PolicedType; + isLoaded: (selector: PolicySelector) => boolean; + }, +): PolicyRuleWarning[] { + const position = new Map(rules.map((rule, index) => [rule.id, index])); + const warnings: PolicyRuleWarning[] = [ + ...lintRulesForShadowing(rules, resolvers.resolvePackage, resolvers.policedType), + ...lintRulesForMissingTypes(rules, resolvers.isLoaded), + ]; + + return warnings.sort((a, b) => (position.get(a.ruleId) ?? 0) - (position.get(b.ruleId) ?? 0)); +} diff --git a/packages/cli/src/modules/type-availability-policies/policy-shadow-lint.ts b/packages/cli/src/modules/type-availability-policies/policy-shadow-lint.ts index e28f7c25abe..f882a824a3a 100644 --- a/packages/cli/src/modules/type-availability-policies/policy-shadow-lint.ts +++ b/packages/cli/src/modules/type-availability-policies/policy-shadow-lint.ts @@ -10,6 +10,7 @@ import type { PolicyRule, PolicySelector } from './policy-rule.types'; * appears earlier in the same rule list and matches every type that `ruleId` would match. */ export type ShadowWarning = { + readonly kind: 'shadowed'; readonly ruleId: string; readonly shadowedByRuleId: string; }; @@ -113,7 +114,7 @@ export function lintRulesForShadowing( } if (shadowedBy) { - warnings.push({ ruleId: rule.id, shadowedByRuleId: shadowedBy.rule.id }); + warnings.push({ kind: 'shadowed', ruleId: rule.id, shadowedByRuleId: shadowedBy.rule.id }); } const firstOccurrenceByValue = firstByKind[selector.kind]; diff --git a/packages/cli/src/modules/type-availability-policies/type-availability-policy.service.ts b/packages/cli/src/modules/type-availability-policies/type-availability-policy.service.ts index bae30f94d28..5d9b21db26f 100644 --- a/packages/cli/src/modules/type-availability-policies/type-availability-policy.service.ts +++ b/packages/cli/src/modules/type-availability-policies/type-availability-policy.service.ts @@ -6,6 +6,7 @@ import { TransactionRunner, type OperationContext } from '@n8n/db'; import { Service } from '@n8n/di'; import { LRUCache } from 'lru-cache'; import { + CREDENTIAL_ONLY_NODE_PREFIX, getCredentialOnlyNodeCredentialType, isCredentialOnlyNodeType, OperationalError, @@ -22,12 +23,7 @@ import { TypeAvailabilityPolicyRepository } from './database/repositories/type-a import type { TypeAvailabilityPolicy } from './database/entities/type-availability-policy.entity'; import type { TypeAvailabilityPolicyScope } from './database/entities/type-availability-policy-scope.entity'; import { CREDENTIAL_TYPES_KIND, NODE_TYPES_KIND } from './constants'; -import { - isCredentialTypeKnown, - isPackageInstalled, - packageResolverFor, - policedTypeFor, -} from './package-resolver'; +import { loadedCheckFor, packageResolverFor, policedTypeFor } from './package-resolver'; import { evaluateComposedType, orderedAttachments, type ComposedVerdict } from './policy-evaluator'; import type { PolicyAction, @@ -35,7 +31,7 @@ import type { PolicyRule, PolicyScopeKey, } from './policy-rule.types'; -import { lintRulesForShadowing, type ShadowWarning } from './policy-shadow-lint'; +import { lintPolicyRules, type PolicyRuleWarning } from './policy-rule-lint'; /** * A scope that has never been written has no row. That "unconfigured" state behaves as @@ -187,7 +183,7 @@ export type ComposedTypeEvaluation = { type PolicyDocumentWrite = { readonly policy: TypeAvailabilityPolicy; - readonly warnings: readonly ShadowWarning[]; + readonly warnings: readonly PolicyRuleWarning[]; }; function flattenRules(attachments: readonly PolicyAttachment[]): PolicyRule[] { @@ -226,33 +222,6 @@ function assertNoDelegateAtProjectScope( } } -/** - * A `package` rule that names a package this instance never loaded can never match anything — - * worse than not supporting the rule at all, since it looks like it works. Checked at every - * write so a policy is never saved with one. - */ -function assertPackagesInstalled( - rules: readonly PolicyRule[], - loadNodesAndCredentials: LoadNodesAndCredentials, -): void { - const missing = new Set(); - - for (const rule of rules) { - if ( - rule.selector.kind === 'package' && - !isPackageInstalled(loadNodesAndCredentials, rule.selector.value) - ) { - missing.add(rule.selector.value); - } - } - - if (missing.size > 0) { - throw new UserError( - `Package rule names a package that is not installed: ${[...missing].join(', ')}`, - ); - } -} - /** * A credential-only node (`n8n-creds-base.`) exists only in the editor. It is stored and * executed as `n8n-nodes-base.httpRequest`, so a node type rule on the generated name would @@ -273,30 +242,25 @@ function assertNoCredentialOnlyNodeRules(kind: string, rules: readonly PolicyRul } /** - * Only credential types declare `extends`, so the selector means nothing for another kind. An - * unknown base would match nothing, the same trap `assertPackagesInstalled` closes. + * No loader provides the credential-only prefix, so a `package` rule on it can never match on + * any instance. Unlike a package that is merely not installed, nothing can fix it later. */ -function assertExtendsSelectorsValid( - kind: string, - rules: readonly PolicyRule[], - loadNodesAndCredentials: LoadNodesAndCredentials, -): void { - const values = rules.flatMap((rule) => - rule.selector.kind === 'extends' ? [rule.selector.value] : [], - ); - if (values.length === 0) return; - - if (kind !== CREDENTIAL_TYPES_KIND) { - throw new UserError('An "extends" rule is only valid in a credential type policy'); +function assertNoCredentialOnlyPackageRules(rules: readonly PolicyRule[]): void { + for (const rule of rules) { + if (rule.selector.kind === 'package' && rule.selector.value === CREDENTIAL_ONLY_NODE_PREFIX) { + throw new UserError( + `Package rule names "${CREDENTIAL_ONLY_NODE_PREFIX}", which no package provides. Write a credential type rule instead.`, + ); + } } +} - const unknown = new Set( - values.filter((value) => !isCredentialTypeKnown(loadNodesAndCredentials, value)), - ); - if (unknown.size > 0) { - throw new UserError( - `Extends rule names a credential type that is not installed: ${[...unknown].join(', ')}`, - ); +/** Only credential types declare `extends`, so the selector means nothing for another kind. */ +function assertExtendsSelectorsValid(kind: string, rules: readonly PolicyRule[]): void { + if (kind === CREDENTIAL_TYPES_KIND) return; + + if (rules.some((rule) => rule.selector.kind === 'extends')) { + throw new UserError('An "extends" rule is only valid in a credential type policy'); } } @@ -490,19 +454,28 @@ export class TypeAvailabilityPolicyService { return scope; } + /** + * Rejects a rule that is malformed for this kind. A rule that only names something not + * loaded saves, and comes back as a `notLoaded` warning. + */ + private validateAndLintRules(kind: string, rules: readonly PolicyRule[]): PolicyRuleWarning[] { + assertNoCredentialOnlyPackageRules(rules); + assertNoCredentialOnlyNodeRules(kind, rules); + assertExtendsSelectorsValid(kind, rules); + + return lintPolicyRules(rules, { + resolvePackage: packageResolverFor(kind, this.loadNodesAndCredentials), + policedType: policedTypeFor(kind, this.nodeTypes, this.loadNodesAndCredentials), + isLoaded: loadedCheckFor(kind, this.nodeTypes, this.loadNodesAndCredentials), + }); + } + async createPolicyDocument( kind: string, rules: readonly PolicyRule[], updatedBy: string, ): Promise { - assertPackagesInstalled(rules, this.loadNodesAndCredentials); - assertNoCredentialOnlyNodeRules(kind, rules); - assertExtendsSelectorsValid(kind, rules, this.loadNodesAndCredentials); - const warnings = lintRulesForShadowing( - rules, - packageResolverFor(kind, this.loadNodesAndCredentials), - policedTypeFor(kind, this.nodeTypes, this.loadNodesAndCredentials), - ); + const warnings = this.validateAndLintRules(kind, rules); const policy = await this.policyRepository.createPolicy({ kind, rules, updatedBy }, {}); @@ -535,14 +508,7 @@ export class TypeAvailabilityPolicyService { expectedVersion: number, updatedBy: string, ): Promise { - assertPackagesInstalled(rules, this.loadNodesAndCredentials); - assertNoCredentialOnlyNodeRules(kind, rules); - assertExtendsSelectorsValid(kind, rules, this.loadNodesAndCredentials); - const warnings = lintRulesForShadowing( - rules, - packageResolverFor(kind, this.loadNodesAndCredentials), - policedTypeFor(kind, this.nodeTypes, this.loadNodesAndCredentials), - ); + const warnings = this.validateAndLintRules(kind, rules); const result = await this.transactionRunner.run({}, async (ctx) => { let invalidated: PolicyScopeKey[] = []; @@ -791,18 +757,10 @@ export class TypeAvailabilityPolicyService { defaultAction: PolicyAction; version: number; rules: PolicyRule[]; - warnings: readonly ShadowWarning[]; + warnings: readonly PolicyRuleWarning[]; }> { assertNoDelegateAtProjectScope(projectId, input.defaultAction, input.rules); - assertPackagesInstalled(input.rules, this.loadNodesAndCredentials); - assertNoCredentialOnlyNodeRules(kind, input.rules); - assertExtendsSelectorsValid(kind, input.rules, this.loadNodesAndCredentials); - - const warnings = lintRulesForShadowing( - input.rules, - packageResolverFor(kind, this.loadNodesAndCredentials), - policedTypeFor(kind, this.nodeTypes, this.loadNodesAndCredentials), - ); + const warnings = this.validateAndLintRules(kind, input.rules); const result = await this.transactionRunner.run({}, async (ctx) => { const scope = await this.scopeRepository.findScopeByKindAndProject( @@ -991,7 +949,8 @@ export class TypeAvailabilityPolicyService { after: result.scopeAfter, rulesBefore: result.documentBefore?.rules ?? null, rulesAfter: result.documentAfter.rules, - warningCount: warnings.length, + shadowWarningCount: warnings.filter((warning) => warning.kind === 'shadowed').length, + notLoadedWarningCount: warnings.filter((warning) => warning.kind === 'notLoaded').length, }); return { diff --git a/packages/cli/src/public-api/v1/controllers/__tests__/credential-type-policies.public.controller.test.ts b/packages/cli/src/public-api/v1/controllers/__tests__/credential-type-policies.public.controller.test.ts index e562e12c7c3..07aae9803fb 100644 --- a/packages/cli/src/public-api/v1/controllers/__tests__/credential-type-policies.public.controller.test.ts +++ b/packages/cli/src/public-api/v1/controllers/__tests__/credential-type-policies.public.controller.test.ts @@ -162,7 +162,7 @@ describe('CredentialTypePoliciesPublicController handler bodies', () => { defaultAction: 'deny' as const, version: 4, rules, - warnings: [{ ruleId: 'r2', shadowedByRuleId: 'r1' }], + warnings: [{ kind: 'shadowed' as const, ruleId: 'r2', shadowedByRuleId: 'r1' }], }; it('getCredentialTypeInstancePolicy reads the null-project scope and maps the response', async () => { diff --git a/packages/cli/src/public-api/v1/controllers/__tests__/node-type-policies.public.controller.test.ts b/packages/cli/src/public-api/v1/controllers/__tests__/node-type-policies.public.controller.test.ts index 62cf37e7d18..11b27368a0b 100644 --- a/packages/cli/src/public-api/v1/controllers/__tests__/node-type-policies.public.controller.test.ts +++ b/packages/cli/src/public-api/v1/controllers/__tests__/node-type-policies.public.controller.test.ts @@ -157,7 +157,7 @@ describe('NodeTypePoliciesPublicController handler bodies', () => { defaultAction: 'deny' as const, version: 4, rules, - warnings: [{ ruleId: 'r2', shadowedByRuleId: 'r1' }], + warnings: [{ kind: 'shadowed' as const, ruleId: 'r2', shadowedByRuleId: 'r1' }], }; it('fails loudly instead of returning an extends rule, which no node type write accepts', async () => { diff --git a/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyDocumentWriteResultPublicDto.generated.yml b/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyDocumentWriteResultPublicDto.generated.yml index 094930d6fec..ae8d9b803d7 100644 --- a/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyDocumentWriteResultPublicDto.generated.yml +++ b/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyDocumentWriteResultPublicDto.generated.yml @@ -88,15 +88,33 @@ properties: warnings: type: array items: + oneOf: + - type: object + properties: + kind: + type: string + enum: + - shadowed + ruleId: + type: string + shadowedByRuleId: + type: string + required: + - kind + - ruleId + - shadowedByRuleId + - type: object + properties: + kind: + type: string + enum: + - notLoaded + ruleId: + type: string + required: + - kind + - ruleId type: object - properties: - ruleId: - type: string - shadowedByRuleId: - type: string - required: - - ruleId - - shadowedByRuleId required: - policy - warnings diff --git a/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyEffectiveWriteResultPublicDto.generated.yml b/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyEffectiveWriteResultPublicDto.generated.yml index 8c3b41c4f29..88dcc1b54b0 100644 --- a/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyEffectiveWriteResultPublicDto.generated.yml +++ b/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyEffectiveWriteResultPublicDto.generated.yml @@ -73,15 +73,33 @@ properties: warnings: type: array items: + oneOf: + - type: object + properties: + kind: + type: string + enum: + - shadowed + ruleId: + type: string + shadowedByRuleId: + type: string + required: + - kind + - ruleId + - shadowedByRuleId + - type: object + properties: + kind: + type: string + enum: + - notLoaded + ruleId: + type: string + required: + - kind + - ruleId type: object - properties: - ruleId: - type: string - shadowedByRuleId: - type: string - required: - - ruleId - - shadowedByRuleId required: - scopeId - rules diff --git a/packages/cli/src/public-api/v1/shared/spec/schemas/policyDocumentWriteResultPublicDto.generated.yml b/packages/cli/src/public-api/v1/shared/spec/schemas/policyDocumentWriteResultPublicDto.generated.yml index 421646a75f1..f92f77e2be8 100644 --- a/packages/cli/src/public-api/v1/shared/spec/schemas/policyDocumentWriteResultPublicDto.generated.yml +++ b/packages/cli/src/public-api/v1/shared/spec/schemas/policyDocumentWriteResultPublicDto.generated.yml @@ -75,15 +75,33 @@ properties: warnings: type: array items: + oneOf: + - type: object + properties: + kind: + type: string + enum: + - shadowed + ruleId: + type: string + shadowedByRuleId: + type: string + required: + - kind + - ruleId + - shadowedByRuleId + - type: object + properties: + kind: + type: string + enum: + - notLoaded + ruleId: + type: string + required: + - kind + - ruleId type: object - properties: - ruleId: - type: string - shadowedByRuleId: - type: string - required: - - ruleId - - shadowedByRuleId required: - policy - warnings diff --git a/packages/cli/src/public-api/v1/shared/spec/schemas/policyEffectiveWriteResultPublicDto.generated.yml b/packages/cli/src/public-api/v1/shared/spec/schemas/policyEffectiveWriteResultPublicDto.generated.yml index 93194bd4e1c..97bb09c5ef2 100644 --- a/packages/cli/src/public-api/v1/shared/spec/schemas/policyEffectiveWriteResultPublicDto.generated.yml +++ b/packages/cli/src/public-api/v1/shared/spec/schemas/policyEffectiveWriteResultPublicDto.generated.yml @@ -60,15 +60,33 @@ properties: warnings: type: array items: + oneOf: + - type: object + properties: + kind: + type: string + enum: + - shadowed + ruleId: + type: string + shadowedByRuleId: + type: string + required: + - kind + - ruleId + - shadowedByRuleId + - type: object + properties: + kind: + type: string + enum: + - notLoaded + ruleId: + type: string + required: + - kind + - ruleId type: object - properties: - ruleId: - type: string - shadowedByRuleId: - type: string - required: - - ruleId - - shadowedByRuleId required: - scopeId - rules diff --git a/packages/cli/test/integration/policy/credential-type-policy.extends.test.ts b/packages/cli/test/integration/policy/credential-type-policy.extends.test.ts index 73a34fd05f1..d00bc771428 100644 --- a/packages/cli/test/integration/policy/credential-type-policy.extends.test.ts +++ b/packages/cli/test/integration/policy/credential-type-policy.extends.test.ts @@ -183,19 +183,19 @@ describe('a rule on a base credential type', () => { }); describe('writing an extends rule', () => { - test('rejects a base credential type that is not installed', async () => { + test('saves a base credential type that is not installed, with a warning', async () => { const response = await ownerAgent.put('/credential-type-policies/instance').send({ - rules: [ - { id: 'r1', action: 'deny', selector: { kind: 'extends', value: 'notInstalledApi' } }, - ], + rules: [{ id: 'r1', action: 'deny', selector: { kind: 'extends', value: 'notLoadedApi' } }], defaultAction: 'allow', version: 0, }); - expect(response.statusCode).toBe(400); - expect(response.body.message).toBe( - 'Extends rule names a credential type that is not installed: notInstalledApi', - ); + expect(response.statusCode).toBe(200); + const persisted = await ownerAgent.get('/credential-type-policies/instance'); + expect(persisted.body.data.rules).toEqual([ + { id: 'r1', action: 'deny', selector: { kind: 'extends', value: 'notLoadedApi' } }, + ]); + expect(response.body.data.warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r1' }]); }); test('rejects the rule in a node type policy request', async () => { diff --git a/packages/cli/test/integration/policy/type-availability-policy-instance.controller.test.ts b/packages/cli/test/integration/policy/type-availability-policy-instance.controller.test.ts index ab51a01ae98..8e6976ca3cc 100644 --- a/packages/cli/test/integration/policy/type-availability-policy-instance.controller.test.ts +++ b/packages/cli/test/integration/policy/type-availability-policy-instance.controller.test.ts @@ -32,8 +32,7 @@ beforeAll(async () => { owner = await createOwner(); member = await createMember(); - // A `package` rule is rejected at write time unless the package is actually loaded — see - // `n8n-nodes-base` used as a stand-in for "an installed package" below. + // `n8n-nodes-base` stands in for an installed package, so its rules save without a warning. Container.get(LoadNodesAndCredentials).loaders = { 'n8n-nodes-base': mock({ known: { nodes: {}, credentials: {} } }), }; @@ -236,6 +235,29 @@ describe('node type availability policy instance controller admin happy path', ( expect(persisted.body.data.rules).toEqual([rule('r2', 'c.d')]); }); + test('a package rule for an uninstalled package saves with a warning, and does not block later writes', async () => { + const agent = testServer.authAgentFor(owner); + const missingPackage = { + id: 'r-mcp', + action: 'deny' as const, + selector: { kind: 'package' as const, value: 'n8n-nodes-mcp' }, + }; + + const first = await agent + .put('/node-type-policies/instance') + .send({ rules: [missingPackage], defaultAction: 'allow', version: 0 }); + expect(first.statusCode).toBe(200); + expect(first.body.data.warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r-mcp' }]); + + const unrelatedChange = await agent.put('/node-type-policies/instance').send({ + rules: [missingPackage], + defaultAction: 'deny', + version: first.body.data.version, + }); + expect(unrelatedChange.statusCode).toBe(200); + expect(unrelatedChange.body.data.warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r-mcp' }]); + }); + test('a PUT /instance that changes the rules and the default action together moves the version by one', async () => { const agent = testServer.authAgentFor(owner); diff --git a/packages/cli/test/integration/public-api/credential-type-policies.test.ts b/packages/cli/test/integration/public-api/credential-type-policies.test.ts index b17e811bab9..afa32491bf1 100644 --- a/packages/cli/test/integration/public-api/credential-type-policies.test.ts +++ b/packages/cli/test/integration/public-api/credential-type-policies.test.ts @@ -61,6 +61,13 @@ afterEach(async () => { ]); }); +// Fixture rules name `slackApi`; without it loaded, every write would also warn `notLoaded`. +beforeEach(() => { + vi.spyOn(Container.get(LoadNodesAndCredentials), 'knownCredentials', 'get').mockReturnValue({ + slackApi: { className: '', sourcePath: '' }, + }); +}); + /** * Least privilege first: the API-key scope check and the RBAC check are independent, so a key * that lacks the scope and a key whose user lacks the RBAC grant are both rejected with 403. @@ -425,7 +432,9 @@ describe('credential type policies public API policy documents', () => { .send({ rules: [DENY_RULE, { ...DENY_RULE, id: 'r2', action: 'allow' }] }); expect(response.statusCode).toBe(201); - expect(response.body.warnings).toEqual([{ ruleId: 'r2', shadowedByRuleId: 'r1' }]); + expect(response.body.warnings).toEqual([ + { kind: 'shadowed', ruleId: 'r2', shadowedByRuleId: 'r1' }, + ]); }); test('POST rejects duplicate rule ids with 400', async () => { diff --git a/packages/cli/test/integration/public-api/node-type-policies.test.ts b/packages/cli/test/integration/public-api/node-type-policies.test.ts index be6ce527447..7076f8d939f 100644 --- a/packages/cli/test/integration/public-api/node-type-policies.test.ts +++ b/packages/cli/test/integration/public-api/node-type-policies.test.ts @@ -4,6 +4,7 @@ import { LICENSE_FEATURES } from '@n8n/constants'; import type { Project, User } from '@n8n/db'; import { Container } from '@n8n/di'; +import { LoadNodesAndCredentials } from '@/load-nodes-and-credentials'; import { TypeAvailabilityPolicyRepository } from '@/modules/type-availability-policies/database/repositories/type-availability-policy.repository'; import { createMemberWithApiKey, createOwnerWithApiKey } from '@test-integration/db/users'; import * as utils from '@test-integration/utils'; @@ -56,6 +57,13 @@ afterEach(async () => { ]); }); +// Fixture rules name `a.b`; without it loaded, every write would also warn `notLoaded`. +beforeEach(() => { + vi.spyOn(Container.get(LoadNodesAndCredentials), 'knownNodes', 'get').mockReturnValue({ + 'a.b': { className: '', sourcePath: '' }, + }); +}); + /** * Least privilege first: the API-key scope check and the RBAC check are independent, so a key * that lacks the scope and a key whose user lacks the RBAC grant are both rejected with 403. @@ -416,7 +424,9 @@ describe('node type policies public API policy documents', () => { .send({ rules: [DENY_RULE, { ...DENY_RULE, id: 'r2', action: 'allow' }] }); expect(response.statusCode).toBe(201); - expect(response.body.warnings).toEqual([{ ruleId: 'r2', shadowedByRuleId: 'r1' }]); + expect(response.body.warnings).toEqual([ + { kind: 'shadowed', ruleId: 'r2', shadowedByRuleId: 'r1' }, + ]); }); test('POST rejects duplicate rule ids with 400', async () => {