From 3163309130865e5ba0e048c56f693af9f2c67e73 Mon Sep 17 00:00:00 2001 From: claire bontempo <68122737+hellobontempo@users.noreply.github.com> Date: Tue, 13 Sep 2022 15:11:08 -0700 Subject: [PATCH] UI: Fix KV engine deleting latest version instead of specified version depending on policy (#17124) * update modal copy to clarify when a user is unable to delete a specific version * add tests * cleanup tests, move console commands into helper function * cleanup hbs * add changelog --- changelog/17124.txt | 2 + ui/app/components/secret-delete-menu.js | 2 +- .../components/secret-delete-menu.hbs | 21 +- .../components/secret-version-menu.hbs | 7 +- .../secrets/backend/kv/secret-test.js | 257 ++++++++++-------- 5 files changed, 167 insertions(+), 122 deletions(-) create mode 100644 changelog/17124.txt diff --git a/changelog/17124.txt b/changelog/17124.txt new file mode 100644 index 000000000..ef4f116d9 --- /dev/null +++ b/changelog/17124.txt @@ -0,0 +1,2 @@ +```release-note:bug +ui: Fix kv deleting the latest version when not allowed to soft delete (and delete a specific version of a secret) \ No newline at end of file diff --git a/ui/app/components/secret-delete-menu.js b/ui/app/components/secret-delete-menu.js index cc9ae6bf9..ab8bd5754 100644 --- a/ui/app/components/secret-delete-menu.js +++ b/ui/app/components/secret-delete-menu.js @@ -129,7 +129,7 @@ export default class SecretDeleteMenu extends Component { }); } else { // if they do not have read access on the metadata endpoint we need to pull the version from modelForData so they can perform delete and undelete operations - // only perform if no access to metatdata otherwise it will only delete latest version for any deleteType === delete + // only perform if no access to metadata otherwise it will only delete latest version for any deleteType === delete let currentVersionForNoReadMetadata; if (!this.args.canReadSecretMetadata) { currentVersionForNoReadMetadata = this.args.modelForData?.version; diff --git a/ui/app/templates/components/secret-delete-menu.hbs b/ui/app/templates/components/secret-delete-menu.hbs index d474ce081..11b1fda7f 100644 --- a/ui/app/templates/components/secret-delete-menu.hbs +++ b/ui/app/templates/components/secret-delete-menu.hbs @@ -57,12 +57,21 @@ @onChange={{fn (mut this.deleteType)}} />
- -

- This deletes Version - {{@modelForData.version}} - of the secret. It can be un-deleted later. -

+ {{#if this.canSoftDeleteSecretData}} + +

+ This deletes Version + {{@modelForData.version}} + of the secret. It can be un-deleted later. +

+ {{else}} + +

+ Your policy does not allow deleting a specific version of this secret. This will delete the + latest version + of this secret. It can be un-deleted later. +

+ {{/if}}
{{/if}} diff --git a/ui/app/templates/components/secret-version-menu.hbs b/ui/app/templates/components/secret-version-menu.hbs index 63aeb7732..a9509b904 100644 --- a/ui/app/templates/components/secret-version-menu.hbs +++ b/ui/app/templates/components/secret-version-menu.hbs @@ -16,7 +16,12 @@ {{/if}} {{#each (reverse @model.versions) as |secretVersion|}}
  • - + Version {{secretVersion.version}} {{#if diff --git a/ui/tests/acceptance/secrets/backend/kv/secret-test.js b/ui/tests/acceptance/secrets/backend/kv/secret-test.js index c983db4ab..767d1fafb 100644 --- a/ui/tests/acceptance/secrets/backend/kv/secret-test.js +++ b/ui/tests/acceptance/secrets/backend/kv/secret-test.js @@ -23,13 +23,13 @@ import consoleClass from 'vault/tests/pages/components/console/ui-panel'; const consoleComponent = create(consoleClass); -let writeSecret = async function (backend, path, key, val) { +const writeSecret = async function (backend, path, key, val) { await listPage.visitRoot({ backend }); await listPage.create(); return editPage.createSecret(path, key, val); }; -let deleteEngine = async function (enginePath, assert) { +const deleteEngine = async function (enginePath, assert) { await logout.visit(); await authPage.login(); await consoleComponent.runCommands([`delete sys/mounts/${enginePath}`]); @@ -41,7 +41,21 @@ let deleteEngine = async function (enginePath, assert) { ); }; -module('Acceptance | secrets/secret/create', function (hooks) { +const mountEngineGeneratePolicyToken = async (enginePath, secretPath, policy, version = 2) => { + await consoleComponent.runCommands([ + // delete any kv previously written here so that tests can be re-run + `delete ${enginePath}/metadata/${secretPath}`, + // delete any previous mount with same name + `delete sys/mounts/${enginePath}`, + // mount engine and generate policy + `write sys/mounts/${enginePath} type=kv options=version=${version}`, + `write sys/policies/acl/kv-v2-test-policy policy=${btoa(policy)}`, + 'write -field=client_token auth/token/create policies=kv-v2-test-policy', + ]); + return consoleComponent.lastLogOutput; +}; + +module('Acceptance | secrets/secret/create, read, delete', function (hooks) { setupApplicationTest(hooks); hooks.beforeEach(async function () { @@ -72,8 +86,8 @@ module('Acceptance | secrets/secret/create', function (hooks) { }); test('it can create a secret when check-and-set is required', async function (assert) { - let enginePath = `kv-${new Date().getTime()}`; - let secretPath = 'foo/bar'; + const enginePath = `kv-${new Date().getTime()}`; + const secretPath = 'foo/bar'; await mountSecrets.visit(); await mountSecrets.enable('kv', enginePath); await consoleComponent.runCommands(`write ${enginePath}/config cas_required=true`); @@ -83,8 +97,8 @@ module('Acceptance | secrets/secret/create', function (hooks) { }); test('it can create a secret with a non default max version and add metadata', async function (assert) { - let enginePath = `kv-${new Date().getTime()}`; - let secretPath = 'maxVersions'; + const enginePath = `kv-${new Date().getTime()}`; + const secretPath = 'maxVersions'; let maxVersions = 101; await mountSecrets.visit(); await mountSecrets.enable('kv', enginePath); @@ -119,8 +133,8 @@ module('Acceptance | secrets/secret/create', function (hooks) { }); test('it can handle validation on custom metadata', async function (assert) { - let enginePath = `kv-${new Date().getTime()}`; - let secretPath = 'customMetadataValidations'; + const enginePath = `kv-${new Date().getTime()}`; + const secretPath = 'customMetadataValidations'; await mountSecrets.visit(); await mountSecrets.enable('kv', enginePath); @@ -146,7 +160,7 @@ module('Acceptance | secrets/secret/create', function (hooks) { }); test('it can mount a KV 2 secret engine with config metadata', async function (assert) { - let enginePath = `kv-${new Date().getTime()}`; + const enginePath = `kv-${new Date().getTime()}`; let maxVersion = 101; await mountSecrets.visit(); await click('[data-test-mount-type="kv"]'); @@ -184,8 +198,8 @@ module('Acceptance | secrets/secret/create', function (hooks) { }); test('it can create a secret and metadata can be created and edited', async function (assert) { - let enginePath = `kv-${new Date().getTime()}`; - let secretPath = 'metadata'; + const enginePath = `kv-${new Date().getTime()}`; + const secretPath = 'metadata'; let maxVersions = 101; await mountSecrets.visit(); await mountSecrets.enable('kv', enginePath); @@ -209,8 +223,8 @@ module('Acceptance | secrets/secret/create', function (hooks) { }); test('it disables save when validation errors occur', async function (assert) { - let enginePath = `kv-${new Date().getTime()}`; - let secretPath = 'not-duplicate'; + const enginePath = `kv-${new Date().getTime()}`; + const secretPath = 'not-duplicate'; await mountSecrets.visit(); await mountSecrets.enable('kv', enginePath); await settled(); @@ -239,8 +253,8 @@ module('Acceptance | secrets/secret/create', function (hooks) { }); test('it navigates to version history and to a specific version', async function (assert) { - let enginePath = `kv-${new Date().getTime()}`; - let secretPath = `specific-version`; + const enginePath = `kv-${new Date().getTime()}`; + const secretPath = `specific-version`; let today = new Date(); let month = today.toString().split(' ')[1]; await mountSecrets.visit(); @@ -273,8 +287,8 @@ module('Acceptance | secrets/secret/create', function (hooks) { }); test('version 1 performs the correct capabilities lookup and does not show metadata tab', async function (assert) { - let enginePath = `kv-${new Date().getTime()}`; - let secretPath = 'foo/bar'; + const enginePath = `kv-${new Date().getTime()}`; + const secretPath = 'foo/bar'; // mount version 1 engine await mountSecrets.visit(); await mountSecrets.selectType('kv'); @@ -289,8 +303,8 @@ module('Acceptance | secrets/secret/create', function (hooks) { // 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'; + const enginePath = `kv-${new Date().getTime()}`; + const secretPath = '1/2/3/4'; // mount version 1 engine await mountSecrets.visit(); await mountSecrets.selectType('kv'); @@ -340,8 +354,8 @@ module('Acceptance | secrets/secret/create', function (hooks) { }); test('first level secrets redirect properly upon deletion', async function (assert) { - let enginePath = `kv-${new Date().getTime()}`; - let secretPath = 'test'; + const enginePath = `kv-${new Date().getTime()}`; + const secretPath = 'test'; // mount version 1 engine await mountSecrets.visit(); await mountSecrets.selectType('kv'); @@ -424,7 +438,7 @@ module('Acceptance | secrets/secret/create', function (hooks) { '_', ].map((char) => `${char}some`); assert.expect(paths.length * 2); - let secretPath = '2'; + const secretPath = '2'; let commands = paths.map((path) => `write '${backend}/${path}/${secretPath}' 3=4`); await consoleComponent.runCommands(['write sys/mounts/kv type=kv', ...commands]); for (let path of paths) { @@ -440,8 +454,8 @@ module('Acceptance | secrets/secret/create', function (hooks) { }); test('create secret with space shows version data and shows space warning', async function (assert) { - let enginePath = `kv-${new Date().getTime()}`; - let secretPath = 'space space'; + const enginePath = `kv-${new Date().getTime()}`; + const secretPath = 'space space'; // mount version 2 await mountSecrets.visit(); await mountSecrets.selectType('kv'); @@ -510,8 +524,8 @@ module('Acceptance | secrets/secret/create', function (hooks) { // All policy tests below this line test('version 2 with restricted policy still allows creation and does not show metadata tab', async function (assert) { - let enginePath = 'dont-show-metadata-tab'; - let secretPath = 'dont-show-metadata-tab-secret-path'; + const enginePath = 'dont-show-metadata-tab'; + const secretPath = 'dont-show-metadata-tab-secret-path'; const V2_POLICY = ` path "${enginePath}/metadata/*" { capabilities = ["list"] @@ -520,15 +534,7 @@ module('Acceptance | secrets/secret/create', function (hooks) { capabilities = ["create", "read", "update"] } `; - await consoleComponent.runCommands([ - `write sys/mounts/${enginePath} type=kv options=version=2`, - `write sys/policies/acl/kv-v2-degrade policy=${btoa(V2_POLICY)}`, - // delete any kv previously written here so that tests can be re-run - `delete ${enginePath}/metadata/${secretPath}`, - 'write -field=client_token auth/token/create policies=kv-v2-degrade', - ]); - - let userToken = consoleComponent.lastLogOutput; + const userToken = await mountEngineGeneratePolicyToken(enginePath, secretPath, V2_POLICY); await logout.visit(); await authPage.login(userToken); @@ -541,20 +547,15 @@ module('Acceptance | secrets/secret/create', function (hooks) { test('version 2 with no access to data but access to metadata shows metadata tab', async function (assert) { assert.expect(5); - let enginePath = 'kv-metadata-access-only'; - let secretPath = 'nested/kv-metadata-access-only-secret-name'; + const enginePath = 'kv-metadata-access-only'; + const secretPath = 'nested/kv-metadata-access-only-secret-name'; const V2_POLICY = ` path "${enginePath}/metadata/nested/*" { capabilities = ["read", "update"] } `; - await consoleComponent.runCommands([ - `write sys/mounts/${enginePath} type=kv options=version=2`, - `write sys/policies/acl/kv-v2-degrade policy=${btoa(V2_POLICY)}`, - 'write -field=client_token auth/token/create policies=kv-v2-degrade', - ]); - let userToken = consoleComponent.lastLogOutput; + const userToken = await mountEngineGeneratePolicyToken(enginePath, secretPath, V2_POLICY); await writeSecret(enginePath, secretPath, 'foo', 'bar'); await logout.visit(); await authPage.login(userToken); @@ -571,9 +572,9 @@ module('Acceptance | secrets/secret/create', function (hooks) { }); test('version 2: with metadata no read or list but with delete access and full access to the data endpoint', async function (assert) { - let enginePath = 'no-metadata-read'; - let secretPath = 'no-metadata-read-secret-name'; - let V2_POLICY_NO_LIST = ` + const enginePath = 'no-metadata-read'; + const secretPath = 'no-metadata-read-secret-name'; + const V2_POLICY_NO_LIST = ` path "${enginePath}/metadata/*" { capabilities = ["update","delete"] } @@ -581,19 +582,7 @@ module('Acceptance | secrets/secret/create', function (hooks) { capabilities = ["create", "read", "update", "delete"] } `; - await consoleComponent.runCommands([ - // delete any kv previously written here so that tests can be re-run - `delete ${enginePath}/metadata/${secretPath}`, - // delete any previous mount with same name - `delete sys/mounts/${enginePath}`, - `write sys/mounts/${enginePath} type=kv options=version=2`, - `write sys/policies/acl/metadata-no-read policy=${btoa(V2_POLICY_NO_LIST)}`, - 'write -field=client_token auth/token/create policies=metadata-no-read', - ]); - - await settled(); - let userToken2 = consoleComponent.lastLogOutput; - await settled(); + const userToken = await mountEngineGeneratePolicyToken(enginePath, secretPath, V2_POLICY_NO_LIST); await listPage.visitRoot({ backend: enginePath }); // confirm they see an empty state and not the get-credentials card await assert.dom('[data-test-empty-state-title]').hasText('No secrets in this backend'); @@ -604,7 +593,7 @@ module('Acceptance | secrets/secret/create', function (hooks) { await settled(); await logout.visit(); await settled(); - await authPage.login(userToken2); + await authPage.login(userToken); await settled(); // test if metadata tab there with no read access message and no ability to edit. await click(`[data-test-auth-backend-link=${enginePath}]`); @@ -664,8 +653,8 @@ module('Acceptance | secrets/secret/create', function (hooks) { // KV delete operations testing test('version 2 with policy with destroy capabilities shows modal', async function (assert) { - let enginePath = 'kv-v2-destroy-capabilities'; - let secretPath = 'kv-v2-destroy-capabilities-secret-path'; + const enginePath = 'kv-v2-destroy-capabilities'; + const secretPath = 'kv-v2-destroy-capabilities-secret-path'; const V2_POLICY = ` path "${enginePath}/destroy/*" { capabilities = ["update"] @@ -677,15 +666,7 @@ module('Acceptance | secrets/secret/create', function (hooks) { capabilities = ["create", "read", "update"] } `; - await consoleComponent.runCommands([ - `write sys/mounts/${enginePath} type=kv options=version=2`, - `write sys/policies/acl/kv-v2-degrade policy=${btoa(V2_POLICY)}`, - // delete any kv previously written here so that tests can be re-run - `delete ${enginePath}/metadata/${secretPath}`, - 'write -field=client_token auth/token/create policies=kv-v2-degrade', - ]); - - let userToken = consoleComponent.lastLogOutput; + const userToken = await mountEngineGeneratePolicyToken(enginePath, secretPath, V2_POLICY); await logout.visit(); await authPage.login(userToken); @@ -712,8 +693,8 @@ module('Acceptance | secrets/secret/create', function (hooks) { }); test('version 2 with policy with only delete option does not show modal and undelete is an option', async function (assert) { - let enginePath = 'kv-v2-only-delete'; - let secretPath = 'kv-v2-only-delete-secret-path'; + const enginePath = 'kv-v2-only-delete'; + const secretPath = 'kv-v2-only-delete-secret-path'; const V2_POLICY = ` path "${enginePath}/delete/*" { capabilities = ["update"] @@ -728,15 +709,7 @@ module('Acceptance | secrets/secret/create', function (hooks) { capabilities = ["create", "read"] } `; - await consoleComponent.runCommands([ - `write sys/mounts/${enginePath} type=kv options=version=2`, - `write sys/policies/acl/kv-v2-degrade policy=${btoa(V2_POLICY)}`, - // delete any kv previously written here so that tests can be re-run - `delete ${enginePath}/metadata/${secretPath}`, - 'write -field=client_token auth/token/create policies=kv-v2-degrade', - ]); - - let userToken = consoleComponent.lastLogOutput; + const userToken = await mountEngineGeneratePolicyToken(enginePath, secretPath, V2_POLICY); await logout.visit(); await authPage.login(userToken); await writeSecret(enginePath, secretPath, 'foo', 'bar'); @@ -753,9 +726,87 @@ module('Acceptance | secrets/secret/create', function (hooks) { assert.dom('[data-test-secret-undelete]').exists('undelete button shows'); }); + test('version 2: policy includes "delete" capability for secret path but does not have "update" to /delete endpoint', async function (assert) { + const enginePath = 'kv-v2-soft-delete-only'; + const secretPath = 'kv-v2-delete-capability-not-path'; + const policy = ` + path "${enginePath}/data/${secretPath}" { capabilities = ["create","read","update","delete","list"] } + path "${enginePath}/metadata/*" { capabilities = ["create","update","delete","list","read"] } + path "${enginePath}/undelete/*" { capabilities = ["update"] } + `; + const userToken = await mountEngineGeneratePolicyToken(enginePath, secretPath, policy); + await logout.visit(); + await authPage.login(userToken); + await writeSecret(enginePath, secretPath, 'foo', 'bar'); + // create multiple versions + await click('[data-test-secret-edit]'); + await editPage.editSecret('foo2', 'bar2'); + await click('[data-test-secret-edit]'); + await editPage.editSecret('foo3', 'bar3'); + // delete oldest version + await click('[data-test-popup-menu-trigger="version"]'); + await click('[data-test-version-dropdown-link="1"]'); + await click('[data-test-delete-open-modal]'); + assert + .dom('[data-test-type-select="delete-version"]') + .hasText('Delete latest version', 'modal reads that it will delete latest version'); + await click('input#delete-version'); + await click('[data-test-modal-delete]'); + await visit(`/vault/secrets/${enginePath}/show/${secretPath}?version=3`); + assert + .dom('[data-test-empty-state-title]') + .hasText( + 'Version 3 of this secret has been deleted', + 'empty state renders latest version has been deleted' + ); + await visit(`/vault/secrets/${enginePath}/show/${secretPath}?version=1`); + assert.dom('[data-test-delete-open-modal]').hasText('Delete', 'version 1 has not been deleted'); + }); + + test('version 2: policy has "update" to /delete endpoint but not "delete" capability for secret path', async function (assert) { + const enginePath = 'kv-v2-can-delete-version'; + const secretPath = 'kv-v2-delete-path-not-capability'; + const policy = ` + path "${enginePath}/data/${secretPath}" { capabilities = ["create","read","update","list"] } + path "${enginePath}/metadata/*" { capabilities = ["create","update","delete","list","read"] } + path "${enginePath}/undelete/*" { capabilities = ["update"] } + path "${enginePath}/delete/*" { capabilities = ["update"] } + `; + const userToken = await mountEngineGeneratePolicyToken(enginePath, secretPath, policy); + await logout.visit(); + await authPage.login(userToken); + await writeSecret(enginePath, secretPath, 'foo', 'bar'); + // create multiple versions + await click('[data-test-secret-edit]'); + await editPage.editSecret('foo2', 'bar2'); + await click('[data-test-secret-edit]'); + await editPage.editSecret('foo3', 'bar3'); + // delete oldest version + await click('[data-test-popup-menu-trigger="version"]'); + await click('[data-test-version-dropdown-link="1"]'); + await click('[data-test-delete-open-modal]'); + assert + .dom('[data-test-type-select="delete-version"]') + .hasText('Delete this version', 'delete option refers to "this" version'); + assert + .dom('[data-test-delete-modal="delete-version"]') + .hasTextContaining('Version 1', 'modal reads that it will delete version 1'); + await click('input#delete-version'); + await click('[data-test-modal-delete]'); + await visit(`/vault/secrets/${enginePath}/show/${secretPath}?version=3`); + assert.dom('[data-test-delete-open-modal]').hasText('Delete', 'latest version (3) has not been deleted'); + await visit(`/vault/secrets/${enginePath}/show/${secretPath}?version=1`); + assert + .dom('[data-test-empty-state-title]') + .hasText( + 'Version 1 of this secret has been deleted', + 'empty state renders oldest version (1) has been deleted' + ); + }); + test('version 2 with path forward slash will show delete button', async function (assert) { - let enginePath = 'kv-v2-forward-slash'; - let secretPath = 'forward/slash'; + const enginePath = 'kv-v2-forward-slash'; + const secretPath = 'forward/slash'; const V2_POLICY = ` path "${enginePath}/delete/${secretPath}" { capabilities = ["update"] @@ -767,15 +818,7 @@ module('Acceptance | secrets/secret/create', function (hooks) { capabilities = ["create", "read"] } `; - await consoleComponent.runCommands([ - `write sys/mounts/${enginePath} type=kv options=version=2`, - `write sys/policies/acl/kv-v2-degrade policy=${btoa(V2_POLICY)}`, - // delete any kv previously written here so that tests can be re-run - `delete ${enginePath}/metadata/${secretPath}`, - 'write -field=client_token auth/token/create policies=kv-v2-degrade', - ]); - - let userToken = consoleComponent.lastLogOutput; + const userToken = await mountEngineGeneratePolicyToken(enginePath, secretPath, V2_POLICY); await logout.visit(); await authPage.login(userToken); await writeSecret(enginePath, secretPath, 'foo', 'bar'); @@ -783,8 +826,8 @@ module('Acceptance | secrets/secret/create', function (hooks) { }); test('version 2 with engine with forward slash will show delete button', async function (assert) { - let enginePath = 'forward/slash'; - let secretPath = 'secret-name'; + const enginePath = 'forward/slash'; + const secretPath = 'secret-name'; const V2_POLICY = ` path "${enginePath}/delete/${secretPath}" { capabilities = ["update"] @@ -796,15 +839,7 @@ module('Acceptance | secrets/secret/create', function (hooks) { capabilities = ["create", "read"] } `; - await consoleComponent.runCommands([ - `write sys/mounts/${enginePath} type=kv options=version=2`, - `write sys/policies/acl/kv-v2-degrade policy=${btoa(V2_POLICY)}`, - // delete any kv previously written here so that tests can be re-run - `delete ${enginePath}/metadata/${secretPath}`, - 'write -field=client_token auth/token/create policies=kv-v2-degrade', - ]); - - let userToken = consoleComponent.lastLogOutput; + const userToken = await mountEngineGeneratePolicyToken(enginePath, secretPath, V2_POLICY); await logout.visit(); await authPage.login(userToken); await writeSecret(enginePath, secretPath, 'foo', 'bar'); @@ -812,7 +847,7 @@ module('Acceptance | secrets/secret/create', function (hooks) { }); // end of KV delete operation testing - let setupNoRead = async function (backend, canReadMeta = false) { + const setupNoRead = async function (backend, canReadMeta = false) { const V2_WRITE_ONLY_POLICY = ` path "${backend}/+/+" { capabilities = ["create", "update", "list"] @@ -839,6 +874,7 @@ module('Acceptance | secrets/secret/create', function (hooks) { } `; + let version = backend === 'kv-v2' ? 2 : 1; let policy; if (backend === 'kv-v2' && canReadMeta) { policy = V2_WRITE_WITH_META_READ_POLICY; @@ -847,15 +883,8 @@ module('Acceptance | secrets/secret/create', function (hooks) { } else if (backend === 'kv-v1') { policy = V1_WRITE_ONLY_POLICY; } - await consoleComponent.runCommands([ - // disable any kv previously enabled kv - `delete sys/mounts/${backend}`, - `write sys/mounts/${backend} type=kv options=version=${backend === 'kv-v2' ? 2 : 1}`, - `write sys/policies/acl/${backend} policy=${btoa(policy)}`, - `write -field=client_token auth/token/create policies=${backend}`, - ]); - return consoleComponent.lastLogOutput; + return await mountEngineGeneratePolicyToken(backend, 'nonexistent-secret', policy, version); }; test('write without read: version 2', async function (assert) { let backend = 'kv-v2';