FEATURE: Secure uploads in PMs only (discourse#23398)

This adds a new secure_uploads_pm_only site setting. When secure_uploads
is true with this setting, only uploads created in PMs will be marked
secure; no uploads in secure categories will be marked as secure, and
the login_required site setting has no bearing on upload security
either.

This is meant to be a stopgap solution to prevent secure uploads
in a single place (private messages) for sensitive admin data exports.
Ideally we would want a more comprehensive way of saying that certain
upload types get secured which is a hybrid/mixed mode secure uploads,
but for now this will do the trick.

app/models/post.rb

@@ -567,8 +567,15 @@ def reviewable_flag
 
   def with_secure_uploads?
     return false if !SiteSetting.secure_uploads?
-    SiteSetting.login_required? ||
-      (topic.present? && (topic.private_message? || topic.category&.read_restricted))
+
+    # NOTE: This is meant to be a stopgap solution to prevent secure uploads
+    # in a single place (private messages) for sensitive admin data exports.
+    # Ideally we would want a more comprehensive way of saying that certain
+    # upload types get secured which is a hybrid/mixed mode secure uploads,
+    # but for now this will do the trick.
+    return topic&.private_message? if SiteSetting.secure_uploads_pm_only?
+
+    SiteSetting.login_required? || topic&.private_message? || topic&.category&.read_restricted?
   end
 
   def hide!(post_action_type_id, reason = nil, custom_message: nil)

config/site_settings.yml

@@ -1439,6 +1439,10 @@ files:
     default: 1024
     min: 1
     max: 10240
+  secure_uploads_pm_only:
+    default: false
+    hidden: true
+    client: true
   enable_s3_uploads:
     default: false
     client: true

lib/tasks/uploads.rake

@@ -609,11 +609,15 @@ task "uploads:secure_upload_analyse_and_update" => :environment do
       update_uploads_access_control_post if SiteSetting.secure_uploads?
 
       puts "", "Analysing which uploads need to be marked secure and be rebaked.", ""
-      if SiteSetting.login_required?
-        # Simply mark all uploads linked to posts secure if login_required because no anons will be able to access them.
+      if SiteSetting.login_required? && !SiteSetting.secure_uploads_pm_only?
+        # Simply mark all uploads linked to posts secure if login_required because
+        # no anons will be able to access them; however if secure_uploads_pm_only is
+        # true then login_required will not mark other uploads secure.
         post_ids_to_rebake, all_upload_ids_changed = mark_all_as_secure_login_required
       else
-        # Otherwise only mark uploads linked to posts in secure categories or PMs as secure.
+        # Otherwise only mark uploads linked to posts either:
+        #   * In secure categories or PMs if !SiteSetting.secure_uploads_pm_only
+        #   * In PMs if SiteSetting.secure_uploads_pm_only
         post_ids_to_rebake, all_upload_ids_changed =
           update_specific_upload_security_no_login_required
       end
@@ -693,15 +697,24 @@ end
 def update_specific_upload_security_no_login_required
   # A simplification of the rules found in UploadSecurity which is a lot faster than
   # having to loop through records and use that class to check security.
+  filter_clause =
+    if SiteSetting.secure_uploads_pm_only?
+      "WHERE topics.archetype = 'private_message'"
+    else
+      <<~SQL
+        LEFT JOIN categories ON categories.id = topics.category_id
+        WHERE (topics.category_id IS NOT NULL AND categories.read_restricted) OR
+          (topics.archetype = 'private_message')
+      SQL
+    end
+
   post_upload_ids_marked_secure = DB.query_single(<<~SQL)
     WITH upl AS (
       SELECT DISTINCT ON (upload_id) upload_id
       FROM upload_references
       INNER JOIN posts ON posts.id = upload_references.target_id AND upload_references.target_type = 'Post'
       INNER JOIN topics ON topics.id = posts.topic_id
-      LEFT JOIN categories ON categories.id = topics.category_id
-      WHERE (topics.category_id IS NOT NULL AND categories.read_restricted) OR
-        (topics.archetype = 'private_message')
+      #{filter_clause}
     )
     UPDATE uploads
     SET secure = true,

lib/topic_upload_security_manager.rb

@@ -29,8 +29,9 @@ def run
 
         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
+            # We already have the post preloaded so we may as well
             # attach it here to avoid another load in UploadSecurity
+            # (which is called via update_secure_status)
             upload.access_control_post = post
             upload.update_secure_status(source: "topic upload security")
           end
@@ -43,14 +44,14 @@ def run
 
     return if !SiteSetting.secure_uploads
 
-    # we only want to do this if secure uploads is enabled. if
+    # We only want to do this if secure uploads 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 uploads is enabled):
+    # Examples (all after secure uploads is enabled):
     #
     #  -> a public topic is moved to a private category after
     #  -> a PM is converted to a public topic

lib/upload_security.rb

@@ -163,7 +163,7 @@ def regular_emoji_check
   #### START PRIVATE CHECKS ####
 
   def login_required_check
-    SiteSetting.login_required?
+    SiteSetting.login_required? && !SiteSetting.secure_uploads_pm_only?
   end
 
   # Whether the upload should remain secure or not after posting depends on its context,

spec/lib/upload_security_spec.rb

@@ -28,6 +28,14 @@
           expect(security.should_be_secure?).to eq(true)
         end
 
+        context "if secure_uploads_pm_only" do
+          before { SiteSetting.secure_uploads_pm_only = true }
+
+          it "returns false" do
+            expect(security.should_be_secure?).to eq(false)
+          end
+        end
+
         context "when uploading in public context" do
           describe "for a public type badge_image" do
             let(:type) { "badge_image" }

spec/models/post_spec.rb

@@ -150,6 +150,7 @@ def post_with_body(body, user = nil)
   describe "with_secure_uploads?" do
     let(:topic) { Fabricate(:topic) }
     let!(:post) { Fabricate(:post, topic: topic) }
+
     it "returns false if secure uploads is not enabled" do
       expect(post.with_secure_uploads?).to eq(false)
     end
@@ -167,6 +168,14 @@ def post_with_body(body, user = nil)
         it "returns true" do
           expect(post.with_secure_uploads?).to eq(true)
         end
+
+        context "if secure_uploads_pm_only" do
+          before { SiteSetting.secure_uploads_pm_only = true }
+
+          it "returns false" do
+            expect(post.with_secure_uploads?).to eq(false)
+          end
+        end
       end
 
       context "if the topic category is read_restricted" do
@@ -176,6 +185,14 @@ def post_with_body(body, user = nil)
         it "returns true" do
           expect(post.with_secure_uploads?).to eq(true)
         end
+
+        context "if secure_uploads_pm_only" do
+          before { SiteSetting.secure_uploads_pm_only = true }
+
+          it "returns false" do
+            expect(post.with_secure_uploads?).to eq(false)
+          end
+        end
       end
 
       context "if the post is in a PM topic" do
@@ -184,6 +201,14 @@ def post_with_body(body, user = nil)
         it "returns true" do
           expect(post.with_secure_uploads?).to eq(true)
         end
+
+        context "if secure_uploads_pm_only" do
+          before { SiteSetting.secure_uploads_pm_only = true }
+
+          it "returns true" do
+            expect(post.with_secure_uploads?).to eq(true)
+          end
+        end
       end
     end
   end

spec/models/upload_spec.rb

@@ -491,16 +491,38 @@
         SiteSetting.login_required = true
         SiteSetting.authorized_extensions = ""
         expect do
-          upl =
-            Fabricate(
-              :upload,
-              access_control_post: Fabricate(:private_message_post),
-              security_last_changed_at: Time.zone.now,
-              security_last_changed_reason: "test",
-              secure: true,
-            )
+          Fabricate(
+            :upload,
+            access_control_post: Fabricate(:private_message_post),
+            security_last_changed_at: Time.zone.now,
+            security_last_changed_reason: "test",
+            secure: true,
+          )
         end.to raise_error(ActiveRecord::RecordInvalid)
       end
+
+      context "when secure_uploads_pm_only is true" do
+        before { SiteSetting.secure_uploads_pm_only = true }
+
+        it "does not mark an image upload as secure if login_required is enabled" do
+          SiteSetting.login_required = true
+          upload.update!(secure: false)
+          expect { upload.update_secure_status }.not_to change { upload.secure }
+          expect(upload.reload.secure).to eq(false)
+        end
+
+        it "marks the upload as not secure if its access control post is a public post" do
+          upload.update!(secure: true, access_control_post: Fabricate(:post))
+          upload.update_secure_status
+          expect(upload.secure).to eq(false)
+        end
+
+        it "leaves the upload as secure if its access control post is a PM post" do
+          upload.update!(secure: true, access_control_post: Fabricate(:private_message_post))
+          upload.update_secure_status
+          expect(upload.secure).to eq(true)
+        end
+      end
     end
   end
 

spec/support/system_helpers.rb

@@ -141,7 +141,7 @@ def select_text_range(selector, start = 0, offset = 5)
     page.execute_script(js, selector, start, offset)
   end
 
-  def setup_s3_system_test
+  def setup_s3_system_test(enable_secure_uploads: false, enable_direct_s3_uploads: true)
     SiteSetting.enable_s3_uploads = true
 
     SiteSetting.s3_upload_bucket = "discoursetest"
@@ -151,6 +151,9 @@ def setup_s3_system_test
     SiteSetting.s3_secret_access_key = MinioRunner.config.minio_root_password
     SiteSetting.s3_endpoint = MinioRunner.config.minio_server_url
 
+    SiteSetting.enable_direct_s3_uploads = enable_direct_s3_uploads
+    SiteSetting.secure_uploads = enable_secure_uploads
+
     MinioRunner.start
   end
 

spec/system/page_objects/components/composer.rb

@@ -82,8 +82,8 @@ def category_chooser
       end
 
       def switch_category(category_name)
-        find(".category-chooser").click
-        find(".category-row[data-name='#{category_name}']").click
+        category_chooser.expand
+        category_chooser.select_row_by_name(category_name)
       end
 
       def preview
@@ -216,6 +216,14 @@ def has_in_progress_uploads?
         find("#{COMPOSER_ID}").has_css?("#file-uploading")
       end
 
+      def select_pm_user(username)
+        select_kit = PageObjects::Components::SelectKit.new("#private-message-users")
+        select_kit.expand
+        select_kit.search(username)
+        select_kit.select_row_by_value(username)
+        select_kit.collapse
+      end
+
       private
 
       def emoji_preview_selector(emoji)

spec/system/page_objects/pages/topic.rb

@@ -16,8 +16,7 @@ def visit_topic(topic, post_number: nil)
       end
 
       def open_new_topic
-        page.visit "/"
-        find("button#create-topic").click
+        page.visit "/new-topic"
         self
       end
 
@@ -32,6 +31,14 @@ def visit_topic_and_open_composer(topic)
         self
       end
 
+      def current_topic_id
+        find("h1[data-topic-id]")["data-topic-id"]
+      end
+
+      def current_topic
+        ::Topic.find(current_topic_id)
+      end
+
       def has_topic_title?(text)
         has_css?("h1 .fancy-title", text: text)
       end
@@ -44,9 +51,9 @@ def has_post_number?(number)
         has_css?("#post_#{number}")
       end
 
-      def post_by_number(post_or_number)
+      def post_by_number(post_or_number, wait: Capybara.default_max_wait_time)
         post_or_number = post_or_number.is_a?(Post) ? post_or_number.post_number : post_or_number
-        find(".topic-post:not(.staged) #post_#{post_or_number}")
+        find(".topic-post:not(.staged) #post_#{post_or_number}", wait: wait)
       end
 
       def post_by_number_selector(post_number)