From abd7ee1fc2ecc15b50944db30c59bedc26ec41b6 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Sat, 1 Aug 2026 04:14:00 +0200 Subject: Let Redaktion grant and revoke its own role Any holder may add or remove another account, witnessed as redaktion_grant/revoke so the vouching is legible. Not behind elevation: onboarding must not wait for a keyholder, and a compromised Redaktion account can already publish. --- app/controllers/users_controller.rb | 30 ++++++++++++++++++++++++++---- app/helpers/node_actions_helper.rb | 14 +++++++++++++- app/models/user.rb | 31 +++++++++++++++++++++++++++++++ app/views/users/_user.html.erb | 17 +++++++++++++++++ app/views/users/index.html.erb | 1 + config/locales/de.yml | 13 +++++++++++++ config/locales/en.yml | 15 +++++++++++++++ config/routes.rb | 2 ++ test/controllers/users_controller_test.rb | 20 ++++++++++++++++---- 9 files changed, 134 insertions(+), 9 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 583ebac0..9b9d64e5 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -5,17 +5,17 @@ class UsersController < ApplicationController # Private 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 :find_user, :only => [:show, :edit, :update, :reset_otp, :deactivate, :reactivate, :grant_redaktion, :revoke_redaktion] + before_action :require_redaktion, :only => [:index] + before_action :require_admin, :only => [:new, :create, :reset_otp, :deactivate, :reactivate] before_action :require_elevation, :only => [:new, :create, :reset_otp, :deactivate, :reactivate] - before_action :verify_status, :except => [:index] + before_action :verify_status, :except => [:index, :grant_redaktion, :revoke_redaktion] layout 'admin' ROLE_PRESETS = { "editor" => [], "redaktion" => ["redaktion"], - "admin" => ["admin", "redaktion"] }.freeze GROUP_ORDER = [:admin, :redaktion, :editor, :alumni].freeze @@ -74,6 +74,28 @@ class UsersController < ApplicationController redirect_to users_path end + def grant_redaktion + return deny_role_access(:redaktion_required) unless current_user.redaktion? + + case @user.grant_redaktion!(:actor => current_user) + when :granted then flash[:notice] = t("flash.users.redaktion_granted", :login => @user.login) + when :no_second_factor then flash[:error] = t("flash.users.redaktion_needs_otp", :login => @user.login) + end + + redirect_to users_path + end + + def revoke_redaktion + return deny_role_access(:redaktion_required) unless current_user.redaktion? + + case @user.revoke_redaktion!(:actor => current_user) + when :revoked then flash[:notice] = t("flash.users.redaktion_revoked", :login => @user.login) + when :self then flash[:error] = t("flash.users.redaktion_not_self") + end + + redirect_to users_path + end + def reset_otp @user.disable_otp!(:actor => current_user) flash[:notice] = t("flash.users.otp_reset", :login => @user.login) diff --git a/app/helpers/node_actions_helper.rb b/app/helpers/node_actions_helper.rb index f57ef84f..53f6ecd0 100644 --- a/app/helpers/node_actions_helper.rb +++ b/app/helpers/node_actions_helper.rb @@ -20,7 +20,9 @@ module NodeActionsHelper "otp_disable" => "shield-off", "otp_reset" => "shield-x", "user_deactivate" => "user-off", - "user_reactivate" => "user-check" + "user_reactivate" => "user-check", + "redaktion_grant" => "user-plus", + "redaktion_revoke" => "user-minus" }.freeze def verb_icon action @@ -283,4 +285,14 @@ module NodeActionsHelper t("node_actions.user_reactivate", :actor => actor_ref(action), :target => user_participant_ref(action)).html_safe end + + def summarize_redaktion_grant action + t("node_actions.redaktion_grant", :actor => actor_ref(action), + :target => user_participant_ref(action)).html_safe + end + + def summarize_redaktion_revoke action + t("node_actions.redaktion_revoke", :actor => actor_ref(action), + :target => user_participant_ref(action)).html_safe + end end diff --git a/app/models/user.rb b/app/models/user.rb index bf0f40ee..c3035a02 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -25,6 +25,7 @@ class User < ApplicationRecord :message => Authentication.bad_email_message validate :roles_are_known + validate :admin_needs_second_factor # Authenticates a user by their login name and unencrypted password. Returns the user or nil. def self.authenticate(login, password) @@ -136,6 +137,30 @@ class User < ApplicationRecord true end + def grant_redaktion!(actor:) + return :already if redaktion? + return :no_second_factor unless otp_enrolled? + + transaction do + update_column(:roles, (roles | ["redaktion"]).sort) + NodeAction.record!(:participants => [self], :user => actor, + :action => "redaktion_grant", :target_login => login) + end + :granted + end + + def revoke_redaktion!(actor:) + return :already unless redaktion? + return :self unless actor != self + + transaction do + update_column(:roles, (roles - ["redaktion"]).sort) + NodeAction.record!(:participants => [self], :user => actor, + :action => "redaktion_revoke", :target_login => login) + end + :revoked + 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 @@ -213,4 +238,10 @@ class User < ApplicationRecord unknown = roles.to_a - ROLES errors.add(:roles, :unknown, :list => unknown.join(", ")) if unknown.any? end + + def admin_needs_second_factor + return unless roles.include?("admin") + return if otp_secret.present? + errors.add(:roles, :admin_needs_otp) + end end diff --git a/app/views/users/_user.html.erb b/app/views/users/_user.html.erb index ff9d4e37..ba82375d 100644 --- a/app/views/users/_user.html.erb +++ b/app/views/users/_user.html.erb @@ -26,5 +26,22 @@ <% end %> <% end %> + + <% if current_user.redaktion? && !user.alumni? %> + <% if user.redaktion? %> + <% unless user == current_user %> + <%= button_to t(".revoke_redaktion"), revoke_redaktion_user_path(user), method: :put, + form: { data: { confirm: t(".confirm_revoke_redaktion", :login => user.login) }, + class: 'button_to destructive' } %> + <% end %> + <% elsif user.otp_enrolled? %> + <%= button_to t(".grant_redaktion"), grant_redaktion_user_path(user), method: :put, + form: { data: { confirm: t(".confirm_grant_redaktion", :login => user.login) }, + class: 'button_to state_changing' } %> + <% else %> + <%= t(".needs_otp") %> + <% end %> + <% end %> + <% end %> diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index 854811a2..2936bbea 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -8,6 +8,7 @@ <% end %> <% end %>

+

<%= t(".admin_hint") %>

<% UsersController::GROUP_ORDER.each do |group| %> <% members = @users[group] || [] %> diff --git a/config/locales/de.yml b/config/locales/de.yml index 6b5c97cc..67117945 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -150,6 +150,7 @@ de: attributes: roles: unknown: "enthält unbekannte Rollen: %{list}" + admin_needs_otp: "Die Administrator-Rolle setzt einen zweiten Faktor voraus. Der Nutzer muss diesen zuerst selbst unter »Mein Konto« einrichten" related_asset: attributes: headline: @@ -226,6 +227,8 @@ de: otp_reset: "%{actor} hat den zweiten Faktor von %{target} zurückgesetzt" user_deactivate: "%{actor} hat %{target} deaktiviert" user_reactivate: "%{actor} hat %{target} reaktiviert" + redaktion_grant: "%{actor} hat %{target} in die Redaktion aufgenommen" + redaktion_revoke: "%{actor} hat %{target} aus der Redaktion entfernt" open_gallery: "Gallerie anzeigen" asset_licenses: @@ -291,6 +294,11 @@ de: deactivate: "Deaktivieren" reactivate: "Reaktivieren" confirm_deactivate: "%{login} deaktivieren? Die Anmeldung wird sofort verweigert, Zuschreibungen bleiben erhalten." + grant_redaktion: "In die Redaktion aufnehmen" + revoke_redaktion: "Aus der Redaktion entfernen" + confirm_grant_redaktion: "%{login} in die Redaktion aufnehmen? Damit darf %{login} in den geschützten Bereichen veröffentlichen." + confirm_revoke_redaktion: "%{login} aus der Redaktion entfernen?" + needs_otp: "zweiter Faktor fehlt" index: title: "Benutzerkonten" create_editor: "Editor-Konto anlegen" @@ -301,6 +309,7 @@ de: group_editor: "Editors" group_alumni: "Ehemalige" group_empty: "— keine —" + admin_hint: "Administrative Rechte lassen sich erst vergeben, nachdem der Nutzer sich angemeldet und einen zweiten Faktor eingerichtet hat." labels: roles: "Rollen" roles: @@ -595,6 +604,10 @@ de: deactivated: "%{login} ist jetzt alumni und kann sich nicht mehr anmelden." reactivated: "%{login} kann sich wieder anmelden." cannot_deactivate_self: "Das eigene Konto kann nicht deaktiviert werden." + redaktion_granted: "%{login} gehört jetzt zur Redaktion." + 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." assets: created: "Asset wurde angelegt." updated: "Asset wurde aktualisiert." diff --git a/config/locales/en.yml b/config/locales/en.yml index e4c7ecc4..95eb7e94 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -97,6 +97,7 @@ en: attributes: roles: unknown: "contains unknown roles: %{list}" + admin_needs_otp: "Administrator role requires an enrolled second factor. The user has to set one up under My account first" related_asset: attributes: headline: @@ -170,6 +171,8 @@ en: otp_reset: "%{actor} reset the second factor of %{target}" user_deactivate: "%{actor} deactivated %{target}" user_reactivate: "%{actor} reactivated %{target}" + redaktion_grant: "%{actor} added %{target} to Redaktion" + redaktion_revoke: "%{actor} removed %{target} from Redaktion" open_gallery: "Open gallery" asset_licenses: @@ -235,6 +238,12 @@ en: deactivate: "Deactivate" reactivate: "Reactivate" confirm_deactivate: "Deactivate %{login}? Sign-in is refused immediately; attributions are preserved." + grant_redaktion: "Add to Redaktion" + revoke_redaktion: "Remove from Redaktion" + confirm_grant_redaktion: "Add %{login} to Redaktion? They will be able to publish in the protected sections." + confirm_revoke_redaktion: "Remove %{login} from Redaktion?" + needs_otp: "no second factor" + index: title: "User accounts" create_editor: "Create editor account" @@ -245,6 +254,7 @@ en: group_editor: "Editors" group_alumni: "Alumni" group_empty: "— none —" + admin_hint: "Administrative rights can only be granted after the account has signed in and set up a second factor." labels: roles: "Roles" roles: @@ -554,6 +564,11 @@ en: deactivated: "%{login} is now an alumnus and can no longer sign in." reactivated: "%{login} can sign in again." cannot_deactivate_self: "You cannot deactivate your own account." + redaktion_granted: "%{login} is now part of Redaktion." + 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." + assets: created: "Asset was successfully created." updated: "Asset was successfully updated." diff --git a/config/routes.rb b/config/routes.rb index 58e1e632..d19ac25b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -102,6 +102,8 @@ Cccms::Application.routes.draw do put :reset_otp put :deactivate put :reactivate + put :grant_redaktion + put :revoke_redaktion end end resource :otp_enrollment, :only => [:show, :create, :update, :destroy] diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index b6f0970d..d7d8b9a6 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -6,7 +6,7 @@ class UsersControllerTest < ActionController::TestCase login_as :quentin get :index assert_redirected_to admin_path - assert_equal I18n.t("flash.common.admin_required"), flash[:error] + assert_equal I18n.t("flash.common.redaktion_required"), flash[:error] end test "get index as admin shows every group with per-row actions" do @@ -53,7 +53,7 @@ class UsersControllerTest < ActionController::TestCase assert !User.last.admin end - test "creating new admin users being logged in as admin" do + test "creating a Redaktion account" do login_as :aaron elevate_session! assert_difference "User.count", +1 do @@ -63,13 +63,14 @@ class UsersControllerTest < ActionController::TestCase :email => "foo@bar.com", :password => "xxxzzz", :password_confirmation => "xxxzzz", - :roles => ["admin", "redaktion"] + :roles => ["redaktion"] } } end assert_redirected_to user_path(User.last) - assert User.last.admin + assert User.last.redaktion? + assert_not User.last.is_admin? end test "creating new users not being logged as regular user wont work" do @@ -196,6 +197,7 @@ class UsersControllerTest < ActionController::TestCase login_as :aaron elevate_session! user = users(:quentin) + user.update_column(:otp_secret, ROTP::Base32.random) put :update, params: { :id => user.id, :user => {:roles => ["admin", "redaktion"]} } assert_equal true, user.reload.is_admin? @@ -261,4 +263,14 @@ class UsersControllerTest < ActionController::TestCase assert_not user.reload.is_admin? end + + test "an account without a second factor cannot be promoted to admin" do + login_as :aaron + elevate_session! + user = users(:quentin) + + put :update, params: { :id => user.id, :user => { :roles => ["admin"] } } + + assert_not user.reload.is_admin? + end end -- cgit v1.3