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 cross-sign issuers #18695

Merged
merged 24 commits into from
Jan 25, 2023
Merged

Conversation

hellobontempo
Copy link
Contributor

@hellobontempo hellobontempo commented Jan 13, 2023

Cross-signing issuers in the UI, along with error messages

full-cross-sign

Updated screenshot with success/error count, buttons and alert/warning banners:
Screen Shot 2023-01-25 at 10 41 43 AM

To do in follow-on PR:

  • write tests to make sure that parsed certificate data isn't completely stripped by serializer
  • write test verifying issued leaf certs are valid/accurate copies

Copy link
Contributor

@zofskeez zofskeez left a comment

Choose a reason for hiding this comment

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

Nice work on all this! 👏

@hellobontempo hellobontempo marked this pull request as ready for review January 25, 2023 01:19
@@ -0,0 +1 @@
export { default } from 'core/helpers/img-path';
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 is right for using a helper in an engine, right? I'm noticing there's a loading lag - wondering if I did something incorrectly 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

That looks right! Shouldn't be a lag loading the actual component itself. Not sure about the image itself which depend on size and location.

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 - it's just the image (you can see it in the gif that it shows up a little after the page loads). I haven't actively developed with one on the page, so just wanted to confirm that was working as expected 😄

</div>
<div class="box is-fullwidth has-only-top-shadow">
{{#each this.signedIssuers as |crossSignRow|}}
<div class="box is-marginless no-top-shadow has-slim-padding">
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 considered making this table (Lines 20- 66) a component <InfoTableColumns>, but there aren't any current design plans to use a table column display elsewhere. I can pull out into a component, though, if preferred!

Screen Shot 2023-01-24 at 5 23 25 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

This component isn't terribly large and if we're unsure if the pattern will reappear I would suggest leaving as is and we can always come back and rework it into a generic component if need be.

Copy link
Collaborator

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

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

🤩 Amazing work on this!! A couple minor comments but nothing blocking

ui/app/adapters/pki/issuer.js Outdated Show resolved Hide resolved
);
this.signedIssuers.addObject({ ...data, hasError: false });
} catch (error) {
this.signedIssuers.addObject({
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be a follow-on, but have we tested this workflow with various permissions? I wonder if this error format is going to make sense if they run into a permissions error on one of the earlier steps 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely a great point to include in testing. I haven't tested but any errors that happen from making API requests in the crossSignIntermediate method are caught and should be added on line 92 with hasError: errorMessage(error).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🎉
Screen Shot 2023-01-25 at 9 47 29 AM

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yay! One step better would be to know on which path the permission was denied, but at least we get something out of the box 🎉

@@ -1,3 +1,14 @@
import Route from '@ember/routing/route';
import PkiIssuerIndexRoute from './index';
// TODO comment in before merging
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want to comment this back in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes! 🙃

@hellobontempo hellobontempo enabled auto-merge (squash) January 25, 2023 18:53
@hellobontempo hellobontempo merged commit ccaed88 into main Jan 25, 2023
@hellobontempo hellobontempo deleted the ui/VAULT-9360/pki-cross-sign-issuers branch January 25, 2023 19:40
jayant07-yb pushed a commit to jayant07-yb/hashicorp-vault-integrations that referenced this pull request Mar 15, 2023
* make cross-sign component

* remove type from obj-list-input

* finish skeleton of component

* handle change on init

* finish cross-sign form

* add cancel transition

* update pki/issuer adapter to accept backend passed from adapterOptions

* first draft of cross-signing issuers component

* refactor to accommodate listing signed certs

* changes to config adapter and model, likely will need to revert and manually add to pki/action

* add args to infotooltip, move header to cross-sign route

* use pki/action model

* move header to route file

* finish displaying signed certificates

* finish styling

* add issuer id to cross-sign breadcrumbs

* add parsed cert data to requests

* add status count

* add error banner back
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants