Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Revert "FIX: Post uploads setting access_control_post_id unnecessarily (#26627)" (#26643) 

This reverts commit cdc8e9de1b.

It's made things worse internally and on meta.
This commit is contained in:
Martin Brennan 2024-04-16 14:10:25 +10:00 committed by GitHub
parent cdc8e9de1b
commit 7a083daf27
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 22 additions and 73 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
app/models/post.rb
View File

@ -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

4
app/models/upload_reference.rb
View File

@ -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"

2
spec/integration/secure_uploads_spec.rb
View File

@ -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 <img src=\"#{upload.url}\" />",

3
spec/lib/post_revisor_spec.rb
View File

@ -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

61
spec/models/post_spec.rb
View File

@ -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