From e94eab1eaf71d17a86edc2975d9a816edf4c288e Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Wed, 5 Mar 2025 15:21:40 +0100 Subject: [PATCH 1/3] Protected containers: Integrate delete protection in GraphQL In GitLab v17.8, push protection for new container repositories has been released. This commits adds the delete protection for container repositories for the GraphQL API. Considers the feature flag :container_registry_protected_containers_delete by ignoring delete protection rules. This commits has been implemented in the context of the following EPIC https://gitlab.com/groups/gitlab-org/-/epics/9825 . Changelog: added --- .../container_repositories/destroy.rb | 20 +++++++ .../container_repository/destroy_spec.rb | 56 +++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/app/graphql/mutations/container_repositories/destroy.rb b/app/graphql/mutations/container_repositories/destroy.rb index f05d79ee08d3be..24e888c01675fe 100644 --- a/app/graphql/mutations/container_repositories/destroy.rb +++ b/app/graphql/mutations/container_repositories/destroy.rb @@ -20,6 +20,13 @@ class Destroy < ::Mutations::ContainerRepositories::DestroyBase def resolve(id:) container_repository = authorized_find!(id: id) + if protected_for_delete?(container_repository) + return { + container_repository: container_repository, + errors: ['Deleting protected repository path forbidden'] + } + end + container_repository.delete_scheduled! && audit_event(container_repository) track_event(:delete_repository, :container) @@ -35,6 +42,19 @@ def resolve(id:) def audit_event(repository) # defined in EE end + + def protected_for_delete?(container_repository) + return false unless Feature.enabled?(:container_registry_protected_containers_delete, + container_repository.project) + + service_response = ::ContainerRegistry::Protection::CheckRuleExistenceService.for_delete( + current_user: current_user, + project: container_repository.project, + params: { repository_path: container_repository.path.to_s } + ).execute + + service_response.success? && service_response[:protection_rule_exists?] + end end end end diff --git a/spec/requests/api/graphql/mutations/container_repository/destroy_spec.rb b/spec/requests/api/graphql/mutations/container_repository/destroy_spec.rb index 833fc6f4e9dc7b..8ace76af312f5f 100644 --- a/spec/requests/api/graphql/mutations/container_repository/destroy_spec.rb +++ b/spec/requests/api/graphql/mutations/container_repository/destroy_spec.rb @@ -139,5 +139,61 @@ it_behaves_like 'destroying the container repository' end end + + context 'with delete protection rule', :enable_admin_mode do + let_it_be(:maintainer) { create(:user, maintainer_of: [project]) } + let_it_be(:owner) { create(:user, owner_of: [project]) } + let_it_be(:instance_admin) { create(:admin) } + + let_it_be_with_reload(:container_registry_protection_rule) do + create(:container_registry_protection_rule, project: project) + end + + before do + container_registry_protection_rule.update!( + repository_path_pattern: container_repository.path, + minimum_access_level_for_delete: minimum_access_level_for_delete + ) + end + + shared_examples 'protected deletion of container repository' do + it_behaves_like 'returning response status', :success + + it 'returns error message' do + subject + + expect(mutation_response).to include 'errors' => ['Deleting protected repository path forbidden'] + end + + context 'when feature flag :container_registry_protected_containers_delete is disabled' do + before do + stub_feature_flags(container_registry_protected_containers_delete: false) + end + + it_behaves_like 'destroying the container repository' + end + end + + where(:minimum_access_level_for_delete, :current_user, :expected_shared_example) do + nil | ref(:maintainer) | 'destroying the container repository' + nil | ref(:owner) | 'destroying the container repository' + + :maintainer | ref(:maintainer) | 'destroying the container repository' + :maintainer | ref(:owner) | 'destroying the container repository' + :maintainer | ref(:instance_admin) | 'destroying the container repository' + + :owner | ref(:maintainer) | 'protected deletion of container repository' + :owner | ref(:owner) | 'destroying the container repository' + :owner | ref(:instance_admin) | 'destroying the container repository' + + :admin | ref(:maintainer) | 'protected deletion of container repository' + :admin | ref(:owner) | 'protected deletion of container repository' + :admin | ref(:instance_admin) | 'destroying the container repository' + end + + with_them do + it_behaves_like params[:expected_shared_example] + end + end end end -- GitLab From bba13c0ad027c7d8400150729b4325ab85b2354f Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Mon, 9 Jun 2025 10:00:04 +0200 Subject: [PATCH 2/3] refactor: Apply suggestion from @annabeldunstone - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/183545#note_2547022001 --- app/graphql/mutations/container_repositories/destroy.rb | 2 +- .../api/graphql/mutations/container_repository/destroy_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/graphql/mutations/container_repositories/destroy.rb b/app/graphql/mutations/container_repositories/destroy.rb index 24e888c01675fe..ed5346fdb41bd2 100644 --- a/app/graphql/mutations/container_repositories/destroy.rb +++ b/app/graphql/mutations/container_repositories/destroy.rb @@ -23,7 +23,7 @@ def resolve(id:) if protected_for_delete?(container_repository) return { container_repository: container_repository, - errors: ['Deleting protected repository path forbidden'] + errors: ['Deleting the protected repository path is forbidden'] } end diff --git a/spec/requests/api/graphql/mutations/container_repository/destroy_spec.rb b/spec/requests/api/graphql/mutations/container_repository/destroy_spec.rb index 8ace76af312f5f..71cb537ae4eb76 100644 --- a/spec/requests/api/graphql/mutations/container_repository/destroy_spec.rb +++ b/spec/requests/api/graphql/mutations/container_repository/destroy_spec.rb @@ -162,7 +162,7 @@ it 'returns error message' do subject - expect(mutation_response).to include 'errors' => ['Deleting protected repository path forbidden'] + expect(mutation_response).to include 'errors' => ['Deleting the protected repository path is forbidden'] end context 'when feature flag :container_registry_protected_containers_delete is disabled' do -- GitLab From 40f2d4ecd0f3470bc894c66c46170ca0085de219 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Sat, 5 Jul 2025 09:00:24 +0200 Subject: [PATCH 3/3] refactor: Apply suggestion from @dmeshcharakou - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/183545#note_2567145948 --- app/graphql/mutations/container_repositories/destroy.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/graphql/mutations/container_repositories/destroy.rb b/app/graphql/mutations/container_repositories/destroy.rb index ed5346fdb41bd2..3f59e1b396aea5 100644 --- a/app/graphql/mutations/container_repositories/destroy.rb +++ b/app/graphql/mutations/container_repositories/destroy.rb @@ -45,7 +45,7 @@ def audit_event(repository) def protected_for_delete?(container_repository) return false unless Feature.enabled?(:container_registry_protected_containers_delete, - container_repository.project) + container_repository.project&.root_ancestor) service_response = ::ContainerRegistry::Protection::CheckRuleExistenceService.for_delete( current_user: current_user, -- GitLab