From 254b57c81201635a386af556e1dc938634d5d5dd Mon Sep 17 00:00:00 2001
From: Martin Brennan <mjrbrennan@gmail.com>
Date: Thu, 20 Feb 2020 13:42:57 +1000
Subject: [PATCH] FIX: When admin changes staff email still enforce old email
 confirm (#9007)

A follow-up correction to this change https://github.com/discourse/discourse/pull/9001.

When admin changes staff email still enforce old email confirm. Only allow auto-confirm of a new email by admin IF the target user is not also an admin. If an admin gets locked out of their email the site admin can use the rails console to solve the issue in a pinch.
---
 lib/email_updater.rb                  | 24 +++++++++++++-----------
 spec/components/email_updater_spec.rb | 27 +++++++++++++++------------
 2 files changed, 28 insertions(+), 23 deletions(-)

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