From df4ff9a8e13d776e1670c232655db0275a353a0f Mon Sep 17 00:00:00 2001 From: Patrick Figel Date: Sat, 15 Apr 2017 13:26:03 +0200 Subject: [PATCH] Add recovery code support for two-factor auth (#1773) * Add recovery code support for two-factor auth When users enable two-factor auth, the app now generates ten single-use recovery codes. Users are encouraged to print the codes and store them in a safe place. The two-factor prompt during login now accepts both OTP codes and recovery codes. The two-factor settings UI allows users to regenerated lost recovery codes. Users who have set up two-factor auth prior to this feature being added can use it to generate recovery codes for the first time. Fixes #563 and fixes #987 * Set OTP_SECRET in test enviroment * add missing .html to view file names --- .env.test | 1 + app/assets/stylesheets/lists.scss | 9 ++ app/controllers/auth/sessions_controller.rb | 3 +- .../settings/two_factor_auths_controller.rb | 10 +- app/models/user.rb | 4 +- app/views/auth/sessions/two_factor.html.haml | 4 +- .../_recovery_codes.html.haml | 7 ++ .../two_factor_auths/create.html.haml | 4 + .../two_factor_auths/recovery_codes.html.haml | 4 + .../settings/two_factor_auths/show.html.haml | 5 + config/initializers/devise.rb | 1 + config/locales/en.yml | 5 + config/locales/simple_form.en.yml | 2 + config/routes.rb | 1 + ...d_devise_two_factor_backupable_to_users.rb | 5 + db/schema.rb | 3 +- .../auth/sessions_controller_spec.rb | 93 +++++++++++++++++-- spec/models/user_spec.rb | 3 + 18 files changed, 149 insertions(+), 15 deletions(-) create mode 100644 app/views/settings/two_factor_auths/_recovery_codes.html.haml create mode 100644 app/views/settings/two_factor_auths/create.html.haml create mode 100644 app/views/settings/two_factor_auths/recovery_codes.html.haml create mode 100644 db/migrate/20170414080609_add_devise_two_factor_backupable_to_users.rb diff --git a/.env.test b/.env.test index b57f52e309b..e25c040ac04 100644 --- a/.env.test +++ b/.env.test @@ -1,3 +1,4 @@ # Federation LOCAL_DOMAIN=cb6e6126.ngrok.io LOCAL_HTTPS=true +OTP_SECRET=100c7faeef00caa29242f6b04156742bf76065771fd4117990c4282b8748ff3d99f8fdae97c982ab5bd2e6756a159121377cce4421f4a8ecd2d67bd7749a3fb4 diff --git a/app/assets/stylesheets/lists.scss b/app/assets/stylesheets/lists.scss index eac9f5a2ca7..13243aae5cd 100644 --- a/app/assets/stylesheets/lists.scss +++ b/app/assets/stylesheets/lists.scss @@ -6,3 +6,12 @@ margin: 0 5px; } } + +.recovery-codes { + column-count: 2; + height: 100px; + li { + list-style: decimal; + margin-left: 20px; + } +} diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb index 4184750f339..a187ae6daa1 100644 --- a/app/controllers/auth/sessions_controller.rb +++ b/app/controllers/auth/sessions_controller.rb @@ -49,7 +49,8 @@ class Auth::SessionsController < Devise::SessionsController end def valid_otp_attempt?(user) - user.validate_and_consume_otp!(user_params[:otp_attempt]) + user.validate_and_consume_otp!(user_params[:otp_attempt]) || + user.invalidate_otp_backup_code!(user_params[:otp_attempt]) end def authenticate_with_two_factor diff --git a/app/controllers/settings/two_factor_auths_controller.rb b/app/controllers/settings/two_factor_auths_controller.rb index 203d1fc46bb..bfe3868f3b7 100644 --- a/app/controllers/settings/two_factor_auths_controller.rb +++ b/app/controllers/settings/two_factor_auths_controller.rb @@ -19,9 +19,9 @@ class Settings::TwoFactorAuthsController < ApplicationController def create if current_user.validate_and_consume_otp!(confirmation_params[:code]) current_user.otp_required_for_login = true + @codes = current_user.generate_otp_backup_codes! current_user.save! - - redirect_to settings_two_factor_auth_path, notice: I18n.t('two_factor_auth.enabled_success') + flash[:notice] = I18n.t('two_factor_auth.enabled_success') else @confirmation = Form::TwoFactorConfirmation.new set_qr_code @@ -30,6 +30,12 @@ class Settings::TwoFactorAuthsController < ApplicationController end end + def recovery_codes + @codes = current_user.generate_otp_backup_codes! + current_user.save! + flash[:notice] = I18n.t('two_factor_auth.recovery_codes_regenerated') + end + def disable current_user.otp_required_for_login = false current_user.save! diff --git a/app/models/user.rb b/app/models/user.rb index d2aa5d809f8..27a38674e44 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -5,7 +5,9 @@ class User < ApplicationRecord devise :registerable, :recoverable, :rememberable, :trackable, :validatable, :confirmable, - :two_factor_authenticatable, otp_secret_encryption_key: ENV['OTP_SECRET'] + :two_factor_authenticatable, :two_factor_backupable, + otp_secret_encryption_key: ENV['OTP_SECRET'], + otp_number_of_backup_codes: 10 belongs_to :account, inverse_of: :user accepts_nested_attributes_for :account diff --git a/app/views/auth/sessions/two_factor.html.haml b/app/views/auth/sessions/two_factor.html.haml index 1deff82b2e3..3dec40c44ca 100644 --- a/app/views/auth/sessions/two_factor.html.haml +++ b/app/views/auth/sessions/two_factor.html.haml @@ -2,7 +2,9 @@ = t('auth.login') = simple_form_for(resource, as: resource_name, url: session_path(resource_name), method: :post) do |f| - = f.input :otp_attempt, type: :number, placeholder: t('simple_form.labels.defaults.otp_attempt'), input_html: { 'aria-label' => t('simple_form.labels.defaults.otp_attempt') }, required: true, autofocus: true, autocomplete: 'off' + = f.input :otp_attempt, type: :number, placeholder: t('simple_form.labels.defaults.otp_attempt'), + input_html: { 'aria-label' => t('simple_form.labels.defaults.otp_attempt') }, required: true, autofocus: true, autocomplete: 'off', + hint: t('simple_form.hints.sessions.otp') .actions = f.button :button, t('auth.login'), type: :submit diff --git a/app/views/settings/two_factor_auths/_recovery_codes.html.haml b/app/views/settings/two_factor_auths/_recovery_codes.html.haml new file mode 100644 index 00000000000..c23311e2a04 --- /dev/null +++ b/app/views/settings/two_factor_auths/_recovery_codes.html.haml @@ -0,0 +1,7 @@ +%p.hint= t('two_factor_auth.recovery_instructions') + +%h3= t('two_factor_auth.recovery_codes') +%ol.recovery-codes + - @codes.each do |code| + %li + %samp= code diff --git a/app/views/settings/two_factor_auths/create.html.haml b/app/views/settings/two_factor_auths/create.html.haml new file mode 100644 index 00000000000..8710b6e020f --- /dev/null +++ b/app/views/settings/two_factor_auths/create.html.haml @@ -0,0 +1,4 @@ +- content_for :page_title do + = t('settings.two_factor_auth') + += render 'recovery_codes' diff --git a/app/views/settings/two_factor_auths/recovery_codes.html.haml b/app/views/settings/two_factor_auths/recovery_codes.html.haml new file mode 100644 index 00000000000..8710b6e020f --- /dev/null +++ b/app/views/settings/two_factor_auths/recovery_codes.html.haml @@ -0,0 +1,4 @@ +- content_for :page_title do + = t('settings.two_factor_auth') + += render 'recovery_codes' diff --git a/app/views/settings/two_factor_auths/show.html.haml b/app/views/settings/two_factor_auths/show.html.haml index 047fe0c544e..bf19d24f134 100644 --- a/app/views/settings/two_factor_auths/show.html.haml +++ b/app/views/settings/two_factor_auths/show.html.haml @@ -8,3 +8,8 @@ = link_to t('two_factor_auth.disable'), disable_settings_two_factor_auth_path, data: { method: 'POST' }, class: 'block-button' - else = link_to t('two_factor_auth.setup'), new_settings_two_factor_auth_path, class: 'block-button' + +- if current_user.otp_required_for_login + .simple_form + %p.hint= t('two_factor_auth.lost_recovery_codes') + = link_to t('two_factor_auth.generate_recovery_codes'), recovery_codes_settings_two_factor_auth_path, data: { method: 'POST' }, class: 'block-button' diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 3c23e7b2e87..4754c2c8ca2 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -1,6 +1,7 @@ Devise.setup do |config| config.warden do |manager| manager.default_strategies(scope: :user).unshift :two_factor_authenticatable + manager.default_strategies(scope: :user).unshift :two_factor_backupable end # The secret key used by Devise. Devise uses this key to generate diff --git a/config/locales/en.yml b/config/locales/en.yml index e2f1873994d..474de398533 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -290,8 +290,13 @@ en: disable: Disable enable: Enable enabled_success: Two-factor authentication successfully enabled + generate_recovery_codes: Generate Recovery Codes instructions_html: "Scan this QR code into Google Authenticator or a similiar TOTP app on your phone. From now on, that app will generate tokens that you will have to enter when logging in." + lost_recovery_codes: Recovery codes allow you to regain access to your account if you lose your phone. If you've lost your recovery codes, you can regenerate them here. Your old recovery codes will be invalidated. manual_instructions: 'If you can''t scan the QR code and need to enter it manually, here is the plain-text secret:' + recovery_codes: Recovery Codes + recovery_codes_regenerated: Recovery codes successfully regenerated + recovery_instructions: If you ever lose access to your phone, you can use one of the recovery codes below to regain access to your account. Keep the recovery codes safe, for example by printing them and storing them with other important documents. setup: Set up warning: If you cannot configure an authenticator app right now, you should click "disable" or you won't be able to login. wrong_code: The entered code was invalid! Are server time and device time correct? diff --git a/config/locales/simple_form.en.yml b/config/locales/simple_form.en.yml index 74649da5103..c25407f2b5d 100644 --- a/config/locales/simple_form.en.yml +++ b/config/locales/simple_form.en.yml @@ -10,6 +10,8 @@ en: note: At most 160 characters imports: data: CSV file exported from another Mastodon instance + sessions: + otp: Enter the Two-factor code from your phone or use one of your recovery codes. labels: defaults: avatar: Avatar diff --git a/config/routes.rb b/config/routes.rb index 045be940e3d..8dcd4b330af 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -64,6 +64,7 @@ Rails.application.routes.draw do resource :two_factor_auth, only: [:show, :new, :create] do member do post :disable + post :recovery_codes end end end diff --git a/db/migrate/20170414080609_add_devise_two_factor_backupable_to_users.rb b/db/migrate/20170414080609_add_devise_two_factor_backupable_to_users.rb new file mode 100644 index 00000000000..65517d9f444 --- /dev/null +++ b/db/migrate/20170414080609_add_devise_two_factor_backupable_to_users.rb @@ -0,0 +1,5 @@ +class AddDeviseTwoFactorBackupableToUsers < ActiveRecord::Migration[5.0] + def change + add_column :users, :otp_backup_codes, :string, array: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 2decf5471f6..5f995ebdafb 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: 20170406215816) do +ActiveRecord::Schema.define(version: 20170414080609) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -313,6 +313,7 @@ ActiveRecord::Schema.define(version: 20170406215816) do t.integer "consumed_timestep" t.boolean "otp_required_for_login" t.datetime "last_emailed_at" + t.string "otp_backup_codes", array: true t.index ["account_id"], name: "index_users_on_account_id", using: :btree t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true, using: :btree t.index ["email"], name: "index_users_on_email", unique: true, using: :btree diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index c2a1fbe9100..0ec0b8f2f34 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/spec/controllers/auth/sessions_controller_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Auth::SessionsController, type: :controller do describe 'GET #new' do before do - request.env["devise.mapping"] = Devise.mappings[:user] + request.env['devise.mapping'] = Devise.mappings[:user] end it 'returns http success' do @@ -15,19 +15,94 @@ RSpec.describe Auth::SessionsController, type: :controller do end describe 'POST #create' do - let(:user) { Fabricate(:user, email: 'foo@bar.com', password: 'abcdefgh') } - before do - request.env["devise.mapping"] = Devise.mappings[:user] - post :create, params: { user: { email: user.email, password: user.password } } + request.env['devise.mapping'] = Devise.mappings[:user] end - it 'redirects to home' do - expect(response).to redirect_to(root_path) + context 'using password authentication' do + let(:user) { Fabricate(:user, email: 'foo@bar.com', password: 'abcdefgh') } + + context 'using a valid password' do + before do + post :create, params: { user: { email: user.email, password: user.password } } + end + + it 'redirects to home' do + expect(response).to redirect_to(root_path) + end + + it 'logs the user in' do + expect(controller.current_user).to eq user + end + end + + context 'using an invalid password' do + before do + post :create, params: { user: { email: user.email, password: 'wrongpw' } } + end + + it 'shows a login error' do + expect(flash[:alert]).to match I18n.t('devise.failure.invalid', authentication_keys: 'Email') + end + + it "doesn't log the user in" do + expect(controller.current_user).to be_nil + end + end end - it 'logs the user in' do - expect(controller.current_user).to eq user + context 'using two-factor authentication' do + let(:user) do + Fabricate(:user, email: 'x@y.com', password: 'abcdefgh', + otp_required_for_login: true, otp_secret: User.generate_otp_secret(32)) + end + let(:recovery_codes) do + codes = user.generate_otp_backup_codes! + user.save + return codes + end + + context 'using a valid OTP' do + before do + post :create, params: { user: { otp_attempt: user.current_otp } }, session: { otp_user_id: user.id } + end + + it 'redirects to home' do + expect(response).to redirect_to(root_path) + end + + it 'logs the user in' do + expect(controller.current_user).to eq user + end + end + + context 'using a valid recovery code' do + before do + post :create, params: { user: { otp_attempt: recovery_codes.first } }, session: { otp_user_id: user.id } + end + + it 'redirects to home' do + expect(response).to redirect_to(root_path) + end + + it 'logs the user in' do + expect(controller.current_user).to eq user + end + end + + context 'using an invalid OTP' do + before do + post :create, params: { user: { otp_attempt: 'wrongotp' } }, session: { otp_user_id: user.id } + end + + it 'shows a login error' do + expect(flash[:alert]).to match I18n.t('users.invalid_otp_token') + end + + it "doesn't log the user in" do + expect(controller.current_user).to be_nil + end + end end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index eb2a4aaeaf8..4ed1d5a0ac1 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1,6 +1,9 @@ require 'rails_helper' +require 'devise_two_factor/spec_helpers' RSpec.describe User, type: :model do + it_behaves_like 'two_factor_backupable' + describe 'validations' do it 'is invalid without an account' do user = Fabricate.build(:user, account: nil)