FEATURE: Update upload security status on post move, topic conversion, category change (#8731)

Add TopicUploadSecurityManager to handle post moves. When a post moves around or a topic changes between categories and public/private message status the uploads connected to posts in the topic need to have their secure status updated, depending on the security context the topic now lives in.

app/jobs/regular/update_topic_upload_security.rb

@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module Jobs
+
+  class UpdateTopicUploadSecurity < ::Jobs::Base
+
+    def execute(args)
+      topic = Topic.find_by(id: args[:topic_id])
+      if topic.blank?
+        Rails.logger.info("Could not find topic #{args[:topic_id]} for topic upload security updater.")
+        return
+      end
+      TopicUploadSecurityManager.new(topic).run
+    end
+  end
+end

app/models/post.rb

@@ -1044,6 +1044,10 @@ class Post < ActiveRecord::Base
     { uploads: missing_uploads, post_uploads: missing_post_uploads, count: count }
   end
 
+  def owned_uploads_via_access_control
+    Upload.where(access_control_post_id: self.id)
+  end
+
   private
 
   def parse_quote_into_arguments(quote)
@@ -1070,7 +1074,6 @@ class Post < ActiveRecord::Base
       end
     end
   end
-
 end
 
 # == Schema Information

app/models/post_mover.rb

@@ -70,6 +70,7 @@ class PostMover
     update_statistics
     update_user_actions
     update_last_post_stats
+    update_upload_security_status
 
     if moving_all_posts
       @original_topic.update_status('closed', true, @user)
@@ -497,6 +498,12 @@ class PostMover
     end
   end
 
+  def update_upload_security_status
+    DB.after_commit do
+      Jobs.enqueue(:update_topic_upload_security, topic_id: @destination_topic.id)
+    end
+  end
+
   def watch_new_topic
     if @destination_topic.archetype == Archetype.private_message
       if @original_topic.archetype == Archetype.private_message

app/models/topic.rb

@@ -705,6 +705,13 @@ class Topic < ActiveRecord::Base
         CategoryFeaturedTopic.feature_topics_for(old_category) unless @import_mode
         CategoryFeaturedTopic.feature_topics_for(new_category) unless @import_mode || old_category.try(:id) == new_category.id
       end
+
+      # when a topic changes category we may need to make uploads
+      # linked to posts secure/not secure depending on whether the
+      # category is private
+      DB.after_commit do
+        Jobs.enqueue(:update_topic_upload_security, topic_id: self.id)
+      end
     end
 
     true

app/models/topic_converter.rb

@@ -99,10 +99,8 @@ class TopicConverter
   end
 
   def update_post_uploads_secure_status
-    @topic.posts.each do |post|
-      next if post.uploads.empty?
-      post.update_uploads_secure_status
-      post.rebake!
+    DB.after_commit do
+      Jobs.enqueue(:update_topic_upload_security, topic_id: @topic.id)
     end
   end
 end

app/models/upload.rb

@@ -235,8 +235,11 @@ class Upload < ActiveRecord::Base
     return false if self.for_theme || self.for_site_setting
     mark_secure = secure_override_value.nil? ? UploadSecurity.new(self).should_be_secure? : secure_override_value
 
+    secure_status_did_change = self.secure? != mark_secure
     self.update_column("secure", mark_secure)
     Discourse.store.update_upload_ACL(self) if Discourse.store.external?
+
+    secure_status_did_change
   end
 
   def self.migrate_to_new_scheme(limit: nil)

lib/topic_upload_security_manager.rb

@@ -0,0 +1,93 @@
+# frozen_string_literal: true
+
+##
+# There are certain conditions with secure media when the security of
+# uploads will need to change depending on the context they reside in.
+#
+# For example on these conditions:
+#  * Topic category change
+#  * Topic switching between PM and public topic
+#  * Post moving between topics
+#
+# We need to go through all of the posts in that topic that
+# own uploads via access_control_post_id, then for those uploads determine
+# if they still need to be secure or not. For example an upload could be
+# secure if it is in a PM, and then when the topic gets converted to a public
+# topic the upload no longer needs to remain secure as it is no longer in
+# a secure context.
+class TopicUploadSecurityManager
+  def initialize(topic)
+    @topic = topic
+  end
+
+  def run
+    Rails.logger.debug("Updating upload security in topic #{@topic.id}")
+    posts_owning_uploads.each do |post|
+      Post.transaction do
+        Rails.logger.debug("Updating upload security in topic #{@topic.id} - post #{post.id}")
+        post.topic = @topic
+
+        secure_status_did_change = post.owned_uploads_via_access_control.any? do |upload|
+          # we have already got the post preloaded so we may as well
+          # attach it here to avoid another load in UploadSecurity
+          upload.access_control_post = post
+          upload.update_secure_status
+        end
+        post.rebake! if secure_status_did_change
+        Rails.logger.debug("Security updated & rebake complete in topic #{@topic.id} - post #{post.id}")
+      end
+    end
+
+    return if !SiteSetting.secure_media
+
+    # we only want to do this if secure media is enabled. if
+    # the setting is turned on after a site has been running
+    # already, we want to make sure that any post moves after
+    # this are handled and upload secure statuses and ACLs
+    # are updated appropriately, as well as setting the access control
+    # post for secure uploads missing it.
+    #
+    # examples (all after secure media is enabled):
+    #
+    #  -> a public topic is moved to a private category after
+    #  -> a PM is converted to a public topic
+    #  -> a public topic is converted to a PM
+    #  -> a topic is moved from a private to a public category
+    posts_with_unowned_uploads.each do |post|
+      Post.transaction do
+        Rails.logger.debug("Setting upload access control posts in topic #{@topic.id} - post #{post.id}")
+        post.topic = @topic
+
+        secure_status_did_change = post.uploads.any? do |upload|
+          first_post_upload_appeared_in = upload.post_uploads.first.post
+          if first_post_upload_appeared_in == post
+            upload.update(access_control_post: post)
+            upload.update_secure_status
+          else
+            false
+          end
+        end
+
+        post.rebake! if secure_status_did_change
+        Rails.logger.debug("Completed changing access control posts #{secure_status_did_change ? 'and rebaking' : ''} in topic #{@topic.id} - post #{post.id}")
+      end
+    end
+
+    Rails.logger.debug("Completed updating upload security in topic #{@topic.id}!")
+  end
+
+  private
+
+  def posts_owning_uploads
+    Post.where(topic_id: @topic.id).joins('INNER JOIN uploads ON access_control_post_id = posts.id')
+  end
+
+  def posts_with_unowned_uploads
+    Post
+      .where(topic_id: @topic.id)
+      .joins('INNER JOIN post_uploads ON post_uploads.post_id = posts.id')
+      .joins('INNER JOIN uploads ON post_uploads.upload_id = uploads.id')
+      .where('uploads.access_control_post_id IS NULL')
+      .includes(:uploads)
+  end
+end

lib/upload_security.rb

@@ -60,7 +60,7 @@ class UploadSecurity
   # if there is no access control post id and the upload is currently secure, we
   # do not want to make it un-secure to avoid unintentionally exposing it
   def access_control_post_has_secure_media?
-    Post.find_by(id: @upload.access_control_post_id).with_secure_media?
+    @upload.access_control_post.with_secure_media?
   end
 
   def public_type?

spec/components/post_creator_spec.rb

@@ -217,6 +217,7 @@ describe PostCreator do
         Jobs.stubs(:enqueue).with(:feature_topic_users, has_key(:topic_id))
         Jobs.expects(:enqueue).with(:notify_mailing_list_subscribers, has_key(:post_id))
         Jobs.expects(:enqueue).with(:post_alert, has_key(:post_id))
+        Jobs.expects(:enqueue).with(:update_topic_upload_security, has_key(:topic_id))
         Jobs.expects(:enqueue).with(:process_post, has_key(:invalidate_oneboxes))
         creator.opts[:invalidate_oneboxes] = true
         creator.create
@@ -226,6 +227,7 @@ describe PostCreator do
         Jobs.stubs(:enqueue).with(:feature_topic_users, has_key(:topic_id))
         Jobs.expects(:enqueue).with(:notify_mailing_list_subscribers, has_key(:post_id))
         Jobs.expects(:enqueue).with(:post_alert, has_key(:post_id))
+        Jobs.expects(:enqueue).with(:update_topic_upload_security, has_key(:topic_id))
         Jobs.expects(:enqueue).with(:process_post, has_key(:image_sizes))
         creator.opts[:image_sizes] = { 'http://an.image.host/image.jpg' => { 'width' => 17, 'height' => 31 } }
         creator.create

spec/lib/topic_upload_security_manager_spec.rb

@@ -0,0 +1,189 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe TopicUploadSecurityManager do
+  let(:group) { Fabricate(:group) }
+  let(:category) { Fabricate(:category) }
+  let!(:topic) { Fabricate(:topic, user: user, category: category) }
+  let!(:user) { Fabricate(:user) }
+  let!(:post1) { Fabricate(:post, topic: topic) }
+  let!(:post2) { Fabricate(:post, topic: topic) }
+  let!(:post3) { Fabricate(:post, topic: topic) }
+  let!(:post4) { Fabricate(:post, topic: topic) }
+
+  subject { described_class.new(topic) }
+
+  context "when a topic has posts linked to secure uploads" do
+    let!(:upload) { Fabricate(:secure_upload) }
+    let!(:upload2) { Fabricate(:secure_upload) }
+    let!(:upload3) { Fabricate(:secure_upload) }
+
+    before do
+      PostUpload.create(upload: upload, post: post2)
+      PostUpload.create(upload: upload2, post: post3)
+      upload.update(access_control_post: post2)
+      upload2.update(access_control_post: post3)
+    end
+
+    context "when the topic category is read restricted" do
+      let(:category) { Fabricate(:private_category, group: group) }
+
+      context "when secure media is enabled" do
+        before { enable_secure_media }
+
+        it "does not change any upload statuses or update ACLs or rebake" do
+          expect_upload_status_not_to_change
+        end
+
+        context "when changing the topic to a non-private category" do
+          before do
+            topic.update(category: Fabricate(:category))
+          end
+          it "changes the upload secure statuses to false and updates ACLs and rebakes" do
+            expect_upload_status_to_change_and_rebake
+          end
+        end
+      end
+
+      context "when secure media is disabled" do
+        it "changes the upload secure statuses to false and updates ACLs and rebakes" do
+          expect_upload_status_to_change_and_rebake
+        end
+      end
+    end
+
+    context "when the topic is a private message" do
+      let(:topic) { Fabricate(:private_message_topic, category: category, user: user) }
+
+      context "when secure media is enabled" do
+        before { enable_secure_media }
+
+        it "does not change any upload statuses or update ACLs or rebake" do
+          expect_upload_status_not_to_change
+        end
+
+        context "when making the PM into a public topic" do
+          before do
+            topic.update(archetype: Archetype.default)
+          end
+          it "changes the upload secure statuses to false and updates ACLs and rebakes" do
+            expect_upload_status_to_change_and_rebake
+          end
+        end
+      end
+
+      context "when secure media is disabled" do
+        it "changes the upload secure statuses to false and updates ACLs and rebakes" do
+          expect_upload_status_to_change_and_rebake
+        end
+      end
+    end
+
+    context "when the topic is public" do
+      context "when secure media is enabled" do
+        before { enable_secure_media }
+
+        context "when login required is enabled" do
+          before do
+            SiteSetting.login_required = true
+          end
+
+          it "does not change any upload statuses or update ACLs or rebake" do
+            expect_upload_status_not_to_change
+          end
+        end
+
+        context "when login required is not enabled" do
+          before do
+            SiteSetting.login_required = false
+          end
+
+          it "changes the upload secure statuses to false and updates ACLs and rebakes" do
+            expect_upload_status_to_change_and_rebake
+          end
+        end
+      end
+    end
+
+    context "when one of the posts has an upload without an access control post" do
+      let(:category) { Fabricate(:private_category, group: group) }
+      let!(:upload3) { Fabricate(:upload) }
+
+      before do
+        enable_secure_media
+      end
+
+      context "when this is the first post the upload has appeared in" do
+        before do
+          PostUpload.create(upload: upload3, post: post4)
+        end
+
+        it "changes the upload secure status to true and changes the ACL and rebakes the post and sets the access control post" do
+          expect(Post.any_instance.expects(:rebake!).once)
+          subject.run
+          expect(upload3.reload.secure?).to eq(true)
+          expect(upload3.reload.access_control_post).to eq(post4)
+        end
+
+        context "when secure media is not enabled" do
+          before do
+            SiteSetting.secure_media = false
+          end
+
+          it "does not change the upload secure status and does not set the access control post" do
+            subject.run
+            expect(upload3.reload.secure?).to eq(false)
+            expect(upload3.reload.access_control_post).to eq(nil)
+          end
+        end
+      end
+
+      context "when this is not the first post the upload has appeared in" do
+        before do
+          PostUpload.create(upload: upload3, post: Fabricate(:post))
+          PostUpload.create(upload: upload3, post: post4)
+        end
+
+        it "does not change the upload secure status and does not set the access control post" do
+          expect(Post.any_instance.expects(:rebake!).never)
+          subject.run
+          expect(upload3.reload.secure?).to eq(false)
+          expect(upload3.reload.access_control_post).to eq(nil)
+        end
+      end
+    end
+  end
+
+  def enable_secure_media
+    SiteSetting.enable_s3_uploads = true
+    SiteSetting.s3_upload_bucket = "s3-upload-bucket"
+    SiteSetting.s3_access_key_id = "some key"
+    SiteSetting.s3_secret_access_key = "some secrets3_region key"
+    SiteSetting.secure_media = true
+
+    stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
+
+    # because the ACLs will be changing...
+    [upload, upload2, upload3].each do |upl|
+      stub_request(
+        :put,
+        "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/original/1X/#{upl.sha1}.#{upl.extension}?acl"
+      )
+    end
+  end
+
+  def expect_upload_status_not_to_change
+    expect(Post.any_instance.expects(:rebake!).never)
+    subject.run
+    expect(upload.reload.secure?).to eq(true)
+    expect(upload2.reload.secure?).to eq(true)
+  end
+
+  def expect_upload_status_to_change_and_rebake
+    expect(Post.any_instance.expects(:rebake!).twice)
+    subject.run
+    expect(upload.reload.secure?).to eq(false)
+    expect(upload2.reload.secure?).to eq(false)
+  end
+end

spec/models/topic_converter_spec.rb

@@ -107,6 +107,10 @@ describe TopicConverter do
         fab!(:image_upload) { Fabricate(:upload) }
         fab!(:public_topic) { Fabricate(:topic, user: author) }
 
+        before do
+          Jobs.run_immediately!
+        end
+
         before do
           SiteSetting.enable_s3_uploads = true
           SiteSetting.s3_upload_bucket = "s3-upload-bucket"