From f2d00e5eff06ccd5fd3455d04eff86b8ba901f68 Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Fri, 30 Mar 2018 14:37:19 +0200 Subject: [PATCH] FEATURE: Use Message-ID for detecting email replies to group Ignores the site setting "find_related_post_with_key" and always tries to honor the `In-Reply-To` and `References` header for emails sent to a group. The senders email address must be included in the `To` or `CC` header of a previous email sent to the group and the `Message-ID` of that email must be included in the current email's `In-Reply-To` or `References` header. --- app/models/incoming_email.rb | 19 ++++- lib/email/receiver.rb | 80 +++++++++++++------ script/import_scripts/mbox/support/indexer.rb | 6 +- spec/components/email/receiver_spec.rb | 25 +++++- 4 files changed, 98 insertions(+), 32 deletions(-) diff --git a/app/models/incoming_email.rb b/app/models/incoming_email.rb index be5bfdce60a..b355235d267 100644 --- a/app/models/incoming_email.rb +++ b/app/models/incoming_email.rb @@ -5,7 +5,24 @@ class IncomingEmail < ActiveRecord::Base scope :errored, -> { where("NOT is_bounce AND error IS NOT NULL") } - scope :addressed_to, -> (email) { where('incoming_emails.to_addresses ILIKE :email OR incoming_emails.cc_addresses ILIKE :email', email: "%#{email}%") } + scope :addressed_to, -> (email) do + where(<<~SQL, email: "%#{email}%") + incoming_emails.to_addresses ILIKE :email OR + incoming_emails.cc_addresses ILIKE :email + SQL + end + + 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 + (incoming_emails.to_addresses ILIKE '%' || user_emails.email || '%' OR + incoming_emails.cc_addresses ILIKE '%' || user_emails.email || '%') + ) + SQL + end end # == Schema Information diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index 68ba99198fc..4c093de916b 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -537,14 +537,7 @@ module Email case destination[:type] when :group group = destination[:obj] - create_topic(user: user, - raw: body, - elided: elided, - title: subject, - archetype: Archetype.private_message, - target_group_names: [group.name], - is_group_message: true, - skip_validations: true) + create_group_post(group, user, body, elided) when :category category = destination[:obj] @@ -562,7 +555,7 @@ module Email when :reply email_log = destination[:obj] - if email_log.user_id != user.id && !forwareded_reply_key?(email_log, user) + if email_log.user_id != user.id && !forwarded_reply_key?(email_log, user) raise ReplyUserNotMatchingError, "email_log.user_id => #{email_log.user_id.inspect}, user.id => #{user.id.inspect}" end @@ -575,27 +568,63 @@ module Email end end - def forwareded_reply_key?(email_log, user) + 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 = IncomingEmail + .where(message_id: message_ids) + .addressed_to_user(user) + .pluck(:post_id, :to_addresses, :cc_addresses) + + incoming_emails.each do |post_id, to_addresses, cc_addresses| + post_ids << post_id if contains_email_address_of_user?(to_addresses, user) || + contains_email_address_of_user?(cc_addresses, user) + end + + if post_ids.any? && post = Post.where(id: post_ids).order(:created_at).last + create_reply(user: user, + raw: body, + elided: elided, + post: post, + topic: post.topic, + skip_validations: true) + else + create_topic(user: user, + raw: body, + elided: elided, + title: subject, + archetype: Archetype.private_message, + target_group_names: [group.name], + is_group_message: true, + skip_validations: true) + end + end + + def forwarded_reply_key?(email_log, user) incoming_emails = IncomingEmail .joins(:post) .where('posts.topic_id = ?', email_log.topic_id) .addressed_to(email_log.reply_key) - .addressed_to(user.email) + .addressed_to_user(user) + .pluck(:to_addresses, :cc_addresses) - incoming_emails.each do |email| - next unless contains_email_address?(email.to_addresses, user.email) || - contains_email_address?(email.cc_addresses, user.email) + incoming_emails.each do |to_addresses, cc_addresses| + next unless contains_email_address_of_user?(to_addresses, user) || + contains_email_address_of_user?(cc_addresses, user) - return true if contains_reply_by_email_address(email.to_addresses, email_log.reply_key) || - contains_reply_by_email_address(email.cc_addresses, email_log.reply_key) + return true if contains_reply_by_email_address(to_addresses, email_log.reply_key) || + contains_reply_by_email_address(cc_addresses, email_log.reply_key) end false end - def contains_email_address?(addresses, email) + def contains_email_address_of_user?(addresses, user) return false if addresses.blank? - addresses.split(";").include?(email) + + addresses = addresses.split(";") + user.user_emails.any? { |user_email| addresses.include?(user_email.email) } end def contains_reply_by_email_address(addresses, reply_key) @@ -703,14 +732,9 @@ module Email def find_related_post return if SiteSetting.find_related_post_with_key && !sent_to_mailinglist_mirror? - message_ids = [@mail.in_reply_to, Email::Receiver.extract_references(@mail.references)] - message_ids.flatten! - message_ids.select!(&:present?) - message_ids.uniq! + message_ids = Email::Receiver.extract_reply_message_ids(@mail, max_message_id_count: 5) return if message_ids.empty? - message_ids = message_ids.first(5) - host = Email::Sender.host_for(Discourse.base_url) post_id_regexp = Regexp.new "topic/\\d+/(\\d+)@#{Regexp.escape(host)}" topic_id_regexp = Regexp.new "topic/(\\d+)@#{Regexp.escape(host)}" @@ -729,6 +753,14 @@ module Email Post.where(id: post_ids).order(:created_at).last end + def self.extract_reply_message_ids(mail, max_message_id_count:) + message_ids = [mail.in_reply_to, Email::Receiver.extract_references(mail.references)] + message_ids.flatten! + message_ids.select!(&:present?) + message_ids.uniq! + message_ids.first(max_message_id_count) + end + def self.extract_references(references) if Array === references references diff --git a/script/import_scripts/mbox/support/indexer.rb b/script/import_scripts/mbox/support/indexer.rb index 46265220a86..dfaf74f68f7 100644 --- a/script/import_scripts/mbox/support/indexer.rb +++ b/script/import_scripts/mbox/support/indexer.rb @@ -197,11 +197,7 @@ module ImportScripts::Mbox end def extract_reply_message_ids(mail) - message_ids = [mail.in_reply_to, Email::Receiver.extract_references(mail.references)] - message_ids.flatten! - message_ids.select!(&:present?) - message_ids.uniq! - message_ids.first(20) + Email::Receiver.extract_reply_message_ids(mail, max_message_id_count: 20) end def extract_subject(receiver, list_name) diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb index c8ba5ee0efa..ef1f2f161c2 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -412,7 +412,7 @@ describe Email::Receiver do expect(topic.posts.last.created_at).to be_within(1.minute).of(DateTime.now) end - it "accepts emails with wrong reply key if the system knows about the forwareded email" do + it "accepts emails with wrong reply key if the system knows about the forwarded email" do Fabricate(:incoming_email, raw: <<~RAW, Return-Path: @@ -549,6 +549,27 @@ describe Email::Receiver do end + context "when message sent to a group has no key and find_related_post_with_key is enabled" do + let!(:topic) do + SiteSetting.find_related_post_with_key = true + process(:email_reply_1) + Topic.last + end + + it "creates a reply when the sender and referenced message id are known" do + expect { process(:email_reply_2) }.to change { topic.posts.count }.by(1).and change { Topic.count }.by(0) + end + + it "creates a new topic when the sender is not known" do + IncomingEmail.where(message_id: '34@foo.bar.mail').update(cc_addresses: 'three@foo.com') + expect { process(:email_reply_2) }.to change { topic.posts.count }.by(0).and change { Topic.count }.by(1) + end + + it "creates a new topic when the referenced message id is not known" do + IncomingEmail.where(message_id: '34@foo.bar.mail').update(message_id: '99@foo.bar.mail') + expect { process(:email_reply_2) }.to change { topic.posts.count }.by(0).and change { Topic.count }.by(1) + end + end end context "new topic in a category" do @@ -887,7 +908,7 @@ describe Email::Receiver do end end - it "tries to fix unparsable email addresses in To, CC and BBC headers" do + it "tries to fix unparsable email addresses in To and CC headers" do expect { process(:unparsable_email_addresses) }.to raise_error(Email::Receiver::BadDestinationAddress) email = IncomingEmail.last