From 0d0dbd391a9473ff09d9b781eb503427d3a1f2be Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 9 Apr 2024 13:23:11 +1000 Subject: [PATCH] DEV: Rename with_secure_uploads? to should_secure_uploads? on Post (#26549) This method name is a bit confusing; with_secure_uploads implies it may return a block or something with the uploads of the post, and has_secure_uploads implies that it's checking whether the post is linked to any secure uploads. should_secure_uploads? communicates the true intent of this method -- which is to say whether uploads attached to this post should be secure or not. --- app/models/post.rb | 4 ++-- lib/cooked_post_processor.rb | 14 +++++++------- lib/pretty_text.rb | 2 +- lib/tasks/uploads.rake | 2 +- lib/upload_security.rb | 12 ++++++------ plugins/chat/lib/chat/message_processor.rb | 2 +- spec/lib/cooked_post_processor_spec.rb | 2 +- spec/models/post_spec.rb | 20 ++++++++++---------- 8 files changed, 29 insertions(+), 29 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index 5b4b1d54757..1c7ce99d334 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -326,7 +326,7 @@ class Post < ActiveRecord::Base options[:user_id] = self.last_editor_id options[:omit_nofollow] = true if omit_nofollow? - if self.with_secure_uploads? + if self.should_secure_uploads? each_upload_url do |url| uri = URI.parse(url) if FileHelper.is_supported_media?(File.basename(uri.path)) @@ -565,7 +565,7 @@ class Post < ActiveRecord::Base ReviewableFlaggedPost.pending.find_by(target: self) end - def with_secure_uploads? + def should_secure_uploads? return false if !SiteSetting.secure_uploads? topic_including_deleted = Topic.with_deleted.find_by(id: self.topic_id) return false if topic_including_deleted.blank? diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb index b82995e855e..15158109dbf 100644 --- a/lib/cooked_post_processor.rb +++ b/lib/cooked_post_processor.rb @@ -22,7 +22,7 @@ class CookedPostProcessor @cooking_options = post.cooking_options || opts[:cooking_options] || {} @cooking_options[:topic_id] = post.topic_id @cooking_options = @cooking_options.symbolize_keys - @with_secure_uploads = @post.with_secure_uploads? + @should_secure_uploads = @post.should_secure_uploads? @category_id = @post&.topic&.category_id cooked = post.cook(post.raw, @cooking_options) @@ -243,16 +243,16 @@ class CookedPostProcessor resized_h = (h * ratio).to_i if !cropped && upload.width && resized_w > upload.width - cooked_url = UrlHelper.cook_url(upload.url, secure: @post.with_secure_uploads?) + cooked_url = UrlHelper.cook_url(upload.url, secure: @should_secure_uploads) srcset << ", #{cooked_url} #{ratio.to_s.sub(/\.0\z/, "")}x" elsif t = upload.thumbnail(resized_w, resized_h) - cooked_url = UrlHelper.cook_url(t.url, secure: @post.with_secure_uploads?) + cooked_url = UrlHelper.cook_url(t.url, secure: @should_secure_uploads) srcset << ", #{cooked_url} #{ratio.to_s.sub(/\.0\z/, "")}x" end img[ "srcset" - ] = "#{UrlHelper.cook_url(img["src"], secure: @post.with_secure_uploads?)}#{srcset}" if srcset.present? + ] = "#{UrlHelper.cook_url(img["src"], secure: @should_secure_uploads)}#{srcset}" if srcset.present? end end else @@ -273,7 +273,7 @@ class CookedPostProcessor # then, the link to our larger image src_url = Upload.secure_uploads_url?(img["src"]) ? upload&.url : img["src"] - src = UrlHelper.cook_url(src_url || img["src"], secure: @post.with_secure_uploads?) + src = UrlHelper.cook_url(src_url || img["src"], secure: @should_secure_uploads) a = create_link_node("lightbox", src) img.add_next_sibling(a) @@ -348,7 +348,7 @@ class CookedPostProcessor custom_emoji = img["class"]&.include?("emoji-custom") && Emoji.custom?(img["title"]) img[selector] = UrlHelper.cook_url( img[selector].to_s, - secure: @post.with_secure_uploads? && !custom_emoji, + secure: @should_secure_uploads && !custom_emoji, ) end end @@ -415,7 +415,7 @@ class CookedPostProcessor still_an_image = false elsif info&.downloaded? && upload = info&.upload - img["src"] = UrlHelper.cook_url(upload.url, secure: @with_secure_uploads) + img["src"] = UrlHelper.cook_url(upload.url, secure: @should_secure_uploads) img["data-dominant-color"] = upload.dominant_color(calculate_if_missing: true).presence img.delete(PrettyText::BLOCKED_HOTLINKED_SRC_ATTR) end diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb index aed31e1a912..dc7520d50ad 100644 --- a/lib/pretty_text.rb +++ b/lib/pretty_text.rb @@ -653,7 +653,7 @@ module PrettyText def self.format_for_email(html, post = nil) doc = Nokogiri::HTML5.fragment(html) DiscourseEvent.trigger(:reduce_cooked, doc, post) - strip_secure_uploads(doc) if post&.with_secure_uploads? + strip_secure_uploads(doc) if post&.should_secure_uploads? strip_image_wrapping(doc) convert_vimeo_iframes(doc) make_all_links_absolute(doc) diff --git a/lib/tasks/uploads.rake b/lib/tasks/uploads.rake index 3e869e54cf7..329936dd15c 100644 --- a/lib/tasks/uploads.rake +++ b/lib/tasks/uploads.rake @@ -604,7 +604,7 @@ task "uploads:secure_upload_analyse_and_update" => :environment do # If secure upload is enabled we need to first set the access control post of # all post uploads (even uploads that are linked to multiple posts). If the # upload is not set to secure upload then this has no other effect on the upload, - # but we _must_ know what the access control post is because the with_secure_uploads? + # but we _must_ know what the access control post is because the should_secure_uploads? # method is on the post, and this knows about the category security & PM status update_uploads_access_control_post if SiteSetting.secure_uploads? diff --git a/lib/upload_security.rb b/lib/upload_security.rb index 4f97bdd797d..baf387450f9 100644 --- a/lib/upload_security.rb +++ b/lib/upload_security.rb @@ -101,7 +101,7 @@ class UploadSecurity def secure_context_checks { login_required: "login is required", - access_control_post_has_secure_uploads: "access control post dictates security", + access_control_post_should_secure_uploads: "access control post dictates security", secure_creation_for_modifiers: "one or more creation for_modifiers was satisfied", uploading_in_composer: "uploading via the composer", already_secure: "upload is already secure", @@ -112,7 +112,7 @@ class UploadSecurity # of whether an upload should be secure or not, and thus should be returned # immediately if there is an access control post. def priority_check?(check) - check == :access_control_post_has_secure_uploads && access_control_post + check == :access_control_post_should_secure_uploads && access_control_post end def perform_check(check) @@ -169,15 +169,15 @@ class UploadSecurity # Whether the upload should remain secure or not after posting depends on its context, # which is based on the post it is linked to via access_control_post_id. # - # If that post is with_secure_uploads? then the upload should also be secure. + # If that post should_secure_uploads? then the upload should also be secure. # # This may change to false if the upload was set to secure on upload e.g. in # a post composer then it turned out that the post itself was not in a secure context. # # A post is with secure uploads if it is a private message or in a read restricted - # category. See `Post#with_secure_uploads?` for the full definition. - def access_control_post_has_secure_uploads_check - access_control_post&.with_secure_uploads? + # category. See `Post#should_secure_uploads?` for the full definition. + def access_control_post_should_secure_uploads_check + access_control_post&.should_secure_uploads? end def uploading_in_composer_check diff --git a/plugins/chat/lib/chat/message_processor.rb b/plugins/chat/lib/chat/message_processor.rb index 74658ffde99..423820dcc50 100644 --- a/plugins/chat/lib/chat/message_processor.rb +++ b/plugins/chat/lib/chat/message_processor.rb @@ -8,7 +8,7 @@ module Chat def initialize(chat_message, opts = {}) @model = chat_message @previous_cooked = (chat_message.cooked || "").dup - @with_secure_uploads = false + @should_secure_uploads = false @size_cache = {} @opts = opts diff --git a/spec/lib/cooked_post_processor_spec.rb b/spec/lib/cooked_post_processor_spec.rb index f13fe03585f..285c613d54e 100644 --- a/spec/lib/cooked_post_processor_spec.rb +++ b/spec/lib/cooked_post_processor_spec.rb @@ -1160,7 +1160,7 @@ RSpec.describe CookedPostProcessor do Oneboxer.unstub(:onebox) end - context "when the post is with_secure_uploads and the upload is secure and secure uploads is enabled" do + context "when the post is should_secure_uploads and the upload is secure and secure uploads is enabled" do before do setup_s3 upload.update(secure: true) diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index 91a54e778e5..fe9648aa5d5 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -148,12 +148,12 @@ RSpec.describe Post do end end - describe "with_secure_uploads?" do + describe "should_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) + expect(post.should_secure_uploads?).to eq(false) end context "when secure uploads is enabled" do @@ -167,14 +167,14 @@ RSpec.describe Post do before { SiteSetting.login_required = true } it "returns true" do - expect(post.with_secure_uploads?).to eq(true) + expect(post.should_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) + expect(post.should_secure_uploads?).to eq(false) end end end @@ -184,7 +184,7 @@ RSpec.describe Post do before { topic.change_category_to_id(category.id) } it "returns true" do - expect(post.with_secure_uploads?).to eq(true) + expect(post.should_secure_uploads?).to eq(true) end context "when the topic is deleted" do @@ -194,7 +194,7 @@ RSpec.describe Post do end it "returns true" do - expect(post.with_secure_uploads?).to eq(true) + expect(post.should_secure_uploads?).to eq(true) end end @@ -202,7 +202,7 @@ RSpec.describe Post do before { SiteSetting.secure_uploads_pm_only = true } it "returns false" do - expect(post.with_secure_uploads?).to eq(false) + expect(post.should_secure_uploads?).to eq(false) end end end @@ -211,14 +211,14 @@ RSpec.describe Post do let(:topic) { Fabricate(:private_message_topic) } it "returns true" do - expect(post.with_secure_uploads?).to eq(true) + expect(post.should_secure_uploads?).to eq(true) end context "when the topic is deleted" do before { topic.trash! } it "returns true" do - expect(post.with_secure_uploads?).to eq(true) + expect(post.should_secure_uploads?).to eq(true) end end @@ -226,7 +226,7 @@ RSpec.describe Post do before { SiteSetting.secure_uploads_pm_only = true } it "returns true" do - expect(post.with_secure_uploads?).to eq(true) + expect(post.should_secure_uploads?).to eq(true) end end end