From f59fc43a7504d7248db3aa57fc60ab1382a86ba5 Mon Sep 17 00:00:00 2001 From: Chelsea Shaw Date: Mon, 6 May 2024 14:15:15 -0500 Subject: [PATCH 1/6] Update error states on secret list template --- .../vault/cluster/secrets/backend/list.hbs | 119 +++++++++--------- 1 file changed, 61 insertions(+), 58 deletions(-) diff --git a/ui/app/templates/vault/cluster/secrets/backend/list.hbs b/ui/app/templates/vault/cluster/secrets/backend/list.hbs index 4f31df5457b4..be80abe79e7c 100644 --- a/ui/app/templates/vault/cluster/secrets/backend/list.hbs +++ b/ui/app/templates/vault/cluster/secrets/backend/list.hbs @@ -13,39 +13,37 @@ {{#let (options-for-backend this.backendType this.tab) as |options|}} {{#if (or this.model.meta.total (not this.isConfigurableTab))}} - {{#if this.model.meta.total}} - - - {{#if this.filterFocused}} - {{#if this.filterMatchesKey}} - {{#unless this.filterIsFolder}} -

- Enter - to view - {{this.filter}} -

- {{/unless}} - {{/if}} - {{#if this.firstPartialMatch}} + + + {{#if this.filterFocused}} + {{#if this.filterMatchesKey}} + {{#unless this.filterIsFolder}}

- Tab - to autocomplete + Enter + to view + {{this.filter}}

- {{/if}} + {{/unless}} {{/if}} - - {{/if}} + {{#if this.firstPartialMatch}} +

+ Tab + to autocomplete +

+ {{/if}} + {{/if}} + {{/let}} {{else}} -
- {{#if this.filterFocused}} - There are no - {{pluralize options.item}} - matching - {{this.filter}}, press - ENTER - to add one. - {{else}} - There are no - {{pluralize options.item}} - matching - {{this.filter}}. - {{/if}} -
- {{/each}} + {{! Empty state here means that a filter was applied on top of the results list }} + - + {{/each}} + {{#if this.model.length}} + {{! Only show pagination when there are results on the page }} + + {{/if}} {{else}} {{#if (eq this.baseKey.id "")}} {{#if (and options.firstStep (not this.tab))}} @@ -124,10 +117,20 @@ + > + + Back to root + + + {{else}} + + + Back to root + + {{/if}} {{/if}} {{/if}} From ed0ccad58f46dc59703706d6ae0349fd5619cf75 Mon Sep 17 00:00:00 2001 From: Chelsea Shaw Date: Mon, 6 May 2024 14:16:04 -0500 Subject: [PATCH 2/6] Remove usage of navToNearestAncestor mixin --- .../vault/cluster/secrets/backend/list.js | 8 +--- ui/app/mixins/with-nav-to-nearest-ancestor.js | 45 ------------------- .../vault/cluster/secrets/backend/list.js | 6 --- 3 files changed, 2 insertions(+), 57 deletions(-) delete mode 100644 ui/app/mixins/with-nav-to-nearest-ancestor.js diff --git a/ui/app/controllers/vault/cluster/secrets/backend/list.js b/ui/app/controllers/vault/cluster/secrets/backend/list.js index e65c09cc8e71..bb0d07079344 100644 --- a/ui/app/controllers/vault/cluster/secrets/backend/list.js +++ b/ui/app/controllers/vault/cluster/secrets/backend/list.js @@ -8,11 +8,10 @@ import { computed } from '@ember/object'; import { service } from '@ember/service'; import Controller from '@ember/controller'; import BackendCrumbMixin from 'vault/mixins/backend-crumb'; -import WithNavToNearestAncestor from 'vault/mixins/with-nav-to-nearest-ancestor'; import ListController from 'core/mixins/list-controller'; import { keyIsFolder } from 'core/utils/key-utils'; -export default Controller.extend(ListController, BackendCrumbMixin, WithNavToNearestAncestor, { +export default Controller.extend(ListController, BackendCrumbMixin, { flashMessages: service(), queryParams: ['page', 'pageFilter', 'tab'], @@ -52,16 +51,13 @@ export default Controller.extend(ListController, BackendCrumbMixin, WithNavToNea }); }, - delete(item, type) { + delete(item) { const name = item.id; item .destroyRecord() .then(() => { this.flashMessages.success(`${name} was successfully deleted.`); this.send('reload'); - if (type === 'secret') { - this.navToNearestAncestor.perform(name); - } }) .catch((e) => { const error = e.errors ? e.errors.join('. ') : e.message; diff --git a/ui/app/mixins/with-nav-to-nearest-ancestor.js b/ui/app/mixins/with-nav-to-nearest-ancestor.js deleted file mode 100644 index bc48036273a6..000000000000 --- a/ui/app/mixins/with-nav-to-nearest-ancestor.js +++ /dev/null @@ -1,45 +0,0 @@ -/** - * Copyright (c) HashiCorp, Inc. - * SPDX-License-Identifier: BUSL-1.1 - */ - -import Mixin from '@ember/object/mixin'; -import { task } from 'ember-concurrency'; -import { ancestorKeysForKey } from 'core/utils/key-utils'; - -// 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) { - const ancestors = ancestorKeysForKey(key); - let errored = false; - let nearest = ancestors.pop(); - while (nearest) { - try { - const 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'); - }), -}); diff --git a/ui/app/routes/vault/cluster/secrets/backend/list.js b/ui/app/routes/vault/cluster/secrets/backend/list.js index 16a36d232239..478858f110d2 100644 --- a/ui/app/routes/vault/cluster/secrets/backend/list.js +++ b/ui/app/routes/vault/cluster/secrets/backend/list.js @@ -189,12 +189,6 @@ export default Route.extend({ /* eslint-disable-next-line ember/no-controller-access-in-routes */ const hasModel = this.controllerFor(this.routeName).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); From 83f5ff23399201b06b9128f0da965f0b23df5e1b Mon Sep 17 00:00:00 2001 From: Chelsea Shaw Date: Mon, 6 May 2024 14:50:10 -0500 Subject: [PATCH 3/6] don't throw error on list when 404 --- .../vault/cluster/secrets/backend/list.js | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/ui/app/routes/vault/cluster/secrets/backend/list.js b/ui/app/routes/vault/cluster/secrets/backend/list.js index 478858f110d2..16dec5e90e7c 100644 --- a/ui/app/routes/vault/cluster/secrets/backend/list.js +++ b/ui/app/routes/vault/cluster/secrets/backend/list.js @@ -15,6 +15,20 @@ import { pathIsDirectory } from 'kv/utils/kv-breadcrumbs'; const SUPPORTED_BACKENDS = supportedSecretBackends(); +function getValidPage(pageParam) { + if (typeof pageParam === 'number') { + return pageParam; + } + if (typeof pageParam === 'string') { + try { + return parseInt(pageParam, 10) || 1; + } catch (e) { + return 1; + } + } + return 1; +} + export default Route.extend({ store: service(), templateName: 'vault/cluster/secrets/backend/list', @@ -119,7 +133,7 @@ export default Route.extend({ id: secret, backend, responsePath: 'data.keys', - page: params.page || 1, + page: getValidPage(params.page), pageFilter: params.pageFilter, }) .then((model) => { @@ -127,8 +141,7 @@ export default Route.extend({ return model; }) .catch((err) => { - // if we're at the root we don't want to throw - if (backendModel && err.httpStatus === 404 && secret === '') { + if (backendModel && err.httpStatus === 404) { return []; } else { // else we're throwing and dealing with this in the error action From 3fdffb10c33971d4dbf187878e019d168b906593 Mon Sep 17 00:00:00 2001 From: Chelsea Shaw Date: Mon, 6 May 2024 14:51:00 -0500 Subject: [PATCH 4/6] Update test with expected behavior --- .../secrets/backend/kv/secret-test.js | 62 ++++++++++--------- 1 file changed, 32 insertions(+), 30 deletions(-) diff --git a/ui/tests/acceptance/secrets/backend/kv/secret-test.js b/ui/tests/acceptance/secrets/backend/kv/secret-test.js index 34fdb277c354..5c0791cda018 100644 --- a/ui/tests/acceptance/secrets/backend/kv/secret-test.js +++ b/ui/tests/acceptance/secrets/backend/kv/secret-test.js @@ -19,6 +19,7 @@ import { writeSecret, writeVersionedSecret } from 'vault/tests/helpers/kv/kv-run import { runCmd } from 'vault/tests/helpers/commands'; import { PAGE } from 'vault/tests/helpers/kv/kv-selectors'; import codemirror from 'vault/tests/helpers/codemirror'; +import { GENERAL } from 'vault/tests/helpers/general-selectors'; const deleteEngine = async function (enginePath, assert) { await logout.visit(); @@ -157,31 +158,29 @@ module('Acceptance | secrets/secret/create, read, delete', function (hooks) { // https://github.com/hashicorp/vault/issues/5960 test('version 1: nested paths creation maintains ability to navigate the tree', async function (assert) { const enginePath = this.backend; - const secretPath = '1/2/3/4'; - 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(); - const secretLink = listPage.secrets.filterBy('text', '3/')[0]; - assert.ok(secretLink, 'link to the 3/ branch displays properly'); + // await visit('/vault/secrets'); + await runCmd([ + `write ${enginePath}/1/2 foo=bar`, + `write ${enginePath}/1/2/3/4 foo=bar`, + `write ${enginePath}/1/2/3/4a foo=bar`, + 'refresh', + ]); + await settled(); + // navigate to farthest leaf + await visit(`/vault/secrets/${enginePath}/list`); + assert.dom('[data-test-component="navigate-input"]').hasNoValue(); + assert.dom('[data-test-secret-link]').exists({ count: 1 }); + await click('[data-test-secret-link="1/"]'); + assert.dom('[data-test-component="navigate-input"]').hasValue('1/'); + assert.dom('[data-test-secret-link]').exists({ count: 2 }); + await click('[data-test-secret-link="1/2/"]'); + assert.dom('[data-test-component="navigate-input"]').hasValue('1/2/'); + assert.dom('[data-test-secret-link]').exists({ count: 1 }); + await click('[data-test-secret-link="1/2/3/"]'); + assert.dom('[data-test-component="navigate-input"]').hasValue('1/2/3/'); + assert.dom('[data-test-secret-link]').exists({ count: 2 }); - await listPage.secrets.filterBy('text', '3/')[0].click(); + // delete the items await listPage.secrets.objectAt(0).menuToggle(); await settled(); await listPage.delete(); @@ -190,17 +189,20 @@ module('Acceptance | secrets/secret/create, read, delete', function (hooks) { assert.strictEqual(currentRouteName(), 'vault.cluster.secrets.backend.list'); assert.strictEqual(currentURL(), `/vault/secrets/${enginePath}/list/1/2/3/`, 'remains on the page'); + assert.dom('[data-test-secret-link]').exists({ count: 1 }); await listPage.secrets.objectAt(0).menuToggle(); await listPage.delete(); await listPage.confirmDelete(); await settled(); - assert.strictEqual(currentRouteName(), 'vault.cluster.secrets.backend.list'); - assert.strictEqual( - currentURL(), - `/vault/secrets/${enginePath}/list/1/`, - 'navigates to the ancestor created earlier' - ); + assert.strictEqual(currentURL(), `/vault/secrets/${enginePath}/list/1/2/3/`, 'remains on the page'); + assert.dom(GENERAL.emptyStateTitle).hasText('No secrets under "1/2/3/".'); + await fillIn('[data-test-component="navigate-input"]', '1/2/'); + assert.dom(GENERAL.emptyStateTitle).hasText('No secrets under "1/2/".'); + await click('[data-test-list-root-link]'); + assert.strictEqual(currentURL(), `/vault/secrets/${enginePath}/list`); + assert.dom('[data-test-secret-link]').exists({ count: 1 }); }); + test('first level secrets redirect properly upon deletion', async function (assert) { const secretPath = 'test'; await listPage.create(); From e7882116a1c9ceb3da165398f4e8997b5adddf45 Mon Sep 17 00:00:00 2001 From: Chelsea Shaw Date: Mon, 6 May 2024 14:51:24 -0500 Subject: [PATCH 5/6] cleanup --- ui/tests/acceptance/secrets/backend/kv/secret-test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/ui/tests/acceptance/secrets/backend/kv/secret-test.js b/ui/tests/acceptance/secrets/backend/kv/secret-test.js index 5c0791cda018..ab224d0aa869 100644 --- a/ui/tests/acceptance/secrets/backend/kv/secret-test.js +++ b/ui/tests/acceptance/secrets/backend/kv/secret-test.js @@ -158,7 +158,6 @@ module('Acceptance | secrets/secret/create, read, delete', function (hooks) { // https://github.com/hashicorp/vault/issues/5960 test('version 1: nested paths creation maintains ability to navigate the tree', async function (assert) { const enginePath = this.backend; - // await visit('/vault/secrets'); await runCmd([ `write ${enginePath}/1/2 foo=bar`, `write ${enginePath}/1/2/3/4 foo=bar`, From 471b40d2b19b1d4d8f42dc6eb45732ae787cca03 Mon Sep 17 00:00:00 2001 From: Chelsea Shaw Date: Mon, 6 May 2024 15:51:09 -0500 Subject: [PATCH 6/6] Add changelog --- changelog/26845.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/26845.txt diff --git a/changelog/26845.txt b/changelog/26845.txt new file mode 100644 index 000000000000..2f19eaf8f28f --- /dev/null +++ b/changelog/26845.txt @@ -0,0 +1,3 @@ +```release-note:change +ui: deleting a nested secret will no longer redirect you to the nearest path segment +``` \ No newline at end of file