From beb91e7eff495626df1d6c38ba598cc1e2032fbd Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 17 Dec 2019 10:33:51 +1000 Subject: [PATCH] FIX: require: false for rotp gem (#8540) The ROTP gem is only used in a very small amount of places in the app, we don't need to globally require it. Also set the Addressable gem to not have a specific version range, as it has not been a problem yet. Some slight refactoring of UserSecondFactor here too to use SecondFactorManager to avoid code repetition --- Gemfile | 4 ++-- Gemfile.lock | 2 +- app/controllers/users_controller.rb | 1 + app/models/concerns/second_factor_manager.rb | 8 +++++++- app/models/user_second_factor.rb | 7 ++++--- spec/components/concern/second_factor_manager_spec.rb | 6 +++--- spec/requests/session_controller_spec.rb | 1 + spec/requests/users_controller_spec.rb | 1 + spec/requests/users_email_controller_spec.rb | 1 + 9 files changed, 21 insertions(+), 10 deletions(-) diff --git a/Gemfile b/Gemfile index 52da8538fe1..205de4ccbe1 100644 --- a/Gemfile +++ b/Gemfile @@ -134,7 +134,7 @@ gem 'highline', '~> 1.7.0', require: false gem 'rack-protection' # security gem 'cbor', require: false gem 'cose', require: false -gem 'addressable', '~> 2.7.0' +gem 'addressable' # Gems used only for assets and not required in production environments by default. # Allow everywhere for now cause we are allowing asset debugging in production @@ -229,7 +229,7 @@ gem 'logster' gem 'sassc', '2.0.1', require: false gem "sassc-rails" -gem 'rotp' +gem 'rotp', require: false gem 'rqrcode' gem 'rubyzip', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 2c0d6bfe783..a66f25b462c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -437,7 +437,7 @@ DEPENDENCIES activemodel (= 6.0.1) activerecord (= 6.0.1) activesupport (= 6.0.1) - addressable (~> 2.7.0) + addressable annotate aws-sdk-s3 aws-sdk-sns diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index c08952c59af..17acb63fedf 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1218,6 +1218,7 @@ class UsersController < ApplicationController end def create_second_factor_totp + require 'rotp' if !defined? ROTP totp_data = ROTP::Base32.random secure_session["staged-totp-#{current_user.id}"] = totp_data qrcode_svg = RQRCode::QRCode.new(current_user.totp_provisioning_uri(totp_data)).as_svg( diff --git a/app/models/concerns/second_factor_manager.rb b/app/models/concerns/second_factor_manager.rb index 1f713ed5120..4fde41c08bd 100644 --- a/app/models/concerns/second_factor_manager.rb +++ b/app/models/concerns/second_factor_manager.rb @@ -6,6 +6,7 @@ module SecondFactorManager extend ActiveSupport::Concern def create_totp(opts = {}) + require_rotp UserSecondFactor.create!({ user_id: self.id, method: UserSecondFactor.methods[:totp], @@ -14,6 +15,7 @@ module SecondFactorManager end def get_totp_object(data) + require_rotp ROTP::TOTP.new(data, issuer: SiteSetting.title) end @@ -32,7 +34,7 @@ module SecondFactorManager last_used = totp.last_used.to_i end - authenticated = !token.blank? && totp.get_totp_object.verify( + authenticated = !token.blank? && totp.totp_object.verify( token, drift_ahead: TOTP_ALLOWED_DRIFT_SECONDS, drift_behind: TOTP_ALLOWED_DRIFT_SECONDS, @@ -132,4 +134,8 @@ module SecondFactorManager def hash_backup_code(code, salt) Pbkdf2.hash_password(code, salt, Rails.configuration.pbkdf2_iterations, Rails.configuration.pbkdf2_algorithm) end + + def require_rotp + require 'rotp' if !defined? ROTP + end end diff --git a/app/models/user_second_factor.rb b/app/models/user_second_factor.rb index 88dcfefd4e3..a5e34216dcc 100644 --- a/app/models/user_second_factor.rb +++ b/app/models/user_second_factor.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class UserSecondFactor < ActiveRecord::Base + include SecondFactorManager belongs_to :user scope :backup_codes, -> do @@ -22,12 +23,12 @@ class UserSecondFactor < ActiveRecord::Base ) end - def get_totp_object - ROTP::TOTP.new(self.data, issuer: SiteSetting.title) + def totp_object + get_totp_object(self.data) end def totp_provisioning_uri - get_totp_object.provisioning_uri(user.email) + totp_object.provisioning_uri(user.email) end end diff --git a/spec/components/concern/second_factor_manager_spec.rb b/spec/components/concern/second_factor_manager_spec.rb index 11edba1a303..9e6c845caf6 100644 --- a/spec/components/concern/second_factor_manager_spec.rb +++ b/spec/components/concern/second_factor_manager_spec.rb @@ -18,8 +18,8 @@ RSpec.describe SecondFactorManager do totp = another_user.create_totp(enabled: true) end.to change { UserSecondFactor.count }.by(1) - expect(totp.get_totp_object.issuer).to eq(SiteSetting.title) - expect(totp.get_totp_object.secret).to eq(another_user.reload.user_second_factors.totps.first.data) + expect(totp.totp_object.issuer).to eq(SiteSetting.title) + expect(totp.totp_object.secret).to eq(another_user.reload.user_second_factors.totps.first.data) end end @@ -46,7 +46,7 @@ RSpec.describe SecondFactorManager do freeze_time do expect(user.user_second_factors.totps.first.last_used).to eq(nil) - token = user.user_second_factors.totps.first.get_totp_object.now + token = user.user_second_factors.totps.first.totp_object.now expect(user.authenticate_totp(token)).to eq(true) expect(user.user_second_factors.totps.first.last_used).to eq_time(DateTime.now) diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index a815d4bd348..9cb114679c5 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'rails_helper' +require 'rotp' RSpec.describe SessionController do let(:email_token) { Fabricate(:email_token) } diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 7c7b370b085..8d02f52d126 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'rails_helper' +require 'rotp' describe UsersController do let(:user) { Fabricate(:user) } diff --git a/spec/requests/users_email_controller_spec.rb b/spec/requests/users_email_controller_spec.rb index 55bd65c62f5..618a5155527 100644 --- a/spec/requests/users_email_controller_spec.rb +++ b/spec/requests/users_email_controller_spec.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'rails_helper' +require 'rotp' describe UsersEmailController do