From 974062f16169f6d07f2289564be6d72610b8770e Mon Sep 17 00:00:00 2001 From: erdgeist Date: Tue, 4 Aug 2026 05:40:54 +0200 Subject: Witness every promotion and demotion made through the roles form --- app/controllers/users_controller.rb | 20 ++++++++-- app/helpers/node_actions_helper.rb | 12 ++++++ app/models/node_action.rb | 6 +++ app/models/user.rb | 51 ++++++++++++++++++++++++ config/locales/de.yml | 8 ++++ config/locales/en.yml | 9 ++++- test/controllers/users_controller_test.rb | 46 +++++++++++++++++++++ test/models/user_test.rb | 66 +++++++++++++++++++++++++++++++ 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 = {}) -- cgit v1.3