From 4dbada17feae1db00118c913215e243ac63d6969 Mon Sep 17 00:00:00 2001 From: Jens Kraemer Date: Tue, 6 Oct 2026 11:14:46 +0800 Subject: [PATCH] Invalidate sessions and autologin tokens when an email address is changed or deleted So far, changing or deleting an email address of a user deleted only their password recovery tokens. With this change, it now deletes their autologin and session tokens as well, like a password change does, so that all sessions of the user end. The session that made the change stays logged in: MyController#account, UsersController#update and EmailAddressesController#destroy issue a new session token when the current user changed or deleted one of their own addresses. The token deletion moves to User#delete_login_tokens, which User and EmailAddress share. Issuing the new session token moves to ApplicationController#renew_session_token, which MyController#password and TwofaController#activate use as well. It does nothing for API requests, as these do not use the session. --- app/controllers/application_controller.rb | 7 ++ app/controllers/email_addresses_controller.rb | 4 + app/controllers/my_controller.rb | 7 +- app/controllers/twofa_controller.rb | 2 +- app/controllers/users_controller.rb | 6 ++ app/models/email_address.rb | 4 +- app/models/user.rb | 8 +- test/integration/api_test/my_test.rb | 13 +++ test/integration/api_test/users_test.rb | 13 +++ test/integration/sessions_test.rb | 95 ++++++++++++++++++- test/integration/twofa_test.rb | 6 ++ test/unit/email_address_test.rb | 46 +++++++++ test/unit/user_test.rb | 50 +++++++++- 13 files changed, 253 insertions(+), 8 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index d40159652..4bfec81bf 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -112,6 +112,13 @@ class ApplicationController < ActionController::Base end end + # Issues a new session token after the session tokens of the current user + # were deleted, so that the current session stays valid. + # API requests do not use the session and thus do not need a token. + def renew_session_token + session[:tk] = User.current.generate_session_token unless api_request? + end + def user_setup # Check the settings cache for each request Setting.check_cache diff --git a/app/controllers/email_addresses_controller.rb b/app/controllers/email_addresses_controller.rb index ac36693f5..a0acd8b5b 100644 --- a/app/controllers/email_addresses_controller.rb +++ b/app/controllers/email_addresses_controller.rb @@ -76,6 +76,10 @@ class EmailAddressesController < ApplicationController def destroy @address.destroy + if @user == User.current + # The session token was destroyed by the address deletion, generate a new one + renew_session_token + end respond_to do |format| format.html do diff --git a/app/controllers/my_controller.rb b/app/controllers/my_controller.rb index 3d27f3ba4..077b8c6ce 100644 --- a/app/controllers/my_controller.rb +++ b/app/controllers/my_controller.rb @@ -55,8 +55,13 @@ class MyController < ApplicationController if request.put? @user.safe_attributes = params[:user] @user.pref.safe_attributes = params[:pref] + mail_changed = @user.mail_changed? if @user.save @user.pref.save + if mail_changed + # The session token was destroyed by the email address change, generate a new one + renew_session_token + end respond_to do |format| format.html do flash[:notice] = l(:notice_account_updated) @@ -110,7 +115,7 @@ class MyController < ApplicationController @user.must_change_passwd = false if @user.save # The session token was destroyed by the password change, generate a new one - session[:tk] = @user.generate_session_token + renew_session_token Mailer.deliver_password_updated(@user, User.current) flash[:notice] = l(:notice_account_password_updated) redirect_to my_account_path diff --git a/app/controllers/twofa_controller.rb b/app/controllers/twofa_controller.rb index 3023caa9b..0f951cf20 100644 --- a/app/controllers/twofa_controller.rb +++ b/app/controllers/twofa_controller.rb @@ -53,7 +53,7 @@ class TwofaController < ApplicationController def activate if @twofa.confirm_pairing!(params[:twofa_code].to_s) # The session token was destroyed by the twofa pairing, generate a new one - session[:tk] = @user.generate_session_token + renew_session_token flash[:notice] = l('twofa_activated', bc_path: my_twofa_backup_codes_init_path) redirect_to my_account_path else diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index c112b68d4..7e559f5ad 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -186,12 +186,18 @@ class UsersController < ApplicationController @user.safe_attributes = params[:user] # Was the account actived ? (do it before User#save clears the change) was_activated = (@user.status_change == [User::STATUS_REGISTERED, User::STATUS_ACTIVE]) + mail_changed = @user.mail_changed? # TODO: Similar to My#account @user.pref.safe_attributes = params[:pref] if @user.save @user.pref.save + if mail_changed && @user == User.current + # The session token was destroyed by the email address change, generate a new one + renew_session_token + end + Mailer.deliver_password_updated(@user, User.current) if is_updating_password if was_activated Mailer.deliver_account_activated(@user) diff --git a/app/models/email_address.rb b/app/models/email_address.rb index de8c86531..9d3d42562 100644 --- a/app/models/email_address.rb +++ b/app/models/email_address.rb @@ -139,12 +139,12 @@ class EmailAddress < ApplicationRecord end # Delete all outstanding password reset tokens on email change. + # Delete the autologin and session tokens as well to prohibit session leakage. # This helps to keep the account secure in case the associated email account # was compromised. def destroy_tokens if saved_change_to_address? || destroyed? - tokens = ['recovery'] - Token.where(:user_id => user_id, :action => tokens).delete_all + user&.delete_login_tokens end end diff --git a/app/models/user.rb b/app/models/user.rb index 417d66fa6..dad8213a5 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -481,6 +481,11 @@ class User < Principal Token.where(:user_id => id, :action => 'autologin', :value => value).delete_all end + # Deletes all tokens that could be used to log in + def delete_login_tokens + Token.where(:user_id => id, :action => ['recovery', 'autologin', 'session']).delete_all + end + def twofa_totp_key read_ciphered_attribute(:twofa_totp_key) end @@ -983,8 +988,7 @@ class User < Principal # was compromised. def destroy_tokens if saved_change_to_hashed_password? || (saved_change_to_status? && !active?) || (saved_change_to_twofa_scheme? && twofa_scheme.present?) - tokens = ['recovery', 'autologin', 'session'] - Token.where(:user_id => id, :action => tokens).delete_all + delete_login_tokens end end diff --git a/test/integration/api_test/my_test.rb b/test/integration/api_test/my_test.rb index 03e6164f4..c5514d4c6 100644 --- a/test/integration/api_test/my_test.rb +++ b/test/integration/api_test/my_test.rb @@ -101,6 +101,19 @@ class Redmine::ApiTest::MyTest < Redmine::ApiTest::Base assert_kind_of Array, json['errors'] end + test "PUT /my/account.json with a changed mail should not generate a session token" do + Token.create!(:user_id => 3, :action => 'session') + + put( + '/my/account.json', + :params => {:user => {:mail => 'dave@somenet.foo'}}, + :headers => credentials('dlopper', 'foo')) + assert_response :no_content + + assert_equal 'dave@somenet.foo', User.find(3).mail + assert_empty Token.where(:user_id => 3, :action => 'session') + end + test "GET /my/account.json authenticated via OAuth should not disclose the api_key" do application = Doorkeeper::Application.create!( :name => 'Test App', diff --git a/test/integration/api_test/users_test.rb b/test/integration/api_test/users_test.rb index 50c8533e6..8cf5194d8 100644 --- a/test/integration/api_test/users_test.rb +++ b/test/integration/api_test/users_test.rb @@ -488,6 +488,19 @@ class Redmine::ApiTest::UsersTest < Redmine::ApiTest::Base assert_equal '', @response.body end + test "PUT /users/:id.json by an admin changing their own mail should not generate a session token" do + Token.create!(:user_id => 1, :action => 'session') + + put( + '/users/1.json', + :params => {:user => {:mail => 'newadmin@somenet.foo'}}, + :headers => credentials('admin')) + assert_response :no_content + + assert_equal 'newadmin@somenet.foo', User.find(1).mail + assert_empty Token.where(:user_id => 1, :action => 'session') + end + test "PUT /users/:id.xml with invalid parameters" do assert_no_difference('User.count') do put( diff --git a/test/integration/sessions_test.rb b/test/integration/sessions_test.rb index b41d05b1c..a7b3d6dfa 100644 --- a/test/integration/sessions_test.rb +++ b/test/integration/sessions_test.rb @@ -63,7 +63,8 @@ class SessionsTest < Redmine::IntegrationTest assert_response :ok end - def test_change_password_generates_a_new_token_for_current_session + def test_change_password_kills_all_sessions_and_generates_a_new_token_for_current_session + other_session_token = Token.create!(:user_id => 2, :action => 'session') log_user('jsmith', 'jsmith') assert_not_nil token = session[:tk] @@ -79,6 +80,98 @@ class SessionsTest < Redmine::IntegrationTest ) assert_response :found assert_not_equal token, session[:tk] + assert_nil Token.find_by(:user_id => 2, :action => 'session', :value => token) + assert_nil Token.find_by_id(other_session_token.id) + + get '/my/account' + assert_response :ok + end + + def test_change_mail_kills_sessions + log_user('jsmith', 'jsmith') + + jsmith = User.find(2) + jsmith.mail = 'anotheraddress@somenet.foo' + jsmith.save! + + get '/my/account' + assert_response :found + assert_includes flash[:error], 'Your session has expired' + end + + def test_change_mail_kills_all_sessions_and_generates_a_new_token_for_current_session + other_session_token = Token.create!(:user_id => 2, :action => 'session') + log_user('jsmith', 'jsmith') + assert_not_nil token = session[:tk] + + put '/my/account', :params => {:user => {:mail => 'anotheraddress@somenet.foo'}} + assert_response :found + assert_not_equal token, session[:tk] + assert_nil Token.find_by(:user_id => 2, :action => 'session', :value => token) + assert_nil Token.find_by_id(other_session_token.id) + + get '/my/account' + assert_response :ok + end + + def test_destroy_email_address_kills_all_sessions_and_generates_a_new_token_for_current_session + email = EmailAddress.create!(:user_id => 2, :address => 'another@somenet.foo') + other_session_token = Token.create!(:user_id => 2, :action => 'session') + + log_user('jsmith', 'jsmith') + assert_not_nil token = session[:tk] + + delete "/users/2/email_addresses/#{email.id}" + assert_response :found + assert_not_equal token, session[:tk] + assert_nil Token.find_by(:user_id => 2, :action => 'session', :value => token) + assert_nil Token.find_by_id(other_session_token.id) + + get '/my/account' + assert_response :ok + end + + def test_admin_destroying_other_users_email_address_should_not_touch_admin_session + email = EmailAddress.create!(:user_id => 2, :address => 'another@somenet.foo') + jsmith_session_token = Token.create!(:user_id => 2, :action => 'session') + + log_user('admin', 'admin') + assert_not_nil token = session[:tk] + + delete "/users/2/email_addresses/#{email.id}" + assert_response :found + assert_equal token, session[:tk] + assert_nil Token.find_by_id(jsmith_session_token.id) + + get '/my/account' + assert_response :ok + end + + def test_admin_changing_own_mail_kills_all_sessions_and_generates_a_new_token_for_current_session + other_session_token = Token.create!(:user_id => 1, :action => 'session') + log_user('admin', 'admin') + assert_not_nil token = session[:tk] + + put '/users/1', :params => {:user => {:mail => 'newadmin@somenet.foo'}} + assert_response :found + assert_not_equal token, session[:tk] + assert_nil Token.find_by(:user_id => 1, :action => 'session', :value => token) + assert_nil Token.find_by_id(other_session_token.id) + + get '/my/account' + assert_response :ok + end + + def test_admin_changing_other_users_mail_should_kill_their_sessions_and_keep_admin_session + jsmith_session_token = Token.create!(:user_id => 2, :action => 'session') + + log_user('admin', 'admin') + assert_not_nil token = session[:tk] + + put '/users/2', :params => {:user => {:mail => 'anotheraddress@somenet.foo'}} + assert_response :found + assert_equal token, session[:tk] + assert_nil Token.find_by_id(jsmith_session_token.id) get '/my/account' assert_response :ok diff --git a/test/integration/twofa_test.rb b/test/integration/twofa_test.rb index 162d01d15..65dca4061 100644 --- a/test/integration/twofa_test.rb +++ b/test/integration/twofa_test.rb @@ -273,9 +273,11 @@ class TwofaTest < Redmine::IntegrationTest def test_enable_twofa_should_destroy_tokens recovery_token = Token.create!(:user_id => 2, :action => 'recovery') autologin_token = Token.create!(:user_id => 2, :action => 'autologin') + other_session_token = Token.create!(:user_id => 2, :action => 'session') with_settings twofa: "2" do log_user('jsmith', 'jsmith') + assert_not_nil token = session[:tk] follow_redirect! assert_redirected_to "/my/twofa/totp/activate/confirm" follow_redirect! @@ -290,9 +292,13 @@ class TwofaTest < Redmine::IntegrationTest post "/my/twofa/totp/activate", params: {twofa_code: totp.now} assert_redirected_to "/my/account" + assert_not_equal token, session[:tk] + assert_nil Token.find_by(:user_id => 2, :action => 'session', :value => token) + assert User.verify_session_token(2, session[:tk]) end assert_nil Token.find_by_id(recovery_token.id) assert_nil Token.find_by_id(autologin_token.id) + assert_nil Token.find_by_id(other_session_token.id) end end diff --git a/test/unit/email_address_test.rb b/test/unit/email_address_test.rb index 923df897a..c5b2375f4 100644 --- a/test/unit/email_address_test.rb +++ b/test/unit/email_address_test.rb @@ -24,6 +24,52 @@ class EmailAddressTest < ActiveSupport::TestCase User.current = nil end + def test_destroy_should_destroy_tokens + email = EmailAddress.create!(:user_id => 2, :address => 'another@somenet.foo') + recovery_token = Token.create!(:user_id => 2, :action => 'recovery') + autologin_token = Token.create!(:user_id => 2, :action => 'autologin') + session_token = Token.create!(:user_id => 2, :action => 'session') + + assert email.destroy + + assert_nil Token.find_by_id(recovery_token.id) + assert_nil Token.find_by_id(autologin_token.id) + assert_nil Token.find_by_id(session_token.id) + end + + def test_create_should_not_destroy_tokens + autologin_token = Token.create!(:user_id => 2, :action => 'autologin') + session_token = Token.create!(:user_id => 2, :action => 'session') + + EmailAddress.create!(:user_id => 2, :address => 'another@somenet.foo') + + assert_equal autologin_token, Token.find_by_id(autologin_token.id) + assert_equal session_token, Token.find_by_id(session_token.id) + end + + def test_notify_change_should_not_destroy_tokens + email = EmailAddress.create!(:user_id => 2, :address => 'another@somenet.foo') + recovery_token = Token.create!(:user_id => 2, :action => 'recovery') + autologin_token = Token.create!(:user_id => 2, :action => 'autologin') + session_token = Token.create!(:user_id => 2, :action => 'session') + + email.notify = false + assert email.save + + assert_equal recovery_token, Token.find_by_id(recovery_token.id) + assert_equal autologin_token, Token.find_by_id(autologin_token.id) + assert_equal session_token, Token.find_by_id(session_token.id) + end + + def test_destroy_address_of_missing_user_should_succeed + email = EmailAddress.create!(:user_id => 2, :address => 'another@somenet.foo') + EmailAddress.where(:id => email.id).update_all(:user_id => 999) + email.reload + + assert email.destroy + assert_nil EmailAddress.find_by_id(email.id) + end + def test_address_with_punycode_tld_should_be_valid email = EmailAddress.new(address: 'jsmith@example.xn--80akhbyknj4f') assert email.valid? diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index d30651075..3b19aabe2 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -507,6 +507,7 @@ class UserTest < ActiveSupport::TestCase def test_password_change_should_destroy_tokens recovery_token = Token.create!(:user_id => 2, :action => 'recovery') autologin_token = Token.create!(:user_id => 2, :action => 'autologin') + session_token = Token.create!(:user_id => 2, :action => 'session') user = User.find(2) user.password, user.password_confirmation = "a new password", "a new password" @@ -514,18 +515,49 @@ class UserTest < ActiveSupport::TestCase assert_nil Token.find_by_id(recovery_token.id) assert_nil Token.find_by_id(autologin_token.id) + assert_nil Token.find_by_id(session_token.id) end def test_mail_change_should_destroy_tokens recovery_token = Token.create!(:user_id => 2, :action => 'recovery') autologin_token = Token.create!(:user_id => 2, :action => 'autologin') + session_token = Token.create!(:user_id => 2, :action => 'session') user = User.find(2) user.mail = "user@somwehere.com" assert user.save assert_nil Token.find_by_id(recovery_token.id) - assert_equal autologin_token, Token.find_by_id(autologin_token.id) + assert_nil Token.find_by_id(autologin_token.id) + assert_nil Token.find_by_id(session_token.id) + end + + def test_lock_should_destroy_tokens + recovery_token = Token.create!(:user_id => 2, :action => 'recovery') + autologin_token = Token.create!(:user_id => 2, :action => 'autologin') + session_token = Token.create!(:user_id => 2, :action => 'session') + + user = User.find(2) + user.status = User::STATUS_LOCKED + assert user.save + + assert_nil Token.find_by_id(recovery_token.id) + assert_nil Token.find_by_id(autologin_token.id) + assert_nil Token.find_by_id(session_token.id) + end + + def test_twofa_activation_should_destroy_tokens + recovery_token = Token.create!(:user_id => 2, :action => 'recovery') + autologin_token = Token.create!(:user_id => 2, :action => 'autologin') + session_token = Token.create!(:user_id => 2, :action => 'session') + + user = User.find(2) + user.twofa_scheme = 'totp' + assert user.save + + assert_nil Token.find_by_id(recovery_token.id) + assert_nil Token.find_by_id(autologin_token.id) + assert_nil Token.find_by_id(session_token.id) end def test_change_on_other_fields_should_not_destroy_tokens @@ -540,6 +572,22 @@ class UserTest < ActiveSupport::TestCase assert_equal autologin_token, Token.find_by_id(autologin_token.id) end + def test_delete_login_tokens_should_delete_recovery_autologin_and_session_tokens + recovery_token = Token.create!(:user_id => 2, :action => 'recovery') + autologin_token = Token.create!(:user_id => 2, :action => 'autologin') + session_token = Token.create!(:user_id => 2, :action => 'session') + api_token = Token.create!(:user_id => 2, :action => 'api') + other_users_token = Token.create!(:user_id => 3, :action => 'session') + + User.find(2).delete_login_tokens + + assert_nil Token.find_by_id(recovery_token.id) + assert_nil Token.find_by_id(autologin_token.id) + assert_nil Token.find_by_id(session_token.id) + assert Token.find_by_id(api_token.id) + assert Token.find_by_id(other_users_token.id) + end + def test_validate_login_presence @admin.login = "" assert !@admin.save -- 2.55.0