From f49e3e5731ded58843fea0f693d26a36cd20f6a0 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 29 Jan 2021 09:03:44 +1000 Subject: [PATCH] DEV: Add security_last_changed_at and security_last_changed_reason to uploads (#11860) This PR adds security_last_changed_at and security_last_changed_reason to uploads. This has been done to make it easier to track down why an upload's secure column has changed and when. This necessitated a refactor of the UploadSecurity class to provide reasons why the upload security would have changed. As well as this, a source is now provided from the location which called for the upload's security status to be updated as they are several (e.g. post creator, topic security updater, rake tasks, manual change). --- app/models/post.rb | 4 +- app/models/topic.rb | 15 ++- app/models/upload.rb | 64 +++++---- ...7013637_add_upload_security_log_columns.rb | 8 ++ lib/post_creator.rb | 3 +- lib/tasks/uploads.rake | 18 ++- lib/topic_upload_security_manager.rb | 4 +- lib/upload_creator.rb | 8 +- lib/upload_security.rb | 125 +++++++++++++----- spec/components/email/sender_spec.rb | 2 +- spec/lib/upload_security_spec.rb | 2 +- spec/models/post_spec.rb | 10 +- spec/models/upload_spec.rb | 6 +- 13 files changed, 182 insertions(+), 87 deletions(-) create mode 100644 db/migrate/20210127013637_add_upload_security_log_columns.rb diff --git a/app/models/post.rb b/app/models/post.rb index 1ce6628a419..1b525a16413 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -951,9 +951,9 @@ class Post < ActiveRecord::Base end end - def update_uploads_secure_status + def update_uploads_secure_status(source:) if Discourse.store.external? - self.uploads.each { |upload| upload.update_secure_status } + self.uploads.each { |upload| upload.update_secure_status(source: source) } end end diff --git a/app/models/topic.rb b/app/models/topic.rb index a6cc367baf7..bed3e1b1003 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -865,6 +865,14 @@ class Topic < ActiveRecord::Base Jobs.enqueue(:notify_category_change, post_id: post.id, notified_user_ids: notified_user_ids) end end + + # when a topic changes category we may need to make uploads + # linked to posts secure/not secure depending on whether the + # category is private. this is only done if the category + # has actually changed to avoid noise. + DB.after_commit do + Jobs.enqueue(:update_topic_upload_security, topic_id: self.id) + end end Category.where(id: new_category.id).update_all("topic_count = topic_count + 1") @@ -873,13 +881,6 @@ class Topic < ActiveRecord::Base CategoryFeaturedTopic.feature_topics_for(old_category) unless @import_mode CategoryFeaturedTopic.feature_topics_for(new_category) unless @import_mode || old_category.try(:id) == new_category.id end - - # when a topic changes category we may need to make uploads - # linked to posts secure/not secure depending on whether the - # category is private - DB.after_commit do - Jobs.enqueue(:update_topic_upload_security, topic_id: self.id) - end end true diff --git a/app/models/upload.rb b/app/models/upload.rb index dac34616af3..be740f9e687 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -311,16 +311,30 @@ class Upload < ActiveRecord::Base self.posts.where("cooked LIKE '%/_optimized/%'").find_each(&:rebake!) end - def update_secure_status(secure_override_value: nil) - mark_secure = secure_override_value.nil? ? UploadSecurity.new(self).should_be_secure? : secure_override_value + def update_secure_status(source: "unknown", override: nil) + if override.nil? + mark_secure, reason = UploadSecurity.new(self).should_be_secure_with_reason + else + mark_secure = override + reason = "manually overridden" + end secure_status_did_change = self.secure? != mark_secure - self.update_column("secure", mark_secure) + self.update(secure_params(mark_secure, reason, source)) + Discourse.store.update_upload_ACL(self) if Discourse.store.external? secure_status_did_change end + def secure_params(secure, reason, source = "unknown") + { + secure: secure, + security_last_changed_reason: reason + " | source: #{source}", + security_last_changed_at: Time.zone.now + } + end + def self.migrate_to_new_scheme(limit: nil) problems = [] @@ -451,27 +465,29 @@ end # # Table name: uploads # -# id :integer not null, primary key -# user_id :integer not null -# original_filename :string not null -# filesize :integer not null -# width :integer -# height :integer -# url :string not null -# created_at :datetime not null -# updated_at :datetime not null -# sha1 :string(40) -# origin :string(1000) -# retain_hours :integer -# extension :string(10) -# thumbnail_width :integer -# thumbnail_height :integer -# etag :string -# secure :boolean default(FALSE), not null -# access_control_post_id :bigint -# original_sha1 :string -# verification_status :integer default(1), not null -# animated :boolean +# id :integer not null, primary key +# user_id :integer not null +# original_filename :string not null +# filesize :integer not null +# width :integer +# height :integer +# url :string not null +# created_at :datetime not null +# updated_at :datetime not null +# sha1 :string(40) +# origin :string(1000) +# retain_hours :integer +# extension :string(10) +# thumbnail_width :integer +# thumbnail_height :integer +# etag :string +# secure :boolean default(FALSE), not null +# access_control_post_id :bigint +# original_sha1 :string +# verification_status :integer default(1), not null +# animated :boolean +# security_last_changed_at :datetime +# security_last_changed_reason :string # # Indexes # diff --git a/db/migrate/20210127013637_add_upload_security_log_columns.rb b/db/migrate/20210127013637_add_upload_security_log_columns.rb new file mode 100644 index 00000000000..10818c8b2a6 --- /dev/null +++ b/db/migrate/20210127013637_add_upload_security_log_columns.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddUploadSecurityLogColumns < ActiveRecord::Migration[6.0] + def change + add_column :uploads, :security_last_changed_at, :datetime, null: true + add_column :uploads, :security_last_changed_reason, :string, null: true + end +end diff --git a/lib/post_creator.rb b/lib/post_creator.rb index 7884c225817..5b921119905 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -419,8 +419,7 @@ class PostCreator end def update_uploads_secure_status - return if !SiteSetting.secure_media? - @post.update_uploads_secure_status + @post.update_uploads_secure_status(source: "post creator") if SiteSetting.secure_media? end def delete_owned_bookmarks diff --git a/lib/tasks/uploads.rake b/lib/tasks/uploads.rake index fd1955f7276..d2ab4e82c8d 100644 --- a/lib/tasks/uploads.rake +++ b/lib/tasks/uploads.rake @@ -650,7 +650,11 @@ end def mark_all_as_secure_login_required(uploads_to_update) puts "Marking #{uploads_to_update.count} upload(s) as secure because login_required is true.", "" - uploads_to_update.update_all(secure: true) + uploads_to_update.update_all( + secure: true, + security_last_changed_at: Time.zone.now, + security_last_changed_reason: "upload security rake task all secure login required" + ) puts "Finished marking upload(s) as secure." end @@ -665,11 +669,19 @@ end def update_specific_upload_security_no_login_required(upload_ids_to_mark_as_secure, upload_ids_to_mark_as_not_secure) if upload_ids_to_mark_as_secure.any? puts "Marking #{upload_ids_to_mark_as_secure.length} uploads as secure because UploadSecurity determined them to be secure." - Upload.where(id: upload_ids_to_mark_as_secure).update_all(secure: true) + Upload.where(id: upload_ids_to_mark_as_secure).update_all( + secure: true, + security_last_changed_at: Time.zone.now, + security_last_changed_reason: "upload security rake task mark as secure" + ) end if upload_ids_to_mark_as_not_secure.any? puts "Marking #{upload_ids_to_mark_as_not_secure.length} uploads as not secure because UploadSecurity determined them to be not secure." - Upload.where(id: upload_ids_to_mark_as_not_secure).update_all(secure: false) + Upload.where(id: upload_ids_to_mark_as_not_secure).update_all( + secure: false, + security_last_changed_at: Time.zone.now, + security_last_changed_reason: "upload security rake task mark as not secure" + ) end puts "Finished updating upload security." end diff --git a/lib/topic_upload_security_manager.rb b/lib/topic_upload_security_manager.rb index 124219a4cb9..896b813d822 100644 --- a/lib/topic_upload_security_manager.rb +++ b/lib/topic_upload_security_manager.rb @@ -31,7 +31,7 @@ class TopicUploadSecurityManager # we have already got the post preloaded so we may as well # attach it here to avoid another load in UploadSecurity upload.access_control_post = post - upload.update_secure_status + upload.update_secure_status(source: "topic upload security") end post.rebake! if secure_status_did_change Rails.logger.debug("Security updated & rebake complete in topic #{@topic.id} - post #{post.id}") @@ -62,7 +62,7 @@ class TopicUploadSecurityManager first_post_upload_appeared_in = upload.post_uploads.first.post if first_post_upload_appeared_in == post upload.update(access_control_post: post) - upload.update_secure_status + upload.update_secure_status(source: "topic upload security") else false end diff --git a/lib/upload_creator.rb b/lib/upload_creator.rb index 3d8b0e8383d..98df85db338 100644 --- a/lib/upload_creator.rb +++ b/lib/upload_creator.rb @@ -130,6 +130,13 @@ class UploadCreator end add_metadata! + + if SiteSetting.secure_media + secure, reason = UploadSecurity.new(@upload, @opts.merge(creating: true)).should_be_secure_with_reason + attrs = @upload.secure_params(secure, reason, "upload creator") + @upload.assign_attributes(attrs) + end + return @upload unless @upload.save DiscourseEvent.trigger(:before_upload_creation, @file, is_image, @opts[:for_export]) @@ -424,7 +431,6 @@ class UploadCreator @upload.for_export = true if @opts[:for_export] @upload.for_site_setting = true if @opts[:for_site_setting] @upload.for_gravatar = true if @opts[:for_gravatar] - @upload.secure = UploadSecurity.new(@upload, @opts).should_be_secure? end private diff --git a/lib/upload_security.rb b/lib/upload_security.rb index 45724735544..9af270de917 100644 --- a/lib/upload_security.rb +++ b/lib/upload_security.rb @@ -5,14 +5,16 @@ # # Some of these flags checked (e.g. all of the for_X flags and the opts[:type]) # are only set when _initially uploading_ via UploadCreator and are not present -# when an upload already exists. +# when an upload already exists, these will only be checked when the @creating +# option is present. # # If the upload already exists the best way to figure out whether it should be # secure alongside the site settings is the access_control_post_id, because the # original post the upload is linked to has far more bearing on its security context # post-upload. If the access_control_post_id does not exist then we just rely # on the current secure? status, otherwise there would be a lot of additional -# complex queries and joins to perform. +# complex queries and joins to perform. Over time more of these specific +# queries will be implemented. class UploadSecurity @@custom_public_types = [] @@ -38,32 +40,52 @@ class UploadSecurity @upload = upload @opts = opts @upload_type = @opts[:type] + @creating = @opts[:creating] end def should_be_secure? - return false if !SiteSetting.secure_media? - return false if uploading_in_public_context? - uploading_in_secure_context? + should_be_secure_with_reason.first end - def uploading_in_public_context? - @upload.for_theme || - @upload.for_site_setting || - @upload.for_gravatar || - public_type? || - used_for_custom_emoji? || - based_on_regular_emoji? - end - - def uploading_in_secure_context? - return true if SiteSetting.login_required? - if @upload.access_control_post_id.present? - return access_control_post_has_secure_media? + def should_be_secure_with_reason + insecure_context_checks.each do |check, reason| + return [false, reason] if perform_check(check) end - uploading_in_composer? || @upload.for_private_message || @upload.for_group_message || @upload.secure? + secure_context_checks.each do |check, reason| + return [perform_check(check), reason] if priority_check?(check) + return [true, reason] if perform_check(check) + end + + [false, "no checks satisfied"] end - private + def secure_media_disabled_check + !SiteSetting.secure_media? + end + + def insecure_creation_for_modifiers_check + return false if !@creating + @upload.for_theme || @upload.for_site_setting || @upload.for_gravatar + end + + def public_type_check + PUBLIC_TYPES.include?(@upload_type) || @@custom_public_types.include?(@upload_type) + end + + def custom_emoji_check + @upload.id.present? && CustomEmoji.exists?(upload_id: @upload.id) + end + + def regular_emoji_check + return false if @upload.origin.blank? + uri = URI.parse(@upload.origin) + return true if Emoji.all.map(&:url).include?("#{uri.path}?#{uri.query}") + uri.path.include?("images/emoji") + end + + def login_required_check + SiteSetting.login_required? + end # 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. @@ -71,28 +93,59 @@ class UploadSecurity # 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 # - # if there is no access control post id and the upload is currently secure, we - # do not want to make it un-secure to avoid unintentionally exposing it - def access_control_post_has_secure_media? - @upload.access_control_post.with_secure_media? + # a post is with secure media if it is a private message or in a read restricted + # category + def access_control_post_has_secure_media_check + access_control_post&.with_secure_media? end - def public_type? - PUBLIC_TYPES.include?(@upload_type) || @@custom_public_types.include?(@upload_type) - end - - def uploading_in_composer? + def uploading_in_composer_check @upload_type == "composer" end - def used_for_custom_emoji? - @upload.id.present? && CustomEmoji.exists?(upload_id: @upload.id) + def secure_creation_for_modifiers_check + return false if !@creating + @upload.for_private_message || @upload.for_group_message end - def based_on_regular_emoji? - return false if @upload.origin.blank? - uri = URI.parse(@upload.origin) - return true if Emoji.all.map(&:url).include?("#{uri.path}?#{uri.query}") - uri.path.include?("images/emoji") + def already_secure_check + @upload.secure? + end + + private + + def access_control_post + @access_control_post ||= @upload.access_control_post_id.present? ? @upload.access_control_post : nil + end + + def insecure_context_checks + { + secure_media_disabled: "secure media is disabled", + insecure_creation_for_modifiers: "one or more creation for_modifiers was satisfied", + public_type: "upload is public type", + custom_emoji: "upload is used for custom emoji", + regular_emoji: "upload is used for regular emoji" + } + end + + def secure_context_checks + { + login_required: "login is required", + access_control_post_has_secure_media: "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" + } + end + + # the access control check is important because that is the truest indicator + # 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_media && access_control_post + end + + def perform_check(check) + send("#{check}_check") end end diff --git a/spec/components/email/sender_spec.rb b/spec/components/email/sender_spec.rb index a5b48929914..3f80e23b2a2 100644 --- a/spec/components/email/sender_spec.rb +++ b/spec/components/email/sender_spec.rb @@ -439,7 +439,7 @@ describe Email::Sender do @secure_image_file = file_from_fixtures("logo.png", "images") @secure_image = UploadCreator.new(@secure_image_file, "logo.png") .create_for(Discourse.system_user.id) - @secure_image.update_secure_status(secure_override_value: true) + @secure_image.update_secure_status(override: true) @secure_image.update(access_control_post_id: reply.id) reply.update(raw: reply.raw + "\n" + "#{UploadMarkdown.new(@secure_image).image_markdown}") reply.rebake! diff --git a/spec/lib/upload_security_spec.rb b/spec/lib/upload_security_spec.rb index 698e5b12759..12fced51630 100644 --- a/spec/lib/upload_security_spec.rb +++ b/spec/lib/upload_security_spec.rb @@ -7,7 +7,7 @@ RSpec.describe UploadSecurity do let(:post_in_secure_context) { Fabricate(:post, topic: Fabricate(:topic, category: private_category)) } fab!(:upload) { Fabricate(:upload) } let(:type) { nil } - let(:opts) { { type: type } } + let(:opts) { { type: type, creating: true } } subject { described_class.new(upload, opts) } context "when secure media is enabled" do diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index 5fa6c13a049..856d46426f4 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -1495,7 +1495,7 @@ describe Post do SiteSetting.secure_media = true post = Fabricate(:post, raw: raw, user: user, topic: Fabricate(:private_message_topic, user: user)) post.link_post_uploads - post.update_uploads_secure_status + post.update_uploads_secure_status(source: "test") expect(PostUpload.where(post: post).joins(:upload).pluck(:upload_id, :secure)).to contain_exactly( [attachment_upload.id, true], @@ -1507,7 +1507,7 @@ describe Post do SiteSetting.secure_media = false post = Fabricate(:post, raw: raw, user: user, topic: Fabricate(:private_message_topic, user: user)) post.link_post_uploads - post.update_uploads_secure_status + post.update_uploads_secure_status(source: "test") expect(PostUpload.where(post: post).joins(:upload).pluck(:upload_id, :secure)).to contain_exactly( [attachment_upload.id, false], @@ -1520,7 +1520,7 @@ describe Post do private_category = Fabricate(:private_category, group: Fabricate(:group)) post = Fabricate(:post, raw: raw, user: user, topic: Fabricate(:topic, user: user, category: private_category)) post.link_post_uploads - post.update_uploads_secure_status + post.update_uploads_secure_status(source: "test") expect(PostUpload.where(post: post).joins(:upload).pluck(:upload_id, :secure)).to contain_exactly( [attachment_upload.id, true], @@ -1531,11 +1531,11 @@ describe Post do it "does not mark an upload as secure if it has already been used in a public topic" do post = Fabricate(:post, raw: raw, user: user, topic: Fabricate(:topic, user: user)) post.link_post_uploads - post.update_uploads_secure_status + post.update_uploads_secure_status(source: "test") pm = Fabricate(:post, raw: raw, user: user, topic: Fabricate(:private_message_topic, user: user)) pm.link_post_uploads - pm.update_uploads_secure_status + pm.update_uploads_secure_status(source: "test") expect(PostUpload.where(post: pm).joins(:upload).pluck(:upload_id, :secure)).to contain_exactly( [attachment_upload.id, false], diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index bf7024c04e4..6613a29cb0c 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -343,14 +343,14 @@ describe Upload do end describe '.update_secure_status' do - it "respects the secure_override_value parameter if provided" do + it "respects the override parameter if provided" do upload.update!(secure: true) - upload.update_secure_status(secure_override_value: true) + upload.update_secure_status(override: true) expect(upload.secure).to eq(true) - upload.update_secure_status(secure_override_value: false) + upload.update_secure_status(override: false) expect(upload.secure).to eq(false) end