diff --git a/app/controllers/emails_controller.rb b/app/controllers/emails_controller.rb index de301bab2..429392048 100644 --- a/app/controllers/emails_controller.rb +++ b/app/controllers/emails_controller.rb @@ -1,4 +1,5 @@ class EmailsController < ApplicationController + before_action :requires_reauthentication, only: [:edit, :update] respond_to :html, :xml, :json def index @@ -24,17 +25,10 @@ class EmailsController < ApplicationController def update @user = authorize User.find(params[:user_id]), policy_class: EmailAddressPolicy - - if @user.authenticate_password(params[:user][:password]) - UserEvent.build_from_request(@user, :email_change, request) - @user.update(email_address_attributes: { address: params[:user][:email] }) - else - @user.errors.add(:base, "Password was incorrect") - end + @user.change_email(params[:user][:email], request) if @user.errors.none? flash[:notice] = "Email updated. Check your email to confirm your new address" - UserMailer.email_change_confirmation(@user).deliver_later respond_with(@user, location: settings_url) else flash[:notice] = @user.errors.full_messages.join("; ") diff --git a/app/models/user.rb b/app/models/user.rb index 5af3503d6..fe470dad1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -361,6 +361,17 @@ class User < ApplicationRecord def can_receive_email?(require_verification: true) email_address.present? && email_address.is_deliverable? && (email_address.is_verified? || !require_verification) end + + def change_email(new_email, request) + transaction do + update(email_address_attributes: { address: new_email }) + + if errors.none? + UserEvent.create_from_request!(self, :email_change, request) + UserMailer.email_change_confirmation(self).deliver_later + end + end + end end concerning :BlacklistMethods do diff --git a/app/views/emails/edit.html.erb b/app/views/emails/edit.html.erb index 6db4e8281..4e909ec86 100644 --- a/app/views/emails/edit.html.erb +++ b/app/views/emails/edit.html.erb @@ -1,17 +1,10 @@
+ <% page_title "Change Email" %> +

Change Email

+ <% if @user.email_address.present? %> - <% page_title "Change Email" %> -

Change Email

-

Your current email address is <%= @user.email_address.address %>. - You must re-enter your password in order to update your email address.

- <% else %> - <% page_title "Add Email" %> -

Add Email

- -

Add a new email address below. You must re-enter your password in - order to update your email address.

<% end %> <% if @user.is_restricted? %> @@ -23,8 +16,7 @@ <% end %> <%= edit_form_for(@user, url: user_email_path(@user)) do |f| %> - <%= f.input :email, as: :email, label: "New Email", input_html: { value: "" } %> - <%= f.input :password %> + <%= f.input :email, as: :email, input_html: { value: @user&.email_address&.address } %> <%= f.submit "Save" %> <% end %>
diff --git a/test/factories/email_address.rb b/test/factories/email_address.rb index e0b77da34..f2ead6b25 100644 --- a/test/factories/email_address.rb +++ b/test/factories/email_address.rb @@ -2,5 +2,6 @@ FactoryBot.define do factory(:email_address) do address { FFaker::Internet.email } is_verified { true } + user end end diff --git a/test/functional/emails_controller_test.rb b/test/functional/emails_controller_test.rb index 91a8e3f1e..8750010e6 100644 --- a/test/functional/emails_controller_test.rb +++ b/test/functional/emails_controller_test.rb @@ -43,6 +43,17 @@ class EmailsControllerTest < ActionDispatch::IntegrationTest end context "#edit" do + context "for a user who hasn't recently authenticated" do + should "redirect to the confirm password page" do + post session_path, params: { name: @user.name, password: @user.password } + travel_to 2.hours.from_now do + get edit_user_email_path(@user) + end + + assert_redirected_to confirm_password_session_path(url: edit_user_email_path(@user.id)) + end + end + context "for a user with an email address" do should "render" do get_auth edit_user_email_path(@user), @user @@ -59,7 +70,7 @@ class EmailsControllerTest < ActionDispatch::IntegrationTest assert_equal false, @user.email_address.present? assert_response :success - assert_select "h1", text: "Add Email" + assert_select "h1", text: "Change Email" end end @@ -79,10 +90,24 @@ class EmailsControllerTest < ActionDispatch::IntegrationTest end context "#update" do + context "for a user who hasn't recently authenticated" do + should "redirect to the confirm password page" do + post session_path, params: { name: @user.name, password: @user.password } + travel_to 2.hours.from_now do + put user_email_path(@user), params: { user: { email: "abc@ogres.net" }} + end + + assert_redirected_to confirm_password_session_path(url: user_email_path(@user.id)) + assert_equal("bob@ogres.net", @user.reload.email_address.address) + assert_no_emails + assert_equal(false, @user.user_events.email_change.exists?) + end + end + context "with the correct password" do should "update an existing address" do assert_difference("EmailAddress.count", 0) do - put_auth user_email_path(@user), @user, params: { user: { password: "password", email: "abc@ogres.net" }} + put_auth user_email_path(@user), @user, params: { user: { email: "abc@ogres.net" }} end assert_redirected_to(settings_path) @@ -96,7 +121,7 @@ class EmailsControllerTest < ActionDispatch::IntegrationTest @user.email_address.destroy assert_difference("EmailAddress.count", 1) do - put_auth user_email_path(@user), @user, params: { user: { password: "password", email: "abc@ogres.net" }} + put_auth user_email_path(@user), @user, params: { user: { email: "abc@ogres.net" }} end assert_redirected_to(settings_path) @@ -108,7 +133,7 @@ class EmailsControllerTest < ActionDispatch::IntegrationTest should "not allow banned users to change their email address" do create(:ban, user: @user, duration: 1.week) - put_auth user_email_path(@user), @user, params: { user: { password: "password", email: "abc@ogres.net" }} + put_auth user_email_path(@user), @user, params: { user: { email: "abc@ogres.net" }} assert_response 403 assert_equal("bob@ogres.net", @user.reload.email_address.address) @@ -117,18 +142,6 @@ class EmailsControllerTest < ActionDispatch::IntegrationTest end end - context "with the incorrect password" do - should "not work" do - put_auth user_email_path(@user), @user, params: { user: { password: "passwordx", email: "abc@ogres.net" }} - - assert_response :success - assert_equal("bob@ogres.net", @user.reload.email_address.address) - assert_no_emails - assert_equal(false, @user.user_events.email_change.exists?) - end - end - end - context "#verify" do context "with a correct verification key" do should "mark the email address as verified" do