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
hashicorp · Mar 18, 2022 · 4df3a7c · 4df3a7c
1 parent 9796cf2
commit 4df3a7c
Show file tree

Hide file tree

Showing 19 changed files with 92 additions and 96 deletions.
diff --git a/changelog/14551.txt b/changelog/14551.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+ui: Fix issue where UI incorrectly handled API errors when mounting backends
+```
diff --git 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
@@ -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
@@ -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
@@ -113,32 +113,49 @@ export default Component.extend({
         }
       }
 
-      if (!capabilities.get('canUpdate')) {
-        // if there is no sys/mount issue then error is config endpoint.
-        this.flashMessages.warning(
-          'You do not have access to the config endpoint. The secret engine was mounted, but the configuration settings were not saved.'
-        );
-        // remove the config data from the model otherwise it will save it even if the network request failed.
-        [this.mountModel.maxVersions, this.mountModel.casRequired, this.mountModel.deleteVersionAfter] = [
-          0,
-          false,
-          0,
-        ];
-      }
+      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.message === 'mountIssue') {
+        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;
         }
-        this.set('errorMessage', 'This mount path already exist.');
+        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.'
+        );
+        // remove the config data from the model otherwise it will save it even if the network request failed.
+        [this.mountModel.maxVersions, this.mountModel.casRequired, this.mountModel.deleteVersionAfter] = [
+          0,
+          false,
+          0,
+        ];
+      }
       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
@@ -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
@@ -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
@@ -150,7 +150,7 @@
   {{else}}
     <h2 data-test-title class="title is-3">Sign intermediate</h2>
     <NamespaceReminder @mode="save" @noun="PKI change" />
-    <MessageError @model={{this.model}} />
+    <MessageError @model={{this.model}} @errors={{this.errors}} />
     <form {{action "saveCA" on="submit"}} data-test-sign-intermediate-form="true">
       <FormFieldGroupsLoop @model={{this.model}} @mode={{this.mode}} />
       <div class="field is-grouped box is-fullwidth is-bottomless">

diff --git a/ui/app/templates/components/config-pki.hbs b/ui/app/templates/components/config-pki.hbs
@@ -14,7 +14,7 @@
   </p>
 {{/if}}
 
-<MessageError @model={{this.config}} />
+<MessageError @model={{this.config}} @errors={{this.errors}} />
 <form {{action "save" this.section on="submit"}} class="box is-shadowless is-marginless is-fullwidth has-slim-padding">
   {{#each (get this.config (concat this.section "Attrs")) as |attr|}}
     <FormField @attr={{attr}} @model={{this.config}} @data-test-field={{attr.name}} />

diff --git a/ui/app/templates/components/mount-backend-form.hbs b/ui/app/templates/components/mount-backend-form.hbs
@@ -21,7 +21,7 @@
 <form {{action (perform this.mountBackend) on="submit"}}>
   <div class="box is-sideless is-fullwidth is-marginless">
     <NamespaceReminder @mode="enable" @noun={{if (eq this.mountType "auth") "Auth Method" "Secret Engine"}} />
-    <MessageError @model={{this.model}} @errorMessage={{this.errorMessage}} />
+    <MessageError @model={{this.model}} @errorMessage={{this.errorMessage}} @errors={{this.errors}} />
     {{#if this.showEnable}}
       <FormFieldGroups
         @model={{this.mountModel}}

diff --git a/ui/app/templates/vault/cluster/policies/create.hbs b/ui/app/templates/vault/cluster/policies/create.hbs
@@ -19,7 +19,7 @@
 
 <form {{action "savePolicy" this.model on="submit"}}>
   <div class="box is-bottomless is-fullwidth is-marginless">
-    <MessageError @model={{this.model}} />
+    <MessageError @model={{this.model}} @errors={{this.model.errors}} />
     <NamespaceReminder @mode="create" @noun="policy" />
     <div class="field">
       <label for="policy-name" class="is-label">Name</label>

diff --git 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';
  * <MessageError @model={{model}} />
  * ```
  *
- * @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];
-      }
+class MessageError extends Component {
+  get displayErrors() {
+    let { errorMessage, errors, model } = this.args;
+    if (errorMessage) {
+      return [errorMessage];
+    }
 
-      if (errors && errors.length > 0) {
-        return errors;
-      }
+    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];
+    if (model?.isError) {
+      let adapterError = model?.adapterError;
+      if (!adapterError) {
+        return null;
       }
-
-      return 'no error';
+      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
@@ -1,5 +1,5 @@
-{{#if this.displayErrors.length}}
+{{#if this.displayErrors}}
   {{#each this.displayErrors as |error|}}
-    <AlertBanner @type="danger" @message={{error}} data-test-error />
+    <AlertBanner @type="danger" @message={{error}} data-test-error ...attributes />
   {{/each}}
 {{/if}}
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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]'),
 };