LG-528 Use IdV specific phone otp confirmation by jmhooper · Pull Request #2430 · 18F/identity-idp

jmhooper · 2018-08-14T18:48:11Z

Why: The otp verification controller serves multiple puprose, phone
confirmation during sign up, sign in, edit phone, and idv. As a result
the logic is very complex and error prone. This commit creates a
separate controller specific for the IdV phone OTP confirmation since it
differs a good bit from the other use cases for phone confirmation.

This commit will need to be followed up by another to remove the old
code. I didn't do that here because the diff is already pretty
substantial without it.

Hi! Before submitting your PR for review, and/or before merging it, please
go through the checklists below. These represent the more critical elements
of our code quality guidelines. The rest of the list can be found in
CONTRIBUTING.md

Controllers

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

Database

Unsafe migrations are implemented over several PRs and over several
deploys to avoid production errors. The strong_migrations gem
will warn you about unsafe migrations and has great step-by-step instructions
for various scenarios.
Indexes were added if necessary. This article provides a good overview
of indexes in Rails.
Verified that the changes don't affect other apps (such as the dashboard)
When relevant, a rake task is created to populate the necessary DB columns
in the various environments right before deploying, taking into account the users
who might not have interacted with this column yet (such as users who have not
set a password yet)
Migrations against existing tables have been tested against a copy of the
production database. See LG-228 Make migrations safer and more resilient #2127 for an example when a migration caused deployment
issues. In that case, all the migration did was add a new column and an index to
the Users table, which might seem innocuous.

Encryption

The changes are compatible with data that was encrypted with the old code.

Routes

GET requests are not vulnerable to CSRF attacks (i.e. they don't change
state or result in destructive behavior).

Session

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.

Testing

Tests added for this feature/bug
Prefer feature/integration specs over controller specs
When adding code that reads data, write tests for nil values, empty strings,
and invalid inputs.

**Why**: The otp verification controller serves multiple puprose, phone confirmation during sign up, sign in, edit phone, and idv. As a result the logic is very complex and error prone. This commit creates a separate controller specific for the IdV phone OTP confirmation since it differs a good bit from the other use cases for phone confirmation. This commit will need to be followed up by another to remove the old code. I didn't do that here because the diff is already pretty substantial without it.

**Why**: We started using separate otp verification controllers for MFA and IdV in #2430. This commit is a follow-on to that to remove the code that supported IdV in the MFA controller.

monfresh · 2018-08-17T00:23:32Z

app/controllers/idv/otp_verification_controller.rb

+module Idv
+  class OtpVerificationController < ApplicationController
+    include IdvSession
+    include TwoFactorAuthenticatable


Since we only need a few things from TwoFactorAuthenticatable, what do you think about duplicating them here for the moment, and then once we see where else they are used, we can extract them into separate concerns or service objects?

Yeah, the thing I'm using them for displaying the error screens, which is very tricky w/o the concern. That goes pretty deep and I didn't wanna be ambitious. We can see how it looks though?

monfresh · 2018-08-17T00:30:39Z

app/controllers/idv/otp_delivery_method_controller.rb

@@ -13,11 +13,8 @@ def new; end
    def create
      result = @otp_delivery_selection_form.submit(otp_delivery_selection_params)


Thanks for starting this separation. One thing I noticed that I hadn't before is that we probably shouldn't be using the OtpDeliverySelectionForm here. Most of the logic only applies to 2FA in the authentication context. For example, we don't want to risk changing the user's OTP delivery preference, and the check for unsupported_phone? is redundant because the phone was already validated in the previous step in IdV::PhoneForm.

I can build out a IdV specific form here, though I think we may want to open another PR for that to keep this one small.

Did this here: https://github.com/18F/identity-idp/pull/2440/files

monfresh · 2018-08-17T14:37:10Z

app/controllers/idv/otp_verification_controller.rb

+    before_action :set_code, only: %i[show update]
+    before_action :set_otp_verification_presenter, only: %i[show update]
+
+    def new


What do you think about creating a separate controller for sending the OTP and using create as the method? Seeing new for a non-user facing page keeps tripping me up. Ideally, sending the OTP would be a POST request.

It gets a touch tricky since you can't redirect via a POST request. That said, we can change it up so the delivery method selection controller sends the initial OTP and this controller is used for resending. Didn't do that here since a GET request is the status quo and I didn't wanna take on too much at once, but I can if we think it is important.

Moving the code to send the OTP to a service object will also make this a bit easier

yeah, that's what I was thinking. I'll leave it up to you. Eventually, my hope is that we replace the "get another code" link with a link to go back to the delivery method page and choose another way to send the OTP (or try the same method again).

Yeah, I'm thinking a service object to send the code that we use in the otp delivery preference controller and then in a new OtpResendController will come out a bit neater. Then the OtpVerification controller can focus on verifying OTPs.

monfresh · 2018-08-17T14:38:57Z

app/forms/idv/send_phone_confirmation_otp_form.rb

@@ -0,0 +1,106 @@
+module Idv
+  # :reek:InstanceVariableAssumption
+  class SendPhoneConfirmationOtpForm


Since this is not a user-facing form in the traditional sense, what do you think about making this a service object and dropping the Form from the name?

Yeah, agree. This started it's life as something that made more sense as a form (it took the delivery method as a param like the form on the send_otp endpoint does) but is pretty far removed from that now.

monfresh · 2018-08-17T14:40:25Z

app/forms/idv/send_phone_confirmation_otp_form.rb

+
+    attr_accessor :user, :idv_session, :locale
+
+    validates :otp_delivery_preference, inclusion: { in: %i[sms voice] }


I think we can remove this since the preference was already validated in the previous step.

Yeah, there's some slippery spots in the code that result from trying to validate this. I'll pull them out. I'll probs raise for an invalid delivery preference instead.

monfresh · 2018-08-17T14:53:45Z

app/controllers/idv/otp_verification_controller.rb

+      else
+        redirect_to idv_otp_delivery_method_url
+      end
+    end


We will also need to rescue any Twilio errors, like in the main 2FA controller in case the user enters a phone Twilio doesn't like, and so we can display an appropriate message.

Hmm, interesting. In the main 2FA controllers, the user is shown the errors on the phone entry screen. Here is would appear on the OTP delivery method selection screen. I wonder what the best way to deal with that is...

monfresh · 2018-08-17T15:07:48Z

app/services/idv/phone_confirmation_otp_generator.rb

@@ -0,0 +1,8 @@
+module Idv
+  class PhoneConfirmationOtpGenerator
+    def self.generate_otp


What do you think about calling this method call for consistency with other service objects, and for less redundancy with the class name?

I don't mind

Or maybe use a verb for the class name: GeneratePhoneConfirmationOtp?

monfresh · 2018-08-17T15:13:19Z

app/forms/idv/send_phone_confirmation_otp_form.rb

+      parsed_phone = Phonelib.parse(phone)
+      {
+        otp_delivery_preference: otp_delivery_preference,
+        country_code: parsed_phone.country_code,


These should all be US numbers, but just in case, can we use country instead of country_code? The former will return the 2-letter abbreviation of the country, whereas the latter will be the country dialing code, and since non-US countries also use 1, it won't be easy to tell if it was a US number or not.

Yup, good call

monfresh · 2018-08-17T16:49:56Z

app/services/analytics.rb

  IDV_PHONE_CONFIRMATION_VENDOR = 'IdV: phone confirmation vendor'.freeze
+  IDV_PHONE_CONFIRMATION_OTP_SENT = 'IdV: phone confirmation otp sent'.freeze
+  IDV_PHONE_CONFIRMATION_OTP_SUBMITTED = 'IdV: phone confirmation otp submitted'.freeze
+  IDV_PHONE_CONFIRMATION_OTP_VISIT = 'IdV: phone confirmation otp visitted'.freeze


s/visitted/visited

monfresh · 2018-08-31T17:26:29Z

app/controllers/concerns/idv/phone_otp_rate_limitable.rb

+    end
+
+    def handle_too_many_otp_attempts
+      analytics.track_event(Analytics::IDV_PHONE_CONFIRMATION_OTP_RATE_LIMIT_ATTEMTS)


s/ATTEMTS/ATTEMPTS

monfresh · 2018-08-31T18:28:25Z

app/services/analytics.rb

  IDV_JURISDICTION_FORM = 'IdV: jurisdiction form submitted'.freeze
  IDV_PHONE_CONFIRMATION_FORM = 'IdV: phone confirmation form'.freeze
  IDV_PHONE_CONFIRMATION_VENDOR = 'IdV: phone confirmation vendor'.freeze
+  IDV_PHONE_CONFIRMATION_OTP_RATE_LIMIT_ATTEMPTS = 'Idv: Phone OTP attempts rate limitted'.freeze


s/limitted/limited

monfresh · 2018-08-31T18:28:31Z

app/services/analytics.rb

  IDV_PHONE_CONFIRMATION_VENDOR = 'IdV: phone confirmation vendor'.freeze
+  IDV_PHONE_CONFIRMATION_OTP_RATE_LIMIT_ATTEMPTS = 'Idv: Phone OTP attempts rate limitted'.freeze
+  IDV_PHONE_CONFIRMATION_OTP_RATE_LIMIT_LOCKED_OUT = 'Idv: Phone OTP rate limited user'.freeze
+  IDV_PHONE_CONFIRMATION_OTP_RATE_LIMIT_SENDS = 'Idv: Phone OTP sends rate limitted'.freeze


s/limitted/limited

monfresh · 2018-08-31T18:47:13Z

app/services/idv/send_phone_confirmation_otp.rb

+module Idv
+  # Ignore instance variable assumption on @user_locked_out :reek:InstanceVariableAssumption
+  class SendPhoneConfirmationOtp
+    attr_accessor :user, :idv_session, :locale


Since these methods are not meant to be writable outside of the class, what do you think about replacing this with a private attr_reader, and using @user = user etc. in the initialize method?

jmhooper · 2018-08-31T18:54:46Z

@monfresh: I just pushed some commits that I think address everything you've mentioned

monfresh · 2018-08-31T18:58:18Z

app/views/idv/otp_verification/show.html.slim

+
+.mt3.border-top
+  .mt1
+    = link_to t('links.cancel'), idv_cancel_path


It looks like this has changed the UI a bit compared to what's on master. Here is what's on master:

And here is what's on this PR:

Perhaps we should ask @donjo and @rtwell for design feedback?

Should we keep the "entered the wrong number" text and link and add the new "choose another option" under that?

jmhooper · 2018-08-31T20:11:01Z

I just pushed a commit to put the view back the way it was

monfresh · 2018-08-31T23:53:50Z

app/controllers/idv/otp_delivery_method_controller.rb

      @otp_delivery_selection_form ||= Idv::OtpDeliveryMethodForm.new
    end
+
+    def invalid_phone_number(exception)


Thanks! I think we also want to copy over the capture_analytics_for_exception(exception) method to track these errors.

monfresh

LGTM % twilio error analytics

rtwell · 2018-09-04T12:17:57Z

if it matters, this is an issue: https://cm-jira.usa.gov/browse/LG-618 i'd love to remove that *Get another code* button and make it hierarchically equal to *Use a different number* plus, it doesnt look like a game of tetris. 🎉

…

On Fri, Aug 31, 2018 at 8:25 PM, Moncef Belyamani ***@***.***> wrote: ***@***.**** approved this pull request. LGTM % twilio error analytics — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2430 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AL-58a_sO00EzIwTB_hHKWw8hGp5UISSks5uWdQVgaJpZM4V8-va> .

jmhooper · 2018-09-04T16:02:47Z

@monfresh: I just updated against master and added the twilio error analytics events. Mind taking another look?

monfresh

LGTM

Why: We started using separate otp verification controllers for MFA and IdV in #2430. This commit is a follow-on to that to remove the code that supported IdV in the MFA controller.

jmhooper added the status - work in progress label Aug 14, 2018

jmhooper added 4 commits August 14, 2018 13:49

a little bit of cleanup

898517a

Add otp verification form coverage

e3d0e18

add coverage for controllers and analytics

6ec8d59

more controller coverage

142e638

jmhooper added status - ready for review and removed status - work in progress labels Aug 14, 2018

write tests for send otp form

8597231

jmhooper requested review from gregory-casamento, jgsmith-usds, monfresh, mryenq and stevegsa August 14, 2018 23:38

jmhooper mentioned this pull request Aug 15, 2018

Prune IdV branches from MFA otp verification logic #2437

Closed

12 tasks

monfresh reviewed Aug 17, 2018

View reviewed changes

jmhooper mentioned this pull request Aug 17, 2018

Add separate OtpDeliveryMethodForm for IdV #2440

Merged

12 tasks

jmhooper added status - work in progress and removed status - ready for review labels Aug 17, 2018

jmhooper added 2 commits August 17, 2018 15:32

Use a service class to send otps

d2083e5

Send OTPs from the otp delivery method controllers

110b8b0

monfresh reviewed Aug 31, 2018

View reviewed changes

jmhooper added 2 commits August 31, 2018 12:57

fix country code

f4c1b4f

fix analytics typo

32e61b2

monfresh reviewed Aug 31, 2018

View reviewed changes

jmhooper added 4 commits August 31, 2018 13:49

rescue twilio errors

70b6d5d

analytics typos

56311e1

make method in send phone confirmation otp private

09e1a39

rename phone confirmation otp generator service class

1c3eeb9

monfresh reviewed Aug 31, 2018

View reviewed changes

fix view to look like it used to

a699b69

oops, broke the tests

d811947

monfresh reviewed Aug 31, 2018

View reviewed changes

monfresh previously approved these changes Sep 1, 2018

View reviewed changes

jmhooper added 2 commits September 4, 2018 08:42

Merge branch 'master' into jmhooper-idv-specific-otp-verification

3cd2d4b

Add analytics for twilio errors

ed11743

jmhooper dismissed monfresh’s stale review via ed11743 September 4, 2018 16:02

monfresh reviewed Sep 5, 2018

View reviewed changes

jmhooper merged commit 635df15 into master Sep 5, 2018

jmhooper mentioned this pull request Sep 5, 2018

Prune IdV branches from MFA otp verification logic #2489

Merged

12 tasks

jmhooper deleted the jmhooper-idv-specific-otp-verification branch February 15, 2019 19:15

aduth mentioned this pull request Jun 3, 2022

Remove no-JavaScript supports for identity proofing #6423

Merged

mitchellhenke mentioned this pull request Aug 4, 2023

Remove expensive remembered device test #8941

Merged

		@@ -13,11 +13,8 @@ def new; end
		def create
		result = @otp_delivery_selection_form.submit(otp_delivery_selection_params)


		attr_accessor :user, :idv_session, :locale

		validates :otp_delivery_preference, inclusion: { in: %i[sms voice] }

Conversation

jmhooper commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Controllers

Database

Encryption

Routes

Session

Testing

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmhooper commented Aug 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmhooper commented Aug 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfresh left a comment

Choose a reason for hiding this comment

Uh oh!

rtwell commented Sep 4, 2018 via email

Uh oh!

jmhooper commented Sep 4, 2018

Uh oh!

monfresh left a comment

Choose a reason for hiding this comment

Uh oh!

jmhooper commented Aug 14, 2018 •

edited

Loading