Witness every promotion and demotion made through the roles form

8 files changed, 214 insertions, 4 deletions

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 1bd436e5..b06d11fd 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -50,12 +50,26 @@ class UsersController < ApplicationController
     end
 
     permitted = user_params
+    desired   = permitted.key?(:roles) ? permitted.delete(:roles) : nil
+    refusals  = []
+    saved     = false
 
-    if @user.update(permitted)
+    User.transaction do
+      saved = @user.update(permitted)
+      raise ActiveRecord::Rollback unless saved
+
+      refusals = desired ? @user.update_roles!(desired, :actor => current_user) : []
+      raise ActiveRecord::Rollback if refusals.any?
+    end
+
+    if !saved
+      render :edit
+    elsif refusals.any?
+      flash.now[:error] = refusals.map { |r| t("flash.users.#{r}", :login => @user.login) }.to_sentence
+      render :edit
+    else
       flash[:notice] = t("flash.users.updated", :login => @user.login)
       redirect_to user_path(@user)
-    else
-      render :edit
     end
   end
 
diff --git a/app/helpers/node_actions_helper.rb b/app/helpers/node_actions_helper.rb
index 7dd55bdb..4cf990b8 100644
--- a/app/helpers/node_actions_helper.rb
+++ b/app/helpers/node_actions_helper.rb
@@ -24,6 +24,8 @@ module NodeActionsHelper
     "user_reactivate"    => "user-check",
     "redaktion_grant"    => "users-plus",
     "redaktion_revoke"   => "users-minus",
+    "admin_grant"        => "shield-plus",
+    "admin_revoke"       => "shield-minus",
     "event_create"       => "calendar-plus",
     "event_update"       => "calendar-event",
     "event_destroy"      => "calendar-x"
@@ -371,6 +373,16 @@ module NodeActionsHelper
        :target => user_participant_ref(action)).html_safe
   end
 
+  def summarize_admin_grant action
+    t("node_actions.admin_grant", :actor => actor_ref(action),
+       :target => user_participant_ref(action)).html_safe
+  end
+
+  def summarize_admin_revoke action
+    t("node_actions.admin_revoke", :actor => actor_ref(action),
+       :target => user_participant_ref(action)).html_safe
+  end
+
   def summarize_user_create action
     t("node_actions.user_create", :actor => actor_ref(action),
        :target => user_participant_ref(action)).html_safe
diff --git a/app/models/node_action.rb b/app/models/node_action.rb
index 0167762b..bfa469b1 100644
--- a/app/models/node_action.rb
+++ b/app/models/node_action.rb
@@ -105,6 +105,12 @@ class NodeAction < ApplicationRecord
   # otp_disable is self-service; otp_reset and all three account
   # verbs are an administrator acting on someone else, so actor and
   # participant differ:
+  #   "redaktion_grant" / "redaktion_revoke" / "admin_grant" /
+  #   "admin_revoke" -- role changes. Both pairs come from
+  #   User#grant_* / #revoke_*, so the roles form reaches them through
+  #   update_roles! rather than writing the attribute: witnessing is the
+  #   reason the form does not touch roles directly. Alumni changes record
+  #   as user_deactivate / user_reactivate, not as a role verb.
   #   "target_login" -- flat string, the affected account's login
   #
   # "event_create" / "event_update" / "event_destroy" (calendar
diff --git a/app/models/user.rb b/app/models/user.rb
index adfdc564..786f8d14 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -132,6 +132,7 @@ class User < ApplicationRecord
 
   def deactivate!(actor:)
     return false if alumni?
+    return false if actor == self
     transaction do
       update_column(:roles, (roles | ["alumni"]).sort)
       NodeAction.record!(:participants => [self], :user => actor,
@@ -174,6 +175,56 @@ class User < ApplicationRecord
     :revoked
   end
 
+  def grant_admin!(actor:)
+    return :already if is_admin?
+    return :no_second_factor unless otp_enrolled?
+
+    transaction do
+      update_column(:roles, (roles | ["admin"]).sort)
+      NodeAction.record!(:participants => [self], :user => actor,
+                          :action => "admin_grant", :target_login => login)
+    end
+    :granted
+  end
+
+  def revoke_admin!(actor:)
+    return :already unless is_admin?
+    return :self unless actor != self
+
+    transaction do
+      update_column(:roles, (roles - ["admin"]).sort)
+      NodeAction.record!(:participants => [self], :user => actor,
+                          :action => "admin_revoke", :target_login => login)
+    end
+    :revoked
+  end
+
+  def update_roles!(desired, actor:)
+    desired = Array(desired).map(&:to_s) & ROLES
+    refusals = []
+
+    transaction do
+      refusals << :admin_not_self         if is_admin?  && !desired.include?("admin")     && actor == self
+      refusals << :redaktion_not_self     if redaktion? && !desired.include?("redaktion") && actor == self
+      refusals << :cannot_deactivate_self if !alumni?   &&  desired.include?("alumni")    && actor == self
+      refusals << :admin_needs_otp        if !is_admin?  && desired.include?("admin")     && !otp_enrolled?
+      refusals << :redaktion_needs_otp    if !redaktion? && desired.include?("redaktion") && !otp_enrolled?
+
+      raise ActiveRecord::Rollback if refusals.any?
+
+      revoke_admin!(:actor => actor)     if is_admin?   && !desired.include?("admin")
+      revoke_redaktion!(:actor => actor) if redaktion?  && !desired.include?("redaktion")
+      reactivate!(:actor => actor)       if alumni?     && !desired.include?("alumni")
+
+      grant_admin!(:actor => actor)     if !is_admin?  && desired.include?("admin")
+      grant_redaktion!(:actor => actor) if !redaktion? && desired.include?("redaktion")
+
+      deactivate!(:actor => actor) if !alumni? && desired.include?("alumni")
+    end
+
+    refusals
+  end
+
   # otp_secret present == enrolled. otp_pending_secret holds the secret
   # between QR display and first-code confirmation. otp_consumed_timestep
   # makes every accepted code single-use (replay guard within the drift
diff --git a/config/locales/de.yml b/config/locales/de.yml
index 2139e0da..a1b29199 100644
--- a/config/locales/de.yml
+++ b/config/locales/de.yml
@@ -237,6 +237,8 @@ de:
     user_reactivate: "%{actor} hat %{target} reaktiviert"
     redaktion_grant: "%{actor} hat %{target} in die Redaktion aufgenommen"
     redaktion_revoke: "%{actor} hat %{target} aus der Redaktion entfernt"
+    admin_grant: "%{actor} hat %{target} zum Administrator gemacht"
+    admin_revoke: "%{actor} hat %{target} die Administratorenrechte entzogen"
     event_create: "%{actor} hat den Termin %{event} angelegt"
     event_create_on: "%{actor} hat den Termin %{event} unter %{subject} angelegt"
     event_update: "%{actor} hat den Termin %{event} geändert"
@@ -553,6 +555,10 @@ de:
     node_list:
       search_placeholder: "Titel, Abstract, Text durchsuchen…"
       no_revision: "keine"
+      flag_locked: "Gesperrt von %{login}"
+      flag_embargo: "Wird am %{date} veröffentlicht"
+      flag_draft: "Unveröffentlichter Entwurf"
+      flag_no_head: "Noch nie veröffentlicht"
     trashed:
       title: "Papierkorb"
       empty: "Der Papierkorb ist leer."
@@ -640,6 +646,8 @@ de:
       redaktion_revoked: "%{login} gehört nicht mehr zur Redaktion."
       redaktion_needs_otp: "%{login} braucht zuerst einen zweiten Faktor."
       redaktion_not_self: "Die eigene Redaktions-Rolle kann nicht abgegeben werden."
+      admin_not_self: "Du kannst dir die Administratorenrechte nicht selbst entziehen."
+      admin_needs_otp: "%{login} muss zuerst einen zweiten Faktor einrichten, um Administrator zu werden."
     assets:
       created: "Asset wurde angelegt."
       updated: "Asset wurde aktualisiert."
diff --git a/config/locales/en.yml b/config/locales/en.yml
index b4d6e4ae..fc011437 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -184,6 +184,8 @@ en:
     user_reactivate: "%{actor} reactivated %{target}"
     redaktion_grant: "%{actor} added %{target} to Redaktion"
     redaktion_revoke: "%{actor} removed %{target} from Redaktion"
+    admin_grant: "%{actor} made %{target} an administrator"
+    admin_revoke: "%{actor} removed administrator rights from %{target}"
     event_create: "%{actor} added the event %{event}"
     event_create_on: "%{actor} added the event %{event} to %{subject}"
     event_update: "%{actor} changed the event %{event}"
@@ -500,6 +502,10 @@ en:
     node_list:
       search_placeholder: "Search title, abstract, body…"
       no_revision: "none"
+      flag_locked: "Locked by %{login}"
+      flag_embargo: "Publishes %{date}"
+      flag_draft: "Unpublished draft"
+      flag_no_head: "Never published"
     trashed:
       title: "Trash"
       empty: "The Trash is empty."
@@ -602,7 +608,8 @@ en:
       redaktion_revoked: "%{login} is no longer part of Redaktion."
       redaktion_needs_otp: "%{login} needs a second factor first."
       redaktion_not_self: "You cannot give up your own Redaktion role."
-
+      admin_not_self: "You cannot remove your own administrator rights."
+      admin_needs_otp: "%{login} must enrol a second factor before becoming an administrator."
     assets:
       created: "Asset was successfully created."
       updated: "Asset was successfully updated."
diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb
index 69c535f5..aeff9bc7 100644
--- a/test/controllers/users_controller_test.rb
+++ b/test/controllers/users_controller_test.rb
@@ -357,4 +357,50 @@ class UsersControllerTest < ActionController::TestCase
     assert_redirected_to new_elevation_path
     assert_equal ["admin", "redaktion"], users(:aaron).reload.roles.sort
   end
+
+  test "promoting through the roles form leaves a log entry" do
+    login_as :aaron
+    elevate_session!
+    target = users(:redella)
+    target.update_column(:otp_secret, ROTP::Base32.random)
+
+    assert_difference -> { NodeAction.where(:action => "admin_grant").count }, 1 do
+      put :update, params: { :locale => "de", :id => target.id,
+                             :user => { :roles => ["", "redaktion", "admin"] } }
+    end
+
+    assert_redirected_to user_path(target)
+    assert_equal %w[admin redaktion], target.reload.roles.sort
+  end
+
+  test "a refused promotion re-renders with an error and changes nothing" do
+    login_as :aaron
+    elevate_session!
+    target = users(:quentin)
+
+    assert_no_difference -> { NodeAction.count } do
+      put :update, params: { :locale => "de", :id => target.id,
+                             :user => { :roles => ["", "admin"] } }
+    end
+
+    assert_response :success
+    assert_empty target.reload.roles
+    assert_not_nil flash[:error]
+    assert_nil flash[:notice]
+  end
+
+  test "a validation failure leaves no promotion behind" do
+    login_as :aaron
+    elevate_session!
+    target = users(:redella)
+    target.update_column(:otp_secret, ROTP::Base32.random)
+
+    assert_no_difference -> { NodeAction.count } do
+      put :update, params: { :locale => "de", :id => target.id,
+                             :user => { :email => "", :roles => ["", "redaktion", "admin"] } }
+    end
+
+    assert_response :success
+    assert_equal %w[redaktion], target.reload.roles
+  end
 end
diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index 62552ee7..5ccc53a9 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -6,6 +6,11 @@ class UserTest < ActiveSupport::TestCase
   include AuthenticatedTestHelper
   fixtures :users
 
+  def role_entries
+    NodeAction.where(:action => %w[admin_grant admin_revoke redaktion_grant
+                                   redaktion_revoke user_deactivate user_reactivate])
+  end
+
   def test_should_create_user
     assert_difference 'User.count' do
       user = create_user
@@ -187,6 +192,67 @@ class UserTest < ActiveSupport::TestCase
     redella.update_column(:last_login_at, nil)
     assert_equal :never, redella.staleness_tier(now)
   end
+
+  test "granting a role through update_roles! is witnessed" do
+    target = users(:redella)
+    target.update_column(:otp_secret, ROTP::Base32.random)
+
+    assert_difference -> { role_entries.count }, 1 do
+      assert_empty target.update_roles!(%w[redaktion admin], :actor => users(:aaron))
+    end
+
+    assert_equal %w[admin redaktion], target.reload.roles.sort
+    entry = role_entries.order(:id).last
+    assert_equal "admin_grant", entry.action
+    assert_equal users(:aaron).id, entry.user_id
+    assert_equal "redella", entry.metadata["target_login"]
+  end
+
+  test "a refused grant applies nothing at all" do
+    target = users(:quentin)
+    assert_not target.otp_enrolled?
+
+    assert_no_difference -> { role_entries.count } do
+      refusals = target.update_roles!(%w[redaktion admin], :actor => users(:aaron))
+      assert_includes refusals, :admin_needs_otp
+      assert_includes refusals, :redaktion_needs_otp
+    end
+
+    assert_empty target.reload.roles, "a refusal must leave the stored set untouched"
+  end
+
+  test "nobody demotes themselves through update_roles!" do
+    actor = users(:aaron)
+
+    refusals = actor.update_roles!([], :actor => actor)
+
+    assert_includes refusals, :admin_not_self
+    assert_includes refusals, :redaktion_not_self
+    assert_equal %w[admin redaktion], actor.reload.roles.sort
+  end
+
+  test "marking an account alumni through update_roles! is witnessed as a deactivation" do
+    target = users(:redella)
+
+    assert_difference -> { role_entries.where(:action => "user_deactivate").count }, 1 do
+      assert_empty target.update_roles!(%w[redaktion alumni], :actor => users(:aaron))
+    end
+
+    assert target.reload.alumni?
+    assert target.redaktion?, "deactivate! preserves the other roles"
+  end
+
+  test "revoking and granting in one submission apply in the right order" do
+    target = users(:redella)
+    target.update_column(:otp_secret, ROTP::Base32.random)
+
+    assert_empty target.update_roles!(%w[admin], :actor => users(:aaron))
+
+    assert_equal %w[admin], target.reload.roles
+    actions = role_entries.order(:id).last(2).map(&:action)
+    assert_includes actions, "redaktion_revoke"
+    assert_includes actions, "admin_grant"
+  end
   
 protected
   def create_user(options = {})