From 3cd601dcc926cb6d6b09fd10fc3a5e622fc4a98b Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 29 Sep 2020 09:45:45 +1000 Subject: [PATCH] FIX: Admin change email for user process improvements and fixes (#10755) See https://meta.discourse.org/t/changing-a-users-email/164512 for context. When admin changes an email for a user, we were incorrectly sending the password reset email to the user's old address. Also the new email does not come into effect until the reset password process is done, so this PR adds some notes to the admin to make this clearer. --- .../app/controllers/preferences/email.js | 25 +++++++++++++++++-- .../app/templates/preferences-email.hbs | 11 +++----- config/locales/client.en.yml | 2 ++ lib/email_updater.rb | 1 + spec/components/email_updater_spec.rb | 4 ++- 5 files changed, 33 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/email.js b/app/assets/javascripts/discourse/app/controllers/preferences/email.js index f18ed92c997..2448eedae80 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 } from "@ember/object/computed"; +import { empty, or, alias } from "@ember/object/computed"; import Controller from "@ember/controller"; import { propertyEqual } from "discourse/lib/computed"; import EmberObject from "@ember/object"; @@ -15,6 +15,7 @@ export default Controller.extend({ success: false, oldEmail: null, newEmail: null, + successMessage: null, newEmailEmpty: empty("newEmail"), @@ -28,6 +29,8 @@ export default Controller.extend({ unchanged: propertyEqual("newEmailLower", "oldEmail"), + currentUserAdmin: alias("currentUser.admin"), + @discourseComputed("newEmail") newEmailLower(newEmail) { return newEmail.toLowerCase().trim(); @@ -77,7 +80,25 @@ export default Controller.extend({ ? this.model.addEmail(this.newEmail) : this.model.changeEmail(this.newEmail) ).then( - () => this.set("success", true), + () => { + this.set("success", true); + + if (this.model.staff) { + this.set( + "successMessage", + I18n.t("user.change_email.success_staff") + ); + } else { + if (this.currentUser.admin) { + this.set( + "successMessage", + I18n.t("user.change_email.success_via_admin") + ); + } else { + this.set("successMessage", I18n.t("user.change_email.success")); + } + } + }, (e) => { this.setProperties({ error: true, saving: false }); if ( diff --git a/app/assets/javascripts/discourse/app/templates/preferences-email.hbs b/app/assets/javascripts/discourse/app/templates/preferences-email.hbs index 447d220403c..d4f42533f39 100644 --- a/app/assets/javascripts/discourse/app/templates/preferences-email.hbs +++ b/app/assets/javascripts/discourse/app/templates/preferences-email.hbs @@ -11,13 +11,7 @@
-

- {{#if model.staff}} - {{i18n "user.change_email.success_staff"}} - {{else}} - {{i18n "user.change_email.success"}} - {{/if}} -

+

{{ successMessage }}

@@ -44,6 +38,9 @@ {{i18n "user.email.instructions"}} {{/if}} + {{#if currentUserAdmin}} +

{{i18n "user.email.admin_note"}}

+ {{/if}} diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 2396307e2bb..34c4cc95553 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1148,6 +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_staff: "We've sent an email to your current address. Please follow the confirmation instructions." change_avatar: @@ -1190,6 +1191,7 @@ en: sso_override_instructions: "Email can be updated from SSO provider." no_secondary: "No secondary emails" instructions: "Never shown to the public." + admin_note: "Note: An admin user changing another non-admin user's email indicates the user has lost access to their original email account, so a reset password email will be sent to their new address. The user's email will not change until they complete the reset password process." ok: "We will email you to confirm" required: "Please enter an email address" invalid: "Please enter a valid email address" diff --git a/lib/email_updater.rb b/lib/email_updater.rb index ad886a066eb..13633a09944 100644 --- a/lib/email_updater.rb +++ b/lib/email_updater.rb @@ -120,6 +120,7 @@ class EmailUpdater else @user.user_emails.create!(email: new_email) end + @user.reload DiscourseEvent.trigger(:user_updated, @user) @user.set_automatic_groups diff --git a/spec/components/email_updater_spec.rb b/spec/components/email_updater_spec.rb index 4fd6cbaf6c9..ff9d23093b8 100644 --- a/spec/components/email_updater_spec.rb +++ b/spec/components/email_updater_spec.rb @@ -49,7 +49,7 @@ describe EmailUpdater do 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 + user.reload expect(user.reload.email).to eq(new_email) end @@ -59,6 +59,8 @@ describe EmailUpdater do updater.change_to(new_email) end end + + expect(EmailToken.where(user: user).last.email).to eq(new_email) end end