From f00275ded37af233af31740656a64e01c4dff5e3 Mon Sep 17 00:00:00 2001 From: Penar Musaraj Date: Wed, 5 Jun 2019 23:27:24 -0400 Subject: [PATCH] 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 | 2 +- .../regular/update_private_uploads_acl.rb | 17 +++++ app/models/upload.rb | 5 ++ .../initializers/014-track-setting-changes.rb | 2 + lib/backup_restore/s3_backup_store.rb | 4 +- lib/file_store/s3_store.rb | 35 +++++++++- lib/s3_helper.rb | 2 + lib/tasks/uploads.rake | 4 ++ spec/components/file_store/s3_store_spec.rb | 58 ++++++++++++++++ .../backup_restore/s3_backup_store_spec.rb | 2 +- spec/lib/upload_creator_spec.rb | 48 +++++++++++++ spec/multisite/s3_store_spec.rb | 67 +++++++++++++++++++ spec/requests/uploads_controller_spec.rb | 3 +- 13 files changed, 240 insertions(+), 9 deletions(-) create mode 100644 app/jobs/regular/update_private_uploads_acl.rb diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 73bf3b674a5..8b7ddfae5fc 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 00000000000..80a6993dc7f --- /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 5244d1e9496..c25011a53fa 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 9720c1e7d88..effd7f0192d 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 4504314fa64..6425478c110 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 a23ccb2af56..4389ba3d051 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 6349fb9ba33..5c091f88b05 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 64c1a75f8ec..3c4dc29ef57 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 65d132b3b7d..0399efcedb8 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 + + 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 diff --git a/spec/lib/backup_restore/s3_backup_store_spec.rb b/spec/lib/backup_restore/s3_backup_store_spec.rb index 52251284cc9..312c396fa38 100644 --- a/spec/lib/backup_restore/s3_backup_store_spec.rb +++ b/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 diff --git a/spec/lib/upload_creator_spec.rb b/spec/lib/upload_creator_spec.rb index 1225873778f..5754a5c6cbe 100644 --- a/spec/lib/upload_creator_spec.rb +++ b/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 diff --git a/spec/multisite/s3_store_spec.rb b/spec/multisite/s3_store_spec.rb index 1e4634b8899..3c5451f89be 100644 --- a/spec/multisite/s3_store_spec.rb +++ b/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 diff --git a/spec/requests/uploads_controller_spec.rb b/spec/requests/uploads_controller_spec.rb index b1d60522187..10b5003a85d 100644 --- a/spec/requests/uploads_controller_spec.rb +++ b/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