Merge pull request #3905 from Shopify/eric/mfa-level-validation

User Validation on MFA Level

app/models/concerns/user_multifactor_methods.rb

@@ -6,6 +6,8 @@ module UserMultifactorMethods
     include UserWebauthnMethods
 
     enum mfa_level: { disabled: 0, ui_only: 1, ui_and_api: 2, ui_and_gem_signin: 3 }, _prefix: :mfa
+
+    validate :mfa_level_for_enabled_devices
   end
 
   def mfa_enabled?
@@ -20,6 +22,10 @@ def no_mfa_devices?
     totp_disabled? && webauthn_disabled?
   end
 
+  def mfa_devices_present?
+    !no_mfa_devices?
+  end
+
   def mfa_gem_signin_authorized?(otp)
     return true unless strong_mfa_level?
     api_mfa_verified?(otp)
@@ -73,6 +79,16 @@ def mfa_required?
     rubygems.mfa_required.any?
   end
 
+  def mfa_level_for_enabled_devices
+    return if correct_mfa_level_set_conditions
+
+    errors.add(:mfa_level, :invalid)
+  end
+
+  def correct_mfa_level_set_conditions
+    (mfa_disabled? && no_mfa_devices?) || (mfa_enabled? && mfa_devices_present?)
+  end
+
   class_methods do
     def without_mfa
       where(mfa_level: "disabled")

test/factories/user.rb

@@ -11,5 +11,29 @@
       mfa_level { User.mfa_levels["ui_and_api"] }
       mfa_recovery_codes { %w[aaa bbb ccc] }
     end
+
+    trait :disabled do
+      totp_seed { "" }
+      mfa_level { User.mfa_levels["disabled"] }
+      mfa_recovery_codes { [] }
+    end
+
+    trait :ui_only do
+      totp_seed { "123abc" }
+      mfa_level { User.mfa_levels["ui_only"] }
+      mfa_recovery_codes { %w[aaa bbb ccc] }
+    end
+
+    trait :ui_and_api do
+      totp_seed { "123abc" }
+      mfa_level { User.mfa_levels["ui_and_api"] }
+      mfa_recovery_codes { %w[aaa bbb ccc] }
+    end
+
+    trait :ui_and_gem_signin do
+      totp_seed { "123abc" }
+      mfa_level { User.mfa_levels["ui_and_gem_signin"] }
+      mfa_recovery_codes { %w[aaa bbb ccc] }
+    end
   end
 end

test/integration/gems_test.rb

@@ -103,7 +103,7 @@ class GemsSystemTest < SystemTest
 
   test "shows owners without mfa when logged in as owner" do
     @user.enable_totp!("some-seed", "ui_and_api")
-    user_without_mfa = create(:user, mfa_level: "disabled")
+    user_without_mfa = create(:user)
 
     create(:ownership, rubygem: @rubygem, user: @user)
     create(:ownership, rubygem: @rubygem, user: user_without_mfa)
@@ -116,7 +116,8 @@ class GemsSystemTest < SystemTest
 
   test "show mfa enabled when logged in as owner but everyone has mfa enabled" do
     @user.enable_totp!("some-seed", "ui_and_api")
-    user_with_mfa = create(:user, mfa_level: "ui_only")
+    user_with_mfa = create(:user)
+    user_with_mfa.enable_totp!("some-seed", "ui_and_api")
 
     create(:ownership, rubygem: @rubygem, user: @user)
     create(:ownership, rubygem: @rubygem, user: user_with_mfa)
@@ -129,7 +130,7 @@ class GemsSystemTest < SystemTest
 
   test "does not show owners without mfa when not logged in as owner" do
     @user.enable_totp!("some-seed", "ui_and_api")
-    user_without_mfa = create(:user, mfa_level: "disabled")
+    user_without_mfa = create(:user)
 
     create(:ownership, rubygem: @rubygem, user: @user)
     create(:ownership, rubygem: @rubygem, user: user_without_mfa)

test/jobs/mfa_usage_stats_job_test.rb

@@ -4,10 +4,15 @@ class MfaUsageStatsJobTest < ActiveJob::TestCase
   include StatsD::Instrument::Assertions
 
   setup do
-    create(:user, mfa_level: :disabled) # non-mfa user
-    2.times { create(:user, totp_seed: ROTP::Base32.random_base32) } # otp-only users
+    create(:user) # non-mfa user
+    2.times { create(:user).enable_totp!(ROTP::Base32.random_base32, :ui_and_api) } # otp-only users
     3.times { create(:webauthn_credential, user: create(:user)) } # webauthn-only users
-    4.times { create(:webauthn_credential, user: create(:user, totp_seed: ROTP::Base32.random_base32)) } # webauthn-and-otp users
+    # webauthn-and-otp users
+    4.times do
+      user = create(:user)
+      user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
+      create(:webauthn_credential, user: user)
+    end
   end
 
   test "it sends the count of non-MFA users to statsd" do

test/models/concerns/user_multifactor_methods_test.rb

@@ -7,6 +7,101 @@ class UserMultifactorMethodsTest < ActiveSupport::TestCase
     @user = create(:user)
   end
 
+  context "validation" do
+    context "#mfa_level_for_enabled_devices" do
+      context "when mfa_level is disabled" do
+        should "be valid if there no mfa devices" do
+          assert_predicate @user, :valid?
+          assert_predicate @user, :no_mfa_devices?
+        end
+
+        should "be invalid if totp is enabled" do
+          @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
+          @user.mfa_level = :disabled
+
+          refute_predicate @user, :valid?
+        end
+
+        should "be invalid if webauthn is enabled" do
+          create(:webauthn_credential, user: @user)
+          @user.mfa_level = :disabled
+
+          refute_predicate @user, :valid?
+        end
+
+        should "be invalid if both totp and webauthn are enabled" do
+          @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
+          create(:webauthn_credential, user: @user)
+          @user.mfa_level = :disabled
+
+          refute_predicate @user, :valid?
+        end
+      end
+
+      context "when mfa_level is ui_and_gem_signin" do
+        should "be invalid if there no mfa devices" do
+          @user.mfa_level = :ui_and_gem_signin
+
+          assert_predicate @user, :no_mfa_devices?
+          refute_predicate @user, :valid?
+        end
+
+        should "be valid if totp is enabled" do
+          @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
+          @user.mfa_level = :ui_and_gem_signin
+
+          assert_predicate @user, :valid?
+        end
+
+        should "be valid if webauthn is enabled" do
+          create(:webauthn_credential, user: @user)
+          @user.mfa_level = :ui_and_gem_signin
+
+          assert_predicate @user, :valid?
+        end
+
+        should "be valid if both totp and webauthn are enabled" do
+          @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
+          create(:webauthn_credential, user: @user)
+          @user.mfa_level = :ui_and_gem_signin
+
+          assert_predicate @user, :valid?
+        end
+      end
+
+      context "when mfa_level is ui_and_api" do
+        should "be invalid if there no mfa devices" do
+          @user.mfa_level = :ui_and_api
+
+          assert_predicate @user, :no_mfa_devices?
+          refute_predicate @user, :valid?
+        end
+
+        should "be valid if totp is enabled" do
+          @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
+          @user.mfa_level = :ui_and_api
+
+          assert_predicate @user, :valid?
+        end
+
+        should "be valid if webauthn is enabled" do
+          create(:webauthn_credential, user: @user)
+          @user.mfa_level = :ui_and_api
+
+          assert_predicate @user, :valid?
+        end
+
+        should "be valid if both totp and webauthn are enabled" do
+          @user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
+          create(:webauthn_credential, user: @user)
+          @user.mfa_level = :ui_and_api
+
+          assert_predicate @user, :valid?
+        end
+      end
+    end
+  end
+
   context "#mfa_enabled" do
     should "return true if multifactor auth is not disabled using totp" do
       @user.enable_totp!(ROTP::Base32.random_base32, :ui_only)
@@ -310,8 +405,8 @@ class UserMultifactorMethodsTest < ActiveSupport::TestCase
         refute @user.ui_mfa_verified?(ROTP::TOTP.new(ROTP::Base32.random_base32).now)
       end
 
-      should "return false if the totp_seed is blank" do
-        @user.update!(totp_seed: nil)
+      should "return false if the mfa_seed is blank" do
+        @user.disable_totp!
 
         refute @user.ui_mfa_verified?(ROTP::TOTP.new(ROTP::Base32.random_base32).now)
       end
@@ -405,7 +500,7 @@ class UserMultifactorMethodsTest < ActiveSupport::TestCase
 
   context ".without_mfa" do
     setup do
-      create(:user, mfa_level: :ui_and_api)
+      create(:user).enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
     end
 
     should "return instances without mfa" do

test/models/user_test.rb

@@ -579,8 +579,8 @@ class UserTest < ActiveSupport::TestCase
 
   context ".without_mfa" do
     setup do
-      create(:user, handle: "has_mfa", mfa_level: "ui_and_api")
-      create(:user, handle: "no_mfa", mfa_level: "disabled")
+      create(:user, handle: "has_mfa").enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
+      create(:user, handle: "no_mfa")
     end
 
     should "return only users without mfa" do

test/unit/helpers/rubygems_helper_test.rb

@@ -125,8 +125,9 @@ class RubygemsHelperTest < ActionView::TestCase
     end
 
     should "create links to gem owners without mfa" do
-      with_mfa = create(:user, mfa_level: "ui_and_api")
-      without_mfa = create_list(:user, 2, mfa_level: "disabled")
+      with_mfa = create(:user)
+      with_mfa.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
+      without_mfa = create_list(:user, 2)
       rubygem = create(:rubygem, owners: [*without_mfa, with_mfa])
 
       expected_links = without_mfa.sort_by(&:id).map do |u|

test/unit/mailer_test.rb

@@ -46,7 +46,9 @@ class MailerTest < ActionMailer::TestCase
     end
 
     should "send mail to users with with more than 180M+ downloads and have weak MFA" do
-      user = create(:user, mfa_level: "ui_only")
+      user = create(:user)
+      user.enable_totp!(ROTP::Base32.random_base32, :ui_only)
+
       create(:rubygem, owners: [user], downloads: MIN_DOWNLOADS_FOR_MFA_REQUIRED_POLICY)
 
       perform_enqueued_jobs only: ActionMailer::MailDeliveryJob do
@@ -64,7 +66,8 @@ class MailerTest < ActionMailer::TestCase
     end
 
     should "not send mail to users with with more than 180M+ downloads and have strong MFA" do
-      user = create(:user, mfa_level: "ui_and_api")
+      user = create(:user)
+      user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
       create(:rubygem, owners: [user], downloads: MIN_DOWNLOADS_FOR_MFA_REQUIRED_POLICY)
 
       perform_enqueued_jobs only: ActionMailer::MailDeliveryJob do
@@ -108,7 +111,8 @@ class MailerTest < ActionMailer::TestCase
     end
 
     should "send mail to users with more than 180M+ downloads and have weak MFA enabled" do
-      user = create(:user, mfa_level: "ui_only")
+      user = create(:user)
+      user.enable_totp!(ROTP::Base32.random_base32, :ui_only)
       create(:rubygem, owners: [user], downloads: MIN_DOWNLOADS_FOR_MFA_REQUIRED_POLICY)
 
       perform_enqueued_jobs only: ActionMailer::MailDeliveryJob do
@@ -126,7 +130,8 @@ class MailerTest < ActionMailer::TestCase
     end
 
     should "not send mail to users with more than 180M+ downloads and have strong MFA enabled" do
-      user = create(:user, mfa_level: "ui_and_api")
+      user = create(:user)
+      user.enable_totp!(ROTP::Base32.random_base32, :ui_and_api)
       create(:rubygem, owners: [user], downloads: MIN_DOWNLOADS_FOR_MFA_REQUIRED_POLICY)
 
       perform_enqueued_jobs only: ActionMailer::MailDeliveryJob do