FIX: Prevent resurrecting old topics via email reply for group inboxes with SMTP enabled (#13382)

We already reject email replies to public topics via `SiteSetting.disallow_reply_by_email_after_days` and raising the `OldDestinationError`. This PR introduces similar behaviour for group inboxes, but without the rejection, and **only when SMTP is enabled for the group**.

If a reply is sent via email and the post is older than `SiteSetting.disallow_reply_by_email_after_days` days ago, then we create a new topic instead of making a reply in the old one and link back to the original topic. This is done to prevent long running group inbox discussions.

config/locales/server.en.yml

@@ -111,7 +111,10 @@ en:
       maximum_staged_user_per_email_reached: "Reached maximum number of staged users created per email."
       no_subject: "(no subject)"
       no_body: "(no body)"
-      missing_attachment: "(Attachment %{filename} is missing)"
+      missing_attachment: "(Attachment %{filename} is missing) ago."
+      continuing_old_discussion:
+        one: "Continuing the discussion from [%{title}](%{url}), because it was created more than %{count} day ago."
+        other: "Continuing the discussion from [%{title}](%{url}), because it was created more than %{count} days ago."
       errors:
         empty_email_error: "Happens when the raw mail we received was blank."
         no_message_id_error: "Happens when the mail has no 'Message-Id' header."

lib/email/receiver.rb

@@ -224,12 +224,12 @@ module Email
         # We don't stage new users for emails to reply addresses, exit if user is nil
         raise BadDestinationAddress if user.blank?
 
+        # We only get here if there are no destinations (the email is not going to
+        # a Category, Group, or PostReplyKey)
         post = find_related_post(force: true)
 
         if post && Guardian.new(user).can_see_post?(post)
-          num_of_days = SiteSetting.disallow_reply_by_email_after_days
-
-          if num_of_days > 0 && post.created_at < num_of_days.days.ago
+          if destination_too_old?(post)
             raise OldDestinationError.new("#{Discourse.base_url}/p/#{post.id}")
           end
         end
@@ -765,37 +765,47 @@ module Email
           .order(created_at: :desc)
           .limit(1)
           .pluck(:post_id).last
-        post_ids << post_id_from_email_log
+        post_ids << post_id_from_email_log if post_id_from_email_log
       end
 
-      if post_ids.any? && post = Post.where(id: post_ids).order(:created_at).last
-
-        # this must be done for the unknown user (who is staged) to
-        # be allowed to post a reply in the topic
-        if group.allow_unknown_sender_topic_replies
-          post.topic.topic_allowed_users.find_or_create_by!(user_id: user.id)
-        end
-
-        create_reply(user: user,
-                     raw: body,
-                     elided: elided,
-                     post: post,
-                     topic: post.topic,
-                     skip_validations: true)
-      else
-        enable_email_pm_setting(user)
+      target_post = post_ids.any? && Post.where(id: post_ids).order(:created_at).last
+      too_old_for_group_smtp = (destination_too_old?(target_post) && group.smtp_enabled)
 
+      if target_post.blank? || too_old_for_group_smtp
         create_topic(user: user,
-                     raw: body,
+                     raw: new_group_topic_body(body, target_post, too_old_for_group_smtp),
                      elided: elided,
                      title: subject,
                      archetype: Archetype.private_message,
                      target_group_names: [group.name],
                      is_group_message: true,
                      skip_validations: true)
+      else
+        # This must be done for the unknown user (who is staged) to
+        # be allowed to post a reply in the topic.
+        if group.allow_unknown_sender_topic_replies
+          target_post.topic.topic_allowed_users.find_or_create_by!(user_id: user.id)
+        end
+
+        create_reply(user: user,
+                     raw: body,
+                     elided: elided,
+                     post: target_post,
+                     topic: target_post.topic,
+                     skip_validations: true)
       end
     end
 
+    def new_group_topic_body(body, target_post, too_old_for_group_smtp)
+      return body if !too_old_for_group_smtp
+      body + "\n\n----\n\n" + I18n.t(
+        "emails.incoming.continuing_old_discussion",
+        url: target_post.topic.url,
+        title: target_post.topic.title,
+        count: SiteSetting.disallow_reply_by_email_after_days
+      )
+    end
+
     def forwarded_reply_key?(post_reply_key, user)
       incoming_emails = IncomingEmail
         .joins(:post)
@@ -1042,6 +1052,10 @@ module Email
     end
 
     def create_topic(options = {})
+      if options[:archetype] == Archetype.private_message
+        enable_email_pm_setting(options[:user])
+      end
+
       create_post_with_attachments(options)
     end
 
@@ -1327,5 +1341,11 @@ module Email
         user.user_option.update!(email_messages_level: UserOption.email_level_types[:always])
       end
     end
+
+    def destination_too_old?(post)
+      return false if post.blank?
+      num_of_days = SiteSetting.disallow_reply_by_email_after_days
+      num_of_days > 0 && post.created_at < num_of_days.days.ago
+    end
   end
 end

spec/components/email/receiver_spec.rb

@@ -986,8 +986,14 @@ describe Email::Receiver do
     end
 
     context "emailing a group by email_username and following reply flow" do
-      let!(:topic) do
-        group.update!(email_username: "team@somesmtpaddress.com")
+      let!(:original_inbound_email_topic) do
+        group.update!(
+          email_username: "team@somesmtpaddress.com",
+          smtp_server: "smtp.test.com",
+          smtp_port: 587,
+          smtp_ssl: true,
+          smtp_enabled: true
+        )
         process(:email_to_group_email_username_1)
         Topic.last
       end
@@ -999,6 +1005,7 @@ describe Email::Receiver do
 
       before do
         NotificationEmailer.enable
+        SiteSetting.disallow_reply_by_email_after_days = 10000
         Jobs.run_immediately!
       end
 
@@ -1006,17 +1013,17 @@ describe Email::Receiver do
         group_post = PostCreator.create(
           user_in_group,
           raw: "Thanks for your request. Please try to restart.",
-          topic_id: topic.id
+          topic_id: original_inbound_email_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)
+        incoming = IncomingEmail.find_by(topic: original_inbound_email_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(original_inbound_email_topic.topic_allowed_users.first.user_id).to eq(user.id)
+        expect(original_inbound_email_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")
@@ -1024,7 +1031,7 @@ describe Email::Receiver do
 
       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.message_id).to eq("topic/#{original_inbound_email_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)
@@ -1056,7 +1063,7 @@ describe Email::Receiver do
         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)
+        expect(reply_post.topic_id).not_to eq(original_inbound_email_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
@@ -1070,7 +1077,32 @@ describe Email::Receiver do
         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)
+        expect(reply_post.topic_id).to eq(original_inbound_email_topic.id)
+      end
+
+      it "creates a new topic with a reference back to the original if replying to a too old topic" do
+        SiteSetting.disallow_reply_by_email_after_days = 2
+        email_log, group_post = reply_as_group_user
+
+        group_post.update(created_at: 10.days.ago)
+        group_post.topic.update(created_at: 10.days.ago)
+
+        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(1).and change { Post.count }.by(1)
+
+        reply_post = Post.last
+        new_topic = Topic.last
+
+        expect(reply_post.topic).to eq(new_topic)
+        expect(reply_post.raw).to include(
+          I18n.t(
+            "emails.incoming.continuing_old_discussion",
+            url: group_post.topic.url, title: group_post.topic.title, count: SiteSetting.disallow_reply_by_email_after_days
+          )
+        )
       end
     end