diff --git a/lib/email_updater.rb b/lib/email_updater.rb index b0502256b83..68799e304a1 100644 --- a/lib/email_updater.rb +++ b/lib/email_updater.rb @@ -15,7 +15,7 @@ class EmailUpdater User.human_attribute_name(name, options) end - def authorize_both? + def changing_staff_user_email? @user.staff? end @@ -44,7 +44,7 @@ class EmailUpdater args, email_token = prepare_change_request(args) @user.email_change_requests.create!(args) - if initiating_admin_changing_another_user_email? + if initiating_admin_changing_another_user_email? && !changing_staff_user_email? auto_confirm_and_send_password_reset(email_token) return end @@ -115,20 +115,22 @@ class EmailUpdater email_token: email_token.token end + # regular users or changes requested by admin are always + # authorizing new only (as long as they are not changing their + # own email!) + # + # however even if an admin requests an email change for another + # admin the other admin must always confirm their old email def prepare_change_request(args) - # regular users or changes requested by admin are always - # authorizing new only (as long as they are not changing their - # own email!) - if !authorize_both? || initiating_admin_changing_another_user_email? - args[:change_state] = EmailChangeRequest.states[:authorizing_new] - email_token = @user.email_tokens.create!(email: args[:new_email]) - args[:new_email_token] = email_token - else + if changing_staff_user_email? args[:change_state] = EmailChangeRequest.states[:authorizing_old] email_token = @user.email_tokens.create!(email: args[:old_email]) args[:old_email_token] = email_token + else + args[:change_state] = EmailChangeRequest.states[:authorizing_new] + email_token = @user.email_tokens.create!(email: args[:new_email]) + args[:new_email_token] = email_token end - [args, email_token] end diff --git a/spec/components/email_updater_spec.rb b/spec/components/email_updater_spec.rb index a36958f3e65..b5a06937e68 100644 --- a/spec/components/email_updater_spec.rb +++ b/spec/components/email_updater_spec.rb @@ -55,23 +55,26 @@ describe EmailUpdater do context "for a staff user" do let(:user) { Fabricate(:moderator, email: old_email) } - it "does not send an email to the user for them to confirm their new email but still sends the notification to the old email" do - Jobs.expects(:enqueue).with(:critical_user_email, has_entries(type: :confirm_new_email, to_address: new_email)).never - expect_old_email_job - expect_forgot_password_job + before do + Jobs.expects(:enqueue).once.with(:critical_user_email, has_entries(type: :confirm_old_email, to_address: old_email)) updater.change_to(new_email) + @change_req = user.email_change_requests.first end - it "creates a change request authorizing the new email and immediately confirms it " do - updater.change_to(new_email) - change_req = user.email_change_requests.first - expect(change_req.change_state).to eq(EmailChangeRequest.states[:complete]) + it "starts the old confirmation process" do + expect(updater.errors).to be_blank + + expect(@change_req.old_email).to eq(old_email) + expect(@change_req.new_email).to eq(new_email) + expect(@change_req).to be_present + expect(@change_req.change_state).to eq(EmailChangeRequest.states[:authorizing_old]) + + expect(@change_req.old_email_token.email).to eq(old_email) + expect(@change_req.new_email_token).to be_blank end - it "sends a reset password email to the user so they can set a password for their new email" do - expect_old_email_job - expect_forgot_password_job - updater.change_to(new_email) + it "does not immediately confirm the request" do + expect(@change_req.change_state).not_to eq(EmailChangeRequest.states[:complete]) end end