From 048f04c8246c04e49c69653a6628d1e18f7ffa3c Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Tue, 4 Feb 2025 18:11:07 +0100 Subject: [PATCH 1/6] Protected packages: Add minimum_access_level_for_delete to GRAPHQL API The attribute `minimum_access_level_for_delete` that was introduced in the MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179739 . This commit includes the attribute `minimum_access_level_for_delete` in the GRAPHQL API and also updates the documentation. Changelog: added --- .../packages/protection/rule/create.rb | 14 ++++++-- .../packages/protection/rule/update.rb | 7 +++- .../rule_access_level_for_delete_enum.rb | 19 +++++++++++ .../types/packages/protection/rule_type.rb | 12 ++++++- .../protection/create_rule_service.rb | 1 + .../protection/delete_rule_service.rb | 3 +- .../protection/update_rule_service.rb | 1 + doc/api/graphql/reference/_index.md | 16 +++++++-- .../rule_access_level_for_delete_enum_spec.rb | 9 +++++ .../packages/protection/rule_type_spec.rb | 8 ++++- .../packages/protection/rule/create_spec.rb | 33 +++++++++++++++++++ .../packages/protection/rule/delete_spec.rb | 1 + .../packages/protection/rule/update_spec.rb | 23 ++++++++++--- .../project/packages_protection_rules_spec.rb | 1 + .../protection/create_rule_service_spec.rb | 7 ++++ .../protection/update_rule_service_spec.rb | 11 +++++-- 16 files changed, 149 insertions(+), 17 deletions(-) create mode 100644 app/graphql/types/packages/protection/rule_access_level_for_delete_enum.rb create mode 100644 spec/graphql/types/packages/protection/rule_access_level_for_delete_enum_spec.rb diff --git a/app/graphql/mutations/packages/protection/rule/create.rb b/app/graphql/mutations/packages/protection/rule/create.rb index 2e5fe790b4826b..554af34678cf1e 100644 --- a/app/graphql/mutations/packages/protection/rule/create.rb +++ b/app/graphql/mutations/packages/protection/rule/create.rb @@ -27,9 +27,15 @@ class Create < ::Mutations::BaseMutation required: true, description: copy_field_description(Types::Packages::Protection::RuleType, :package_type) + argument :minimum_access_level_for_delete, + Types::Packages::Protection::RuleAccessLevelForDeleteEnum, + required: false, + experiment: { milestone: '17.10' }, + description: copy_field_description(Types::Packages::Protection::RuleType, :minimum_access_level_for_delete) + argument :minimum_access_level_for_push, Types::Packages::Protection::RuleAccessLevelEnum, - required: true, + required: false, description: copy_field_description(Types::Packages::Protection::RuleType, :minimum_access_level_for_push) field :package_protection_rule, @@ -40,8 +46,10 @@ class Create < ::Mutations::BaseMutation def resolve(project_path:, **kwargs) project = authorized_find!(project_path) - response = ::Packages::Protection::CreateRuleService.new(project: project, current_user: current_user, - params: kwargs).execute + response = + ::Packages::Protection::CreateRuleService + .new(project: project, current_user: current_user, params: kwargs) + .execute { package_protection_rule: response.payload[:package_protection_rule], errors: response.errors } end diff --git a/app/graphql/mutations/packages/protection/rule/update.rb b/app/graphql/mutations/packages/protection/rule/update.rb index 5255f0b24304cf..df2127589acf88 100644 --- a/app/graphql/mutations/packages/protection/rule/update.rb +++ b/app/graphql/mutations/packages/protection/rule/update.rb @@ -28,10 +28,15 @@ class Update < ::Mutations::BaseMutation validates: { allow_blank: false }, description: copy_field_description(Types::Packages::Protection::RuleType, :package_type) + argument :minimum_access_level_for_delete, + Types::Packages::Protection::RuleAccessLevelForDeleteEnum, + required: false, + experiment: { milestone: '17.10' }, + description: copy_field_description(Types::Packages::Protection::RuleType, :minimum_access_level_for_delete) + argument :minimum_access_level_for_push, Types::Packages::Protection::RuleAccessLevelEnum, required: false, - validates: { allow_blank: false }, description: copy_field_description(Types::Packages::Protection::RuleType, :minimum_access_level_for_push) field :package_protection_rule, diff --git a/app/graphql/types/packages/protection/rule_access_level_for_delete_enum.rb b/app/graphql/types/packages/protection/rule_access_level_for_delete_enum.rb new file mode 100644 index 00000000000000..7936b96f9ee988 --- /dev/null +++ b/app/graphql/types/packages/protection/rule_access_level_for_delete_enum.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Types + module Packages + module Protection + class RuleAccessLevelForDeleteEnum < BaseEnum + graphql_name 'PackagesProtectionRuleAccessLevelForDelete' + description 'Access level for delete of a package protection rule resource' + + ::Packages::Protection::Rule.minimum_access_level_for_deletes.each_key do |access_level_key| + value access_level_key.upcase, + value: access_level_key.to_s, + description: "#{access_level_key.capitalize} access." \ + 'Available only when feature flag `packages_protected_packages_delete` is enabled.' + end + end + end + end +end diff --git a/app/graphql/types/packages/protection/rule_type.rb b/app/graphql/types/packages/protection/rule_type.rb index fd369f25be45a5..8bf2b18a458002 100644 --- a/app/graphql/types/packages/protection/rule_type.rb +++ b/app/graphql/types/packages/protection/rule_type.rb @@ -27,9 +27,19 @@ class RuleType < ::Types::BaseObject null: false, description: 'Package type protected by the protection rule. For example, `NPM`, `PYPI`.' + field :minimum_access_level_for_delete, + Types::Packages::Protection::RuleAccessLevelForDeleteEnum, + null: true, + experiment: { milestone: '17.10' }, + description: + 'Minimum GitLab access required to delete packages to the package registry. ' \ + 'Valid values include `OWNER` or `ADMIN`. ' \ + 'If the value is `nil`, the default minimum access level is `MAINTAINER`.' \ + 'Available only when feature flag `packages_protected_packages_delete` is enabled.' + field :minimum_access_level_for_push, Types::Packages::Protection::RuleAccessLevelEnum, - null: false, + null: true, description: 'Minimum GitLab access required to push packages to the package registry. ' \ 'Valid values include `MAINTAINER`, `OWNER`, or `ADMIN`. ' \ diff --git a/app/services/packages/protection/create_rule_service.rb b/app/services/packages/protection/create_rule_service.rb index 22149b2be1c0fa..c9958db8c13324 100644 --- a/app/services/packages/protection/create_rule_service.rb +++ b/app/services/packages/protection/create_rule_service.rb @@ -6,6 +6,7 @@ class CreateRuleService < BaseProjectService ALLOWED_ATTRIBUTES = %i[ package_name_pattern package_type + minimum_access_level_for_delete minimum_access_level_for_push ].freeze diff --git a/app/services/packages/protection/delete_rule_service.rb b/app/services/packages/protection/delete_rule_service.rb index a1fa111b57bee1..c25b6ae106a54c 100644 --- a/app/services/packages/protection/delete_rule_service.rb +++ b/app/services/packages/protection/delete_rule_service.rb @@ -7,8 +7,7 @@ class DeleteRuleService def initialize(package_protection_rule, current_user:) if package_protection_rule.blank? || current_user.blank? - raise ArgumentError, - 'package_protection_rule and current_user must be set' + raise ArgumentError, 'package_protection_rule and current_user must be set' end @package_protection_rule = package_protection_rule diff --git a/app/services/packages/protection/update_rule_service.rb b/app/services/packages/protection/update_rule_service.rb index 9bfa91dc902b73..b39dbfac39678d 100644 --- a/app/services/packages/protection/update_rule_service.rb +++ b/app/services/packages/protection/update_rule_service.rb @@ -8,6 +8,7 @@ class UpdateRuleService ALLOWED_ATTRIBUTES = %i[ package_name_pattern package_type + minimum_access_level_for_delete minimum_access_level_for_push ].freeze diff --git a/doc/api/graphql/reference/_index.md b/doc/api/graphql/reference/_index.md index 0d23c0d04783f7..26681ce18cbd84 100644 --- a/doc/api/graphql/reference/_index.md +++ b/doc/api/graphql/reference/_index.md @@ -4367,7 +4367,8 @@ Input type: `CreatePackagesProtectionRuleInput` | Name | Type | Description | | ---- | ---- | ----------- | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | -| `minimumAccessLevelForPush` | [`PackagesProtectionRuleAccessLevel!`](#packagesprotectionruleaccesslevel) | Minimum GitLab access required to push packages to the package registry. Valid values include `MAINTAINER`, `OWNER`, or `ADMIN`. If the value is `nil`, the default minimum access level is `DEVELOPER`. | +| `minimumAccessLevelForDelete` {{< icon name="warning-solid" >}} | [`PackagesProtectionRuleAccessLevelForDelete`](#packagesprotectionruleaccesslevelfordelete) | **Deprecated:** **Status**: Experiment. Introduced in GitLab 17.10. | +| `minimumAccessLevelForPush` | [`PackagesProtectionRuleAccessLevel`](#packagesprotectionruleaccesslevel) | Minimum GitLab access required to push packages to the package registry. Valid values include `MAINTAINER`, `OWNER`, or `ADMIN`. If the value is `nil`, the default minimum access level is `DEVELOPER`. | | `packageNamePattern` | [`String!`](#string) | Package name protected by the protection rule. For example, `@my-scope/my-package-*`. Wildcard character `*` allowed. | | `packageType` | [`PackagesProtectionRulePackageType!`](#packagesprotectionrulepackagetype) | Package type protected by the protection rule. For example, `NPM`, `PYPI`. | | `projectPath` | [`ID!`](#id) | Full path of the project where a protection rule is located. | @@ -11514,6 +11515,7 @@ Input type: `UpdatePackagesProtectionRuleInput` | ---- | ---- | ----------- | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `id` | [`PackagesProtectionRuleID!`](#packagesprotectionruleid) | Global ID of the package protection rule to be updated. | +| `minimumAccessLevelForDelete` {{< icon name="warning-solid" >}} | [`PackagesProtectionRuleAccessLevelForDelete`](#packagesprotectionruleaccesslevelfordelete) | **Deprecated:** **Status**: Experiment. Introduced in GitLab 17.10. | | `minimumAccessLevelForPush` | [`PackagesProtectionRuleAccessLevel`](#packagesprotectionruleaccesslevel) | Minimum GitLab access required to push packages to the package registry. Valid values include `MAINTAINER`, `OWNER`, or `ADMIN`. If the value is `nil`, the default minimum access level is `DEVELOPER`. | | `packageNamePattern` | [`String`](#string) | Package name protected by the protection rule. For example, `@my-scope/my-package-*`. Wildcard character `*` allowed. | | `packageType` | [`PackagesProtectionRulePackageType`](#packagesprotectionrulepackagetype) | Package type protected by the protection rule. For example, `NPM`, `PYPI`. | @@ -33076,7 +33078,8 @@ A packages protection rule designed to protect packages from being pushed by use | Name | Type | Description | | ---- | ---- | ----------- | | `id` | [`PackagesProtectionRuleID!`](#packagesprotectionruleid) | Global ID of the package protection rule. | -| `minimumAccessLevelForPush` | [`PackagesProtectionRuleAccessLevel!`](#packagesprotectionruleaccesslevel) | Minimum GitLab access required to push packages to the package registry. Valid values include `MAINTAINER`, `OWNER`, or `ADMIN`. If the value is `nil`, the default minimum access level is `DEVELOPER`. | +| `minimumAccessLevelForDelete` {{< icon name="warning-solid" >}} | [`PackagesProtectionRuleAccessLevelForDelete`](#packagesprotectionruleaccesslevelfordelete) | **Introduced** in GitLab 17.10. **Status**: Experiment. Minimum GitLab access required to delete packages to the package registry. Valid values include `OWNER` or `ADMIN`. If the value is `nil`, the default minimum access level is `MAINTAINER`.Available only when feature flag `packages_protected_packages_delete` is enabled. | +| `minimumAccessLevelForPush` | [`PackagesProtectionRuleAccessLevel`](#packagesprotectionruleaccesslevel) | Minimum GitLab access required to push packages to the package registry. Valid values include `MAINTAINER`, `OWNER`, or `ADMIN`. If the value is `nil`, the default minimum access level is `DEVELOPER`. | | `packageNamePattern` | [`String!`](#string) | Package name protected by the protection rule. For example, `@my-scope/my-package-*`. Wildcard character `*` allowed. | | `packageType` | [`PackagesProtectionRulePackageType!`](#packagesprotectionrulepackagetype) | Package type protected by the protection rule. For example, `NPM`, `PYPI`. | @@ -43357,6 +43360,15 @@ Access level of a package protection rule resource. | `MAINTAINER` | Maintainer access. | | `OWNER` | Owner access. | +### `PackagesProtectionRuleAccessLevelForDelete` + +Access level for delete of a package protection rule resource. + +| Value | Description | +| ----- | ----------- | +| `ADMIN` | Admin access.Available only when feature flag `packages_protected_packages_delete` is enabled. | +| `OWNER` | Owner access.Available only when feature flag `packages_protected_packages_delete` is enabled. | + ### `PackagesProtectionRulePackageType` Package type of a package protection rule resource. diff --git a/spec/graphql/types/packages/protection/rule_access_level_for_delete_enum_spec.rb b/spec/graphql/types/packages/protection/rule_access_level_for_delete_enum_spec.rb new file mode 100644 index 00000000000000..c716715a436338 --- /dev/null +++ b/spec/graphql/types/packages/protection/rule_access_level_for_delete_enum_spec.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GitlabSchema.types['PackagesProtectionRuleAccessLevelForDelete'], feature_category: :package_registry do + it 'exposes all options' do + expect(described_class.values.keys).to match_array(%w[OWNER ADMIN]) + end +end diff --git a/spec/graphql/types/packages/protection/rule_type_spec.rb b/spec/graphql/types/packages/protection/rule_type_spec.rb index 479fc324265d53..a9aabe88718e32 100644 --- a/spec/graphql/types/packages/protection/rule_type_spec.rb +++ b/spec/graphql/types/packages/protection/rule_type_spec.rb @@ -27,9 +27,15 @@ it { is_expected.to have_non_null_graphql_type(Types::Packages::Protection::RulePackageTypeEnum) } end + describe 'minimum_access_level_for_delete' do + subject { described_class.fields['minimumAccessLevelForDelete'] } + + it { is_expected.to have_nullable_graphql_type(Types::Packages::Protection::RuleAccessLevelForDeleteEnum) } + end + describe 'minimum_access_level_for_push' do subject { described_class.fields['minimumAccessLevelForPush'] } - it { is_expected.to have_non_null_graphql_type(Types::Packages::Protection::RuleAccessLevelEnum) } + it { is_expected.to have_nullable_graphql_type(Types::Packages::Protection::RuleAccessLevelEnum) } end end diff --git a/spec/requests/api/graphql/mutations/packages/protection/rule/create_spec.rb b/spec/requests/api/graphql/mutations/packages/protection/rule/create_spec.rb index ffa4ba6197e358..0a7fc06bb73c62 100644 --- a/spec/requests/api/graphql/mutations/packages/protection/rule/create_spec.rb +++ b/spec/requests/api/graphql/mutations/packages/protection/rule/create_spec.rb @@ -15,6 +15,7 @@ project_path: project.full_path, package_name_pattern: package_protection_rule_attributes.package_name_pattern, package_type: 'NPM', + minimum_access_level_for_delete: 'OWNER', minimum_access_level_for_push: 'MAINTAINER' } end @@ -26,6 +27,7 @@ id packageNamePattern packageType + minimumAccessLevelForDelete minimumAccessLevelForPush } errors @@ -56,6 +58,7 @@ 'id' => be_present, 'packageNamePattern' => kwargs[:package_name_pattern], 'packageType' => kwargs[:package_type], + 'minimumAccessLevelForDelete' => kwargs[:minimum_access_level_for_delete], 'minimumAccessLevelForPush' => kwargs[:minimum_access_level_for_push] ) end @@ -67,6 +70,7 @@ project: project, package_name_pattern: kwargs[:package_name_pattern], package_type: kwargs[:package_type].downcase, + minimum_access_level_for_delete: kwargs[:minimum_access_level_for_delete].downcase, minimum_access_level_for_push: kwargs[:minimum_access_level_for_push].downcase ) end @@ -109,6 +113,35 @@ end end + context 'with invalid input fields `minimumAccessLevelForPush` and `minimumAccessLevelForDelete`' do + let(:kwargs) do + super().merge!( + minimum_access_level_for_push: 'INVALID_ACCESS_LEVEL', + minimum_access_level_for_delete: 'INVALID_ACCESS_LEVEL' + ) + end + + it_behaves_like 'an erroneous response' + + it 'returns an error' do + subject + + expect_graphql_errors_to_include([/minimumAccessLevelForPush/, /minimumAccessLevelForDelete/]) + end + end + + context 'with blank input fields `minimumAccessLevelForPush` and `minimumAccessLevelForDelete`' do + let(:kwargs) { super().merge(minimum_access_level_for_push: nil, minimum_access_level_for_delete: nil) } + + it_behaves_like 'an erroneous response' + + it 'returns an error' do + subject + + expect(mutation_response_errors).to include('A rule must have at least a minimum access role for push or delete.') + end + end + context 'with existing packages protection rule' do let_it_be(:existing_package_protection_rule) do create(:package_protection_rule, project: project, minimum_access_level_for_push: :maintainer) diff --git a/spec/requests/api/graphql/mutations/packages/protection/rule/delete_spec.rb b/spec/requests/api/graphql/mutations/packages/protection/rule/delete_spec.rb index f4acdc6dbabe35..bef8fe0c697419 100644 --- a/spec/requests/api/graphql/mutations/packages/protection/rule/delete_spec.rb +++ b/spec/requests/api/graphql/mutations/packages/protection/rule/delete_spec.rb @@ -31,6 +31,7 @@ 'id' => package_protection_rule.to_global_id.to_s, 'packageNamePattern' => package_protection_rule.package_name_pattern, 'packageType' => package_protection_rule.package_type.upcase, + 'minimumAccessLevelForDelete' => package_protection_rule.minimum_access_level_for_delete.upcase, 'minimumAccessLevelForPush' => package_protection_rule.minimum_access_level_for_push.upcase } ) diff --git a/spec/requests/api/graphql/mutations/packages/protection/rule/update_spec.rb b/spec/requests/api/graphql/mutations/packages/protection/rule/update_spec.rb index 9b0e88c2cb7c4c..970e28d18ca608 100644 --- a/spec/requests/api/graphql/mutations/packages/protection/rule/update_spec.rb +++ b/spec/requests/api/graphql/mutations/packages/protection/rule/update_spec.rb @@ -19,6 +19,7 @@ <<~QUERY packageProtectionRule { packageNamePattern + minimumAccessLevelForDelete minimumAccessLevelForPush } clientMutationId @@ -31,11 +32,13 @@ { id: package_protection_rule.to_global_id, package_name_pattern: "#{package_protection_rule.package_name_pattern}-updated", + minimum_access_level_for_delete: 'OWNER', minimum_access_level_for_push: 'MAINTAINER' } end let(:mutation_response) { graphql_mutation_response(:update_packages_protection_rule) } + let(:mutation_response_errors) { mutation_response['errors'] } subject { post_graphql_mutation(mutation, current_user: current_user) } @@ -48,6 +51,7 @@ expect(mutation_response).to include( 'packageProtectionRule' => { 'packageNamePattern' => input[:package_name_pattern], + 'minimumAccessLevelForDelete' => input[:minimum_access_level_for_delete], 'minimumAccessLevelForPush' => input[:minimum_access_level_for_push] } ) @@ -57,6 +61,7 @@ subject.tap do expect(package_protection_rule.reload).to have_attributes( package_name_pattern: input[:package_name_pattern], + minimum_access_level_for_delete: input[:minimum_access_level_for_delete].downcase, minimum_access_level_for_push: input[:minimum_access_level_for_push].downcase ) end @@ -85,16 +90,14 @@ end it 'includes error message in response' do - is_expected.tap { expect(mutation_response['errors']).to eq ['Package name pattern has already been taken'] } + is_expected.tap { expect(mutation_response_errors).to eq ['Package name pattern has already been taken'] } end end context 'with invalid input param `minimumAccessLevelForPush`' do - let(:input) { super().merge(minimum_access_level_for_push: nil) } + let(:input) { super().merge(minimum_access_level_for_push: 'INVALID_ACCESS_LEVEL') } - it_behaves_like 'an erroneous response' - - it { is_expected.tap { expect_graphql_errors_to_include(/minimumAccessLevelForPush can't be blank/) } } + it { is_expected.tap { expect_graphql_errors_to_include(/invalid value for minimumAccessLevelForPush/) } } end context 'with invalid input param `packageNamePattern`' do @@ -105,6 +108,16 @@ it { is_expected.tap { expect_graphql_errors_to_include(/packageNamePattern can't be blank/) } } end + context 'with blank input fields `minimumAccessLevelForPush` and `minimumAccessLevelForDelete`' do + let(:input) { super().merge(minimum_access_level_for_push: nil, minimum_access_level_for_delete: nil) } + + it 'includes error message in response' do + is_expected.tap do + expect(mutation_response_errors).to include(/at least a minimum access role for push or delete/) + end + end + end + context 'when current_user does not have permission' do let_it_be(:developer) { create(:user, developer_of: project) } let_it_be(:reporter) { create(:user, reporter_of: project) } diff --git a/spec/requests/api/graphql/project/packages_protection_rules_spec.rb b/spec/requests/api/graphql/project/packages_protection_rules_spec.rb index 5b60a1458c3777..ac716f5e3e128d 100644 --- a/spec/requests/api/graphql/project/packages_protection_rules_spec.rb +++ b/spec/requests/api/graphql/project/packages_protection_rules_spec.rb @@ -37,6 +37,7 @@ hash_including( 'packageNamePattern' => package_protection_rule.package_name_pattern, 'packageType' => 'NPM', + 'minimumAccessLevelForDelete' => 'OWNER', 'minimumAccessLevelForPush' => 'MAINTAINER' ) ) diff --git a/spec/services/packages/protection/create_rule_service_spec.rb b/spec/services/packages/protection/create_rule_service_spec.rb index 1e8d8985cf5e18..52f8a096643f41 100644 --- a/spec/services/packages/protection/create_rule_service_spec.rb +++ b/spec/services/packages/protection/create_rule_service_spec.rb @@ -61,6 +61,13 @@ it_behaves_like 'an erroneous service response with side effect', message: "'unknown_package_type' is not a valid package_type" end + + context 'when minimum_access_level_for_delete and minimum_access_level_for_push are blank' do + let(:params) { super().merge(minimum_access_level_for_delete: nil, minimum_access_level_for_push: nil) } + + it_behaves_like 'an erroneous service response with side effect', + message: ['A rule must have at least a minimum access role for push or delete.'] + end end context 'with existing PackageProtectionRule' do diff --git a/spec/services/packages/protection/update_rule_service_spec.rb b/spec/services/packages/protection/update_rule_service_spec.rb index 2d8f98aa6cda11..3f38c0ee6c96ba 100644 --- a/spec/services/packages/protection/update_rule_service_spec.rb +++ b/spec/services/packages/protection/update_rule_service_spec.rb @@ -14,8 +14,8 @@ :package_protection_rule, package_name_pattern: "#{package_protection_rule.package_name_pattern}-updated", package_type: 'npm', - minimum_access_level_for_push: 'owner', - minimum_access_level_for_delete: 'owner' + minimum_access_level_for_delete: 'admin', + minimum_access_level_for_push: 'owner' ) end @@ -72,6 +72,13 @@ message: "'unknown_package_type' is not a valid package_type" end + context 'when minimum_access_level_for_delete and minimum_access_level_for_push are blank' do + let(:params) { super().merge(minimum_access_level_for_delete: nil, minimum_access_level_for_push: nil) } + + it_behaves_like 'an erroneous service response with side effect', + message: ['A rule must have at least a minimum access role for push or delete.'] + end + context 'with empty params' do let(:params) { {} } -- GitLab From 613c37b5904e0e1d035a67e18e3c1f489002c9d8 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Mon, 3 Mar 2025 18:17:05 +0100 Subject: [PATCH 2/6] docs: Apply suggestion from @z_painter - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/180260#note_2373377883 - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/180260#note_2373377873 - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/180260#note_2373377870 --- .../protection/rule_access_level_for_delete_enum.rb | 4 ++-- app/graphql/types/packages/protection/rule_type.rb | 2 +- doc/api/graphql/reference/_index.md | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/graphql/types/packages/protection/rule_access_level_for_delete_enum.rb b/app/graphql/types/packages/protection/rule_access_level_for_delete_enum.rb index 7936b96f9ee988..646d0bef56d645 100644 --- a/app/graphql/types/packages/protection/rule_access_level_for_delete_enum.rb +++ b/app/graphql/types/packages/protection/rule_access_level_for_delete_enum.rb @@ -5,12 +5,12 @@ module Packages module Protection class RuleAccessLevelForDeleteEnum < BaseEnum graphql_name 'PackagesProtectionRuleAccessLevelForDelete' - description 'Access level for delete of a package protection rule resource' + description 'Access level for the deletion of a package protection rule resource.' ::Packages::Protection::Rule.minimum_access_level_for_deletes.each_key do |access_level_key| value access_level_key.upcase, value: access_level_key.to_s, - description: "#{access_level_key.capitalize} access." \ + description: "#{access_level_key.capitalize} access. " \ 'Available only when feature flag `packages_protected_packages_delete` is enabled.' end end diff --git a/app/graphql/types/packages/protection/rule_type.rb b/app/graphql/types/packages/protection/rule_type.rb index 8bf2b18a458002..da0daff0f4d996 100644 --- a/app/graphql/types/packages/protection/rule_type.rb +++ b/app/graphql/types/packages/protection/rule_type.rb @@ -34,7 +34,7 @@ class RuleType < ::Types::BaseObject description: 'Minimum GitLab access required to delete packages to the package registry. ' \ 'Valid values include `OWNER` or `ADMIN`. ' \ - 'If the value is `nil`, the default minimum access level is `MAINTAINER`.' \ + 'If the value is `nil`, the default minimum access level is `MAINTAINER`. ' \ 'Available only when feature flag `packages_protected_packages_delete` is enabled.' field :minimum_access_level_for_push, diff --git a/doc/api/graphql/reference/_index.md b/doc/api/graphql/reference/_index.md index 26681ce18cbd84..0848130d08e4f0 100644 --- a/doc/api/graphql/reference/_index.md +++ b/doc/api/graphql/reference/_index.md @@ -33078,7 +33078,7 @@ A packages protection rule designed to protect packages from being pushed by use | Name | Type | Description | | ---- | ---- | ----------- | | `id` | [`PackagesProtectionRuleID!`](#packagesprotectionruleid) | Global ID of the package protection rule. | -| `minimumAccessLevelForDelete` {{< icon name="warning-solid" >}} | [`PackagesProtectionRuleAccessLevelForDelete`](#packagesprotectionruleaccesslevelfordelete) | **Introduced** in GitLab 17.10. **Status**: Experiment. Minimum GitLab access required to delete packages to the package registry. Valid values include `OWNER` or `ADMIN`. If the value is `nil`, the default minimum access level is `MAINTAINER`.Available only when feature flag `packages_protected_packages_delete` is enabled. | +| `minimumAccessLevelForDelete` {{< icon name="warning-solid" >}} | [`PackagesProtectionRuleAccessLevelForDelete`](#packagesprotectionruleaccesslevelfordelete) | **Introduced** in GitLab 17.10. **Status**: Experiment. Minimum GitLab access required to delete packages to the package registry. Valid values include `OWNER` or `ADMIN`. If the value is `nil`, the default minimum access level is `MAINTAINER`. Available only when feature flag `packages_protected_packages_delete` is enabled. | | `minimumAccessLevelForPush` | [`PackagesProtectionRuleAccessLevel`](#packagesprotectionruleaccesslevel) | Minimum GitLab access required to push packages to the package registry. Valid values include `MAINTAINER`, `OWNER`, or `ADMIN`. If the value is `nil`, the default minimum access level is `DEVELOPER`. | | `packageNamePattern` | [`String!`](#string) | Package name protected by the protection rule. For example, `@my-scope/my-package-*`. Wildcard character `*` allowed. | | `packageType` | [`PackagesProtectionRulePackageType!`](#packagesprotectionrulepackagetype) | Package type protected by the protection rule. For example, `NPM`, `PYPI`. | @@ -43362,12 +43362,12 @@ Access level of a package protection rule resource. ### `PackagesProtectionRuleAccessLevelForDelete` -Access level for delete of a package protection rule resource. +Access level for the deletion of a package protection rule resource. | Value | Description | | ----- | ----------- | -| `ADMIN` | Admin access.Available only when feature flag `packages_protected_packages_delete` is enabled. | -| `OWNER` | Owner access.Available only when feature flag `packages_protected_packages_delete` is enabled. | +| `ADMIN` | Admin access. Available only when feature flag `packages_protected_packages_delete` is enabled. | +| `OWNER` | Owner access. Available only when feature flag `packages_protected_packages_delete` is enabled. | ### `PackagesProtectionRulePackageType` -- GitLab From ee782d8e4a4aac9830028311f067c4ed77ab6b38 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Mon, 3 Mar 2025 18:27:11 +0100 Subject: [PATCH 3/6] refactor: Apply suggestions from @evakadlecova - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/180260#note_2375397103 - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/180260#note_2375397062 - Added tests when feature flag `packages_protected_packages_delete` is disabled --- .../packages/protection/rule/create.rb | 3 + .../packages/protection/rule/update.rb | 3 + .../types/packages/protection/rule_type.rb | 6 ++ .../packages/protection/rule_type_spec.rb | 22 +++++++ .../packages/protection/rule/create_spec.rb | 47 +++++++++++---- .../packages/protection/rule/update_spec.rb | 58 ++++++++++++++----- 6 files changed, 115 insertions(+), 24 deletions(-) diff --git a/app/graphql/mutations/packages/protection/rule/create.rb b/app/graphql/mutations/packages/protection/rule/create.rb index 554af34678cf1e..2105569f2970f1 100644 --- a/app/graphql/mutations/packages/protection/rule/create.rb +++ b/app/graphql/mutations/packages/protection/rule/create.rb @@ -46,6 +46,9 @@ class Create < ::Mutations::BaseMutation def resolve(project_path:, **kwargs) project = authorized_find!(project_path) + kwargs.except!(:minimum_access_level_for_delete) if Feature.disabled?(:packages_protected_packages_delete, + project) + response = ::Packages::Protection::CreateRuleService .new(project: project, current_user: current_user, params: kwargs) diff --git a/app/graphql/mutations/packages/protection/rule/update.rb b/app/graphql/mutations/packages/protection/rule/update.rb index df2127589acf88..1deda1d5ca063d 100644 --- a/app/graphql/mutations/packages/protection/rule/update.rb +++ b/app/graphql/mutations/packages/protection/rule/update.rb @@ -47,6 +47,9 @@ class Update < ::Mutations::BaseMutation def resolve(id:, **kwargs) package_protection_rule = authorized_find!(id: id) + kwargs.except!(:minimum_access_level_for_delete) if Feature.disabled?(:packages_protected_packages_delete, + package_protection_rule.project) + response = ::Packages::Protection::UpdateRuleService.new(package_protection_rule, current_user: current_user, params: kwargs).execute diff --git a/app/graphql/types/packages/protection/rule_type.rb b/app/graphql/types/packages/protection/rule_type.rb index da0daff0f4d996..16dd84623ad4b9 100644 --- a/app/graphql/types/packages/protection/rule_type.rb +++ b/app/graphql/types/packages/protection/rule_type.rb @@ -44,6 +44,12 @@ class RuleType < ::Types::BaseObject 'Minimum GitLab access required to push packages to the package registry. ' \ 'Valid values include `MAINTAINER`, `OWNER`, or `ADMIN`. ' \ 'If the value is `nil`, the default minimum access level is `DEVELOPER`.' + + def minimum_access_level_for_delete + return unless Feature.enabled?(:packages_protected_packages_delete, object&.project) + + object.minimum_access_level_for_delete + end end end end diff --git a/spec/graphql/types/packages/protection/rule_type_spec.rb b/spec/graphql/types/packages/protection/rule_type_spec.rb index a9aabe88718e32..39250282b48fd9 100644 --- a/spec/graphql/types/packages/protection/rule_type_spec.rb +++ b/spec/graphql/types/packages/protection/rule_type_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe GitlabSchema.types['PackagesProtectionRule'], feature_category: :package_registry do + include GraphqlHelpers + specify { expect(described_class.graphql_name).to eq('PackagesProtectionRule') } specify { expect(described_class.description).to be_present } @@ -31,6 +33,26 @@ subject { described_class.fields['minimumAccessLevelForDelete'] } it { is_expected.to have_nullable_graphql_type(Types::Packages::Protection::RuleAccessLevelForDeleteEnum) } + + describe 'resolve field' do + let_it_be(:package_protection_rule) { create(:package_protection_rule) } + let(:user) { package_protection_rule.project.owner } + + subject do + resolve_field(:minimum_access_level_for_delete, package_protection_rule, current_user: user, + object_type: described_class) + end + + it { is_expected.to eq 'owner' } + + context 'when the feature flag `packages_protected_packages_delete` is disabled' do + before do + stub_feature_flags(packages_protected_packages_delete: false) + end + + it { is_expected.to be_nil } + end + end end describe 'minimum_access_level_for_push' do diff --git a/spec/requests/api/graphql/mutations/packages/protection/rule/create_spec.rb b/spec/requests/api/graphql/mutations/packages/protection/rule/create_spec.rb index 0a7fc06bb73c62..df24dca1024816 100644 --- a/spec/requests/api/graphql/mutations/packages/protection/rule/create_spec.rb +++ b/spec/requests/api/graphql/mutations/packages/protection/rule/create_spec.rb @@ -56,10 +56,10 @@ expect(mutation_response_package_protection_rule).to include( 'id' => be_present, - 'packageNamePattern' => kwargs[:package_name_pattern], - 'packageType' => kwargs[:package_type], - 'minimumAccessLevelForDelete' => kwargs[:minimum_access_level_for_delete], - 'minimumAccessLevelForPush' => kwargs[:minimum_access_level_for_push] + 'packageNamePattern' => expected_attributes[:package_name_pattern], + 'packageType' => expected_attributes[:package_type]&.upcase, + 'minimumAccessLevelForDelete' => expected_attributes[:minimum_access_level_for_delete]&.upcase, + 'minimumAccessLevelForPush' => expected_attributes[:minimum_access_level_for_push]&.upcase ) end @@ -68,10 +68,10 @@ expect(Packages::Protection::Rule.last).to have_attributes( project: project, - package_name_pattern: kwargs[:package_name_pattern], - package_type: kwargs[:package_type].downcase, - minimum_access_level_for_delete: kwargs[:minimum_access_level_for_delete].downcase, - minimum_access_level_for_push: kwargs[:minimum_access_level_for_push].downcase + package_name_pattern: expected_attributes[:package_name_pattern], + package_type: expected_attributes[:package_type]&.downcase, + minimum_access_level_for_delete: expected_attributes[:minimum_access_level_for_delete]&.downcase, + minimum_access_level_for_push: expected_attributes[:minimum_access_level_for_push]&.downcase ) end end @@ -82,7 +82,31 @@ end end - it_behaves_like 'a successful response' + it_behaves_like 'a successful response' do + let(:expected_attributes) { kwargs } + end + + context 'when feature flag `packages_protected_packages_delete` is disabled' do + before do + stub_feature_flags(packages_protected_packages_delete: false) + end + + it_behaves_like 'a successful response' do + let(:expected_attributes) { kwargs.merge(minimum_access_level_for_delete: nil) } + end + + context 'when minimum_access_level_for_push is nil' do + let(:kwargs) { super().merge(minimum_access_level_for_push: nil) } + + it_behaves_like 'an erroneous response' + + it 'returns an error' do + subject + + expect(mutation_response_errors).to include(/must have at least a minimum access role for push or delete./) + end + end + end context 'with invalid kwargs leading to error from graphql' do let(:kwargs) do @@ -161,14 +185,15 @@ super().merge!(package_name_pattern: "#{existing_package_protection_rule.package_name_pattern}-unique") end - it_behaves_like 'a successful response' + it_behaves_like 'a successful response' do + let(:expected_attributes) { kwargs } + end end context 'when field `minimum_access_level_for_push` is different than existing one' do let(:kwargs) { super().merge!(minimum_access_level_for_push: 'MAINTAINER') } it_behaves_like 'an erroneous response' - it 'returns an error' do subject.tap { expect(mutation_response_errors).to include(/Package name pattern has already been taken/) } end diff --git a/spec/requests/api/graphql/mutations/packages/protection/rule/update_spec.rb b/spec/requests/api/graphql/mutations/packages/protection/rule/update_spec.rb index 970e28d18ca608..5d263a7c31a854 100644 --- a/spec/requests/api/graphql/mutations/packages/protection/rule/update_spec.rb +++ b/spec/requests/api/graphql/mutations/packages/protection/rule/update_spec.rb @@ -32,7 +32,7 @@ { id: package_protection_rule.to_global_id, package_name_pattern: "#{package_protection_rule.package_name_pattern}-updated", - minimum_access_level_for_delete: 'OWNER', + minimum_access_level_for_delete: 'ADMIN', minimum_access_level_for_push: 'MAINTAINER' } end @@ -50,21 +50,21 @@ expect(mutation_response).to include( 'packageProtectionRule' => { - 'packageNamePattern' => input[:package_name_pattern], - 'minimumAccessLevelForDelete' => input[:minimum_access_level_for_delete], - 'minimumAccessLevelForPush' => input[:minimum_access_level_for_push] + 'packageNamePattern' => expected_attributes[:package_name_pattern], + 'minimumAccessLevelForDelete' => expected_attributes[:minimum_access_level_for_delete]&.upcase, + 'minimumAccessLevelForPush' => expected_attributes[:minimum_access_level_for_push]&.upcase } ) end - it do - subject.tap do - expect(package_protection_rule.reload).to have_attributes( - package_name_pattern: input[:package_name_pattern], - minimum_access_level_for_delete: input[:minimum_access_level_for_delete].downcase, - minimum_access_level_for_push: input[:minimum_access_level_for_push].downcase - ) - end + it 'updates attributes of existing package protection rule' do + expect { subject }.not_to change { ::Packages::Protection::Rule.count } + + expect(package_protection_rule.reload).to have_attributes( + package_name_pattern: expected_attributes[:package_name_pattern], + minimum_access_level_for_delete: expected_attributes[:minimum_access_level_for_delete]&.downcase, + minimum_access_level_for_push: expected_attributes[:minimum_access_level_for_push]&.downcase + ) end end @@ -73,7 +73,33 @@ it { expect { subject }.not_to change { package_protection_rule.reload.updated_at } } end - it_behaves_like 'a successful response' + it_behaves_like 'a successful response' do + let(:expected_attributes) { input } + end + + context 'when feature flag `packages_protected_packages_delete` is disabled' do + before do + package_protection_rule.update!(minimum_access_level_for_delete: nil) + + stub_feature_flags(packages_protected_packages_delete: false) + end + + it_behaves_like 'a successful response' do + let(:expected_attributes) do + input.merge(minimum_access_level_for_delete: package_protection_rule.minimum_access_level_for_delete) + end + end + + context 'when minimum_access_level_for_push is nil' do + let(:input) { super().merge(minimum_access_level_for_push: nil) } + + it 'includes error message in response' do + is_expected.tap do + expect(mutation_response_errors).to include(/at least a minimum access role for push or delete/) + end + end + end + end context 'with other existing package protection rule with same package_name_pattern' do let_it_be_with_reload(:other_existing_package_protection_rule) do @@ -100,6 +126,12 @@ it { is_expected.tap { expect_graphql_errors_to_include(/invalid value for minimumAccessLevelForPush/) } } end + context 'with invalid input param `minimumAccessLevelForDelete`' do + let(:input) { super().merge(minimum_access_level_for_delete: 'INVALID_ACCESS_LEVEL') } + + it { is_expected.tap { expect_graphql_errors_to_include(/invalid value for minimumAccessLevelForDelete/) } } + end + context 'with invalid input param `packageNamePattern`' do let(:input) { super().merge(package_name_pattern: '') } -- GitLab From 7e7e3a524e50ab941a815944db1f0d4bf2ef4dfb Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Sun, 9 Mar 2025 19:30:02 +0100 Subject: [PATCH 4/6] refactor: Apply suggestions from @dmeshcharakou - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/180260#note_2384534383 - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/180260#note_2384534374 --- .../gitlab_com_derisk/packages_protected_packages_delete.yml | 2 +- .../graphql/mutations/packages/protection/rule/create_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 index 7911ee3ca2e022..a500be1ee40db9 100644 --- a/config/feature_flags/gitlab_com_derisk/packages_protected_packages_delete.yml +++ b/config/feature_flags/gitlab_com_derisk/packages_protected_packages_delete.yml @@ -1,7 +1,7 @@ --- 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 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/180260 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/516215 milestone: '17.10' group: group::package registry diff --git a/spec/requests/api/graphql/mutations/packages/protection/rule/create_spec.rb b/spec/requests/api/graphql/mutations/packages/protection/rule/create_spec.rb index df24dca1024816..baa3ee81756016 100644 --- a/spec/requests/api/graphql/mutations/packages/protection/rule/create_spec.rb +++ b/spec/requests/api/graphql/mutations/packages/protection/rule/create_spec.rb @@ -57,7 +57,7 @@ expect(mutation_response_package_protection_rule).to include( 'id' => be_present, 'packageNamePattern' => expected_attributes[:package_name_pattern], - 'packageType' => expected_attributes[:package_type]&.upcase, + 'packageType' => expected_attributes[:package_type].upcase, 'minimumAccessLevelForDelete' => expected_attributes[:minimum_access_level_for_delete]&.upcase, 'minimumAccessLevelForPush' => expected_attributes[:minimum_access_level_for_push]&.upcase ) -- GitLab From 12472f59479ee458a22b1d7bea7ed36b196870a1 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Tue, 11 Mar 2025 14:32:52 +0100 Subject: [PATCH 5/6] docs: Apply suggestion from @z_painter - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/180260#note_2389179335 --- app/graphql/types/packages/protection/rule_type.rb | 2 +- doc/api/graphql/reference/_index.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/graphql/types/packages/protection/rule_type.rb b/app/graphql/types/packages/protection/rule_type.rb index 16dd84623ad4b9..62bfcd46d56787 100644 --- a/app/graphql/types/packages/protection/rule_type.rb +++ b/app/graphql/types/packages/protection/rule_type.rb @@ -32,7 +32,7 @@ class RuleType < ::Types::BaseObject null: true, experiment: { milestone: '17.10' }, description: - 'Minimum GitLab access required to delete packages to the package registry. ' \ + 'Minimum GitLab access required to delete packages from the package registry. ' \ 'Valid values include `OWNER` or `ADMIN`. ' \ 'If the value is `nil`, the default minimum access level is `MAINTAINER`. ' \ 'Available only when feature flag `packages_protected_packages_delete` is enabled.' diff --git a/doc/api/graphql/reference/_index.md b/doc/api/graphql/reference/_index.md index 0848130d08e4f0..45ec7b27141c55 100644 --- a/doc/api/graphql/reference/_index.md +++ b/doc/api/graphql/reference/_index.md @@ -33078,7 +33078,7 @@ A packages protection rule designed to protect packages from being pushed by use | Name | Type | Description | | ---- | ---- | ----------- | | `id` | [`PackagesProtectionRuleID!`](#packagesprotectionruleid) | Global ID of the package protection rule. | -| `minimumAccessLevelForDelete` {{< icon name="warning-solid" >}} | [`PackagesProtectionRuleAccessLevelForDelete`](#packagesprotectionruleaccesslevelfordelete) | **Introduced** in GitLab 17.10. **Status**: Experiment. Minimum GitLab access required to delete packages to the package registry. Valid values include `OWNER` or `ADMIN`. If the value is `nil`, the default minimum access level is `MAINTAINER`. Available only when feature flag `packages_protected_packages_delete` is enabled. | +| `minimumAccessLevelForDelete` {{< icon name="warning-solid" >}} | [`PackagesProtectionRuleAccessLevelForDelete`](#packagesprotectionruleaccesslevelfordelete) | **Introduced** in GitLab 17.10. **Status**: Experiment. Minimum GitLab access required to delete packages from the package registry. Valid values include `OWNER` or `ADMIN`. If the value is `nil`, the default minimum access level is `MAINTAINER`. Available only when feature flag `packages_protected_packages_delete` is enabled. | | `minimumAccessLevelForPush` | [`PackagesProtectionRuleAccessLevel`](#packagesprotectionruleaccesslevel) | Minimum GitLab access required to push packages to the package registry. Valid values include `MAINTAINER`, `OWNER`, or `ADMIN`. If the value is `nil`, the default minimum access level is `DEVELOPER`. | | `packageNamePattern` | [`String!`](#string) | Package name protected by the protection rule. For example, `@my-scope/my-package-*`. Wildcard character `*` allowed. | | `packageType` | [`PackagesProtectionRulePackageType!`](#packagesprotectionrulepackagetype) | Package type protected by the protection rule. For example, `NPM`, `PYPI`. | -- GitLab From 95f8539fc2750575351e574e26221af903163677 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Wed, 12 Mar 2025 08:55:15 +0100 Subject: [PATCH 6/6] revert: Undo changes in feature flag file due to rebase with master --- .../gitlab_com_derisk/packages_protected_packages_delete.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index a500be1ee40db9..7911ee3ca2e022 100644 --- a/config/feature_flags/gitlab_com_derisk/packages_protected_packages_delete.yml +++ b/config/feature_flags/gitlab_com_derisk/packages_protected_packages_delete.yml @@ -1,7 +1,7 @@ --- 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/180260 +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 -- GitLab