Merge pull request #152 from Retrospring/feature/totp-recovery-codes

TOTP Recovery Keys

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 {

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)

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

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<TotpRecoveryCode>]
+  def self.generate_for(user)
+    TotpRecoveryCode.create!(Array.new(16) { {user: user, code: SecureRandom.base58(8).downcase} })
+  end
+end

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
 

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'
 

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 }

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
+%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 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.' }

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.

app/views/shared/_locales.haml

@@ -1,4 +1,4 @@
-.container
+.container.d-print-none
   .locales
     %span
       %a{ href: '#', id: 'locale-switch' }

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'

config/locales/en.yml

@@ -265,6 +265,7 @@ en:
       done: "Done"
       y: "Yes"
       n: "No"
+      remove: "Remove"
     sessions:
       destroy: "Logout"
       create: "Sign in"

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

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

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

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',
+              user: { otp_validation: 123456 }
-                      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',
+              user: { otp_validation: 187894 }
-                      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