Merge pull request #3806 from rubygems/otp-mfa-concern

Move TOTP related methods to UserTotpMethods concern [1/4]

app/avo/actions/reset_user_2fa.rb

@@ -12,7 +12,7 @@ class ResetUser2fa < BaseAction
 
   class ActionHandler < ActionHandler
     def handle_model(user)
-      user.disable_mfa!
+      user.disable_totp!
       user.password = SecureRandom.hex(20).encode("UTF-8")
       user.save!
     end

app/controllers/multifactor_auths_controller.rb

@@ -14,7 +14,7 @@ def new
   end
 
   def create
-    current_user.verify_and_enable_mfa!(@seed, :ui_and_api, otp_param, @expire)
+    current_user.verify_and_enable_totp!(@seed, :ui_and_api, otp_param, @expire)
     if current_user.errors.any?
       flash[:error] = current_user.errors[:base].join
       redirect_to edit_settings_url
@@ -76,7 +76,7 @@ def handle_new_level_param
     case level_param
     when "disabled"
       flash[:success] = t("multifactor_auths.destroy.success")
-      current_user.disable_mfa!
+      current_user.disable_totp!
     when "ui_only"
       flash[:error] = t("multifactor_auths.ui_only_warning")
     else

app/models/concerns/user_multifactor_methods.rb

@@ -2,38 +2,14 @@ module UserMultifactorMethods
   extend ActiveSupport::Concern
 
   included do
+    include UserTotpMethods
+
     enum mfa_level: { disabled: 0, ui_only: 1, ui_and_api: 2, ui_and_gem_signin: 3 }, _prefix: :mfa
 
     def mfa_enabled?
       !mfa_disabled?
     end
 
-    def disable_mfa!
-      mfa_disabled!
-      self.mfa_seed = ""
-      self.mfa_recovery_codes = []
-      save!(validate: false)
-      Mailer.mfa_disabled(id, Time.now.utc).deliver_later
-    end
-
-    def verify_and_enable_mfa!(seed, level, otp, expiry)
-      if expiry < Time.now.utc
-        errors.add(:base, I18n.t("multifactor_auths.create.qrcode_expired"))
-      elsif verify_digit_otp(seed, otp)
-        enable_mfa!(seed, level)
-      else
-        errors.add(:base, I18n.t("multifactor_auths.incorrect_otp"))
-      end
-    end
-
-    def enable_mfa!(seed, level)
-      self.mfa_level = level
-      self.mfa_seed = seed
-      self.mfa_recovery_codes = Array.new(10).map { SecureRandom.hex(6) }
-      save!(validate: false)
-      Mailer.mfa_enabled(id, Time.now.utc).deliver_later
-    end
-
     def mfa_gem_signin_authorized?(otp)
       return true unless strong_mfa_level? || webauthn_credentials.present?
       api_otp_verified?(otp)
@@ -57,7 +33,7 @@ def mfa_required_weak_level_enabled?
 
     def ui_otp_verified?(otp)
       otp = otp.to_s
-      return true if verify_digit_otp(mfa_seed, otp)
+      return true if verify_totp(mfa_seed, otp)
       return false unless mfa_recovery_codes.include? otp
       mfa_recovery_codes.delete(otp)
       save!(validate: false)
@@ -87,15 +63,6 @@ def mfa_required?
       rubygems.mfa_required.any?
     end
 
-    def verify_digit_otp(seed, otp)
-      return false if seed.blank?
-
-      totp = ROTP::TOTP.new(seed)
-      return false unless totp.verify(otp, drift_behind: 30, drift_ahead: 30)
-
-      save!(validate: false)
-    end
-
     def verify_webauthn_otp(otp)
       webauthn_verification&.verify_otp(otp)
     end

app/models/concerns/user_totp_methods.rb

@@ -0,0 +1,40 @@
+module UserTotpMethods
+  extend ActiveSupport::Concern
+
+  def disable_totp!
+    mfa_disabled!
+    self.mfa_seed = ""
+    self.mfa_recovery_codes = []
+    save!(validate: false)
+    Mailer.mfa_disabled(id, Time.now.utc).deliver_later
+  end
+
+  def verify_and_enable_totp!(seed, level, otp, expiry)
+    if expiry < Time.now.utc
+      errors.add(:base, I18n.t("multifactor_auths.create.qrcode_expired"))
+    elsif verify_totp(seed, otp)
+      enable_totp!(seed, level)
+    else
+      errors.add(:base, I18n.t("multifactor_auths.incorrect_otp"))
+    end
+  end
+
+  def enable_totp!(seed, level)
+    self.mfa_level = level
+    self.mfa_seed = seed
+    self.mfa_recovery_codes = Array.new(10).map { SecureRandom.hex(6) }
+    save!(validate: false)
+    Mailer.mfa_enabled(id, Time.now.utc).deliver_later
+  end
+
+  private
+
+  def verify_totp(seed, otp)
+    return false if seed.blank?
+
+    totp = ROTP::TOTP.new(seed)
+    return false unless totp.verify(otp, drift_behind: 30, drift_ahead: 30)
+
+    save!(validate: false)
+  end
+end

app/models/user.rb

@@ -222,7 +222,7 @@ def block!
     transaction do
       update_attribute(:email, "security+locked-#{SecureRandom.hex(4)}-#{display_handle.downcase}@rubygems.org")
       confirm_email!
-      disable_mfa!
+      disable_totp!
       update_attribute(:password, SecureRandom.alphanumeric)
       update!(
         remember_token: nil,

script/disable_mfa

@@ -15,7 +15,7 @@ require_relative "../config/environment"
 begin
   user = User.find_by_name(name)
   puts "Found user: #{user.handle} #{user.email}"
-  user.disable_mfa!
+  user.disable_totp!
   user.password = SecureRandom.hex(20).encode("UTF-8")
   user.save!
   puts "Done."

test/functional/api/v1/api_keys_controller_test.rb

@@ -224,7 +224,7 @@ def self.should_expect_otp_for_update
     context "when user has enabled MFA for UI and API" do
       setup do
         @user = create(:user)
-        @user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_api)
+        @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
         authorize_with("#{@user.email}:#{@user.password}")
       end
 
@@ -234,7 +234,7 @@ def self.should_expect_otp_for_update
     context "when user has enabled MFA for UI and gem signin" do
       setup do
         @user = create(:user)
-        @user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_gem_signin)
+        @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_gem_signin)
         authorize_with("#{@user.email}:#{@user.password}")
       end
 
@@ -383,7 +383,7 @@ def self.should_expect_otp_for_update
     context "when a user provides an OTP code" do
       setup do
         @user = create(:user)
-        @user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_gem_signin)
+        @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_gem_signin)
         authorize_with("#{@user.email}:#{@user.password}")
         @request.env["HTTP_OTP"] = ROTP::TOTP.new(@user.mfa_seed).now
         post :create, params: { name: "test-key", index_rubygems: "true" }, format: "text"
@@ -431,7 +431,7 @@ def self.should_expect_otp_for_update
 
     context "when user has enabled MFA for UI and API" do
       setup do
-        @user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_api)
+        @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
         authorize_with("#{@user.email}:#{@user.password}")
       end
 
@@ -440,7 +440,7 @@ def self.should_expect_otp_for_update
 
     context "when user has enabled MFA for UI and gem signin" do
       setup do
-        @user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_gem_signin)
+        @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_gem_signin)
         authorize_with("#{@user.email}:#{@user.password}")
       end
 
@@ -473,7 +473,7 @@ def self.should_expect_otp_for_update
 
       context "by user on `ui_only` level" do
         setup do
-          @user.enable_mfa!(ROTP::Base32.random_base32, :ui_only)
+          @user.enable_totp!(ROTP::Base32.random_base32, :ui_only)
           post :create, params: { name: "test-key", index_rubygems: "true" }, format: "text"
         end
 
@@ -492,7 +492,7 @@ def self.should_expect_otp_for_update
 
       context "by user on `ui_and_gem_signin` level" do
         setup do
-          @user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_gem_signin)
+          @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_gem_signin)
           post :create, params: { name: "test-key", index_rubygems: "true" }, format: "text"
         end
 
@@ -505,7 +505,7 @@ def self.should_expect_otp_for_update
 
       context "by user on `ui_and_api` level" do
         setup do
-          @user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_api)
+          @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
           post :create, params: { name: "test-key", index_rubygems: "true" }, format: "text"
         end
 
@@ -579,7 +579,7 @@ def self.should_expect_otp_for_update
 
     context "when user has enabled MFA for UI and API" do
       setup do
-        @user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_api)
+        @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
         authorize_with("#{@user.email}:#{@user.password}")
       end
 
@@ -588,7 +588,7 @@ def self.should_expect_otp_for_update
 
     context "when user has enabled MFA for UI and gem signin" do
       setup do
-        @user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_gem_signin)
+        @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_gem_signin)
         authorize_with("#{@user.email}:#{@user.password}")
       end
 

test/functional/api/v1/deletions_controller_test.rb

@@ -40,7 +40,7 @@ class Api::V1::DeletionsControllerTest < ActionController::TestCase
 
       context "when mfa for UI and API is enabled" do
         setup do
-          @user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_api)
+          @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
         end
 
         context "ON DELETE to create for existing gem version without OTP" do
@@ -82,7 +82,7 @@ class Api::V1::DeletionsControllerTest < ActionController::TestCase
 
       context "when mfa for UI only is enabled" do
         setup do
-          @user.enable_mfa!(ROTP::Base32.random_base32, :ui_only)
+          @user.enable_totp!(ROTP::Base32.random_base32, :ui_only)
         end
 
         context "api key has mfa enabled" do
@@ -110,7 +110,7 @@ class Api::V1::DeletionsControllerTest < ActionController::TestCase
 
         context "when user has mfa enabled" do
           setup do
-            @user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_api)
+            @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
             @request.env["HTTP_OTP"] = ROTP::TOTP.new(@user.mfa_seed).now
             delete :create, params: { gem_name: @rubygem.to_param, version: @v1.number }
           end
@@ -202,7 +202,7 @@ class Api::V1::DeletionsControllerTest < ActionController::TestCase
 
         context "by user on `ui_only` level" do
           setup do
-            @user.enable_mfa!(ROTP::Base32.random_base32, :ui_only)
+            @user.enable_totp!(ROTP::Base32.random_base32, :ui_only)
             delete :create, params: { gem_name: @rubygem.name, version: @v1.number }
           end
 
@@ -222,7 +222,7 @@ class Api::V1::DeletionsControllerTest < ActionController::TestCase
 
         context "by user on `ui_and_gem_signin` level" do
           setup do
-            @user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_gem_signin)
+            @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_gem_signin)
             delete :create, params: { gem_name: @rubygem.name, version: @v1.number }
           end
 
@@ -235,7 +235,7 @@ class Api::V1::DeletionsControllerTest < ActionController::TestCase
 
         context "by user on `ui_and_api` level" do
           setup do
-            @user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_api)
+            @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
             @request.env["HTTP_OTP"] = ROTP::TOTP.new(@user.mfa_seed).now
             delete :create, params: { gem_name: @rubygem.name, version: @v1.number }
           end
@@ -304,7 +304,7 @@ class Api::V1::DeletionsControllerTest < ActionController::TestCase
 
         context "by user on `ui_only` mfa level" do
           setup do
-            @user.enable_mfa!(ROTP::Base32.random_base32, :ui_only)
+            @user.enable_totp!(ROTP::Base32.random_base32, :ui_only)
           end
 
           should "include change mfa level warning" do
@@ -327,7 +327,7 @@ class Api::V1::DeletionsControllerTest < ActionController::TestCase
 
         context "by user on `ui_and_gem_signin` mfa level" do
           setup do
-            @user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_gem_signin)
+            @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_gem_signin)
           end
 
           should "not include mfa warnings" do
@@ -345,7 +345,7 @@ class Api::V1::DeletionsControllerTest < ActionController::TestCase
 
         context "by user on `ui_and_api` mfa level" do
           setup do
-            @user.enable_mfa!(ROTP::Base32.random_base32, :ui_and_api)
+            @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
           end
 
           should "not include mfa warnings" do