From 32613f9bb1abebd0e9aeebd93e16312ef86997be Mon Sep 17 00:00:00 2001 From: evazion Date: Sun, 2 Jan 2022 12:23:01 -0600 Subject: [PATCH] emails: fix sending emails to invalid addresses. Fix mailers to not attempt deliveries to invalid or nonexistent email addresses. This usually happened when someone changed their email, and we tried to send a confirmation email to a nonexistent address. --- app/controllers/password_resets_controller.rb | 2 +- app/controllers/users_controller.rb | 2 +- app/mailers/application_mailer.rb | 7 +++++++ app/mailers/user_mailer.rb | 8 ++++---- app/models/dmail.rb | 2 +- app/models/user.rb | 6 +----- test/functional/users_controller_test.rb | 2 +- test/unit/user_mailer_test.rb | 14 ++++++++++++++ 8 files changed, 30 insertions(+), 13 deletions(-) diff --git a/app/controllers/password_resets_controller.rb b/app/controllers/password_resets_controller.rb index ff09b5c70..202c54930 100644 --- a/app/controllers/password_resets_controller.rb +++ b/app/controllers/password_resets_controller.rb @@ -9,7 +9,7 @@ class PasswordResetsController < ApplicationController if @user.blank? flash[:notice] = "That account does not exist" redirect_to password_reset_path - elsif @user.can_receive_email?(require_verification: false) + elsif @user.can_receive_email?(require_verified_email: false) UserMailer.password_reset(@user).deliver_later UserEvent.create_from_request!(@user, :password_reset, request) flash[:notice] = "Password reset email sent. Check your email" diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index f2bbf88e1..95b8a8be8 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -90,7 +90,7 @@ class UsersController < ApplicationController flash[:notice] = "Sign up failed: #{@user.errors.full_messages.join("; ")}" else session[:user_id] = @user.id - UserMailer.welcome_user(@user).deliver_later if @user.can_receive_email?(require_verification: false) + UserMailer.welcome_user(@user).deliver_later set_current_user end diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index ae19ae2c1..6dbb708de 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -5,4 +5,11 @@ # @see https://guides.rubyonrails.org/action_mailer_basics.html class ApplicationMailer < ActionMailer::Base default from: "#{Danbooru.config.canonical_app_name} <#{Danbooru.config.contact_email}>", content_type: "text/html" + + def mail(user, require_verified_email:, **options) + to = email_address_with_name(user.email_address&.address, user.name) + message = super(to: to, **options) + message.perform_deliveries = user.can_receive_email?(require_verified_email: require_verified_email) + message + end end diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index 7dec38cab..0e176df0f 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -7,24 +7,24 @@ class UserMailer < ApplicationMailer # The email sent when a user receives a DMail. def dmail_notice(dmail) @dmail = dmail - mail to: dmail.to.email_with_name, subject: "#{Danbooru.config.app_name} - Message received from #{dmail.from.name}" + mail(dmail.to, require_verified_email: true, subject: "#{Danbooru.config.app_name} - Message received from #{dmail.from.name}") end # The email sent when a user requests a password reset. def password_reset(user) @user = user - mail to: @user.email_with_name, subject: "#{Danbooru.config.app_name} password reset request" + mail(@user, require_verified_email: false, subject: "#{Danbooru.config.app_name} password reset request") end # The email sent when a user changes their email address. def email_change_confirmation(user) @user = user - mail to: @user.email_with_name, subject: "Confirm your email address" + mail(@user, require_verified_email: false, subject: "Confirm your email address") end # The email sent when a new user signs up with an email address. def welcome_user(user) @user = user - mail to: @user.email_with_name, subject: "Welcome to #{Danbooru.config.app_name}! Confirm your email address" + mail(@user, require_verified_email: false, subject: "Welcome to #{Danbooru.config.app_name}! Confirm your email address") end end diff --git a/app/models/dmail.rb b/app/models/dmail.rb index 8fdb661cd..57aa89cac 100644 --- a/app/models/dmail.rb +++ b/app/models/dmail.rb @@ -141,7 +141,7 @@ class Dmail < ApplicationRecord end def send_email - if is_recipient? && !is_deleted? && to.receive_email_notifications? && to.can_receive_email? + if is_recipient? && !is_deleted? && to.receive_email_notifications? UserMailer.dmail_notice(self).deliver_later end end diff --git a/app/models/user.rb b/app/models/user.rb index 5e54b50c0..a6594285f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -361,11 +361,7 @@ class User < ApplicationRecord end module EmailMethods - def email_with_name - "#{name} <#{email_address.address}>" - end - - def can_receive_email?(require_verification: true) + def can_receive_email?(require_verified_email: true) email_address.present? && email_address.is_deliverable? && (email_address.is_verified? || !require_verification) end diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb index 8053f0ddb..9a4a111e2 100644 --- a/test/functional/users_controller_test.rb +++ b/test/functional/users_controller_test.rb @@ -259,7 +259,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest assert_equal(User::Levels::MEMBER, User.last.level) assert_equal(User.last, User.last.authenticate_password("xxxxx1")) assert_nil(User.last.email_address) - assert_no_enqueued_emails + assert_enqueued_email_with UserMailer, :welcome_user, args: [User.last], queue: "default" assert_equal(true, User.last.user_events.user_creation.exists?) end diff --git a/test/unit/user_mailer_test.rb b/test/unit/user_mailer_test.rb index 7b96b86c4..4242d60fb 100644 --- a/test/unit/user_mailer_test.rb +++ b/test/unit/user_mailer_test.rb @@ -12,6 +12,20 @@ class UserMailerTest < ActionMailer::TestCase mail = UserMailer.dmail_notice(@dmail) assert_emails(1) { mail.deliver_now } end + + should "not send an email for a user without an email address" do + @user = create(:user) + @dmail = create(:dmail, owner: @user, to: @user) + mail = UserMailer.dmail_notice(@dmail) + assert_emails(0) { mail.deliver_now } + end + + should "not send an email for a user with an undeliverable address" do + @user = create(:user, email_address: build(:email_address, is_deliverable: false)) + @dmail = create(:dmail, owner: @user, to: @user) + mail = UserMailer.dmail_notice(@dmail) + assert_emails(0) { mail.deliver_now } + end end context "password_reset method" do