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)
This commit is contained in:
parent
d9f9904c60
commit
5012c62240
|
@ -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'))
|
||||
|
|
|
@ -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
|
|
@ -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'))
|
||||
|
|
|
@ -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 }
|
||||
|
||||
|
|
Loading…
Reference in New Issue