Fix #795, fix #704, fix #835 - 2FA requires confirmation to be enabled (#1278)

* Fix #795, fix #704, fix #835 - 2FA requires confirmation to be enabled
TOTP secret is not shown again after 2FA is enabled

* Clean up

5 files changed, 76 insertions, 19 deletions

diff --git a/app/assets/stylesheets/forms.scss b/app/assets/stylesheets/forms.scss
index ceccc14cd..2e3a4f147 100644
--- a/app/assets/stylesheets/forms.scss
+++ b/app/assets/stylesheets/forms.scss
@@ -25,6 +25,10 @@ code {
     margin-bottom: 15px;
   }
 
+  strong {
+    font-weight: 500;
+  }
+
   .label_input {
     display: flex;
 
@@ -224,7 +228,12 @@ code {
   }
 }
 
+.qr-wrapper {
+  display: flex;
+}
+
 .qr-code {
+  flex: 0 0 auto;
   background: #fff;
   padding: 4px;
   margin-bottom: 20px;
@@ -236,3 +245,13 @@ code {
     margin: 0;
   }
 }
+
+.qr-alternative {
+  margin-left: 10px;
+  color: $color3;
+
+  samp {
+    display: block;
+    font-size: 14px;
+  }
+}
diff --git a/app/controllers/settings/two_factor_auths_controller.rb b/app/controllers/settings/two_factor_auths_controller.rb
index cfee92391..203d1fc46 100644
--- a/app/controllers/settings/two_factor_auths_controller.rb
+++ b/app/controllers/settings/two_factor_auths_controller.rb
@@ -5,19 +5,29 @@ class Settings::TwoFactorAuthsController < ApplicationController
 
   before_action :authenticate_user!
 
-  def show
-    return unless current_user.otp_required_for_login
+  def show; end
 
-    @provision_url = current_user.otp_provisioning_uri(current_user.email, issuer: Rails.configuration.x.local_domain)
-    @qrcode        = RQRCode::QRCode.new(@provision_url)
-  end
+  def new
+    redirect_to settings_two_factor_auth_path if current_user.otp_required_for_login
 
-  def enable
-    current_user.otp_required_for_login = true
-    current_user.otp_secret = User.generate_otp_secret
+    @confirmation = Form::TwoFactorConfirmation.new
+    current_user.otp_secret = User.generate_otp_secret(32)
     current_user.save!
+    set_qr_code
+  end
 
-    redirect_to settings_two_factor_auth_path
+  def create
+    if current_user.validate_and_consume_otp!(confirmation_params[:code])
+      current_user.otp_required_for_login = true
+      current_user.save!
+
+      redirect_to settings_two_factor_auth_path, 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 action: :new
+    end
   end
 
   def disable
@@ -26,4 +36,15 @@ class Settings::TwoFactorAuthsController < ApplicationController
 
     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/models/form/two_factor_confirmation.rb b/app/models/form/two_factor_confirmation.rb
new file mode 100644
index 000000000..b8cf76d05
--- /dev/null
+++ b/app/models/form/two_factor_confirmation.rb
@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class Form::TwoFactorConfirmation
+  include ActiveModel::Model
+
+  attr_accessor :code
+end
diff --git a/app/views/settings/two_factor_auths/new.html.haml b/app/views/settings/two_factor_auths/new.html.haml
new file mode 100644
index 000000000..5bae743ef
--- /dev/null
+++ b/app/views/settings/two_factor_auths/new.html.haml
@@ -0,0 +1,17 @@
+- 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/show.html.haml b/app/views/settings/two_factor_auths/show.html.haml
index 87bfadc69..047fe0c54 100644
--- a/app/views/settings/two_factor_auths/show.html.haml
+++ b/app/views/settings/two_factor_auths/show.html.haml
@@ -2,16 +2,9 @@
   = t('settings.two_factor_auth')
 
 .simple_form
-  - if current_user.otp_required_for_login
-    %p.hint= t('two_factor_auth.instructions_html')
-
-    .qr-code= raw @qrcode.as_svg(padding: 0, module_size: 5)
-
-    %p.hint= t('two_factor_auth.plaintext_secret_html', secret: current_user.otp_secret)
-
-    %p.hint= t('two_factor_auth.warning')
+  %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
-    %p.hint= t('two_factor_auth.description_html')
-    = link_to t('two_factor_auth.enable'), enable_settings_two_factor_auth_path, data: { method: 'POST' }, class: 'block-button'
+    = link_to t('two_factor_auth.setup'), new_settings_two_factor_auth_path, class: 'block-button'