Require a fresh second factor for user management

Administrative actions are gated behind a 30-minute elevation window:
creating and retiring accounts, editing roles, clearing a second factor.
Reading the list is not gated, and content work is untouched.

elevated? is tied to is_admin?, so losing the role closes the window at
once. The window opens when the second factor verifies at login, so an
admin heading straight for user management is already elevated, and closes
on logout with the other session state. Five wrong codes end the session,
mirroring the login challenge.

users#update carries no elevation filter, since self-service reaches it;
the role field is gated in user_params instead and fails closed.

11 files changed, 155 insertions, 3 deletions

diff --git a/app/controllers/concerns/role_required.rb b/app/controllers/concerns/role_required.rb
index b841b8cc..22ce625e 100644
--- a/app/controllers/concerns/role_required.rb
+++ b/app/controllers/concerns/role_required.rb
@@ -20,4 +20,13 @@ module RoleRequired
       flash[:error] = t("flash.common.#{key}")
       redirect_to admin_path
     end
+
+    # require_admin must precede this in the filter chain, so a non-admin is
+    # denied by role before elevation is ever considered.
+    def require_elevation
+      return if elevated?
+
+      session[:elevation_return_to] = request.fullpath
+      redirect_to new_elevation_path
+    end
 end
diff --git a/app/controllers/elevations_controller.rb b/app/controllers/elevations_controller.rb
new file mode 100644
index 00000000..804b8f95
--- /dev/null
+++ b/app/controllers/elevations_controller.rb
@@ -0,0 +1,44 @@
+# Step-up authentication for janitorial work. Mirrors the two-step login's
+# attempt cap: verify_otp! guards replay but not rate, and an attacker with a
+# stolen cookie would otherwise have unlimited tries at a six-digit code.
+class ElevationsController < ApplicationController
+  include RoleRequired
+
+  layout 'admin'
+
+  before_action :login_required
+  before_action :require_admin
+
+  MAX_ATTEMPTS = 5
+
+  def new
+    redirect_to admin_path if elevated?
+  end
+
+  def create
+    return render(:new) unless current_user.otp_enrolled?
+
+    session[:elevation_attempts] = session[:elevation_attempts].to_i + 1
+    if session[:elevation_attempts] > MAX_ATTEMPTS
+      logout_killing_session!
+      flash[:error] = t("flash.otp.too_many_attempts")
+      return redirect_to(login_path)
+    end
+
+    if current_user.verify_otp!(params[:code])
+      session.delete(:elevation_attempts)
+      elevate!
+      redirect_to safe_return_to(session.delete(:elevation_return_to),
+                                 :default => users_path)
+    else
+      flash.now[:error] = t("flash.otp.code_mismatch")
+      render :new
+    end
+  end
+
+  def destroy
+    drop_elevation!
+    flash[:notice] = t("flash.elevation.dropped")
+    redirect_to admin_path
+  end
+end
diff --git a/app/controllers/otp_challenges_controller.rb b/app/controllers/otp_challenges_controller.rb
index eeaeac20..87586241 100644
--- a/app/controllers/otp_challenges_controller.rb
+++ b/app/controllers/otp_challenges_controller.rb
@@ -28,6 +28,9 @@ class OtpChallengesController < ApplicationController
       reset_session
       self.current_user = user
       session[:logged_in_at] = Time.now.to_i
+      # an admin who logs in and goes straight to user management
+      # is already elevated
+      elevate! if user.is_admin?
       flash[:notice] = t("flash.common.logged_in")
       redirect_to safe_return_to(return_to, :default => admin_path)
     else
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 052b2928..583ebac0 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -7,6 +7,7 @@ class UsersController < ApplicationController
   before_action :login_required
   before_action :find_user,     :only => [:show, :edit, :update, :reset_otp, :deactivate, :reactivate]
   before_action :require_admin, :only => [:index, :new, :create, :reset_otp, :deactivate, :reactivate]
+  before_action :require_elevation, :only => [:new, :create, :reset_otp, :deactivate, :reactivate]
   before_action :verify_status, :except => [:index]
 
   layout 'admin'
@@ -87,7 +88,7 @@ class UsersController < ApplicationController
                                 :roles => [])
       # Checkbox arrays post a leading blank from the hidden field.
       permitted[:roles] = Array(permitted[:roles]).reject(&:blank?) if permitted.key?(:roles)
-      permitted.delete(:roles) unless current_user.is_admin?
+      permitted.delete(:roles) unless current_user.is_admin? && elevated?
       permitted
     end
 
diff --git a/app/views/elevations/new.html.erb b/app/views/elevations/new.html.erb
new file mode 100644
index 00000000..1091ff36
--- /dev/null
+++ b/app/views/elevations/new.html.erb
@@ -0,0 +1,23 @@
+<h1><%= t(".title") %></h1>
+
+<div id="admin_layout">
+  <% if current_user.otp_enrolled? %>
+    <p class="field_hint">
+      <%= t(".hint", :minutes => AuthenticatedSystem::ELEVATION_MAX_AGE.in_minutes.to_i) %>
+    </p>
+    <%= form_tag elevation_path do %>
+      <div class="layout_row_label"><%= t(".code") %></div>
+      <div class="layout_row_content">
+        <%= text_field_tag :code, nil, :autocomplete => "one-time-code",
+              :inputmode => "numeric", :autofocus => true %>
+      </div>
+      <div class="layout_row_label"></div>
+      <div class="layout_row_content"><%= submit_tag t(".elevate") %></div>
+    <% end %>
+  <% else %>
+    <%# An admin without an enrolled factor cannot elevate at all. Say so and
+        point at enrolment rather than showing a field that cannot work. %>
+    <p><%= t(".not_enrolled") %></p>
+    <%= link_to t(".enrol"), otp_enrollment_path, :class => "action_button" %>
+  <% end %>
+</div>
diff --git a/config/locales/de.yml b/config/locales/de.yml
index 04eb738e..7edffe2d 100644
--- a/config/locales/de.yml
+++ b/config/locales/de.yml
@@ -628,6 +628,8 @@ de:
       none_to_remove: "Es existiert keine %{lang}-Übersetzung zum Entfernen."
       last_translation: "Die einzige verbliebene Übersetzung kann nicht entfernt werden."
       removed: "%{lang}-Übersetzung aus dem Entwurf entfernt. Veröffentlichen, um das dauerhaft zu machen."
+    elevation:
+      dropped: "Administrative Rechte abgelegt."
 
   assets:
     index:
@@ -721,6 +723,15 @@ de:
     title_fields:
       falls_back: "Menu-Titel. Leere Titel werden aus deutsch übernommen"
 
+  elevations:
+    new:
+      title: "Administrative Rechte anfordern"
+      hint: "Für Benutzerverwaltung ist ein aktueller zweiter Faktor nötig. Die Rechte gelten dann %{minutes} Minuten."
+      code: "Aktueller Code"
+      elevate: "Rechte anfordern"
+      not_enrolled: "Für administrative Arbeiten ist ein zweiter Faktor erforderlich. Bitte zuerst einen einrichten."
+      enrol: "Zweiten Faktor einrichten"
+
   layouts:
     application:
       light_mode_aria: "Zwischen dunklem und hellem Modus wechseln"
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 2f7ac72f..9a8041bf 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -587,6 +587,8 @@ en:
       none_to_remove: "No %{lang} translation exists to remove."
       last_translation: "Can't remove the only remaining translation."
       removed: "%{lang} translation removed from the draft. Publish to make this permanent."
+    elevation:
+      dropped: "Administrative rights dropped."
 
   assets:
     index:
@@ -664,6 +666,15 @@ en:
       title: "Compare"
       default_marker: "default"
 
+  elevations:
+    new:
+      title: "Request administrative rights"
+      hint: "User management needs a current second factor. The rights then last %{minutes} minutes."
+      code: "Current code"
+      elevate: "Request rights"
+      not_enrolled: "Administrative work requires a second factor. Please set one up first."
+      enrol: "Set up a second factor"
+
   layouts:
     application:
       light_mode_aria: "Switch between dark and light mode"
diff --git a/config/routes.rb b/config/routes.rb
index 1898dbbb..4c37e70f 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -106,6 +106,7 @@ Cccms::Application.routes.draw do
     end
     resource :otp_enrollment, :only => [:show, :create, :update, :destroy]
     resource :otp_challenge, :only => [:new, :create]
+    resource :elevation, :only => [:new, :create, :destroy]
 
     resources :menu_items, :except => :show do
       member do
diff --git a/lib/authenticated_system.rb b/lib/authenticated_system.rb
index 4e70c28d..9a351dde 100644
--- a/lib/authenticated_system.rb
+++ b/lib/authenticated_system.rb
@@ -1,5 +1,6 @@
 module AuthenticatedSystem
   SESSION_MAX_AGE = 7.days
+  ELEVATION_MAX_AGE = 30.minutes
 
   protected
     # Returns true or false if the user is logged in.
@@ -20,6 +21,26 @@ module AuthenticatedSystem
       @current_user = new_user || false
     end
 
+    # 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
+
+    def elevation_expires_at
+      return nil unless elevated?
+      Time.at(session[:elevated_at].to_i) + ELEVATION_MAX_AGE
+    end
+
+    def elevate!
+      session[:elevated_at] = Time.now.to_i
+    end
+
+    def drop_elevation!
+      session.delete(:elevated_at)
+    end
+
     # Check if the user is authorized
     #
     # Override this method in your controllers if you want to restrict access
@@ -91,7 +112,8 @@ module AuthenticatedSystem
     # Inclusion hook to make #current_user and #logged_in?
     # available as ActionView helper methods.
     def self.included(base)
-      base.send :helper_method, :current_user, :logged_in?, :authorized? if base.respond_to? :helper_method
+      base.send :helper_method, :current_user, :logged_in?, :authorized?,
+                :elevated?, :elevation_expires_at if base.respond_to? :helper_method
     end
 
     #
@@ -123,7 +145,8 @@ module AuthenticatedSystem
     def logout_keeping_session!
       @current_user = false     # not logged in, and don't do it for me
       session[:user_id] = nil   # keeps the session but kill our variable
-      # explicitly kill any other session variables you set
+      session.delete(:elevated_at)
+      session.delete(:elevation_attempts)
     end
 
     # The session should only be reset at the tail end of a form POST --
diff --git a/lib/authenticated_test_helper.rb b/lib/authenticated_test_helper.rb
index 8f3a3732..065a5f7d 100644
--- a/lib/authenticated_test_helper.rb
+++ b/lib/authenticated_test_helper.rb
@@ -4,4 +4,8 @@ module AuthenticatedTestHelper
     @request.session[:user_id] = user ? users(user).id : nil
     @request.session[:logged_in_at] = Time.now.to_i
   end
+
+  def elevate_session!
+    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 fe099928..b6f0970d 100644
--- a/test/controllers/users_controller_test.rb
+++ b/test/controllers/users_controller_test.rb
@@ -19,6 +19,7 @@ class UsersControllerTest < ActionController::TestCase
  
   test "get new when logged in as admin" do
     login_as :aaron
+    elevate_session!
     get :new
     assert_response :success
   end
@@ -36,6 +37,7 @@ class UsersControllerTest < ActionController::TestCase
   
   test "creating new users being logged in as admin" do
     login_as :aaron
+    elevate_session!
     assert_difference "User.count", +1 do
       post :create, params: {
         :user => {
@@ -53,6 +55,7 @@ class UsersControllerTest < ActionController::TestCase
   
   test "creating new admin users being logged in as admin" do
     login_as :aaron
+    elevate_session!
     assert_difference "User.count", +1 do
       post :create, params: {
         :user => {
@@ -154,6 +157,7 @@ class UsersControllerTest < ActionController::TestCase
   
   test "an admin deactivates another user, who can no longer sign in" do
     login_as :aaron
+    elevate_session!
     user = users(:quentin)
 
     put :deactivate, params: { :id => user.id }
@@ -165,6 +169,7 @@ class UsersControllerTest < ActionController::TestCase
 
   test "reactivation restores the other roles untouched" do
     login_as :aaron
+    elevate_session!
     user = users(:quentin)
     user.update_column(:roles, ["alumni", "redaktion"])
 
@@ -189,6 +194,7 @@ class UsersControllerTest < ActionController::TestCase
   
   test "admin user can promote regular users to admins" do
     login_as :aaron
+    elevate_session!
     user = users(:quentin)
     put :update, params: { :id => user.id, :user => {:roles => ["admin", "redaktion"]} }
     
@@ -212,6 +218,7 @@ class UsersControllerTest < ActionController::TestCase
     assert user.reload.otp_enrolled?, "non-admin must be refused"
 
     login_as :aaron
+    elevate_session!
     put :reset_otp, params: { :id => user.id }
     assert_not user.reload.otp_enrolled?
     assert_equal "otp_reset", NodeAction.last.action
@@ -239,4 +246,19 @@ class UsersControllerTest < ActionController::TestCase
     get :show, params: { :id => users(:aaron).id }
     assert_redirected_to admin_path
   end
+
+  test "an admin without an open window is sent to elevation" do
+    login_as :aaron
+    get :new
+    assert_redirected_to new_elevation_path
+  end
+
+  test "an unelevated admin cannot change roles" do
+    login_as :aaron
+    user = users(:quentin)
+
+    put :update, params: { :id => user.id, :user => { :roles => ["admin"] } }
+
+    assert_not user.reload.is_admin?
+  end
 end