From 141ff59f6320f6d35eb7fd9df384828b72e5f87a Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Sun, 18 Oct 2020 10:39:46 +0200 Subject: [PATCH 01/27] Implement Two Factor Authentication --- Gemfile | 2 + Gemfile.lock | 89 +++++++++++-------- app/controllers/user/sessions_controller.rb | 42 +++++++++ app/controllers/user_controller.rb | 29 ++++++ app/models/user.rb | 4 + app/views/auth/two_factor_authentication.haml | 14 +++ app/views/settings/_security.haml | 15 ++++ app/views/tabs/_settings.haml | 1 + app/views/user/edit_security.haml | 4 + app/views/user/security.html.erb | 0 config/locales/en.yml | 8 ++ config/routes.rb | 7 +- ...01001172537_add_otp_secret_key_to_users.rb | 5 ++ db/schema.rb | 3 +- 14 files changed, 183 insertions(+), 40 deletions(-) create mode 100644 app/controllers/user/sessions_controller.rb create mode 100644 app/views/auth/two_factor_authentication.haml create mode 100644 app/views/settings/_security.haml create mode 100644 app/views/user/edit_security.haml create mode 100644 app/views/user/security.html.erb create mode 100644 db/migrate/20201001172537_add_otp_secret_key_to_users.rb diff --git a/Gemfile b/Gemfile index 5266c57d..73efed4b 100644 --- a/Gemfile +++ b/Gemfile @@ -24,6 +24,8 @@ gem 'sweetalert-rails' gem 'devise', '~> 4.0' gem 'devise-i18n' gem 'devise-async' +gem 'active_model_otp' +gem 'rqrcode' gem 'bootstrap_form' gem 'font-kit-rails' gem 'nprogress-rails' diff --git a/Gemfile.lock b/Gemfile.lock index a71e1561..91034a28 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -59,6 +59,9 @@ GEM erubi (~> 1.4) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.0, >= 1.0.3) + active_model_otp (2.0.1) + activemodel + rotp (~> 5.0.0) activejob (5.2.4.3) activesupport (= 5.2.4.3) globalid (>= 0.3.6) @@ -84,8 +87,8 @@ GEM addressable (2.7.0) public_suffix (>= 2.0.2, < 5.0) arel (9.0.0) - ast (2.4.0) - autoprefixer-rails (9.7.6) + ast (2.4.1) + autoprefixer-rails (9.8.5) execjs bcrypt (3.1.13) better_errors (2.7.1) @@ -110,7 +113,7 @@ GEM buftok (0.2.0) builder (3.2.4) byebug (11.1.3) - capybara (3.32.2) + capybara (3.33.0) addressable mini_mime (>= 0.1.3) nokogiri (~> 1.8) @@ -125,8 +128,9 @@ GEM image_processing (~> 1.1) mimemagic (>= 0.3.0) mini_mime (>= 0.1.3) + chunky_png (1.3.12) cliver (0.3.2) - coderay (1.1.2) + coderay (1.1.3) coffee-rails (4.2.2) coffee-script (>= 2.2.0) railties (>= 4.0.0) @@ -140,7 +144,7 @@ GEM crass (1.0.6) database_cleaner (1.8.5) debug_inspector (0.0.3) - devise (4.7.1) + devise (4.7.2) bcrypt (~> 3.0) orm_adapter (~> 0.1) railties (>= 4.1.0) @@ -151,19 +155,19 @@ GEM devise (>= 4.0) devise-i18n (1.9.1) devise (>= 4.7.1) - diff-lcs (1.3) + diff-lcs (1.4.4) docile (1.3.2) domain_name (0.5.20190701) unf (>= 0.0.5, < 1.0.0) equalizer (0.0.11) erubi (1.9.0) - excon (0.73.0) + excon (0.75.0) execjs (2.7.0) - factory_bot (5.2.0) - activesupport (>= 4.2.0) - factory_bot_rails (5.2.0) - factory_bot (~> 5.2.0) - railties (>= 4.2.0) + factory_bot (6.1.0) + activesupport (>= 5.0.0) + factory_bot_rails (6.1.0) + factory_bot (~> 6.1.0) + railties (>= 5.0.0) fake_email_validator (1.0.11) activemodel mail @@ -173,11 +177,11 @@ GEM multipart-post (>= 1.2, < 3) faraday_middleware (1.0.0) faraday (~> 1.0) - ffi (1.12.2) + ffi (1.13.1) ffi-compiler (1.0.1) ffi (>= 1.0.0) rake - fog-aws (3.6.5) + fog-aws (3.6.6) fog-core (~> 2.1) fog-json (~> 1.1) fog-xml (~> 0.1) @@ -236,7 +240,7 @@ GEM http-parser (1.2.1) ffi-compiler (>= 1.0, < 2.0) http_parser.rb (0.6.0) - httparty (0.18.0) + httparty (0.18.1) mime-types (~> 3.0) multi_xml (>= 0.5.2) i18n (0.9.5) @@ -261,7 +265,7 @@ GEM turbolinks jquery-ui-rails (6.0.1) railties (>= 3.2.16) - json (2.3.0) + json (2.3.1) kaminari (1.2.1) activesupport (>= 4.1.0) kaminari-actionview (= 1.2.1) @@ -281,10 +285,10 @@ GEM listen (3.2.1) rb-fsevent (~> 0.10, >= 0.10.3) rb-inotify (~> 0.9, >= 0.9.10) - loofah (2.5.0) + loofah (2.6.0) crass (~> 1.0.2) nokogiri (>= 1.5.9) - lumberjack (1.2.4) + lumberjack (1.2.6) mail (2.7.1) mini_mime (>= 0.1.1) marcel (0.3.3) @@ -304,15 +308,15 @@ GEM momentjs-rails (>= 2.10.5, <= 3.0.0) momentjs-rails (2.20.1) railties (>= 3.1) - multi_json (1.14.1) + multi_json (1.15.0) multi_xml (0.6.0) multipart-post (2.1.1) naught (1.1.0) nenv (0.3.0) nested_form (0.3.2) - newrelic_rpm (6.10.0.364) + newrelic_rpm (6.11.0.365) nio4r (2.5.2) - nokogiri (1.10.9) + nokogiri (1.10.10) mini_portile2 (~> 2.4.0) nokogumbo (2.0.2) nokogiri (~> 1.8, >= 1.8.4) @@ -334,9 +338,9 @@ GEM omniauth-oauth (~> 1.1) rack orm_adapter (0.5.0) - parallel (1.19.1) - parser (2.7.1.2) - ast (~> 2.4.0) + parallel (1.19.2) + parser (2.7.1.4) + ast (~> 2.4.1) pg (1.2.3) pghero (2.7.0) activerecord (>= 5) @@ -375,10 +379,10 @@ GEM rails-assets-growl (1.3.5) rails-assets-jquery rails-assets-jquery (2.2.4) - rails-controller-testing (1.0.4) - actionpack (>= 5.0.1.x) - actionview (>= 5.0.1.x) - activesupport (>= 5.0.1.x) + rails-controller-testing (1.0.5) + actionpack (>= 5.0.1.rc1) + actionview (>= 5.0.1.rc1) + activesupport (>= 5.0.1.rc1) rails-dom-testing (2.0.3) activesupport (>= 4.2.0) nokogiri (>= 1.6) @@ -412,13 +416,19 @@ GEM ffi (~> 1.0) redcarpet (3.5.0) redis (4.1.4) - regexp_parser (1.7.0) + regexp_parser (1.7.1) remotipart (1.4.4) - responders (3.0.0) + responders (3.0.1) actionpack (>= 5.0) railties (>= 5.0) rexml (3.2.4) - rolify (5.2.0) + rolify (5.3.0) + rotp (5.0.0) + addressable (~> 2.5) + rqrcode (1.1.2) + chunky_png (~> 1.0) + rqrcode_core (~> 0.1) + rqrcode_core (0.1.2) rspec-core (3.9.2) rspec-support (~> 3.9.3) rspec-expectations (3.9.2) @@ -438,19 +448,20 @@ GEM rspec-expectations (~> 3.9.0) rspec-mocks (~> 3.9.0) rspec-support (~> 3.9.0) - rspec-sidekiq (3.0.3) + rspec-sidekiq (3.1.0) rspec-core (~> 3.0, >= 3.0.0) sidekiq (>= 2.4.0) rspec-support (3.9.3) - rubocop (0.84.0) + rubocop (0.88.0) parallel (~> 1.10) - parser (>= 2.7.0.1) + parser (>= 2.7.1.1) rainbow (>= 2.2.2, < 4.0) + regexp_parser (>= 1.7) rexml - rubocop-ast (>= 0.0.3) + rubocop-ast (>= 0.1.0, < 1.0) ruby-progressbar (~> 1.7) unicode-display_width (>= 1.4.0, < 2.0) - rubocop-ast (0.0.3) + rubocop-ast (0.1.0) parser (>= 2.7.0.1) ruby-progressbar (1.10.1) ruby-vips (2.0.17) @@ -470,7 +481,7 @@ GEM sprockets (>= 2.8, < 4.0) sprockets-rails (>= 2.0, < 4.0) tilt (>= 1.1, < 3) - sassc (2.3.0) + sassc (2.4.0) ffi (~> 1.9) sassc-rails (2.1.2) railties (>= 4.0.0) @@ -540,7 +551,7 @@ GEM activemodel (>= 5.0) bindex (>= 0.4.0) railties (>= 5.0) - websocket-driver (0.7.2) + websocket-driver (0.7.3) websocket-extensions (>= 0.1.0) websocket-extensions (0.1.5) xpath (3.2.0) @@ -550,6 +561,7 @@ PLATFORMS ruby DEPENDENCIES + active_model_otp bcrypt (~> 3.1.7) better_errors binding_of_caller @@ -609,6 +621,7 @@ DEPENDENCIES redcarpet redis rolify (~> 5.2) + rqrcode rspec-its (~> 1.3) rspec-mocks rspec-rails (~> 3.9) diff --git a/app/controllers/user/sessions_controller.rb b/app/controllers/user/sessions_controller.rb new file mode 100644 index 00000000..76e1578f --- /dev/null +++ b/app/controllers/user/sessions_controller.rb @@ -0,0 +1,42 @@ +class User::SessionsController < Devise::SessionsController + 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) + else + self.resource = warden.authenticate!(auth_options) + end + + if resource.active_for_authentication? && !resource.otp_secret_key.nil? + if params[:user][:otp_attempt].blank? + session[:user_sign_in_uid] = resource.id + sign_out(resource) + redirect_to user_two_factor_entry_url + 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 + end + end + else + continue_sign_in(resource, resource_name) + end + end + + def two_factor_entry + self.resource = User.find(session[:user_sign_in_uid]) + render 'auth/two_factor_authentication' + end + + private + + def continue_sign_in(resource, resource_name) + set_flash_message!(:notice, :signed_in) + sign_in(resource_name, resource) + yield resource if block_given? + respond_with resource, location: after_sign_in_path_for(resource) + end +end \ No newline at end of file diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index 204044cf..13f306d2 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -172,4 +172,33 @@ class UserController < ApplicationController redirect_to user_export_path end + + def edit_security + current_user.otp_secret_key = User.otp_random_secret + + @provisioning_uri = current_user.provisioning_uri(nil, issuer: APP_CONFIG[:hostname]) + qr_code = RQRCode::QRCode.new(@provisioning_uri, :size => 12, :level => :h) + @qr_svg = qr_code.as_svg(offset: 0, color: '000', + shape_rendering: 'crispEdges', + module_size: 4) + 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] + + if current_user.authenticate_otp(req_params[:otp_validation]) + flash[:success] = 'yay' + current_user.save! + else + flash[:error] = current_user.otp_code + end + + redirect_to edit_user_security_path + end + + def destroy_2fa + + end end diff --git a/app/models/user.rb b/app/models/user.rb index 8bec2c60..de4031bd 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -4,6 +4,7 @@ class User < ApplicationRecord include User::QuestionMethods include User::RelationshipMethods include User::TimelineMethods + include ActiveModel::OneTimePassword # Include default devise modules. Others available are: # :confirmable, :lockable, :timeoutable and :omniauthable @@ -11,6 +12,9 @@ class User < ApplicationRecord :recoverable, :rememberable, :trackable, :validatable, :confirmable, :authentication_keys => [:login] + has_one_time_password + attr_accessor :otp_attempt, :otp_validation + rolify # attr_accessor :login diff --git a/app/views/auth/two_factor_authentication.haml b/app/views/auth/two_factor_authentication.haml new file mode 100644 index 00000000..75583213 --- /dev/null +++ b/app/views/auth/two_factor_authentication.haml @@ -0,0 +1,14 @@ +.container + .row + .col-sm-4.offset-sm-4 + = render 'layouts/messages' + .card.mt-3 + .card-body + %h1.mb-3.mt-0= t('views.auth.2fa.title') + = bootstrap_form_for(resource, as: resource_name, url: session_path(resource_name), method: :post) do |f| + + = f.text_field :otp_attempt, autofocus: true, label: t('views.auth.2fa.otp_field') + + = f.submit t('views.sessions.create'), class: 'btn btn-primary mt-3 mb-3' + += render 'shared/links' diff --git a/app/views/settings/_security.haml b/app/views/settings/_security.haml new file mode 100644 index 00000000..e2d7046b --- /dev/null +++ b/app/views/settings/_security.haml @@ -0,0 +1,15 @@ +.card + .card-body + %h2= t('views.settings.security.2fa.title') + - if current_user.otp_secret_key.nil? + = bootstrap_form_for(current_user, url: { action: :update_2fa, method: :post }) do |f| + %a{:href => "https://play.google.com/store/apps/details?id=com.beemdevelopment.aegis"} Aegis Authenticator for Android + %a{:href => "https://apps.apple.com/gb/app/strongbox-authenticator/id1023839880"} Strongbox Authenticator for iOS + = RQRCode::QRCode.new(current_user.provisioning_uri("Retrospring:#{current_user.screen_name}", issuer: "Retrospring")).as_svg.html_safe + %pre= current_user.otp_secret_key + = f.text_field :otp_validation + = f.hidden_field :otp_secret_key, value: current_user.otp_secret_key + = f.submit t('views.actions.save'), class: 'btn btn-primary' + - else + %p= t('views.settings.security.2fa.enabled_hint') + = link_to t('views.actions.remove'), destroy_user_2fa_path, :class => 'btn btn-primary', :method => 'delete' \ No newline at end of file diff --git a/app/views/tabs/_settings.haml b/app/views/tabs/_settings.haml index 13d4a420..d6210214 100644 --- a/app/views/tabs/_settings.haml +++ b/app/views/tabs/_settings.haml @@ -3,6 +3,7 @@ = list_group_item t('views.settings.tabs.account'), edit_user_registration_path = list_group_item t('views.settings.tabs.profile'), edit_user_profile_path = list_group_item t('views.settings.tabs.privacy'), edit_user_privacy_path + = list_group_item t('views.settings.tabs.security'), edit_user_security_path = list_group_item t('views.settings.tabs.sharing'), services_path = list_group_item 'Theme', edit_user_theme_path = list_group_item 'Your Data', user_data_path diff --git a/app/views/user/edit_security.haml b/app/views/user/edit_security.haml new file mode 100644 index 00000000..41d8ef4d --- /dev/null +++ b/app/views/user/edit_security.haml @@ -0,0 +1,4 @@ += render 'settings/security' + +- provide(:title, generate_title('Security Settings')) +- parent_layout 'user/settings' diff --git a/app/views/user/security.html.erb b/app/views/user/security.html.erb new file mode 100644 index 00000000..e69de29b diff --git a/config/locales/en.yml b/config/locales/en.yml index f663e63c..95d98ea5 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -377,6 +377,7 @@ en: profile: "Profile" privacy: "Privacy" sharing: "Sharing" + security: "Security" account: modal: title: "Save account changes" @@ -415,6 +416,9 @@ en: connect: "Connect to %{service}" disconnect: "Disconnect" confirm: "Really disconnect service %{service}?" + security: + 2fa: + title: "Two-factor authentication" modal: ask: title: "Ask your followers" @@ -443,3 +447,7 @@ en: admin: "Admin" moderator: "Moderator" banned: "Banned" + auth: + 2fa: + title: "Two-factor authentication" + otp_field: "One-time password" diff --git a/config/routes.rb b/config/routes.rb index 92d57d42..e94f322d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -48,7 +48,8 @@ Rails.application.routes.draw do as :user do # :sessions get 'sign_in' => 'devise/sessions#new', as: :new_user_session - post 'sign_in' => 'devise/sessions#create', as: :user_session + post 'sign_in' => 'user/sessions#create', as: :user_session + get 'otp_auth' => 'user/sessions#two_factor_entry', as: :user_two_factor_entry delete 'sign_out' => 'devise/sessions#destroy', as: :destroy_user_session # :registrations get 'settings/delete_account' => 'devise/registrations#cancel', as: :cancel_user_registration @@ -67,6 +68,10 @@ Rails.application.routes.draw do match '/settings/theme', to: 'user#update_theme', via: 'patch', as: :update_user_theme match '/settings/theme/delete', to: 'user#delete_theme', via: 'delete', as: :delete_user_theme + match '/settings/security', to: 'user#edit_security', via: :get, as: :edit_user_security + match '/settings/security/2fa', to: 'user#update_2fa', via: :patch, as: :update_user_2fa + match '/settings/security/2fa', to: 'user#destroy_2fa', via: :delete, as: :destroy_user_2fa + # resources :services, only: [:index, :destroy] match '/settings/services', to: 'services#index', via: 'get', as: :services match '/settings/services/:id', to: 'services#destroy', via: 'delete', as: :service diff --git a/db/migrate/20201001172537_add_otp_secret_key_to_users.rb b/db/migrate/20201001172537_add_otp_secret_key_to_users.rb new file mode 100644 index 00000000..5fd89b8a --- /dev/null +++ b/db/migrate/20201001172537_add_otp_secret_key_to_users.rb @@ -0,0 +1,5 @@ +class AddOtpSecretKeyToUsers < ActiveRecord::Migration[5.2] + def change + add_column :users, :otp_secret_key, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index f399bfce..71c4b9e0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2020_07_04_163504) do +ActiveRecord::Schema.define(version: 2020_10_01_172537) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -273,6 +273,7 @@ ActiveRecord::Schema.define(version: 2020_07_04_163504) do t.string "export_url" t.boolean "export_processing", default: false, null: false t.datetime "export_created_at" + t.string "otp_secret_key" t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true t.index ["email"], name: "index_users_on_email", unique: true t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true From 25410e111d471e527d44630b5ff229e41aa57bd7 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Sun, 18 Oct 2020 11:39:28 +0200 Subject: [PATCH 02/27] Fix OTP auth triggering for users who haven't set it up I thought I could be clever by using a null secret key as an indicator of it being disabled --- app/controllers/user/sessions_controller.rb | 2 +- app/controllers/user_controller.rb | 2 -- app/models/user.rb | 1 + db/migrate/20201001172537_add_otp_secret_key_to_users.rb | 5 +++++ db/schema.rb | 3 ++- 5 files changed, 9 insertions(+), 4 deletions(-) diff --git a/app/controllers/user/sessions_controller.rb b/app/controllers/user/sessions_controller.rb index 76e1578f..bb72cb2c 100644 --- a/app/controllers/user/sessions_controller.rb +++ b/app/controllers/user/sessions_controller.rb @@ -7,7 +7,7 @@ class User::SessionsController < Devise::SessionsController self.resource = warden.authenticate!(auth_options) end - if resource.active_for_authentication? && !resource.otp_secret_key.nil? + if resource.active_for_authentication? && resource.otp_module_enabled? if params[:user][:otp_attempt].blank? session[:user_sign_in_uid] = resource.id sign_out(resource) diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index 13f306d2..7f9ce77a 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -185,8 +185,6 @@ class UserController < ApplicationController def update_2fa req_params = params.require(:user).permit(:otp_secret_key, :otp_validation) - - current_user.otp_secret_key = req_params[:otp_secret_key] if current_user.authenticate_otp(req_params[:otp_validation]) flash[:success] = 'yay' diff --git a/app/models/user.rb b/app/models/user.rb index de4031bd..b1e636d7 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -13,6 +13,7 @@ class User < ApplicationRecord :validatable, :confirmable, :authentication_keys => [:login] has_one_time_password + enum otp_module: { disabled: 0, enabled: 1 }, _prefix: true attr_accessor :otp_attempt, :otp_validation rolify 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 5fd89b8a..5b7ea48d 100644 --- a/db/migrate/20201001172537_add_otp_secret_key_to_users.rb +++ b/db/migrate/20201001172537_add_otp_secret_key_to_users.rb @@ -1,5 +1,10 @@ class AddOtpSecretKeyToUsers < ActiveRecord::Migration[5.2] def change add_column :users, :otp_secret_key, :string + add_column :users, :otp_module, :integer + + User.find_each do |user| + user.update_attribute(:otp_secret_key, User.otp_random_secret) + end end end diff --git a/db/schema.rb b/db/schema.rb index 71c4b9e0..4ad56134 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2020_10_01_172537) do +ActiveRecord::Schema.define(version: 2020_10_18_090453) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -274,6 +274,7 @@ ActiveRecord::Schema.define(version: 2020_10_01_172537) do t.boolean "export_processing", default: false, null: false t.datetime "export_created_at" t.string "otp_secret_key" + t.integer "otp_module" t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true t.index ["email"], name: "index_users_on_email", unique: true t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true From 4ce5dfc92a6410b7225cff51e550515233e4bda2 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Sun, 18 Oct 2020 19:48:12 +0200 Subject: [PATCH 03/27] Fix detaching, improve UI for attaching 2FA --- app/assets/stylesheets/application.scss | 1 + .../stylesheets/components/_totp-setup.scss | 35 +++++++++++++++++++ app/controllers/user_controller.rb | 11 ++++-- app/views/settings/_security.haml | 35 ++++++++++++++----- ...01001172537_add_otp_secret_key_to_users.rb | 2 +- 5 files changed, 71 insertions(+), 13 deletions(-) create mode 100644 app/assets/stylesheets/components/_totp-setup.scss diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index 768706ef..5b033152 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -91,6 +91,7 @@ "components/profile", "components/question", "components/smiles", +"components/totp-setup", "components/userbox"; /** diff --git a/app/assets/stylesheets/components/_totp-setup.scss b/app/assets/stylesheets/components/_totp-setup.scss new file mode 100644 index 00000000..633c91ec --- /dev/null +++ b/app/assets/stylesheets/components/_totp-setup.scss @@ -0,0 +1,35 @@ +.totp-setup { + display: flex; + + &__card { + background: var(--primary); + padding: 10px; + border-radius: 5px; + width: 256px; + } + + &__qr { + background: white; + border-radius: 5px; + } + + &__text { + background: #000; + color: #fff; + margin: 10px 0 0 0; + padding: 5px; + border-radius: 5px; + + code { + color: var(--warning); + } + } + + &__right { + margin-left: 20px; + } + + &__code-field { + font-family: "Monaco", "Inconsolata", "Cascadia Code", "Consolas", monospace; + } +} \ No newline at end of file diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index 7f9ce77a..8f513995 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -185,18 +185,23 @@ class UserController < ApplicationController def update_2fa req_params = params.require(:user).permit(:otp_secret_key, :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]) - flash[:success] = 'yay' + flash[:success] = 'Two factor authentication has been enabled for your account.' current_user.save! else - flash[:error] = current_user.otp_code + flash[:error] = 'The code you entered was invalid.' end redirect_to edit_user_security_path end def destroy_2fa - + current_user.otp_module = :disabled + current_user.save! + flash[:success] = 'Two factor authentication has been disabled for your account.' + redirect_to edit_user_security_path end end diff --git a/app/views/settings/_security.haml b/app/views/settings/_security.haml index e2d7046b..764d4a82 100644 --- a/app/views/settings/_security.haml +++ b/app/views/settings/_security.haml @@ -1,15 +1,32 @@ .card .card-body %h2= t('views.settings.security.2fa.title') - - if current_user.otp_secret_key.nil? - = bootstrap_form_for(current_user, url: { action: :update_2fa, method: :post }) do |f| - %a{:href => "https://play.google.com/store/apps/details?id=com.beemdevelopment.aegis"} Aegis Authenticator for Android - %a{:href => "https://apps.apple.com/gb/app/strongbox-authenticator/id1023839880"} Strongbox Authenticator for iOS - = RQRCode::QRCode.new(current_user.provisioning_uri("Retrospring:#{current_user.screen_name}", issuer: "Retrospring")).as_svg.html_safe - %pre= current_user.otp_secret_key - = f.text_field :otp_validation - = f.hidden_field :otp_secret_key, value: current_user.otp_secret_key - = f.submit t('views.actions.save'), class: 'btn btn-primary' + - if current_user.otp_module_disabled? + .totp-setup + .totp-setup__left + .totp-setup__card + .totp-setup__qr + = RQRCode::QRCode.new(current_user.provisioning_uri("Retrospring:#{current_user.screen_name}", issuer: "Retrospring")).as_svg({:offset => 4, :module_size => 4, :color => '000;fill:var(--primary)'}).html_safe + %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 + %a{:href => "https://play.google.com/store/apps/details?id=com.beemdevelopment.aegis"} + %i.fa.fa-android + Aegis Authenticator for Android + %li + %a{:href => "https://apps.apple.com/gb/app/strongbox-authenticator/id1023839880"} + %i.fa.fa-apple + Strongbox Authenticator for iOS + %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' - else %p= t('views.settings.security.2fa.enabled_hint') = link_to t('views.actions.remove'), destroy_user_2fa_path, :class => 'btn btn-primary', :method => 'delete' \ No newline at end of file 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 5b7ea48d..4e736ab8 100644 --- a/db/migrate/20201001172537_add_otp_secret_key_to_users.rb +++ b/db/migrate/20201001172537_add_otp_secret_key_to_users.rb @@ -1,7 +1,7 @@ class AddOtpSecretKeyToUsers < ActiveRecord::Migration[5.2] def change add_column :users, :otp_secret_key, :string - add_column :users, :otp_module, :integer + 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) From d550e6d4c7b7d478718e22e6ec57b7511fc7cb8a Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Sun, 18 Oct 2020 20:49:11 +0200 Subject: [PATCH 04/27] Add help text and confirmation for disabling 2FA --- app/views/settings/_security.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/settings/_security.haml b/app/views/settings/_security.haml index 764d4a82..aec88f17 100644 --- a/app/views/settings/_security.haml +++ b/app/views/settings/_security.haml @@ -28,5 +28,5 @@ = f.hidden_field :otp_secret_key, value: current_user.otp_secret_key = f.submit t('views.actions.save'), class: 'btn btn-primary' - else - %p= t('views.settings.security.2fa.enabled_hint') - = link_to t('views.actions.remove'), destroy_user_2fa_path, :class => 'btn btn-primary', :method => 'delete' \ No newline at end of file + %p Your account is set up to require the use of a one-time password in order to log in + = link_to t('views.actions.remove'), destroy_user_2fa_path, :class => 'btn btn-primary', :method => 'delete', :data => { confirm: "Are you sure you want to disable two-factor authentication?" } \ No newline at end of file From 5447e905c018b8f0232ea9d24f016cd0b99985f0 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Sun, 18 Oct 2020 20:49:30 +0200 Subject: [PATCH 05/27] Shorten 2FA setup OTP validation field --- app/assets/stylesheets/components/_totp-setup.scss | 1 + 1 file changed, 1 insertion(+) diff --git a/app/assets/stylesheets/components/_totp-setup.scss b/app/assets/stylesheets/components/_totp-setup.scss index 633c91ec..7e76bbfe 100644 --- a/app/assets/stylesheets/components/_totp-setup.scss +++ b/app/assets/stylesheets/components/_totp-setup.scss @@ -31,5 +31,6 @@ &__code-field { font-family: "Monaco", "Inconsolata", "Cascadia Code", "Consolas", monospace; + width: 86px; } } \ No newline at end of file From 00da21a13d268ee9dd39db8bbaa5357367a500c0 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Mon, 19 Oct 2020 12:20:44 +0200 Subject: [PATCH 06/27] Redirect away from two factor entry page if no target user is set in session --- app/controllers/user/sessions_controller.rb | 5 +++++ spec/controllers/user/sessions_controller_spec.rb | 14 ++++++++++++++ 2 files changed, 19 insertions(+) create mode 100644 spec/controllers/user/sessions_controller_spec.rb diff --git a/app/controllers/user/sessions_controller.rb b/app/controllers/user/sessions_controller.rb index bb72cb2c..8930deee 100644 --- a/app/controllers/user/sessions_controller.rb +++ b/app/controllers/user/sessions_controller.rb @@ -27,6 +27,11 @@ class User::SessionsController < Devise::SessionsController end def two_factor_entry + unless session.has_key? :user_sign_in_uid + redirect_to root_url + return + end + self.resource = User.find(session[:user_sign_in_uid]) render 'auth/two_factor_authentication' end diff --git a/spec/controllers/user/sessions_controller_spec.rb b/spec/controllers/user/sessions_controller_spec.rb new file mode 100644 index 00000000..fcca30a2 --- /dev/null +++ b/spec/controllers/user/sessions_controller_spec.rb @@ -0,0 +1,14 @@ +require 'rails_helper' + +describe User::SessionsController do + before do + @request.env["devise.mapping"] = Devise.mappings[:user] + end + + describe "#two_factor_entry" do + subject { get :two_factor_entry } + it "redirects back to the home page if no sign in target is set" do + expect(subject).to redirect_to :root + end + end +end \ No newline at end of file From 433f1d45e566c7ae1cd78a832f9a36e0b7400a58 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Mon, 19 Oct 2020 14:56:13 +0200 Subject: [PATCH 07/27] Use controller for setting up QR Code --- app/controllers/user_controller.rb | 7 +++---- app/views/settings/_security.haml | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index 8f513995..7bfabe55 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -177,10 +177,9 @@ class UserController < ApplicationController current_user.otp_secret_key = User.otp_random_secret @provisioning_uri = current_user.provisioning_uri(nil, issuer: APP_CONFIG[:hostname]) - qr_code = RQRCode::QRCode.new(@provisioning_uri, :size => 12, :level => :h) - @qr_svg = qr_code.as_svg(offset: 0, color: '000', - shape_rendering: 'crispEdges', - module_size: 4) + qr_code = RQRCode::QRCode.new(current_user.provisioning_uri("Retrospring:#{current_user.screen_name}", issuer: "Retrospring")) + + @qr_svg = qr_code.as_svg({offset: 4, module_size: 4, color: '000;fill:var(--primary)'}).html_safe end def update_2fa diff --git a/app/views/settings/_security.haml b/app/views/settings/_security.haml index aec88f17..78e52403 100644 --- a/app/views/settings/_security.haml +++ b/app/views/settings/_security.haml @@ -6,7 +6,7 @@ .totp-setup__left .totp-setup__card .totp-setup__qr - = RQRCode::QRCode.new(current_user.provisioning_uri("Retrospring:#{current_user.screen_name}", issuer: "Retrospring")).as_svg({:offset => 4, :module_size => 4, :color => '000;fill:var(--primary)'}).html_safe + = @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(' ') @@ -29,4 +29,4 @@ = f.submit t('views.actions.save'), class: 'btn btn-primary' - else %p Your account is set up to require the use of a one-time password in order to log in - = link_to t('views.actions.remove'), destroy_user_2fa_path, :class => 'btn btn-primary', :method => 'delete', :data => { confirm: "Are you sure you want to disable two-factor authentication?" } \ No newline at end of file + = link_to t('views.actions.remove'), destroy_user_2fa_path, class: 'btn btn-primary', method: 'delete', data: { confirm: "Are you sure you want to disable two-factor authentication?" } \ No newline at end of file From 70b8053d15f66002ad7e4e85691f5f4ac4e9c625 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Mon, 19 Oct 2020 14:56:30 +0200 Subject: [PATCH 08/27] Add F-Droid & GitHub links for Android TOTP App --- app/views/settings/_security.haml | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/app/views/settings/_security.haml b/app/views/settings/_security.haml index 78e52403..91459589 100644 --- a/app/views/settings/_security.haml +++ b/app/views/settings/_security.haml @@ -16,13 +16,21 @@ 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 - %a{:href => "https://play.google.com/store/apps/details?id=com.beemdevelopment.aegis"} - %i.fa.fa-android - Aegis Authenticator for Android + %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 - %a{:href => "https://apps.apple.com/gb/app/strongbox-authenticator/id1023839880"} - %i.fa.fa-apple - Strongbox Authenticator for iOS + %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 %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 From dc88ac3f0661da0b01adb638ff0ccab2cce28462 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Mon, 19 Oct 2020 15:02:04 +0200 Subject: [PATCH 09/27] haml-lint fixes --- app/views/settings/_security.haml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/views/settings/_security.haml b/app/views/settings/_security.haml index 91459589..cba67b8b 100644 --- a/app/views/settings/_security.haml +++ b/app/views/settings/_security.haml @@ -20,21 +20,21 @@ 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 + %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 + %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 + %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 + %a{ href: 'https://apps.apple.com/gb/app/strongbox-authenticator/id1023839880' } App Store %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' - else %p Your account is set up to require the use of a one-time password in order to log in - = link_to t('views.actions.remove'), destroy_user_2fa_path, class: 'btn btn-primary', method: 'delete', data: { confirm: "Are you sure you want to disable two-factor authentication?" } \ No newline at end of file + = link_to t('views.actions.remove'), destroy_user_2fa_path, class: 'btn btn-primary', method: 'delete', data: { confirm: 'Are you sure you want to disable two-factor authentication?' } From ea99805da1ff4addae6527da2ac5ef2a6895ff86 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Mon, 19 Oct 2020 15:55:00 +0200 Subject: [PATCH 10/27] Fix remaining lint warnings --- app/views/discover/_userbox.haml | 5 +++-- app/views/settings/_security.haml | 14 ++++++++++++-- app/views/user/edit_security.haml | 2 +- app/views/user/security.html.erb | 0 config/locales/en.yml | 1 + 5 files changed, 17 insertions(+), 5 deletions(-) delete mode 100644 app/views/user/security.html.erb diff --git a/app/views/discover/_userbox.haml b/app/views/discover/_userbox.haml index b087ca2f..8ae7567f 100644 --- a/app/views/discover/_userbox.haml +++ b/app/views/discover/_userbox.haml @@ -14,9 +14,10 @@ = u.display_name %span.text-muted= u.screen_name %p.answerbox__question-text - - if type == 'new' + - case type + - when 'new' = t('views.discover.userbox.new', time: time_ago_in_words(u.created_at)) - - elsif type == 'most' + - when 'most' = t('views.discover.userbox.answers', questions: pluralize(a, t('views.general.question'))) - else = t('views.discover.userbox.questions', questions: pluralize(q, t('views.general.question'))) diff --git a/app/views/settings/_security.haml b/app/views/settings/_security.haml index cba67b8b..90e7dc27 100644 --- a/app/views/settings/_security.haml +++ b/app/views/settings/_security.haml @@ -6,7 +6,7 @@ .totp-setup__left .totp-setup__card .totp-setup__qr - = @qr_svg + = 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(' ') @@ -31,10 +31,20 @@ %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' - else %p Your account is set up to require the use of a one-time password in order to log in - = link_to t('views.actions.remove'), destroy_user_2fa_path, class: 'btn btn-primary', method: 'delete', data: { confirm: 'Are you sure you want to disable two-factor authentication?' } + = link_to t('views.actions.remove'), destroy_user_2fa_path, class: 'btn btn-primary', method: 'delete', + data: { confirm: t('views.settings.security.2fa.detach_confirm') } diff --git a/app/views/user/edit_security.haml b/app/views/user/edit_security.haml index 41d8ef4d..bcdebbea 100644 --- a/app/views/user/edit_security.haml +++ b/app/views/user/edit_security.haml @@ -1,4 +1,4 @@ -= render 'settings/security' += render 'settings/security', qr_svg: @qr_svg - provide(:title, generate_title('Security Settings')) - parent_layout 'user/settings' diff --git a/app/views/user/security.html.erb b/app/views/user/security.html.erb deleted file mode 100644 index e69de29b..00000000 diff --git a/config/locales/en.yml b/config/locales/en.yml index 95d98ea5..89d295f1 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -419,6 +419,7 @@ en: security: 2fa: title: "Two-factor authentication" + detach_confirm: "Are you sure you want to disable two-factor authentication?" modal: ask: title: "Ask your followers" From 3211f8f59b3d6776bbef64bcf9f89423f7bf763a Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Mon, 19 Oct 2020 20:25:18 +0200 Subject: [PATCH 11/27] Make OTP secret longer --- app/controllers/user_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index 7bfabe55..409dae43 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -174,7 +174,7 @@ class UserController < ApplicationController end def edit_security - current_user.otp_secret_key = User.otp_random_secret + current_user.otp_secret_key = User.otp_random_secret(26) @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")) From febcf347ee03e60a1bc7bd94a7d3426ef8c9ca41 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Tue, 20 Oct 2020 11:44:20 +0200 Subject: [PATCH 12/27] Add basic login form tests --- .../user/sessions_controller_spec.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/spec/controllers/user/sessions_controller_spec.rb b/spec/controllers/user/sessions_controller_spec.rb index fcca30a2..4b9d520d 100644 --- a/spec/controllers/user/sessions_controller_spec.rb +++ b/spec/controllers/user/sessions_controller_spec.rb @@ -5,8 +5,26 @@ describe User::SessionsController do @request.env["devise.mapping"] = Devise.mappings[:user] end + describe "#create" do + let(:user) { FactoryBot.create(:user, password: '/bin/animals64') } + + subject { post :create, params: { user: { login: user.email, password: user.password } } } + + it "logs in users without 2FA enabled without any further input" do + expect(subject).to redirect_to :root + end + + it "prompts users with 2FA enabled to enter a code" do + user.otp_module = :enabled + user.save + + expect(subject).to redirect_to :user_two_factor_entry + end + end + describe "#two_factor_entry" do subject { get :two_factor_entry } + it "redirects back to the home page if no sign in target is set" do expect(subject).to redirect_to :root end From 556050aa66980fa200cf23c30e674be3a6444e98 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Wed, 21 Oct 2020 13:44:00 +0200 Subject: [PATCH 13/27] Add tests for security settings page --- app/controllers/user_controller.rb | 10 ++-- app/views/settings/_security.haml | 47 +------------------ .../settings/security/_totp-enabled.haml | 3 ++ app/views/settings/security/_totp-setup.haml | 42 +++++++++++++++++ spec/controllers/user_controller_spec.rb | 26 +++++++++- 5 files changed, 78 insertions(+), 50 deletions(-) create mode 100644 app/views/settings/security/_totp-enabled.haml create mode 100644 app/views/settings/security/_totp-setup.haml diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index 409dae43..36e4d42b 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -174,12 +174,14 @@ class UserController < ApplicationController end def edit_security - current_user.otp_secret_key = User.otp_random_secret(26) + if current_user.otp_module_disabled? + current_user.otp_secret_key = User.otp_random_secret(26) - @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")) + @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")) - @qr_svg = qr_code.as_svg({offset: 4, module_size: 4, color: '000;fill:var(--primary)'}).html_safe + @qr_svg = qr_code.as_svg({offset: 4, module_size: 4, color: '000;fill:var(--primary)'}).html_safe + end end def update_2fa diff --git a/app/views/settings/_security.haml b/app/views/settings/_security.haml index 90e7dc27..5217bc6a 100644 --- a/app/views/settings/_security.haml +++ b/app/views/settings/_security.haml @@ -2,49 +2,6 @@ .card-body %h2= t('views.settings.security.2fa.title') - if current_user.otp_module_disabled? - .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' + = render partial: 'settings/security/totp-setup', locals: { qr_svg: qr_svg } - else - %p Your account is set up to require the use of a one-time password in order to log in - = link_to t('views.actions.remove'), destroy_user_2fa_path, class: 'btn btn-primary', method: 'delete', - data: { confirm: t('views.settings.security.2fa.detach_confirm') } + = render partial: 'settings/security/totp-enabled' diff --git a/app/views/settings/security/_totp-enabled.haml b/app/views/settings/security/_totp-enabled.haml new file mode 100644 index 00000000..37cbc70a --- /dev/null +++ b/app/views/settings/security/_totp-enabled.haml @@ -0,0 +1,3 @@ +%p Your account is set up to require the use of a one-time password in order to log in += link_to t('views.actions.remove'), destroy_user_2fa_path, class: 'btn btn-primary', method: 'delete', + data: { confirm: t('views.settings.security.2fa.detach_confirm') } \ No newline at end of file diff --git a/app/views/settings/security/_totp-setup.haml b/app/views/settings/security/_totp-setup.haml new file mode 100644 index 00000000..5f82701a --- /dev/null +++ b/app/views/settings/security/_totp-setup.haml @@ -0,0 +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' \ No newline at end of file diff --git a/spec/controllers/user_controller_spec.rb b/spec/controllers/user_controller_spec.rb index 08d2f53c..dea62bc3 100644 --- a/spec/controllers/user_controller_spec.rb +++ b/spec/controllers/user_controller_spec.rb @@ -3,7 +3,7 @@ require "rails_helper" describe UserController, type: :controller do - let(:user) { FactoryBot.create :user } + let(:user) { FactoryBot.create :user, otp_module: :disabled } describe "#edit" do subject { get :edit } @@ -63,4 +63,28 @@ describe UserController, type: :controller do end end end + + describe "#edit_security" do + subject { get :edit_security } + + context "user signed in" do + before(:each) { sign_in user } + render_views + + it "shows a setup form for users who don't have 2FA enabled" do + subject + expect(response).to have_rendered(:edit_security) + expect(response).to have_rendered(partial: 'settings/security/_totp-setup') + end + + it "shows the option to disable 2FA for users who have 2FA already enabled" do + user.otp_module = :enabled + user.save + + subject + expect(response).to have_rendered(:edit_security) + expect(response).to have_rendered(partial: 'settings/security/_totp-enabled') + end + end + end end From d3cc4212259d2c31fb2274b89bbf6a6b031ebbd2 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Wed, 21 Oct 2020 13:49:12 +0200 Subject: [PATCH 14/27] Rename settings partials to match naming conventions --- app/views/settings/_security.haml | 4 ++-- .../security/{_totp-enabled.haml => _totp_enabled.haml} | 0 .../settings/security/{_totp-setup.haml => _totp_setup.haml} | 0 spec/controllers/user_controller_spec.rb | 4 ++-- 4 files changed, 4 insertions(+), 4 deletions(-) rename app/views/settings/security/{_totp-enabled.haml => _totp_enabled.haml} (100%) rename app/views/settings/security/{_totp-setup.haml => _totp_setup.haml} (100%) diff --git a/app/views/settings/_security.haml b/app/views/settings/_security.haml index 5217bc6a..e9bfa3b4 100644 --- a/app/views/settings/_security.haml +++ b/app/views/settings/_security.haml @@ -2,6 +2,6 @@ .card-body %h2= t('views.settings.security.2fa.title') - if current_user.otp_module_disabled? - = render partial: 'settings/security/totp-setup', locals: { qr_svg: qr_svg } + = render partial: 'settings/totp_setup', locals: { qr_svg: qr_svg } - else - = render partial: 'settings/security/totp-enabled' + = render partial: 'settings/totp_enabled' diff --git a/app/views/settings/security/_totp-enabled.haml b/app/views/settings/security/_totp_enabled.haml similarity index 100% rename from app/views/settings/security/_totp-enabled.haml rename to app/views/settings/security/_totp_enabled.haml diff --git a/app/views/settings/security/_totp-setup.haml b/app/views/settings/security/_totp_setup.haml similarity index 100% rename from app/views/settings/security/_totp-setup.haml rename to app/views/settings/security/_totp_setup.haml diff --git a/spec/controllers/user_controller_spec.rb b/spec/controllers/user_controller_spec.rb index dea62bc3..b4bf4b01 100644 --- a/spec/controllers/user_controller_spec.rb +++ b/spec/controllers/user_controller_spec.rb @@ -74,7 +74,7 @@ describe UserController, type: :controller do it "shows a setup form for users who don't have 2FA enabled" do subject expect(response).to have_rendered(:edit_security) - expect(response).to have_rendered(partial: 'settings/security/_totp-setup') + expect(response).to have_rendered(partial: 'settings/security/_totp_setup') end it "shows the option to disable 2FA for users who have 2FA already enabled" do @@ -83,7 +83,7 @@ describe UserController, type: :controller do subject expect(response).to have_rendered(:edit_security) - expect(response).to have_rendered(partial: 'settings/security/_totp-enabled') + expect(response).to have_rendered(partial: 'settings/security/_totp_enabled') end end end From 68b1bbb908312e1975cae47ca0e605b83de6c156 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Wed, 21 Oct 2020 13:55:55 +0200 Subject: [PATCH 15/27] Fix bad refactor --- app/views/settings/_security.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/settings/_security.haml b/app/views/settings/_security.haml index e9bfa3b4..bc673ba7 100644 --- a/app/views/settings/_security.haml +++ b/app/views/settings/_security.haml @@ -2,6 +2,6 @@ .card-body %h2= t('views.settings.security.2fa.title') - if current_user.otp_module_disabled? - = render partial: 'settings/totp_setup', locals: { qr_svg: qr_svg } + = render partial: 'settings/security/totp_setup', locals: { qr_svg: qr_svg } - else - = render partial: 'settings/totp_enabled' + = render partial: 'settings/security/totp_enabled' From d89d7a0e7fb21c93cc7af2efc56f421fc729a332 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Wed, 21 Oct 2020 13:59:13 +0200 Subject: [PATCH 16/27] Add trailing new line to settings partials --- app/views/settings/security/_totp_enabled.haml | 2 +- app/views/settings/security/_totp_setup.haml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/settings/security/_totp_enabled.haml b/app/views/settings/security/_totp_enabled.haml index 37cbc70a..4c383227 100644 --- a/app/views/settings/security/_totp_enabled.haml +++ b/app/views/settings/security/_totp_enabled.haml @@ -1,3 +1,3 @@ %p Your account is set up to require the use of a one-time password in order to log in = link_to t('views.actions.remove'), destroy_user_2fa_path, class: 'btn btn-primary', method: 'delete', - data: { confirm: t('views.settings.security.2fa.detach_confirm') } \ No newline at end of file + data: { confirm: t('views.settings.security.2fa.detach_confirm') } diff --git a/app/views/settings/security/_totp_setup.haml b/app/views/settings/security/_totp_setup.haml index 5f82701a..3b60b589 100644 --- a/app/views/settings/security/_totp_setup.haml +++ b/app/views/settings/security/_totp_setup.haml @@ -39,4 +39,4 @@ %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' \ No newline at end of file + = f.submit t('views.actions.save'), class: 'btn btn-primary' From 55de0e45d23ca718741dd1c8106d525c13f59acf Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Wed, 21 Oct 2020 16:47:07 +0200 Subject: [PATCH 17/27] Add test for #update_2fa endpoint --- spec/controllers/user_controller_spec.rb | 47 ++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/spec/controllers/user_controller_spec.rb b/spec/controllers/user_controller_spec.rb index b4bf4b01..75d17031 100644 --- a/spec/controllers/user_controller_spec.rb +++ b/spec/controllers/user_controller_spec.rb @@ -87,4 +87,51 @@ describe UserController, type: :controller do end end end + + describe "#update_2fa" do + subject { post :update_2fa, params: update_params } + + context "user signed in" do + before(:each) { sign_in user } + + context "user enters the incorrect code" do + let(:update_params) do + { + user: { otp_secret_key: 'EJFNIJPYXXTCQSRTQY6AG7XQLAT2IDG5H7NGLJE3', + otp_validation: 123456 } + } + end + + it "shows an error if the user enters the incorrect code" do + Timecop.freeze(Time.at(1603290888)) do + subject + expect(response).to redirect_to :edit_user_security + end + end + end + + context "user enters the correct code" do + let(:update_params) do + { + user: { otp_secret_key: 'EJFNIJPYXXTCQSRTQY6AG7XQLAT2IDG5H7NGLJE3', + otp_validation: 187894 } + } + end + + it "enables 2FA for the logged in user" do + Timecop.freeze(Time.at(1603290888)) do + subject + expect(response).to redirect_to :edit_user_security + end + end + + it "shows an error if the user attempts to use the code once it has expired" do + Timecop.freeze(Time.at(1603290910)) do + subject + expect(flash[:error]).to eq 'The code you entered was invalid.' + end + end + end + end + end end From 482b7992a9e1d5b1d56efb224bec264083cf7561 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Wed, 21 Oct 2020 16:52:59 +0200 Subject: [PATCH 18/27] Add test for #destroy_2fa endpoint --- spec/controllers/user_controller_spec.rb | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/spec/controllers/user_controller_spec.rb b/spec/controllers/user_controller_spec.rb index 75d17031..9a18b75d 100644 --- a/spec/controllers/user_controller_spec.rb +++ b/spec/controllers/user_controller_spec.rb @@ -128,10 +128,28 @@ describe UserController, type: :controller do it "shows an error if the user attempts to use the code once it has expired" do Timecop.freeze(Time.at(1603290910)) do subject - expect(flash[:error]).to eq 'The code you entered was invalid.' + expect(flash[:error]).to eq('The code you entered was invalid.') end end end end end + + describe "#destroy_2fa" do + subject { delete :destroy_2fa } + + context "user signed in" do + before(:each) do + user.otp_module = :enabled + user.save + sign_in user + end + + it "disables 2FA for the logged in user" do + subject + user.reload + expect(user.otp_module_enabled?).to be_falsey + end + end + end end From d7a1750694821be583b2061f5a2ba54c069de638 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Fri, 23 Oct 2020 20:45:06 +0200 Subject: [PATCH 19/27] 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 From 66cccbb5d677d0dff5d7b83ac210fd7369b750c6 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Fri, 23 Oct 2020 20:58:42 +0200 Subject: [PATCH 20/27] Use the same string for 2FA failures --- app/controllers/user/sessions_controller.rb | 2 +- app/controllers/user_controller.rb | 4 ++-- config/locales/en.yml | 4 ++++ 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/app/controllers/user/sessions_controller.rb b/app/controllers/user/sessions_controller.rb index 58a18d94..a6fea348 100644 --- a/app/controllers/user/sessions_controller.rb +++ b/app/controllers/user/sessions_controller.rb @@ -22,7 +22,7 @@ class User::SessionsController < Devise::SessionsController continue_sign_in(resource, resource_name) else sign_out(resource) - flash[:error] = "Invalid code provided" + flash[:error] = t('views.auth.2fa.errors.invalid_code') redirect_to new_user_session_url end end diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index f8b25bfb..3caed751 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -190,10 +190,10 @@ class UserController < ApplicationController current_user.otp_module = :enabled if current_user.authenticate_otp(req_params[:otp_validation]) - flash[:success] = 'Two factor authentication has been enabled for your account.' + flash[:success] = I18n.t('views.auth.2fa.setup.success') current_user.save! else - flash[:error] = 'The code you entered was invalid.' + flash[:error] = t('views.auth.2fa.errors.invalid_code') end redirect_to edit_user_security_path diff --git a/config/locales/en.yml b/config/locales/en.yml index 89d295f1..68a2c8bc 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -452,3 +452,7 @@ en: 2fa: title: "Two-factor authentication" otp_field: "One-time password" + errors: + invalid_code: "" + setup: + success: "Two factor authentication has been enabled for your account." From 702156258cd42c573291d87d3caa18f7650abc2e Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Fri, 23 Oct 2020 21:00:06 +0200 Subject: [PATCH 21/27] Remove user/sessions#two_factor_entry --- app/controllers/user/sessions_controller.rb | 10 ---------- config/routes.rb | 3 +-- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/app/controllers/user/sessions_controller.rb b/app/controllers/user/sessions_controller.rb index a6fea348..2a88612a 100644 --- a/app/controllers/user/sessions_controller.rb +++ b/app/controllers/user/sessions_controller.rb @@ -31,16 +31,6 @@ class User::SessionsController < Devise::SessionsController end end - def two_factor_entry - unless session.has_key? :user_sign_in_uid - redirect_to root_url - return - end - - self.resource = User.find(session[:user_sign_in_uid]) - render 'auth/two_factor_authentication' - end - private def continue_sign_in(resource, resource_name) diff --git a/config/routes.rb b/config/routes.rb index e94f322d..757b520c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -47,9 +47,8 @@ Rails.application.routes.draw do devise_for :users, path: 'user', skip: [:sessions, :registrations] as :user do # :sessions - get 'sign_in' => 'devise/sessions#new', as: :new_user_session + get 'sign_in' => 'user/sessions#new', as: :new_user_session post 'sign_in' => 'user/sessions#create', as: :user_session - get 'otp_auth' => 'user/sessions#two_factor_entry', as: :user_two_factor_entry delete 'sign_out' => 'devise/sessions#destroy', as: :destroy_user_session # :registrations get 'settings/delete_account' => 'devise/registrations#cancel', as: :cancel_user_registration From 0f80bcef144583254a2949b05d31c7ca70a6f9bb Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Fri, 23 Oct 2020 21:01:00 +0200 Subject: [PATCH 22/27] Remove I18n. prefix --- app/controllers/user_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index 3caed751..544ede5f 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -190,7 +190,7 @@ class UserController < ApplicationController current_user.otp_module = :enabled if current_user.authenticate_otp(req_params[:otp_validation]) - flash[:success] = I18n.t('views.auth.2fa.setup.success') + flash[:success] = t('views.auth.2fa.setup.success') current_user.save! else flash[:error] = t('views.auth.2fa.errors.invalid_code') From 75c782705a2d84e576b3d9e1b9453429adafac54 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Fri, 23 Oct 2020 21:02:13 +0200 Subject: [PATCH 23/27] Add string for views.auth.2fa.errors.invalid_code --- config/locales/en.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index 68a2c8bc..0c2e5325 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -453,6 +453,6 @@ en: title: "Two-factor authentication" otp_field: "One-time password" errors: - invalid_code: "" + invalid_code: "The code you entered was invalid." setup: success: "Two factor authentication has been enabled for your account." From 7f4d6cdc0f854b41b9babd695434c04bfb0914d5 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Fri, 23 Oct 2020 21:05:04 +0200 Subject: [PATCH 24/27] Remove #two_factor_entry test --- spec/controllers/user/sessions_controller_spec.rb | 8 -------- 1 file changed, 8 deletions(-) diff --git a/spec/controllers/user/sessions_controller_spec.rb b/spec/controllers/user/sessions_controller_spec.rb index c4ed6451..ced9528c 100644 --- a/spec/controllers/user/sessions_controller_spec.rb +++ b/spec/controllers/user/sessions_controller_spec.rb @@ -22,12 +22,4 @@ describe User::SessionsController do expect(subject).to have_rendered('auth/two_factor_authentication') end end - - describe "#two_factor_entry" do - subject { get :two_factor_entry } - - it "redirects back to the home page if no sign in target is set" do - expect(subject).to redirect_to :root - end - end end \ No newline at end of file From 37d2b430237d136b2c1a1e0d6032b415f08adbc9 Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Sat, 24 Oct 2020 00:13:14 +0200 Subject: [PATCH 25/27] Apply styling to OTP attempt field --- app/assets/stylesheets/components/_totp-setup.scss | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/app/assets/stylesheets/components/_totp-setup.scss b/app/assets/stylesheets/components/_totp-setup.scss index 914af86e..67d0c932 100644 --- a/app/assets/stylesheets/components/_totp-setup.scss +++ b/app/assets/stylesheets/components/_totp-setup.scss @@ -1,3 +1,8 @@ +%totp-input { + font-family: "Monaco", "Inconsolata", "Cascadia Code", "Consolas", monospace; + width: 86px; +} + .totp-setup { &__card { background: var(--primary); @@ -36,7 +41,10 @@ } &__code-field { - font-family: "Monaco", "Inconsolata", "Cascadia Code", "Consolas", monospace; - width: 86px; + @extend %totp-input; } +} + +#user_otp_attempt { + @extend %totp-input; } \ No newline at end of file From ee4b7e2cb1118e984d5e8667d5f227817498e55b Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Sat, 24 Oct 2020 00:13:26 +0200 Subject: [PATCH 26/27] Auto focus OTP validation field on setup page --- app/views/settings/security/_totp_setup.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/settings/security/_totp_setup.haml b/app/views/settings/security/_totp_setup.haml index 828ea749..a32d0661 100644 --- a/app/views/settings/security/_totp_setup.haml +++ b/app/views/settings/security/_totp_setup.haml @@ -38,5 +38,5 @@ %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.text_field :otp_validation, class: 'totp-setup__code-field', label: 'Enter the code displayed in the app here:', autofocus: true = f.submit t('views.actions.save'), class: 'btn btn-primary' From d20f527d8cee64bc9a20a6ec59215f28f38db94c Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Sat, 24 Oct 2020 00:24:04 +0200 Subject: [PATCH 27/27] Add drift period --- app/controllers/user/sessions_controller.rb | 2 +- app/controllers/user_controller.rb | 2 +- config/justask.yml.example | 3 +++ 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/app/controllers/user/sessions_controller.rb b/app/controllers/user/sessions_controller.rb index 2a88612a..cd8d96bf 100644 --- a/app/controllers/user/sessions_controller.rb +++ b/app/controllers/user/sessions_controller.rb @@ -18,7 +18,7 @@ class User::SessionsController < Devise::SessionsController warden.lock! render 'auth/two_factor_authentication' else - if resource.authenticate_otp(params[:user][:otp_attempt]) + if resource.authenticate_otp(params[:user][:otp_attempt], drift: APP_CONFIG.fetch(:otp_drift_period, 30).to_i) continue_sign_in(resource, resource_name) else sign_out(resource) diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index 544ede5f..b2fc4b7c 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -189,7 +189,7 @@ class UserController < ApplicationController req_params = params.require(:user).permit(:otp_validation) current_user.otp_module = :enabled - if current_user.authenticate_otp(req_params[:otp_validation]) + if current_user.authenticate_otp(req_params[:otp_validation], drift: APP_CONFIG.fetch(:otp_drift_period, 30).to_i) flash[:success] = t('views.auth.2fa.setup.success') current_user.save! else diff --git a/config/justask.yml.example b/config/justask.yml.example index 57ce7c8a..280b871d 100644 --- a/config/justask.yml.example +++ b/config/justask.yml.example @@ -68,3 +68,6 @@ hcaptcha: enabled: false site_key: '' secret_key: '' + +# TOTP Drift period in seconds +otp_drift_period: 30