summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorStan Hu <stanhu@gmail.com>2017-02-19 05:00:27 -0800
committerStan Hu <stanhu@gmail.com>2017-02-19 05:00:27 -0800
commit45f94ea78cf85a58e71e9a8ae62adc7e3f86bc99 (patch)
treed1e1ce1bb52ca2324f5a40ed5d8322d57ce2b45c
parent12cd4c83604e43dc308ba13fa3d5a6571409c1f8 (diff)
downloadgitlab-ce-45f94ea78cf85a58e71e9a8ae62adc7e3f86bc99.tar.gz
Prevent project team from being truncated too early during project destructionsh-fix-project-team-truncation-in-destroy
There are two issues with truncating the project team early: 1. `Projects::UnlinkForkService` may not close merge requests properly since permissions may be revoked early. 2. If an error is encountered during flushing of caches, then the user will lose all privileges, possibly causing an issue on deletion on retry.
-rw-r--r--app/services/projects/destroy_service.rb3
-rw-r--r--spec/services/projects/destroy_service_spec.rb19
2 files changed, 20 insertions, 2 deletions
diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index a08c6fcd94b..9716a1780a9 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -17,8 +17,6 @@ module Projects
def execute
return false unless can?(current_user, :remove_project, project)
- project.team.truncate
-
repo_path = project.path_with_namespace
wiki_path = repo_path + '.wiki'
@@ -30,6 +28,7 @@ module Projects
Projects::UnlinkForkService.new(project, current_user).execute
Project.transaction do
+ project.team.truncate
project.destroy!
unless remove_registry_tags
diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb
index 3faa88c00a1..74bfba44dfd 100644
--- a/spec/services/projects/destroy_service_spec.rb
+++ b/spec/services/projects/destroy_service_spec.rb
@@ -50,6 +50,25 @@ describe Projects::DestroyService, services: true do
it { expect(Dir.exist?(remove_path)).to be_truthy }
end
+ context 'when flushing caches fail' do
+ before do
+ new_user = create(:user)
+ project.team.add_user(new_user, Gitlab::Access::DEVELOPER)
+ allow_any_instance_of(Projects::DestroyService).to receive(:flush_caches).and_raise(Redis::CannotConnectError)
+ end
+
+ it 'keeps project team intact upon an error' do
+ Sidekiq::Testing.inline! do
+ begin
+ destroy_project(project, user, {})
+ rescue Redis::CannotConnectError
+ end
+ end
+
+ expect(project.team.members.count).to eq 1
+ end
+ end
+
context 'with async_execute' do
let(:async) { true }