Fix #3910 - Require OTP authentication to disable 2FA (#3935)

* Fix #3910 - Require OTP authentication to disable 2FA. Also, remove ability
to generate new OTP backup codes *after* initial backup codes were handed
out during activation

* Restore recovery code re-generation

* Improve display of some 2FA elements

6 files changed, 47 insertions, 24 deletions

diff --git a/app/controllers/settings/two_factor_authentications_controller.rb b/app/controllers/settings/two_factor_authentications_controller.rb
index f66c3a908..983483881 100644
--- a/app/controllers/settings/two_factor_authentications_controller.rb
+++ b/app/controllers/settings/two_factor_authentications_controller.rb
@@ -7,7 +7,9 @@ module Settings
     before_action :authenticate_user!
     before_action :verify_otp_required, only: [:create]
 
-    def show; end
+    def show
+      @confirmation = Form::TwoFactorConfirmation.new
+    end
 
     def create
       current_user.otp_secret = User.generate_otp_secret(32)
@@ -16,13 +18,23 @@ module Settings
     end
 
     def destroy
-      current_user.otp_required_for_login = false
-      current_user.save!
-      redirect_to settings_two_factor_authentication_path
+      if current_user.validate_and_consume_otp!(confirmation_params[:code])
+        current_user.otp_required_for_login = false
+        current_user.save!
+        redirect_to settings_two_factor_authentication_path
+      else
+        flash.now[:alert] = I18n.t('two_factor_authentication.wrong_code')
+        @confirmation = Form::TwoFactorConfirmation.new
+        render :show
+      end
     end
 
     private
 
+    def confirmation_params
+      params.require(:form_two_factor_confirmation).permit(:code)
+    end
+
     def verify_otp_required
       redirect_to settings_two_factor_authentication_path if current_user.otp_required_for_login?
     end
diff --git a/app/javascript/styles/admin.scss b/app/javascript/styles/admin.scss
index c2bfc10a0..3bc713566 100644
--- a/app/javascript/styles/admin.scss
+++ b/app/javascript/styles/admin.scss
@@ -129,6 +129,11 @@
         color: $ui-primary-color;
       }
     }
+
+    .positive-hint {
+      color: $valid-value-color;
+      font-weight: 500;
+    }
   }
 
   .simple_form {
diff --git a/app/javascript/styles/forms.scss b/app/javascript/styles/forms.scss
index 059c4a7d8..7a181f36b 100644
--- a/app/javascript/styles/forms.scss
+++ b/app/javascript/styles/forms.scss
@@ -358,7 +358,6 @@ code {
 }
 
 .user_filtered_languages {
-
   & > label {
     font-family: inherit;
     font-size: 16px;
diff --git a/app/javascript/styles/lists.scss b/app/javascript/styles/lists.scss
index 47805663f..6019cd800 100644
--- a/app/javascript/styles/lists.scss
+++ b/app/javascript/styles/lists.scss
@@ -10,7 +10,6 @@
 .recovery-codes {
   list-style: none;
   margin: 0 auto;
-  text-align: center;
 
   li {
     font-size: 125%;
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
index 7d409826e..d47ee840e 100644
--- a/app/views/settings/two_factor_authentication/recovery_codes/index.html.haml
+++ b/app/views/settings/two_factor_authentication/recovery_codes/index.html.haml
@@ -1,7 +1,7 @@
 - content_for :page_title do
   = t('settings.two_factor_authentication')
 
-%p.hint= t('two_factor_authentication.recovery_instructions')
+%p.hint= t('two_factor_authentication.recovery_instructions_html')
 
 %ol.recovery-codes
   - @recovery_codes.each do |code|
diff --git a/app/views/settings/two_factor_authentications/show.html.haml b/app/views/settings/two_factor_authentications/show.html.haml
index 88b5bd20e..8ba42a101 100644
--- a/app/views/settings/two_factor_authentications/show.html.haml
+++ b/app/views/settings/two_factor_authentications/show.html.haml
@@ -1,26 +1,34 @@
 - 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
+  %p.positive-hint
+    = fa_icon 'check'
+    = ' '
+    = t 'two_factor_authentication.enabled'
 
-  - 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'
+  %hr/
 
-- if current_user.otp_required_for_login
-  .simple_form
-    %p.hint
-      = t('two_factor_authentication.lost_recovery_codes')
+  = simple_form_for @confirmation, url: settings_two_factor_authentication_path, method: :delete do |f|
+    = 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.disable'), type: :submit
+
+  %hr/
+
+  %h6= t('two_factor_authentication.recovery_codes')
+  %p.muted-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 }
+
+- else
+  .simple_form
+    %p.hint= t('two_factor_authentication.description_html')
+
+    = link_to t('two_factor_authentication.setup'),
+      settings_two_factor_authentication_path,
       data: { method: :post },
       class: 'block-button'