From 332db579e54f2bb765add2dcea818206f026d33c Mon Sep 17 00:00:00 2001 From: Gerardo Date: Mon, 10 Jul 2023 17:51:19 +0200 Subject: [PATCH 1/6] feat: Add graphql mutation to create package protection rules This MR adds a new graphql endpoint for creating package protection rules. Changelog: added --- .../packages/protection/rule/create.rb | 61 +++++++ app/graphql/types/mutation_type.rb | 1 + .../protection/rule_access_level_enum.rb | 18 ++ .../protection/rule_package_type_enum.rb | 19 ++ .../package_protection_rule_policy.rb | 9 + app/policies/project_policy.rb | 1 + .../protection/create_rule_service.rb | 40 +++++ .../packages_protected_packages.yml | 8 + doc/api/graphql/reference/index.md | 45 +++++ locale/gitlab.pot | 3 + spec/policies/project_policy_spec.rb | 22 +++ .../packages/protection/rule/create_spec.rb | 167 ++++++++++++++++++ .../protection/create_rule_service_spec.rb | 139 +++++++++++++++ 13 files changed, 533 insertions(+) create mode 100644 app/graphql/mutations/packages/protection/rule/create.rb create mode 100644 app/graphql/types/packages/protection/rule_access_level_enum.rb create mode 100644 app/graphql/types/packages/protection/rule_package_type_enum.rb create mode 100644 app/policies/packages/package_protection_rule_policy.rb create mode 100644 app/services/packages/protection/create_rule_service.rb create mode 100644 config/feature_flags/development/packages_protected_packages.yml create mode 100644 spec/requests/api/graphql/mutations/packages/protection/rule/create_spec.rb create mode 100644 spec/services/packages/protection/create_rule_service_spec.rb diff --git a/app/graphql/mutations/packages/protection/rule/create.rb b/app/graphql/mutations/packages/protection/rule/create.rb new file mode 100644 index 00000000000000..a8b349d2e0875d --- /dev/null +++ b/app/graphql/mutations/packages/protection/rule/create.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +module Mutations + module Packages + module Protection + module Rule + class Create < ::Mutations::BaseMutation + graphql_name 'CreatePackagesProtectionRule' + description 'CreatePackagesProtectionRule. ' \ + 'Available only when feature flag `packages_protected_packages` is enabled.' + + include FindsProject + + authorize :create_package_protection_rule + + argument :project_path, + GraphQL::Types::ID, + required: true, + description: 'Full path of the project.' + + argument :package_name_pattern, + GraphQL::Types::String, + required: true, + description: + 'Package name protected by the protection rule. For example `@my-scope/my-package-*`. ' \ + 'Wildcard character `*` allowed.' + + argument :package_type, + Types::Packages::Protection::RulePackageTypeEnum, + required: true, + description: 'Package type protected by the protection rule. For example `NPM`.' + + argument :push_protected_up_to_access_level, + Types::Packages::Protection::RuleAccessLevelEnum, + required: true, + description: + 'Max GitLab access level unable to push a package. For example `DEVELOPER`, `MAINTAINER`, `OWNER`.' + + def resolve(project_path:, **kwargs) + project = authorized_find!(project_path) + + if Feature.disabled?(:packages_protected_packages, project) + raise_resource_not_available_error!("'packages_protected_packages' feature flag is disabled") + end + + response = ::Packages::Protection::CreateRuleService.new(project, current_user, kwargs).execute + + if response.error? + return { + package_protection_rule: response.payload[:package_protection_rule], + errors: response.errors + } + end + + { package_protection_rule: response.payload[:package_protection_rule], errors: [] } + end + end + end + end + end +end diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index 9d74ca1342410f..3af7140aed30d2 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -170,6 +170,7 @@ class MutationType < BaseObject mount_mutation Mutations::Packages::BulkDestroy, extensions: [::Gitlab::Graphql::Limit::FieldCallCount => { limit: 1 }] mount_mutation Mutations::Packages::DestroyFile + mount_mutation Mutations::Packages::Protection::Rule::Create, alpha: { milestone: '16.5' } mount_mutation Mutations::Packages::DestroyFiles mount_mutation Mutations::Packages::Cleanup::Policy::Update mount_mutation Mutations::Echo diff --git a/app/graphql/types/packages/protection/rule_access_level_enum.rb b/app/graphql/types/packages/protection/rule_access_level_enum.rb new file mode 100644 index 00000000000000..788765a923008d --- /dev/null +++ b/app/graphql/types/packages/protection/rule_access_level_enum.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Types + module Packages + module Protection + class RuleAccessLevelEnum < BaseEnum + graphql_name 'RuleAccessLevel' + description 'Access level to a package protection rule ressource' + + value 'DEVELOPER', value: Gitlab::Access::DEVELOPER, description: 'Developer access.' + value 'MAINTAINER', value: Gitlab::Access::MAINTAINER, description: 'Maintainer access.' + value 'OWNER', value: Gitlab::Access::OWNER, description: 'Owner access.' + end + end + end +end + +Types::Packages::Protection::RuleAccessLevelEnum.prepend_mod_with('Types::Packages::Protection::RuleAccessLevelEnum') diff --git a/app/graphql/types/packages/protection/rule_package_type_enum.rb b/app/graphql/types/packages/protection/rule_package_type_enum.rb new file mode 100644 index 00000000000000..53a4aeb40c3311 --- /dev/null +++ b/app/graphql/types/packages/protection/rule_package_type_enum.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Types + module Packages + module Protection + class RulePackageTypeEnum < BaseEnum + graphql_name 'RulePackageType' + description 'Package type to a package protection rule ressource' + + ::Packages::Protection::Rule.package_types.each_key do |package_type| + value package_type.upcase, value: package_type.to_s, + description: "Packages from the #{package_type} package manager" + end + end + end + end +end + +Types::Packages::Protection::RulePackageTypeEnum.prepend_mod_with('Types::Packages::Protection::RulePackageTypeEnum') diff --git a/app/policies/packages/package_protection_rule_policy.rb b/app/policies/packages/package_protection_rule_policy.rb new file mode 100644 index 00000000000000..8d5f40fe062849 --- /dev/null +++ b/app/policies/packages/package_protection_rule_policy.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module Packages + class PackageProtectionRulePolicy < BasePolicy + delegate { @subject.project } + + rule { can?(:maintainer) }.prevent :create_package_protection_rule + end +end diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index a3a0ab70a43158..4f98a642e8565b 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -570,6 +570,7 @@ class ProjectPolicy < BasePolicy enable :admin_incident_management_timeline_event_tag enable :stop_environment enable :read_import_error + enable :create_package_protection_rule end rule { public_project & metrics_dashboard_allowed }.policy do diff --git a/app/services/packages/protection/create_rule_service.rb b/app/services/packages/protection/create_rule_service.rb new file mode 100644 index 00000000000000..8e176b9a6b6f9d --- /dev/null +++ b/app/services/packages/protection/create_rule_service.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +module Packages + module Protection + class CreateRuleService < BaseService + ALLOWED_ATTRIBUTES = %i[ + package_name_pattern + package_type + push_protected_up_to_access_level + ].freeze + + def execute + unless can?(current_user, :create_package_protection_rule, project) + error_message = _('Unauthorized to create a package protection rule') + return service_response_error(message: error_message) + end + + package_protection_rule = project.package_protection_rules.create(params.slice(*ALLOWED_ATTRIBUTES)) + + unless package_protection_rule.persisted? + return service_response_error(message: package_protection_rule.errors.full_messages) + end + + ServiceResponse.success(payload: { package_protection_rule: package_protection_rule }) + rescue ArgumentError => e + service_response_error(message: e.message) + end + + private + + def service_response_error(message:, **args) + ServiceResponse.error( + message: message, + payload: { package_protection_rule: nil }, + **args + ) + end + end + end +end diff --git a/config/feature_flags/development/packages_protected_packages.yml b/config/feature_flags/development/packages_protected_packages.yml new file mode 100644 index 00000000000000..6e159248e01502 --- /dev/null +++ b/config/feature_flags/development/packages_protected_packages.yml @@ -0,0 +1,8 @@ +--- +name: packages_protected_packages +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/125915 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/416395 +milestone: '16.5' +type: development +group: group::package registry +default_enabled: false \ No newline at end of file diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index b4f57847408be2..9fd76381dafe6b 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -2392,6 +2392,33 @@ Input type: `CreateNoteInput` | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | | `note` | [`Note`](#note) | Note after mutation. | +### `Mutation.createPackagesProtectionRule` + +CreatePackagesProtectionRule. Available only when feature flag `packages_protected_packages` is enabled. + +WARNING: +**Introduced** in 16.5. +This feature is an Experiment. It can be changed or removed at any time. + +Input type: `CreatePackagesProtectionRuleInput` + +#### Arguments + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `packageNamePattern` | [`String!`](#string) | Package name protected by the protection rule. For example `@my-scope/my-package-*`. Wildcard character `*` allowed. | +| `packageType` | [`RulePackageType!`](#rulepackagetype) | Package type protected by the protection rule. For example `NPM`. | +| `projectPath` | [`ID!`](#id) | Full path of the project. | +| `pushProtectedUpToAccessLevel` | [`RuleAccessLevel!`](#ruleaccesslevel) | Max GitLab access level unable to push a package. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | + ### `Mutation.createRequirement` Input type: `CreateRequirementInput` @@ -28701,6 +28728,24 @@ Status of a requirement based on last test report. | `MISSING` | Requirements without any test report. | | `PASSED` | Passed test report. | +### `RuleAccessLevel` + +Access level to a package protection rule ressource. + +| Value | Description | +| ----- | ----------- | +| `DEVELOPER` | Developer access. | +| `MAINTAINER` | Maintainer access. | +| `OWNER` | Owner access. | + +### `RulePackageType` + +Package type to a package protection rule ressource. + +| Value | Description | +| ----- | ----------- | +| `NPM` | Packages from the npm package manager. | + ### `SastUiComponentSize` Size of UI component in SAST configuration page. diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 283d28b8771332..96d652ddf4cad8 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -50404,6 +50404,9 @@ msgstr "" msgid "Unauthorized to access the cluster agent in this project" msgstr "" +msgid "Unauthorized to create a package protection rule" +msgstr "" + msgid "Unauthorized to create an environment" msgstr "" diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index e7c2dcc4158edb..fb2e9ab419c014 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -3315,6 +3315,28 @@ def permissions_abilities(role) end end + describe ':create_package_protection_rule' do + using RSpec::Parameterized::TableSyntax + + where(:current_user, :allowed) do + ref(:anonymous) | false + ref(:non_member) | false + ref(:guest) | false + ref(:reporter) | false + ref(:developer) | false + ref(:maintainer) | true + ref(:owner) | true + end + + with_them do + if params[:allowed] + it { is_expected.to be_allowed(:create_package_protection_rule) } + else + it { is_expected.not_to be_allowed(:create_package_protection_rule) } + end + end + end + private def project_subject(project_type) 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 new file mode 100644 index 00000000000000..5f4e1a764ccf33 --- /dev/null +++ b/spec/requests/api/graphql/mutations/packages/protection/rule/create_spec.rb @@ -0,0 +1,167 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Creating the packages protection rule', feature_category: :package_registry do + include GraphqlHelpers + using RSpec::Parameterized::TableSyntax + + let_it_be(:project) { create(:project) } + let_it_be(:maintainer) { create(:user).tap { |u| project.add_maintainer(u) } } + let(:user) { maintainer } + + let(:package_protection_rule_attributes) { build_stubbed(:package_protection_rule, project: project) } + + let(:kwargs) do + { + project_path: project.full_path, + package_name_pattern: package_protection_rule_attributes.package_name_pattern, + package_type: "NPM", + push_protected_up_to_access_level: "MAINTAINER" + } + end + + let(:mutation) do + graphql_mutation(:create_packages_protection_rule, kwargs, + <<~QUERY + clientMutationId + errors + QUERY + ) + end + + let(:mutation_response) { graphql_mutation_response(:create_packages_protection_rule) } + + describe 'post graphql mutation' do + subject { post_graphql_mutation(mutation, current_user: user) } + + context 'without existing packages protection rule' do + it 'returns without error' do + subject + + expect_graphql_errors_to_be_empty + end + + it 'returns the created packages protection rule' do + expect { subject }.to change { ::Packages::Protection::Rule.count }.by(1) + + expect_graphql_errors_to_be_empty + expect(Packages::Protection::Rule.where(project: project).count).to eq 1 + + expect(Packages::Protection::Rule.where(project: project, + package_name_pattern: kwargs[:package_name_pattern])).to exist + end + + context 'when invalid fields are given' do + let(:kwargs) do + { + project_path: project.full_path, + package_name_pattern: '', + package_type: 'UNKNOWN_PACKAGE_TYPE', + push_protected_up_to_access_level: "UNKNOWN_ACCESS_LEVEL" + } + end + + it 'returns error about required argument' do + subject + + expect_graphql_errors_to_include(/was provided invalid value for packageType/) + expect_graphql_errors_to_include(/pushProtectedUpToAccessLevel/) + end + end + end + + context 'when user does not have permission' do + let_it_be(:developer) { create(:user).tap { |u| project.add_developer(u) } } + let_it_be(:reporter) { create(:user).tap { |u| project.add_reporter(u) } } + let_it_be(:guest) { create(:user).tap { |u| project.add_guest(u) } } + let_it_be(:anonymous) { create(:user) } + + where(:user) do + [ref(:developer), ref(:reporter), ref(:guest), ref(:anonymous)] + end + + with_them do + it 'returns an error' do + expect { subject }.not_to change { ::Packages::Protection::Rule.count } + + expect_graphql_errors_to_include(/you don't have permission to perform this action/) + end + end + end + + context 'with existing packages protection rule' do + let_it_be(:existing_package_protection_rule) do + create(:package_protection_rule, project: project, push_protected_up_to_access_level: Gitlab::Access::DEVELOPER) + end + + context 'when package name pattern is slightly different' do + let(:kwargs) do + { + project_path: project.full_path, + # The field `package_name_pattern` is unique; this is why we change the value in a minimum way + package_name_pattern: "#{existing_package_protection_rule.package_name_pattern}-unique", + package_type: "NPM", + push_protected_up_to_access_level: "DEVELOPER" + } + end + + it 'returns the created packages protection rule' do + expect { subject }.to change { ::Packages::Protection::Rule.count }.by(1) + + expect(Packages::Protection::Rule.where(project: project).count).to eq 2 + expect(Packages::Protection::Rule.where(project: project, + package_name_pattern: kwargs[:package_name_pattern])).to exist + end + + it 'returns without error' do + subject + + expect_graphql_errors_to_be_empty + end + end + + context 'when field `package_name_pattern` is taken' do + let(:kwargs) do + { + project_path: project.full_path, + package_name_pattern: existing_package_protection_rule.package_name_pattern, + package_type: "NPM", + push_protected_up_to_access_level: 'MAINTAINER' + } + end + + it 'returns without error' do + subject + + expect(mutation_response).to include 'errors' => ['Package name pattern has already been taken'] + end + + it 'does not create new package protection rules' do + expect { subject }.to change { Packages::Protection::Rule.count }.by(0) + + expect(Packages::Protection::Rule.where(project: project, + package_name_pattern: kwargs[:package_name_pattern], + push_protected_up_to_access_level: Gitlab::Access::MAINTAINER)).not_to exist + end + end + end + + context "when feature flag ':packages_protected_packages' disabled" do + before do + stub_feature_flags(packages_protected_packages: false) + end + + it 'does not create any pacakge protection rules' do + expect { subject }.to change { Packages::Protection::Rule.count }.by(0) + + expect(Packages::Protection::Rule.where(project: project)).not_to exist + expect(Packages::Protection::Rule.where(project: project)).not_to exist + end + + it 'returns error of disabled feature flag' do + subject.tap { expect_graphql_errors_to_include(/'packages_protected_packages' feature flag is disabled/) } + end + end + end +end diff --git a/spec/services/packages/protection/create_rule_service_spec.rb b/spec/services/packages/protection/create_rule_service_spec.rb new file mode 100644 index 00000000000000..cd9cb74c29bb0a --- /dev/null +++ b/spec/services/packages/protection/create_rule_service_spec.rb @@ -0,0 +1,139 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Protection::CreateRuleService, '#execute', feature_category: :environment_management do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:maintainer) { create(:user).tap { |u| project.add_maintainer(u) } } + + let(:service) { described_class.new(project, current_user, params) } + let(:current_user) { maintainer } + let(:params) { attributes_for(:package_protection_rule) } + + subject { service.execute } + + shared_examples 'a successful service response' do + let(:package_protection_rule_count_expected) { 1 } + it { is_expected.to be_success } + + it do + is_expected.to have_attributes( + payload: include( + package_protection_rule: be_a(Packages::Protection::Rule) + ) + ) + end + + it { expect(subject.payload).to include(package_protection_rule: be_a(Packages::Protection::Rule)) } + + it do + expect { subject }.to change { Packages::Protection::Rule.count }.by(1) + + expect(Packages::Protection::Rule.where(project: project).count).to eq package_protection_rule_count_expected + expect(Packages::Protection::Rule.where(project: project, + package_name_pattern: params[:package_name_pattern])).to exist + end + end + + shared_examples 'an erroneous service response' do + let(:package_protection_rule_count_expected) { 0 } + it { is_expected.to be_error } + it { is_expected.to have_attributes(payload: include(package_protection_rule: nil)) } + + it do + expect { subject }.to change { Packages::Protection::Rule.count }.by(0) + + expect(Packages::Protection::Rule.where(project: project).count).to eq package_protection_rule_count_expected + expect(Packages::Protection::Rule.where(project: project, + package_name_pattern: params[:package_name_pattern])).not_to exist + end + end + + context 'without existing PackageProtectionRules' do + context 'when fields are valid' do + it_behaves_like 'a successful service response' + end + + context 'when fields are invalid' do + let(:params) do + { + package_name_pattern: '', + package_type: 'unknown_package_type', + push_protected_up_to_access_level: 1000 + } + end + + it_behaves_like 'an erroneous service response' + end + end + + context 'with existing PackageProtectionRule' do + let_it_be(:existing_package_protection_rule) { create(:package_protection_rule, project: project) } + + context 'when package name pattern is slightly different' do + let(:params) do + attributes_for( + :package_protection_rule, + # The field `package_name_pattern` is unique; this is why we change the value in a minimum way + package_name_pattern: "#{existing_package_protection_rule.package_name_pattern}-unique", + package_type: existing_package_protection_rule.package_type, + push_protected_up_to_access_level: existing_package_protection_rule.push_protected_up_to_access_level + ) + end + + it_behaves_like 'a successful service response' do + let(:package_protection_rule_count_expected) { 2 } + end + end + + context 'when field `package_name_pattern` is taken' do + let(:params) do + attributes_for( + :package_protection_rule, + package_name_pattern: existing_package_protection_rule.package_name_pattern, + package_type: existing_package_protection_rule.package_type, + push_protected_up_to_access_level: existing_package_protection_rule.push_protected_up_to_access_level + ) + end + + it { is_expected.to be_error } + + it do + expect { subject }.to change { Packages::Protection::Rule.count }.by(0) + + expect(Packages::Protection::Rule.where(project: project).count).to eq 1 + expect( + Packages::Protection::Rule.where( + project: project, + package_name_pattern: params[:package_name_pattern] + ) + ).to exist + end + end + end + + context 'when disallowed params are passed' do + let(:params) do + attributes_for(:package_protection_rule) + .merge( + project_id: 1, + unsupported_param: 'unsupported_param_value' + ) + end + + it_behaves_like 'a successful service response' + end + + context 'with forbidden user access level (project developer role)' do + # Because of the access level hierarchy, we can assume that + # other access levels below developer role will also not be able to + # create package protection rules. + let_it_be(:developer) { create(:user).tap { |u| project.add_developer(u) } } + + let(:current_user) { developer } + + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes(message: match(/Unauthorized/)) } + end +end -- GitLab From cbb65eb84e9a87cb836f57a3112cad581a716e10 Mon Sep 17 00:00:00 2001 From: Phillip Wells Date: Sun, 24 Sep 2023 13:43:15 +0000 Subject: [PATCH 2/6] docs: Apply suggestion from @phillipwells --- doc/api/graphql/reference/index.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 9fd76381dafe6b..2cfcea90934f96 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -28730,7 +28730,7 @@ Status of a requirement based on last test report. ### `RuleAccessLevel` -Access level to a package protection rule ressource. +Access level of a package protection rule resource. | Value | Description | | ----- | ----------- | @@ -28740,7 +28740,7 @@ Access level to a package protection rule ressource. ### `RulePackageType` -Package type to a package protection rule ressource. +Package type of a package protection rule resource. | Value | Description | | ----- | ----------- | -- GitLab From 3c187f4ebc4a0fe8ff673d93f8225abc657b8cdc Mon Sep 17 00:00:00 2001 From: Dzmitry Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com> Date: Sun, 24 Sep 2023 13:49:34 +0000 Subject: [PATCH 3/6] refactor: Apply suggestion from @dmeshcharakou --- .../mutations/packages/protection/rule/create.rb | 13 +++---------- .../packages/protection/rule_access_level_enum.rb | 4 +--- .../packages/protection/rule_package_type_enum.rb | 8 +++----- .../packages/protection/create_rule_service.rb | 9 ++++----- doc/api/graphql/reference/index.md | 4 ++-- .../packages/protection/rule/create_spec.rb | 12 +++++------- 6 files changed, 18 insertions(+), 32 deletions(-) diff --git a/app/graphql/mutations/packages/protection/rule/create.rb b/app/graphql/mutations/packages/protection/rule/create.rb index a8b349d2e0875d..2f0a8e2ccc7501 100644 --- a/app/graphql/mutations/packages/protection/rule/create.rb +++ b/app/graphql/mutations/packages/protection/rule/create.rb @@ -11,12 +11,12 @@ class Create < ::Mutations::BaseMutation include FindsProject - authorize :create_package_protection_rule + authorize :admin_package argument :project_path, GraphQL::Types::ID, required: true, - description: 'Full path of the project.' + description: 'Full path of the project where a protection rule is located.' argument :package_name_pattern, GraphQL::Types::String, @@ -45,14 +45,7 @@ def resolve(project_path:, **kwargs) response = ::Packages::Protection::CreateRuleService.new(project, current_user, kwargs).execute - if response.error? - return { - package_protection_rule: response.payload[:package_protection_rule], - errors: response.errors - } - end - - { package_protection_rule: response.payload[:package_protection_rule], errors: [] } + { package_protection_rule: response.payload[:package_protection_rule], errors: response.errors } end end end diff --git a/app/graphql/types/packages/protection/rule_access_level_enum.rb b/app/graphql/types/packages/protection/rule_access_level_enum.rb index 788765a923008d..5f3aa7ed480b81 100644 --- a/app/graphql/types/packages/protection/rule_access_level_enum.rb +++ b/app/graphql/types/packages/protection/rule_access_level_enum.rb @@ -5,7 +5,7 @@ module Packages module Protection class RuleAccessLevelEnum < BaseEnum graphql_name 'RuleAccessLevel' - description 'Access level to a package protection rule ressource' + description 'Access level of a package protection rule resource' value 'DEVELOPER', value: Gitlab::Access::DEVELOPER, description: 'Developer access.' value 'MAINTAINER', value: Gitlab::Access::MAINTAINER, description: 'Maintainer access.' @@ -14,5 +14,3 @@ class RuleAccessLevelEnum < BaseEnum end end end - -Types::Packages::Protection::RuleAccessLevelEnum.prepend_mod_with('Types::Packages::Protection::RuleAccessLevelEnum') diff --git a/app/graphql/types/packages/protection/rule_package_type_enum.rb b/app/graphql/types/packages/protection/rule_package_type_enum.rb index 53a4aeb40c3311..58110ff54654da 100644 --- a/app/graphql/types/packages/protection/rule_package_type_enum.rb +++ b/app/graphql/types/packages/protection/rule_package_type_enum.rb @@ -5,15 +5,13 @@ module Packages module Protection class RulePackageTypeEnum < BaseEnum graphql_name 'RulePackageType' - description 'Package type to a package protection rule ressource' + description 'Package type of a package protection rule resource' ::Packages::Protection::Rule.package_types.each_key do |package_type| - value package_type.upcase, value: package_type.to_s, - description: "Packages from the #{package_type} package manager" + value package_type.upcase, value: package_type, + description: "Packages of the #{package_type} format" end end end end end - -Types::Packages::Protection::RulePackageTypeEnum.prepend_mod_with('Types::Packages::Protection::RulePackageTypeEnum') diff --git a/app/services/packages/protection/create_rule_service.rb b/app/services/packages/protection/create_rule_service.rb index 8e176b9a6b6f9d..f0bd7f3d90051d 100644 --- a/app/services/packages/protection/create_rule_service.rb +++ b/app/services/packages/protection/create_rule_service.rb @@ -10,7 +10,7 @@ class CreateRuleService < BaseService ].freeze def execute - unless can?(current_user, :create_package_protection_rule, project) + unless can?(current_user, :admin_package, project) error_message = _('Unauthorized to create a package protection rule') return service_response_error(message: error_message) end @@ -22,17 +22,16 @@ def execute end ServiceResponse.success(payload: { package_protection_rule: package_protection_rule }) - rescue ArgumentError => e + rescue StandardError => e service_response_error(message: e.message) end private - def service_response_error(message:, **args) + def service_response_error(message:) ServiceResponse.error( message: message, - payload: { package_protection_rule: nil }, - **args + payload: { package_protection_rule: nil } ) end end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 2cfcea90934f96..c9e9f2e58e90b5 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -2409,7 +2409,7 @@ Input type: `CreatePackagesProtectionRuleInput` | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `packageNamePattern` | [`String!`](#string) | Package name protected by the protection rule. For example `@my-scope/my-package-*`. Wildcard character `*` allowed. | | `packageType` | [`RulePackageType!`](#rulepackagetype) | Package type protected by the protection rule. For example `NPM`. | -| `projectPath` | [`ID!`](#id) | Full path of the project. | +| `projectPath` | [`ID!`](#id) | Full path of the project where a protection rule is located. | | `pushProtectedUpToAccessLevel` | [`RuleAccessLevel!`](#ruleaccesslevel) | Max GitLab access level unable to push a package. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | #### Fields @@ -28744,7 +28744,7 @@ Package type of a package protection rule resource. | Value | Description | | ----- | ----------- | -| `NPM` | Packages from the npm package manager. | +| `NPM` | Packages of the npm format. | ### `SastUiComponentSize` 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 5f4e1a764ccf33..b0c8526fa1cd48 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 @@ -2,13 +2,12 @@ require 'spec_helper' -RSpec.describe 'Creating the packages protection rule', feature_category: :package_registry do +RSpec.describe 'Creating the packages protection rule', :aggregate_failures, feature_category: :package_registry do include GraphqlHelpers using RSpec::Parameterized::TableSyntax let_it_be(:project) { create(:project) } - let_it_be(:maintainer) { create(:user).tap { |u| project.add_maintainer(u) } } - let(:user) { maintainer } + let_it_be(:user) { create(:user, maintainer_projects: [project]) } let(:package_protection_rule_attributes) { build_stubbed(:package_protection_rule, project: project) } @@ -58,7 +57,7 @@ project_path: project.full_path, package_name_pattern: '', package_type: 'UNKNOWN_PACKAGE_TYPE', - push_protected_up_to_access_level: "UNKNOWN_ACCESS_LEVEL" + push_protected_up_to_access_level: 'UNKNOWN_ACCESS_LEVEL' } end @@ -126,7 +125,7 @@ { project_path: project.full_path, package_name_pattern: existing_package_protection_rule.package_name_pattern, - package_type: "NPM", + package_type: 'NPM', push_protected_up_to_access_level: 'MAINTAINER' } end @@ -152,11 +151,10 @@ stub_feature_flags(packages_protected_packages: false) end - it 'does not create any pacakge protection rules' do + it 'does not create any package protection rules' do expect { subject }.to change { Packages::Protection::Rule.count }.by(0) expect(Packages::Protection::Rule.where(project: project)).not_to exist - expect(Packages::Protection::Rule.where(project: project)).not_to exist end it 'returns error of disabled feature flag' do -- GitLab From bab5dc6d3b3edb04d6659d53fb0010ea41f38294 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Sun, 24 Sep 2023 16:34:04 +0200 Subject: [PATCH 4/6] refactor: Apply suggestion from @l.rosa --- app/graphql/mutations/packages/protection/rule/create.rb | 2 +- doc/api/graphql/reference/index.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/graphql/mutations/packages/protection/rule/create.rb b/app/graphql/mutations/packages/protection/rule/create.rb index 2f0a8e2ccc7501..2c04821aed68f1 100644 --- a/app/graphql/mutations/packages/protection/rule/create.rb +++ b/app/graphql/mutations/packages/protection/rule/create.rb @@ -6,7 +6,7 @@ module Protection module Rule class Create < ::Mutations::BaseMutation graphql_name 'CreatePackagesProtectionRule' - description 'CreatePackagesProtectionRule. ' \ + description 'Creates a protection rule to restrict access to project packages. ' \ 'Available only when feature flag `packages_protected_packages` is enabled.' include FindsProject diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index c9e9f2e58e90b5..1beeef0b8efefe 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -2394,7 +2394,7 @@ Input type: `CreateNoteInput` ### `Mutation.createPackagesProtectionRule` -CreatePackagesProtectionRule. Available only when feature flag `packages_protected_packages` is enabled. +Creates a protection rule to restrict access to project packages. Available only when feature flag `packages_protected_packages` is enabled. WARNING: **Introduced** in 16.5. -- GitLab From 250ed1e923011ee3c65b4c3e673058110f198af1 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Sun, 24 Sep 2023 16:42:08 +0200 Subject: [PATCH 5/6] refactor: Remove unnecessary policy :create_package_protection --- .../package_protection_rule_policy.rb | 9 -------- app/policies/project_policy.rb | 1 - spec/policies/project_policy_spec.rb | 22 ------------------- 3 files changed, 32 deletions(-) delete mode 100644 app/policies/packages/package_protection_rule_policy.rb diff --git a/app/policies/packages/package_protection_rule_policy.rb b/app/policies/packages/package_protection_rule_policy.rb deleted file mode 100644 index 8d5f40fe062849..00000000000000 --- a/app/policies/packages/package_protection_rule_policy.rb +++ /dev/null @@ -1,9 +0,0 @@ -# frozen_string_literal: true - -module Packages - class PackageProtectionRulePolicy < BasePolicy - delegate { @subject.project } - - rule { can?(:maintainer) }.prevent :create_package_protection_rule - end -end diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 4f98a642e8565b..a3a0ab70a43158 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -570,7 +570,6 @@ class ProjectPolicy < BasePolicy enable :admin_incident_management_timeline_event_tag enable :stop_environment enable :read_import_error - enable :create_package_protection_rule end rule { public_project & metrics_dashboard_allowed }.policy do diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index fb2e9ab419c014..e7c2dcc4158edb 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -3315,28 +3315,6 @@ def permissions_abilities(role) end end - describe ':create_package_protection_rule' do - using RSpec::Parameterized::TableSyntax - - where(:current_user, :allowed) do - ref(:anonymous) | false - ref(:non_member) | false - ref(:guest) | false - ref(:reporter) | false - ref(:developer) | false - ref(:maintainer) | true - ref(:owner) | true - end - - with_them do - if params[:allowed] - it { is_expected.to be_allowed(:create_package_protection_rule) } - else - it { is_expected.not_to be_allowed(:create_package_protection_rule) } - end - end - end - private def project_subject(project_type) -- GitLab From f25338f3298c216b72fa63f224d036e5704a28ff Mon Sep 17 00:00:00 2001 From: Gerardo Date: Sat, 30 Sep 2023 08:36:02 +0200 Subject: [PATCH 6/6] refactor: Apply suggestion from @dmeshcharakou This commit includes: - Defining a graphql type for the package protection rule (incl. tests) - Defining the response for the graphql mutation --- .../packages/protection/rule/create.rb | 5 ++ .../protection/rule_access_level_enum.rb | 2 +- .../protection/rule_package_type_enum.rb | 2 +- .../types/packages/protection/rule_type.rb | 33 ++++++++++++ doc/api/graphql/reference/index.md | 53 ++++++++++++------- .../protection/rule_access_level_enum_spec.rb | 9 ++++ .../protection/rule_package_type_enum_spec.rb | 9 ++++ .../packages/protection/rule_type_spec.rb | 29 ++++++++++ 8 files changed, 120 insertions(+), 22 deletions(-) create mode 100644 app/graphql/types/packages/protection/rule_type.rb create mode 100644 spec/graphql/types/packages/protection/rule_access_level_enum_spec.rb create mode 100644 spec/graphql/types/packages/protection/rule_package_type_enum_spec.rb create mode 100644 spec/graphql/types/packages/protection/rule_type_spec.rb diff --git a/app/graphql/mutations/packages/protection/rule/create.rb b/app/graphql/mutations/packages/protection/rule/create.rb index 2c04821aed68f1..2b8b18c5b30043 100644 --- a/app/graphql/mutations/packages/protection/rule/create.rb +++ b/app/graphql/mutations/packages/protection/rule/create.rb @@ -36,6 +36,11 @@ class Create < ::Mutations::BaseMutation description: 'Max GitLab access level unable to push a package. For example `DEVELOPER`, `MAINTAINER`, `OWNER`.' + field :package_protection_rule, + Types::Packages::Protection::RuleType, + null: true, + description: 'Packages protection rule after mutation.' + def resolve(project_path:, **kwargs) project = authorized_find!(project_path) diff --git a/app/graphql/types/packages/protection/rule_access_level_enum.rb b/app/graphql/types/packages/protection/rule_access_level_enum.rb index 5f3aa7ed480b81..fbc19847bcc6a6 100644 --- a/app/graphql/types/packages/protection/rule_access_level_enum.rb +++ b/app/graphql/types/packages/protection/rule_access_level_enum.rb @@ -4,7 +4,7 @@ module Types module Packages module Protection class RuleAccessLevelEnum < BaseEnum - graphql_name 'RuleAccessLevel' + graphql_name 'PackagesProtectionRuleAccessLevel' description 'Access level of a package protection rule resource' value 'DEVELOPER', value: Gitlab::Access::DEVELOPER, description: 'Developer access.' diff --git a/app/graphql/types/packages/protection/rule_package_type_enum.rb b/app/graphql/types/packages/protection/rule_package_type_enum.rb index 58110ff54654da..28e9df76adc2c8 100644 --- a/app/graphql/types/packages/protection/rule_package_type_enum.rb +++ b/app/graphql/types/packages/protection/rule_package_type_enum.rb @@ -4,7 +4,7 @@ module Types module Packages module Protection class RulePackageTypeEnum < BaseEnum - graphql_name 'RulePackageType' + graphql_name 'PackagesProtectionRulePackageType' description 'Package type of a package protection rule resource' ::Packages::Protection::Rule.package_types.each_key do |package_type| diff --git a/app/graphql/types/packages/protection/rule_type.rb b/app/graphql/types/packages/protection/rule_type.rb new file mode 100644 index 00000000000000..1e969d39ce2903 --- /dev/null +++ b/app/graphql/types/packages/protection/rule_type.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Types + module Packages + module Protection + class RuleType < ::Types::BaseObject + graphql_name 'PackagesProtectionRule' + description 'A packages protection rule designed to protect packages ' \ + 'from being pushed by users with a certain access level.' + + authorize :admin_package + + field :package_name_pattern, + GraphQL::Types::String, + null: false, + description: + 'Package name protected by the protection rule. For example `@my-scope/my-package-*`. ' \ + 'Wildcard character `*` allowed.' + + field :package_type, + Types::Packages::Protection::RulePackageTypeEnum, + null: false, + description: 'Package type protected by the protection rule. For example `NPM`.' + + field :push_protected_up_to_access_level, + Types::Packages::Protection::RuleAccessLevelEnum, + null: false, + description: + 'Max GitLab access level unable to push a package. For example `DEVELOPER`, `MAINTAINER`, `OWNER`.' + end + end + end +end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 1beeef0b8efefe..d35d5f3c6c4df9 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -2408,9 +2408,9 @@ Input type: `CreatePackagesProtectionRuleInput` | ---- | ---- | ----------- | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `packageNamePattern` | [`String!`](#string) | Package name protected by the protection rule. For example `@my-scope/my-package-*`. Wildcard character `*` allowed. | -| `packageType` | [`RulePackageType!`](#rulepackagetype) | Package type protected by the protection rule. For example `NPM`. | +| `packageType` | [`PackagesProtectionRulePackageType!`](#packagesprotectionrulepackagetype) | Package type protected by the protection rule. For example `NPM`. | | `projectPath` | [`ID!`](#id) | Full path of the project where a protection rule is located. | -| `pushProtectedUpToAccessLevel` | [`RuleAccessLevel!`](#ruleaccesslevel) | Max GitLab access level unable to push a package. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | +| `pushProtectedUpToAccessLevel` | [`PackagesProtectionRuleAccessLevel!`](#packagesprotectionruleaccesslevel) | Max GitLab access level unable to push a package. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | #### Fields @@ -2418,6 +2418,7 @@ Input type: `CreatePackagesProtectionRuleInput` | ---- | ---- | ----------- | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | +| `packageProtectionRule` | [`PackagesProtectionRule`](#packagesprotectionrule) | Packages protection rule after mutation. | ### `Mutation.createRequirement` @@ -21560,6 +21561,18 @@ A packages cleanup policy designed to keep only packages and packages assets tha | `keepNDuplicatedPackageFiles` | [`PackagesCleanupKeepDuplicatedPackageFilesEnum!`](#packagescleanupkeepduplicatedpackagefilesenum) | Number of duplicated package files to retain. | | `nextRunAt` | [`Time`](#time) | Next time that this packages cleanup policy will be executed. | +### `PackagesProtectionRule` + +A packages protection rule designed to protect packages from being pushed by users with a certain access level. + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `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`. | +| `pushProtectedUpToAccessLevel` | [`PackagesProtectionRuleAccessLevel!`](#packagesprotectionruleaccesslevel) | Max GitLab access level unable to push a package. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | + ### `PageInfo` Information about pagination in a connection. @@ -28559,6 +28572,24 @@ Values for sorting package. | `THIRTY_PACKAGE_FILES` | Value to keep 30 package files. | | `TWENTY_PACKAGE_FILES` | Value to keep 20 package files. | +### `PackagesProtectionRuleAccessLevel` + +Access level of a package protection rule resource. + +| Value | Description | +| ----- | ----------- | +| `DEVELOPER` | Developer access. | +| `MAINTAINER` | Maintainer access. | +| `OWNER` | Owner access. | + +### `PackagesProtectionRulePackageType` + +Package type of a package protection rule resource. + +| Value | Description | +| ----- | ----------- | +| `NPM` | Packages of the npm format. | + ### `PipelineConfigSourceEnum` | Value | Description | @@ -28728,24 +28759,6 @@ Status of a requirement based on last test report. | `MISSING` | Requirements without any test report. | | `PASSED` | Passed test report. | -### `RuleAccessLevel` - -Access level of a package protection rule resource. - -| Value | Description | -| ----- | ----------- | -| `DEVELOPER` | Developer access. | -| `MAINTAINER` | Maintainer access. | -| `OWNER` | Owner access. | - -### `RulePackageType` - -Package type of a package protection rule resource. - -| Value | Description | -| ----- | ----------- | -| `NPM` | Packages of the npm format. | - ### `SastUiComponentSize` Size of UI component in SAST configuration page. diff --git a/spec/graphql/types/packages/protection/rule_access_level_enum_spec.rb b/spec/graphql/types/packages/protection/rule_access_level_enum_spec.rb new file mode 100644 index 00000000000000..421b5fb0f399d7 --- /dev/null +++ b/spec/graphql/types/packages/protection/rule_access_level_enum_spec.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GitlabSchema.types['PackagesProtectionRuleAccessLevel'], feature_category: :package_registry do + it 'exposes all options' do + expect(described_class.values.keys).to match_array(%w[DEVELOPER MAINTAINER OWNER]) + end +end diff --git a/spec/graphql/types/packages/protection/rule_package_type_enum_spec.rb b/spec/graphql/types/packages/protection/rule_package_type_enum_spec.rb new file mode 100644 index 00000000000000..b0d9772f285c57 --- /dev/null +++ b/spec/graphql/types/packages/protection/rule_package_type_enum_spec.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GitlabSchema.types['PackagesProtectionRulePackageType'], feature_category: :package_registry do + it 'exposes all options' do + expect(described_class.values.keys).to contain_exactly('NPM') + end +end diff --git a/spec/graphql/types/packages/protection/rule_type_spec.rb b/spec/graphql/types/packages/protection/rule_type_spec.rb new file mode 100644 index 00000000000000..a4a458d35682b9 --- /dev/null +++ b/spec/graphql/types/packages/protection/rule_type_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GitlabSchema.types['PackagesProtectionRule'], feature_category: :package_registry do + specify { expect(described_class.graphql_name).to eq('PackagesProtectionRule') } + + specify { expect(described_class.description).to be_present } + + specify { expect(described_class).to require_graphql_authorizations(:admin_package) } + + describe 'package_name_pattern' do + subject { described_class.fields['packageNamePattern'] } + + it { is_expected.to have_non_null_graphql_type(GraphQL::Types::String) } + end + + describe 'package_type' do + subject { described_class.fields['packageType'] } + + it { is_expected.to have_non_null_graphql_type(Types::Packages::Protection::RulePackageTypeEnum) } + end + + describe 'push_protected_up_to_access_level' do + subject { described_class.fields['pushProtectedUpToAccessLevel'] } + + it { is_expected.to have_non_null_graphql_type(Types::Packages::Protection::RuleAccessLevelEnum) } + end +end -- GitLab