From fea80aa12d6a0b55513e497b2b8e5d0aa86a7d76 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Tue, 4 Oct 2016 20:53:15 -0700
Subject: [PATCH] Fix project deletion when feature visibility is set to
private
Projects that are destroyed are put in the pending_delete state.
The ProjectDestroyWorker checks whether the current user has
access, but since the ProjectFeature class uses the default scope
of the Project, it will not be able to find the right project.
This was a regression in 8.12 that caused the following stack trace:
```
NoMethodError: undefined method `team' for nil:NilClass
from app/models/project_feature.rb:62:in `get_permission'
from app/models/project_feature.rb:34:in `feature_available?'
from app/models/project.rb:21:in `feature_available?'
from app/policies/project_policy.rb:170:in `disabled_features!'
from app/policies/project_policy.rb:29:in `rules'
from app/policies/base_policy.rb:82:in `block in abilities'
from app/policies/base_policy.rb:113:in `collect_rules'
from app/policies/base_policy.rb:82:in `abilities'
from app/policies/base_policy.rb:50:in `abilities'
from app/models/ability.rb:64:in `uncached_allowed'
from app/models/ability.rb:58:in `allowed'
from app/models/ability.rb:49:in `allowed?'
from app/services/base_service.rb:11:in `can?'
from lib/gitlab/metrics/instrumentation.rb:155:in `block in can?'
from lib/gitlab/metrics/method_call.rb:23:in `measure'
from lib/gitlab/metrics/instrumentation.rb:155:in `can?'
from app/services/projects/destroy_service.rb:18:in `execute'
```
Closes #22948
---
CHANGELOG | 1 +
app/models/project_feature.rb | 5 +++-
.../services/projects/destroy_service_spec.rb | 23 ++++++++++++++++++-
3 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/CHANGELOG b/CHANGELOG
index e2e3033615f..9635309b052 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -53,6 +53,7 @@ v 8.13.0 (unreleased)
v 8.12.4 (unreleased)
- Fix type mismatch bug when closing Jira issue
- Skip wiki creation when GitHub project has wiki enabled
+ - Fix failed project deletion when feature visibility set to private
- Fix issues importing services via Import/Export
- Restrict failed login attempts for users with 2FA enabled
- Fix "Copy to clipboard" tooltip to say "Copied!" when clipboard button is clicked. (lukehowell)
diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb
index 8c9534c3565..530f7d5a30e 100644
--- a/app/models/project_feature.rb
+++ b/app/models/project_feature.rb
@@ -20,7 +20,10 @@ class ProjectFeature < ActiveRecord::Base
FEATURES = %i(issues merge_requests wiki snippets builds)
- belongs_to :project
+ # Default scopes force us to unscope here since a service may need to check
+ # permissions for a project in pending_delete
+ # http://stackoverflow.com/questions/1540645/how-to-disable-default-scope-for-a-belongs-to
+ belongs_to :project, -> { unscope(where: :pending_delete) }
default_value_for :builds_access_level, value: ENABLED, allows_nil: false
default_value_for :issues_access_level, value: ENABLED, allows_nil: false
diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb
index 29341c5e57e..7dcd03496bb 100644
--- a/spec/services/projects/destroy_service_spec.rb
+++ b/spec/services/projects/destroy_service_spec.rb
@@ -5,6 +5,7 @@ describe Projects::DestroyService, services: true do
let!(:project) { create(:project, namespace: user.namespace) }
let!(:path) { project.repository.path_to_repo }
let!(:remove_path) { path.sub(/\.git\Z/, "+#{project.id}+deleted.git") }
+ let!(:async) { false } # execute or async_execute
context 'Sidekiq inline' do
before do
@@ -28,6 +29,22 @@ describe Projects::DestroyService, services: true do
it { expect(Dir.exist?(remove_path)).to be_truthy }
end
+ context 'async delete of project with private issue visibility' do
+ let!(:async) { true }
+
+ before do
+ project.project_feature.update_attribute("issues_access_level", ProjectFeature::PRIVATE)
+ # Run sidekiq immediately to check that renamed repository will be removed
+ Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
+ end
+
+ it 'deletes the project' do
+ expect(Project.all).not_to include(project)
+ expect(Dir.exist?(path)).to be_falsey
+ expect(Dir.exist?(remove_path)).to be_falsey
+ end
+ end
+
context 'container registry' do
before do
stub_container_registry_config(enabled: true)
@@ -52,6 +69,10 @@ describe Projects::DestroyService, services: true do
end
def destroy_project(project, user, params)
- Projects::DestroyService.new(project, user, params).execute
+ if async
+ Projects::DestroyService.new(project, user, params).async_execute
+ else
+ Projects::DestroyService.new(project, user, params).execute
+ end
end
end
--
GitLab