FIX: When admin changes an email for the user the user must confirm the change (#10830)

See https://meta.discourse.org/t/changing-a-users-email/164512 for additional context.

Previously when an admin user changed a user's email we assumed that they would need a password reset too because they likely did not have access to their account. This proved to be incorrect, as there are other reasons a user needs admin to change their email. This PR:

* Changes the admin change email for user flow so the user is sent an email to confirm the change
* We now record who the email change request was requested by
* If the requested by user is admin and not the user we note this in the email sent to the user
* We also make the confirm change email route open to anonymous users, so it can be clicked by the user even if they do not have access to their account. If there is a logged in user we make sure the confirmation matches the current user.

app/assets/javascripts/discourse/app/controllers/preferences/email.js

@@ -1,6 +1,6 @@
 import I18n from "I18n";
 import discourseComputed from "discourse-common/utils/decorators";
-import { empty, or, alias } from "@ember/object/computed";
+import { empty, or } from "@ember/object/computed";
 import Controller from "@ember/controller";
 import { propertyEqual } from "discourse/lib/computed";
 import EmberObject from "@ember/object";
@@ -29,8 +29,6 @@ export default Controller.extend({
 
   unchanged: propertyEqual("newEmailLower", "oldEmail"),
 
-  currentUserAdmin: alias("currentUser.admin"),
-
   @discourseComputed("newEmail")
   newEmailLower(newEmail) {
     return newEmail.toLowerCase().trim();

app/assets/javascripts/discourse/app/templates/preferences-email.hbs

@@ -38,9 +38,6 @@
               {{i18n "user.email.instructions"}}
             {{/if}}
           </div>
-          {{#if currentUserAdmin}}
-            <p>{{i18n "user.email.admin_note"}}</p>
-          {{/if}}
         </div>
       </div>
 

app/controllers/users_email_controller.rb

@@ -13,16 +13,12 @@ class UsersEmailController < ApplicationController
 
   skip_before_action :redirect_to_login_if_required, only: [
     :confirm_old_email,
-    :show_confirm_old_email,
+    :show_confirm_old_email
-    :confirm_new_email,
-    :show_confirm_new_email
   ]
 
   before_action :require_login, only: [
     :confirm_old_email,
-    :show_confirm_old_email,
+    :show_confirm_old_email
-    :confirm_new_email,
-    :show_confirm_new_email
   ]
 
   def index
@@ -218,7 +214,7 @@ class UsersEmailController < ApplicationController
       @error = I18n.t("change_email.already_done")
     end
 
-    if current_user.id != @user&.id
+    if current_user && current_user.id != @user&.id
       @error = I18n.t 'change_email.wrong_account_error'
     end
   end

app/jobs/regular/user_email.rb

@@ -153,7 +153,18 @@ module Jobs
       # Make sure that mailer exists
       raise Discourse::InvalidParameters.new("type=#{type}") unless UserNotifications.respond_to?(type)
 
-      email_args[:email_token] = email_token if email_token.present?
+      if email_token.present?
+        email_args[:email_token] = email_token
+
+        if type.to_s == "confirm_new_email"
+          change_req = EmailChangeRequest.find_by_new_token(email_token)
+
+          if change_req
+            email_args[:requested_by_admin] = change_req.requested_by_admin?
+          end
+        end
+      end
+
       email_args[:new_email] = args[:new_email] || user.email if type.to_s == "notify_old_email" || type.to_s == "notify_old_email_add"
 
       if args[:client_ip] && args[:user_agent]

app/mailers/user_notifications.rb

@@ -88,7 +88,7 @@ class UserNotifications < ActionMailer::Base
 
   def confirm_new_email(user, opts = {})
     build_user_email_token_by_template(
-      "user_notifications.confirm_new_email",
+      opts[:requested_by_admin] ? "user_notifications.confirm_new_email_via_admin" : "user_notifications.confirm_new_email",
       user,
       opts[:email_token]
     )

app/models/email_change_request.rb

@@ -4,6 +4,7 @@ class EmailChangeRequest < ActiveRecord::Base
   belongs_to :old_email_token, class_name: 'EmailToken'
   belongs_to :new_email_token, class_name: 'EmailToken'
   belongs_to :user
+  belongs_to :requested_by, class_name: "User", foreign_key: :requested_by_user_id
 
   validates :new_email, presence: true, format: { with: EmailValidator.email_regex }
 
@@ -11,23 +12,38 @@ class EmailChangeRequest < ActiveRecord::Base
     @states ||= Enum.new(authorizing_old: 1, authorizing_new: 2, complete: 3)
   end
 
+  def requested_by_admin?
+    self.requested_by.admin? && !self.requested_by_self?
+  end
+
+  def requested_by_self?
+    self.requested_by_user_id == self.user_id
+  end
+
+  def self.find_by_new_token(token)
+    joins(
+      "INNER JOIN email_tokens ON email_tokens.id = email_change_requests.new_email_token_id"
+    ).where("email_tokens.token = ?", token).last
+  end
 end
 
 # == Schema Information
 #
 # Table name: email_change_requests
 #
-#  id                 :integer          not null, primary key
+#  id                   :integer          not null, primary key
-#  user_id            :integer          not null
+#  user_id              :integer          not null
-#  old_email          :string
+#  old_email            :string
-#  new_email          :string           not null
+#  new_email            :string           not null
-#  old_email_token_id :integer
+#  old_email_token_id   :integer
-#  new_email_token_id :integer
+#  new_email_token_id   :integer
-#  change_state       :integer          not null
+#  change_state         :integer          not null
-#  created_at         :datetime         not null
+#  created_at           :datetime         not null
-#  updated_at         :datetime         not null
+#  updated_at           :datetime         not null
+#  requested_by_user_id :integer
 #
 # Indexes
 #
-#  index_email_change_requests_on_user_id  (user_id)
+#  idx_email_change_requests_on_requested_by  (requested_by_user_id)
+#  index_email_change_requests_on_user_id     (user_id)
 #

config/locales/client.en.yml

@@ -1148,7 +1148,7 @@ en:
         taken: "Sorry, that email is not available."
         error: "There was an error changing your email. Perhaps that address is already in use?"
         success: "We've sent an email to that address. Please follow the confirmation instructions."
-        success_via_admin: "Because you are changing the email of the user, we assume that they have lost access to their original email account. We sent a reset password email to the new email address for this user. Please note the user must complete the reset password process for the email address change to take effect."
+        success_via_admin: "We've sent an email to that address. The user will need to follow the confirmation instructions in the email."
         success_staff: "We've sent an email to your current address. Please follow the confirmation instructions."
 
       change_avatar:

config/locales/server.en.yml

@@ -3715,6 +3715,18 @@ en:
 
         %{base_url}/u/confirm-new-email/%{email_token}
 
+        If you did not request this change, please contact %{site_name} admin.
+
+    confirm_new_email_via_admin:
+      title: "Confirm New Email"
+      subject_template: "[%{email_prefix}] Confirm your new email address"
+      text_body_template: |
+        Confirm your new email address for %{site_name} by clicking on the following link:
+
+        %{base_url}/u/confirm-new-email/%{email_token}
+
+        This email change was requested by a site admin. If you did not request this change, please contact %{site_name} admin.
+
     confirm_old_email:
       title: "Confirm Old Email"
       subject_template: "[%{email_prefix}] Confirm your current email address"

db/migrate/20201006021020_add_requested_by_to_email_change_request.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+class AddRequestedByToEmailChangeRequest < ActiveRecord::Migration[6.0]
+  def up
+    add_column :email_change_requests, :requested_by_user_id, :integer, null: true
+
+    DB.exec("CREATE INDEX IF NOT EXISTS idx_email_change_requests_on_requested_by ON email_change_requests(requested_by_user_id)")
+  end
+
+  def down
+    remove_column :email_change_requests, :requested_by_user_id
+  end
+end

lib/email_updater.rb

@@ -41,14 +41,9 @@ class EmailUpdater
       UserHistory.create!(action: UserHistory.actions[:add_email], acting_user_id: @user.id)
     end
 
-    if @guardian.is_staff? && !@user.staff?
+    change_req = EmailChangeRequest.find_or_initialize_by(
-      send_email_notification(@user.email, email)
+      user_id: @user.id, new_email: email, requested_by: @guardian.user
-      update_user_email(old_email, email)
+    )
-      send_email(:forgot_password, @user.email_tokens.create!(email: @user.email))
-      return
-    end
-
-    change_req = EmailChangeRequest.find_or_initialize_by(user_id: @user.id, new_email: email)
     if change_req.new_record?
       change_req.old_email = old_email
       change_req.new_email = email

spec/components/email_updater_spec.rb

@@ -27,40 +27,33 @@ describe EmailUpdater do
       end
     end
 
-    def expect_forgot_password_job
-      expect_enqueued_with(job: :critical_user_email, args: { type: :forgot_password, user_id: user.id }) do
-        yield
-      end
-    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
+      it "sends an email to the user for them to confirm the email change" do
-        expect_old_email_job do
+        expect_enqueued_with(job: :critical_user_email, args: { type: :confirm_new_email, to_address: new_email }) do
-          expect_forgot_password_job do
+          updater.change_to(new_email)
-            updater.change_to(new_email)
-
-            expect(Jobs::CriticalUserEmail.jobs.size).to eq(2)
-          end
         end
-
       end
 
-      it "creates a change request authorizing the new email and immediately confirms it " do
+      it "logs the admin user as the requester" do
         updater.change_to(new_email)
-        user.reload
+        @change_req = user.email_change_requests.first
-        expect(user.reload.email).to eq(new_email)
+        expect(@change_req.requested_by).to eq(admin)
       end
 
-      it "sends a reset password email to the user so they can set a password for their new email" do
+      it "starts the new confirmation process" do
-        expect_old_email_job do
+        updater.change_to(new_email)
-          expect_forgot_password_job do
+        @change_req = user.email_change_requests.first
-            updater.change_to(new_email)
+        expect(updater.errors).to be_blank
-          end
-        end
 
-        expect(EmailToken.where(user: user).last.email).to eq(new_email)
+        expect(@change_req).to be_present
+        expect(@change_req.change_state).to eq(EmailChangeRequest.states[:authorizing_new])
+
+        expect(@change_req.old_email).to eq(old_email)
+        expect(@change_req.new_email).to eq(new_email)
+        expect(@change_req.old_email_token).to be_blank
+        expect(@change_req.new_email_token.email).to eq(new_email)
       end
     end
 
@@ -105,6 +98,12 @@ describe EmailUpdater do
         @change_req = user.email_change_requests.first
       end
 
+      it "logs the user as the requester" do
+        updater.change_to(new_email)
+        @change_req = user.email_change_requests.first
+        expect(@change_req.requested_by).to eq(user)
+      end
+
       it "starts the old confirmation process" do
         expect(updater.errors).to be_blank
 

spec/jobs/user_email_spec.rb

@@ -288,6 +288,37 @@ describe Jobs::UserEmail do
       expect(mail.body).to include("asdfasdf")
     end
 
+    context "confirm_new_email" do
+      let(:email_token) { Fabricate(:email_token, user: user) }
+      before do
+        EmailChangeRequest.create!(
+          user: user,
+          requested_by: requested_by,
+          new_email_token: email_token,
+          new_email: "testnew@test.com",
+          change_state: EmailChangeRequest.states[:authorizing_new]
+        )
+      end
+
+      context "when the change was requested by admin" do
+        let(:requested_by) { Fabricate(:admin) }
+        it "passes along true for the requested_by_admin param which changes the wording in the email" do
+          Jobs::UserEmail.new.execute(type: :confirm_new_email, user_id: user.id, email_token: email_token.token)
+          mail = ActionMailer::Base.deliveries.first
+          expect(mail.body).to include("This email change was requested by a site admin.")
+        end
+      end
+
+      context "when the change was requested by the user" do
+        let(:requested_by) { user }
+        it "passes along false for the requested_by_admin param which changes the wording in the email" do
+          Jobs::UserEmail.new.execute(type: :confirm_new_email, user_id: user.id, email_token: email_token.token)
+          mail = ActionMailer::Base.deliveries.first
+          expect(mail.body).not_to include("This email change was requested by a site admin.")
+        end
+      end
+    end
+
     context "post" do
       fab!(:post) { Fabricate(:post, user: user) }
 

spec/mailers/user_notifications_spec.rb

@@ -78,7 +78,33 @@ describe UserNotifications do
       expect(subject.from).to eq([SiteSetting.notification_email])
       expect(subject.body).to be_present
     end
+  end
 
+  describe ".confirm_new_email" do
+    let(:opts) do
+      { requested_by_admin: requested_by_admin, email_token: token }
+    end
+    let(:token) { "test123" }
+
+    context "when requested by admin" do
+      let(:requested_by_admin) { true }
+
+      it "uses the requested by admin template" do
+        expect(UserNotifications.confirm_new_email(user, opts).body).to include(
+          "This email change was requested by a site admin."
+        )
+      end
+    end
+
+    context "when not requested by admin" do
+      let(:requested_by_admin) { false }
+
+      it "uses the normal template" do
+        expect(UserNotifications.confirm_new_email(user, opts).body).not_to include(
+          "This email change was requested by a site admin."
+        )
+      end
+    end
   end
 
   describe '.email_login' do

spec/requests/users_email_controller_spec.rb

@@ -9,11 +9,10 @@ describe UsersEmailController do
   fab!(:moderator) { Fabricate(:moderator) }
 
   describe "#confirm-new-email" do
-    it 'redirects to login for signed out accounts' do
+    it 'does not redirect to login for signed out accounts, this route works fine as anon user' do
       get "/u/confirm-new-email/asdfasdf"
 
-      expect(response.status).to eq(302)
+      expect(response.status).to eq(200)
-      expect(response.redirect_url).to eq("http://test.localhost/login")
     end
 
     it 'errors out for invalid tokens' do
@@ -25,7 +24,7 @@ describe UsersEmailController do
       expect(response.body).to include(I18n.t('change_email.already_done'))
     end
 
-    it 'does not change email if accounts mismatch' do
+    it 'does not change email if accounts mismatch for a signed in user' do
       updater = EmailUpdater.new(guardian: user.guardian, user: user)
       updater.change_to('new.n.cool@example.com')