diff --git a/app/models/packages/protection/rule.rb b/app/models/packages/protection/rule.rb index 0913bc875ec414dba158c195b5896791460078fb..fe05c69470a764bf1b39d1d2b912972f87efe8fd 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 0000000000000000000000000000000000000000..c7d162c714bc8724dd86aa0a8c832af5b352e762 --- /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 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) + + 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 + + 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 8a705e3bda0c3631f3a789ffba71c5c724c21008..d97ae99838429a3fb7758ed9bdef85bb98368c4d 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 0000000000000000000000000000000000000000..7911ee3ca2e022f3c74252a8357b4f2df7791d37 --- /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 dcbca62da2f792b8524229d1e77501d74b6224e6..3186f0f1df23e34963cedcd19d1cb24d27a1b4ea 100644 --- a/doc/api/packages.md +++ b/doc/api/packages.md @@ -433,12 +433,15 @@ 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. -- `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 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 {{< 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 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. diff --git a/lib/api/package_files.rb b/lib/api/package_files.rb index 9f9ad173d32398d98da79cb409366dc299234095..65eeb50c604761fd242cb835cce20c2fe83464de 100644 --- a/lib/api/package_files.rb +++ b/lib/api/package_files.rb @@ -67,6 +67,17 @@ class PackageFiles < ::API::Base not_found! unless package + if Feature.enabled?(:packages_protected_packages_delete, user_project) + service_response = + Packages::Protection::CheckDeleteRuleExistenceService.new( + project: user_project, + current_user: current_user, + params: { package_name: package.name, package_type: package.package_type } + ).execute + + 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 99d9658d9d51591036cd62bfd9d6386fcd526e29..cc845deb595e605d5cbaa2bb32c06a8944c23111 100644 --- a/lib/api/project_packages.rb +++ b/lib/api/project_packages.rb @@ -139,6 +139,17 @@ def package delete ':id/packages/:package_id' do authorize_destroy_package!(user_project) + if Feature.enabled?(:packages_protected_packages_delete, user_project) + service_response = + Packages::Protection::CheckDeleteRuleExistenceService.new( + project: user_project, + current_user: current_user, + params: { package_name: package.name, package_type: package.package_type } + ).execute + + 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 301e2cd81bf0fbd8db392d84fa9ea4a92a18251b..602061a56ae9cc89f8ca0744876b069df7412158 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,112 @@ 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 + + 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' + 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 d26bbdb7b06004bd159012cdaccb559b14e71ca9..927150b9f12d2ec73fbbcdbca2829b24fb3c2147 100644 --- a/spec/requests/api/project_packages_spec.rb +++ b/spec/requests/api/project_packages_spec.rb @@ -735,6 +735,94 @@ 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_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) } + + let(:package_name) { package1.name } + let(:package_name_no_match) { "#{package_name}_no_match" } + + subject do + delete api(package_url), headers: headers + response + 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_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_pat_instance_admin) | 'deleting package' + + ref(:package_name_no_match) | :owner | ref(:headers_pat_project_owner) | 'deleting package' + 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 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 0000000000000000000000000000000000000000..bd4eb1fe1a5fb94b2a81368fe8a59074f2d0d173 --- /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