FIX: Bucket name is missing in S3 inventory data path

lib/s3_inventory.rb

@@ -10,6 +10,7 @@ class S3Inventory
   CSV_KEY_INDEX ||= 1
   CSV_ETAG_INDEX ||= 2
   INVENTORY_PREFIX ||= "inventory"
+  INVENTORY_VERSION ||= "1"
 
   def initialize(s3_helper, type)
     @s3_helper = s3_helper
@@ -96,10 +97,10 @@ class S3Inventory
             "Effect": "Allow",
             "Principal": { "Service": "s3.amazonaws.com" },
             "Action": ["s3:PutObject"],
-            "Resource": ["arn:aws:s3:::#{inventory_root_path}/*"],
+            "Resource": ["#{inventory_path_arn}/*"],
             "Condition": {
               "ArnLike": {
-                "aws:SourceArn": "arn:aws:s3:::#{bucket_name}"
+                "aws:SourceArn": bucket_arn
               },
               "StringEquals": {
                 "s3:x-amz-acl": "bucket-owner-full-control"
@@ -134,7 +135,7 @@ class S3Inventory
     {
       destination: {
         s3_bucket_destination: {
-          bucket: "arn:aws:s3:::#{bucket_name}",
+          bucket: bucket_arn,
           prefix: destination_prefix,
           format: "CSV"
         }
@@ -163,7 +164,7 @@ class S3Inventory
   def unsorted_files
     objects = []
 
-    @s3_helper.list(File.join(inventory_path, "data")).each do |obj|
+    @s3_helper.list(inventory_data_path).each do |obj|
       if obj.key.match?(/\.csv\.gz$/i)
         objects << obj
       end
@@ -174,12 +175,22 @@ class S3Inventory
     log("Failed to list inventory from S3", e)
   end
 
-  def inventory_path
-    File.join(inventory_root_path, inventory_id)
+  def inventory_data_path
+    File.join(inventory_path, bucket_name, inventory_id, "data")
   end
 
-  def inventory_root_path
-    File.join(bucket_name, bucket_folder_path || "", INVENTORY_PREFIX)
+  def inventory_path_arn
+    File.join(bucket_arn, inventory_path)
+  end
+
+  def inventory_path
+    path = File.join(INVENTORY_PREFIX, INVENTORY_VERSION)
+    path = File.join(bucket_folder_path, path) if bucket_folder_path.present?
+    path
+  end
+
+  def bucket_arn
+    "arn:aws:s3:::#{bucket_name}"
   end
 
   def log(message, ex = nil)

spec/components/s3_inventory_spec.rb

@@ -16,7 +16,11 @@ describe "S3Inventory" do
     SiteSetting.s3_secret_access_key = "def"
     SiteSetting.enable_s3_inventory = true
 
-    client.stub_responses(:list_objects,
+    client.stub_responses(:list_objects, -> (context) {
+      inventory_data_path = "#{S3Inventory::INVENTORY_PREFIX}/#{S3Inventory::INVENTORY_VERSION}/bucket/original/data"
+      expect(context.params[:prefix]).to eq(inventory_data_path)
+
+      {
         contents: [
           {
             etag: "\"70ee1738b6b21e2c8a43f3a5ab0eee71\"",
@@ -42,7 +46,8 @@ describe "S3Inventory" do
           }
         ],
         next_marker: "eyJNYXJrZXIiOiBudWxsLCAiYm90b190cnVuY2F0ZV9hbW91bnQiOiAyfQ=="
-    )
+      }
+    })
   end
 
   it "should return the latest inventory file name" do

spec/jobs/update_s3_inventory_spec.rb

@@ -18,7 +18,7 @@ describe Jobs::UpdateS3Inventory do
     id = "original"
     @client.expects(:put_bucket_policy).with(
       bucket: "bucket",
-      policy: "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Sid\":\"InventoryAndAnalyticsPolicy\",\"Effect\":\"Allow\",\"Principal\":{\"Service\":\"s3.amazonaws.com\"},\"Action\":[\"s3:PutObject\"],\"Resource\":[\"arn:aws:s3:::bucket/inventory/*\"],\"Condition\":{\"ArnLike\":{\"aws:SourceArn\":\"arn:aws:s3:::bucket\"},\"StringEquals\":{\"s3:x-amz-acl\":\"bucket-owner-full-control\"}}}]}"
+      policy: "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Sid\":\"InventoryAndAnalyticsPolicy\",\"Effect\":\"Allow\",\"Principal\":{\"Service\":\"s3.amazonaws.com\"},\"Action\":[\"s3:PutObject\"],\"Resource\":[\"arn:aws:s3:::bucket/#{S3Inventory::INVENTORY_PREFIX}/#{S3Inventory::INVENTORY_VERSION}/*\"],\"Condition\":{\"ArnLike\":{\"aws:SourceArn\":\"arn:aws:s3:::bucket\"},\"StringEquals\":{\"s3:x-amz-acl\":\"bucket-owner-full-control\"}}}]}"
     )
     @client.expects(:put_bucket_inventory_configuration)
     @client.expects(:put_bucket_inventory_configuration).with(