diff options
author | Nick Thomas <nick@gitlab.com> | 2018-08-17 10:57:48 +0000 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2018-08-17 10:57:48 +0000 |
commit | 10cf9383f67864b39727b6f641a92d5a78ec5b77 (patch) | |
tree | 51491d90e23ecb61d3def0730ca6b3b76f8097eb | |
parent | c6f7f9389c775cd9f76cdfe545a643f0e1b89229 (diff) | |
parent | fbf618fc3817442ef17848b67738ff456d65e7e4 (diff) | |
download | gitlab-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
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, {}) |