diff options
| -rw-r--r-- | app/controllers/import/base_controller.rb | 24 | ||||
| -rw-r--r-- | app/controllers/import/bitbucket_controller.rb | 2 | ||||
| -rw-r--r-- | app/services/groups/nested_create_service.rb | 10 | ||||
| -rw-r--r-- | changelogs/unreleased/fix-gh-namespace-issue.yml | 5 | ||||
| -rw-r--r-- | spec/controllers/import/bitbucket_controller_spec.rb | 10 | ||||
| -rw-r--r-- | spec/controllers/import/gitlab_controller_spec.rb | 10 | ||||
| -rw-r--r-- | spec/support/controllers/githubish_import_controller_shared_examples.rb | 57 |
7 files changed, 88 insertions, 30 deletions
diff --git a/app/controllers/import/base_controller.rb b/app/controllers/import/base_controller.rb index 9de0297ecfd..c84fc2d305d 100644 --- a/app/controllers/import/base_controller.rb +++ b/app/controllers/import/base_controller.rb @@ -2,26 +2,16 @@ class Import::BaseController < ApplicationController private def find_or_create_namespace(names, owner) - return current_user.namespace if names == owner - return current_user.namespace unless current_user.can_create_group? - names = params[:target_namespace].presence || names - full_path_namespace = Namespace.find_by_full_path(names) - return full_path_namespace if full_path_namespace + return current_user.namespace if names == owner + + group = Groups::NestedCreateService.new(current_user, group_path: names).execute - names.split('/').inject(nil) do |parent, name| - begin - namespace = Group.create!(name: name, - path: name, - owner: current_user, - parent: parent) - namespace.add_owner(current_user) + group.errors.any? ? current_user.namespace : group + rescue => e + Gitlab::AppLogger.error(e) - namespace - rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid - Namespace.where(parent: parent).find_by_path_or_name(name) - end - end + current_user.namespace end end diff --git a/app/controllers/import/bitbucket_controller.rb b/app/controllers/import/bitbucket_controller.rb index 5ad1e116e4e..6b65d80999f 100644 --- a/app/controllers/import/bitbucket_controller.rb +++ b/app/controllers/import/bitbucket_controller.rb @@ -46,7 +46,7 @@ class Import::BitbucketController < Import::BaseController repo_owner = current_user.username if repo_owner == bitbucket_client.user.username namespace_path = params[:new_namespace].presence || repo_owner - @target_namespace = find_or_create_namespace(namespace_path, current_user) + @target_namespace = find_or_create_namespace(namespace_path, current_user.namespace_path) if current_user.can?(:create_projects, @target_namespace) # The token in a session can be expired, we need to get most recent one because diff --git a/app/services/groups/nested_create_service.rb b/app/services/groups/nested_create_service.rb index d6f08fc3cce..5c337a9faa5 100644 --- a/app/services/groups/nested_create_service.rb +++ b/app/services/groups/nested_create_service.rb @@ -11,8 +11,8 @@ module Groups def execute return nil unless group_path - if group = Group.find_by_full_path(group_path) - return group + if namespace = namespace_or_group(group_path) + return namespace end if group_path.include?('/') && !Group.supports_nested_groups? @@ -40,10 +40,14 @@ module Groups ) new_params[:visibility_level] ||= Gitlab::CurrentSettings.current_application_settings.default_group_visibility - last_group = Group.find_by_full_path(partial_path) || Groups::CreateService.new(current_user, new_params).execute + last_group = namespace_or_group(partial_path) || Groups::CreateService.new(current_user, new_params).execute end last_group end + + def namespace_or_group(group_path) + Namespace.find_by_full_path(group_path) + end end end diff --git a/changelogs/unreleased/fix-gh-namespace-issue.yml b/changelogs/unreleased/fix-gh-namespace-issue.yml new file mode 100644 index 00000000000..2db7abb9d58 --- /dev/null +++ b/changelogs/unreleased/fix-gh-namespace-issue.yml @@ -0,0 +1,5 @@ +--- +title: Fix namespace access issue for GitHub, BitBucket, and GitLab.com project importers +merge_request: +author: +type: security diff --git a/spec/controllers/import/bitbucket_controller_spec.rb b/spec/controllers/import/bitbucket_controller_spec.rb index e8707760a5a..f5e75ff3fdb 100644 --- a/spec/controllers/import/bitbucket_controller_spec.rb +++ b/spec/controllers/import/bitbucket_controller_spec.rb @@ -200,7 +200,7 @@ describe Import::BitbucketController do end end - context 'user has chosen an existing nested namespace and name for the project' do + context 'user has chosen an existing nested namespace and name for the project', :postgresql do let(:parent_namespace) { create(:group, name: 'foo', owner: user) } let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) } let(:test_name) { 'test_name' } @@ -218,7 +218,7 @@ describe Import::BitbucketController do end end - context 'user has chosen a non-existent nested namespaces and name for the project' do + context 'user has chosen a non-existent nested namespaces and name for the project', :postgresql do let(:test_name) { 'test_name' } it 'takes the selected namespace and name' do @@ -249,10 +249,14 @@ describe Import::BitbucketController do end end - context 'user has chosen existent and non-existent nested namespaces and name for the project' do + context 'user has chosen existent and non-existent nested namespaces and name for the project', :postgresql do let(:test_name) { 'test_name' } let!(:parent_namespace) { create(:group, name: 'foo', owner: user) } + before do + parent_namespace.add_owner(user) + end + it 'takes the selected namespace and name' do expect(Gitlab::BitbucketImport::ProjectCreator) .to receive(:new).with(bitbucket_repo, test_name, kind_of(Namespace), user, access_params) diff --git a/spec/controllers/import/gitlab_controller_spec.rb b/spec/controllers/import/gitlab_controller_spec.rb index faf1e6f63ea..3bfb34d8d2a 100644 --- a/spec/controllers/import/gitlab_controller_spec.rb +++ b/spec/controllers/import/gitlab_controller_spec.rb @@ -174,7 +174,7 @@ describe Import::GitlabController do end end - context 'user has chosen an existing nested namespace for the project' do + context 'user has chosen an existing nested namespace for the project', :postgresql do let(:parent_namespace) { create(:group, name: 'foo', owner: user) } let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) } @@ -191,7 +191,7 @@ describe Import::GitlabController do end end - context 'user has chosen a non-existent nested namespaces for the project' do + context 'user has chosen a non-existent nested namespaces for the project', :postgresql do let(:test_name) { 'test_name' } it 'takes the selected namespace and name' do @@ -222,10 +222,14 @@ describe Import::GitlabController do end end - context 'user has chosen existent and non-existent nested namespaces and name for the project' do + context 'user has chosen existent and non-existent nested namespaces and name for the project', :postgresql do let(:test_name) { 'test_name' } let!(:parent_namespace) { create(:group, name: 'foo', owner: user) } + before do + parent_namespace.add_owner(user) + end + it 'takes the selected namespace and name' do expect(Gitlab::GitlabImport::ProjectCreator) .to receive(:new).with(gitlab_repo, kind_of(Namespace), user, access_params) diff --git a/spec/support/controllers/githubish_import_controller_shared_examples.rb b/spec/support/controllers/githubish_import_controller_shared_examples.rb index a0839eefe6c..7facf34302b 100644 --- a/spec/support/controllers/githubish_import_controller_shared_examples.rb +++ b/spec/support/controllers/githubish_import_controller_shared_examples.rb @@ -235,7 +235,7 @@ shared_examples 'a GitHub-ish import controller: POST create' do end end - context 'user has chosen an existing nested namespace and name for the project' do + context 'user has chosen an existing nested namespace and name for the project', :postgresql do let(:parent_namespace) { create(:group, name: 'foo', owner: user) } let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) } let(:test_name) { 'test_name' } @@ -253,7 +253,7 @@ shared_examples 'a GitHub-ish import controller: POST create' do end end - context 'user has chosen a non-existent nested namespaces and name for the project' do + context 'user has chosen a non-existent nested namespaces and name for the project', :postgresql do let(:test_name) { 'test_name' } it 'takes the selected namespace and name' do @@ -284,10 +284,14 @@ shared_examples 'a GitHub-ish import controller: POST create' do end end - context 'user has chosen existent and non-existent nested namespaces and name for the project' do + context 'user has chosen existent and non-existent nested namespaces and name for the project', :postgresql do let(:test_name) { 'test_name' } let!(:parent_namespace) { create(:group, name: 'foo', owner: user) } + before do + parent_namespace.add_owner(user) + end + it 'takes the selected namespace and name' do expect(Gitlab::LegacyGithubImport::ProjectCreator) .to receive(:new).with(provider_repo, test_name, kind_of(Namespace), user, access_params, type: provider) @@ -304,6 +308,53 @@ shared_examples 'a GitHub-ish import controller: POST create' do expect { post :create, { target_namespace: 'foo/foobar/bar', new_name: test_name, format: :js } } .to change { Namespace.count }.by(2) end + + it 'does not create a new namespace under the user namespace' do + expect(Gitlab::LegacyGithubImport::ProjectCreator) + .to receive(:new).with(provider_repo, test_name, user.namespace, user, access_params, type: provider) + .and_return(double(execute: true)) + + expect { post :create, { target_namespace: "#{user.namespace_path}/test_group", new_name: test_name, format: :js } } + .not_to change { Namespace.count } + end + end + + context 'user cannot create a subgroup inside a group is not a member of' do + let(:test_name) { 'test_name' } + let!(:parent_namespace) { create(:group, name: 'foo') } + + it 'does not take the selected namespace and name' do + expect(Gitlab::LegacyGithubImport::ProjectCreator) + .to receive(:new).with(provider_repo, test_name, user.namespace, user, access_params, type: provider) + .and_return(double(execute: true)) + + post :create, { target_namespace: 'foo/foobar/bar', new_name: test_name, format: :js } + end + + it 'does not create the namespaces' do + allow(Gitlab::LegacyGithubImport::ProjectCreator) + .to receive(:new).with(provider_repo, test_name, kind_of(Namespace), user, access_params, type: provider) + .and_return(double(execute: true)) + + expect { post :create, { target_namespace: 'foo/foobar/bar', new_name: test_name, format: :js } } + .not_to change { Namespace.count } + end + end + + context 'user can use a group without having permissions to create a group' do + let(:test_name) { 'test_name' } + let!(:group) { create(:group, name: 'foo') } + + it 'takes the selected namespace and name' do + group.add_owner(user) + user.update!(can_create_group: false) + + expect(Gitlab::LegacyGithubImport::ProjectCreator) + .to receive(:new).with(provider_repo, test_name, group, user, access_params, type: provider) + .and_return(double(execute: true)) + + post :create, { target_namespace: 'foo', new_name: test_name, format: :js } + end end end end |