Require a second factor for elevation, not for holding admin

7 files changed, 56 insertions, 6 deletions

diff --git a/app/models/user.rb b/app/models/user.rb
index c3035a02..4b70ebe5 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -242,6 +242,7 @@ class User < ApplicationRecord
     def admin_needs_second_factor
       return unless roles.include?("admin")
       return if otp_secret.present?
+      return if persisted? && roles_in_database.to_a.include?("admin")
       errors.add(:roles, :admin_needs_otp)
     end
 end
diff --git a/config/locales/de.yml b/config/locales/de.yml
index fcceb023..93712574 100644
--- a/config/locales/de.yml
+++ b/config/locales/de.yml
@@ -598,7 +598,7 @@ de:
       code_mismatch_rescan: "Der Code hat nicht gepasst. Neu scannen oder auf den nächsten Code warten."
       wrong_password: "Falsches Passwort."
       wrong_credentials: "Passwort oder Code falsch — zweiter Faktor unverändert."
-      enabled: "Zweiter Faktor aktiviert. Der eben eingegebene Code ist verbraucht — für die Anmeldung auf den nächsten warten."
+      enabled: "Zweiter Faktor aktiviert. (Der eben eingegebene Code ist jetzt verbraucht.)"
       disabled: "Zweiter Faktor entfernt."
     users:
       created: "Benutzer %{login} angelegt"
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 81d9f4fb..cd502b31 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -558,7 +558,7 @@ en:
       code_mismatch_rescan: "That code did not match. Rescan or wait for the next code."
       wrong_password: "Wrong password."
       wrong_credentials: "Password or code wrong -- second factor unchanged."
-      enabled: "Second factor enabled. The code you just entered is spent -- wait for the next one before logging in with it."
+      enabled: "Second factor enabled. (The OTP code is spent now.)"
       disabled: "Second factor disabled."
     users:
       created: "User created %{login}"
diff --git a/lib/authenticated_system.rb b/lib/authenticated_system.rb
index 04d8051f..668436b5 100644
--- a/lib/authenticated_system.rb
+++ b/lib/authenticated_system.rb
@@ -25,9 +25,10 @@ module AuthenticatedSystem
     # Tied to is_admin? so losing the role closes the window at once, rather
     # than leaving a timestamp that would count again if the role returned.
     def elevated?
-      return false unless current_user&.is_admin?
-      session[:elevated_at].to_i > ELEVATION_MAX_AGE.ago.to_i
-    end
+    return false unless current_user&.is_admin?
+    return false unless current_user.otp_enrolled?
+    session[:elevated_at].to_i > ELEVATION_MAX_AGE.ago.to_i
+  end
 
     def elevation_expires_at
       return nil unless elevated?
diff --git a/lib/authenticated_test_helper.rb b/lib/authenticated_test_helper.rb
index 065a5f7d..483e6c88 100644
--- a/lib/authenticated_test_helper.rb
+++ b/lib/authenticated_test_helper.rb
@@ -6,6 +6,10 @@ module AuthenticatedTestHelper
   end
 
   def elevate_session!
-    session[:elevated_at] = Time.now.to_i
+    user = User.find_by(:id => @request.session[:user_id])
+    if user && !user.otp_enrolled?
+      user.update_column(:otp_secret, ROTP::Base32.random)
+    end
+    @request.session[:elevated_at] = Time.now.to_i
   end
 end
diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb
index 2dd0759a..0c517647 100644
--- a/test/controllers/users_controller_test.rb
+++ b/test/controllers/users_controller_test.rb
@@ -273,4 +273,16 @@ class UsersControllerTest < ActionController::TestCase
 
     assert_not user.reload.is_admin?
   end
+
+  test "clearing the factor closes an open elevation window" do
+    login_as :aaron
+    elevate_session!
+    get :new, params: { :locale => "de" }
+    assert_response :success
+
+    users(:aaron).update_column(:otp_secret, nil)
+
+    get :new, params: { :locale => "de" }
+    assert_redirected_to new_elevation_path
+  end
 end
diff --git a/test/models/user_otp_test.rb b/test/models/user_otp_test.rb
index 81f25575..2044ac18 100644
--- a/test/models/user_otp_test.rb
+++ b/test/models/user_otp_test.rb
@@ -74,6 +74,38 @@ class UserOtpTest < ActiveSupport::TestCase
     assert_equal @user.login, action.metadata["target_login"]
   end
 
+  test "an admin without a factor can start enrollment" do
+    admin = users(:aaron)
+    assert admin.is_admin?
+    assert_not admin.otp_enrolled?
+
+    uri = admin.begin_otp_enrollment!
+
+    assert admin.reload.otp_pending_secret.present?
+    assert uri.present?
+  end
+
+  test "an admin disabling their factor keeps the role" do
+    admin = users(:aaron)
+    admin.update_column(:otp_secret, ROTP::Base32.random)
+
+    admin.disable_otp!(:actor => admin)
+
+    assert_not admin.reload.otp_enrolled?
+    assert admin.is_admin?
+  end
+
+  test "a fellow admin can reset an admin's factor" do
+    admin = users(:aaron)
+    admin.update_column(:otp_secret, ROTP::Base32.random)
+    actor = users(:redella)
+
+    admin.disable_otp!(:actor => actor)
+
+    assert_not admin.reload.otp_enrolled?
+    assert_equal "otp_reset", NodeAction.last.action
+  end
+
   private
 
     def enroll!(user)