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

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

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

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}")
 

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)

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
 

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)')

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)

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

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
+
+        it "returns regular URL for ineligible private upload" do
+          SiteSetting.prevent_anons_from_downloading_files = true
+
+          s3_helper.expects(:s3_bucket).returns(s3_bucket).at_least_once
+          s3_bucket.expects(:object).with("original/1X/#{upload.sha1}.png").returns(s3_object).at_least_once
+
+          expect(store.store_upload(uploaded_file, upload)).to eq(
+            "//s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/original/1X/#{upload.sha1}.png"
+          )
+
+          expect(store.url_for(upload)).to eq(upload.url)
+        end
+      end
     end
 
     describe "#store_optimized_image" do
@@ -320,6 +351,33 @@ describe FileStore::S3Store do
     end
   end
 
+  context 'update ACL' do
+    include_context "s3 helpers"
+    let(:s3_object) { stub }
+
+    describe ".update_upload_ACL" do
+      it "sets acl to private when private uploads are enabled" do
+        SiteSetting.prevent_anons_from_downloading_files = true
+        s3_helper.expects(:s3_bucket).returns(s3_bucket)
+        s3_bucket.expects(:object).with("original/1X/#{upload.sha1}.png").returns(s3_object)
+        s3_object.expects(:acl).returns(s3_object)
+        s3_object.expects(:put).with(acl: "private").returns(s3_object)
+
+        expect(store.update_upload_ACL(upload)).to be_truthy
+      end
+
+      it "sets acl to public when private uploads are disabled" do
+        SiteSetting.prevent_anons_from_downloading_files = false
+        s3_helper.expects(:s3_bucket).returns(s3_bucket)
+        s3_bucket.expects(:object).with("original/1X/#{upload.sha1}.png").returns(s3_object)
+        s3_object.expects(:acl).returns(s3_object)
+        s3_object.expects(:put).with(acl: "public-read").returns(s3_object)
+
+        expect(store.update_upload_ACL(upload)).to be_truthy
+      end
+    end
+  end
+
   describe '.cdn_url' do
 
     it 'supports subfolder' do

spec/lib/backup_restore/s3_backup_store_spec.rb

@@ -131,7 +131,7 @@ describe BackupRestore::S3BackupStore do
     bucket = Regexp.escape(SiteSetting.s3_backup_bucket)
     prefix = file_prefix(db_name, multisite)
     filename = Regexp.escape(filename)
-    expires = BackupRestore::S3BackupStore::DOWNLOAD_URL_EXPIRES_AFTER_SECONDS
+    expires = S3Helper::DOWNLOAD_URL_EXPIRES_AFTER_SECONDS
 
     /\Ahttps:\/\/#{bucket}.*#{prefix}\/#{filename}\?.*X-Amz-Expires=#{expires}.*X-Amz-Signature=.*\z/
   end

spec/lib/upload_creator_spec.rb

@@ -170,9 +170,44 @@ RSpec.describe UploadCreator do
       end
     end
 
+    describe 'private uploads' do
+      let(:filename) { "small.pdf" }
+      let(:file) { file_from_fixtures(filename, "pdf") }
+
+      before do
+        SiteSetting.prevent_anons_from_downloading_files = true
+        SiteSetting.authorized_extensions = 'pdf|svg|jpg'
+      end
+
+      it 'should mark uploads as private' do
+        upload = UploadCreator.new(file, filename).create_for(user.id)
+        stored_upload = Upload.last
+
+        expect(stored_upload.private?).to eq(true)
+      end
+
+      it 'should not mark theme uploads as private' do
+        fname = "custom-theme-icon-sprite.svg"
+        upload = UploadCreator.new(file_from_fixtures(fname), fname, for_theme: true).create_for(-1)
+
+        expect(upload.private?).to eq(false)
+      end
+
+      it 'should not mark image uploads as private' do
+        fname = "logo.jpg"
+        upload = UploadCreator.new(file_from_fixtures(fname), fname).create_for(user.id)
+        stored_upload = Upload.last
+
+        expect(stored_upload.original_filename).to eq(fname)
+        expect(stored_upload.private?).to eq(false)
+      end
+    end
+
     describe 'uploading to s3' do
       let(:filename) { "should_be_jpeg.png" }
       let(:file) { file_from_fixtures(filename) }
+      let(:pdf_filename) { "small.pdf" }
+      let(:pdf_file) { file_from_fixtures(pdf_filename, "pdf") }
 
       before do
         SiteSetting.s3_upload_bucket = "s3-upload-bucket"
@@ -197,6 +232,19 @@ RSpec.describe UploadCreator do
 
         expect(upload.etag).to eq('ETag')
       end
+
+      it 'should return signed URL for private uploads in S3' do
+        SiteSetting.prevent_anons_from_downloading_files = true
+        SiteSetting.authorized_extensions = 'pdf'
+
+        upload = UploadCreator.new(pdf_file, pdf_filename).create_for(user.id)
+        stored_upload = Upload.last
+        signed_url = Discourse.store.url_for(stored_upload)
+
+        expect(stored_upload.private?).to eq(true)
+        expect(stored_upload.url).not_to eq(signed_url)
+        expect(signed_url).to match(/Amz-Credential/)
+      end
     end
   end
 end

spec/multisite/s3_store_spec.rb

@@ -118,4 +118,71 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
       end
     end
   end
+
+  context 'private uploads' do
+    let(:store) { FileStore::S3Store.new }
+    let(:client) { Aws::S3::Client.new(stub_responses: true) }
+    let(:resource) { Aws::S3::Resource.new(client: client) }
+    let(:s3_bucket) { resource.bucket("some-really-cool-bucket") }
+    let(:s3_helper) { store.instance_variable_get(:@s3_helper) }
+    let(:s3_object) { stub }
+
+    before(:each) do
+      SiteSetting.s3_upload_bucket = "some-really-cool-bucket"
+      SiteSetting.s3_access_key_id = "s3-access-key-id"
+      SiteSetting.s3_secret_access_key = "s3-secret-access-key"
+      SiteSetting.enable_s3_uploads = true
+      SiteSetting.prevent_anons_from_downloading_files = true
+    end
+
+    before do
+      s3_object.stubs(:put).returns(Aws::S3::Types::PutObjectOutput.new(etag: "etag"))
+    end
+
+    describe "when private uploads are enabled" do
+      it "returns signed URL with correct path" do
+        test_multisite_connection('default') do
+          SiteSetting.authorized_extensions = "pdf|png|jpg|gif"
+          upload = build_upload
+          upload.update!(original_filename: "small.pdf", extension: "pdf")
+
+          s3_helper.expects(:s3_bucket).returns(s3_bucket).at_least_once
+          s3_bucket.expects(:object).with("uploads/default/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(
+            "//some-really-cool-bucket.s3.dualstack.us-east-1.amazonaws.com/uploads/default/original/1X/#{upload.sha1}.pdf"
+          )
+
+          expect(store.url_for(upload)).not_to eq(upload.url)
+        end
+      end
+    end
+
+    describe "#update_upload_ACL" do
+      it "updates correct file for default and second multisite db" do
+        test_multisite_connection('default') do
+          upload = build_upload
+
+          s3_helper.expects(:s3_bucket).returns(s3_bucket).at_least_once
+          s3_bucket.expects(:object).with("uploads/default/original/1X/#{upload.sha1}.png").returns(s3_object)
+          s3_object.expects(:acl).returns(s3_object)
+          s3_object.expects(:put).with(acl: "private").returns(s3_object)
+
+          expect(store.update_upload_ACL(upload)).to be_truthy
+        end
+
+        test_multisite_connection('second') do
+          upload = build_upload
+
+          s3_helper.expects(:s3_bucket).returns(s3_bucket).at_least_once
+          s3_bucket.expects(:object).with("uploads/second/original/1X/#{upload.sha1}.png").returns(s3_object)
+          s3_object.expects(:acl).returns(s3_object)
+          s3_object.expects(:put).with(acl: "private").returns(s3_object)
+
+          expect(store.update_upload_ACL(upload)).to be_truthy
+        end
+      end
+    end
+  end
 end

spec/requests/uploads_controller_spec.rb

@@ -285,12 +285,11 @@ describe UploadsController do
 
     context "prevent anons from downloading files" do
       it "returns 404 when an anonymous user tries to download a file" do
-        skip("this only works when nginx/apache is asset server") if Discourse::Application.config.public_file_server.enabled
         upload = upload_file("small.pdf", "pdf")
         delete "/session/#{user.username}.json"
 
         SiteSetting.prevent_anons_from_downloading_files = true
-        get upload.url
+        get "/uploads/#{site}/#{upload.sha1}.#{upload.extension}"
         expect(response.status).to eq(404)
       end
     end