From 45d6989f1b43b997b4cb5b1cdbfcf5f6ac69a0e2 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Tue, 5 Mar 2024 18:34:19 +0100 Subject: [PATCH 1/3] Protected containers: Integrate delete protection in REST API - In GitLab v17.8, push protection for new container repositories has been released. - This commits adds the delete protection for container repositories - This commits has been implemented in the context of the following EPIC https://gitlab.com/groups/gitlab-org/-/epics/9825 - Introduces feature flag :container_registry_protected_containers_delete Changelog: added --- .../container_registry/protection/rule.rb | 10 ++ .../check_rule_existence_service.rb | 62 ++++++++ ...r_registry_protected_containers_delete.yml | 9 ++ lib/api/project_container_repositories.rb | 13 ++ .../protection/rule_spec.rb | 148 ++++++++++++++++++ .../project_container_repositories_spec.rb | 75 +++++++++ .../check_rule_existence_service_spec.rb | 107 +++++++++++++ 7 files changed, 424 insertions(+) create mode 100644 app/services/container_registry/protection/check_rule_existence_service.rb create mode 100644 config/feature_flags/gitlab_com_derisk/container_registry_protected_containers_delete.yml create mode 100644 spec/services/container_registry/protection/check_rule_existence_service_spec.rb diff --git a/app/models/container_registry/protection/rule.rb b/app/models/container_registry/protection/rule.rb index d677a47a4892e6..f1f06ff88adbd4 100644 --- a/app/models/container_registry/protection/rule.rb +++ b/app/models/container_registry/protection/rule.rb @@ -42,6 +42,16 @@ def self.for_push_exists?(access_level:, repository_path:) .exists? end + def self.for_action_exists?(action:, access_level:, repository_path:) + return false if [access_level, repository_path].any?(&:blank?) + + minimum_access_level_column = "minimum_access_level_for_#{action}" + + for_repository_path(repository_path) + .where(":access_level < #{minimum_access_level_column}", access_level: access_level) + .exists? + end + ## # Accepts a list of projects and repository paths and returns a result set # indicating whether the repository path is protected. diff --git a/app/services/container_registry/protection/check_rule_existence_service.rb b/app/services/container_registry/protection/check_rule_existence_service.rb new file mode 100644 index 00000000000000..e755a52a8c54ca --- /dev/null +++ b/app/services/container_registry/protection/check_rule_existence_service.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +module ContainerRegistry + module Protection + class CheckRuleExistenceService < BaseProjectService + SUCCESS_RESPONSE_RULE_EXISTS = ServiceResponse.success(payload: { protection_rule_exists?: true }).freeze + SUCCESS_RESPONSE_RULE_DOESNT_EXIST = ServiceResponse.success(payload: { protection_rule_exists?: false }).freeze + + ERROR_RESPONSE_UNAUTHORIZED = ServiceResponse.error(message: 'Unauthorized', reason: :unauthorized).freeze + + def self.for_delete(params:, **args) + new(params: params.merge(action: :delete), **args) + end + + def initialize(params:, **args) + raise(ArgumentError, 'Invalid param :action') unless params[:action].in?([:push, :delete]) + + super + end + + def execute + return ERROR_RESPONSE_UNAUTHORIZED unless current_user_can_do_action? + + return service_response_for(check_rule_exists_for_user) if current_user.is_a?(User) + return service_response_for(check_rule_exists_for_deploy_token) if current_user.is_a?(DeployToken) + + raise ArgumentError, 'Invalid user' + end + + private + + def current_user_can_do_action? + case params[:action] + when :push then can?(current_user, :create_container_image, project) + when :delete then can?(current_user, :destroy_container_image, project) + else false + end + end + + def check_rule_exists_for_user + return false if current_user.can_admin_all_resources? + + user_project_authorization_access_level = current_user.max_member_access_for_project(project.id) + project.container_registry_protection_rules.for_action_exists?( + action: params[:action], + access_level: user_project_authorization_access_level, + repository_path: params[:repository_path] + ) + end + + def check_rule_exists_for_deploy_token + project.container_registry_protection_rules + .for_repository_path(params[:repository_path]) + .exists? + end + + def service_response_for(protection_rule_exists) + protection_rule_exists ? SUCCESS_RESPONSE_RULE_EXISTS : SUCCESS_RESPONSE_RULE_DOESNT_EXIST + end + end + end +end diff --git a/config/feature_flags/gitlab_com_derisk/container_registry_protected_containers_delete.yml b/config/feature_flags/gitlab_com_derisk/container_registry_protected_containers_delete.yml new file mode 100644 index 00000000000000..07a05b8a601901 --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/container_registry_protected_containers_delete.yml @@ -0,0 +1,9 @@ +--- +name: container_registry_protected_containers_delete +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/406797 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/146686 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/517986 +milestone: '17.10' +group: group::container registry +type: gitlab_com_derisk +default_enabled: false diff --git a/lib/api/project_container_repositories.rb b/lib/api/project_container_repositories.rb index 3605ecb0b41aea..0d9914a0b91225 100644 --- a/lib/api/project_container_repositories.rb +++ b/lib/api/project_container_repositories.rb @@ -62,6 +62,19 @@ class ProjectContainerRepositories < ::API::Base end delete ':id/registry/repositories/:repository_id', requirements: REPOSITORY_ENDPOINT_REQUIREMENTS do authorize_admin_container_image! + + if Feature.enabled?(:container_registry_protected_containers_delete, user_project&.root_ancestor) && + !current_user.can_admin_all_resources? + + service_response = ContainerRegistry::Protection::CheckRuleExistenceService.for_delete( + current_user: current_user, + project: repository.project, + params: { repository_path: repository.path.to_s } + ).execute + + forbidden!('Deleting protected container repository forbidden.') if service_response[:protection_rule_exists?] + end + repository.delete_scheduled! track_package_event('delete_repository', :container, project: user_project, namespace: user_project.namespace) diff --git a/spec/models/container_registry/protection/rule_spec.rb b/spec/models/container_registry/protection/rule_spec.rb index f157bb13ecaaca..6cb4c4c6c25435 100644 --- a/spec/models/container_registry/protection/rule_spec.rb +++ b/spec/models/container_registry/protection/rule_spec.rb @@ -334,6 +334,154 @@ end end + describe '.for_action_exists?' do + let_it_be(:project1) { create(:project) } + let_it_be(:project_no_crpr) { create(:project) } + + let_it_be(:protection_rule_for_developer) do + create(:container_registry_protection_rule, + project: project1, + repository_path_pattern: "#{project1.full_path}/stage*", + minimum_access_level_for_delete: :owner, + minimum_access_level_for_push: :maintainer + ) + end + + let_it_be(:protection_rule_for_maintainer) do + create(:container_registry_protection_rule, + project: project1, + repository_path_pattern: "#{project1.full_path}/prod*", + minimum_access_level_for_delete: :owner, + minimum_access_level_for_push: :owner + ) + end + + let_it_be(:protection_rule_for_owner) do + create(:container_registry_protection_rule, + project: project1, + repository_path_pattern: "#{project1.full_path}/release*", + minimum_access_level_for_delete: :admin, + minimum_access_level_for_push: :admin + ) + end + + # Creating an identical container protection rule for the same project + # to ensure that overlapping rules are considered properly + let_it_be(:protection_rule_overlapping_for_developer) do + create(:container_registry_protection_rule, + project: project1, + repository_path_pattern: "#{project1.full_path}/stage-sha*", + minimum_access_level_for_delete: :owner, + minimum_access_level_for_push: :maintainer + ) + end + + let_it_be(:protection_rule_only_deletion_protection) do + create(:container_registry_protection_rule, + repository_path_pattern: "#{project1.full_path}/only-delete-protected*", + project: project1, + minimum_access_level_for_delete: :admin, + minimum_access_level_for_push: nil + ) + end + + let_it_be(:protection_rule_only_push_protection) do + create(:container_registry_protection_rule, + repository_path_pattern: "#{project1.full_path}/only-push-protected*", + project: project1, + minimum_access_level_for_delete: nil, + minimum_access_level_for_push: :admin + ) + end + + subject do + project + .container_registry_protection_rules + .for_action_exists?( + action: action, + access_level: access_level, + repository_path: repository_path + ) + end + + # rubocop:disable Layout/LineLength -- Avoid formatting to ensure one-line table syntax + where(:project, :action, :access_level, :repository_path, :protected?) do + ref(:project1) | :push | Gitlab::Access::REPORTER | lazy { "#{project.full_path}/stage-sha-1234" } | true + ref(:project1) | :push | Gitlab::Access::DEVELOPER | lazy { "#{project.full_path}/stage-sha-1234" } | true + ref(:project1) | :push | Gitlab::Access::MAINTAINER | lazy { "#{project.full_path}/stage-sha-1234" } | false + ref(:project1) | :push | Gitlab::Access::OWNER | lazy { "#{project.full_path}/stage-sha-1234" } | false + ref(:project1) | :push | Gitlab::Access::ADMIN | lazy { "#{project.full_path}/stage-sha-1234" } | false + ref(:project1) | :delete | Gitlab::Access::DEVELOPER | lazy { "#{project.full_path}/stage-sha-1234" } | true + ref(:project1) | :delete | Gitlab::Access::MAINTAINER | lazy { "#{project.full_path}/stage-sha-1234" } | true + ref(:project1) | :delete | Gitlab::Access::OWNER | lazy { "#{project.full_path}/stage-sha-1234" } | false + ref(:project1) | :delete | Gitlab::Access::ADMIN | lazy { "#{project.full_path}/stage-sha-1234" } | false + + ref(:project1) | :push | Gitlab::Access::MAINTAINER | lazy { "#{project.full_path}/prod-sha-1234" } | true + ref(:project1) | :push | Gitlab::Access::OWNER | lazy { "#{project.full_path}/prod-sha-1234" } | false + ref(:project1) | :push | Gitlab::Access::ADMIN | lazy { "#{project.full_path}/prod-sha-1234" } | false + ref(:project1) | :delete | Gitlab::Access::MAINTAINER | lazy { "#{project.full_path}/prod-sha-1234" } | true + ref(:project1) | :delete | Gitlab::Access::OWNER | lazy { "#{project.full_path}/prod-sha-1234" } | false + ref(:project1) | :delete | Gitlab::Access::ADMIN | lazy { "#{project.full_path}/prod-sha-1234" } | false + + ref(:project1) | :push | Gitlab::Access::MAINTAINER | lazy { "#{project.full_path}/release-v1" } | true + ref(:project1) | :push | Gitlab::Access::OWNER | lazy { "#{project.full_path}/release-v1" } | true + ref(:project1) | :push | Gitlab::Access::ADMIN | lazy { "#{project.full_path}/release-v1" } | false + ref(:project1) | :delete | Gitlab::Access::MAINTAINER | lazy { "#{project.full_path}/release-v1" } | true + ref(:project1) | :delete | Gitlab::Access::OWNER | lazy { "#{project.full_path}/release-v1" } | true + ref(:project1) | :delete | Gitlab::Access::ADMIN | lazy { "#{project.full_path}/release-v1" } | false + + ref(:project1) | :push | Gitlab::Access::DEVELOPER | lazy { "#{project.full_path}/only-delete-protected" } | false + ref(:project1) | :push | Gitlab::Access::MAINTAINER | lazy { "#{project.full_path}/only-delete-protected" } | false + ref(:project1) | :push | Gitlab::Access::OWNER | lazy { "#{project.full_path}/only-delete-protected" } | false + ref(:project1) | :push | Gitlab::Access::ADMIN | lazy { "#{project.full_path}/only-delete-protected" } | false + ref(:project1) | :push | Gitlab::Access::DEVELOPER | lazy { "#{project.full_path}/only-push-protected" } | true + ref(:project1) | :push | Gitlab::Access::MAINTAINER | lazy { "#{project.full_path}/only-push-protected" } | true + ref(:project1) | :push | Gitlab::Access::OWNER | lazy { "#{project.full_path}/only-push-protected" } | true + ref(:project1) | :push | Gitlab::Access::ADMIN | lazy { "#{project.full_path}/only-push-protected" } | false + ref(:project1) | :delete | Gitlab::Access::DEVELOPER | lazy { "#{project.full_path}/only-delete-protected" } | true + ref(:project1) | :delete | Gitlab::Access::MAINTAINER | lazy { "#{project.full_path}/only-delete-protected" } | true + ref(:project1) | :delete | Gitlab::Access::OWNER | lazy { "#{project.full_path}/only-delete-protected" } | true + ref(:project1) | :delete | Gitlab::Access::ADMIN | lazy { "#{project.full_path}/only-delete-protected" } | false + ref(:project1) | :delete | Gitlab::Access::DEVELOPER | lazy { "#{project.full_path}/only-push-protected" } | false + ref(:project1) | :delete | Gitlab::Access::MAINTAINER | lazy { "#{project.full_path}/only-push-protected" } | false + ref(:project1) | :delete | Gitlab::Access::OWNER | lazy { "#{project.full_path}/only-push-protected" } | false + ref(:project1) | :delete | Gitlab::Access::ADMIN | lazy { "#{project.full_path}/only-push-protected" } | false + + # For non-matching containers + ref(:project1) | :push | Gitlab::Access::DEVELOPER | lazy { "#{project.full_path}/any-suffix" } | false + ref(:project1) | :push | Gitlab::Access::NO_ACCESS | lazy { "#{project.full_path}/prod" } | true + ref(:project1) | :delete | Gitlab::Access::DEVELOPER | lazy { "#{project.full_path}/any-suffix" } | false + ref(:project1) | :delete | Gitlab::Access::NO_ACCESS | lazy { "#{project.full_path}/prod" } | true + + # Edge cases + ref(:project1) | :push | nil | lazy { "#{project.full_path}/stage-sha-1234" } | false + ref(:project1) | :push | Gitlab::Access::DEVELOPER | nil | false + ref(:project1) | :push | Gitlab::Access::DEVELOPER | '' | false + ref(:project1) | :push | nil | nil | false + + # For projects that have no container protection rules + ref(:project_no_crpr) | :push | Gitlab::Access::DEVELOPER | lazy { "#{project.full_path}/prod" } | false + ref(:project_no_crpr) | :push | Gitlab::Access::OWNER | lazy { "#{project.full_path}/prod" } | false + ref(:project_no_crpr) | :delete | Gitlab::Access::DEVELOPER | lazy { "#{project.full_path}/prod" } | false + ref(:project_no_crpr) | :delete | Gitlab::Access::OWNER | lazy { "#{project.full_path}/prod" } | false + end + # rubocop:enable Layout/LineLength + + with_them do + it { is_expected.to eq protected? } + end + + # context 'for invalid action' do + # let(:action) { :invalid_action } + # let(:access_level) { Gitlab::Access::DEVELOPER } + # let(:repository_path) { "#{project.full_path}/stage-sha-1234" } + + # it 'raises an error' do + # expect { for_action_exists_result }.to raise_error ActiveRecord::StatementInvalid + # end + # end + end + describe '.for_push_exists_for_projects_and_repository_paths' do let_it_be(:project1) { create(:project) } let_it_be(:project1_crpr) { create(:container_registry_protection_rule, project: project1) } diff --git a/spec/requests/api/project_container_repositories_spec.rb b/spec/requests/api/project_container_repositories_spec.rb index 91a5b31f4b3c20..cd244c25961338 100644 --- a/spec/requests/api/project_container_repositories_spec.rb +++ b/spec/requests/api/project_container_repositories_spec.rb @@ -116,6 +116,14 @@ let(:method) { :delete } let(:url) { "/projects/#{project.id}/registry/repositories/#{root_repository.id}" } + shared_examples 'destroying the container repository' do + it 'marks the repository as delete_scheduled' do + expect { subject }.to change { root_repository.reload.status }.from(nil).to('delete_scheduled') + + expect(response).to have_gitlab_http_status(:accepted) + end + end + ['using API user', 'using job token'].each do |context| context context do include_context context @@ -127,6 +135,8 @@ context 'for maintainer' do let(:api_user) { maintainer } + it_behaves_like 'destroying the container repository' + it 'marks the repository as delete_scheduled' do expect { subject }.to change { root_repository.reload.status }.from(nil).to('delete_scheduled') @@ -137,6 +147,71 @@ end include_examples 'rejected job token scopes' + + context 'with delete protection rule', :enable_admin_mode do + using RSpec::Parameterized::TableSyntax + + include_context 'using API user' + + let_it_be(:owner) { create(:user, owner_of: [project, project2]) } + let_it_be(:instance_admin) { create(:admin) } + + let_it_be_with_reload(:container_registry_protection_rule) do + create(:container_registry_protection_rule, project: project) + end + + let(:params) { { admin_mode: admin_mode } } + + before do + container_registry_protection_rule.update!( + repository_path_pattern: root_repository.path, + minimum_access_level_for_delete: minimum_access_level_for_delete + ) + end + + shared_examples 'protected deletion of container repository' do + it 'returns the expected status' do + subject + + expect(response).to have_gitlab_http_status(:forbidden) + end + + it 'returns error message' do + subject + + expect(json_response).to include('message' => '403 Forbidden - Deleting protected container repository forbidden.') + end + + context 'when feature flag :container_registry_protected_containers_delete is disabled' do + before do + stub_feature_flags(container_registry_protected_containers_delete: false) + end + + it_behaves_like 'destroying the container repository' + end + end + + where(:minimum_access_level_for_delete, :api_user, :admin_mode, :expected_shared_example) do + nil | ref(:maintainer) | false | 'destroying the container repository' + nil | ref(:owner) | false | 'destroying the container repository' + + :maintainer | ref(:maintainer) | false | 'destroying the container repository' + :maintainer | ref(:owner) | false | 'destroying the container repository' + :maintainer | ref(:instance_admin) | true | 'destroying the container repository' + + :owner | ref(:maintainer) | false | 'protected deletion of container repository' + :owner | ref(:owner) | false | 'destroying the container repository' + :owner | ref(:instance_admin) | true | 'destroying the container repository' + + :admin | ref(:maintainer) | false | 'protected deletion of container repository' + :admin | ref(:owner) | false | 'protected deletion of container repository' + :admin | ref(:instance_admin) | true | 'destroying the container repository' + end + + with_them do + it_behaves_like params[:expected_shared_example] + end + end end describe 'GET /projects/:id/registry/repositories/:repository_id/tags' do diff --git a/spec/services/container_registry/protection/check_rule_existence_service_spec.rb b/spec/services/container_registry/protection/check_rule_existence_service_spec.rb new file mode 100644 index 00000000000000..0cf2f37a8d4ded --- /dev/null +++ b/spec/services/container_registry/protection/check_rule_existence_service_spec.rb @@ -0,0 +1,107 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ContainerRegistry::Protection::CheckRuleExistenceService, feature_category: :package_registry do + using RSpec::Parameterized::TableSyntax + + let_it_be(:project) { create(:project) } + let_it_be(:unauthorized_user) { create(:user) } + let_it_be(:project_developer) { create(:user, developer_of: project) } + let_it_be(:project_maintainer) { create(:user, maintainer_of: project) } + let_it_be(:project_owner) { project.owner } + let_it_be(:instance_admin) { create(:admin) } + + let_it_be(:project_deploy_token) { create(:deploy_token, :all_scopes, projects: [project]) } + let_it_be(:other_deploy_token) { create(:deploy_token, :all_scopes) } + + let_it_be(:container_registry_protection_rule) do + create(:container_registry_protection_rule, + project: project, + repository_path_pattern: "#{project.full_path}/protected*", + minimum_access_level_for_push: :owner, + minimum_access_level_for_delete: :admin) + end + + let(:params) { { repository_path: repository_path_pattern, action: action } } + + let(:service) { described_class.new(project: project, current_user: current_user, params: params) } + + subject(:service_response) { service.execute } + + shared_examples 'protection rule exists' do + it_behaves_like 'returning a success service response' + it { is_expected.to have_attributes(payload: { protection_rule_exists?: true }) } + end + + shared_examples 'protection rule does not exist' do + it_behaves_like 'returning a success service response' + it { is_expected.to have_attributes(payload: { protection_rule_exists?: false }) } + end + + shared_examples 'error response for unauthorized actor' do + it_behaves_like 'returning an error service response', message: 'Unauthorized' + it { is_expected.to have_attributes reason: :unauthorized } + end + + shared_examples 'raising an error for invalid param :action' do + it 'raises an error' do + expect { service_response }.to raise_error(ArgumentError, 'Invalid param :action') + end + end + + describe '#execute', :enable_admin_mode do + # rubocop:disable Layout/LineLength -- Avoid formatting to ensure one-line table syntax + where(:action, :repository_path_pattern, :current_user, :expected_shared_example) do + :push | lazy { "#{project.full_path}/protected" } | ref(:project_developer) | 'protection rule exists' + :push | lazy { "#{project.full_path}/protected" } | ref(:project_maintainer) | 'protection rule exists' + :push | lazy { "#{project.full_path}/protected" } | ref(:project_owner) | 'protection rule does not exist' + :push | lazy { "#{project.full_path}/protected" } | ref(:instance_admin) | 'protection rule does not exist' + :push | lazy { "#{project.full_path}/protected" } | ref(:project_deploy_token) | 'protection rule exists' + :push | lazy { "#{project.full_path}/protected" } | ref(:other_deploy_token) | 'error response for unauthorized actor' + + :push | lazy { "other/#{project.full_path}/protected" } | ref(:project_developer) | 'protection rule does not exist' + :push | lazy { "other/#{project.full_path}/protected" } | ref(:project_owner) | 'protection rule does not exist' + :push | lazy { "other/#{project.full_path}/protected" } | ref(:project_deploy_token) | 'protection rule does not exist' + :push | lazy { "other/#{project.full_path}/protected" } | ref(:instance_admin) | 'protection rule does not exist' + + :delete | lazy { "#{project.full_path}/protected" } | ref(:project_developer) | 'protection rule exists' + :delete | lazy { "#{project.full_path}/protected" } | ref(:project_maintainer) | 'protection rule exists' + :delete | lazy { "#{project.full_path}/protected" } | ref(:project_owner) | 'protection rule exists' + :delete | lazy { "#{project.full_path}/protected" } | ref(:project_deploy_token) | 'error response for unauthorized actor' + :delete | lazy { "#{project.full_path}/protected" } | ref(:instance_admin) | 'protection rule does not exist' + + :delete | lazy { "other/#{project.full_path}/protected" } | ref(:project_maintainer) | 'protection rule does not exist' + :delete | lazy { "other/#{project.full_path}/protected" } | ref(:project_owner) | 'protection rule does not exist' + :delete | lazy { "other/#{project.full_path}/protected" } | ref(:project_deploy_token) | 'error response for unauthorized actor' + + # # Edge cases + :push | lazy { "#{project.full_path}/protected" } | ref(:unauthorized_user) | 'error response for unauthorized actor' + :push | lazy { "#{project.full_path}/protected" } | nil | 'error response for unauthorized actor' + :push | '' | ref(:project_developer) | 'protection rule does not exist' + :push | nil | ref(:project_developer) | 'protection rule does not exist' + :delete | lazy { "#{project.full_path}/protected" } | ref(:unauthorized_user) | 'error response for unauthorized actor' + :other | lazy { "#{project.full_path}/protected" } | ref(:project_developer) | 'raising an error for invalid param :action' + nil | lazy { "#{project.full_path}/protected" } | ref(:project_developer) | 'raising an error for invalid param :action' + end + # rubocop:enable Layout/LineLength + + with_them do + it_behaves_like params[:expected_shared_example] + end + + context 'for unexpected current_user' do + let(:current_user) { Object.new } + let(:action) { :push } + let(:repository_path_pattern) { "#{project.full_path}/protected" } + + before do + allow(service).to receive(:can?).and_return(true) + end + + it 'raises an error' do + expect { service_response }.to raise_error(ArgumentError, "Invalid user") + end + end + end +end -- GitLab From 8625ec25c04b28cdc89a6c1c13bb259c91ca591c Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Thu, 13 Mar 2025 17:39:15 +0100 Subject: [PATCH 2/3] refactor: Apply suggestions from @vk3g - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/183729#note_2391845096 - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/183729#note_2391845110 --- .../container_registry/protection/rule.rb | 1 + .../protection/rule_spec.rb | 19 ++++++++++--------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/app/models/container_registry/protection/rule.rb b/app/models/container_registry/protection/rule.rb index f1f06ff88adbd4..143be825f8d49f 100644 --- a/app/models/container_registry/protection/rule.rb +++ b/app/models/container_registry/protection/rule.rb @@ -44,6 +44,7 @@ def self.for_push_exists?(access_level:, repository_path:) def self.for_action_exists?(action:, access_level:, repository_path:) return false if [access_level, repository_path].any?(&:blank?) + raise ArgumentError, 'action must be :push or :delete' unless %i[push delete].include?(action) minimum_access_level_column = "minimum_access_level_for_#{action}" diff --git a/spec/models/container_registry/protection/rule_spec.rb b/spec/models/container_registry/protection/rule_spec.rb index 6cb4c4c6c25435..9a700fa762d33d 100644 --- a/spec/models/container_registry/protection/rule_spec.rb +++ b/spec/models/container_registry/protection/rule_spec.rb @@ -394,7 +394,7 @@ ) end - subject do + subject(:for_action_exists_result) do project .container_registry_protection_rules .for_action_exists?( @@ -471,15 +471,16 @@ it { is_expected.to eq protected? } end - # context 'for invalid action' do - # let(:action) { :invalid_action } - # let(:access_level) { Gitlab::Access::DEVELOPER } - # let(:repository_path) { "#{project.full_path}/stage-sha-1234" } + context 'for invalid action' do + let(:project) { project1 } + let(:action) { :invalid_action } + let(:access_level) { Gitlab::Access::DEVELOPER } + let(:repository_path) { "#{project.full_path}/stage-sha-1234" } - # it 'raises an error' do - # expect { for_action_exists_result }.to raise_error ActiveRecord::StatementInvalid - # end - # end + it 'raises an error' do + expect { for_action_exists_result }.to raise_error ArgumentError, 'action must be :push or :delete' + end + end end describe '.for_push_exists_for_projects_and_repository_paths' do -- GitLab From 7fc9e4af6769aae11513aea1d6ef0a2a58a5f909 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Mon, 17 Mar 2025 13:23:38 +0100 Subject: [PATCH 3/3] refactor: Apply suggestion from @ck3g - Fix undercoverage fir CheckRuleExistenceService, see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/183729#note_2397152393 --- .../protection/check_rule_existence_service.rb | 8 ++++---- .../protection/check_rule_existence_service_spec.rb | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/services/container_registry/protection/check_rule_existence_service.rb b/app/services/container_registry/protection/check_rule_existence_service.rb index e755a52a8c54ca..e2065eff91210e 100644 --- a/app/services/container_registry/protection/check_rule_existence_service.rb +++ b/app/services/container_registry/protection/check_rule_existence_service.rb @@ -30,10 +30,10 @@ def execute private def current_user_can_do_action? - case params[:action] - when :push then can?(current_user, :create_container_image, project) - when :delete then can?(current_user, :destroy_container_image, project) - else false + if params[:action] == :push + can?(current_user, :create_container_image, project) + else + can?(current_user, :destroy_container_image, project) end end diff --git a/spec/services/container_registry/protection/check_rule_existence_service_spec.rb b/spec/services/container_registry/protection/check_rule_existence_service_spec.rb index 0cf2f37a8d4ded..d273f24ec99529 100644 --- a/spec/services/container_registry/protection/check_rule_existence_service_spec.rb +++ b/spec/services/container_registry/protection/check_rule_existence_service_spec.rb @@ -53,7 +53,7 @@ describe '#execute', :enable_admin_mode do # rubocop:disable Layout/LineLength -- Avoid formatting to ensure one-line table syntax where(:action, :repository_path_pattern, :current_user, :expected_shared_example) do - :push | lazy { "#{project.full_path}/protected" } | ref(:project_developer) | 'protection rule exists' + :push | lazy { "#{project.full_path}/protected" } | ref(:project_developer) | 'protection rule exists' :push | lazy { "#{project.full_path}/protected" } | ref(:project_maintainer) | 'protection rule exists' :push | lazy { "#{project.full_path}/protected" } | ref(:project_owner) | 'protection rule does not exist' :push | lazy { "#{project.full_path}/protected" } | ref(:instance_admin) | 'protection rule does not exist' -- GitLab