Commit f8c6c950 authored by drew cimino's avatar drew cimino Committed by Markus Koller

Allow destruction of records in archived pending_delete projects

This change stops the prevent-destroy permission from applying to records within
projects that are `pending_delete: true`. The purpose of the `prevent` is to
make "archived" projects reliably read-only, but this obviously shouldn't be the
case if someone is deleting a project.

This hasn't been an issue before since we relied on FK constraints to remove the
associated records after a project is deleted. But with the FKs going away in
the database decomposition effort, so too must our cascading destroys.

Changelog: fixed
parent ef7aafde
...@@ -47,6 +47,9 @@ class ProjectPolicy < BasePolicy ...@@ -47,6 +47,9 @@ class ProjectPolicy < BasePolicy
desc "Project is archived" desc "Project is archived"
condition(:archived, scope: :subject, score: 0) { project.archived? } condition(:archived, scope: :subject, score: 0) { project.archived? }
desc "Project is in the process of being deleted"
condition(:pending_delete) { project.pending_delete? }
condition(:default_issues_tracker, scope: :subject) { project.default_issues_tracker? } condition(:default_issues_tracker, scope: :subject) { project.default_issues_tracker? }
desc "Container registry is disabled" desc "Container registry is disabled"
...@@ -457,7 +460,13 @@ class ProjectPolicy < BasePolicy ...@@ -457,7 +460,13 @@ class ProjectPolicy < BasePolicy
prevent(*readonly_abilities) prevent(*readonly_abilities)
readonly_features.each do |feature| readonly_features.each do |feature|
prevent(*create_update_admin_destroy(feature)) prevent(*create_update_admin(feature))
end
end
rule { archived & ~pending_delete }.policy do
readonly_features.each do |feature|
prevent(:"destroy_#{feature}")
end end
end end
......
...@@ -105,28 +105,70 @@ RSpec.describe ProjectPolicy do ...@@ -105,28 +105,70 @@ RSpec.describe ProjectPolicy do
context 'pipeline feature' do context 'pipeline feature' do
let(:project) { private_project } let(:project) { private_project }
let(:current_user) { developer }
let(:pipeline) { create(:ci_pipeline, project: project) }
before do describe 'for confirmed user' do
private_project.add_developer(current_user) it 'allows modify pipelines' do
expect_allowed(:create_pipeline)
expect_allowed(:update_pipeline)
expect_allowed(:create_pipeline_schedule)
end
end end
describe 'for unconfirmed user' do describe 'for unconfirmed user' do
let(:current_user) { create(:user, confirmed_at: nil) } let(:current_user) { project.owner.tap { |u| u.update!(confirmed_at: nil) } }
it 'disallows to modify pipelines' do it 'disallows to modify pipelines' do
expect_disallowed(:create_pipeline) expect_disallowed(:create_pipeline)
expect_disallowed(:update_pipeline) expect_disallowed(:update_pipeline)
expect_disallowed(:destroy_pipeline)
expect_disallowed(:create_pipeline_schedule) expect_disallowed(:create_pipeline_schedule)
end end
end end
describe 'for confirmed user' do describe 'destroy permission' do
let(:current_user) { developer } describe 'for developers' do
it 'prevents :destroy_pipeline' do
expect(current_user.can?(:destroy_pipeline, pipeline)).to be_falsey
end
end
it 'allows modify pipelines' do describe 'for maintainers' do
expect_allowed(:create_pipeline) let(:current_user) { maintainer }
expect_allowed(:update_pipeline)
expect_allowed(:create_pipeline_schedule) it 'prevents :destroy_pipeline' do
project.add_maintainer(maintainer)
expect(current_user.can?(:destroy_pipeline, pipeline)).to be_falsey
end
end
describe 'for project owner' do
let(:current_user) { project.owner }
it 'allows :destroy_pipeline' do
expect(current_user.can?(:destroy_pipeline, pipeline)).to be_truthy
end
context 'on archived projects' do
before do
project.update!(archived: true)
end
it 'prevents :destroy_pipeline' do
expect(current_user.can?(:destroy_pipeline, pipeline)).to be_falsey
end
end
context 'on archived pending_delete projects' do
before do
project.update!(archived: true, pending_delete: true)
end
it 'allows :destroy_pipeline' do
expect(current_user.can?(:destroy_pipeline, pipeline)).to be_truthy
end
end
end end
end end
end end
......
...@@ -331,6 +331,14 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do ...@@ -331,6 +331,14 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do
end end
end end
end end
context 'for an archived project' do
before do
project.update!(archived: true)
end
it_behaves_like 'deleting the project with pipeline and build'
end
end end
describe 'container registry' do describe 'container registry' do
......
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