Merge branch 'fj-fix-migration-when-user-invalid-commit-name' into 'master'

Fix snippet migration when user has invalid info

See merge request gitlab-org/gitlab!31488

app/models/snippet_repository.rb

@@ -8,6 +8,7 @@ class SnippetRepository < ApplicationRecord
 
   CommitError = Class.new(StandardError)
   InvalidPathError = Class.new(CommitError)
+  InvalidSignatureError = Class.new(CommitError)
 
   belongs_to :snippet, inverse_of: :snippet_repository
 
@@ -41,8 +42,8 @@ class SnippetRepository < ApplicationRecord
   rescue Gitlab::Git::Index::IndexError,
          Gitlab::Git::CommitError,
          Gitlab::Git::PreReceiveError,
-         Gitlab::Git::CommandError => error
-
+         Gitlab::Git::CommandError,
+         ArgumentError => error
     raise commit_error_exception(error)
   end
 
@@ -88,15 +89,23 @@ class SnippetRepository < ApplicationRecord
     "#{DEFAULT_EMPTY_FILE_NAME}#{index}.txt"
   end
 
-  def commit_error_exception(error)
-    if error.is_a?(Gitlab::Git::Index::IndexError) && invalid_path_error?(error.message)
+  def commit_error_exception(err)
+    if invalid_path_error?(err)
       InvalidPathError.new('Invalid Path') # To avoid returning the message with the path included
+    elsif invalid_signature_error?(err)
+      InvalidSignatureError.new(err.message)
     else
-      CommitError.new(error.message)
+      CommitError.new(err.message)
+    end
   end
+
+  def invalid_path_error?(err)
+    err.is_a?(Gitlab::Git::Index::IndexError) &&
+      err.message.downcase.start_with?('invalid path', 'path cannot include directory traversal')
   end
 
-  def invalid_path_error?(message)
-    message.downcase.start_with?('invalid path', 'path cannot include directory traversal')
+  def invalid_signature_error?(err)
+    err.is_a?(ArgumentError) &&
+      err.message.downcase.match?(/failed to parse signature/)
   end
 end

changelogs/unreleased/fj-fix-migration-when-user-invalid-commit-name.yml

+---
+title: Fix snippet migration when user has invalid info
+merge_request: 31488
+author:
+type: fixed

lib/gitlab/background_migration/backfill_snippet_repositories.rb

@@ -16,6 +16,7 @@ module Gitlab
 
           retry_index = 0
           @invalid_path_error = false
+          @invalid_signature_error = false
 
           begin
             create_repository_and_files(snippet)
@@ -23,10 +24,11 @@ module Gitlab
             logger.info(message: 'Snippet Migration: repository created and migrated', snippet: snippet.id)
           rescue => e
             set_file_path_error(e)
+            set_signature_error(e)
 
             retry_index += 1
 
-            retry if retry_index < MAX_RETRIES
+            retry if retry_index < max_retries
 
             logger.error(message: "Snippet Migration: error migrating snippet. Reason: #{e.message}", snippet: snippet.id)
 
@@ -101,6 +103,7 @@ module Gitlab
       # In this scenario the migration bot user will be the one that will commit the files.
       def commit_author(snippet)
         return migration_bot_user if snippet_content_size_over_limit?(snippet)
+        return migration_bot_user if @invalid_signature_error
 
         if Gitlab::UserAccessSnippet.new(snippet.author, snippet: snippet).can_do_action?(:update_snippet)
           snippet.author
@@ -119,7 +122,23 @@ module Gitlab
       # the migration can succeed, to achieve that, we'll identify in migration retries
       # that the path is invalid
       def set_file_path_error(error)
-        @invalid_path_error = error.is_a?(SnippetRepository::InvalidPathError)
+        @invalid_path_error ||= error.is_a?(SnippetRepository::InvalidPathError)
+      end
+
+      # We sometimes receive invalid signature from Gitaly if the commit author
+      # name or email is invalid to create the commit signature.
+      # In this situation, we set the error and use the migration_bot since
+      # the information used to build it is valid
+      def set_signature_error(error)
+        @invalid_signature_error ||= error.is_a?(SnippetRepository::InvalidSignatureError)
+      end
+
+      # In the case where the snippet file_name is invalid and also the
+      # snippet author has invalid commit info, we need to increase the
+      # number of retries by 1, because we will receive two errors
+      # from Gitaly and, in the third one, we will commit successfully.
+      def max_retries
+        MAX_RETRIES + (@invalid_signature_error && @invalid_path_error ? 1 : 0)
       end
 
       def snippet_content_size_over_limit?(snippet)

spec/lib/gitlab/background_migration/backfill_snippet_repositories_spec.rb

@@ -11,13 +11,14 @@ describe Gitlab::BackgroundMigration::BackfillSnippetRepositories, :migration, s
   let(:user_state) { 'active' }
   let(:ghost) { false }
   let(:user_type) { nil }
+  let(:user_name) { 'Test' }
 
   let!(:user) do
     users.create(id: 1,
                  email: 'user@example.com',
                  projects_limit: 10,
                  username: 'test',
-                 name: 'Test',
+                 name: user_name,
                  state: user_state,
                  ghost: ghost,
                  last_activity_on: 1.minute.ago,
@@ -232,6 +233,69 @@ describe Gitlab::BackgroundMigration::BackfillSnippetRepositories, :migration, s
 
       it_behaves_like 'migration_bot user commits files'
     end
+
+    context 'when user name is invalid' do
+      let(:user_name) { '.' }
+      let!(:snippet) { snippets.create(id: 4, type: 'PersonalSnippet', author_id: user.id, file_name: file_name, content: content) }
+      let(:ids) { [4, 4] }
+
+      after do
+        raw_repository(snippet).remove
+      end
+
+      it_behaves_like 'migration_bot user commits files'
+    end
+
+    context 'when both user name and snippet file_name are invalid' do
+      let(:user_name) { '.' }
+      let!(:other_user) do
+        users.create(id: 2,
+                     email: 'user2@example.com',
+                     projects_limit: 10,
+                     username: 'test2',
+                     name: 'Test2',
+                     state: user_state,
+                     ghost: ghost,
+                     last_activity_on: 1.minute.ago,
+                     user_type: user_type,
+                     confirmed_at: 1.day.ago)
+      end
+      let!(:invalid_snippet) { snippets.create(id: 4, type: 'PersonalSnippet', author_id: user.id, file_name: '.', content: content) }
+      let!(:snippet) { snippets.create(id: 5, type: 'PersonalSnippet', author_id: other_user.id, file_name: file_name, content: content) }
+      let(:ids) { [4, 5] }
+
+      after do
+        raw_repository(snippet).remove
+        raw_repository(invalid_snippet).remove
+      end
+
+      it 'updates the file_name only when it is invalid' do
+        subject
+
+        expect(blob_at(invalid_snippet, 'snippetfile1.txt')).to be
+        expect(blob_at(snippet, file_name)).to be
+      end
+
+      it_behaves_like 'migration_bot user commits files' do
+        let(:snippet) { invalid_snippet }
+      end
+
+      it 'does not alter the commit author in subsequent migrations' do
+        subject
+
+        last_commit = raw_repository(snippet).commit
+
+        expect(last_commit.author_name).to eq other_user.name
+        expect(last_commit.author_email).to eq other_user.email
+      end
+
+      it "increases the number of retries temporarily from #{described_class::MAX_RETRIES} to #{described_class::MAX_RETRIES + 1}" do
+        expect(service).to receive(:create_commit).with(Snippet.find(invalid_snippet.id)).exactly(described_class::MAX_RETRIES + 1).times.and_call_original
+        expect(service).to receive(:create_commit).with(Snippet.find(snippet.id)).once.and_call_original
+
+        subject
+      end
+    end
   end
 
   def blob_at(snippet, path)

spec/models/snippet_repository_spec.rb

@@ -217,6 +217,22 @@ describe SnippetRepository do
       it_behaves_like 'snippet repository with git errors', 'invalid://path/here', described_class::InvalidPathError
       it_behaves_like 'snippet repository with git errors', '../../path/traversal/here', described_class::InvalidPathError
       it_behaves_like 'snippet repository with git errors', 'README', described_class::CommitError
+
+      context 'when user name is invalid' do
+        let(:user) { create(:user, name: '.') }
+
+        it_behaves_like 'snippet repository with git errors', 'non_existing_file', described_class::InvalidSignatureError
+      end
+
+      context 'when user email is empty' do
+        let(:user) { create(:user) }
+
+        before do
+          user.update_column(:email, '')
+        end
+
+        it_behaves_like 'snippet repository with git errors', 'non_existing_file', described_class::InvalidSignatureError
+      end
     end
   end