Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
10 changes: 10 additions & 0 deletions app/assets/stylesheets/application.css.scss.erb
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,13 @@ $image-path: 'img';
}
}

.certificate-expired {
color: white;
background: red;
}

.certificate-warning {
color: black;
background: yellow;
}

3 changes: 2 additions & 1 deletion app/models/service_provider.rb
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,11 @@ def redirect_uris=(uris)
super uris.select(&:present?)
end

# @return [ServiceProviderCertificate]
def certificate
@certificate ||= begin
if saml_client_cert
ServiceProviderCertificate.new saml_client_cert
ServiceProviderCertificate.new(OpenSSL::X509::Certificate.new(saml_client_cert))
else
null_certificate
end
Expand Down
40 changes: 32 additions & 8 deletions app/models/service_provider_certificate.rb
Original file line number Diff line number Diff line change
@@ -1,14 +1,30 @@
# A class to provide colorized expiration text
class ServiceProviderCertificate < OpenSSL::X509::Certificate
def expiration_time_to_colorized_s
self.class.expiration_time_to_colorized_s(not_after)
# A class to decorate a certificate, for easier warning and expiration
class ServiceProviderCertificate
attr_reader :cert

# @param [OpenSSL::X509::Certificate,String] cert
def initialize(cert)
Rails.logger.info(cert.class)
Comment thread
zachmargolis marked this conversation as resolved.
Outdated
@cert = cert
end

def method_missing(name, *args, &block)
if cert.respond_to?(name)
cert.send(name, *args, &block)
else
super
end
end

def self.expiration_time_to_colorized_s(time)
time_s = time.to_s
if time < Time.zone.now
def respond_to_missing?(name)
cert.respond_to_missing?(name) || super
end

def expiration_time_to_colorized_s
time_s = not_after.to_s
if not_after < Time.zone.now
time_s.colorize(color: :black, background: :red)
elsif time < warning_period
elsif not_after < self.class.warning_period
time_s.colorize(color: :black, background: :light_yellow)
else
time_s
Expand All @@ -18,4 +34,12 @@ def self.expiration_time_to_colorized_s(time)
def self.warning_period
(Figaro.env.certificate_expiration_warning_period || 60).to_i.days.from_now
end

def expiration_css_class
if not_after < Time.zone.now
'certificate-expired'
elsif not_after < self.class.warning_period
'certificate-warning'
end
Comment thread
zachmargolis marked this conversation as resolved.
end
end
10 changes: 0 additions & 10 deletions app/views/service_providers/_certificate_expiration.html.erb

This file was deleted.

24 changes: 11 additions & 13 deletions app/views/service_providers/_certificate_expiration_td.html.erb
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
<% cert = app.certificate %>
<% if cert.not_after < Time.zone.now %>
<td style='color: white; background: red'>
<% elsif cert.not_after < ServiceProviderCertificate.warning_period %>
<td style='color: black; background: yellow'>
<% else %>
<td>
<%
# locals: app
cert = app.certificate
%>

<%= content_tag(:td, class: cert.expiration_css_class) do %>
Comment thread
zachmargolis marked this conversation as resolved.
<% if cert.issuer.to_s == 'Null Certificate' %>
Invalid
<% else %>
<%= cert.not_after.localtime.strftime("%F") %>
<% end %>
<% end %>
<% if cert.issuer.to_s == 'Null Certificate' %>
Invalid
<% else %>
<%= cert.not_after.localtime.strftime("%F") %>
<% end %>
</td>
53 changes: 37 additions & 16 deletions app/views/service_providers/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -58,27 +58,48 @@
<% end %>
<% end %>

<% if service_provider.identity_protocol == 'saml' %>
<h2><label for="acs_url">Assertion Consumer Service URL:</label></h2>
<p class="font-mono-xs margin-top-0" name="acs_url"><%= service_provider.acs_url %></p>
<% if service_provider.identity_protocol == 'saml' %>
<h2><label for="acs_url">Assertion Consumer Service URL:</label></h2>
<p class="font-mono-xs margin-top-0" name="acs_url"><%= service_provider.acs_url %></p>

<h2><label for="assertion_consumer_logout_service_url">Assertion Consumer Logout Service URL:</label></h2>
<p class="font-mono-xs margin-top-0" name="assertion_consumer_logout_service_url"><%= service_provider.assertion_consumer_logout_service_url %></p>
<h2><label for="assertion_consumer_logout_service_url">Assertion Consumer Logout Service URL:</label></h2>
<p class="font-mono-xs margin-top-0" name="assertion_consumer_logout_service_url"><%= service_provider.assertion_consumer_logout_service_url %></p>

<h2><label for="assertion_consumer_logout_service_url">SP Initiated Login URL:</label></h2>
<p class="font-mono-xs margin-top-0" name="assertion_consumer_logout_service_url"><%= service_provider.sp_initiated_login_url %></p>
<h2><label for="assertion_consumer_logout_service_url">SP Initiated Login URL:</label></h2>
<p class="font-mono-xs margin-top-0" name="assertion_consumer_logout_service_url"><%= service_provider.sp_initiated_login_url %></p>

<h2><label for="block_encryption">SAML Assertion Encryption:</label></h2>
<p class="font-mono-xs margin-top-0" name="block_encryption"><%= service_provider.block_encryption %></p>
<% end %>
<h2><label for="block_encryption">SAML Assertion Encryption:</label></h2>
<p class="font-mono-xs margin-top-0" name="block_encryption"><%= service_provider.block_encryption %></p>
<% end %>

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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?

</h2>

<% unless service_provider.saml_client_cert.blank? %>
<dl>
<dt>Issuer</dt>
<dd><%= service_provider.certificate.issuer %></dd>

<h2><label for="saml_client_cert">Public certificate:</label></h2>
<pre><code><%= service_provider.saml_client_cert %></code></pre>
<% unless service_provider.saml_client_cert.blank? %>
<p class="font-mono-xs margin-top-0" name="saml_client_cert">Expires:
<%= render partial: 'certificate_expiration', locals: { app: service_provider } %></p>
<% end %>
<dt>Subject</dt>
<dd><%= service_provider.certificate.subject %></dd>

<dt>Serial Number</dt>
<dd class="font-mono-xs"><%= service_provider.certificate.serial %></dd>

<dt>Expiration</dt>
<%= content_tag(:dd, class: ['font-mono-xs', service_provider.certificate.expiration_css_class]) do %>
<%= service_provider.certificate.not_after %>
<% end %>
</dl>

<details name="saml_client_cert">
<summary>
Raw Certificate
</summary>
<pre><code><%= service_provider.saml_client_cert %></code></pre>
</details>
<% end %>

<h2><label for="return_to_sp_url">Return to App URL:</label></h2>
<p class="font-mono-xs margin-top-0" name="return_to_sp_url"><%= service_provider.return_to_sp_url %></p>
Expand Down
3 changes: 1 addition & 2 deletions lib/tasks/check_certificates.rake
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ namespace :dashboard do

puts "\nExpiration Issuer"
sps.each do |sp|
exp = collection[sp]&.not_after
puts "#{ServiceProviderCertificate.expiration_time_to_colorized_s(exp)}: #{sp}"
puts "#{collection[sp]&.expiration_time_to_colorized_s} #{sp}"
end
end
end
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
},
"dependencies": {
"@rails/webpacker": "^4.3.0",
"identity-style-guide": "^2.2.3",
"identity-style-guide": "^5.0.3",

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

"jquery": "^3.5.0",
"jquery-ujs": "^1.2.2",
"kind-of": "^6.0.3",
Expand Down
47 changes: 36 additions & 11 deletions spec/models/service_provider_certificate_spec.rb
Original file line number Diff line number Diff line change
@@ -1,29 +1,54 @@
require 'rails_helper'

describe ServiceProviderCertificate do
RSpec.describe ServiceProviderCertificate do
before do
allow(Figaro.env).to receive(:certificate_expiration_warning_period).and_return('5')
end

let(:plain_cert) do
instance_double('OpenSSL::X509::Certificate', not_after: not_after)
end

subject(:cert) do
ServiceProviderCertificate.new(plain_cert)
end

context 'certificate is expired' do
let(:not_after) { 1.day.ago }

it 'wraps the expiration in ansi color codes to make it black on red' do
expired_time = 1.day.ago
expect(ServiceProviderCertificate.expiration_time_to_colorized_s(expired_time)).
to match(/\A\e\[0;30;41m#{expired_time.to_s}\e\[0m\z/)
expect(cert.expiration_time_to_colorized_s).
to match(/\A\e\[0;30;41m#{not_after.to_s}\e\[0m\z/)
end

it 'has an expired CSS style' do
expect(cert.expiration_css_class).to eq('certificate-expired')
end
end

context 'certificate is near expiration' do
let(:not_after) { (5.days - 10.seconds).from_now }

it 'wraps the expiration in ansi color codes to make it black on yellow' do
expired_time = (5.days - 10.seconds).from_now
expect(ServiceProviderCertificate.expiration_time_to_colorized_s(expired_time)).
to match(/\A\e\[0;30;103m#{expired_time.to_s}\e\[0m\z/)
expect(cert.expiration_time_to_colorized_s).
to match(/\A\e\[0;30;103m#{not_after.to_s}\e\[0m\z/)
end

it 'has a warning CSS style' do
expect(cert.expiration_css_class).to eq('certificate-warning')
end
end

context 'certificate is not near expiration' do
it 'does not wraps the expiration in ansi color codes' do
expired_time = (5.days + 10.seconds).from_now
expect(ServiceProviderCertificate.expiration_time_to_colorized_s(expired_time)).
to match(/\A#{expired_time.to_s}\z/)
let(:not_after) { (5.days + 10.seconds).from_now }

it 'does not wrap the expiration in ansi color codes' do
expect(cert.expiration_time_to_colorized_s).
to match(/\A#{not_after.to_s}\z/)
end

it 'does not have a CSS style' do
expect(cert.expiration_css_class).to be_nil
end
end
end
Loading