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/eslint-plugin-community-nodes/README.md b/packages/@n8n/eslint-plugin-community-nodes/README.md index db539c78ff6..96a5fd63178 100644 --- a/packages/@n8n/eslint-plugin-community-nodes/README.md +++ b/packages/@n8n/eslint-plugin-community-nodes/README.md @@ -77,6 +77,7 @@ export default [ | [node-connection-type-literal](docs/rules/node-connection-type-literal.md) | Disallow string literals in node description `inputs`/`outputs` — use `NodeConnectionTypes` enum instead | ✅ ☑️ | | 🔧 | | | | [node-registration-complete](docs/rules/node-registration-complete.md) | Ensure every `.node.ts` file in the `nodes/` directory is registered in the "n8n.nodes" array of package.json | | ✅ ☑️ | | | | | [node-usable-as-tool](docs/rules/node-usable-as-tool.md) | Ensure node classes have usableAsTool property | ✅ ☑️ | | 🔧 | | | +| [open-ended-node-fields](docs/rules/open-ended-node-fields.md) | Discourage optional open-ended JSON fields in node properties | | ✅ ☑️ | | | | | [package-name-convention](docs/rules/package-name-convention.md) | Enforce correct package naming convention for n8n community nodes | ✅ ☑️ | | | 💡 | | | [require-files-array](docs/rules/require-files-array.md) | Require a non-empty "files" array in package.json | | ✅ ☑️ | | | | | [require-homepage](docs/rules/require-homepage.md) | Require a "homepage" field with a valid URL in package.json | | ✅ ☑️ | | | | diff --git a/packages/@n8n/eslint-plugin-community-nodes/docs/rules/open-ended-node-fields.md b/packages/@n8n/eslint-plugin-community-nodes/docs/rules/open-ended-node-fields.md new file mode 100644 index 00000000000..5907ae66bdc --- /dev/null +++ b/packages/@n8n/eslint-plugin-community-nodes/docs/rules/open-ended-node-fields.md @@ -0,0 +1,35 @@ +# Discourage optional open-ended JSON fields in node properties (`@n8n/community-nodes/open-ended-node-fields`) + +⚠️ This rule _warns_ in the following configs: ✅ `recommended`, ☑️ `recommendedWithoutN8nCloudSupport`. + + + +## Rule Details + +This rule warns about optional top-level JSON properties in node descriptions. A JSON configuration field can hide parameters from the editor. Typed parameters make configuration easier to find, validate, and change. + +Place optional parameters under Additional Options or Additional Fields. The rule does not report required JSON properties or typed collection options. + +## Examples + +### Incorrect + +```typescript +properties: [ + { displayName: 'Configuration', name: 'configuration', type: 'json', default: '{}' }, +]; +``` + +### Correct + +```typescript +properties: [ + { + displayName: 'Additional Fields', + name: 'additionalFields', + type: 'collection', + default: {}, + options: [{ displayName: 'Limit', name: 'limit', type: 'number', default: 10 }], + }, +]; +``` diff --git a/packages/@n8n/eslint-plugin-community-nodes/src/plugin.ts b/packages/@n8n/eslint-plugin-community-nodes/src/plugin.ts index 622119334fb..049c9234493 100644 --- a/packages/@n8n/eslint-plugin-community-nodes/src/plugin.ts +++ b/packages/@n8n/eslint-plugin-community-nodes/src/plugin.ts @@ -29,6 +29,7 @@ const configs = { '@n8n/community-nodes/no-deprecated-workflow-functions': 'error', '@n8n/community-nodes/no-emoji-in-options': 'error', '@n8n/community-nodes/node-usable-as-tool': 'error', + '@n8n/community-nodes/open-ended-node-fields': 'warn', '@n8n/community-nodes/package-name-convention': 'error', '@n8n/community-nodes/credential-test-required': 'error', '@n8n/community-nodes/no-credential-reuse': 'error', @@ -79,6 +80,7 @@ const configs = { '@n8n/community-nodes/no-deprecated-workflow-functions': 'error', '@n8n/community-nodes/no-emoji-in-options': 'error', '@n8n/community-nodes/node-usable-as-tool': 'error', + '@n8n/community-nodes/open-ended-node-fields': 'warn', '@n8n/community-nodes/package-name-convention': 'error', '@n8n/community-nodes/credential-test-required': 'error', '@n8n/community-nodes/no-credential-reuse': 'error', diff --git a/packages/@n8n/eslint-plugin-community-nodes/src/rules/index.ts b/packages/@n8n/eslint-plugin-community-nodes/src/rules/index.ts index d178a210639..cb51527cb18 100644 --- a/packages/@n8n/eslint-plugin-community-nodes/src/rules/index.ts +++ b/packages/@n8n/eslint-plugin-community-nodes/src/rules/index.ts @@ -31,6 +31,7 @@ import { NodeClassDescriptionIconMissingRule } from './node-class-description-ic import { NodeConnectionTypeLiteralRule } from './node-connection-type-literal.js'; import { NodeRegistrationCompleteRule } from './node-registration-complete.js'; import { NodeUsableAsToolRule } from './node-usable-as-tool.js'; +import { OpenEndedNodeFieldsRule } from './open-ended-node-fields.js'; import { PackageNameConventionRule } from './package-name-convention.js'; import { RequireFilesArrayRule } from './require-files-array.js'; import { RequireHomepageRule } from './require-homepage.js'; @@ -54,6 +55,7 @@ export const rules = { 'no-deprecated-workflow-functions': NoDeprecatedWorkflowFunctionsRule, 'no-emoji-in-options': NoEmojiInOptionsRule, 'node-usable-as-tool': NodeUsableAsToolRule, + 'open-ended-node-fields': OpenEndedNodeFieldsRule, 'package-name-convention': PackageNameConventionRule, 'credential-test-required': CredentialTestRequiredRule, 'credential-unnecessary-password': CredentialUnnecessaryPasswordRule, diff --git a/packages/@n8n/eslint-plugin-community-nodes/src/rules/open-ended-node-fields.test.ts b/packages/@n8n/eslint-plugin-community-nodes/src/rules/open-ended-node-fields.test.ts new file mode 100644 index 00000000000..5b50fe308cc --- /dev/null +++ b/packages/@n8n/eslint-plugin-community-nodes/src/rules/open-ended-node-fields.test.ts @@ -0,0 +1,122 @@ +import { Linter } from 'eslint'; +import { describe, expect, it } from 'vitest'; + +import { configs } from '../plugin.js'; + +const linter = new Linter(); + +function createNodeCode(property: string): string { + return ` +export class TestNode extends Node { + description = { + displayName: 'Test Node', + name: 'testNode', + group: ['transform'], + version: 1, + inputs: ['main'], + outputs: ['main'], + properties: [${property}], + }; +}`; +} + +function lintNode(code: string, config: keyof typeof configs) { + return linter.verify(code, [{ ...configs[config], files: ['**/*.node.ts'] }], { + filename: 'src/rules/TestNode.node.ts', + }); +} + +describe.each(['recommended', 'recommendedWithoutN8nCloudSupport'] as const)( + '%s node property review', + (config) => { + // CE-1879: Warn when an optional JSON field replaces typed node parameters. + it('warns about an optional JSON configuration catch-all', () => { + const code = createNodeCode(`{ + displayName: 'Configuration', + name: 'configuration', + type: 'json', + required: false, + default: '{}', + }`); + + const warnings = lintNode(code, config).filter( + ({ severity, message }) => severity === 1 && /Additional (Options|Fields)/i.test(message), + ); + + expect(warnings).toEqual([ + expect.objectContaining({ + severity: 1, + message: expect.stringMatching(/Additional (Options|Fields)/i), + }), + ]); + }); + + it('does not warn about typed parameters inside Additional Fields', () => { + const code = createNodeCode(`{ + displayName: 'Additional Fields', + name: 'additionalFields', + type: 'collection', + default: {}, + options: [{ displayName: 'Limit', name: 'limit', type: 'number', default: 10 }], + }`); + + const messages = lintNode(code, config); + expect(messages).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + ruleId: '@n8n/community-nodes/require-node-description-fields', + }), + ]), + ); + + const warnings = messages.filter( + ({ severity, message }) => severity === 1 && /Additional (Options|Fields)/i.test(message), + ); + + expect(warnings).toEqual([]); + }); + + it('warns when an open-ended JSON field does not declare required', () => { + const code = createNodeCode(`{ + displayName: 'Configuration', + name: 'configuration', + type: 'json', + default: '{}', + }`); + + expect( + lintNode(code, config).filter( + ({ ruleId }) => ruleId === '@n8n/community-nodes/open-ended-node-fields', + ), + ).toHaveLength(1); + }); + + it('does not warn about required JSON fields', () => { + const code = createNodeCode(`{ + displayName: 'Request Body', + name: 'body', + type: 'json', + required: true, + default: '{}', + }`); + + expect( + lintNode(code, config).filter( + ({ ruleId }) => ruleId === '@n8n/community-nodes/open-ended-node-fields', + ), + ).toEqual([]); + }); + + it('does not inspect classes that are not nodes', () => { + const code = `class Helper { + description = { properties: [{ type: 'json', required: false }] }; + }`; + + expect( + lintNode(code, config).filter( + ({ ruleId }) => ruleId === '@n8n/community-nodes/open-ended-node-fields', + ), + ).toEqual([]); + }); + }, +); diff --git a/packages/@n8n/eslint-plugin-community-nodes/src/rules/open-ended-node-fields.ts b/packages/@n8n/eslint-plugin-community-nodes/src/rules/open-ended-node-fields.ts new file mode 100644 index 00000000000..58e99caca0e --- /dev/null +++ b/packages/@n8n/eslint-plugin-community-nodes/src/rules/open-ended-node-fields.ts @@ -0,0 +1,55 @@ +import { AST_NODE_TYPES } from '@typescript-eslint/utils'; + +import { + createRule, + findArrayLiteralProperty, + findNodeDescriptionObject, + findObjectProperty, + getBooleanLiteralValue, + getStringLiteralValue, + isFileType, + isNodeTypeClass, +} from '../utils/index.js'; + +export const OpenEndedNodeFieldsRule = createRule({ + name: 'open-ended-node-fields', + meta: { + type: 'suggestion', + docs: { + description: 'Discourage optional open-ended JSON fields in node properties', + }, + messages: { + optionalJson: + 'Optional JSON fields can hide configuration. Use typed parameters under Additional Options or Additional Fields instead.', + }, + schema: [], + }, + defaultOptions: [], + create(context) { + if (!isFileType(context.filename, '.node.ts')) return {}; + + return { + ClassDeclaration(node) { + if (!isNodeTypeClass(node)) return; + + const description = findNodeDescriptionObject(node); + if (!description) return; + + const properties = findArrayLiteralProperty(description, 'properties'); + if (!properties) return; + + for (const property of properties.elements) { + if (property?.type !== AST_NODE_TYPES.ObjectExpression) continue; + + const type = findObjectProperty(property, 'type'); + if (getStringLiteralValue(type?.value ?? null) !== 'json') continue; + + const required = findObjectProperty(property, 'required'); + if (getBooleanLiteralValue(required?.value ?? null) === true) continue; + + context.report({ node: property, messageId: 'optionalJson' }); + } + }, + }; + }, +}); 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..58407d6ba3a 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 @@ -15,7 +15,7 @@ import type { TypeAvailabilityPolicyScopeRepository } from '../database/reposito import type { TypeAvailabilityPolicyRepository } from '../database/repositories/type-availability-policy.repository'; import { TypeAvailabilityPolicy } from '../database/entities/type-availability-policy.entity'; import { TypeAvailabilityPolicyScope } from '../database/entities/type-availability-policy-scope.entity'; -import type { PolicyRule } from '../policy-rule.types'; +import type { PolicyAttachment, PolicyRule } from '../policy-rule.types'; import { LOCAL_READ_MAX_ENTRIES, TypeAvailabilityPolicyService, @@ -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, @@ -159,6 +166,85 @@ describe('TypeAvailabilityPolicyService', () => { }); }); + describe('getAttachments', () => { + it.each([ + [KIND, null], + [KIND, 'project-1'], + [CREDENTIAL_TYPES_KIND, null], + [CREDENTIAL_TYPES_KIND, 'project-1'], + ])('reads %s attachments for project %s in evaluation order', async (kind, projectId) => { + const ctx: OperationContext = {}; + const scope = makeScope({ kind, projectId, version: 7 }); + const normalLate: PolicyAttachment = { + policyId: 'normal-late', + rules: [RULE], + priority: 10, + isFloor: false, + }; + const floorLate: PolicyAttachment = { + policyId: 'floor-late', + rules: [], + priority: 20, + isFloor: true, + }; + const normalEarly: PolicyAttachment = { + policyId: 'normal-early', + rules: [], + priority: -5, + isFloor: false, + }; + const floorEarly: PolicyAttachment = { + policyId: 'floor-early', + rules: [RULE], + priority: 0, + isFloor: true, + }; + const stored = [normalLate, floorLate, normalEarly, floorEarly]; + scopeRepository.findScopeByIdAndKind.mockResolvedValue(scope); + attachmentRepository.listAttachmentsForScope.mockResolvedValue(stored); + + const result = await service.getAttachments(kind, scope.id, ctx); + + expect(result).toEqual({ + attachments: [floorEarly, floorLate, normalEarly, normalLate], + version: 7, + }); + expect(stored).toEqual([normalLate, floorLate, normalEarly, floorEarly]); + expect(scopeRepository.findScopeByIdAndKind).toHaveBeenCalledWith(scope.id, kind, ctx); + expect(attachmentRepository.listAttachmentsForScope).toHaveBeenCalledWith(scope.id, ctx); + expect(scopeRepository.findScopeByIdAndKind.mock.invocationCallOrder[0]).toBeLessThan( + attachmentRepository.listAttachmentsForScope.mock.invocationCallOrder[0], + ); + expect(transactionRunner.run).not.toHaveBeenCalled(); + expect(eventService.emit).not.toHaveBeenCalled(); + expect(cacheService.get).not.toHaveBeenCalled(); + expect(cacheService.set).not.toHaveBeenCalled(); + expect(cacheService.deleteMany).not.toHaveBeenCalled(); + }); + + it.each([KIND, CREDENTIAL_TYPES_KIND])('rejects an unknown scope for %s', async (kind) => { + scopeRepository.findScopeByIdAndKind.mockResolvedValue(null); + + await expect(service.getAttachments(kind, 'missing')).rejects.toThrow(NotFoundError); + expect(attachmentRepository.listAttachmentsForScope).not.toHaveBeenCalled(); + }); + + it.each([KIND, CREDENTIAL_TYPES_KIND])( + 'rejects a scope of another kind for %s', + async (kind) => { + const otherKind = kind === KIND ? CREDENTIAL_TYPES_KIND : KIND; + const scope = makeScope({ kind: otherKind }); + scopeRepository.findScopeByIdAndKind.mockImplementation(async (id, requestedKind) => + id === scope.id && requestedKind === scope.kind ? scope : null, + ); + + await expect(service.getAttachments(kind, scope.id)).rejects.toThrow(NotFoundError); + expect(scopeRepository.findScopeByIdAndKind).toHaveBeenCalledWith(scope.id, kind, {}); + expect(attachmentRepository.listAttachmentsForScope).not.toHaveBeenCalled(); + }, + ); + }); + describe('setDefaultAction', () => { it('rejects a delegate defaultAction at project scope before opening a transaction', async () => { await expect( @@ -315,21 +401,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 +497,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 +546,7 @@ describe('TypeAvailabilityPolicyService', () => { 'user-1', ); - expect(warnings).toEqual([{ ruleId: 'r2', shadowedByRuleId: 'r1' }]); + expect(warnings).toEqual([{ kind: 'shadowed', ruleId: 'r2', shadowedByRuleId: 'r1' }]); }); }); @@ -438,7 +595,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 +608,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 () => { @@ -818,6 +985,42 @@ describe('TypeAvailabilityPolicyService', () => { expect(result.version).toBe(2); }); + it.each([ + [KIND, null], + [KIND, 'project-1'], + [CREDENTIAL_TYPES_KIND, null], + [CREDENTIAL_TYPES_KIND, 'project-1'], + ])('returns %s attachments for project %s in evaluation order', async (kind, projectId) => { + const scope = makeScope({ kind, projectId, version: 1 }); + const stored: PolicyAttachment[] = [ + { policyId: 'normal-late', rules: [RULE], priority: 10, isFloor: false }, + { policyId: 'floor-late', rules: [], priority: 20, isFloor: true }, + { policyId: 'normal-early', rules: [], priority: -5, isFloor: false }, + { policyId: 'floor-early', rules: [RULE], priority: 0, isFloor: true }, + ]; + scopeRepository.findScopeByIdAndKind.mockResolvedValueOnce(scope); + scopeRepository.findScopeById.mockResolvedValueOnce( + makeScope({ kind, projectId, version: 2 }), + ); + policyRepository.findManyByIds.mockResolvedValue([]); + attachmentRepository.listAttachmentsForScope + .mockResolvedValueOnce([]) + .mockResolvedValueOnce(stored); + + const result = await service.replaceAttachments( + kind, + scope.id, + stored.map(({ policyId, priority, isFloor }) => ({ policyId, priority, isFloor })), + scope.version, + 'user-1', + ); + + expect(result).toEqual({ + attachments: [stored[3], stored[1], stored[2], stored[0]], + version: 2, + }); + }); + it('rejects attaching a document with a delegate rule to a project scope, and writes nothing', async () => { scopeRepository.findScopeByIdAndKind.mockResolvedValue(makeScope({ projectId: 'project-1' })); policyRepository.findManyByIds.mockResolvedValue([ @@ -868,11 +1071,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 +1086,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 +1338,8 @@ describe('TypeAvailabilityPolicyService', () => { after: { defaultAction: 'deny', version: 1 }, rulesBefore: null, rulesAfter: [RULE], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }); }); @@ -1188,10 +1390,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/credential-type-policy-instance.controller.ts b/packages/cli/src/modules/type-availability-policies/credential-type-policy-instance.controller.ts index cd5f27da477..1b0d891e49a 100644 --- a/packages/cli/src/modules/type-availability-policies/credential-type-policy-instance.controller.ts +++ b/packages/cli/src/modules/type-availability-policies/credential-type-policy-instance.controller.ts @@ -143,6 +143,17 @@ export class CredentialTypePolicyInstanceController { return { success: true }; } + @Get('/scopes/:scopeId/attachments') + @Licensed(LICENSE_FEATURES.TYPE_AVAILABILITY_POLICIES) + @GlobalScope('credentialTypePolicy:manage') + async getAttachments( + _req: AuthenticatedRequest, + _res: Response, + @Param('scopeId') scopeId: string, + ) { + return await this.service.getAttachments(CREDENTIAL_TYPES_KIND, scopeId); + } + @Put('/scopes/:scopeId/attachments') @Licensed(LICENSE_FEATURES.TYPE_AVAILABILITY_POLICIES) @GlobalScope('credentialTypePolicy:manage') 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-instance.controller.ts b/packages/cli/src/modules/type-availability-policies/type-availability-policy-instance.controller.ts index 32c27af3da6..ee6c4f231ee 100644 --- a/packages/cli/src/modules/type-availability-policies/type-availability-policy-instance.controller.ts +++ b/packages/cli/src/modules/type-availability-policies/type-availability-policy-instance.controller.ts @@ -143,6 +143,17 @@ export class TypeAvailabilityPolicyInstanceController { return { success: true }; } + @Get('/scopes/:scopeId/attachments') + @Licensed(LICENSE_FEATURES.TYPE_AVAILABILITY_POLICIES) + @GlobalScope('nodeTypePolicy:manage') + async getAttachments( + _req: AuthenticatedRequest, + _res: Response, + @Param('scopeId') scopeId: string, + ) { + return await this.service.getAttachments(NODE_TYPES_KIND, scopeId); + } + @Put('/scopes/:scopeId/attachments') @Licensed(LICENSE_FEATURES.TYPE_AVAILABILITY_POLICIES) @GlobalScope('nodeTypePolicy:manage') 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..fc2f74176cd 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 @@ -146,6 +142,11 @@ export type EffectivePolicy = { readonly attachments: readonly PolicyAttachment[]; }; +export type ScopeAttachments = { + readonly attachments: readonly PolicyAttachment[]; + readonly version: number; +}; + /** The allow-all state of a scope with no row, for a composition with no project to read. */ const UNCONFIGURED_PROJECT: Omit = { scopeId: null, @@ -187,7 +188,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 +227,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 +247,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 +459,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 +513,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[] = []; @@ -677,6 +648,21 @@ export class TypeAvailabilityPolicyService { return await this.policyRepository.findPageByKind(kind, offset, limit, {}); } + async getAttachments( + kind: string, + scopeId: string, + ctx: OperationContext = {}, + ): Promise { + const scope = await this.scopeRepository.findScopeByIdAndKind(scopeId, kind, ctx); + if (!scope) { + throw new NotFoundError(`Policy scope not found: ${scopeId}`); + } + + const attachments = await this.attachmentRepository.listAttachmentsForScope(scopeId, ctx); + + return { attachments: orderedAttachments(attachments), version: scope.version }; + } + /** * Replaces every attachment on one scope, keyed by `scopeId` rather than `(kind, * projectId)` — a caller replacing attachments already holds a scope id from a prior @@ -699,7 +685,7 @@ export class TypeAvailabilityPolicyService { attachments: readonly AttachmentInput[], expectedVersion: number, updatedBy: string, - ): Promise<{ attachments: readonly PolicyAttachment[]; version: number }> { + ): Promise { assertNoDuplicateAttachmentSlots(attachments); const result = await this.transactionRunner.run({}, async (ctx) => { @@ -768,7 +754,7 @@ export class TypeAvailabilityPolicyService { after: { attachments: result.after, version: result.versionAfter }, }); - return { attachments: result.after, version: result.versionAfter }; + return { attachments: orderedAttachments(result.after), version: result.versionAfter }; } /** @@ -791,18 +777,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 +969,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..8476959e843 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 @@ -43,6 +43,7 @@ describe('CredentialTypePoliciesPublicController route metadata', () => { 'createCredentialTypePolicyDocument', 'deleteCredentialTypePolicyDocument', 'getCredentialTypeInstancePolicy', + 'getCredentialTypePolicyAttachments', 'getCredentialTypePolicyDocument', 'getCredentialTypeProjectPolicy', 'listCredentialTypePolicyDocuments', @@ -119,6 +120,9 @@ describe('CredentialTypePoliciesPublicController with the module disabled', () = await expect(controller.getCredentialTypePolicyDocument(req, res, 'policy-id')).rejects.toThrow( ServiceUnavailableError, ); + await expect( + controller.getCredentialTypePolicyAttachments(req, res, 'scope-id'), + ).rejects.toThrow(ServiceUnavailableError); await expect( controller.deleteCredentialTypePolicyDocument(req, res, 'policy-id'), ).rejects.toThrow(ServiceUnavailableError); @@ -162,9 +166,35 @@ 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('getCredentialTypePolicyAttachments reads the credential type scope with extends rules', async () => { + const result = { + attachments: [ + { + policyId: 'policy-id', + rules: [ + { + id: 'r1', + action: 'deny' as const, + selector: { kind: 'extends' as const, value: 'oAuth2Api' }, + }, + ], + priority: 5, + isFloor: true, + }, + ], + version: 3, + }; + service.getAttachments.mockResolvedValue(result); + + expect(await controller.getCredentialTypePolicyAttachments(req, res, 'scope-id')).toEqual( + result, + ); + expect(service.getAttachments).toHaveBeenCalledWith(CREDENTIAL_TYPES_KIND, 'scope-id'); + }); + it('getCredentialTypeInstancePolicy reads the null-project scope and maps the response', async () => { service.getEffectivePolicy.mockResolvedValue(effectivePolicy); 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..4710dea68a2 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 @@ -40,6 +40,7 @@ describe('NodeTypePoliciesPublicController route metadata', () => { 'createNodeTypePolicyDocument', 'deleteNodeTypePolicyDocument', 'getNodeTypeInstancePolicy', + 'getNodeTypePolicyAttachments', 'getNodeTypePolicyDocument', 'getNodeTypeProjectPolicy', 'listNodeTypePolicyDocuments', @@ -114,6 +115,9 @@ describe('NodeTypePoliciesPublicController with the module disabled', () => { await expect(controller.getNodeTypePolicyDocument(req, res, 'policy-id')).rejects.toThrow( ServiceUnavailableError, ); + await expect(controller.getNodeTypePolicyAttachments(req, res, 'scope-id')).rejects.toThrow( + ServiceUnavailableError, + ); await expect(controller.deleteNodeTypePolicyDocument(req, res, 'policy-id')).rejects.toThrow( ServiceUnavailableError, ); @@ -157,7 +161,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 () => { @@ -171,6 +175,35 @@ describe('NodeTypePoliciesPublicController handler bodies', () => { ); }); + it('getNodeTypePolicyAttachments reads the node type scope and maps the response', async () => { + const result = { + attachments: [{ policyId: 'policy-id', rules, priority: 5, isFloor: true }], + version: 3, + }; + service.getAttachments.mockResolvedValue(result); + + expect(await controller.getNodeTypePolicyAttachments(req, res, 'scope-id')).toEqual(result); + expect(service.getAttachments).toHaveBeenCalledWith(NODE_TYPES_KIND, 'scope-id'); + }); + + it('getNodeTypePolicyAttachments rejects an extends rule', async () => { + service.getAttachments.mockResolvedValue({ + attachments: [ + { + policyId: 'policy-id', + rules: [{ id: 'r1', action: 'deny', selector: { kind: 'extends', value: 'oAuth2Api' } }], + priority: 0, + isFloor: false, + }, + ], + version: 3, + }); + + await expect(controller.getNodeTypePolicyAttachments(req, res, 'scope-id')).rejects.toThrow( + 'A node type policy holds an extends rule', + ); + }); + it('getNodeTypeInstancePolicy reads the null-project scope and maps the response', async () => { service.getEffectivePolicy.mockResolvedValue(effectivePolicy); diff --git a/packages/cli/src/public-api/v1/controllers/credential-type-policies.public.controller.ts b/packages/cli/src/public-api/v1/controllers/credential-type-policies.public.controller.ts index 736c0642491..b4d538a720c 100644 --- a/packages/cli/src/public-api/v1/controllers/credential-type-policies.public.controller.ts +++ b/packages/cli/src/public-api/v1/controllers/credential-type-policies.public.controller.ts @@ -43,6 +43,7 @@ import type { Response } from 'express'; import { NotFoundError, ServiceUnavailableError } from '@n8n/errors'; import { CREDENTIAL_TYPES_KIND } from '@/modules/type-availability-policies/constants'; import type { TypeAvailabilityPolicy } from '@/modules/type-availability-policies/database/entities/type-availability-policy.entity'; +import type { ScopeAttachments } from '@/modules/type-availability-policies/type-availability-policy.service'; import { encodeNextCursor, resolveOffsetPagination, @@ -62,6 +63,18 @@ function toPublicDocument(policy: TypeAvailabilityPolicy): CredentialTypePolicyD }; } +function toPublicAttachments(result: ScopeAttachments): CredentialTypePolicyAttachmentsPublicDto { + return { + attachments: result.attachments.map((attachment) => ({ + policyId: attachment.policyId, + rules: [...attachment.rules], + priority: attachment.priority, + isFloor: attachment.isFloor, + })), + version: result.version, + }; +} + /** * Public API surface for credential type availability policies. Every route delegates to the * same `TypeAvailabilityPolicyService` methods as the internal `/rest` controllers and reuses @@ -331,6 +344,28 @@ export class CredentialTypePoliciesPublicController { await (await this.service()).deletePolicyDocument(CREDENTIAL_TYPES_KIND, policyId, req.user.id); } + @Get('/scopes/:scopeId/attachments') + @Licensed(LICENSE_FEATURES.TYPE_AVAILABILITY_POLICIES) + @ApiKeyScope('credentialTypePolicy:manage') + @GlobalScope('credentialTypePolicy:manage') + @ApiSummary("Preview: Retrieve a scope's attached policy documents") + @ApiDescription( + '**Preview** — breaking changes may still occur without major version bump.\n\nReturns every attached policy document and the scope version. Attachments use evaluation order: floor first, then priority ascending within each group. Use this version when replacing the attachments. `scopeId` comes from `GET /instance` or `GET /projects/{projectId}`, and is null until the scope is first written. This route needs instance-level access, also for a project scope.', + ) + @ApiTags(tags) + @ApiResponse(200, CredentialTypePolicyAttachmentsPublicDto) + @ApiErrorResponse(404) + @ApiErrorResponse(503) + async getCredentialTypePolicyAttachments( + _req: AuthenticatedRequest, + _res: Response, + @Param('scopeId', credentialTypePolicyScopeIdParamSchema) scopeId: string, + ): Promise { + const result = await (await this.service()).getAttachments(CREDENTIAL_TYPES_KIND, scopeId); + + return toPublicAttachments(result); + } + @Put('/scopes/:scopeId/attachments') @Licensed(LICENSE_FEATURES.TYPE_AVAILABILITY_POLICIES) @ApiKeyScope('credentialTypePolicy:manage') @@ -358,14 +393,6 @@ export class CredentialTypePoliciesPublicController { req.user.id, ); - return { - attachments: result.attachments.map((attachment) => ({ - policyId: attachment.policyId, - rules: [...attachment.rules], - priority: attachment.priority, - isFloor: attachment.isFloor, - })), - version: result.version, - }; + return toPublicAttachments(result); } } diff --git a/packages/cli/src/public-api/v1/controllers/node-type-policies.public.controller.ts b/packages/cli/src/public-api/v1/controllers/node-type-policies.public.controller.ts index 80a6ddf9dfe..af0551c9596 100644 --- a/packages/cli/src/public-api/v1/controllers/node-type-policies.public.controller.ts +++ b/packages/cli/src/public-api/v1/controllers/node-type-policies.public.controller.ts @@ -47,6 +47,7 @@ import { UnexpectedError } from 'n8n-workflow'; import { NotFoundError, ServiceUnavailableError } from '@n8n/errors'; import { NODE_TYPES_KIND } from '@/modules/type-availability-policies/constants'; import type { TypeAvailabilityPolicy } from '@/modules/type-availability-policies/database/entities/type-availability-policy.entity'; +import type { ScopeAttachments } from '@/modules/type-availability-policies/type-availability-policy.service'; import { encodeNextCursor, resolveOffsetPagination, @@ -74,6 +75,18 @@ function toPublicDocument(policy: TypeAvailabilityPolicy): PolicyDocumentPublicD }; } +function toPublicAttachments(result: ScopeAttachments): PolicyAttachmentsPublicDto { + return { + attachments: result.attachments.map((attachment) => ({ + policyId: attachment.policyId, + rules: toNodeTypeRules(attachment.rules), + priority: attachment.priority, + isFloor: attachment.isFloor, + })), + version: result.version, + }; +} + /** * Public API surface for node type availability policies. Every route delegates to the same * `TypeAvailabilityPolicyService` methods as the internal `/rest` controllers and reuses their @@ -340,6 +353,28 @@ export class NodeTypePoliciesPublicController { await (await this.service()).deletePolicyDocument(NODE_TYPES_KIND, policyId, req.user.id); } + @Get('/scopes/:scopeId/attachments') + @Licensed(LICENSE_FEATURES.TYPE_AVAILABILITY_POLICIES) + @ApiKeyScope('nodeTypePolicy:manage') + @GlobalScope('nodeTypePolicy:manage') + @ApiSummary("Preview: Retrieve a scope's attached policy documents") + @ApiDescription( + '**Preview** — breaking changes may still occur without major version bump.\n\nReturns every attached policy document and the scope version. Attachments use evaluation order: floor first, then priority ascending within each group. Use this version when replacing the attachments. `scopeId` comes from `GET /instance` or `GET /projects/{projectId}`, and is null until the scope is first written. This route needs instance-level access, also for a project scope.', + ) + @ApiTags(tags) + @ApiResponse(200, PolicyAttachmentsPublicDto) + @ApiErrorResponse(404) + @ApiErrorResponse(503) + async getNodeTypePolicyAttachments( + _req: AuthenticatedRequest, + _res: Response, + @Param('scopeId', nodeTypePolicyScopeIdParamSchema) scopeId: string, + ): Promise { + const result = await (await this.service()).getAttachments(NODE_TYPES_KIND, scopeId); + + return toPublicAttachments(result); + } + @Put('/scopes/:scopeId/attachments') @Licensed(LICENSE_FEATURES.TYPE_AVAILABILITY_POLICIES) @ApiKeyScope('nodeTypePolicy:manage') @@ -367,14 +402,6 @@ export class NodeTypePoliciesPublicController { req.user.id, ); - return { - attachments: result.attachments.map((attachment) => ({ - policyId: attachment.policyId, - rules: toNodeTypeRules(attachment.rules), - priority: attachment.priority, - isFloor: attachment.isFloor, - })), - version: result.version, - }; + return toPublicAttachments(result); } } diff --git a/packages/cli/src/public-api/v1/handlers/credential-type-policies/spec/paths/getCredentialTypePolicyAttachments.generated.yml b/packages/cli/src/public-api/v1/handlers/credential-type-policies/spec/paths/getCredentialTypePolicyAttachments.generated.yml new file mode 100644 index 00000000000..ac3e874f3b6 --- /dev/null +++ b/packages/cli/src/public-api/v1/handlers/credential-type-policies/spec/paths/getCredentialTypePolicyAttachments.generated.yml @@ -0,0 +1,31 @@ +operationId: getCredentialTypePolicyAttachments +tags: + - CredentialTypePolicy +summary: "Preview: Retrieve a scope's attached policy documents" +description: |- + **Preview** — breaking changes may still occur without major version bump. + + Returns every attached policy document and the scope version. Attachments use evaluation order: floor first, then priority ascending within each group. Use this version when replacing the attachments. `scopeId` comes from `GET /instance` or `GET /projects/{projectId}`, and is null until the scope is first written. This route needs instance-level access, also for a project scope. +x-required-scope: credentialTypePolicy:manage +parameters: + - schema: + type: string + required: true + description: The ID of the credential type policy scope. + name: scopeId + in: path +responses: + '200': + description: Operation successful. + content: + application/json: + schema: + $ref: ../../../../shared/spec/schemas/credentialTypePolicyAttachmentsPublicDto.generated.yml + '401': + $ref: ../../../../shared/spec/responses/unauthorized.yml + '403': + $ref: ../../../../shared/spec/responses/forbidden.yml + '404': + $ref: ../../../../shared/spec/responses/notFound.yml + '503': + $ref: ../../../../shared/spec/responses/serviceUnavailable.yml diff --git a/packages/cli/src/public-api/v1/handlers/credential-type-policies/spec/paths/replaceCredentialTypePolicyAttachments.generated.yml b/packages/cli/src/public-api/v1/handlers/credential-type-policies/spec/paths/replaceCredentialTypePolicyAttachments.generated.yml index 27cfa44eb81..714a79b9469 100644 --- a/packages/cli/src/public-api/v1/handlers/credential-type-policies/spec/paths/replaceCredentialTypePolicyAttachments.generated.yml +++ b/packages/cli/src/public-api/v1/handlers/credential-type-policies/spec/paths/replaceCredentialTypePolicyAttachments.generated.yml @@ -49,89 +49,7 @@ responses: content: application/json: schema: - type: object - properties: - attachments: - type: array - items: - type: object - properties: - policyId: - type: string - rules: - type: array - items: - type: object - properties: - id: - type: string - minLength: 1 - action: - type: string - enum: - - allow - - deny - - delegate - selector: - oneOf: - - type: object - properties: - kind: - type: string - enum: - - name - value: - type: string - minLength: 1 - required: - - kind - - value - - type: object - properties: - kind: - type: string - enum: - - package - value: - type: string - minLength: 1 - required: - - kind - - value - - type: object - properties: - kind: - type: string - enum: - - extends - value: - type: string - minLength: 1 - required: - - kind - - value - description: Matches the named credential type and every type that extends it - type: object - required: - - id - - action - - selector - priority: - type: integer - isFloor: - type: boolean - required: - - policyId - - rules - - priority - - isFloor - version: - type: integer - minimum: 0 - exclusiveMinimum: true - required: - - attachments - - version + $ref: ../../../../shared/spec/schemas/credentialTypePolicyAttachmentsPublicDto.generated.yml '400': $ref: ../../../../shared/spec/responses/badRequest.yml '401': diff --git a/packages/cli/src/public-api/v1/handlers/node-type-policies/spec/paths/getNodeTypePolicyAttachments.generated.yml b/packages/cli/src/public-api/v1/handlers/node-type-policies/spec/paths/getNodeTypePolicyAttachments.generated.yml new file mode 100644 index 00000000000..d5b3a4e3dab --- /dev/null +++ b/packages/cli/src/public-api/v1/handlers/node-type-policies/spec/paths/getNodeTypePolicyAttachments.generated.yml @@ -0,0 +1,31 @@ +operationId: getNodeTypePolicyAttachments +tags: + - NodeTypePolicy +summary: "Preview: Retrieve a scope's attached policy documents" +description: |- + **Preview** — breaking changes may still occur without major version bump. + + Returns every attached policy document and the scope version. Attachments use evaluation order: floor first, then priority ascending within each group. Use this version when replacing the attachments. `scopeId` comes from `GET /instance` or `GET /projects/{projectId}`, and is null until the scope is first written. This route needs instance-level access, also for a project scope. +x-required-scope: nodeTypePolicy:manage +parameters: + - schema: + type: string + required: true + description: The ID of the node type policy scope. + name: scopeId + in: path +responses: + '200': + description: Operation successful. + content: + application/json: + schema: + $ref: ../../../../shared/spec/schemas/policyAttachmentsPublicDto.generated.yml + '401': + $ref: ../../../../shared/spec/responses/unauthorized.yml + '403': + $ref: ../../../../shared/spec/responses/forbidden.yml + '404': + $ref: ../../../../shared/spec/responses/notFound.yml + '503': + $ref: ../../../../shared/spec/responses/serviceUnavailable.yml diff --git a/packages/cli/src/public-api/v1/handlers/node-type-policies/spec/paths/replaceNodeTypePolicyAttachments.generated.yml b/packages/cli/src/public-api/v1/handlers/node-type-policies/spec/paths/replaceNodeTypePolicyAttachments.generated.yml index 6cec17206a9..272bd197b0a 100644 --- a/packages/cli/src/public-api/v1/handlers/node-type-policies/spec/paths/replaceNodeTypePolicyAttachments.generated.yml +++ b/packages/cli/src/public-api/v1/handlers/node-type-policies/spec/paths/replaceNodeTypePolicyAttachments.generated.yml @@ -49,76 +49,7 @@ responses: content: application/json: schema: - type: object - properties: - attachments: - type: array - items: - type: object - properties: - policyId: - type: string - rules: - type: array - items: - type: object - properties: - id: - type: string - minLength: 1 - action: - type: string - enum: - - allow - - deny - - delegate - selector: - oneOf: - - type: object - properties: - kind: - type: string - enum: - - name - value: - type: string - minLength: 1 - required: - - kind - - value - - type: object - properties: - kind: - type: string - enum: - - package - value: - type: string - minLength: 1 - required: - - kind - - value - type: object - required: - - id - - action - - selector - priority: - type: integer - isFloor: - type: boolean - required: - - policyId - - rules - - priority - - isFloor - version: - type: integer - minimum: 0 - exclusiveMinimum: true - required: - - attachments - - version + $ref: ../../../../shared/spec/schemas/policyAttachmentsPublicDto.generated.yml '400': $ref: ../../../../shared/spec/responses/badRequest.yml '401': diff --git a/packages/cli/src/public-api/v1/openapi.decorator-routes.generated.yml b/packages/cli/src/public-api/v1/openapi.decorator-routes.generated.yml index 19ae7fa79b5..82d6fd71415 100644 --- a/packages/cli/src/public-api/v1/openapi.decorator-routes.generated.yml +++ b/packages/cli/src/public-api/v1/openapi.decorator-routes.generated.yml @@ -39,6 +39,8 @@ paths: delete: $ref: ./handlers/credential-type-policies/spec/paths/deleteCredentialTypePolicyDocument.generated.yml /credential-type-policies/scopes/{scopeId}/attachments: + get: + $ref: ./handlers/credential-type-policies/spec/paths/getCredentialTypePolicyAttachments.generated.yml put: $ref: ./handlers/credential-type-policies/spec/paths/replaceCredentialTypePolicyAttachments.generated.yml /credentials: @@ -215,6 +217,8 @@ paths: delete: $ref: ./handlers/node-type-policies/spec/paths/deleteNodeTypePolicyDocument.generated.yml /node-type-policies/scopes/{scopeId}/attachments: + get: + $ref: ./handlers/node-type-policies/spec/paths/getNodeTypePolicyAttachments.generated.yml put: $ref: ./handlers/node-type-policies/spec/paths/replaceNodeTypePolicyAttachments.generated.yml /settings/otel: diff --git a/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyAttachmentsPublicDto.generated.yml b/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyAttachmentsPublicDto.generated.yml new file mode 100644 index 00000000000..c79f4139346 --- /dev/null +++ b/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyAttachmentsPublicDto.generated.yml @@ -0,0 +1,83 @@ +type: object +properties: + attachments: + type: array + items: + type: object + properties: + policyId: + type: string + rules: + type: array + items: + type: object + properties: + id: + type: string + minLength: 1 + action: + type: string + enum: + - allow + - deny + - delegate + selector: + oneOf: + - type: object + properties: + kind: + type: string + enum: + - name + value: + type: string + minLength: 1 + required: + - kind + - value + - type: object + properties: + kind: + type: string + enum: + - package + value: + type: string + minLength: 1 + required: + - kind + - value + - type: object + properties: + kind: + type: string + enum: + - extends + value: + type: string + minLength: 1 + required: + - kind + - value + description: Matches the named credential type and every type that extends it + type: object + required: + - id + - action + - selector + priority: + type: integer + isFloor: + type: boolean + required: + - policyId + - rules + - priority + - isFloor + version: + type: integer + minimum: 0 + exclusiveMinimum: true +required: + - attachments + - version 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/policyAttachmentsPublicDto.generated.yml b/packages/cli/src/public-api/v1/shared/spec/schemas/policyAttachmentsPublicDto.generated.yml new file mode 100644 index 00000000000..91e496b3ee6 --- /dev/null +++ b/packages/cli/src/public-api/v1/shared/spec/schemas/policyAttachmentsPublicDto.generated.yml @@ -0,0 +1,70 @@ +type: object +properties: + attachments: + type: array + items: + type: object + properties: + policyId: + type: string + rules: + type: array + items: + type: object + properties: + id: + type: string + minLength: 1 + action: + type: string + enum: + - allow + - deny + - delegate + selector: + oneOf: + - type: object + properties: + kind: + type: string + enum: + - name + value: + type: string + minLength: 1 + required: + - kind + - value + - type: object + properties: + kind: + type: string + enum: + - package + value: + type: string + minLength: 1 + required: + - kind + - value + type: object + required: + - id + - action + - selector + priority: + type: integer + isFloor: + type: boolean + required: + - policyId + - rules + - priority + - isFloor + version: + type: integer + minimum: 0 + exclusiveMinimum: true +required: + - attachments + - version 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/src/services/__tests__/frontend.service.test.ts b/packages/cli/src/services/__tests__/frontend.service.test.ts index 40fbb156634..4e9d6a03c7e 100644 --- a/packages/cli/src/services/__tests__/frontend.service.test.ts +++ b/packages/cli/src/services/__tests__/frontend.service.test.ts @@ -220,31 +220,40 @@ describe('FrontendService', () => { // constructor-injected mock above must also be registered here. Container.set(License, license); - return { - service: new FrontendService( - globalConfig, - logger, - loadNodesAndCredentials, - credentialTypes, - credentialsOverwrites, + // Suppress the constructor's background write so it cannot outlive the test. + // Restore the method so tests and reload handlers can call it explicitly. + const generateTypes = vi + .spyOn(FrontendService.prototype, 'generateTypes') + .mockResolvedValueOnce(); + try { + return { + service: new FrontendService( + globalConfig, + logger, + loadNodesAndCredentials, + credentialTypes, + credentialsOverwrites, + license, + mailer, + instanceSettings, + urlService, + securityConfig, + pushConfig, + binaryDataConfig, + licenseState, + moduleRegistry, + mfaService, + ownershipService, + aiUsageService, + workflowRepository, + workflowReviewPolicyService, + inboxService, + ), license, - mailer, - instanceSettings, - urlService, - securityConfig, - pushConfig, - binaryDataConfig, - licenseState, - moduleRegistry, - mfaService, - ownershipService, - aiUsageService, - workflowRepository, - workflowReviewPolicyService, - inboxService, - ), - license, - }; + }; + } finally { + generateTypes.mockRestore(); + } }; beforeEach(() => { @@ -1368,9 +1377,9 @@ describe('FrontendService', () => { const largeNodes = createNodes(5000); const smallNodes = createNodes(1); const collectTypes = loadNodesAndCredentials.collectTypes as Mock; - collectTypes.mockReturnValueOnce(new Promise(() => {})); const { service } = createMockService(); + expect(collectTypes).not.toHaveBeenCalled(); for (let attempt = 0; attempt < 10; attempt++) { collectTypes @@ -1383,6 +1392,8 @@ describe('FrontendService', () => { expect(readTypesFile('credentials')).toEqual([]); expect(readTypesFile('node-versions')).toEqual(expect.any(Array)); } + + expect(collectTypes).toHaveBeenCalledTimes(20); }); }); }); diff --git a/packages/cli/test/integration/policy/credential-type-policy-instance.controller.test.ts b/packages/cli/test/integration/policy/credential-type-policy-instance.controller.test.ts index a4bc3e4ab4f..4b43a6b8b2b 100644 --- a/packages/cli/test/integration/policy/credential-type-policy-instance.controller.test.ts +++ b/packages/cli/test/integration/policy/credential-type-policy-instance.controller.test.ts @@ -45,10 +45,11 @@ describe('credential type availability policy instance controller RBAC', () => { expect(response.statusCode).toBe(403); }); - test('GET /credential-type-policies/instance rejects a member with 403', async () => { - const response = await testServer - .authAgentFor(member) - .get('/credential-type-policies/instance'); + test.each([ + '/credential-type-policies/instance', + '/credential-type-policies/scopes/does-not-exist/attachments', + ])('GET %s rejects a member with 403', async (path) => { + const response = await testServer.authAgentFor(member).get(path); expect(response.statusCode).toBe(403); }); @@ -71,16 +72,65 @@ describe('credential type availability policy instance controller license gating testServer.license.enable(LICENSE_FEATURES.TYPE_AVAILABILITY_POLICIES); }); - test('rejects an owner with 403 when the license feature is disabled', async () => { + test.each([ + '/credential-type-policies/instance', + '/credential-type-policies/scopes/does-not-exist/attachments', + ])('GET %s rejects an owner with 403 when the license feature is disabled', async (path) => { testServer.license.disable(LICENSE_FEATURES.TYPE_AVAILABILITY_POLICIES); - const response = await testServer.authAgentFor(owner).get('/credential-type-policies/instance'); + const response = await testServer.authAgentFor(owner).get(path); expect(response.statusCode).toBe(403); }); }); describe('credential type availability policy instance controller admin happy path', () => { + test('GET attachments returns rules and a version that PUT accepts without a read audit event', async () => { + const agent = testServer.authAgentFor(owner); + const rule = { id: 'r1', action: 'deny', selector: { kind: 'name', value: 'slackApi' } }; + const instance = await agent.put('/credential-type-policies/instance').send({ + rules: [rule], + defaultAction: 'allow', + version: 0, + }); + expect(instance.statusCode).toBe(200); + const path = `/credential-type-policies/scopes/${instance.body.data.scopeId}/attachments`; + const emitSpy = vi.spyOn(Container.get(EventService), 'emit'); + + const read = await agent.get(path); + + expect(read.statusCode).toBe(200); + expect(read.body.data).toEqual({ + attachments: [{ policyId: expect.any(String), rules: [rule], priority: 0, isFloor: false }], + version: instance.body.data.version, + }); + expect(emitSpy).not.toHaveBeenCalled(); + const updated = await agent + .put(path) + .send({ attachments: [], version: read.body.data.version }); + expect(updated.statusCode).toBe(200); + expect(updated.body.data).toEqual({ attachments: [], version: read.body.data.version + 1 }); + const empty = await agent.get(path); + expect(empty.body.data).toEqual(updated.body.data); + }); + + test('GET attachments returns 404 for an unknown scope and a node type scope', async () => { + const agent = testServer.authAgentFor(owner); + expect( + (await agent.get('/credential-type-policies/scopes/does-not-exist/attachments')).statusCode, + ).toBe(404); + const other = await agent.put('/node-type-policies/instance').send({ + rules: [], + defaultAction: 'allow', + version: 0, + }); + expect(other.statusCode).toBe(200); + expect( + (await agent.get(`/credential-type-policies/scopes/${other.body.data.scopeId}/attachments`)) + .statusCode, + ).toBe(404); + }); + test('PUT /instance persists and fires an audit event carrying the credential-types kind', async () => { const eventService = Container.get(EventService); const emitSpy = vi.spyOn(eventService, 'emit'); 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..48201dd423c 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: {} } }), }; @@ -62,8 +61,11 @@ describe('node type availability policy instance controller RBAC', () => { expect(response.statusCode).toBe(403); }); - test('GET /node-type-policies/instance rejects a member with 403', async () => { - const response = await testServer.authAgentFor(member).get('/node-type-policies/instance'); + test.each([ + '/node-type-policies/instance', + '/node-type-policies/scopes/does-not-exist/attachments', + ])('GET %s rejects a member with 403', async (path) => { + const response = await testServer.authAgentFor(member).get(path); expect(response.statusCode).toBe(403); }); @@ -130,16 +132,65 @@ describe('node type availability policy instance controller license gating', () testServer.license.enable(LICENSE_FEATURES.TYPE_AVAILABILITY_POLICIES); }); - test('rejects an owner with 403 when the license feature is disabled', async () => { + test.each([ + '/node-type-policies/instance', + '/node-type-policies/scopes/does-not-exist/attachments', + ])('GET %s rejects an owner with 403 when the license feature is disabled', async (path) => { testServer.license.disable(LICENSE_FEATURES.TYPE_AVAILABILITY_POLICIES); - const response = await testServer.authAgentFor(owner).get('/node-type-policies/instance'); + const response = await testServer.authAgentFor(owner).get(path); expect(response.statusCode).toBe(403); }); }); describe('node type availability policy instance controller admin happy path', () => { + test('GET attachments returns rules and a version that PUT accepts without a read audit event', async () => { + const agent = testServer.authAgentFor(owner); + const rule = { id: 'r1', action: 'deny', selector: { kind: 'name', value: 'a.b' } }; + const instance = await agent.put('/node-type-policies/instance').send({ + rules: [rule], + defaultAction: 'allow', + version: 0, + }); + expect(instance.statusCode).toBe(200); + const path = `/node-type-policies/scopes/${instance.body.data.scopeId}/attachments`; + const emitSpy = vi.spyOn(Container.get(EventService), 'emit'); + + const read = await agent.get(path); + + expect(read.statusCode).toBe(200); + expect(read.body.data).toEqual({ + attachments: [{ policyId: expect.any(String), rules: [rule], priority: 0, isFloor: false }], + version: instance.body.data.version, + }); + expect(emitSpy).not.toHaveBeenCalled(); + const updated = await agent + .put(path) + .send({ attachments: [], version: read.body.data.version }); + expect(updated.statusCode).toBe(200); + expect(updated.body.data).toEqual({ attachments: [], version: read.body.data.version + 1 }); + const empty = await agent.get(path); + expect(empty.body.data).toEqual(updated.body.data); + }); + + test('GET attachments returns 404 for an unknown scope and a credential type scope', async () => { + const agent = testServer.authAgentFor(owner); + expect( + (await agent.get('/node-type-policies/scopes/does-not-exist/attachments')).statusCode, + ).toBe(404); + const other = await agent.put('/credential-type-policies/instance').send({ + rules: [], + defaultAction: 'allow', + version: 0, + }); + expect(other.statusCode).toBe(200); + expect( + (await agent.get(`/node-type-policies/scopes/${other.body.data.scopeId}/attachments`)) + .statusCode, + ).toBe(404); + }); + test('PUT /instance persists, bumps version, and fires an audit event', async () => { const eventService = Container.get(EventService); const emitSpy = vi.spyOn(eventService, 'emit'); @@ -236,6 +287,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..cbfe68cbe38 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. @@ -82,6 +89,7 @@ describe('credential type policies public API authorization', () => { ['GET', '/credential-type-policies/policies/does-not-exist'], ['PUT', '/credential-type-policies/policies/does-not-exist'], ['DELETE', '/credential-type-policies/policies/does-not-exist'], + ['GET', '/credential-type-policies/scopes/does-not-exist/attachments'], ['PUT', '/credential-type-policies/scopes/does-not-exist/attachments'], ])('%s %s rejects an owner key without the scope with 403', async (method, path) => { const agent = testServer.publicApiAgentFor(unscopedOwner); @@ -113,6 +121,7 @@ describe('credential type policies public API authorization', () => { ['GET', '/credential-type-policies/policies/does-not-exist'], ['PUT', '/credential-type-policies/policies/does-not-exist'], ['DELETE', '/credential-type-policies/policies/does-not-exist'], + ['GET', '/credential-type-policies/scopes/does-not-exist/attachments'], ['PUT', '/credential-type-policies/scopes/does-not-exist/attachments'], ])( '%s %s rejects a project admin with 403 (instance routes are global-only)', @@ -168,12 +177,13 @@ describe('credential type policies public API license gating', () => { testServer.license.enable(LICENSE_FEATURES.TYPE_AVAILABILITY_POLICIES); }); - test('rejects an owner with 403 when the license feature is disabled', async () => { + test.each([ + '/credential-type-policies/instance', + '/credential-type-policies/scopes/does-not-exist/attachments', + ])('GET %s rejects an owner with 403 when the license feature is disabled', async (path) => { testServer.license.disable(LICENSE_FEATURES.TYPE_AVAILABILITY_POLICIES); - const instance = await testServer - .publicApiAgentFor(owner) - .get('/credential-type-policies/instance'); + const instance = await testServer.publicApiAgentFor(owner).get(path); expect(instance.statusCode).toBe(403); const projectPolicy = await testServer @@ -425,7 +435,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 () => { @@ -596,22 +608,117 @@ describe('credential type policies public API policy documents', () => { }); describe('credential type policies public API attachments', () => { + test.each(['instance', 'project'])( + 'GET and PUT return every attachment in evaluation order for %s scope', + async (scope) => { + const agent = testServer.publicApiAgentFor(owner); + const effectivePath = + scope === 'instance' ? '/credential-type-policies/instance' : projectRoute(project.id); + const configured = await agent.put(effectivePath).send(ALLOW_ALL); + expect(configured.statusCode).toBe(200); + const path = `/credential-type-policies/scopes/${configured.body.scopeId}/attachments`; + const slots = [ + { priority: 10, isFloor: false }, + { priority: 20, isFloor: true }, + { priority: -5, isFloor: false }, + { priority: 0, isFloor: true }, + ]; + const attachments = []; + for (const [index, slot] of slots.entries()) { + const rules = [{ ...DENY_RULE, id: `r${index}` }]; + const created = await agent.post('/credential-type-policies/policies').send({ rules }); + expect(created.statusCode).toBe(201); + attachments.push({ policyId: created.body.policy.id as string, rules, ...slot }); + } + const replaced = await agent.put(path).send({ + attachments: attachments.map(({ rules: _rules, ...attachment }) => attachment), + version: configured.body.version, + }); + expect(replaced.statusCode).toBe(200); + expect(replaced.body).toEqual({ + attachments: [attachments[3], attachments[1], attachments[2], attachments[0]], + version: configured.body.version + 1, + }); + const emitSpy = vi.spyOn(Container.get(EventService), 'emit'); + + const read = await agent.get(path); + + expect(read.statusCode).toBe(200); + expect(read.body).toEqual(replaced.body); + expect(emitSpy.mock.calls.filter(([event]) => event.startsWith('node-type-policy-'))).toEqual( + [], + ); + const effective = await agent.get(effectivePath); + expect(Object.keys(effective.body).sort()).toEqual([...EFFECTIVE_KEYS].sort()); + const updated = await agent.put(path).send({ attachments: [], version: read.body.version }); + expect(updated.statusCode).toBe(200); + expect(updated.body).toEqual({ attachments: [], version: read.body.version + 1 }); + const empty = await agent.get(path); + expect(empty.statusCode).toBe(200); + expect(empty.body).toEqual(updated.body); + }, + ); + + test('GET returns 404 for an unknown scope and a node type scope', async () => { + const agent = testServer.publicApiAgentFor(owner); + expect( + (await agent.get('/credential-type-policies/scopes/does-not-exist/attachments')).statusCode, + ).toBe(404); + const other = await agent.put('/node-type-policies/instance').send(ALLOW_ALL); + expect(other.statusCode).toBe(200); + expect( + (await agent.get(`/credential-type-policies/scopes/${other.body.scopeId}/attachments`)) + .statusCode, + ).toBe(404); + }); + test('PUT with a stale version returns 409 and preserves the attachments and version', async () => { const agent = testServer.publicApiAgentFor(owner); - const instance = await agent.put('/credential-type-policies/instance').send({ - ...ALLOW_ALL, - rules: [DENY_RULE], - }); + const instance = await agent.put('/credential-type-policies/instance').send(ALLOW_ALL); expect(instance.statusCode).toBe(200); + const created = await agent + .post('/credential-type-policies/policies') + .send({ rules: [DENY_RULE] }); + expect(created.statusCode).toBe(201); + const policyId = created.body.policy.id as string; - const stale = await agent - .put(`/credential-type-policies/scopes/${instance.body.scopeId}/attachments`) - .send({ attachments: [], version: 0 }); + const path = `/credential-type-policies/scopes/${instance.body.scopeId}/attachments`; + const attached = await agent.put(path).send({ + attachments: [{ policyId, priority: 0, isFloor: false }], + version: instance.body.version, + }); + expect(attached.statusCode).toBe(200); + const read = await agent.get(path); + expect(read.statusCode).toBe(200); + expect(read.body).toEqual({ + attachments: [{ policyId, rules: [DENY_RULE], priority: 0, isFloor: false }], + version: instance.body.version + 1, + }); + const replaced = await agent.put(path).send({ + attachments: read.body.attachments.map(({ policyId }: { policyId: string }) => ({ + policyId, + priority: 5, + isFloor: false, + })), + version: read.body.version, + }); + expect(replaced.statusCode).toBe(200); + expect(replaced.body).toEqual({ + attachments: [{ policyId, rules: [DENY_RULE], priority: 5, isFloor: false }], + version: read.body.version + 1, + }); + + const stale = await agent.put(path).send({ attachments: [], version: read.body.version }); expect(stale.statusCode).toBe(409); - const persisted = await agent.get('/credential-type-policies/instance'); - expect(persisted.body.rules).toEqual([DENY_RULE]); - expect(persisted.body.version).toBe(instance.body.version); + const persisted = await agent.get(path); + expect(persisted.statusCode).toBe(200); + expect(persisted.body).toEqual(replaced.body); + const updated = await agent + .put(path) + .send({ attachments: [], version: persisted.body.version }); + expect(updated.statusCode).toBe(200); + expect(updated.body).toEqual({ attachments: [], version: persisted.body.version + 1 }); }); test.each([undefined, -1, 0.5])('PUT rejects invalid version %s with 400', async (version) => { @@ -767,5 +874,10 @@ describe('credential type policies public API extends rules', () => { expect(attachments.body.attachments).toEqual([ { policyId, rules: [EXTENDS_RULE], priority: 1, isFloor: false }, ]); + const readAttachments = await agent.get( + `/credential-type-policies/scopes/${instancePut.body.scopeId}/attachments`, + ); + expect(readAttachments.statusCode).toBe(200); + expect(readAttachments.body).toEqual(attachments.body); }); }); 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..ce3b197b813 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. @@ -77,6 +85,7 @@ describe('node type policies public API authorization', () => { ['GET', '/node-type-policies/policies/does-not-exist'], ['PUT', '/node-type-policies/policies/does-not-exist'], ['DELETE', '/node-type-policies/policies/does-not-exist'], + ['GET', '/node-type-policies/scopes/does-not-exist/attachments'], ['PUT', '/node-type-policies/scopes/does-not-exist/attachments'], ])('%s %s rejects an owner key without the scope with 403', async (method, path) => { const agent = testServer.publicApiAgentFor(unscopedOwner); @@ -108,6 +117,7 @@ describe('node type policies public API authorization', () => { ['GET', '/node-type-policies/policies/does-not-exist'], ['PUT', '/node-type-policies/policies/does-not-exist'], ['DELETE', '/node-type-policies/policies/does-not-exist'], + ['GET', '/node-type-policies/scopes/does-not-exist/attachments'], ['PUT', '/node-type-policies/scopes/does-not-exist/attachments'], ])( '%s %s rejects a project admin with 403 (instance routes are global-only)', @@ -163,10 +173,13 @@ describe('node type policies public API license gating', () => { testServer.license.enable(LICENSE_FEATURES.TYPE_AVAILABILITY_POLICIES); }); - test('rejects an owner with 403 when the license feature is disabled', async () => { + test.each([ + '/node-type-policies/instance', + '/node-type-policies/scopes/does-not-exist/attachments', + ])('GET %s rejects an owner with 403 when the license feature is disabled', async (path) => { testServer.license.disable(LICENSE_FEATURES.TYPE_AVAILABILITY_POLICIES); - const instance = await testServer.publicApiAgentFor(owner).get('/node-type-policies/instance'); + const instance = await testServer.publicApiAgentFor(owner).get(path); expect(instance.statusCode).toBe(403); const projectPolicy = await testServer @@ -416,7 +429,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 () => { @@ -564,22 +579,114 @@ describe('node type policies public API policy documents', () => { }); describe('node type policies public API attachments', () => { + test.each(['instance', 'project'])( + 'GET and PUT return every attachment in evaluation order for %s scope', + async (scope) => { + const agent = testServer.publicApiAgentFor(owner); + const effectivePath = + scope === 'instance' ? '/node-type-policies/instance' : projectRoute(project.id); + const configured = await agent.put(effectivePath).send(ALLOW_ALL); + expect(configured.statusCode).toBe(200); + const path = `/node-type-policies/scopes/${configured.body.scopeId}/attachments`; + const slots = [ + { priority: 10, isFloor: false }, + { priority: 20, isFloor: true }, + { priority: -5, isFloor: false }, + { priority: 0, isFloor: true }, + ]; + const attachments = []; + for (const [index, slot] of slots.entries()) { + const rules = [{ ...DENY_RULE, id: `r${index}` }]; + const created = await agent.post('/node-type-policies/policies').send({ rules }); + expect(created.statusCode).toBe(201); + attachments.push({ policyId: created.body.policy.id as string, rules, ...slot }); + } + const replaced = await agent.put(path).send({ + attachments: attachments.map(({ rules: _rules, ...attachment }) => attachment), + version: configured.body.version, + }); + expect(replaced.statusCode).toBe(200); + expect(replaced.body).toEqual({ + attachments: [attachments[3], attachments[1], attachments[2], attachments[0]], + version: configured.body.version + 1, + }); + const emitSpy = vi.spyOn(Container.get(EventService), 'emit'); + + const read = await agent.get(path); + + expect(read.statusCode).toBe(200); + expect(read.body).toEqual(replaced.body); + expect(emitSpy.mock.calls.filter(([event]) => event.startsWith('node-type-policy-'))).toEqual( + [], + ); + const effective = await agent.get(effectivePath); + expect(Object.keys(effective.body).sort()).toEqual([...EFFECTIVE_KEYS].sort()); + const updated = await agent.put(path).send({ attachments: [], version: read.body.version }); + expect(updated.statusCode).toBe(200); + expect(updated.body).toEqual({ attachments: [], version: read.body.version + 1 }); + const empty = await agent.get(path); + expect(empty.statusCode).toBe(200); + expect(empty.body).toEqual(updated.body); + }, + ); + + test('GET returns 404 for an unknown scope and a credential type scope', async () => { + const agent = testServer.publicApiAgentFor(owner); + expect( + (await agent.get('/node-type-policies/scopes/does-not-exist/attachments')).statusCode, + ).toBe(404); + const other = await agent.put('/credential-type-policies/instance').send(ALLOW_ALL); + expect(other.statusCode).toBe(200); + expect( + (await agent.get(`/node-type-policies/scopes/${other.body.scopeId}/attachments`)).statusCode, + ).toBe(404); + }); + test('PUT with a stale version returns 409 and preserves the attachments and version', async () => { const agent = testServer.publicApiAgentFor(owner); - const instance = await agent.put('/node-type-policies/instance').send({ - ...ALLOW_ALL, - rules: [DENY_RULE], - }); + const instance = await agent.put('/node-type-policies/instance').send(ALLOW_ALL); expect(instance.statusCode).toBe(200); + const created = await agent.post('/node-type-policies/policies').send({ rules: [DENY_RULE] }); + expect(created.statusCode).toBe(201); + const policyId = created.body.policy.id as string; - const stale = await agent - .put(`/node-type-policies/scopes/${instance.body.scopeId}/attachments`) - .send({ attachments: [], version: 0 }); + const path = `/node-type-policies/scopes/${instance.body.scopeId}/attachments`; + const attached = await agent.put(path).send({ + attachments: [{ policyId, priority: 0, isFloor: false }], + version: instance.body.version, + }); + expect(attached.statusCode).toBe(200); + const read = await agent.get(path); + expect(read.statusCode).toBe(200); + expect(read.body).toEqual({ + attachments: [{ policyId, rules: [DENY_RULE], priority: 0, isFloor: false }], + version: instance.body.version + 1, + }); + const replaced = await agent.put(path).send({ + attachments: read.body.attachments.map(({ policyId }: { policyId: string }) => ({ + policyId, + priority: 5, + isFloor: false, + })), + version: read.body.version, + }); + expect(replaced.statusCode).toBe(200); + expect(replaced.body).toEqual({ + attachments: [{ policyId, rules: [DENY_RULE], priority: 5, isFloor: false }], + version: read.body.version + 1, + }); + + const stale = await agent.put(path).send({ attachments: [], version: read.body.version }); expect(stale.statusCode).toBe(409); - const persisted = await agent.get('/node-type-policies/instance'); - expect(persisted.body.rules).toEqual([DENY_RULE]); - expect(persisted.body.version).toBe(instance.body.version); + const persisted = await agent.get(path); + expect(persisted.statusCode).toBe(200); + expect(persisted.body).toEqual(replaced.body); + const updated = await agent + .put(path) + .send({ attachments: [], version: persisted.body.version }); + expect(updated.statusCode).toBe(200); + expect(updated.body).toEqual({ attachments: [], version: persisted.body.version + 1 }); }); test.each([undefined, -1, 0.5])('PUT rejects invalid version %s with 400', async (version) => { diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index 8d3601b36fe..766587e622f 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -3558,6 +3558,8 @@ "settings.communityNodes.uninstallAction.label": "Uninstall package", "settings.communityNodes.upToDate.tooltip": "You are up to date", "settings.communityNodes.failedToLoad.tooltip": "This package failed to load. It may be corrupted, or it may need a newer version of n8n. Try uninstalling and reinstalling it, or upgrade n8n.", + "settings.communityNodes.restricted.badge.label": "Restricted", + "settings.communityNodes.restricted.tooltip.heading": "Blocked by an instance policy", "settings.communityNodes.managedByEnv": "Community packages are managed via environment variables and cannot be modified from the UI.", "settings.communityNodes.fetchError.title": "Problem fetching installed packages", "settings.communityNodes.fetchError.message": "There may be a problem with your internet connection or your n8n instance", diff --git a/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageCard.test.ts b/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageCard.test.ts index 5c81e9c4ff1..4b2d47bb600 100644 --- a/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageCard.test.ts +++ b/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageCard.test.ts @@ -5,6 +5,9 @@ import { STORES } from '@n8n/stores'; import CommunityPackageCard from './CommunityPackageCard.vue'; import { createComponentRenderer } from '@/__tests__/render'; import type { PublicInstalledNode, PublicInstalledPackage } from 'n8n-workflow'; +import { useTypeAvailabilityPoliciesStore } from '@n8n/frontend-module-type-availability-policies'; +import type { NodeTypeAvailability } from '@n8n/api-types'; +import { waitFor } from '@testing-library/vue'; const communityPackage: PublicInstalledPackage = { packageName: 'n8n-nodes-test', @@ -113,4 +116,72 @@ describe('CommunityPackageCard', () => { expect(queryByText('Uninstall package')).not.toBeInTheDocument(); }); }); + + describe('restricted badge', () => { + const mockNodeAvailability = ( + unavailable: Record>, + ) => { + vi.spyOn(useTypeAvailabilityPoliciesStore(), 'getNodeTypeAvailability').mockImplementation( + (name: string) => + name in unavailable + ? { name, available: false, ...unavailable[name] } + : { name, available: true }, + ); + }; + + // Reka UI only mounts tooltip content once the trigger is hovered. + function hoverTrigger(element: Element) { + element.dispatchEvent( + new PointerEvent('pointermove', { + bubbles: true, + cancelable: true, + pointerType: 'mouse', + clientX: 100, + clientY: 100, + }), + ); + } + + it('shows the badge and lists instance-blocked node types in the tooltip', async () => { + mockNodeAvailability({ 'n8n-nodes-test.oldNode': { scope: 'instance' } }); + + const { getByText } = renderComponent({ props: { communityPackage } }); + + const badge = getByText('Restricted'); + expect(badge).toBeInTheDocument(); + + hoverTrigger(badge); + + await waitFor(() => { + expect(document.querySelector('[data-dismissable-layer]')).toHaveTextContent('OldNode'); + }); + expect(document.querySelector('[data-dismissable-layer]')).not.toHaveTextContent('TestNode'); + }); + + it('does not show the badge when no node type is blocked', () => { + mockNodeAvailability({}); + + const { queryByText } = renderComponent({ props: { communityPackage } }); + + expect(queryByText('Restricted')).not.toBeInTheDocument(); + }); + + it('ignores a project-scoped restriction', () => { + mockNodeAvailability({ 'n8n-nodes-test.oldNode': { scope: 'project' } }); + + const { queryByText } = renderComponent({ props: { communityPackage } }); + + expect(queryByText('Restricted')).not.toBeInTheDocument(); + }); + + it('ignores an instance restriction that projects can opt into', () => { + mockNodeAvailability({ + 'n8n-nodes-test.oldNode': { scope: 'instance', optInAvailable: true }, + }); + + const { queryByText } = renderComponent({ props: { communityPackage } }); + + expect(queryByText('Restricted')).not.toBeInTheDocument(); + }); + }); }); diff --git a/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageCard.vue b/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageCard.vue index 3db55978b42..3244b65268f 100644 --- a/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageCard.vue +++ b/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageCard.vue @@ -1,6 +1,6 @@