Merge branch 'sh-disable-google-backup-acl' into 'master'

Backup: Disable setting of ACL for Google uploads

Closes #35662

See merge request gitlab-org/gitlab!20407

changelogs/unreleased/sh-disable-google-backup-acl.yml

+---
+title: 'Backup: Disable setting of ACL for Google uploads'
+merge_request: 20407
+author:
+type: fixed

lib/backup/manager.rb

@@ -47,11 +47,7 @@ module Backup
 
       directory = connect_to_remote_directory(connection_settings)
 
-      if directory.files.create(key: remote_target, body: File.open(tar_file), public: false,
-                                multipart_chunk_size: Gitlab.config.backup.upload.multipart_chunk_size,
-                                encryption: Gitlab.config.backup.upload.encryption,
-                                encryption_key: Gitlab.config.backup.upload.encryption_key,
-                                storage_class: Gitlab.config.backup.upload.storage_class)
+      if directory.files.create(create_attributes)
         progress.puts "done".color(:green)
       else
         puts "uploading backup to #{remote_directory} failed".color(:red)
@@ -252,5 +248,27 @@ module Backup
         skipped: ENV["SKIP"]
       }
     end
+
+    def create_attributes
+      attrs = {
+        key: remote_target,
+        body: File.open(tar_file),
+        multipart_chunk_size: Gitlab.config.backup.upload.multipart_chunk_size,
+        encryption: Gitlab.config.backup.upload.encryption,
+        encryption_key: Gitlab.config.backup.upload.encryption_key,
+        storage_class: Gitlab.config.backup.upload.storage_class
+      }
+
+      # Google bucket-only policies prevent setting an ACL. In any case, by default,
+      # all objects are set to the default ACL, which is project-private:
+      # https://cloud.google.com/storage/docs/json_api/v1/defaultObjectAccessControls
+      attrs[:public] = false unless google_provider?
+
+      attrs
+    end
+
+    def google_provider?
+      Gitlab.config.backup.upload.connection&.provider&.downcase == 'google'
+    end
   end
 end

spec/lib/backup/manager_spec.rb

@@ -326,7 +326,7 @@ describe Backup::Manager do
     context 'target path' do
       it 'uses the tar filename by default' do
         expect_any_instance_of(Fog::Collection).to receive(:create)
-          .with(hash_including(key: backup_filename))
+          .with(hash_including(key: backup_filename, public: false))
           .and_return(true)
 
         Dir.chdir(Gitlab.config.backup.path) do
@@ -338,7 +338,39 @@ describe Backup::Manager do
         stub_env('DIRECTORY', 'daily')
 
         expect_any_instance_of(Fog::Collection).to receive(:create)
-          .with(hash_including(key: "daily/#{backup_filename}"))
+          .with(hash_including(key: "daily/#{backup_filename}", public: false))
+          .and_return(true)
+
+        Dir.chdir(Gitlab.config.backup.path) do
+          subject.upload
+        end
+      end
+    end
+
+    context 'with Google provider' do
+      before do
+        stub_backup_setting(
+          upload: {
+            connection: {
+              provider: 'Google',
+              google_storage_access_key_id: 'test-access-id',
+              google_storage_secret_access_key: 'secret'
+            },
+            remote_directory: 'directory',
+            multipart_chunk_size: Gitlab.config.backup.upload.multipart_chunk_size,
+            encryption: nil,
+            encryption_key: nil,
+            storage_class: nil
+          }
+        )
+
+        connection = ::Fog::Storage.new(Gitlab.config.backup.upload.connection.symbolize_keys)
+        connection.directories.create(key: Gitlab.config.backup.upload.remote_directory)
+      end
+
+      it 'does not attempt to set ACL' do
+        expect_any_instance_of(Fog::Collection).to receive(:create)
+          .with(hash_excluding(public: false))
           .and_return(true)
 
         Dir.chdir(Gitlab.config.backup.path) do