From 0f688f45bddaac17b2840ab9eed6b2dec2a7f9cf Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 5 Jul 2021 14:56:32 +1000 Subject: [PATCH] FIX: Handle edge cases for group SMTP email job (#13631) Skip group SMTP email (and add log) if: * topic is deleted * post is deleted * smtp has been disabled for the group Skip without log if: * enable_smtp site setting is false * disable_emails site setting is yes Co-authored-by: Alan Guo Xiang Tan --- app/jobs/regular/group_smtp_email.rb | 37 +++++++++- app/models/skipped_email_log.rb | 3 + app/models/user.rb | 12 ++- config/locales/server.en.yml | 3 + spec/jobs/regular/group_smtp_email_spec.rb | 86 ++++++++++++++++++++++ 5 files changed, 136 insertions(+), 5 deletions(-) diff --git a/app/jobs/regular/group_smtp_email.rb b/app/jobs/regular/group_smtp_email.rb index 8092dc38aa4..66eadb9ab6d 100644 --- a/app/jobs/regular/group_smtp_email.rb +++ b/app/jobs/regular/group_smtp_email.rb @@ -4,13 +4,32 @@ require_dependency 'email/sender' module Jobs class GroupSmtpEmail < ::Jobs::Base + include Skippable + sidekiq_options queue: 'critical' def execute(args) + return if quit_email_early? + group = Group.find_by(id: args[:group_id]) + return if group.blank? + post = Post.find_by(id: args[:post_id]) email = args[:email] cc_addresses = args[:cc_emails] + recipient_user = User.find_by_email(email, primary: true) + + if post.blank? + return skip(email, nil, recipient_user, :group_smtp_post_deleted) + end + + if !Topic.exists?(id: post.topic_id) + return skip(email, post, recipient_user, :group_smtp_topic_deleted) + end + + if !group.smtp_enabled + return skip(email, post, recipient_user, :group_smtp_disabled_for_group) + end # There is a rare race condition causing the Imap::Sync class to create # an incoming email and associated post/topic, which then kicks off @@ -27,12 +46,10 @@ module Jobs ImapSyncLog.debug("Sending SMTP email for post #{post.id} in topic #{post.topic_id} to #{email}.", group) - recipient_user = ::UserEmail.find_by(email: email, primary: true)&.user - message = GroupSmtpMailer.send_mail(group, email, post, cc_addresses) - # The EmailLog record created by the sender will have the raw email # stored, the group smtp ID, and any cc addresses recorded for later # cross referencing. + message = GroupSmtpMailer.send_mail(group, email, post, cc_addresses) Email::Sender.new(message, :group_smtp, recipient_user).send # Create an incoming email record to avoid importing again from IMAP @@ -52,5 +69,19 @@ module Jobs created_via: IncomingEmail.created_via_types[:group_smtp] ) end + + def quit_email_early? + SiteSetting.disable_emails == 'yes' || !SiteSetting.enable_smtp + end + + def skip(email, post, recipient_user, reason) + create_skipped_email_log( + email_type: :group_smtp, + to_address: email, + user_id: recipient_user&.id, + post_id: post&.id, + reason_type: SkippedEmailLog.reason_types[reason] + ) + end end end diff --git a/app/models/skipped_email_log.rb b/app/models/skipped_email_log.rb index 1a4ec415aa8..2af1b2dd12f 100644 --- a/app/models/skipped_email_log.rb +++ b/app/models/skipped_email_log.rb @@ -39,6 +39,9 @@ class SkippedEmailLog < ActiveRecord::Base user_email_access_denied: 22, sender_topic_deleted: 23, user_email_no_email: 24, + group_smtp_post_deleted: 25, + group_smtp_topic_deleted: 26, + group_smtp_disabled_for_group: 27, # you need to add the reason in server.en.yml below the "skipped_email_log" key # when you add a new enum value ) diff --git a/app/models/user.rb b/app/models/user.rb index 6dcd9dd9cdd..f9a23792506 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -193,6 +193,10 @@ class User < ActiveRecord::Base joins(:user_emails).where("lower(user_emails.email) IN (?)", email) end + scope :with_primary_email, ->(email) do + joins(:user_emails).where("lower(user_emails.email) IN (?) AND user_emails.primary", email) + end + scope :human_users, -> { where('users.id > 0') } # excluding fake users like the system user or anonymous users @@ -371,8 +375,12 @@ class User < ActiveRecord::Base end end - def self.find_by_email(email) - self.with_email(Email.downcase(email)).first + def self.find_by_email(email, primary: false) + if primary + self.with_primary_email(Email.downcase(email)).first + else + self.with_email(Email.downcase(email)).first + end end def self.find_by_username(username) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 61a2eef9705..f8d75b4fcec 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -4043,6 +4043,9 @@ en: sender_post_deleted: "post has been deleted" sender_message_to_invalid: "recipient has invalid email address" sender_topic_deleted: "topic has been deleted" + group_smtp_post_deleted: "post has been deleted" + group_smtp_topic_deleted: "topic has been deleted" + group_smtp_disabled_for_group: "smtp has been disabled for the group" color_schemes: base_theme_name: "Base" diff --git a/spec/jobs/regular/group_smtp_email_spec.rb b/spec/jobs/regular/group_smtp_email_spec.rb index 8e24ac7eeff..9b792a84584 100644 --- a/spec/jobs/regular/group_smtp_email_spec.rb +++ b/spec/jobs/regular/group_smtp_email_spec.rb @@ -85,6 +85,8 @@ RSpec.describe Jobs::GroupSmtpEmail do it "creates an IncomingEmail record with the correct details to avoid double processing IMAP" do subject.execute(args) + expect(ActionMailer::Base.deliveries.count).to eq(1) + expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support") incoming_email = IncomingEmail.find_by(post_id: post.id, topic_id: post.topic_id, user_id: post.user.id) expect(incoming_email).not_to eq(nil) expect(incoming_email.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost") @@ -96,6 +98,8 @@ RSpec.describe Jobs::GroupSmtpEmail do it "does not create a post reply key, it always replies to the group email_username" do subject.execute(args) + expect(ActionMailer::Base.deliveries.count).to eq(1) + expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support") email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id) post_reply_key = PostReplyKey.where(user_id: recipient_user, post_id: post.id).first expect(post_reply_key).to eq(nil) @@ -105,6 +109,8 @@ RSpec.describe Jobs::GroupSmtpEmail do it "creates an EmailLog record with the correct details" do subject.execute(args) + expect(ActionMailer::Base.deliveries.count).to eq(1) + expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support") email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id) expect(email_log).not_to eq(nil) expect(email_log.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost") @@ -112,6 +118,8 @@ RSpec.describe Jobs::GroupSmtpEmail do it "creates an IncomingEmail record with the correct details to avoid double processing IMAP" do subject.execute(args) + expect(ActionMailer::Base.deliveries.count).to eq(1) + expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support") incoming_email = IncomingEmail.find_by(post_id: post.id, topic_id: post.topic_id, user_id: post.user.id) expect(incoming_email).not_to eq(nil) expect(incoming_email.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost") @@ -123,6 +131,8 @@ RSpec.describe Jobs::GroupSmtpEmail do it "does not create a post reply key, it always replies to the group email_username" do subject.execute(args) + expect(ActionMailer::Base.deliveries.count).to eq(1) + expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support") email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id) post_reply_key = PostReplyKey.where(user_id: recipient_user, post_id: post.id).first expect(post_reply_key).to eq(nil) @@ -133,12 +143,16 @@ RSpec.describe Jobs::GroupSmtpEmail do it "falls back to the group name if full name is blank" do group.update(full_name: "") subject.execute(args) + expect(ActionMailer::Base.deliveries.count).to eq(1) + expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support") email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id) expect(email_log.raw_headers).to include("From: support-group via Discourse <#{group.email_username}") end it "has the group_smtp_id and the to_address filled in correctly" do subject.execute(args) + expect(ActionMailer::Base.deliveries.count).to eq(1) + expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support") email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id) expect(email_log.to_address).to eq("test@test.com") expect(email_log.smtp_group_id).to eq(group.id) @@ -147,6 +161,8 @@ RSpec.describe Jobs::GroupSmtpEmail do context "when there are cc_addresses" do it "has the cc_addresses and cc_user_ids filled in correctly" do subject.execute(args) + expect(ActionMailer::Base.deliveries.count).to eq(1) + expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support") email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id) expect(email_log.cc_addresses).to eq("otherguy@test.com;cormac@lit.com") expect(email_log.cc_user_ids).to match_array([staged1.id, staged2.id]) @@ -157,6 +173,76 @@ RSpec.describe Jobs::GroupSmtpEmail do let(:post_id) { post.topic.posts.first.id } it "aborts and does not send a group SMTP email; the OP is the one that sent the email in the first place" do expect { subject.execute(args) }.not_to(change { EmailLog.count }) + expect(ActionMailer::Base.deliveries.count).to eq(0) + end + end + + context "when the post is deleted" do + it "aborts and adds a skipped email log" do + post.trash! + subject.execute(args) + expect(ActionMailer::Base.deliveries.count).to eq(0) + expect(SkippedEmailLog.exists?( + email_type: "group_smtp", + user: recipient_user, + post: nil, + to_address: recipient_user.email, + reason_type: SkippedEmailLog.reason_types[:group_smtp_post_deleted] + )).to eq(true) + end + end + + context "when the topic is deleted" do + it "aborts and adds a skipped email log" do + post.topic.trash! + subject.execute(args) + expect(ActionMailer::Base.deliveries.count).to eq(0) + expect(SkippedEmailLog.exists?( + email_type: "group_smtp", + user: recipient_user, + post: post, + to_address: recipient_user.email, + reason_type: SkippedEmailLog.reason_types[:group_smtp_topic_deleted] + )).to eq(true) + end + end + + context "when smtp is not enabled" do + it "returns without sending email" do + SiteSetting.enable_smtp = false + subject.execute(args) + expect(ActionMailer::Base.deliveries.count).to eq(0) + end + end + + context "when disable_emails is yes" do + it "returns without sending email" do + SiteSetting.disable_emails = "yes" + subject.execute(args) + expect(ActionMailer::Base.deliveries.count).to eq(0) + end + end + + context "group is deleted" do + it "returns without sending email" do + group.destroy + subject.execute(args) + expect(ActionMailer::Base.deliveries.count).to eq(0) + end + end + + context "when smtp is not enabled for the group" do + it "returns without sending email" do + group.update!(smtp_enabled: false) + subject.execute(args) + expect(ActionMailer::Base.deliveries.count).to eq(0) + expect(SkippedEmailLog.exists?( + email_type: "group_smtp", + user: recipient_user, + post: post, + to_address: recipient_user.email, + reason_type: SkippedEmailLog.reason_types[:group_smtp_disabled_for_group] + )).to eq(true) end end end