summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorNick Thomas <nick@gitlab.com>2018-08-17 10:57:48 +0000
committerNick Thomas <nick@gitlab.com>2018-08-17 10:57:48 +0000
commit10cf9383f67864b39727b6f641a92d5a78ec5b77 (patch)
tree51491d90e23ecb61d3def0730ca6b3b76f8097eb
parentc6f7f9389c775cd9f76cdfe545a643f0e1b89229 (diff)
parentfbf618fc3817442ef17848b67738ff456d65e7e4 (diff)
downloadgitlab-ce-10cf9383f67864b39727b6f641a92d5a78ec5b77.tar.gz
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
-rw-r--r--app/services/projects/destroy_service.rb3
-rw-r--r--app/services/users/destroy_service.rb7
-rw-r--r--changelogs/unreleased/49796-project-deletion-may-not-log-audit-events-during-user-deletion.yml5
-rw-r--r--spec/services/users/destroy_service_spec.rb39
-rw-r--r--spec/workers/project_destroy_worker_spec.rb7
5 files changed, 35 insertions, 26 deletions
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-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/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, {})