diff --git a/app/assets/stylesheets/components/_totp-setup.scss b/app/assets/stylesheets/components/_totp-setup.scss index 67d0c932..2bcf341b 100644 --- a/app/assets/stylesheets/components/_totp-setup.scss +++ b/app/assets/stylesheets/components/_totp-setup.scss @@ -1,6 +1,6 @@ %totp-input { font-family: "Monaco", "Inconsolata", "Cascadia Code", "Consolas", monospace; - width: 86px; + width: 100px; } .totp-setup { @@ -43,6 +43,32 @@ &__code-field { @extend %totp-input; } + + &__recovery { + &-container { + max-width: 455px; + } + + &-icon { + font-size: .75in; + } + + &-title { + text-align: left; + } + + &-codes { + display: grid; + grid-template-columns: 1fr 1fr; + padding: 0; + + li { + list-style-type: none; + font-size: 16px; + text-align: center; + } + } + } } #user_otp_attempt { diff --git a/app/controllers/user/sessions_controller.rb b/app/controllers/user/sessions_controller.rb index cd8d96bf..2cab208b 100644 --- a/app/controllers/user/sessions_controller.rb +++ b/app/controllers/user/sessions_controller.rb @@ -18,7 +18,16 @@ class User::SessionsController < Devise::SessionsController warden.lock! render 'auth/two_factor_authentication' else - if resource.authenticate_otp(params[:user][:otp_attempt], drift: APP_CONFIG.fetch(:otp_drift_period, 30).to_i) + if params[:user][:otp_attempt].length == 8 + found = TotpRecoveryCode.where(user_id: resource.id, code: params[:user][:otp_attempt].downcase).delete_all + if found == 1 + flash[:info] = "You have #{TotpRecoveryCode.where(user_id: resource.id).count} recovery codes remaining." + continue_sign_in(resource, resource_name) + else + flash[:error] = t('views.auth.2fa.errors.invalid_code') + redirect_to new_user_session_url + end + elsif 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 b2fc4b7c..0cb802cb 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -175,13 +175,14 @@ class UserController < ApplicationController def edit_security if current_user.otp_module_disabled? - current_user.otp_secret_key = User.otp_random_secret(26) + current_user.otp_secret_key = User.otp_random_secret(25) 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")) @qr_svg = qr_code.as_svg({offset: 4, module_size: 4, color: '000;fill:var(--primary)'}).html_safe + else + @recovery_code_count = current_user.totp_recovery_codes.count end end @@ -190,19 +191,27 @@ class UserController < ApplicationController current_user.otp_module = :enabled 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') + @recovery_keys = TotpRecoveryCode.generate_for(current_user) current_user.save! + + render 'settings/security/recovery_keys' else flash[:error] = t('views.auth.2fa.errors.invalid_code') + redirect_to edit_user_security_path end - - redirect_to edit_user_security_path end def destroy_2fa current_user.otp_module = :disabled current_user.save! + current_user.totp_recovery_codes.delete_all flash[:success] = 'Two factor authentication has been disabled for your account.' redirect_to edit_user_security_path end + + def reset_user_recovery_codes + current_user.totp_recovery_codes.delete_all + @recovery_keys = TotpRecoveryCode.generate_for(current_user) + render 'settings/security/recovery_keys' + end end diff --git a/app/models/totp_recovery_code.rb b/app/models/totp_recovery_code.rb new file mode 100644 index 00000000..846f52d8 --- /dev/null +++ b/app/models/totp_recovery_code.rb @@ -0,0 +1,11 @@ +class TotpRecoveryCode < ApplicationRecord + NUMBER_OF_CODES_TO_GENERATE = 16 + + belongs_to :user + + # @param user [User] + # @return [Array] + def self.generate_for(user) + TotpRecoveryCode.create!(Array.new(16) { {user: user, code: SecureRandom.base58(8).downcase} }) + end +end diff --git a/app/models/user.rb b/app/models/user.rb index b1e636d7..c0606081 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -43,6 +43,7 @@ class User < ApplicationRecord has_many :list_memberships, class_name: "ListMember", foreign_key: 'user_id', dependent: :destroy has_many :subscriptions, dependent: :destroy + has_many :totp_recovery_codes, dependent: :destroy has_one :theme, dependent: :destroy diff --git a/app/views/auth/two_factor_authentication.haml b/app/views/auth/two_factor_authentication.haml index 75583213..01547311 100644 --- a/app/views/auth/two_factor_authentication.haml +++ b/app/views/auth/two_factor_authentication.haml @@ -7,7 +7,7 @@ %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.text_field :otp_attempt, autofocus: true, label: 'Please enter the code from your authenticator app' = f.submit t('views.sessions.create'), class: 'btn btn-primary mt-3 mb-3' diff --git a/app/views/settings/_security.haml b/app/views/settings/_security.haml index bc673ba7..8d7f1f67 100644 --- a/app/views/settings/_security.haml +++ b/app/views/settings/_security.haml @@ -4,4 +4,4 @@ - if current_user.otp_module_disabled? = render partial: 'settings/security/totp_setup', locals: { qr_svg: qr_svg } - else - = render partial: 'settings/security/totp_enabled' + = render partial: 'settings/security/totp_enabled', locals: { recovery_code_count: recovery_code_count } diff --git a/app/views/settings/security/_totp_enabled.haml b/app/views/settings/security/_totp_enabled.haml index 4c383227..5e15f872 100644 --- a/app/views/settings/security/_totp_enabled.haml +++ b/app/views/settings/security/_totp_enabled.haml @@ -1,3 +1,6 @@ -%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', +%p Your account is set up to require the use of a one-time password in order to log in. +%p You currently have #{recovery_code_count} unused recovery codes. += link_to t('views.actions.remove'), destroy_user_2fa_path, class: 'btn btn-danger', method: 'delete', data: { confirm: t('views.settings.security.2fa.detach_confirm') } += link_to 'Re-generate recovery codes', reset_user_recovery_codes_path, class: 'btn btn-primary', method: 'delete', + data: { confirm: 'Are you sure? This will disable your previous set of recovery codes.' } diff --git a/app/views/settings/security/recovery_keys.haml b/app/views/settings/security/recovery_keys.haml new file mode 100644 index 00000000..c09e9164 --- /dev/null +++ b/app/views/settings/security/recovery_keys.haml @@ -0,0 +1,23 @@ +.container.container--main + .row.justify-content-center + .col-md-5.totp-setup__recovery-container + .card + .card-body + .d-none.d-print-block.totp-setup__recovery-icon + %i.fa.fa-comments + %h1.totp-setup__recovery-title Your Retrospring recovery codes + + %ul.totp-setup__recovery-codes + - @recovery_keys.each do |key| + %li + %code= key.code + .d-none.d-print-block These codes were generated #{Time.now.strftime('%F at %T %Z')} + .card-footer.d-print-none + We recommend storing these in a password manager or printing them out on paper. + %br + %a.btn{ onclick: 'print()' } + %i.fa.fa-print + Print + .d-none.d-print-block.text-success + %i.fa.fa-tree + Please consider the environment before printing this page. diff --git a/app/views/shared/_locales.haml b/app/views/shared/_locales.haml index 52e687d7..f1d8f1ff 100644 --- a/app/views/shared/_locales.haml +++ b/app/views/shared/_locales.haml @@ -1,4 +1,4 @@ -.container +.container.d-print-none .locales %span %a{ href: '#', id: 'locale-switch' } diff --git a/app/views/user/edit_security.haml b/app/views/user/edit_security.haml index bcdebbea..e68d2636 100644 --- a/app/views/user/edit_security.haml +++ b/app/views/user/edit_security.haml @@ -1,4 +1,4 @@ -= render 'settings/security', qr_svg: @qr_svg += render 'settings/security', qr_svg: @qr_svg, recovery_code_count: @recovery_code_count - provide(:title, generate_title('Security Settings')) - parent_layout 'user/settings' diff --git a/config/locales/en.yml b/config/locales/en.yml index 0c2e5325..1bb41e5e 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -265,6 +265,7 @@ en: done: "Done" y: "Yes" n: "No" + remove: "Remove" sessions: destroy: "Logout" create: "Sign in" diff --git a/config/routes.rb b/config/routes.rb index 757b520c..dc67cee9 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -70,6 +70,7 @@ Rails.application.routes.draw do 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 + match '/settings/security/recovery', to: 'user#reset_user_recovery_codes', via: :delete, as: :reset_user_recovery_codes # resources :services, only: [:index, :destroy] match '/settings/services', to: 'services#index', via: 'get', as: :services diff --git a/db/migrate/20201101155648_create_totp_recovery_codes.rb b/db/migrate/20201101155648_create_totp_recovery_codes.rb new file mode 100644 index 00000000..3b506256 --- /dev/null +++ b/db/migrate/20201101155648_create_totp_recovery_codes.rb @@ -0,0 +1,9 @@ +class CreateTotpRecoveryCodes < ActiveRecord::Migration[5.2] + def change + create_table :totp_recovery_codes do |t| + t.bigint :user_id + t.string :code, limit: 8 + end + add_index :totp_recovery_codes, [:user_id, :code] + end +end diff --git a/db/schema.rb b/db/schema.rb index 4ad56134..a297997d 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_18_090453) do +ActiveRecord::Schema.define(version: 2020_11_01_155648) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -219,6 +219,12 @@ ActiveRecord::Schema.define(version: 2020_10_18_090453) do t.index ["user_id", "created_at"], name: "index_themes_on_user_id_and_created_at" end + create_table "totp_recovery_codes", force: :cascade do |t| + t.bigint "user_id" + t.string "code", limit: 8 + t.index ["user_id", "code"], name: "index_totp_recovery_codes_on_user_id_and_code" + end + create_table "users", id: :bigint, default: -> { "gen_timestamp_id('users'::text)" }, force: :cascade do |t| t.string "email", default: "", null: false t.string "encrypted_password", default: "", null: false diff --git a/spec/controllers/user_controller_spec.rb b/spec/controllers/user_controller_spec.rb index 9a18b75d..d3789066 100644 --- a/spec/controllers/user_controller_spec.rb +++ b/spec/controllers/user_controller_spec.rb @@ -3,7 +3,9 @@ require "rails_helper" describe UserController, type: :controller do - let(:user) { FactoryBot.create :user, otp_module: :disabled } + let(:user) { FactoryBot.create :user, + otp_module: :disabled, + otp_secret_key: 'EJFNIJPYXXTCQSRTQY6AG7XQLAT2IDG5H7NGLJE3'} describe "#edit" do subject { get :edit } @@ -97,8 +99,7 @@ describe UserController, type: :controller do context "user enters the incorrect code" do let(:update_params) do { - user: { otp_secret_key: 'EJFNIJPYXXTCQSRTQY6AG7XQLAT2IDG5H7NGLJE3', - otp_validation: 123456 } + user: { otp_validation: 123456 } } end @@ -106,6 +107,7 @@ describe UserController, type: :controller do Timecop.freeze(Time.at(1603290888)) do subject expect(response).to redirect_to :edit_user_security + expect(flash[:error]).to eq('The code you entered was invalid.') end end end @@ -113,22 +115,24 @@ describe UserController, type: :controller do context "user enters the correct code" do let(:update_params) do { - user: { otp_secret_key: 'EJFNIJPYXXTCQSRTQY6AG7XQLAT2IDG5H7NGLJE3', - otp_validation: 187894 } + user: { otp_validation: 187894 } } end - it "enables 2FA for the logged in user" do + it "enables 2FA for the logged in user and generates recovery keys" do Timecop.freeze(Time.at(1603290888)) do subject - expect(response).to redirect_to :edit_user_security + expect(response).to have_rendered(:recovery_keys) + + expect(user.totp_recovery_codes.count).to be(TotpRecoveryCode::NUMBER_OF_CODES_TO_GENERATE) 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 + Timecop.freeze(Time.at(1603290950)) do subject - expect(flash[:error]).to eq('The code you entered was invalid.') + expect(response).to redirect_to :edit_user_security + expect(flash[:error]).to eq(I18n.t('views.auth.2fa.errors.invalid_code')) end end end @@ -142,13 +146,31 @@ describe UserController, type: :controller do before(:each) do user.otp_module = :enabled user.save - sign_in user + sign_in(user) end it "disables 2FA for the logged in user" do subject user.reload expect(user.otp_module_enabled?).to be_falsey + expect(user.totp_recovery_codes.count).to be(0) + end + end + end + + describe "#reset_user_recovery_codes" do + subject { delete :reset_user_recovery_codes } + + context "user signed in" do + before(:each) do + sign_in(user) + end + + it "regenerates codes on request" do + old_codes = user.totp_recovery_codes.pluck(:code) + subject + new_codes = user.totp_recovery_codes.pluck(:code) + expect(new_codes).not_to match_array(old_codes) end end end