From 7a083daf270ea720eba3fef09fd54b7fb7d54b92 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 16 Apr 2024 14:10:25 +1000 Subject: [PATCH] Revert "FIX: Post uploads setting access_control_post_id unnecessarily (#26627)" (#26643) This reverts commit cdc8e9de1b8d269b7d1704b210462276042a8a38. It's made things worse internally and on meta. --- app/models/post.rb | 25 ++-------- app/models/upload_reference.rb | 4 -- spec/integration/secure_uploads_spec.rb | 2 - spec/lib/post_revisor_spec.rb | 3 +- spec/models/post_spec.rb | 61 +++++++------------------ 5 files changed, 22 insertions(+), 73 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index 1435d000337..7445c8de9c0 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -1093,28 +1093,11 @@ class Post < ActiveRecord::Base UploadReference.where(target: self).delete_all UploadReference.insert_all(upload_references) if upload_references.size > 0 - # NOTE: The access_control_post_id needs to be set whenever a post is linked to an upload - # when secure uploads is enabled because it is later used for UploadSecurity checks. - # The caveat here is that access_control_post_id should only be set if this post is - # the first time the upload is referenced, otherwise we can end up hijacking upload - # ownership from other posts. if SiteSetting.secure_uploads? - uploads_in_post = - Upload - .joins(:upload_references) - .includes(:upload_references) - .where(id: upload_ids, access_control_post_id: nil) - .where("uploads.id NOT IN (SELECT upload_id FROM custom_emojis)") - - access_control_will_change_upload_ids = - uploads_in_post.filter_map do |upl| - first_ref = upl.upload_references.min_by { |ur| [ur.created_at, ur.id] } - upl.id if first_ref.blank? || first_ref.target?(self) - end - - Upload.where(id: access_control_will_change_upload_ids).update_all( - access_control_post_id: self.id, - ) + Upload + .where(id: upload_ids, access_control_post_id: nil) + .where("id NOT IN (SELECT upload_id FROM custom_emojis)") + .update_all(access_control_post_id: self.id) end end end diff --git a/app/models/upload_reference.rb b/app/models/upload_reference.rb index 031f9aa5971..ea5401e1233 100644 --- a/app/models/upload_reference.rb +++ b/app/models/upload_reference.rb @@ -6,10 +6,6 @@ class UploadReference < ActiveRecord::Base delegate :to_markdown, to: :upload - def target?(target_to_check) - self.target_id == target_to_check.id && self.target_type == target_to_check.class.to_s - end - def self.ensure_exist!(upload_ids: [], target: nil, target_type: nil, target_id: nil) if !target && !(target_type && target_id) raise "target OR target_type and target_id are required" diff --git a/spec/integration/secure_uploads_spec.rb b/spec/integration/secure_uploads_spec.rb index cffa9d7000a..490d6dfd79c 100644 --- a/spec/integration/secure_uploads_spec.rb +++ b/spec/integration/secure_uploads_spec.rb @@ -40,7 +40,6 @@ describe "Secure uploads" do it "does not convert an upload to secure when it was first used in a site setting then in a post" do upload = create_upload - stub_presign_upload_get(upload) SiteSetting.favicon = upload expect(upload.reload.upload_references.count).to eq(1) create_post( @@ -56,7 +55,6 @@ describe "Secure uploads" do it "does not convert an upload to insecure when it was first used in a secure post then a site setting" do upload = create_upload - stub_presign_upload_get(upload) create_post( title: "Secure upload post", raw: "This is a new post ", diff --git a/spec/lib/post_revisor_spec.rb b/spec/lib/post_revisor_spec.rb index 52f33034aa5..ffff3bcfdc7 100644 --- a/spec/lib/post_revisor_spec.rb +++ b/spec/lib/post_revisor_spec.rb @@ -1542,8 +1542,7 @@ RSpec.describe PostRevisor do end context "with secure uploads uploads" do - let!(:image5) { Fabricate(:secure_upload, access_control_post: post) } - + let!(:image5) { Fabricate(:secure_upload) } before do Jobs.run_immediately! setup_s3 diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index dc76ad7cfcf..fe9648aa5d5 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -1732,58 +1732,31 @@ RSpec.describe Post do expect(post.reload.upload_references.pluck(:id)).to_not contain_exactly(post_uploads_ids) end - describe "setting access_control_post on linked uploads" do - fab!(:other_post) { Fabricate(:post) } - + context "when secure uploads is enabled" do before do setup_s3 SiteSetting.authorized_extensions = "pdf|png|jpg|csv" SiteSetting.secure_uploads = true - SiteSetting.login_required = true end - context "for uploads in the post which have a null access_control_post_id" do - before do - image_upload.update!(access_control_post_id: nil) - video_upload.update!(access_control_post_id: other_post.id) - audio_upload.update!(access_control_post_id: other_post.id) - end + it "sets the access_control_post_id on uploads in the post that don't already have the value set" do + other_post = Fabricate(:post) + video_upload.update(access_control_post_id: other_post.id) + audio_upload.update(access_control_post_id: other_post.id) - it "does nothing if secure uploads is disabled" do - SiteSetting.secure_uploads = false + post.link_post_uploads + + image_upload.reload + video_upload.reload + expect(image_upload.access_control_post_id).to eq(post.id) + expect(video_upload.access_control_post_id).not_to eq(post.id) + end + + context "for custom emoji" do + before { CustomEmoji.create(name: "meme", upload: image_upload) } + it "never sets an access control post because they should not be secure" do post.link_post_uploads - image_upload.reload - expect(image_upload.access_control_post_id).to eq(nil) - end - - it "sets access_control_post_id to the current post if it is the first time the upload is being referenced" do - post.link_post_uploads - expect(image_upload.reload.access_control_post_id).to eq(post.id) - end - - it "does not set access_control_post_id if the current post is not the first time the upload is being referenced" do - UploadReference.create!( - upload: image_upload, - target: Fabricate(:post), - created_at: 1.year.ago, - ) - post.link_post_uploads - expect(image_upload.reload.access_control_post_id).not_to eq(post.id) - end - - it "does not set access_control_post_id for uploads that already have one" do - post.link_post_uploads - expect(video_upload.access_control_post_id).not_to eq(post.id) - expect(audio_upload.access_control_post_id).not_to eq(post.id) - end - - context "for custom emoji" do - before { CustomEmoji.create(name: "meme", upload: image_upload) } - - it "never sets an access control post because they should not be secure" do - post.link_post_uploads - expect(image_upload.reload.access_control_post_id).to eq(nil) - end + expect(image_upload.reload.access_control_post_id).to eq(nil) end end end