FIX: Change secure media to encompass attachments as well (#9271)

FIX: Change secure media to encompass attachments as well (#9271)

If the “secure media” site setting is enabled then ALL files uploaded to Discourse (images, video, audio, pdf, txt, zip etc. etc.) will follow the secure media rules. The “prevent anons from downloading files” setting will no longer have any bearing on upload security. Basically, the feature will more appropriately be called “secure uploads” instead of “secure media”.

This is being done because there are communities out there that would like all attachments and media to be secure based on category rules but still allow anonymous users to download attachments in public places, which is not possible in the current arrangement.

diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb
index ad72f99..ccbd762 100644
--- a/app/controllers/uploads_controller.rb
+++ b/app/controllers/uploads_controller.rb
@@ -3,7 +3,7 @@
 require "mini_mime"
 
 class UploadsController < ApplicationController
-  requires_login except: [:show, :show_short]
+  requires_login except: [:show, :show_short, :show_secure]
 
   skip_before_action :preload_json, :check_xhr, :redirect_to_login_if_required, only: [:show, :show_short, :show_secure]
   protect_from_forgery except: :show
@@ -130,6 +130,7 @@ class UploadsController < ApplicationController
     upload = Upload.find_by(sha1: sha1)
     return render_404 if upload.blank?
 
+    return render_404 if SiteSetting.prevent_anons_from_downloading_files && current_user.nil?
     return handle_secure_upload_request(upload, path_with_ext) if SiteSetting.secure_media?
 
     # we don't want to 404 here if secure media gets disabled
@@ -146,6 +147,8 @@ class UploadsController < ApplicationController
   def handle_secure_upload_request(upload, path_with_ext = nil)
     if upload.access_control_post_id.present?
       raise Discourse::InvalidAccess if !guardian.can_see?(upload.access_control_post)
+    else
+      return render_404 if current_user.nil?
     end
 
     # url_for figures out the full URL, handling multisite DBs,
diff --git a/app/jobs/regular/update_private_uploads_acl.rb b/app/jobs/regular/update_private_uploads_acl.rb
deleted file mode 100644
index 661f844..0000000
--- a/app/jobs/regular/update_private_uploads_acl.rb
+++ /dev/null
@@ -1,17 +0,0 @@
-# frozen_string_literal: true
-
-module Jobs
-  class UpdatePrivateUploadsAcl < ::Jobs::Base
-    # only runs when SiteSetting.prevent_anons_from_downloading_files is updated
-    def execute(args)
-      return if !SiteSetting.Upload.enable_s3_uploads
-
-      Upload.find_each do |upload|
-        if !FileHelper.is_supported_media?(upload.original_filename)
-          upload.update_secure_status
-        end
-      end
-    end
-
-  end
-end
diff --git a/config/initializers/014-track-setting-changes.rb b/config/initializers/014-track-setting-changes.rb
index 200f2c2..d3156d4 100644
--- a/config/initializers/014-track-setting-changes.rb
+++ b/config/initializers/014-track-setting-changes.rb
@@ -35,8 +35,6 @@ DiscourseEvent.on(:site_setting_changed) do |name, old_value, new_value|
 
   Jobs.enqueue(:update_s3_inventory) if [:enable_s3_inventory, :s3_upload_bucket].include?(name)
 
-  Jobs.enqueue(:update_private_uploads_acl) if name == :prevent_anons_from_downloading_files
-
   SvgSprite.expire_cache if name.to_s.include?("_icon")
 
   if SiteIconManager::WATCHED_SETTINGS.include?(name)
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 986905f..110d398 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -2079,7 +2079,7 @@ en:
     bootstrap_mode_min_users: "Minimum number of users required to disable bootstrap mode (set to 0 to disable)"
 
     prevent_anons_from_downloading_files: "Prevent anonymous users from downloading attachments. WARNING: this will prevent any non-image site assets posted as attachments from working."
-    secure_media: 'Limits access to media uploads (images, video, audio). If "login required" is enabled, only logged-in users can access media uploads. Otherwise, access will be limited only for media uploads in private messages. Note: S3 uploads must be enabled prior to enabling this setting.'
+    secure_media: 'Limits access to ALL uploads (images, video, audio, text, pdfs, zips, and others). If “login required” is enabled, only logged-in users can access uploads. Otherwise, access will be limited only for media uploads in private messages and private categories. WARNING: This setting is complex and requires deep administrative understanding. See <a target="_blank" href="https://meta.discourse.org/t/secure-media-uploads/140017">the secure media topic on Meta</a> for details.'
     slug_generation_method: "Choose a slug generation method. 'encoded' will generate percent encoding string. 'none' will disable slug at all."
 
     enable_emoji: "Enable emoji"
diff --git a/lib/post_creator.rb b/lib/post_creator.rb
index 39fe22c..fab3de1 100644
--- a/lib/post_creator.rb
+++ b/lib/post_creator.rb
@@ -379,9 +379,8 @@ class PostCreator
   end
 
   def update_uploads_secure_status
-    if SiteSetting.secure_media? || SiteSetting.prevent_anons_from_downloading_files?
-      @post.update_uploads_secure_status
-    end
+    return if !SiteSetting.secure_media?
+    @post.update_uploads_secure_status
   end
 
   def handle_spam
diff --git a/lib/tasks/s3.rake b/lib/tasks/s3.rake
index f1b42dd..b72b9e0 100644
--- a/lib/tasks/s3.rake
+++ b/lib/tasks/s3.rake
@@ -147,7 +147,6 @@ task 's3:correct_cachecontrol' => :environment do
 
   base_url = Discourse.store.absolute_base_url
 
-  acl = SiteSetting.prevent_anons_from_downloading_files ? 'private' : 'public-read'
   cache_control = 'max-age=31556952, public, immutable'
 
   objects = Upload.pluck(:id, :url).map { |array| array << :upload }
@@ -165,7 +164,7 @@ task 's3:correct_cachecontrol' => :environment do
         object = Discourse.store.s3_helper.object(key)
         object.copy_from(
           copy_source: "#{object.bucket_name}/#{object.key}",
-          acl: acl,
+          acl: "public-read",
           cache_control: cache_control,
           content_type: object.content_type,
           content_disposition: object.content_disposition,
diff --git a/lib/upload_security.rb b/lib/upload_security.rb
index 2c9d2f2..5ce74ec 100644
--- a/lib/upload_security.rb
+++ b/lib/upload_security.rb
@@ -23,7 +23,7 @@ class UploadSecurity
   def should_be_secure?
     return false if !SiteSetting.secure_media?
     return false if uploading_in_public_context?
-    (secure_attachment? || supported_media?) && uploading_in_secure_context?
+    uploading_in_secure_context?
   end
 
   private
@@ -32,14 +32,6 @@ class UploadSecurity
     @upload.for_theme || @upload.for_site_setting || @upload.for_gravatar || public_type? || used_for_custom_emoji? || based_on_regular_emoji?
   end
 
-  def supported_media?
-    FileHelper.is_supported_media?(@upload.original_filename)
-  end
-
-  def secure_attachment?
-    !supported_media? && SiteSetting.prevent_anons_from_downloading_files
-  end
-
   def uploading_in_secure_context?
     return true if SiteSetting.login_required?
     if @upload.access_control_post_id.present?
diff --git a/spec/jobs/regular/update_private_uploads_acl_spec.rb b/spec/jobs/regular/update_private_uploads_acl_spec.rb
deleted file mode 100644
index 9c51ef7..0000000
--- a/spec/jobs/regular/update_private_uploads_acl_spec.rb
+++ /dev/null
@@ -1,54 +0,0 @@
-# frozen_string_literal: true
-
-require 'rails_helper'
-
-describe Jobs::UpdatePrivateUploadsAcl do
-  let(:args) { [] }
-
-  before do
-    SiteSetting.authorized_extensions = "pdf"
-  end
-
-  describe '#execute' do
-    context "if not SiteSetting.Upload.enable_s3_uploads" do
-      before do
-        SiteSetting.Upload.stubs(:enable_s3_uploads).returns(false)
-      end
-      it "returns early and changes no uploads" do
-        Upload.expects(:find_each).never
-        subject.execute(args)
-      end
-    end
-    context "if SiteSetting.Upload.enable_s3_uploads" do
-      let!(:upload) { Fabricate(:upload_s3, extension: 'pdf', original_filename: "watchmen.pdf", secure: false) }
-      before do
-        SiteSetting.login_required = true
-        SiteSetting.prevent_anons_from_downloading_files = true
-        Discourse.stubs(:store).returns(stub(external?: false))
-        enable_s3_uploads([upload])
-        SiteSetting.secure_media = true
-      end
-
-      it "changes the upload to secure" do
-        subject.execute(args)
-        expect(upload.reload.secure).to eq(true)
-      end
-    end
-  end
-
-  def enable_s3_uploads(uploads)
-    SiteSetting.enable_s3_uploads = true

[... diff too long, it was truncated ...]

GitHub sha: 097851c1

This commit appears in #9271 which was merged by martin.

No need for caps here :slight_smile: