UI - write without read for kv #6570

meirish · 2019-04-11T15:00:55Z

This is a follow up to #5879 (comment) which allowed you to write KV v2 secret versions without being able to read secret metadata. This extends that so the forms in KV v1 and KV v2 do not need read access to the secret / version data in order to write new data, allowing you to use workflows in the UI and practice the principle of least privilege.

Our tests cover 3 scenarios:

KV v2 where you can't read the metadata or the secret version:

KV v2 where you can read the metadata but not the secret version:

KV v1 where you can't read the secret data

KV v2 where you can do everything except read

Tool bar where you can only write

TODO

clean up the toolbar on the show screens (hide the JSON toggle)
determine what history items are available in v2 when you can read the metadata
are the empty state messages and the alert banners styled properly? are the messages appropriate and helpful?

madalynrose · 2019-04-11T16:30:03Z

ui/app/routes/vault/cluster/secrets/backend/secret-edit.js

+          id: secret,
+          // so we know it's a stub model and won't be saving it
+          // because we don't have access to that endpoint
+          isStub: true,


is there a way to make this more clear? isStub makes me think fake or incomplete data but doesn't make my mind go all the way to "doesn't have permission to see this"

Do you have suggestions about how we might change it? It is fake / incomplete data - we need to return an object so that the template has something to render, so we create a stub model. This code path is only run if you get a 403 from the read endpoint which is what the comment is here: https://github.com/hashicorp/vault/pull/6570/files#diff-66ae76528cc7c3eea9365b29bde71ce9R112

Something like failedRead starts to cross over to the canRead canEdit, etc. properties that the model has so I could see that being confusing too.

Chatted about this and will change it to failedServerRead - thanks @madalynrose !

joshuaogle

Had a small copy question, but looks great!

joshuaogle · 2019-04-11T17:04:25Z

ui/app/templates/partials/secret-form-show.hbs

+    @title="You do not have permission to read this secret."
+    @message={{if isV2
+      "A new version of this secret may be written but your associated policies do not allow you to read its current contents."
+      "Your policies permit you to overwrite the value at this path, but do not allow you to read its current one."


How about a slightly less technical: "Your policies permit you to overwrite this secret, but do not allow you to read this version."

Should we differentiate v1 and v2 ? In v1 you're going to overwrite the value, in v2 you'll create a new version.

Oh! I don't know why I misread that. Yeah, maybe something like "Your policies permit you to overwrite this secret, but do not allow you to read it."

andaley · 2019-04-11T22:50:32Z

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

+    {{else if @isV2}}
+      <AlertBanner
+        @type="warning"
+        @message="We were not able to read the current secret version to provide editing. Saving this form will create a new version of the secret and will utilize the available check-and-set mechanism."


I like the way you specified on the previous pages that the policies are what prevent the user from being able to read secret metadata. I think "we were not able to read metadata" is a little ambiguous -- as a user, I might see that and think there was some kind of unexpected error (vs. expected). How about something like:

"Your policies prevent you from reading and editing the current secret version. Saving this form will create a new version of the secret and will utilize the available check-and-set mechanism."

andaley · 2019-04-11T22:52:06Z

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

+    {{else}}
+      <AlertBanner
+        @type="warning"
+        @message="We were not able to read the current secret data. Saving using this form will overwrite the existing values."


Same comment as above -- assuming that this warning message is only displayed when the users' policy prevents them from reading a secret, we should be explicit that it's the policy and not an unexpected server error.

andaley · 2019-04-11T22:57:43Z

ui/app/components/secret-edit.js

+    'isV2',
+    function() {
+      // if the version couldn't be read from the server
+      if (this.isV2 && this.model.selectedVersion.failedServerRead) {


Aside from the user not having read access in their policy, are there any other scenarios which would cause the version to not be read from the server? If so, my comments above regarding the alert banner warning message aren't relevant. 😝

Nope, it'd only be the policy missing (or an additional Sentinel policy on the path) that could prevent it.

andaley · 2019-04-11T23:04:44Z

ui/app/routes/vault/cluster/secrets/backend/secret-edit.js

+      if (err.httpStatus === 403 && (modelType === 'secret-v2' || modelType === 'secret')) {
+        await capabilities;
+        // can't read the path and don't have update permission, so re-throw
+        if (!capabilities.get('canUpdate') && modelType === 'secret') {


Are you using the .get('canUpdate') syntax here to prevent an exception from being thrown? I thought we were able to access properties directly safely now?

Nice catch 🦅 👀 ! This is actually here because without it I got a very detailed message that I do indeed need to use get- it's one of the edge cases with native getters where ember's get is still needed because the capabilities object is a promise proxy which is a primitive ember provides. https://api.emberjs.com/ember/release/classes/PromiseProxyMixin

andaley · 2019-04-11T23:25:28Z

ui/app/templates/partials/secret-form-show.hbs

+    data-test-write-without-read-empty-message
+    @title="You do not have permission to read this secret."
+    @message={{if isV2
+      "A new version of this secret may be written but your associated policies do not allow you to read its current contents."


Also a nit, but I don't think you need the word associated here 😬

if you'd like, you could follow the sentence structure on line 7, like:

"Your policies permit you to write a new version of this secret, but do not allow you to read its current contents."

Heh, not a nit at all - I also dig following a similar structure for the messages 👍

andaley · 2019-04-11T23:36:29Z

This is looking so good! I had a few questions/comments related to clarifying the warning and empty state messages, but no blockers. :D

Also, the screenshots and overall PR description are great. I appreciate that you included examples and requests for specific kinds of feedback -- it helps me narrow in on what to review!

andaley · 2019-04-15T18:24:17Z

ui/app/routes/vault/cluster/secrets/backend/secret-edit.js

+      let versionId = targetVersion ? [backend, secret, targetVersion] : [backend, secret];
+
+      // if we have the metadata, a list of versions are part of the payload
+      let version = secretModel.versions && secretModel.versions.findBy('version', targetVersion);


hmm, so i've noticed after reading through this method a few times that this is the point where i start losing track of what's happening. i know there's a lot of logic that needs to happen inside model, but i wonder if we could break some of it up either into smaller methods like getSecretModel getTargetVersion fetchVersion etc for easier legibility. if it's too difficult to set that up, i think adding comment headers above each code block would be a good alternative, especially since the existing comments you have here are so helpful. whaddya think?

Yep, that's a great idea! Have an update to push 🔜. Thanks!

…show page

* master: (94 commits) Add type to documentation for create in token auth API (#6622) Merge multiple functions for creating consul containers into one. (#6612) Update deep to pull in default full-level-diff behavior Update base64 decrypt command documentation (#6614) Add some missing default values (#6611) Typo fixes (#6610) UI - no ember cli eslint (#6613) Update githook to work with go mod workflows (#6604) Cut version 1.1.2 Prep for 1.1.2 changelog++ Add a get handler function (#6603) Move cluster logic out of vault package (#6601) Adding common prefix known issue to upgrade guide (#6575) changelog++ UI - write without read for kv (#6570) Add known issue section to the upgrade guide (#6593) Update pre-push hook changelog++ Fix a dropped Okta error (#6592) ...

madalynrose reviewed Apr 11, 2019

View reviewed changes

joshuaogle reviewed Apr 11, 2019

View reviewed changes

andaley reviewed Apr 11, 2019

View reviewed changes

meirish force-pushed the ui-write-without-read-kv branch from 1f891f1 to 7454c98 Compare April 12, 2019 22:45

meirish marked this pull request as ready for review April 12, 2019 22:49

meirish requested review from a team April 12, 2019 22:49

meirish force-pushed the ui-write-without-read-kv branch from 80800fa to 9bdb799 Compare April 13, 2019 02:43

andaley reviewed Apr 15, 2019

View reviewed changes

meirish force-pushed the ui-write-without-read-kv branch from 7b0f3f3 to 103d918 Compare April 15, 2019 20:57

meirish added 15 commits April 16, 2019 10:20

wait for all hash promises to be settled

d147143

skeleton tests with policies for write without read

fcd8ab1

adjust what gets returned from the model hook

ddfb04b

refactor secret-edit model hook to use async/await

67261ab

return a stub version if we can't read secret data

d368e8c

return a stub model for v1 kv

0ee42b6

tweak tests to make re-runs friendlier

120f650

allow write without CAS if both v2 models cannot be read

af83586

show warnings on edit pages for different write without read scenarios

5def9d6

add no read empty states on secret show pages

5e4305b

review feedback

bc938b0

make message language consistent

8088924

use version models from metadata if we can read it

d745650

refresh route on delete / undelete / destroy

bfd9ec1

hide controls in the toolbar when you can't read the secret data

b455343

meirish added 4 commits April 16, 2019 10:21

show deleted / destroyed messaging over cannot read messaging on the …

2ad6040

…show page

fix test with model stub

f66afca

refactor large model hook into several functions

78f9bcc

comment clarifications

5632943

meirish force-pushed the ui-write-without-read-kv branch from 103d918 to 5632943 Compare April 16, 2019 15:29

andaley approved these changes Apr 16, 2019

View reviewed changes

meirish merged commit e6ec125 into master Apr 16, 2019

meirish deleted the ui-write-without-read-kv branch April 16, 2019 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI - write without read for kv #6570

UI - write without read for kv #6570

meirish commented Apr 11, 2019 •

edited

Loading

madalynrose Apr 11, 2019

meirish Apr 11, 2019

meirish Apr 11, 2019

joshuaogle left a comment

joshuaogle Apr 11, 2019

meirish Apr 11, 2019

joshuaogle Apr 11, 2019

andaley Apr 11, 2019

andaley Apr 11, 2019

andaley Apr 11, 2019 •

edited

Loading

meirish Apr 12, 2019

andaley Apr 11, 2019

meirish Apr 12, 2019

andaley Apr 11, 2019

andaley Apr 11, 2019

meirish Apr 12, 2019

andaley commented Apr 11, 2019

andaley Apr 15, 2019

meirish Apr 15, 2019

UI - write without read for kv #6570

UI - write without read for kv #6570

Conversation

meirish commented Apr 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuaogle left a 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

Choose a reason for hiding this comment

andaley Apr 11, 2019 • 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

Choose a reason for hiding this comment

andaley commented Apr 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meirish commented Apr 11, 2019 •

edited

Loading

andaley Apr 11, 2019 •

edited

Loading