Skip to content

Certificate cleanup#404

Merged
zachmargolis merged 7 commits intomainfrom
margolis-certificate-cleanup
Mar 31, 2021
Merged

Certificate cleanup#404
zachmargolis merged 7 commits intomainfrom
margolis-certificate-cleanup

Conversation

@zachmargolis
Copy link
Contributor

@zachmargolis zachmargolis commented Mar 30, 2021

I was starting to make changes to the dashboard to support multiple to correspond to 18F/identity-idp#4851

But then I noticed a lot of things I wanted to clean up so I started on that:

  1. Removed _certificate_expiration.html.erb partial
    • Inline styles are blocked by our CSP
    • Moved CSS logic on to ServiceProviderCertificate class
  2. Updated ServiceProviderCertificate class, it now wraps an X509 cert rather than subclasses one (easier to test)
  3. Visual changes: updated the SP "show" page:
    • Full cert body is now hidden behind a details toggle
    • Added some more summary details (I expect these will make it easier to tell multiple certificates apart)
state before after
initial Screen Shot 2021-03-30 at 10 17 42 AM Screen Shot 2021-03-30 at 10 17 57 AM
expanded N/A Screen Shot 2021-03-30 at 10 18 03 AM

package.json Outdated
"dependencies": {
"@rails/webpacker": "^4.3.0",
"identity-style-guide": "^2.2.3",
"identity-style-guide": "^5.0.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't be quite so easy an upgrade, unfortunately. Some of the breaking changes do affect the dashboard.

Notably:

Several button variants have been removed. These include: [...]

There's one instance of a dropdown:

<button type="button" class="usa-button usa-button--dropdown">

... though last I checked, I'm not sure this partial is even referenced anywhere.

And a tiny button:

<%= link_to service_provider.friendly_name, service_provider, class: "usa-button usa-button--inverse usa-button--tiny margin-top-1" %>

2.x to 3.x is the major upgrade of USWDS: https://github.com/18F/identity-style-guide/blob/main/CHANGELOG.md#300 . This usually includes updating banner markup.

The original ticket for this is LG-3762.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok so then I'll revert that maybe I'll just do a quick !important or something one-off style to get the <details>/<summary> fix in?

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 that bad if we could remove the dropdown and check or substitute the teams list "tiny" button, but yeah, a one-off fix would be a reasonable alternative if we don't want to deal with it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with the patch approach in 5c6741f

<% end %>

<h2>
<label for="saml_client_cert">Public certificate:</label>
Copy link
Contributor

Choose a reason for hiding this comment

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

It existed previously, though the name on the new <details> had caught my attention: Does the for attribute actually do anything for us? This doesn't seem like an appropriate use of a label, since it's not associated with an input.

https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Content_categories#Form_labelable

I could maybe see it as a case for aria-labelledby or aria-describedby.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like they came from #366 from AxE matchers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok no that was not right... looks like those have just been there a long time.... it's probably time for them to go, especially since this page isn't a form?

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 they were copied over from the edit form?

@zachmargolis zachmargolis requested a review from orenyk March 30, 2021 21:20
Copy link
Contributor

@orenyk orenyk left a comment

Choose a reason for hiding this comment

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

Looks good!

@zachmargolis
Copy link
Contributor Author

(overriding snyk because it's stalled out)

@zachmargolis zachmargolis merged commit 588c334 into main Mar 31, 2021
@zachmargolis zachmargolis deleted the margolis-certificate-cleanup branch March 31, 2021 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants