FIX: Correct the forwarded by user small post for group inbox (#14252)

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.

diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb
index cba2208..2a0ce19 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 cef30c1..e8be9d1 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_group.eml

[... diff too long, it was truncated ...]

GitHub sha: 7b392cee50a7c404acb47b7082920c1d27d4f53e

This commit appears in #14252 which was approved by lis2. It was merged by martin.