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

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.
diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/email.js b/app/assets/javascripts/discourse/app/controllers/preferences/email.js
index 2448eed..16772da 100644
--- a/app/assets/javascripts/discourse/app/controllers/preferences/email.js
+++ b/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();
diff --git a/app/assets/javascripts/discourse/app/templates/preferences-email.hbs b/app/assets/javascripts/discourse/app/templates/preferences-email.hbs
index d4f4253..5dd2eb1 100644
--- a/app/assets/javascripts/discourse/app/templates/preferences-email.hbs
+++ b/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>
 
diff --git a/app/controllers/users_email_controller.rb b/app/controllers/users_email_controller.rb
index 898ddd6..05a0a55 100644
--- a/app/controllers/users_email_controller.rb
+++ b/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,
-    :confirm_new_email,
-    :show_confirm_new_email
+    :show_confirm_old_email
   ]
 
   before_action :require_login, only: [
     :confirm_old_email,
-    :show_confirm_old_email,
-    :confirm_new_email,
-    :show_confirm_new_email
+    :show_confirm_old_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
diff --git a/app/jobs/regular/user_email.rb b/app/jobs/regular/user_email.rb
index d39e02c..f9d9fd6 100644
--- a/app/jobs/regular/user_email.rb
+++ b/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]
diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb
index 6e8b69a..93ee5e3 100644
--- a/app/mailers/user_notifications.rb
+++ b/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]
     )
diff --git a/app/models/email_change_request.rb b/app/models/email_change_request.rb
index 4307cdb..be1448f 100644
--- a/app/models/email_change_request.rb
+++ b/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
-#  user_id            :integer          not null
-#  old_email          :string
-#  new_email          :string           not null
-#  old_email_token_id :integer
-#  new_email_token_id :integer
-#  change_state       :integer          not null
-#  created_at         :datetime         not null
-#  updated_at         :datetime         not null
+#  id                   :integer          not null, primary key
+#  user_id              :integer          not null
+#  old_email            :string
+#  new_email            :string           not null
+#  old_email_token_id   :integer
+#  new_email_token_id   :integer
+#  change_state         :integer          not null
+#  created_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)
 #
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index 5fa50d9..edefecc 100644
--- a/config/locales/client.en.yml
+++ b/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:
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index eff5312..6d0ccad 100644
--- a/config/locales/server.en.yml
+++ b/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:
+

[... diff too long, it was truncated ...]

GitHub sha: 6e2be3e6

1 Like

This commit appears in #10830 which was approved by eviltrout and tgxworld. It was merged by martin.