Commit 8e53ce74 authored by Stan Hu's avatar Stan Hu

Fix overwrite check in GitLab import/export

In https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/17883, we
added a feature that would allow a project export to be uploaded and
imported successfully before deleting the old copy of the project.

This overwrite feature would always check whether the user had owner
privileges in the group of the project. If the user only had maintainer
privileges or less--even if an overwrite was not happening--the
import/export would fail with an empty `Projects::ImportService::Error`.

To fix this, we make two changes:

1. Only do the permission check if we actually need to overwrite
the project.
2. If the permission check fails, raise an exception with details.
parent bf44d054
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
module Projects module Projects
class ImportService < BaseService class ImportService < BaseService
Error = Class.new(StandardError) Error = Class.new(StandardError)
PermissionError = Class.new(StandardError)
# Returns true if this importer is supposed to perform its work in the # Returns true if this importer is supposed to perform its work in the
# background. # background.
......
---
title: Fix overwrite check in GitLab import/export
merge_request: 31439
author:
type: fixed
...@@ -111,13 +111,17 @@ module Gitlab ...@@ -111,13 +111,17 @@ module Gitlab
end end
def overwrite_project def overwrite_project
return unless can?(current_user, :admin_namespace, project.namespace) return true unless overwrite_project?
if overwrite_project? unless can?(current_user, :admin_namespace, project.namespace)
::Projects::OverwriteProjectService.new(project, current_user) message = "User #{current_user&.username} (#{current_user&.id}) cannot overwrite a project in #{project.namespace.path}"
.execute(project_to_overwrite) @shared.error(::Projects::ImportService::PermissionError.new(message))
return false
end end
::Projects::OverwriteProjectService.new(project, current_user)
.execute(project_to_overwrite)
true true
end end
......
...@@ -89,36 +89,74 @@ describe Gitlab::ImportExport::Importer do ...@@ -89,36 +89,74 @@ describe Gitlab::ImportExport::Importer do
end end
context 'when project successfully restored' do context 'when project successfully restored' do
let!(:existing_project) { create(:project, namespace: user.namespace) } context "with a project in a user's namespace" do
let(:project) { create(:project, namespace: user.namespace, name: 'whatever', path: 'whatever') } let!(:existing_project) { create(:project, namespace: user.namespace) }
let(:project) { create(:project, namespace: user.namespace, name: 'whatever', path: 'whatever') }
before do before do
restorers = double(:restorers, all?: true) restorers = double(:restorers, all?: true)
allow(subject).to receive(:import_file).and_return(true) allow(subject).to receive(:import_file).and_return(true)
allow(subject).to receive(:check_version!).and_return(true) allow(subject).to receive(:check_version!).and_return(true)
allow(subject).to receive(:restorers).and_return(restorers) allow(subject).to receive(:restorers).and_return(restorers)
allow(project).to receive(:import_data).and_return(double(data: { 'original_path' => existing_project.path })) 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 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 context 'has original_path' do
it 'overwrites existing project' do it 'overwrites existing project' do
expect_any_instance_of(::Projects::OverwriteProjectService).to receive(:execute).with(existing_project) group.add_owner(user)
subject.execute expect_next_instance_of(::Projects::OverwriteProjectService) do |service|
end expect(service).to receive(:execute).with(existing_project)
end end
context 'has not original_path' do subject.execute
before do
allow(project).to receive(:import_data).and_return(double(data: {}))
end end
it 'does not call the overwrite service' do it 'does not allow user to overwrite existing project' do
expect_any_instance_of(::Projects::OverwriteProjectService).not_to receive(:execute).with(existing_project) 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 end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment