diff --git a/app/models/post.rb b/app/models/post.rb index 7445c8de9c0..1435d000337 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -1093,11 +1093,28 @@ 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? - 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) + 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, + ) end end end diff --git a/app/models/upload_reference.rb b/app/models/upload_reference.rb index ea5401e1233..031f9aa5971 100644 --- a/app/models/upload_reference.rb +++ b/app/models/upload_reference.rb @@ -6,6 +6,10 @@ 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 490d6dfd79c..cffa9d7000a 100644 --- a/spec/integration/secure_uploads_spec.rb +++ b/spec/integration/secure_uploads_spec.rb @@ -40,6 +40,7 @@ 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( @@ -55,6 +56,7 @@ 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 ffff3bcfdc7..52f33034aa5 100644 --- a/spec/lib/post_revisor_spec.rb +++ b/spec/lib/post_revisor_spec.rb @@ -1542,7 +1542,8 @@ RSpec.describe PostRevisor do end context "with secure uploads uploads" do - let!(:image5) { Fabricate(:secure_upload) } + let!(:image5) { Fabricate(:secure_upload, access_control_post: post) } + before do Jobs.run_immediately! setup_s3 diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index fe9648aa5d5..dc76ad7cfcf 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -1732,31 +1732,58 @@ RSpec.describe Post do expect(post.reload.upload_references.pluck(:id)).to_not contain_exactly(post_uploads_ids) end - context "when secure uploads is enabled" do + describe "setting access_control_post on linked uploads" do + fab!(:other_post) { Fabricate(:post) } + before do setup_s3 SiteSetting.authorized_extensions = "pdf|png|jpg|csv" SiteSetting.secure_uploads = true + SiteSetting.login_required = true 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) + 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 - 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 + it "does nothing if secure uploads is disabled" do + SiteSetting.secure_uploads = false post.link_post_uploads - expect(image_upload.reload.access_control_post_id).to eq(nil) + 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 end end end