From 7b392cee50a7c404acb47b7082920c1d27d4f53e Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 6 Sep 2021 15:02:13 +1000 Subject: [PATCH] FIX: Correct the forwarded by user small post for group inbox (#14252) When 2ac9fd9dff5a2a9210e2b1b765e1a324bbb4f421 was done, this affected the small post that is created when forwarding an email into the group inbox. Instead of using the name and the email of the user who forwarded the email, it used the original from email and name to create the small post. So instead of something like "Discourse Team forwarded the above email" we ended up with "John Smith forwarded the above email" which is incorrect. This fixes the issue by creating a staged user for the forwarding email address (if such a user does not yet exist) and uses that for the "forwarded" small post instead. --- lib/email/receiver.rb | 72 +++++++++++++------ spec/components/email/receiver_spec.rb | 46 +++++++++++- ...up.eml => forwarded_by_group_to_inbox.eml} | 0 3 files changed, 94 insertions(+), 24 deletions(-) rename spec/fixtures/emails/{forwarded_by_group_to_group.eml => forwarded_by_group_to_inbox.eml} (100%) diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index cba22085c89..2a0ce1920de 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -589,12 +589,8 @@ module Email # which we can extract from embedded_email_raw. if has_been_forwarded? if mail[:from].to_s =~ group_incoming_emails_regex && embedded_email[:from].errors.blank? - embedded_email[:from].each do |address_field| - from_address = address_field.address - from_display_name = address_field.display_name&.to_s - next if !from_address&.include?("@") - return [from_address&.downcase, from_display_name&.strip] - end + from_address, from_display_name = extract_from_fields_from_header(embedded_email, :from) + return [from_address, from_display_name] if from_address end end @@ -604,23 +600,16 @@ module Email # header in more cases. if mail['X-Original-From'].present? if mail[:reply_to] && mail[:reply_to].errors.blank? - mail[:reply_to].each do |address_field| - from_address = address_field.address - from_display_name = address_field.display_name&.to_s - next if address_field.to_s != mail['X-Original-From'].to_s - next if !from_address&.include?("@") - return [from_address&.downcase, from_display_name&.strip] - end + from_address, from_display_name = extract_from_fields_from_header( + mail, :reply_to, comparison_headers: ['X-Original-From'] + ) + return [from_address, from_display_name] if from_address end end if mail[:from].errors.blank? - mail[:from].each do |address_field| - from_address = address_field.address - from_display_name = address_field.display_name&.to_s - next if !from_address&.include?("@") - return [from_address&.downcase, from_display_name&.strip] - end + from_address, from_display_name = extract_from_fields_from_header(mail, :from) + return [from_address, from_display_name] if from_address end return extract_from_address_and_name(mail.from) if mail.from.is_a? String @@ -637,6 +626,24 @@ module Email nil end + def extract_from_fields_from_header(mail_object, header, comparison_headers: []) + mail_object[header].each do |address_field| + from_address = address_field.address + from_display_name = address_field.display_name&.to_s + + comparison_failed = false + comparison_headers.each do |comparison_header| + comparison_failed = true if address_field.to_s != mail_object[comparison_header].to_s + end + + next if comparison_failed + next if !from_address&.include?("@") + return [from_address&.downcase, from_display_name&.strip] + end + + [nil, nil] + end + def extract_from_address_and_name(value) if value[";"] from_display_name, from_address = value.split(";") @@ -948,6 +955,10 @@ module Email embedded = Mail.new(embedded_email_raw) email, display_name = parse_from_field(embedded) + if forwarded_by_address && forwarded_by_name + forwarded_by_user = stage_sender_user(forwarded_by_address, forwarded_by_name) + end + return false if email.blank? || !email["@"] post = forwarded_email_create_topic(destination: destination, @@ -974,13 +985,28 @@ module Email post_type: post_type, skip_validations: user.staged?) else - post.topic.add_small_action(user, "forwarded") + if forwarded_by_user + post.topic.topic_allowed_users.find_or_create_by!(user_id: forwarded_by_user.id) + end + post.topic.add_small_action(forwarded_by_user || user, "forwarded") end end true end + def forwarded_by_sender + @forwarded_by_sender ||= extract_from_fields_from_header(@mail, :from) + end + + def forwarded_by_address + @forwarded_by_address ||= forwarded_by_sender&.first + end + + def forwarded_by_name + @forwarded_by_name ||= forwarded_by_sender&.first + end + def forwarded_email_quote_forwarded(destination, user) embedded = embedded_email_raw raw = <<~EOF @@ -1367,7 +1393,11 @@ module Email end def stage_from_user - @from_user ||= find_or_create_user!(@from_email, @from_display_name).tap do |u| + @from_user ||= stage_sender_user(@from_email, @from_display_name) + end + + def stage_sender_user(email, display_name) + find_or_create_user!(email, display_name).tap do |u| log_and_validate_user(u) end end diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb index cef30c146dc..e8be9d1148f 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -1033,7 +1033,7 @@ describe Email::Receiver do end context "when a group forwards an email to its inbox" do - let!(:topic) do + before do group.update!( email_username: "team@somesmtpaddress.com", incoming_email: "team@somesmtpaddress.com|support+team@bar.com", @@ -1042,14 +1042,54 @@ describe Email::Receiver do smtp_ssl: true, smtp_enabled: true ) - process(:forwarded_by_group_to_group) - Topic.last end it "does not use the team's address as the from_address; it uses the original sender address" do + process(:forwarded_by_group_to_inbox) + topic = Topic.last expect(topic.incoming_email.first.to_addresses).to include("support+team@bar.com") expect(topic.incoming_email.first.from_address).to eq("fred@bedrock.com") end + + context "with forwarded emails behaviour set to create replies" do + before do + SiteSetting.forwarded_emails_behaviour = "create_replies" + end + + it "does not use the team's address as the from_address; it uses the original sender address" do + process(:forwarded_by_group_to_inbox) + topic = Topic.last + expect(topic.incoming_email.first.to_addresses).to include("support+team@bar.com") + expect(topic.incoming_email.first.from_address).to eq("fred@bedrock.com") + end + + it "does not say the email was forwarded by the original sender, it says the email is forwarded by the group" do + expect { process(:forwarded_by_group_to_inbox) }.to change { User.where(staged: true).count }.by(2) + topic = Topic.last + forwarded_small_post = topic.ordered_posts.last + expect(forwarded_small_post.action_code).to eq("forwarded") + expect(forwarded_small_post.user).to eq(User.find_by_email("team@somesmtpaddress.com")) + end + + context "when staged user for the team email already exists" do + let!(:staged_team_user) do + User.create!( + email: "team@somesmtpaddress.com", + username: UserNameSuggester.suggest("team@somesmtpaddress.com"), + name: "team teamson", + staged: true + ) + end + + it "uses that and does not create another staged user" do + expect { process(:forwarded_by_group_to_inbox) }.to change { User.where(staged: true).count }.by(1) + topic = Topic.last + forwarded_small_post = topic.ordered_posts.last + expect(forwarded_small_post.action_code).to eq("forwarded") + expect(forwarded_small_post.user).to eq(staged_team_user) + end + end + end end context "emailing a group by email_username and following reply flow" do diff --git a/spec/fixtures/emails/forwarded_by_group_to_group.eml b/spec/fixtures/emails/forwarded_by_group_to_inbox.eml similarity index 100% rename from spec/fixtures/emails/forwarded_by_group_to_group.eml rename to spec/fixtures/emails/forwarded_by_group_to_inbox.eml