2FA controller cleanup (#2296)

* Add spec coverage for settings/two_factor_auth area

* extract setup method for qr code

* Move otp required check to before action

* Merge method only used once

* Remove duplicate view

* Consolidate creation of @codes for backup

* Move settings/2fq#recovery_codes to settings/recovery_codes#create

* Rename settings/two_factor_auth#disable to #destroy

* Add coverage for the otp required path on 2fa#show

* Clean up the recovery codes list styles

* Move settings/two_factor_auth to settings/two_factor_authentication

* Reorganize the settings two factor auth area

Updated to use a flow like:

- settings/two_factor_authentication goes to a #show view which has a button
  either enable or disable 2fa on the account
- the disable button turns off the otp requirement for the user
- the enable button cycles the user secret and redirects to a confirmation page
- the confirmation page is a #new view which shows the QR code for user
- that page posts to #create which verifies the code, and creates the recovery
  codes
- that create action shares a view with a recovery codes controller which can be
  used separately to reset codes if needed

14 files changed, 153 insertions, 106 deletions

diff --git a/app/assets/stylesheets/lists.scss b/app/assets/stylesheets/lists.scss
index 9cfc46ecf..47805663f 100644
--- a/app/assets/stylesheets/lists.scss
+++ b/app/assets/stylesheets/lists.scss
@@ -9,4 +9,12 @@
 
 .recovery-codes {
   list-style: none;
+  margin: 0 auto;
+  text-align: center;
+
+  li {
+    font-size: 125%;
+    line-height: 1.5;
+    letter-spacing: 1px;
+  }
 }
diff --git a/app/controllers/settings/two_factor_authentication/confirmations_controller.rb b/app/controllers/settings/two_factor_authentication/confirmations_controller.rb
new file mode 100644
index 000000000..4cf62db13
--- /dev/null
+++ b/app/controllers/settings/two_factor_authentication/confirmations_controller.rb
@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+module Settings
+  module TwoFactorAuthentication
+    class ConfirmationsController < ApplicationController
+      layout 'admin'
+
+      before_action :authenticate_user!
+
+      def new
+        prepare_two_factor_form
+      end
+
+      def create
+        if current_user.validate_and_consume_otp!(confirmation_params[:code])
+          flash[:notice] = I18n.t('two_factor_authentication.enabled_success')
+
+          current_user.otp_required_for_login = true
+          @recovery_codes = current_user.generate_otp_backup_codes!
+          current_user.save!
+
+          render 'settings/two_factor_authentication/recovery_codes/index'
+        else
+          flash.now[:alert] = I18n.t('two_factor_authentication.wrong_code')
+          prepare_two_factor_form
+          render :new
+        end
+      end
+
+      private
+
+      def confirmation_params
+        params.require(:form_two_factor_confirmation).permit(:code)
+      end
+
+      def prepare_two_factor_form
+        @confirmation = Form::TwoFactorConfirmation.new
+        @provision_url = current_user.otp_provisioning_uri(current_user.email, issuer: Rails.configuration.x.local_domain)
+        @qrcode = RQRCode::QRCode.new(@provision_url)
+      end
+    end
+  end
+end
diff --git a/app/controllers/settings/two_factor_authentication/recovery_codes_controller.rb b/app/controllers/settings/two_factor_authentication/recovery_codes_controller.rb
new file mode 100644
index 000000000..e591e9502
--- /dev/null
+++ b/app/controllers/settings/two_factor_authentication/recovery_codes_controller.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+module Settings
+  module TwoFactorAuthentication
+    class RecoveryCodesController < ApplicationController
+      layout 'admin'
+
+      before_action :authenticate_user!
+
+      def create
+        @recovery_codes = current_user.generate_otp_backup_codes!
+        current_user.save!
+        flash[:notice] = I18n.t('two_factor_authentication.recovery_codes_regenerated')
+        render :index
+      end
+    end
+  end
+end
diff --git a/app/controllers/settings/two_factor_authentications_controller.rb b/app/controllers/settings/two_factor_authentications_controller.rb
new file mode 100644
index 000000000..f66c3a908
--- /dev/null
+++ b/app/controllers/settings/two_factor_authentications_controller.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+module Settings
+  class TwoFactorAuthenticationsController < ApplicationController
+    layout 'admin'
+
+    before_action :authenticate_user!
+    before_action :verify_otp_required, only: [:create]
+
+    def show; end
+
+    def create
+      current_user.otp_secret = User.generate_otp_secret(32)
+      current_user.save!
+      redirect_to new_settings_two_factor_authentication_confirmation_path
+    end
+
+    def destroy
+      current_user.otp_required_for_login = false
+      current_user.save!
+      redirect_to settings_two_factor_authentication_path
+    end
+
+    private
+
+    def verify_otp_required
+      redirect_to settings_two_factor_authentication_path if current_user.otp_required_for_login?
+    end
+  end
+end
diff --git a/app/controllers/settings/two_factor_auths_controller.rb b/app/controllers/settings/two_factor_auths_controller.rb
deleted file mode 100644
index a06b2a9f9..000000000
--- a/app/controllers/settings/two_factor_auths_controller.rb
+++ /dev/null
@@ -1,56 +0,0 @@
-# frozen_string_literal: true
-
-class Settings::TwoFactorAuthsController < ApplicationController
-  layout 'admin'
-
-  before_action :authenticate_user!
-
-  def show; end
-
-  def new
-    redirect_to settings_two_factor_auth_path if current_user.otp_required_for_login
-
-    @confirmation = Form::TwoFactorConfirmation.new
-    current_user.otp_secret = User.generate_otp_secret(32)
-    current_user.save!
-    set_qr_code
-  end
-
-  def create
-    if current_user.validate_and_consume_otp!(confirmation_params[:code])
-      current_user.otp_required_for_login = true
-      @codes = current_user.generate_otp_backup_codes!
-      current_user.save!
-      flash[:notice] = I18n.t('two_factor_auth.enabled_success')
-    else
-      @confirmation = Form::TwoFactorConfirmation.new
-      set_qr_code
-      flash.now[:alert] = I18n.t('two_factor_auth.wrong_code')
-      render :new
-    end
-  end
-
-  def recovery_codes
-    @codes = current_user.generate_otp_backup_codes!
-    current_user.save!
-    flash[:notice] = I18n.t('two_factor_auth.recovery_codes_regenerated')
-  end
-
-  def disable
-    current_user.otp_required_for_login = false
-    current_user.save!
-
-    redirect_to settings_two_factor_auth_path
-  end
-
-  private
-
-  def set_qr_code
-    @provision_url = current_user.otp_provisioning_uri(current_user.email, issuer: Rails.configuration.x.local_domain)
-    @qrcode        = RQRCode::QRCode.new(@provision_url)
-  end
-
-  def confirmation_params
-    params.require(:form_two_factor_confirmation).permit(:code)
-  end
-end
diff --git a/app/views/settings/shared/_links.html.haml b/app/views/settings/shared/_links.html.haml
index 6490ffdd8..0abb5a7ab 100644
--- a/app/views/settings/shared/_links.html.haml
+++ b/app/views/settings/shared/_links.html.haml
@@ -5,6 +5,6 @@
     %li= link_to t('settings.preferences'), settings_preferences_path
   - if controller_name != 'registrations'
     %li= link_to t('auth.change_password'), edit_user_registration_path
-  - if controller_name != 'two_factor_auths'
-    %li= link_to t('settings.two_factor_auth'), settings_two_factor_auth_path
+  - if controller_name != 'two_factor_authentications'
+    %li= link_to t('settings.two_factor_authentication'), settings_two_factor_authentication_path
   %li= link_to t('settings.back'), root_path
diff --git a/app/views/settings/two_factor_authentication/confirmations/new.html.haml b/app/views/settings/two_factor_authentication/confirmations/new.html.haml
new file mode 100644
index 000000000..9d950c78e
--- /dev/null
+++ b/app/views/settings/two_factor_authentication/confirmations/new.html.haml
@@ -0,0 +1,17 @@
+- content_for :page_title do
+  = t('settings.two_factor_authentication')
+
+= simple_form_for @confirmation, url: settings_two_factor_authentication_confirmation_path, method: :post do |f|
+  %p.hint= t('two_factor_authentication.instructions_html')
+
+  .qr-wrapper
+    .qr-code= raw @qrcode.as_svg(padding: 0, module_size: 4)
+
+    .qr-alternative
+      %p.hint= t('two_factor_authentication.manual_instructions')
+      %samp.qr-alternative__code= current_user.otp_secret.scan(/.{4}/).join(' ')
+
+  = f.input :code, hint: t('two_factor_authentication.code_hint'), placeholder: t('simple_form.labels.defaults.otp_attempt')
+
+  .actions
+    = f.button :button, t('two_factor_authentication.enable'), type: :submit
diff --git a/app/views/settings/two_factor_authentication/recovery_codes/index.html.haml b/app/views/settings/two_factor_authentication/recovery_codes/index.html.haml
new file mode 100644
index 000000000..7d409826e
--- /dev/null
+++ b/app/views/settings/two_factor_authentication/recovery_codes/index.html.haml
@@ -0,0 +1,9 @@
+- content_for :page_title do
+  = t('settings.two_factor_authentication')
+
+%p.hint= t('two_factor_authentication.recovery_instructions')
+
+%ol.recovery-codes
+  - @recovery_codes.each do |code|
+    %li<
+      %samp= code
diff --git a/app/views/settings/two_factor_authentications/show.html.haml b/app/views/settings/two_factor_authentications/show.html.haml
new file mode 100644
index 000000000..88b5bd20e
--- /dev/null
+++ b/app/views/settings/two_factor_authentications/show.html.haml
@@ -0,0 +1,26 @@
+- content_for :page_title do
+  = t('settings.two_factor_authentication')
+
+.simple_form
+  %p.hint
+    = t('two_factor_authentication.description_html')
+
+  - if current_user.otp_required_for_login
+    = link_to t('two_factor_authentication.disable'),
+      settings_two_factor_authentication_path,
+      data: { method: :delete },
+      class: 'block-button'
+  - else
+    = link_to t('two_factor_authentication.setup'),
+      settings_two_factor_authentication_path,
+      data: { method: :post },
+      class: 'block-button'
+
+- if current_user.otp_required_for_login
+  .simple_form
+    %p.hint
+      = t('two_factor_authentication.lost_recovery_codes')
+    = link_to t('two_factor_authentication.generate_recovery_codes'),
+      settings_two_factor_authentication_recovery_codes_path,
+      data: { method: :post },
+      class: 'block-button'
diff --git a/app/views/settings/two_factor_auths/_recovery_codes.html.haml b/app/views/settings/two_factor_auths/_recovery_codes.html.haml
deleted file mode 100644
index 054588b97..000000000
--- a/app/views/settings/two_factor_auths/_recovery_codes.html.haml
+++ /dev/null
@@ -1,6 +0,0 @@
-%p.hint= t('two_factor_auth.recovery_instructions')
-
-%ol.recovery-codes
-  - recovery_codes.each do |code|
-    %li
-      %samp= code
diff --git a/app/views/settings/two_factor_auths/create.html.haml b/app/views/settings/two_factor_auths/create.html.haml
deleted file mode 100644
index aaeebaf2e..000000000
--- a/app/views/settings/two_factor_auths/create.html.haml
+++ /dev/null
@@ -1,4 +0,0 @@
-- content_for :page_title do
-  = t('settings.two_factor_auth')
-
-= render 'recovery_codes', recovery_codes: @codes
diff --git a/app/views/settings/two_factor_auths/new.html.haml b/app/views/settings/two_factor_auths/new.html.haml
deleted file mode 100644
index 5bae743ef..000000000
--- a/app/views/settings/two_factor_auths/new.html.haml
+++ /dev/null
@@ -1,17 +0,0 @@
-- content_for :page_title do
-  = t('settings.two_factor_auth')
-
-= simple_form_for @confirmation, url: settings_two_factor_auth_path, method: :post do |f|
-  %p.hint= t('two_factor_auth.instructions_html')
-
-  .qr-wrapper
-    .qr-code= raw @qrcode.as_svg(padding: 0, module_size: 4)
-
-    .qr-alternative
-      %p.hint= t('two_factor_auth.manual_instructions')
-      %samp.qr-alternative__code= current_user.otp_secret.scan(/.{4}/).join(' ')
-
-  = f.input :code, hint: t('two_factor_auth.code_hint'), placeholder: t('simple_form.labels.defaults.otp_attempt')
-
-  .actions
-    = f.button :button, t('two_factor_auth.enable'), type: :submit
diff --git a/app/views/settings/two_factor_auths/recovery_codes.html.haml b/app/views/settings/two_factor_auths/recovery_codes.html.haml
deleted file mode 100644
index aaeebaf2e..000000000
--- a/app/views/settings/two_factor_auths/recovery_codes.html.haml
+++ /dev/null
@@ -1,4 +0,0 @@
-- content_for :page_title do
-  = t('settings.two_factor_auth')
-
-= render 'recovery_codes', recovery_codes: @codes
diff --git a/app/views/settings/two_factor_auths/show.html.haml b/app/views/settings/two_factor_auths/show.html.haml
deleted file mode 100644
index c966ecebf..000000000
--- a/app/views/settings/two_factor_auths/show.html.haml
+++ /dev/null
@@ -1,17 +0,0 @@
-- content_for :page_title do
-  = t('settings.two_factor_auth')
-
-.simple_form
-  %p.hint= t('two_factor_auth.description_html')
-
-  - if current_user.otp_required_for_login
-    = link_to t('two_factor_auth.disable'), disable_settings_two_factor_auth_path, data: { method: 'POST' }, class: 'block-button'
-  - else
-    = link_to t('two_factor_auth.setup'), new_settings_two_factor_auth_path, class: 'block-button'
-
-- if current_user.otp_required_for_login
-  %p
-
-  .simple_form
-    %p.hint= t('two_factor_auth.lost_recovery_codes')
-    = link_to t('two_factor_auth.generate_recovery_codes'), recovery_codes_settings_two_factor_auth_path, data: { method: 'POST' }, class: 'block-button'