From 97d8f193872c609eac1f0a44caba8501ce2d8aff Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 20 Feb 2020 09:52:21 +1000 Subject: [PATCH] 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 | 4 +- lib/email_updater.rb | 44 +++++++-- spec/components/email_updater_spec.rb | 94 ++++++++++++++++++- .../omniauth_callbacks_controller_spec.rb | 2 +- spec/requests/users_email_controller_spec.rb | 8 +- 5 files changed, 131 insertions(+), 21 deletions(-) diff --git a/app/controllers/users_email_controller.rb b/app/controllers/users_email_controller.rb index 96f027880c4..a34cf215927 100644 --- a/app/controllers/users_email_controller.rb +++ b/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]) diff --git a/lib/email_updater.rb b/lib/email_updater.rb index 023389fc020..b0502256b83 100644 --- a/lib/email_updater.rb +++ b/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[: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 + args, email_token = prepare_change_request(args) @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 diff --git a/spec/components/email_updater_spec.rb b/spec/components/email_updater_spec.rb index 81018733dcb..a36958f3e65 100644 --- a/spec/components/email_updater_spec.rb +++ b/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) diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index 43290fa7a44..204c3f1572c 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/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 diff --git a/spec/requests/users_email_controller_spec.rb b/spec/requests/users_email_controller_spec.rb index 2025ec2c75b..f7c350199d5 100644 --- a/spec/requests/users_email_controller_spec.rb +++ b/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}"