From d8a2587e1ebc7faedc0947d1dc7326d812b35b02 Mon Sep 17 00:00:00 2001 From: Vault Automation Date: Mon, 11 May 2026 10:23:32 -0600 Subject: [PATCH] UI: Add validations to visual policy editor (#14688) (#14697) (#14698) * add validations to policy flyout * add validations to policy form * remove passing formatted policy back from policy/builder * add changelog * change label to "path" Co-authored-by: claire b <68122737+hellobontempo@users.noreply.github.com> --- changelog/_14688.txt | 3 + ui/app/components/policy-form.hbs | 3 +- ui/app/components/policy-form.ts | 52 +++++++- .../code-generator/policy/builder.hbs | 1 + .../code-generator/policy/builder.ts | 5 +- .../code-generator/policy/flyout.hbs | 3 +- .../code-generator/policy/flyout.ts | 27 +++- .../code-generator/policy/stanza.hbs | 20 ++- .../core/addon/components/message-error.hbs | 14 +- ui/lib/core/addon/components/message-error.js | 1 + .../addon/utils/code-generators/policy.ts | 17 +++ ui/tests/acceptance/policy/index-test.js | 3 +- ui/tests/helpers/general-selectors.ts | 4 +- .../code-generator/policy/builder-test.js | 18 ++- .../code-generator/policy/flyout-test.js | 123 +++++++++++++----- .../code-generator/policy/stanza-test.js | 58 +++++++++ .../components/message-error-test.js | 22 ++++ .../components/policy-form-test.js | 35 ++++- .../utils/code-generators/policy-test.js | 52 ++++++++ 19 files changed, 400 insertions(+), 61 deletions(-) create mode 100644 changelog/_14688.txt diff --git a/changelog/_14688.txt b/changelog/_14688.txt new file mode 100644 index 0000000000..0788c7ea17 --- /dev/null +++ b/changelog/_14688.txt @@ -0,0 +1,3 @@ +```release-note:improvement +ui: add validations to the ACL visual policy editor to prevent it from saving policies with empty paths or capabilities. +``` diff --git a/ui/app/components/policy-form.hbs b/ui/app/components/policy-form.hbs index a7e91dfdd3..123dde0e89 100644 --- a/ui/app/components/policy-form.hbs +++ b/ui/app/components/policy-form.hbs @@ -7,7 +7,7 @@ - + {{#if @model.isNew}} @@ -88,6 +88,7 @@ @policyName={{@model.name}} @onPolicyChange={{this.handlePolicyChange}} @stanzas={{this.stanzas}} + @renderValidations={{if (this.validationError "stanzas") true false}} data-test-field="visual editor" /> {{else}} diff --git a/ui/app/components/policy-form.ts b/ui/app/components/policy-form.ts index 9e1a3fc61b..03c32dd900 100644 --- a/ui/app/components/policy-form.ts +++ b/ui/app/components/policy-form.ts @@ -16,11 +16,12 @@ import { PolicyTypes, } from 'core/utils/code-generators/policy'; import errorMessage from 'vault/utils/error-message'; +import { validate } from 'vault/utils/forms/validate'; import type FlashMessageService from 'ember-cli-flash/services/flash-messages'; import type { HTMLElementEvent } from 'vault/forms'; import type { PolicyData } from 'core/components/code-generator/policy/builder'; -import type { FormField } from 'vault/vault/app-types'; +import type { FormField, ValidationMap, Validations } from 'vault/vault/app-types'; /** * @module PolicyForm @@ -67,13 +68,24 @@ export default class PolicyFormComponent extends Component { @service declare readonly flashMessages: FlashMessageService; editTypes = { [EditorTypes.VISUAL]: 'Visual editor', [EditorTypes.CODE]: 'Code editor' } as const; + validations: Validations = { + stanzas: [ + { + validator: ({ stanzas }) => + stanzas.length > 0 && stanzas.every((stanza: PolicyStanza) => stanza.isValid), + message: 'Invalid policy content.', + }, + ], + }; @tracked editType: EditorTypes = EditorTypes.VISUAL; @tracked errorBanner = ''; + @tracked errorDetails: string[] = []; @tracked showFileUpload = false; @tracked showSwitchEditorsModal = false; @tracked showTemplateModal = false; @tracked stanzas: PolicyStanza[] = [new PolicyStanza()]; + @tracked validationErrors: ValidationMap | null = null; constructor(owner: unknown, args: Args) { super(owner, args); @@ -81,19 +93,29 @@ export default class PolicyFormComponent extends Component { this.editType = this.args.model.policyType === PolicyTypes.ACL ? EditorTypes.VISUAL : EditorTypes.CODE; } + // Template helpers isActiveEditor = (type: string): boolean => type === this.editType; + validationError = (param: string) => { + const { isValid, errors } = this.validationErrors?.[param] ?? {}; + return !isValid && errors ? errors.join(' ') : ''; + }; + + get formattedStanzas() { + return formatStanzas(this.stanzas); + } + get hasPolicyDiff() { const { policy } = this.args.model; // Make sure policy has a value (if it's undefined, neither editor has been used) // Return true if there is a difference between stanzas and policy arg // which means the user has made changes using the code editor - return policy && formatStanzas(this.stanzas) !== policy; + return policy && this.formattedStanzas !== policy; } get snippetArgs() { const policyName = this.args.model.name || ''; - const policy = formatStanzas(this.stanzas); + const policy = this.formattedStanzas; return policySnippetArgs(policyName, policy); } @@ -108,7 +130,7 @@ export default class PolicyFormComponent extends Component { this.editType = EditorTypes.VISUAL; this.showSwitchEditorsModal = false; // Reset this.args.model.policy to match visual editor stanzas - this.setPolicy(formatStanzas(this.stanzas)); + this.setPolicy(this.formattedStanzas); } @action @@ -118,9 +140,10 @@ export default class PolicyFormComponent extends Component { } @action - handlePolicyChange({ policy, stanzas }: PolicyData) { - this.setPolicy(policy); + handlePolicyChange({ stanzas }: PolicyData) { + // Update tracked stanzas first, then pass formatted policy back to model this.stanzas = stanzas; + this.setPolicy(this.formattedStanzas); } @action @@ -146,6 +169,23 @@ export default class PolicyFormComponent extends Component { @task *save(event: HTMLElementEvent) { event.preventDefault(); + + // Name is intentionally not validated here because the input has @isRequired=true + // which prevents the submit event all together when it is empty. + const { isValid, state } = validate({ stanzas: this.stanzas }, this.validations); + // Only enforce stanza validations for the Visual Editor + const shouldValidate = this.visualEditorSupported && this.editType === EditorTypes.VISUAL; + if (!isValid && shouldValidate) { + this.validationErrors = state; + this.errorDetails = Object.values(state).flatMap((s) => s.errors); + // Render general error message instead of exact count from validate() because + // stanzas (which are validated as a single input) can have up to 2 errors each. + const msg = this.errorDetails.length > 1 ? 'are errors' : 'is an error'; + this.errorBanner = `There ${msg} with this form.`; + // Abort saving + return; + } + try { const { name, policyType, isNew } = this.args.model; yield this.args.model.save(); diff --git a/ui/lib/core/addon/components/code-generator/policy/builder.hbs b/ui/lib/core/addon/components/code-generator/policy/builder.hbs index 9c40ff97ab..20bcdc7ef2 100644 --- a/ui/lib/core/addon/components/code-generator/policy/builder.hbs +++ b/ui/lib/core/addon/components/code-generator/policy/builder.hbs @@ -10,6 +10,7 @@ @onChange={{this.updateStanzas}} @onDelete={{fn this.deleteStanza stanza}} @stanza={{stanza}} + @renderValidations={{@renderValidations}} class="has-bottom-margin-m" /> {{/each}} diff --git a/ui/lib/core/addon/components/code-generator/policy/builder.ts b/ui/lib/core/addon/components/code-generator/policy/builder.ts index d31a36af9e..8e365e4fdb 100644 --- a/ui/lib/core/addon/components/code-generator/policy/builder.ts +++ b/ui/lib/core/addon/components/code-generator/policy/builder.ts @@ -5,11 +5,10 @@ import { action } from '@ember/object'; import Component from '@glimmer/component'; -import { formatStanzas, PolicyStanza } from 'core/utils/code-generators/policy'; +import { PolicyStanza } from 'core/utils/code-generators/policy'; import { assert } from '@ember/debug'; export interface PolicyData { - policy: string; stanzas: PolicyStanza[]; } interface Args { @@ -45,6 +44,6 @@ export default class CodeGeneratorPolicyBuilder extends Component { @action updateStanzas(stanzas?: PolicyStanza[]) { const updated = stanzas ?? this.args.stanzas; - this.args.onPolicyChange({ policy: formatStanzas(updated), stanzas: updated }); + this.args.onPolicyChange({ stanzas: updated }); } } diff --git a/ui/lib/core/addon/components/code-generator/policy/flyout.hbs b/ui/lib/core/addon/components/code-generator/policy/flyout.hbs index d67540b736..b99acf2fe9 100644 --- a/ui/lib/core/addon/components/code-generator/policy/flyout.hbs +++ b/ui/lib/core/addon/components/code-generator/policy/flyout.hbs @@ -28,7 +28,7 @@ {{#if this.errorMessage}} - + {{/if}} @@ -65,6 +65,7 @@ @policyName={{this.policyName}} @onPolicyChange={{this.handlePolicyChange}} @stanzas={{this.stanzas}} + @renderValidations={{if (this.validationError "stanzas") true false}} data-test-field="visual editor" /> diff --git a/ui/lib/core/addon/components/code-generator/policy/flyout.ts b/ui/lib/core/addon/components/code-generator/policy/flyout.ts index 90c906cb20..8793c0aab1 100644 --- a/ui/lib/core/addon/components/code-generator/policy/flyout.ts +++ b/ui/lib/core/addon/components/code-generator/policy/flyout.ts @@ -36,10 +36,17 @@ export default class CodeGeneratorPolicyFlyout extends Component { validations: Validations = { name: [{ type: 'presence', message: 'Name is required.' }], + stanzas: [ + { + validator: ({ stanzas }) => + stanzas.length > 0 && stanzas.every((stanza: PolicyStanza) => stanza.isValid), + message: 'Invalid policy content.', + }, + ], }; + @tracked errorDetails: string[] = []; @tracked errorMessage = ''; - @tracked policyContent = ''; @tracked policyName = ''; @tracked showFlyout = false; @tracked stanzas: PolicyStanza[] = this.defaultStanzas; @@ -61,11 +68,14 @@ export default class CodeGeneratorPolicyFlyout extends Component { } @action - handlePolicyChange({ policy, stanzas }: PolicyData) { - this.policyContent = policy; + handlePolicyChange({ stanzas }: PolicyData) { this.stanzas = stanzas; } + get policyContent() { + return formatStanzas(this.stanzas); + } + get snippetArgs() { const policyName = this.policyName || ''; const policy = formatStanzas(this.stanzas); @@ -77,10 +87,15 @@ export default class CodeGeneratorPolicyFlyout extends Component { event.preventDefault(); this.resetErrors(); - const { isValid, state, invalidFormMessage } = validate({ name: this.policyName }, this.validations); + const { isValid, state } = validate({ name: this.policyName, stanzas: this.stanzas }, this.validations); + if (!isValid) { this.validationErrors = state; - this.errorMessage = invalidFormMessage; + this.errorDetails = Object.values(state).flatMap((s) => s.errors); + // Render general error message instead of exact count from validate() because + // stanzas (which are validated as a single input) can have up to 2 errors each. + const msg = this.errorDetails.length > 1 ? 'are errors' : 'is an error'; + this.errorMessage = `There ${msg} with this form.`; return; } @@ -125,11 +140,11 @@ export default class CodeGeneratorPolicyFlyout extends Component { resetErrors() { this.validationErrors = null; this.errorMessage = ''; + this.errorDetails = []; } resetFlyoutState() { this.policyName = ''; - this.policyContent = ''; this.stanzas = [new PolicyStanza()]; } diff --git a/ui/lib/core/addon/components/code-generator/policy/stanza.hbs b/ui/lib/core/addon/components/code-generator/policy/stanza.hbs index 1d5ae86718..9414bdf0d8 100644 --- a/ui/lib/core/addon/components/code-generator/policy/stanza.hbs +++ b/ui/lib/core/addon/components/code-generator/policy/stanza.hbs @@ -12,7 +12,7 @@ > - Rule + Path {{if this.showPreview "Hide" "Show"}} preview @@ -28,17 +28,23 @@ data-test-field="preview" /> {{else}} - - + + as |F| + > + {{#if (and @renderValidations @stanza.invalidPath)}} + {{@stanza.invalidPath}} + {{/if}} + - + Capabilities {{#each this.permissions as |capability|}} {{capability}} {{/each}} + {{#if (and @renderValidations @stanza.invalidCapabilities)}} + {{@stanza.invalidCapabilities}} + {{/if}} {{/if}} diff --git a/ui/lib/core/addon/components/message-error.hbs b/ui/lib/core/addon/components/message-error.hbs index 485e4ae214..85bac3a8db 100644 --- a/ui/lib/core/addon/components/message-error.hbs +++ b/ui/lib/core/addon/components/message-error.hbs @@ -39,7 +39,19 @@ as |A| > Error - {{error}} + + {{error}} + + {{#if @errorDetails}} +
    + {{#each @errorDetails as |detail|}} +
  • + {{detail}} +
    • + {{/each}} +
+ {{/if}} + {{/each}} {{/if}} \ No newline at end of file diff --git a/ui/lib/core/addon/components/message-error.js b/ui/lib/core/addon/components/message-error.js index 9f6463b248..ae402c7b71 100644 --- a/ui/lib/core/addon/components/message-error.js +++ b/ui/lib/core/addon/components/message-error.js @@ -16,6 +16,7 @@ import Component from '@glimmer/component'; * @param {object} [model=null] - An Ember data model that will be used to bind error states to the internal `errors` property. * @param {array} [errors=null] - An array of error strings to show. * @param {string} [errorMessage=null] - An Error string to display. + * @param {array} [errorDetails=null] - Renders a list of errors when error is not from the API. Helpful for rendering a list of client-side validation errors. */ export default class MessageError extends Component { diff --git a/ui/lib/core/addon/utils/code-generators/policy.ts b/ui/lib/core/addon/utils/code-generators/policy.ts index e6a36f21f6..4d632844ca 100644 --- a/ui/lib/core/addon/utils/code-generators/policy.ts +++ b/ui/lib/core/addon/utils/code-generators/policy.ts @@ -26,6 +26,23 @@ export class PolicyStanza { get preview() { return aclTemplate(this.path, Array.from(this.capabilities)); } + + get isValid() { + // Stanza must have a non-empty path and capabilities selected to be valid + return !this.invalidCapabilities && !this.invalidPath; + } + + // These getters return an error message when invalid and an empty string when valid. + // Negative naming is a little counterintuitive, but simplifies template logic + // so the message only renders when the value is truthy. + get invalidCapabilities() { + return this.capabilities.size > 0 ? '' : 'Rule must have at least one capability.'; + } + + get invalidPath() { + const isValid = typeof this.path === 'string' && this.path.trim().length > 0; + return isValid ? '' : 'Path cannot be empty.'; + } } export const formatStanzas = (stanzas: PolicyStanza[]) => stanzas.map((s) => s.preview).join('\n'); diff --git a/ui/tests/acceptance/policy/index-test.js b/ui/tests/acceptance/policy/index-test.js index 9bcdc46a6f..460aa3941e 100644 --- a/ui/tests/acceptance/policy/index-test.js +++ b/ui/tests/acceptance/policy/index-test.js @@ -110,11 +110,12 @@ module('Acceptance | policies/acl', function (hooks) { await click(SELECT.createPolicy); await fillIn(GENERAL.inputByAttr('name'), policyName); + // Select code editor first to bypass visual policy editor validations so we get an API error + await click(GENERAL.radioByAttr('code')); await click(GENERAL.submitButton); assert .dom(GENERAL.messageError) .hasText(`Error 'policy' parameter not supplied or empty`, 'renders error message on save'); - await click(GENERAL.radioByAttr('code')); await waitFor('.cm-editor'); const editor = codemirror(); setCodeEditorValue(editor, policyString); diff --git a/ui/tests/helpers/general-selectors.ts b/ui/tests/helpers/general-selectors.ts index bd88896d2e..c65d1e1d3b 100644 --- a/ui/tests/helpers/general-selectors.ts +++ b/ui/tests/helpers/general-selectors.ts @@ -150,8 +150,8 @@ export const GENERAL = { inlineError: '[data-test-inline-error-message]', messageError: '[data-test-message-error]', messageDescription: '[data-test-message-error-description]', - validationErrorByAttr: (attr: string) => `[data-test-validation-error=${attr}]`, - validationWarningByAttr: (attr: string) => `[data-test-validation-warning=${attr}]`, + validationErrorByAttr: (attr: string) => `[data-test-validation-error="${attr}"]`, + validationWarningByAttr: (attr: string) => `[data-test-validation-warning="${attr}"]`, pageError: { error: '[data-test-page-error]', diff --git a/ui/tests/integration/components/code-generator/policy/builder-test.js b/ui/tests/integration/components/code-generator/policy/builder-test.js index 6c6caa47cf..dbeed33fe8 100644 --- a/ui/tests/integration/components/code-generator/policy/builder-test.js +++ b/ui/tests/integration/components/code-generator/policy/builder-test.js @@ -8,19 +8,21 @@ import { setupRenderingTest } from 'vault/tests/helpers'; import { render, click, fillIn, setupOnerror } from '@ember/test-helpers'; import { hbs } from 'ember-cli-htmlbars'; import { GENERAL } from 'vault/tests/helpers/general-selectors'; -import { ACL_CAPABILITIES, PolicyStanza } from 'core/utils/code-generators/policy'; +import { ACL_CAPABILITIES, formatStanzas, PolicyStanza } from 'core/utils/code-generators/policy'; module('Integration | Component | code-generator/policy/builder', function (hooks) { setupRenderingTest(hooks); hooks.beforeEach(function () { - this.onPolicyChange = ({ policy, stanzas }) => { - this.set('policyCallback', policy); + this.onPolicyChange = ({ stanzas }) => { + // Mimics consumers of this component which use formatStanzas as needed + this.set('policyCallback', formatStanzas(stanzas)); this.set('stanzas', stanzas); }; this.policyCallback = ''; this.policyName = undefined; + this.renderValidations = false; this.stanzas = [new PolicyStanza()]; this.renderComponent = () => { @@ -28,6 +30,7 @@ module('Integration | Component | code-generator/policy/builder', function (hook `); }; @@ -230,4 +233,13 @@ path "" { }`; this.assertPolicyUpdate(assert, expectedPolicy, 'when rules do have an empty path'); }); + + test('it passes @renderValidations through to CodeGenerator::Policy::Stanza', async function (assert) { + this.renderValidations = true; + await this.renderComponent(); + assert.dom(GENERAL.validationErrorByAttr('path-0')).hasText('Path cannot be empty.'); + assert + .dom(GENERAL.validationErrorByAttr('capabilities-0')) + .hasText('Rule must have at least one capability.'); + }); }); diff --git a/ui/tests/integration/components/code-generator/policy/flyout-test.js b/ui/tests/integration/components/code-generator/policy/flyout-test.js index 662c712736..5aa64532b6 100644 --- a/ui/tests/integration/components/code-generator/policy/flyout-test.js +++ b/ui/tests/integration/components/code-generator/policy/flyout-test.js @@ -208,15 +208,6 @@ EOT`; assert.dom(GENERAL.inputByAttr('name')).hasValue('mypolicy', 'name is converted to lowercase'); }); - test('it does not submit default stanza templates as policy payload', async function (assert) { - assert.expect(3); - const expectedPolicy = ''; - this.assertSaveRequest(assert, expectedPolicy, 'policy payload is empty when visual editor is untouched'); - await this.renderComponent(); - await fillIn(GENERAL.inputByAttr('name'), 'test-policy'); - await click(GENERAL.submitButton); - }); - test('it saves a policy', async function (assert) { assert.expect(7); const flashSuccessSpy = Sinon.spy(this.owner.lookup('service:flash-messages'), 'success'); @@ -246,7 +237,7 @@ EOT`; }); test('it resets after saving a policy', async function (assert) { - assert.expect(11); + assert.expect(8); const expectedPolicy = `path "secret/data/*" {\n capabilities = ["read"]\n}`; this.assertSaveRequest(assert, expectedPolicy); await this.renderComponent(); @@ -277,20 +268,18 @@ EOT } EOT`; assert.dom(GENERAL.fieldByAttr('cli')).hasText(expectedCli); - // Fill in name and save again to make sure policyContent is reset - this.assertSaveRequest(assert, '', 'policy content is empty after a successful save'); - await fillIn(GENERAL.inputByAttr('name'), 'test-policy'); - await click(GENERAL.submitButton); }); - test('it displays error message when save fails', async function (assert) { + test('it displays API error message when save fails', async function (assert) { this.server.post('/sys/policies/acl/:name', () => { - return overrideResponse(400, { errors: ["'policy' parameter not supplied or empty"] }); + return overrideResponse(400, { errors: ['something went very wrong'] }); }); await this.renderComponent(); - await fillIn(GENERAL.inputByAttr('name'), 'empty-policy'); + await fillIn(GENERAL.inputByAttr('name'), 'failed-policy'); + await fillIn(GENERAL.inputByAttr('path'), 'secret/data/*'); + await click(GENERAL.checkboxByAttr('read')); await click(GENERAL.submitButton); - assert.dom(GENERAL.messageError).exists().hasText("Error 'policy' parameter not supplied or empty"); + assert.dom(GENERAL.messageError).exists().hasText('Error something went very wrong'); assert.dom(GENERAL.flyout).exists('flyout remains open after error'); }); @@ -326,22 +315,85 @@ EOT`; await click(GENERAL.submitButton); }); - test('it renders validation errors', async function (assert) { + test('it renders validation errors for invalid policy content', async function (assert) { await this.renderComponent(); + await fillIn(GENERAL.inputByAttr('name'), 'test-policy'); + // Submit without filling in path or capabilities (missing path AND no capabilities) await click(GENERAL.submitButton); - assert.dom(GENERAL.messageError).exists().hasText('Error There is an error with this form.'); + assert + .dom(GENERAL.messageError) + .exists() + .hasText('Error There is an error with this form. Invalid policy content.'); + assert.dom(SELECTORS.pathByContainer(0)).hasClass('hds-form-text-input--is-invalid'); + assert.dom(GENERAL.validationErrorByAttr('path-0')).exists().hasText('Path cannot be empty.'); + assert + .dom(GENERAL.validationErrorByAttr('capabilities-0')) + .exists() + .hasText('Rule must have at least one capability.'); + }); + + test('it renders validation errors for missing path', async function (assert) { + await this.renderComponent(); + await fillIn(GENERAL.inputByAttr('name'), 'test-policy'); + await fillIn(GENERAL.inputByAttr('path'), 'secret/*'); + await click(GENERAL.checkboxByAttr('read')); + // Add a second rule and just select capabilities (leave path empty) + await click(GENERAL.button('Add rule')); + await click(SELECTORS.checkboxByContainer(1, 'update')); + await click(GENERAL.submitButton); + assert + .dom(GENERAL.messageError) + .exists() + .hasText('Error There is an error with this form. Invalid policy content.'); + assert.dom(SELECTORS.pathByContainer(1)).hasClass('hds-form-text-input--is-invalid'); + assert.dom(GENERAL.validationErrorByAttr('path-1')).hasText('Path cannot be empty.'); + assert.dom('[data-test-validation-error]').exists({ count: 1 }, 'only one validation error renders'); + }); + + test('it renders validation errors for missing capabilities', async function (assert) { + await this.renderComponent(); + await fillIn(GENERAL.inputByAttr('name'), 'test-policy'); + await fillIn(GENERAL.inputByAttr('path'), 'secret/*'); + await click(GENERAL.checkboxByAttr('read')); + // Add a second rule and just fill in path (no capabilities selected) + await click(GENERAL.button('Add rule')); + await fillIn(SELECTORS.pathByContainer(1), 'new/path/*'); + await click(GENERAL.submitButton); + assert + .dom(GENERAL.messageError) + .exists() + .hasText('Error There is an error with this form. Invalid policy content.'); + assert + .dom(GENERAL.validationErrorByAttr('capabilities-1')) + .exists() + .hasText('Rule must have at least one capability.'); + assert.dom('[data-test-validation-error]').exists({ count: 1 }, 'only one validation error renders'); + }); + + test('it renders validation error for empty policy name', async function (assert) { + await this.renderComponent(); + await fillIn(GENERAL.inputByAttr('path'), 'secret/*'); + await click(GENERAL.checkboxByAttr('read')); + await click(GENERAL.submitButton); + assert + .dom(GENERAL.messageError) + .exists() + .hasText('Error There is an error with this form. Name is required.'); assert.dom(GENERAL.inputByAttr('name')).hasClass('hds-form-text-input--is-invalid'); assert.dom(GENERAL.validationErrorByAttr('name')).hasText('Name is required.'); }); - test('it resets errors after saving', async function (assert) { + test('it resets validation errors after saving', async function (assert) { const expectedPolicy = `path "secret/*" {\n capabilities = ["read"]\n}`; this.assertSaveRequest(assert, expectedPolicy); await this.renderComponent(); - // First attempt without name + // First attempt without name and policy content await click(GENERAL.submitButton); - assert.dom(GENERAL.messageError).exists().hasText('Error There is an error with this form.'); + assert + .dom(GENERAL.messageError) + .exists() + .hasText('Error There are errors with this form. Name is required. Invalid policy content.'); assert.dom(GENERAL.validationErrorByAttr('name')).exists('validation error shows'); // Second attempt with name @@ -356,11 +408,14 @@ EOT`; assert.dom(GENERAL.validationErrorByAttr('name')).doesNotExist('validation error is cleared'); }); - test('it resets errors if flyout is closed and policy is NOT saved', async function (assert) { + test('it resets validation errors if flyout is closed and policy is NOT saved', async function (assert) { await this.renderComponent(); // Attempt to save await click(GENERAL.submitButton); - assert.dom(GENERAL.messageError).exists().hasText('Error There is an error with this form.'); + assert + .dom(GENERAL.messageError) + .exists() + .hasText('Error There are errors with this form. Name is required. Invalid policy content.'); assert.dom(GENERAL.validationErrorByAttr('name')).exists('validation error shows'); // Cancel and close flyout await click(GENERAL.cancelButton); @@ -493,14 +548,24 @@ EOT`; assert.dom(SELECTORS.pathByContainer(1)).hasValue('new/path/*', 'user added path still exists'); }); - test('it does not save prepopulated paths as policy content', async function (assert) { - assert.expect(3); + test('prepopulated paths trigger validation errors', async function (assert) { this.cacheCapabilityPaths('vault.cluster.secrets.secret', ['path/one', 'path/two']); await this.renderComponent(); - // Fill in name and save to make sure policyContent is empty - this.assertSaveRequest(assert, '', 'policy content is empty despite pre-filled paths'); + // Only fill in name to make sure capabilities validation triggers await fillIn(GENERAL.inputByAttr('name'), 'test-policy'); await click(GENERAL.submitButton); + assert + .dom(GENERAL.messageError) + .exists() + .hasText('Error There is an error with this form. Invalid policy content.'); + assert + .dom(GENERAL.validationErrorByAttr('capabilities-0')) + .exists() + .hasText('Rule must have at least one capability.'); + assert + .dom(GENERAL.validationErrorByAttr('capabilities-1')) + .exists() + .hasText('Rule must have at least one capability.'); }); }); }); diff --git a/ui/tests/integration/components/code-generator/policy/stanza-test.js b/ui/tests/integration/components/code-generator/policy/stanza-test.js index 247b76342c..d75898d8eb 100644 --- a/ui/tests/integration/components/code-generator/policy/stanza-test.js +++ b/ui/tests/integration/components/code-generator/policy/stanza-test.js @@ -26,6 +26,7 @@ module('Integration | Component | code-generator/policy/stanza', function (hooks @onChange={{this.onChange}} @onDelete={{this.onDelete}} @stanza={{this.stanza}} + @renderValidations={{this.renderValidations}} />`); }; }); @@ -168,4 +169,61 @@ module('Integration | Component | code-generator/policy/stanza', function (hooks await click(GENERAL.button('Delete')); assert.true(this.onDelete.calledOnce, 'onDelete is called'); }); + + test('it does not render validations when @renderValidations is undefined', async function (assert) { + await this.renderComponent(); + // Make sure path and capabilities are empty (which would normally render validations) + assert.dom(GENERAL.inputByAttr('path')).hasNoValue(); + ACL_CAPABILITIES.forEach((capability) => { + assert.dom(GENERAL.fieldLabel(capability)).hasText(capability); + assert.dom(GENERAL.checkboxByAttr(capability)).isNotChecked(); + }); + assert.dom('[data-test-validation-error]').doesNotExist(); + }); + + module('validations', function (hooks) { + hooks.beforeEach(function () { + this.renderValidations = true; + }); + + test('it renders validations when path is empty', async function (assert) { + await this.renderComponent(); + // Click capability to just assert path + await click(GENERAL.checkboxByAttr('update')); + assert.dom(GENERAL.validationErrorByAttr('path-0')).exists().hasText('Path cannot be empty.'); + assert.dom('[data-test-validation-error]').exists({ count: 1 }); + }); + + test('it renders validations when no capabilities are selected', async function (assert) { + await this.renderComponent(); + // Fill in path to only assert capabilities + await fillIn(GENERAL.inputByAttr('path'), 'my/super/secret/*'); + assert + .dom(GENERAL.validationErrorByAttr('capabilities-0')) + .exists() + .hasText('Rule must have at least one capability.'); + assert.dom('[data-test-validation-error]').exists({ count: 1 }); + }); + + test('it renders validations for neither path or capabilities', async function (assert) { + await this.renderComponent(); + assert.dom(GENERAL.validationErrorByAttr('path-0')).exists(); + assert.dom(GENERAL.validationErrorByAttr('capabilities-0')).exists(); + assert.dom('[data-test-validation-error]').exists({ count: 2 }); + }); + + test('it removes validation when path is valid', async function (assert) { + await this.renderComponent(); + assert.dom(GENERAL.validationErrorByAttr('path-0')).exists(); + await fillIn(GENERAL.inputByAttr('path'), 'secret/data/*'); + assert.dom(GENERAL.validationErrorByAttr('path-0')).doesNotExist(); + }); + + test('it removes validation when at least one capability is selected', async function (assert) { + await this.renderComponent(); + assert.dom(GENERAL.validationErrorByAttr('capabilities-0')).exists(); + await click(GENERAL.checkboxByAttr('read')); + assert.dom(GENERAL.validationErrorByAttr('capabilities-0')).doesNotExist(); + }); + }); }); diff --git a/ui/tests/integration/components/message-error-test.js b/ui/tests/integration/components/message-error-test.js index 3ef56b2d91..258c7e975a 100644 --- a/ui/tests/integration/components/message-error-test.js +++ b/ui/tests/integration/components/message-error-test.js @@ -110,4 +110,26 @@ module('Integration | Component | message-error', function (hooks) { await click(GENERAL.icon('x')); assert.true(this.onDismiss.calledOnce, '@onDismiss is called once'); }); + + test('it renders @errorDetails as a list under the error message', async function (assert) { + this.errorMessage = 'There is an error with this form.'; + await render(hbs` + `); + assert.dom(GENERAL.messageError).exists({ count: 1 }); + assert.dom(GENERAL.messageDescription).hasTextContaining('There is an error with this form.'); + assert.dom(`${GENERAL.messageDescription} li`).exists({ count: 2 }); + assert.dom(`${GENERAL.messageDescription} li:first-child`).hasText('Path cannot be empty.'); + assert + .dom(`${GENERAL.messageDescription} li:last-child`) + .hasText('Rule must have at least one capability.'); + }); + + test('it does not render error details list when @errorDetails is not provided', async function (assert) { + this.errorMessage = 'Something went wrong'; + await this.renderComponent(); + assert.dom(`${GENERAL.messageDescription} ul`).doesNotExist(); + }); }); diff --git a/ui/tests/integration/components/policy-form-test.js b/ui/tests/integration/components/policy-form-test.js index c8a297f335..9f9a462850 100644 --- a/ui/tests/integration/components/policy-form-test.js +++ b/ui/tests/integration/components/policy-form-test.js @@ -151,10 +151,12 @@ module('Integration | Component | policy-form', function (hooks) { ); }); - test('it shows the error message on form when save fails', async function (assert) { + test('it shows the error message on form when save returns API error', async function (assert) { this.model.name = 'bad-policy'; this.model.policy = 'some policy content'; await this.renderComponent(); + // Change editors so we don't trigger visual editor validations + await click(GENERAL.radioByAttr('code')); await click(GENERAL.submitButton); assert.true(this.onSave.notCalled, 'onSave is not called yet'); assert.dom(GENERAL.messageError).includesText('An error occurred'); @@ -231,7 +233,7 @@ module('Integration | Component | policy-form', function (hooks) { assert.dom(GENERAL.codemirror).doesNotExist('JSON editor does not render by default'); assert .dom(GENERAL.fieldByAttr('visual editor')) - .hasTextContaining('Rule Show preview') + .hasTextContaining('Path Show preview') .exists('it renders visual policy editor by default'); // Select Code editor await click(GENERAL.radioByAttr('code')); @@ -246,7 +248,7 @@ module('Integration | Component | policy-form', function (hooks) { assert.dom(GENERAL.codemirror).doesNotExist(); assert .dom(GENERAL.fieldByAttr('visual editor')) - .hasTextContaining('Rule Show preview') + .hasTextContaining('Path Show preview') .exists('Visual editor renders after selecting radio'); }); @@ -276,6 +278,33 @@ path "second/path" { assert.strictEqual(actual.policy, expectedPolicy, 'save is called with expected policy'); }); + test('it shows validation errors for invalid policy stanzas', async function (assert) { + await this.renderComponent(); + await fillIn(GENERAL.inputByAttr('name'), 'test-policy'); + await click(GENERAL.submitButton); + + assert.true(this.onSave.notCalled, 'onSave is not called'); + assert + .dom(GENERAL.messageError) + .exists() + .hasText('Error There is an error with this form. Invalid policy content.'); + assert.dom(GENERAL.validationErrorByAttr('path-0')).hasText('Path cannot be empty.'); + assert + .dom(GENERAL.validationErrorByAttr('capabilities-0')) + .hasText('Rule must have at least one capability.'); + }); + + test('it still saves from the code editor when visual stanzas are invalid', async function (assert) { + await this.renderComponent(); + await fillIn(GENERAL.inputByAttr('name'), 'test-policy'); + await click(GENERAL.radioByAttr('code')); + await setEditorValue(this.policy); + await click(GENERAL.submitButton); + + assert.true(this.onSave.calledOnceWith(this.model), 'onSave is called with model'); + assert.dom(GENERAL.messageError).doesNotExist('validation banner does not render in code editor'); + }); + // Automation snippets are only supported for "ACL" policy types at this time test('it renders empty snippets by default', async function (assert) { await this.renderComponent(); diff --git a/ui/tests/integration/utils/code-generators/policy-test.js b/ui/tests/integration/utils/code-generators/policy-test.js index 3b69826202..42f018ff0e 100644 --- a/ui/tests/integration/utils/code-generators/policy-test.js +++ b/ui/tests/integration/utils/code-generators/policy-test.js @@ -142,4 +142,56 @@ module('Integration | Util | code-generators/policy', function (hooks) { assert.true(secondPreview.includes('"read", "list"'), 'new preview includes both capabilities'); assert.strictEqual(secondPreview, expected, 'new preview reflects updates'); }); + + test('PolicyStanza: invalidPath returns error message when path is empty', function (assert) { + const stanza = new PolicyStanza(); + assert.strictEqual(stanza.invalidPath, 'Path cannot be empty.', 'returns error for empty path'); + }); + + test('PolicyStanza: invalidPath returns error message when path is only whitespace', function (assert) { + const stanza = new PolicyStanza({ path: ' ' }); + assert.strictEqual(stanza.invalidPath, 'Path cannot be empty.', 'returns error for whitespace path'); + }); + + test('PolicyStanza: invalidPath returns empty string when path is valid', function (assert) { + const stanza = new PolicyStanza({ path: 'secret/data/*' }); + assert.strictEqual(stanza.invalidPath, '', 'returns empty string for valid path'); + }); + + test('PolicyStanza: invalidCapabilities returns error message when no capabilities are selected', function (assert) { + const stanza = new PolicyStanza(); + assert.strictEqual( + stanza.invalidCapabilities, + 'Rule must have at least one capability.', + 'returns error when no capabilities selected' + ); + }); + + test('PolicyStanza: invalidCapabilities returns empty string when at least one capability is selected', function (assert) { + const stanza = new PolicyStanza(); + stanza.capabilities.add('read'); + assert.strictEqual(stanza.invalidCapabilities, '', 'returns empty string when capability is selected'); + }); + + test('PolicyStanza: isValid returns false when path and capabilities are both empty', function (assert) { + const stanza = new PolicyStanza(); + assert.false(stanza.isValid, 'invalid when path and capabilities are empty'); + }); + + test('PolicyStanza: isValid returns false when path is empty but capabilities are set', function (assert) { + const stanza = new PolicyStanza(); + stanza.capabilities.add('read'); + assert.false(stanza.isValid, 'invalid when path is empty'); + }); + + test('PolicyStanza: isValid returns false when path is set but capabilities are empty', function (assert) { + const stanza = new PolicyStanza({ path: 'secret/data/*' }); + assert.false(stanza.isValid, 'invalid when no capabilities selected'); + }); + + test('PolicyStanza: isValid returns true when path and capabilities are both set', function (assert) { + const stanza = new PolicyStanza({ path: 'secret/data/*' }); + stanza.capabilities.add('read'); + assert.true(stanza.isValid, 'valid when path and capabilities are set'); + }); });