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.
This commit is contained in:
Gerhard Schlager 2018-03-30 14:37:19 +02:00
parent e36e9de28a
commit f2d00e5eff
4 changed files with 98 additions and 32 deletions

View File

@ -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

View File

@ -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

View File

@ -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)

View File

@ -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: <discourse@bar.com>
@ -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