Merge branch 'kassio/github-importer-dont-match-user-id-when-github-enterprise' into 'master'

GithubImporter: match user by external id only from github.com

See merge request gitlab-org/gitlab!66387

app/models/project.rb

@@ -1184,6 +1184,15 @@ class Project < ApplicationRecord
     import_type == 'gitea'
   end
 
+  def github_import?
+    import_type == 'github'
+  end
+
+  def github_enterprise_import?
+    github_import? &&
+      URI.parse(import_url).host != URI.parse(Octokit::Default::API_ENDPOINT).host
+  end
+
   def has_remote_mirror?
     remote_mirror_available? && remote_mirrors.enabled.exists?
   end

doc/development/github_importer.md

@@ -183,6 +183,9 @@ perform:
    tries to find the user based on the GitHub user ID, while the second query
    is used to find the user using their GitHub Email address.
 
+To avoid mismatching users, the search by GitHub user ID is not done when importing from GitHub
+Enterprise.
+
 Because this process is quite expensive we cache the result of these lookups in
 Redis. For every user looked up we store three keys:
 

lib/gitlab/github_import/user_finder.rb

@@ -120,10 +120,18 @@ module Gitlab
         read_id_from_cache(ID_FOR_EMAIL_CACHE_KEY % email)
       end
 
-      # Queries and caches the GitLab user ID for a GitHub user ID, if one was
-      # found.
+      # If importing from github.com, queries and caches the GitLab user ID for
+      # a GitHub user ID, if one was found.
+      #
+      # When importing from Github Enterprise, do not query user by Github ID
+      # since we only have users' Github ID from github.com.
       def id_for_github_id(id)
-        gitlab_id = query_id_for_github_id(id) || nil
+        gitlab_id =
+          if project.github_enterprise_import?
+            nil
+          else
+            query_id_for_github_id(id)
+          end
 
         Gitlab::Cache::Import::Caching.write(ID_CACHE_KEY % id, gitlab_id)
       end

spec/lib/gitlab/github_import/user_finder_spec.rb

@@ -3,7 +3,14 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::GithubImport::UserFinder, :clean_gitlab_redis_cache do
-  let(:project) { create(:project) }
+  let(:project) do
+    create(
+      :project,
+      import_type: 'github',
+      import_url: 'https://api.github.com/user/repo'
+    )
+  end
+
   let(:client) { double(:client) }
   let(:finder) { described_class.new(project, client) }
 
@@ -263,6 +270,26 @@ RSpec.describe Gitlab::GithubImport::UserFinder, :clean_gitlab_redis_cache do
 
       finder.id_for_github_id(id)
     end
+
+    context 'when importing from github enterprise' do
+      let(:project) do
+        create(
+          :project,
+          import_type: 'github',
+          import_url: 'https://othergithub.net/user/repo'
+        )
+      end
+
+      it 'does not look up the user by external id' do
+        expect(finder).not_to receive(:query_id_for_github_id)
+
+        expect(Gitlab::Cache::Import::Caching)
+          .to receive(:write)
+          .with(described_class::ID_CACHE_KEY % id, nil)
+
+        finder.id_for_github_id(id)
+      end
+    end
   end
 
   describe '#id_for_github_email' do

spec/models/project_spec.rb

@@ -2872,6 +2872,36 @@ RSpec.describe Project, factory_default: :keep do
     it { expect(project.import?).to be true }
   end
 
+  describe '#github_import?' do
+    let_it_be(:project) { build(:project, import_type: 'github') }
+
+    it { expect(project.github_import?).to be true }
+  end
+
+  describe '#github_enterprise_import?' do
+    let_it_be(:github_com_project) do
+      build(
+        :project,
+        import_type: 'github',
+        import_url: 'https://api.github.com/user/repo'
+      )
+    end
+
+    let_it_be(:github_enterprise_project) do
+      build(
+        :project,
+        import_type: 'github',
+        import_url: 'https://othergithub.net/user/repo'
+      )
+    end
+
+    it { expect(github_com_project.github_import?).to be true }
+    it { expect(github_com_project.github_enterprise_import?).to be false }
+
+    it { expect(github_enterprise_project.github_import?).to be true }
+    it { expect(github_enterprise_project.github_enterprise_import?).to be true }
+  end
+
   describe '#remove_import_data' do
     let(:import_data) { ProjectImportData.new(data: { 'test' => 'some data' }) }