Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI: Pki key read view #18087

Merged
merged 6 commits into from
Nov 23, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 10 additions & 13 deletions ui/app/adapters/pki/key.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,21 @@ import { encodePath } from 'vault/utils/path-encoding-helpers';
export default class PkiKeyAdapter extends ApplicationAdapter {
namespace = 'v1';

optionsForQuery(id) {
const data = {};
if (!id) {
data['list'] = true;
}
return { data };
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 } });
  }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

const url = `${this.buildURL()}/${encodePath(backend)}`;
if (id) {
url = url + '/' + encodePath(id);
return url + '/key/' + encodePath(id);
}
return url;
return url + '/keys';
}

query(store, type, query) {
const { backend } = query;
return this.ajax(this._urlForQuery(backend), 'GET', { data: { list: true } });
}

queryRecord(store, type, query) {
const { backend, id } = query;
return this.ajax(this.urlForQuery(backend, id), 'GET', this.optionsForQuery(id));
return this.ajax(this._urlForQuery(backend, id), 'GET');
}
}
15 changes: 13 additions & 2 deletions ui/app/models/pki/key.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,21 @@
import Model, { attr } from '@ember-data/model';
import { expandAttributeMeta } from 'vault/utils/field-to-attrs';

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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@attr('string') keyId;
@attr('string', { possibleValues: ['internal', 'external'] }) type;
@attr('string', { detailsLabel: 'Key ID' }) keyId;
@attr('string') keyName;
@attr('string') keyType;
@attr('string', { detailsLabel: 'Key bit length' }) keyBits;

// TODO refactor when field-to-attrs util is refactored as decorator
_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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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']);
}

}
3 changes: 2 additions & 1 deletion ui/app/serializers/pki/key.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import ApplicationSerializer from '../application';

export default class PkiKeySerializer extends ApplicationSerializer {
primaryKey = 'key_id';
// rehydrate each keys model so all model attributes are accessible from the LIST response
normalizeItems(payload) {
if (payload.data) {
if (payload.data?.keys && Array.isArray(payload.data.keys)) {
return payload.data.keys.map((key) => ({ id: key, ...payload.data.key_info[key] }));
return payload.data.keys.map((key) => ({ key_id: key, ...payload.data.key_info[key] }));
}
Object.assign(payload, payload.data);
delete payload.data;
Expand Down
59 changes: 59 additions & 0 deletions ui/lib/pki/addon/components/pki-key-details.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<PageHeader as |p|>
<p.top>
{{! TODO: This should be replaced with HDS::Breadcrumbs }}
<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>
Comment on lines +4 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</p.top>
<p.levelLeft>
<h1 class="title is-3" data-test-key-details-title>
<Icon @name="certificate" @size="24" class="has-text-grey-light" />
View key
</h1>
</p.levelLeft>
</PageHeader>
<Toolbar>
<ToolbarActions>
<ConfirmAction
@buttonClasses="toolbar-link"
@onConfirmAction={{this.deleteKey}}
@confirmTitle="Delete key?"
@confirmButtonText="Delete"
data-test-pki-key-delete
>
Delete
</ConfirmAction>
<ToolbarLink @route="keys.key.edit" @model={{@key.model.name}}>
Edit key
</ToolbarLink>
</ToolbarActions>
</Toolbar>

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<InfoTableRow
@label={{capitalize (or attr.options.detailsLabel attr.options.label (humanize (dasherize attr.name)))}}
@value={{value}}
@alwaysRender={{true}}
/>
{{/let}}
{{/each}}
</div>
24 changes: 24 additions & 0 deletions ui/lib/pki/addon/components/pki-key-details.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { action } from '@ember/object';
import Component from '@glimmer/component';

interface Args {
key: {
backend: string;
keyName: string;
keyId: string;
};
}
Comment on lines +4 to +10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉


export default class PkiKeyDetails extends Component<Args> {
get breadcrumbs() {
return [
{ label: this.args.key.backend || 'pki', path: 'overview' },
{ label: 'keys', path: 'keys.index' },
{ label: this.args.key.keyId },
];
}

@action deleteKey() {
// TODO handle delete
}
}
2 changes: 1 addition & 1 deletion ui/lib/pki/addon/controllers/issuers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { action } from '@ember/object';
import { next } from '@ember/runloop';
import { getOwner } from '@ember/application';

export default class PkiRolesIssuerController extends Controller {
export default class PkiIssuerIndexController extends Controller {
get mountPoint() {
return getOwner(this).mountPoint;
}
Expand Down
2 changes: 1 addition & 1 deletion ui/lib/pki/addon/controllers/keys/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import Controller from '@ember/controller';
import { getOwner } from '@ember/application';

export default class PkiRolesIssuerController extends Controller {
export default class PkiKeysIndexController extends Controller {
get mountPoint() {
return getOwner(this).mountPoint;
}
Expand Down
2 changes: 1 addition & 1 deletion ui/lib/pki/addon/routes.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export default buildRoutes(function () {
this.route('index', { path: '/' });
this.route('create');
this.route('import');
this.route('key', { path: '/:key_ref' }, function () {
this.route('key', { path: '/:key_id' }, function () {
this.route('details');
this.route('edit');
});
Expand Down
11 changes: 11 additions & 0 deletions ui/lib/pki/addon/routes/keys/key.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import PkiKeysIndexRoute from './index';

export default class PkiKeyRoute extends PkiKeysIndexRoute {
model() {
const { key_id } = this.paramsFor('keys/key');
return this.store.queryRecord('pki/key', {
backend: this.secretMountPath.currentPath,
id: key_id,
});
}
}
8 changes: 4 additions & 4 deletions ui/lib/pki/addon/templates/keys/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
secret used to prove who they are and who they trust.</p>
{{#if this.model.keyModel.length}}
{{#each this.model.keyModel as |pkiKey|}}
<LinkedBlock class="list-item-row" @params={{array "roles.role.details" pkiKey.id}} @linkPrefix={{this.mountPoint}}>
<LinkedBlock class="list-item-row" @params={{array "keys.key.details" pkiKey.keyId}} @linkPrefix={{this.mountPoint}}>
<div class="level is-mobile">
<div class="level-left">
<div>
Expand All @@ -44,12 +44,12 @@
<nav class="menu">
<ul class="menu-list">
<li>
<LinkTo @route="keys.key.details" @model={{pkiKey.id}}>
<LinkTo @route="keys.key.details" @model={{pkiKey.keyId}}>
Details
</LinkTo>
</li>
<li>
<LinkTo @route="keys.key.edit" @model={{pkiKey.id}}>
<LinkTo @route="keys.key.edit" @model={{pkiKey.keyId}}>
Edit
</LinkTo>
</li>
Expand All @@ -64,7 +64,7 @@
{{else}}
<EmptyState @title="PKI not configured" @message="This PKI mount hasn’t yet been configured with a certificate issuer.">
<LinkTo @route="configuration.create">
ARG TODO waiting for language from design
Configure PKI
</LinkTo>
</EmptyState>
{{/if}}
2 changes: 1 addition & 1 deletion ui/lib/pki/addon/templates/keys/key/details.hbs
Original file line number Diff line number Diff line change
@@ -1 +1 @@
keys.key.:id.details
<PkiKeyDetails @key={{this.model}} />