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

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.

diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index cf7534a..dd99be5 100644
--- a/config/locales/server.en.yml
+++ b/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."
diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb
index da699c0..a6da67c 100644
--- a/lib/email/receiver.rb
+++ b/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
diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb
index 11a87b4..79e1444 100644
--- a/spec/components/email/receiver_spec.rb
+++ b/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

[... diff too long, it was truncated ...]

GitHub sha: 22b96c9ce14a6387056f3f9bbeb564537448be1c

1 Like

This commit appears in #13382 which was approved by SamSaffron. It was merged by martin.