Merge branch 'fj-update-snippet-backfilling-with-migrate-bot' into 'master'

Use migration bot user in snippet backfilling migrations

See merge request gitlab-org/gitlab!30762

changelogs/unreleased/fj-update-snippet-backfilling-with-migrate-bot.yml

+---
+title: Use migration bot user in snippet migration
+merge_request: 30762
+author:
+type: fixed

lib/gitlab/background_migration/backfill_snippet_repositories.rb

@@ -98,17 +98,17 @@ module Gitlab
       # because it is blocked, internal, ghost, ... we cannot commit
       # files because these users are not allowed to, but we need to
       # migrate their snippets as well.
-      # In this scenario an admin user will be the one that will commit the files.
+      # In this scenario the migration bot user will be the one that will commit the files.
       def commit_author(snippet)
         if Gitlab::UserAccessSnippet.new(snippet.author, snippet: snippet).can_do_action?(:update_snippet)
           snippet.author
         else
-          admin_user
+          migration_bot_user
         end
       end
 
-      def admin_user
-        @admin_user ||= User.admins.active.first
+      def migration_bot_user
+        @migration_bot_user ||= User.migration_bot
       end
 
       # We sometimes receive invalid path errors from Gitaly if the Snippet filename

lib/gitlab/git_access_snippet.rb

@@ -61,6 +61,13 @@ module Gitlab
       end
     end
 
+    override :can_read_project?
+    def can_read_project?
+      return true if user&.migration_bot?
+
+      super
+    end
+
     override :check_download_access!
     def check_download_access!
       passed = guest_can_download_code? || user_can_download_code?

lib/gitlab/user_access_snippet.rb

@@ -17,7 +17,14 @@ module Gitlab
       @project = snippet&.project
     end
 
+    def allowed?
+      return true if snippet_migration?
+
+      super
+    end
+
     def can_do_action?(action)
+      return true if snippet_migration?
       return false unless can_access_git?
 
       permission_cache[action] =
@@ -35,7 +42,10 @@ module Gitlab
     end
 
     def can_push_to_branch?(ref)
+      return true if snippet_migration?
+
       super
+
       return false unless snippet
       return false unless can_do_action?(:update_snippet)
 
@@ -45,5 +55,9 @@ module Gitlab
     def can_merge_to_branch?(ref)
       false
     end
+
+    def snippet_migration?
+      user&.migration_bot? && snippet
+    end
   end
 end

spec/lib/gitlab/background_migration/backfill_snippet_repositories_spec.rb

@@ -25,7 +25,7 @@ describe Gitlab::BackgroundMigration::BackfillSnippetRepositories, :migration, s
                  confirmed_at: 1.day.ago)
   end
 
-  let!(:admin) { users.create(id: 2, email: 'admin@example.com', projects_limit: 10, username: 'admin', name: 'Admin', admin: true, state: 'active') }
+  let(:migration_bot) { User.migration_bot }
   let!(:snippet_with_repo) { snippets.create(id: 1, type: 'PersonalSnippet', author_id: user.id, file_name: file_name, content: content) }
   let!(:snippet_with_empty_repo) { snippets.create(id: 2, type: 'PersonalSnippet', author_id: user.id, file_name: file_name, content: content) }
   let!(:snippet_without_repo) { snippets.create(id: 3, type: 'PersonalSnippet', author_id: user.id, file_name: file_name, content: content) }
@@ -88,34 +88,34 @@ describe Gitlab::BackgroundMigration::BackfillSnippetRepositories, :migration, s
       end
 
       context 'when author cannot update snippet or use git' do
-        shared_examples 'admin user commits files' do
+        shared_examples 'migration_bot user commits files' do
           it do
             subject
 
             last_commit = raw_repository(snippet).commit
 
-            expect(last_commit.author_name).to eq admin.name
-            expect(last_commit.author_email).to eq admin.email
+            expect(last_commit.author_name).to eq migration_bot.name
+            expect(last_commit.author_email).to eq migration_bot.email
           end
         end
 
         context 'when user is blocked' do
           let(:user_state) { 'blocked' }
 
-          it_behaves_like 'admin user commits files'
+          it_behaves_like 'migration_bot user commits files'
         end
 
         context 'when user is deactivated' do
           let(:user_state) { 'deactivated' }
 
-          it_behaves_like 'admin user commits files'
+          it_behaves_like 'migration_bot user commits files'
         end
 
         context 'when user is a ghost' do
           let(:ghost) { true }
           let(:user_type) { 'ghost' }
 
-          it_behaves_like 'admin user commits files'
+          it_behaves_like 'migration_bot user commits files'
         end
       end
     end

spec/lib/gitlab/git_access_snippet_spec.rb

@@ -11,8 +11,9 @@ describe Gitlab::GitAccessSnippet do
   let_it_be(:user) { create(:user) }
   let_it_be(:project) { create(:project, :public) }
   let_it_be(:snippet) { create(:project_snippet, :public, :repository, project: project) }
-  let(:repository) { snippet.repository }
+  let_it_be(:migration_bot) { User.migration_bot }
 
+  let(:repository) { snippet.repository }
   let(:actor) { user }
   let(:protocol) { 'ssh' }
   let(:changes) { Gitlab::GitAccess::ANY }
@@ -27,10 +28,22 @@ describe Gitlab::GitAccessSnippet do
     let(:actor) { build(:deploy_key) }
 
     it 'does not allow push and pull access' do
+      expect { push_access_check }.to raise_forbidden(described_class::ERROR_MESSAGES[:authentication_mechanism])
       expect { pull_access_check }.to raise_forbidden(described_class::ERROR_MESSAGES[:authentication_mechanism])
     end
   end
 
+  shared_examples 'actor is migration bot' do
+    context 'when user is the migration bot' do
+      let(:user) { migration_bot }
+
+      it 'can perform git operations' do
+        expect { push_access_check }.not_to raise_error
+        expect { pull_access_check }.not_to raise_error
+      end
+    end
+  end
+
   describe '#check_snippet_accessibility!' do
     context 'when the snippet exists' do
       it 'allows access' do
@@ -77,6 +90,12 @@ describe Gitlab::GitAccessSnippet do
       expect { push_access_check }.not_to raise_error
       expect { pull_access_check }.not_to raise_error
     end
+
+    it_behaves_like 'actor is migration bot' do
+      before do
+        expect(migration_bot.required_terms_not_accepted?).to be_truthy
+      end
+    end
   end
 
   context 'project snippet accessibility', :aggregate_failures do
@@ -107,6 +126,7 @@ describe Gitlab::GitAccessSnippet do
 
     context 'when project is public' do
       it_behaves_like 'checks accessibility'
+      it_behaves_like 'actor is migration bot'
     end
 
     context 'when project is public but snippet feature is private' do
@@ -117,6 +137,7 @@ describe Gitlab::GitAccessSnippet do
       end
 
       it_behaves_like 'checks accessibility'
+      it_behaves_like 'actor is migration bot'
     end
 
     context 'when project is not accessible' do
@@ -127,11 +148,58 @@ describe Gitlab::GitAccessSnippet do
           let(:membership) { membership }
 
           it 'respects accessibility' do
-            expect { push_access_check }.to raise_error(described_class::NotFoundError)
-            expect { pull_access_check }.to raise_error(described_class::NotFoundError)
+            expect { push_access_check }.to raise_snippet_not_found
+            expect { pull_access_check }.to raise_snippet_not_found
+          end
+        end
+      end
+
+      it_behaves_like 'actor is migration bot'
+    end
+
+    context 'when project is archived' do
+      let(:project) { create(:project, :public, :archived) }
+
+      [:anonymous, :non_member].each do |membership|
+        context membership.to_s do
+          let(:membership) { membership }
+
+          it 'cannot perform git operations' do
+            expect { push_access_check }.to raise_error(described_class::ForbiddenError)
+            expect { pull_access_check }.to raise_error(described_class::ForbiddenError)
+          end
+        end
+      end
+
+      [:guest, :reporter, :maintainer, :author, :admin].each do |membership|
+        context membership.to_s do
+          let(:membership) { membership }
+
+          it 'cannot perform git pushes' do
+            expect { push_access_check }.to raise_error(described_class::ForbiddenError)
+            expect { pull_access_check }.not_to raise_error
+          end
+        end
+      end
+
+      it_behaves_like 'actor is migration bot'
+    end
+
+    context 'when snippet feature is disabled' do
+      let(:project) { create(:project, :public, :snippets_disabled) }
+
+      [:anonymous, :non_member, :author, :admin].each do |membership|
+        context membership.to_s do
+          let(:membership) { membership }
+
+          it 'cannot perform git operations' do
+            expect { push_access_check }.to raise_error(described_class::ForbiddenError)
+            expect { pull_access_check }.to raise_error(described_class::ForbiddenError)
           end
         end
       end
+
+      it_behaves_like 'actor is migration bot'
     end
   end
 
@@ -159,6 +227,8 @@ describe Gitlab::GitAccessSnippet do
           expect { pull_access_check }.to raise_error(error_class)
         end
       end
+
+      it_behaves_like 'actor is migration bot'
     end
   end
 
@@ -166,36 +236,56 @@ describe Gitlab::GitAccessSnippet do
     let(:user) { snippet.author }
     let!(:primary_node) { FactoryBot.create(:geo_node, :primary) }
 
-    # Without override, push access would return Gitlab::GitAccessResult::CustomAction
-    it 'skips geo for snippet' do
+    before do
       allow(::Gitlab::Database).to receive(:read_only?).and_return(true)
       allow(::Gitlab::Geo).to receive(:secondary_with_primary?).and_return(true)
+    end
 
+    # Without override, push access would return Gitlab::GitAccessResult::CustomAction
+    it 'skips geo for snippet' do
       expect { push_access_check }.to raise_forbidden(/You can't push code to a read-only GitLab instance/)
     end
+
+    context 'when user is migration bot' do
+      let(:user) { migration_bot }
+
+      it 'skips geo for snippet' do
+        expect { push_access_check }.to raise_forbidden(/You can't push code to a read-only GitLab instance/)
+      end
+    end
   end
 
   context 'when changes are specific' do
     let(:changes) { "2d1db523e11e777e49377cfb22d368deec3f0793 ddd0f15ae83993f5cb66a927a28673882e99100b master" }
     let(:user) { snippet.author }
 
-    it 'does not raise error if SnippetCheck does not raise error' do
-      expect_next_instance_of(Gitlab::Checks::SnippetCheck) do |check|
-        expect(check).to receive(:validate!).and_call_original
-      end
-      expect_next_instance_of(Gitlab::Checks::PushFileCountCheck) do |check|
-        expect(check).to receive(:validate!)
+    shared_examples 'snippet checks' do
+      it 'does not raise error if SnippetCheck does not raise error' do
+        expect_next_instance_of(Gitlab::Checks::SnippetCheck) do |check|
+          expect(check).to receive(:validate!).and_call_original
+        end
+        expect_next_instance_of(Gitlab::Checks::PushFileCountCheck) do |check|
+          expect(check).to receive(:validate!)
+        end
+
+        expect { push_access_check }.not_to raise_error
       end
 
-      expect { push_access_check }.not_to raise_error
-    end
+      it 'raises error if SnippetCheck raises error' do
+        expect_next_instance_of(Gitlab::Checks::SnippetCheck) do |check|
+          allow(check).to receive(:validate!).and_raise(Gitlab::GitAccess::ForbiddenError, 'foo')
+        end
 
-    it 'raises error if SnippetCheck raises error' do
-      expect_next_instance_of(Gitlab::Checks::SnippetCheck) do |check|
-        allow(check).to receive(:validate!).and_raise(Gitlab::GitAccess::ForbiddenError, 'foo')
+        expect { push_access_check }.to raise_forbidden('foo')
       end
+    end
 
-      expect { push_access_check }.to raise_forbidden('foo')
+    it_behaves_like 'snippet checks'
+
+    context 'when user is migration bot' do
+      let(:user) { migration_bot }
+
+      it_behaves_like 'snippet checks'
     end
   end
 
@@ -260,6 +350,14 @@ describe Gitlab::GitAccessSnippet do
       it_behaves_like 'a push to repository already over the limit'
       it_behaves_like 'a push to repository below the limit'
       it_behaves_like 'a push to repository to make it over the limit'
+
+      context 'when user is migration bot' do
+        let(:actor) { migration_bot }
+
+        it_behaves_like 'a push to repository already over the limit'
+        it_behaves_like 'a push to repository below the limit'
+        it_behaves_like 'a push to repository to make it over the limit'
+      end
     end
 
     context 'when GIT_OBJECT_DIRECTORY_RELATIVE env var is not set' do
@@ -274,6 +372,14 @@ describe Gitlab::GitAccessSnippet do
       it_behaves_like 'a push to repository already over the limit'
       it_behaves_like 'a push to repository below the limit'
       it_behaves_like 'a push to repository to make it over the limit'
+
+      context 'when user is migration bot' do
+        let(:actor) { migration_bot }
+
+        it_behaves_like 'a push to repository already over the limit'
+        it_behaves_like 'a push to repository below the limit'
+        it_behaves_like 'a push to repository to make it over the limit'
+      end
     end
   end
 

spec/lib/gitlab/user_access_snippet_spec.rb

@@ -7,6 +7,8 @@ describe Gitlab::UserAccessSnippet do
 
   let_it_be(:project) { create(:project, :private) }
   let_it_be(:snippet) { create(:project_snippet, :private, project: project) }
+  let_it_be(:migration_bot) { User.migration_bot }
+
   let(:user) { create(:user) }
 
   describe '#can_do_action?' do
@@ -36,6 +38,14 @@ describe Gitlab::UserAccessSnippet do
         expect(access.can_do_action?(:ability)).to eq(false)
       end
     end
+
+    context 'when user is migration bot' do
+      let(:user) { migration_bot }
+
+      it 'allows access' do
+        expect(access.can_do_action?(:ability)).to eq(true)
+      end
+    end
   end
 
   describe '#can_push_to_branch?' do
@@ -65,6 +75,16 @@ describe Gitlab::UserAccessSnippet do
       end
     end
 
+    context 'when user is migration bot' do
+      let(:user) { migration_bot }
+
+      it 'allows access' do
+        allow(Ability).to receive(:allowed?).and_return(false)
+
+        expect(access.can_push_to_branch?('random_branch')).to eq(true)
+      end
+    end
+
     context 'when snippet is nil' do
       let(:user) { create_user_from_membership(project, :admin) }
       let(:snippet) { nil }
@@ -72,6 +92,14 @@ describe Gitlab::UserAccessSnippet do
       it 'disallows access' do
         expect(access.can_push_to_branch?('random_branch')).to eq(false)
       end
+
+      context 'when user is migration bot' do
+        let(:user) { migration_bot }
+
+        it 'disallows access' do
+          expect(access.can_push_to_branch?('random_branch')).to eq(false)
+        end
+      end
     end
   end
 
@@ -79,17 +107,41 @@ describe Gitlab::UserAccessSnippet do
     it 'returns false' do
       expect(access.can_create_tag?('random_tag')).to be_falsey
     end
+
+    context 'when user is migration bot' do
+      let(:user) { migration_bot }
+
+      it 'returns false' do
+        expect(access.can_create_tag?('random_tag')).to be_falsey
+      end
+    end
   end
 
   describe '#can_delete_branch?' do
     it 'returns false' do
       expect(access.can_delete_branch?('random_branch')).to be_falsey
     end
+
+    context 'when user is migration bot' do
+      let(:user) { migration_bot }
+
+      it 'returns false' do
+        expect(access.can_delete_branch?('random_branch')).to be_falsey
+      end
+    end
   end
 
   describe '#can_merge_to_branch?' do
     it 'returns false' do
       expect(access.can_merge_to_branch?('random_branch')).to be_falsey
     end
+
+    context 'when user is migration bot' do
+      let(:user) { migration_bot }
+
+      it 'returns false' do
+        expect(access.can_merge_to_branch?('random_branch')).to be_falsey
+      end
+    end
   end
 end