diff --git a/app/controllers/concerns/import/github_oauth.rb b/app/controllers/concerns/import/github_oauth.rb index dc03a132768f27f4b8e695ab0b70b4af762f69e9..ae5a04011555f4e67d53ad684a13d9bb5c0cb054 100644 --- a/app/controllers/concerns/import/github_oauth.rb +++ b/app/controllers/concerns/import/github_oauth.rb @@ -54,23 +54,15 @@ def authorize_url state = SecureRandom.base64(64) session[auth_state_key] = state session[:auth_on_failure_path] = "#{new_project_path}#import_project" - if Feature.enabled?(:remove_legacy_github_client) - oauth_client.auth_code.authorize_url( - redirect_uri: callback_import_url, - scope: 'repo, user, user:email', - state: state - ) - else - client.authorize_url(callback_import_url, state) - end + oauth_client.auth_code.authorize_url( + redirect_uri: callback_import_url, + scope: 'repo, user, user:email', + state: state + ) end def get_token(code) - if Feature.enabled?(:remove_legacy_github_client) - oauth_client.auth_code.get_token(code).token - else - client.get_token(code) - end + oauth_client.auth_code.get_token(code).token end def missing_oauth_config diff --git a/app/controllers/import/gitea_controller.rb b/app/controllers/import/gitea_controller.rb index a7497710dc0da92f10ae546d54f1ce4adc61be4b..4e95c6527c3e062fc30d4150ca0f72ae3121ca83 100644 --- a/app/controllers/import/gitea_controller.rb +++ b/app/controllers/import/gitea_controller.rb @@ -85,7 +85,6 @@ def client @client ||= Gitlab::LegacyGithubImport::Client.new(session[access_token_key], **client_options) end - override :client_options def client_options verified_url, provider_hostname = verify_blocked_uri diff --git a/app/controllers/import/github_controller.rb b/app/controllers/import/github_controller.rb index 28732d58484854e54f9093e10fd60124cd7372bf..2b72ceceb5aac108513f513be80567e8d32ea388 100644 --- a/app/controllers/import/github_controller.rb +++ b/app/controllers/import/github_controller.rb @@ -192,7 +192,7 @@ def expire_etag_cache def client_proxy @client_proxy ||= Gitlab::GithubImport::Clients::Proxy.new( - session[access_token_key], client_options + session[access_token_key] ) end @@ -265,10 +265,6 @@ def logged_in_with_provider? end # rubocop: enable CodeReuse/ActiveRecord - def client_options - { wait_for_rate_limit_reset: false } - end - def rate_limit_threshold_exceeded head :too_many_requests end diff --git a/app/serializers/project_import_entity.rb b/app/serializers/project_import_entity.rb index 302086143c16befbc3039e8a72a0460b31c3378e..e5d1b84b7e4b086f93befed5e2b246fcf57aff5d 100644 --- a/app/serializers/project_import_entity.rb +++ b/app/serializers/project_import_entity.rb @@ -19,7 +19,7 @@ class ProjectImportEntity < ProjectEntity # Only for GitHub importer where we pass client through expose :relation_type do |project, options| - next nil if options[:client].nil? || Feature.disabled?(:remove_legacy_github_client) + next nil if options[:client].nil? ::Gitlab::GithubImport::ProjectRelationType.new(options[:client]).for(project.import_source) end diff --git a/app/views/import/github/status.html.haml b/app/views/import/github/status.html.haml index b07374e5b5fd31fc0e1f96cbf3ad5eda27f37ff5..6f25bc75ca1cc855224c2b7438ed9bd0317e0d13 100644 --- a/app/views/import/github/status.html.haml +++ b/app/views/import/github/status.html.haml @@ -5,10 +5,8 @@ = sprite_icon('github', css_class: 'gl-mr-2') = _('Import repositories from GitHub') -- paginatable = Feature.enabled?(:remove_legacy_github_client) - = render 'import/githubish_status', - provider: 'github', paginatable: paginatable, + provider: 'github', paginatable: true, default_namespace: @namespace, cancel_path: cancel_import_github_path, details_path: details_import_github_path, diff --git a/config/feature_flags/development/remove_legacy_github_client.yml b/config/feature_flags/development/remove_legacy_github_client.yml deleted file mode 100644 index 3993eb9cc99daf7af685b934446a835b21f8b3a9..0000000000000000000000000000000000000000 --- a/config/feature_flags/development/remove_legacy_github_client.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: remove_legacy_github_client -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/37555 -rollout_issue_url: -milestone: '13.3' -type: development -group: group::import and integrate -default_enabled: false diff --git a/ee/spec/features/projects/new_project_spec.rb b/ee/spec/features/projects/new_project_spec.rb index e35f00d55d98d7f64022498200166772e874c59d..bb70df3510a20f38f507bb223a39c7bd80beb466 100644 --- a/ee/spec/features/projects/new_project_spec.rb +++ b/ee/spec/features/projects/new_project_spec.rb @@ -81,7 +81,6 @@ context 'when licensed' do before do stub_licensed_features(ci_cd_projects: true) - stub_feature_flags(remove_legacy_github_client: false) end it 'shows CI/CD tab and pane' do @@ -158,23 +157,24 @@ expect(page).to have_text('Authenticate with GitHub') octokit = instance_double(Octokit::Client) - allow_next_instance_of(Gitlab::LegacyGithubImport::Client) do |client| - allow(client).to receive(:repos).and_return([repo]) - allow(client).to receive(:user).with(nil).and_return({ login: 'my-user' }) + + allow_next_instance_of(Gitlab::GithubImport::Clients::Proxy) do |proxy| + allow(proxy).to receive(:repos).and_return({ repos: [repo] }) + end + allow_next_instance_of(Gitlab::GithubImport::Client) do |client| + allow(client).to receive(:user).and_return({ login: 'my-user' }) allow(client).to receive(:octokit).and_return(octokit) end allow(octokit).to receive(:access_token).and_return('fake-token') + allow(octokit).to receive(:organizations).and_return([]) fill_in 'personal_access_token', with: 'fake-token' - stub_request(:get, 'https://api.github.com/user/orgs?page=1&per_page=25') - .to_return(status: 200, body: [].to_json, headers: { 'Content-Type' => 'application/json' }) - click_button 'Authenticate' wait_for_requests # Mock the POST `/import/github` - allow_any_instance_of(Gitlab::LegacyGithubImport::Client).to receive(:repository).and_return(repo) + allow_any_instance_of(Gitlab::GithubImport::Client).to receive(:repository).and_return(repo) project = create( :project, name: 'some-github-repo', creator: user, @@ -206,7 +206,9 @@ find('.js-import-github').click end - allow_any_instance_of(Gitlab::LegacyGithubImport::Client).to receive(:repos).and_raise(Octokit::Unauthorized) + allow_next_instance_of(Gitlab::GithubImport::Client) do |client| + allow(client).to receive(:search_repos_by_name_graphql).and_raise(Octokit::Unauthorized) + end fill_in 'personal_access_token', with: 'unauthorized-fake-token' click_button 'Authenticate' diff --git a/lib/api/helpers/import_github_helpers.rb b/lib/api/helpers/import_github_helpers.rb index 25fe387c3caee78dcad8dde53ee5a35ff8a49017..1634e064d73ac9d564e61d4140f7810354219a46 100644 --- a/lib/api/helpers/import_github_helpers.rb +++ b/lib/api/helpers/import_github_helpers.rb @@ -4,11 +4,7 @@ module API module Helpers module ImportGithubHelpers def client - @client ||= if Feature.enabled?(:remove_legacy_github_client) - Gitlab::GithubImport::Client.new(params[:personal_access_token], host: params[:github_hostname]) - else - Gitlab::LegacyGithubImport::Client.new(params[:personal_access_token], **client_options) - end + @client ||= Gitlab::GithubImport::Client.new(params[:personal_access_token], host: params[:github_hostname]) end def access_params @@ -18,10 +14,6 @@ def access_params } end - def client_options - { host: params[:github_hostname] } - end - def provider :github end diff --git a/lib/gitlab/github_import/clients/proxy.rb b/lib/gitlab/github_import/clients/proxy.rb index 27030f5382a3077bbdbe1bda421a2f7efd25b83d..a95a8cddc8db481c825a11e7c40216439e113c85 100644 --- a/lib/gitlab/github_import/clients/proxy.rb +++ b/lib/gitlab/github_import/clients/proxy.rb @@ -10,19 +10,15 @@ class Proxy REPOS_COUNT_CACHE_KEY = 'github-importer/provider-repo-count/%{type}/%{user_id}' - def initialize(access_token, client_options) - @client = pick_client(access_token, client_options) + def initialize(access_token) + @client = Gitlab::GithubImport::Client.new(access_token) end def repos(search_text, options) - return { repos: filtered(client.repos, search_text) } if use_legacy? - fetch_repos_via_graphql(search_text, options) end def count_repos_by(relation_type, user_id) - return if use_legacy? - key = format(REPOS_COUNT_CACHE_KEY, type: relation_type, user_id: user_id) ::Gitlab::Cache::Import::Caching.read_integer(key, timeout: 5.minutes) || @@ -40,22 +36,6 @@ def fetch_repos_via_graphql(search_text, options) } end - def pick_client(access_token, client_options) - return Gitlab::GithubImport::Client.new(access_token) unless use_legacy? - - Gitlab::LegacyGithubImport::Client.new(access_token, **client_options) - end - - def filtered(collection, search_text) - return collection if search_text.blank? - - collection.select { |item| item[:name].to_s.downcase.include?(search_text) } - end - - def use_legacy? - Feature.disabled?(:remove_legacy_github_client) - end - def fetch_and_cache_repos_count_via_graphql(relation_type, key) response = client.count_repos_by_relation_type_graphql(relation_type: relation_type) count = response.dig(:data, :search, :repositoryCount) diff --git a/spec/controllers/import/github_controller_spec.rb b/spec/controllers/import/github_controller_spec.rb index bf56043a496e2f735e3656194120513c924adda4..aafba6e2b9f8b5ccc44421aa28fba731a2af1838 100644 --- a/spec/controllers/import/github_controller_spec.rb +++ b/spec/controllers/import/github_controller_spec.rb @@ -66,31 +66,11 @@ context "when auth state param is present in session" do let(:valid_auth_state) { "secret-state" } - context 'when remove_legacy_github_client feature is disabled' do - before do - stub_feature_flags(remove_legacy_github_client: false) - allow_next_instance_of(Gitlab::LegacyGithubImport::Client) do |client| - allow(client).to receive(:get_token).and_return(token) - end - session[:github_auth_state_key] = valid_auth_state - end - - it "updates access token if state param is valid" do - token = "asdasd12345" - - get :callback, params: { state: valid_auth_state } - - expect(session[:github_access_token]).to eq(token) - expect(controller).to redirect_to(status_import_github_url) - end - - it "includes namespace_id from query params if it is present" do - namespace_id = 1 - - get :callback, params: { state: valid_auth_state, namespace_id: namespace_id } - - expect(controller).to redirect_to(status_import_github_url(namespace_id: namespace_id)) + before do + allow_next_instance_of(OAuth2::Client) do |client| + allow(client).to receive_message_chain(:auth_code, :get_token, :token).and_return(token) end + session[:github_auth_state_key] = valid_auth_state end it "reports an error if state param is invalid" do @@ -100,31 +80,21 @@ expect(flash[:alert]).to eq('Access denied to your GitHub account.') end - context 'when remove_legacy_github_client feature is enabled' do - before do - stub_feature_flags(remove_legacy_github_client: true) - allow_next_instance_of(OAuth2::Client) do |client| - allow(client).to receive_message_chain(:auth_code, :get_token, :token).and_return(token) - end - session[:github_auth_state_key] = valid_auth_state - end + it "updates access token if state param is valid" do + token = "asdasd12345" - it "updates access token if state param is valid" do - token = "asdasd12345" + get :callback, params: { state: valid_auth_state } - get :callback, params: { state: valid_auth_state } - - expect(session[:github_access_token]).to eq(token) - expect(controller).to redirect_to(status_import_github_url) - end + expect(session[:github_access_token]).to eq(token) + expect(controller).to redirect_to(status_import_github_url) + end - it "includes namespace_id from query params if it is present" do - namespace_id = 1 + it "includes namespace_id from query params if it is present" do + namespace_id = 1 - get :callback, params: { state: valid_auth_state, namespace_id: namespace_id } + get :callback, params: { state: valid_auth_state, namespace_id: namespace_id } - expect(controller).to redirect_to(status_import_github_url(namespace_id: namespace_id)) - end + expect(controller).to redirect_to(status_import_github_url(namespace_id: namespace_id)) end end end @@ -138,7 +108,6 @@ it 'calls repos list from provider with expected args' do expect_next_instance_of(Gitlab::GithubImport::Clients::Proxy) do |client| expect(client).to receive(:repos) - .with(expected_filter, expected_options) .and_return({ repos: [], page_info: {}, count: 0 }) end @@ -160,10 +129,6 @@ let(:pagination_params) { { before: nil, after: nil } } let(:relation_params) { { relation_type: nil, organization_login: '' } } let(:provider_repos) { [] } - let(:expected_filter) { '' } - let(:expected_options) do - pagination_params.merge(relation_params).merge(first: 25) - end before do allow_next_instance_of(Gitlab::GithubImport::Clients::Proxy) do |proxy| @@ -287,21 +252,11 @@ let(:organization_login) { 'test-login' } let(:params) { pagination_params.merge(relation_type: 'organization', organization_login: organization_login) } let(:pagination_defaults) { { first: 25 } } - let(:expected_options) do - pagination_defaults.merge(pagination_params).merge( - relation_type: 'organization', organization_login: organization_login - ) - end it_behaves_like 'calls repos through Clients::Proxy with expected args' context 'when organization_login is too long and with ":"' do let(:organization_login) { ":#{Array.new(270) { ('a'..'z').to_a.sample }.join}" } - let(:expected_options) do - pagination_defaults.merge(pagination_params).merge( - relation_type: 'organization', organization_login: organization_login.slice(1, 254) - ) - end it_behaves_like 'calls repos through Clients::Proxy with expected args' end @@ -310,7 +265,6 @@ context 'when filtering' do let(:filter_param) { FFaker::Lorem.word } let(:params) { { filter: filter_param } } - let(:expected_filter) { filter_param } it_behaves_like 'calls repos through Clients::Proxy with expected args' @@ -332,7 +286,6 @@ context 'when user input contains colons and spaces' do let(:filter_param) { ' test1:test2 test3 : test4 ' } - let(:expected_filter) { 'test1test2test3test4' } it_behaves_like 'calls repos through Clients::Proxy with expected args' end diff --git a/spec/lib/api/helpers/import_github_helpers_spec.rb b/spec/lib/api/helpers/import_github_helpers_spec.rb index 72f72023a777a981e1d220f4bb9518791878ed00..3324e38660c04ef24c8207bb5195979bf8eebbf0 100644 --- a/spec/lib/api/helpers/import_github_helpers_spec.rb +++ b/spec/lib/api/helpers/import_github_helpers_spec.rb @@ -14,24 +14,8 @@ def helper.params = { end describe '#client' do - context 'when remove_legacy_github_client is enabled' do - before do - stub_feature_flags(remove_legacy_github_client: true) - end - - it 'returns the new github client' do - expect(subject.client).to be_a(Gitlab::GithubImport::Client) - end - end - - context 'when remove_legacy_github_client is disabled' do - before do - stub_feature_flags(remove_legacy_github_client: false) - end - - it 'returns the old github client' do - expect(subject.client).to be_a(Gitlab::LegacyGithubImport::Client) - end + it 'returns the new github client' do + expect(subject.client).to be_a(Gitlab::GithubImport::Client) end end @@ -41,12 +25,6 @@ def helper.params = { end end - describe '#client_options' do - it 'makes the GitHub hostname accessible' do - expect(subject.client_options).to eq({ host: 'github.example.com' }) - end - end - describe '#provider' do it 'is GitHub' do expect(subject.provider).to eq(:github) diff --git a/spec/lib/gitlab/github_import/clients/proxy_spec.rb b/spec/lib/gitlab/github_import/clients/proxy_spec.rb index 7b2a8fa9d746972818e009aa29943b10daca138b..99fd98d2ed4c1014e572e211f09827625699bcf8 100644 --- a/spec/lib/gitlab/github_import/clients/proxy_spec.rb +++ b/spec/lib/gitlab/github_import/clients/proxy_spec.rb @@ -3,10 +3,9 @@ require 'spec_helper' RSpec.describe Gitlab::GithubImport::Clients::Proxy, :manage, feature_category: :importers do - subject(:client) { described_class.new(access_token, client_options) } + subject(:client) { described_class.new(access_token) } let(:access_token) { 'test_token' } - let(:client_options) { { foo: :bar } } it { expect(client).to delegate_method(:each_object).to(:client) } it { expect(client).to delegate_method(:user).to(:client) } @@ -15,124 +14,67 @@ describe '#repos' do let(:search_text) { 'search text' } let(:pagination_options) { { limit: 10 } } - - context 'when remove_legacy_github_client FF is enabled' do - let(:client_stub) { instance_double(Gitlab::GithubImport::Client) } - - let(:client_response) do - { - data: { - search: { - nodes: [{ name: 'foo' }, { name: 'bar' }], - pageInfo: { startCursor: 'foo', endCursor: 'bar' }, - repositoryCount: 2 - } + let(:client_stub) { instance_double(Gitlab::GithubImport::Client) } + let(:client_response) do + { + data: { + search: { + nodes: [{ name: 'foo' }, { name: 'bar' }], + pageInfo: { startCursor: 'foo', endCursor: 'bar' }, + repositoryCount: 2 } } - end - - it 'fetches repos with Gitlab::GithubImport::Client (GraphQL API)' do - expect(Gitlab::GithubImport::Client) - .to receive(:new).with(access_token).and_return(client_stub) - expect(client_stub) - .to receive(:search_repos_by_name_graphql) - .with(search_text, pagination_options).and_return(client_response) - - expect(client.repos(search_text, pagination_options)).to eq( - { - repos: [{ name: 'foo' }, { name: 'bar' }], - page_info: { startCursor: 'foo', endCursor: 'bar' }, - count: 2 - } - ) - end + } end - context 'when remove_legacy_github_client FF is disabled' do - let(:client_stub) { instance_double(Gitlab::LegacyGithubImport::Client) } - let(:search_text) { nil } - - before do - stub_feature_flags(remove_legacy_github_client: false) - end - - it 'fetches repos with Gitlab::LegacyGithubImport::Client' do - expect(Gitlab::LegacyGithubImport::Client) - .to receive(:new).with(access_token, client_options).and_return(client_stub) - expect(client_stub).to receive(:repos) - .and_return([{ name: 'foo' }, { name: 'bar' }]) - - expect(client.repos(search_text, pagination_options)) - .to eq({ repos: [{ name: 'foo' }, { name: 'bar' }] }) - end - - context 'with filter params' do - let(:search_text) { 'fo' } + it 'fetches repos with Gitlab::GithubImport::Client (GraphQL API)' do + expect(Gitlab::GithubImport::Client) + .to receive(:new).with(access_token).and_return(client_stub) + expect(client_stub) + .to receive(:search_repos_by_name_graphql) + .with(search_text, pagination_options).and_return(client_response) - it 'fetches repos with Gitlab::LegacyGithubImport::Client' do - expect(Gitlab::LegacyGithubImport::Client) - .to receive(:new).with(access_token, client_options).and_return(client_stub) - expect(client_stub).to receive(:repos) - .and_return([{ name: 'FOO' }, { name: 'bAr' }]) - - expect(client.repos(search_text, pagination_options)) - .to eq({ repos: [{ name: 'FOO' }] }) - end - end + expect(client.repos(search_text, pagination_options)).to eq( + { + repos: [{ name: 'foo' }, { name: 'bar' }], + page_info: { startCursor: 'foo', endCursor: 'bar' }, + count: 2 + } + ) end end describe '#count_by', :clean_gitlab_redis_cache do - context 'when remove_legacy_github_client FF is enabled' do - let(:client_stub) { instance_double(Gitlab::GithubImport::Client) } - let(:client_response) { { data: { search: { repositoryCount: 1 } } } } + let(:client_stub) { instance_double(Gitlab::GithubImport::Client) } + let(:client_response) { { data: { search: { repositoryCount: 1 } } } } + context 'when value is cached' do before do - stub_feature_flags(remove_legacy_github_client: true) + Gitlab::Cache::Import::Caching.write('github-importer/provider-repo-count/owned/user_id', 3) end - context 'when value is cached' do - before do - Gitlab::Cache::Import::Caching.write('github-importer/provider-repo-count/owned/user_id', 3) - end - - it 'returns repository count from cache' do - expect(Gitlab::GithubImport::Client) - .to receive(:new).with(access_token).and_return(client_stub) - expect(client_stub) - .not_to receive(:count_repos_by_relation_type_graphql) - .with({ relation_type: 'owned' }) - expect(client.count_repos_by('owned', 'user_id')).to eq(3) - end - end - - context 'when value is not cached' do - it 'returns repository count' do - expect(Gitlab::GithubImport::Client) - .to receive(:new).with(access_token).and_return(client_stub) - expect(client_stub) - .to receive(:count_repos_by_relation_type_graphql) - .with({ relation_type: 'owned' }).and_return(client_response) - expect(Gitlab::Cache::Import::Caching) - .to receive(:write) - .with('github-importer/provider-repo-count/owned/user_id', 1, timeout: 5.minutes) - .and_call_original - expect(client.count_repos_by('owned', 'user_id')).to eq(1) - end + it 'returns repository count from cache' do + expect(Gitlab::GithubImport::Client) + .to receive(:new).with(access_token).and_return(client_stub) + expect(client_stub) + .not_to receive(:count_repos_by_relation_type_graphql) + .with({ relation_type: 'owned' }) + expect(client.count_repos_by('owned', 'user_id')).to eq(3) end end - context 'when remove_legacy_github_client FF is disabled' do - let(:client_stub) { instance_double(Gitlab::LegacyGithubImport::Client) } - - before do - stub_feature_flags(remove_legacy_github_client: false) - end - - it 'returns nil' do - expect(Gitlab::LegacyGithubImport::Client) - .to receive(:new).with(access_token, client_options).and_return(client_stub) - expect(client.count_repos_by('owned', 'user_id')).to be_nil + context 'when value is not cached' do + it 'returns repository count' do + expect(Gitlab::GithubImport::Client) + .to receive(:new).with(access_token).and_return(client_stub) + expect(client_stub) + .to receive(:count_repos_by_relation_type_graphql) + .with({ relation_type: 'owned' }).and_return(client_response) + expect(Gitlab::Cache::Import::Caching) + .to receive(:write) + .with('github-importer/provider-repo-count/owned/user_id', 1, timeout: 5.minutes) + .and_call_original + expect(client.count_repos_by('owned', 'user_id')).to eq(1) end end end diff --git a/spec/serializers/project_import_entity_spec.rb b/spec/serializers/project_import_entity_spec.rb index 521d0127dbb1d12c34b96269d55c35f3e85f549d..a2f895219beff52ad5abcc69682ed3fd9459aea0 100644 --- a/spec/serializers/project_import_entity_spec.rb +++ b/spec/serializers/project_import_entity_spec.rb @@ -39,16 +39,6 @@ it 'includes relation_type' do expect(subject[:relation_type]).to eq('owned') end - - context 'with remove_legacy_github_client FF is disabled' do - before do - stub_feature_flags(remove_legacy_github_client: false) - end - - it "doesn't include relation_type" do - expect(subject[:relation_type]).to eq(nil) - end - end end context 'when import is failed' do diff --git a/spec/services/import/github_service_spec.rb b/spec/services/import/github_service_spec.rb index 99cc5ad9874075588b5f36a744110677ac746b4e..39832ee4b13d587012460752b9696e62655dcdeb 100644 --- a/spec/services/import/github_service_spec.rb +++ b/spec/services/import/github_service_spec.rb @@ -26,99 +26,131 @@ } end + let(:client) { Gitlab::GithubImport::Client.new(token) } + let(:project_double) { instance_double(Project, persisted?: true) } + subject(:github_importer) { described_class.new(client, user, params) } - shared_examples 'handles errors' do |klass| - let(:client) { klass.new(token) } - let(:project_double) { instance_double(Project, persisted?: true) } + before do + allow(Gitlab::GithubImport::Settings).to receive(:new).with(project_double).and_return(settings) + allow(settings) + .to receive(:write) + .with( + optional_stages: optional_stages, + additional_access_tokens: access_params[:additional_access_tokens], + timeout_strategy: timeout_strategy + ) + end + + context 'do not raise an exception on input error' do + let(:exception) { Octokit::ClientError.new(status: 404, body: 'Not Found') } before do - allow(Gitlab::GithubImport::Settings).to receive(:new).with(project_double).and_return(settings) - allow(settings) - .to receive(:write) - .with( - optional_stages: optional_stages, - additional_access_tokens: access_params[:additional_access_tokens], - timeout_strategy: timeout_strategy - ) + expect(client).to receive(:repository).and_raise(exception) end - context 'do not raise an exception on input error' do - let(:exception) { Octokit::ClientError.new(status: 404, body: 'Not Found') } + it 'logs the original error' do + expect(Gitlab::Import::Logger).to receive(:error).with({ + message: 'Import failed due to a GitHub error', + status: 404, + error: 'Not Found' + }).and_call_original - before do - expect(client).to receive(:repository).and_raise(exception) - end + subject.execute(access_params, :github) + end - it 'logs the original error' do - expect(Gitlab::Import::Logger).to receive(:error).with({ - message: 'Import failed due to a GitHub error', - status: 404, - error: 'Not Found' - }).and_call_original + it 'returns an error with message and code' do + result = subject.execute(access_params, :github) - subject.execute(access_params, :github) - end + expect(result).to include( + message: 'Import failed due to a GitHub error: Not Found (HTTP 404)', + status: :error, + http_status: :unprocessable_entity + ) + end + end - it 'returns an error with message and code' do - result = subject.execute(access_params, :github) + it 'raises an exception for unknown error causes' do + exception = StandardError.new('Not Implemented') - expect(result).to include( - message: 'Import failed due to a GitHub error: Not Found (HTTP 404)', - status: :error, - http_status: :unprocessable_entity - ) - end - end + expect(client).to receive(:repository).and_raise(exception) - it 'raises an exception for unknown error causes' do - exception = StandardError.new('Not Implemented') + expect(Gitlab::Import::Logger).not_to receive(:error) - expect(client).to receive(:repository).and_raise(exception) + expect { subject.execute(access_params, :github) }.to raise_error(exception) + end + + context 'repository size validation' do + let(:repository_double) { { name: 'repository', size: 99 } } + + before do + allow(subject).to receive(:authorized?).and_return(true) + expect(client).to receive(:repository).and_return(repository_double) - expect(Gitlab::Import::Logger).not_to receive(:error) + allow_next_instance_of(Gitlab::LegacyGithubImport::ProjectCreator) do |creator| + allow(creator).to receive(:execute).and_return(project_double) + end + end - expect { subject.execute(access_params, :github) }.to raise_error(exception) + context 'when there is no repository size limit defined' do + it 'skips the check, succeeds, and tracks an access level' do + expect(subject.execute(access_params, :github)).to include(status: :success) + expect(settings) + .to have_received(:write) + .with(optional_stages: nil, + additional_access_tokens: access_params[:additional_access_tokens], + timeout_strategy: timeout_strategy + ) + expect_snowplow_event( + category: 'Import::GithubService', + action: 'create', + label: 'import_access_level', + user: user, + extra: { import_type: 'github', user_role: 'Owner' } + ) + end end - context 'repository size validation' do - let(:repository_double) { { name: 'repository', size: 99 } } + context 'when the target namespace repository size limit is defined' do + let_it_be(:group) { create(:group, repository_size_limit: 100) } before do - allow(subject).to receive(:authorized?).and_return(true) - expect(client).to receive(:repository).and_return(repository_double) - - allow_next_instance_of(Gitlab::LegacyGithubImport::ProjectCreator) do |creator| - allow(creator).to receive(:execute).and_return(project_double) - end + params[:target_namespace] = group.full_path end - context 'when there is no repository size limit defined' do - it 'skips the check, succeeds, and tracks an access level' do - expect(subject.execute(access_params, :github)).to include(status: :success) - expect(settings) - .to have_received(:write) - .with(optional_stages: nil, - additional_access_tokens: access_params[:additional_access_tokens], - timeout_strategy: timeout_strategy - ) - expect_snowplow_event( - category: 'Import::GithubService', - action: 'create', - label: 'import_access_level', - user: user, - extra: { import_type: 'github', user_role: 'Owner' } + it 'succeeds when the repository is smaller than the limit' do + expect(subject.execute(access_params, :github)).to include(status: :success) + expect(settings) + .to have_received(:write) + .with( + optional_stages: nil, + additional_access_tokens: access_params[:additional_access_tokens], + timeout_strategy: timeout_strategy ) - end + expect_snowplow_event( + category: 'Import::GithubService', + action: 'create', + label: 'import_access_level', + user: user, + extra: { import_type: 'github', user_role: 'Not a member' } + ) end - context 'when the target namespace repository size limit is defined' do - let_it_be(:group) { create(:group, repository_size_limit: 100) } + it 'returns error when the repository is larger than the limit' do + repository_double[:size] = 101 - before do - params[:target_namespace] = group.full_path - end + expect(subject.execute(access_params, :github)).to include(size_limit_error) + end + end + context 'when target namespace repository limit is not defined' do + let_it_be(:group) { create(:group) } + + before do + stub_application_setting(repository_size_limit: 100) + end + + context 'when application size limit is defined' do it 'succeeds when the repository is smaller than the limit' do expect(subject.execute(access_params, :github)).to include(status: :success) expect(settings) @@ -133,7 +165,7 @@ action: 'create', label: 'import_access_level', user: user, - extra: { import_type: 'github', user_role: 'Not a member' } + extra: { import_type: 'github', user_role: 'Owner' } ) end @@ -143,192 +175,142 @@ expect(subject.execute(access_params, :github)).to include(size_limit_error) end end + end - context 'when target namespace repository limit is not defined' do - let_it_be(:group) { create(:group) } - - before do - stub_application_setting(repository_size_limit: 100) - end - - context 'when application size limit is defined' do - it 'succeeds when the repository is smaller than the limit' do - expect(subject.execute(access_params, :github)).to include(status: :success) - expect(settings) - .to have_received(:write) - .with( - optional_stages: nil, - additional_access_tokens: access_params[:additional_access_tokens], - timeout_strategy: timeout_strategy - ) - expect_snowplow_event( - category: 'Import::GithubService', - action: 'create', - label: 'import_access_level', - user: user, - extra: { import_type: 'github', user_role: 'Owner' } - ) - end - - it 'returns error when the repository is larger than the limit' do - repository_double[:size] = 101 - - expect(subject.execute(access_params, :github)).to include(size_limit_error) - end - end + context 'when optional stages params present' do + let(:optional_stages) do + { + single_endpoint_issue_events_import: true, + single_endpoint_notes_import: 'false', + attachments_import: false, + collaborators_import: true + } end - context 'when optional stages params present' do - let(:optional_stages) do - { - single_endpoint_issue_events_import: true, - single_endpoint_notes_import: 'false', - attachments_import: false, - collaborators_import: true - } - end - - it 'saves optional stages choice to import_data' do - subject.execute(access_params, :github) + it 'saves optional stages choice to import_data' do + subject.execute(access_params, :github) - expect(settings) - .to have_received(:write) - .with( - optional_stages: optional_stages, - additional_access_tokens: access_params[:additional_access_tokens], - timeout_strategy: timeout_strategy - ) - end + expect(settings) + .to have_received(:write) + .with( + optional_stages: optional_stages, + additional_access_tokens: access_params[:additional_access_tokens], + timeout_strategy: timeout_strategy + ) end + end - context 'when timeout strategy param is present' do - let(:timeout_strategy) { 'pessimistic' } - - it 'saves timeout strategy to import_data' do - subject.execute(access_params, :github) + context 'when timeout strategy param is present' do + let(:timeout_strategy) { 'pessimistic' } - expect(settings) - .to have_received(:write) - .with( - optional_stages: optional_stages, - additional_access_tokens: access_params[:additional_access_tokens], - timeout_strategy: timeout_strategy - ) - end - end - - context 'when additional access tokens are present' do - it 'saves additional access tokens to import_data' do - subject.execute(access_params, :github) + it 'saves timeout strategy to import_data' do + subject.execute(access_params, :github) - expect(settings) - .to have_received(:write) - .with( - optional_stages: optional_stages, - additional_access_tokens: %w[foo bar], - timeout_strategy: timeout_strategy - ) - end + expect(settings) + .to have_received(:write) + .with( + optional_stages: optional_stages, + additional_access_tokens: access_params[:additional_access_tokens], + timeout_strategy: timeout_strategy + ) end end - context 'when import source is disabled' do - let(:repository_double) do - { - name: 'vim', - description: 'test', - full_name: 'test/vim', - clone_url: 'http://repo.com/repo/repo.git', - private: false, - has_wiki?: false - } - end - - before do - stub_application_setting(import_sources: nil) - allow(client).to receive(:repository).and_return(repository_double) - end - - it 'returns forbidden' do - result = subject.execute(access_params, :github) + context 'when additional access tokens are present' do + it 'saves additional access tokens to import_data' do + subject.execute(access_params, :github) - expect(result).to include( - status: :error, - http_status: :forbidden - ) + expect(settings) + .to have_received(:write) + .with( + optional_stages: optional_stages, + additional_access_tokens: %w[foo bar], + timeout_strategy: timeout_strategy + ) end end + end - context 'when a blocked/local URL is used as github_hostname' do - let(:message) { 'Error while attempting to import from GitHub' } - let(:error) { "Invalid URL: #{url}" } - - before do - stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) - end + context 'when import source is disabled' do + let(:repository_double) do + { + name: 'vim', + description: 'test', + full_name: 'test/vim', + clone_url: 'http://repo.com/repo/repo.git', + private: false, + has_wiki?: false + } + end - where(url: %w[https://localhost https://10.0.0.1]) + before do + stub_application_setting(import_sources: nil) + allow(client).to receive(:repository).and_return(repository_double) + end - with_them do - it 'returns and logs an error' do - allow(github_importer).to receive(:url).and_return(url) + it 'returns forbidden' do + result = subject.execute(access_params, :github) - expect(Gitlab::Import::Logger).to receive(:error).with({ - message: message, - error: error - }).and_call_original - expect(github_importer.execute(access_params, :github)).to include(blocked_url_error(url)) - end - end + expect(result).to include( + status: :error, + http_status: :forbidden + ) end + end - context 'when target_namespace is blank' do - before do - params[:target_namespace] = '' - end + context 'when a blocked/local URL is used as github_hostname' do + let(:message) { 'Error while attempting to import from GitHub' } + let(:error) { "Invalid URL: #{url}" } - it 'raises an exception' do - expect { subject.execute(access_params, :github) }.to raise_error(ArgumentError, 'Target namespace is required') - end + before do + stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) end - context 'when namespace to import repository into does not exist' do - before do - params[:target_namespace] = 'unknown_path' - end + where(url: %w[https://localhost https://10.0.0.1]) - it 'returns an error' do - expect(github_importer.execute(access_params, :github)).to include(not_existed_namespace_error) + with_them do + it 'returns and logs an error' do + allow(github_importer).to receive(:url).and_return(url) + + expect(Gitlab::Import::Logger).to receive(:error).with({ + message: message, + error: error + }).and_call_original + expect(github_importer.execute(access_params, :github)).to include(blocked_url_error(url)) end end + end - context 'when user has no permissions to import repository into the specified namespace' do - let_it_be(:group) { create(:group) } - - before do - params[:target_namespace] = group.full_path - end + context 'when target_namespace is blank' do + before do + params[:target_namespace] = '' + end - it 'returns an error' do - expect(github_importer.execute(access_params, :github)).to include(taken_namespace_error) - end + it 'raises an exception' do + expect { subject.execute(access_params, :github) }.to raise_error(ArgumentError, 'Target namespace is required') end end - context 'when remove_legacy_github_client feature flag is enabled' do + context 'when namespace to import repository into does not exist' do before do - stub_feature_flags(remove_legacy_github_client: true) + params[:target_namespace] = 'unknown_path' end - include_examples 'handles errors', Gitlab::GithubImport::Client + it 'returns an error' do + expect(github_importer.execute(access_params, :github)).to include(not_existed_namespace_error) + end end - context 'when remove_legacy_github_client feature flag is disabled' do + context 'when user has no permissions to import repository into the specified namespace' do + let_it_be(:group) { create(:group) } + before do - stub_feature_flags(remove_legacy_github_client: false) + params[:target_namespace] = group.full_path end - include_examples 'handles errors', Gitlab::LegacyGithubImport::Client + it 'returns an error' do + expect(github_importer.execute(access_params, :github)).to include(taken_namespace_error) + end end def size_limit_error diff --git a/spec/support/shared_examples/controllers/githubish_import_controller_shared_examples.rb b/spec/support/shared_examples/controllers/githubish_import_controller_shared_examples.rb index af1843bae2857eb2e64c0b5fb3f49270974a96df..c921da103479d0deca9dbb01fd9625556e299f30 100644 --- a/spec/support/shared_examples/controllers/githubish_import_controller_shared_examples.rb +++ b/spec/support/shared_examples/controllers/githubish_import_controller_shared_examples.rb @@ -161,8 +161,6 @@ def assign_session_token(provider) group.add_owner(user) client = stub_client(repos: repos, orgs: [org], org_repos: [org_repo]) allow(client).to receive(:each_page).and_return([double('client', objects: repos)].to_enum) - # GitHub controller has filtering done using GitHub Search API - stub_feature_flags(remove_legacy_github_client: false) end it 'filters list of repositories by name' do