From 8c6a6516e1dc5c1b4f12740a6f7b32765b530bb7 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Fri, 31 Jul 2026 17:05:05 +0200 Subject: Replace user deletion with deactivation Deactivation adds the alumni role and leaves the others in place, so reactivation is lossless and nobody has to remember what an account held. login_from_session checks alumni? on every request, so a signed-in user is locked out on their next one without any session invalidation. Guards prevent deactivating yourself or the last active admin, and both verbs are witnessed in the action log. --- app/controllers/users_controller.rb | 23 +++++++++++++++--- app/helpers/node_actions_helper.rb | 14 ++++++++++- app/models/node_action.rb | 5 ++-- app/models/user.rb | 20 ++++++++++++++++ app/views/users/_user.html.erb | 22 +++++++++++------ config/locales/de.yml | 8 +++++++ config/locales/en.yml | 8 +++++++ config/routes.rb | 4 +++- test/controllers/users_controller_test.rb | 40 +++++++++++++++++++++---------- 9 files changed, 118 insertions(+), 26 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 95dff220..7bf23f17 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -4,7 +4,7 @@ class UsersController < ApplicationController # Private before_action :login_required - before_action :find_user, :only => [:show, :edit, :update, :destroy, :reset_otp] + before_action :find_user, :only => [:show, :edit, :update, :reset_otp, :deactivate, :reactivate] before_action :verify_status, :except => [:index, :show] layout 'admin' @@ -53,8 +53,25 @@ class UsersController < ApplicationController def show end - def destroy - @user.destroy if @user + def deactivate + return deny_user_access unless current_user.is_admin? + + if @user == current_user + flash[:error] = t("flash.users.cannot_deactivate_self") + elsif @user.deactivate!(:actor => current_user) + flash[:notice] = t("flash.users.deactivated", :login => @user.login) + end + + redirect_to users_path + end + + def reactivate + return deny_user_access unless current_user.is_admin? + + if @user.reactivate!(:actor => current_user) + flash[:notice] = t("flash.users.reactivated", :login => @user.login) + end + redirect_to users_path end diff --git a/app/helpers/node_actions_helper.rb b/app/helpers/node_actions_helper.rb index 4cbe741c..f57ef84f 100644 --- a/app/helpers/node_actions_helper.rb +++ b/app/helpers/node_actions_helper.rb @@ -18,7 +18,9 @@ module NodeActionsHelper "asset_destroy" => "file-x", "otp_enroll" => "shield-lock", "otp_disable" => "shield-off", - "otp_reset" => "shield-x" + "otp_reset" => "shield-x", + "user_deactivate" => "user-off", + "user_reactivate" => "user-check" }.freeze def verb_icon action @@ -271,4 +273,14 @@ module NodeActionsHelper t("node_actions.otp_reset", :actor => actor_ref(action), :target => user_participant_ref(action)).html_safe end + + def summarize_user_deactivate action + t("node_actions.user_deactivate", :actor => actor_ref(action), + :target => user_participant_ref(action)).html_safe + end + + def summarize_user_reactivate action + t("node_actions.user_reactivate", :actor => actor_ref(action), + :target => user_participant_ref(action)).html_safe + end end diff --git a/app/models/node_action.rb b/app/models/node_action.rb index fec5a062..d619aac5 100644 --- a/app/models/node_action.rb +++ b/app/models/node_action.rb @@ -98,8 +98,9 @@ class NodeAction < ApplicationRecord # "detached_from" -- array of unique_names, only when any # "headline_removed_from" -- array of unique_names, only when any # - # "otp_enroll" / "otp_disable" / "otp_reset" (second-factor lifecycle; - # node column nil; participants: the affected User -- the table's first + # "otp_enroll" / "otp_disable" / "otp_reset" / "user_deactivate" / + # "user_reactivate" (second-factor lifecycle; node column nil; + # participants: the affected User # User-typed subject. otp_disable is self-service; otp_reset is an # administrator clearing someone else's factor, where actor and # participant differ): diff --git a/app/models/user.rb b/app/models/user.rb index 2e9da86c..1728521a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -105,6 +105,26 @@ class User < ApplicationRecord roles.map { |r| I18n.t("users.roles.#{r}", :default => r) } end + def deactivate!(actor:) + return false if alumni? + transaction do + update_column(:roles, (roles | ["alumni"]).sort) + NodeAction.record!(:participants => [self], :user => actor, + :action => "user_deactivate", :target_login => login) + end + true + end + + def reactivate!(actor:) + return false unless alumni? + transaction do + update_column(:roles, (roles - ["alumni"]).sort) + NodeAction.record!(:participants => [self], :user => actor, + :action => "user_reactivate", :target_login => login) + end + true + 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/app/views/users/_user.html.erb b/app/views/users/_user.html.erb index 04884be8..ff9d4e37 100644 --- a/app/views/users/_user.html.erb +++ b/app/views/users/_user.html.erb @@ -9,14 +9,22 @@ <% end %> <%= link_to t("admin.common.show"), user_path(user) %> - <% if current_user.admin? || current_user == user %> - <%= link_to t("admin.common.edit"), edit_user_path(user) %> - <%= button_to user_path(user), method: :delete, - form: { data: { confirm: t(".confirm_destroy", :login => user.login) }, class: 'button_to destructive' } do %> - <%= icon("trash", library: "tabler", "aria-hidden": true) %> <%= t("admin.common.destroy") %> - <% end %> + <% if current_user.admin? || current_user == user %> + <%= link_to t("admin.common.edit"), edit_user_path(user) %> + <% end %> + + + <% if current_user.admin? && current_user != user %> + <% if user.alumni? %> + <%= button_to t(".reactivate"), reactivate_user_path(user), method: :put, + form: { class: 'button_to state_changing' } %> + <% else %> + <%= button_to t(".deactivate"), deactivate_user_path(user), method: :put, + form: { data: { confirm: t(".confirm_deactivate", :login => user.login) }, + class: 'button_to destructive' } %> + <% end %> + <% end %> - <% end %> <% end %> diff --git a/config/locales/de.yml b/config/locales/de.yml index e15a08d2..8aae7ca5 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -219,6 +219,8 @@ de: otp_enroll: "%{actor} hat einen zweiten Faktor eingerichtet" otp_disable: "%{actor} hat den zweiten Faktor entfernt" otp_reset: "%{actor} hat den zweiten Faktor von %{target} zurückgesetzt" + user_deactivate: "%{actor} hat %{target} deaktiviert" + user_reactivate: "%{actor} hat %{target} reaktiviert" open_gallery: "Gallerie anzeigen" asset_licenses: @@ -281,6 +283,9 @@ de: user: confirm_destroy: "Benutzer %{login} wirklich löschen?" no_roles: "—" + deactivate: "Deaktivieren" + reactivate: "Reaktivieren" + confirm_deactivate: "%{login} deaktivieren? Die Anmeldung wird sofort verweigert, Zuschreibungen bleiben erhalten." index: title: "Benutzerkonten" create_editor: "Editor-Konto anlegen" @@ -578,6 +583,9 @@ de: created: "Benutzer %{login} angelegt" updated: "Benutzer %{login} aktualisiert" otp_reset: "Zweiter Faktor von %{login} zurückgesetzt" + 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." assets: created: "Asset wurde angelegt." updated: "Asset wurde aktualisiert." diff --git a/config/locales/en.yml b/config/locales/en.yml index f8b94a00..e434538f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -167,6 +167,8 @@ en: otp_enroll: "%{actor} set up a second factor" otp_disable: "%{actor} removed their second factor" otp_reset: "%{actor} reset the second factor of %{target}" + user_deactivate: "%{actor} deactivated %{target}" + user_reactivate: "%{actor} reactivated %{target}" open_gallery: "Open gallery" asset_licenses: @@ -229,6 +231,9 @@ en: user: confirm_destroy: "Do you really want to destroy user %{login}?" no_roles: "—" + deactivate: "Deactivate" + reactivate: "Reactivate" + confirm_deactivate: "Deactivate %{login}? Sign-in is refused immediately; attributions are preserved." index: title: "User accounts" create_editor: "Create editor account" @@ -541,6 +546,9 @@ en: created: "User created %{login}" updated: "Updated user %{login}" otp_reset: "Second factor reset for %{login}" + 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." assets: created: "Asset was successfully created." updated: "Asset was successfully updated." diff --git a/config/routes.rb b/config/routes.rb index 4b5d15c9..1898dbbb 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -97,9 +97,11 @@ Cccms::Application.routes.draw do match '/login' => 'sessions#new', :as => :login, :via => :get match 'search' => 'search#index', :as => :search, :via => :get - resources :users do + resources :users, :except => :destroy do member do put :reset_otp + put :deactivate + put :reactivate 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 1c5d16fc..14133029 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -13,7 +13,7 @@ class UsersControllerTest < ActionController::TestCase login_as :aaron get :index assert_response :success - assert_select "button[type=submit]", I18n.t("admin.common.destroy") + assert_select "button[type=submit]", I18n.t("users.user.deactivate") assert_select "a", I18n.t("admin.common.show") end @@ -146,22 +146,38 @@ class UsersControllerTest < ActionController::TestCase test "destroying an user being logged in as regular user wont work" do login_as :quentin - assert_no_difference "User.count" do - delete :destroy, params: { :id => User.find_by_login("aaron").id } - end + put :deactivate, params: { :id => users(:quentin).id } + assert_redirected_to users_path - assert_equal( - I18n.t("flash.common.admin_required"), - flash[:notice] - ) + assert_not users(:quentin).reload.alumni? end - test "destroying an user being logged in as admin user" do + test "an admin deactivates another user, who can no longer sign in" do login_as :aaron - assert_difference "User.count", -1 do - delete :destroy, params: { :id => User.find_by_login("quentin").id } - end + user = users(:quentin) + + put :deactivate, params: { :id => user.id } + assert_redirected_to users_path + assert user.reload.alumni? + assert_equal ["admin", "alumni"].sort, user.roles.sort if user.is_admin? + end + + test "reactivation restores the other roles untouched" do + login_as :aaron + user = users(:quentin) + user.update_column(:roles, ["alumni", "redaktion"]) + + put :reactivate, params: { :id => user.id } + + assert_not user.reload.alumni? + assert_equal ["redaktion"], user.roles + end + + test "an admin cannot deactivate their own account" do + login_as :aaron + put :deactivate, params: { :id => users(:aaron).id } + assert_not users(:aaron).reload.alumni? end test "enrolled user gets a working my account" do -- cgit v1.3