diff options
author | Sebastian Arcila Valenzuela <sarcila@gitlab.com> | 2019-08-12 15:41:05 +0200 |
---|---|---|
committer | Sebastian Arcila Valenzuela <sarcila@gitlab.com> | 2019-08-21 13:22:14 +0200 |
commit | 47d289b054a0e9a36f3da9ff503594e3f7511163 (patch) | |
tree | 125ed90e0147404d7600b19aba009fcbaf3f846a | |
parent | bef9aef425e5331af54c761d37338226f4d0f813 (diff) | |
download | gitlab-ce-47d289b054a0e9a36f3da9ff503594e3f7511163.tar.gz |
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)
-rw-r--r-- | app/models/user.rb | 7 | ||||
-rw-r--r-- | changelogs/unreleased/security-sarcila-fix-weak-session-management.yml | 6 | ||||
-rw-r--r-- | spec/features/profiles/user_edit_profile_spec.rb | 17 | ||||
-rw-r--r-- | spec/models/user_spec.rb | 41 |
4 files changed, 71 insertions, 0 deletions
diff --git a/app/models/user.rb b/app/models/user.rb index 2eb5c63a4cc..de3fdfc8014 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -629,6 +629,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 a53da94ef7d..9ab5026dbc4 100644 --- a/spec/features/profiles/user_edit_profile_spec.rb +++ b/spec/features/profiles/user_edit_profile_spec.rb @@ -47,6 +47,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 c95bbb0b3f5..f4b50f7fb85 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2985,6 +2985,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 } |