From 64b0b50ac013c7273b2d560df073bc670648b779 Mon Sep 17 00:00:00 2001
From: Martin Brennan <mjrbrennan@gmail.com>
Date: Thu, 22 Oct 2020 10:49:08 +1000
Subject: [PATCH] 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.
---
 app/jobs/regular/group_smtp_email.rb       |  2 +-
 lib/email/sender.rb                        |  4 +-
 spec/fabricators/group_fabricator.rb       | 14 +++++++
 spec/jobs/regular/group_smtp_email_spec.rb | 44 ++++++++++++++++++++++
 4 files changed, 60 insertions(+), 4 deletions(-)
 create mode 100644 spec/jobs/regular/group_smtp_email_spec.rb

diff --git a/app/jobs/regular/group_smtp_email.rb b/app/jobs/regular/group_smtp_email.rb
index 6ff4301b3f..4035a1f070 100644
--- a/app/jobs/regular/group_smtp_email.rb
+++ b/app/jobs/regular/group_smtp_email.rb
@@ -13,7 +13,7 @@ module Jobs
 
       Rails.logger.debug("[IMAP] Sending email for group #{group.name} and post #{post.id}")
       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
       # server.
diff --git a/lib/email/sender.rb b/lib/email/sender.rb
index 0668d7ea5a..db9b55aaab 100644
--- a/lib/email/sender.rb
+++ b/lib/email/sender.rb
@@ -388,9 +388,7 @@ module Email
     end
 
     def set_reply_key(post_id, user_id)
-      return unless user_id &&
-        post_id &&
-        header_value(Email::MessageBuilder::ALLOW_REPLY_BY_EMAIL_HEADER).present?
+      return if !user_id || !post_id || !header_value(Email::MessageBuilder::ALLOW_REPLY_BY_EMAIL_HEADER).present?
 
       # use safe variant here cause we tend to see concurrency issue
       reply_key = PostReplyKey.find_or_create_by_safe!(
diff --git a/spec/fabricators/group_fabricator.rb b/spec/fabricators/group_fabricator.rb
index 8b66bdacda..9f04918300 100644
--- a/spec/fabricators/group_fabricator.rb
+++ b/spec/fabricators/group_fabricator.rb
@@ -8,3 +8,17 @@ Fabricator(:public_group, from: :group) do
   public_admission true
   public_exit true
 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
diff --git a/spec/jobs/regular/group_smtp_email_spec.rb b/spec/jobs/regular/group_smtp_email_spec.rb
new file mode 100644
index 0000000000..af01d4a2bc
--- /dev/null
+++ b/spec/jobs/regular/group_smtp_email_spec.rb
@@ -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