FEATURE: Support private attachments when using S3 storage (#7677)

FEATURE: Support private attachments when using S3 storage (#7677)

  • Support private uploads in S3
  • Use localStore for local avatars
  • Add job to update private upload ACL on S3
  • Test multisite paths
  • update ACL for private uploads in migrate_to_s3 task
diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb
index 73bf3b6..8b7ddfa 100644
--- a/app/controllers/uploads_controller.rb
+++ b/app/controllers/uploads_controller.rb
@@ -98,7 +98,7 @@ class UploadsController < ApplicationController
       if Discourse.store.internal?
         send_file_local_upload(upload)
       else
-        redirect_to upload.url
+        redirect_to Discourse.store.url_for(upload)
       end
     else
       render_404
diff --git a/app/jobs/regular/update_private_uploads_acl.rb b/app/jobs/regular/update_private_uploads_acl.rb
new file mode 100644
index 0000000..80a6993
--- /dev/null
+++ b/app/jobs/regular/update_private_uploads_acl.rb
@@ -0,0 +1,17 @@
+# 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.enable_s3_uploads
+
+      Upload.find_each do |upload|
+        if !FileHelper.is_supported_image?(upload.original_filename)
+          Discourse.store.update_upload_ACL(upload)
+        end
+      end
+    end
+
+  end
+end
diff --git a/app/models/upload.rb b/app/models/upload.rb
index 5244d1e..c25011a 100644
--- a/app/models/upload.rb
+++ b/app/models/upload.rb
@@ -145,6 +145,11 @@ class Upload < ActiveRecord::Base
     !(url =~ /^(https?:)?\/\//)
   end
 
+  def private?
+    return false if self.for_theme || self.for_site_setting
+    SiteSetting.prevent_anons_from_downloading_files && !FileHelper.is_supported_image?(self.original_filename)
+  end
+
   def fix_dimensions!
     return if !FileHelper.is_supported_image?("image.#{extension}")
 
diff --git a/config/initializers/014-track-setting-changes.rb b/config/initializers/014-track-setting-changes.rb
index 9720c1e..effd7f0 100644
--- a/config/initializers/014-track-setting-changes.rb
+++ b/config/initializers/014-track-setting-changes.rb
@@ -34,6 +34,8 @@ DiscourseEvent.on(:site_setting_changed) do |name, old_value, new_value|
 
   Jobs.enqueue(:update_s3_inventory) if [:s3_inventory, :s3_upload_bucket].include?(name)
 
+  Jobs.enqueue(:update_private_uploads_acl) if [:prevent_anons_from_downloading_files].include?(name)
+
   SvgSprite.expire_cache if name.to_s.include?("_icon")
 
   if SiteIconManager::WATCHED_SETTINGS.include?(name)
diff --git a/lib/backup_restore/s3_backup_store.rb b/lib/backup_restore/s3_backup_store.rb
index 4504314..6425478 100644
--- a/lib/backup_restore/s3_backup_store.rb
+++ b/lib/backup_restore/s3_backup_store.rb
@@ -5,7 +5,6 @@ require_dependency "s3_helper"
 
 module BackupRestore
   class S3BackupStore < BackupStore
-    DOWNLOAD_URL_EXPIRES_AFTER_SECONDS ||= 15
     UPLOAD_URL_EXPIRES_AFTER_SECONDS ||= 21_600 # 6 hours
     MULTISITE_PREFIX = "backups"
 
@@ -74,11 +73,12 @@ module BackupRestore
     end
 
     def create_file_from_object(obj, include_download_source = false)
+      expires = S3Helper::DOWNLOAD_URL_EXPIRES_AFTER_SECONDS
       BackupFile.new(
         filename: File.basename(obj.key),
         size: obj.size,
         last_modified: obj.last_modified,
-        source: include_download_source ? presigned_url(obj, :get, DOWNLOAD_URL_EXPIRES_AFTER_SECONDS) : nil
+        source: include_download_source ? presigned_url(obj, :get, expires) : nil
       )
     end
 
diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb
index a23ccb2..4389ba3 100644
--- a/lib/file_store/s3_store.rb
+++ b/lib/file_store/s3_store.rb
@@ -21,7 +21,7 @@ module FileStore
 
     def store_upload(file, upload, content_type = nil)
       path = get_path_for_upload(upload)
-      url, upload.etag = store_file(file, path, filename: upload.original_filename, content_type: content_type, cache_locally: true)
+      url, upload.etag = store_file(file, path, filename: upload.original_filename, content_type: content_type, cache_locally: true, private: upload.private?)
       url
     end
 
@@ -41,9 +41,8 @@ module FileStore
       filename = opts[:filename].presence || File.basename(path)
       # cache file locally when needed
       cache_file(file, File.basename(path)) if opts[:cache_locally]
-      # stored uploaded are public by default
       options = {
-        acl: "public-read",
+        acl: opts[:private] ? "private" : "public-read",
         content_type: opts[:content_type].presence || MiniMime.lookup_by_filename(filename)&.content_type
       }
       # add a "content disposition" header for "attachments"
@@ -105,6 +104,17 @@ module FileStore
       FileStore::LocalStore.new.path_for(upload) if url && url[/^\/[^\/]/]
     end
 
+    def url_for(upload)
+      if upload.private?
+        obj = @s3_helper.object(get_upload_key(upload))
+        url = obj.presigned_url(:get, expires_in: S3Helper::DOWNLOAD_URL_EXPIRES_AFTER_SECONDS)
+      else
+        url = upload.url
+      end
+
+      url
+    end
+
     def cdn_url(url)
       return url if SiteSetting.Upload.s3_cdn_url.blank?
       schema = url[/^(https?:)?\/\//, 1]
@@ -138,8 +148,27 @@ module FileStore
       end
     end
 
+    def update_upload_ACL(upload)
+      private_uploads = SiteSetting.prevent_anons_from_downloading_files
+      key = get_upload_key(upload)
+
+      begin
+        @s3_helper.object(key).acl.put(acl: private_uploads ? "private" : "public-read")
+      rescue Aws::S3::Errors::NoSuchKey
+        Rails.logger.warn("Could not update ACL on upload with key: '#{key}'. Upload is missing.")
+      end
+    end
+
     private
 
+    def get_upload_key(upload)
+      if Rails.configuration.multisite
+        File.join(upload_path, "/", get_path_for_upload(upload))
+      else
+        get_path_for_upload(upload)
+      end
+    end
+
     def list_missing(model, prefix)
       connection = ActiveRecord::Base.connection.raw_connection
       connection.exec('CREATE TEMP TABLE verified_ids(val integer PRIMARY KEY)')
diff --git a/lib/s3_helper.rb b/lib/s3_helper.rb
index 6349fb9..5c091f8 100644
--- a/lib/s3_helper.rb
+++ b/lib/s3_helper.rb
@@ -8,6 +8,8 @@ class S3Helper
 
   attr_reader :s3_bucket_name, :s3_bucket_folder_path
 
+  DOWNLOAD_URL_EXPIRES_AFTER_SECONDS ||= 15
+
   def initialize(s3_bucket_name, tombstone_prefix = '', options = {})
     @s3_client = options.delete(:client)
     @s3_options = default_s3_options.merge(options)
diff --git a/lib/tasks/uploads.rake b/lib/tasks/uploads.rake
index 64c1a75..3c4dc29 100644
--- a/lib/tasks/uploads.rake
+++ b/lib/tasks/uploads.rake
@@ -425,6 +425,10 @@ def migrate_to_s3
         options[:content_disposition] =
           %Q{attachment; filename="#{upload.original_filename}"}
       end
+
+      if upload&.private?
+        options[:acl] = "private"
+      end
     end
 
     etag ||= Digest::MD5.file(path).hexdigest
diff --git a/spec/components/file_store/s3_store_spec.rb b/spec/components/file_store/s3_store_spec.rb
index 65d132b..0399efc 100644
--- a/spec/components/file_store/s3_store_spec.rb
+++ b/spec/components/file_store/s3_store_spec.rb
@@ -77,6 +77,37 @@ describe FileStore::S3Store do
           expect(upload.etag).to eq(etag)
         end
       end
+
+      describe "when private uploads are enabled" do
+        it "returns signed URL for eligible private upload" do
+          SiteSetting.prevent_anons_from_downloading_files = true
+          SiteSetting.authorized_extensions = "pdf|png|jpg|gif"
+          upload.update!(original_filename: "small.pdf", extension: "pdf")
+
+          s3_helper.expects(:s3_bucket).returns(s3_bucket).at_least_once
+          s3_bucket.expects(:object).with("original/1X/#{upload.sha1}.pdf").returns(s3_object).at_least_once
+          s3_object.expects(:presigned_url).with(:get, expires_in: S3Helper::DOWNLOAD_URL_EXPIRES_AFTER_SECONDS)
+
+          expect(store.store_upload(uploaded_file, upload)).to eq(
+            "//s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/original/1X/#{upload.sha1}.pdf"
+          )
+
+          expect(store.url_for(upload)).not_to eq(upload.url)
+        end
+

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

GitHub sha: f00275de

2 Likes