Invalidate security tokens on password or email changes (#17717).

Contributed by Jan Schulz-Hofen.

git-svn-id: http://svn.redmine.org/redmine/trunk@13396 e93f8b46-1217-0410-a6f0-8f06a7374b81

diff --git a/app/models/user.rb b/app/models/user.rb
index 5f9674d2de1c31040b11c7d6fe101eb4b28d195c..36594137d9302d12ee69b1eaaa3446b311ebcc97 100644 (file)
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -112,7 +112,7 @@ class User < Principal
   before_create :set_mail_notification
   before_save   :generate_password_if_needed, :update_hashed_password
   before_destroy :remove_references_before_destroy
-  after_save :update_notified_project_ids
+  after_save :update_notified_project_ids, :destroy_tokens
 
   scope :in_group, lambda {|group|
     group_id = group.is_a?(Group) ? group.id : group.to_i
@@ -681,6 +681,18 @@ class User < Principal
     end
   end
 
+  # Delete all outstanding password reset tokens on password or email change.
+  # Delete the autologin tokens on password change to prohibit session leakage.
+  # This helps to keep the account secure in case the associated email account
+  # was compromised.
+  def destroy_tokens
+    tokens  = []
+    tokens |= ['recovery', 'autologin'] if changes.has_key?('hashed_password')
+    tokens |= ['recovery'] if changes.has_key?('mail')
+
+    Token.delete_all(['user_id = ? AND action IN (?)', self.id, tokens]) if tokens.any?
+  end
+
   # Removes references that are not handled by associations
   # Things that are not deleted are reassociated with the anonymous user
   def remove_references_before_destroy

diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb
index 847c25c3b1d3e59578095bc7481c5e1919986cb5..69753960ac01bce4c7de5e06b4c8418aa3d42d04 100644 (file)
--- a/test/unit/user_test.rb
+++ b/test/unit/user_test.rb
@@ -403,6 +403,42 @@ class UserTest < ActiveSupport::TestCase
     end
   end
 
+  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')
+
+    user = User.find(2)
+    user.password, user.password_confirmation = "a new password", "a new password"
+    assert user.save
+
+    assert_nil Token.find_by_id(recovery_token.id)
+    assert_nil Token.find_by_id(autologin_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')
+
+    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)
+  end
+
+  def test_change_on_other_fields_should_not_destroy_tokens
+    recovery_token = Token.create!(:user_id => 2, :action => 'recovery')
+    autologin_token = Token.create!(:user_id => 2, :action => 'autologin')
+
+    user = User.find(2)
+    user.firstname = "Bobby"
+    assert user.save
+
+    assert_equal recovery_token, Token.find_by_id(recovery_token.id)
+    assert_equal autologin_token, Token.find_by_id(autologin_token.id)
+  end
+
   def test_validate_login_presence
     @admin.login = ""
     assert !@admin.save