FIX: When admin changes another user's email auto-confirm the change (#9001)

When admin changes a user's email from the preferences page of that user:

* The user will not be sent an email to confirm that their
  email is changing. They will be sent a reset password email
  so they can set the password for their account at the new
  email address.
* The user will still be sent an email to their old email to inform
  them that it was changed.
* Admin and staff users still need to follow the same old + new
  confirm process, as do users changing their own email.

app/controllers/users_email_controller.rb

@@ -35,7 +35,7 @@ class UsersEmailController < ApplicationController
     RateLimiter.new(user, "change-email-hr-#{request.remote_ip}", 6, 1.hour).performed!
     RateLimiter.new(user, "change-email-min-#{request.remote_ip}", 3, 1.minute).performed!
 
-    updater = EmailUpdater.new(guardian, user)
+    updater = EmailUpdater.new(guardian: guardian, user: user, initiating_user: current_user)
     updater.change_to(params[:email])
 
     if updater.errors.present?
@@ -60,7 +60,7 @@ class UsersEmailController < ApplicationController
 
     if !@error
       # this is needed becase the form posts this field as JSON and it can be a
-      # hash when authenticatong security key.
+      # hash when authenticating security key.
       if params[:second_factor_method].to_i == UserSecondFactor.methods[:security_key]
         begin
           params[:second_factor_token] = JSON.parse(params[:second_factor_token])

lib/email_updater.rb

@@ -5,9 +5,10 @@ class EmailUpdater
 
   attr_reader :user
 
-  def initialize(guardian = nil, user = nil)
+  def initialize(guardian: nil, user: nil, initiating_user: nil)
     @guardian = guardian
     @user = user
+    @initiating_user = initiating_user
   end
 
   def self.human_attribute_name(name, options = {})
@@ -40,17 +41,14 @@ class EmailUpdater
         new_email: email,
       }
 
-      if authorize_both?
+      args, email_token = prepare_change_request(args)
-        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
       @user.email_change_requests.create!(args)
 
+      if initiating_admin_changing_another_user_email?
+        auto_confirm_and_send_password_reset(email_token)
+        return
+      end
+
       if args[:change_state] == EmailChangeRequest.states[:authorizing_new]
         send_email(:confirm_new_email, email_token)
       elsif args[:change_state] == EmailChangeRequest.states[:authorizing_old]
@@ -117,4 +115,30 @@ class EmailUpdater
                  email_token: email_token.token
   end
 
+  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
+      args[:change_state] = EmailChangeRequest.states[:authorizing_old]
+      email_token = @user.email_tokens.create!(email: args[:old_email])
+      args[:old_email_token] = email_token
+    end
+
+    [args, email_token]
+  end
+
+  def initiating_admin_changing_another_user_email?
+    @initiating_user&.admin? && @initiating_user != @user
+  end
+
+  def auto_confirm_and_send_password_reset(email_token)
+    confirm(email_token.token)
+    reset_email_token = @user.email_tokens.create(email: @user.email)
+    send_email(:forgot_password, reset_email_token)
+  end
 end

spec/components/email_updater_spec.rb

@@ -10,16 +10,102 @@ describe EmailUpdater do
     Fabricate(:user, staged: true, email: new_email)
 
     user = Fabricate(:user, email: old_email)
-    updater = EmailUpdater.new(user.guardian, user)
+    updater = EmailUpdater.new(guardian: user.guardian, user: user)
     updater.change_to(new_email)
 
     expect(updater.errors).to be_present
     expect(updater.errors.messages[:base].first).to be I18n.t("change_email.error_staged")
   end
 
+  context "when an admin is changing the email of another user" do
+    let(:admin) { Fabricate(:admin) }
+    let(:updater) { EmailUpdater.new(guardian: user.guardian, user: user, initiating_user: admin) }
+
+    def expect_old_email_job
+      Jobs.expects(:enqueue).with(:critical_user_email, has_entries(to_address: old_email, type: :notify_old_email, user_id: user.id))
+    end
+
+    def expect_forgot_password_job
+      Jobs.expects(:enqueue).with(:critical_user_email, has_entries(type: :forgot_password, user_id: user.id))
+    end
+
+    context "for a regular user" do
+      let(:user) { Fabricate(:user, 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
+        updater.change_to(new_email)
+      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])
+      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)
+      end
+    end
+
+    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
+        updater.change_to(new_email)
+      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])
+      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)
+      end
+    end
+
+    context "when changing their own email" do
+      let(:user) { admin }
+
+      before do
+        admin.update(email: old_email)
+        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 "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 "does not immediately confirm the request" do
+        expect(@change_req.change_state).not_to eq(EmailChangeRequest.states[:complete])
+      end
+    end
+  end
+
   context 'as a regular user' do
     let(:user) { Fabricate(:user, email: old_email) }
-    let(:updater) { EmailUpdater.new(user.guardian, user) }
+    let(:updater) { EmailUpdater.new(guardian: user.guardian, user: user) }
 
     before do
       Jobs.expects(:enqueue).once.with(:critical_user_email, has_entries(type: :confirm_new_email, to_address: new_email))
@@ -63,7 +149,7 @@ describe EmailUpdater do
 
   context 'as a staff user' do
     let(:user) { Fabricate(:moderator, email: old_email) }
-    let(:updater) { EmailUpdater.new(user.guardian, user) }
+    let(:updater) { EmailUpdater.new(guardian: user.guardian, user: user) }
 
     before do
       Jobs.expects(:enqueue).once.with(:critical_user_email, has_entries(type: :confirm_old_email, to_address: old_email))
@@ -140,7 +226,7 @@ describe EmailUpdater do
 
     let(:user) { Fabricate(:user, email: old_email) }
     let(:existing) { Fabricate(:user, email: new_email) }
-    let(:updater) { EmailUpdater.new(user.guardian, user) }
+    let(:updater) { EmailUpdater.new(guardian: user.guardian, user: user) }
 
     it "doesn't error if user exists with new email" do
       updater.change_to(existing.email)

spec/requests/omniauth_callbacks_controller_spec.rb

@@ -655,7 +655,7 @@ RSpec.describe Users::OmniauthCallbacksController do
                          username: 'boguslaw',
                          email: new_email }
 
-        updater = EmailUpdater.new(user.guardian, user)
+        updater = EmailUpdater.new(guardian: user.guardian, user: user)
         updater.change_to(new_email)
 
         user.reload

spec/requests/users_email_controller_spec.rb

@@ -26,7 +26,7 @@ describe UsersEmailController do
     end
 
     it 'does not change email if accounts mismatch' do
-      updater = EmailUpdater.new(user.guardian, user)
+      updater = EmailUpdater.new(guardian: user.guardian, user: user)
       updater.change_to('new.n.cool@example.com')
 
       old_email = user.email
@@ -42,7 +42,7 @@ describe UsersEmailController do
     end
 
     context "with a valid user" do
-      let(:updater) { EmailUpdater.new(user.guardian, user) }
+      let(:updater) { EmailUpdater.new(guardian: user.guardian, user: user) }
 
       before do
         sign_in(user)
@@ -221,7 +221,7 @@ describe UsersEmailController do
     it 'bans change when accounts do not match' do
 
       sign_in(user)
-      updater = EmailUpdater.new(moderator.guardian, moderator)
+      updater = EmailUpdater.new(guardian: moderator.guardian, user: moderator)
       updater.change_to('new.n.cool@example.com')
 
       get "/u/confirm-old-email/#{moderator.email_tokens.last.token}"
@@ -235,7 +235,7 @@ describe UsersEmailController do
       it 'confirms with a correct token' do
         # NOTE: only moderators need to confirm both old and new
         sign_in(moderator)
-        updater = EmailUpdater.new(moderator.guardian, moderator)
+        updater = EmailUpdater.new(guardian: moderator.guardian, user: moderator)
         updater.change_to('new.n.cool@example.com')
 
         get "/u/confirm-old-email/#{moderator.email_tokens.last.token}"