From 1858f59565be8a8a6e5c49a2c0a1d22b0c98d744 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Sun, 8 Oct 2023 08:13:16 +0200 Subject: [PATCH 1/3] feat: Graphql mutation to create container registry protection rules This MR includes: - Adding a new graphql endpoint for creating container registry protection rules Changelog: added --- .../protection/rule/create.rb | 63 ++++++ .../protection/rule_access_level_enum.rb | 17 ++ .../protection/rule_type.rb | 41 ++++ app/graphql/types/mutation_type.rb | 1 + .../protection/rule_policy.rb | 9 + .../protection/create_rule_service.rb | 40 ++++ ...ontainer_registry_protected_containers.yml | 8 + doc/api/graphql/reference/index.md | 57 ++++++ locale/gitlab.pot | 3 + .../protection/rule_access_level_enum_spec.rb | 9 + .../protection/rule_type_spec.rb | 35 ++++ .../protection/rule/create_spec.rb | 182 ++++++++++++++++++ .../protection/create_rule_service_spec.rb | 134 +++++++++++++ 13 files changed, 599 insertions(+) create mode 100644 app/graphql/mutations/container_registry/protection/rule/create.rb create mode 100644 app/graphql/types/container_registry/protection/rule_access_level_enum.rb create mode 100644 app/graphql/types/container_registry/protection/rule_type.rb create mode 100644 app/policies/container_registry/protection/rule_policy.rb create mode 100644 app/services/container_registry/protection/create_rule_service.rb create mode 100644 config/feature_flags/development/container_registry_protected_containers.yml create mode 100644 spec/graphql/types/container_registry/protection/rule_access_level_enum_spec.rb create mode 100644 spec/graphql/types/container_registry/protection/rule_type_spec.rb create mode 100644 spec/requests/api/graphql/mutations/container_registry/protection/rule/create_spec.rb create mode 100644 spec/services/container_registry/protection/create_rule_service_spec.rb diff --git a/app/graphql/mutations/container_registry/protection/rule/create.rb b/app/graphql/mutations/container_registry/protection/rule/create.rb new file mode 100644 index 00000000000000..eb9c3bf6f32487 --- /dev/null +++ b/app/graphql/mutations/container_registry/protection/rule/create.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +module Mutations + module ContainerRegistry + module Protection + module Rule + class Create < ::Mutations::BaseMutation + graphql_name 'CreateContainerRegistryProtectionRule' + description 'Creates a protection rule to restrict access to project container registry. ' \ + 'Available only when feature flag `container_registry_protected_containers` is enabled.' + + include FindsProject + + authorize :admin_container_image + + argument :project_path, + GraphQL::Types::ID, + required: true, + description: 'Full path of the project where a protection rule is located.' + + argument :container_path_pattern, + GraphQL::Types::String, + required: true, + description: + 'ContainerRegistryname protected by the protection rule. For example `@my-scope/my-container-*`. ' \ + 'Wildcard character `*` allowed.' + + argument :push_protected_up_to_access_level, + Types::ContainerRegistry::Protection::RuleAccessLevelEnum, + required: true, + description: + 'Max GitLab access level unable to push a container image to the container registry. ' \ + 'For example `DEVELOPER`, `MAINTAINER`, `OWNER`.' + + argument :delete_protected_up_to_access_level, + Types::ContainerRegistry::Protection::RuleAccessLevelEnum, + required: true, + description: + 'Max GitLab access level unable to delete a container image to the container registry. ' \ + 'For example `DEVELOPER`, `MAINTAINER`, `OWNER`.' + + field :container_registry_protection_rule, + Types::ContainerRegistry::Protection::RuleType, + null: true, + description: 'Container registry protection rule after mutation.' + + def resolve(project_path:, **kwargs) + project = authorized_find!(project_path) + + if Feature.disabled?(:container_registry_protected_containers, project) + raise_resource_not_available_error!("'container_registry_protected_containers' feature flag is disabled") + end + + response = ::ContainerRegistry::Protection::CreateRuleService.new(project, current_user, kwargs).execute + + { container_registry_protection_rule: response.payload[:container_registry_protection_rule], + errors: response.errors } + end + end + end + end + end +end diff --git a/app/graphql/types/container_registry/protection/rule_access_level_enum.rb b/app/graphql/types/container_registry/protection/rule_access_level_enum.rb new file mode 100644 index 00000000000000..31e8cbe2e49d3a --- /dev/null +++ b/app/graphql/types/container_registry/protection/rule_access_level_enum.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Types + module ContainerRegistry + module Protection + class RuleAccessLevelEnum < BaseEnum + graphql_name 'ContainerRegistryProtectionRuleAccessLevel' + description 'Access level of a container registry protection rule resource' + + ::ContainerRegistry::Protection::Rule.push_protected_up_to_access_levels.each_key do |access_level_key| + value access_level_key.upcase, value: access_level_key.to_s, + description: "#{access_level_key.capitalize} access." + end + end + end + end +end diff --git a/app/graphql/types/container_registry/protection/rule_type.rb b/app/graphql/types/container_registry/protection/rule_type.rb new file mode 100644 index 00000000000000..68966721267473 --- /dev/null +++ b/app/graphql/types/container_registry/protection/rule_type.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module Types + module ContainerRegistry + module Protection + class RuleType < ::Types::BaseObject + graphql_name 'ContainerRegistryProtectionRule' + description 'A container registry protection rule designed to protect container repositories ' \ + 'from being pushed by users with a certain access level.' + + authorize :admin_container_image + + field :id, + ::Types::GlobalIDType[::ContainerRegistry::Protection::Rule], + null: false, + description: 'ID of the container registry protection rule.' + + field :container_path_pattern, + GraphQL::Types::String, + null: false, + description: + 'Container repository path pattern protected by the protection rule. ' \ + 'For example `@my-scope/my-container-*`. Wildcard character `*` allowed.' + + field :push_protected_up_to_access_level, + Types::ContainerRegistry::Protection::RuleAccessLevelEnum, + null: false, + description: + 'Max GitLab access level unable to push images to a container repository. ' \ + 'For example `DEVELOPER`, `MAINTAINER`, `OWNER`.' + + field :delete_protected_up_to_access_level, + Types::ContainerRegistry::Protection::RuleAccessLevelEnum, + null: false, + description: + 'Max GitLab access level unable to delete images to a container repository. ' \ + 'For example `DEVELOPER`, `MAINTAINER`, `OWNER`.' + end + end + end +end diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index 475066e7279138..4ff64d6f60c9a9 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -134,6 +134,7 @@ class MutationType < BaseObject mount_mutation Mutations::DesignManagement::Move mount_mutation Mutations::DesignManagement::Update mount_mutation Mutations::ContainerExpirationPolicies::Update + mount_mutation Mutations::ContainerRegistry::Protection::Rule::Create, alpha: { milestone: '16.6' } mount_mutation Mutations::ContainerRepositories::Destroy mount_mutation Mutations::ContainerRepositories::DestroyTags mount_mutation Mutations::Ci::Catalog::Resources::Create, alpha: { milestone: '15.11' } diff --git a/app/policies/container_registry/protection/rule_policy.rb b/app/policies/container_registry/protection/rule_policy.rb new file mode 100644 index 00000000000000..4dc8dba3276974 --- /dev/null +++ b/app/policies/container_registry/protection/rule_policy.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module ContainerRegistry + module Protection + class RulePolicy < BasePolicy + delegate { @subject.project } + end + end +end diff --git a/app/services/container_registry/protection/create_rule_service.rb b/app/services/container_registry/protection/create_rule_service.rb new file mode 100644 index 00000000000000..038c644a509a30 --- /dev/null +++ b/app/services/container_registry/protection/create_rule_service.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +module ContainerRegistry + module Protection + class CreateRuleService < BaseService + ALLOWED_ATTRIBUTES = %i[ + container_path_pattern + push_protected_up_to_access_level + delete_protected_up_to_access_level + ].freeze + + def execute + unless can?(current_user, :admin_container_image, project) + error_message = _('Unauthorized to create a container registry protection rule') + return service_response_error(message: error_message) + end + + container_registry_protection_rule = + project.container_registry_protection_rules.create(params.slice(*ALLOWED_ATTRIBUTES)) + + unless container_registry_protection_rule.persisted? + return service_response_error(message: container_registry_protection_rule.errors.full_messages) + end + + ServiceResponse.success(payload: { container_registry_protection_rule: container_registry_protection_rule }) + rescue StandardError => e + service_response_error(message: e.message) + end + + private + + def service_response_error(message:) + ServiceResponse.error( + message: message, + payload: { container_registry_protection_rule: nil } + ) + end + end + end +end diff --git a/config/feature_flags/development/container_registry_protected_containers.yml b/config/feature_flags/development/container_registry_protected_containers.yml new file mode 100644 index 00000000000000..94305b7251b582 --- /dev/null +++ b/config/feature_flags/development/container_registry_protected_containers.yml @@ -0,0 +1,8 @@ +--- +name: container_registry_protected_containers +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/133527 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/429074 +milestone: '16.6' +type: development +group: group::container 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 1a84a8729427cb..468668f95db519 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -2290,6 +2290,34 @@ Input type: `CreateComplianceFrameworkInput` | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | | `framework` | [`ComplianceFramework`](#complianceframework) | Created compliance framework. | +### `Mutation.createContainerRegistryProtectionRule` + +Creates a protection rule to restrict access to project container registry. Available only when feature flag `container_registry_protected_containers` is enabled. + +WARNING: +**Introduced** in 16.6. +This feature is an Experiment. It can be changed or removed at any time. + +Input type: `CreateContainerRegistryProtectionRuleInput` + +#### Arguments + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `containerPathPattern` | [`String!`](#string) | ContainerRegistryname protected by the protection rule. For example `@my-scope/my-container-*`. Wildcard character `*` allowed. | +| `deleteProtectedUpToAccessLevel` | [`ContainerRegistryProtectionRuleAccessLevel!`](#containerregistryprotectionruleaccesslevel) | Max GitLab access level unable to delete a container image to the container registry. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | +| `projectPath` | [`ID!`](#id) | Full path of the project where a protection rule is located. | +| `pushProtectedUpToAccessLevel` | [`ContainerRegistryProtectionRuleAccessLevel!`](#containerregistryprotectionruleaccesslevel) | Max GitLab access level unable to push a container image to the container registry. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `containerRegistryProtectionRule` | [`ContainerRegistryProtectionRule`](#containerregistryprotectionrule) | Container registry protection rule after mutation. | +| `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | + ### `Mutation.createCustomEmoji` WARNING: @@ -15588,6 +15616,19 @@ A tag expiration policy designed to keep only the images that matter most. | `olderThan` | [`ContainerExpirationPolicyOlderThanEnum`](#containerexpirationpolicyolderthanenum) | Tags older that this will expire. | | `updatedAt` | [`Time!`](#time) | Timestamp of when the container expiration policy was updated. | +### `ContainerRegistryProtectionRule` + +A container registry protection rule designed to protect container repositories from being pushed by users with a certain access level. + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `containerPathPattern` | [`String!`](#string) | Container repository path pattern protected by the protection rule. For example `@my-scope/my-container-*`. Wildcard character `*` allowed. | +| `deleteProtectedUpToAccessLevel` | [`ContainerRegistryProtectionRuleAccessLevel!`](#containerregistryprotectionruleaccesslevel) | Max GitLab access level unable to delete images to a container repository. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | +| `id` | [`ContainerRegistryProtectionRuleID!`](#containerregistryprotectionruleid) | ID of the container registry protection rule. | +| `pushProtectedUpToAccessLevel` | [`ContainerRegistryProtectionRuleAccessLevel!`](#containerregistryprotectionruleaccesslevel) | Max GitLab access level unable to push images to a container repository. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | + ### `ContainerRepository` A container repository. @@ -28088,6 +28129,16 @@ Values for sorting contacts. | `SIXTY_DAYS` | 60 days until tags are automatically removed. | | `THIRTY_DAYS` | 30 days until tags are automatically removed. | +### `ContainerRegistryProtectionRuleAccessLevel` + +Access level of a container registry protection rule resource. + +| Value | Description | +| ----- | ----------- | +| `DEVELOPER` | Developer access. | +| `MAINTAINER` | Maintainer access. | +| `OWNER` | Owner access. | + ### `ContainerRepositoryCleanupStatus` Status of the tags cleanup of a container repository. @@ -30369,6 +30420,12 @@ A `ComplianceManagementFrameworkID` is a global ID. It is encoded as a string. An example `ComplianceManagementFrameworkID` is: `"gid://gitlab/ComplianceManagement::Framework/1"`. +### `ContainerRegistryProtectionRuleID` + +A `ContainerRegistryProtectionRuleID` is a global ID. It is encoded as a string. + +An example `ContainerRegistryProtectionRuleID` is: `"gid://gitlab/ContainerRegistry::Protection::Rule/1"`. + ### `ContainerRepositoryID` A `ContainerRepositoryID` is a global ID. It is encoded as a string. diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 1d7f90efca8e4f..80989ce3a88857 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -50703,6 +50703,9 @@ msgstr "" msgid "Unauthorized to access the cluster agent in this project" msgstr "" +msgid "Unauthorized to create a container registry protection rule" +msgstr "" + msgid "Unauthorized to create a package protection rule" msgstr "" diff --git a/spec/graphql/types/container_registry/protection/rule_access_level_enum_spec.rb b/spec/graphql/types/container_registry/protection/rule_access_level_enum_spec.rb new file mode 100644 index 00000000000000..57ad3e50b21051 --- /dev/null +++ b/spec/graphql/types/container_registry/protection/rule_access_level_enum_spec.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GitlabSchema.types['ContainerRegistryProtectionRuleAccessLevel'], 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/container_registry/protection/rule_type_spec.rb b/spec/graphql/types/container_registry/protection/rule_type_spec.rb new file mode 100644 index 00000000000000..58b53af80fb26c --- /dev/null +++ b/spec/graphql/types/container_registry/protection/rule_type_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GitlabSchema.types['ContainerRegistryProtectionRule'], feature_category: :container_registry do + specify { expect(described_class.graphql_name).to eq('ContainerRegistryProtectionRule') } + + specify { expect(described_class.description).to be_present } + + specify { expect(described_class).to require_graphql_authorizations(:admin_container_image) } + + describe 'id' do + subject { described_class.fields['id'] } + + it { is_expected.to have_non_null_graphql_type(::Types::GlobalIDType[::ContainerRegistry::Protection::Rule]) } + end + + describe 'container_path_pattern' do + subject { described_class.fields['containerPathPattern'] } + + it { is_expected.to have_non_null_graphql_type(GraphQL::Types::String) } + end + + describe 'push_protected_up_to_access_level' do + subject { described_class.fields['pushProtectedUpToAccessLevel'] } + + it { is_expected.to have_non_null_graphql_type(Types::ContainerRegistry::Protection::RuleAccessLevelEnum) } + end + + describe 'delete_protected_up_to_access_level' do + subject { described_class.fields['deleteProtectedUpToAccessLevel'] } + + it { is_expected.to have_non_null_graphql_type(Types::ContainerRegistry::Protection::RuleAccessLevelEnum) } + end +end diff --git a/spec/requests/api/graphql/mutations/container_registry/protection/rule/create_spec.rb b/spec/requests/api/graphql/mutations/container_registry/protection/rule/create_spec.rb new file mode 100644 index 00000000000000..5b59d90ff0ee65 --- /dev/null +++ b/spec/requests/api/graphql/mutations/container_registry/protection/rule/create_spec.rb @@ -0,0 +1,182 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Creating the container registry protection rule', :aggregate_failures, feature_category: :container_registry do + include GraphqlHelpers + + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user, maintainer_projects: [project]) } + + let(:container_registry_protection_rule_attributes) do + build_stubbed(:container_registry_protection_rule, project: project) + end + + let(:kwargs) do + { + project_path: project.full_path, + container_path_pattern: container_registry_protection_rule_attributes.container_path_pattern, + push_protected_up_to_access_level: 'MAINTAINER', + delete_protected_up_to_access_level: 'MAINTAINER' + } + end + + let(:mutation) do + graphql_mutation(:create_container_registry_protection_rule, kwargs, + <<~QUERY + containerRegistryProtectionRule { + id + containerPathPattern + } + clientMutationId + errors + QUERY + ) + end + + let(:mutation_response) { graphql_mutation_response(:create_container_registry_protection_rule) } + + subject { post_graphql_mutation(mutation, current_user: user) } + + shared_examples 'a successful response' do + it { subject.tap { expect_graphql_errors_to_be_empty } } + + it do + subject + + expect(mutation_response).to include( + 'errors' => be_blank, + 'containerRegistryProtectionRule' => { + 'id' => be_present, + 'containerPathPattern' => kwargs[:container_path_pattern] + } + ) + end + + it 'creates container registry protection rule in the database' do + expect { subject }.to change { ::ContainerRegistry::Protection::Rule.count }.by(1) + + expect(::ContainerRegistry::Protection::Rule.where(project: project, + container_path_pattern: kwargs[:container_path_pattern])).to exist + end + end + + shared_examples 'an erroneous response' do + it { expect { subject }.not_to change { ::ContainerRegistry::Protection::Rule.count } } + end + + it_behaves_like 'a successful response' + + context 'with invalid input fields `pushProtectedUpToAccessLevel` and `deleteProtectedUpToAccessLevel`' do + let(:kwargs) do + super().merge( + push_protected_up_to_access_level: 'UNKNOWN_ACCESS_LEVEL', + delete_protected_up_to_access_level: 'UNKNOWN_ACCESS_LEVEL' + ) + end + + it_behaves_like 'an erroneous response' + + it { + subject + + expect_graphql_errors_to_include([/pushProtectedUpToAccessLevel/, /deleteProtectedUpToAccessLevel/]) + } + end + + context 'with invalid input field `containerPathPattern`' do + let(:kwargs) do + super().merge(container_path_pattern: '') + end + + it_behaves_like 'an erroneous response' + + it { subject.tap { expect_graphql_errors_to_be_empty } } + + it { + subject.tap do + expect(mutation_response['errors']).to eq ["Container path pattern can't be blank"] + end + } + end + + context 'with existing containers protection rule' do + let_it_be(:existing_container_registry_protection_rule) do + create(:container_registry_protection_rule, project: project, + push_protected_up_to_access_level: Gitlab::Access::DEVELOPER) + end + + context 'when container name pattern is slightly different' do + let(:kwargs) do + # The field `container_path_pattern` is unique; this is why we change the value in a minimum way + super().merge( + container_path_pattern: "#{existing_container_registry_protection_rule.container_path_pattern}-unique" + ) + end + + it_behaves_like 'a successful response' + + it 'adds another container registry protection rule to the database' do + expect { subject }.to change { ::ContainerRegistry::Protection::Rule.count }.from(1).to(2) + end + end + + context 'when field `container_path_pattern` is taken' do + let(:kwargs) do + super().merge(container_path_pattern: existing_container_registry_protection_rule.container_path_pattern, + push_protected_up_to_access_level: 'MAINTAINER') + end + + it_behaves_like 'an erroneous response' + + it { subject.tap { expect_graphql_errors_to_be_empty } } + + it 'returns without error' do + subject + + expect(mutation_response['errors']).to eq ['Container path pattern has already been taken'] + end + + it 'does not create new container protection rules' do + # expect { subject }.to change { ::ContainerRegistry::Protection::Rule.count }.by(0) + + expect(::ContainerRegistry::Protection::Rule.where(project: project, + container_path_pattern: kwargs[:container_path_pattern], + push_protected_up_to_access_level: Gitlab::Access::MAINTAINER)).not_to exist + 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_behaves_like 'an erroneous response' + + it { subject.tap { expect_graphql_errors_to_include(/you don't have permission to perform this action/) } } + end + end + + context "when feature flag ':container_registry_protected_containers' disabled" do + before do + stub_feature_flags(container_registry_protected_containers: false) + end + + it_behaves_like 'an erroneous response' + + it { subject.tap { expect(::ContainerRegistry::Protection::Rule.where(project: project)).not_to exist } } + + it 'returns error of disabled feature flag' do + subject.tap do + expect_graphql_errors_to_include(/'container_registry_protected_containers' feature flag is disabled/) + end + end + end +end diff --git a/spec/services/container_registry/protection/create_rule_service_spec.rb b/spec/services/container_registry/protection/create_rule_service_spec.rb new file mode 100644 index 00000000000000..899d0cbb85c238 --- /dev/null +++ b/spec/services/container_registry/protection/create_rule_service_spec.rb @@ -0,0 +1,134 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ContainerRegistry::Protection::CreateRuleService, '#execute', feature_category: :container_registry do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:current_user) { create(:user, maintainer_projects: [project]) } + + let(:service) { described_class.new(project, current_user, params) } + let(:params) { attributes_for(:container_registry_protection_rule) } + + subject { service.execute } + + shared_examples 'a successful service response' do + it { is_expected.to be_success } + + it { is_expected.to have_attributes(errors: be_blank) } + + it do + is_expected.to have_attributes( + payload: { + container_registry_protection_rule: + be_a(ContainerRegistry::Protection::Rule) + .and(have_attributes( + container_path_pattern: params[:container_path_pattern], + push_protected_up_to_access_level: params[:push_protected_up_to_access_level].to_s, + delete_protected_up_to_access_level: params[:delete_protected_up_to_access_level].to_s + )) + } + ) + end + + it 'creates a new container registry protection rule in the database' do + expect { subject }.to change { ContainerRegistry::Protection::Rule.count }.by(1) + + expect( + ContainerRegistry::Protection::Rule.where( + project: project, + container_path_pattern: params[:container_path_pattern], + push_protected_up_to_access_level: params[:push_protected_up_to_access_level] + ) + ).to exist + end + end + + shared_examples 'an erroneous service response' do + it { is_expected.to be_error } + it { is_expected.to have_attributes(errors: be_present, payload: include(container_registry_protection_rule: nil)) } + + it 'does not create a new container registry protection rule in the database' do + expect { subject }.not_to change { ContainerRegistry::Protection::Rule.count } + end + + it 'does not create a container registry protection rule with the given params' do + subject + + expect( + ContainerRegistry::Protection::Rule.where( + project: project, + container_path_pattern: params[:container_path_pattern], + push_protected_up_to_access_level: params[:push_protected_up_to_access_level] + ) + ).not_to exist + end + end + + it_behaves_like 'a successful service response' + + it { expect { subject }.to change { ContainerRegistry::Protection::Rule.count }.from(0).to(1) } + + context 'when fields are invalid' do + let(:params) do + super().merge(container_path_pattern: '', push_protected_up_to_access_level: 1000) + end + + it_behaves_like 'an erroneous service response' + end + + context 'with existing container registry protection rule in the database' do + let_it_be_with_reload(:existing_container_registry_protection_rule) do + create(:container_registry_protection_rule, project: project) + end + + context 'when container registry name pattern is slightly different' do + let(:params) do + super().merge( + # The field `container_path_pattern` is unique; this is why we change the value in a minimum way + container_path_pattern: "#{existing_container_registry_protection_rule.container_path_pattern}-unique", + push_protected_up_to_access_level: + existing_container_registry_protection_rule.push_protected_up_to_access_level + ) + end + + it_behaves_like 'a successful service response' + + it { expect { subject }.to change { ContainerRegistry::Protection::Rule.count }.from(1).to(2) } + end + + context 'when field `container_path_pattern` is taken' do + let(:params) do + super().merge( + container_path_pattern: existing_container_registry_protection_rule.container_path_pattern, + push_protected_up_to_access_level: :maintainer + # existing_container_registry_protection_rule.push_protected_up_to_access_level + ) + end + + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes(errors: ['Container path pattern has already been taken']) } + + it { expect { subject }.not_to change { existing_container_registry_protection_rule.updated_at } } + end + end + + context 'with disallowed params' do + let(:params) { super().merge(project_id: 1, unsupported_param: 'unsupported_param_value') } + + it_behaves_like 'a successful service response' + + it { expect { subject }.to change { ContainerRegistry::Protection::Rule.count }.from(0).to(1) } + 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 container registry protection rules. + let_it_be(:current_user) { create(:user).tap { |u| project.add_developer(u) } } + + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes(message: match(/Unauthorized/)) } + end +end -- GitLab From 3a846b331cf7127bddefc69860f99ba79d8d9464 Mon Sep 17 00:00:00 2001 From: Marcel Amirault Date: Wed, 25 Oct 2023 08:44:53 +0000 Subject: [PATCH 2/3] refactor: Apply suggestion from @marcel.amirault This commit includes: - Improving the readability of the documentation --- .../container_registry/protection/rule/create.rb | 6 +++--- .../types/container_registry/protection/rule_type.rb | 8 ++++---- doc/api/graphql/reference/index.md | 12 ++++++------ 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/app/graphql/mutations/container_registry/protection/rule/create.rb b/app/graphql/mutations/container_registry/protection/rule/create.rb index eb9c3bf6f32487..cf8416480a27fd 100644 --- a/app/graphql/mutations/container_registry/protection/rule/create.rb +++ b/app/graphql/mutations/container_registry/protection/rule/create.rb @@ -6,7 +6,7 @@ module Protection module Rule class Create < ::Mutations::BaseMutation graphql_name 'CreateContainerRegistryProtectionRule' - description 'Creates a protection rule to restrict access to project container registry. ' \ + description 'Creates a protection rule to restrict access to a project\'s container registry. ' \ 'Available only when feature flag `container_registry_protected_containers` is enabled.' include FindsProject @@ -29,14 +29,14 @@ class Create < ::Mutations::BaseMutation Types::ContainerRegistry::Protection::RuleAccessLevelEnum, required: true, description: - 'Max GitLab access level unable to push a container image to the container registry. ' \ + 'Max GitLab access level to prevent from pushing container images to the container registry. ' \ 'For example `DEVELOPER`, `MAINTAINER`, `OWNER`.' argument :delete_protected_up_to_access_level, Types::ContainerRegistry::Protection::RuleAccessLevelEnum, required: true, description: - 'Max GitLab access level unable to delete a container image to the container registry. ' \ + 'Max GitLab access level to prevent from deleting container images in the container registry. ' \ 'For example `DEVELOPER`, `MAINTAINER`, `OWNER`.' field :container_registry_protection_rule, diff --git a/app/graphql/types/container_registry/protection/rule_type.rb b/app/graphql/types/container_registry/protection/rule_type.rb index 68966721267473..387f0202d2d734 100644 --- a/app/graphql/types/container_registry/protection/rule_type.rb +++ b/app/graphql/types/container_registry/protection/rule_type.rb @@ -5,8 +5,8 @@ module ContainerRegistry module Protection class RuleType < ::Types::BaseObject graphql_name 'ContainerRegistryProtectionRule' - description 'A container registry protection rule designed to protect container repositories ' \ - 'from being pushed by users with a certain access level.' + description 'A container registry protection rule designed to prevent users with a certain ' \ + 'access level or lower from altering the container registry.' authorize :admin_container_image @@ -26,14 +26,14 @@ class RuleType < ::Types::BaseObject Types::ContainerRegistry::Protection::RuleAccessLevelEnum, null: false, description: - 'Max GitLab access level unable to push images to a container repository. ' \ + 'Max GitLab access level to prevent from pushing container images to the container registry. ' \ 'For example `DEVELOPER`, `MAINTAINER`, `OWNER`.' field :delete_protected_up_to_access_level, Types::ContainerRegistry::Protection::RuleAccessLevelEnum, null: false, description: - 'Max GitLab access level unable to delete images to a container repository. ' \ + 'Max GitLab access level to prevent from pushing container images to the container registry. ' \ 'For example `DEVELOPER`, `MAINTAINER`, `OWNER`.' end end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 468668f95db519..68036bc4d83e8f 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -2292,7 +2292,7 @@ Input type: `CreateComplianceFrameworkInput` ### `Mutation.createContainerRegistryProtectionRule` -Creates a protection rule to restrict access to project container registry. Available only when feature flag `container_registry_protected_containers` is enabled. +Creates a protection rule to restrict access to a project's container registry. Available only when feature flag `container_registry_protected_containers` is enabled. WARNING: **Introduced** in 16.6. @@ -2306,9 +2306,9 @@ Input type: `CreateContainerRegistryProtectionRuleInput` | ---- | ---- | ----------- | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `containerPathPattern` | [`String!`](#string) | ContainerRegistryname protected by the protection rule. For example `@my-scope/my-container-*`. Wildcard character `*` allowed. | -| `deleteProtectedUpToAccessLevel` | [`ContainerRegistryProtectionRuleAccessLevel!`](#containerregistryprotectionruleaccesslevel) | Max GitLab access level unable to delete a container image to the container registry. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | +| `deleteProtectedUpToAccessLevel` | [`ContainerRegistryProtectionRuleAccessLevel!`](#containerregistryprotectionruleaccesslevel) | Max GitLab access level to prevent from deleting container images in the container registry. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | | `projectPath` | [`ID!`](#id) | Full path of the project where a protection rule is located. | -| `pushProtectedUpToAccessLevel` | [`ContainerRegistryProtectionRuleAccessLevel!`](#containerregistryprotectionruleaccesslevel) | Max GitLab access level unable to push a container image to the container registry. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | +| `pushProtectedUpToAccessLevel` | [`ContainerRegistryProtectionRuleAccessLevel!`](#containerregistryprotectionruleaccesslevel) | Max GitLab access level to prevent from pushing container images to the container registry. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | #### Fields @@ -15618,16 +15618,16 @@ A tag expiration policy designed to keep only the images that matter most. ### `ContainerRegistryProtectionRule` -A container registry protection rule designed to protect container repositories from being pushed by users with a certain access level. +A container registry protection rule designed to prevent users with a certain access level or lower from altering the container registry. #### Fields | Name | Type | Description | | ---- | ---- | ----------- | | `containerPathPattern` | [`String!`](#string) | Container repository path pattern protected by the protection rule. For example `@my-scope/my-container-*`. Wildcard character `*` allowed. | -| `deleteProtectedUpToAccessLevel` | [`ContainerRegistryProtectionRuleAccessLevel!`](#containerregistryprotectionruleaccesslevel) | Max GitLab access level unable to delete images to a container repository. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | +| `deleteProtectedUpToAccessLevel` | [`ContainerRegistryProtectionRuleAccessLevel!`](#containerregistryprotectionruleaccesslevel) | Max GitLab access level to prevent from pushing container images to the container registry. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | | `id` | [`ContainerRegistryProtectionRuleID!`](#containerregistryprotectionruleid) | ID of the container registry protection rule. | -| `pushProtectedUpToAccessLevel` | [`ContainerRegistryProtectionRuleAccessLevel!`](#containerregistryprotectionruleaccesslevel) | Max GitLab access level unable to push images to a container repository. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | +| `pushProtectedUpToAccessLevel` | [`ContainerRegistryProtectionRuleAccessLevel!`](#containerregistryprotectionruleaccesslevel) | Max GitLab access level to prevent from pushing container images to the container registry. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | ### `ContainerRepository` -- GitLab From 7e488c8e10be003a45815edc4e380991a8d17f78 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Thu, 26 Oct 2023 14:20:58 +0200 Subject: [PATCH 3/3] refactor: Apply suggestion from @wandering_person This commit includes: - Adjusting test case files with the feedback from @wandering_person --- .../protection/create_rule_service.rb | 2 +- .../protection/rule_access_level_enum_spec.rb | 2 +- .../protection/rule/create_spec.rb | 2 -- .../protection/create_rule_service_spec.rb | 31 +++++++++++++------ 4 files changed, 23 insertions(+), 14 deletions(-) diff --git a/app/services/container_registry/protection/create_rule_service.rb b/app/services/container_registry/protection/create_rule_service.rb index 038c644a509a30..34ec6f42b19107 100644 --- a/app/services/container_registry/protection/create_rule_service.rb +++ b/app/services/container_registry/protection/create_rule_service.rb @@ -19,7 +19,7 @@ def execute project.container_registry_protection_rules.create(params.slice(*ALLOWED_ATTRIBUTES)) unless container_registry_protection_rule.persisted? - return service_response_error(message: container_registry_protection_rule.errors.full_messages) + return service_response_error(message: container_registry_protection_rule.errors.full_messages.to_sentence) end ServiceResponse.success(payload: { container_registry_protection_rule: container_registry_protection_rule }) diff --git a/spec/graphql/types/container_registry/protection/rule_access_level_enum_spec.rb b/spec/graphql/types/container_registry/protection/rule_access_level_enum_spec.rb index 57ad3e50b21051..295401f89f95ea 100644 --- a/spec/graphql/types/container_registry/protection/rule_access_level_enum_spec.rb +++ b/spec/graphql/types/container_registry/protection/rule_access_level_enum_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe GitlabSchema.types['ContainerRegistryProtectionRuleAccessLevel'], feature_category: :package_registry do +RSpec.describe GitlabSchema.types['ContainerRegistryProtectionRuleAccessLevel'], feature_category: :container_registry do it 'exposes all options' do expect(described_class.values.keys).to match_array(%w[DEVELOPER MAINTAINER OWNER]) end diff --git a/spec/requests/api/graphql/mutations/container_registry/protection/rule/create_spec.rb b/spec/requests/api/graphql/mutations/container_registry/protection/rule/create_spec.rb index 5b59d90ff0ee65..0c708c3dc41483 100644 --- a/spec/requests/api/graphql/mutations/container_registry/protection/rule/create_spec.rb +++ b/spec/requests/api/graphql/mutations/container_registry/protection/rule/create_spec.rb @@ -138,8 +138,6 @@ end it 'does not create new container protection rules' do - # expect { subject }.to change { ::ContainerRegistry::Protection::Rule.count }.by(0) - expect(::ContainerRegistry::Protection::Rule.where(project: project, container_path_pattern: kwargs[:container_path_pattern], push_protected_up_to_access_level: Gitlab::Access::MAINTAINER)).not_to exist diff --git a/spec/services/container_registry/protection/create_rule_service_spec.rb b/spec/services/container_registry/protection/create_rule_service_spec.rb index 899d0cbb85c238..3c319caf25c820 100644 --- a/spec/services/container_registry/protection/create_rule_service_spec.rb +++ b/spec/services/container_registry/protection/create_rule_service_spec.rb @@ -66,14 +66,30 @@ it_behaves_like 'a successful service response' - it { expect { subject }.to change { ContainerRegistry::Protection::Rule.count }.from(0).to(1) } - context 'when fields are invalid' do - let(:params) do - super().merge(container_path_pattern: '', push_protected_up_to_access_level: 1000) + context 'when container_path_pattern is invalid' do + let(:params) { super().merge(container_path_pattern: '') } + + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes(message: match(/Container path pattern can't be blank/)) } end - it_behaves_like 'an erroneous service response' + context 'when delete_protected_up_to_access_level is invalid' do + let(:params) { super().merge(delete_protected_up_to_access_level: 1000) } + + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes(message: match(/is not a valid delete_protected_up_to_access_level/)) } + end + + context 'when push_protected_up_to_access_level is invalid' do + let(:params) { super().merge(push_protected_up_to_access_level: 1000) } + + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes(message: match(/is not a valid push_protected_up_to_access_level/)) } + end end context 'with existing container registry protection rule in the database' do @@ -92,8 +108,6 @@ end it_behaves_like 'a successful service response' - - it { expect { subject }.to change { ContainerRegistry::Protection::Rule.count }.from(1).to(2) } end context 'when field `container_path_pattern` is taken' do @@ -101,7 +115,6 @@ super().merge( container_path_pattern: existing_container_registry_protection_rule.container_path_pattern, push_protected_up_to_access_level: :maintainer - # existing_container_registry_protection_rule.push_protected_up_to_access_level ) end @@ -117,8 +130,6 @@ let(:params) { super().merge(project_id: 1, unsupported_param: 'unsupported_param_value') } it_behaves_like 'a successful service response' - - it { expect { subject }.to change { ContainerRegistry::Protection::Rule.count }.from(0).to(1) } end context 'with forbidden user access level (project developer role)' do -- GitLab