From c819cad0741d07338e5543d0638d7e029525ee91 Mon Sep 17 00:00:00 2001
From: Gerardo Navarro <gerardo@b310.de>
Date: Fri, 31 Jan 2025 15:13:50 +0100
Subject: [PATCH 1/6] Protected packages: Integrate delete protection

In GitLab 17.6, the package protection feature was made generally
available. But, this realease did not include
the deletion package protection,
see https://gitlab.com/gitlab-org/gitlab/-/issues/472655 .

The attribute `minimum_access_level_for_delete` that was introduced
in the MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179739 .

This attribute is integrated in this MR in order to prevent the deletion
of delete protected packages.

At the moment, the attribute `minimum_access_level_for_delete` cannot be
updated via UI, REST nor GraphQL API. This possibility will be added in
a future iteration.

Changelog: added
---
 app/models/packages/protection/rule.rb        |  5 +
 .../check_delete_rule_existence_service.rb    | 44 +++++++++
 .../check_rule_existence_service.rb           |  3 +-
 .../packages_protected_packages_delete.yml    |  9 ++
 doc/api/packages.md                           |  7 +-
 lib/api/package_files.rb                      | 13 +++
 lib/api/project_packages.rb                   | 12 +++
 spec/requests/api/package_files_spec.rb       | 99 ++++++++++++++++++-
 spec/requests/api/project_packages_spec.rb    | 76 ++++++++++++++
 ...heck_delete_rule_existence_service_spec.rb | 83 ++++++++++++++++
 10 files changed, 347 insertions(+), 4 deletions(-)
 create mode 100644 app/services/packages/protection/check_delete_rule_existence_service.rb
 create mode 100644 config/feature_flags/gitlab_com_derisk/packages_protected_packages_delete.yml
 create mode 100644 spec/services/packages/protection/check_delete_rule_existence_service_spec.rb

diff --git a/app/models/packages/protection/rule.rb b/app/models/packages/protection/rule.rb
index 0913bc875ec414..fe05c69470a764 100644
--- a/app/models/packages/protection/rule.rb
+++ b/app/models/packages/protection/rule.rb
@@ -42,6 +42,11 @@ class Rule < ApplicationRecord
 
       scope :for_package_type, ->(package_type) { where(package_type: package_type) }
 
+      def self.for_delete_exists?(access_level:, package_name:, package_type:)
+        for_action_exists?(action: :delete, access_level: access_level, package_name: package_name,
+          package_type: package_type)
+      end
+
       def self.for_action_exists?(action:, access_level:, package_name:, package_type:)
         return false if [access_level, package_name, package_type].any?(&:blank?)
 
diff --git a/app/services/packages/protection/check_delete_rule_existence_service.rb b/app/services/packages/protection/check_delete_rule_existence_service.rb
new file mode 100644
index 00000000000000..f25018cf5eb8c5
--- /dev/null
+++ b/app/services/packages/protection/check_delete_rule_existence_service.rb
@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+module Packages
+  module Protection
+    class CheckDeleteRuleExistenceService < BaseProjectService
+      SUCCESS_RESPONSE_RULE_EXISTS = ServiceResponse.success(payload: { protection_rule_exists?: true }).freeze
+      SUCCESS_RESPONSE_RULE_DOESNT_EXIST = ServiceResponse.success(payload: { protection_rule_exists?: false }).freeze
+
+      ERROR_RESPONSE_UNAUTHORIZED = ServiceResponse.error(message: 'Unauthorized', reason: :unauthorized).freeze
+      ERROR_RESPONSE_INVALID_PACKAGE_TYPE = ServiceResponse.error(message: 'Invalid package type',
+        reason: :invalid_package_type).freeze
+
+      def execute
+        return ERROR_RESPONSE_INVALID_PACKAGE_TYPE unless package_type_allowed?
+        return ERROR_RESPONSE_UNAUTHORIZED unless current_user_can_destroy_package?
+        return service_response_for(false) if current_user.can_admin_all_resources?
+
+        user_project_authorization_access_level = current_user.max_member_access_for_project(project.id)
+
+        project.package_protection_rules
+               .for_delete_exists?(
+                 access_level: user_project_authorization_access_level,
+                 package_name: params[:package_name],
+                 package_type: params[:package_type]
+               )
+               .then { |result| service_response_for(result) }
+      end
+
+      private
+
+      def package_type_allowed?
+        Packages::Protection::Rule.package_types.key?(params[:package_type])
+      end
+
+      def current_user_can_destroy_package?
+        can?(current_user, :destroy_package, project)
+      end
+
+      def service_response_for(protection_rule_exists)
+        protection_rule_exists ? SUCCESS_RESPONSE_RULE_EXISTS : SUCCESS_RESPONSE_RULE_DOESNT_EXIST
+      end
+    end
+  end
+end
diff --git a/app/services/packages/protection/check_rule_existence_service.rb b/app/services/packages/protection/check_rule_existence_service.rb
index 8a705e3bda0c36..d97ae99838429a 100644
--- a/app/services/packages/protection/check_rule_existence_service.rb
+++ b/app/services/packages/protection/check_rule_existence_service.rb
@@ -34,8 +34,7 @@ def check_rule_exists_for_user
         return false if current_user.can_admin_all_resources?
 
         user_project_authorization_access_level = current_user.max_member_access_for_project(project.id)
-        project.package_protection_rules
-        .for_action_exists?(
+        project.package_protection_rules.for_action_exists?(
           action: :push,
           access_level: user_project_authorization_access_level,
           package_name: params[:package_name],
diff --git a/config/feature_flags/gitlab_com_derisk/packages_protected_packages_delete.yml b/config/feature_flags/gitlab_com_derisk/packages_protected_packages_delete.yml
new file mode 100644
index 00000000000000..7911ee3ca2e022
--- /dev/null
+++ b/config/feature_flags/gitlab_com_derisk/packages_protected_packages_delete.yml
@@ -0,0 +1,9 @@
+---
+name: packages_protected_packages_delete
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/323970
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179931
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/516215
+milestone: '17.10'
+group: group::package registry
+type: gitlab_com_derisk
+default_enabled: false
diff --git a/doc/api/packages.md b/doc/api/packages.md
index dcbca62da2f792..982433ba565f32 100644
--- a/doc/api/packages.md
+++ b/doc/api/packages.md
@@ -434,11 +434,14 @@ curl --request DELETE --header "PRIVATE-TOKEN: <your_access_token>" "https://git
 Can return the following status codes:
 
 - `204 No Content`, if the package was deleted successfully.
+- `403 Forbidden`, if the package is protected from deletion.
 - `404 Not Found`, if the package was not found.
 
 If [request forwarding](../user/packages/package_registry/supported_functionality.md#forwarding-requests) is enabled,
 deleting a package can introduce a [dependency confusion risk](../user/packages/package_registry/supported_functionality.md#deleting-packages).
 
+If [a protection rule for this package exists](../user/packages/package_registry/package_protection_rules.md#protect-a-package), then deleting a package is forbidden.
+
 ## Delete a package file
 
 {{< alert type="warning" >}}
@@ -467,5 +470,7 @@ curl --request DELETE --header "PRIVATE-TOKEN: <your_access_token>" "https://git
 Can return the following status codes:
 
 - `204 No Content`: The package was deleted successfully.
-- `403 Forbidden`: The user does not have permission to delete the file.
+- `403 Forbidden`: The user does not have permission to delete the file or the package is protected from deletion.
 - `404 Not Found`: The package or package file was not found.
+
+If [a protection rule for the package exists](../user/packages/package_registry/package_protection_rules.md#protect-a-package) that this package file belongs to, then deleting this package file is forbidden.
diff --git a/lib/api/package_files.rb b/lib/api/package_files.rb
index 9f9ad173d32398..dfeb5bd4966c81 100644
--- a/lib/api/package_files.rb
+++ b/lib/api/package_files.rb
@@ -67,6 +67,19 @@ class PackageFiles < ::API::Base
 
         not_found! unless package
 
+        if Feature.enabled?(:packages_protected_packages_delete, user_project) &&
+            !current_user.can_admin_all_resources?
+          service_response =
+            Packages::Protection::CheckDeleteRuleExistenceService.new(
+              project: user_project,
+              current_user: current_user,
+              params: { package_name: package.name, package_type: package.package_type }
+            ).execute
+
+          bad_request(service_response.message) if service_response.error?
+          forbidden!('Package is deletion protected.') if service_response[:protection_rule_exists?]
+        end
+
         package_file = package.installable_package_files
                               .find_by_id(params[:package_file_id])
 
diff --git a/lib/api/project_packages.rb b/lib/api/project_packages.rb
index 99d9658d9d5159..9a11fe0ecda5d4 100644
--- a/lib/api/project_packages.rb
+++ b/lib/api/project_packages.rb
@@ -139,6 +139,18 @@ def package
       delete ':id/packages/:package_id' do
         authorize_destroy_package!(user_project)
 
+        if Feature.enabled?(:packages_protected_packages_delete, user_project) && !current_user.can_admin_all_resources?
+          service_response =
+            Packages::Protection::CheckDeleteRuleExistenceService.new(
+              project: user_project,
+              current_user: current_user,
+              params: { package_name: package.name, package_type: package.package_type }
+            ).execute
+
+          bad_request(service_response.message) if service_response.error?
+          forbidden!('Package is deletion protected.') if service_response[:protection_rule_exists?]
+        end
+
         destroy_conditionally!(package) do |package|
           ::Packages::MarkPackageForDestructionService.new(container: package, current_user: current_user).execute
         end
diff --git a/spec/requests/api/package_files_spec.rb b/spec/requests/api/package_files_spec.rb
index 301e2cd81bf0fb..e77a26e681d3a6 100644
--- a/spec/requests/api/package_files_spec.rb
+++ b/spec/requests/api/package_files_spec.rb
@@ -4,7 +4,7 @@
 
 RSpec.describe API::PackageFiles, feature_category: :package_registry do
   let(:user) { create(:user) }
-  let(:project) { create(:project, :public) }
+  let_it_be(:project) { create(:project, :public) }
   let(:package) { create(:maven_package, project: project) }
 
   describe 'GET /projects/:id/packages/:package_id/package_files' do
@@ -274,5 +274,102 @@
         end
       end
     end
+
+    context 'with package protection rule for different roles and package_name_patterns', :enable_admin_mode do
+      using RSpec::Parameterized::TableSyntax
+
+      let_it_be(:pat_project_maintainer) do
+        create(:personal_access_token, user: create(:user, maintainer_of: [project]))
+      end
+
+      let_it_be(:pat_project_owner) { create(:personal_access_token, user: create(:user, owner_of: [project])) }
+      let_it_be(:pat_instance_admin) { create(:personal_access_token, :admin_mode, user: create(:admin)) }
+      let_it_be(:headers_pat_project_maintainer) do
+        { Gitlab::Auth::AuthFinders::PRIVATE_TOKEN_HEADER => pat_project_maintainer.token }
+      end
+
+      let_it_be(:headers_pat_project_owner) do
+        { Gitlab::Auth::AuthFinders::PRIVATE_TOKEN_HEADER => pat_project_owner.token }
+      end
+
+      let_it_be(:headers_pat_instance_admin) do
+        { Gitlab::Auth::AuthFinders::PRIVATE_TOKEN_HEADER => pat_instance_admin.token }
+      end
+
+      let_it_be(:job_from_project_maintainer) do
+        create(:ci_build, :running, user: pat_project_maintainer.user, project: project)
+      end
+
+      let_it_be(:job_from_project_owner) { create(:ci_build, :running, user: pat_project_owner.user, project: project) }
+      let(:headers_job_token_from_maintainer) do
+        { Gitlab::Auth::AuthFinders::JOB_TOKEN_HEADER => job_from_project_maintainer.token }
+      end
+
+      let(:headers_job_token_from_owner) do
+        { Gitlab::Auth::AuthFinders::JOB_TOKEN_HEADER => job_from_project_owner.token }
+      end
+
+      let(:package_protection_rule) { create(:package_protection_rule, project: project) }
+
+      let(:package_name) { package.name }
+      let(:package_name_no_match) { "#{package_name}_no_match" }
+
+      subject do
+        delete api(url), headers: headers
+        response
+      end
+
+      before do
+        package_protection_rule.update!(
+          package_name_pattern: package_name_pattern,
+          package_type: package.package_type,
+          minimum_access_level_for_delete: minimum_access_level_for_delete
+        )
+      end
+
+      shared_examples 'deleting package protected' do
+        it_behaves_like 'returning response status', :forbidden
+        it 'responds with correct error message' do
+          subject
+
+          expect(json_response).to include('message' => "403 Forbidden - Package is deletion protected.")
+        end
+
+        it { expect { subject }.not_to change { ::Packages::Package.pending_destruction.count } }
+
+        context 'when feature flag :packages_protected_packages_delete disabled' do
+          before do
+            stub_feature_flags(packages_protected_packages_delete: false)
+          end
+
+          it_behaves_like 'deleting package'
+        end
+      end
+
+      shared_examples 'deleting package' do
+        it_behaves_like 'returning response status', :no_content
+        it { expect { subject }.to change { package.package_files.pending_destruction.count }.by(1) }
+      end
+
+      where(:package_name_pattern, :minimum_access_level_for_delete, :headers, :shared_examples_name) do
+        ref(:package_name)          | :owner | ref(:headers_job_token_from_maintainer) | 'deleting package protected'
+        ref(:package_name)          | :owner | ref(:headers_job_token_from_owner)      | 'deleting package'
+        ref(:package_name)          | :owner | ref(:headers_pat_project_maintainer)    | 'deleting package protected'
+        ref(:package_name)          | :owner | ref(:headers_pat_project_owner)         | 'deleting package'
+        ref(:package_name)          | :owner | ref(:headers_pat_instance_admin)        | 'deleting package'
+
+        ref(:package_name)          | :admin | ref(:headers_pat_project_maintainer)    | 'deleting package protected'
+        ref(:package_name)          | :admin | ref(:headers_pat_project_owner)         | 'deleting package protected'
+        ref(:package_name)          | :admin | ref(:headers_job_token_from_owner)      | 'deleting package protected'
+        ref(:package_name)          | :admin | ref(:headers_pat_instance_admin)        | 'deleting package'
+
+        ref(:package_name_no_match) | :owner | ref(:headers_pat_project_owner)         | 'deleting package'
+        ref(:package_name_no_match) | :owner | ref(:headers_pat_project_owner)         | 'deleting package'
+      end
+
+      with_them do
+        it_behaves_like params[:shared_examples_name]
+      end
+    end
   end
 end
diff --git a/spec/requests/api/project_packages_spec.rb b/spec/requests/api/project_packages_spec.rb
index d26bbdb7b06004..b44e2af5de18a7 100644
--- a/spec/requests/api/project_packages_spec.rb
+++ b/spec/requests/api/project_packages_spec.rb
@@ -735,6 +735,82 @@
             expect(response).to have_gitlab_http_status(:no_content)
           end
         end
+
+        context 'with package protection rule for different roles and package_name_patterns', :enable_admin_mode do
+          let_it_be(:pat_project_maintainer) { create(:personal_access_token, user: create(:user, maintainer_of: [project])) }
+          let_it_be(:pat_project_owner) { create(:personal_access_token, user: create(:user, owner_of: [project])) }
+          let_it_be(:pat_instance_admin) { create(:personal_access_token, :admin_mode, user: create(:admin)) }
+          let_it_be(:headers_pat_project_maintainer) { { Gitlab::Auth::AuthFinders::PRIVATE_TOKEN_HEADER => pat_project_maintainer.token } }
+          let_it_be(:headers_pat_project_owner) { { Gitlab::Auth::AuthFinders::PRIVATE_TOKEN_HEADER => pat_project_owner.token } }
+          let_it_be(:headers_pat_instance_admin) { { Gitlab::Auth::AuthFinders::PRIVATE_TOKEN_HEADER => pat_instance_admin.token } }
+
+          let_it_be(:job_from_project_maintainer) { create(:ci_build, :running, user: pat_project_maintainer.user, project: project) }
+          let_it_be(:job_from_project_owner) { create(:ci_build, :running, user: pat_project_owner.user, project: project) }
+          let(:headers_job_token_from_maintainer) { { Gitlab::Auth::AuthFinders::JOB_TOKEN_HEADER => job_from_project_maintainer.token } }
+          let(:headers_job_token_from_owner) { { Gitlab::Auth::AuthFinders::JOB_TOKEN_HEADER => job_from_project_owner.token } }
+
+          let(:package_protection_rule) { create(:package_protection_rule, project: project) }
+
+          let(:package_name) { package1.name }
+          let(:package_name_no_match) { "#{package_name}_no_match" }
+
+          subject do
+            delete api(package_url), headers: headers
+            response
+          end
+
+          before do
+            package_protection_rule.update!(
+              package_name_pattern: package_name_pattern,
+              package_type: package1.package_type,
+              minimum_access_level_for_delete: minimum_access_level_for_delete
+            )
+          end
+
+          shared_examples 'deleting package protected' do
+            it_behaves_like 'returning response status', :forbidden
+            it do
+              subject
+
+              expect(json_response).to include('message' => "403 Forbidden - Package is deletion protected.")
+            end
+
+            it { expect { subject }.not_to change { ::Packages::Package.pending_destruction.count } }
+
+            context 'when feature flag :packages_protected_packages_delete disabled' do
+              before do
+                stub_feature_flags(packages_protected_packages_delete: false)
+              end
+
+              it_behaves_like 'deleting package'
+            end
+          end
+
+          shared_examples 'deleting package' do
+            it_behaves_like 'returning response status', :no_content
+            it { expect { subject }.to change { ::Packages::Package.pending_destruction.count }.by(1) }
+          end
+
+          where(:package_name_pattern, :minimum_access_level_for_delete, :headers, :shared_examples_name) do
+            ref(:package_name)          | :owner | ref(:headers_job_token_from_maintainer) | 'deleting package protected'
+            ref(:package_name)          | :owner | ref(:headers_job_token_from_owner)      | 'deleting package'
+            ref(:package_name)          | :owner | ref(:headers_pat_project_maintainer)    | 'deleting package protected'
+            ref(:package_name)          | :owner | ref(:headers_pat_project_owner)         | 'deleting package'
+            ref(:package_name)          | :owner | ref(:headers_pat_instance_admin)        | 'deleting package'
+
+            ref(:package_name)          | :admin | ref(:headers_pat_project_maintainer)    | 'deleting package protected'
+            ref(:package_name)          | :admin | ref(:headers_pat_project_owner)         | 'deleting package protected'
+            ref(:package_name)          | :admin | ref(:headers_job_token_from_owner)      | 'deleting package protected'
+            ref(:package_name)          | :admin | ref(:headers_pat_instance_admin)        | 'deleting package'
+
+            ref(:package_name_no_match) | :owner | ref(:headers_pat_project_owner)         | 'deleting package'
+            ref(:package_name_no_match) | :owner | ref(:headers_pat_project_owner)         | 'deleting package'
+          end
+
+          with_them do
+            it_behaves_like params[:shared_examples_name]
+          end
+        end
       end
 
       context 'with a maven package' do
diff --git a/spec/services/packages/protection/check_delete_rule_existence_service_spec.rb b/spec/services/packages/protection/check_delete_rule_existence_service_spec.rb
new file mode 100644
index 00000000000000..bd4eb1fe1a5fb9
--- /dev/null
+++ b/spec/services/packages/protection/check_delete_rule_existence_service_spec.rb
@@ -0,0 +1,83 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Packages::Protection::CheckDeleteRuleExistenceService, feature_category: :package_registry do
+  using RSpec::Parameterized::TableSyntax
+
+  let_it_be(:project) { create(:project) }
+  let_it_be(:unauthorized_user) { create(:user) }
+  let_it_be(:project_developer) { create(:user, developer_of: project) }
+  let_it_be(:project_maintainer) { create(:user, maintainer_of: project) }
+  let_it_be(:project_owner) { project.owner }
+  let_it_be(:instance_admin) { create(:admin) }
+
+  let_it_be(:container_protection_rule_npm) do
+    create(:package_protection_rule,
+      project: project,
+      package_type: :npm,
+      package_name_pattern: "@#{project.full_path}*",
+      minimum_access_level_for_delete: :owner)
+  end
+
+  let_it_be(:container_protection_rule_pypi) do
+    create(:package_protection_rule,
+      project: project,
+      package_type: :pypi,
+      package_name_pattern: "#{project.full_path}*",
+      minimum_access_level_for_delete: :admin)
+  end
+
+  let(:params) { { package_name: package_name, package_type: package_type } }
+  let(:service) { described_class.new(project: project, current_user: current_user, params: params) }
+
+  subject(:service_response) { service.execute }
+
+  shared_examples 'a service response for protection rule exists' do
+    it_behaves_like 'returning a success service response'
+    it { is_expected.to have_attributes(payload: { protection_rule_exists?: true }) }
+  end
+
+  shared_examples 'a service response for protection rule does not exist' do
+    it_behaves_like 'returning a success service response'
+    it { is_expected.to have_attributes(payload: { protection_rule_exists?: false }) }
+  end
+
+  shared_examples 'an error service response for unauthorized actor' do
+    it_behaves_like 'returning an error service response', message: 'Unauthorized'
+    it { is_expected.to have_attributes reason: :unauthorized }
+  end
+
+  shared_examples 'an error service response for invalid package type' do
+    it_behaves_like 'returning an error service response', message: 'Invalid package type'
+    it { is_expected.to have_attributes reason: :invalid_package_type }
+  end
+
+  describe '#execute', :enable_admin_mode do
+    # rubocop:disable Layout/LineLength -- Avoid formatting in favor of one-line table syntax
+    where(:package_name, :package_type, :current_user, :expected_shared_example) do
+      lazy { "@#{project.full_path}" }             | :npm     | ref(:project_developer)  | 'an error service response for unauthorized actor'
+      lazy { "@#{project.full_path}" }             | :npm     | ref(:project_maintainer) | 'a service response for protection rule exists'
+      lazy { "@#{project.full_path}" }             | :npm     | ref(:project_owner)      | 'a service response for protection rule does not exist'
+      lazy { "@#{project.full_path}" }             | :npm     | ref(:instance_admin)     | 'a service response for protection rule does not exist'
+      lazy { "@other-scope/#{project.full_path}" } | :npm     | ref(:project_maintainer) | 'a service response for protection rule does not exist'
+      lazy { "@other-scope/#{project.full_path}" } | :npm     | ref(:project_owner)      | 'a service response for protection rule does not exist'
+      lazy { project.full_path }                   | :pypi    | ref(:project_maintainer) | 'a service response for protection rule exists'
+      lazy { project.full_path }                   | :pypi    | ref(:project_owner)      | 'a service response for protection rule exists'
+      lazy { project.full_path }                   | :pypi    | ref(:instance_admin)     | 'a service response for protection rule does not exist'
+
+      # Edge cases
+      lazy { "@#{project.full_path}" }             | :npm     | ref(:unauthorized_user)  | 'an error service response for unauthorized actor'
+      lazy { "@#{project.full_path}" }             | :npm     | nil                      | 'an error service response for unauthorized actor'
+      lazy { "@#{project.full_path}" }             | :no_type | nil                      | 'an error service response for invalid package type'
+      lazy { "@#{project.full_path}" }             | nil      | ref(:project_owner)      | 'an error service response for invalid package type'
+      nil                                          | :npm     | ref(:project_owner)      | 'a service response for protection rule does not exist'
+      nil                                          | nil      | ref(:project_owner)      | 'an error service response for invalid package type'
+    end
+    # rubocop:enable Layout/LineLength
+
+    with_them do
+      it_behaves_like params[:expected_shared_example]
+    end
+  end
+end
-- 
GitLab


From a487c7439fe0910d32e19cfef36fe7769e7481b8 Mon Sep 17 00:00:00 2001
From: Gerardo Navarro <gerardo@b310.de>
Date: Thu, 27 Feb 2025 14:31:52 +0000
Subject: [PATCH 2/6] docs: Apply suggestions from @z_painter

- https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179931#note_2370677818
- https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179931#note_2370677806
- https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179931#note_2370639583
---
 doc/api/packages.md | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/doc/api/packages.md b/doc/api/packages.md
index 982433ba565f32..3186f0f1df23e3 100644
--- a/doc/api/packages.md
+++ b/doc/api/packages.md
@@ -433,14 +433,14 @@ curl --request DELETE --header "PRIVATE-TOKEN: <your_access_token>" "https://git
 
 Can return the following status codes:
 
-- `204 No Content`, if the package was deleted successfully.
-- `403 Forbidden`, if the package is protected from deletion.
-- `404 Not Found`, if the package was not found.
+- `204 No Content`: The package was deleted successfully.
+- `403 Forbidden`: The package is protected from deletion.
+- `404 Not Found`: The package was not found.
 
 If [request forwarding](../user/packages/package_registry/supported_functionality.md#forwarding-requests) is enabled,
 deleting a package can introduce a [dependency confusion risk](../user/packages/package_registry/supported_functionality.md#deleting-packages).
 
-If [a protection rule for this package exists](../user/packages/package_registry/package_protection_rules.md#protect-a-package), then deleting a package is forbidden.
+If a package is protected by a [protection rule](../user/packages/package_registry/package_protection_rules.md#protect-a-package), then deleting the package is forbidden.
 
 ## Delete a package file
 
@@ -473,4 +473,4 @@ Can return the following status codes:
 - `403 Forbidden`: The user does not have permission to delete the file or the package is protected from deletion.
 - `404 Not Found`: The package or package file was not found.
 
-If [a protection rule for the package exists](../user/packages/package_registry/package_protection_rules.md#protect-a-package) that this package file belongs to, then deleting this package file is forbidden.
+If a package that a package file belongs to is protected by a [protection rule](../user/packages/package_registry/package_protection_rules.md#protect-a-package), then deleting the package file is forbidden.
-- 
GitLab


From 4de2e214beffb55dc3c42ab29164347a46c13ca3 Mon Sep 17 00:00:00 2001
From: Gerardo Navarro <gerardo@b310.de>
Date: Fri, 28 Feb 2025 12:21:24 +0100
Subject: [PATCH 3/6] refcator: Apply suggestions from @srushik

---
 lib/api/package_files.rb    | 5 ++---
 lib/api/project_packages.rb | 4 ++--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/api/package_files.rb b/lib/api/package_files.rb
index dfeb5bd4966c81..b495ec2c53c877 100644
--- a/lib/api/package_files.rb
+++ b/lib/api/package_files.rb
@@ -67,8 +67,7 @@ class PackageFiles < ::API::Base
 
         not_found! unless package
 
-        if Feature.enabled?(:packages_protected_packages_delete, user_project) &&
-            !current_user.can_admin_all_resources?
+        if Feature.enabled?(:packages_protected_packages_delete, user_project)
           service_response =
             Packages::Protection::CheckDeleteRuleExistenceService.new(
               project: user_project,
@@ -76,7 +75,7 @@ class PackageFiles < ::API::Base
               params: { package_name: package.name, package_type: package.package_type }
             ).execute
 
-          bad_request(service_response.message) if service_response.error?
+          bad_request!(service_response.message) if service_response.error?
           forbidden!('Package is deletion protected.') if service_response[:protection_rule_exists?]
         end
 
diff --git a/lib/api/project_packages.rb b/lib/api/project_packages.rb
index 9a11fe0ecda5d4..5928852d8386b9 100644
--- a/lib/api/project_packages.rb
+++ b/lib/api/project_packages.rb
@@ -139,7 +139,7 @@ def package
       delete ':id/packages/:package_id' do
         authorize_destroy_package!(user_project)
 
-        if Feature.enabled?(:packages_protected_packages_delete, user_project) && !current_user.can_admin_all_resources?
+        if Feature.enabled?(:packages_protected_packages_delete, user_project)
           service_response =
             Packages::Protection::CheckDeleteRuleExistenceService.new(
               project: user_project,
@@ -147,7 +147,7 @@ def package
               params: { package_name: package.name, package_type: package.package_type }
             ).execute
 
-          bad_request(service_response.message) if service_response.error?
+          bad_request!(service_response.message) if service_response.error?
           forbidden!('Package is deletion protected.') if service_response[:protection_rule_exists?]
         end
 
-- 
GitLab


From 709fa7320170cbbe5f051342339584db7c71e4c6 Mon Sep 17 00:00:00 2001
From: Gerardo Navarro <gerardo@b310.de>
Date: Sun, 2 Mar 2025 17:59:05 +0100
Subject: [PATCH 4/6] test: Add test case for unsupported package type

We have not implemented package protection for all package types.

This commit adds a test case to ensure that unsupported package types
are not deletion protected.

In the future, it is the plan to implement package protection
for all package types. Until then, this test case is necessary.
---
 lib/api/package_files.rb                   |  1 -
 lib/api/project_packages.rb                |  1 -
 spec/requests/api/package_files_spec.rb    | 27 +++++++++++-----
 spec/requests/api/project_packages_spec.rb | 37 +++++++++++++++-------
 4 files changed, 44 insertions(+), 22 deletions(-)

diff --git a/lib/api/package_files.rb b/lib/api/package_files.rb
index b495ec2c53c877..65eeb50c604761 100644
--- a/lib/api/package_files.rb
+++ b/lib/api/package_files.rb
@@ -75,7 +75,6 @@ class PackageFiles < ::API::Base
               params: { package_name: package.name, package_type: package.package_type }
             ).execute
 
-          bad_request!(service_response.message) if service_response.error?
           forbidden!('Package is deletion protected.') if service_response[:protection_rule_exists?]
         end
 
diff --git a/lib/api/project_packages.rb b/lib/api/project_packages.rb
index 5928852d8386b9..cc845deb595e60 100644
--- a/lib/api/project_packages.rb
+++ b/lib/api/project_packages.rb
@@ -147,7 +147,6 @@ def package
               params: { package_name: package.name, package_type: package.package_type }
             ).execute
 
-          bad_request!(service_response.message) if service_response.error?
           forbidden!('Package is deletion protected.') if service_response[:protection_rule_exists?]
         end
 
diff --git a/spec/requests/api/package_files_spec.rb b/spec/requests/api/package_files_spec.rb
index e77a26e681d3a6..4407ac2cb1e252 100644
--- a/spec/requests/api/package_files_spec.rb
+++ b/spec/requests/api/package_files_spec.rb
@@ -319,14 +319,6 @@
         response
       end
 
-      before do
-        package_protection_rule.update!(
-          package_name_pattern: package_name_pattern,
-          package_type: package.package_type,
-          minimum_access_level_for_delete: minimum_access_level_for_delete
-        )
-      end
-
       shared_examples 'deleting package protected' do
         it_behaves_like 'returning response status', :forbidden
         it 'responds with correct error message' do
@@ -368,8 +360,27 @@
       end
 
       with_them do
+        before do
+          package_protection_rule.update!(
+            package_name_pattern: package_name_pattern,
+            package_type: package.package_type,
+            minimum_access_level_for_delete: minimum_access_level_for_delete
+          )
+        end
+
         it_behaves_like params[:shared_examples_name]
       end
+
+      context 'for package with unsupported package type for package protection rule' do
+        let_it_be(:nuget_package) { create(:nuget_package, project: project) }
+
+        let(:package) { nuget_package }
+        let(:package_file_id) { nuget_package.package_files.first.id }
+
+        let(:headers) { headers_pat_project_maintainer }
+
+        it_behaves_like 'deleting package'
+      end
     end
   end
 end
diff --git a/spec/requests/api/project_packages_spec.rb b/spec/requests/api/project_packages_spec.rb
index b44e2af5de18a7..4e9cd69a84ec69 100644
--- a/spec/requests/api/project_packages_spec.rb
+++ b/spec/requests/api/project_packages_spec.rb
@@ -743,11 +743,11 @@
           let_it_be(:headers_pat_project_maintainer) { { Gitlab::Auth::AuthFinders::PRIVATE_TOKEN_HEADER => pat_project_maintainer.token } }
           let_it_be(:headers_pat_project_owner) { { Gitlab::Auth::AuthFinders::PRIVATE_TOKEN_HEADER => pat_project_owner.token } }
           let_it_be(:headers_pat_instance_admin) { { Gitlab::Auth::AuthFinders::PRIVATE_TOKEN_HEADER => pat_instance_admin.token } }
-
           let_it_be(:job_from_project_maintainer) { create(:ci_build, :running, user: pat_project_maintainer.user, project: project) }
           let_it_be(:job_from_project_owner) { create(:ci_build, :running, user: pat_project_owner.user, project: project) }
-          let(:headers_job_token_from_maintainer) { { Gitlab::Auth::AuthFinders::JOB_TOKEN_HEADER => job_from_project_maintainer.token } }
-          let(:headers_job_token_from_owner) { { Gitlab::Auth::AuthFinders::JOB_TOKEN_HEADER => job_from_project_owner.token } }
+
+          let_it_be(:headers_job_token_from_maintainer) { { Gitlab::Auth::AuthFinders::JOB_TOKEN_HEADER => job_from_project_maintainer.token } }
+          let_it_be(:headers_job_token_from_owner) { { Gitlab::Auth::AuthFinders::JOB_TOKEN_HEADER => job_from_project_owner.token } }
 
           let(:package_protection_rule) { create(:package_protection_rule, project: project) }
 
@@ -759,14 +759,6 @@
             response
           end
 
-          before do
-            package_protection_rule.update!(
-              package_name_pattern: package_name_pattern,
-              package_type: package1.package_type,
-              minimum_access_level_for_delete: minimum_access_level_for_delete
-            )
-          end
-
           shared_examples 'deleting package protected' do
             it_behaves_like 'returning response status', :forbidden
             it do
@@ -798,9 +790,9 @@
             ref(:package_name)          | :owner | ref(:headers_pat_project_owner)         | 'deleting package'
             ref(:package_name)          | :owner | ref(:headers_pat_instance_admin)        | 'deleting package'
 
+            ref(:package_name)          | :admin | ref(:headers_job_token_from_owner)      | 'deleting package protected'
             ref(:package_name)          | :admin | ref(:headers_pat_project_maintainer)    | 'deleting package protected'
             ref(:package_name)          | :admin | ref(:headers_pat_project_owner)         | 'deleting package protected'
-            ref(:package_name)          | :admin | ref(:headers_job_token_from_owner)      | 'deleting package protected'
             ref(:package_name)          | :admin | ref(:headers_pat_instance_admin)        | 'deleting package'
 
             ref(:package_name_no_match) | :owner | ref(:headers_pat_project_owner)         | 'deleting package'
@@ -808,8 +800,29 @@
           end
 
           with_them do
+            before do
+              package_protection_rule.update!(
+                package_name_pattern: package_name_pattern,
+                package_type: package1.package_type,
+                minimum_access_level_for_delete: minimum_access_level_for_delete
+              )
+            end
+
             it_behaves_like params[:shared_examples_name]
           end
+
+          context 'for package with unsupported package type for package protection rule' do
+            let_it_be(:golang_package) { create(:golang_package, project: project, name: "#{project.root_namespace.path}/golang.org/x/pkg") }
+
+            let(:headers) { headers_pat_project_maintainer }
+
+            subject do
+              delete api("/projects/#{project.id}/packages/#{golang_package.id}"), headers: headers
+              response
+            end
+
+            it_behaves_like 'deleting package'
+          end
         end
       end
 
-- 
GitLab


From e7d8fdd06fbe6d7a165dc36884d7c1331d541275 Mon Sep 17 00:00:00 2001
From: Gerardo Navarro <gerardo@b310.de>
Date: Thu, 6 Mar 2025 11:19:10 +0100
Subject: [PATCH 5/6] refactor: Appy suggestions from @mc_rocha

- https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179931#note_2381683967
- https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179931#note_2381685951
- https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179931#note_2381686986
---
 .../packages/protection/check_delete_rule_existence_service.rb  | 2 +-
 spec/requests/api/package_files_spec.rb                         | 1 -
 spec/requests/api/project_packages_spec.rb                      | 1 -
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/app/services/packages/protection/check_delete_rule_existence_service.rb b/app/services/packages/protection/check_delete_rule_existence_service.rb
index f25018cf5eb8c5..d98a130b0bae16 100644
--- a/app/services/packages/protection/check_delete_rule_existence_service.rb
+++ b/app/services/packages/protection/check_delete_rule_existence_service.rb
@@ -13,7 +13,7 @@ class CheckDeleteRuleExistenceService < BaseProjectService
       def execute
         return ERROR_RESPONSE_INVALID_PACKAGE_TYPE unless package_type_allowed?
         return ERROR_RESPONSE_UNAUTHORIZED unless current_user_can_destroy_package?
-        return service_response_for(false) if current_user.can_admin_all_resources?
+        return SUCCESS_RESPONSE_RULE_DOESNT_EXIST if current_user.can_admin_all_resources?
 
         user_project_authorization_access_level = current_user.max_member_access_for_project(project.id)
 
diff --git a/spec/requests/api/package_files_spec.rb b/spec/requests/api/package_files_spec.rb
index 4407ac2cb1e252..602061a56ae9cc 100644
--- a/spec/requests/api/package_files_spec.rb
+++ b/spec/requests/api/package_files_spec.rb
@@ -356,7 +356,6 @@
         ref(:package_name)          | :admin | ref(:headers_pat_instance_admin)        | 'deleting package'
 
         ref(:package_name_no_match) | :owner | ref(:headers_pat_project_owner)         | 'deleting package'
-        ref(:package_name_no_match) | :owner | ref(:headers_pat_project_owner)         | 'deleting package'
       end
 
       with_them do
diff --git a/spec/requests/api/project_packages_spec.rb b/spec/requests/api/project_packages_spec.rb
index 4e9cd69a84ec69..927150b9f12d2e 100644
--- a/spec/requests/api/project_packages_spec.rb
+++ b/spec/requests/api/project_packages_spec.rb
@@ -796,7 +796,6 @@
             ref(:package_name)          | :admin | ref(:headers_pat_instance_admin)        | 'deleting package'
 
             ref(:package_name_no_match) | :owner | ref(:headers_pat_project_owner)         | 'deleting package'
-            ref(:package_name_no_match) | :owner | ref(:headers_pat_project_owner)         | 'deleting package'
           end
 
           with_them do
-- 
GitLab


From ed31a9d74cc8a4406f796419a4efe161f9e7507c Mon Sep 17 00:00:00 2001
From: Gerardo Navarro <gerardo@b310.de>
Date: Mon, 10 Mar 2025 16:29:49 +0100
Subject: [PATCH 6/6] refactor: Fix undercoverage by avoiding then method

- https://gitlab.com/gitlab-community/gitlab/-/jobs/9355097830#L127
---
 .../check_delete_rule_existence_service.rb         | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/app/services/packages/protection/check_delete_rule_existence_service.rb b/app/services/packages/protection/check_delete_rule_existence_service.rb
index d98a130b0bae16..c7d162c714bc87 100644
--- a/app/services/packages/protection/check_delete_rule_existence_service.rb
+++ b/app/services/packages/protection/check_delete_rule_existence_service.rb
@@ -17,13 +17,13 @@ def execute
 
         user_project_authorization_access_level = current_user.max_member_access_for_project(project.id)
 
-        project.package_protection_rules
-               .for_delete_exists?(
-                 access_level: user_project_authorization_access_level,
-                 package_name: params[:package_name],
-                 package_type: params[:package_type]
-               )
-               .then { |result| service_response_for(result) }
+        response = project.package_protection_rules.for_delete_exists?(
+          access_level: user_project_authorization_access_level,
+          package_name: params[:package_name],
+          package_type: params[:package_type]
+        )
+
+        service_response_for(response)
       end
 
       private
-- 
GitLab