Implement @nilsding's review changes

app/assets/stylesheets/components/_totp-setup.scss

@@ -1,11 +1,20 @@
 .totp-setup {
-  display: flex;
-
   &__card {
     background: var(--primary);
     padding: 10px;
     border-radius: 5px;
-    width: 256px;
+
+    min-width: 256px;
+    max-width: 256px;
+    width: 100%;
+    margin: 0 auto;
+  }
+
+  &__card-container {
+    min-width: 276px;
+    max-width: 276px;
+    width: 100%;
+    padding: 0;
   }
 
   &__qr {
@@ -21,14 +30,11 @@
     border-radius: 5px;
 
     code {
+      display: block;
       color: var(--warning);
     }
   }
 
-  &__right {
-    margin-left: 20px;
-  }
-
   &__code-field {
     font-family: "Monaco", "Inconsolata", "Cascadia Code", "Consolas", monospace;
     width: 86px;

app/controllers/user/sessions_controller.rb

@@ -1,8 +1,12 @@
 class User::SessionsController < Devise::SessionsController
+  def new
+    session.delete(:user_sign_in_uid)
+    super
+  end
+
   def create
     if session.has_key?(:user_sign_in_uid)
-      self.resource = User.find(session[:user_sign_in_uid])
+      self.resource = User.find(session.delete(:user_sign_in_uid))
-      session.delete(:user_sign_in_uid)
     else
       self.resource = warden.authenticate!(auth_options)
     end
@@ -11,14 +15,15 @@ class User::SessionsController < Devise::SessionsController
       if params[:user][:otp_attempt].blank?
         session[:user_sign_in_uid] = resource.id
         sign_out(resource)
-        redirect_to user_two_factor_entry_url
+        warden.lock!
+        render 'auth/two_factor_authentication'
       else
         if resource.authenticate_otp(params[:user][:otp_attempt])
           continue_sign_in(resource, resource_name)
         else
           sign_out(resource)
-          flash[:error] = t('devise.failure.invalid')
+          flash[:error] = "Invalid code provided"
-          redirect_to root_url
+          redirect_to new_user_session_url
         end
       end
     else

app/controllers/user_controller.rb

@@ -176,6 +176,7 @@ class UserController < ApplicationController
   def edit_security
     if current_user.otp_module_disabled?
       current_user.otp_secret_key = User.otp_random_secret(26)
+      current_user.save
 
       @provisioning_uri = current_user.provisioning_uri(nil, issuer: APP_CONFIG[:hostname])
       qr_code = RQRCode::QRCode.new(current_user.provisioning_uri("Retrospring:#{current_user.screen_name}", issuer: "Retrospring"))
@@ -185,8 +186,7 @@ class UserController < ApplicationController
   end
 
   def update_2fa
-    req_params = params.require(:user).permit(:otp_secret_key, :otp_validation)
+    req_params = params.require(:user).permit(:otp_validation)
-    current_user.otp_secret_key = req_params[:otp_secret_key]
     current_user.otp_module = :enabled
 
     if current_user.authenticate_otp(req_params[:otp_validation])

app/views/settings/security/_totp_setup.haml

@@ -1,42 +1,42 @@
-.totp-setup
+.totp-setup.container
-  .totp-setup__left
+  .row
-    .totp-setup__card
+    .totp-setup__card-container.col
-      .totp-setup__qr
+      .totp-setup__card
-        = qr_svg
+        .totp-setup__qr
-      %p.totp-setup__text
+          = qr_svg
-        If you cannot scan the QR code, use the following key instead:
+        %p.totp-setup__text
-        %code= current_user.otp_secret_key.scan(/.{4}/).flatten.join(' ')
+          If you cannot scan the QR code, use the following key instead:
-  .totp-setup__right
+          %code= current_user.otp_secret_key.scan(/.{4}/).flatten.join(' ')
-    = bootstrap_form_for(current_user, url: { action: :update_2fa, method: :post }) do |f|
+    .totp-setup__content.col
-      %p
+      = bootstrap_form_for(current_user, url: { action: :update_2fa, method: :post }) do |f|
-        If you do not have an authenticator app already installed on your device, we suggest one of the following:
+        %p
-      %ul.list-unstyled.pl-3
+          If you do not have an authenticator app already installed on your device, we suggest one of the following:
-        %li
+        %ul.list-unstyled.pl-3
-          %i.fa.fa-android
+          %li
-          Aegis Authenticator for Android
+            %i.fa.fa-android
-          %ul.list-inline
+            Aegis Authenticator for Android
-            %li.list-inline-item
+            %ul.list-inline
-              %a{ href: 'https://play.google.com/store/apps/details?id=com.beemdevelopment.aegis' } Google Play
+              %li.list-inline-item
-            %li.list-inline-item
+                %a{ href: 'https://play.google.com/store/apps/details?id=com.beemdevelopment.aegis' } Google Play
-              %a{ href: 'https://f-droid.org/app/com.beemdevelopment.aegis' } F-Droid
+              %li.list-inline-item
-            %li.list-inline-item
+                %a{ href: 'https://f-droid.org/app/com.beemdevelopment.aegis' } F-Droid
-              %a{ href: 'https://github.com/beemdevelopment/Aegis' } Source Code
+              %li.list-inline-item
-        %li
+                %a{ href: 'https://github.com/beemdevelopment/Aegis' } Source Code
-          %i.fa.fa-apple
+          %li
-          Strongbox Authenticator for iOS
+            %i.fa.fa-apple
-          %ul.list-inline
+            Strongbox Authenticator for iOS
-            %li.list-inline-item
+            %ul.list-inline
-              %a{ href: 'https://apps.apple.com/gb/app/strongbox-authenticator/id1023839880' } App Store
+              %li.list-inline-item
-        %li
+                %a{ href: 'https://apps.apple.com/gb/app/strongbox-authenticator/id1023839880' } App Store
-          %i.fa.fa-apple
+          %li
-          %i.fa.fa-android
+            %i.fa.fa-apple
-          Microsoft Authenticator
+            %i.fa.fa-android
-          %ul.list-inline
+            Microsoft Authenticator
-            %li.list-inline-item
+            %ul.list-inline
-              %a{ href: 'https://apps.apple.com/gb/app/microsoft-authenticator/id983156458' } App Store
+              %li.list-inline-item
-            %li.list-inline-item
+                %a{ href: 'https://apps.apple.com/gb/app/microsoft-authenticator/id983156458' } App Store
-              %a{ href: 'https://play.google.com/store/apps/details?id=com.azure.authenticator' } Google Play
+              %li.list-inline-item
-      %p Once you have downloaded an authenticator app, add your Retrospring account by scanning the QR code displayed on the left.
+                %a{ href: 'https://play.google.com/store/apps/details?id=com.azure.authenticator' } Google Play
-      = f.text_field :otp_validation, class: 'totp-setup__code-field', label: 'Enter the code displayed in the app here:'
+        %p Once you have downloaded an authenticator app, add your Retrospring account by scanning the QR code displayed on the left.
-      = f.hidden_field :otp_secret_key, value: current_user.otp_secret_key
+        = f.text_field :otp_validation, class: 'totp-setup__code-field', label: 'Enter the code displayed in the app here:'
-      = f.submit t('views.actions.save'), class: 'btn btn-primary'
+        = f.submit t('views.actions.save'), class: 'btn btn-primary'

db/migrate/20201001172537_add_otp_secret_key_to_users.rb

@@ -2,9 +2,5 @@ class AddOtpSecretKeyToUsers < ActiveRecord::Migration[5.2]
   def change
     add_column :users, :otp_secret_key, :string
     add_column :users, :otp_module, :integer, default: 0, null: false
-
-    User.find_each do |user|
-      user.update_attribute(:otp_secret_key, User.otp_random_secret)
-    end
   end
 end

spec/controllers/user/registration_controller_spec.rb

@@ -4,6 +4,7 @@ require "rails_helper"
 
 describe User::RegistrationsController, type: :controller do
   before do
+    # Required for devise to register routes
     @request.env["devise.mapping"] = Devise.mappings[:user]
   end
 

spec/controllers/user/sessions_controller_spec.rb

@@ -2,6 +2,7 @@ require 'rails_helper'
 
 describe User::SessionsController do
   before do
+    # Required for devise to register routes
     @request.env["devise.mapping"] = Devise.mappings[:user]
   end
 
@@ -18,7 +19,7 @@ describe User::SessionsController do
       user.otp_module = :enabled
       user.save
 
-      expect(subject).to redirect_to :user_two_factor_entry
+      expect(subject).to have_rendered('auth/two_factor_authentication')
     end
   end