From 5012c622405e63655256735d266168450ad1d159 Mon Sep 17 00:00:00 2001
From: Sebastian Arcila Valenzuela <sarcila@gitlab.com>
Date: Mon, 12 Aug 2019 15:41:05 +0200
Subject: Add User#will_save_change_to_login? to clear reset_password_tokens

Devise checks before updating any of the authentication_keys if it
needs to clear the reset_password_tokens.

This should fix:
https://gitlab.com/gitlab-org/gitlab-ce/issues/42733 (Weak
authentication and session management)
---
 app/models/user.rb                                 |  7 ++++
 ...ecurity-sarcila-fix-weak-session-management.yml |  6 ++++
 spec/features/profiles/user_edit_profile_spec.rb   | 17 +++++++++
 spec/models/user_spec.rb                           | 41 ++++++++++++++++++++++
 4 files changed, 71 insertions(+)
 create mode 100644 changelogs/unreleased/security-sarcila-fix-weak-session-management.yml

diff --git a/app/models/user.rb b/app/models/user.rb
index 6131a8dc710..909f5f3873d 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -643,6 +643,13 @@ class User < ApplicationRecord
     end
   end
 
+  # will_save_change_to_attribute? is used by Devise to check if it is necessary
+  # to clear any existing reset_password_tokens before updating an authentication_key
+  # and login in our case is a virtual attribute to allow login by username or email.
+  def will_save_change_to_login?
+    will_save_change_to_username? || will_save_change_to_email?
+  end
+
   def unique_email
     if !emails.exists?(email: email) && Email.exists?(email: email)
       errors.add(:email, _('has already been taken'))
diff --git a/changelogs/unreleased/security-sarcila-fix-weak-session-management.yml b/changelogs/unreleased/security-sarcila-fix-weak-session-management.yml
new file mode 100644
index 00000000000..a37a3099519
--- /dev/null
+++ b/changelogs/unreleased/security-sarcila-fix-weak-session-management.yml
@@ -0,0 +1,6 @@
+---
+title: Fix weak session management by clearing password reset tokens after login (username/email)
+  are updated
+merge_request:
+author:
+type: security
diff --git a/spec/features/profiles/user_edit_profile_spec.rb b/spec/features/profiles/user_edit_profile_spec.rb
index 1ab7742b36e..0905ab0aef8 100644
--- a/spec/features/profiles/user_edit_profile_spec.rb
+++ b/spec/features/profiles/user_edit_profile_spec.rb
@@ -49,6 +49,23 @@ describe 'User edit profile' do
     end
   end
 
+  describe 'when I change my email' do
+    before do
+      user.send_reset_password_instructions
+    end
+
+    it 'clears the reset password token' do
+      expect(user.reset_password_token?).to be true
+
+      fill_in 'user_email', with: 'new-email@example.com'
+      submit_settings
+
+      user.reload
+      expect(user.confirmation_token).not_to be_nil
+      expect(user.reset_password_token?).to be false
+    end
+  end
+
   context 'user avatar' do
     before do
       attach_file(:user_avatar, Rails.root.join('spec', 'fixtures', 'banana_sample.gif'))
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 46b86e8393d..1a641c868d9 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -3045,6 +3045,47 @@ describe User do
     end
   end
 
+  describe '#will_save_change_to_login?' do
+    let(:user) { create(:user, username: 'old-username', email: 'old-email@example.org') }
+    let(:new_username) { 'new-name' }
+    let(:new_email) { 'new-email@example.org' }
+
+    subject { user.will_save_change_to_login? }
+
+    context 'when the username is changed' do
+      before do
+        user.username = new_username
+      end
+
+      it { is_expected.to be true }
+    end
+
+    context 'when the email is changed' do
+      before do
+        user.email = new_email
+      end
+
+      it { is_expected.to be true }
+    end
+
+    context 'when both email and username are changed' do
+      before do
+        user.username = new_username
+        user.email = new_email
+      end
+
+      it { is_expected.to be true }
+    end
+
+    context 'when email and username aren\'t changed' do
+      before do
+        user.name = 'new_name'
+      end
+
+      it { is_expected.to be_falsy }
+    end
+  end
+
   describe '#sync_attribute?' do
     let(:user) { described_class.new }
 
-- 
cgit v1.2.1