From e9dc88a7b672dc9938b8f5f59058f501302870c2 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 10 Jun 2021 15:28:50 +1000 Subject: [PATCH] FIX: Link up reply to post correctly when emailing group (#13339) When replying to a user_private_message email originating from a group PM that does _not_ have a reply key (e.g. when replying directly to the group's SMTP address), we were mistakenly linking the new post created from the reply to the OP and the user who created the topic, based on the first IncomingEmail message ID in the topic, rather than using the correct reply to user and post number that the user actually replied to. We now use the In-Reply-To header to look up the corresponding EmailLog record when the user who replied was sent a user_private_message email, and use the post from that as the reply_to_user/post. This also removes superfluous filtering of incoming_email records. After already filtering by message_id and then addressed_to_user (which only returns incoming emails where the to, from, or cc address includes any of the user's emails), we were filtering again but in the ruby code for the exact same conditions. After removing this all existing tests still pass. --- app/models/email_log.rb | 11 +++ lib/email/receiver.rb | 34 +++---- spec/components/email/receiver_spec.rb | 89 +++++++++++++++++++ .../email_to_group_email_username_1.eml | 11 +++ .../email_to_group_email_username_2.eml | 14 +++ ...oup_email_username_2_as_unknown_sender.eml | 14 +++ 6 files changed, 158 insertions(+), 15 deletions(-) create mode 100644 spec/fixtures/emails/email_to_group_email_username_1.eml create mode 100644 spec/fixtures/emails/email_to_group_email_username_2.eml create mode 100644 spec/fixtures/emails/email_to_group_email_username_2_as_unknown_sender.eml diff --git a/app/models/email_log.rb b/app/models/email_log.rb index 6cfbca497a8..33c8ea12e73 100644 --- a/app/models/email_log.rb +++ b/app/models/email_log.rb @@ -22,6 +22,17 @@ class EmailLog < ActiveRecord::Base scope :bounced, -> { where(bounced: true) } + 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 + email_logs.to_address = user_emails.email + ) + SQL + end + after_create do # Update last_emailed_at if the user_id is present and email was sent User.where(id: user_id).update_all("last_emailed_at = CURRENT_TIMESTAMP") if user_id.present? diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index 5ec587ed377..da699c08bcd 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -109,7 +109,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? + return unless discourse_generated_message_id?(@message_id) incoming_email.update( imap_uid_validity: @opts[:imap_uid_validity], @@ -745,23 +745,27 @@ module Email 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 with matching message ids, and then cross references + # these with any email addresses for the user vs to/from/cc of the + # incoming emails. in effect, any incoming email record for these + # message ids where the user is involved in any way will be returned incoming_emails = IncomingEmail.where(message_id: message_ids) if !group.allow_unknown_sender_topic_replies incoming_emails = incoming_emails.addressed_to_user(user) end + post_ids = incoming_emails.pluck(:post_id) || [] - incoming_emails = incoming_emails.pluck(:post_id, :from_address, :to_addresses, :cc_addresses) - - incoming_emails.each do |post_id, from_address, to_addresses, cc_addresses| - if group.allow_unknown_sender_topic_replies - post_ids << post_id - else - post_ids << post_id if contains_email_address_of_user?(from_address, user) || - contains_email_address_of_user?(to_addresses, user) || - contains_email_address_of_user?(cc_addresses, user) - end + # 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) + post_id_from_email_log = EmailLog.where(message_id: mail.in_reply_to) + .addressed_to_user(user) + .order(created_at: :desc) + .limit(1) + .pluck(:post_id).last + post_ids << post_id_from_email_log end if post_ids.any? && post = Post.where(id: post_ids).order(:created_at).last @@ -989,9 +993,9 @@ module Email @host ||= Email::Sender.host_for(Discourse.base_url) end - def discourse_generated_message_id? - !!(@message_id =~ message_id_post_id_regexp) || - !!(@message_id =~ message_id_topic_id_regexp) + 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 diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb index 636cc73cad5..11a87b4e632 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -985,6 +985,95 @@ describe Email::Receiver do end end + context "emailing a group by email_username and following reply flow" do + let!(:topic) do + group.update!(email_username: "team@somesmtpaddress.com") + process(:email_to_group_email_username_1) + Topic.last + end + fab!(:user_in_group) do + u = Fabricate(:user) + Fabricate(:group_user, user: u, group: group) + u + end + + before do + NotificationEmailer.enable + Jobs.run_immediately! + end + + def reply_as_group_user + group_post = PostCreator.create( + user_in_group, + raw: "Thanks for your request. Please try to restart.", + topic_id: 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) + 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(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") + end + + 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.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) + expect(IncomingEmail.exists?(post_id: group_post.id)).to eq(false) + end + + it "processes a reply from the OP user to the group SMTP username, linking the reply_to_post_number correctly by + matching in_reply_to to the email log" do + email_log, group_post = reply_as_group_user + + 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(0).and change { Post.count }.by(1) + + reply_post = Post.last + expect(reply_post.reply_to_user).to eq(user_in_group) + expect(reply_post.reply_to_post_number).to eq(group_post.post_number) + end + + it "processes the reply from the user as a brand new topic if they have replied from a different address (e.g. auto forward) and allow_unknown_sender_topic_replies is disabled" do + email_log, group_post = reply_as_group_user + + reply_email = email(:email_to_group_email_username_2_as_unknown_sender) + 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 + expect(reply_post.topic_id).not_to eq(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 + group.update!(allow_unknown_sender_topic_replies: true) + email_log, group_post = reply_as_group_user + + reply_email = email(:email_to_group_email_username_2_as_unknown_sender) + 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(0).and change { Post.count }.by(1) + + reply_post = Post.last + expect(reply_post.topic_id).to eq(topic.id) + end + end + context "when message sent to a group has no key and find_related_post_with_key is enabled" do let!(:topic) do process(:email_reply_1) diff --git a/spec/fixtures/emails/email_to_group_email_username_1.eml b/spec/fixtures/emails/email_to_group_email_username_1.eml new file mode 100644 index 00000000000..5a7a0aee6b3 --- /dev/null +++ b/spec/fixtures/emails/email_to_group_email_username_1.eml @@ -0,0 +1,11 @@ +Return-Path: +From: Two +To: team@somesmtpaddress.com +Subject: Full email group username flow +Date: Fri, 15 Jan 2016 00:12:43 +0100 +Message-ID: +Mime-Version: 1.0 +Content-Type: text/plain +Content-Transfer-Encoding: 7bit + +This is the first email. diff --git a/spec/fixtures/emails/email_to_group_email_username_2.eml b/spec/fixtures/emails/email_to_group_email_username_2.eml new file mode 100644 index 00000000000..df9bd7c2027 --- /dev/null +++ b/spec/fixtures/emails/email_to_group_email_username_2.eml @@ -0,0 +1,14 @@ +Return-Path: +From: Two +To: team@somesmtpaddress.com +Subject: Full email group username flow +Date: Saturday, 16 Jan 2016 00:12:43 +0100 +Message-ID: <348ct38794nyt9338dsfsd@foo.bar.mail> +In-Reply-To: +References: + +Mime-Version: 1.0 +Content-Type: text/plain +Content-Transfer-Encoding: 7bit + +Hey thanks for the suggestion, it didn't work. diff --git a/spec/fixtures/emails/email_to_group_email_username_2_as_unknown_sender.eml b/spec/fixtures/emails/email_to_group_email_username_2_as_unknown_sender.eml new file mode 100644 index 00000000000..ddb591ddd1b --- /dev/null +++ b/spec/fixtures/emails/email_to_group_email_username_2_as_unknown_sender.eml @@ -0,0 +1,14 @@ +Return-Path: +From: Mysterio +To: team@somesmtpaddress.com +Subject: Full email group username flow +Date: Saturday, 16 Jan 2016 00:12:43 +0100 +Message-ID: <348ct38794nyt9338dsfsd@foo.bar.mail> +In-Reply-To: +References: + +Mime-Version: 1.0 +Content-Type: text/plain +Content-Transfer-Encoding: 7bit + +No I don't think that did it.