From 6e19eac043295cadd4b25e711814e1406170aa4c Mon Sep 17 00:00:00 2001 From: Nicholas Wittstruck Date: Thu, 2 May 2024 01:28:20 -0700 Subject: [PATCH 1/7] Protected packages: REST API DELETE package protection rules Adds a DELETE route to the REST API to delete package protection rules for a given rule id. Changelog: added --- doc/api/api_resources.md | 1 + doc/api/project_packages_protection_rules.md | 38 ++++++++++ lib/api/api.rb | 3 +- .../projects/packages/protection/rule.rb | 18 +++++ lib/api/project_packages_protection_rules.rb | 49 ++++++++++++ .../project_packages_protection_rules_spec.rb | 76 +++++++++++++++++++ ...ckages_protection_rules_shared_examples.rb | 13 ++++ 7 files changed, 197 insertions(+), 1 deletion(-) create mode 100644 doc/api/project_packages_protection_rules.md create mode 100644 lib/api/entities/projects/packages/protection/rule.rb create mode 100644 lib/api/project_packages_protection_rules.rb create mode 100644 spec/requests/api/project_packages_protection_rules_spec.rb create mode 100644 spec/support/shared_examples/requests/api/project_packages_protection_rules_shared_examples.rb diff --git a/doc/api/api_resources.md b/doc/api/api_resources.md index c2483ff496d72f..8b8c71d7fda535 100644 --- a/doc/api/api_resources.md +++ b/doc/api/api_resources.md @@ -93,6 +93,7 @@ The following API resources are available in the project context: | [Projects](projects.md) including setting Webhooks | `/projects`, `/projects/:id/hooks` (also available for users) | | [Protected branches](protected_branches.md) | `/projects/:id/protected_branches` | | [Protected environments](protected_environments.md) | `/projects/:id/protected_environments` | +| [Protected package rules](project_packages_protection_rules.md) | `/projects/:id/protection/rules` | | [Protected tags](protected_tags.md) | `/projects/:id/protected_tags` | | [PyPI packages](packages/pypi.md) | `/projects/:id/packages/pypi` (also available for groups) | | [Release links](releases/links.md) | `/projects/:id/releases/.../assets/links` | diff --git a/doc/api/project_packages_protection_rules.md b/doc/api/project_packages_protection_rules.md new file mode 100644 index 00000000000000..7ed678ee6dfa21 --- /dev/null +++ b/doc/api/project_packages_protection_rules.md @@ -0,0 +1,38 @@ +--- +stage: Package +group: Package Registry +info: "To determine the technical writer assigned to the Stage/Group associated with this page, see https://handbook.gitlab.com/handbook/product/ux/technical-writing/#assignments" +description: "Documentation for the REST API for Package Protection Rules in GitLab." +--- + +## Package protection rule API + +This API manages the protection rules for packages. + +FLAG: +The availability of this feature is controlled by a feature flag. +This feature is available for testing, but not ready for production use. Enable the feature flag with `Feature.enable(:packages_protected_packages)`. + +### Delete a package protection rule + +Deletes a package protection rule from a project. + +```plaintext +DELETE /api/v4/projects/:id/packages/protection/rules/:package_protection_rule_id +``` + +Supported attributes: + +| Attribute | Type | Required | Description | +|-------------------------------|-----------------|----------|--------------------------------| +| `id` | integer/string | Yes | ID or [URL-encoded path of the project](rest/index.md#namespaced-path-encoding) owned by the authenticated user. | +| `package_protection_rule_id` | integer | Yes | ID of the package protection rule to be deleted. | + +If successful, returns [`200`](rest/index.md#status-codes) and the deleted entity. + +Example request: + +```shell +curl --request DELETE --header "PRIVATE-TOKEN: " \ + --url "https://gitlab.example.com/api/v4/projects/5/packages/protection/rules/32" +``` diff --git a/lib/api/api.rb b/lib/api/api.rb index 3a5c4f4e1e5d4b..081c4ce9391a22 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -309,9 +309,10 @@ def initialize(location_url) mount ::API::ProjectImport mount ::API::ProjectJobTokenScope mount ::API::ProjectPackages + mount ::API::ProjectPackagesProtectionRules mount ::API::ProjectRepositoryStorageMoves - mount ::API::ProjectSnippets mount ::API::ProjectSnapshots + mount ::API::ProjectSnippets mount ::API::ProjectStatistics mount ::API::ProjectTemplates mount ::API::Projects diff --git a/lib/api/entities/projects/packages/protection/rule.rb b/lib/api/entities/projects/packages/protection/rule.rb new file mode 100644 index 00000000000000..4821d2ec28e9b5 --- /dev/null +++ b/lib/api/entities/projects/packages/protection/rule.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module API + module Entities + module Projects + module Packages + module Protection + class Rule < Grape::Entity + expose :project, using: Entities::ProjectIdentity + expose :package_name_pattern, documentation: { type: 'string', example: 'flightjs/flight' } + expose :package_type, documentation: { type: 'string', example: 'npm' } + expose :push_protected_up_to_access_level, documentation: { type: 'string', example: 'maintainer' } + end + end + end + end + end +end diff --git a/lib/api/project_packages_protection_rules.rb b/lib/api/project_packages_protection_rules.rb new file mode 100644 index 00000000000000..30ade1a88589a1 --- /dev/null +++ b/lib/api/project_packages_protection_rules.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +module API + class ProjectPackagesProtectionRules < ::API::Base + feature_category :package_registry + PACKAGE_RPOTECTION_RULE_ENDPOINT_REQUIREMENTS = API::NAMESPACE_OR_PROJECT_REQUIREMENTS.merge( + tag_name: API::NO_SLASH_URL_PART_REGEX) + + before do + authenticate! + end + + params do + requires :id, types: [String, Integer], desc: 'The ID or URL-encoded path of the project' + end + resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do + desc 'Delete package protection rule' do + success Entities::Projects::Packages::Protection::Rule + failure [ + { code: 401, message: 'Unauthorized' }, + { code: 404, message: 'Not Found' } + ] + tags %w[projects] + end + + params do + requires :package_protection_rule_id, type: Integer, desc: 'The ID of the package protection rule' + end + delete ":id/packages/protection/rules/:package_protection_rule_id", + requirements: PACKAGE_RPOTECTION_RULE_ENDPOINT_REQUIREMENTS do + authorize_admin_project + + package_protection_rule = Packages::Protection::Rule.find(params[:package_protection_rule_id]) + + # make sure the rule is present and belongs to the project: + not_found! unless package_protection_rule.present? && package_protection_rule.project_id.to_s == params[:id] + + result = ::Packages::Protection::DeleteRuleService.new(package_protection_rule, + current_user: current_user).execute + + if result[:status] == :success + present package_protection_rule, with: Entities::Projects::Packages::Protection::Rule + else + render_api_error!(result[:message], result[:http_status]) + end + end + end + end +end diff --git a/spec/requests/api/project_packages_protection_rules_spec.rb b/spec/requests/api/project_packages_protection_rules_spec.rb new file mode 100644 index 00000000000000..a4dc9af9ade698 --- /dev/null +++ b/spec/requests/api/project_packages_protection_rules_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe API::ProjectPackagesProtectionRules, feature_category: :package_registry do + include ExclusiveLeaseHelpers + + let_it_be(:project) { create(:project, :private) } + let_it_be(:other_project) { create(:project, :private) } + let_it_be(:package_protection_rule) { create(:package_protection_rule, project: project) } + + let_it_be(:maintainer) { create(:user, maintainer_of: [project, other_project]) } + let_it_be(:developer) { create(:user, developer_of: [project]) } + let_it_be(:reporter) { create(:user, reporter_of: [project]) } + let_it_be(:guest) { create(:user, guest_of: [project]) } + + let(:users) do + { + anonymous: nil, + developer: developer, + guest: guest, + maintainer: maintainer, + reporter: reporter + } + end + + let(:api_user) { maintainer } + let(:params) { {} } + + shared_context 'with API user' do + subject(:api_project_packages_protection_rules_subject) { public_send(method, api(url, api_user), params: params) } + end + + describe 'DELETE /projects/:id/packages/protection/rules/:package_protection_rule_id' do + let(:method) { :delete } + let(:url) { "/projects/#{project.id}/packages/protection/rules/#{package_protection_rule.id}" } + + include_context 'with API user' + + it_behaves_like 'rejected project packages protection rules access', :reporter, :forbidden + it_behaves_like 'rejected project packages protection rules access', :developer, :forbidden + it_behaves_like 'rejected project packages protection rules access', :guest, :forbidden + it_behaves_like 'rejected project packages protection rules access', :anonymous, :unauthorized + + context 'for maintainer' do + let(:api_user) { maintainer } + + it 'deletes the package protection rule' do + api_project_packages_protection_rules_subject + expect do + Packages::Protection::Rule.find(package_protection_rule.id) + end.to raise_error(ActiveRecord::RecordNotFound) + expect(response).to have_gitlab_http_status(:ok) + end + + it 'returns 404 for a non-existing rule' do + package_protection_rule.destroy! + api_project_packages_protection_rules_subject + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'for maintainer with a different project' do + let(:api_user) { maintainer } + let(:url) { "/projects/#{other_project.id}/packages/protection/rules/#{package_protection_rule.id}" } + + it 'does not delete rule if it does not belong to project' do + api_project_packages_protection_rules_subject + + # rule still exists and was not deleted: + expect(Packages::Protection::Rule.find(package_protection_rule.id)).to be_present + expect(response).to have_gitlab_http_status(:not_found) + end + end + end +end diff --git a/spec/support/shared_examples/requests/api/project_packages_protection_rules_shared_examples.rb b/spec/support/shared_examples/requests/api/project_packages_protection_rules_shared_examples.rb new file mode 100644 index 00000000000000..d3a81c57c6c432 --- /dev/null +++ b/spec/support/shared_examples/requests/api/project_packages_protection_rules_shared_examples.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'rejected project packages protection rules access' do |user_type, status| + context "for #{user_type}" do + let(:api_user) { users[user_type] } + + it "returns #{status}" do + subject + + expect(response).to have_gitlab_http_status(status) + end + end +end -- GitLab From 864dafa32cb92e4aaf8f954ebcfffc41316c1066 Mon Sep 17 00:00:00 2001 From: Nicholas Wittstruck Date: Sun, 12 May 2024 07:49:18 -0700 Subject: [PATCH 2/7] Protected packages: REST API DELETE package protection rules Adds a DELETE route to the REST API to delete package protection rules for a given rule id. - Add DETAILS block to documentation Changelog: added --- doc/api/project_packages_protection_rules.md | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/doc/api/project_packages_protection_rules.md b/doc/api/project_packages_protection_rules.md index 7ed678ee6dfa21..d9e9055e3783ac 100644 --- a/doc/api/project_packages_protection_rules.md +++ b/doc/api/project_packages_protection_rules.md @@ -5,13 +5,21 @@ info: "To determine the technical writer assigned to the Stage/Group associated description: "Documentation for the REST API for Package Protection Rules in GitLab." --- -## Package protection rule API +## Package protection rules API -This API manages the protection rules for packages. +DETAILS: +**Tier:** Free, Premium, Ultimate +**Offering:** GitLab.com, Self-managed +**Status:** Experiment + +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/151741) in GitLab 16.5 [with a flag](../administration/feature_flags.md) named `packages_protected_packages`. Disabled by default. FLAG: The availability of this feature is controlled by a feature flag. -This feature is available for testing, but not ready for production use. Enable the feature flag with `Feature.enable(:packages_protected_packages)`. +For more information, see the history. +This feature is available for testing, but not ready for production use. + +This API manages the protection rules for packages. This feature is an experiment. ### Delete a package protection rule -- GitLab From e02c2da3d5c6e618e1dd3e2ede938cbc7f920037 Mon Sep 17 00:00:00 2001 From: Nicholas Wittstruck Date: Sun, 12 May 2024 07:58:14 -0700 Subject: [PATCH 3/7] Protected packages: REST API DELETE package protection rules Adds a DELETE route to the REST API to delete package protection rules for a given rule id. - Uses euser_project.package_protection_rules.find so that we don't need to check if the package protection rule belongs to the project. Changelog: added --- lib/api/project_packages_protection_rules.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/api/project_packages_protection_rules.rb b/lib/api/project_packages_protection_rules.rb index 30ade1a88589a1..2088bb33e07906 100644 --- a/lib/api/project_packages_protection_rules.rb +++ b/lib/api/project_packages_protection_rules.rb @@ -30,10 +30,7 @@ class ProjectPackagesProtectionRules < ::API::Base requirements: PACKAGE_RPOTECTION_RULE_ENDPOINT_REQUIREMENTS do authorize_admin_project - package_protection_rule = Packages::Protection::Rule.find(params[:package_protection_rule_id]) - - # make sure the rule is present and belongs to the project: - not_found! unless package_protection_rule.present? && package_protection_rule.project_id.to_s == params[:id] + package_protection_rule = user_project.package_protection_rules.find(params[:package_protection_rule_id]) result = ::Packages::Protection::DeleteRuleService.new(package_protection_rule, current_user: current_user).execute -- GitLab From 809c944cafb6f2c9145b127317ea0853e624936b Mon Sep 17 00:00:00 2001 From: Nicholas Wittstruck Date: Fri, 17 May 2024 01:59:32 -0700 Subject: [PATCH 4/7] Protected packages: REST API DELETE package protection rules Adds a DELETE route to the REST API to delete package protection rules for a given rule id. Based on review feedback this commit changes the authorization to admin_package, instead of admin_project. It also adds the missing feature flag check. Changelog: added --- doc/api/project_packages_protection_rules.md | 2 +- lib/api/helpers.rb | 4 ++ lib/api/project_packages_protection_rules.rb | 6 ++- .../project_packages_protection_rules_spec.rb | 43 +++++++++++-------- ...ckages_protection_rules_shared_examples.rb | 13 ------ 5 files changed, 35 insertions(+), 33 deletions(-) delete mode 100644 spec/support/shared_examples/requests/api/project_packages_protection_rules_shared_examples.rb diff --git a/doc/api/project_packages_protection_rules.md b/doc/api/project_packages_protection_rules.md index d9e9055e3783ac..69984d42eef751 100644 --- a/doc/api/project_packages_protection_rules.md +++ b/doc/api/project_packages_protection_rules.md @@ -12,7 +12,7 @@ DETAILS: **Offering:** GitLab.com, Self-managed **Status:** Experiment -> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/151741) in GitLab 16.5 [with a flag](../administration/feature_flags.md) named `packages_protected_packages`. Disabled by default. +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/151741) in GitLab 17.1 [with a flag](../administration/feature_flags.md) named `packages_protected_packages`. Disabled by default. FLAG: The availability of this feature is controlled by a feature flag. diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 771a101cdc6707..560c9abd443c9b 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -380,6 +380,10 @@ def authorize_admin_project authorize! :admin_project, user_project end + def authorize_admin_package + authorize! :admin_package, user_project + end + def authorize_admin_group authorize! :admin_group, user_group end diff --git a/lib/api/project_packages_protection_rules.rb b/lib/api/project_packages_protection_rules.rb index 2088bb33e07906..a5a0ccc39335e9 100644 --- a/lib/api/project_packages_protection_rules.rb +++ b/lib/api/project_packages_protection_rules.rb @@ -8,6 +8,10 @@ class ProjectPackagesProtectionRules < ::API::Base before do authenticate! + + if Feature.disabled?(:packages_protected_packages, user_project) + render_api_error!("'packages_protected_packages' feature flag is disabled", :forbidden) + end end params do @@ -28,7 +32,7 @@ class ProjectPackagesProtectionRules < ::API::Base end delete ":id/packages/protection/rules/:package_protection_rule_id", requirements: PACKAGE_RPOTECTION_RULE_ENDPOINT_REQUIREMENTS do - authorize_admin_project + authorize_admin_package package_protection_rule = user_project.package_protection_rules.find(params[:package_protection_rule_id]) diff --git a/spec/requests/api/project_packages_protection_rules_spec.rb b/spec/requests/api/project_packages_protection_rules_spec.rb index a4dc9af9ade698..57eee83779d0e2 100644 --- a/spec/requests/api/project_packages_protection_rules_spec.rb +++ b/spec/requests/api/project_packages_protection_rules_spec.rb @@ -2,6 +2,18 @@ require 'spec_helper' +RSpec.shared_examples 'rejected project packages protection rules access' do |method, user_type, status| + context "for #{user_type}" do + let(:api_user) { users[user_type] } + + it "returns #{status}" do + public_send(method, api(url, api_user), params: params) + + expect(response).to have_gitlab_http_status(status) + end + end +end + RSpec.describe API::ProjectPackagesProtectionRules, feature_category: :package_registry do include ExclusiveLeaseHelpers @@ -27,26 +39,19 @@ let(:api_user) { maintainer } let(:params) { {} } - shared_context 'with API user' do - subject(:api_project_packages_protection_rules_subject) { public_send(method, api(url, api_user), params: params) } - end - describe 'DELETE /projects/:id/packages/protection/rules/:package_protection_rule_id' do - let(:method) { :delete } let(:url) { "/projects/#{project.id}/packages/protection/rules/#{package_protection_rule.id}" } - include_context 'with API user' - - it_behaves_like 'rejected project packages protection rules access', :reporter, :forbidden - it_behaves_like 'rejected project packages protection rules access', :developer, :forbidden - it_behaves_like 'rejected project packages protection rules access', :guest, :forbidden - it_behaves_like 'rejected project packages protection rules access', :anonymous, :unauthorized + it_behaves_like 'rejected project packages protection rules access', :delete, :reporter, :forbidden + it_behaves_like 'rejected project packages protection rules access', :delete, :developer, :forbidden + it_behaves_like 'rejected project packages protection rules access', :delete, :guest, :forbidden + it_behaves_like 'rejected project packages protection rules access', :delete, :anonymous, :unauthorized context 'for maintainer' do let(:api_user) { maintainer } it 'deletes the package protection rule' do - api_project_packages_protection_rules_subject + delete(api(url, api_user)) expect do Packages::Protection::Rule.find(package_protection_rule.id) end.to raise_error(ActiveRecord::RecordNotFound) @@ -55,17 +60,19 @@ it 'returns 404 for a non-existing rule' do package_protection_rule.destroy! - api_project_packages_protection_rules_subject + delete(api(url, api_user)) expect(response).to have_gitlab_http_status(:not_found) end - end - context 'for maintainer with a different project' do - let(:api_user) { maintainer } - let(:url) { "/projects/#{other_project.id}/packages/protection/rules/#{package_protection_rule.id}" } + it 'returns feature disabled when the feature is disabled' do + stub_feature_flags(packages_protected_packages: false) + delete(api(url, api_user)) + expect(response).to have_gitlab_http_status(:forbidden) + end it 'does not delete rule if it does not belong to project' do - api_project_packages_protection_rules_subject + other_project_url = "/projects/#{other_project.id}/packages/protection/rules/#{package_protection_rule.id}" + delete(api(other_project_url, api_user)) # rule still exists and was not deleted: expect(Packages::Protection::Rule.find(package_protection_rule.id)).to be_present diff --git a/spec/support/shared_examples/requests/api/project_packages_protection_rules_shared_examples.rb b/spec/support/shared_examples/requests/api/project_packages_protection_rules_shared_examples.rb deleted file mode 100644 index d3a81c57c6c432..00000000000000 --- a/spec/support/shared_examples/requests/api/project_packages_protection_rules_shared_examples.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -RSpec.shared_examples 'rejected project packages protection rules access' do |user_type, status| - context "for #{user_type}" do - let(:api_user) { users[user_type] } - - it "returns #{status}" do - subject - - expect(response).to have_gitlab_http_status(status) - end - end -end -- GitLab From df757b3678e82d5c6556b138f9cb4ac7014b8570 Mon Sep 17 00:00:00 2001 From: Nicholas Wittstruck Date: Wed, 22 May 2024 06:02:14 -0700 Subject: [PATCH 5/7] Protected packages: REST API DELETE package protection rules Adds a DELETE route to the REST API to delete package protection rules for a given rule id. Based on review feedback this commit changes the handling of the response from the DeleteService. We no longer return a 500 on expected errors, but a 400. Changelog: added --- lib/api/project_packages_protection_rules.rb | 22 ++++------ .../project_packages_protection_rules_spec.rb | 41 +++++++++++++++---- 2 files changed, 41 insertions(+), 22 deletions(-) diff --git a/lib/api/project_packages_protection_rules.rb b/lib/api/project_packages_protection_rules.rb index a5a0ccc39335e9..331d2496ee6a4e 100644 --- a/lib/api/project_packages_protection_rules.rb +++ b/lib/api/project_packages_protection_rules.rb @@ -3,15 +3,13 @@ module API class ProjectPackagesProtectionRules < ::API::Base feature_category :package_registry - PACKAGE_RPOTECTION_RULE_ENDPOINT_REQUIREMENTS = API::NAMESPACE_OR_PROJECT_REQUIREMENTS.merge( - tag_name: API::NO_SLASH_URL_PART_REGEX) before do - authenticate! - if Feature.disabled?(:packages_protected_packages, user_project) - render_api_error!("'packages_protected_packages' feature flag is disabled", :forbidden) + render_api_error!("'packages_protected_packages' feature flag is disabled", :not_found) end + + authenticate! end params do @@ -26,23 +24,19 @@ class ProjectPackagesProtectionRules < ::API::Base ] tags %w[projects] end - params do requires :package_protection_rule_id, type: Integer, desc: 'The ID of the package protection rule' end - delete ":id/packages/protection/rules/:package_protection_rule_id", - requirements: PACKAGE_RPOTECTION_RULE_ENDPOINT_REQUIREMENTS do + delete ":id/packages/protection/rules/:package_protection_rule_id" do authorize_admin_package package_protection_rule = user_project.package_protection_rules.find(params[:package_protection_rule_id]) - result = ::Packages::Protection::DeleteRuleService.new(package_protection_rule, - current_user: current_user).execute + destroy_conditionally!(package_protection_rule) do |package_protection_rule| + response = ::Packages::Protection::DeleteRuleService.new(package_protection_rule, + current_user: current_user).execute - if result[:status] == :success - present package_protection_rule, with: Entities::Projects::Packages::Protection::Rule - else - render_api_error!(result[:message], result[:http_status]) + render_api_error!({ error: response.message }, :bad_request) if response.error? end end end diff --git a/spec/requests/api/project_packages_protection_rules_spec.rb b/spec/requests/api/project_packages_protection_rules_spec.rb index 57eee83779d0e2..0ac29e7858646d 100644 --- a/spec/requests/api/project_packages_protection_rules_spec.rb +++ b/spec/requests/api/project_packages_protection_rules_spec.rb @@ -45,7 +45,7 @@ it_behaves_like 'rejected project packages protection rules access', :delete, :reporter, :forbidden it_behaves_like 'rejected project packages protection rules access', :delete, :developer, :forbidden it_behaves_like 'rejected project packages protection rules access', :delete, :guest, :forbidden - it_behaves_like 'rejected project packages protection rules access', :delete, :anonymous, :unauthorized + it_behaves_like 'rejected project packages protection rules access', :delete, :anonymous, :not_found context 'for maintainer' do let(:api_user) { maintainer } @@ -55,7 +55,7 @@ expect do Packages::Protection::Rule.find(package_protection_rule.id) end.to raise_error(ActiveRecord::RecordNotFound) - expect(response).to have_gitlab_http_status(:ok) + expect(response).to have_gitlab_http_status(:no_content) end it 'returns 404 for a non-existing rule' do @@ -64,12 +64,6 @@ expect(response).to have_gitlab_http_status(:not_found) end - it 'returns feature disabled when the feature is disabled' do - stub_feature_flags(packages_protected_packages: false) - delete(api(url, api_user)) - expect(response).to have_gitlab_http_status(:forbidden) - end - it 'does not delete rule if it does not belong to project' do other_project_url = "/projects/#{other_project.id}/packages/protection/rules/#{package_protection_rule.id}" delete(api(other_project_url, api_user)) @@ -78,6 +72,37 @@ expect(Packages::Protection::Rule.find(package_protection_rule.id)).to be_present expect(response).to have_gitlab_http_status(:not_found) end + + context 'when the project id is invalid' do + let(:url_with_invalid_project_id) do + "/projects/invalid/packages/protection/rules/#{package_protection_rule.id}" + end + + it 'returns not_found' do + delete(api(url_with_invalid_project_id, api_user)) + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when the rule id is invalid' do + let(:url_with_invalid_rule_id) { "/projects/#{project.id}/packages/protection/rules/invalid" } + + it 'returns bad request' do + delete(api(url_with_invalid_rule_id, api_user)) + expect(response).to have_gitlab_http_status(:bad_request) + end + end + end + + context 'when packages_protected_packages is disabled' do + before do + stub_feature_flags(packages_protected_packages: false) + end + + it 'returns feature disabled when the feature is disabled' do + delete(api(url, api_user)) + expect(response).to have_gitlab_http_status(:not_found) + end end end end -- GitLab From 170aa6dc652604473d7c638a0ddf05dacf3ae57e Mon Sep 17 00:00:00 2001 From: Nicholas Wittstruck Date: Wed, 22 May 2024 23:54:31 -0700 Subject: [PATCH 6/7] Protected packages: REST API DELETE package protection rules Adds a DELETE route to the REST API to delete package protection rules for a given rule id. Based on review feedback this adjusts the docs to reflect that no longer an entity is returned. It also removes the Grape Entity. We'll add it for the GET request. Changelog: added --- doc/api/project_packages_protection_rules.md | 2 +- .../projects/packages/protection/rule.rb | 18 ------------------ lib/api/project_packages_protection_rules.rb | 5 +++-- 3 files changed, 4 insertions(+), 21 deletions(-) delete mode 100644 lib/api/entities/projects/packages/protection/rule.rb diff --git a/doc/api/project_packages_protection_rules.md b/doc/api/project_packages_protection_rules.md index 69984d42eef751..8904c742c6e0a9 100644 --- a/doc/api/project_packages_protection_rules.md +++ b/doc/api/project_packages_protection_rules.md @@ -36,7 +36,7 @@ Supported attributes: | `id` | integer/string | Yes | ID or [URL-encoded path of the project](rest/index.md#namespaced-path-encoding) owned by the authenticated user. | | `package_protection_rule_id` | integer | Yes | ID of the package protection rule to be deleted. | -If successful, returns [`200`](rest/index.md#status-codes) and the deleted entity. +If successful, returns [`204 No Content`](rest/index.md#status-codes) Example request: diff --git a/lib/api/entities/projects/packages/protection/rule.rb b/lib/api/entities/projects/packages/protection/rule.rb deleted file mode 100644 index 4821d2ec28e9b5..00000000000000 --- a/lib/api/entities/projects/packages/protection/rule.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -module API - module Entities - module Projects - module Packages - module Protection - class Rule < Grape::Entity - expose :project, using: Entities::ProjectIdentity - expose :package_name_pattern, documentation: { type: 'string', example: 'flightjs/flight' } - expose :package_type, documentation: { type: 'string', example: 'npm' } - expose :push_protected_up_to_access_level, documentation: { type: 'string', example: 'maintainer' } - end - end - end - end - end -end diff --git a/lib/api/project_packages_protection_rules.rb b/lib/api/project_packages_protection_rules.rb index 331d2496ee6a4e..377133ff027966 100644 --- a/lib/api/project_packages_protection_rules.rb +++ b/lib/api/project_packages_protection_rules.rb @@ -17,9 +17,10 @@ class ProjectPackagesProtectionRules < ::API::Base end resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do desc 'Delete package protection rule' do - success Entities::Projects::Packages::Protection::Rule + success code: 204, message: '204 No Content' failure [ { code: 401, message: 'Unauthorized' }, + { code: 403, message: 'Forbidden' }, { code: 404, message: 'Not Found' } ] tags %w[projects] @@ -27,7 +28,7 @@ class ProjectPackagesProtectionRules < ::API::Base params do requires :package_protection_rule_id, type: Integer, desc: 'The ID of the package protection rule' end - delete ":id/packages/protection/rules/:package_protection_rule_id" do + delete ':id/packages/protection/rules/:package_protection_rule_id' do authorize_admin_package package_protection_rule = user_project.package_protection_rules.find(params[:package_protection_rule_id]) -- GitLab From f6b1f1ef954f29768a56a001a95f10696c9ad8ac Mon Sep 17 00:00:00 2001 From: Nicholas Wittstruck Date: Fri, 24 May 2024 07:38:30 -0700 Subject: [PATCH 7/7] Protected packages: REST API DELETE package protection rules Adds a DELETE route to the REST API to delete package protection rules for a given rule id. Based on review feedback this commit changes the usage of shared examples in the spec. Changelog: added --- doc/api/project_packages_protection_rules.md | 9 +- lib/api/helpers.rb | 4 - lib/api/helpers/packages_helpers.rb | 4 + lib/api/project_packages_protection_rules.rb | 5 +- spec/lib/api/helpers/packages_helpers_spec.rb | 2 +- .../project_packages_protection_rules_spec.rb | 89 ++++++++----------- 6 files changed, 54 insertions(+), 59 deletions(-) diff --git a/doc/api/project_packages_protection_rules.md b/doc/api/project_packages_protection_rules.md index 8904c742c6e0a9..8972606c558709 100644 --- a/doc/api/project_packages_protection_rules.md +++ b/doc/api/project_packages_protection_rules.md @@ -36,7 +36,14 @@ Supported attributes: | `id` | integer/string | Yes | ID or [URL-encoded path of the project](rest/index.md#namespaced-path-encoding) owned by the authenticated user. | | `package_protection_rule_id` | integer | Yes | ID of the package protection rule to be deleted. | -If successful, returns [`204 No Content`](rest/index.md#status-codes) +If successful, returns [`204 No Content`](rest/index.md#status-codes). + +Can return the following status codes: + +- `204 No Content`: The package protection rule was deleted successfully. +- `400 Bad Request`: The `id` or the `package_protection_rule_id` are missing or are invalid. +- `403 Forbidden`: The user does not have permission to delete the package protection rule. +- `404 Not Found`: The project or the package protection rule was not found. Example request: diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 560c9abd443c9b..771a101cdc6707 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -380,10 +380,6 @@ def authorize_admin_project authorize! :admin_project, user_project end - def authorize_admin_package - authorize! :admin_package, user_project - end - def authorize_admin_group authorize! :admin_group, user_group end diff --git a/lib/api/helpers/packages_helpers.rb b/lib/api/helpers/packages_helpers.rb index c2b5f31dc77c56..1d823ca9352f83 100644 --- a/lib/api/helpers/packages_helpers.rb +++ b/lib/api/helpers/packages_helpers.rb @@ -17,6 +17,10 @@ def require_dependency_proxy_enabled! not_found! unless ::Gitlab.config.dependency_proxy.enabled end + def authorize_admin_package!(subject = user_project) + authorize!(:admin_package, subject) + end + def authorize_read_package!(subject = user_project) authorize!(:read_package, subject.try(:packages_policy_subject) || subject) end diff --git a/lib/api/project_packages_protection_rules.rb b/lib/api/project_packages_protection_rules.rb index 377133ff027966..3b684aaa1ec73a 100644 --- a/lib/api/project_packages_protection_rules.rb +++ b/lib/api/project_packages_protection_rules.rb @@ -3,6 +3,7 @@ module API class ProjectPackagesProtectionRules < ::API::Base feature_category :package_registry + helpers ::API::Helpers::PackagesHelpers before do if Feature.disabled?(:packages_protected_packages, user_project) @@ -19,7 +20,7 @@ class ProjectPackagesProtectionRules < ::API::Base desc 'Delete package protection rule' do success code: 204, message: '204 No Content' failure [ - { code: 401, message: 'Unauthorized' }, + { code: 400, message: 'Bad Request' }, { code: 403, message: 'Forbidden' }, { code: 404, message: 'Not Found' } ] @@ -29,7 +30,7 @@ class ProjectPackagesProtectionRules < ::API::Base requires :package_protection_rule_id, type: Integer, desc: 'The ID of the package protection rule' end delete ':id/packages/protection/rules/:package_protection_rule_id' do - authorize_admin_package + authorize_admin_package! package_protection_rule = user_project.package_protection_rules.find(params[:package_protection_rule_id]) diff --git a/spec/lib/api/helpers/packages_helpers_spec.rb b/spec/lib/api/helpers/packages_helpers_spec.rb index 51819b4910e839..f44581dcd853dc 100644 --- a/spec/lib/api/helpers/packages_helpers_spec.rb +++ b/spec/lib/api/helpers/packages_helpers_spec.rb @@ -62,7 +62,7 @@ end end - %i[create_package destroy_package].each do |action| + %i[create_package destroy_package admin_package].each do |action| describe "authorize_#{action}!" do subject { helper.send("authorize_#{action}!", project) } diff --git a/spec/requests/api/project_packages_protection_rules_spec.rb b/spec/requests/api/project_packages_protection_rules_spec.rb index 0ac29e7858646d..304ce170c51475 100644 --- a/spec/requests/api/project_packages_protection_rules_spec.rb +++ b/spec/requests/api/project_packages_protection_rules_spec.rb @@ -2,18 +2,6 @@ require 'spec_helper' -RSpec.shared_examples 'rejected project packages protection rules access' do |method, user_type, status| - context "for #{user_type}" do - let(:api_user) { users[user_type] } - - it "returns #{status}" do - public_send(method, api(url, api_user), params: params) - - expect(response).to have_gitlab_http_status(status) - end - end -end - RSpec.describe API::ProjectPackagesProtectionRules, feature_category: :package_registry do include ExclusiveLeaseHelpers @@ -36,62 +24,64 @@ } end - let(:api_user) { maintainer } - let(:params) { {} } + shared_examples 'rejecting project packages protection rules request' do |user_type, status| + context "for #{user_type}" do + let(:api_user) { users[user_type] } + + it_behaves_like 'returning response status', status + end + end describe 'DELETE /projects/:id/packages/protection/rules/:package_protection_rule_id' do let(:url) { "/projects/#{project.id}/packages/protection/rules/#{package_protection_rule.id}" } - it_behaves_like 'rejected project packages protection rules access', :delete, :reporter, :forbidden - it_behaves_like 'rejected project packages protection rules access', :delete, :developer, :forbidden - it_behaves_like 'rejected project packages protection rules access', :delete, :guest, :forbidden - it_behaves_like 'rejected project packages protection rules access', :delete, :anonymous, :not_found + subject(:destroy_package_rule) { delete(api(url, api_user)) } + + it_behaves_like 'rejecting project packages protection rules request', :reporter, :forbidden + it_behaves_like 'rejecting project packages protection rules request', :developer, :forbidden + it_behaves_like 'rejecting project packages protection rules request', :guest, :forbidden + it_behaves_like 'rejecting project packages protection rules request', :anonymous, :not_found context 'for maintainer' do let(:api_user) { maintainer } it 'deletes the package protection rule' do - delete(api(url, api_user)) + destroy_package_rule expect do Packages::Protection::Rule.find(package_protection_rule.id) end.to raise_error(ActiveRecord::RecordNotFound) expect(response).to have_gitlab_http_status(:no_content) end + end - it 'returns 404 for a non-existing rule' do - package_protection_rule.destroy! - delete(api(url, api_user)) - expect(response).to have_gitlab_http_status(:not_found) - end + context 'when the package protection rule does belong to another project' do + let(:url) { "/projects/#{other_project.id}/packages/protection/rules/#{package_protection_rule.id}" } - it 'does not delete rule if it does not belong to project' do - other_project_url = "/projects/#{other_project.id}/packages/protection/rules/#{package_protection_rule.id}" - delete(api(other_project_url, api_user)) + it_behaves_like 'rejecting project packages protection rules request', :maintainer, :not_found + end - # rule still exists and was not deleted: - expect(Packages::Protection::Rule.find(package_protection_rule.id)).to be_present - expect(response).to have_gitlab_http_status(:not_found) - end + context 'when the project id is invalid' do + let(:url) { "/projects/invalid/packages/protection/rules/#{package_protection_rule.id}" } - context 'when the project id is invalid' do - let(:url_with_invalid_project_id) do - "/projects/invalid/packages/protection/rules/#{package_protection_rule.id}" - end + it_behaves_like 'rejecting project packages protection rules request', :maintainer, :not_found + end - it 'returns not_found' do - delete(api(url_with_invalid_project_id, api_user)) - expect(response).to have_gitlab_http_status(:not_found) - end - end + context 'when the project id does not exist' do + let(:url) { "/projects/#{non_existing_record_id}/packages/protection/rules/#{package_protection_rule.id}" } - context 'when the rule id is invalid' do - let(:url_with_invalid_rule_id) { "/projects/#{project.id}/packages/protection/rules/invalid" } + it_behaves_like 'rejecting project packages protection rules request', :maintainer, :not_found + end - it 'returns bad request' do - delete(api(url_with_invalid_rule_id, api_user)) - expect(response).to have_gitlab_http_status(:bad_request) - end - end + context 'when the rule id is invalid' do + let(:url) { "/projects/#{project.id}/packages/protection/rules/invalid" } + + it_behaves_like 'rejecting project packages protection rules request', :maintainer, :bad_request + end + + context 'when the rule id does not exist' do + let(:url) { "/projects/#{project.id}/packages/protection/rules/#{non_existing_record_id}" } + + it_behaves_like 'rejecting project packages protection rules request', :maintainer, :not_found end context 'when packages_protected_packages is disabled' do @@ -99,10 +89,7 @@ stub_feature_flags(packages_protected_packages: false) end - it 'returns feature disabled when the feature is disabled' do - delete(api(url, api_user)) - expect(response).to have_gitlab_http_status(:not_found) - end + it_behaves_like 'rejecting project packages protection rules request', :maintainer, :not_found end end end -- GitLab