Commit 3f73b6be authored by Rémy Coutable's avatar Rémy Coutable

Don't set the notification_email when only unconfirmed_email is changed

Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent 4d674bd3
...@@ -165,8 +165,7 @@ class User < ActiveRecord::Base ...@@ -165,8 +165,7 @@ class User < ActiveRecord::Base
validate :signup_domain_valid?, on: :create, if: ->(user) { !user.created_by_id } validate :signup_domain_valid?, on: :create, if: ->(user) { !user.created_by_id }
before_validation :sanitize_attrs before_validation :sanitize_attrs
before_validation :set_notification_email, if: :email_changed? before_validation :set_notification_email, if: :new_record?
before_save :set_notification_email, if: :email_changed? # in case validation is skipped
before_validation :set_public_email, if: :public_email_changed? before_validation :set_public_email, if: :public_email_changed?
before_save :set_public_email, if: :public_email_changed? # in case validation is skipped before_save :set_public_email, if: :public_email_changed? # in case validation is skipped
before_save :ensure_incoming_email_token before_save :ensure_incoming_email_token
...@@ -179,8 +178,21 @@ class User < ActiveRecord::Base ...@@ -179,8 +178,21 @@ class User < ActiveRecord::Base
after_update :username_changed_hook, if: :username_changed? after_update :username_changed_hook, if: :username_changed?
after_destroy :post_destroy_hook after_destroy :post_destroy_hook
after_destroy :remove_key_cache after_destroy :remove_key_cache
after_commit :update_emails_with_primary_email, on: :update, if: -> { previous_changes.key?('email') } after_commit(on: :update) do
after_commit :update_invalid_gpg_signatures, on: :update, if: -> { previous_changes.key?('email') } if previous_changes.key?('email')
# Grab previous_email here since previous_changes changes after
# #update_emails_with_primary_email and #update_notification_email are called
previous_email = previous_changes[:email][0]
update_emails_with_primary_email(previous_email)
update_invalid_gpg_signatures
if previous_email == notification_email
self.notification_email = email
save
end
end
end
after_initialize :set_projects_limit after_initialize :set_projects_limit
...@@ -546,8 +558,7 @@ class User < ActiveRecord::Base ...@@ -546,8 +558,7 @@ class User < ActiveRecord::Base
# hash and `_was` variables getting munged. # hash and `_was` variables getting munged.
# By using an `after_commit` instead of `after_update`, we avoid the recursive callback # By using an `after_commit` instead of `after_update`, we avoid the recursive callback
# scenario, though it then requires us to use the `previous_changes` hash # scenario, though it then requires us to use the `previous_changes` hash
def update_emails_with_primary_email def update_emails_with_primary_email(previous_email)
previous_email = previous_changes[:email][0] # grab this before the DestroyService is called
primary_email_record = emails.find_by(email: email) primary_email_record = emails.find_by(email: email)
Emails::DestroyService.new(self, user: self).execute(primary_email_record) if primary_email_record Emails::DestroyService.new(self, user: self).execute(primary_email_record) if primary_email_record
...@@ -772,13 +783,13 @@ class User < ActiveRecord::Base ...@@ -772,13 +783,13 @@ class User < ActiveRecord::Base
end end
def set_notification_email def set_notification_email
if notification_email.blank? || !all_emails.include?(notification_email) if notification_email.blank? || all_emails.exclude?(notification_email)
self.notification_email = email self.notification_email = email
end end
end end
def set_public_email def set_public_email
if public_email.blank? || !all_emails.include?(public_email) if public_email.blank? || all_emails.exclude?(public_email)
self.public_email = '' self.public_email = ''
end end
end end
......
---
title: Fix an issue where the notification email address would be set to an unconfirmed
email address
merge_request: 18474
author:
type: fixed
...@@ -393,24 +393,6 @@ describe User do ...@@ -393,24 +393,6 @@ describe User do
end end
describe 'after commit hook' do describe 'after commit hook' do
describe '.update_invalid_gpg_signatures' do
let(:user) do
create(:user, email: 'tula.torphy@abshire.ca').tap do |user|
user.skip_reconfirmation!
end
end
it 'does nothing when the name is updated' do
expect(user).not_to receive(:update_invalid_gpg_signatures)
user.update_attributes!(name: 'Bette')
end
it 'synchronizes the gpg keys when the email is updated' do
expect(user).to receive(:update_invalid_gpg_signatures).at_most(:twice)
user.update_attributes!(email: 'shawnee.ritchie@denesik.com')
end
end
describe '#update_emails_with_primary_email' do describe '#update_emails_with_primary_email' do
before do before do
@user = create(:user, email: 'primary@example.com').tap do |user| @user = create(:user, email: 'primary@example.com').tap do |user|
...@@ -450,6 +432,76 @@ describe User do ...@@ -450,6 +432,76 @@ describe User do
expect(@user.emails.first.confirmed_at).not_to eq nil expect(@user.emails.first.confirmed_at).not_to eq nil
end end
end end
describe '#update_notification_email' do
# Regression: https://gitlab.com/gitlab-org/gitlab-ce/issues/22846
context 'when changing :email' do
let(:user) { create(:user) }
let(:new_email) { 'new-email@example.com' }
it 'sets :unconfirmed_email' do
expect do
user.tap { |u| u.update!(email: new_email) }.reload
end.to change(user, :unconfirmed_email).to(new_email)
end
it 'does not change :notification_email' do
expect do
user.tap { |u| u.update!(email: new_email) }.reload
end.not_to change(user, :notification_email)
end
it 'updates :notification_email to the new email once confirmed' do
user.update!(email: new_email)
expect do
user.tap(&:confirm).reload
end.to change(user, :notification_email).to eq(new_email)
end
context 'and :notification_email is set to a secondary email' do
let!(:email_attrs) { attributes_for(:email, :confirmed, user: user) }
let(:secondary) { create(:email, :confirmed, email: 'secondary@example.com', user: user) }
before do
user.emails.create(email_attrs)
user.tap { |u| u.update!(notification_email: email_attrs[:email]) }.reload
end
it 'does not change :notification_email to :email' do
expect do
user.tap { |u| u.update!(email: new_email) }.reload
end.not_to change(user, :notification_email)
end
it 'does not change :notification_email to :email once confirmed' do
user.update!(email: new_email)
expect do
user.tap(&:confirm).reload
end.not_to change(user, :notification_email)
end
end
end
end
describe '#update_invalid_gpg_signatures' do
let(:user) do
create(:user, email: 'tula.torphy@abshire.ca').tap do |user|
user.skip_reconfirmation!
end
end
it 'does nothing when the name is updated' do
expect(user).not_to receive(:update_invalid_gpg_signatures)
user.update_attributes!(name: 'Bette')
end
it 'synchronizes the gpg keys when the email is updated' do
expect(user).to receive(:update_invalid_gpg_signatures).at_most(:twice)
user.update_attributes!(email: 'shawnee.ritchie@denesik.com')
end
end
end end
describe '#update_tracked_fields!', :clean_gitlab_redis_shared_state do describe '#update_tracked_fields!', :clean_gitlab_redis_shared_state do
......
...@@ -488,10 +488,6 @@ describe API::Users do ...@@ -488,10 +488,6 @@ describe API::Users do
describe "PUT /users/:id" do describe "PUT /users/:id" do
let!(:admin_user) { create(:admin) } let!(:admin_user) { create(:admin) }
before do
admin
end
it "updates user with new bio" do it "updates user with new bio" do
put api("/users/#{user.id}", admin), { bio: 'new test bio' } put api("/users/#{user.id}", admin), { bio: 'new test bio' }
...@@ -525,27 +521,28 @@ describe API::Users do ...@@ -525,27 +521,28 @@ describe API::Users do
expect(json_response['avatar_url']).to include(user.avatar_path) expect(json_response['avatar_url']).to include(user.avatar_path)
end end
it 'updates user with his own email' do
put api("/users/#{user.id}", admin), email: user.email
expect(response).to have_gitlab_http_status(200)
expect(json_response['email']).to eq(user.email)
expect(user.reload.email).to eq(user.email)
end
it 'updates user with a new email' do it 'updates user with a new email' do
old_email = user.email
old_notification_email = user.notification_email
put api("/users/#{user.id}", admin), email: 'new@email.com' put api("/users/#{user.id}", admin), email: 'new@email.com'
user.reload
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
expect(user.reload.notification_email).to eq('new@email.com') expect(user).to be_confirmed
expect(user.email).to eq(old_email)
expect(user.notification_email).to eq(old_notification_email)
expect(user.unconfirmed_email).to eq('new@email.com')
end end
it 'skips reconfirmation when requested' do it 'skips reconfirmation when requested' do
put api("/users/#{user.id}", admin), { skip_reconfirmation: true } put api("/users/#{user.id}", admin), email: 'new@email.com', skip_reconfirmation: true
user.reload user.reload
expect(user.confirmed_at).to be_present expect(response).to have_gitlab_http_status(200)
expect(user).to be_confirmed
expect(user.email).to eq('new@email.com')
end end
it 'updates user with his own username' do it 'updates user with his own username' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment