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