diff --git a/config/site_settings.yml b/config/site_settings.yml index f501aae40ce..9b9ae8e3c74 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1013,7 +1013,7 @@ backups: shadowed_by_global: true s3_backup_bucket: default: '' - regex: "^[a-z0-9-]+$" # can't use '.' when using HTTPS + regex: '^[a-z0-9\-\/]+$' # can't use '.' when using HTTPS shadowed_by_global: true s3_disable_cleanup: default: false diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb index 8b834ec323c..51342ad31ac 100644 --- a/lib/file_store/s3_store.rb +++ b/lib/file_store/s3_store.rb @@ -7,33 +7,21 @@ require_dependency "file_helper" module FileStore class S3Store < BaseStore - attr_reader :s3_bucket, :s3_bucket_folder_path + attr_reader :s3_bucket TOMBSTONE_PREFIX ||= "tombstone/" def initialize(s3_helper=nil) - @s3_bucket, @s3_bucket_folder_path = begin - raise Discourse::SiteSettingMissing.new("s3_upload_bucket") if SiteSetting.s3_upload_bucket.blank? - SiteSetting.s3_upload_bucket.downcase.split("/".freeze, 2) - end - - tombstone_prefix = - if @s3_bucket_folder_path - File.join(@s3_bucket_folder_path, TOMBSTONE_PREFIX) - else - TOMBSTONE_PREFIX - end - - @s3_helper = s3_helper || S3Helper.new(s3_bucket, tombstone_prefix) + @s3_helper = s3_helper || S3Helper.new(s3_bucket, TOMBSTONE_PREFIX) end def store_upload(file, upload, content_type = nil) - path = get_path_for_s3_upload(get_path_for_upload(upload)) + path = get_path_for_upload(upload) store_file(file, path, filename: upload.original_filename, content_type: content_type, cache_locally: true) end def store_optimized_image(file, optimized_image) - path = get_path_for_s3_upload(get_path_for_optimized_image(optimized_image)) + path = get_path_for_optimized_image(optimized_image) store_file(file, path) end @@ -53,7 +41,7 @@ module FileStore # add a "content type" header when provided options[:content_type] = content_type if content_type # if this fails, it will throw an exception - @s3_helper.upload(file, path, options) + path = @s3_helper.upload(file, path, options) # return the upload url "#{absolute_base_url}/#{path}" end @@ -61,7 +49,7 @@ module FileStore def remove_file(url, path) return unless has_been_uploaded?(url) # copy the removed file to tombstone - @s3_helper.remove(get_path_for_s3_upload(path), path) + @s3_helper.remove(path, true) end def has_been_uploaded?(url) @@ -76,13 +64,15 @@ module FileStore end def absolute_base_url + bucket = s3_bucket.split("/".freeze, 2).first + # cf. http://docs.aws.amazon.com/general/latest/gr/rande.html#s3_region @absolute_base_url ||= if SiteSetting.s3_region == "us-east-1" - "//#{s3_bucket}.s3.amazonaws.com" + "//#{bucket}.s3.amazonaws.com" elsif SiteSetting.s3_region == 'cn-north-1' - "//#{s3_bucket}.s3.cn-north-1.amazonaws.com.cn" + "//#{bucket}.s3.cn-north-1.amazonaws.com.cn" else - "//#{s3_bucket}.s3-#{SiteSetting.s3_region}.amazonaws.com" + "//#{bucket}.s3-#{SiteSetting.s3_region}.amazonaws.com" end end @@ -115,9 +105,11 @@ module FileStore UserAvatar.external_avatar_url(user_id, avatar.upload_id, avatar.width) end - def get_path_for_s3_upload(path) - path = File.join(@s3_bucket_folder_path, path) if @s3_bucket_folder_path - path + def s3_bucket + @s3_bucket ||= begin + raise Discourse::SiteSettingMissing.new("s3_upload_bucket") if SiteSetting.s3_upload_bucket.blank? + SiteSetting.s3_upload_bucket.downcase + end end end diff --git a/lib/s3_helper.rb b/lib/s3_helper.rb index bc35c0df016..fec256bd4c4 100644 --- a/lib/s3_helper.rb +++ b/lib/s3_helper.rb @@ -2,32 +2,41 @@ require "aws-sdk" class S3Helper - def initialize(s3_bucket, tombstone_prefix=nil) - raise Discourse::InvalidParameters.new("s3_bucket") if s3_bucket.blank? + def initialize(s3_upload_bucket, tombstone_prefix='') + @s3_bucket, @s3_bucket_folder_path = begin + raise Discourse::InvalidParameters.new("s3_bucket") if s3_upload_bucket.blank? + s3_upload_bucket.downcase.split("/".freeze, 2) + end - @s3_bucket = s3_bucket - @tombstone_prefix = tombstone_prefix + @tombstone_prefix = + if @s3_bucket_folder_path + File.join(@s3_bucket_folder_path, tombstone_prefix) + else + tombstone_prefix + end check_missing_site_settings end def upload(file, path, options={}) + path = get_path_for_s3_upload(path) obj = s3_bucket.object(path) obj.upload_file(file, options) + path end - def remove(s3_filename, tombstone_filename=false) + def remove(s3_filename, copy_to_tombstone=false) bucket = s3_bucket # copy the file in tombstone - if tombstone_filename && @tombstone_prefix.present? + if copy_to_tombstone && @tombstone_prefix.present? bucket - .object(File.join(@tombstone_prefix, tombstone_filename)) - .copy_from(copy_source: File.join(@s3_bucket, s3_filename)) + .object(File.join(@tombstone_prefix, s3_filename)) + .copy_from(copy_source: File.join(@s3_bucket, get_path_for_s3_upload(s3_filename))) end # delete the file - bucket.object(s3_filename).delete + bucket.object(get_path_for_s3_upload(s3_filename)).delete rescue Aws::S3::Errors::NoSuchKey end @@ -52,28 +61,32 @@ class S3Helper private - def s3_resource - opts = { region: SiteSetting.s3_region } + def get_path_for_s3_upload(path) + path = File.join(@s3_bucket_folder_path, path) if @s3_bucket_folder_path + path + end - unless SiteSetting.s3_use_iam_profile - opts[:access_key_id] = SiteSetting.s3_access_key_id - opts[:secret_access_key] = SiteSetting.s3_secret_access_key - end + def s3_resource + opts = { region: SiteSetting.s3_region } - Aws::S3::Resource.new(opts) + unless SiteSetting.s3_use_iam_profile + opts[:access_key_id] = SiteSetting.s3_access_key_id + opts[:secret_access_key] = SiteSetting.s3_secret_access_key end - def s3_bucket - bucket = s3_resource.bucket(@s3_bucket) - bucket.create unless bucket.exists? - bucket - end + Aws::S3::Resource.new(opts) + end - def check_missing_site_settings - unless SiteSetting.s3_use_iam_profile - raise Discourse::SiteSettingMissing.new("s3_access_key_id") if SiteSetting.s3_access_key_id.blank? - raise Discourse::SiteSettingMissing.new("s3_secret_access_key") if SiteSetting.s3_secret_access_key.blank? - end - end + def s3_bucket + bucket = s3_resource.bucket(@s3_bucket) + bucket.create unless bucket.exists? + bucket + end + def check_missing_site_settings + unless SiteSetting.s3_use_iam_profile + raise Discourse::SiteSettingMissing.new("s3_access_key_id") if SiteSetting.s3_access_key_id.blank? + raise Discourse::SiteSettingMissing.new("s3_secret_access_key") if SiteSetting.s3_secret_access_key.blank? + end + end end diff --git a/spec/components/file_store/s3_store_spec.rb b/spec/components/file_store/s3_store_spec.rb index 57df6e4c6f2..49ad6af2c53 100644 --- a/spec/components/file_store/s3_store_spec.rb +++ b/spec/components/file_store/s3_store_spec.rb @@ -20,6 +20,10 @@ describe FileStore::S3Store do end shared_context 's3 helpers' do + let(:upload) do + Fabricate(:upload, sha1: Digest::SHA1.hexdigest('secreet image string')) + end + let(:store) { FileStore::S3Store.new } let(:client) { Aws::S3::Client.new(stub_responses: true) } let(:resource) { Aws::S3::Resource.new(client: client) } @@ -36,6 +40,7 @@ describe FileStore::S3Store do describe "#store_upload" do it "returns an absolute schemaless url" do + store.expects(:get_depth_for).with(upload.id).returns(0) s3_helper.expects(:s3_bucket).returns(s3_bucket) s3_object = stub @@ -53,6 +58,7 @@ describe FileStore::S3Store do end it "returns an absolute schemaless url" do + store.expects(:get_depth_for).with(upload.id).returns(0) s3_helper.expects(:s3_bucket).returns(s3_bucket) s3_object = stub @@ -68,6 +74,7 @@ describe FileStore::S3Store do describe "#store_optimized_image" do it "returns an absolute schemaless url" do + store.expects(:get_depth_for).with(optimized_image.upload.id).returns(0) s3_helper.expects(:s3_bucket).returns(s3_bucket) s3_object = stub path = "optimized/1X/#{optimized_image.upload.sha1}_#{OptimizedImage::VERSION}_100x200.png" @@ -86,6 +93,7 @@ describe FileStore::S3Store do end it "returns an absolute schemaless url" do + store.expects(:get_depth_for).with(optimized_image.upload.id).returns(0) s3_helper.expects(:s3_bucket).returns(s3_bucket) s3_object = stub path = "discourse-uploads/optimized/1X/#{optimized_image.upload.sha1}_#{OptimizedImage::VERSION}_100x200.png" @@ -106,6 +114,7 @@ describe FileStore::S3Store do describe "#remove_upload" do it "removes the file from s3 with the right paths" do + store.expects(:get_depth_for).with(upload.id).returns(0) s3_helper.expects(:s3_bucket).returns(s3_bucket) upload.update_attributes!(url: "//s3-upload-bucket.s3-us-west-1.amazonaws.com/original/1X/#{upload.sha1}.png") s3_object = stub @@ -124,6 +133,7 @@ describe FileStore::S3Store do end it "removes the file from s3 with the right paths" do + store.expects(:get_depth_for).with(upload.id).returns(0) s3_helper.expects(:s3_bucket).returns(s3_bucket) upload.update_attributes!(url: "//s3-upload-bucket.s3-us-west-1.amazonaws.com/discourse-uploads/original/1X/#{upload.sha1}.png") s3_object = stub @@ -147,6 +157,7 @@ describe FileStore::S3Store do end it "removes the file from s3 with the right paths" do + store.expects(:get_depth_for).with(optimized_image.upload.id).returns(0) s3_helper.expects(:s3_bucket).returns(s3_bucket) s3_object = stub @@ -164,6 +175,7 @@ describe FileStore::S3Store do end it "removes the file from s3 with the right paths" do + store.expects(:get_depth_for).with(optimized_image.upload.id).returns(0) s3_helper.expects(:s3_bucket).returns(s3_bucket) s3_object = stub @@ -249,25 +261,5 @@ describe FileStore::S3Store do it "should return the right bucket name" do expect(store.s3_bucket).to eq('s3-upload-bucket') end - - it "should return the right bucket name when folders is included" do - SiteSetting.s3_upload_bucket = 's3-upload-bucket/this-site-upload-folder' - expect(store.s3_bucket).to eq('s3-upload-bucket') - end - end - - describe "s3_bucket_folder_path" do - context "when no folder path is specified" do - it "should return the right folder path" do - expect(store.s3_bucket_folder_path).to eq(nil) - end - end - - context "when site setting contains a folder path" do - it "should return the right folder path" do - SiteSetting.s3_upload_bucket = 's3-upload-bucket/this-site-upload-folder' - expect(store.s3_bucket_folder_path).to eq("this-site-upload-folder") - end - end end end diff --git a/spec/models/backup_spec.rb b/spec/models/backup_spec.rb index 14b4544e9f8..8d06582aa38 100644 --- a/spec/models/backup_spec.rb +++ b/spec/models/backup_spec.rb @@ -1,4 +1,5 @@ require 'rails_helper' +require "s3_helper" require_dependency 'backup' @@ -28,11 +29,58 @@ describe Backup do end end + shared_context 's3 helpers' do + let(:client) { Aws::S3::Client.new(stub_responses: true) } + let(:resource) { Aws::S3::Resource.new(client: client) } + let!(:s3_bucket) { resource.bucket("s3-upload-bucket") } + let(:s3_helper) { b1.s3 } + + before(:each) do + SiteSetting.s3_backup_bucket = "s3-upload-bucket" + SiteSetting.s3_access_key_id = "s3-access-key-id" + SiteSetting.s3_secret_access_key = "s3-secret-access-key" + end + end + context ".after_create_hook" do - it "calls upload_to_s3 if the SiteSetting is true" do - SiteSetting.enable_s3_backups = true - b1.expects(:upload_to_s3).once - b1.after_create_hook + context "when SiteSetting is true" do + include_context "s3 helpers" + + before do + SiteSetting.enable_s3_backups = true + end + + it "should upload the backup to S3 with the right paths" do + b1.path = 'some/path/backup.gz' + File.expects(:open).with(b1.path).yields(stub) + + s3_helper.expects(:s3_bucket).returns(s3_bucket) + s3_object = stub + + s3_bucket.expects(:object).with(b1.filename).returns(s3_object) + s3_object.expects(:upload_file) + + b1.after_create_hook + end + + context "when s3_backup_bucket includes folders path" do + before do + SiteSetting.s3_backup_bucket = "s3-upload-bucket/discourse-backups" + end + + it "should upload the backup to S3 with the right paths" do + b1.path = 'some/path/backup.gz' + File.expects(:open).with(b1.path).yields(stub) + + s3_helper.expects(:s3_bucket).returns(s3_bucket) + s3_object = stub + + s3_bucket.expects(:object).with("discourse-backups/#{b1.filename}").returns(s3_object) + s3_object.expects(:upload_file) + + b1.after_create_hook + end + end end it "calls upload_to_s3 if the SiteSetting is false" do @@ -43,10 +91,38 @@ describe Backup do end context ".after_remove_hook" do - it "calls remove_from_s3 if the SiteSetting is true" do - SiteSetting.enable_s3_backups = true - b1.expects(:remove_from_s3).once - b1.after_remove_hook + include_context "s3 helpers" + + context "when SiteSetting is true" do + before do + SiteSetting.enable_s3_backups = true + end + + it "should upload the backup to S3 with the right paths" do + s3_helper.expects(:s3_bucket).returns(s3_bucket) + s3_object = stub + + s3_bucket.expects(:object).with(b1.filename).returns(s3_object) + s3_object.expects(:delete) + + b1.after_remove_hook + end + + context "when s3_backup_bucket includes folders path" do + before do + SiteSetting.s3_backup_bucket = "s3-upload-bucket/discourse-backups" + end + + it "should upload the backup to S3 with the right paths" do + s3_helper.expects(:s3_bucket).returns(s3_bucket) + s3_object = stub + + s3_bucket.expects(:object).with("discourse-backups/#{b1.filename}").returns(s3_object) + s3_object.expects(:delete) + + b1.after_remove_hook + end + end end it "calls remove_from_s3 if the SiteSetting is false" do