Skip to content

Improve PIV/CAC error messages (LG-3991)#4718

Merged
mitchellhenke merged 4 commits intomasterfrom
mitchellhenke/better-piv-cac-errors-lg-3710
Mar 1, 2021
Merged

Improve PIV/CAC error messages (LG-3991)#4718
mitchellhenke merged 4 commits intomasterfrom
mitchellhenke/better-piv-cac-errors-lg-3710

Conversation

@mitchellhenke
Copy link
Contributor

@mitchellhenke mitchellhenke commented Feb 25, 2021

In updating PIV/CAC error messages, I ended up shaving a pretty large yak. PIV/CAC has a few places it gets used:

  1. Sign in
  2. Setting up a PIV/CAC within a signed in account
  3. To encourage adding PIV/CAC, if a user tries to use an unassociated PIV/CAC and then signs in normally, they get redirected to a different PIV/CAC page to add it (the PivCacSetupFromSignInController)

The translations for 1 and 2 were separate, but don't appear to need to be. Sign in translations were less descriptive, "PIV/CAC did not work" instead of "Your PIV/CAC is expired", so the less descriptive translations were removed in favor of more helpful error messages.

The routing for 1 and 2 used different methods for displaying errors. Sign in used individual routes and templates, ex:

      get '/login/piv_cac_account_not_found' => 'users/piv_cac_login#account_not_found'
      get '/login/piv_cac_did_not_work' => 'users/piv_cac_login#did_not_work'
      get '/login/piv_cac_temporary_error' => 'users/piv_cac_login#temporary_error'

The PIV/CAC setup flow relied on flash[:error_type] existing within the session to show errors, and would render a different template on the new route if it existed. It would also dynamically interpolate what was in flash[:error_type] into the i18n key, which could cause unhandled exceptions if we added a new error in PKI without a corresponding translation. That strategy also required not doing unused/missing key checks since they weren't explicitly used.

I've tried to consolidate the error pages into using the same messaging, and moved the error message logic into a parameter added on the redirects. Some of the existing logic and routes will need to exist to prevent backwards incompatibility until the next release.

In draft mode to see how tests go and get new translations done

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 controller was never able to reach these error branches, so I removed them.

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 method is never called

Copy link
Contributor Author

Choose a reason for hiding this comment

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

combines the PivCacAuthenticationSetupErrorPresenter and PivCacAuthenticationLoginPresenter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

certificate.timeout and certificate.ocsp_error are no longer returned from PKI

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/better-piv-cac-errors-lg-3710 branch from bb24eea to 276f9e0 Compare February 25, 2021 17:50
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 template was not being used, so it's been repurposed

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

should we add an internal warn logging of what the missing error code was?

Copy link
Contributor

Choose a reason for hiding this comment

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

all these try_again links are the same, WDYT of just generating it once at the top as a local or making a helper method for them?

def try_again_link
  @view.link_to(t('instructions.mfa.piv_cac.try_again'), @try_again_url) if @try_again_url
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call, added in 62f234b0e8b97f6e585ceeec70bb39e3ccc06f15

Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/better-piv-cac-errors-lg-3710 branch 6 times, most recently from 92c1379 to e96a551 Compare February 26, 2021 22:19
@mitchellhenke
Copy link
Contributor Author

Translations added in e96a551

Spanish translations included a couple different ways to say "try again", so I had to split up those into two separate pieces of content.

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/better-piv-cac-errors-lg-3710 branch from e96a551 to 461e171 Compare February 26, 2021 23:06
@mitchellhenke mitchellhenke marked this pull request as ready for review March 1, 2021 14:23
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/better-piv-cac-errors-lg-3710 branch from 461e171 to a92d47f Compare March 1, 2021 17:54
@mitchellhenke mitchellhenke merged commit 3fab497 into master Mar 1, 2021
@mitchellhenke mitchellhenke deleted the mitchellhenke/better-piv-cac-errors-lg-3710 branch March 1, 2021 19:17
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.

2 participants