From 31f06cab37f7c855f81f22133a793c4db793d05a Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Thu, 30 Jan 2025 18:03:43 +0100 Subject: [PATCH 1/4] Protected packages: Add basics for package delete protection This MR introduces the basics for the package delete protection, e.g. database migration, data model, etc. The scope of the basic functionality was discussed in the epic, see https://gitlab.com/groups/gitlab-org/-/epics/5574 Changelog: added --- app/models/packages/protection/rule.rb | 36 ++++- ...for_delete_to_packages_protection_rules.rb | 9 ++ ...aint_from_mininum_access_level_for_push.rb | 15 ++ ..._protection_rules_minimum_access_levels.rb | 25 +++ db/schema_migrations/20250130161447 | 1 + db/schema_migrations/20250130162000 | 1 + db/schema_migrations/20250130162633 | 1 + db/structure.sql | 3 +- spec/factories/packages/protection/rules.rb | 1 + spec/models/packages/protection/rule_spec.rb | 143 ++++++++++++------ 10 files changed, 187 insertions(+), 48 deletions(-) create mode 100644 db/migrate/20250130161447_add_minimum_access_level_for_delete_to_packages_protection_rules.rb create mode 100644 db/migrate/20250130162000_remove_not_null_constraint_from_mininum_access_level_for_push.rb create mode 100644 db/migrate/20250130162633_add_multi_not_null_constraint_on_packages_protection_rules_minimum_access_levels.rb create mode 100644 db/schema_migrations/20250130161447 create mode 100644 db/schema_migrations/20250130162000 create mode 100644 db/schema_migrations/20250130162633 diff --git a/app/models/packages/protection/rule.rb b/app/models/packages/protection/rule.rb index fafcbd025d7114..de665b32d90cd1 100644 --- a/app/models/packages/protection/rule.rb +++ b/app/models/packages/protection/rule.rb @@ -4,6 +4,9 @@ module Packages module Protection class Rule < ApplicationRecord enum package_type: Packages::Package.package_types.slice(:conan, :maven, :npm, :pypi) + enum minimum_access_level_for_delete: + Gitlab::Access.sym_options_with_admin.slice(:maintainer, :owner, :admin), + _prefix: :minimum_access_level_for_delete enum minimum_access_level_for_push: Gitlab::Access.sym_options_with_admin.slice(:maintainer, :owner, :admin), _prefix: :minimum_access_level_for_push @@ -25,7 +28,8 @@ class Rule < ApplicationRecord }, if: :pypi? validates :package_type, presence: true - validates :minimum_access_level_for_push, presence: true + + validate :at_least_one_minimum_access_level_must_be_present scope :for_package_name, ->(package_name) do return none if package_name.blank? @@ -39,10 +43,32 @@ class Rule < ApplicationRecord scope :for_package_type, ->(package_type) { where(package_type: package_type) } def self.for_push_exists?(access_level:, package_name:, package_type:) + for_action_exists?( + minimum_access_level_column: :minimum_access_level_for_push, + access_level: access_level, + package_name: package_name, + package_type: package_type + ) + end + + def self.for_delete_exists?(access_level:, package_name:, package_type:) + for_action_exists?( + minimum_access_level_column: :minimum_access_level_for_delete, + access_level: access_level, + package_name: package_name, + package_type: package_type + ) + end + + private_class_method def self.for_action_exists?( + minimum_access_level_column:, + access_level:, + package_name:, + package_type:) return false if [access_level, package_name, package_type].any?(&:blank?) for_package_type(package_type) - .where(':access_level < minimum_access_level_for_push', access_level: access_level) + .where(":access_level < #{minimum_access_level_column}", access_level: access_level) .for_package_name(package_name) .exists? end @@ -113,6 +139,12 @@ def self.for_push_exists_for_projects_and_packages(projects_and_packages) connection.exec_query(query.with(cte.to_arel).to_sql) end + + def at_least_one_minimum_access_level_must_be_present + return unless minimum_access_level_for_delete.blank? && minimum_access_level_for_push.blank? + + errors.add(:base, _('A rule must have at least a minimum access role for push or delete.')) + end end end end diff --git a/db/migrate/20250130161447_add_minimum_access_level_for_delete_to_packages_protection_rules.rb b/db/migrate/20250130161447_add_minimum_access_level_for_delete_to_packages_protection_rules.rb new file mode 100644 index 00000000000000..abf9739e36d342 --- /dev/null +++ b/db/migrate/20250130161447_add_minimum_access_level_for_delete_to_packages_protection_rules.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddMinimumAccessLevelForDeleteToPackagesProtectionRules < Gitlab::Database::Migration[2.2] + milestone '17.9' + + def change + add_column :packages_protection_rules, :minimum_access_level_for_delete, :smallint, null: true, if_not_exists: true + end +end diff --git a/db/migrate/20250130162000_remove_not_null_constraint_from_mininum_access_level_for_push.rb b/db/migrate/20250130162000_remove_not_null_constraint_from_mininum_access_level_for_push.rb new file mode 100644 index 00000000000000..7dadc63fcc2f4e --- /dev/null +++ b/db/migrate/20250130162000_remove_not_null_constraint_from_mininum_access_level_for_push.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class RemoveNotNullConstraintFromMininumAccessLevelForPush < Gitlab::Database::Migration[2.2] + milestone '17.9' + + disable_ddl_transaction! + + def up + remove_not_null_constraint :packages_protection_rules, :minimum_access_level_for_push + end + + def down + add_not_null_constraint :packages_protection_rules, :minimum_access_level_for_push + end +end diff --git a/db/migrate/20250130162633_add_multi_not_null_constraint_on_packages_protection_rules_minimum_access_levels.rb b/db/migrate/20250130162633_add_multi_not_null_constraint_on_packages_protection_rules_minimum_access_levels.rb new file mode 100644 index 00000000000000..75e80b2cf3ab04 --- /dev/null +++ b/db/migrate/20250130162633_add_multi_not_null_constraint_on_packages_protection_rules_minimum_access_levels.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class AddMultiNotNullConstraintOnPackagesProtectionRulesMinimumAccessLevels < Gitlab::Database::Migration[2.2] + milestone '17.9' + + disable_ddl_transaction! + + TABLE_NAME = :packages_protection_rules + + def up + add_multi_column_not_null_constraint(TABLE_NAME, + :minimum_access_level_for_push, + :minimum_access_level_for_delete, + operator: '>', + limit: 0 + ) + end + + def down + remove_multi_column_not_null_constraint(TABLE_NAME, + :minimum_access_level_for_push, + :minimum_access_level_for_delete + ) + end +end diff --git a/db/schema_migrations/20250130161447 b/db/schema_migrations/20250130161447 new file mode 100644 index 00000000000000..32a588d12c1046 --- /dev/null +++ b/db/schema_migrations/20250130161447 @@ -0,0 +1 @@ +eccf6685d15f1f4b585257e7b7e8ec84e9bf0a334a9524f1da3b165357cae48e \ No newline at end of file diff --git a/db/schema_migrations/20250130162000 b/db/schema_migrations/20250130162000 new file mode 100644 index 00000000000000..6d08b504f4effb --- /dev/null +++ b/db/schema_migrations/20250130162000 @@ -0,0 +1 @@ +bec94a6bc401509b2c95bcc81c30410bd278618be2de21ae5b437f9ad2324283 \ No newline at end of file diff --git a/db/schema_migrations/20250130162633 b/db/schema_migrations/20250130162633 new file mode 100644 index 00000000000000..28caf79d6ce4f6 --- /dev/null +++ b/db/schema_migrations/20250130162633 @@ -0,0 +1 @@ +b125cbe451b64dccd4a329af90efc785d37b3df5dbae7c27d9b473c762c10805 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 595cdcbbc25edc..dfbbeabd0a3f4c 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -18346,7 +18346,8 @@ CREATE TABLE packages_protection_rules ( package_type smallint NOT NULL, package_name_pattern text NOT NULL, minimum_access_level_for_push smallint, - CONSTRAINT check_12e13c202a CHECK ((minimum_access_level_for_push IS NOT NULL)), + minimum_access_level_for_delete smallint, + CONSTRAINT check_520a0596a3 CHECK ((num_nonnulls(minimum_access_level_for_delete, minimum_access_level_for_push) > 0)), CONSTRAINT check_d2d75d206d CHECK ((char_length(package_name_pattern) <= 255)) ); diff --git a/spec/factories/packages/protection/rules.rb b/spec/factories/packages/protection/rules.rb index feccee72a8d0ad..8e67633b28faf8 100644 --- a/spec/factories/packages/protection/rules.rb +++ b/spec/factories/packages/protection/rules.rb @@ -5,6 +5,7 @@ project package_name_pattern { '@my_scope/my_package' } package_type { :npm } + minimum_access_level_for_delete { :maintainer } minimum_access_level_for_push { :maintainer } end end diff --git a/spec/models/packages/protection/rule_spec.rb b/spec/models/packages/protection/rule_spec.rb index d0610de18c43a5..050c287a790e1c 100644 --- a/spec/models/packages/protection/rule_spec.rb +++ b/spec/models/packages/protection/rule_spec.rb @@ -93,8 +93,32 @@ it { is_expected.to validate_presence_of(:package_type) } end - describe '#minimum_access_level_for_push' do - it { is_expected.to validate_presence_of(:minimum_access_level_for_push) } + describe '#at_least_one_minimum_access_level_must_be_present' do + where(:minimum_access_level_for_delete, :minimum_access_level_for_push, :valid) do + :maintainer | :maintainer | true + :maintainer | nil | true + nil | :maintainer | true + nil | nil | false + end + + with_them do + subject(:package_protection_rule) do + build(:package_protection_rule, + minimum_access_level_for_delete: minimum_access_level_for_delete, + minimum_access_level_for_push: minimum_access_level_for_push + ) + end + + if params[:valid] + it { is_expected.to be_valid } + else + it 'is invalid' do + expect(package_protection_rule).not_to be_valid + expect(package_protection_rule.errors[:base]) + .to include('A rule must have at least a minimum access role for push or delete.') + end + end + end end end @@ -209,7 +233,7 @@ end end - describe '.for_push_exists?' do + describe '.for_push_exists? and .for_delete_exists?' do let_it_be(:project_with_ppr) { create(:project) } let_it_be(:project_without_ppr) { create(:project) } @@ -218,6 +242,7 @@ package_name_pattern: '@my-scope/my-package-stage*', project: project_with_ppr, package_type: :npm, + minimum_access_level_for_delete: :maintainer, minimum_access_level_for_push: :maintainer ) end @@ -227,84 +252,112 @@ package_name_pattern: '@my-scope/my-package-prod*', project: project_with_ppr, package_type: :npm, + minimum_access_level_for_delete: :owner, minimum_access_level_for_push: :owner ) end - let_it_be(:ppr_owner) do + let_it_be(:ppr_for_owner) do create(:package_protection_rule, package_name_pattern: '@my-scope/my-package-release*', project: project_with_ppr, package_type: :npm, + minimum_access_level_for_delete: :admin, minimum_access_level_for_push: :admin ) end - let_it_be(:ppr_2_for_developer) do + let_it_be(:ppr_for_developer_2) do create(:package_protection_rule, package_name_pattern: '@my-scope/my-package-*', project: project_with_ppr, package_type: :npm, + minimum_access_level_for_delete: :maintainer, minimum_access_level_for_push: :maintainer ) end - subject do - project - .package_protection_rules - .for_push_exists?( - access_level: access_level, - package_name: package_name, - package_type: package_type - ) + let_it_be(:ppr_only_deletion_protection) do + create(:package_protection_rule, + package_name_pattern: '@my-scope/my-package-only-delete-protected*', + project: project_with_ppr, + package_type: :npm, + minimum_access_level_for_delete: :admin, + minimum_access_level_for_push: :maintainer + ) end describe 'with different users and protection levels' do - where(:project, :access_level, :package_name, :package_type, :push_protected) do - ref(:project_with_ppr) | Gitlab::Access::REPORTER | '@my-scope/my-package-stage-sha-1234' | :npm | true - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-stage-sha-1234' | :npm | true - ref(:project_with_ppr) | Gitlab::Access::MAINTAINER | '@my-scope/my-package-stage-sha-1234' | :npm | false - ref(:project_with_ppr) | Gitlab::Access::MAINTAINER | '@my-scope/my-package-stage-sha-1234' | :npm | false - ref(:project_with_ppr) | Gitlab::Access::OWNER | '@my-scope/my-package-stage-sha-1234' | :npm | false - ref(:project_with_ppr) | Gitlab::Access::ADMIN | '@my-scope/my-package-stage-sha-1234' | :npm | false - - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-prod-sha-1234' | :npm | true - ref(:project_with_ppr) | Gitlab::Access::MAINTAINER | '@my-scope/my-package-prod-sha-1234' | :npm | true - ref(:project_with_ppr) | Gitlab::Access::OWNER | '@my-scope/my-package-prod-sha-1234' | :npm | false - ref(:project_with_ppr) | Gitlab::Access::ADMIN | '@my-scope/my-package-prod-sha-1234' | :npm | false - - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-release-v1' | :npm | true - ref(:project_with_ppr) | Gitlab::Access::OWNER | '@my-scope/my-package-release-v1' | :npm | true - ref(:project_with_ppr) | Gitlab::Access::ADMIN | '@my-scope/my-package-release-v1' | :npm | false - - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :npm | true - ref(:project_with_ppr) | Gitlab::Access::MAINTAINER | '@my-scope/my-package-any-suffix' | :npm | false - ref(:project_with_ppr) | Gitlab::Access::OWNER | '@my-scope/my-package-any-suffix' | :npm | false + # rubocop:disable Layout/LineLength -- Avoid formatting to ensure one-line table syntax + where(:project, :access_level, :package_name, :package_type, :push_protected, :deletion_protected) do + ref(:project_with_ppr) | Gitlab::Access::REPORTER | '@my-scope/my-package-stage-sha-1234' | :npm | true | true + ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-stage-sha-1234' | :npm | true | true + ref(:project_with_ppr) | Gitlab::Access::MAINTAINER | '@my-scope/my-package-stage-sha-1234' | :npm | false | false + ref(:project_with_ppr) | Gitlab::Access::MAINTAINER | '@my-scope/my-package-stage-sha-1234' | :npm | false | false + ref(:project_with_ppr) | Gitlab::Access::OWNER | '@my-scope/my-package-stage-sha-1234' | :npm | false | false + ref(:project_with_ppr) | Gitlab::Access::ADMIN | '@my-scope/my-package-stage-sha-1234' | :npm | false | false + + ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-prod-sha-1234' | :npm | true | true + ref(:project_with_ppr) | Gitlab::Access::MAINTAINER | '@my-scope/my-package-prod-sha-1234' | :npm | true | true + ref(:project_with_ppr) | Gitlab::Access::OWNER | '@my-scope/my-package-prod-sha-1234' | :npm | false | false + ref(:project_with_ppr) | Gitlab::Access::ADMIN | '@my-scope/my-package-prod-sha-1234' | :npm | false | false + + ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-release-v1' | :npm | true | true + ref(:project_with_ppr) | Gitlab::Access::OWNER | '@my-scope/my-package-release-v1' | :npm | true | true + ref(:project_with_ppr) | Gitlab::Access::ADMIN | '@my-scope/my-package-release-v1' | :npm | false | false + + ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :npm | true | true + ref(:project_with_ppr) | Gitlab::Access::MAINTAINER | '@my-scope/my-package-any-suffix' | :npm | false | false + ref(:project_with_ppr) | Gitlab::Access::OWNER | '@my-scope/my-package-any-suffix' | :npm | false | false + + ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-only-delete-protected' | :npm | true | true + ref(:project_with_ppr) | Gitlab::Access::MAINTAINER | '@my-scope/my-package-only-delete-protected' | :npm | false | true + ref(:project_with_ppr) | Gitlab::Access::OWNER | '@my-scope/my-package-only-delete-protected' | :npm | false | true + ref(:project_with_ppr) | Gitlab::Access::ADMIN | '@my-scope/my-package-only-delete-protected' | :npm | false | false # For non-matching package_name - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/non-matching-package' | :npm | false + ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/non-matching-package' | :npm | false | false # For non-matching package_type - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :conan | false + ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :conan | false | false # For no access level - ref(:project_with_ppr) | Gitlab::Access::NO_ACCESS | '@my-scope/my-package-prod' | :npm | true + ref(:project_with_ppr) | Gitlab::Access::NO_ACCESS | '@my-scope/my-package-prod' | :npm | true | true # Edge cases - ref(:project_with_ppr) | nil | '@my-scope/my-package-stage-sha-1234' | :npm | false - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | nil | :npm | false - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '' | :npm | false - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-stage-sha-1234' | nil | false - ref(:project_with_ppr) | nil | nil | nil | false + ref(:project_with_ppr) | nil | '@my-scope/my-package-stage-sha-1234' | :npm | false | false + ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | nil | :npm | false | false + ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '' | :npm | false | false + ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-stage-sha-1234' | nil | false | false + ref(:project_with_ppr) | nil | nil | nil | false | false # For projects that have no package protection rules - ref(:project_without_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-prod' | :npm | false - ref(:project_without_ppr) | Gitlab::Access::MAINTAINER | '@my-scope/my-package-prod' | :npm | false - ref(:project_without_ppr) | Gitlab::Access::OWNER | '@my-scope/my-package-prod' | :npm | false + ref(:project_without_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-prod' | :npm | false | false + ref(:project_without_ppr) | Gitlab::Access::MAINTAINER | '@my-scope/my-package-prod' | :npm | false | false + ref(:project_without_ppr) | Gitlab::Access::OWNER | '@my-scope/my-package-prod' | :npm | false | false end + # rubocop:enable Layout/LineLength with_them do - it { is_expected.to eq push_protected } + describe '.for_push_exists?' do + subject do + project + .package_protection_rules + .for_push_exists?(access_level: access_level, package_name: package_name, package_type: package_type) + end + + it { is_expected.to eq push_protected } + end + + describe '.for_delete_exists?' do + subject do + project + .package_protection_rules + .for_delete_exists?(access_level: access_level, package_name: package_name, package_type: package_type) + end + + it { is_expected.to eq deletion_protected } + end end end end -- GitLab From 51337de36b8803b7b8f7fa01ba39ed35509cf499 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Thu, 6 Feb 2025 18:10:14 +0100 Subject: [PATCH 2/4] refactor: Apply suggestion from @10io - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179739#note_2334019884 --- app/models/packages/protection/rule.rb | 26 +-- .../check_rule_existence_service.rb | 3 +- spec/models/packages/protection/rule_spec.rb | 173 ++++++++++-------- 3 files changed, 105 insertions(+), 97 deletions(-) diff --git a/app/models/packages/protection/rule.rb b/app/models/packages/protection/rule.rb index de665b32d90cd1..c428cbcd52c2d3 100644 --- a/app/models/packages/protection/rule.rb +++ b/app/models/packages/protection/rule.rb @@ -42,31 +42,11 @@ class Rule < ApplicationRecord scope :for_package_type, ->(package_type) { where(package_type: package_type) } - def self.for_push_exists?(access_level:, package_name:, package_type:) - for_action_exists?( - minimum_access_level_column: :minimum_access_level_for_push, - access_level: access_level, - package_name: package_name, - package_type: package_type - ) - end - - def self.for_delete_exists?(access_level:, package_name:, package_type:) - for_action_exists?( - minimum_access_level_column: :minimum_access_level_for_delete, - access_level: access_level, - package_name: package_name, - package_type: package_type - ) - end - - private_class_method def self.for_action_exists?( - minimum_access_level_column:, - access_level:, - package_name:, - package_type:) + def self.for_action_exists?(action:, access_level:, package_name:, package_type:) return false if [access_level, package_name, package_type].any?(&:blank?) + minimum_access_level_column = "minimum_access_level_for_#{action}" + for_package_type(package_type) .where(":access_level < #{minimum_access_level_column}", access_level: access_level) .for_package_name(package_name) diff --git a/app/services/packages/protection/check_rule_existence_service.rb b/app/services/packages/protection/check_rule_existence_service.rb index 1af5ec0621e141..8a705e3bda0c36 100644 --- a/app/services/packages/protection/check_rule_existence_service.rb +++ b/app/services/packages/protection/check_rule_existence_service.rb @@ -35,7 +35,8 @@ def check_rule_exists_for_user user_project_authorization_access_level = current_user.max_member_access_for_project(project.id) project.package_protection_rules - .for_push_exists?( + .for_action_exists?( + action: :push, access_level: user_project_authorization_access_level, package_name: params[:package_name], package_type: params[:package_type] diff --git a/spec/models/packages/protection/rule_spec.rb b/spec/models/packages/protection/rule_spec.rb index 050c287a790e1c..ae9eee72702b52 100644 --- a/spec/models/packages/protection/rule_spec.rb +++ b/spec/models/packages/protection/rule_spec.rb @@ -233,7 +233,7 @@ end end - describe '.for_push_exists? and .for_delete_exists?' do + describe '.for_action_exists?' do let_it_be(:project_with_ppr) { create(:project) } let_it_be(:project_without_ppr) { create(:project) } @@ -267,7 +267,7 @@ ) end - let_it_be(:ppr_for_developer_2) do + let_it_be(:ppr_2_for_developer) do create(:package_protection_rule, package_name_pattern: '@my-scope/my-package-*', project: project_with_ppr, @@ -279,86 +279,113 @@ let_it_be(:ppr_only_deletion_protection) do create(:package_protection_rule, - package_name_pattern: '@my-scope/my-package-only-delete-protected*', + package_name_pattern: '@my-scope/only_delete-protected-package*', project: project_with_ppr, package_type: :npm, minimum_access_level_for_delete: :admin, - minimum_access_level_for_push: :maintainer + minimum_access_level_for_push: nil ) end - describe 'with different users and protection levels' do - # rubocop:disable Layout/LineLength -- Avoid formatting to ensure one-line table syntax - where(:project, :access_level, :package_name, :package_type, :push_protected, :deletion_protected) do - ref(:project_with_ppr) | Gitlab::Access::REPORTER | '@my-scope/my-package-stage-sha-1234' | :npm | true | true - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-stage-sha-1234' | :npm | true | true - ref(:project_with_ppr) | Gitlab::Access::MAINTAINER | '@my-scope/my-package-stage-sha-1234' | :npm | false | false - ref(:project_with_ppr) | Gitlab::Access::MAINTAINER | '@my-scope/my-package-stage-sha-1234' | :npm | false | false - ref(:project_with_ppr) | Gitlab::Access::OWNER | '@my-scope/my-package-stage-sha-1234' | :npm | false | false - ref(:project_with_ppr) | Gitlab::Access::ADMIN | '@my-scope/my-package-stage-sha-1234' | :npm | false | false - - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-prod-sha-1234' | :npm | true | true - ref(:project_with_ppr) | Gitlab::Access::MAINTAINER | '@my-scope/my-package-prod-sha-1234' | :npm | true | true - ref(:project_with_ppr) | Gitlab::Access::OWNER | '@my-scope/my-package-prod-sha-1234' | :npm | false | false - ref(:project_with_ppr) | Gitlab::Access::ADMIN | '@my-scope/my-package-prod-sha-1234' | :npm | false | false - - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-release-v1' | :npm | true | true - ref(:project_with_ppr) | Gitlab::Access::OWNER | '@my-scope/my-package-release-v1' | :npm | true | true - ref(:project_with_ppr) | Gitlab::Access::ADMIN | '@my-scope/my-package-release-v1' | :npm | false | false - - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :npm | true | true - ref(:project_with_ppr) | Gitlab::Access::MAINTAINER | '@my-scope/my-package-any-suffix' | :npm | false | false - ref(:project_with_ppr) | Gitlab::Access::OWNER | '@my-scope/my-package-any-suffix' | :npm | false | false - - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-only-delete-protected' | :npm | true | true - ref(:project_with_ppr) | Gitlab::Access::MAINTAINER | '@my-scope/my-package-only-delete-protected' | :npm | false | true - ref(:project_with_ppr) | Gitlab::Access::OWNER | '@my-scope/my-package-only-delete-protected' | :npm | false | true - ref(:project_with_ppr) | Gitlab::Access::ADMIN | '@my-scope/my-package-only-delete-protected' | :npm | false | false - - # For non-matching package_name - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/non-matching-package' | :npm | false | false - - # For non-matching package_type - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :conan | false | false - - # For no access level - ref(:project_with_ppr) | Gitlab::Access::NO_ACCESS | '@my-scope/my-package-prod' | :npm | true | true - - # Edge cases - ref(:project_with_ppr) | nil | '@my-scope/my-package-stage-sha-1234' | :npm | false | false - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | nil | :npm | false | false - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '' | :npm | false | false - ref(:project_with_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-stage-sha-1234' | nil | false | false - ref(:project_with_ppr) | nil | nil | nil | false | false - - # For projects that have no package protection rules - ref(:project_without_ppr) | Gitlab::Access::DEVELOPER | '@my-scope/my-package-prod' | :npm | false | false - ref(:project_without_ppr) | Gitlab::Access::MAINTAINER | '@my-scope/my-package-prod' | :npm | false | false - ref(:project_without_ppr) | Gitlab::Access::OWNER | '@my-scope/my-package-prod' | :npm | false | false - end - # rubocop:enable Layout/LineLength - - with_them do - describe '.for_push_exists?' do - subject do - project - .package_protection_rules - .for_push_exists?(access_level: access_level, package_name: package_name, package_type: package_type) - end + let_it_be(:ppr_only_push_protection) do + create(:package_protection_rule, + package_name_pattern: '@my-scope/only-push-protected-package*', + project: project_with_ppr, + package_type: :npm, + minimum_access_level_for_delete: nil, + minimum_access_level_for_push: :admin + ) + end - it { is_expected.to eq push_protected } - end + subject do + project + .package_protection_rules + .for_action_exists?( + action: action, + access_level: access_level, + package_name: package_name, + package_type: package_type + ) + end - describe '.for_delete_exists?' do - subject do - project - .package_protection_rules - .for_delete_exists?(access_level: access_level, package_name: package_name, package_type: package_type) - end + # rubocop:disable Layout/LineLength -- Avoid formatting to ensure one-line table syntax + where(:project, :action, :access_level, :package_name, :package_type, :protected?) do + ref(:project_with_ppr) | :push | Gitlab::Access::REPORTER | '@my-scope/my-package-stage-sha-1234' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/my-package-stage-sha-1234' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/my-package-stage-sha-1234' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/my-package-stage-sha-1234' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/my-package-stage-sha-1234' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::REPORTER | '@my-scope/my-package-stage-sha-1234' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/my-package-stage-sha-1234' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/my-package-stage-sha-1234' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/my-package-stage-sha-1234' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/my-package-stage-sha-1234' | :npm | false + + ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/my-package-prod-sha-1234' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/my-package-prod-sha-1234' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/my-package-prod-sha-1234' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/my-package-prod-sha-1234' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/my-package-prod-sha-1234' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/my-package-prod-sha-1234' | :npm | false + + ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/my-package-release-v1' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/my-package-release-v1' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/my-package-release-v1' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/my-package-release-v1' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/my-package-release-v1' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/my-package-release-v1' | :npm | false + + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/my-package-any-suffix' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/my-package-any-suffix' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/my-package-any-suffix' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/my-package-any-suffix' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/my-package-any-suffix' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/my-package-any-suffix' | :npm | false + + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/only_delete-protected-package' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/only_delete-protected-package' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/only_delete-protected-package' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/only_delete-protected-package' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/only-push-protected-package' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/only-push-protected-package' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/only-push-protected-package' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/only-push-protected-package' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/only_delete-protected-package' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/only_delete-protected-package' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/only_delete-protected-package' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/only_delete-protected-package' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/only-push-protected-package' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/only-push-protected-package' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/only-push-protected-package' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/only-push-protected-package' | :npm | false + + # For non-matching packages + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/non-matching-package' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/non-matching-package' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :conan | false + ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :conan | false + ref(:project_with_ppr) | :push | Gitlab::Access::NO_ACCESS | '@my-scope/my-package-prod' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::NO_ACCESS | '@my-scope/my-package-prod' | :npm | true + + # Edge cases + ref(:project_with_ppr) | :push | nil | '@my-scope/my-package-stage-sha-1234' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | nil | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/my-package-stage-sha-1234' | nil | false + ref(:project_with_ppr) | :push | nil | nil | nil | false + + # For projects that have no package protection rules + ref(:project_without_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/my-package-prod' | :npm | false + ref(:project_without_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/my-package-prod' | :npm | false + ref(:project_without_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/my-package-prod' | :npm | false + ref(:project_without_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/my-package-prod' | :npm | false + end + # rubocop:enable Layout/LineLength - it { is_expected.to eq deletion_protected } - end - end + with_them do + it { is_expected.to eq protected? } end end -- GitLab From fffa0dc508f51d2375ea755c98b455bb585fb04c Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Fri, 7 Feb 2025 17:14:04 +0100 Subject: [PATCH 3/4] reactor: Apply suggestion from @10io, @terrichu - Add migration to support down migration case, see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179739#note_2334019898 - Add tests for down migration case --- ...int_from_mininum_access_level_for_push.rb} | 0 ...kage_protection_rules_on_down_migration.rb | 28 ++++++++++ ...or_delete_to_packages_protection_rules.rb} | 0 ...protection_rules_minimum_access_levels.rb} | 0 db/schema_migrations/20250130161000 | 1 + db/schema_migrations/20250130161447 | 1 - db/schema_migrations/20250130162633 | 1 - db/schema_migrations/20250130163000 | 1 + db/schema_migrations/20250130164000 | 1 + ...protection_rules_on_down_migration_spec.rb | 56 +++++++++++++++++++ 10 files changed, 87 insertions(+), 2 deletions(-) rename db/migrate/{20250130162000_remove_not_null_constraint_from_mininum_access_level_for_push.rb => 20250130161000_remove_not_null_constraint_from_mininum_access_level_for_push.rb} (100%) create mode 100644 db/migrate/20250130162000_remove_package_protection_rules_on_down_migration.rb rename db/migrate/{20250130161447_add_minimum_access_level_for_delete_to_packages_protection_rules.rb => 20250130163000_add_minimum_access_level_for_delete_to_packages_protection_rules.rb} (100%) rename db/migrate/{20250130162633_add_multi_not_null_constraint_on_packages_protection_rules_minimum_access_levels.rb => 20250130164000_add_multi_not_null_constraint_on_packages_protection_rules_minimum_access_levels.rb} (100%) create mode 100644 db/schema_migrations/20250130161000 delete mode 100644 db/schema_migrations/20250130161447 delete mode 100644 db/schema_migrations/20250130162633 create mode 100644 db/schema_migrations/20250130163000 create mode 100644 db/schema_migrations/20250130164000 create mode 100644 spec/migrations/20250130162000_remove_package_protection_rules_on_down_migration_spec.rb diff --git a/db/migrate/20250130162000_remove_not_null_constraint_from_mininum_access_level_for_push.rb b/db/migrate/20250130161000_remove_not_null_constraint_from_mininum_access_level_for_push.rb similarity index 100% rename from db/migrate/20250130162000_remove_not_null_constraint_from_mininum_access_level_for_push.rb rename to db/migrate/20250130161000_remove_not_null_constraint_from_mininum_access_level_for_push.rb diff --git a/db/migrate/20250130162000_remove_package_protection_rules_on_down_migration.rb b/db/migrate/20250130162000_remove_package_protection_rules_on_down_migration.rb new file mode 100644 index 00000000000000..e5ac915e1e1892 --- /dev/null +++ b/db/migrate/20250130162000_remove_package_protection_rules_on_down_migration.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +# This migration is only relevant for the down migration case. +# It ensures that package protection rules with nil minimum_access_level_for_push are removed +# before the NOT NULL constraint is added in the previous down migration, +# i.e. 20250130161000_remove_not_null_constraint_from_mininum_access_level_for_push.rb . +# See the related discussion thread https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179739#note_2334019898 +class RemovePackageProtectionRulesOnDownMigration < Gitlab::Database::Migration[2.2] + milestone '17.9' + + disable_ddl_transaction! + + restrict_gitlab_migration gitlab_schema: :gitlab_main + + class PackagesProtectionRule < MigrationRecord + self.table_name = "packages_protection_rules" + end + + def up + # no-op + end + + def down + PackagesProtectionRule.where(minimum_access_level_for_push: nil).find_in_batches(batch_size: 100) do |ids| + PackagesProtectionRule.where(id: ids).delete_all + end + end +end diff --git a/db/migrate/20250130161447_add_minimum_access_level_for_delete_to_packages_protection_rules.rb b/db/migrate/20250130163000_add_minimum_access_level_for_delete_to_packages_protection_rules.rb similarity index 100% rename from db/migrate/20250130161447_add_minimum_access_level_for_delete_to_packages_protection_rules.rb rename to db/migrate/20250130163000_add_minimum_access_level_for_delete_to_packages_protection_rules.rb diff --git a/db/migrate/20250130162633_add_multi_not_null_constraint_on_packages_protection_rules_minimum_access_levels.rb b/db/migrate/20250130164000_add_multi_not_null_constraint_on_packages_protection_rules_minimum_access_levels.rb similarity index 100% rename from db/migrate/20250130162633_add_multi_not_null_constraint_on_packages_protection_rules_minimum_access_levels.rb rename to db/migrate/20250130164000_add_multi_not_null_constraint_on_packages_protection_rules_minimum_access_levels.rb diff --git a/db/schema_migrations/20250130161000 b/db/schema_migrations/20250130161000 new file mode 100644 index 00000000000000..50645029677884 --- /dev/null +++ b/db/schema_migrations/20250130161000 @@ -0,0 +1 @@ +d7540cd5185033e059ef5cd1fec7534cd72d6817d38515cf5ad09494ca53adca \ No newline at end of file diff --git a/db/schema_migrations/20250130161447 b/db/schema_migrations/20250130161447 deleted file mode 100644 index 32a588d12c1046..00000000000000 --- a/db/schema_migrations/20250130161447 +++ /dev/null @@ -1 +0,0 @@ -eccf6685d15f1f4b585257e7b7e8ec84e9bf0a334a9524f1da3b165357cae48e \ No newline at end of file diff --git a/db/schema_migrations/20250130162633 b/db/schema_migrations/20250130162633 deleted file mode 100644 index 28caf79d6ce4f6..00000000000000 --- a/db/schema_migrations/20250130162633 +++ /dev/null @@ -1 +0,0 @@ -b125cbe451b64dccd4a329af90efc785d37b3df5dbae7c27d9b473c762c10805 \ No newline at end of file diff --git a/db/schema_migrations/20250130163000 b/db/schema_migrations/20250130163000 new file mode 100644 index 00000000000000..304887054dd7aa --- /dev/null +++ b/db/schema_migrations/20250130163000 @@ -0,0 +1 @@ +2228d1a19ee7f088c24212cba45511a8e8f5ae67cdfd5b70506b20494e8d10e8 \ No newline at end of file diff --git a/db/schema_migrations/20250130164000 b/db/schema_migrations/20250130164000 new file mode 100644 index 00000000000000..e0ef7fcb98462d --- /dev/null +++ b/db/schema_migrations/20250130164000 @@ -0,0 +1 @@ +c35f7df366f60a6593cde2450ee35072f579cdc19eb2368d8f1a74504a329c9a \ No newline at end of file diff --git a/spec/migrations/20250130162000_remove_package_protection_rules_on_down_migration_spec.rb b/spec/migrations/20250130162000_remove_package_protection_rules_on_down_migration_spec.rb new file mode 100644 index 00000000000000..73eb936c5a2b94 --- /dev/null +++ b/spec/migrations/20250130162000_remove_package_protection_rules_on_down_migration_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe RemovePackageProtectionRulesOnDownMigration, migration: :gitlab_main, feature_category: :package_registry do + let(:migration) { described_class.new } + + let(:organization) { table(:organizations).create!(name: 'organization', path: 'organization') } + let(:namespace) do + table(:namespaces).create!(name: 'group', path: 'group', type: 'Group', organization_id: organization.id) + end + + let(:project) do + table(:projects).create!(namespace_id: namespace.id, project_namespace_id: namespace.id, + organization_id: organization.id) + end + + let(:packages_protection_rules) { table(:packages_protection_rules) } + + describe '#down' do + before do + packages_protection_rules.create!(project_id: project.id, package_type: 2, + package_name_pattern: '@group/package-1', minimum_access_level_for_push: nil) + + packages_protection_rules.create!(project_id: project.id, package_type: 2, + package_name_pattern: '@group/package-2', minimum_access_level_for_push: nil) + + packages_protection_rules.create!(project_id: project.id, package_type: 2, + package_name_pattern: '@group/package-3', minimum_access_level_for_push: 30) + + packages_protection_rules.create!(project_id: project.id, package_type: 2, + package_name_pattern: '@group/package-4', minimum_access_level_for_push: 40) + end + + it 'removes records with nil minimum_access_level_for_push' do + expect { migration.down } + .to change { packages_protection_rules.count }.from(4).to(2) + .and change { packages_protection_rules.where(minimum_access_level_for_push: nil).count }.from(2).to(0) + .and not_change { packages_protection_rules.where.not(minimum_access_level_for_push: nil).count } + end + + it 'preserves records with non-nil minimum_access_level_for_push' do + migration.down + + expect(packages_protection_rules.where( + package_name_pattern: '@group/package-3', + minimum_access_level_for_push: 30 + )).to exist + expect(packages_protection_rules.where( + package_name_pattern: '@group/package-4', + minimum_access_level_for_push: 40 + )).to exist + end + end +end -- GitLab From e5ef3e2508571733e765ac77d0ffad676a79a37e Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Wed, 12 Feb 2025 12:25:08 +0100 Subject: [PATCH 4/4] refactor: Remove maintainer from minimum_access_level_for_delete options Remove the :maintainer value from allowed options for minimum_access_level_for_delete since it represents the default authorization policy for package deletion. According to GitLab permissions, maintainers can delete packages by default, so this option is redundant. Only :owner and :admin values will be allowed, maintaining consistency with the container protection rules approach where default permissions are represented by nil values in the database. Docs: https://docs.gitlab.com/user/permissions/#packages-and-registry --- app/models/packages/protection/rule.rb | 2 +- spec/factories/packages/protection/rules.rb | 2 +- spec/models/packages/protection/rule_spec.rb | 170 +++++++++--------- .../protection/update_rule_service_spec.rb | 3 +- 4 files changed, 94 insertions(+), 83 deletions(-) diff --git a/app/models/packages/protection/rule.rb b/app/models/packages/protection/rule.rb index c428cbcd52c2d3..0913bc875ec414 100644 --- a/app/models/packages/protection/rule.rb +++ b/app/models/packages/protection/rule.rb @@ -5,7 +5,7 @@ module Protection class Rule < ApplicationRecord enum package_type: Packages::Package.package_types.slice(:conan, :maven, :npm, :pypi) enum minimum_access_level_for_delete: - Gitlab::Access.sym_options_with_admin.slice(:maintainer, :owner, :admin), + Gitlab::Access.sym_options_with_admin.slice(:owner, :admin), _prefix: :minimum_access_level_for_delete enum minimum_access_level_for_push: Gitlab::Access.sym_options_with_admin.slice(:maintainer, :owner, :admin), diff --git a/spec/factories/packages/protection/rules.rb b/spec/factories/packages/protection/rules.rb index 8e67633b28faf8..ee46c7532146cf 100644 --- a/spec/factories/packages/protection/rules.rb +++ b/spec/factories/packages/protection/rules.rb @@ -5,7 +5,7 @@ project package_name_pattern { '@my_scope/my_package' } package_type { :npm } - minimum_access_level_for_delete { :maintainer } + minimum_access_level_for_delete { :owner } minimum_access_level_for_push { :maintainer } end end diff --git a/spec/models/packages/protection/rule_spec.rb b/spec/models/packages/protection/rule_spec.rb index ae9eee72702b52..7ab7ab9c4a41c9 100644 --- a/spec/models/packages/protection/rule_spec.rb +++ b/spec/models/packages/protection/rule_spec.rb @@ -32,6 +32,14 @@ .with_prefix(:minimum_access_level_for_push) ) } + + it { + is_expected.to( + define_enum_for(:minimum_access_level_for_delete) + .with_values(owner: Gitlab::Access::OWNER, admin: Gitlab::Access::ADMIN) + .with_prefix(:minimum_access_level_for_delete) + ) + } end describe 'validations' do @@ -95,10 +103,10 @@ describe '#at_least_one_minimum_access_level_must_be_present' do where(:minimum_access_level_for_delete, :minimum_access_level_for_push, :valid) do - :maintainer | :maintainer | true - :maintainer | nil | true - nil | :maintainer | true - nil | nil | false + :owner | :maintainer | true + :owner | nil | true + nil | :maintainer | true + nil | nil | false end with_them do @@ -242,7 +250,19 @@ package_name_pattern: '@my-scope/my-package-stage*', project: project_with_ppr, package_type: :npm, - minimum_access_level_for_delete: :maintainer, + minimum_access_level_for_delete: :owner, + minimum_access_level_for_push: :maintainer + ) + end + + # Creating an identical package protection rule for the same project + # to ensure that overlapping rules are considered properly + let_it_be(:ppr_2_for_developer) do + create(:package_protection_rule, + package_name_pattern: '@my-scope/my-package-*', + project: project_with_ppr, + package_type: :npm, + minimum_access_level_for_delete: :owner, minimum_access_level_for_push: :maintainer ) end @@ -267,16 +287,6 @@ ) end - let_it_be(:ppr_2_for_developer) do - create(:package_protection_rule, - package_name_pattern: '@my-scope/my-package-*', - project: project_with_ppr, - package_type: :npm, - minimum_access_level_for_delete: :maintainer, - minimum_access_level_for_push: :maintainer - ) - end - let_it_be(:ppr_only_deletion_protection) do create(:package_protection_rule, package_name_pattern: '@my-scope/only_delete-protected-package*', @@ -310,77 +320,77 @@ # rubocop:disable Layout/LineLength -- Avoid formatting to ensure one-line table syntax where(:project, :action, :access_level, :package_name, :package_type, :protected?) do - ref(:project_with_ppr) | :push | Gitlab::Access::REPORTER | '@my-scope/my-package-stage-sha-1234' | :npm | true - ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/my-package-stage-sha-1234' | :npm | true - ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/my-package-stage-sha-1234' | :npm | false - ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/my-package-stage-sha-1234' | :npm | false - ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/my-package-stage-sha-1234' | :npm | false - ref(:project_with_ppr) | :delete | Gitlab::Access::REPORTER | '@my-scope/my-package-stage-sha-1234' | :npm | true - ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/my-package-stage-sha-1234' | :npm | true - ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/my-package-stage-sha-1234' | :npm | false - ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/my-package-stage-sha-1234' | :npm | false - ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/my-package-stage-sha-1234' | :npm | false - - ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/my-package-prod-sha-1234' | :npm | true - ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/my-package-prod-sha-1234' | :npm | false - ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/my-package-prod-sha-1234' | :npm | false - ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/my-package-prod-sha-1234' | :npm | true - ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/my-package-prod-sha-1234' | :npm | false - ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/my-package-prod-sha-1234' | :npm | false - - ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/my-package-release-v1' | :npm | true - ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/my-package-release-v1' | :npm | true - ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/my-package-release-v1' | :npm | false - ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/my-package-release-v1' | :npm | true - ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/my-package-release-v1' | :npm | true - ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/my-package-release-v1' | :npm | false - - ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :npm | true - ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/my-package-any-suffix' | :npm | false - ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/my-package-any-suffix' | :npm | false - ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/my-package-any-suffix' | :npm | false - ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :npm | true - ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/my-package-any-suffix' | :npm | false - ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/my-package-any-suffix' | :npm | false - ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/my-package-any-suffix' | :npm | false - - ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/only_delete-protected-package' | :npm | false - ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/only_delete-protected-package' | :npm | false - ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/only_delete-protected-package' | :npm | false - ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/only_delete-protected-package' | :npm | false - ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/only-push-protected-package' | :npm | true - ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/only-push-protected-package' | :npm | true - ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/only-push-protected-package' | :npm | true - ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/only-push-protected-package' | :npm | false - ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/only_delete-protected-package' | :npm | true - ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/only_delete-protected-package' | :npm | true - ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/only_delete-protected-package' | :npm | true - ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/only_delete-protected-package' | :npm | false - ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/only-push-protected-package' | :npm | false - ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/only-push-protected-package' | :npm | false - ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/only-push-protected-package' | :npm | false - ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/only-push-protected-package' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::REPORTER | '@my-scope/my-package-stage-sha-1234' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/my-package-stage-sha-1234' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/my-package-stage-sha-1234' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/my-package-stage-sha-1234' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/my-package-stage-sha-1234' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::REPORTER | '@my-scope/my-package-stage-sha-1234' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/my-package-stage-sha-1234' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/my-package-stage-sha-1234' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/my-package-stage-sha-1234' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/my-package-stage-sha-1234' | :npm | false + + ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/my-package-prod-sha-1234' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/my-package-prod-sha-1234' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/my-package-prod-sha-1234' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/my-package-prod-sha-1234' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/my-package-prod-sha-1234' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/my-package-prod-sha-1234' | :npm | false + + ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/my-package-release-v1' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/my-package-release-v1' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/my-package-release-v1' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/my-package-release-v1' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/my-package-release-v1' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/my-package-release-v1' | :npm | false + + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/my-package-any-suffix' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/my-package-any-suffix' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/my-package-any-suffix' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/my-package-any-suffix' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/my-package-any-suffix' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/my-package-any-suffix' | :npm | false + + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/only_delete-protected-package' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/only_delete-protected-package' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/only_delete-protected-package' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/only_delete-protected-package' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/only-push-protected-package' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::MAINTAINER | '@my-scope/only-push-protected-package' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/only-push-protected-package' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::ADMIN | '@my-scope/only-push-protected-package' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/only_delete-protected-package' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/only_delete-protected-package' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/only_delete-protected-package' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/only_delete-protected-package' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/only-push-protected-package' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::MAINTAINER | '@my-scope/only-push-protected-package' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/only-push-protected-package' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::ADMIN | '@my-scope/only-push-protected-package' | :npm | false # For non-matching packages - ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/non-matching-package' | :npm | false - ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/non-matching-package' | :npm | false - ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :conan | false - ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :conan | false - ref(:project_with_ppr) | :push | Gitlab::Access::NO_ACCESS | '@my-scope/my-package-prod' | :npm | true - ref(:project_with_ppr) | :delete | Gitlab::Access::NO_ACCESS | '@my-scope/my-package-prod' | :npm | true + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/non-matching-package' | :npm | false + ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/non-matching-package' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :conan | false + ref(:project_with_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/my-package-any-suffix' | :conan | false + ref(:project_with_ppr) | :push | Gitlab::Access::NO_ACCESS | '@my-scope/my-package-prod' | :npm | true + ref(:project_with_ppr) | :delete | Gitlab::Access::NO_ACCESS | '@my-scope/my-package-prod' | :npm | true # Edge cases - ref(:project_with_ppr) | :push | nil | '@my-scope/my-package-stage-sha-1234' | :npm | false - ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | nil | :npm | false - ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '' | :npm | false - ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/my-package-stage-sha-1234' | nil | false - ref(:project_with_ppr) | :push | nil | nil | nil | false + ref(:project_with_ppr) | :push | nil | '@my-scope/my-package-stage-sha-1234' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | nil | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '' | :npm | false + ref(:project_with_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/my-package-stage-sha-1234' | nil | false + ref(:project_with_ppr) | :push | nil | nil | nil | false # For projects that have no package protection rules - ref(:project_without_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/my-package-prod' | :npm | false - ref(:project_without_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/my-package-prod' | :npm | false - ref(:project_without_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/my-package-prod' | :npm | false - ref(:project_without_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/my-package-prod' | :npm | false + ref(:project_without_ppr) | :push | Gitlab::Access::DEVELOPER | '@my-scope/my-package-prod' | :npm | false + ref(:project_without_ppr) | :push | Gitlab::Access::OWNER | '@my-scope/my-package-prod' | :npm | false + ref(:project_without_ppr) | :delete | Gitlab::Access::DEVELOPER | '@my-scope/my-package-prod' | :npm | false + ref(:project_without_ppr) | :delete | Gitlab::Access::OWNER | '@my-scope/my-package-prod' | :npm | false end # rubocop:enable Layout/LineLength diff --git a/spec/services/packages/protection/update_rule_service_spec.rb b/spec/services/packages/protection/update_rule_service_spec.rb index 3d4b913e0ce2e7..2d8f98aa6cda11 100644 --- a/spec/services/packages/protection/update_rule_service_spec.rb +++ b/spec/services/packages/protection/update_rule_service_spec.rb @@ -14,7 +14,8 @@ :package_protection_rule, package_name_pattern: "#{package_protection_rule.package_name_pattern}-updated", package_type: 'npm', - minimum_access_level_for_push: 'owner' + minimum_access_level_for_push: 'owner', + minimum_access_level_for_delete: 'owner' ) end -- GitLab