FIX: Correctly re-attach allowed images in activity summary e-mail (#26642)

For e-mails, secure uploads redacts all secure images, and later uses the access control post to re-attached allowed ones. We pass the ID of this post through the X-Discourse-Post-Id header. As the name suggests, this assumes there's only ever one access control post. This is not true for activity summary e-mails, as they summarize across posts.

This adds a new header, X-Discourse-Post-Ids, which is used the same way as the old header, but also works for the case where an e-mail is associated with multiple posts.

app/mailers/user_notifications.rb

@@ -231,14 +231,14 @@ class UserNotifications < ActionMailer::Base
       limit: SiteSetting.digest_topics + SiteSetting.digest_other_topics,
       top_order: true,
     }
-    topics_for_digest = Topic.for_digest(user, min_date, digest_opts).to_a
+    topics_for_digest = Topic.for_digest(user, min_date, digest_opts)
     if topics_for_digest.empty? && !user.user_option.try(:include_tl0_in_digests)
       # Find some topics from new users that are at least 24 hours old
       topics_for_digest =
-        Topic
-          .for_digest(user, min_date, digest_opts.merge(include_tl0: true))
-          .where("topics.created_at < ?", 24.hours.ago)
-          .to_a
+        Topic.for_digest(user, min_date, digest_opts.merge(include_tl0: true)).where(
+          "topics.created_at < ?",
+          24.hours.ago,
+        )
     end
 
     @popular_topics = topics_for_digest[0, SiteSetting.digest_topics]
@@ -349,6 +349,9 @@ class UserNotifications < ActionMailer::Base
           ),
         add_unsubscribe_link: true,
         unsubscribe_url: "#{Discourse.base_url}/email/unsubscribe/#{@unsubscribe_key}",
+        topic_ids: topics_for_digest.pluck(:id),
+        post_ids:
+          topics_for_digest.joins(:posts).where(posts: { post_number: 1 }).pluck("posts.id"),
       }
 
       build_email(user.email, opts)

lib/email/message_builder.rb

@@ -218,7 +218,9 @@ module Email
       end
 
       result["X-Discourse-Post-Id"] = @opts[:post_id].to_s if @opts[:post_id]
+      result["X-Discourse-Post-Ids"] = @opts[:post_ids].join(",") if @opts[:post_ids].present?
       result["X-Discourse-Topic-Id"] = @opts[:topic_id].to_s if @opts[:topic_id]
+      result["X-Discourse-Topic-Ids"] = @opts[:topic_ids].join(",") if @opts[:topic_ids].present?
 
       # at this point these have been filtered by the recipient's guardian for visibility,
       # see UserNotifications#send_notification_email

lib/email/sender.rb

@@ -253,6 +253,9 @@ module Email
       if post.present?
         @stripped_secure_upload_shas = style.stripped_upload_sha_map.values
         add_attachments(post)
+      elsif @email_type == :digest
+        @stripped_secure_upload_shas = style.stripped_upload_sha_map.values
+        digest_posts.each { |p| add_attachments(p) }
       end
 
       # Suppress images from short emails
@@ -347,6 +350,10 @@ module Email
 
     private
 
+    def digest_posts
+      Post.where(id: header_value("X-Discourse-Post-Ids")&.split(","))
+    end
+
     def add_attachments(post)
       max_email_size = SiteSetting.email_total_attachment_size_limit_kb.kilobytes
       return if max_email_size == 0

spec/lib/email/sender_spec.rb

@@ -640,6 +640,31 @@ RSpec.describe Email::Sender do
           expect(message.to_s.scan(/cid:[\w\-@.]+/).uniq.length).to eq(2)
         end
 
+        it "attaches allowed images from multiple posts in the activity summary" do
+          other_post = Fabricate(:post)
+          @secure_image_2 =
+            UploadCreator.new(
+              file_from_fixtures("logo-dev.png", "images"),
+              "something-cool.png",
+            ).create_for(Discourse.system_user.id)
+          @secure_image_2.update_secure_status(override: true)
+          @secure_image_2.update(access_control_post_id: other_post.id)
+
+          Jobs::PullHotlinkedImages.any_instance.expects(:execute)
+          other_post.update(
+            raw:
+              "#{UploadMarkdown.new(@secure_image).image_markdown}\n#{UploadMarkdown.new(@secure_image_2).image_markdown}",
+          )
+          other_post.rebake!
+
+          message.header["X-Discourse-Post-Id"] = nil
+          message.header["X-Discourse-Post-Ids"] = "#{reply.id},#{other_post.id}"
+          Email::Sender.new(message, :digest).send
+          expect(message.attachments.map(&:filename)).to include(
+            *[image, @secure_image, @secure_image_2].map(&:original_filename),
+          )
+        end
+
         it "does not attach images that are not marked as secure, in the case of a non-secure upload copied to a PM" do
           SiteSetting.login_required = false
           @secure_image.update_secure_status(override: false)

spec/mailers/user_notifications_spec.rb

@@ -181,10 +181,18 @@ RSpec.describe UserNotifications do
     end
 
     context "with new topics" do
-      let!(:popular_topic) do
-        Fabricate(:topic, user: Fabricate(:coding_horror), created_at: 1.hour.ago)
+      fab!(:coding_horror)
+
+      let!(:popular_topic) { Fabricate(:topic, user: coding_horror, created_at: 1.hour.ago) }
+
+      let!(:another_popular_topic) do
+        Fabricate(:topic, user: coding_horror, created_at: 1.hour.ago)
       end
 
+      let!(:post) { Fabricate(:post, topic: popular_topic, post_number: 1) }
+
+      let!(:another_post) { Fabricate(:post, topic: another_popular_topic, post_number: 1) }
+
       it "works" do
         expect(email.to).to eq([user.email])
         expect(email.subject).to be_present
@@ -192,6 +200,10 @@ RSpec.describe UserNotifications do
         expect(email.html_part.body.to_s).to be_present
         expect(email.text_part.body.to_s).to be_present
         expect(email.header["List-Unsubscribe"].to_s).to match(/\/email\/unsubscribe\/\h{64}/)
+        expect(email.header["X-Discourse-Topic-Ids"].to_s).to eq(
+          "#{another_popular_topic.id},#{popular_topic.id}",
+        )
+        expect(email.header["X-Discourse-Post-Ids"].to_s).to eq("#{another_post.id},#{post.id}")
         expect(email.html_part.body.to_s).to include("New Users")
       end