diff --git a/config/site_settings.yml b/config/site_settings.yml
index 6c2a7d5def0..a0f37772f62 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -690,7 +690,7 @@ files:
enum: 'S3RegionSiteSetting'
s3_upload_bucket:
default: ''
- regex: "^[a-z0-9-]+$" # can't use '.' when using HTTPS
+ regex: '^[a-z0-9\-\/]+$' # can't use '.' when using HTTPS
s3_cdn_url:
default: ''
regex: '^https?:\/\/.+[^\/]$'
diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb
index 83dd2855976..8b834ec323c 100644
--- a/lib/file_store/s3_store.rb
+++ b/lib/file_store/s3_store.rb
@@ -7,18 +7,36 @@ require_dependency "file_helper"
module FileStore
class S3Store < BaseStore
+ attr_reader :s3_bucket, :s3_bucket_folder_path
TOMBSTONE_PREFIX ||= "tombstone/"
def initialize(s3_helper=nil)
- @s3_helper = s3_helper || S3Helper.new(s3_bucket, TOMBSTONE_PREFIX)
+ @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)
end
def store_upload(file, upload, content_type = nil)
- path = get_path_for_upload(upload)
+ path = get_path_for_s3_upload(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))
+ store_file(file, path)
+ end
+
# options
# - filename
# - content_type
@@ -43,7 +61,7 @@ module FileStore
def remove_file(url, path)
return unless has_been_uploaded?(url)
# copy the removed file to tombstone
- @s3_helper.remove(path, true)
+ @s3_helper.remove(get_path_for_s3_upload(path), path)
end
def has_been_uploaded?(url)
@@ -97,11 +115,9 @@ module FileStore
UserAvatar.external_avatar_url(user_id, avatar.upload_id, avatar.width)
end
- 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
+ def get_path_for_s3_upload(path)
+ path = File.join(@s3_bucket_folder_path, path) if @s3_bucket_folder_path
+ path
end
end
diff --git a/lib/s3_helper.rb b/lib/s3_helper.rb
index 0958949065b..bc35c0df016 100644
--- a/lib/s3_helper.rb
+++ b/lib/s3_helper.rb
@@ -16,18 +16,18 @@ class S3Helper
obj.upload_file(file, options)
end
- def remove(path, copy_to_tombstone=false)
+ def remove(s3_filename, tombstone_filename=false)
bucket = s3_bucket
# copy the file in tombstone
- if copy_to_tombstone && @tombstone_prefix.present?
+ if tombstone_filename && @tombstone_prefix.present?
bucket
- .object(File.join(@tombstone_prefix, path))
- .copy_from(copy_source: File.join(@s3_bucket, path))
+ .object(File.join(@tombstone_prefix, tombstone_filename))
+ .copy_from(copy_source: File.join(@s3_bucket, s3_filename))
end
# delete the file
- bucket.object(path).delete
+ bucket.object(s3_filename).delete
rescue Aws::S3::Errors::NoSuchKey
end
diff --git a/spec/components/file_store/s3_store_spec.rb b/spec/components/file_store/s3_store_spec.rb
index 360253aec45..57df6e4c6f2 100644
--- a/spec/components/file_store/s3_store_spec.rb
+++ b/spec/components/file_store/s3_store_spec.rb
@@ -19,25 +19,7 @@ describe FileStore::S3Store do
SiteSetting.s3_secret_access_key = "s3-secret-access-key"
end
- describe ".store_upload" do
-
- it "returns an absolute schemaless url" do
- s3_helper.expects(:upload)
- expect(store.store_upload(uploaded_file, upload)).to match(/\/\/s3-upload-bucket\.s3\.amazonaws\.com\/original\/.+#{upload.sha1}\.png/)
- end
-
- end
-
- describe ".store_optimized_image" do
-
- it "returns an absolute schemaless url" do
- s3_helper.expects(:upload)
- expect(store.store_optimized_image(optimized_image_file, optimized_image)).to match(/\/\/s3-upload-bucket\.s3\.amazonaws\.com\/optimized\/.+#{optimized_image.upload.sha1}_#{OptimizedImage::VERSION}_100x200\.png/)
- end
-
- end
-
- context 'removal from s3' do
+ shared_context 's3 helpers' do
let(:store) { FileStore::S3Store.new }
let(:client) { Aws::S3::Client.new(stub_responses: true) }
let(:resource) { Aws::S3::Resource.new(client: client) }
@@ -45,10 +27,84 @@ describe FileStore::S3Store do
let(:s3_helper) { store.instance_variable_get(:@s3_helper) }
before do
- SiteSetting.s3_region = 'us-west-1'
+ SiteSetting.s3_region ='us-west-1'
+ end
+ end
+
+ context 'uploading to s3' do
+ include_context "s3 helpers"
+
+ describe "#store_upload" do
+ it "returns an absolute schemaless url" do
+ s3_helper.expects(:s3_bucket).returns(s3_bucket)
+ s3_object = stub
+
+ s3_bucket.expects(:object).with("original/1X/#{upload.sha1}.png").returns(s3_object)
+ s3_object.expects(:upload_file)
+
+ expect(store.store_upload(uploaded_file, upload)).to eq(
+ "//s3-upload-bucket.s3-us-west-1.amazonaws.com/original/1X/#{upload.sha1}.png"
+ )
+ end
+
+ describe "when s3_upload_bucket includes folders path" do
+ before do
+ SiteSetting.s3_upload_bucket = "s3-upload-bucket/discourse-uploads"
+ end
+
+ it "returns an absolute schemaless url" do
+ s3_helper.expects(:s3_bucket).returns(s3_bucket)
+ s3_object = stub
+
+ s3_bucket.expects(:object).with("discourse-uploads/original/1X/#{upload.sha1}.png").returns(s3_object)
+ s3_object.expects(:upload_file)
+
+ expect(store.store_upload(uploaded_file, upload)).to eq(
+ "//s3-upload-bucket.s3-us-west-1.amazonaws.com/discourse-uploads/original/1X/#{upload.sha1}.png"
+ )
+ end
+ end
end
- describe ".remove_upload" do
+ describe "#store_optimized_image" do
+ it "returns an absolute schemaless url" do
+ s3_helper.expects(:s3_bucket).returns(s3_bucket)
+ s3_object = stub
+ path = "optimized/1X/#{optimized_image.upload.sha1}_#{OptimizedImage::VERSION}_100x200.png"
+
+ s3_bucket.expects(:object).with(path).returns(s3_object)
+ s3_object.expects(:upload_file)
+
+ expect(store.store_optimized_image(optimized_image_file, optimized_image)).to eq(
+ "//s3-upload-bucket.s3-us-west-1.amazonaws.com/#{path}"
+ )
+ end
+
+ describe "when s3_upload_bucket includes folders path" do
+ before do
+ SiteSetting.s3_upload_bucket = "s3-upload-bucket/discourse-uploads"
+ end
+
+ it "returns an absolute schemaless url" do
+ s3_helper.expects(:s3_bucket).returns(s3_bucket)
+ s3_object = stub
+ path = "discourse-uploads/optimized/1X/#{optimized_image.upload.sha1}_#{OptimizedImage::VERSION}_100x200.png"
+
+ s3_bucket.expects(:object).with(path).returns(s3_object)
+ s3_object.expects(:upload_file)
+
+ expect(store.store_optimized_image(optimized_image_file, optimized_image)).to eq(
+ "//s3-upload-bucket.s3-us-west-1.amazonaws.com/#{path}"
+ )
+ end
+ end
+ end
+ end
+
+ context 'removal from s3' do
+ include_context "s3 helpers"
+
+ describe "#remove_upload" do
it "removes the file from s3 with the right paths" do
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")
@@ -61,9 +117,28 @@ describe FileStore::S3Store do
store.remove_upload(upload)
end
+
+ describe "when s3_upload_bucket includes folders path" do
+ before do
+ SiteSetting.s3_upload_bucket = "s3-upload-bucket/discourse-uploads"
+ end
+
+ it "removes the file from s3 with the right paths" do
+ 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
+
+ s3_bucket.expects(:object).with("discourse-uploads/tombstone/original/1X/#{upload.sha1}.png").returns(s3_object)
+ s3_object.expects(:copy_from).with(copy_source: "s3-upload-bucket/discourse-uploads/original/1X/#{upload.sha1}.png")
+ s3_bucket.expects(:object).with("discourse-uploads/original/1X/#{upload.sha1}.png").returns(s3_object)
+ s3_object.expects(:delete)
+
+ store.remove_upload(upload)
+ end
+ end
end
- describe ".remove_optimized_image" do
+ describe "#remove_optimized_image" do
let(:optimized_image) do
Fabricate(:optimized_image,
url: "//s3-upload-bucket.s3-us-west-1.amazonaws.com/optimized/1X/#{upload.sha1}_1_100x200.png",
@@ -82,6 +157,24 @@ describe FileStore::S3Store do
store.remove_optimized_image(optimized_image)
end
+
+ describe "when s3_upload_bucket includes folders path" do
+ before do
+ SiteSetting.s3_upload_bucket = "s3-upload-bucket/discourse-uploads"
+ end
+
+ it "removes the file from s3 with the right paths" do
+ s3_helper.expects(:s3_bucket).returns(s3_bucket)
+ s3_object = stub
+
+ s3_bucket.expects(:object).with("discourse-uploads/tombstone/optimized/1X/#{upload.sha1}_1_100x200.png").returns(s3_object)
+ s3_object.expects(:copy_from).with(copy_source: "s3-upload-bucket/discourse-uploads/optimized/1X/#{upload.sha1}_1_100x200.png")
+ s3_bucket.expects(:object).with("discourse-uploads/optimized/1X/#{upload.sha1}_1_100x200.png").returns(s3_object)
+ s3_object.expects(:delete)
+
+ store.remove_optimized_image(optimized_image)
+ end
+ end
end
end
@@ -152,4 +245,29 @@ describe FileStore::S3Store do
end
end
+ describe "#s3_bucket" 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