From fbce5986c17fc917ab1206c3280b416166609995 Mon Sep 17 00:00:00 2001 From: claire bontempo <68122737+hellobontempo@users.noreply.github.com> Date: Fri, 18 Mar 2022 16:47:42 -0700 Subject: [PATCH] UI/Wrong sentinel error message for auth methods (#14551) * priortize adapter error over model error * glimmerize message-error component * message error tweaks * fix glimmerize * fix some tests * change error handling for mount backend form * throw API error for secret engine not mounting * fix tests" * fix tests * cleanup error handling for secret engine mounts * fix test selector * add changelog * STOP BEING FLAKY --- changelog/14551.txt | 3 + ui/app/adapters/secret-engine.js | 11 +-- ui/app/components/config-pki-ca.js | 5 +- ui/app/components/config-pki.js | 6 +- ui/app/components/mount-backend-form.js | 49 ++++++++---- ui/app/mixins/policy-edit-controller.js | 5 +- ui/app/models/policy.js | 2 +- ui/app/templates/components/config-pki-ca.hbs | 2 +- ui/app/templates/components/config-pki.hbs | 2 +- .../components/mount-backend-form.hbs | 2 +- .../vault/cluster/policies/create.hbs | 2 +- ui/lib/core/addon/components/message-error.js | 75 ++++++++----------- .../templates/components/message-error.hbs | 4 +- ui/tests/acceptance/client-current-test.js | 13 +--- .../pki/section-urls-test.js | 5 +- .../settings/mount-secret-backend-test.js | 2 +- .../integration/components/auth-form-test.js | 2 +- .../integration/components/ttl-picker-test.js | 5 +- ui/tests/pages/components/auth-form.js | 3 +- 19 files changed, 97 insertions(+), 101 deletions(-) create mode 100644 changelog/14551.txt diff --git a/changelog/14551.txt b/changelog/14551.txt new file mode 100644 index 000000000..47ef9add2 --- /dev/null +++ b/changelog/14551.txt @@ -0,0 +1,3 @@ +```release-note:bug +ui: Fix issue where UI incorrectly handled API errors when mounting backends +``` \ No newline at end of file diff --git a/ui/app/adapters/secret-engine.js b/ui/app/adapters/secret-engine.js index 1d75aae42..40edfc300 100644 --- a/ui/app/adapters/secret-engine.js +++ b/ui/app/adapters/secret-engine.js @@ -61,15 +61,8 @@ export default ApplicationAdapter.extend({ data.id = path; } // first create the engine - try { - await this.ajax(this.url(path), 'POST', { data }); - } catch (e) { - // if error determine if path duplicate or permissions - if (e.httpStatus === 400) { - throw new Error('samePath'); - } - throw new Error('mountIssue'); - } + await this.ajax(this.url(path), 'POST', { data }); + // second post to config try { await this.ajax(this.urlForConfig(path), 'POST', { data: configData }); diff --git a/ui/app/components/config-pki-ca.js b/ui/app/components/config-pki-ca.js index c5814bd70..f90bbddbe 100644 --- a/ui/app/components/config-pki-ca.js +++ b/ui/app/components/config-pki-ca.js @@ -7,6 +7,7 @@ export default Component.extend({ classNames: 'config-pki-ca', store: service('store'), flashMessages: service(), + errors: null, /* * @param boolean @@ -150,8 +151,8 @@ export default Component.extend({ ); } }) - .catch(() => { - // handle promise rejection - error will be shown by message-error component + .catch((e) => { + this.set('errors', e.errors); }) .finally(() => { this.set('loading', false); diff --git a/ui/app/components/config-pki.js b/ui/app/components/config-pki.js index 1a45d6fa4..50b5a9119 100644 --- a/ui/app/components/config-pki.js +++ b/ui/app/components/config-pki.js @@ -5,7 +5,7 @@ import { get } from '@ember/object'; export default Component.extend({ classNames: 'config-pki', flashMessages: service(), - + errors: null, /* * * @param String @@ -55,8 +55,8 @@ export default Component.extend({ } this.send('refresh'); }) - .catch(() => { - // handle promise rejection - error will be shown by message-error component + .catch((e) => { + this.set('errors', e.errors); }) .finally(() => { this.set('loading', false); diff --git a/ui/app/components/mount-backend-form.js b/ui/app/components/mount-backend-form.js index 2433af9bc..0493003d9 100644 --- a/ui/app/components/mount-backend-form.js +++ b/ui/app/components/mount-backend-form.js @@ -113,8 +113,39 @@ export default Component.extend({ } } - if (!capabilities.get('canUpdate')) { - // if there is no sys/mount issue then error is config endpoint. + let changedAttrKeys = Object.keys(mountModel.changedAttributes()); + const updatesConfig = + mountModel.isV2KV && + (changedAttrKeys.includes('casRequired') || + changedAttrKeys.includes('deleteVersionAfter') || + changedAttrKeys.includes('maxVersions')); + + try { + yield mountModel.save(); + } catch (err) { + if (err.httpStatus === 403) { + this.mountIssue = true; + this.set('isFormInvalid', this.mountIssue); + this.flashMessages.danger( + 'You do not have access to the sys/mounts endpoint. The secret engine was not mounted.' + ); + return; + } + if (err.errors) { + let errors = err.errors.map((e) => { + if (typeof e === 'object') return e.title || e.message || JSON.stringify(e); + return e; + }); + this.set('errors', errors); + } else if (err.message) { + this.set('errorMessage', err.message); + } else { + this.set('errorMessage', 'An error occurred, check the vault logs.'); + } + return; + } + if (updatesConfig && !capabilities.get('canUpdate')) { + // config error is not thrown from secret-engine adapter, so handling here this.flashMessages.warning( 'You do not have access to the config endpoint. The secret engine was mounted, but the configuration settings were not saved.' ); @@ -125,20 +156,6 @@ export default Component.extend({ 0, ]; } - try { - yield mountModel.save(); - } catch (err) { - if (err.message === 'mountIssue') { - this.mountIssue = true; - this.set('isFormInvalid', this.mountIssue); - this.flashMessages.danger( - 'You do not have access to the sys/mounts endpoint. The secret engine was not mounted.' - ); - return; - } - this.set('errorMessage', 'This mount path already exist.'); - return; - } let mountType = this.mountType; mountType = mountType === 'secret' ? `${mountType}s engine` : `${mountType} method`; this.flashMessages.success(`Successfully mounted the ${type} ${mountType} at ${path}.`); diff --git a/ui/app/mixins/policy-edit-controller.js b/ui/app/mixins/policy-edit-controller.js index 85fefea65..4361bfe43 100644 --- a/ui/app/mixins/policy-edit-controller.js +++ b/ui/app/mixins/policy-edit-controller.js @@ -36,9 +36,8 @@ export default Mixin.create({ } return this.transitionToRoute('vault.cluster.policy.show', m.get('policyType'), m.get('name')); }) - .catch(() => { - // do nothing here... - // message-error component will show errors + .catch((e) => { + model.set('errors', e.errors); }); }, diff --git a/ui/app/models/policy.js b/ui/app/models/policy.js index 1cb15a30f..af8c3d8a4 100644 --- a/ui/app/models/policy.js +++ b/ui/app/models/policy.js @@ -4,12 +4,12 @@ import { computed } from '@ember/object'; import lazyCapabilities, { apiPath } from 'vault/macros/lazy-capabilities'; export default Model.extend({ + errors: attr('array'), name: attr('string'), policy: attr('string'), policyType: computed('constructor.modelName', function () { return this.constructor.modelName.split('/')[1]; }), - updatePath: lazyCapabilities(apiPath`sys/policies/${'policyType'}/${'id'}`, 'id', 'policyType'), canDelete: alias('updatePath.canDelete'), canEdit: alias('updatePath.canUpdate'), diff --git a/ui/app/templates/components/config-pki-ca.hbs b/ui/app/templates/components/config-pki-ca.hbs index b56a06023..3bf28bfb2 100644 --- a/ui/app/templates/components/config-pki-ca.hbs +++ b/ui/app/templates/components/config-pki-ca.hbs @@ -150,7 +150,7 @@ {{else}}

Sign intermediate

- +
diff --git a/ui/app/templates/components/config-pki.hbs b/ui/app/templates/components/config-pki.hbs index b537a6925..bb9dd12e7 100644 --- a/ui/app/templates/components/config-pki.hbs +++ b/ui/app/templates/components/config-pki.hbs @@ -14,7 +14,7 @@

{{/if}} - + {{#each (get this.config (concat this.section "Attrs")) as |attr|}} diff --git a/ui/app/templates/components/mount-backend-form.hbs b/ui/app/templates/components/mount-backend-form.hbs index 58a4c5eaa..0d9612ca5 100644 --- a/ui/app/templates/components/mount-backend-form.hbs +++ b/ui/app/templates/components/mount-backend-form.hbs @@ -21,7 +21,7 @@
- + {{#if this.showEnable}}
- +
diff --git a/ui/lib/core/addon/components/message-error.js b/ui/lib/core/addon/components/message-error.js index 9a29f158b..cdd7b1812 100644 --- a/ui/lib/core/addon/components/message-error.js +++ b/ui/lib/core/addon/components/message-error.js @@ -1,6 +1,6 @@ -import { computed } from '@ember/object'; -import Component from '@ember/component'; +import Component from '@glimmer/component'; import layout from '../templates/components/message-error'; +import { setComponentTemplate } from '@ember/component'; /** * @module MessageError @@ -11,48 +11,37 @@ import layout from '../templates/components/message-error'; * * ``` * - * @param model=null{DS.Model} - An Ember data model that will be used to bind error statest to the internal + * @param {object} [model=null] - An Ember data model that will be used to bind error states to the internal * `errors` property. - * @param errors=null{Array} - An array of error strings to show. - * @param errorMessage=null{String} - An Error string to display. + * @param {array} [errors=null] - An array of error strings to show. + * @param {string} [errorMessage=null] - An Error string to display. */ -export default Component.extend({ - layout, - model: null, - errors: null, - errorMessage: null, - displayErrors: computed( - 'errorMessage', - 'model.{isError,adapterError.message,adapterError.errors.@each}', - 'errors', - 'errors.[]', - function () { - const errorMessage = this.errorMessage; - const errors = this.errors; - const modelIsError = this.model?.isError; - if (errorMessage) { - return [errorMessage]; - } - - if (errors && errors.length > 0) { - return errors; - } - - if (modelIsError) { - if (!this.model.adapterError) { - return; - } - if (this.model.adapterError.errors.length > 0) { - return this.model.adapterError.errors.map((e) => { - if (typeof e === 'object') return e.title || e.message || JSON.stringify(e); - return e; - }); - } - return [this.model.adapterError.message]; - } - - return 'no error'; +class MessageError extends Component { + get displayErrors() { + let { errorMessage, errors, model } = this.args; + if (errorMessage) { + return [errorMessage]; } - ), -}); + + if (errors && errors.length > 0) { + return errors; + } + + if (model?.isError) { + let adapterError = model?.adapterError; + if (!adapterError) { + return null; + } + if (adapterError.errors.length > 0) { + return adapterError.errors.map((e) => { + if (typeof e === 'object') return e.title || e.message || JSON.stringify(e); + return e; + }); + } + return [adapterError.message]; + } + return null; + } +} +export default setComponentTemplate(layout, MessageError); diff --git a/ui/lib/core/addon/templates/components/message-error.hbs b/ui/lib/core/addon/templates/components/message-error.hbs index 6790d458a..af5c40866 100644 --- a/ui/lib/core/addon/templates/components/message-error.hbs +++ b/ui/lib/core/addon/templates/components/message-error.hbs @@ -1,5 +1,5 @@ -{{#if this.displayErrors.length}} +{{#if this.displayErrors}} {{#each this.displayErrors as |error|}} - + {{/each}} {{/if}} \ No newline at end of file diff --git a/ui/tests/acceptance/client-current-test.js b/ui/tests/acceptance/client-current-test.js index 28cdb0b0c..00cc5bd2c 100644 --- a/ui/tests/acceptance/client-current-test.js +++ b/ui/tests/acceptance/client-current-test.js @@ -112,9 +112,8 @@ module('Acceptance | clients current', function (hooks) { // FILTER BY NAMESPACE await clickTrigger(); await searchSelect.options.objectAt(0).click(); - await waitUntil(() => { - return find('[data-test-horizontal-bar-chart]'); - }); + await settled(); + await waitUntil(() => find('[data-test-horizontal-bar-chart]')); assert.dom('[data-test-stat-text="total-clients"] .stat-value').hasText('15'); assert.dom('[data-test-stat-text="entity-clients"] .stat-value').hasText('5'); assert.dom('[data-test-stat-text="non-entity-clients"] .stat-value').hasText('10'); @@ -145,18 +144,14 @@ module('Acceptance | clients current', function (hooks) { // FILTER BY AUTH METHOD await clickTrigger(); await searchSelect.options.objectAt(0).click(); - await waitUntil(() => { - return find('#auth-method-search-select'); - }); + await waitUntil(() => find('#auth-method-search-select')); assert.dom('[data-test-stat-text="total-clients"] .stat-value').hasText('5'); assert.dom('[data-test-stat-text="entity-clients"] .stat-value').hasText('3'); assert.dom('[data-test-stat-text="non-entity-clients"] .stat-value').hasText('2'); assert.dom(SELECTORS.attributionBlock).doesNotExist('Does not show attribution block'); // Delete auth filter goes back to filtered only on namespace await click('#auth-method-search-select [data-test-selected-list-button="delete"]'); - await waitUntil(() => { - return find('[data-test-horizontal-bar-chart]'); - }); + await waitUntil(() => find('[data-test-horizontal-bar-chart]')); assert.dom('[data-test-stat-text="total-clients"] .stat-value').hasText('15'); assert.dom('[data-test-stat-text="entity-clients"] .stat-value').hasText('5'); assert.dom('[data-test-stat-text="non-entity-clients"] .stat-value').hasText('10'); diff --git a/ui/tests/acceptance/settings/configure-secret-backends/pki/section-urls-test.js b/ui/tests/acceptance/settings/configure-secret-backends/pki/section-urls-test.js index a67d8c902..b70d1b8d2 100644 --- a/ui/tests/acceptance/settings/configure-secret-backends/pki/section-urls-test.js +++ b/ui/tests/acceptance/settings/configure-secret-backends/pki/section-urls-test.js @@ -1,4 +1,4 @@ -import { currentRouteName, settled } from '@ember/test-helpers'; +import { currentRouteName, settled, find, waitUntil } from '@ember/test-helpers'; import { module, test } from 'qunit'; import { setupApplicationTest } from 'ember-qunit'; import page from 'vault/tests/pages/settings/configure-secret-backends/pki/section'; @@ -22,8 +22,7 @@ module('Acceptance | settings/configure/secrets/pki/urls', function (hooks) { await page.form.fields.objectAt(0).textarea('foo').change(); await page.form.submit(); - await settled(); - + await waitUntil(() => find('[data-test-error]')); assert.ok(page.form.hasError, 'shows error on invalid input'); await page.form.fields.objectAt(0).textarea('foo.example.com').change(); diff --git a/ui/tests/acceptance/settings/mount-secret-backend-test.js b/ui/tests/acceptance/settings/mount-secret-backend-test.js index d73eaf746..2f3cbfe07 100644 --- a/ui/tests/acceptance/settings/mount-secret-backend-test.js +++ b/ui/tests/acceptance/settings/mount-secret-backend-test.js @@ -89,7 +89,7 @@ module('Acceptance | settings/mount-secret-backend', function (hooks) { await page.enableEngine(); await page.selectType('kv'); await page.next().path(path).submit(); - assert.dom('.alert-banner-message-body').hasText('This mount path already exist.'); + assert.dom('.alert-banner-message-body').containsText(`path is already in use at ${path}`); assert.equal(currentRouteName(), 'vault.cluster.settings.mount-secret-backend'); await page.secretList(); diff --git a/ui/tests/integration/components/auth-form-test.js b/ui/tests/integration/components/auth-form-test.js index d414fa089..ba34ba09d 100644 --- a/ui/tests/integration/components/auth-form-test.js +++ b/ui/tests/integration/components/auth-form-test.js @@ -61,7 +61,7 @@ module('Integration | Component | auth form', function (hooks) { this.set('cluster', EmberObject.create({ standby: true })); this.set('selectedAuth', 'token'); await render(hbs`{{auth-form cluster=cluster selectedAuth=selectedAuth}}`); - assert.equal(component.errorText, ''); + assert.equal(component.errorMessagePresent, false); component.login(); // because this is an ember-concurrency backed service, // we have to manually force settling the run queue diff --git a/ui/tests/integration/components/ttl-picker-test.js b/ui/tests/integration/components/ttl-picker-test.js index 3999aae44..1142c0fc9 100644 --- a/ui/tests/integration/components/ttl-picker-test.js +++ b/ui/tests/integration/components/ttl-picker-test.js @@ -17,11 +17,10 @@ module('Integration | Component | ttl picker', function (hooks) { let callCount = this.changeSpy.callCount; await fillIn('[data-test-ttl-value]', 'foo'); - assert.equal(this.changeSpy.callCount, callCount, "it did't call onChange again"); + assert.equal(this.changeSpy.callCount, callCount, "it didn't call onChange again"); assert.dom('[data-test-ttl-error]').includesText('Error', 'renders the error box'); - await fillIn('[data-test-ttl-value]', '33'); - assert.dom('[data-test-ttl-error]').doesNotIncludeText('Error', 'removes the error box'); + assert.dom('[data-test-ttl-error]').doesNotExist('removes the error box'); }); test('it shows 30s for invalid duration initialValue input', async function (assert) { diff --git a/ui/tests/pages/components/auth-form.js b/ui/tests/pages/components/auth-form.js index 64c1de34e..4c10de515 100644 --- a/ui/tests/pages/components/auth-form.js +++ b/ui/tests/pages/components/auth-form.js @@ -1,4 +1,4 @@ -import { collection, clickable, fillable, text, value } from 'ember-cli-page-object'; +import { collection, clickable, fillable, text, value, isPresent } from 'ember-cli-page-object'; export default { tabs: collection('[data-test-auth-method]', { @@ -11,6 +11,7 @@ export default { tokenValue: value('[data-test-token]'), password: fillable('[data-test-password]'), errorText: text('[data-test-auth-error]'), + errorMessagePresent: isPresent('[data-test-auth-error]'), descriptionText: text('[data-test-description]'), login: clickable('[data-test-auth-submit]'), };