From 45f94ea78cf85a58e71e9a8ae62adc7e3f86bc99 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sun, 19 Feb 2017 05:00:27 -0800 Subject: [PATCH] Prevent project team from being truncated too early during project destruction 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. --- app/services/projects/destroy_service.rb | 3 +-- .../services/projects/destroy_service_spec.rb | 19 +++++++++++++++++++ 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 }