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

@@ -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
 

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