Fix snippet migration when user has invalid info

When the user has invalid info like invalid chars
in the name or the email, git operation will fail.

These errors will be raised by libgit2 not us. We
need to capture them and use a reliable user
to perform the migration for those snippets, like
the migration bot.

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?(message)
-    message.downcase.start_with?('invalid path', 'path cannot include directory traversal')
+  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_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