diff --git a/app/models/container_registry/protection/rule.rb b/app/models/container_registry/protection/rule.rb index d677a47a4892e6708978e85d8334f5d7faf8c0ba..143be825f8d49f88a41fd51fcd3c6d12d06e13bc 100644 --- a/app/models/container_registry/protection/rule.rb +++ b/app/models/container_registry/protection/rule.rb @@ -42,6 +42,17 @@ 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?) + raise ArgumentError, 'action must be :push or :delete' unless %i[push delete].include?(action) + + 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 0000000000000000000000000000000000000000..e2065eff91210eadd40f5d541242e3d06e5f2ae2 --- /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? + if params[:action] == :push + can?(current_user, :create_container_image, project) + else + can?(current_user, :destroy_container_image, project) + 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 0000000000000000000000000000000000000000..07a05b8a6019019f62c60ae24c11d858f5cb69bc --- /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 3605ecb0b41aea7232722c55e3654fe0aad4aa17..0d9914a0b9122503530818059e15f989b27699e7 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 f157bb13ecaacab659e620179904dc0f17006c4a..9a700fa762d33d9449c570d5bc70f00728ecd0bb 100644 --- a/spec/models/container_registry/protection/rule_spec.rb +++ b/spec/models/container_registry/protection/rule_spec.rb @@ -334,6 +334,155 @@ 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(:for_action_exists_result) 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(: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 ArgumentError, 'action must be :push or :delete' + 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 91a5b31f4b3c204dd853b0311a3936163e67df5c..cd244c259613388a5656878505fb8b0fe872bb92 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 0000000000000000000000000000000000000000..d273f24ec99529a77fff98468015facf076f6b68 --- /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