KV 2 Toolbar delete redesign #11530

Monkeychip · 2021-05-04T20:34:59Z

This PR redesigns the Delete functionality of the KV2 secret engine. Before we didn't cover all the situations for the various deletes based on permissions. Under this new design, we make sure to cover all the various combinations of deletes based on permissions as well as make it a little more intuitive on how to reach the delete actions and what they each do.

Different Delete options:

1. Delete latest version only.

path "secret/data/*" {
  capabilities = [ "delete"]
}

In this scenario we only show the delete dropdown option when the user is on the current version, it disappears if the user is on an older version.

2. Delete any version
In this scenario, we show the delete dropdown option for all versions.

path "secret/delete/*" {
 capabilities = ["update"]
}

3. Destroy version
Whenever a user has the destroy permissions we show a modal instead of the dropdown. The modal will then either show all three options or a mix of them based on permissions but it will always show a destroy option. In the example, the user also can delete any version so that option is also shown.

path "secret/destroy/*" {
 capabilities = ["update"]
}

4. Permanently remove all versions
This policy allows you to permanently remove all the versions, essentially a destroy on all versions.

path "secret/metadata/*" {
 capabilities = ["delete"]
}

This is the modal with all permissions.

5. Undelete only shows if you have the following permissions

path "secret/undelete/*" {
 capabilities = ["update"]
}

When the delete/destroy/undelete action occurs the page is refreshed. Below is a gif. I couldn't find a better solution for refreshing the model and closing the modal, but I'm very open to suggestions. However, as it currently works the functionality is clean.

Icons were also added to the version dropdown menu as well as the version page (where we removed deleted actions).

…o POST

Monkeychip · 2021-05-14T15:41:37Z

ui/app/adapters/secret-v2-version.js

-      }
-    );
+    // deleteType should be 'delete', 'destroy', 'undelete', 'delete-latest-version', 'destroy-version'
+    if ((!version && deleteType === 'delete') || deleteType === 'delete-latest-version') {


before we were not handling the delete-latest-version which made it impossible for some people with proper permissions to execute this action. They'd get a permissions denied, because we executing the delete any version action and not the latest version deletion.

We also weren't handling errors, so they would silently fail.

Monkeychip · 2021-05-14T15:42:20Z

ui/app/components/secret-delete-menu.js

@@ -0,0 +1,147 @@
+import Ember from 'ember';


moved all deletion actions into one glimmerized component.

Monkeychip · 2021-05-14T15:43:21Z

ui/app/components/secret-delete-menu.js

+
+  @tracked showDeleteModal = false;
+
+  @maybeQueryRecord(


TIL that if you have a macro (even a custom one like maybeQueryRecord, you just @'it and then put the name below it. This took me a while to figure out, so I thought it worth pointing out.

ui/app/components/secret-delete-menu.js

Monkeychip · 2021-05-14T15:44:15Z

ui/app/components/secret-version-menu.js

-import Component from '@ember/component';
-import { inject as service } from '@ember/service';
-import { alias, or } from '@ember/object/computed';
+import Component from '@glimmer/component';



secret-version-menu used to house all the deletion actions, now it only handles version things.

Monkeychip · 2021-05-14T15:44:43Z

ui/app/styles/components/masked-input.scss

@@ -66,7 +66,6 @@
    height: auto;
    line-height: 1rem;
    min-width: $spacing-l;


the icons were showing in front of the modal, we determined it was the z-index causing this.

Monkeychip · 2021-05-14T15:45:38Z

ui/app/templates/components/secret-edit.hbs

@@ -33,75 +33,15 @@
    </ToolbarFilters>
  {{/unless}}
  <ToolbarActions>
-    {{#if (and (eq @mode "show") this.isV2 (not @model.failedServerRead))}}


the ordering of toolbar-actions changed. Now deletion comes first and version dropdown is scooted to the far right.

🛴 😄

P.S. Boooooo github not putting comments inline in the Convo tab spoils my emoji fun 😄

hashishaw

Really great work so far, and good questions. I'll pull down the branch to play around with it too. 👏

ui/app/components/secret-edit.js

ui/app/templates/components/secret-delete-menu.hbs

johncowen

Hey @Monkeychip !

I took a quick look at this and noticed something that looked a little strange, but not massively consequential. Also, I checked it out and tried a different approach to closing the modal which 'seemed' to work, but I might have missed something due to lack of context. I tried to detail it in the comments.

Happy to hook up separately to chat through this if it helps though, feel free to give me a ping when you are around if that helps.

johncowen · 2021-05-17T07:52:04Z

ui/app/adapters/secret-v2-version.js

+      return this.ajax(this._url(backend, path, deleteType), 'POST', { data: { versions: [version] } })
+        .then(() => {
+          let model = store.peekRecord('secret-v2-version', id);
+          return model && model.rollbackAttributes() && model.reload();


I saw these came from the previous iteration of this, but these lines look a little strange to me, I'm not totally sure but is model.reload() ever called?

return model && model.rollbackAttributes() && model.reload();

If model is falsey (in this case undefined) just return undefined, else move on to: If the result of model.rollbackAttributes() is falsey (looking at the code rollbackAttributes always returns undefined) then return its result (undefined) else move on to: Return the result of model.reload which is the model wrapped in a promise, but if rollbackAttributes always returns undefined then we never get to this point?

As I said I'm not totally sure, and it would be weird that this method always returns undefined wrapped in a promise, maybe its not being used where ever it gets picked up?

I just ran this in web console:

({}) && (function(){})() && console.log('hi');

Which never logs 'hi'

Investigated and John is potentially right. However, in order to not cause an unknown bug, I added a comment here instead of a complete removal.

johncowen · 2021-05-17T07:55:19Z

ui/app/components/secret-delete-menu.js

+      }
+      let backend = context.args.model.backend;
+      let id = context.args.model.id;
+      let path = context.args.isV2 ? `${backend}/data/${id}` : `${backend}/${id}`;


I'm guessing this eventually ends up in an API call? Is this one of those places that might need its parameters URL encoding? Is id or backend every likely to have URL unsafe characters in it? Maybe it gets slash split and url encoded elsewhere tho?

(there are a few more of these further down also)

Great point. Let me check this out.

Note to self: it errors before it ever gets here if the user has entered an unsafe id or backend. Something to revisit.

I think it's worth urlEncoding here even though it currently fails upstream, so that in the future if/when the upstream thing is fixed we don't have unsafe strings here

johncowen · 2021-05-17T08:01:47Z

ui/app/templates/components/secret-edit.hbs

@@ -33,75 +33,15 @@
    </ToolbarFilters>
  {{/unless}}
  <ToolbarActions>
-    {{#if (and (eq @mode "show") this.isV2 (not @model.failedServerRead))}}


🛴 😄

P.S. Boooooo github not putting comments inline in the Convo tab spoils my emoji fun 😄

johncowen · 2021-05-17T10:18:09Z

ui/app/components/secret-delete-menu.js

+        .adapterFor('secret-v2-version')
+        .v2DeleteOperation(this.store, this.args.modelForData.id, deleteType)
+        .then(resp => {
+          if (Ember.testing) {


Ah so I'm guessing resp is where the undefined comes in from the comment I made above for the adapter bits?

resp is only ever set when its come in via the:

catch((e) => { return e })

...in the adapter?

correct. We only get a resp if it's an adapter error.

johncowen · 2021-05-17T10:35:22Z

ui/app/templates/components/secret-delete-menu.hbs

+      <button
+        type="button"
+        class="button has-text-danger"
+        {{on "click" (fn this.handleDelete this.deleteType)}}


Potential Modal Closing Solution Part 1:

Add a third 'success' function here:

{{on "click" (fn this.handleDelete this.deleteType (action (mut this.showDeleteModal) false))}}

ui/app/components/secret-delete-menu.js

johncowen · 2021-05-17T10:37:07Z

ui/app/components/secret-delete-menu.js

+            return;
+          }
+          if (!resp) {
+            location.reload();


Potential Modal Closing Solution Part 3:

Remove the location.reload() and use success() instead:

if(!resp) { success() }

@johncowen this is very helpful! Thank you. I'll start working through some of these solutions. Closing the modal was the whole reason I was forcing a reload of the page, but in this case I may not have to. Thanks again!

Awesome 🙌

hashishaw · 2021-05-17T20:23:54Z

ui/app/components/secret-delete-menu.js

+      if (!context.args || !context.args.modelForData || !context.args.modelForData.id) return;
+      let [backend, id] = JSON.parse(context.args.modelForData.id);
+      return {
+        id: `${backend}/delete/${id}`,


here too probably (re: encoding)

hashishaw

Great work on this!

initial setup, modify toolbar header

0cd6d3f

Monkeychip added the ui label May 4, 2021

Monkeychip added this to the 1.8 milestone May 4, 2021

Monkeychip added 4 commits May 5, 2021 09:57

footer buttons setup

9b02bd1

setup first delete version delete method

a94fefb

clean up

5733a64

handle destory all versions

f613b28

vercel bot deployed to Preview – vault-storybook May 5, 2021 17:42 View deployment

vercel bot temporarily deployed to Preview – vault May 5, 2021 17:42 Inactive

Monkeychip added 2 commits May 5, 2021 14:04

handle undelete

73ef1a1

conditional for modal and undelete

d66f584

vercel bot deployed to Preview – vault-storybook May 5, 2021 20:25 View deployment

vercel bot temporarily deployed to Preview – vault May 5, 2021 20:25 Inactive

Monkeychip added 2 commits May 5, 2021 14:34

remove delete from version area

10f5064

modelForData in permissions

bfdc688

vercel bot deployed to Preview – vault-storybook May 5, 2021 20:42 View deployment

vercel bot temporarily deployed to Preview – vault May 5, 2021 20:42 Inactive

Monkeychip added 2 commits May 7, 2021 10:12

setup for soft delete and modify adpater to allow DELETE in additon t…

86099cc

…o POST

dropdown for soft delete

772b22e

vercel bot temporarily deployed to Preview – vault May 7, 2021 19:02 Inactive

vercel bot deployed to Preview – vault-storybook May 7, 2021 19:02 View deployment

stuck

eef8d75

vercel bot deployed to Preview – vault-storybook May 7, 2021 19:59 View deployment

vercel bot temporarily deployed to Preview – vault May 7, 2021 19:59 Inactive

handle all soft deletes

28d6617

vercel bot temporarily deployed to Preview – vault May 10, 2021 18:50 Inactive

vercel bot deployed to Preview – vault-storybook May 10, 2021 18:50 View deployment

conditional for destroy all versions

7ac7d6a

vercel bot deployed to Preview – vault-storybook May 11, 2021 15:44 View deployment

vercel bot temporarily deployed to Preview – vault May 11, 2021 15:44 Inactive

add to test

0c912f8

vercel bot deployed to Preview – vault-storybook May 13, 2021 16:40 View deployment

vercel bot temporarily deployed to Preview – vault May 13, 2021 16:40 Inactive

amend test

c365da4

Monkeychip marked this pull request as ready for review May 14, 2021 15:34

Monkeychip commented May 14, 2021

View reviewed changes

ui/app/components/secret-delete-menu.js Show resolved Hide resolved

Monkeychip commented May 14, 2021

View reviewed changes

hashishaw reviewed May 14, 2021

View reviewed changes

ui/app/components/secret-edit.js Outdated Show resolved Hide resolved

ui/app/templates/components/secret-delete-menu.hbs Outdated Show resolved Hide resolved

ui/app/templates/components/secret-delete-menu.hbs Outdated Show resolved Hide resolved

johncowen reviewed May 17, 2021

View reviewed changes

address PR comments

49af0ef

vercel bot deployed to Preview – vault-storybook May 17, 2021 18:56 View deployment

vercel bot temporarily deployed to Preview – vault May 17, 2021 18:56 Inactive

whoopies

1a9c91b

vercel bot deployed to Preview – vault-storybook May 17, 2021 19:12 View deployment

vercel bot temporarily deployed to Preview – vault May 17, 2021 19:12 Inactive

hashishaw reviewed May 17, 2021

View reviewed changes

add urlEncoding

7f20aca

vercel bot deployed to Preview – vault-storybook May 19, 2021 15:50 View deployment

vercel bot temporarily deployed to Preview – vault May 19, 2021 15:50 Inactive

hashishaw approved these changes May 19, 2021

View reviewed changes

Monkeychip merged commit 00ac563 into master May 19, 2021

Monkeychip deleted the ui/kv-toolbar branch May 19, 2021 16:43

Monkeychip mentioned this pull request Sep 14, 2021

Bug fix: allow forward slash in paths for delete menu #12550

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KV 2 Toolbar delete redesign #11530

KV 2 Toolbar delete redesign #11530

Monkeychip commented May 4, 2021 •

edited

Loading

Monkeychip May 14, 2021 •

edited

Loading

Monkeychip May 14, 2021

Monkeychip May 14, 2021

Monkeychip May 14, 2021

Monkeychip May 14, 2021

Monkeychip May 14, 2021

Monkeychip May 14, 2021 •

edited

Loading

johncowen May 17, 2021 •

edited

Loading

hashishaw left a comment

johncowen left a comment

johncowen May 17, 2021

Monkeychip May 17, 2021 •

edited

Loading

johncowen May 17, 2021

Monkeychip May 17, 2021

Monkeychip May 17, 2021 •

edited

Loading

hashishaw May 17, 2021

johncowen May 17, 2021 •

edited

Loading

johncowen May 17, 2021

Monkeychip May 17, 2021

johncowen May 17, 2021 •

edited

Loading

johncowen May 17, 2021 •

edited

Loading

Monkeychip May 17, 2021

johncowen May 17, 2021

hashishaw May 17, 2021

hashishaw left a comment

KV 2 Toolbar delete redesign #11530

KV 2 Toolbar delete redesign #11530

Conversation

Monkeychip commented May 4, 2021 • edited Loading

Monkeychip May 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monkeychip May 14, 2021 • edited Loading

Choose a reason for hiding this comment

johncowen May 17, 2021 • edited Loading

Choose a reason for hiding this comment

hashishaw left a comment

Choose a reason for hiding this comment

johncowen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monkeychip May 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monkeychip May 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johncowen May 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johncowen May 17, 2021 • edited Loading

Choose a reason for hiding this comment

johncowen May 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashishaw left a comment

Choose a reason for hiding this comment

Monkeychip commented May 4, 2021 •

edited

Loading

Monkeychip May 14, 2021 •

edited

Loading

Monkeychip May 14, 2021 •

edited

Loading

johncowen May 17, 2021 •

edited

Loading

Monkeychip May 17, 2021 •

edited

Loading

Monkeychip May 17, 2021 •

edited

Loading

johncowen May 17, 2021 •

edited

Loading

johncowen May 17, 2021 •

edited

Loading

johncowen May 17, 2021 •

edited

Loading