From 7be88bbe8a03e0ed2e31b05d3c745ac6a78c2ad2 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Mon, 10 Feb 2025 14:37:42 +1000
Subject: [PATCH] FIX: Improvements for uploads:disable_secure_uploads task
 (#31231)

This commit contains a couple of improvements for this
rake task.

* We no longer limit the uploads to only ones with Post
  upload references, it doesn't matter what the secure uploads
  are linked to, they should all be un-secured
* We now only get distinct uploads from the initial query,
  multiple upload references on the same upload caused
  double ups and confusing counts for the task
* We now also disable the secure_uploads_pm_only site
  setting at the same time
---
 lib/tasks/uploads.rake               |  8 ++----
 spec/fabricators/draft_fabricator.rb | 38 ++++++++++++++++++++++++++++
 spec/tasks/uploads_spec.rb           | 17 ++++++++-----
 3 files changed, 51 insertions(+), 12 deletions(-)
 create mode 100644 spec/fabricators/draft_fabricator.rb

diff --git a/lib/tasks/uploads.rake b/lib/tasks/uploads.rake
index 984588b85c5..ea7f48e9791 100644
--- a/lib/tasks/uploads.rake
+++ b/lib/tasks/uploads.rake
@@ -597,13 +597,9 @@ task "uploads:disable_secure_uploads" => :environment do
     puts "Disabling secure upload and resetting uploads to not secure in #{db}...", ""
 
     SiteSetting.secure_uploads = false
+    SiteSetting.secure_uploads_pm_only = false
 
-    secure_uploads =
-      Upload
-        .joins(:upload_references)
-        .where(upload_references: { target_type: "Post" })
-        .where(secure: true)
-
+    secure_uploads = Upload.where(secure: true).distinct
     secure_upload_count = secure_uploads.count
 
     puts "", "Marking #{secure_upload_count} uploads as not secure.", ""
diff --git a/spec/fabricators/draft_fabricator.rb b/spec/fabricators/draft_fabricator.rb
new file mode 100644
index 00000000000..ec3183b8b7b
--- /dev/null
+++ b/spec/fabricators/draft_fabricator.rb
@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+Fabricator(:draft) do
+  user { Fabricate(:user) }
+  transient :category_id
+  transient :topic
+  transient :post
+  transient :archetype
+  transient :tags
+  draft_key do |transients|
+    topic = transients[:topic] || transients[:post]&.topic
+
+    return "#{Draft::EXISTING_TOPIC}_#{topic.id}" if topic
+
+    if transients[:archetype] == "regular"
+      Draft::NEW_TOPIC
+    else
+      Draft::NEW_PRIVATE_MESSAGE
+    end
+  end
+  owner { SecureRandom.hex(10) }
+  revisions { sequence(:revisions) { |n| n } }
+  sequence { sequence(:sequence) { |n| n } }
+  data do |transients|
+    topic = transients[:topic] || transients[:post]&.topic
+
+    {
+      reply: "This is my really long draft",
+      action: topic.present? ? "reply" : "createTopic",
+      categoryId: transients[:category_id] || topic&.category_id,
+      tags: transients[:tags],
+      archetypeId: transients[:archetype],
+      metaData: nil,
+      composerTime: SecureRandom.random_number(10_000),
+      typingTime: SecureRandom.random_number(10_000),
+    }
+  end
+end
diff --git a/spec/tasks/uploads_spec.rb b/spec/tasks/uploads_spec.rb
index 5523b51ee86..02383df4722 100644
--- a/spec/tasks/uploads_spec.rb
+++ b/spec/tasks/uploads_spec.rb
@@ -197,7 +197,8 @@ RSpec.describe "tasks/uploads" do
       UploadReference.create(target: post_1, upload: upload_1)
       UploadReference.create(target: post_1, upload: upload_2)
       UploadReference.create(target: post_2, upload: upload_3)
-      UploadReference.create(target: post_2, upload: upload4)
+      UploadReference.create(target: post_2, upload: upload_4)
+      UploadReference.create(target: draft, upload: upload_6)
     end
 
     after do
@@ -206,14 +207,16 @@ RSpec.describe "tasks/uploads" do
       end
     end
 
-    let!(:uploads) { [upload_1, upload_2, upload_3, upload4, upload5] }
+    let!(:uploads) { [upload_1, upload_2, upload_3, upload_4, upload_5] }
     let(:post_1) { Fabricate(:post) }
     let(:post_2) { Fabricate(:post) }
+    let(:draft) { Fabricate(:draft) }
     let(:upload_1) { Fabricate(:upload_s3, secure: true, access_control_post: post_1) }
     let(:upload_2) { Fabricate(:upload_s3, secure: true, access_control_post: post_1) }
     let(:upload_3) { Fabricate(:upload_s3, secure: true, access_control_post: post_2) }
-    let(:upload4) { Fabricate(:upload_s3, secure: true, access_control_post: post_2) }
-    let(:upload5) { Fabricate(:upload_s3, secure: false) }
+    let(:upload_4) { Fabricate(:upload_s3, secure: true, access_control_post: post_2) }
+    let(:upload_5) { Fabricate(:upload_s3, secure: false) }
+    let(:upload_6) { Fabricate(:upload_s3, secure: true) }
 
     it "disables the secure upload setting" do
       invoke_task
@@ -222,7 +225,9 @@ RSpec.describe "tasks/uploads" do
 
     it "updates all secure uploads to secure: false" do
       invoke_task
-      [upload_1, upload_2, upload_3, upload4].each { |upl| expect(upl.reload.secure).to eq(false) }
+      [upload_1, upload_2, upload_3, upload_4, upload_6].each do |upl|
+        expect(upl.reload.secure).to eq(false)
+      end
     end
 
     it "sets the baked_version to NULL for affected posts" do
@@ -244,7 +249,7 @@ RSpec.describe "tasks/uploads" do
     it "updates the affected ACLs via the SyncAclsForUploads job" do
       invoke_task
       expect(Jobs::SyncAclsForUploads.jobs.last["args"][0]["upload_ids"]).to match_array(
-        [upload_1.id, upload_2.id, upload_3.id, upload4.id],
+        [upload_1.id, upload_2.id, upload_3.id, upload_4.id, upload_6.id],
       )
     end
   end