Merge branch 'fix/gh-namespace-issue' into 'security-10-4'

[10.4] Fix GH namespace security issue

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

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

changelogs/unreleased/fix-gh-namespace-issue.yml

+---
+title: Fix namespace access issue for GitHub, BitBucket, and GitLab.com project importers
+merge_request:
+author:
+type: security

spec/controllers/import/bitbucket_controller_spec.rb

@@ -222,7 +222,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' }
@@ -240,7 +240,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
@@ -271,10 +271,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)

spec/controllers/import/gitlab_controller_spec.rb

@@ -195,7 +195,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) }
 
@@ -212,7 +212,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
@@ -243,10 +243,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)

spec/support/controllers/githubish_import_controller_shared_examples.rb

@@ -256,7 +256,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' }
@@ -274,7 +274,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
@@ -305,10 +305,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)
@@ -325,6 +329,53 @@ shared_examples 'a GitHub-ish import controller: POST create' do
         expect { post :create, { target_namespace: 'foo/foobar/bar', new_name: test_name, format: :json } }
           .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
 
     context 'when user can not create projects in the chosen namespace' do