UI: Pki key read view #18087

hellobontempo · 2022-11-22T18:27:47Z

github-advanced-security

Semgrep Scanner found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

hellobontempo · 2022-11-23T15:54:55Z

ui/app/models/pki/key.js


 export default class PkiKeyModel extends Model {
  @attr('string', { readOnly: true }) backend;
  @attr('boolean') isDefault;
-  @attr('string') keyRef; // reference to an existing key: either, vault generate identifier, literal string 'default', or the name assigned to the key. Part of the request URL.


the docs refer to key_ref but this is either the key_name (which is optional) or the key_id

zofskeez

Nice work! I left a few suggestions.

zofskeez · 2022-11-23T16:17:01Z

ui/app/adapters/pki/key.js

-
-  urlForQuery(backend, id) {
-    let url = `${this.buildURL()}/${encodePath(backend)}/keys`;
+  _urlForQuery(backend, id) {


I wonder if changing the name to something like getURL would be more succinct since you are bypassing the urlForQuery and urlForQueryRecord hooks? It seems like this method could handle constructing the url for other methods like createRecord as well if need be.

Yeah! I can do that. I struggled with method naming in this adapter, and wasn't sure if combining the methods into one was okay in the first place

I think it is. I might have done it slightly different. All you are doing in the queryRecord override is calling that custom url method. You could instead override urlForQueryRecord and return the url you need and then not have to override queryRecord.

So the adapter instead would just be these three methods?

urlForQuery(backend) { return `${this.buildURL()}/${encodePath(backend)}/key`; } urlForQueryRecord(query) { const { backend, id } = query; return `${this.buildURL()}/${encodePath(backend)}/key/${encodePath(id)}`; } query(store, type, query) { const { backend } = query; return this.ajax(this.urlForQuery(backend), 'GET', { data: { list: true } }); }

Yep that looks good! The way you approached it works too but I personally like to work with the provided hooks when possible.

Yeah, I haven't worked much with adapters and couldn't discern a clear pattern that we use. I also didn't realize urlForQueryRecord was a built in hook!

zofskeez · 2022-11-23T16:23:09Z

ui/app/models/pki/key.js

+  _attributeMeta = null; // cache initial result of expandAttributeMeta in getter and return
+  get formFields() {
+    if (!this._attributeMeta) {
+      this._attributeMeta = expandAttributeMeta(this, ['keyId', 'keyName', 'keyType', 'keyBits']);
+    }
+    return this._attributeMeta;
+  }


In the mean time you could move this to the constructor instead which is how formFields is populated when the decorator is applied.

Suggested change

_attributeMeta = null; // cache initial result of expandAttributeMeta in getter and return

get formFields() {

if (!this._attributeMeta) {

this._attributeMeta = expandAttributeMeta(this, ['keyId', 'keyName', 'keyType', 'keyBits']);

}

return this._attributeMeta;

}

constructor() {

super(...arguments);

this.formFields = expandAttributeMeta(this, ['keyId', 'keyName', 'keyType', 'keyBits']);

}

zofskeez · 2022-11-23T16:30:36Z

ui/lib/pki/addon/components/pki-key-details.hbs

+    <nav class="breadcrumb" aria-label="breadcrumbs" data-test-breadcrumbs="key-details">
+      <ul>
+        <li>
+          <span class="sep">/</span>
+          <LinkToExternal @route="secrets">secrets</LinkToExternal>
+        </li>
+        {{#each this.breadcrumbs as |breadcrumb|}}
+          <li>
+            <span class="sep">/</span>
+            {{#if breadcrumb.path}}
+              <LinkTo @route={{breadcrumb.path}}>
+                {{breadcrumb.label}}
+              </LinkTo>
+            {{else}}
+              {{breadcrumb.label}}
+            {{/if}}
+          </li>
+        {{/each}}
+      </ul>
+    </nav>


It might not be worth it if this exists in HDS but if not could we turn this into a component? This pattern appears to be copied in a lot of places.

I copied this from Chelsea's PR along with her "TODO" - I can ask what her thoughts are

This reminds me that I also wanted to consult @hashishaw about which also includes breadcrumbs and is used elsewhere in the PKI engine. I think some general cleanup could happen with all of these headers and would be a good follow-on ticket to tackle all of the views in one PR

zofskeez · 2022-11-23T16:32:17Z

ui/lib/pki/addon/components/pki-key-details.hbs

+
+<div class="box is-fullwidth is-sideless is-paddingless is-marginless">
+  {{#each @key.formFields as |attr|}}
+    {{#let (get @key attr.name) as |value|}}


I think since value is only used once you could remove the let block and use the get helper directly on @value.

thank you - good catch! I forgot to fix this when I added formFields to the model!

zofskeez · 2022-11-23T16:32:45Z

ui/lib/pki/addon/components/pki-key-details.ts

+interface Args {
+  key: {
+    backend: string;
+    keyName: string;
+    keyId: string;
+  };
+}


…ructor

zofskeez · 2022-11-23T18:17:28Z

ui/lib/pki/addon/components/pki-key-details.hbs

+    <InfoTableRow
+      @label={{capitalize (or attr.options.detailsLabel attr.options.label (humanize (dasherize attr.name)))}}
+      @value={{get @key attr.name}}
+    />


Was the alwaysRender arg not needed?

keyBits is an attribute available when generating a key, but is not currently included in the GET response for a key. (There's discussion to add it, but it's not looking likely) Since it will always be empty, I opted to remove alwaysRender=true so it can still appear in the create form, but won't render the row in the details page

zofskeez

Thanks!

hellobontempo added 2 commits November 21, 2022 16:34

WIP read view for pki keys

49a3c9f

remove options for query

889ba39

hellobontempo added ui pr/no-changelog labels Nov 22, 2022

hellobontempo added this to the 1.13.0-rc1 milestone Nov 22, 2022

github-advanced-security bot found potential problems Nov 22, 2022

View reviewed changes

hellobontempo added 3 commits November 22, 2022 11:50

combine adapter query methods

62e1041

fix controller class names

71b87b6

remove class

5a22ef6

hellobontempo commented Nov 23, 2022

View reviewed changes

zofskeez requested changes Nov 23, 2022

View reviewed changes

rename adapter method, cleanup template, move formFields to key const…

1cab111

…ructor

hellobontempo requested a review from zofskeez November 23, 2022 18:08

zofskeez reviewed Nov 23, 2022

View reviewed changes

zofskeez approved these changes Nov 23, 2022

View reviewed changes

hellobontempo merged commit 7a7657b into main Nov 23, 2022

hellobontempo deleted the ui/VAULT-11651/pki-key-view-page branch November 23, 2022 19:47

hashishaw mentioned this pull request Jan 27, 2023

UI: PKI Sign Intermediate #18842

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: Pki key read view #18087

UI: Pki key read view #18087

hellobontempo commented Nov 22, 2022

github-advanced-security bot left a comment

hellobontempo Nov 23, 2022

zofskeez left a comment

zofskeez Nov 23, 2022

hellobontempo Nov 23, 2022

zofskeez Nov 23, 2022

hellobontempo Nov 23, 2022

zofskeez Nov 23, 2022

hellobontempo Nov 23, 2022

zofskeez Nov 23, 2022

zofskeez Nov 23, 2022

hellobontempo Nov 23, 2022

hellobontempo Nov 23, 2022

zofskeez Nov 23, 2022

hellobontempo Nov 23, 2022

zofskeez Nov 23, 2022

zofskeez Nov 23, 2022

hellobontempo Nov 23, 2022

zofskeez left a comment

UI: Pki key read view #18087

UI: Pki key read view #18087

Conversation

hellobontempo commented Nov 22, 2022

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zofskeez 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zofskeez left a comment

Choose a reason for hiding this comment