Merge branch 'revert-3a75f033' into 'master'

Revert "Remove use of LegacyGithubClient in GithubController"

See merge request gitlab-org/gitlab!38819

app/controllers/import/gitea_controller.rb

@@ -54,16 +54,6 @@ class Import::GiteaController < Import::GithubController
     end
   end
 
-  override :client_repos
-  def client_repos
-    @client_repos ||= filtered(client.repos)
-  end
-
-  override :client
-  def client
-    @client ||= Gitlab::LegacyGithubImport::Client.new(session[access_token_key], client_options)
-  end
-
   override :client_options
   def client_options
     { host: provider_url, api_version: 'v1' }

app/controllers/import/github_controller.rb

@@ -10,9 +10,6 @@ class Import::GithubController < Import::BaseController
   before_action :provider_auth, only: [:status, :realtime_changes, :create]
   before_action :expire_etag_cache, only: [:status, :create]
 
-  OAuthConfigMissingError = Class.new(StandardError)
-
-  rescue_from OAuthConfigMissingError, with: :missing_oauth_config
   rescue_from Octokit::Unauthorized, with: :provider_unauthorized
   rescue_from Octokit::TooManyRequests, with: :provider_rate_limit
 
@@ -25,7 +22,7 @@ class Import::GithubController < Import::BaseController
   end
 
   def callback
-    session[access_token_key] = get_token(params[:code])
+    session[access_token_key] = client.get_token(params[:code])
     redirect_to status_import_url
   end
 
@@ -80,7 +77,9 @@ class Import::GithubController < Import::BaseController
   override :provider_url
   def provider_url
     strong_memoize(:provider_url) do
-      oauth_config&.dig('url').presence || 'https://github.com'
+      provider = Gitlab::Auth::OAuth::Provider.config_for('github')
+
+      provider&.dig('url').presence || 'https://github.com'
     end
   end
 
@@ -105,56 +104,11 @@ class Import::GithubController < Import::BaseController
   end
 
   def client
-    @client ||= if Feature.enabled?(:remove_legacy_github_client, default_enabled: false)
-                  Gitlab::GithubImport::Client.new(session[access_token_key])
-                else
-                  Gitlab::LegacyGithubImport::Client.new(session[access_token_key], client_options)
-                end
+    @client ||= Gitlab::LegacyGithubImport::Client.new(session[access_token_key], client_options)
   end
 
   def client_repos
-    @client_repos ||= filtered(client.octokit.repos)
-  end
-
-  def oauth_client
-    raise OAuthConfigMissingError unless oauth_config
-
-    @oauth_client ||= ::OAuth2::Client.new(
-      oauth_config.app_id,
-      oauth_config.app_secret,
-      oauth_options.merge(ssl: { verify: oauth_config['verify_ssl'] })
-    )
-  end
-
-  def oauth_config
-    @oauth_config ||= Gitlab::Auth::OAuth::Provider.config_for('github')
-  end
-
-  def oauth_options
-    if oauth_config
-      oauth_config.dig('args', 'client_options').deep_symbolize_keys
-    else
-      OmniAuth::Strategies::GitHub.default_options[:client_options].symbolize_keys
-    end
-  end
-
-  def authorize_url(redirect_uri)
-    if Feature.enabled?(:remove_legacy_github_client, default_enabled: false)
-      oauth_client.auth_code.authorize_url({
-                                             redirect_uri: redirect_uri,
-                                             scope: 'repo, user, user:email'
-                                           })
-    else
-      client.authorize_url(callback_import_url)
-    end
-  end
-
-  def get_token(code)
-    if Feature.enabled?(:remove_legacy_github_client, default_enabled: false)
-      oauth_client.auth_code.get_token(code).token
-    else
-      client.get_token(code)
-    end
+    @client_repos ||= filtered(client.repos)
   end
 
   def verify_import_enabled
@@ -162,7 +116,7 @@ class Import::GithubController < Import::BaseController
   end
 
   def go_to_provider_for_permissions
-    redirect_to authorize_url(callback_import_url)
+    redirect_to client.authorize_url(callback_import_url)
   end
 
   def import_enabled?
@@ -198,12 +152,6 @@ class Import::GithubController < Import::BaseController
       alert: _("GitHub API rate limit exceeded. Try again after %{reset_time}") % { reset_time: reset_time }
   end
 
-  def missing_oauth_config
-    session[access_token_key] = nil
-    redirect_to new_import_url,
-      alert: _('OAuth configuration for GitHub missing.')
-  end
-
   def access_token_key
     :"#{provider_name}_access_token"
   end

app/services/import/github_service.rb

@@ -33,7 +33,7 @@ module Import
     end
 
     def repo
-      @repo ||= client.repository(params[:repo_id].to_i)
+      @repo ||= client.repo(params[:repo_id].to_i)
     end
 
     def project_name

ee/spec/features/projects/new_project_spec.rb

@@ -115,18 +115,14 @@ RSpec.describe 'New project' do
 
         expect(page).to have_text('Authenticate with GitHub')
 
-        allow_next_instance_of(Octokit::Client) do |client|
-          allow(client).to receive(:repos).and_return([repo])
-        end
+        allow_any_instance_of(Gitlab::LegacyGithubImport::Client).to receive(:repos).and_return([repo])
 
         fill_in 'personal_access_token', with: 'fake-token'
         click_button 'Authenticate'
         wait_for_requests
 
         # Mock the POST `/import/github`
-        allow_next_instance_of(Gitlab::GithubImport::Client) do |client|
-          allow(client).to receive(:repository).and_return(repo)
-        end
+        allow_any_instance_of(Gitlab::LegacyGithubImport::Client).to receive(:repo).and_return(repo)
         project = create(:project, name: 'some-github-repo', creator: user, import_type: 'github')
         create(:import_state, :finished, import_url: repo.clone_url, project: project)
         allow_any_instance_of(CiCd::SetupProject).to receive(:setup_external_service)
@@ -154,9 +150,7 @@ RSpec.describe 'New project' do
           find('.js-import-github').click
         end
 
-        allow_next_instance_of(Octokit::Client) do |client|
-          allow(client).to receive(:repos).and_raise(Octokit::Unauthorized)
-        end
+        allow_any_instance_of(Gitlab::LegacyGithubImport::Client).to receive(:repos).and_raise(Octokit::Unauthorized)
 
         fill_in 'personal_access_token', with: 'unauthorized-fake-token'
         click_button 'Authenticate'

lib/api/import_github.rb

@@ -10,11 +10,7 @@ module API
 
     helpers do
       def client
-        @client ||= if Feature.enabled?(:remove_legacy_github_client, default_enabled: false)
-                      Gitlab::GithubImport::Client.new(params[:personal_access_token])
-                    else
-                      Gitlab::LegacyGithubImport::Client.new(params[:personal_access_token], client_options)
-                    end
+        @client ||= Gitlab::LegacyGithubImport::Client.new(params[:personal_access_token], client_options)
       end
 
       def access_params

lib/gitlab/legacy_github_import/client.rb

@@ -32,8 +32,6 @@ module Gitlab
         )
       end
 
-      alias_method :octokit, :api
-
       def client
         unless config
           raise Projects::ImportService::Error,

locale/gitlab.pot

@@ -16616,9 +16616,6 @@ msgstr ""
 msgid "Number of files touched"
 msgstr ""
 
-msgid "OAuth configuration for GitHub missing."
-msgstr ""
-
 msgid "OK"
 msgstr ""
 

spec/controllers/import/github_controller_spec.rb

@@ -15,7 +15,10 @@ RSpec.describe Import::GithubController do
     it "redirects to GitHub for an access token if logged in with GitHub" do
       allow(controller).to receive(:logged_in_with_provider?).and_return(true)
       expect(controller).to receive(:go_to_provider_for_permissions).and_call_original
-      allow(controller).to receive(:authorize_url).with(users_import_github_callback_url).and_call_original
+      allow_any_instance_of(Gitlab::LegacyGithubImport::Client)
+        .to receive(:authorize_url)
+        .with(users_import_github_callback_url)
+        .and_call_original
 
       get :new
 
@@ -43,15 +46,13 @@ RSpec.describe Import::GithubController do
   end
 
   describe "GET callback" do
-    before do
-      allow(controller).to receive(:get_token).and_return(token)
-      allow(controller).to receive(:oauth_options).and_return({})
-
-      stub_omniauth_provider('github')
-    end
-
     it "updates access token" do
       token = "asdasd12345"
+      allow_any_instance_of(Gitlab::LegacyGithubImport::Client)
+        .to receive(:get_token).and_return(token)
+      allow_any_instance_of(Gitlab::LegacyGithubImport::Client)
+        .to receive(:github_options).and_return({})
+      stub_omniauth_provider('github')
 
       get :callback
 
@@ -66,54 +67,6 @@ RSpec.describe Import::GithubController do
 
   describe "GET status" do
     it_behaves_like 'a GitHub-ish import controller: GET status'
-
-    context 'when using OAuth' do
-      before do
-        allow(controller).to receive(:logged_in_with_provider?).and_return(true)
-      end
-
-      context 'when OAuth config is missing' do
-        let(:new_import_url) { public_send("new_import_#{provider}_url") }
-
-        before do
-          allow(controller).to receive(:oauth_config).and_return(nil)
-        end
-
-        it 'returns missing config error' do
-          expect(controller).to receive(:go_to_provider_for_permissions).and_call_original
-
-          get :status
-
-          expect(session[:"#{provider}_access_token"]).to be_nil
-          expect(controller).to redirect_to(new_import_url)
-          expect(flash[:alert]).to eq('OAuth configuration for GitHub missing.')
-        end
-      end
-    end
-
-    context 'when feature remove_legacy_github_client is disabled' do
-      before do
-        stub_feature_flags(remove_legacy_github_client: false)
-      end
-
-      it 'uses Gitlab::LegacyGitHubImport::Client' do
-        expect(controller.send(:client)).to be_instance_of(Gitlab::LegacyGithubImport::Client)
-
-        get :status
-      end
-    end
-
-    context 'when feature remove_legacy_github_client is enabled' do
-      before do
-        stub_feature_flags(remove_legacy_github_client: true)
-      end
-
-      it 'uses Gitlab::GithubImport::Client' do
-        expect(controller.send(:client)).to be_instance_of(Gitlab::GithubImport::Client)
-
-        get :status
-      end
-    end
   end
 
   describe "POST create" do

spec/requests/api/import_github_spec.rb

@@ -22,7 +22,7 @@ RSpec.describe API::ImportGithub do
 
     before do
       Grape::Endpoint.before_each do |endpoint|
-        allow(endpoint).to receive(:client).and_return(double('client', user: provider_user, repository: provider_repo).as_null_object)
+        allow(endpoint).to receive(:client).and_return(double('client', user: provider_user, repo: provider_repo).as_null_object)
       end
     end
 

spec/services/import/github_service_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe Import::GithubService do
   let_it_be(:user) { create(:user) }
   let_it_be(:token) { 'complex-token' }
   let_it_be(:access_params) { { github_access_token: 'github-complex-token' } }
-  let_it_be(:client) { Gitlab::GithubImport::Client.new(token) }
+  let_it_be(:client) { Gitlab::LegacyGithubImport::Client.new(token) }
   let_it_be(:params) { { repo_id: 123, new_name: 'new_repo', target_namespace: 'root' } }
 
   let(:subject) { described_class.new(client, user, params) }
@@ -19,7 +19,7 @@ RSpec.describe Import::GithubService do
     let(:exception) { Octokit::ClientError.new(status: 404, body: 'Not Found') }
 
     before do
-      expect(client).to receive(:repository).and_raise(exception)
+      expect(client).to receive(:repo).and_raise(exception)
     end
 
     it 'logs the original error' do
@@ -46,7 +46,7 @@ RSpec.describe Import::GithubService do
   it 'raises an exception for unknown error causes' do
     exception = StandardError.new('Not Implemented')
 
-    expect(client).to receive(:repository).and_raise(exception)
+    expect(client).to receive(:repo).and_raise(exception)
 
     expect(Gitlab::Import::Logger).not_to receive(:error)
 

spec/support/shared_examples/controllers/githubish_import_controller_shared_examples.rb

@@ -111,11 +111,8 @@ RSpec.shared_examples 'a GitHub-ish import controller: GET status' do
   end
 
   it "handles an invalid access token" do
-    allow_any_instance_of(Gitlab::LegacyGithubImport::Client).to receive(:repos).and_raise(Octokit::Unauthorized)
-
-    allow_next_instance_of(Octokit::Client) do |client|
-      allow(client).to receive(:repos).and_raise(Octokit::Unauthorized)
-    end
+    allow_any_instance_of(Gitlab::LegacyGithubImport::Client)
+      .to receive(:repos).and_raise(Octokit::Unauthorized)
 
     get :status
 
@@ -190,7 +187,7 @@ RSpec.shared_examples 'a GitHub-ish import controller: POST create' do
   end
 
   before do
-    stub_client(user: provider_user, repo: provider_repo, repository: provider_repo)
+    stub_client(user: provider_user, repo: provider_repo)
     assign_session_token(provider)
   end