UI secret navigation improvements (#5976)

* don't pass id when using createRecord

* add find nearest ancestor mixin

* re-throw the error if we've deleted something and encounter a 404

* use the with-nav-to-nearest-ancestor mixin

* add some comments

* add acceptance test to verify new behavior

* yield final transition in ec task

ui/app/components/secret-edit.js

@@ -5,6 +5,7 @@ import { computed, set } from '@ember/object';
 import { alias, or } from '@ember/object/computed';
 import { task, waitForEvent } from 'ember-concurrency';
 import FocusOnInsertMixin from 'vault/mixins/focus-on-insert';
+import WithNavToNearestAncestor from 'vault/mixins/with-nav-to-nearest-ancestor';
 import keys from 'vault/lib/keycodes';
 import KVObject from 'vault/lib/kv-object';
 import { maybeQueryRecord } from 'vault/macros/maybe-query-record';
@@ -13,7 +14,7 @@ const LIST_ROUTE = 'vault.cluster.secrets.backend.list';
 const LIST_ROOT_ROUTE = 'vault.cluster.secrets.backend.list-root';
 const SHOW_ROUTE = 'vault.cluster.secrets.backend.show';
 
-export default Component.extend(FocusOnInsertMixin, {
+export default Component.extend(FocusOnInsertMixin, WithNavToNearestAncestor, {
   wizard: service(),
   router: service(),
   store: service(),
@@ -163,7 +164,7 @@ export default Component.extend(FocusOnInsertMixin, {
   }),
 
   transitionToRoute() {
-    this.router.transitionTo(...arguments);
+    return this.router.transitionTo(...arguments);
   },
 
   onEscape(e) {
@@ -304,8 +305,9 @@ export default Component.extend(FocusOnInsertMixin, {
     },
 
     deleteKey() {
+      let { id } = this.model;
       this.model.destroyRecord().then(() => {
-        this.transitionToRoute(LIST_ROOT_ROUTE);
+        this.navToNearestAncestor.perform(id);
       });
     },
 

ui/app/controllers/vault/cluster/secrets/backend/list.js

@@ -3,8 +3,9 @@ import { inject as service } from '@ember/service';
 import Controller from '@ember/controller';
 import utils from 'vault/lib/key-utils';
 import BackendCrumbMixin from 'vault/mixins/backend-crumb';
+import WithNavToNearestAncestor from 'vault/mixins/with-nav-to-nearest-ancestor';
 
-export default Controller.extend(BackendCrumbMixin, {
+export default Controller.extend(BackendCrumbMixin, WithNavToNearestAncestor, {
   flashMessages: service(),
   queryParams: ['page', 'pageFilter', 'tab'],
 
@@ -71,7 +72,7 @@ export default Controller.extend(BackendCrumbMixin, {
         this.get('flashMessages').success(`${name} was successfully deleted.`);
         this.send('reload');
         if (type === 'secret') {
-          return this.transitionToRoute('vault.cluster.secrets.backend.list-root');
+          this.navToNearestAncestor.perform(name);
         }
       });
     },

ui/app/mixins/key-mixin.js

@@ -1,6 +1,6 @@
 import { computed } from '@ember/object';
 import Mixin from '@ember/object/mixin';
-import utils from '../lib/key-utils';
+import utils from 'vault/lib/key-utils';
 
 export default Mixin.create({
   // what attribute has the path for the key

ui/app/mixins/with-nav-to-nearest-ancestor.js

@@ -0,0 +1,40 @@
+import Mixin from '@ember/object/mixin';
+import utils from 'vault/lib/key-utils';
+import { task } from 'ember-concurrency';
+
+// This mixin is currently used in a controller and a component, but we
+// don't see cancellation of the task as the while loop runs in either
+
+// Controller in Ember are singletons so there's no cancellation there
+// during the loop. For components, it might be expected that the task would
+// be cancelled when we transitioned to a new route and a rerender occured, but this is not
+// the case since we are catching the error. Since Ember's route transitions are lazy
+// and we're catching any 404s, the loop continues until the transtion succeeds, or exhausts
+// the ancestors array and transitions to the root
+export default Mixin.create({
+  navToNearestAncestor: task(function*(key) {
+    let ancestors = utils.ancestorKeysForKey(key);
+    let errored = false;
+    let nearest = ancestors.pop();
+    while (nearest) {
+      try {
+        let transition = this.transitionToRoute('vault.cluster.secrets.backend.list', nearest);
+        transition.data.isDeletion = true;
+        yield transition.promise;
+      } catch (e) {
+        // in the route error event handler, we're only throwing when it's a 404,
+        // other errors will be in the route and will not be caught, so the task will complete
+        errored = true;
+        nearest = ancestors.pop();
+      } finally {
+        if (!errored) {
+          nearest = null;
+          // eslint-disable-next-line
+          return;
+        }
+        errored = false;
+      }
+    }
+    yield this.transitionToRoute('vault.cluster.secrets.backend.list-root');
+  }),
+});

ui/app/routes/vault/cluster/secrets/backend/create-root.js

@@ -6,9 +6,9 @@ let secretModel = (store, backend, key) => {
   let backendModel = store.peekRecord('secret-engine', backend);
   let modelType = backendModel.get('modelTypeForKV');
   if (modelType !== 'secret-v2') {
-    return store.createRecord(modelType, {
-      id: key,
-    });
+    let model = store.createRecord(modelType);
+    model.set('id', key);
+    return model;
   }
   let secret = store.createRecord(modelType);
   secret.set('engine', backendModel);

ui/app/routes/vault/cluster/secrets/backend/list.js

@@ -148,15 +148,22 @@ export default Route.extend({
 
   actions: {
     error(error, transition) {
-      const { secret } = this.paramsFor(this.routeName);
-      const { backend } = this.paramsFor('vault.cluster.secrets.backend');
+      let { secret } = this.paramsFor(this.routeName);
+      let { backend } = this.paramsFor('vault.cluster.secrets.backend');
+      let is404 = error.httpStatus === 404;
+      let hasModel = this.controllerFor(this.routeName).get('hasModel');
 
+      // this will occur if we've deleted something,
+      // and navigate to its parent and the parent doesn't exist -
+      // this if often the case with nested keys in kv-like engines
+      if (transition.data.isDeletion && is404) {
+        throw error;
+      }
       set(error, 'secret', secret);
       set(error, 'isRoot', true);
       set(error, 'backend', backend);
-      const hasModel = this.controllerFor(this.routeName).get('hasModel');
       // only swallow the error if we have a previous model
-      if (hasModel && error.httpStatus === 404) {
+      if (hasModel && is404) {
         this.set('has404', true);
         transition.abort();
         return false;

ui/tests/acceptance/secrets/backend/kv/secret-test.js

@@ -1,4 +1,4 @@
-import { currentURL, currentRouteName } from '@ember/test-helpers';
+import { settled, currentURL, currentRouteName } from '@ember/test-helpers';
 import { create } from 'ember-cli-page-object';
 import { module, test } from 'qunit';
 import { setupApplicationTest } from 'ember-qunit';
@@ -79,6 +79,65 @@ module('Acceptance | secrets/secret/create', function(hooks) {
     assert.ok(showPage.editIsPresent, 'shows the edit button');
   });
 
+  // https://github.com/hashicorp/vault/issues/5960
+  test('version 1: nested paths creation maintains ability to navigate the tree', async function(assert) {
+    let enginePath = `kv-${new Date().getTime()}`;
+    let secretPath = '1/2/3/4';
+    // mount version 1 engine
+    await mountSecrets.visit();
+    await mountSecrets.selectType('kv');
+    await withFlash(
+      mountSecrets
+        .next()
+        .path(enginePath)
+        .version(1)
+        .submit()
+    );
+
+    await listPage.create();
+    await editPage.createSecret(secretPath, 'foo', 'bar');
+
+    // setup an ancestor for when we delete
+    await listPage.visitRoot({ backend: enginePath });
+    await listPage.secrets.filterBy('text', '1/')[0].click();
+    await listPage.create();
+    await editPage.createSecret('1/2', 'foo', 'bar');
+
+    // lol we have to do this because ember-cli-page-object doesn't like *'s in visitable
+    await listPage.visitRoot({ backend: enginePath });
+    await listPage.secrets.filterBy('text', '1/')[0].click();
+    await listPage.secrets.filterBy('text', '2/')[0].click();
+    await listPage.secrets.filterBy('text', '3/')[0].click();
+    await listPage.create();
+
+    await editPage.createSecret(secretPath + 'a', 'foo', 'bar');
+    await listPage.visitRoot({ backend: enginePath });
+    await listPage.secrets.filterBy('text', '1/')[0].click();
+    await listPage.secrets.filterBy('text', '2/')[0].click();
+    let secretLink = listPage.secrets.filterBy('text', '3/')[0];
+    assert.ok(secretLink, 'link to the 3/ branch displays properly');
+
+    await listPage.secrets.filterBy('text', '3/')[0].click();
+    await listPage.secrets[0].menuToggle();
+    await settled();
+    await listPage.delete();
+    await listPage.confirmDelete();
+    await settled();
+    assert.equal(currentRouteName(), 'vault.cluster.secrets.backend.list');
+    assert.equal(currentURL(), `/vault/secrets/${enginePath}/list/1/2/3/`, 'remains on the page');
+
+    await listPage.secrets[0].menuToggle();
+    await listPage.delete();
+    await listPage.confirmDelete();
+    await settled();
+    assert.equal(currentRouteName(), 'vault.cluster.secrets.backend.list');
+    assert.equal(
+      currentURL(),
+      `/vault/secrets/${enginePath}/list/1/`,
+      'navigates to the ancestor created earlier'
+    );
+  });
+
   test('it redirects to the path ending in / for list pages', async function(assert) {
     const path = `foo/bar/kv-path-${new Date().getTime()}`;
     await listPage.visitRoot({ backend: 'secret' });

ui/tests/pages/secrets/backend/list.js

@@ -1,4 +1,4 @@
-import { create, collection, visitable, clickable, isPresent } from 'ember-cli-page-object';
+import { create, collection, text, visitable, clickable, isPresent } from 'ember-cli-page-object';
 import { getter } from 'ember-cli-page-object/macros';
 
 export default create({
@@ -8,15 +8,21 @@ export default create({
   createIsPresent: isPresent('[data-test-secret-create]'),
   configure: clickable('[data-test-secret-backend-configure]'),
   configureIsPresent: isPresent('[data-test-secret-backend-configure]'),
-
   tabs: collection('[data-test-tab]'),
   secrets: collection('[data-test-secret-link]', {
     menuToggle: clickable('[data-test-popup-menu-trigger]'),
+    id: text(),
+    click: clickable(),
   }),
   menuItems: collection('.ember-basic-dropdown-content li', {
     testContainer: '#ember-testing',
   }),
-
+  delete: clickable('[data-test-confirm-action-trigger]', {
+    testContainer: '#ember-testing',
+  }),
+  confirmDelete: clickable('[data-test-confirm-button]', {
+    testContainer: '#ember-testing',
+  }),
   backendIsEmpty: getter(function() {
     return this.secrets.length === 0;
   }),