Fix: Project deletion may not log audit events during user deletion49796-project-deletion-may-not-log-audit-events-during-user-deletion

8 files changed, 55 insertions, 29 deletions

diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb
index c4554ce45fb..12aeba4af71 100644
--- a/app/services/groups/destroy_service.rb
+++ b/app/services/groups/destroy_service.rb
@@ -2,6 +2,8 @@
 
 module Groups
   class DestroyService < Groups::BaseService
+    DestroyError = Class.new(StandardError)
+
     def async_execute
       job_id = GroupDestroyWorker.perform_async(group.id, current_user.id)
       Rails.logger.info("User #{current_user.id} scheduled a deletion of group ID #{group.id} with job ID #{job_id}")
@@ -12,9 +14,8 @@ module Groups
 
       group.projects.each do |project|
         # Execute the destruction of the models immediately to ensure atomic cleanup.
-        # Skip repository removal because we remove directory with namespace
-        # that contain all these repositories
-        ::Projects::DestroyService.new(project, current_user, skip_repo: project.legacy_storage?).execute
+        success = ::Projects::DestroyService.new(project, current_user).execute
+        raise DestroyError, "Project #{project.id} can't be deleted" unless success
       end
 
       group.children.each do |group|
diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index 46a8a5e4d98..76e22507698 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -83,9 +83,6 @@ module Projects
     end
 
     def remove_repository(path)
-      # Skip repository removal. We use this flag when remove user or group
-      return true if params[:skip_repo] == true
-
       # There is a possibility project does not have repository or wiki
       return true unless repo_exists?(path)
 
diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb
index 4bc78b5b64e..73fa6089945 100644
--- a/app/services/users/destroy_service.rb
+++ b/app/services/users/destroy_service.rb
@@ -2,6 +2,8 @@
 
 module Users
   class DestroyService
+    DestroyError = Class.new(StandardError)
+
     attr_accessor :current_user
 
     def initialize(current_user)
@@ -46,9 +48,8 @@ module Users
       namespace.prepare_for_destroy
 
       user.personal_projects.each do |project|
-        # Skip repository removal because we remove directory with namespace
-        # that contain all this repositories
-        ::Projects::DestroyService.new(project, current_user, skip_repo: project.legacy_storage?).execute
+        success = ::Projects::DestroyService.new(project, current_user).execute
+        raise DestroyError, "Project #{project.id} can't be deleted" unless success
       end
 
       yield(user) if block_given?
diff --git a/changelogs/unreleased/49796-project-deletion-may-not-log-audit-events-during-group-deletion.yml b/changelogs/unreleased/49796-project-deletion-may-not-log-audit-events-during-group-deletion.yml
new file mode 100644
index 00000000000..bb7633abdb1
--- /dev/null
+++ b/changelogs/unreleased/49796-project-deletion-may-not-log-audit-events-during-group-deletion.yml
@@ -0,0 +1,5 @@
+---
+title: 'Fix: Project deletion may not log audit events during group deletion'
+merge_request: 21162
+author:
+type: fixed
diff --git a/changelogs/unreleased/49796-project-deletion-may-not-log-audit-events-during-user-deletion.yml b/changelogs/unreleased/49796-project-deletion-may-not-log-audit-events-during-user-deletion.yml
new file mode 100644
index 00000000000..a8e3d590a4a
--- /dev/null
+++ b/changelogs/unreleased/49796-project-deletion-may-not-log-audit-events-during-user-deletion.yml
@@ -0,0 +1,5 @@
+---
+title: 'Fix: Project deletion may not log audit events during user deletion'
+merge_request:
+author:
+type: fixed
diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb
index b54491cf5f9..d80d0f5a8a8 100644
--- a/spec/services/groups/destroy_service_spec.rb
+++ b/spec/services/groups/destroy_service_spec.rb
@@ -135,6 +135,17 @@ describe Groups::DestroyService do
     it_behaves_like 'group destruction', false
   end
 
+  context 'repository removal status is taken into account' do
+    it 'raises exception' do
+      expect_next_instance_of(::Projects::DestroyService) do |destroy_service|
+        expect(destroy_service).to receive(:execute).and_return(false)
+      end
+
+      expect { destroy_group(group, user, false) }
+        .to raise_error(Groups::DestroyService::DestroyError, "Project #{project.id} can't be deleted" )
+    end
+  end
+
   describe 'repository removal' do
     before do
       destroy_group(group, user, false)
diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb
index 3bae8bfbd42..83f1495a1c6 100644
--- a/spec/services/users/destroy_service_spec.rb
+++ b/spec/services/users/destroy_service_spec.rb
@@ -20,7 +20,7 @@ describe Users::DestroyService do
 
       it 'will delete the project' do
         expect_next_instance_of(Projects::DestroyService) do |destroy_service|
-          expect(destroy_service).to receive(:execute).once
+          expect(destroy_service).to receive(:execute).once.and_return(true)
         end
 
         service.execute(user)
@@ -35,7 +35,7 @@ describe Users::DestroyService do
 
       it 'destroys a project in pending_delete' do
         expect_next_instance_of(Projects::DestroyService) do |destroy_service|
-          expect(destroy_service).to receive(:execute).once
+          expect(destroy_service).to receive(:execute).once.and_return(true)
         end
 
         service.execute(user)
@@ -172,23 +172,36 @@ describe Users::DestroyService do
     end
 
     describe "user personal's repository removal" do
-      before do
-        perform_enqueued_jobs { service.execute(user) }
-      end
+      context 'storages' do
+        before do
+          perform_enqueued_jobs { service.execute(user) }
+        end
+
+        context 'legacy storage' do
+          let!(:project) { create(:project, :empty_repo, :legacy_storage, namespace: user.namespace) }
+
+          it 'removes repository' do
+            expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey
+          end
+        end
 
-      context 'legacy storage' do
-        let!(:project) { create(:project, :empty_repo, :legacy_storage, namespace: user.namespace) }
+        context 'hashed storage' do
+          let!(:project) { create(:project, :empty_repo, namespace: user.namespace) }
 
-        it 'removes repository' do
-          expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey
+          it 'removes repository' do
+            expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey
+          end
         end
       end
 
-      context 'hashed storage' do
-        let!(:project) { create(:project, :empty_repo, namespace: user.namespace) }
+      context 'repository removal status is taken into account' do
+        it 'raises exception' do
+          expect_next_instance_of(::Projects::DestroyService) do |destroy_service|
+            expect(destroy_service).to receive(:execute).and_return(false)
+          end
 
-        it 'removes repository' do
-          expect(gitlab_shell.exists?(project.repository_storage, "#{project.disk_path}.git")).to be_falsey
+          expect { service.execute(user) }
+            .to raise_error(Users::DestroyService::DestroyError, "Project #{project.id} can't be deleted" )
         end
       end
     end
diff --git a/spec/workers/project_destroy_worker_spec.rb b/spec/workers/project_destroy_worker_spec.rb
index 42e1d86e3bb..6132f145f8d 100644
--- a/spec/workers/project_destroy_worker_spec.rb
+++ b/spec/workers/project_destroy_worker_spec.rb
@@ -18,13 +18,6 @@ describe ProjectDestroyWorker do
       expect(Dir.exist?(path)).to be_falsey
     end
 
-    it 'deletes the project but skips repo deletion' do
-      subject.perform(project.id, project.owner.id, { "skip_repo" => true })
-
-      expect(Project.all).not_to include(project)
-      expect(Dir.exist?(path)).to be_truthy
-    end
-
     it 'does not raise error when project could not be found' do
       expect do
         subject.perform(-1, project.owner.id, {})