Make OTP delivery independent of 2FA gem (#452)

**Why**: We should not have to modify the
`send_two_factor_authentication_code` method in the
`two_factor_authentication` gem in order to accommodate multiple
delivery mechanisms. This violates the "O" in the SOLID object-oriented
design principles. The "O" stands for Open for extension, Closed
for modification.

**How**:
We can work around the gem's undesirable design choices by making a few
sensible changes:

- Make the `send_two_factor_authentication_code` method a no-op method,
i.e. it doesn't do anything. This means that even though the gem calls
this method right after Warden signs the user in, nothing will happen,
which is what we want.

- Instead of relying on the `send_two_factor_authentication_code`
method and the `UserOtpSender` class to invoke the proper background
job, we do this work in the controller, and use the delivery method
chosen by the user to determine which job to invoke.

This allows us to continue our work without having to fork the gem or
wait for our PRs to be approved and merged. We can work on our code
the way we want and propose changes to the gem in parallel, without
one depending on the other.

Note that these are the smallest changes I could come up with to remove
the dependency on Amos's fork, without changing behavior. Due to
preexisting duplication, new duplication was introduced, but it will be
cleaned up in subsequent PRs, which are part of a larger refactor I am
working on.

Gemfile

@@ -41,9 +41,7 @@ gem 'sinatra', require: false
 gem 'slim-rails'
 gem 'split', require: 'split/dashboard'
 gem 'twilio-ruby'
-# TODO(amos): Unfork this gem: https://github.com/18F/identity-private/issues/708
-gem 'two_factor_authentication', github: 'amoose/two_factor_authentication',
-                                 branch: 'feature/more_options'
+gem 'two_factor_authentication', github: 'Houdini/two_factor_authentication', ref: '1d6abe3'
 gem 'uglifier', '>= 1.3.0'
 gem 'whenever', require: false
 gem 'xmlenc', '~> 0.6.4'

Gemfile.lock

@@ -7,9 +7,9 @@ GIT
       dotenv
 
 GIT
-  remote: https://github.com/amoose/two_factor_authentication.git
-  revision: 58521f467ab0e3a8b02342f9d7ae11bbf9afdb14
-  branch: feature/more_options
+  remote: https://github.com/Houdini/two_factor_authentication.git
+  revision: 1d6abe30a61c849863d6ec3f6f9e44f978d7b181
+  ref: 1d6abe3
   specs:
     two_factor_authentication (1.1.5)
       devise
@@ -410,7 +410,7 @@ GEM
     request_store (1.3.1)
     responders (2.2.0)
       railties (>= 4.2.0, < 5.1)
-    rotp (3.1.0)
+    rotp (3.2.0)
     rqrcode (0.10.1)
       chunky_png (~> 1.0)
     rspec (3.4.0)

app/controllers/concerns/otp_delivery_fallback.rb

@@ -29,11 +29,9 @@ def sms_enabled?
 
   def current_otp_method
     query_method = params[:otp_method]
-    query_method.to_sym if
-      %w(sms voice totp).include? query_method
-  end
 
-  def use_sms_or_voice_otp_delivery?
-    %i(sms voice).include? current_otp_method
+    return :sms unless %w(sms voice totp).include? query_method
+
+    query_method.to_sym
   end
 end

app/controllers/concerns/phone_confirmation_flow.rb

@@ -72,11 +72,10 @@ def send_confirmation_code
     # user's session. Re-sending the confirmation code doesn't generate a new one.
     self.confirmation_code = generate_confirmation_code unless confirmation_code
 
-    if current_otp_method == :voice
-      VoiceSenderOtpJob.perform_later(confirmation_code, unconfirmed_phone)
-    else
-      SmsSenderOtpJob.perform_later(confirmation_code, unconfirmed_phone)
-    end
+    job = "#{current_otp_method.to_s.capitalize}SenderOtpJob".constantize
+
+    job.perform_later(confirmation_code, unconfirmed_phone)
+
     flash[:success] = t("notices.send_code.#{current_otp_method}")
   end
 
@@ -117,7 +116,9 @@ def sms_enabled?
 
   def current_otp_method
     query_method = params[:otp_method]
-    query_method.to_sym if
-      %w(sms voice totp).include? query_method
+
+    return :sms unless %w(sms voice totp).include? query_method
+
+    query_method.to_sym
   end
 end

app/controllers/devise/two_factor_authentication_controller.rb

@@ -19,7 +19,7 @@ def new
     end
 
     def show
-      if use_totp?
+      if current_user.totp_enabled?
         show_totp_prompt
       else
         @phone_number = user_decorator.masked_two_factor_phone_number
@@ -53,12 +53,6 @@ def check_already_authenticated
       redirect_to profile_path if user_fully_authenticated?
     end
 
-    def use_totp?
-      # Present the TOTP entry screen to users who are TOTP enabled,
-      # unless the user explictly selects SMS or voice
-      current_user.totp_enabled? && !use_sms_or_voice_otp_delivery?
-    end
-
     def verify_user_is_not_second_factor_locked
       handle_second_factor_locked_resource if user_decorator.blocked_from_entering_2fa_code?
     end
@@ -132,7 +126,12 @@ def prompt_for_otp_reentry
     end
 
     def send_user_otp
-      current_user.send_new_otp(otp_method: current_otp_method)
+      current_user.create_direct_otp
+
+      job = "#{current_otp_method.to_s.capitalize}SenderOtpJob".constantize
+
+      job.perform_later(current_user.direct_otp, current_user.phone)
+
       show_direct_otp_prompt
     end
 

app/models/user.rb

@@ -30,8 +30,19 @@ def two_factor_enabled?
     phone.present?
   end
 
-  def send_two_factor_authentication_code(code, options = {})
-    UserOtpSender.new(self).send_otp(code, options)
+  def send_two_factor_authentication_code(_code)
+    # The two_factor_authentication gem assumes that if a user needs to receive
+    # a code, the code should be automatically sent right after Warden signs
+    # the user in by calling this method. However, we don't want a code to be
+    # automatically sent until the user has reached the TwoFactorAuthenticationController,
+    # where we prompt them to select how they would like to receive the OTP code.
+    # Sending an OTP code is not a User responsibility. It belongs either in the
+    # controller, or in a dedicated class that the controller sends messages to.
+    # Based on the delivery method chosen by the user, the controller calls the
+    # appropriate background job to send the code, such as SmsSenderOtpJob.
+    #
+    # Hence, we define this method as a no-op method, meaning it doesn't do anything.
+    # See https://github.com/18F/identity-idp/pull/452 for more details.
   end
 
   def confirmation_period_expired?

config/initializers/devise.rb

@@ -268,5 +268,4 @@
   config.otp_length = 6
   config.direct_otp_length = 6
   config.direct_otp_valid_for = 5.minutes
-  config.enable_auto_send_otp = false # disable auto-sending of OTP after authentication
 end

spec/controllers/users/sessions_controller_spec.rb

@@ -107,14 +107,6 @@
   end
 
   describe 'POST /' do
-    it 'does not call User#send_two_factor_authentication_code' do
-      create(:user, :signed_up, email: 'user@example.com')
-
-      expect_any_instance_of(User).to_not receive(:send_two_factor_authentication_code)
-
-      post :create, user: { email: 'user@example.com', password: '!1aZ' * 32 }
-    end
-
     it 'tracks the authentication for existing user' do
       user = create(:user, :signed_up)
 

spec/models/user_spec.rb

@@ -177,18 +177,6 @@
     end
   end
 
-  describe '#send_two_factor_authentication_code' do
-    it 'calls UserOtpSender#send_otp' do
-      user = build_stubbed(:user)
-      otp_sender = instance_double(UserOtpSender)
-
-      expect(UserOtpSender).to receive(:new).with(user).and_return(otp_sender)
-      expect(otp_sender).to receive(:send_otp).with(123, {})
-
-      user.send_two_factor_authentication_code(123)
-    end
-  end
-
   describe 'OTP length' do
     it 'uses Devise setting when set' do
       allow(Devise).to receive(:direct_otp_length).and_return(10)