From 3b13f1146b2a406238c50d6b45bc9aa721094f46 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 6 Dec 2021 10:34:39 +1000 Subject: [PATCH] FIX: Add random suffix to outbound Message-ID for email (#15179) Currently the Message-IDs we send out for outbound email are not unique; for a post they look like: topic/TOPIC_ID/POST_ID@HOST And for a topic they look like: topic/TOPIC_ID@HOST This commit changes the outbound Message-IDs to also have a random suffix before the host, so the new format is like this: topic/TOPIC_ID/POST_ID.RANDOM_SUFFIX@HOST Or: topic/TOPIC_ID.RANDOM_SUFFIX@HOST This should help with email deliverability. This change is backwards-compatible, the old Message-ID format will still be recognized in the mail receiver flow, so people will still be able to reply using Message-IDs, In-Reply-To, and References headers that have already been sent. This commit also refactors Message-ID related logic to a central location, and adds judicious amounts of tests and documentation. --- app/controllers/webhooks_controller.rb | 4 +- lib/email.rb | 13 --- lib/email/message_id_service.rb | 103 ++++++++++++++++++ lib/email/receiver.rb | 36 +------ lib/email/sender.rb | 16 +-- lib/imap/providers/generic.rb | 6 +- lib/imap/sync.rb | 2 +- spec/components/email/email_spec.rb | 26 ----- spec/components/email/receiver_spec.rb | 49 ++++++++- spec/components/email/sender_spec.rb | 24 +++-- spec/jobs/regular/group_smtp_email_spec.rb | 12 ++- spec/lib/message_id_service_spec.rb | 117 +++++++++++++++++++++ 12 files changed, 304 insertions(+), 104 deletions(-) create mode 100644 lib/email/message_id_service.rb create mode 100644 spec/lib/message_id_service_spec.rb diff --git a/app/controllers/webhooks_controller.rb b/app/controllers/webhooks_controller.rb index 93995df0984..befa6ea22e5 100644 --- a/app/controllers/webhooks_controller.rb +++ b/app/controllers/webhooks_controller.rb @@ -13,7 +13,7 @@ class WebhooksController < ActionController::Base def sendgrid events = params["_json"] || [params] events.each do |event| - message_id = Email.message_id_clean((event["smtp-id"] || "")) + message_id = Email::MessageIdService.message_id_clean((event["smtp-id"] || "")) to_address = event["email"] if event["event"] == "bounce" if event["status"]["4."] @@ -150,7 +150,7 @@ class WebhooksController < ActionController::Base return mailgun_failure unless valid_mailgun_signature?(params["token"], params["timestamp"], params["signature"]) event = params["event"] - message_id = Email.message_id_clean(params["Message-Id"]) + message_id = Email::MessageIdService.message_id_clean(params["Message-Id"]) to_address = params["recipient"] # only handle soft bounces, because hard bounces are also handled diff --git a/lib/email.rb b/lib/email.rb index f1b8329a7b0..91c48dda43f 100644 --- a/lib/email.rb +++ b/lib/email.rb @@ -52,21 +52,8 @@ module Email SiteSetting.email_site_title.presence || SiteSetting.title end - # https://tools.ietf.org/html/rfc850#section-2.1.7 - def self.message_id_rfc_format(message_id) - message_id.present? && !is_message_id_rfc?(message_id) ? "<#{message_id}>" : message_id - end - - def self.message_id_clean(message_id) - message_id.present? && is_message_id_rfc?(message_id) ? message_id.gsub(/^<|>$/, "") : message_id - end - private - def self.is_message_id_rfc?(message_id) - message_id.start_with?('<') && message_id.include?('@') && message_id.end_with?('>') - end - def self.obfuscate_part(part) if part.size < 3 "*" * part.size diff --git a/lib/email/message_id_service.rb b/lib/email/message_id_service.rb new file mode 100644 index 00000000000..744d51990ff --- /dev/null +++ b/lib/email/message_id_service.rb @@ -0,0 +1,103 @@ +# frozen_string_literal: true + +module Email + ## + # Email Message-IDs are used in both our outbound and inbound email + # flow. For the outbound flow via Email::Sender, we assign a unique + # Message-ID for any emails sent out from the application. + # If we are sending an email related to a topic, such as through the + # PostAlerter class, then the Message-ID will contain references to + # the topic ID, and if it is for a specific post, the post ID, + # along with a random suffix to make the Message-ID truly unique. + # The host must also be included on the Message-IDs. + # + # For the inbound email flow via Email::Receiver, we use Message-IDs + # to discern which topic or post the inbound email reply should be + # in response to. In this case, the Message-ID is extracted from the + # References and/or In-Reply-To headers, and compared with either + # the IncomingEmail table, the Post table, or the IncomingEmail to + # determine where to send the reply. + # + # See https://datatracker.ietf.org/doc/html/rfc2822#section-3.6.4 for + # more specific information around Message-IDs in email. + # + # See https://tools.ietf.org/html/rfc850#section-2.1.7 for the + # Message-ID format specification. + class MessageIdService + class << self + def generate_default + "<#{SecureRandom.uuid}@#{host}>" + end + + def generate_for_post(post, use_incoming_email_if_present: false) + if use_incoming_email_if_present && post.incoming_email&.message_id.present? + return "<#{post.incoming_email.message_id}>" + end + + "" + end + + def generate_for_topic(topic, use_incoming_email_if_present: false) + first_post = topic.ordered_posts.first + + if use_incoming_email_if_present && first_post.incoming_email&.message_id.present? + return "<#{first_post.incoming_email.message_id}>" + end + + "" + end + + def find_post_from_message_ids(message_ids) + message_ids = message_ids.map { |message_id| message_id_clean(message_id) } + post_ids = message_ids.map { |message_id| message_id[message_id_post_id_regexp, 1] }.compact.map(&:to_i) + post_ids << Post.where( + topic_id: message_ids.map { |message_id| message_id[message_id_topic_id_regexp, 1] }.compact, + post_number: 1 + ).pluck(:id) + post_ids << EmailLog.where(message_id: message_ids).pluck(:post_id) + post_ids << IncomingEmail.where(message_id: message_ids).pluck(:post_id) + + post_ids.flatten! + post_ids.compact! + post_ids.uniq! + + return if post_ids.empty? + + Post.where(id: post_ids).order(:created_at).last + end + + def random_suffix + SecureRandom.hex(12) + end + + def discourse_generated_message_id?(message_id) + !!(message_id =~ message_id_post_id_regexp) || + !!(message_id =~ message_id_topic_id_regexp) + end + + def message_id_post_id_regexp + @message_id_post_id_regexp ||= Regexp.new "topic/\\d+/(\\d+|\\d+\.\\w+)@#{Regexp.escape(host)}" + end + + def message_id_topic_id_regexp + @message_id_topic_id_regexp ||= Regexp.new "topic/(\\d+|\\d+\.\\w+)@#{Regexp.escape(host)}" + end + + def message_id_rfc_format(message_id) + message_id.present? && !is_message_id_rfc?(message_id) ? "<#{message_id}>" : message_id + end + + def message_id_clean(message_id) + message_id.present? && is_message_id_rfc?(message_id) ? message_id.gsub(/^<|>$/, "") : message_id + end + + def is_message_id_rfc?(message_id) + message_id.start_with?('<') && message_id.include?('@') && message_id.end_with?('>') + end + + def host + @host ||= Email::Sender.host_for(Discourse.base_url) + end + end + end +end diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index 7c76c44d613..2f8c4031915 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -107,7 +107,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?(@message_id) + return unless Email::MessageIdService.discourse_generated_message_id?(@message_id) incoming_email.update( imap_uid_validity: @opts[:imap_uid_validity], @@ -801,7 +801,7 @@ module Email # 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 # reply post if it exists - if discourse_generated_message_id?(mail.in_reply_to) + if Email::MessageIdService.discourse_generated_message_id?(mail.in_reply_to) post_id_from_email_log = EmailLog.where(message_id: mail.in_reply_to) .addressed_to_user(user) .order(created_at: :desc) @@ -1056,35 +1056,7 @@ module Email message_ids = Email::Receiver.extract_reply_message_ids(@mail, max_message_id_count: 5) return if message_ids.empty? - post_ids = message_ids.map { |message_id| message_id[message_id_post_id_regexp, 1] }.compact.map(&:to_i) - post_ids << Post.where(topic_id: message_ids.map { |message_id| message_id[message_id_topic_id_regexp, 1] }.compact, post_number: 1).pluck(:id) - post_ids << EmailLog.where(message_id: message_ids).pluck(:post_id) - post_ids << IncomingEmail.where(message_id: message_ids).pluck(:post_id) - - post_ids.flatten! - post_ids.compact! - post_ids.uniq! - - return if post_ids.empty? - - Post.where(id: post_ids).order(:created_at).last - end - - def host - @host ||= Email::Sender.host_for(Discourse.base_url) - end - - def discourse_generated_message_id?(message_id) - !!(message_id =~ message_id_post_id_regexp) || - !!(message_id =~ message_id_topic_id_regexp) - end - - def message_id_post_id_regexp - @message_id_post_id_regexp ||= Regexp.new "topic/\\d+/(\\d+)@#{Regexp.escape(host)}" - end - - def message_id_topic_id_regexp - @message_id_topic_id_regexp ||= Regexp.new "topic/(\\d+)@#{Regexp.escape(host)}" + Email::MessageIdService.find_post_from_message_ids(message_ids) end def self.extract_reply_message_ids(mail, max_message_id_count:) @@ -1100,7 +1072,7 @@ module Email references elsif references.present? references.split(/[\s,]/).map do |r| - Email.message_id_clean(r) + Email::MessageIdService.message_id_clean(r) end end end diff --git a/lib/email/sender.rb b/lib/email/sender.rb index cf2c98b2c08..14a538b4c6d 100644 --- a/lib/email/sender.rb +++ b/lib/email/sender.rb @@ -109,7 +109,7 @@ module Email ).pluck_first(:id) # always set a default Message ID from the host - @message.header['Message-ID'] = "<#{SecureRandom.uuid}@#{host}>" + @message.header['Message-ID'] = Email::MessageIdService.generate_default if topic_id.present? && post_id.present? post = Post.find_by(id: post_id, topic_id: topic_id) @@ -121,15 +121,9 @@ module Email return skip(SkippedEmailLog.reason_types[:sender_topic_deleted]) if topic.blank? add_attachments(post) - first_post = topic.ordered_posts.first - topic_message_id = first_post.incoming_email&.message_id.present? ? - "<#{first_post.incoming_email.message_id}>" : - "" - - post_message_id = post.incoming_email&.message_id.present? ? - "<#{post.incoming_email.message_id}>" : - "" + topic_message_id = Email::MessageIdService.generate_for_topic(topic, use_incoming_email_if_present: true) + post_message_id = Email::MessageIdService.generate_for_post(post, use_incoming_email_if_present: true) referenced_posts = Post.includes(:incoming_email) .joins("INNER JOIN post_replies ON post_replies.post_id = posts.id ") @@ -141,9 +135,9 @@ module Email "<#{referenced_post.incoming_email.message_id}>" else if referenced_post.post_number == 1 - "" + Email::MessageIdService.generate_for_topic(topic) else - "" + Email::MessageIdService.generate_for_post(referenced_post) end end end diff --git a/lib/imap/providers/generic.rb b/lib/imap/providers/generic.rb index ac18ca9f467..53ec57459d0 100644 --- a/lib/imap/providers/generic.rb +++ b/lib/imap/providers/generic.rb @@ -236,7 +236,7 @@ module Imap trashed_email_uids = find_uids_by_message_ids(message_ids) if trashed_email_uids.any? trashed_emails = emails(trashed_email_uids, ["UID", "ENVELOPE"]).map do |e| - BasicMail.new(message_id: Email.message_id_clean(e['ENVELOPE'].message_id), uid: e['UID']) + BasicMail.new(message_id: Email::MessageIdService.message_id_clean(e['ENVELOPE'].message_id), uid: e['UID']) end end end @@ -253,7 +253,7 @@ module Imap spam_email_uids = find_uids_by_message_ids(message_ids) if spam_email_uids.any? spam_emails = emails(spam_email_uids, ["UID", "ENVELOPE"]).map do |e| - BasicMail.new(message_id: Email.message_id_clean(e['ENVELOPE'].message_id), uid: e['UID']) + BasicMail.new(message_id: Email::MessageIdService.message_id_clean(e['ENVELOPE'].message_id), uid: e['UID']) end end end @@ -266,7 +266,7 @@ module Imap def find_uids_by_message_ids(message_ids) header_message_id_terms = message_ids.map do |msgid| - "HEADER Message-ID '#{Email.message_id_rfc_format(msgid)}'" + "HEADER Message-ID '#{Email::MessageIdService.message_id_rfc_format(msgid)}'" end # OR clauses are written in Polish notation...so the query looks like this: diff --git a/lib/imap/sync.rb b/lib/imap/sync.rb index 040910cbc8d..f60338064c9 100644 --- a/lib/imap/sync.rb +++ b/lib/imap/sync.rb @@ -138,7 +138,7 @@ module Imap else # try finding email by message-id instead, we may be able to set the uid etc. incoming_email = IncomingEmail.where( - message_id: Email.message_id_clean(email['ENVELOPE'].message_id), + message_id: Email::MessageIdService.message_id_clean(email['ENVELOPE'].message_id), imap_uid: nil, imap_uid_validity: nil ).where("to_addresses LIKE ?", "%#{@group.email_username}%").first diff --git a/spec/components/email/email_spec.rb b/spec/components/email/email_spec.rb index b2127041e31..382c321fe15 100644 --- a/spec/components/email/email_spec.rb +++ b/spec/components/email/email_spec.rb @@ -64,30 +64,4 @@ describe Email do end end - - describe "message_id_rfc_format" do - - it "returns message ID in RFC format" do - expect(Email.message_id_rfc_format("test@test")).to eq("") - end - - it "returns input if already in RFC format" do - expect(Email.message_id_rfc_format("")).to eq("") - end - - end - - describe "message_id_clean" do - - it "returns message ID if in RFC format" do - expect(Email.message_id_clean("")).to eq("test@test") - end - - it "returns input if a clean message ID is not in RFC format" do - message_id = "<" + "@" * 50 - expect(Email.message_id_clean(message_id)).to eq(message_id) - end - - end - end diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb index a7bbd40650a..4b7480e5c52 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -947,6 +947,52 @@ describe Email::Receiver do ordered_posts[1..-1].each(&:trash!) expect { process(:email_reply_4) }.to change { topic.posts.count }.by(1) end + + describe "replying with various message-id formats" do + let!(:topic) do + process(:email_reply_1) + Topic.last + end + let!(:post) { Fabricate(:post, topic: topic) } + + def process_mail_with_message_id(message_id) + mail_string = <<~REPLY + Return-Path: + From: Two + To: one@foo.com + Subject: RE: Testing email threading + Date: Fri, 15 Jan 2016 00:12:43 +0100 + Message-ID: <44@foo.bar.mail> + In-Reply-To: <#{message_id}> + Mime-Version: 1.0 + Content-Type: text/plain + Content-Transfer-Encoding: 7bit + + This is email reply testing with Message-ID formats. + REPLY + Email::Receiver.new(mail_string).process! + end + + it "posts a reply using a message-id in the format topic/TOPIC_ID/POST_ID@HOST" do + expect { process_mail_with_message_id("topic/#{topic.id}/#{post.id}@test.localhost") }.to change { Post.count }.by(1) + expect(topic.reload.posts.last.raw).to include("This is email reply testing with Message-ID formats") + end + + it "posts a reply using a message-id in the format topic/TOPIC_ID@HOST" do + expect { process_mail_with_message_id("topic/#{topic.id}@test.localhost") }.to change { Post.count }.by(1) + expect(topic.reload.posts.last.raw).to include("This is email reply testing with Message-ID formats") + end + + it "posts a reply using a message-id in the format topic/TOPIC_ID/POST_ID.RANDOM_SUFFIX@HOST" do + expect { process_mail_with_message_id("topic/#{topic.id}/#{post.id}.rjc3yr79834y@test.localhost") }.to change { Post.count }.by(1) + expect(topic.reload.posts.last.raw).to include("This is email reply testing with Message-ID formats") + end + + it "posts a reply using a message-id in the format topic/TOPIC_ID.RANDOM_SUFFIX@HOST" do + expect { process_mail_with_message_id("topic/#{topic.id}/#{post.id}.x3487nxy877843x@test.localhost") }.to change { Post.count }.by(1) + expect(topic.reload.posts.last.raw).to include("This is email reply testing with Message-ID formats") + end + end end it "supports any kind of attachments when 'allow_all_attachments_for_group_messages' is enabled" do @@ -1161,6 +1207,7 @@ describe Email::Receiver do NotificationEmailer.enable SiteSetting.disallow_reply_by_email_after_days = 10000 Jobs.run_immediately! + Email::MessageIdService.stubs(:random_suffix).returns("blah123") end def reply_as_group_user @@ -1185,7 +1232,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/#{original_inbound_email_topic.id}/#{group_post.id}@test.localhost") + expect(email_log.message_id).to eq("topic/#{original_inbound_email_topic.id}/#{group_post.id}.blah123@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) diff --git a/spec/components/email/sender_spec.rb b/spec/components/email/sender_spec.rb index eedb7e8ef49..a2ab5f6c519 100644 --- a/spec/components/email/sender_spec.rb +++ b/spec/components/email/sender_spec.rb @@ -260,6 +260,7 @@ describe Email::Sender do end context "email threading" do + let(:random_message_id_suffix) { "5f1330cfd941f323d7f99b9e" } fab!(:topic) { Fabricate(:topic) } fab!(:post_1) { Fabricate(:post, topic: topic, post_number: 1) } @@ -271,14 +272,17 @@ describe Email::Sender do let!(:post_reply_2_4) { PostReply.create(post: post_2, reply: post_4) } let!(:post_reply_3_4) { PostReply.create(post: post_3, reply: post_4) } - before { message.header['X-Discourse-Topic-Id'] = topic.id } + before do + message.header['X-Discourse-Topic-Id'] = topic.id + Email::MessageIdService.stubs(:random_suffix).returns(random_message_id_suffix) + end it "doesn't set the 'In-Reply-To' and 'References' headers on the first post" do message.header['X-Discourse-Post-Id'] = post_1.id email_sender.send - expect(message.header['Message-Id'].to_s).to eq("") + expect(message.header['Message-Id'].to_s).to eq("") expect(message.header['In-Reply-To'].to_s).to be_blank expect(message.header['References'].to_s).to be_blank end @@ -288,8 +292,8 @@ describe Email::Sender do email_sender.send - expect(message.header['Message-Id'].to_s).to eq("") - expect(message.header['In-Reply-To'].to_s).to eq("") + expect(message.header['Message-Id'].to_s).to eq("") + expect(message.header['In-Reply-To'].to_s).to eq("") end it "sets the 'In-Reply-To' header to the newest replied post" do @@ -297,8 +301,8 @@ describe Email::Sender do email_sender.send - expect(message.header['Message-Id'].to_s).to eq("") - expect(message.header['In-Reply-To'].to_s).to eq("") + expect(message.header['Message-Id'].to_s).to eq("") + expect(message.header['In-Reply-To'].to_s).to eq("") end it "sets the 'References' header to the topic and all replied posts" do @@ -307,9 +311,9 @@ describe Email::Sender do email_sender.send references = [ - "", - "", - "", + "", + "", + "", ] expect(message.header['References'].to_s).to eq(references.join(" ")) @@ -328,7 +332,7 @@ describe Email::Sender do references = [ "<#{topic_incoming_email.message_id}>", - "", + "", "<#{post_2_incoming_email.message_id}>", ] diff --git a/spec/jobs/regular/group_smtp_email_spec.rb b/spec/jobs/regular/group_smtp_email_spec.rb index 81600bedada..88872a93943 100644 --- a/spec/jobs/regular/group_smtp_email_spec.rb +++ b/spec/jobs/regular/group_smtp_email_spec.rb @@ -23,6 +23,7 @@ RSpec.describe Jobs::GroupSmtpEmail do let(:staged1) { Fabricate(:staged, email: "otherguy@test.com") } let(:staged2) { Fabricate(:staged, email: "cormac@lit.com") } let(:normaluser) { Fabricate(:user, email: "justanormalguy@test.com", username: "normaluser") } + let(:random_message_id_suffix) { "5f1330cfd941f323d7f99b9e" } before do SiteSetting.enable_smtp = true @@ -34,6 +35,7 @@ RSpec.describe Jobs::GroupSmtpEmail do TopicAllowedUser.create(user: staged1, topic: topic) TopicAllowedUser.create(user: staged2, topic: topic) TopicAllowedUser.create(user: normaluser, topic: topic) + Email::MessageIdService.stubs(:random_suffix).returns(random_message_id_suffix) end it "sends an email using the GroupSmtpMailer and Email::Sender" do @@ -61,7 +63,7 @@ RSpec.describe Jobs::GroupSmtpEmail do PostReply.create(post: second_post, reply: post) subject.execute(args) email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id) - expect(email_log.raw_headers).to include("In-Reply-To: ") + expect(email_log.raw_headers).to include("In-Reply-To: ") expect(email_log.as_mail_message.html_part.to_s).not_to include(I18n.t("user_notifications.in_reply_to")) end @@ -82,7 +84,7 @@ RSpec.describe Jobs::GroupSmtpEmail do subject.execute(args) email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id) expect(email_log).not_to eq(nil) - expect(email_log.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost") + expect(email_log.message_id).to eq("topic/#{post.topic_id}/#{post.id}.#{random_message_id_suffix}@test.localhost") end it "creates an IncomingEmail record with the correct details to avoid double processing IMAP" do @@ -91,7 +93,7 @@ RSpec.describe Jobs::GroupSmtpEmail do expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support") incoming_email = IncomingEmail.find_by(post_id: post.id, topic_id: post.topic_id, user_id: post.user.id) expect(incoming_email).not_to eq(nil) - expect(incoming_email.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost") + expect(incoming_email.message_id).to eq("topic/#{post.topic_id}/#{post.id}.#{random_message_id_suffix}@test.localhost") expect(incoming_email.created_via).to eq(IncomingEmail.created_via_types[:group_smtp]) expect(incoming_email.to_addresses).to eq("test@test.com") expect(incoming_email.cc_addresses).to eq("otherguy@test.com;cormac@lit.com") @@ -115,7 +117,7 @@ RSpec.describe Jobs::GroupSmtpEmail do expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support") email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id) expect(email_log).not_to eq(nil) - expect(email_log.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost") + expect(email_log.message_id).to eq("topic/#{post.topic_id}/#{post.id}.#{random_message_id_suffix}@test.localhost") end it "creates an IncomingEmail record with the correct details to avoid double processing IMAP" do @@ -124,7 +126,7 @@ RSpec.describe Jobs::GroupSmtpEmail do expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support") incoming_email = IncomingEmail.find_by(post_id: post.id, topic_id: post.topic_id, user_id: post.user.id) expect(incoming_email).not_to eq(nil) - expect(incoming_email.message_id).to eq("topic/#{post.topic_id}/#{post.id}@test.localhost") + expect(incoming_email.message_id).to eq("topic/#{post.topic_id}/#{post.id}.#{random_message_id_suffix}@test.localhost") expect(incoming_email.created_via).to eq(IncomingEmail.created_via_types[:group_smtp]) expect(incoming_email.to_addresses).to eq("test@test.com") expect(incoming_email.cc_addresses).to eq("otherguy@test.com;cormac@lit.com") diff --git a/spec/lib/message_id_service_spec.rb b/spec/lib/message_id_service_spec.rb new file mode 100644 index 00000000000..151b55bff63 --- /dev/null +++ b/spec/lib/message_id_service_spec.rb @@ -0,0 +1,117 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Email::MessageIdService do + fab!(:topic) { Fabricate(:topic) } + fab!(:post) { Fabricate(:post, topic: topic) } + fab!(:second_post) { Fabricate(:post, topic: topic) } + + subject { described_class } + + describe "#generate_for_post" do + it "generates for the post using the message_id on the post's incoming_email" do + Fabricate(:incoming_email, message_id: "test@test.localhost", post: post) + post.reload + expect(subject.generate_for_post(post, use_incoming_email_if_present: true)).to eq("") + end + + it "generates for the post without an incoming_email record" do + expect(subject.generate_for_post(post)).to match(subject.message_id_post_id_regexp) + expect(subject.generate_for_post(post, use_incoming_email_if_present: true)).to match(subject.message_id_post_id_regexp) + end + end + + describe "#generate_for_topic" do + it "generates for the topic using the message_id on the first post's incoming_email" do + Fabricate(:incoming_email, message_id: "test@test.localhost", post: post) + post.reload + expect(subject.generate_for_topic(topic, use_incoming_email_if_present: true)).to eq("") + end + + it "generates for the topic without an incoming_email record" do + expect(subject.generate_for_topic(topic)).to match(subject.message_id_topic_id_regexp) + expect(subject.generate_for_topic(topic, use_incoming_email_if_present: true)).to match(subject.message_id_topic_id_regexp) + end + end + + describe "find_post_from_message_ids" do + let(:post_format_message_id) { "" } + let(:topic_format_message_id) { "" } + let(:default_format_message_id) { "<36ac1ddd-5083-461d-b72c-6372fb0e7f33@test.localhost>" } + let(:gmail_format_message_id) { "" } + + it "finds a post based only on a post-format message id" do + expect(subject.find_post_from_message_ids([post_format_message_id])).to eq(post) + end + + it "finds a post based only on a topic-format message id" do + expect(subject.find_post_from_message_ids([topic_format_message_id])).to eq(post) + end + + it "finds a post from the email log" do + email_log = Fabricate(:email_log, message_id: subject.message_id_clean(default_format_message_id)) + expect(subject.find_post_from_message_ids([default_format_message_id])).to eq(email_log.post) + end + + it "finds a post from the incoming email log" do + incoming_email = Fabricate( + :incoming_email, + message_id: subject.message_id_clean(gmail_format_message_id), + post: Fabricate(:post) + ) + expect(subject.find_post_from_message_ids([gmail_format_message_id])).to eq(incoming_email.post) + end + + it "gets the last created post if multiple are returned" do + incoming_email = Fabricate( + :incoming_email, + message_id: subject.message_id_clean(post_format_message_id), + post: Fabricate(:post, created_at: 10.days.ago) + ) + expect(subject.find_post_from_message_ids([post_format_message_id])).to eq(post) + end + end + + describe "#discourse_generated_message_id?" do + def check_format(message_id) + subject.discourse_generated_message_id?(message_id) + end + + it "works correctly for the different possible formats" do + expect(check_format("topic/1223/4525.3c4f8n9@test.localhost")).to eq(true) + expect(check_format("")).to eq(true) + expect(check_format("topic/1223.fc3j4843@test.localhost")).to eq(true) + expect(check_format("")).to eq(true) + expect(check_format("topic/1223/4525@test.localhost")).to eq(true) + expect(check_format("")).to eq(true) + expect(check_format("topic/1223@test.localhost")).to eq(true) + expect(check_format("")).to eq(true) + + expect(check_format("topic/1223@blah")).to eq(false) + expect(check_format("")).to eq(false) + expect(check_format("t/1223@test.localhost")).to eq(false) + end + end + + describe "#message_id_rfc_format" do + it "returns message ID in RFC format" do + expect(Email::MessageIdService.message_id_rfc_format("test@test")).to eq("") + end + + it "returns input if already in RFC format" do + expect(Email::MessageIdService.message_id_rfc_format("")).to eq("") + end + end + + describe "#message_id_clean" do + it "returns message ID if in RFC format" do + expect(Email::MessageIdService.message_id_clean("")).to eq("test@test") + end + + it "returns input if a clean message ID is not in RFC format" do + message_id = "<" + "@" * 50 + expect(Email::MessageIdService.message_id_clean(message_id)).to eq(message_id) + end + end +end