FIX: Pass user to Email::Sender to avoid broken reply key for group_smtp email (#10978)

Our Email::Sender class accepts an optional user argument, which is used to create a PostReplyKey record when present. This record is used to sub out the %{reply_key} placeholder in the Reply-To mail header, so if we do not pass in the user we get a broken Reply-To header.

This is especially problematic in the IMAP group SMTP situation, because these emails go to customers that we are replying to, and when they reply to us the email bounces! This fixes the issue by passing user to the Email::Sender when sending a group_smtp email but there is still more to do in another PR.

This Email::Sender optional user is a bit of a footgun IMO, especially because most of the time we use it there is a user we can source. I would like to do another PR for this after this one to make the parameter not optional, so we don't end up with these reply issues down the line again.
This commit is contained in:
Martin Brennan 2020-10-22 10:49:08 +10:00 committed by GitHub
parent abb00c3780
commit 64b0b50ac0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 60 additions and 4 deletions

View File

@ -13,7 +13,7 @@ module Jobs
Rails.logger.debug("[IMAP] Sending email for group #{group.name} and post #{post.id}") Rails.logger.debug("[IMAP] Sending email for group #{group.name} and post #{post.id}")
message = GroupSmtpMailer.send_mail(group, email, post) message = GroupSmtpMailer.send_mail(group, email, post)
Email::Sender.new(message, :group_smtp).send Email::Sender.new(message, :group_smtp, post.user).send
# Create an incoming email record to avoid importing again from IMAP # Create an incoming email record to avoid importing again from IMAP
# server. # server.

View File

@ -388,9 +388,7 @@ module Email
end end
def set_reply_key(post_id, user_id) def set_reply_key(post_id, user_id)
return unless user_id && return if !user_id || !post_id || !header_value(Email::MessageBuilder::ALLOW_REPLY_BY_EMAIL_HEADER).present?
post_id &&
header_value(Email::MessageBuilder::ALLOW_REPLY_BY_EMAIL_HEADER).present?
# use safe variant here cause we tend to see concurrency issue # use safe variant here cause we tend to see concurrency issue
reply_key = PostReplyKey.find_or_create_by_safe!( reply_key = PostReplyKey.find_or_create_by_safe!(

View File

@ -8,3 +8,17 @@ Fabricator(:public_group, from: :group) do
public_admission true public_admission true
public_exit true public_exit true
end end
Fabricator(:imap_group, from: :group) do
smtp_server "smtp.ponyexpress.com"
smtp_port 587
smtp_ssl true
imap_server "imap.ponyexpress.com"
imap_port 993
imap_ssl true
imap_mailbox_name "All Mail"
imap_uid_validity 0
imap_last_uid 0
email_username "discourseteam@ponyexpress.com"
email_password "test"
end

View File

@ -0,0 +1,44 @@
# frozen_string_literal: true
require 'rails_helper'
RSpec.describe Jobs::GroupSmtpEmail do
let(:post) { Fabricate(:post) }
let(:group) { Fabricate(:imap_group) }
let(:args) do
{
group_id: group.id,
post_id: post.id,
email: "test@test.com"
}
end
before do
SiteSetting.reply_by_email_address = "test+%{reply_key}@incoming.com"
SiteSetting.manual_polling_enabled = true
SiteSetting.reply_by_email_enabled = true
SiteSetting.enable_smtp = true
end
it "sends an email using the GroupSmtpMailer and Email::Sender" do
message = Mail::Message.new(body: "hello", to: "myemail@example.invalid")
GroupSmtpMailer.expects(:send_mail).with(group, "test@test.com", post).returns(message)
Email::Sender.expects(:new).with(message, :group_smtp, post.user).returns(stub(send: nil))
subject.execute(args)
end
it "creates an IncomingEmail record to avoid double processing via IMAP" do
subject.execute(args)
incoming = IncomingEmail.find_by(post_id: post.id, user_id: post.user_id, topic_id: post.topic_id)
expect(incoming).not_to eq(nil)
expect(incoming.message_id).to eq("topic/#{post.topic_id}@test.localhost")
end
it "creates a PostReplyKey and correctly uses it for the email reply_key substitution" do
subject.execute(args)
incoming = IncomingEmail.find_by(post_id: post.id, user_id: post.user_id, topic_id: post.topic_id)
post_reply_key = PostReplyKey.where(user_id: post.user_id, post_id: post.id).first
expect(post_reply_key).not_to eq(nil)
expect(incoming.raw).to include("Reply-To: Discourse <test+#{post_reply_key.reply_key}@incoming.com>")
end
end