From ce34307e4c8557fdb14c55d8ef7fc844e0d9273a Mon Sep 17 00:00:00 2001
From: Holger Just <holger@plan.io>
Date: Tue, 19 Aug 2014 15:39:59 +0200
Subject: [PATCH] Delete tokens on mail or password change

When the user updates their password or email address, this could well
be in response to a security breach, i.e. if their email account was
compromised or their password became known.

Thus, after a change of the critical attributes of a user, we have to
ensure that the account is safe and there are no tokens which might
still float around which might allow others to access the account.
---
 app/models/user.rb     | 14 +++++++++++++-
 test/unit/user_test.rb | 25 +++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 2993cc1..b5c29f1 100644
--- 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
@@ -677,6 +677,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 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 aa930a7..98d26c1 100644
--- a/test/unit/user_test.rb
+++ b/test/unit/user_test.rb
@@ -403,6 +403,31 @@ 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_validate_login_presence
     @admin.login = ""
     assert !@admin.save
--
2.0.0