From d7a1750694821be583b2061f5a2ba54c069de638 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Fri, 23 Oct 2020 20:45:06 +0200 Subject: [PATCH] Implement @nilsding's review changes --- .../stylesheets/components/_totp-setup.scss | 20 +++-- app/controllers/user/sessions_controller.rb | 15 ++-- app/controllers/user_controller.rb | 4 +- app/views/settings/security/_totp_setup.haml | 84 +++++++++---------- ...01001172537_add_otp_secret_key_to_users.rb | 4 - .../user/registration_controller_spec.rb | 1 + .../user/sessions_controller_spec.rb | 3 +- 7 files changed, 70 insertions(+), 61 deletions(-) diff --git a/app/assets/stylesheets/components/_totp-setup.scss b/app/assets/stylesheets/components/_totp-setup.scss index 7e76bbfe..914af86e 100644 --- a/app/assets/stylesheets/components/_totp-setup.scss +++ b/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; diff --git a/app/controllers/user/sessions_controller.rb b/app/controllers/user/sessions_controller.rb index 8930deee..58a18d94 100644 --- a/app/controllers/user/sessions_controller.rb +++ b/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]) - session.delete(:user_sign_in_uid) + self.resource = User.find(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') - redirect_to root_url + flash[:error] = "Invalid code provided" + redirect_to new_user_session_url end end else diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index 36e4d42b..f8b25bfb 100644 --- a/app/controllers/user_controller.rb +++ b/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) - current_user.otp_secret_key = req_params[:otp_secret_key] + req_params = params.require(:user).permit(:otp_validation) current_user.otp_module = :enabled if current_user.authenticate_otp(req_params[:otp_validation]) diff --git a/app/views/settings/security/_totp_setup.haml b/app/views/settings/security/_totp_setup.haml index 3b60b589..828ea749 100644 --- a/app/views/settings/security/_totp_setup.haml +++ b/app/views/settings/security/_totp_setup.haml @@ -1,42 +1,42 @@ -.totp-setup - .totp-setup__left - .totp-setup__card - .totp-setup__qr - = qr_svg - %p.totp-setup__text - If you cannot scan the QR code, use the following key instead: - %code= current_user.otp_secret_key.scan(/.{4}/).flatten.join(' ') - .totp-setup__right - = bootstrap_form_for(current_user, url: { action: :update_2fa, method: :post }) do |f| - %p - If you do not have an authenticator app already installed on your device, we suggest one of the following: - %ul.list-unstyled.pl-3 - %li - %i.fa.fa-android - Aegis Authenticator for Android - %ul.list-inline - %li.list-inline-item - %a{ href: 'https://play.google.com/store/apps/details?id=com.beemdevelopment.aegis' } Google Play - %li.list-inline-item - %a{ href: 'https://f-droid.org/app/com.beemdevelopment.aegis' } F-Droid - %li.list-inline-item - %a{ href: 'https://github.com/beemdevelopment/Aegis' } Source Code - %li - %i.fa.fa-apple - Strongbox Authenticator for iOS - %ul.list-inline - %li.list-inline-item - %a{ href: 'https://apps.apple.com/gb/app/strongbox-authenticator/id1023839880' } App Store - %li - %i.fa.fa-apple - %i.fa.fa-android - Microsoft Authenticator - %ul.list-inline - %li.list-inline-item - %a{ href: 'https://apps.apple.com/gb/app/microsoft-authenticator/id983156458' } App Store - %li.list-inline-item - %a{ href: 'https://play.google.com/store/apps/details?id=com.azure.authenticator' } Google Play - %p Once you have downloaded an authenticator app, add your Retrospring account by scanning the QR code displayed on the left. - = f.text_field :otp_validation, class: 'totp-setup__code-field', label: 'Enter the code displayed in the app here:' - = f.hidden_field :otp_secret_key, value: current_user.otp_secret_key - = f.submit t('views.actions.save'), class: 'btn btn-primary' +.totp-setup.container + .row + .totp-setup__card-container.col + .totp-setup__card + .totp-setup__qr + = qr_svg + %p.totp-setup__text + If you cannot scan the QR code, use the following key instead: + %code= current_user.otp_secret_key.scan(/.{4}/).flatten.join(' ') + .totp-setup__content.col + = bootstrap_form_for(current_user, url: { action: :update_2fa, method: :post }) do |f| + %p + If you do not have an authenticator app already installed on your device, we suggest one of the following: + %ul.list-unstyled.pl-3 + %li + %i.fa.fa-android + Aegis Authenticator for Android + %ul.list-inline + %li.list-inline-item + %a{ href: 'https://play.google.com/store/apps/details?id=com.beemdevelopment.aegis' } Google Play + %li.list-inline-item + %a{ href: 'https://f-droid.org/app/com.beemdevelopment.aegis' } F-Droid + %li.list-inline-item + %a{ href: 'https://github.com/beemdevelopment/Aegis' } Source Code + %li + %i.fa.fa-apple + Strongbox Authenticator for iOS + %ul.list-inline + %li.list-inline-item + %a{ href: 'https://apps.apple.com/gb/app/strongbox-authenticator/id1023839880' } App Store + %li + %i.fa.fa-apple + %i.fa.fa-android + Microsoft Authenticator + %ul.list-inline + %li.list-inline-item + %a{ href: 'https://apps.apple.com/gb/app/microsoft-authenticator/id983156458' } App Store + %li.list-inline-item + %a{ href: 'https://play.google.com/store/apps/details?id=com.azure.authenticator' } Google Play + %p Once you have downloaded an authenticator app, add your Retrospring account by scanning the QR code displayed on the left. + = 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' diff --git a/db/migrate/20201001172537_add_otp_secret_key_to_users.rb b/db/migrate/20201001172537_add_otp_secret_key_to_users.rb index 4e736ab8..59a33b48 100644 --- a/db/migrate/20201001172537_add_otp_secret_key_to_users.rb +++ b/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 diff --git a/spec/controllers/user/registration_controller_spec.rb b/spec/controllers/user/registration_controller_spec.rb index 3e7670fc..a2b699b1 100644 --- a/spec/controllers/user/registration_controller_spec.rb +++ b/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 diff --git a/spec/controllers/user/sessions_controller_spec.rb b/spec/controllers/user/sessions_controller_spec.rb index 4b9d520d..c4ed6451 100644 --- a/spec/controllers/user/sessions_controller_spec.rb +++ b/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