Commit eb202136 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'sh-fix-project-deletion-private-visibility' into 'master'

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

See merge request !6688
parents 3bf0d143 fea80aa1
...@@ -53,6 +53,7 @@ v 8.13.0 (unreleased) ...@@ -53,6 +53,7 @@ v 8.13.0 (unreleased)
v 8.12.4 (unreleased) v 8.12.4 (unreleased)
- Fix type mismatch bug when closing Jira issue - Fix type mismatch bug when closing Jira issue
- Skip wiki creation when GitHub project has wiki enabled - 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 - Fix issues importing services via Import/Export
- Restrict failed login attempts for users with 2FA enabled - Restrict failed login attempts for users with 2FA enabled
- Fix "Copy to clipboard" tooltip to say "Copied!" when clipboard button is clicked. (lukehowell) - Fix "Copy to clipboard" tooltip to say "Copied!" when clipboard button is clicked. (lukehowell)
......
...@@ -20,7 +20,10 @@ class ProjectFeature < ActiveRecord::Base ...@@ -20,7 +20,10 @@ class ProjectFeature < ActiveRecord::Base
FEATURES = %i(issues merge_requests wiki snippets builds) 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 :builds_access_level, value: ENABLED, allows_nil: false
default_value_for :issues_access_level, value: ENABLED, allows_nil: false default_value_for :issues_access_level, value: ENABLED, allows_nil: false
......
...@@ -5,6 +5,7 @@ describe Projects::DestroyService, services: true do ...@@ -5,6 +5,7 @@ describe Projects::DestroyService, services: true do
let!(:project) { create(:project, namespace: user.namespace) } let!(:project) { create(:project, namespace: user.namespace) }
let!(:path) { project.repository.path_to_repo } let!(:path) { project.repository.path_to_repo }
let!(:remove_path) { path.sub(/\.git\Z/, "+#{project.id}+deleted.git") } let!(:remove_path) { path.sub(/\.git\Z/, "+#{project.id}+deleted.git") }
let!(:async) { false } # execute or async_execute
context 'Sidekiq inline' do context 'Sidekiq inline' do
before do before do
...@@ -28,6 +29,22 @@ describe Projects::DestroyService, services: true do ...@@ -28,6 +29,22 @@ describe Projects::DestroyService, services: true do
it { expect(Dir.exist?(remove_path)).to be_truthy } it { expect(Dir.exist?(remove_path)).to be_truthy }
end 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 context 'container registry' do
before do before do
stub_container_registry_config(enabled: true) stub_container_registry_config(enabled: true)
...@@ -52,6 +69,10 @@ describe Projects::DestroyService, services: true do ...@@ -52,6 +69,10 @@ describe Projects::DestroyService, services: true do
end end
def destroy_project(project, user, params) def destroy_project(project, user, params)
if async
Projects::DestroyService.new(project, user, params).async_execute
else
Projects::DestroyService.new(project, user, params).execute Projects::DestroyService.new(project, user, params).execute
end end
end
end end
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