From aeb13964222aeda4e3578b75f4d4fd4aeca8a93b Mon Sep 17 00:00:00 2001 From: Dominik Kwiatek Date: Sun, 15 Nov 2020 10:21:06 +0100 Subject: [PATCH] Add tests for recovery codes --- app/controllers/user_controller.rb | 11 ++++--- app/models/totp_recovery_code.rb | 14 +++++++++ app/models/user.rb | 1 + spec/controllers/user_controller_spec.rb | 38 +++++++++++++++++++----- 4 files changed, 50 insertions(+), 14 deletions(-) diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index 8ca8588a..eca7ae09 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -175,10 +175,9 @@ 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 @@ -192,7 +191,7 @@ 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) - @recovery_keys = TotpRecoveryCode.create!(Array.new(10) { {user: current_user, code: SecureRandom.base58(8).downcase} }) + @recovery_keys = TotpRecoveryCode.generate_for(current_user) current_user.save! render 'settings/security/recovery_keys' @@ -205,14 +204,14 @@ class UserController < ApplicationController def destroy_2fa current_user.otp_module = :disabled current_user.save! - TotpRecoveryCode.where(user_id: current_user.id).delete_all + TotpRecoveryCode.remove_all_for(current_user) flash[:success] = 'Two factor authentication has been disabled for your account.' redirect_to edit_user_security_path end def reset_user_recovery_codes - TotpRecoveryCode.where(user_id: current_user.id).delete_all - @recovery_keys = TotpRecoveryCode.create!(Array.new(10) { {user: current_user, code: SecureRandom.base58(8).downcase} }) + TotpRecoveryCode.remove_all_for(current_user) + @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 index a83df0e4..b8a3b479 100644 --- a/app/models/totp_recovery_code.rb +++ b/app/models/totp_recovery_code.rb @@ -1,3 +1,17 @@ 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 + + # @param user [User] + # @return [Integer] + def self.remove_all_for(user) + TotpRecoveryCode.where(user: user).delete_all + 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/spec/controllers/user_controller_spec.rb b/spec/controllers/user_controller_spec.rb index 9a18b75d..4966f936 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,21 +115,23 @@ 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 subject + expect(response).to redirect_to :edit_user_security expect(flash[:error]).to eq('The code you entered was invalid.') 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