Commit 10cf9383 authored by Nick Thomas's avatar Nick Thomas

Merge branch '49796-project-deletion-may-not-log-audit-events-during-user-deletion' into 'master'

Resolve "Project deletion may not log audit events during user deletion"

Closes #49796

See merge request gitlab-org/gitlab-ce!21248
parents c6f7f938 fbf618fc
...@@ -83,9 +83,6 @@ module Projects ...@@ -83,9 +83,6 @@ module Projects
end end
def remove_repository(path) 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 # There is a possibility project does not have repository or wiki
return true unless repo_exists?(path) return true unless repo_exists?(path)
......
...@@ -2,6 +2,8 @@ ...@@ -2,6 +2,8 @@
module Users module Users
class DestroyService class DestroyService
DestroyError = Class.new(StandardError)
attr_accessor :current_user attr_accessor :current_user
def initialize(current_user) def initialize(current_user)
...@@ -46,9 +48,8 @@ module Users ...@@ -46,9 +48,8 @@ module Users
namespace.prepare_for_destroy namespace.prepare_for_destroy
user.personal_projects.each do |project| user.personal_projects.each do |project|
# Skip repository removal because we remove directory with namespace success = ::Projects::DestroyService.new(project, current_user).execute
# that contain all this repositories raise DestroyError, "Project #{project.id} can't be deleted" unless success
::Projects::DestroyService.new(project, current_user, skip_repo: project.legacy_storage?).execute
end end
yield(user) if block_given? yield(user) if block_given?
......
---
title: 'Fix: Project deletion may not log audit events during user deletion'
merge_request:
author:
type: fixed
...@@ -20,7 +20,7 @@ describe Users::DestroyService do ...@@ -20,7 +20,7 @@ describe Users::DestroyService do
it 'will delete the project' do it 'will delete the project' do
expect_next_instance_of(Projects::DestroyService) do |destroy_service| 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 end
service.execute(user) service.execute(user)
...@@ -35,7 +35,7 @@ describe Users::DestroyService do ...@@ -35,7 +35,7 @@ describe Users::DestroyService do
it 'destroys a project in pending_delete' do it 'destroys a project in pending_delete' do
expect_next_instance_of(Projects::DestroyService) do |destroy_service| 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 end
service.execute(user) service.execute(user)
...@@ -172,6 +172,7 @@ describe Users::DestroyService do ...@@ -172,6 +172,7 @@ describe Users::DestroyService do
end end
describe "user personal's repository removal" do describe "user personal's repository removal" do
context 'storages' do
before do before do
perform_enqueued_jobs { service.execute(user) } perform_enqueued_jobs { service.execute(user) }
end end
...@@ -193,6 +194,18 @@ describe Users::DestroyService do ...@@ -193,6 +194,18 @@ describe Users::DestroyService do
end end
end 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 { service.execute(user) }
.to raise_error(Users::DestroyService::DestroyError, "Project #{project.id} can't be deleted" )
end
end
end
describe "calls the before/after callbacks" do describe "calls the before/after callbacks" do
it 'of project_members' do it 'of project_members' do
expect_any_instance_of(ProjectMember).to receive(:run_callbacks).with(:destroy).once expect_any_instance_of(ProjectMember).to receive(:run_callbacks).with(:destroy).once
......
...@@ -18,13 +18,6 @@ describe ProjectDestroyWorker do ...@@ -18,13 +18,6 @@ describe ProjectDestroyWorker do
expect(Dir.exist?(path)).to be_falsey expect(Dir.exist?(path)).to be_falsey
end 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 it 'does not raise error when project could not be found' do
expect do expect do
subject.perform(-1, project.owner.id, {}) subject.perform(-1, project.owner.id, {})
......
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