FIX: Ensure that multisite s3 uploads are tombstoned correctly (#6769)

* FIX: Ensure that multisite uploads are tombstoned into the correct paths

* Move multisite specs to spec/multisite/s3_store_spec.rb

lib/file_store/s3_store.rb

@@ -12,7 +12,9 @@ module FileStore
     attr_reader :s3_helper
 
     def initialize(s3_helper = nil)
-      @s3_helper = s3_helper || S3Helper.new(s3_bucket, TOMBSTONE_PREFIX)
+      @s3_helper = s3_helper || S3Helper.new(s3_bucket,
+        Rails.configuration.multisite ? multisite_tombstone_prefix : TOMBSTONE_PREFIX
+      )
     end
 
     def store_upload(file, upload, content_type = nil)
@@ -87,6 +89,10 @@ module FileStore
       @s3_helper.update_tombstone_lifecycle(grace_period)
     end
 
+    def multisite_tombstone_prefix
+      File.join("uploads", "tombstone", RailsMultisite::ConnectionManagement.current_db, "/")
+    end
+
     def path_for(upload)
       url = upload.try(:url)
       FileStore::LocalStore.new.path_for(upload) if url && url[/^\/[^\/]/]

lib/s3_helper.rb

@@ -39,14 +39,27 @@ class S3Helper
     end
 
     # delete the file
+    s3_filename.prepend(multisite_upload_path) if Rails.configuration.multisite
     s3_bucket.object(get_path_for_s3_upload(s3_filename)).delete
   rescue Aws::S3::Errors::NoSuchKey
   end
 
   def copy(source, destination, options: {})
+    if !Rails.configuration.multisite
+      options[:copy_source] = File.join(@s3_bucket_name, source)
+    else
+      if @s3_bucket_folder_path
+        bucket_folder, filename = begin
+          source.split("/".freeze, 2)
+        end
+        options[:copy_source] = File.join(@s3_bucket_name, bucket_folder, multisite_upload_path, filename)
+      else
+        options[:copy_source] = File.join(@s3_bucket_name, multisite_upload_path, source)
+      end
+    end
     s3_bucket
       .object(destination)
-      .copy_from(options.merge(copy_source: File.join(@s3_bucket_name, source)))
+      .copy_from(options)
   end
 
   # make sure we have a cors config for assets
@@ -194,6 +207,10 @@ class S3Helper
     path
   end
 
+  def multisite_upload_path
+    File.join("uploads", RailsMultisite::ConnectionManagement.current_db, "/")
+  end
+
   def s3_resource
     Aws::S3::Resource.new(@s3_options)
   end

spec/multisite/s3_store_spec.rb

@@ -4,18 +4,7 @@ require 'file_store/s3_store'
 RSpec.describe 'Multisite s3 uploads', type: :multisite do
   let(:conn) { RailsMultisite::ConnectionManagement }
   let(:uploaded_file) { file_from_fixtures("smallest.png") }
-
-  let(:upload) do
-    Fabricate(:upload, sha1: Digest::SHA1.hexdigest(File.read(uploaded_file)))
-  end
-
-  let(:s3_client) { Aws::S3::Client.new(stub_responses: true) }
-
-  let(:s3_helper) do
-    S3Helper.new(SiteSetting.s3_upload_bucket, '', client: s3_client)
-  end
-
-  let(:store) { FileStore::S3Store.new(s3_helper) }
+  let(:upload) { Fabricate(:upload, sha1: Digest::SHA1.hexdigest(File.read(uploaded_file))) }
 
   context 'uploading to s3' do
     before(:each) do
@@ -26,6 +15,10 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
     end
 
     describe "#store_upload" do
+      let(:s3_client) { Aws::S3::Client.new(stub_responses: true) }
+      let(:s3_helper) { S3Helper.new(SiteSetting.s3_upload_bucket, '', client: s3_client) }
+      let(:store) { FileStore::S3Store.new(s3_helper) }
+
       it "returns the correct url for default and second multisite db" do
         conn.with_connection('default') do
           expect(store.store_upload(uploaded_file, upload)).to eq(
@@ -41,4 +34,76 @@ RSpec.describe 'Multisite s3 uploads', type: :multisite do
       end
     end
   end
+
+  context 'removal from s3' do
+    before do
+      SiteSetting.s3_region = 'us-west-1'
+      SiteSetting.s3_upload_bucket = "s3-upload-bucket"
+      SiteSetting.s3_access_key_id = "s3-access-key-id"
+      SiteSetting.s3_secret_access_key = "s3-secret-access-key"
+      SiteSetting.enable_s3_uploads = true
+    end
+
+    describe "#remove_upload" 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(SiteSetting.s3_upload_bucket) }
+      let(:s3_helper) { store.s3_helper }
+
+      it "removes the file from s3 on multisite" do
+        conn.with_connection('default') do
+          store.expects(:get_depth_for).with(upload.id).returns(0)
+          s3_helper.expects(:s3_bucket).returns(s3_bucket).at_least_once
+          upload.update_attributes!(url: "//s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/uploads/default/original/1X/#{upload.sha1}.png")
+          s3_object = stub
+
+          s3_bucket.expects(:object).with("uploads/tombstone/default/original/1X/#{upload.sha1}.png").returns(s3_object)
+          s3_object.expects(:copy_from).with(copy_source: "s3-upload-bucket/uploads/default/original/1X/#{upload.sha1}.png")
+          s3_bucket.expects(:object).with("uploads/default/original/1X/#{upload.sha1}.png").returns(s3_object)
+          s3_object.expects(:delete)
+
+          store.remove_upload(upload)
+        end
+      end
+
+      it "removes the file from s3 on another multisite db" do
+        conn.with_connection('second') do
+          store.expects(:get_depth_for).with(upload.id).returns(0)
+          s3_helper.expects(:s3_bucket).returns(s3_bucket).at_least_once
+          upload.update_attributes!(url: "//s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/uploads/second/original/1X/#{upload.sha1}.png")
+          s3_object = stub
+
+          s3_bucket.expects(:object).with("uploads/tombstone/second/original/1X/#{upload.sha1}.png").returns(s3_object)
+          s3_object.expects(:copy_from).with(copy_source: "s3-upload-bucket/uploads/second/original/1X/#{upload.sha1}.png")
+          s3_bucket.expects(:object).with("uploads/second/original/1X/#{upload.sha1}.png").returns(s3_object)
+          s3_object.expects(:delete)
+
+          store.remove_upload(upload)
+        end
+      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 on multisite" do
+          conn.with_connection('default') do
+            store.expects(:get_depth_for).with(upload.id).returns(0)
+            s3_helper.expects(:s3_bucket).returns(s3_bucket).at_least_once
+            upload.update_attributes!(url: "//s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/discourse-uploads/uploads/default/original/1X/#{upload.sha1}.png")
+            s3_object = stub
+
+            s3_bucket.expects(:object).with("discourse-uploads/uploads/tombstone/default/original/1X/#{upload.sha1}.png").returns(s3_object)
+            s3_object.expects(:copy_from).with(copy_source: "s3-upload-bucket/discourse-uploads/uploads/default/original/1X/#{upload.sha1}.png")
+            s3_bucket.expects(:object).with("discourse-uploads/uploads/default/original/1X/#{upload.sha1}.png").returns(s3_object)
+            s3_object.expects(:delete)
+
+            store.remove_upload(upload)
+          end
+        end
+      end
+    end
+  end
 end