FIX: Link up reply to post correctly when emailing group (#13339)

When replying to a user_private_message email originating from
a group PM that does _not_ have a reply key (e.g. when replying
directly to the group's SMTP address), we were mistakenly linking
the new post created from the reply to the OP and the user who
created the topic, based on the first IncomingEmail message ID in
the topic, rather than using the correct reply to user and post number
that the user actually replied to.

We now use the In-Reply-To header to look up the corresponding EmailLog
record when the user who replied was sent a user_private_message email,
and use the post from that as the reply_to_user/post.

This also removes superfluous filtering of incoming_email records. After
already filtering by message_id and then addressed_to_user (which only
returns incoming emails where the to, from, or cc address includes any
of the user's emails), we were filtering again but in the ruby code for
the exact same conditions. After removing this all existing tests still
pass.

app/models/email_log.rb

@@ -22,6 +22,17 @@ class EmailLog < ActiveRecord::Base
 
   scope :bounced, -> { where(bounced: true) }
 
+  scope :addressed_to_user, ->(user) do
+    where(<<~SQL, user_id: user.id)
+      EXISTS(
+        SELECT 1
+        FROM user_emails
+        WHERE user_emails.user_id = :user_id AND
+        email_logs.to_address = user_emails.email
+      )
+    SQL
+  end
+
   after_create do
     # Update last_emailed_at if the user_id is present and email was sent
     User.where(id: user_id).update_all("last_emailed_at = CURRENT_TIMESTAMP") if user_id.present?

lib/email/receiver.rb

@@ -109,7 +109,7 @@ module Email
       # server (e.g. a message_id generated by Gmail) and does not need to
       # be updated, because message_ids from the IMAP server are not guaranteed
       # to be unique.
-      return unless discourse_generated_message_id?
+      return unless discourse_generated_message_id?(@message_id)
 
       incoming_email.update(
         imap_uid_validity: @opts[:imap_uid_validity],
@@ -745,23 +745,27 @@ module Email
 
     def create_group_post(group, user, body, elided)
       message_ids = Email::Receiver.extract_reply_message_ids(@mail, max_message_id_count: 5)
-      post_ids = []
 
+      # incoming emails with matching message ids, and then cross references
+      # these with any email addresses for the user vs to/from/cc of the
+      # incoming emails. in effect, any incoming email record for these
+      # message ids where the user is involved in any way will be returned
       incoming_emails = IncomingEmail.where(message_id: message_ids)
       if !group.allow_unknown_sender_topic_replies
         incoming_emails = incoming_emails.addressed_to_user(user)
       end
+      post_ids = incoming_emails.pluck(:post_id) || []
 
-      incoming_emails = incoming_emails.pluck(:post_id, :from_address, :to_addresses, :cc_addresses)
+      # if the user is directly replying to an email send to them from discourse,
-
+      # there will be a corresponding EmailLog record, so we can use that as the
-      incoming_emails.each do |post_id, from_address, to_addresses, cc_addresses|
+      # reply post if it exists
-        if group.allow_unknown_sender_topic_replies
+      if discourse_generated_message_id?(mail.in_reply_to)
-          post_ids << post_id
+        post_id_from_email_log = EmailLog.where(message_id: mail.in_reply_to)
-        else
+          .addressed_to_user(user)
-          post_ids << post_id if contains_email_address_of_user?(from_address, user) ||
+          .order(created_at: :desc)
-            contains_email_address_of_user?(to_addresses, user) ||
+          .limit(1)
-            contains_email_address_of_user?(cc_addresses, user)
+          .pluck(:post_id).last
-        end
+        post_ids << post_id_from_email_log
       end
 
       if post_ids.any? && post = Post.where(id: post_ids).order(:created_at).last
@@ -989,9 +993,9 @@ module Email
       @host ||= Email::Sender.host_for(Discourse.base_url)
     end
 
-    def discourse_generated_message_id?
+    def discourse_generated_message_id?(message_id)
-      !!(@message_id =~ message_id_post_id_regexp) ||
+      !!(message_id =~ message_id_post_id_regexp) ||
-        !!(@message_id =~ message_id_topic_id_regexp)
+        !!(message_id =~ message_id_topic_id_regexp)
     end
 
     def message_id_post_id_regexp

spec/components/email/receiver_spec.rb

@@ -985,6 +985,95 @@ describe Email::Receiver do
       end
     end
 
+    context "emailing a group by email_username and following reply flow" do
+      let!(:topic) do
+        group.update!(email_username: "team@somesmtpaddress.com")
+        process(:email_to_group_email_username_1)
+        Topic.last
+      end
+      fab!(:user_in_group) do
+        u = Fabricate(:user)
+        Fabricate(:group_user, user: u, group: group)
+        u
+      end
+
+      before do
+        NotificationEmailer.enable
+        Jobs.run_immediately!
+      end
+
+      def reply_as_group_user
+        group_post = PostCreator.create(
+          user_in_group,
+          raw: "Thanks for your request. Please try to restart.",
+          topic_id: topic.id
+        )
+        email_log = EmailLog.last
+        [email_log, group_post]
+      end
+
+      it "the inbound processed email creates an incoming email and topic record correctly, and adds the group to the topic" do
+        incoming = IncomingEmail.find_by(topic: topic)
+        user = User.find_by_email("two@foo.com")
+        expect(topic.topic_allowed_users.first.user_id).to eq(user.id)
+        expect(topic.topic_allowed_groups.first.group_id).to eq(group.id)
+        expect(incoming.to_addresses).to eq("team@somesmtpaddress.com")
+        expect(incoming.from_address).to eq("two@foo.com")
+        expect(incoming.message_id).to eq("u4w8c9r4y984yh98r3h69873@foo.bar.mail")
+      end
+
+      it "creates an EmailLog when someone from the group replies, and does not create an IncomingEmail record for the reply" do
+        email_log, group_post = reply_as_group_user
+        expect(email_log.message_id).to eq("topic/#{topic.id}/#{group_post.id}@test.localhost")
+        expect(email_log.to_address).to eq("two@foo.com")
+        expect(email_log.email_type).to eq("user_private_message")
+        expect(email_log.post_id).to eq(group_post.id)
+        expect(IncomingEmail.exists?(post_id: group_post.id)).to eq(false)
+      end
+
+      it "processes a reply from the OP user to the group SMTP username, linking the reply_to_post_number correctly by
+      matching in_reply_to to the email log" do
+        email_log, group_post = reply_as_group_user
+
+        reply_email = email(:email_to_group_email_username_2)
+        reply_email.gsub!("MESSAGE_ID_REPLY_TO", email_log.message_id)
+        expect do
+          Email::Receiver.new(reply_email).process!
+        end.to change { Topic.count }.by(0).and change { Post.count }.by(1)
+
+        reply_post = Post.last
+        expect(reply_post.reply_to_user).to eq(user_in_group)
+        expect(reply_post.reply_to_post_number).to eq(group_post.post_number)
+      end
+
+      it "processes the reply from the user as a brand new topic if they have replied from a different address (e.g. auto forward) and allow_unknown_sender_topic_replies is disabled" do
+        email_log, group_post = reply_as_group_user
+
+        reply_email = email(:email_to_group_email_username_2_as_unknown_sender)
+        reply_email.gsub!("MESSAGE_ID_REPLY_TO", email_log.message_id)
+        expect do
+          Email::Receiver.new(reply_email).process!
+        end.to change { Topic.count }.by(1).and change { Post.count }.by(1)
+
+        reply_post = Post.last
+        expect(reply_post.topic_id).not_to eq(topic.id)
+      end
+
+      it "processes the reply from the user as a reply if they have replied from a different address (e.g. auto forward) and allow_unknown_sender_topic_replies is enabled" do
+        group.update!(allow_unknown_sender_topic_replies: true)
+        email_log, group_post = reply_as_group_user
+
+        reply_email = email(:email_to_group_email_username_2_as_unknown_sender)
+        reply_email.gsub!("MESSAGE_ID_REPLY_TO", email_log.message_id)
+        expect do
+          Email::Receiver.new(reply_email).process!
+        end.to change { Topic.count }.by(0).and change { Post.count }.by(1)
+
+        reply_post = Post.last
+        expect(reply_post.topic_id).to eq(topic.id)
+      end
+    end
+
     context "when message sent to a group has no key and find_related_post_with_key is enabled" do
       let!(:topic) do
         process(:email_reply_1)

spec/fixtures/emails/email_to_group_email_username_1.eml

@@ -0,0 +1,11 @@
+Return-Path: <two@foo.com>
+From: Two <two@foo.com>
+To: team@somesmtpaddress.com
+Subject: Full email group username flow
+Date: Fri, 15 Jan 2016 00:12:43 +0100
+Message-ID: <u4w8c9r4y984yh98r3h69873@foo.bar.mail>
+Mime-Version: 1.0
+Content-Type: text/plain
+Content-Transfer-Encoding: 7bit
+
+This is the first email.

spec/fixtures/emails/email_to_group_email_username_2.eml

@@ -0,0 +1,14 @@
+Return-Path: <two@foo.com>
+From: Two <two@foo.com>
+To: team@somesmtpaddress.com
+Subject: Full email group username flow
+Date: Saturday, 16 Jan 2016 00:12:43 +0100
+Message-ID: <348ct38794nyt9338dsfsd@foo.bar.mail>
+In-Reply-To: <MESSAGE_ID_REPLY_TO>
+References: <u4w8c9r4y984yh98r3h69873@foo.bar.mail>
+  <MESSAGE_ID_REPLY_TO>
+Mime-Version: 1.0
+Content-Type: text/plain
+Content-Transfer-Encoding: 7bit
+
+Hey thanks for the suggestion, it didn't work.

spec/fixtures/emails/email_to_group_email_username_2_as_unknown_sender.eml

@@ -0,0 +1,14 @@
+Return-Path: <unknownsender@bar.com>
+From: Mysterio <unknownsender@bar.com>
+To: team@somesmtpaddress.com
+Subject: Full email group username flow
+Date: Saturday, 16 Jan 2016 00:12:43 +0100
+Message-ID: <348ct38794nyt9338dsfsd@foo.bar.mail>
+In-Reply-To: <MESSAGE_ID_REPLY_TO>
+References: <u4w8c9r4y984yh98r3h69873@foo.bar.mail>
+  <MESSAGE_ID_REPLY_TO>
+Mime-Version: 1.0
+Content-Type: text/plain
+Content-Transfer-Encoding: 7bit
+
+No I don't think that did it.