From a0cd4aeb391c56722d1823290e058d106797e2d5 Mon Sep 17 00:00:00 2001 From: Jenny Shen Date: Wed, 17 May 2023 23:44:46 -0400 Subject: [PATCH 1/3] Only include required methods in UserMultifactorMethods included do block --- .../concerns/user_multifactor_methods.rb | 86 +++++++++---------- 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/app/models/concerns/user_multifactor_methods.rb b/app/models/concerns/user_multifactor_methods.rb index 33d246c87af..c1bb8e91574 100644 --- a/app/models/concerns/user_multifactor_methods.rb +++ b/app/models/concerns/user_multifactor_methods.rb @@ -6,63 +6,63 @@ module UserMultifactorMethods include UserWebauthnMethods enum mfa_level: { disabled: 0, ui_only: 1, ui_and_api: 2, ui_and_gem_signin: 3 }, _prefix: :mfa + end - def mfa_enabled? - !mfa_disabled? - end + def mfa_enabled? + !mfa_disabled? + end - def mfa_gem_signin_authorized?(otp) - return true unless strong_mfa_level? || webauthn_credentials.present? - api_otp_verified?(otp) - end + def mfa_gem_signin_authorized?(otp) + return true unless strong_mfa_level? || webauthn_credentials.present? + api_otp_verified?(otp) + end - def mfa_recommended_not_yet_enabled? - mfa_recommended? && mfa_disabled? - end + def mfa_recommended_not_yet_enabled? + mfa_recommended? && mfa_disabled? + end - def mfa_recommended_weak_level_enabled? - mfa_recommended? && mfa_ui_only? - end + def mfa_recommended_weak_level_enabled? + mfa_recommended? && mfa_ui_only? + end - def mfa_required_not_yet_enabled? - mfa_required? && mfa_disabled? - end + def mfa_required_not_yet_enabled? + mfa_required? && mfa_disabled? + end - def mfa_required_weak_level_enabled? - mfa_required? && mfa_ui_only? - end + def mfa_required_weak_level_enabled? + mfa_required? && mfa_ui_only? + end - def ui_otp_verified?(otp) - otp = otp.to_s - return true if verify_totp(mfa_seed, otp) - return false unless mfa_recovery_codes.include? otp - mfa_recovery_codes.delete(otp) - save!(validate: false) - end + def ui_otp_verified?(otp) + otp = otp.to_s + return true if verify_totp(mfa_seed, otp) + return false unless mfa_recovery_codes.include? otp + mfa_recovery_codes.delete(otp) + save!(validate: false) + end - def api_otp_verified?(otp) - return true if verify_webauthn_otp(otp) - return true if ui_otp_verified?(otp) - false - end + def api_otp_verified?(otp) + return true if verify_webauthn_otp(otp) + return true if ui_otp_verified?(otp) + false + end - private + private - def strong_mfa_level? - mfa_ui_and_gem_signin? || mfa_ui_and_api? - end + def strong_mfa_level? + mfa_ui_and_gem_signin? || mfa_ui_and_api? + end - def mfa_recommended? - return false if strong_mfa_level? || mfa_required? + def mfa_recommended? + return false if strong_mfa_level? || mfa_required? - rubygems.mfa_recommended.any? - end + rubygems.mfa_recommended.any? + end - def mfa_required? - return false if strong_mfa_level? + def mfa_required? + return false if strong_mfa_level? - rubygems.mfa_required.any? - end + rubygems.mfa_required.any? end class_methods do From f8a5772d8c1e7a9466e911a209c547475f6f85da Mon Sep 17 00:00:00 2001 From: Jenny Shen Date: Wed, 17 May 2023 23:56:18 -0400 Subject: [PATCH 2/3] Rename ui_otp_verified? to ui_mfa_verified? --- .../email_confirmations_controller.rb | 2 +- app/controllers/multifactor_auths_controller.rb | 2 +- app/controllers/passwords_controller.rb | 2 +- app/controllers/sessions_controller.rb | 2 +- app/models/concerns/user_multifactor_methods.rb | 4 ++-- .../concerns/user_multifactor_methods_test.rb | 16 ++++++++-------- test/models/user_test.rb | 12 ++++++------ 7 files changed, 20 insertions(+), 20 deletions(-) diff --git a/app/controllers/email_confirmations_controller.rb b/app/controllers/email_confirmations_controller.rb index 726054ad5e7..ff6eba684c8 100644 --- a/app/controllers/email_confirmations_controller.rb +++ b/app/controllers/email_confirmations_controller.rb @@ -97,7 +97,7 @@ def token_params end def mfa_update_conditions_met? - @user.mfa_enabled? && @user.ui_otp_verified?(params[:otp]) && session_active? + @user.mfa_enabled? && @user.ui_mfa_verified?(params[:otp]) && session_active? end def setup_mfa_authentication diff --git a/app/controllers/multifactor_auths_controller.rb b/app/controllers/multifactor_auths_controller.rb index 1ed2b4f623d..d9362ca6b32 100644 --- a/app/controllers/multifactor_auths_controller.rb +++ b/app/controllers/multifactor_auths_controller.rb @@ -27,7 +27,7 @@ def create end def update - if current_user.ui_otp_verified?(otp_param) + if current_user.ui_mfa_verified?(otp_param) handle_new_level_param redirect_to session.fetch("mfa_redirect_uri", edit_settings_path) session.delete("mfa_redirect_uri") diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb index 5cb7e395169..ff05b24d956 100644 --- a/app/controllers/passwords_controller.rb +++ b/app/controllers/passwords_controller.rb @@ -79,7 +79,7 @@ def setup_mfa_authentication end def mfa_edit_conditions_met? - @user.mfa_enabled? && @user.ui_otp_verified?(params[:otp]) && session_active? + @user.mfa_enabled? && @user.ui_mfa_verified?(params[:otp]) && session_active? end def login_failure(message) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 49ee998bfd2..f6f814723aa 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -143,7 +143,7 @@ def setup_mfa_authentication end def login_conditions_met? - @user&.mfa_enabled? && @user&.ui_otp_verified?(params[:otp]) && session_active? + @user&.mfa_enabled? && @user&.ui_mfa_verified?(params[:otp]) && session_active? end def record_mfa_login_duration(mfa_type:) diff --git a/app/models/concerns/user_multifactor_methods.rb b/app/models/concerns/user_multifactor_methods.rb index c1bb8e91574..d4d2018feab 100644 --- a/app/models/concerns/user_multifactor_methods.rb +++ b/app/models/concerns/user_multifactor_methods.rb @@ -33,7 +33,7 @@ def mfa_required_weak_level_enabled? mfa_required? && mfa_ui_only? end - def ui_otp_verified?(otp) + def ui_mfa_verified?(otp) otp = otp.to_s return true if verify_totp(mfa_seed, otp) return false unless mfa_recovery_codes.include? otp @@ -43,7 +43,7 @@ def ui_otp_verified?(otp) def api_otp_verified?(otp) return true if verify_webauthn_otp(otp) - return true if ui_otp_verified?(otp) + return true if ui_mfa_verified?(otp) false end diff --git a/test/models/concerns/user_multifactor_methods_test.rb b/test/models/concerns/user_multifactor_methods_test.rb index abf1700d0a1..48475d7c98a 100644 --- a/test/models/concerns/user_multifactor_methods_test.rb +++ b/test/models/concerns/user_multifactor_methods_test.rb @@ -214,36 +214,36 @@ class UserMultifactorMethodsTest < ActiveSupport::TestCase end end - context "#ui_otp_verified?" do + context "#ui_mfa_verified?" do setup do @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api) end context "with totp" do should "return true when correct" do - assert @user.ui_otp_verified?(ROTP::TOTP.new(@user.mfa_seed).now) + assert @user.ui_mfa_verified?(ROTP::TOTP.new(@user.mfa_seed).now) end should "return true when correct in last interval" do last_otp = ROTP::TOTP.new(@user.mfa_seed).at(Time.current - 30) - assert @user.ui_otp_verified?(last_otp) + assert @user.ui_mfa_verified?(last_otp) end should "return true when correct in next interval" do next_otp = ROTP::TOTP.new(@user.mfa_seed).at(Time.current + 30) - assert @user.ui_otp_verified?(next_otp) + assert @user.ui_mfa_verified?(next_otp) end should "return false when incorrect" do - refute @user.ui_otp_verified?(ROTP::TOTP.new(ROTP::Base32.random_base32).now) + refute @user.ui_mfa_verified?(ROTP::TOTP.new(ROTP::Base32.random_base32).now) end should "return false if the mfa_seed is blank" do @user.update!(mfa_seed: nil) - refute @user.ui_otp_verified?(ROTP::TOTP.new(ROTP::Base32.random_base32).now) + refute @user.ui_mfa_verified?(ROTP::TOTP.new(ROTP::Base32.random_base32).now) end end @@ -251,14 +251,14 @@ class UserMultifactorMethodsTest < ActiveSupport::TestCase should "return false" do webauthn_verification = create(:webauthn_verification, user: @user) - refute @user.ui_otp_verified?(webauthn_verification.otp) + refute @user.ui_mfa_verified?(webauthn_verification.otp) end end should "return true if recovery code is correct" do recovery_code = @user.mfa_recovery_codes.first - assert @user.ui_otp_verified?(recovery_code) + assert @user.ui_mfa_verified?(recovery_code) refute_includes @user.mfa_recovery_codes, recovery_code end end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 0b98b9b8cf4..2dfa99260de 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -360,12 +360,12 @@ class UserTest < ActiveSupport::TestCase should "be able to use a recovery code only once" do code = @user.mfa_recovery_codes.first - assert @user.ui_otp_verified?(code) - refute @user.ui_otp_verified?(code) + assert @user.ui_mfa_verified?(code) + refute @user.ui_mfa_verified?(code) end should "be able to verify correct OTP" do - assert @user.ui_otp_verified?(ROTP::TOTP.new(@user.mfa_seed).now) + assert @user.ui_mfa_verified?(ROTP::TOTP.new(@user.mfa_seed).now) end should "return true for mfa status check" do @@ -376,13 +376,13 @@ class UserTest < ActiveSupport::TestCase should "return true for otp in last interval" do last_otp = ROTP::TOTP.new(@user.mfa_seed).at(Time.current - 30) - assert @user.ui_otp_verified?(last_otp) + assert @user.ui_mfa_verified?(last_otp) end should "return true for otp in next interval" do next_otp = ROTP::TOTP.new(@user.mfa_seed).at(Time.current + 30) - assert @user.ui_otp_verified?(next_otp) + assert @user.ui_mfa_verified?(next_otp) end context "blocking user with api key" do @@ -414,7 +414,7 @@ class UserTest < ActiveSupport::TestCase end should "return false for verifying OTP" do - refute @user.ui_otp_verified?("") + refute @user.ui_mfa_verified?("") end should "return false for mfa status check" do From ef9c91b14bcc2225a80f6d45e1df000a5809fb53 Mon Sep 17 00:00:00 2001 From: Jenny Shen Date: Wed, 17 May 2023 23:57:18 -0400 Subject: [PATCH 3/3] Rename ui_otp_verified? to api_mfa_verified? --- app/models/api_key.rb | 2 +- .../concerns/user_multifactor_methods.rb | 4 ++-- .../concerns/user_multifactor_methods_test.rb | 22 +++++++++---------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/app/models/api_key.rb b/app/models/api_key.rb index 34fb74efe5f..a187729e86d 100644 --- a/app/models/api_key.rb +++ b/app/models/api_key.rb @@ -28,7 +28,7 @@ def enabled_scopes def mfa_authorized?(otp) return true unless mfa_enabled? - user.api_otp_verified?(otp) + user.api_mfa_verified?(otp) end def mfa_enabled? diff --git a/app/models/concerns/user_multifactor_methods.rb b/app/models/concerns/user_multifactor_methods.rb index d4d2018feab..21e1bf28c68 100644 --- a/app/models/concerns/user_multifactor_methods.rb +++ b/app/models/concerns/user_multifactor_methods.rb @@ -14,7 +14,7 @@ def mfa_enabled? def mfa_gem_signin_authorized?(otp) return true unless strong_mfa_level? || webauthn_credentials.present? - api_otp_verified?(otp) + api_mfa_verified?(otp) end def mfa_recommended_not_yet_enabled? @@ -41,7 +41,7 @@ def ui_mfa_verified?(otp) save!(validate: false) end - def api_otp_verified?(otp) + def api_mfa_verified?(otp) return true if verify_webauthn_otp(otp) return true if ui_mfa_verified?(otp) false diff --git a/test/models/concerns/user_multifactor_methods_test.rb b/test/models/concerns/user_multifactor_methods_test.rb index 48475d7c98a..7feec7eb926 100644 --- a/test/models/concerns/user_multifactor_methods_test.rb +++ b/test/models/concerns/user_multifactor_methods_test.rb @@ -263,30 +263,30 @@ class UserMultifactorMethodsTest < ActiveSupport::TestCase end end - context "#api_otp_verified?" do + context "#api_mfa_verified?" do setup do @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api) end context "with totp" do should "return true when correct" do - assert @user.api_otp_verified?(ROTP::TOTP.new(@user.mfa_seed).now) + assert @user.api_mfa_verified?(ROTP::TOTP.new(@user.mfa_seed).now) end should "return true when correct in last interval" do last_otp = ROTP::TOTP.new(@user.mfa_seed).at(Time.current - 30) - assert @user.api_otp_verified?(last_otp) + assert @user.api_mfa_verified?(last_otp) end should "return true when correct in next interval" do next_otp = ROTP::TOTP.new(@user.mfa_seed).at(Time.current + 30) - assert @user.api_otp_verified?(next_otp) + assert @user.api_mfa_verified?(next_otp) end should "return false if otp is incorrect" do - refute @user.api_otp_verified?(ROTP::TOTP.new(ROTP::Base32.random_base32).now) + refute @user.api_mfa_verified?(ROTP::TOTP.new(ROTP::Base32.random_base32).now) end end @@ -294,20 +294,20 @@ class UserMultifactorMethodsTest < ActiveSupport::TestCase should "return true when correct" do webauthn_verification = create(:webauthn_verification, user: @user) - assert @user.api_otp_verified?(webauthn_verification.otp) + assert @user.api_mfa_verified?(webauthn_verification.otp) end should "return false when incorrect" do create(:webauthn_verification, user: @user, otp: "jiEm2mm2sJtRqAVx") incorrect_otp = "Yxf57d1wEUSWyXrr" - refute @user.api_otp_verified?(incorrect_otp) + refute @user.api_mfa_verified?(incorrect_otp) end should "return false when expired" do webauthn_verification = create(:webauthn_verification, user: @user, otp_expires_at: 2.minutes.ago) - refute @user.api_otp_verified?(webauthn_verification.otp) + refute @user.api_mfa_verified?(webauthn_verification.otp) end context "when webauthn otp has not been generated" do @@ -316,11 +316,11 @@ class UserMultifactorMethodsTest < ActiveSupport::TestCase end should "return false for an otp" do - refute @user.api_otp_verified?("Yxf57d1wEUSWyXrr") + refute @user.api_mfa_verified?("Yxf57d1wEUSWyXrr") end should "return false if otp is nil" do - refute @user.api_otp_verified?(nil) + refute @user.api_mfa_verified?(nil) end end end @@ -328,7 +328,7 @@ class UserMultifactorMethodsTest < ActiveSupport::TestCase should "return true if recovery code is correct" do recovery_code = @user.mfa_recovery_codes.first - assert @user.api_otp_verified?(recovery_code) + assert @user.api_mfa_verified?(recovery_code) refute_includes @user.mfa_recovery_codes, recovery_code end end