From 6e2be3e60bb882dd92d270d11c71fba9b0901b5e Mon Sep 17 00:00:00 2001
From: Martin Brennan <mjrbrennan@gmail.com>
Date: Wed, 7 Oct 2020 13:02:24 +1000
Subject: [PATCH] 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/controllers/preferences/email.js | 4 +-
.../app/templates/preferences-email.hbs | 3 --
app/controllers/users_email_controller.rb | 10 ++--
app/jobs/regular/user_email.rb | 13 ++++-
app/mailers/user_notifications.rb | 2 +-
app/models/email_change_request.rb | 36 ++++++++++----
config/locales/client.en.yml | 2 +-
config/locales/server.en.yml | 12 +++++
...dd_requested_by_to_email_change_request.rb | 13 +++++
lib/email_updater.rb | 11 ++---
spec/components/email_updater_spec.rb | 47 +++++++++----------
spec/jobs/user_email_spec.rb | 31 ++++++++++++
spec/mailers/user_notifications_spec.rb | 26 ++++++++++
spec/requests/users_email_controller_spec.rb | 7 ++-
14 files changed, 155 insertions(+), 62 deletions(-)
create mode 100644 db/migrate/20201006021020_add_requested_by_to_email_change_request.rb
diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/email.js b/app/assets/javascripts/discourse/app/controllers/preferences/email.js
index 2448eedae80..16772da45f2 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 d4f42533f39..5dd2eb1826c 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 898ddd6897a..05a0a558577 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 d39e02ce4fe..f9d9fd6852f 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 6e8b69aa3de..93ee5e3d880 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 4307cdb457f..be1448ff92b 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 5fa50d9da54..edefecc3c09 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 eff53122527..6d0ccad7d54 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:
+
+ %{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"
diff --git a/db/migrate/20201006021020_add_requested_by_to_email_change_request.rb b/db/migrate/20201006021020_add_requested_by_to_email_change_request.rb
new file mode 100644
index 00000000000..6753a189757
--- /dev/null
+++ b/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
diff --git a/lib/email_updater.rb b/lib/email_updater.rb
index 13633a09944..5b7b0aec950 100644
--- a/lib/email_updater.rb
+++ b/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?
- send_email_notification(@user.email, email)
- 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)
+ change_req = EmailChangeRequest.find_or_initialize_by(
+ user_id: @user.id, new_email: email, requested_by: @guardian.user
+ )
if change_req.new_record?
change_req.old_email = old_email
change_req.new_email = email
diff --git a/spec/components/email_updater_spec.rb b/spec/components/email_updater_spec.rb
index ff9d23093b8..31dbe7af9db 100644
--- a/spec/components/email_updater_spec.rb
+++ b/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
- expect_old_email_job do
- expect_forgot_password_job do
- updater.change_to(new_email)
-
- expect(Jobs::CriticalUserEmail.jobs.size).to eq(2)
- end
+ it "sends an email to the user for them to confirm the email change" do
+ expect_enqueued_with(job: :critical_user_email, args: { type: :confirm_new_email, to_address: new_email }) do
+ updater.change_to(new_email)
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
- expect(user.reload.email).to eq(new_email)
+ @change_req = user.email_change_requests.first
+ 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
- expect_old_email_job do
- expect_forgot_password_job do
- updater.change_to(new_email)
- end
- end
+ it "starts the new confirmation process" do
+ updater.change_to(new_email)
+ @change_req = user.email_change_requests.first
+ expect(updater.errors).to be_blank
- 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
diff --git a/spec/jobs/user_email_spec.rb b/spec/jobs/user_email_spec.rb
index a716f524e1d..4a2b40b24ab 100644
--- a/spec/jobs/user_email_spec.rb
+++ b/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) }
diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb
index eefdbfa9ebd..1b422da2c7f 100644
--- a/spec/mailers/user_notifications_spec.rb
+++ b/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
diff --git a/spec/requests/users_email_controller_spec.rb b/spec/requests/users_email_controller_spec.rb
index 936a9a93fec..45957394419 100644
--- a/spec/requests/users_email_controller_spec.rb
+++ b/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.redirect_url).to eq("http://test.localhost/login")
+ expect(response.status).to eq(200)
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')