From 632942e697064024f9d20c03a56836e539fd038e Mon Sep 17 00:00:00 2001
From: Martin Brennan <mjrbrennan@gmail.com>
Date: Wed, 28 Oct 2020 07:01:58 +1000
Subject: [PATCH] FIX: Ensure group SMTP and message builder always uses from
 address for Reply-To when IMAP is enabled (#11037)

There is a site setting reply_by_email_enabled which when combined with reply_by_email_address creates a Reply-To header in emails in the format "test+%{reply_key}@test.com" along with a PostReplyKey record, so when replying Discourse knows where to route the reply.

However this conflicts with the IMAP implementation. Since we are sending the email for a group via SMTP and from their actual email account, we want all replys to go to that email account as well so the IMAP sync job can pick them up and put them in the correct place. So if the group has IMAP enabled and configured, then the reply-to header will be correct.

This PR also makes a further fix to 64b0b50 by using the correct recipient user for the PostReplyKey record. If the post user is used we encounter this error:

if destination.user_id != user.id && !forwarded_reply_key?(destination, user)
  raise ReplyUserNotMatchingError, "post_reply_key.user_id => #{destination.user_id.inspect}, user.id => #{user.id.inspect}"
end
This is because the user above is found from the from_address, but the destination which is the PostReplyKey is made by the post.user, which will be different people.
---
 app/jobs/regular/group_smtp_email.rb       |  3 +-
 app/mailers/group_smtp_mailer.rb           |  1 +
 app/models/group.rb                        |  5 ++
 lib/email/message_builder.rb               |  2 +-
 spec/jobs/regular/group_smtp_email_spec.rb |  5 +-
 spec/mailers/group_smtp_mailer_spec.rb     | 84 ++++++++++++++++------
 spec/models/group_spec.rb                  | 74 +++++++++++--------
 7 files changed, 120 insertions(+), 54 deletions(-)

diff --git a/app/jobs/regular/group_smtp_email.rb b/app/jobs/regular/group_smtp_email.rb
index 4035a1f0702..7f54d27ddde 100644
--- a/app/jobs/regular/group_smtp_email.rb
+++ b/app/jobs/regular/group_smtp_email.rb
@@ -10,10 +10,11 @@ module Jobs
       group = Group.find_by(id: args[:group_id])
       post = Post.find_by(id: args[:post_id])
       email = args[:email]
+      recipient_user = ::UserEmail.find_by(email: email, primary: true)&.user
 
       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, post.user).send
+      Email::Sender.new(message, :group_smtp, recipient_user).send
 
       # Create an incoming email record to avoid importing again from IMAP
       # server.
diff --git a/app/mailers/group_smtp_mailer.rb b/app/mailers/group_smtp_mailer.rb
index ac203661bbc..58fcbc4a1b8 100644
--- a/app/mailers/group_smtp_mailer.rb
+++ b/app/mailers/group_smtp_mailer.rb
@@ -48,6 +48,7 @@ class GroupSmtpMailer < ActionMailer::Base
       group_name: from_group.name,
       allow_reply_by_email: true,
       only_reply_by_email: true,
+      use_from_address_for_reply_to: from_group.imap_enabled?,
       private_reply: post.topic.private_message?,
       participants: participants(post),
       include_respond_instructions: true,
diff --git a/app/models/group.rb b/app/models/group.rb
index 610e570194c..c673afed46a 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -817,6 +817,11 @@ class Group < ActiveRecord::Base
     }
   end
 
+  def imap_enabled?
+    return false if !SiteSetting.enable_imap
+    imap_config.values.compact.length == imap_config.keys.length
+  end
+
   def email_username_regex
     user, domain = email_username.split('@')
     if user.present? && domain.present?
diff --git a/lib/email/message_builder.rb b/lib/email/message_builder.rb
index efb0abf01f3..463e9c08684 100644
--- a/lib/email/message_builder.rb
+++ b/lib/email/message_builder.rb
@@ -153,7 +153,7 @@ module Email
       # please, don't send us automatic responses...
       result['X-Auto-Response-Suppress'] = 'All'
 
-      if allow_reply_by_email?
+      if allow_reply_by_email? && !@opts[:use_from_address_for_reply_to]
         result[ALLOW_REPLY_BY_EMAIL_HEADER] = true
         result['Reply-To'] = reply_by_email_address
       else
diff --git a/spec/jobs/regular/group_smtp_email_spec.rb b/spec/jobs/regular/group_smtp_email_spec.rb
index af01d4a2bcd..606cd4e087b 100644
--- a/spec/jobs/regular/group_smtp_email_spec.rb
+++ b/spec/jobs/regular/group_smtp_email_spec.rb
@@ -5,6 +5,7 @@ require 'rails_helper'
 RSpec.describe Jobs::GroupSmtpEmail do
   let(:post) { Fabricate(:post) }
   let(:group) { Fabricate(:imap_group) }
+  let!(:recipient_user) { Fabricate(:user, email: "test@test.com") }
   let(:args) do
     {
       group_id: group.id,
@@ -23,7 +24,7 @@ RSpec.describe Jobs::GroupSmtpEmail do
   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))
+    Email::Sender.expects(:new).with(message, :group_smtp, recipient_user).returns(stub(send: nil))
     subject.execute(args)
   end
 
@@ -37,7 +38,7 @@ RSpec.describe Jobs::GroupSmtpEmail do
   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
+    post_reply_key = PostReplyKey.where(user_id: recipient_user, 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
diff --git a/spec/mailers/group_smtp_mailer_spec.rb b/spec/mailers/group_smtp_mailer_spec.rb
index 6ecf9bbb102..4be00abf5d1 100644
--- a/spec/mailers/group_smtp_mailer_spec.rb
+++ b/spec/mailers/group_smtp_mailer_spec.rb
@@ -5,18 +5,18 @@ require 'email/receiver'
 describe GroupSmtpMailer do
   let(:group) {
     Fabricate(:group,
-      name: 'Testers',
-      title: 'Tester',
-      full_name: 'Testers Group',
-      smtp_server: 'smtp.gmail.com',
-      smtp_port: 587,
-      smtp_ssl: true,
-      imap_server: 'imap.gmail.com',
-      imap_port: 993,
-      imap_ssl: true,
-      email_username: 'bugs@gmail.com',
-      email_password: 'super$secret$password'
-    )
+              name: 'Testers',
+              title: 'Tester',
+              full_name: 'Testers Group',
+              smtp_server: 'smtp.gmail.com',
+              smtp_port: 587,
+              smtp_ssl: true,
+              imap_server: 'imap.gmail.com',
+              imap_port: 993,
+              imap_ssl: true,
+              email_username: 'bugs@gmail.com',
+              email_password: 'super$secret$password'
+             )
   }
 
   let(:user) {
@@ -44,33 +44,75 @@ describe GroupSmtpMailer do
 
   let(:receiver) {
     receiver = Email::Receiver.new(email,
-      destinations: [group],
-      uid_validity: 1,
-      uid: 10000
-    )
-    receiver.process!
-    receiver
+                                   destinations: [group],
+                                   uid_validity: 1,
+                                   uid: 10000
+                                  )
+  receiver.process!
+  receiver
   }
 
   let(:raw) { 'hello, how are you doing?' }
 
   before do
     SiteSetting.enable_smtp = true
+    SiteSetting.enable_imap = true
     Jobs.run_immediately!
   end
 
   it 'sends an email as reply' do
     post = PostCreator.create(user,
-      topic_id: receiver.incoming_email.topic.id,
-      raw: raw
-    )
+                              topic_id: receiver.incoming_email.topic.id,
+                              raw: raw
+                             )
 
     expect(ActionMailer::Base.deliveries.size).to eq(1)
 
     sent_mail = ActionMailer::Base.deliveries[0]
     expect(sent_mail.to).to contain_exactly('john@doe.com')
+    expect(sent_mail.reply_to).to contain_exactly('bugs@gmail.com')
     expect(sent_mail.subject).to eq('Re: Hello from John')
     expect(sent_mail.to_s).to include(raw)
   end
 
+  context "when the site has a reply by email address configured" do
+    before do
+      SiteSetting.manual_polling_enabled = true
+      SiteSetting.reply_by_email_address = "test+%{reply_key}@test.com"
+      SiteSetting.reply_by_email_enabled = true
+    end
+
+    it 'uses the correct IMAP/SMTP reply to address' do
+      post = PostCreator.create(user,
+                                topic_id: receiver.incoming_email.topic.id,
+                                raw: raw
+                               )
+
+      expect(ActionMailer::Base.deliveries.size).to eq(1)
+
+      sent_mail = ActionMailer::Base.deliveries[0]
+      expect(sent_mail.reply_to).to contain_exactly('bugs@gmail.com')
+    end
+
+    context "when IMAP is disabled for the group" do
+      before do
+        group.update(
+          imap_server: nil
+        )
+      end
+
+      it "uses the reply key based reply to address" do
+        post = PostCreator.create(user,
+                                  topic_id: receiver.incoming_email.topic.id,
+                                  raw: raw
+                                 )
+
+        expect(ActionMailer::Base.deliveries.size).to eq(1)
+
+        sent_mail = ActionMailer::Base.deliveries[0]
+        post_reply_key = PostReplyKey.last
+        expect(sent_mail.reply_to).to contain_exactly("test+#{post_reply_key.reply_key}@test.com")
+      end
+    end
+  end
 end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 0e501d5360a..9dce1fa07f5 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -973,7 +973,7 @@ describe Group do
     end
   end
 
-  describe "#imap_mailboxes" do
+  describe "IMAP" do
     let(:group) { Fabricate(:group) }
 
     def mock_imap
@@ -1010,39 +1010,55 @@ describe Group do
       Discourse.redis.del("group_imap_mailboxes_#{group.id}")
     end
 
-    it "returns an empty array if group imap is not configured" do
-      expect(group.imap_mailboxes).to eq([])
+    describe "#imap_enabled?" do
+      it "returns true if imap is configured and enabled for the site" do
+        mock_imap
+        configure_imap
+        enable_imap
+        expect(group.imap_enabled?).to eq(true)
+      end
+
+      it "returns false if imap is configured and not enabled for the site" do
+        configure_imap
+        expect(group.imap_enabled?).to eq(false)
+      end
     end
 
-    it "returns an empty array and does not contact IMAP server if group imap is configured but the setting is disabled" do
-      configure_imap
-      Imap::Providers::Detector.expects(:init_with_detected_provider).never
-      expect(group.imap_mailboxes).to eq([])
-    end
+    describe "#imap_mailboxes" do
+      it "returns an empty array if group imap is not configured" do
+        expect(group.imap_mailboxes).to eq([])
+      end
 
-    it "logs the imap error if one occurs" do
-      configure_imap
-      mock_imap
-      SiteSetting.enable_imap = true
-      @mocked_imap_provider.stubs(:connect!).raises(Net::IMAP::NoResponseError)
-      group.imap_mailboxes
-      expect(group.reload.imap_last_error).not_to eq(nil)
-    end
+      it "returns an empty array and does not contact IMAP server if group imap is configured but the setting is disabled" do
+        configure_imap
+        Imap::Providers::Detector.expects(:init_with_detected_provider).never
+        expect(group.imap_mailboxes).to eq([])
+      end
 
-    it "returns a list of mailboxes from the IMAP provider" do
-      configure_imap
-      mock_imap
-      enable_imap
-      expect(group.imap_mailboxes).to eq(["Inbox"])
-    end
+      it "logs the imap error if one occurs" do
+        configure_imap
+        mock_imap
+        SiteSetting.enable_imap = true
+        @mocked_imap_provider.stubs(:connect!).raises(Net::IMAP::NoResponseError)
+        group.imap_mailboxes
+        expect(group.reload.imap_last_error).not_to eq(nil)
+      end
 
-    it "caches the login and mailbox fetch" do
-      configure_imap
-      mock_imap
-      enable_imap
-      group.imap_mailboxes
-      Imap::Providers::Detector.expects(:init_with_detected_provider).never
-      group.imap_mailboxes
+      it "returns a list of mailboxes from the IMAP provider" do
+        configure_imap
+        mock_imap
+        enable_imap
+        expect(group.imap_mailboxes).to eq(["Inbox"])
+      end
+
+      it "caches the login and mailbox fetch" do
+        configure_imap
+        mock_imap
+        enable_imap
+        group.imap_mailboxes
+        Imap::Providers::Detector.expects(:init_with_detected_provider).never
+        group.imap_mailboxes
+      end
     end
   end