From cd582decb99af5049e64d8078815abdf39e5d4b4 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Sat, 9 May 2020 12:09:53 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- app/services/projects/import_service.rb | 1 + .../sh-fix-overwrite-import-export-check.yml | 5 ++ lib/gitlab/import_export/importer.rb | 12 ++- .../lib/gitlab/import_export/importer_spec.rb | 82 ++++++++++++++----- 4 files changed, 74 insertions(+), 26 deletions(-) create mode 100644 changelogs/unreleased/sh-fix-overwrite-import-export-check.yml diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb index 4b294a97516..e55697eebcc 100644 --- a/app/services/projects/import_service.rb +++ b/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. diff --git a/changelogs/unreleased/sh-fix-overwrite-import-export-check.yml b/changelogs/unreleased/sh-fix-overwrite-import-export-check.yml new file mode 100644 index 00000000000..6a897c1cca5 --- /dev/null +++ b/changelogs/unreleased/sh-fix-overwrite-import-export-check.yml @@ -0,0 +1,5 @@ +--- +title: Fix overwrite check in GitLab import/export +merge_request: 31439 +author: +type: fixed diff --git a/lib/gitlab/import_export/importer.rb b/lib/gitlab/import_export/importer.rb index 4b761eb86ae..e88e0128d57 100644 --- a/lib/gitlab/import_export/importer.rb +++ b/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 diff --git a/spec/lib/gitlab/import_export/importer_spec.rb b/spec/lib/gitlab/import_export/importer_spec.rb index e03c95525df..14e643f86c0 100644 --- a/spec/lib/gitlab/import_export/importer_spec.rb +++ b/spec/lib/gitlab/import_export/importer_spec.rb @@ -89,37 +89,75 @@ 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 })) - end - - context 'when import_data' do - context 'has original_path' do - it 'overwrites existing project' do - expect_any_instance_of(::Projects::OverwriteProjectService).to receive(:execute).with(existing_project) - - subject.execute - end + 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 not original_path' do - before do - allow(project).to receive(:import_data).and_return(double(data: {})) + 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 - it 'does not call the overwrite service' do - expect_any_instance_of(::Projects::OverwriteProjectService).not_to receive(:execute).with(existing_project) + 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 "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 + group.add_owner(user) + + expect_next_instance_of(::Projects::OverwriteProjectService) do |service| + expect(service).to receive(:execute).with(existing_project) + end subject.execute end + + it 'does not allow user to overwrite existing project' do + expect(::Projects::OverwriteProjectService).not_to receive(:new) + + expect { subject.execute }.to raise_error(Projects::ImportService::Error, + "User #{user.username} (#{user.id}) cannot overwrite a project in #{group.path}") + end end end end