[LG-358] Allow piv/cac as 2fa for account creation

[jgsmith-usds]

Why:
Some of our users may only have a piv/cac available. We
want them able to create accounts with piv/cac as their
primary 2fa option.

How:
We add piv/cac as an option in the list of 2fa options
during account creation.

Hi! Before submitting your PR for review, and/or before merging it, please
go through the following checklist:

• For DB changes, check for missing indexes, check to see if the changes
  affect other apps (such as the dashboard), make sure the DB columns in the
  various environments are properly populated, coordinate with devops, plan
  migrations in separate steps.

• For route changes, make sure GET requests don't change state or result in
  destructive behavior. GET requests should only result in information being
  read, not written.

• For encryption changes, make sure it is compatible with data that was
  encrypted with the old code.

• For secrets changes, make sure to update the S3 secrets bucket with the
  new configs in all environments.

• Do not disable Rubocop or Reek offenses unless you are absolutely sure
  they are false positives. If you're not sure how to fix the offense, please
  ask a teammate.

• When reading data, write tests for nil values, empty strings,
  and invalid formats.

• When calling redirect_to in a controller, use _url, not _path.

• When adding user data to the session, use the user_session helper
  instead of the session helper so the data does not persist beyond the user's
  session.

• When adding a new controller that requires the user to be fully
  authenticated, make sure to add before_action :confirm_two_factor_authenticated.

[jgsmith-usds]

@monfresh @andrewhughey There are some consequences if someone adds a piv/cac and doesn't add a telephone. We'll probably want to add a check for a phone number and redirect to the "add a phone" flow if they don't have one.

[monfresh]

Can you elaborate on the consequences? One thing we talked about was allowing a user to set up PIV/CAC without a backup phone number during account creation (i.e. prompting them to set it up, but also allowing them to cancel), and then prompting them each time they sign in until they do it.

Do the consequences you mention prevent the above scenario?

[jgsmith-usds]

They do. There is code that assumes that a user who has added a second factor has a phone number. I didn't track them all down, but I did find one in app/models/otp_requests_tracker.rb. Line 4 assumes that phone is a string. Changing that to phone = phone&.strip leads to issues elsewhere. I didn't try to get everything working since that seemed out of scope for this PR. But we may want to hold off on merging this PR regardless of review until we know we can avoid throwing errors if a user hasn't added a phone.

[andrewhughey]

Wouldn't that same code / assumption about phone numbers impact users who have set up an auth app and no number?

[jgsmith-usds]

I think I have a way to work around this. I'll add a commit to this PR once I make sure tests are passing.

[monfresh]

I tested the auth app scenario, and while sign up and signing back in works, I discovered a bug in that if you don't have a phone number configured, it still shows If you can’t use your authenticator app right now you can get a code via text message or Receive a code via phone call. I will fix that.

[monfresh]

Why not read the agency attribute directly from the DB?

[jgsmith-usds]

Good question. I'll change that.

[monfresh]

Should the last 2 conditions be reversed, so that if someone has both PIV/CAC and phone, it defaults to PIV/CAC?

[jgsmith-usds]

I was going for minimal touch on this. If doing so doesn't result in a mass of tests breaking, I don't see why I wouldn't. Let me see what happens.

[monfresh]

Should we also add a controller test to make sure a user can't visit the piv_cac endpoint directly when piv_cac_available_for_agency? is false?

[jgsmith-usds]

I thought about it, but I'm not too worried. If someone does manage to get their piv/cac to work, it's okay. The main reason for showing only when they're associated with a supported agency is to avoid confusion for the majority of government employees we don't support yet.

[monfresh]

Should we replace metadata[:agency] with agency?

[jgsmith-usds]

Probably. I'll fix that when I revert the ordering from the previous commit. It broke more tests and feels like feature creep since that's not part of account creation.

[monfresh]

Same comment here about using agency.

[monfresh]

LGTM % comments