Merge branch 'sh-fix-overwrite-import-export-check' into 'master'

Fix overwrite check in GitLab import/export

See merge request gitlab-org/gitlab!31439

app/services/projects/import_service.rb

@@ -3,6 +3,7 @@
 module Projects
   class ImportService < BaseService
     Error = Class.new(StandardError)
+    PermissionError = Class.new(StandardError)
 
     # Returns true if this importer is supposed to perform its work in the
     # background.

changelogs/unreleased/sh-fix-overwrite-import-export-check.yml

+---
+title: Fix overwrite check in GitLab import/export
+merge_request: 31439
+author:
+type: fixed

lib/gitlab/import_export/importer.rb

@@ -111,13 +111,17 @@ module Gitlab
       end
 
       def overwrite_project
-        return unless can?(current_user, :admin_namespace, project.namespace)
+        return true unless overwrite_project?
 
-        if overwrite_project?
-          ::Projects::OverwriteProjectService.new(project, current_user)
-                                             .execute(project_to_overwrite)
+        unless can?(current_user, :admin_namespace, project.namespace)
+          message = "User #{current_user&.username} (#{current_user&.id}) cannot overwrite a project in #{project.namespace.path}"
+          @shared.error(::Projects::ImportService::PermissionError.new(message))
+          return false
         end
 
+        ::Projects::OverwriteProjectService.new(project, current_user)
+                                            .execute(project_to_overwrite)
+
         true
       end
 

spec/lib/gitlab/import_export/importer_spec.rb

@@ -89,36 +89,74 @@ describe Gitlab::ImportExport::Importer do
     end
 
     context 'when project successfully restored' do
-      let!(:existing_project) { create(:project, namespace: user.namespace) }
-      let(:project) { create(:project, namespace: user.namespace, name: 'whatever', path: 'whatever') }
+      context "with a project in a user's namespace" do
+        let!(:existing_project) { create(:project, namespace: user.namespace) }
+        let(:project) { create(:project, namespace: user.namespace, name: 'whatever', path: 'whatever') }
 
-      before do
-        restorers = double(:restorers, all?: true)
+        before do
+          restorers = double(:restorers, all?: true)
 
-        allow(subject).to receive(:import_file).and_return(true)
-        allow(subject).to receive(:check_version!).and_return(true)
-        allow(subject).to receive(:restorers).and_return(restorers)
-        allow(project).to receive(:import_data).and_return(double(data: { 'original_path' => existing_project.path }))
+          allow(subject).to receive(:import_file).and_return(true)
+          allow(subject).to receive(:check_version!).and_return(true)
+          allow(subject).to receive(:restorers).and_return(restorers)
+          allow(project).to receive(:import_data).and_return(double(data: { 'original_path' => existing_project.path }))
+        end
+
+        context 'when import_data' do
+          context 'has original_path' do
+            it 'overwrites existing project' do
+              expect_next_instance_of(::Projects::OverwriteProjectService) do |service|
+                expect(service).to receive(:execute).with(existing_project)
+              end
+
+              subject.execute
+            end
+          end
+
+          context 'has not original_path' do
+            before do
+              allow(project).to receive(:import_data).and_return(double(data: {}))
+            end
+
+            it 'does not call the overwrite service' do
+              expect(::Projects::OverwriteProjectService).not_to receive(:new)
+
+              subject.execute
+            end
+          end
+        end
       end
 
-      context 'when import_data' do
+      context "with a project in a group namespace" do
+        let(:group) { create(:group) }
+        let!(:existing_project) { create(:project, group: group) }
+        let(:project) { create(:project, creator: user, group: group, name: 'whatever', path: 'whatever') }
+
+        before do
+          restorers = double(:restorers, all?: true)
+
+          allow(subject).to receive(:import_file).and_return(true)
+          allow(subject).to receive(:check_version!).and_return(true)
+          allow(subject).to receive(:restorers).and_return(restorers)
+          allow(project).to receive(:import_data).and_return(double(data: { 'original_path' => existing_project.path }))
+        end
+
         context 'has original_path' do
           it 'overwrites existing project' do
-            expect_any_instance_of(::Projects::OverwriteProjectService).to receive(:execute).with(existing_project)
+            group.add_owner(user)
 
-            subject.execute
-          end
-        end
+            expect_next_instance_of(::Projects::OverwriteProjectService) do |service|
+              expect(service).to receive(:execute).with(existing_project)
+            end
 
-        context 'has not original_path' do
-          before do
-            allow(project).to receive(:import_data).and_return(double(data: {}))
+            subject.execute
           end
 
-          it 'does not call the overwrite service' do
-            expect_any_instance_of(::Projects::OverwriteProjectService).not_to receive(:execute).with(existing_project)
+          it 'does not allow user to overwrite existing project' do
+            expect(::Projects::OverwriteProjectService).not_to receive(:new)
 
-            subject.execute
+            expect { subject.execute }.to raise_error(Projects::ImportService::Error,
+              "User #{user.username} (#{user.id}) cannot overwrite a project in #{group.path}")
           end
         end
       end