LG-428 Build 2FA selection at sign in

[stevegsa]

Why: We need the ability to select from multiple two factor methods in a streamlined manner.

How: Create a new 2fa selection screen upon login which will display 2fa options currently configured for the user. Standardize and simplify all the 2fa screens with a single layout with clear instructions for each 2fa including a remember browser option as well as options to select a different 2fa or cancel. Provide an account reset option (or pending cancel) as a last resort. Finally, this PR DRYs up the code somewhat. However, a subsequent PR will refactor both the creation and login process into a more service based architecture.

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.

[stevegsa]

Screenshots:

sms

/login/two_factor/list_options (via Choose another security option)

voice call

personal key

auth app

piv/cac

pending account reset/delayed delete:

mobile:

[jgsmith-usds]

Should this be a translation?

[jgsmith-usds]

I'm uncomfortable with all_two_factors_enabled? rather than enough_two_factors_enabled? because we really just want two factors rather than all three (for those who have piv/cac available). Not critical since I'll probably do a round of policy object building in a sprint or two.

[stevegsa]

I thought about it and originally coded it to have just two. However, what happens when we introduce a new two factor option? They will never see it if we don't present it to them with this screen.

[jgsmith-usds]

This method name makes me think it returns a url, similar to user_two_factor_authentication_url. Similar to the method by the same name in app/controllers/concerns/two_factor_authenticatable.rb.

[stevegsa]

It does return a url

[mryenq]

For this, I'm thinking we may want to stick with more resource-oriented naming. A controller is typically operating (CRUD'ing) on a noun, and representing some 'thing' at its endpoint. (I understand it can be tricky to name at times, depending on what you're doing, and especially with 'method' in the name.)

[monfresh]

I agree. Controller names should be nouns. Dropping the verb from the name should do it.

[stevegsa]

It needs something to distinguish it from the other controller. Originally it was 'additional_methods' (a noun) but I saw the humongous url and shortened it.

[mryenq]

Same question as above. If I'm new, I'm wondering if I'm creating a new ListOption at this endpoint. So maybe we can think about what resource this represents...

[stevegsa]

Yes this name definitely stinks and I spent too much time thinking about it. I'm open to suggestions. The endpoint is /login/two_factor/XXX and anything with 'login' would be redundant. It also needs to be distinguishable from the additional methods controller.

[monfresh]

Please add an analytics event.

[stevegsa]

It's coming. I'm just trying to get the specs working at the moment.

[monfresh]

What do you think about creating a mapping of 2FA options to endpoints to eliminate this case statement and prevent it from growing as we add more options, something like:

module TwoFactorOptions
  ENDPOINTS = {
    sms: 'phone_setup_url',
    voice: 'phone_setup_url',
    auth_app: 'authenticator_setup_url',
    piv_cac: 'setup_piv_cac_url'
  }
end

and in the controller:

def process_valid_form
  selection = @two_factor_add_methods_form.selection
  url = TwoFactorOptions::ENDPOINTS.fetch(selection, 'phone_setup_url')
  redirect_to public_send(url)
end

[stevegsa]

Have something similar in the other controller.

[monfresh]

This controller is basically duplicating the existing TwoFactorAuthenticationSetupController, which IMO is better named than this one. Why do we need both? Were you planning on only keeping one?

[stevegsa]

The behavior of the setup controller is different from the additional methods screen.

[monfresh]

The way I see it, they both are presenting the user with a choice of options, with the only difference being that on initial account creation, they automatically get all the options (plus PIV/CAC if eligible), and after account creation, they only see the options that they don't have yet and that they are eligible for.

Is there another difference I'm missing?

[stevegsa]

It's also the context of when it's run and where it's going. They do have an option to skip this step and we're not fully sure if we will have it automatically come up when we have two backup second factors (ie key and authenticator) or if it will come up until they have all. Users signing in from SP's won't know new 2fa options are available unless we force this page and allow them to skip it but an argument can be made that it adds more friction.

[stevegsa]

We definitely could make the setup controller more configurable and it would do the job also. I'm just not a fan of adding all those conditionals.

[monfresh]

Nice!

[monfresh]

What do you think about reducing this complexity by adding a new Postgres array field to the Users table that will hold all the 2FA options they have set up so far? That way, all you need is a diff between what they already have and what's available (which should be a constant) as opposed to asking if they are phone_enabled, totp_enabled, etc.

[stevegsa]

This was going to be part of @jgsmith-usds 's refactor. The logic was going to be extracted out into a service.

[stevegsa]

It definitely needs work. If we were to store it in the db I'd opt for exploding it out into a table. I'm not a huge fan of array fields.

[stevegsa]

I'd be happy with getting this logic out of the presenter as a first step.

[jgsmith-usds]

I'd rather not store it in an array when we can derive the information from the configuration. It's begging to get out of sync. Besides, I'm trying to make some kind of progress on refactoring the 2fa support into a modular system. I don't want to make things any more confusing than they already are.

[monfresh]

What will get out of sync? The array that I'm proposing is for storing the options that the user has configured so far, not the total possible options, which I would assume would be defined as a constant.

Now that I see what feature we want to implement, I think the design we have works fine. We got rid of a lot of the complexity in generating the links to the alternative options, which is great. I love the single link that points to a dedicated page that displays which options are available.

Each option points to a specific controller, which points to a specific Form Object for setting up that option. What isn't working with this design?

The only remaining complexity is the logic that determines which options to display, and I think that storing the options that the user has configured so far in an array would be the least complex solution. It would look something like this:

POSSIBLE_OPTIONS = %w[sms voice auth_app piv_cac].freeze # this will only change when we add new options

already_configured_options = current_user.configured_2fa_options
available_options = POSSIBLE_OPTIONS - already_configured_options
rule = UserEligibleForPivCacRule.new(user: user, service_provider: service_provider)
available_options << 'piv_cac' if rule.satisfied?

class UserEligibleForPivCacRule
  def initialize(user:, service_provider:)
    @user = user
    @service_provider = service_provider
  end
  
  def satisfied?
    return false if user_already_has_piv_cac?
    user.piv_cac_available? || service_provider&.piv_cac_available?
  end
  
  private

  def user_already_has_piv_cac?
    user.configured_2fa_options.include?('piv_cac')
  end
end

[stevegsa]

This code looks great. When you mentioned array fields I thought you meant bit fields which are crappy to work with. The array fields in Postgres look pretty cool. I didn't realize that you could query them with straight SQL although it's non-standard SQL.

[stevegsa]

I would still prefer full blown tables with one table defining the 2fa types and their capabilities and another table as a one to many on users specifying which types the user has.

[jgsmith-usds]

Modifying the database and refactoring our 2fa system is out of scope for this PR.

[monfresh]

I know that in American English, "alternate" can also be used to mean "available as another choice", but I prefer to use "alternative", because that can only mean one thing, whereas the main definition of "alternate" is "every other".

[stevegsa]

How about 'different'?

[monfresh]

How about choose_another_security_option to mimic the text on the screen?

[monfresh]

I know a lot of these were here before, but maybe we can use this as an opportunity to move them somewhere else because these are not related to Devise. There is a bug currently in our i18n-tasks config where all Devise translations are being ignored, and so the specs will never tell us if we have missing or unused specs in this file. I noticed this a while back and haven't gotten around to fixing it. I opened an issue just now so we can track it.

[stevegsa]

Are you sure that is still an issue? I'm pretty sure I culled some unused translations from devise using that script. I agree it should be moved from devise. However, I wasn't happy with the files that partitioned them by function (ie links/en.yml, instructions/en.yml, etc.) I'd prefer the files were scoped at the feature level.

[monfresh]

I'm sure it's an issue. For example, phone_voice_info_html in this file is not used in the app, yet the specs are not flagging it. Try running i18n-tasks unused

[andrewhughey]

Some general thoughts after discussing with @stevegsa:

• let's keep things in scope for the user-facing changes to 2FA selection at sign in. We're trying to bring it into line with the sign up experience

• looks like we forgot to include the remember device checkbox in the new designs. Steve's included here underneath the get new code button. This works for now and we'll revisit with a cleaner design in the next sprint

• we don't need the "add additional 2FA methods" yet. That's extra functionality and missing key features like adding a second phone number. Also for a future sprint and dependent on @jgsmith-usds's modularization work

/login/two_factor/add_methods

• the text on the 2FA descriptions doesn't match our text at account creation, they should really be the same text and use the same string to keep them in sync. It looks like the text here is the most current.

/login/two_factor/list_options (via Choose another security option)

[stevegsa]

Rather than rewrite the feature specs from scratch I decided to satisfy each old requirement one by one to be sure I wasn't missing any old functionality and caught several bugs along the way. It makes it easier to diff but there is some redundancy left in the specs. For example, the fallback links all now map to a single link which goes to the selection screen. We can always cull these later or I can remove them now. I leave it up to the approver.

[jgsmith-usds]

@monfresh These changes are all out of scope for this PR. I'm handling this refactoring in parallel work, as we already discussed.

[monfresh]

I disagree. See my comment in one of your refactoring PRs.

[monfresh]

Minor style comment: Any reason not to put this in a single line without the parentheses?

[stevegsa]

The private attrs had the same format. When in Rome...

[monfresh]

Right, that's because there were too many of them to fit on one line. Following what Rubocop recommends for one-line conditionals, we should keep things on one line if they fit on one line IMO.

[monfresh]

How about 'Multi-Factor Authentication: option submitted'?

[stevegsa]

I was following the format of USER_REGISTRATION_2FA_SETUP

[monfresh]

What do you think of calling this LoginOptionsController, and for the feature for adding additional methods, we'll call it SetupOptionsController? If that sounds like a verb, then maybe OptionsForLoginController and OptionsForSetupController?

[stevegsa]

I wasn't fond of having login in the name since it's already in the path /login/two_factor/XXX and it makes it redundant.

[monfresh]

The path name and controller name don't have to match.

[stevegsa]

I'm also a fan of paths that match their associated components. It makes it easier to find the files.

[stevegsa]

In my other life I developed a routing strategy that doesn't even need the routes file which I believe contributes to the problems of the rails monolith.

[monfresh]

Alternatively, what we could do is rename the module to TwoFactorAuthenticationLogin and then call the controller OptionsController. For setup, the module would be TwoFactorAuthenticationSetup and the controller would also be OptionsController. Or you could have a top-level TwoFactorAuthentication module, and then nested under that would be Login and Setup, and both controllers would be called OptionsController.

[monfresh]

What about options instead of option_list? The latter feels redundant.

[stevegsa]

I do like that name. It was one I considered but maybe it's a bit too short.

[monfresh]

I thought what we've been doing is optimizing the route path names to make sense to users. For example, all our LOA3 endpoints have verify in the path, but internally, we use IdV. Similarly, a path like /login/two_factor/options is very clear. It means these are the options for logging in via two factor. The controller name can be completely different. We have many instances of routes where the path name is different from the controller.

[stevegsa]

I'll change it to 'options'. I do like that name and it's one I considered. With respect to optimizing the paths for users I agree with that strategy: short and clear paths. I'm also a fan of having the scoping of the controllers exactly match that path so we can optimize code manageability for the developers. Otherwise, we are introducing one level of indirection unnecessarily.

[monfresh]

We're not using these 2 routes in this PR. Can we remove them?

[monfresh]

LGTM