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).
This commit is contained in:
Martin Brennan 2021-01-29 09:03:44 +10:00 committed by GitHub
parent 0990112d80
commit f49e3e5731
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
13 changed files with 182 additions and 87 deletions

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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