LG-162 - Offer multiple 2FA options during account creation by line47 · Pull Request #2099 · 18F/identity-idp

line47 · 2018-04-13T18:34:15Z

Why: So that users can use phone(voice), text/SMS or an authenticator app to 2FA.

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.

line47 · 2018-04-16T12:00:56Z

would love some help evaluating and fixing these tests.
@jmhooper @monfresh @davemcorwin are any of you available to take a look with me?

monfresh · 2018-04-16T13:44:33Z

app/controllers/users/two_factor_authentication_setup_controller.rb

      analytics.track_event(Analytics::USER_REGISTRATION_PHONE_SETUP_VISIT)
    end

+    def tfa


It's not obvious from looking at this controller, but our convention is to use the standard create, index, show actions for controllers as opposed to custom names. If you find yourself needing a custom name, that's an indication that a new controller is needed.

Since we're making modifications to this controller, what do you think about cleaning it up at the same time? Since we are now introducing a new page where the user can choose from various types of 2FA options, I propose that we keep this controller to represent that options page, and then add a new controller for the phone setup option. So, we would rename the tfa method in this controller to show, and tfa_set to create. Then, in the new phone_setup_controller, we would have a show method for displaying the phone setup form, and a create action for processing the form submission.

That way, we keep things consistent across all our controllers.

line47 · 2018-04-30T13:22:46Z

Hey @jmhooper @monfresh @davemcorwin - I have one more codeclimate issue I'm unsure how to fix.

I also need to send the user to the the two factor options page in the case where they verify email, setup a password, then logout - when they sign back in it's currently going to the phone_setup page and I want it to go back to the two_factor_options page.

After that I think this PR is ready for review 😬 🎉

monfresh · 2018-04-30T14:40:57Z

app/forms/user_phone_form.rb

    )
-    self.otp_delivery_preference = params[:otp_delivery_preference]
+    self.otp_delivery_preference = params[:otp_delivery_preference] if
+      params[:otp_delivery_preference]


What Reek wants you to do here is to assign params[:otp_delivery_preference] to a variable and call the variable instead of calling params[:otp_delivery_preference] twice.

It's not obvious from Code Climate's website, but there is a help section that explains what the offense is and how to fix it. From the GitHub PR, if you click on "Details" next to the Code Climate check, it will take you to the Code Climate website with a list of all the issues. If you hover your cursor over the issue, a set of 3 icons will appear on the right. The one in the far left looks like a book, and if you hover over it it will say "Read up". If you click on it, it will display a modal explaining what the issue is and how to fix it. Here's a screenshot:

Something like this @monfresh ?

tfa_prefs = 'params[:otp_delivery_preference]' self.otp_delivery_preference = tfa_prefs if params[:otp_delivery_preference]

Almost:

tfa_prefs = params[:otp_delivery_preference] self.otp_delivery_preference = tfa_prefs if tfa_prefs

The point is to parse the params only once.

monfresh · 2018-04-30T19:05:42Z

Looks like there are some slim-lint offenses:

/home/circleci/identity-idp/app/views/users/phone_setup/index.html.slim:20 [W] TrailingWhitespace: Line contains trailing whitespace
/home/circleci/identity-idp/app/views/users/phone_setup/index.html.slim:23 [W] TrailingWhitespace: Line contains trailing whitespace
/home/circleci/identity-idp/app/views/users/phone_setup/index.html.slim:25 [W] LineLength: Line is too long. [140/100]
/home/circleci/identity-idp/app/views/users/totp_setup/new.html.slim:49 [W] EmptyLines: Extra empty line detected
/home/circleci/identity-idp/app/views/users/two_factor_authentication_setup/index.html.slim:13 [W] LineLength: Line is too long. [108/100]
/home/circleci/identity-idp/app/views/users/two_factor_authentication_setup/index.html.slim:21 [W] RedundantDiv: `div` is redundant when class attribute shortcut is present
/home/circleci/identity-idp/app/views/users/two_factor_authentication_setup/index.html.slim:30 [W] RedundantDiv: `div` is redundant when class attribute shortcut is present
/home/circleci/identity-idp/app/views/users/two_factor_authentication_setup/index.html.slim:39 [W] RedundantDiv: `div` is redundant when class attribute shortcut is present
/home/circleci/identity-idp/app/views/users/two_factor_authentication_setup/index.html.slim:51 [W] TrailingWhitespace: Line contains trailing whitespace

jmhooper · 2018-04-30T19:53:41Z

spec/controllers/users/phone_setup_controller_spec.rb

        user_phone_form: {
          phone: '703-555-010',
-          otp_delivery_preference: :sms,
+          # otp_delivery_preference: :sms,


We should remove this line instead of commenting it out. Same for the other places this is commented out.

jmhooper · 2018-04-30T19:55:38Z

app/controllers/users/phone_setup_controller.rb

+
+    def create
+      @user_phone_form = UserPhoneForm.new(current_user)
+      result = @user_phone_form.submit(params[:user_phone_form])


Any chance we could pull in strong parameters here to make sure we are only permitting the params we want?

jmhooper · 2018-04-30T19:56:09Z

app/controllers/users/two_factor_authentication_setup_controller.rb

-      analytics.track_event(Analytics::MULTI_FACTOR_AUTH_PHONE_SETUP, result.to_h)
+    def create
+      @two_factor_options_form = TwoFactorOptionsForm.new(current_user)
+      result = @two_factor_options_form.submit(params[:two_factor_options_form])


Same note here about permitting params.

jmhooper · 2018-04-30T19:57:01Z

app/controllers/users/two_factor_authentication_setup_controller.rb

+    def create
+      @two_factor_options_form = TwoFactorOptionsForm.new(current_user)
+      result = @two_factor_options_form.submit(params[:two_factor_options_form])
+      # analytics.track_event(Analytics::USER_REGISTRATION_PHONE_SETUP_VISIT)


We have some dead code here we may want to revisit. We're going to need to make sure we are tracking the analytics event here.

Related, we'll probably want to add a controller spec to make sure this is getting tracked appropriately.

jmhooper · 2018-04-30T19:57:29Z

app/forms/two_factor_options_form.rb

@@ -0,0 +1,40 @@
+class TwoFactorOptionsForm


I don't see any specs covering this class

line47 · 2018-05-04T13:43:00Z

I'm getting 2 fails in CicrcleCI I'm unsure how to fix. @monfresh @jmhooper @davemcorwin can you help here? https://circleci.com/gh/18F/identity-idp/2638?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

After that would be good for some review passes 👍

monfresh · 2018-05-04T14:19:42Z

@line47 If you look at the test failure reason, you will see that it is expecting to land on the phone setup page but is landing on the new 2FA setup page. I would go through each spec and see what the expected page should be and then either change the spec to match the new behavior, or change the code to make the spec pass if the spec is still valid with the new behavior.

line47 · 2018-05-04T15:03:38Z

Thanks @monfresh - I've updated those tests and still have some i18n_spec.rb fails I'm unsure how to fix.

Internationalization visit homepage without a locale param set allows user to manually toggle language from dropdown menu
     Failure/Error:
       within(:css, '.i18n-desktop-toggle') do
         click_link t('i18n.language', locale: 'en')
         click_link t('i18n.locale.es')
       end
     
     Capybara::ElementNotFound:
       Unable to find visible css ".i18n-desktop-toggle"
     # ./spec/features/visitors/i18n_spec.rb:56:in `block (4 levels) in <top (required)>'
     # ./spec/features/visitors/i18n_spec.rb:55:in `block (3 levels) in <top (required)>'

  18n does not have missing keys
     Failure/Error:
       expect(missing_keys).to(
         be_empty,
         "Missing #{missing_keys.leaves.count} i18n keys, run `i18n-tasks missing' to show them"
       )
     
       Missing 2 i18n keys, run `i18n-tasks missing' to show them
     # ./spec/i18n_spec.rb:12:in `block (2 levels) in <top (required)>'

I18n does not have keys with missing interpolation arguments
     Failure/Error: translation.scan(I18n::INTERPOLATION_PATTERN).map(&:compact).map(&:first).to_set
     
     NoMethodError:
       undefined method `scan' for nil:NilClass
     # ./spec/i18n_spec.rb:86:in `extract_interpolation_arguments'
     # ./spec/i18n_spec.rb:32:in `block (4 levels) in <top (required)>'
     # ./spec/i18n_spec.rb:31:in `map'
     # ./spec/i18n_spec.rb:31:in `block (3 levels) in <top (required)>'
     # ./spec/i18n_spec.rb:28:in `block (2 levels) in <top (required)>'

monfresh · 2018-05-04T18:59:50Z

The middle one is due to the app referencing some localized text that isn't present in the config/locale files. If you look at the error message in the middle one, it says to run i18n-tasks missing. That will show you all missing translations.

davemcorwin · 2018-06-01T22:27:30Z

app/assets/stylesheets/components/_space-addon.scss

@@ -1,3 +1,6 @@
+.mb-half { margin-bottom: 4px; }


are these the same as .mt-tiny and .mb-tiny?

yeah, mb-tiny and mt-tiny are .25rem which equates to 4px, so these can be removed.

I took care of this (along with other things) here: f92628b

More fixes will be coming later. The cancellation links, for example, aren't set up properly.

**Why**: To give users more choice during account creation. The current options are: SMS, Voice, Authentication app. PIV/CAC will be added later in the menu, although it is already available if you know the URL.

monfresh · 2018-06-07T18:45:27Z

@jmhooper @davemcorwin This is rebased and ready to go. One last look please?

davemcorwin

LGTM

line47 added the status - work in progress label Apr 13, 2018

monfresh reviewed Apr 16, 2018

View reviewed changes

line47 changed the title ~~[WIP] - Offer multiple 2FA options during account creation~~ Offer multiple 2FA options during account creation Apr 30, 2018

line47 removed the status - work in progress label Apr 30, 2018

line47 requested review from davemcorwin and jmhooper April 30, 2018 13:23

monfresh reviewed Apr 30, 2018

View reviewed changes

jmhooper reviewed Apr 30, 2018

View reviewed changes

app/forms/two_factor_options_form.rb

@@ -0,0 +1,40 @@

class TwoFactorOptionsForm

Copy link

Contributor

jmhooper Apr 30, 2018

Choose a reason for hiding this comment

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

I don't see any specs covering this class

line47 changed the title ~~Offer multiple 2FA options during account creation~~ LG-162 - Offer multiple 2FA options during account creation May 2, 2018

line47 self-assigned this May 4, 2018

line47 added the status - work in progress label May 9, 2018

line47 requested a review from mryenq June 1, 2018 20:35

line47 added status - ready for review and removed status - work in progress labels Jun 1, 2018

davemcorwin reviewed Jun 1, 2018

View reviewed changes

monfresh force-pushed the sw-2fa-options branch from 3ecafa0 to 8efba9e Compare June 7, 2018 18:08

LG-162: Multiple 2FA options during registration

14af7bc

**Why**: To give users more choice during account creation. The current options are: SMS, Voice, Authentication app. PIV/CAC will be added later in the menu, although it is already available if you know the URL.

monfresh force-pushed the sw-2fa-options branch from 8efba9e to 14af7bc Compare June 7, 2018 18:44

monfresh unassigned line47 Jun 7, 2018

davemcorwin approved these changes Jun 8, 2018

View reviewed changes

monfresh merged commit 68813cd into master Jun 8, 2018

monfresh deleted the sw-2fa-options branch June 8, 2018 14:52

Conversation

line47 commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

line47 commented Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

line47 commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfresh commented Apr 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

line47 commented May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

monfresh commented May 4, 2018

Uh oh!

line47 commented May 4, 2018

Uh oh!

monfresh commented May 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfresh commented Jun 7, 2018

Uh oh!

davemcorwin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

line47 commented Apr 13, 2018 •

edited

Loading

line47 commented Apr 16, 2018 •

edited

Loading

line47 commented Apr 30, 2018 •

edited

Loading

line47 commented May 4, 2018 •

edited

Loading