From 30532ad45a25331457a0e576c87e19dbd9de4b3f Mon Sep 17 00:00:00 2001 From: Gerardo Date: Wed, 31 May 2023 14:54:14 +0200 Subject: [PATCH 01/20] Introducing FF and basics for protecting packages This MR introduces the basics for protecting packages, e.g. database migration, model and other utilities. The scope of the basic functionality was discussed in the epic: https://gitlab.com/groups/gitlab-org/-/epics/5574 Furthermore, this MR introduces a development feature flag (FF) that can be used for testing this feauture in a production setup. Changelog: added --- .../packages/package_protection_rule.rb | 37 ++++ app/models/project.rb | 10 +- app/policies/packages/package_policy.rb | 23 +++ .../packages_protected_packages.yml | 8 + db/docs/packages_package_protection_rules.yml | 10 + ...reate_packages_package_protection_rules.rb | 14 ++ db/schema_migrations/20230627170000 | 1 + db/structure.sql | 27 +++ .../packages/package_protection_rules.rb | 10 + .../packages/package_protection_rule_spec.rb | 192 ++++++++++++++++++ spec/policies/packages/package_policy_spec.rb | 152 ++++++++++++-- .../packages/policies/project_policy_spec.rb | 2 +- 12 files changed, 471 insertions(+), 15 deletions(-) create mode 100644 app/models/packages/package_protection_rule.rb create mode 100644 config/feature_flags/development/packages_protected_packages.yml create mode 100644 db/docs/packages_package_protection_rules.yml create mode 100644 db/migrate/20230627170000_create_packages_package_protection_rules.rb create mode 100644 db/schema_migrations/20230627170000 create mode 100644 spec/factories/packages/package_protection_rules.rb create mode 100644 spec/models/packages/package_protection_rule_spec.rb diff --git a/app/models/packages/package_protection_rule.rb b/app/models/packages/package_protection_rule.rb new file mode 100644 index 00000000000000..c4670a6ade7fc5 --- /dev/null +++ b/app/models/packages/package_protection_rule.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module Packages + class PackageProtectionRule < ApplicationRecord + # At the moment, we limit the enum to the package type `npm`. We can extend this at a later stage. + enum package_type: Packages::Package.package_types.slice(:npm) + + belongs_to :project, optional: false, inverse_of: :package_protection_rules + + validates :package_name_pattern, presence: true + validates :package_name_pattern, uniqueness: { scope: [:project_id, :package_type] }, + if: :package_name_pattern_changed? + validates :package_type, presence: true + validates :push_protected_up_to_access_level, + inclusion: { in: [Gitlab::Access::NO_ACCESS, *Gitlab::Access.all_values] } + validates :push_protected_up_to_access_level, presence: true + + scope :for_package_type, ->(package_type) { where(package_type: package_type) } + scope :for_package_name, ->(package_name) { + where( + ":package_name ILIKE REGEXP_REPLACE(package_name_pattern, '\\*', '%', 'g')", + package_name: sanitize_sql_like(package_name.presence || "") + ) + } + scope :push_protection_applicable_for_access_level, ->(user_access_level) { + case user_access_level + when nil + none + else + where(push_protected_up_to_access_level: [ + Gitlab::Access::NO_ACCESS, + user_access_level.. + ]) + end + } + end +end diff --git a/app/models/project.rb b/app/models/project.rb index dd197e4a43daf6..9070202753ad0f 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -266,8 +266,14 @@ def self.integration_association_name(name) has_many :debian_distributions, class_name: 'Packages::Debian::ProjectDistribution', dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - has_many :npm_metadata_caches, class_name: 'Packages::Npm::MetadataCache' - has_one :packages_cleanup_policy, class_name: 'Packages::Cleanup::Policy', inverse_of: :project + has_many :npm_metadata_caches, + class_name: 'Packages::Npm::MetadataCache' + has_one :packages_cleanup_policy, + class_name: 'Packages::Cleanup::Policy', + inverse_of: :project + has_many :package_protection_rules, + class_name: 'Packages::PackageProtectionRule', + inverse_of: :project has_one :import_state, autosave: true, class_name: 'ProjectImportState', inverse_of: :project has_one :import_export_upload, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent diff --git a/app/policies/packages/package_policy.rb b/app/policies/packages/package_policy.rb index 829d62a6430551..b84b223ef6a99e 100644 --- a/app/policies/packages/package_policy.rb +++ b/app/policies/packages/package_policy.rb @@ -2,5 +2,28 @@ module Packages class PackagePolicy < BasePolicy delegate { @subject.project&.packages_policy_subject } + delegate { @subject.project } + + condition(:push_protected_from_user) do + next false if Feature.disabled?(:packages_protected_packages) + + current_user_project_authorization_access_level = @user.max_member_access_for_project(project.id) + next true if current_user_project_authorization_access_level == Gitlab::Access::NO_ACCESS + + # TODO: Add support for `package_version` (later) + # TODO: Consider caching + # TODO: Get advice from GitLab team where the best situation for this test is. + project.package_protection_rules + .for_package_type(@subject.package_type) + .for_package_name(@subject.name) + .push_protection_applicable_for_access_level(current_user_project_authorization_access_level) + .exists? + end + + rule { push_protected_from_user }.prevent :create_package + + def project + @subject.project + end end end diff --git a/config/feature_flags/development/packages_protected_packages.yml b/config/feature_flags/development/packages_protected_packages.yml new file mode 100644 index 00000000000000..88557b2317dcaf --- /dev/null +++ b/config/feature_flags/development/packages_protected_packages.yml @@ -0,0 +1,8 @@ +--- +name: packages_protected_packages +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124776 +rollout_issue_url: +milestone: '16.2' +type: development +group: group::package registry +default_enabled: false diff --git a/db/docs/packages_package_protection_rules.yml b/db/docs/packages_package_protection_rules.yml new file mode 100644 index 00000000000000..c7309270806ff9 --- /dev/null +++ b/db/docs/packages_package_protection_rules.yml @@ -0,0 +1,10 @@ +--- +table_name: packages_package_protection_rules +classes: +- Packages::PackageProtectionRule +feature_categories: +- registry +description: Represents the rules for protecting packages package registry +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124776 +milestone: '16.2' +gitlab_schema: gitlab_main diff --git a/db/migrate/20230627170000_create_packages_package_protection_rules.rb b/db/migrate/20230627170000_create_packages_package_protection_rules.rb new file mode 100644 index 00000000000000..bc18c30b970747 --- /dev/null +++ b/db/migrate/20230627170000_create_packages_package_protection_rules.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class CreatePackagesPackageProtectionRules < Gitlab::Database::Migration[2.1] + def change + create_table :packages_package_protection_rules do |t| + t.text :package_name_pattern, limit: 255 + t.integer :package_type, limit: 2 + t.integer :push_protected_up_to_access_level + t.references :project, null: false + + t.timestamps_with_timezone null: false + end + end +end diff --git a/db/schema_migrations/20230627170000 b/db/schema_migrations/20230627170000 new file mode 100644 index 00000000000000..b3b41be2b57b7a --- /dev/null +++ b/db/schema_migrations/20230627170000 @@ -0,0 +1 @@ +279b9ab1fe5c7f7d80d51eab38ba3db02def47eb2b80db09d2751fb5868ee60a \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 14b61af7aa2183..3ca01aa14e0b8b 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -20208,6 +20208,26 @@ CREATE SEQUENCE packages_package_files_id_seq ALTER SEQUENCE packages_package_files_id_seq OWNED BY packages_package_files.id; +CREATE TABLE packages_package_protection_rules ( + id bigint NOT NULL, + package_name_pattern text, + package_type smallint, + push_protected_up_to_access_level integer, + project_id bigint NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone NOT NULL, + CONSTRAINT check_4ad7e5d3c3 CHECK ((char_length(package_name_pattern) <= 255)) +); + +CREATE SEQUENCE packages_package_protection_rules_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE packages_package_protection_rules_id_seq OWNED BY packages_package_protection_rules.id; + CREATE TABLE packages_packages ( id bigint NOT NULL, project_id integer NOT NULL, @@ -26116,6 +26136,8 @@ ALTER TABLE ONLY packages_package_file_build_infos ALTER COLUMN id SET DEFAULT n ALTER TABLE ONLY packages_package_files ALTER COLUMN id SET DEFAULT nextval('packages_package_files_id_seq'::regclass); +ALTER TABLE ONLY packages_package_protection_rules ALTER COLUMN id SET DEFAULT nextval('packages_package_protection_rules_id_seq'::regclass); + ALTER TABLE ONLY packages_packages ALTER COLUMN id SET DEFAULT nextval('packages_packages_id_seq'::regclass); ALTER TABLE ONLY packages_rpm_repository_files ALTER COLUMN id SET DEFAULT nextval('packages_rpm_repository_files_id_seq'::regclass); @@ -28455,6 +28477,9 @@ ALTER TABLE ONLY packages_package_file_build_infos ALTER TABLE ONLY packages_package_files ADD CONSTRAINT packages_package_files_pkey PRIMARY KEY (id); +ALTER TABLE ONLY packages_package_protection_rules + ADD CONSTRAINT packages_package_protection_rules_pkey PRIMARY KEY (id); + ALTER TABLE ONLY packages_packages ADD CONSTRAINT packages_packages_pkey PRIMARY KEY (id); @@ -32891,6 +32916,8 @@ CREATE INDEX index_packages_package_files_on_status ON packages_package_files US CREATE INDEX index_packages_package_files_on_verification_state ON packages_package_files USING btree (verification_state); +CREATE INDEX index_packages_package_protection_rules_on_project_id ON packages_package_protection_rules USING btree (project_id); + CREATE INDEX index_packages_packages_on_creator_id ON packages_packages USING btree (creator_id); CREATE INDEX index_packages_packages_on_id_and_created_at ON packages_packages USING btree (id, created_at); diff --git a/spec/factories/packages/package_protection_rules.rb b/spec/factories/packages/package_protection_rules.rb new file mode 100644 index 00000000000000..e759b0ba65681a --- /dev/null +++ b/spec/factories/packages/package_protection_rules.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :package_protection_rule, class: 'Packages::PackageProtectionRule' do + project + package_name_pattern { 'my/company/app/my-app' } + package_type { :npm } + push_protected_up_to_access_level { Gitlab::Access::DEVELOPER } + end +end diff --git a/spec/models/packages/package_protection_rule_spec.rb b/spec/models/packages/package_protection_rule_spec.rb new file mode 100644 index 00000000000000..c816647b7cc19e --- /dev/null +++ b/spec/models/packages/package_protection_rule_spec.rb @@ -0,0 +1,192 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::PackageProtectionRule, type: :model, feature_category: :package_registry do + it_behaves_like 'having unique enum values' + + describe 'factories' do + it do + package_protection_rule = create(:package_protection_rule) + expect(package_protection_rule).to be_valid + end + end + + describe 'relationships' do + it { is_expected.to belong_to(:project).inverse_of(:package_protection_rules).required } + end + + describe 'enums' do + describe '#package_type' do + # For now, we focus on the package type `npm` and do not want to allow other package types. + it { is_expected.to define_enum_for(:package_type).with_values(npm: 2) } + end + end + + describe 'validations' do + subject { build(:package_protection_rule) } + + it { is_expected.to validate_presence_of(:package_name_pattern) } + it { is_expected.to validate_presence_of(:package_type) } + it { is_expected.to validate_presence_of(:push_protected_up_to_access_level) } + it { is_expected.to validate_uniqueness_of(:package_name_pattern).scoped_to(:project_id, :package_type) } + + describe '#package_name_pattern' do + it { is_expected.to allow_value("my/domain/com/my-package").for(:package_name_pattern) } + it { is_expected.to allow_value("my/domain/com/my-package*").for(:package_name_pattern) } + it { is_expected.to allow_value("*****my/domain/com/my-package*****").for(:package_name_pattern) } + + # Special cases + it { is_expected.to allow_value("my/domain/com/my package with whitespace").for(:package_name_pattern) } + it { is_expected.to allow_value("my/domain/com/my-package-with-umlauts-üäö").for(:package_name_pattern) } + + it do + is_expected.to( + allow_value("my/domain/com/my-package-with-special-characters-~!@#$%^&()<>?").for(:package_name_pattern) + ) + end + end + end + + describe ".for_package_name" do + context "with several package protection rule scenarios" do + let_it_be(:package_protection_rule) do + create(:package_protection_rule, package_name_pattern: "my/domain/com/my-package") + end + + let_it_be(:package_protection_rule_with_wildcard_start) do + create(:package_protection_rule, package_name_pattern: "*my/domain/com/my-package-with-wildcard-start") + end + + let_it_be(:package_protection_rule_with_wildcard_end) do + create(:package_protection_rule, package_name_pattern: "my/domain/com/my-package-with-wildcard-end*") + end + + let_it_be(:package_protection_rule_with_wildcard_inbetween) do + create(:package_protection_rule, package_name_pattern: "my/domain/com/*my-package-with-wildcard-inbetween") + end + + let_it_be(:package_protection_rule_with_wildcard_multiples) do + create(:package_protection_rule, package_name_pattern: "**my/domain/com/**my-package-with-wildcard-multiple**") + end + + let_it_be(:package_protection_rule_with_unsupported_regex_characters) do + create(:package_protection_rule, + package_name_pattern: "my/domain/com/my-package-with-unsupported-regex-characters.+") + end + + using RSpec::Parameterized::TableSyntax + + where(:package_name, :match_or_not) do + "my/domain/com/my-package" | true + "my/domain/com/my-package-2" | false + + "my/domain/com/my-package-with-wildcard-start" | true + "my/domain/com/my-package-with-wildcard-start-any" | false + "any-my/domain/com/my-package-with-wildcard-start" | true + "any-my/domain/com/my-package-with-wildcard-start-any" | false + + "my/domain/com/my-package-with-wildcard-end" | true + "my/domain/com/my-package-with-wildcard-end-any" | true + "any-my/domain/com/my-package-with-wildcard-end" | false + "any-my/domain/com/my-package-with-wildcard-end-any" | false + + "my/domain/com/my-package-with-wildcard-inbetween" | true + "my/domain/com/any-my-package-with-wildcard-inbetween" | true + "my/domain/com/any-my-package-my-package-wildcard-inbetween-any" | false + + "my/domain/com/my-package-with-wildcard-multiple" | true + "any-my/domain/com/any-my-package-with-wildcard-multiple-any" | true + "****my/domain/com/****my-package-with-wildcard-multiple****" | true + "any-other/domain/com/any-my-package-with-wildcard-multiple-any" | false + + "my/domain/com/my-package-with-unsupported-regex-characters.+" | true + "my/domain/com/my-package-with-unsupported-regex-characters." | false + "my/domain/com/my-package-with-unsupported-regex-characters" | false + "my/domain/com/my-package-with-unsupported-regex-characters-any" | false + + nil | false + "" | false + "any_package" | false + end + + with_them do + it do + if match_or_not + expect(described_class.for_package_name(package_name)).to exist + else + expect(described_class.for_package_name(package_name)).not_to exist + end + end + end + end + end + + describe ".push_protection_applicable_for_access_level" do + context "with different scenarios" do + let_it_be(:ppr_for_developer) do + create(:package_protection_rule, package_name_pattern: "package-name-pattern-for-developer", + push_protected_up_to_access_level: Gitlab::Access::DEVELOPER) + end + + let_it_be(:ppr_for_maintainer) do + create(:package_protection_rule, package_name_pattern: "package-name-pattern-for-maintainer", + push_protected_up_to_access_level: Gitlab::Access::MAINTAINER) + end + + let_it_be(:ppr_for_owner) do + create(:package_protection_rule, package_name_pattern: "package-name-pattern-for-owner", + push_protected_up_to_access_level: Gitlab::Access::OWNER) + end + + let_it_be(:ppr_for_no_access) do + create(:package_protection_rule, package_name_pattern: "package-name-pattern-for-no_access", + push_protected_up_to_access_level: Gitlab::Access::NO_ACCESS) + end + + let_it_be(:access_level_guest) { Gitlab::Access::GUEST } + let_it_be(:access_level_reporter) { Gitlab::Access::REPORTER } + let_it_be(:access_level_developer) { Gitlab::Access::DEVELOPER } + let_it_be(:access_level_maintainer) { Gitlab::Access::MAINTAINER } + let_it_be(:access_level_no_access) { Gitlab::Access::NO_ACCESS } + let_it_be(:access_level_owner) { Gitlab::Access::OWNER } + + using RSpec::Parameterized::TableSyntax + + # rubocop:disable Layout/LineLength + where(:push_protected_up_to_access_level, :expected_package_protection_rules) do + ref(:access_level_guest) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner), ref(:ppr_for_no_access)] + ref(:access_level_reporter) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner), ref(:ppr_for_no_access)] + ref(:access_level_developer) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner), ref(:ppr_for_no_access)] + ref(:access_level_maintainer) | [ref(:ppr_for_maintainer), ref(:ppr_for_owner), ref(:ppr_for_no_access)] + ref(:access_level_owner) | [ref(:ppr_for_owner), ref(:ppr_for_no_access)] + + # Special cases + nil | [] + 0 | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner), ref(:ppr_for_no_access)] + -1 | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner), ref(:ppr_for_no_access)] + end + # rubocop:enable Layout/LineLength + + with_them do + subject { described_class.push_protection_applicable_for_access_level(push_protected_up_to_access_level) } + + it { is_expected.to eq expected_package_protection_rules } + end + end + end + + describe ".for_package_type" do + let_it_be(:ppr_for_npm) do + create(:package_protection_rule, package_type: :npm) + end + + it { expect(described_class.for_package_type(:npm)).to eq [ppr_for_npm] } + it { expect(described_class.for_package_type("npm")).to eq [ppr_for_npm] } + it { expect(described_class.for_package_type(Packages::Package.package_types.fetch(:npm))).to eq [ppr_for_npm] } + + it { expect(described_class.for_package_type("")).to eq [] } + it { expect(described_class.for_package_type(nil)).to eq [] } + it { expect(described_class.for_package_type(Packages::Package.package_types.fetch(:pypi))).to eq [] } + end +end diff --git a/spec/policies/packages/package_policy_spec.rb b/spec/policies/packages/package_policy_spec.rb index 13935974b440af..53eb6c298dad29 100644 --- a/spec/policies/packages/package_policy_spec.rb +++ b/spec/policies/packages/package_policy_spec.rb @@ -2,26 +2,154 @@ require 'spec_helper' -RSpec.describe Packages::PackagePolicy do - let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project) } - let_it_be(:package) { create(:package, project: project) } +RSpec.describe Packages::PackagePolicy, feature_category: :package_registry do + include_context 'ProjectPolicy context' + + let(:package) { build(:package, project: project) } + let(:project) { private_project } subject(:policy) { described_class.new(user, package) } - context 'when the user is part of the project' do - before do - project.add_reporter(user) + describe 'ability :read_package' do + context 'when the user is part of the project' do + let(:user) { reporter } + + it 'allows read_package' do + expect(policy).to be_allowed(:read_package) + end end - it 'allows read_package' do - expect(policy).to be_allowed(:read_package) + context 'when the user is not part of the project' do + let(:user) { non_member } + + it 'disallows read_package for any Package' do + expect(policy).to be_disallowed(:read_package) + end end end - context 'when the user is not part of the project' do - it 'disallows read_package for any Package' do - expect(policy).to be_disallowed(:read_package) + describe 'ability :create_package' do + let(:user) { developer } + + context "without package protection rules" do + it { is_expected.to be_allowed(:create_package) } + end + + context "with package protection rules" do + let(:package) { build(:npm_package, project: project) } + + context "when feature flag disabled" do + before do + stub_feature_flags(packages_protected_packages: false) + + create(:package_protection_rule, + package_name_pattern: package.name, + package_type: package.package_type, + project: package.project, + push_protected_up_to_access_level: Gitlab::Access::NO_ACCESS + ) + end + + it { is_expected.to be_allowed(:create_package) } + end + + describe "permission table" do + let_it_be(:package_private_project) { create(:npm_package, project: private_project) } + + let(:access_level_developer) { Gitlab::Access::DEVELOPER } + let(:access_level_maintainer) { Gitlab::Access::MAINTAINER } + let(:access_level_no_access) { Gitlab::Access::NO_ACCESS } + let(:access_level_owner) { Gitlab::Access::OWNER } + + using RSpec::Parameterized::TableSyntax + + where(:user, :package, :push_protected_up_to_access_level, :expect_to_be_allowed) do + ref(:anonymous) | ref(:package_private_project) | ref(:access_level_developer) | false + ref(:non_member) | ref(:package_private_project) | ref(:access_level_developer) | false + ref(:guest) | ref(:package_private_project) | ref(:access_level_developer) | false + ref(:reporter) | ref(:package_private_project) | ref(:access_level_developer) | false + ref(:developer) | ref(:package_private_project) | ref(:access_level_developer) | false + ref(:maintainer) | ref(:package_private_project) | ref(:access_level_developer) | true + ref(:owner) | ref(:package_private_project) | ref(:access_level_developer) | true + # TODO: Tbd. behavior of user access level `admin` + # ref(:admin) | ref(:package_private_project) | ref(:access_level_developer) | true + + ref(:developer) | ref(:package_private_project) | ref(:access_level_maintainer) | false + ref(:maintainer) | ref(:package_private_project) | ref(:access_level_maintainer) | false + ref(:owner) | ref(:package_private_project) | ref(:access_level_maintainer) | true + # TODO: Tbd. behavior of user access level `admin` + # ref(:admin) | ref(:package_private_project) | ref(:access_level_maintainer) | true + + ref(:maintainer) | ref(:package_private_project) | ref(:access_level_owner) | false + ref(:owner) | ref(:package_private_project) | ref(:access_level_owner) | false + # TODO: Tbd. behavior of user access level `admin` + # ref(:admin) | ref(:package_private_project) | ref(:access_level_owner) | false + + ref(:anonymous) | ref(:package_private_project) | ref(:access_level_no_access) | false + ref(:non_member) | ref(:package_private_project) | ref(:access_level_no_access) | false + ref(:guest) | ref(:package_private_project) | ref(:access_level_no_access) | false + ref(:reporter) | ref(:package_private_project) | ref(:access_level_no_access) | false + ref(:developer) | ref(:package_private_project) | ref(:access_level_no_access) | false + ref(:maintainer) | ref(:package_private_project) | ref(:access_level_no_access) | false + ref(:owner) | ref(:package_private_project) | ref(:access_level_no_access) | false + # TODO: Tbd. behavior of user access level `admin` + # ref(:admin) | ref(:package_private_project) | ref(:access_level_owner) | false + + # # TODO: Tbd. behavior for packages of internal projects + # ref(:developer) | ref(:package_internal_project) | ref(:access_level_developer) | false + # ... + + # # TODO: Tbd. behavior for packages of public projects + # ref(:developer) | ref(:package_public_project) | ref(:access_level_developer) | false + # ... + end + + with_them do + before do + create(:package_protection_rule, + package_name_pattern: package.name, + package_type: package.package_type, + project: package.project, + push_protected_up_to_access_level: push_protected_up_to_access_level + ) + end + + it do + if expect_to_be_allowed + is_expected.to be_allowed(:create_package) + else + is_expected.to be_disallowed(:create_package) + end + end + end + end + + context "with wildcard package name pattern" do + before do + create(:package_protection_rule, + # Convert the given package name into a `package_name_pattern` with a wildcard + package_name_pattern: "#{package.name[..-3]}*", + package_type: package.package_type, + project: package.project, + push_protected_up_to_access_level: Gitlab::Access::DEVELOPER + ) + end + + it { is_expected.to be_disallowed(:create_package) } + end + + context "with no matching package protection rule found" do + before do + create(:package_protection_rule, + package_name_pattern: "#{package.name}_no_match", + package_type: package.package_type, + project: package.project, + push_protected_up_to_access_level: Gitlab::Access::DEVELOPER + ) + end + + it { is_expected.to be_allowed(:create_package) } + end end end end diff --git a/spec/policies/packages/policies/project_policy_spec.rb b/spec/policies/packages/policies/project_policy_spec.rb index fde10f64be82ef..1f8a653f984b71 100644 --- a/spec/policies/packages/policies/project_policy_spec.rb +++ b/spec/policies/packages/policies/project_policy_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Packages::Policies::ProjectPolicy do +RSpec.describe Packages::Policies::ProjectPolicy, feature_category: :package_registry do include_context 'ProjectPolicy context' let(:project) { public_project } -- GitLab From 7be2631293c85defae01780df0f28e434ace3fac Mon Sep 17 00:00:00 2001 From: Gerardo Date: Wed, 5 Jul 2023 09:41:54 +0200 Subject: [PATCH 02/20] docs: Apply reviewer feedback from @aakriti.gupta This includes: - docs: Added rollout issue for FF as suggested by @aakriti.gupta - refactor: Fix failing tests in ci cd pipeline - test: Add link to further discussion as suggested by @aakriti.gupta --- .../development/packages_protected_packages.yml | 2 +- db/docs/packages_package_protection_rules.yml | 2 +- ...0230627170000_create_packages_package_protection_rules.rb | 5 ++++- db/structure.sql | 3 +++ spec/lib/gitlab/import_export/all_models.yml | 1 + spec/models/packages/package_protection_rule_spec.rb | 4 +++- 6 files changed, 13 insertions(+), 4 deletions(-) diff --git a/config/feature_flags/development/packages_protected_packages.yml b/config/feature_flags/development/packages_protected_packages.yml index 88557b2317dcaf..6e997b2f8c733d 100644 --- a/config/feature_flags/development/packages_protected_packages.yml +++ b/config/feature_flags/development/packages_protected_packages.yml @@ -1,7 +1,7 @@ --- name: packages_protected_packages introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124776 -rollout_issue_url: +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/416395 milestone: '16.2' type: development group: group::package registry diff --git a/db/docs/packages_package_protection_rules.yml b/db/docs/packages_package_protection_rules.yml index c7309270806ff9..d37f2eb0ee476f 100644 --- a/db/docs/packages_package_protection_rules.yml +++ b/db/docs/packages_package_protection_rules.yml @@ -3,7 +3,7 @@ table_name: packages_package_protection_rules classes: - Packages::PackageProtectionRule feature_categories: -- registry +- package_registry description: Represents the rules for protecting packages package registry introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124776 milestone: '16.2' diff --git a/db/migrate/20230627170000_create_packages_package_protection_rules.rb b/db/migrate/20230627170000_create_packages_package_protection_rules.rb index bc18c30b970747..415962028be25d 100644 --- a/db/migrate/20230627170000_create_packages_package_protection_rules.rb +++ b/db/migrate/20230627170000_create_packages_package_protection_rules.rb @@ -6,7 +6,10 @@ def change t.text :package_name_pattern, limit: 255 t.integer :package_type, limit: 2 t.integer :push_protected_up_to_access_level - t.references :project, null: false + t.references :project, + null: false, + default: nil, + foreign_key: { on_delete: :cascade } t.timestamps_with_timezone null: false end diff --git a/db/structure.sql b/db/structure.sql index 3ca01aa14e0b8b..1acd9462feb581 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -37328,6 +37328,9 @@ ALTER TABLE ONLY project_authorizations ALTER TABLE ONLY boards_epic_lists ADD CONSTRAINT fk_rails_0f9c7f646f FOREIGN KEY (epic_board_id) REFERENCES boards_epic_boards(id) ON DELETE CASCADE; +ALTER TABLE ONLY packages_package_protection_rules + ADD CONSTRAINT fk_rails_0fa05e57a5 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; + ALTER TABLE ONLY issue_email_participants ADD CONSTRAINT fk_rails_0fdfd8b811 FOREIGN KEY (issue_id) REFERENCES issues(id) ON DELETE CASCADE; diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index de3f28a5b90641..5ca5270a8515cf 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -738,6 +738,7 @@ project: - project_registry - packages - package_files +- package_protection_rules - rpm_repository_files - npm_metadata_caches - packages_cleanup_policy diff --git a/spec/models/packages/package_protection_rule_spec.rb b/spec/models/packages/package_protection_rule_spec.rb index c816647b7cc19e..cd538c68076e8e 100644 --- a/spec/models/packages/package_protection_rule_spec.rb +++ b/spec/models/packages/package_protection_rule_spec.rb @@ -18,7 +18,9 @@ describe 'enums' do describe '#package_type' do - # For now, we focus on the package type `npm` and do not want to allow other package types. + # As discussed in https://gitlab.com/groups/gitlab-org/-/epics/5574#note_1437348728, + # we focus on the package type `npm` at the moment. + # Other package types will be added later. it { is_expected.to define_enum_for(:package_type).with_values(npm: 2) } end end -- GitLab From 40fe0a78d77fb2dacb90b02ef3fc3f9ee36c8fbc Mon Sep 17 00:00:00 2001 From: Gerardo Date: Thu, 6 Jul 2023 15:36:44 +0200 Subject: [PATCH 03/20] test: Extend test case for matching package name --- .../packages/package_protection_rule.rb | 24 +++- .../packages/package_protection_rule_spec.rb | 125 ++++++++++++------ 2 files changed, 103 insertions(+), 46 deletions(-) diff --git a/app/models/packages/package_protection_rule.rb b/app/models/packages/package_protection_rule.rb index c4670a6ade7fc5..efd8bbf4ff3906 100644 --- a/app/models/packages/package_protection_rule.rb +++ b/app/models/packages/package_protection_rule.rb @@ -16,11 +16,27 @@ class PackageProtectionRule < ApplicationRecord validates :push_protected_up_to_access_level, presence: true scope :for_package_type, ->(package_type) { where(package_type: package_type) } + + # We want to allow wildcard pattern (`*`) for the field `package_name_pattern`, e.g. `@my-scope/my_package_*`, etc. + # Therefore, we need to preprocess the field value before the LIKE clause is evaluated, + # e.g. convert wildcard character (`*`) to LIKE match character (`%`), escape certain character, etc. scope :for_package_name, ->(package_name) { - where( - ":package_name ILIKE REGEXP_REPLACE(package_name_pattern, '\\*', '%', 'g')", - package_name: sanitize_sql_like(package_name.presence || "") - ) + where_clause = %q{ + :package_name ILIKE REGEXP_REPLACE( + REGEXP_REPLACE( + REGEXP_REPLACE( + REGEXP_REPLACE( + package_name_pattern, + '\\\\', '\\\\\\\\', 'g' + ), + '\%', '\%', 'g' + ), + '\_', '\_', 'g' + ), + '\*', '%', 'g' + ) + } + where(where_clause, package_name: package_name.presence || "") } scope :push_protection_applicable_for_access_level, ->(user_access_level) { case user_access_level diff --git a/spec/models/packages/package_protection_rule_spec.rb b/spec/models/packages/package_protection_rule_spec.rb index cd538c68076e8e..082ae59dd40d48 100644 --- a/spec/models/packages/package_protection_rule_spec.rb +++ b/spec/models/packages/package_protection_rule_spec.rb @@ -34,17 +34,17 @@ it { is_expected.to validate_uniqueness_of(:package_name_pattern).scoped_to(:project_id, :package_type) } describe '#package_name_pattern' do - it { is_expected.to allow_value("my/domain/com/my-package").for(:package_name_pattern) } - it { is_expected.to allow_value("my/domain/com/my-package*").for(:package_name_pattern) } - it { is_expected.to allow_value("*****my/domain/com/my-package*****").for(:package_name_pattern) } + it { is_expected.to allow_value("@my-scope/my_package").for(:package_name_pattern) } + it { is_expected.to allow_value("@my-scope/my_package*").for(:package_name_pattern) } + it { is_expected.to allow_value("*****@my-scope/my_package*****").for(:package_name_pattern) } # Special cases - it { is_expected.to allow_value("my/domain/com/my package with whitespace").for(:package_name_pattern) } - it { is_expected.to allow_value("my/domain/com/my-package-with-umlauts-üäö").for(:package_name_pattern) } + it { is_expected.to allow_value("@my-scope/my package with whitespace").for(:package_name_pattern) } + it { is_expected.to allow_value("@my-scope/my_package-with-umlauts-üäö").for(:package_name_pattern) } it do is_expected.to( - allow_value("my/domain/com/my-package-with-special-characters-~!@#$%^&()<>?").for(:package_name_pattern) + allow_value("@my-scope/my_package-with-special-characters-~!@#$%^&()<>?").for(:package_name_pattern) ) end end @@ -53,63 +53,104 @@ describe ".for_package_name" do context "with several package protection rule scenarios" do let_it_be(:package_protection_rule) do - create(:package_protection_rule, package_name_pattern: "my/domain/com/my-package") + create(:package_protection_rule, package_name_pattern: "@my-scope/my_package") end let_it_be(:package_protection_rule_with_wildcard_start) do - create(:package_protection_rule, package_name_pattern: "*my/domain/com/my-package-with-wildcard-start") + create(:package_protection_rule, package_name_pattern: "*@my-scope/my_package-with-wildcard-start") end let_it_be(:package_protection_rule_with_wildcard_end) do - create(:package_protection_rule, package_name_pattern: "my/domain/com/my-package-with-wildcard-end*") + create(:package_protection_rule, package_name_pattern: "@my-scope/my_package-with-wildcard-end*") end let_it_be(:package_protection_rule_with_wildcard_inbetween) do - create(:package_protection_rule, package_name_pattern: "my/domain/com/*my-package-with-wildcard-inbetween") + create(:package_protection_rule, package_name_pattern: "@my-scope/*my_package-with-wildcard-inbetween") end let_it_be(:package_protection_rule_with_wildcard_multiples) do - create(:package_protection_rule, package_name_pattern: "**my/domain/com/**my-package-with-wildcard-multiple**") + create(:package_protection_rule, package_name_pattern: "**@my-scope/**my_package-with-wildcard-multiple**") + end + + let_it_be(:package_protection_rule_with_wildcard_escaped) do + create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with-wildcard-escaped-\*') + end + + let_it_be(:package_protection_rule_with_underscore) do + create(:package_protection_rule, package_name_pattern: "@my-scope/my_package-with_____underscore") + end + + let_it_be(:package_protection_rule_with_percent_sign) do + create(:package_protection_rule, package_name_pattern: "@my-scope/my_package-with-percent-sign-%") + end + + let_it_be(:package_protection_rule_with_percent_sign_escaped) do + create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with-percent-sign-escaped-\%') end let_it_be(:package_protection_rule_with_unsupported_regex_characters) do create(:package_protection_rule, - package_name_pattern: "my/domain/com/my-package-with-unsupported-regex-characters.+") + package_name_pattern: "@my-scope/my_package-with-unsupported-regex-characters.+") end using RSpec::Parameterized::TableSyntax where(:package_name, :match_or_not) do - "my/domain/com/my-package" | true - "my/domain/com/my-package-2" | false - - "my/domain/com/my-package-with-wildcard-start" | true - "my/domain/com/my-package-with-wildcard-start-any" | false - "any-my/domain/com/my-package-with-wildcard-start" | true - "any-my/domain/com/my-package-with-wildcard-start-any" | false - - "my/domain/com/my-package-with-wildcard-end" | true - "my/domain/com/my-package-with-wildcard-end-any" | true - "any-my/domain/com/my-package-with-wildcard-end" | false - "any-my/domain/com/my-package-with-wildcard-end-any" | false - - "my/domain/com/my-package-with-wildcard-inbetween" | true - "my/domain/com/any-my-package-with-wildcard-inbetween" | true - "my/domain/com/any-my-package-my-package-wildcard-inbetween-any" | false - - "my/domain/com/my-package-with-wildcard-multiple" | true - "any-my/domain/com/any-my-package-with-wildcard-multiple-any" | true - "****my/domain/com/****my-package-with-wildcard-multiple****" | true - "any-other/domain/com/any-my-package-with-wildcard-multiple-any" | false - - "my/domain/com/my-package-with-unsupported-regex-characters.+" | true - "my/domain/com/my-package-with-unsupported-regex-characters." | false - "my/domain/com/my-package-with-unsupported-regex-characters" | false - "my/domain/com/my-package-with-unsupported-regex-characters-any" | false - - nil | false - "" | false - "any_package" | false + "@my-scope/my_package" | true + "@my-scope/my2package" | false + "@my-scope/my_package-2" | false + + # With wildcard pattern at the start + "@my-scope/my_package-with-wildcard-start" | true + "@my-scope/my_package-with-wildcard-start-any" | false + "prefix-@my-scope/my_package-with-wildcard-start" | true + "prefix-@my-scope/my_package-with-wildcard-start-any" | false + + # With wildcard pattern at the end + "@my-scope/my_package-with-wildcard-end" | true + "@my-scope/my_package-with-wildcard-end:1234567890" | true + "prefix-@my-scope/my_package-with-wildcard-end" | false + "prefix-@my-scope/my_package-with-wildcard-end:1234567890" | false + + # With wildcard pattern inbetween + "@my-scope/my_package-with-wildcard-inbetween" | true + "@my-scope/any-my_package-with-wildcard-inbetween" | true + "@my-scope/any-my_package-my_package-wildcard-inbetween-any" | false + + # With multiple wildcard pattern are used + "@my-scope/my_package-with-wildcard-multiple" | true + "prefix-@my-scope/any-my_package-with-wildcard-multiple-any" | true + "****@my-scope/****my_package-with-wildcard-multiple****" | true + "prefix-@other-scope/any-my_package-with-wildcard-multiple-any" | false + + # With wildcard escaped + '@my-scope/my_package-with-wildcard-escaped-\*' | true + '@my-scope/my_package-with-wildcard-escaped-\any-character' | true + '@my-scope/my_package-with-wildcard-escaped-\%' | true + '@my-scope/my_package-with-wildcard-escaped-any-character' | false + + # With underscore + "@my-scope/my_package-with_____underscore" | true + "@my-scope/my_package-with_any_underscore" | false + + # With percent sign + "@my-scope/my_package-with-percent-sign-%" | true + "@my-scope/my_package-with-percent-sign-any-character" | false + + # With percent sign escaped + '@my-scope/my_package-with-percent-sign-escaped-\%' | true + '@my-scope/my_package-with-percent-sign-escaped-\*' | false + '@my-scope/my_package-with-percent-sign-escaped-\any-character' | false + '@my-scope/my_package-with-percent-sign-escaped-any-character' | false + + "@my-scope/my_package-with-unsupported-regex-characters.+" | true + "@my-scope/my_package-with-unsupported-regex-characters." | false + "@my-scope/my_package-with-unsupported-regex-characters" | false + "@my-scope/my_package-with-unsupported-regex-characters-any" | false + + nil | false + "" | false + "any_package" | false end with_them do -- GitLab From 627f230f6f0ebad1c0d698c54c6e2d67fc9dd8a7 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Thu, 6 Jul 2023 16:13:32 +0200 Subject: [PATCH 04/20] refactor: Apply reviewer feedback from @l.rosa --- ...7170000_create_packages_package_protection_rules.rb | 10 +++------- db/structure.sql | 6 +++--- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/db/migrate/20230627170000_create_packages_package_protection_rules.rb b/db/migrate/20230627170000_create_packages_package_protection_rules.rb index 415962028be25d..4482ba88ce01fd 100644 --- a/db/migrate/20230627170000_create_packages_package_protection_rules.rb +++ b/db/migrate/20230627170000_create_packages_package_protection_rules.rb @@ -3,15 +3,11 @@ class CreatePackagesPackageProtectionRules < Gitlab::Database::Migration[2.1] def change create_table :packages_package_protection_rules do |t| - t.text :package_name_pattern, limit: 255 + t.references :project, null: false, default: nil, foreign_key: { on_delete: :cascade } + t.timestamps_with_timezone null: false t.integer :package_type, limit: 2 t.integer :push_protected_up_to_access_level - t.references :project, - null: false, - default: nil, - foreign_key: { on_delete: :cascade } - - t.timestamps_with_timezone null: false + t.text :package_name_pattern, limit: 255 end end end diff --git a/db/structure.sql b/db/structure.sql index 1acd9462feb581..ad46d7d2e48a00 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -20210,12 +20210,12 @@ ALTER SEQUENCE packages_package_files_id_seq OWNED BY packages_package_files.id; CREATE TABLE packages_package_protection_rules ( id bigint NOT NULL, - package_name_pattern text, - package_type smallint, - push_protected_up_to_access_level integer, project_id bigint NOT NULL, created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone NOT NULL, + package_type smallint, + push_protected_up_to_access_level integer, + package_name_pattern text, CONSTRAINT check_4ad7e5d3c3 CHECK ((char_length(package_name_pattern) <= 255)) ); -- GitLab From d1fdef627a53ed609297687ba4d83989b2cdf142 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Wed, 12 Jul 2023 10:49:29 +0200 Subject: [PATCH 05/20] test: Adjusted factory --- spec/factories/packages/package_protection_rules.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/factories/packages/package_protection_rules.rb b/spec/factories/packages/package_protection_rules.rb index e759b0ba65681a..25e99a4e121411 100644 --- a/spec/factories/packages/package_protection_rules.rb +++ b/spec/factories/packages/package_protection_rules.rb @@ -3,7 +3,7 @@ FactoryBot.define do factory :package_protection_rule, class: 'Packages::PackageProtectionRule' do project - package_name_pattern { 'my/company/app/my-app' } + package_name_pattern { '@my_scope/my_package' } package_type { :npm } push_protected_up_to_access_level { Gitlab::Access::DEVELOPER } end -- GitLab From 60e003ecf9263abfef0b8486fdce7f83a49a466c Mon Sep 17 00:00:00 2001 From: Gerardo Date: Wed, 12 Jul 2023 17:14:28 +0200 Subject: [PATCH 06/20] refactor: Use ruby-based matching approach for package protection rules Before, we included the matching logic for package protection rules in a database query. Based on the feedback and the proposed approach form l.rosa, we decided to move the matching logic to Ruby. --- .../packages/package_protection_rule.rb | 25 +-- app/policies/packages/package_policy.rb | 4 +- .../packages/package_protection_rule_spec.rb | 173 ++++++++++-------- 3 files changed, 99 insertions(+), 103 deletions(-) diff --git a/app/models/packages/package_protection_rule.rb b/app/models/packages/package_protection_rule.rb index efd8bbf4ff3906..cb416096be8537 100644 --- a/app/models/packages/package_protection_rule.rb +++ b/app/models/packages/package_protection_rule.rb @@ -17,27 +17,6 @@ class PackageProtectionRule < ApplicationRecord scope :for_package_type, ->(package_type) { where(package_type: package_type) } - # We want to allow wildcard pattern (`*`) for the field `package_name_pattern`, e.g. `@my-scope/my_package_*`, etc. - # Therefore, we need to preprocess the field value before the LIKE clause is evaluated, - # e.g. convert wildcard character (`*`) to LIKE match character (`%`), escape certain character, etc. - scope :for_package_name, ->(package_name) { - where_clause = %q{ - :package_name ILIKE REGEXP_REPLACE( - REGEXP_REPLACE( - REGEXP_REPLACE( - REGEXP_REPLACE( - package_name_pattern, - '\\\\', '\\\\\\\\', 'g' - ), - '\%', '\%', 'g' - ), - '\_', '\_', 'g' - ), - '\*', '%', 'g' - ) - } - where(where_clause, package_name: package_name.presence || "") - } scope :push_protection_applicable_for_access_level, ->(user_access_level) { case user_access_level when nil @@ -49,5 +28,9 @@ class PackageProtectionRule < ApplicationRecord ]) end } + + def self.matching_package_name(package_name, relation = all) + relation.select { |ppr| RefMatcher.new(ppr.package_name_pattern).matches?(package_name) } + end end end diff --git a/app/policies/packages/package_policy.rb b/app/policies/packages/package_policy.rb index b84b223ef6a99e..659a325f5d58f7 100644 --- a/app/policies/packages/package_policy.rb +++ b/app/policies/packages/package_policy.rb @@ -15,9 +15,9 @@ class PackagePolicy < BasePolicy # TODO: Get advice from GitLab team where the best situation for this test is. project.package_protection_rules .for_package_type(@subject.package_type) - .for_package_name(@subject.name) .push_protection_applicable_for_access_level(current_user_project_authorization_access_level) - .exists? + .matching_package_name(@subject.name) + .present? end rule { push_protected_from_user }.prevent :create_package diff --git a/spec/models/packages/package_protection_rule_spec.rb b/spec/models/packages/package_protection_rule_spec.rb index 082ae59dd40d48..947a5383f4b9aa 100644 --- a/spec/models/packages/package_protection_rule_spec.rb +++ b/spec/models/packages/package_protection_rule_spec.rb @@ -50,118 +50,131 @@ end end - describe ".for_package_name" do - context "with several package protection rule scenarios" do - let_it_be(:package_protection_rule) do - create(:package_protection_rule, package_name_pattern: "@my-scope/my_package") - end + describe ".matching_package_name" do + let_it_be(:package_protection_rule) do + create(:package_protection_rule, package_name_pattern: "@my-scope/my_package") + end - let_it_be(:package_protection_rule_with_wildcard_start) do - create(:package_protection_rule, package_name_pattern: "*@my-scope/my_package-with-wildcard-start") - end + let_it_be(:ppr_with_wildcard_start) do + create(:package_protection_rule, package_name_pattern: "*@my-scope/my_package-with-wildcard-start") + end - let_it_be(:package_protection_rule_with_wildcard_end) do - create(:package_protection_rule, package_name_pattern: "@my-scope/my_package-with-wildcard-end*") - end + let_it_be(:ppr_with_wildcard_end) do + create(:package_protection_rule, package_name_pattern: "@my-scope/my_package-with-wildcard-end*") + end - let_it_be(:package_protection_rule_with_wildcard_inbetween) do - create(:package_protection_rule, package_name_pattern: "@my-scope/*my_package-with-wildcard-inbetween") - end + let_it_be(:ppr_with_wildcard_inbetween) do + create(:package_protection_rule, package_name_pattern: "@my-scope/*my_package-with-wildcard-inbetween") + end - let_it_be(:package_protection_rule_with_wildcard_multiples) do - create(:package_protection_rule, package_name_pattern: "**@my-scope/**my_package-with-wildcard-multiple**") - end + let_it_be(:ppr_with_wildcard_multiples) do + create(:package_protection_rule, package_name_pattern: "**@my-scope/**my_package-with-wildcard-multiple**") + end - let_it_be(:package_protection_rule_with_wildcard_escaped) do - create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with-wildcard-escaped-\*') - end + let_it_be(:ppr_with_wildcard_escaped) do + create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with-wildcard-escaped-\*') + end - let_it_be(:package_protection_rule_with_underscore) do - create(:package_protection_rule, package_name_pattern: "@my-scope/my_package-with_____underscore") - end + let_it_be(:ppr_with_underscore) do + create(:package_protection_rule, package_name_pattern: "@my-scope/my_package-with_____underscore") + end - let_it_be(:package_protection_rule_with_percent_sign) do - create(:package_protection_rule, package_name_pattern: "@my-scope/my_package-with-percent-sign-%") - end + let_it_be(:ppr_with_percent_sign) do + create(:package_protection_rule, package_name_pattern: "@my-scope/my_package-with-percent-sign-%") + end - let_it_be(:package_protection_rule_with_percent_sign_escaped) do - create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with-percent-sign-escaped-\%') - end + let_it_be(:ppr_with_percent_sign_escaped) do + create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with-percent-sign-escaped-\%') + end - let_it_be(:package_protection_rule_with_unsupported_regex_characters) do - create(:package_protection_rule, - package_name_pattern: "@my-scope/my_package-with-unsupported-regex-characters.+") - end + let_it_be(:ppr_with_unsupported_regex_characters) do + create(:package_protection_rule, + package_name_pattern: "@my-scope/my_package-with-unsupported-regex-characters.+") + end + + let(:package_name) { package_protection_rule.package_name_pattern } + + subject { described_class.matching_package_name(package_name) } + context "with several package protection rule scenarios" do using RSpec::Parameterized::TableSyntax - where(:package_name, :match_or_not) do - "@my-scope/my_package" | true - "@my-scope/my2package" | false - "@my-scope/my_package-2" | false + where(:package_name, :expected_package_protection_rules) do + "@my-scope/my_package" | [ref(:package_protection_rule)] + "@my-scope/my2package" | [] + "@my-scope/my_package-2" | [] # With wildcard pattern at the start - "@my-scope/my_package-with-wildcard-start" | true - "@my-scope/my_package-with-wildcard-start-any" | false - "prefix-@my-scope/my_package-with-wildcard-start" | true - "prefix-@my-scope/my_package-with-wildcard-start-any" | false + "@my-scope/my_package-with-wildcard-start" | [ref(:ppr_with_wildcard_start)] + "@my-scope/my_package-with-wildcard-start-any" | [] + "prefix-@my-scope/my_package-with-wildcard-start" | [ref(:ppr_with_wildcard_start)] + "prefix-@my-scope/my_package-with-wildcard-start-any" | [] # With wildcard pattern at the end - "@my-scope/my_package-with-wildcard-end" | true - "@my-scope/my_package-with-wildcard-end:1234567890" | true - "prefix-@my-scope/my_package-with-wildcard-end" | false - "prefix-@my-scope/my_package-with-wildcard-end:1234567890" | false + "@my-scope/my_package-with-wildcard-end" | [ref(:ppr_with_wildcard_end)] + "@my-scope/my_package-with-wildcard-end:1234567890" | [ref(:ppr_with_wildcard_end)] + "prefix-@my-scope/my_package-with-wildcard-end" | [] + "prefix-@my-scope/my_package-with-wildcard-end:1234567890" | [] # With wildcard pattern inbetween - "@my-scope/my_package-with-wildcard-inbetween" | true - "@my-scope/any-my_package-with-wildcard-inbetween" | true - "@my-scope/any-my_package-my_package-wildcard-inbetween-any" | false + "@my-scope/my_package-with-wildcard-inbetween" | [ref(:ppr_with_wildcard_inbetween)] + "@my-scope/any-my_package-with-wildcard-inbetween" | [ref(:ppr_with_wildcard_inbetween)] + "@my-scope/any-my_package-my_package-wildcard-inbetween-any" | [] # With multiple wildcard pattern are used - "@my-scope/my_package-with-wildcard-multiple" | true - "prefix-@my-scope/any-my_package-with-wildcard-multiple-any" | true - "****@my-scope/****my_package-with-wildcard-multiple****" | true - "prefix-@other-scope/any-my_package-with-wildcard-multiple-any" | false + "@my-scope/my_package-with-wildcard-multiple" | [ref(:ppr_with_wildcard_multiples)] + "prefix-@my-scope/any-my_package-with-wildcard-multiple-any" | [ref(:ppr_with_wildcard_multiples)] + "****@my-scope/****my_package-with-wildcard-multiple****" | [ref(:ppr_with_wildcard_multiples)] + "prefix-@other-scope/any-my_package-with-wildcard-multiple-any" | [] # With wildcard escaped - '@my-scope/my_package-with-wildcard-escaped-\*' | true - '@my-scope/my_package-with-wildcard-escaped-\any-character' | true - '@my-scope/my_package-with-wildcard-escaped-\%' | true - '@my-scope/my_package-with-wildcard-escaped-any-character' | false + '@my-scope/my_package-with-wildcard-escaped-\*' | [ref(:ppr_with_wildcard_escaped)] + '@my-scope/my_package-with-wildcard-escaped-\any-character' | [ref(:ppr_with_wildcard_escaped)] + '@my-scope/my_package-with-wildcard-escaped-\%' | [ref(:ppr_with_wildcard_escaped)] + '@my-scope/my_package-with-wildcard-escaped-any-character' | [] # With underscore - "@my-scope/my_package-with_____underscore" | true - "@my-scope/my_package-with_any_underscore" | false + "@my-scope/my_package-with_____underscore" | [ref(:ppr_with_underscore)] + "@my-scope/my_package-with_any_underscore" | [] # With percent sign - "@my-scope/my_package-with-percent-sign-%" | true - "@my-scope/my_package-with-percent-sign-any-character" | false + "@my-scope/my_package-with-percent-sign-%" | [ref(:ppr_with_percent_sign)] + "@my-scope/my_package-with-percent-sign-any-character" | [] # With percent sign escaped - '@my-scope/my_package-with-percent-sign-escaped-\%' | true - '@my-scope/my_package-with-percent-sign-escaped-\*' | false - '@my-scope/my_package-with-percent-sign-escaped-\any-character' | false - '@my-scope/my_package-with-percent-sign-escaped-any-character' | false - - "@my-scope/my_package-with-unsupported-regex-characters.+" | true - "@my-scope/my_package-with-unsupported-regex-characters." | false - "@my-scope/my_package-with-unsupported-regex-characters" | false - "@my-scope/my_package-with-unsupported-regex-characters-any" | false - - nil | false - "" | false - "any_package" | false + '@my-scope/my_package-with-percent-sign-escaped-\%' | [ref(:ppr_with_percent_sign_escaped)] + '@my-scope/my_package-with-percent-sign-escaped-\*' | [] + '@my-scope/my_package-with-percent-sign-escaped-\any-character' | [] + '@my-scope/my_package-with-percent-sign-escaped-any-character' | [] + + "@my-scope/my_package-with-unsupported-regex-characters.+" | [ref(:ppr_with_unsupported_regex_characters)] + "@my-scope/my_package-with-unsupported-regex-characters." | [] + "@my-scope/my_package-with-unsupported-regex-characters" | [] + "@my-scope/my_package-with-unsupported-regex-characters-any" | [] + + # Special cases + nil | [] + "" | [] + "any_package" | [] end with_them do - it do - if match_or_not - expect(described_class.for_package_name(package_name)).to exist - else - expect(described_class.for_package_name(package_name)).not_to exist - end - end + it { is_expected.to eq expected_package_protection_rules } + end + end + + context 'with multiple matching package protection rules' do + let!(:package_protection_rule_second_match) do + create(:package_protection_rule, package_name_pattern: "#{package_name}*") end + + it { is_expected.to contain_exactly package_protection_rule_second_match, package_protection_rule } + end + + context 'with scoped relation' do + subject { described_class.matching_package_name(package_name, described_class.none) } + + it { is_expected.to be_empty } end end -- GitLab From 166f6c47a7ff4bfd8cf4a0af904728f6998e5786 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Thu, 13 Jul 2023 11:41:11 +0200 Subject: [PATCH 07/20] refactor: Apply reviewer feedback from @l.rosa This commit also includes: - Updated migration timestamp to keep migration timestamps to within three weeks of the date it is anticipated that the migration will be merged upstream (migration best practice) --- ... 20230713170000_create_packages_package_protection_rules.rb} | 2 ++ db/schema_migrations/20230627170000 | 1 - db/schema_migrations/20230713170000 | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) rename db/migrate/{20230627170000_create_packages_package_protection_rules.rb => 20230713170000_create_packages_package_protection_rules.rb} (95%) delete mode 100644 db/schema_migrations/20230627170000 create mode 100644 db/schema_migrations/20230713170000 diff --git a/db/migrate/20230627170000_create_packages_package_protection_rules.rb b/db/migrate/20230713170000_create_packages_package_protection_rules.rb similarity index 95% rename from db/migrate/20230627170000_create_packages_package_protection_rules.rb rename to db/migrate/20230713170000_create_packages_package_protection_rules.rb index 4482ba88ce01fd..953e3c3ae524ca 100644 --- a/db/migrate/20230627170000_create_packages_package_protection_rules.rb +++ b/db/migrate/20230713170000_create_packages_package_protection_rules.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class CreatePackagesPackageProtectionRules < Gitlab::Database::Migration[2.1] + enable_lock_retries! + def change create_table :packages_package_protection_rules do |t| t.references :project, null: false, default: nil, foreign_key: { on_delete: :cascade } diff --git a/db/schema_migrations/20230627170000 b/db/schema_migrations/20230627170000 deleted file mode 100644 index b3b41be2b57b7a..00000000000000 --- a/db/schema_migrations/20230627170000 +++ /dev/null @@ -1 +0,0 @@ -279b9ab1fe5c7f7d80d51eab38ba3db02def47eb2b80db09d2751fb5868ee60a \ No newline at end of file diff --git a/db/schema_migrations/20230713170000 b/db/schema_migrations/20230713170000 new file mode 100644 index 00000000000000..fd951dbe875802 --- /dev/null +++ b/db/schema_migrations/20230713170000 @@ -0,0 +1 @@ +06a4bec2eb19fbbcbb92db0d15553e7d3bb97f61df8ce50fffac7974dce262e7 \ No newline at end of file -- GitLab From 570ce073c5e5716af115653e752260006221992c Mon Sep 17 00:00:00 2001 From: Phillip Wells Date: Fri, 14 Jul 2023 07:45:37 +0000 Subject: [PATCH 08/20] style: Apply suggestion from @phillipwells --- db/docs/packages_package_protection_rules.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/docs/packages_package_protection_rules.yml b/db/docs/packages_package_protection_rules.yml index d37f2eb0ee476f..a6d04432ee998d 100644 --- a/db/docs/packages_package_protection_rules.yml +++ b/db/docs/packages_package_protection_rules.yml @@ -4,7 +4,7 @@ classes: - Packages::PackageProtectionRule feature_categories: - package_registry -description: Represents the rules for protecting packages package registry +description: Represents the rules for protecting packages package registry. introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124776 milestone: '16.2' gitlab_schema: gitlab_main -- GitLab From ae4ffd2885b32698bfcba20fa60a23bc19bc77d3 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Fri, 14 Jul 2023 09:53:11 +0200 Subject: [PATCH 09/20] refactor: Apply reviewer feedback from @l.rosa, @phillipwells, @euko This commit includes: - Finalized description for new table - Reorderd column order as suggested by database check --- db/docs/packages_package_protection_rules.yml | 2 +- .../20230713170000_create_packages_package_protection_rules.rb | 2 +- db/structure.sql | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/db/docs/packages_package_protection_rules.yml b/db/docs/packages_package_protection_rules.yml index a6d04432ee998d..f80ade41cb6080 100644 --- a/db/docs/packages_package_protection_rules.yml +++ b/db/docs/packages_package_protection_rules.yml @@ -4,7 +4,7 @@ classes: - Packages::PackageProtectionRule feature_categories: - package_registry -description: Represents the rules for protecting packages package registry. +description: Represents package protection rules for package registry. introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124776 milestone: '16.2' gitlab_schema: gitlab_main diff --git a/db/migrate/20230713170000_create_packages_package_protection_rules.rb b/db/migrate/20230713170000_create_packages_package_protection_rules.rb index 953e3c3ae524ca..cc56e68bc5838a 100644 --- a/db/migrate/20230713170000_create_packages_package_protection_rules.rb +++ b/db/migrate/20230713170000_create_packages_package_protection_rules.rb @@ -7,8 +7,8 @@ def change create_table :packages_package_protection_rules do |t| t.references :project, null: false, default: nil, foreign_key: { on_delete: :cascade } t.timestamps_with_timezone null: false - t.integer :package_type, limit: 2 t.integer :push_protected_up_to_access_level + t.integer :package_type, limit: 2 t.text :package_name_pattern, limit: 255 end end diff --git a/db/structure.sql b/db/structure.sql index ad46d7d2e48a00..e5dde667da2594 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -20213,8 +20213,8 @@ CREATE TABLE packages_package_protection_rules ( project_id bigint NOT NULL, created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone NOT NULL, - package_type smallint, push_protected_up_to_access_level integer, + package_type smallint, package_name_pattern text, CONSTRAINT check_4ad7e5d3c3 CHECK ((char_length(package_name_pattern) <= 255)) ); -- GitLab From 141869c859c5e7738cd2d41d8eaff2972d629c5a Mon Sep 17 00:00:00 2001 From: Eulyeon Ko <5961404-euko@users.noreply.gitlab.com> Date: Mon, 17 Jul 2023 12:58:28 +0000 Subject: [PATCH 10/20] refactor: Apply reviewer feedback from @euko - Added index for uniqueness on `package_name_pattern` - Remove unnecessary code comments --- app/models/packages/package_protection_rule.rb | 3 +-- app/models/project.rb | 7 ++----- app/policies/packages/package_policy.rb | 3 --- ...30713170000_create_packages_package_protection_rules.rb | 7 +++++-- db/structure.sql | 4 +++- 5 files changed, 11 insertions(+), 13 deletions(-) diff --git a/app/models/packages/package_protection_rule.rb b/app/models/packages/package_protection_rule.rb index cb416096be8537..d03cf0941585f5 100644 --- a/app/models/packages/package_protection_rule.rb +++ b/app/models/packages/package_protection_rule.rb @@ -8,8 +8,7 @@ class PackageProtectionRule < ApplicationRecord belongs_to :project, optional: false, inverse_of: :package_protection_rules validates :package_name_pattern, presence: true - validates :package_name_pattern, uniqueness: { scope: [:project_id, :package_type] }, - if: :package_name_pattern_changed? + validates :package_name_pattern, uniqueness: { scope: [:project_id, :package_type] } validates :package_type, presence: true validates :push_protected_up_to_access_level, inclusion: { in: [Gitlab::Access::NO_ACCESS, *Gitlab::Access.all_values] } diff --git a/app/models/project.rb b/app/models/project.rb index 9070202753ad0f..bba93e59ec8a43 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -266,11 +266,8 @@ def self.integration_association_name(name) has_many :debian_distributions, class_name: 'Packages::Debian::ProjectDistribution', dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - has_many :npm_metadata_caches, - class_name: 'Packages::Npm::MetadataCache' - has_one :packages_cleanup_policy, - class_name: 'Packages::Cleanup::Policy', - inverse_of: :project + has_many :npm_metadata_caches, class_name: 'Packages::Npm::MetadataCache' + has_one :packages_cleanup_policy, class_name: 'Packages::Cleanup::Policy', inverse_of: :project has_many :package_protection_rules, class_name: 'Packages::PackageProtectionRule', inverse_of: :project diff --git a/app/policies/packages/package_policy.rb b/app/policies/packages/package_policy.rb index 659a325f5d58f7..701dddfe4981df 100644 --- a/app/policies/packages/package_policy.rb +++ b/app/policies/packages/package_policy.rb @@ -10,9 +10,6 @@ class PackagePolicy < BasePolicy current_user_project_authorization_access_level = @user.max_member_access_for_project(project.id) next true if current_user_project_authorization_access_level == Gitlab::Access::NO_ACCESS - # TODO: Add support for `package_version` (later) - # TODO: Consider caching - # TODO: Get advice from GitLab team where the best situation for this test is. project.package_protection_rules .for_package_type(@subject.package_type) .push_protection_applicable_for_access_level(current_user_project_authorization_access_level) diff --git a/db/migrate/20230713170000_create_packages_package_protection_rules.rb b/db/migrate/20230713170000_create_packages_package_protection_rules.rb index cc56e68bc5838a..c60bd165221b2d 100644 --- a/db/migrate/20230713170000_create_packages_package_protection_rules.rb +++ b/db/migrate/20230713170000_create_packages_package_protection_rules.rb @@ -5,11 +5,14 @@ class CreatePackagesPackageProtectionRules < Gitlab::Database::Migration[2.1] def change create_table :packages_package_protection_rules do |t| - t.references :project, null: false, default: nil, foreign_key: { on_delete: :cascade } + t.references :project, null: false, foreign_key: { on_delete: :cascade } t.timestamps_with_timezone null: false - t.integer :push_protected_up_to_access_level + t.integer :push_protected_up_to_access_level, null: false t.integer :package_type, limit: 2 t.text :package_name_pattern, limit: 255 end + + add_index :packages_package_protection_rules, [:project_id, :package_type, :package_name_pattern], unique: true, + name: :i_packages_package_protection_rules_for_unique_pac_pattern_name end end diff --git a/db/structure.sql b/db/structure.sql index e5dde667da2594..3d4d17c679b04b 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -20213,7 +20213,7 @@ CREATE TABLE packages_package_protection_rules ( project_id bigint NOT NULL, created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone NOT NULL, - push_protected_up_to_access_level integer, + push_protected_up_to_access_level integer NOT NULL, package_type smallint, package_name_pattern text, CONSTRAINT check_4ad7e5d3c3 CHECK ((char_length(package_name_pattern) <= 255)) @@ -30446,6 +30446,8 @@ CREATE INDEX i_dast_profiles_tags_on_scanner_profiles_id ON dast_profiles_tags U CREATE INDEX i_dast_scanner_profiles_tags_on_scanner_profiles_id ON dast_scanner_profiles_tags USING btree (dast_scanner_profile_id); +CREATE UNIQUE INDEX i_packages_package_protection_rules_for_unique_pac_pattern_name ON packages_package_protection_rules USING btree (project_id, package_type, package_name_pattern); + CREATE INDEX i_pkgs_deb_file_meta_on_updated_at_package_file_id_when_unknown ON packages_debian_file_metadata USING btree (updated_at, package_file_id) WHERE (file_type = 1); CREATE UNIQUE INDEX i_pm_licenses_on_spdx_identifier ON pm_licenses USING btree (spdx_identifier); -- GitLab From fa42c709c70f346d5bea97b691cb21e3399d4faf Mon Sep 17 00:00:00 2001 From: Eulyeon Ko <5961404-euko@users.noreply.gitlab.com> Date: Tue, 18 Jul 2023 11:04:12 +0000 Subject: [PATCH 11/20] refactor: Apply reviewer feedback from @euko --- app/models/packages/package_protection_rule.rb | 7 ++----- db/docs/packages_package_protection_rules.yml | 2 +- ...30713170000_create_packages_package_protection_rules.rb | 2 +- db/structure.sql | 2 +- spec/models/packages/package_protection_rule_spec.rb | 3 --- 5 files changed, 5 insertions(+), 11 deletions(-) diff --git a/app/models/packages/package_protection_rule.rb b/app/models/packages/package_protection_rule.rb index d03cf0941585f5..e253cbdd625cdd 100644 --- a/app/models/packages/package_protection_rule.rb +++ b/app/models/packages/package_protection_rule.rb @@ -2,17 +2,14 @@ module Packages class PackageProtectionRule < ApplicationRecord - # At the moment, we limit the enum to the package type `npm`. We can extend this at a later stage. enum package_type: Packages::Package.package_types.slice(:npm) belongs_to :project, optional: false, inverse_of: :package_protection_rules - validates :package_name_pattern, presence: true - validates :package_name_pattern, uniqueness: { scope: [:project_id, :package_type] } + validates :package_name_pattern, presence: true, uniqueness: { scope: [:project_id, :package_type] } validates :package_type, presence: true - validates :push_protected_up_to_access_level, + validates :push_protected_up_to_access_level, presence: true, inclusion: { in: [Gitlab::Access::NO_ACCESS, *Gitlab::Access.all_values] } - validates :push_protected_up_to_access_level, presence: true scope :for_package_type, ->(package_type) { where(package_type: package_type) } diff --git a/db/docs/packages_package_protection_rules.yml b/db/docs/packages_package_protection_rules.yml index f80ade41cb6080..06e8b5937a220e 100644 --- a/db/docs/packages_package_protection_rules.yml +++ b/db/docs/packages_package_protection_rules.yml @@ -6,5 +6,5 @@ feature_categories: - package_registry description: Represents package protection rules for package registry. introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124776 -milestone: '16.2' +milestone: '16.3' gitlab_schema: gitlab_main diff --git a/db/migrate/20230713170000_create_packages_package_protection_rules.rb b/db/migrate/20230713170000_create_packages_package_protection_rules.rb index c60bd165221b2d..f85ff1b0254da2 100644 --- a/db/migrate/20230713170000_create_packages_package_protection_rules.rb +++ b/db/migrate/20230713170000_create_packages_package_protection_rules.rb @@ -13,6 +13,6 @@ def change end add_index :packages_package_protection_rules, [:project_id, :package_type, :package_name_pattern], unique: true, - name: :i_packages_package_protection_rules_for_unique_pac_pattern_name + name: :i_packages_unique_project_id_package_type_package_name_pattern end end diff --git a/db/structure.sql b/db/structure.sql index 3d4d17c679b04b..99658963012abf 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -30446,7 +30446,7 @@ CREATE INDEX i_dast_profiles_tags_on_scanner_profiles_id ON dast_profiles_tags U CREATE INDEX i_dast_scanner_profiles_tags_on_scanner_profiles_id ON dast_scanner_profiles_tags USING btree (dast_scanner_profile_id); -CREATE UNIQUE INDEX i_packages_package_protection_rules_for_unique_pac_pattern_name ON packages_package_protection_rules USING btree (project_id, package_type, package_name_pattern); +CREATE UNIQUE INDEX i_packages_unique_project_id_package_type_package_name_pattern ON packages_package_protection_rules USING btree (project_id, package_type, package_name_pattern); CREATE INDEX i_pkgs_deb_file_meta_on_updated_at_package_file_id_when_unknown ON packages_debian_file_metadata USING btree (updated_at, package_file_id) WHERE (file_type = 1); diff --git a/spec/models/packages/package_protection_rule_spec.rb b/spec/models/packages/package_protection_rule_spec.rb index 947a5383f4b9aa..fd068c78a654c2 100644 --- a/spec/models/packages/package_protection_rule_spec.rb +++ b/spec/models/packages/package_protection_rule_spec.rb @@ -18,9 +18,6 @@ describe 'enums' do describe '#package_type' do - # As discussed in https://gitlab.com/groups/gitlab-org/-/epics/5574#note_1437348728, - # we focus on the package type `npm` at the moment. - # Other package types will be added later. it { is_expected.to define_enum_for(:package_type).with_values(npm: 2) } end end -- GitLab From 76d21219cac016d1220e6f3da24c3e27177bcbb3 Mon Sep 17 00:00:00 2001 From: Dzmitry Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com> Date: Fri, 21 Jul 2023 10:46:28 +0000 Subject: [PATCH 12/20] refactor: Apply reviewer feedback from @dmeshcharakou --- .../packages/package_protection_rule.rb | 14 +++++-- app/policies/packages/package_policy.rb | 23 +++++++---- .../packages_protected_packages.yml | 2 +- ...reate_packages_package_protection_rules.rb | 4 +- .../packages/package_protection_rule_spec.rb | 38 +++++++------------ spec/policies/packages/package_policy_spec.rb | 24 +++++++----- 6 files changed, 59 insertions(+), 46 deletions(-) diff --git a/app/models/packages/package_protection_rule.rb b/app/models/packages/package_protection_rule.rb index e253cbdd625cdd..3e2b98525f2e62 100644 --- a/app/models/packages/package_protection_rule.rb +++ b/app/models/packages/package_protection_rule.rb @@ -6,10 +6,16 @@ class PackageProtectionRule < ApplicationRecord belongs_to :project, optional: false, inverse_of: :package_protection_rules - validates :package_name_pattern, presence: true, uniqueness: { scope: [:project_id, :package_type] } + validates :package_name_pattern, presence: true, uniqueness: { scope: [:project_id, :package_type] }, + length: { maximum: 255 } validates :package_type, presence: true validates :push_protected_up_to_access_level, presence: true, - inclusion: { in: [Gitlab::Access::NO_ACCESS, *Gitlab::Access.all_values] } + inclusion: { in: [ + Gitlab::Access::NO_ACCESS, + Gitlab::Access::DEVELOPER, + Gitlab::Access::MAINTAINER, + Gitlab::Access::OWNER + ] } scope :for_package_type, ->(package_type) { where(package_type: package_type) } @@ -26,7 +32,9 @@ class PackageProtectionRule < ApplicationRecord } def self.matching_package_name(package_name, relation = all) - relation.select { |ppr| RefMatcher.new(ppr.package_name_pattern).matches?(package_name) } + relation.select do |package_protection_rule| + RefMatcher.new(package_protection_rule.package_name_pattern).matches?(package_name) + end end end end diff --git a/app/policies/packages/package_policy.rb b/app/policies/packages/package_policy.rb index 701dddfe4981df..c7a80b87d8465b 100644 --- a/app/policies/packages/package_policy.rb +++ b/app/policies/packages/package_policy.rb @@ -1,26 +1,35 @@ # frozen_string_literal: true module Packages class PackagePolicy < BasePolicy - delegate { @subject.project&.packages_policy_subject } - delegate { @subject.project } + delegate { subject.project&.packages_policy_subject } + delegate { subject.project } condition(:push_protected_from_user) do - next false if Feature.disabled?(:packages_protected_packages) + next false if Feature.disabled?(:packages_protected_packages, project) - current_user_project_authorization_access_level = @user.max_member_access_for_project(project.id) + # TODO: We will tackle this case as part of another issue + # See link https://gitlab.com/gitlab-org/gitlab/-/issues/419537 + # For now, we just disregard deploy tokens and ensure the policy does not break + # when a deploy token is passed / given. + next false if user.is_a?(DeployToken) + + current_user_project_authorization_access_level = + user.can_admin_all_resources? ? Gitlab::Access::ADMIN : user.max_member_access_for_project(project.id) next true if current_user_project_authorization_access_level == Gitlab::Access::NO_ACCESS project.package_protection_rules - .for_package_type(@subject.package_type) + .for_package_type(subject.package_type) .push_protection_applicable_for_access_level(current_user_project_authorization_access_level) - .matching_package_name(@subject.name) + .matching_package_name(subject.name) .present? end rule { push_protected_from_user }.prevent :create_package + private + def project - @subject.project + subject.project end end end diff --git a/config/feature_flags/development/packages_protected_packages.yml b/config/feature_flags/development/packages_protected_packages.yml index 6e997b2f8c733d..cde6a2fb4a2b61 100644 --- a/config/feature_flags/development/packages_protected_packages.yml +++ b/config/feature_flags/development/packages_protected_packages.yml @@ -2,7 +2,7 @@ name: packages_protected_packages introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124776 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/416395 -milestone: '16.2' +milestone: '16.3' type: development group: group::package registry default_enabled: false diff --git a/db/migrate/20230713170000_create_packages_package_protection_rules.rb b/db/migrate/20230713170000_create_packages_package_protection_rules.rb index f85ff1b0254da2..96d7acfd469850 100644 --- a/db/migrate/20230713170000_create_packages_package_protection_rules.rb +++ b/db/migrate/20230713170000_create_packages_package_protection_rules.rb @@ -8,8 +8,8 @@ def change t.references :project, null: false, foreign_key: { on_delete: :cascade } t.timestamps_with_timezone null: false t.integer :push_protected_up_to_access_level, null: false - t.integer :package_type, limit: 2 - t.text :package_name_pattern, limit: 255 + t.integer :package_type, limit: 2, null: false + t.text :package_name_pattern, limit: 255, null: false end add_index :packages_package_protection_rules, [:project_id, :package_type, :package_name_pattern], unique: true, diff --git a/spec/models/packages/package_protection_rule_spec.rb b/spec/models/packages/package_protection_rule_spec.rb index fd068c78a654c2..2f0bb7533779e0 100644 --- a/spec/models/packages/package_protection_rule_spec.rb +++ b/spec/models/packages/package_protection_rule_spec.rb @@ -5,13 +5,6 @@ RSpec.describe Packages::PackageProtectionRule, type: :model, feature_category: :package_registry do it_behaves_like 'having unique enum values' - describe 'factories' do - it do - package_protection_rule = create(:package_protection_rule) - expect(package_protection_rule).to be_valid - end - end - describe 'relationships' do it { is_expected.to belong_to(:project).inverse_of(:package_protection_rules).required } end @@ -25,26 +18,23 @@ describe 'validations' do subject { build(:package_protection_rule) } - it { is_expected.to validate_presence_of(:package_name_pattern) } - it { is_expected.to validate_presence_of(:package_type) } - it { is_expected.to validate_presence_of(:push_protected_up_to_access_level) } - it { is_expected.to validate_uniqueness_of(:package_name_pattern).scoped_to(:project_id, :package_type) } - describe '#package_name_pattern' do - it { is_expected.to allow_value("@my-scope/my_package").for(:package_name_pattern) } - it { is_expected.to allow_value("@my-scope/my_package*").for(:package_name_pattern) } - it { is_expected.to allow_value("*****@my-scope/my_package*****").for(:package_name_pattern) } - - # Special cases - it { is_expected.to allow_value("@my-scope/my package with whitespace").for(:package_name_pattern) } - it { is_expected.to allow_value("@my-scope/my_package-with-umlauts-üäö").for(:package_name_pattern) } - - it do - is_expected.to( - allow_value("@my-scope/my_package-with-special-characters-~!@#$%^&()<>?").for(:package_name_pattern) - ) + it { is_expected.to validate_presence_of(:package_name_pattern) } + it { is_expected.to validate_uniqueness_of(:package_name_pattern).scoped_to(:project_id, :package_type) } + it { is_expected.to validate_length_of(:package_name_pattern).is_at_most(255) } + + it 'allows wildcard character' do + is_expected.to allow_value("my/domain/com/my-package*").for(:package_name_pattern) end end + + describe '#package_type' do + it { is_expected.to validate_presence_of(:package_type) } + end + + describe '#push_protected_up_to_access_level' do + it { is_expected.to validate_presence_of(:push_protected_up_to_access_level) } + end end describe ".matching_package_name" do diff --git a/spec/policies/packages/package_policy_spec.rb b/spec/policies/packages/package_policy_spec.rb index 53eb6c298dad29..fd7538f658f892 100644 --- a/spec/policies/packages/package_policy_spec.rb +++ b/spec/policies/packages/package_policy_spec.rb @@ -53,7 +53,7 @@ it { is_expected.to be_allowed(:create_package) } end - describe "permission table" do + describe "permission table", :enable_admin_mode do let_it_be(:package_private_project) { create(:npm_package, project: private_project) } let(:access_level_developer) { Gitlab::Access::DEVELOPER } @@ -71,19 +71,16 @@ ref(:developer) | ref(:package_private_project) | ref(:access_level_developer) | false ref(:maintainer) | ref(:package_private_project) | ref(:access_level_developer) | true ref(:owner) | ref(:package_private_project) | ref(:access_level_developer) | true - # TODO: Tbd. behavior of user access level `admin` - # ref(:admin) | ref(:package_private_project) | ref(:access_level_developer) | true + ref(:admin) | ref(:package_private_project) | ref(:access_level_developer) | true ref(:developer) | ref(:package_private_project) | ref(:access_level_maintainer) | false ref(:maintainer) | ref(:package_private_project) | ref(:access_level_maintainer) | false ref(:owner) | ref(:package_private_project) | ref(:access_level_maintainer) | true - # TODO: Tbd. behavior of user access level `admin` - # ref(:admin) | ref(:package_private_project) | ref(:access_level_maintainer) | true + ref(:admin) | ref(:package_private_project) | ref(:access_level_maintainer) | true ref(:maintainer) | ref(:package_private_project) | ref(:access_level_owner) | false ref(:owner) | ref(:package_private_project) | ref(:access_level_owner) | false - # TODO: Tbd. behavior of user access level `admin` - # ref(:admin) | ref(:package_private_project) | ref(:access_level_owner) | false + ref(:admin) | ref(:package_private_project) | ref(:access_level_owner) | true ref(:anonymous) | ref(:package_private_project) | ref(:access_level_no_access) | false ref(:non_member) | ref(:package_private_project) | ref(:access_level_no_access) | false @@ -92,8 +89,7 @@ ref(:developer) | ref(:package_private_project) | ref(:access_level_no_access) | false ref(:maintainer) | ref(:package_private_project) | ref(:access_level_no_access) | false ref(:owner) | ref(:package_private_project) | ref(:access_level_no_access) | false - # TODO: Tbd. behavior of user access level `admin` - # ref(:admin) | ref(:package_private_project) | ref(:access_level_owner) | false + ref(:admin) | ref(:package_private_project) | ref(:access_level_no_access) | false # # TODO: Tbd. behavior for packages of internal projects # ref(:developer) | ref(:package_internal_project) | ref(:access_level_developer) | false @@ -150,6 +146,16 @@ it { is_expected.to be_allowed(:create_package) } end + + context 'with deploy token' do + subject { described_class.new(deploy_token, package) } + + context 'with active write_package_registry_deploy_token scope' do + let(:deploy_token) { create(:deploy_token, :project, write_package_registry: true, projects: [project]) } + + it { is_expected.to be_allowed(:create_package) } + end + end end end end -- GitLab From e5ac5242470dc1e300d37cea7a18870f39699bc4 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Thu, 27 Jul 2023 18:10:10 +0200 Subject: [PATCH 13/20] refactor: Apply reviewer feedback from @dmeshcharakou This commit includes: - Resolving review aspect related to package protection from GitLab admins => ANSWER: No, package protection rules should be disregarded for GitLab admins - Resolving review aspect related to implementation of PackagePolicy => ANSWER: Remove and keep PackagePolicy out of this MR - Updating the migration to the latest datetime to keep it up to date - Rebase from master --- .../packages/package_protection_rule.rb | 4 +- app/policies/packages/package_policy.rb | 31 +--- ...eate_packages_package_protection_rules.rb} | 0 db/schema_migrations/20230713170000 | 1 - db/schema_migrations/20230803170000 | 1 + db/structure.sql | 4 +- .../packages/package_protection_rule_spec.rb | 10 -- spec/policies/packages/package_policy_spec.rb | 158 ++---------------- 8 files changed, 18 insertions(+), 191 deletions(-) rename db/migrate/{20230713170000_create_packages_package_protection_rules.rb => 20230803170000_create_packages_package_protection_rules.rb} (100%) delete mode 100644 db/schema_migrations/20230713170000 create mode 100644 db/schema_migrations/20230803170000 diff --git a/app/models/packages/package_protection_rule.rb b/app/models/packages/package_protection_rule.rb index 3e2b98525f2e62..02b75556ab9fdb 100644 --- a/app/models/packages/package_protection_rule.rb +++ b/app/models/packages/package_protection_rule.rb @@ -31,8 +31,8 @@ class PackageProtectionRule < ApplicationRecord end } - def self.matching_package_name(package_name, relation = all) - relation.select do |package_protection_rule| + def self.matching_package_name(package_name) + all.select do |package_protection_rule| RefMatcher.new(package_protection_rule.package_name_pattern).matches?(package_name) end end diff --git a/app/policies/packages/package_policy.rb b/app/policies/packages/package_policy.rb index c7a80b87d8465b..829d62a6430551 100644 --- a/app/policies/packages/package_policy.rb +++ b/app/policies/packages/package_policy.rb @@ -1,35 +1,6 @@ # frozen_string_literal: true module Packages class PackagePolicy < BasePolicy - delegate { subject.project&.packages_policy_subject } - delegate { subject.project } - - condition(:push_protected_from_user) do - next false if Feature.disabled?(:packages_protected_packages, project) - - # TODO: We will tackle this case as part of another issue - # See link https://gitlab.com/gitlab-org/gitlab/-/issues/419537 - # For now, we just disregard deploy tokens and ensure the policy does not break - # when a deploy token is passed / given. - next false if user.is_a?(DeployToken) - - current_user_project_authorization_access_level = - user.can_admin_all_resources? ? Gitlab::Access::ADMIN : user.max_member_access_for_project(project.id) - next true if current_user_project_authorization_access_level == Gitlab::Access::NO_ACCESS - - project.package_protection_rules - .for_package_type(subject.package_type) - .push_protection_applicable_for_access_level(current_user_project_authorization_access_level) - .matching_package_name(subject.name) - .present? - end - - rule { push_protected_from_user }.prevent :create_package - - private - - def project - subject.project - end + delegate { @subject.project&.packages_policy_subject } end end diff --git a/db/migrate/20230713170000_create_packages_package_protection_rules.rb b/db/migrate/20230803170000_create_packages_package_protection_rules.rb similarity index 100% rename from db/migrate/20230713170000_create_packages_package_protection_rules.rb rename to db/migrate/20230803170000_create_packages_package_protection_rules.rb diff --git a/db/schema_migrations/20230713170000 b/db/schema_migrations/20230713170000 deleted file mode 100644 index fd951dbe875802..00000000000000 --- a/db/schema_migrations/20230713170000 +++ /dev/null @@ -1 +0,0 @@ -06a4bec2eb19fbbcbb92db0d15553e7d3bb97f61df8ce50fffac7974dce262e7 \ No newline at end of file diff --git a/db/schema_migrations/20230803170000 b/db/schema_migrations/20230803170000 new file mode 100644 index 00000000000000..2f876ba0546785 --- /dev/null +++ b/db/schema_migrations/20230803170000 @@ -0,0 +1 @@ +83d7072b8f08daf4165f43dccc872974640bbf9aace04c72e0e3e381dbf7a1d7 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 99658963012abf..d8cfdc7b84e0e9 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -20214,8 +20214,8 @@ CREATE TABLE packages_package_protection_rules ( created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone NOT NULL, push_protected_up_to_access_level integer NOT NULL, - package_type smallint, - package_name_pattern text, + package_type smallint NOT NULL, + package_name_pattern text NOT NULL, CONSTRAINT check_4ad7e5d3c3 CHECK ((char_length(package_name_pattern) <= 255)) ); diff --git a/spec/models/packages/package_protection_rule_spec.rb b/spec/models/packages/package_protection_rule_spec.rb index 2f0bb7533779e0..2e41e4e521da2b 100644 --- a/spec/models/packages/package_protection_rule_spec.rb +++ b/spec/models/packages/package_protection_rule_spec.rb @@ -22,10 +22,6 @@ it { is_expected.to validate_presence_of(:package_name_pattern) } it { is_expected.to validate_uniqueness_of(:package_name_pattern).scoped_to(:project_id, :package_type) } it { is_expected.to validate_length_of(:package_name_pattern).is_at_most(255) } - - it 'allows wildcard character' do - is_expected.to allow_value("my/domain/com/my-package*").for(:package_name_pattern) - end end describe '#package_type' do @@ -157,12 +153,6 @@ it { is_expected.to contain_exactly package_protection_rule_second_match, package_protection_rule } end - - context 'with scoped relation' do - subject { described_class.matching_package_name(package_name, described_class.none) } - - it { is_expected.to be_empty } - end end describe ".push_protection_applicable_for_access_level" do diff --git a/spec/policies/packages/package_policy_spec.rb b/spec/policies/packages/package_policy_spec.rb index fd7538f658f892..13935974b440af 100644 --- a/spec/policies/packages/package_policy_spec.rb +++ b/spec/policies/packages/package_policy_spec.rb @@ -2,160 +2,26 @@ require 'spec_helper' -RSpec.describe Packages::PackagePolicy, feature_category: :package_registry do - include_context 'ProjectPolicy context' - - let(:package) { build(:package, project: project) } - let(:project) { private_project } +RSpec.describe Packages::PackagePolicy do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:package) { create(:package, project: project) } subject(:policy) { described_class.new(user, package) } - describe 'ability :read_package' do - context 'when the user is part of the project' do - let(:user) { reporter } - - it 'allows read_package' do - expect(policy).to be_allowed(:read_package) - end + context 'when the user is part of the project' do + before do + project.add_reporter(user) end - context 'when the user is not part of the project' do - let(:user) { non_member } - - it 'disallows read_package for any Package' do - expect(policy).to be_disallowed(:read_package) - end + it 'allows read_package' do + expect(policy).to be_allowed(:read_package) end end - describe 'ability :create_package' do - let(:user) { developer } - - context "without package protection rules" do - it { is_expected.to be_allowed(:create_package) } - end - - context "with package protection rules" do - let(:package) { build(:npm_package, project: project) } - - context "when feature flag disabled" do - before do - stub_feature_flags(packages_protected_packages: false) - - create(:package_protection_rule, - package_name_pattern: package.name, - package_type: package.package_type, - project: package.project, - push_protected_up_to_access_level: Gitlab::Access::NO_ACCESS - ) - end - - it { is_expected.to be_allowed(:create_package) } - end - - describe "permission table", :enable_admin_mode do - let_it_be(:package_private_project) { create(:npm_package, project: private_project) } - - let(:access_level_developer) { Gitlab::Access::DEVELOPER } - let(:access_level_maintainer) { Gitlab::Access::MAINTAINER } - let(:access_level_no_access) { Gitlab::Access::NO_ACCESS } - let(:access_level_owner) { Gitlab::Access::OWNER } - - using RSpec::Parameterized::TableSyntax - - where(:user, :package, :push_protected_up_to_access_level, :expect_to_be_allowed) do - ref(:anonymous) | ref(:package_private_project) | ref(:access_level_developer) | false - ref(:non_member) | ref(:package_private_project) | ref(:access_level_developer) | false - ref(:guest) | ref(:package_private_project) | ref(:access_level_developer) | false - ref(:reporter) | ref(:package_private_project) | ref(:access_level_developer) | false - ref(:developer) | ref(:package_private_project) | ref(:access_level_developer) | false - ref(:maintainer) | ref(:package_private_project) | ref(:access_level_developer) | true - ref(:owner) | ref(:package_private_project) | ref(:access_level_developer) | true - ref(:admin) | ref(:package_private_project) | ref(:access_level_developer) | true - - ref(:developer) | ref(:package_private_project) | ref(:access_level_maintainer) | false - ref(:maintainer) | ref(:package_private_project) | ref(:access_level_maintainer) | false - ref(:owner) | ref(:package_private_project) | ref(:access_level_maintainer) | true - ref(:admin) | ref(:package_private_project) | ref(:access_level_maintainer) | true - - ref(:maintainer) | ref(:package_private_project) | ref(:access_level_owner) | false - ref(:owner) | ref(:package_private_project) | ref(:access_level_owner) | false - ref(:admin) | ref(:package_private_project) | ref(:access_level_owner) | true - - ref(:anonymous) | ref(:package_private_project) | ref(:access_level_no_access) | false - ref(:non_member) | ref(:package_private_project) | ref(:access_level_no_access) | false - ref(:guest) | ref(:package_private_project) | ref(:access_level_no_access) | false - ref(:reporter) | ref(:package_private_project) | ref(:access_level_no_access) | false - ref(:developer) | ref(:package_private_project) | ref(:access_level_no_access) | false - ref(:maintainer) | ref(:package_private_project) | ref(:access_level_no_access) | false - ref(:owner) | ref(:package_private_project) | ref(:access_level_no_access) | false - ref(:admin) | ref(:package_private_project) | ref(:access_level_no_access) | false - - # # TODO: Tbd. behavior for packages of internal projects - # ref(:developer) | ref(:package_internal_project) | ref(:access_level_developer) | false - # ... - - # # TODO: Tbd. behavior for packages of public projects - # ref(:developer) | ref(:package_public_project) | ref(:access_level_developer) | false - # ... - end - - with_them do - before do - create(:package_protection_rule, - package_name_pattern: package.name, - package_type: package.package_type, - project: package.project, - push_protected_up_to_access_level: push_protected_up_to_access_level - ) - end - - it do - if expect_to_be_allowed - is_expected.to be_allowed(:create_package) - else - is_expected.to be_disallowed(:create_package) - end - end - end - end - - context "with wildcard package name pattern" do - before do - create(:package_protection_rule, - # Convert the given package name into a `package_name_pattern` with a wildcard - package_name_pattern: "#{package.name[..-3]}*", - package_type: package.package_type, - project: package.project, - push_protected_up_to_access_level: Gitlab::Access::DEVELOPER - ) - end - - it { is_expected.to be_disallowed(:create_package) } - end - - context "with no matching package protection rule found" do - before do - create(:package_protection_rule, - package_name_pattern: "#{package.name}_no_match", - package_type: package.package_type, - project: package.project, - push_protected_up_to_access_level: Gitlab::Access::DEVELOPER - ) - end - - it { is_expected.to be_allowed(:create_package) } - end - - context 'with deploy token' do - subject { described_class.new(deploy_token, package) } - - context 'with active write_package_registry_deploy_token scope' do - let(:deploy_token) { create(:deploy_token, :project, write_package_registry: true, projects: [project]) } - - it { is_expected.to be_allowed(:create_package) } - end - end + context 'when the user is not part of the project' do + it 'disallows read_package for any Package' do + expect(policy).to be_disallowed(:read_package) end end end -- GitLab From 72b7c39c6b79e6b81c53f28c5b0ad1287310cfee Mon Sep 17 00:00:00 2001 From: Gerardo Date: Fri, 4 Aug 2023 22:48:31 +0200 Subject: [PATCH 14/20] refactor: Apply reviewer feedback from @dmeshcharakou This commit includes: - Removing the feature flag as it is not needed anymore - Adjusting the class method `self.matching_package_name` to receive a scoped collection (ActiveRecord_Relation) - Remove value `Gitlab::Access::NO_ACCESS` from allowed values for field `:push_protected_up_to_access_level` and adjusted scope `:push_protection_applicable_for_access_level` accordingly - Updating the migration to the latest datetime to keep it up to date - Rebase from master --- .../packages/package_protection_rule.rb | 19 +-- .../packages_protected_packages.yml | 8 - .../packages/package_protection_rule_spec.rb | 151 ++++++++++-------- 3 files changed, 85 insertions(+), 93 deletions(-) delete mode 100644 config/feature_flags/development/packages_protected_packages.yml diff --git a/app/models/packages/package_protection_rule.rb b/app/models/packages/package_protection_rule.rb index 02b75556ab9fdb..e6e440316185f8 100644 --- a/app/models/packages/package_protection_rule.rb +++ b/app/models/packages/package_protection_rule.rb @@ -11,7 +11,6 @@ class PackageProtectionRule < ApplicationRecord validates :package_type, presence: true validates :push_protected_up_to_access_level, presence: true, inclusion: { in: [ - Gitlab::Access::NO_ACCESS, Gitlab::Access::DEVELOPER, Gitlab::Access::MAINTAINER, Gitlab::Access::OWNER @@ -19,20 +18,12 @@ class PackageProtectionRule < ApplicationRecord scope :for_package_type, ->(package_type) { where(package_type: package_type) } - scope :push_protection_applicable_for_access_level, ->(user_access_level) { - case user_access_level - when nil - none - else - where(push_protected_up_to_access_level: [ - Gitlab::Access::NO_ACCESS, - user_access_level.. - ]) - end - } + scope :push_protection_applicable_for_access_level, ->(user_access_level) do + where(push_protected_up_to_access_level: user_access_level..) + end - def self.matching_package_name(package_name) - all.select do |package_protection_rule| + def self.matching_package_name(package_name, scope) + scope.select do |package_protection_rule| RefMatcher.new(package_protection_rule.package_name_pattern).matches?(package_name) end end diff --git a/config/feature_flags/development/packages_protected_packages.yml b/config/feature_flags/development/packages_protected_packages.yml deleted file mode 100644 index cde6a2fb4a2b61..00000000000000 --- a/config/feature_flags/development/packages_protected_packages.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: packages_protected_packages -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124776 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/416395 -milestone: '16.3' -type: development -group: group::package registry -default_enabled: false diff --git a/spec/models/packages/package_protection_rule_spec.rb b/spec/models/packages/package_protection_rule_spec.rb index 2e41e4e521da2b..e2afa58b8ceaa6 100644 --- a/spec/models/packages/package_protection_rule_spec.rb +++ b/spec/models/packages/package_protection_rule_spec.rb @@ -30,28 +30,33 @@ describe '#push_protected_up_to_access_level' do it { is_expected.to validate_presence_of(:push_protected_up_to_access_level) } + + it { + is_expected.to validate_inclusion_of(:push_protected_up_to_access_level).in_array([Gitlab::Access::DEVELOPER, + Gitlab::Access::MAINTAINER, Gitlab::Access::OWNER]) + } end end - describe ".matching_package_name" do + describe '.matching_package_name' do let_it_be(:package_protection_rule) do - create(:package_protection_rule, package_name_pattern: "@my-scope/my_package") + create(:package_protection_rule, package_name_pattern: '@my-scope/my_package') end let_it_be(:ppr_with_wildcard_start) do - create(:package_protection_rule, package_name_pattern: "*@my-scope/my_package-with-wildcard-start") + create(:package_protection_rule, package_name_pattern: '*@my-scope/my_package-with-wildcard-start') end let_it_be(:ppr_with_wildcard_end) do - create(:package_protection_rule, package_name_pattern: "@my-scope/my_package-with-wildcard-end*") + create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with-wildcard-end*') end let_it_be(:ppr_with_wildcard_inbetween) do - create(:package_protection_rule, package_name_pattern: "@my-scope/*my_package-with-wildcard-inbetween") + create(:package_protection_rule, package_name_pattern: '@my-scope/*my_package-with-wildcard-inbetween') end let_it_be(:ppr_with_wildcard_multiples) do - create(:package_protection_rule, package_name_pattern: "**@my-scope/**my_package-with-wildcard-multiple**") + create(:package_protection_rule, package_name_pattern: '**@my-scope/**my_package-with-wildcard-multiple**') end let_it_be(:ppr_with_wildcard_escaped) do @@ -59,11 +64,11 @@ end let_it_be(:ppr_with_underscore) do - create(:package_protection_rule, package_name_pattern: "@my-scope/my_package-with_____underscore") + create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with_____underscore') end let_it_be(:ppr_with_percent_sign) do - create(:package_protection_rule, package_name_pattern: "@my-scope/my_package-with-percent-sign-%") + create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with-percent-sign-%') end let_it_be(:ppr_with_percent_sign_escaped) do @@ -72,43 +77,43 @@ let_it_be(:ppr_with_unsupported_regex_characters) do create(:package_protection_rule, - package_name_pattern: "@my-scope/my_package-with-unsupported-regex-characters.+") + package_name_pattern: '@my-scope/my_package-with-unsupported-regex-characters.+') end let(:package_name) { package_protection_rule.package_name_pattern } - subject { described_class.matching_package_name(package_name) } + subject { described_class.matching_package_name(package_name, described_class.all) } - context "with several package protection rule scenarios" do + context 'with several package protection rule scenarios' do using RSpec::Parameterized::TableSyntax where(:package_name, :expected_package_protection_rules) do - "@my-scope/my_package" | [ref(:package_protection_rule)] - "@my-scope/my2package" | [] - "@my-scope/my_package-2" | [] + '@my-scope/my_package' | [ref(:package_protection_rule)] + '@my-scope/my2package' | [] + '@my-scope/my_package-2' | [] # With wildcard pattern at the start - "@my-scope/my_package-with-wildcard-start" | [ref(:ppr_with_wildcard_start)] - "@my-scope/my_package-with-wildcard-start-any" | [] - "prefix-@my-scope/my_package-with-wildcard-start" | [ref(:ppr_with_wildcard_start)] - "prefix-@my-scope/my_package-with-wildcard-start-any" | [] + '@my-scope/my_package-with-wildcard-start' | [ref(:ppr_with_wildcard_start)] + '@my-scope/my_package-with-wildcard-start-any' | [] + 'prefix-@my-scope/my_package-with-wildcard-start' | [ref(:ppr_with_wildcard_start)] + 'prefix-@my-scope/my_package-with-wildcard-start-any' | [] # With wildcard pattern at the end - "@my-scope/my_package-with-wildcard-end" | [ref(:ppr_with_wildcard_end)] - "@my-scope/my_package-with-wildcard-end:1234567890" | [ref(:ppr_with_wildcard_end)] - "prefix-@my-scope/my_package-with-wildcard-end" | [] - "prefix-@my-scope/my_package-with-wildcard-end:1234567890" | [] + '@my-scope/my_package-with-wildcard-end' | [ref(:ppr_with_wildcard_end)] + '@my-scope/my_package-with-wildcard-end:1234567890' | [ref(:ppr_with_wildcard_end)] + 'prefix-@my-scope/my_package-with-wildcard-end' | [] + 'prefix-@my-scope/my_package-with-wildcard-end:1234567890' | [] # With wildcard pattern inbetween - "@my-scope/my_package-with-wildcard-inbetween" | [ref(:ppr_with_wildcard_inbetween)] - "@my-scope/any-my_package-with-wildcard-inbetween" | [ref(:ppr_with_wildcard_inbetween)] - "@my-scope/any-my_package-my_package-wildcard-inbetween-any" | [] + '@my-scope/my_package-with-wildcard-inbetween' | [ref(:ppr_with_wildcard_inbetween)] + '@my-scope/any-my_package-with-wildcard-inbetween' | [ref(:ppr_with_wildcard_inbetween)] + '@my-scope/any-my_package-my_package-wildcard-inbetween-any' | [] # With multiple wildcard pattern are used - "@my-scope/my_package-with-wildcard-multiple" | [ref(:ppr_with_wildcard_multiples)] - "prefix-@my-scope/any-my_package-with-wildcard-multiple-any" | [ref(:ppr_with_wildcard_multiples)] - "****@my-scope/****my_package-with-wildcard-multiple****" | [ref(:ppr_with_wildcard_multiples)] - "prefix-@other-scope/any-my_package-with-wildcard-multiple-any" | [] + '@my-scope/my_package-with-wildcard-multiple' | [ref(:ppr_with_wildcard_multiples)] + 'prefix-@my-scope/any-my_package-with-wildcard-multiple-any' | [ref(:ppr_with_wildcard_multiples)] + '****@my-scope/****my_package-with-wildcard-multiple****' | [ref(:ppr_with_wildcard_multiples)] + 'prefix-@other-scope/any-my_package-with-wildcard-multiple-any' | [] # With wildcard escaped '@my-scope/my_package-with-wildcard-escaped-\*' | [ref(:ppr_with_wildcard_escaped)] @@ -117,12 +122,12 @@ '@my-scope/my_package-with-wildcard-escaped-any-character' | [] # With underscore - "@my-scope/my_package-with_____underscore" | [ref(:ppr_with_underscore)] - "@my-scope/my_package-with_any_underscore" | [] + '@my-scope/my_package-with_____underscore' | [ref(:ppr_with_underscore)] + '@my-scope/my_package-with_any_underscore' | [] # With percent sign - "@my-scope/my_package-with-percent-sign-%" | [ref(:ppr_with_percent_sign)] - "@my-scope/my_package-with-percent-sign-any-character" | [] + '@my-scope/my_package-with-percent-sign-%' | [ref(:ppr_with_percent_sign)] + '@my-scope/my_package-with-percent-sign-any-character' | [] # With percent sign escaped '@my-scope/my_package-with-percent-sign-escaped-\%' | [ref(:ppr_with_percent_sign_escaped)] @@ -130,15 +135,15 @@ '@my-scope/my_package-with-percent-sign-escaped-\any-character' | [] '@my-scope/my_package-with-percent-sign-escaped-any-character' | [] - "@my-scope/my_package-with-unsupported-regex-characters.+" | [ref(:ppr_with_unsupported_regex_characters)] - "@my-scope/my_package-with-unsupported-regex-characters." | [] - "@my-scope/my_package-with-unsupported-regex-characters" | [] - "@my-scope/my_package-with-unsupported-regex-characters-any" | [] + '@my-scope/my_package-with-unsupported-regex-characters.+' | [ref(:ppr_with_unsupported_regex_characters)] + '@my-scope/my_package-with-unsupported-regex-characters.' | [] + '@my-scope/my_package-with-unsupported-regex-characters' | [] + '@my-scope/my_package-with-unsupported-regex-characters-any' | [] # Special cases nil | [] - "" | [] - "any_package" | [] + '' | [] + 'any_package' | [] end with_them do @@ -153,72 +158,76 @@ it { is_expected.to contain_exactly package_protection_rule_second_match, package_protection_rule } end + + context 'with scope `none`' do + subject { described_class.matching_package_name(package_name, described_class.none) } + + it { is_expected.to be_empty } + end end - describe ".push_protection_applicable_for_access_level" do - context "with different scenarios" do - let_it_be(:ppr_for_developer) do - create(:package_protection_rule, package_name_pattern: "package-name-pattern-for-developer", - push_protected_up_to_access_level: Gitlab::Access::DEVELOPER) - end + describe '.push_protection_applicable_for_access_level' do + let_it_be(:ppr_for_developer) do + create(:package_protection_rule, package_name_pattern: 'package-name-pattern-for-developer', + push_protected_up_to_access_level: Gitlab::Access::DEVELOPER) + end - let_it_be(:ppr_for_maintainer) do - create(:package_protection_rule, package_name_pattern: "package-name-pattern-for-maintainer", - push_protected_up_to_access_level: Gitlab::Access::MAINTAINER) - end + let_it_be(:ppr_for_maintainer) do + create(:package_protection_rule, package_name_pattern: 'package-name-pattern-for-maintainer', + push_protected_up_to_access_level: Gitlab::Access::MAINTAINER) + end - let_it_be(:ppr_for_owner) do - create(:package_protection_rule, package_name_pattern: "package-name-pattern-for-owner", - push_protected_up_to_access_level: Gitlab::Access::OWNER) - end + let_it_be(:ppr_for_owner) do + create(:package_protection_rule, package_name_pattern: 'package-name-pattern-for-owner', + push_protected_up_to_access_level: Gitlab::Access::OWNER) + end - let_it_be(:ppr_for_no_access) do - create(:package_protection_rule, package_name_pattern: "package-name-pattern-for-no_access", - push_protected_up_to_access_level: Gitlab::Access::NO_ACCESS) - end + subject { described_class.push_protection_applicable_for_access_level(push_protected_up_to_access_level) } + context 'with different scenarios' do let_it_be(:access_level_guest) { Gitlab::Access::GUEST } let_it_be(:access_level_reporter) { Gitlab::Access::REPORTER } let_it_be(:access_level_developer) { Gitlab::Access::DEVELOPER } let_it_be(:access_level_maintainer) { Gitlab::Access::MAINTAINER } - let_it_be(:access_level_no_access) { Gitlab::Access::NO_ACCESS } let_it_be(:access_level_owner) { Gitlab::Access::OWNER } + let_it_be(:access_level_admin) { Gitlab::Access::ADMIN } + let_it_be(:access_level_no_access) { Gitlab::Access::NO_ACCESS } using RSpec::Parameterized::TableSyntax # rubocop:disable Layout/LineLength where(:push_protected_up_to_access_level, :expected_package_protection_rules) do - ref(:access_level_guest) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner), ref(:ppr_for_no_access)] - ref(:access_level_reporter) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner), ref(:ppr_for_no_access)] - ref(:access_level_developer) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner), ref(:ppr_for_no_access)] - ref(:access_level_maintainer) | [ref(:ppr_for_maintainer), ref(:ppr_for_owner), ref(:ppr_for_no_access)] - ref(:access_level_owner) | [ref(:ppr_for_owner), ref(:ppr_for_no_access)] + ref(:access_level_guest) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] + ref(:access_level_reporter) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] + ref(:access_level_developer) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] + ref(:access_level_maintainer) | [ref(:ppr_for_maintainer), ref(:ppr_for_owner)] + ref(:access_level_owner) | [ref(:ppr_for_owner)] + ref(:access_level_admin) | [] + ref(:access_level_no_access) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] # Special cases - nil | [] - 0 | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner), ref(:ppr_for_no_access)] - -1 | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner), ref(:ppr_for_no_access)] + nil | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] + 0 | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] + -1 | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] end # rubocop:enable Layout/LineLength with_them do - subject { described_class.push_protection_applicable_for_access_level(push_protected_up_to_access_level) } - it { is_expected.to eq expected_package_protection_rules } end end end - describe ".for_package_type" do + describe '.for_package_type' do let_it_be(:ppr_for_npm) do create(:package_protection_rule, package_type: :npm) end it { expect(described_class.for_package_type(:npm)).to eq [ppr_for_npm] } - it { expect(described_class.for_package_type("npm")).to eq [ppr_for_npm] } + it { expect(described_class.for_package_type('npm')).to eq [ppr_for_npm] } it { expect(described_class.for_package_type(Packages::Package.package_types.fetch(:npm))).to eq [ppr_for_npm] } - it { expect(described_class.for_package_type("")).to eq [] } + it { expect(described_class.for_package_type('')).to eq [] } it { expect(described_class.for_package_type(nil)).to eq [] } it { expect(described_class.for_package_type(Packages::Package.package_types.fetch(:pypi))).to eq [] } end -- GitLab From 21a2cc9ff49b3ad357a9e671721be95c4d2ab96a Mon Sep 17 00:00:00 2001 From: Dzmitry Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com> Date: Fri, 18 Aug 2023 10:46:57 +0000 Subject: [PATCH 15/20] refactor: Apply reviewer feedback from @dmeshcharakou --- spec/models/packages/package_protection_rule_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/packages/package_protection_rule_spec.rb b/spec/models/packages/package_protection_rule_spec.rb index e2afa58b8ceaa6..01d8875a3a46b6 100644 --- a/spec/models/packages/package_protection_rule_spec.rb +++ b/spec/models/packages/package_protection_rule_spec.rb @@ -156,7 +156,7 @@ create(:package_protection_rule, package_name_pattern: "#{package_name}*") end - it { is_expected.to contain_exactly package_protection_rule_second_match, package_protection_rule } + it { is_expected.to contain_exactly(package_protection_rule_second_match, package_protection_rule) } end context 'with scope `none`' do -- GitLab From c6b4176368f3557a45981d344699bc019e734b3a Mon Sep 17 00:00:00 2001 From: David Fernandez Date: Sun, 3 Sep 2023 06:08:10 +0000 Subject: [PATCH 16/20] refactor: Apply reviewer feedback from @10io This commit includes: - Renaming class to `Packages::Protection::Rule` - Adjusting all references after class renaming - Move all unused methods and scopes from the model to keep the MR clean - Updating the migration to the latest datetime to keep it up to date --- .../packages/package_protection_rule.rb | 31 --- app/models/packages/protection.rb | 9 + app/models/packages/protection/rule.rb | 21 ++ app/models/project.rb | 2 +- ...ules.yml => packages_protection_rules.yml} | 6 +- ...70000_create_packages_protection_rules.rb} | 6 +- db/schema_migrations/20230803170000 | 1 - db/schema_migrations/20230903170000 | 1 + db/structure.sql | 56 ++--- .../packages/package_protection_rules.rb | 2 +- .../packages/package_protection_rule_spec.rb | 234 ------------------ spec/models/packages/protection/rule_spec.rb | 40 +++ spec/models/project_spec.rb | 1 + 13 files changed, 108 insertions(+), 302 deletions(-) delete mode 100644 app/models/packages/package_protection_rule.rb create mode 100644 app/models/packages/protection.rb create mode 100644 app/models/packages/protection/rule.rb rename db/docs/{packages_package_protection_rules.yml => packages_protection_rules.yml} (70%) rename db/migrate/{20230803170000_create_packages_package_protection_rules.rb => 20230903170000_create_packages_protection_rules.rb} (64%) delete mode 100644 db/schema_migrations/20230803170000 create mode 100644 db/schema_migrations/20230903170000 delete mode 100644 spec/models/packages/package_protection_rule_spec.rb create mode 100644 spec/models/packages/protection/rule_spec.rb diff --git a/app/models/packages/package_protection_rule.rb b/app/models/packages/package_protection_rule.rb deleted file mode 100644 index e6e440316185f8..00000000000000 --- a/app/models/packages/package_protection_rule.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -module Packages - class PackageProtectionRule < ApplicationRecord - enum package_type: Packages::Package.package_types.slice(:npm) - - belongs_to :project, optional: false, inverse_of: :package_protection_rules - - validates :package_name_pattern, presence: true, uniqueness: { scope: [:project_id, :package_type] }, - length: { maximum: 255 } - validates :package_type, presence: true - validates :push_protected_up_to_access_level, presence: true, - inclusion: { in: [ - Gitlab::Access::DEVELOPER, - Gitlab::Access::MAINTAINER, - Gitlab::Access::OWNER - ] } - - scope :for_package_type, ->(package_type) { where(package_type: package_type) } - - scope :push_protection_applicable_for_access_level, ->(user_access_level) do - where(push_protected_up_to_access_level: user_access_level..) - end - - def self.matching_package_name(package_name, scope) - scope.select do |package_protection_rule| - RefMatcher.new(package_protection_rule.package_name_pattern).matches?(package_name) - end - end - end -end diff --git a/app/models/packages/protection.rb b/app/models/packages/protection.rb new file mode 100644 index 00000000000000..ebaecf89992110 --- /dev/null +++ b/app/models/packages/protection.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module Packages + module Protection + def self.table_name_prefix + 'packages_protection_' + end + end +end diff --git a/app/models/packages/protection/rule.rb b/app/models/packages/protection/rule.rb new file mode 100644 index 00000000000000..66678b88f688eb --- /dev/null +++ b/app/models/packages/protection/rule.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Packages + module Protection + class Rule < ApplicationRecord + enum package_type: Packages::Package.package_types.slice(:npm) + + belongs_to :project, optional: false, inverse_of: :package_protection_rules + + validates :package_name_pattern, presence: true, uniqueness: { scope: [:project_id, :package_type] }, + length: { maximum: 255 } + validates :package_type, presence: true + validates :push_protected_up_to_access_level, presence: true, + inclusion: { in: [ + Gitlab::Access::DEVELOPER, + Gitlab::Access::MAINTAINER, + Gitlab::Access::OWNER + ] } + end + end +end diff --git a/app/models/project.rb b/app/models/project.rb index bba93e59ec8a43..dd4d96a14d6c9d 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -269,7 +269,7 @@ def self.integration_association_name(name) has_many :npm_metadata_caches, class_name: 'Packages::Npm::MetadataCache' has_one :packages_cleanup_policy, class_name: 'Packages::Cleanup::Policy', inverse_of: :project has_many :package_protection_rules, - class_name: 'Packages::PackageProtectionRule', + class_name: 'Packages::Protection::Rule', inverse_of: :project has_one :import_state, autosave: true, class_name: 'ProjectImportState', inverse_of: :project diff --git a/db/docs/packages_package_protection_rules.yml b/db/docs/packages_protection_rules.yml similarity index 70% rename from db/docs/packages_package_protection_rules.yml rename to db/docs/packages_protection_rules.yml index 06e8b5937a220e..3007c956e269e5 100644 --- a/db/docs/packages_package_protection_rules.yml +++ b/db/docs/packages_protection_rules.yml @@ -1,10 +1,10 @@ --- -table_name: packages_package_protection_rules +table_name: packages_protection_rules classes: -- Packages::PackageProtectionRule +- Packages::Protection::Rule feature_categories: - package_registry description: Represents package protection rules for package registry. introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124776 -milestone: '16.3' +milestone: '16.4' gitlab_schema: gitlab_main diff --git a/db/migrate/20230803170000_create_packages_package_protection_rules.rb b/db/migrate/20230903170000_create_packages_protection_rules.rb similarity index 64% rename from db/migrate/20230803170000_create_packages_package_protection_rules.rb rename to db/migrate/20230903170000_create_packages_protection_rules.rb index 96d7acfd469850..e13af11ce4202e 100644 --- a/db/migrate/20230803170000_create_packages_package_protection_rules.rb +++ b/db/migrate/20230903170000_create_packages_protection_rules.rb @@ -1,10 +1,10 @@ # frozen_string_literal: true -class CreatePackagesPackageProtectionRules < Gitlab::Database::Migration[2.1] +class CreatePackagesProtectionRules < Gitlab::Database::Migration[2.1] enable_lock_retries! def change - create_table :packages_package_protection_rules do |t| + create_table :packages_protection_rules do |t| t.references :project, null: false, foreign_key: { on_delete: :cascade } t.timestamps_with_timezone null: false t.integer :push_protected_up_to_access_level, null: false @@ -12,7 +12,7 @@ def change t.text :package_name_pattern, limit: 255, null: false end - add_index :packages_package_protection_rules, [:project_id, :package_type, :package_name_pattern], unique: true, + add_index :packages_protection_rules, [:project_id, :package_type, :package_name_pattern], unique: true, name: :i_packages_unique_project_id_package_type_package_name_pattern end end diff --git a/db/schema_migrations/20230803170000 b/db/schema_migrations/20230803170000 deleted file mode 100644 index 2f876ba0546785..00000000000000 --- a/db/schema_migrations/20230803170000 +++ /dev/null @@ -1 +0,0 @@ -83d7072b8f08daf4165f43dccc872974640bbf9aace04c72e0e3e381dbf7a1d7 \ No newline at end of file diff --git a/db/schema_migrations/20230903170000 b/db/schema_migrations/20230903170000 new file mode 100644 index 00000000000000..5189c60657a9f8 --- /dev/null +++ b/db/schema_migrations/20230903170000 @@ -0,0 +1 @@ +07fdd8579009aa1d68106cc9919d82cfca5702373d8b69c54ea7fa552209d540 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index d8cfdc7b84e0e9..10b7809e89b96e 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -20208,48 +20208,48 @@ CREATE SEQUENCE packages_package_files_id_seq ALTER SEQUENCE packages_package_files_id_seq OWNED BY packages_package_files.id; -CREATE TABLE packages_package_protection_rules ( +CREATE TABLE packages_packages ( id bigint NOT NULL, - project_id bigint NOT NULL, + project_id integer NOT NULL, created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone NOT NULL, - push_protected_up_to_access_level integer NOT NULL, + name character varying NOT NULL, + version character varying, package_type smallint NOT NULL, - package_name_pattern text NOT NULL, - CONSTRAINT check_4ad7e5d3c3 CHECK ((char_length(package_name_pattern) <= 255)) + creator_id integer, + status smallint DEFAULT 0 NOT NULL, + last_downloaded_at timestamp with time zone, + status_message text ); -CREATE SEQUENCE packages_package_protection_rules_id_seq +CREATE SEQUENCE packages_packages_id_seq START WITH 1 INCREMENT BY 1 NO MINVALUE NO MAXVALUE CACHE 1; -ALTER SEQUENCE packages_package_protection_rules_id_seq OWNED BY packages_package_protection_rules.id; +ALTER SEQUENCE packages_packages_id_seq OWNED BY packages_packages.id; -CREATE TABLE packages_packages ( +CREATE TABLE packages_protection_rules ( id bigint NOT NULL, - project_id integer NOT NULL, + project_id bigint NOT NULL, created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone NOT NULL, - name character varying NOT NULL, - version character varying, + push_protected_up_to_access_level integer NOT NULL, package_type smallint NOT NULL, - creator_id integer, - status smallint DEFAULT 0 NOT NULL, - last_downloaded_at timestamp with time zone, - status_message text + package_name_pattern text NOT NULL, + CONSTRAINT check_d2d75d206d CHECK ((char_length(package_name_pattern) <= 255)) ); -CREATE SEQUENCE packages_packages_id_seq +CREATE SEQUENCE packages_protection_rules_id_seq START WITH 1 INCREMENT BY 1 NO MINVALUE NO MAXVALUE CACHE 1; -ALTER SEQUENCE packages_packages_id_seq OWNED BY packages_packages.id; +ALTER SEQUENCE packages_protection_rules_id_seq OWNED BY packages_protection_rules.id; CREATE TABLE packages_pypi_metadata ( package_id bigint NOT NULL, @@ -26136,10 +26136,10 @@ ALTER TABLE ONLY packages_package_file_build_infos ALTER COLUMN id SET DEFAULT n ALTER TABLE ONLY packages_package_files ALTER COLUMN id SET DEFAULT nextval('packages_package_files_id_seq'::regclass); -ALTER TABLE ONLY packages_package_protection_rules ALTER COLUMN id SET DEFAULT nextval('packages_package_protection_rules_id_seq'::regclass); - ALTER TABLE ONLY packages_packages ALTER COLUMN id SET DEFAULT nextval('packages_packages_id_seq'::regclass); +ALTER TABLE ONLY packages_protection_rules ALTER COLUMN id SET DEFAULT nextval('packages_protection_rules_id_seq'::regclass); + ALTER TABLE ONLY packages_rpm_repository_files ALTER COLUMN id SET DEFAULT nextval('packages_rpm_repository_files_id_seq'::regclass); ALTER TABLE ONLY packages_tags ALTER COLUMN id SET DEFAULT nextval('packages_tags_id_seq'::regclass); @@ -28477,12 +28477,12 @@ ALTER TABLE ONLY packages_package_file_build_infos ALTER TABLE ONLY packages_package_files ADD CONSTRAINT packages_package_files_pkey PRIMARY KEY (id); -ALTER TABLE ONLY packages_package_protection_rules - ADD CONSTRAINT packages_package_protection_rules_pkey PRIMARY KEY (id); - ALTER TABLE ONLY packages_packages ADD CONSTRAINT packages_packages_pkey PRIMARY KEY (id); +ALTER TABLE ONLY packages_protection_rules + ADD CONSTRAINT packages_protection_rules_pkey PRIMARY KEY (id); + ALTER TABLE ONLY packages_pypi_metadata ADD CONSTRAINT packages_pypi_metadata_pkey PRIMARY KEY (package_id); @@ -30446,7 +30446,7 @@ CREATE INDEX i_dast_profiles_tags_on_scanner_profiles_id ON dast_profiles_tags U CREATE INDEX i_dast_scanner_profiles_tags_on_scanner_profiles_id ON dast_scanner_profiles_tags USING btree (dast_scanner_profile_id); -CREATE UNIQUE INDEX i_packages_unique_project_id_package_type_package_name_pattern ON packages_package_protection_rules USING btree (project_id, package_type, package_name_pattern); +CREATE UNIQUE INDEX i_packages_unique_project_id_package_type_package_name_pattern ON packages_protection_rules USING btree (project_id, package_type, package_name_pattern); CREATE INDEX i_pkgs_deb_file_meta_on_updated_at_package_file_id_when_unknown ON packages_debian_file_metadata USING btree (updated_at, package_file_id) WHERE (file_type = 1); @@ -32918,8 +32918,6 @@ CREATE INDEX index_packages_package_files_on_status ON packages_package_files US CREATE INDEX index_packages_package_files_on_verification_state ON packages_package_files USING btree (verification_state); -CREATE INDEX index_packages_package_protection_rules_on_project_id ON packages_package_protection_rules USING btree (project_id); - CREATE INDEX index_packages_packages_on_creator_id ON packages_packages USING btree (creator_id); CREATE INDEX index_packages_packages_on_id_and_created_at ON packages_packages USING btree (id, created_at); @@ -32940,6 +32938,8 @@ CREATE INDEX index_packages_packages_on_project_id_and_version ON packages_packa CREATE INDEX index_packages_project_id_name_partial_for_nuget ON packages_packages USING btree (project_id, name) WHERE (((name)::text <> 'NuGet.Temporary.Package'::text) AND (version IS NOT NULL) AND (package_type = 4)); +CREATE INDEX index_packages_protection_rules_on_project_id ON packages_protection_rules USING btree (project_id); + CREATE INDEX index_packages_rpm_metadata_on_package_id ON packages_rpm_metadata USING btree (package_id); CREATE INDEX index_packages_rpm_repository_files_on_project_id_and_file_name ON packages_rpm_repository_files USING btree (project_id, file_name); @@ -37330,9 +37330,6 @@ ALTER TABLE ONLY project_authorizations ALTER TABLE ONLY boards_epic_lists ADD CONSTRAINT fk_rails_0f9c7f646f FOREIGN KEY (epic_board_id) REFERENCES boards_epic_boards(id) ON DELETE CASCADE; -ALTER TABLE ONLY packages_package_protection_rules - ADD CONSTRAINT fk_rails_0fa05e57a5 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; - ALTER TABLE ONLY issue_email_participants ADD CONSTRAINT fk_rails_0fdfd8b811 FOREIGN KEY (issue_id) REFERENCES issues(id) ON DELETE CASCADE; @@ -38794,6 +38791,9 @@ ALTER TABLE ONLY clusters_integration_prometheus ALTER TABLE ONLY vulnerability_occurrence_identifiers ADD CONSTRAINT fk_rails_e4ef6d027c FOREIGN KEY (occurrence_id) REFERENCES vulnerability_occurrences(id) ON DELETE CASCADE; +ALTER TABLE ONLY packages_protection_rules + ADD CONSTRAINT fk_rails_e52adb5267 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; + ALTER TABLE ONLY vulnerability_flags ADD CONSTRAINT fk_rails_e59393b48b FOREIGN KEY (vulnerability_occurrence_id) REFERENCES vulnerability_occurrences(id) ON DELETE CASCADE; diff --git a/spec/factories/packages/package_protection_rules.rb b/spec/factories/packages/package_protection_rules.rb index 25e99a4e121411..3038fb847e7d8d 100644 --- a/spec/factories/packages/package_protection_rules.rb +++ b/spec/factories/packages/package_protection_rules.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true FactoryBot.define do - factory :package_protection_rule, class: 'Packages::PackageProtectionRule' do + factory :package_protection_rule, class: 'Packages::Protection::Rule' do project package_name_pattern { '@my_scope/my_package' } package_type { :npm } diff --git a/spec/models/packages/package_protection_rule_spec.rb b/spec/models/packages/package_protection_rule_spec.rb deleted file mode 100644 index 01d8875a3a46b6..00000000000000 --- a/spec/models/packages/package_protection_rule_spec.rb +++ /dev/null @@ -1,234 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Packages::PackageProtectionRule, type: :model, feature_category: :package_registry do - it_behaves_like 'having unique enum values' - - describe 'relationships' do - it { is_expected.to belong_to(:project).inverse_of(:package_protection_rules).required } - end - - describe 'enums' do - describe '#package_type' do - it { is_expected.to define_enum_for(:package_type).with_values(npm: 2) } - end - end - - describe 'validations' do - subject { build(:package_protection_rule) } - - describe '#package_name_pattern' do - it { is_expected.to validate_presence_of(:package_name_pattern) } - it { is_expected.to validate_uniqueness_of(:package_name_pattern).scoped_to(:project_id, :package_type) } - it { is_expected.to validate_length_of(:package_name_pattern).is_at_most(255) } - end - - describe '#package_type' do - it { is_expected.to validate_presence_of(:package_type) } - end - - describe '#push_protected_up_to_access_level' do - it { is_expected.to validate_presence_of(:push_protected_up_to_access_level) } - - it { - is_expected.to validate_inclusion_of(:push_protected_up_to_access_level).in_array([Gitlab::Access::DEVELOPER, - Gitlab::Access::MAINTAINER, Gitlab::Access::OWNER]) - } - end - end - - describe '.matching_package_name' do - let_it_be(:package_protection_rule) do - create(:package_protection_rule, package_name_pattern: '@my-scope/my_package') - end - - let_it_be(:ppr_with_wildcard_start) do - create(:package_protection_rule, package_name_pattern: '*@my-scope/my_package-with-wildcard-start') - end - - let_it_be(:ppr_with_wildcard_end) do - create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with-wildcard-end*') - end - - let_it_be(:ppr_with_wildcard_inbetween) do - create(:package_protection_rule, package_name_pattern: '@my-scope/*my_package-with-wildcard-inbetween') - end - - let_it_be(:ppr_with_wildcard_multiples) do - create(:package_protection_rule, package_name_pattern: '**@my-scope/**my_package-with-wildcard-multiple**') - end - - let_it_be(:ppr_with_wildcard_escaped) do - create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with-wildcard-escaped-\*') - end - - let_it_be(:ppr_with_underscore) do - create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with_____underscore') - end - - let_it_be(:ppr_with_percent_sign) do - create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with-percent-sign-%') - end - - let_it_be(:ppr_with_percent_sign_escaped) do - create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with-percent-sign-escaped-\%') - end - - let_it_be(:ppr_with_unsupported_regex_characters) do - create(:package_protection_rule, - package_name_pattern: '@my-scope/my_package-with-unsupported-regex-characters.+') - end - - let(:package_name) { package_protection_rule.package_name_pattern } - - subject { described_class.matching_package_name(package_name, described_class.all) } - - context 'with several package protection rule scenarios' do - using RSpec::Parameterized::TableSyntax - - where(:package_name, :expected_package_protection_rules) do - '@my-scope/my_package' | [ref(:package_protection_rule)] - '@my-scope/my2package' | [] - '@my-scope/my_package-2' | [] - - # With wildcard pattern at the start - '@my-scope/my_package-with-wildcard-start' | [ref(:ppr_with_wildcard_start)] - '@my-scope/my_package-with-wildcard-start-any' | [] - 'prefix-@my-scope/my_package-with-wildcard-start' | [ref(:ppr_with_wildcard_start)] - 'prefix-@my-scope/my_package-with-wildcard-start-any' | [] - - # With wildcard pattern at the end - '@my-scope/my_package-with-wildcard-end' | [ref(:ppr_with_wildcard_end)] - '@my-scope/my_package-with-wildcard-end:1234567890' | [ref(:ppr_with_wildcard_end)] - 'prefix-@my-scope/my_package-with-wildcard-end' | [] - 'prefix-@my-scope/my_package-with-wildcard-end:1234567890' | [] - - # With wildcard pattern inbetween - '@my-scope/my_package-with-wildcard-inbetween' | [ref(:ppr_with_wildcard_inbetween)] - '@my-scope/any-my_package-with-wildcard-inbetween' | [ref(:ppr_with_wildcard_inbetween)] - '@my-scope/any-my_package-my_package-wildcard-inbetween-any' | [] - - # With multiple wildcard pattern are used - '@my-scope/my_package-with-wildcard-multiple' | [ref(:ppr_with_wildcard_multiples)] - 'prefix-@my-scope/any-my_package-with-wildcard-multiple-any' | [ref(:ppr_with_wildcard_multiples)] - '****@my-scope/****my_package-with-wildcard-multiple****' | [ref(:ppr_with_wildcard_multiples)] - 'prefix-@other-scope/any-my_package-with-wildcard-multiple-any' | [] - - # With wildcard escaped - '@my-scope/my_package-with-wildcard-escaped-\*' | [ref(:ppr_with_wildcard_escaped)] - '@my-scope/my_package-with-wildcard-escaped-\any-character' | [ref(:ppr_with_wildcard_escaped)] - '@my-scope/my_package-with-wildcard-escaped-\%' | [ref(:ppr_with_wildcard_escaped)] - '@my-scope/my_package-with-wildcard-escaped-any-character' | [] - - # With underscore - '@my-scope/my_package-with_____underscore' | [ref(:ppr_with_underscore)] - '@my-scope/my_package-with_any_underscore' | [] - - # With percent sign - '@my-scope/my_package-with-percent-sign-%' | [ref(:ppr_with_percent_sign)] - '@my-scope/my_package-with-percent-sign-any-character' | [] - - # With percent sign escaped - '@my-scope/my_package-with-percent-sign-escaped-\%' | [ref(:ppr_with_percent_sign_escaped)] - '@my-scope/my_package-with-percent-sign-escaped-\*' | [] - '@my-scope/my_package-with-percent-sign-escaped-\any-character' | [] - '@my-scope/my_package-with-percent-sign-escaped-any-character' | [] - - '@my-scope/my_package-with-unsupported-regex-characters.+' | [ref(:ppr_with_unsupported_regex_characters)] - '@my-scope/my_package-with-unsupported-regex-characters.' | [] - '@my-scope/my_package-with-unsupported-regex-characters' | [] - '@my-scope/my_package-with-unsupported-regex-characters-any' | [] - - # Special cases - nil | [] - '' | [] - 'any_package' | [] - end - - with_them do - it { is_expected.to eq expected_package_protection_rules } - end - end - - context 'with multiple matching package protection rules' do - let!(:package_protection_rule_second_match) do - create(:package_protection_rule, package_name_pattern: "#{package_name}*") - end - - it { is_expected.to contain_exactly(package_protection_rule_second_match, package_protection_rule) } - end - - context 'with scope `none`' do - subject { described_class.matching_package_name(package_name, described_class.none) } - - it { is_expected.to be_empty } - end - end - - describe '.push_protection_applicable_for_access_level' do - let_it_be(:ppr_for_developer) do - create(:package_protection_rule, package_name_pattern: 'package-name-pattern-for-developer', - push_protected_up_to_access_level: Gitlab::Access::DEVELOPER) - end - - let_it_be(:ppr_for_maintainer) do - create(:package_protection_rule, package_name_pattern: 'package-name-pattern-for-maintainer', - push_protected_up_to_access_level: Gitlab::Access::MAINTAINER) - end - - let_it_be(:ppr_for_owner) do - create(:package_protection_rule, package_name_pattern: 'package-name-pattern-for-owner', - push_protected_up_to_access_level: Gitlab::Access::OWNER) - end - - subject { described_class.push_protection_applicable_for_access_level(push_protected_up_to_access_level) } - - context 'with different scenarios' do - let_it_be(:access_level_guest) { Gitlab::Access::GUEST } - let_it_be(:access_level_reporter) { Gitlab::Access::REPORTER } - let_it_be(:access_level_developer) { Gitlab::Access::DEVELOPER } - let_it_be(:access_level_maintainer) { Gitlab::Access::MAINTAINER } - let_it_be(:access_level_owner) { Gitlab::Access::OWNER } - let_it_be(:access_level_admin) { Gitlab::Access::ADMIN } - let_it_be(:access_level_no_access) { Gitlab::Access::NO_ACCESS } - - using RSpec::Parameterized::TableSyntax - - # rubocop:disable Layout/LineLength - where(:push_protected_up_to_access_level, :expected_package_protection_rules) do - ref(:access_level_guest) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] - ref(:access_level_reporter) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] - ref(:access_level_developer) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] - ref(:access_level_maintainer) | [ref(:ppr_for_maintainer), ref(:ppr_for_owner)] - ref(:access_level_owner) | [ref(:ppr_for_owner)] - ref(:access_level_admin) | [] - ref(:access_level_no_access) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] - - # Special cases - nil | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] - 0 | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] - -1 | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] - end - # rubocop:enable Layout/LineLength - - with_them do - it { is_expected.to eq expected_package_protection_rules } - end - end - end - - describe '.for_package_type' do - let_it_be(:ppr_for_npm) do - create(:package_protection_rule, package_type: :npm) - end - - it { expect(described_class.for_package_type(:npm)).to eq [ppr_for_npm] } - it { expect(described_class.for_package_type('npm')).to eq [ppr_for_npm] } - it { expect(described_class.for_package_type(Packages::Package.package_types.fetch(:npm))).to eq [ppr_for_npm] } - - it { expect(described_class.for_package_type('')).to eq [] } - it { expect(described_class.for_package_type(nil)).to eq [] } - it { expect(described_class.for_package_type(Packages::Package.package_types.fetch(:pypi))).to eq [] } - end -end diff --git a/spec/models/packages/protection/rule_spec.rb b/spec/models/packages/protection/rule_spec.rb new file mode 100644 index 00000000000000..cb2218913a6c56 --- /dev/null +++ b/spec/models/packages/protection/rule_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Protection::Rule, type: :model, feature_category: :package_registry do + it_behaves_like 'having unique enum values' + + describe 'relationships' do + it { is_expected.to belong_to(:project).inverse_of(:package_protection_rules).required } + end + + describe 'enums' do + describe '#package_type' do + it { is_expected.to define_enum_for(:package_type).with_values(npm: Packages::Package.package_types[:npm]) } + end + end + + describe 'validations' do + subject { build(:package_protection_rule) } + + describe '#package_name_pattern' do + it { is_expected.to validate_presence_of(:package_name_pattern) } + it { is_expected.to validate_uniqueness_of(:package_name_pattern).scoped_to(:project_id, :package_type) } + it { is_expected.to validate_length_of(:package_name_pattern).is_at_most(255) } + end + + describe '#package_type' do + it { is_expected.to validate_presence_of(:package_type) } + end + + describe '#push_protected_up_to_access_level' do + it { is_expected.to validate_presence_of(:push_protected_up_to_access_level) } + + it { + is_expected.to validate_inclusion_of(:push_protected_up_to_access_level).in_array([Gitlab::Access::DEVELOPER, + Gitlab::Access::MAINTAINER, Gitlab::Access::OWNER]) + } + end + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index db5b5c914c9d06..6193a565d76fb4 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -152,6 +152,7 @@ it { is_expected.to have_many(:debian_distributions).class_name('Packages::Debian::ProjectDistribution').dependent(:destroy) } it { is_expected.to have_many(:npm_metadata_caches).class_name('Packages::Npm::MetadataCache') } it { is_expected.to have_one(:packages_cleanup_policy).class_name('Packages::Cleanup::Policy').inverse_of(:project) } + it { is_expected.to have_many(:package_protection_rules).class_name('Packages::Protection::Rule').inverse_of(:project) } it { is_expected.to have_many(:pipeline_artifacts).dependent(:restrict_with_error) } it { is_expected.to have_many(:terraform_states).class_name('Terraform::State').inverse_of(:project) } it { is_expected.to have_many(:timelogs) } -- GitLab From 7c5e76d1809463f7ef8cb091c8d010f5151c5ec4 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Thu, 7 Sep 2023 13:33:15 +0200 Subject: [PATCH 17/20] refactor: Apply reviewer feedback from @euko This commit includes: - Removes an unnecessary index for `project_id` in table `packages_protection_rules` --- .../20230903170000_create_packages_protection_rules.rb | 2 +- db/structure.sql | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/db/migrate/20230903170000_create_packages_protection_rules.rb b/db/migrate/20230903170000_create_packages_protection_rules.rb index e13af11ce4202e..24dc30dc060ee0 100644 --- a/db/migrate/20230903170000_create_packages_protection_rules.rb +++ b/db/migrate/20230903170000_create_packages_protection_rules.rb @@ -5,7 +5,7 @@ class CreatePackagesProtectionRules < Gitlab::Database::Migration[2.1] def change create_table :packages_protection_rules do |t| - t.references :project, null: false, foreign_key: { on_delete: :cascade } + t.references :project, null: false, index: false, foreign_key: { on_delete: :cascade } t.timestamps_with_timezone null: false t.integer :push_protected_up_to_access_level, null: false t.integer :package_type, limit: 2, null: false diff --git a/db/structure.sql b/db/structure.sql index 10b7809e89b96e..2375c92cfaaeef 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -11942,11 +11942,11 @@ CREATE TABLE application_settings ( package_registry_allow_anyone_to_pull_option boolean DEFAULT true NOT NULL, bulk_import_max_download_file_size bigint DEFAULT 5120 NOT NULL, max_import_remote_file_size bigint DEFAULT 10240 NOT NULL, - sentry_clientside_traces_sample_rate double precision DEFAULT 0.0 NOT NULL, protected_paths_for_get_request text[] DEFAULT '{}'::text[] NOT NULL, max_decompressed_archive_size integer DEFAULT 25600 NOT NULL, - ci_max_total_yaml_size_bytes integer DEFAULT 157286400 NOT NULL, + sentry_clientside_traces_sample_rate double precision DEFAULT 0.0 NOT NULL, prometheus_alert_db_indicators_settings jsonb, + ci_max_total_yaml_size_bytes integer DEFAULT 157286400 NOT NULL, decompress_archive_file_timeout integer DEFAULT 210 NOT NULL, search_rate_limit_allowlist text[] DEFAULT '{}'::text[] NOT NULL, CONSTRAINT app_settings_container_reg_cleanup_tags_max_list_size_positive CHECK ((container_registry_cleanup_tags_service_max_list_size >= 0)), @@ -32938,8 +32938,6 @@ CREATE INDEX index_packages_packages_on_project_id_and_version ON packages_packa CREATE INDEX index_packages_project_id_name_partial_for_nuget ON packages_packages USING btree (project_id, name) WHERE (((name)::text <> 'NuGet.Temporary.Package'::text) AND (version IS NOT NULL) AND (package_type = 4)); -CREATE INDEX index_packages_protection_rules_on_project_id ON packages_protection_rules USING btree (project_id); - CREATE INDEX index_packages_rpm_metadata_on_package_id ON packages_rpm_metadata USING btree (package_id); CREATE INDEX index_packages_rpm_repository_files_on_project_id_and_file_name ON packages_rpm_repository_files USING btree (project_id, file_name); -- GitLab From 4f0890955bd4cad1d1fb381c49f31f3a0ff87d1d Mon Sep 17 00:00:00 2001 From: Eulyeon Ko <5961404-euko@users.noreply.gitlab.com> Date: Fri, 8 Sep 2023 12:31:11 +0000 Subject: [PATCH 18/20] refactor: Apply reviewer feedback from @euko --- app/models/packages/protection/rule.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/packages/protection/rule.rb b/app/models/packages/protection/rule.rb index 66678b88f688eb..bb65be92b90f09 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(:npm) - belongs_to :project, optional: false, inverse_of: :package_protection_rules + belongs_to :project, inverse_of: :package_protection_rules validates :package_name_pattern, presence: true, uniqueness: { scope: [:project_id, :package_type] }, length: { maximum: 255 } -- GitLab From b40d1622b3a3a0b3661292fdd798f5a4ea78931b Mon Sep 17 00:00:00 2001 From: Gerardo Date: Fri, 8 Sep 2023 16:24:15 +0200 Subject: [PATCH 19/20] ci: Fix failing ci test --- spec/models/packages/protection/rule_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/packages/protection/rule_spec.rb b/spec/models/packages/protection/rule_spec.rb index cb2218913a6c56..b368687e6d8051 100644 --- a/spec/models/packages/protection/rule_spec.rb +++ b/spec/models/packages/protection/rule_spec.rb @@ -6,7 +6,7 @@ it_behaves_like 'having unique enum values' describe 'relationships' do - it { is_expected.to belong_to(:project).inverse_of(:package_protection_rules).required } + it { is_expected.to belong_to(:project).inverse_of(:package_protection_rules) } end describe 'enums' do -- GitLab From ab3a553d05e2ef8da8e6718dd90ce03c525eb8b0 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Wed, 12 Jul 2023 17:23:57 +0200 Subject: [PATCH 20/20] refactor: Extract matching logic from policy to keep it lean This MR intends to refactor the `PackagePolicy` because the logic for matching packages was concentrated in the class `PackagePolicy`. However, this class should be kept light-weight. We moved this logic to a separate finder `PackageProtectionRuleMatchingFinder` for better reuse by other components. Changelog: other --- .../package_protection_rule_push_finder.rb | 37 ++++ app/models/packages/package.rb | 7 + app/models/packages/protection/rule.rb | 12 ++ ...ackage_protection_rule_push_finder_spec.rb | 133 ++++++++++++ spec/models/packages/package_spec.rb | 32 +++ spec/models/packages/protection/rule_spec.rb | 194 ++++++++++++++++++ spec/support/finder_collection_allowlist.yml | 1 + 7 files changed, 416 insertions(+) create mode 100644 app/finders/packages/package_protection_rule_push_finder.rb create mode 100644 spec/finders/packages/package_protection_rule_push_finder_spec.rb diff --git a/app/finders/packages/package_protection_rule_push_finder.rb b/app/finders/packages/package_protection_rule_push_finder.rb new file mode 100644 index 00000000000000..ed2b9f8fcd34ab --- /dev/null +++ b/app/finders/packages/package_protection_rule_push_finder.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module Packages + class PackageProtectionRulePushFinder + def initialize(package, current_user) + @current_user = current_user + @package = package + + validate_params! + end + + def execute + current_user_project_authorization_access_level = current_user.max_member_access_for_project(project.id) + + if current_user_project_authorization_access_level == Gitlab::Access::NO_ACCESS + return ::Packages::Protection::Rule.none.to_a + end + + project + .package_protection_rules + .for_package_type(package.package_type) + .push_protection_applicable_for_access_level(current_user_project_authorization_access_level) + .then { |scope| ::Packages::Protection::Rule.matching_package_name(package.name, scope) } + end + + private + + attr_reader :current_user, :package + + delegate :project, to: :package, private: true + + def validate_params! + raise ArgumentError, 'package cannot be nil' unless package + raise ArgumentError, 'current_user cannot be nil' unless current_user + end + end +end diff --git a/app/models/packages/package.rb b/app/models/packages/package.rb index 7665f30be62edb..9583951f894430 100644 --- a/app/models/packages/package.rb +++ b/app/models/packages/package.rb @@ -404,6 +404,13 @@ def publish_creation_event ) end + def push_protected_from?(user) + return true unless user + + # If there is one matching package protection rule, then we consider the package protected + Packages::PackageProtectionRulePushFinder.new(self, user).execute.present? + end + private def composer_tag_version? diff --git a/app/models/packages/protection/rule.rb b/app/models/packages/protection/rule.rb index bb65be92b90f09..842f485b492de2 100644 --- a/app/models/packages/protection/rule.rb +++ b/app/models/packages/protection/rule.rb @@ -16,6 +16,18 @@ class Rule < ApplicationRecord Gitlab::Access::MAINTAINER, Gitlab::Access::OWNER ] } + + scope :for_package_type, ->(package_type) { where(package_type: package_type) } + + scope :push_protection_applicable_for_access_level, ->(user_access_level) do + where(push_protected_up_to_access_level: user_access_level..) + end + + def self.matching_package_name(package_name, scope) + scope.select do |package_protection_rule| + RefMatcher.new(package_protection_rule.package_name_pattern).matches?(package_name) + end + end end end end diff --git a/spec/finders/packages/package_protection_rule_push_finder_spec.rb b/spec/finders/packages/package_protection_rule_push_finder_spec.rb new file mode 100644 index 00000000000000..56d458374ae2da --- /dev/null +++ b/spec/finders/packages/package_protection_rule_push_finder_spec.rb @@ -0,0 +1,133 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Packages::PackageProtectionRulePushFinder, feature_category: :package_registry do + include_context 'ProjectPolicy context' + + let_it_be(:project) { private_project } + let_it_be(:package_protection_rule) do + create(:package_protection_rule, + project: project, + package_type: :npm, + push_protected_up_to_access_level: Gitlab::Access::OWNER + ) + end + + let(:current_user) { project.owner } + let(:package) do + build(:package, + project: project, + name: package_protection_rule.package_name_pattern, + package_type: package_protection_rule.package_type + ) + end + + let(:access_level_developer) { Gitlab::Access::DEVELOPER } + let(:access_level_maintainer) { Gitlab::Access::MAINTAINER } + let(:access_level_owner) { Gitlab::Access::OWNER } + + describe "#initialize" do + subject { described_class.new(package, current_user) } + + it { is_expected.to be_present } + it { is_expected.to be_a described_class } + + context 'without package' do + let(:package) { nil } + + it { expect { subject }.to raise_error ArgumentError } + end + + context 'without current_user' do + let(:current_user) { nil } + + it { expect { subject }.to raise_error ArgumentError } + end + end + + describe "#execute" do + let(:finder) { described_class.new(package, current_user) } + + subject { finder.execute } + + describe "with different users and protection levels" do + using RSpec::Parameterized::TableSyntax + + where(:current_user, :push_protected_up_to_access_level, :expected_package_protection_rules) do + ref(:non_member) | ref(:access_level_developer) | [] + ref(:guest) | ref(:access_level_developer) | [ref(:package_protection_rule)] + ref(:reporter) | ref(:access_level_developer) | [ref(:package_protection_rule)] + ref(:developer) | ref(:access_level_developer) | [ref(:package_protection_rule)] + ref(:maintainer) | ref(:access_level_developer) | [] + ref(:owner) | ref(:access_level_developer) | [] + + ref(:developer) | ref(:access_level_maintainer) | [ref(:package_protection_rule)] + ref(:maintainer) | ref(:access_level_maintainer) | [ref(:package_protection_rule)] + ref(:owner) | ref(:access_level_maintainer) | [] + + ref(:maintainer) | ref(:access_level_owner) | [ref(:package_protection_rule)] + ref(:owner) | ref(:access_level_owner) | [ref(:package_protection_rule)] + end + + with_them do + before do + package_protection_rule.reload.update!(push_protected_up_to_access_level: push_protected_up_to_access_level) + end + + it { is_expected.to match_array expected_package_protection_rules } + end + end + + describe "with admin mode enabled", :enable_admin_mode do + using RSpec::Parameterized::TableSyntax + + where(:current_user, :push_protected_up_to_access_level, :expected_package_protection_rules) do + ref(:admin) | ref(:access_level_developer) | [] + ref(:admin) | ref(:access_level_maintainer) | [] + ref(:admin) | ref(:access_level_owner) | [] + end + + with_them do + before do + package_protection_rule.reload.update!(push_protected_up_to_access_level: push_protected_up_to_access_level) + end + + it { is_expected.to match_array expected_package_protection_rules } + end + end + + context 'with wildcard-based matching package protection rule' do + let_it_be(:package_protection_rule_wildcard) do + create(:package_protection_rule, + project: project, + package_name_pattern: "#{package_protection_rule.package_name_pattern}*", + push_protected_up_to_access_level: Gitlab::Access::OWNER + ) + end + + it { is_expected.to be_an Array } + it { is_expected.to contain_exactly(package_protection_rule, package_protection_rule_wildcard) } + end + + context 'with non-matching package protection rule' do + before do + package.assign_attributes( + name: "#{package_protection_rule.package_name_pattern}_no_match" + ) + end + + it { is_expected.to be_an Array } + it { is_expected.to be_empty } + end + + context 'for package of non-matching package_type' do + before do + package.assign_attributes(package_type: :maven) + end + + it { is_expected.to be_an Array } + it { is_expected.to be_empty } + end + end +end diff --git a/spec/models/packages/package_spec.rb b/spec/models/packages/package_spec.rb index 74ad2925058975..0213a3af706425 100644 --- a/spec/models/packages/package_spec.rb +++ b/spec/models/packages/package_spec.rb @@ -1531,4 +1531,36 @@ end end end + + describe '#push_protected_from?' do + let_it_be(:project) { create(:project) } + let_it_be(:package) { create(:npm_package, project: project) } + let_it_be(:user) { project.owner } + + subject { package.push_protected_from?(user) } + + context 'without PackageProtectionRule' do + it { is_expected.to be_falsy } + end + + context 'with PackageProtectionRule' do + let_it_be(:package_protection_rule) do + create( + :package_protection_rule, + project: project, + package_name_pattern: package.name, + package_type: package.package_type, + push_protected_up_to_access_level: Gitlab::Access::OWNER + ) + end + + it { is_expected.to be_truthy } + end + + context 'without user' do + let(:user) { nil } + + it { is_expected.to be_truthy } + end + end end diff --git a/spec/models/packages/protection/rule_spec.rb b/spec/models/packages/protection/rule_spec.rb index b368687e6d8051..6214fb0b5c0fcb 100644 --- a/spec/models/packages/protection/rule_spec.rb +++ b/spec/models/packages/protection/rule_spec.rb @@ -37,4 +37,198 @@ } end end + + describe '.matching_package_name' do + let_it_be(:package_protection_rule) do + create(:package_protection_rule, package_name_pattern: '@my-scope/my_package') + end + + let_it_be(:ppr_with_wildcard_start) do + create(:package_protection_rule, package_name_pattern: '*@my-scope/my_package-with-wildcard-start') + end + + let_it_be(:ppr_with_wildcard_end) do + create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with-wildcard-end*') + end + + let_it_be(:ppr_with_wildcard_inbetween) do + create(:package_protection_rule, package_name_pattern: '@my-scope/*my_package-with-wildcard-inbetween') + end + + let_it_be(:ppr_with_wildcard_multiples) do + create(:package_protection_rule, package_name_pattern: '**@my-scope/**my_package-with-wildcard-multiple**') + end + + let_it_be(:ppr_with_wildcard_escaped) do + create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with-wildcard-escaped-\*') + end + + let_it_be(:ppr_with_underscore) do + create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with_____underscore') + end + + let_it_be(:ppr_with_percent_sign) do + create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with-percent-sign-%') + end + + let_it_be(:ppr_with_percent_sign_escaped) do + create(:package_protection_rule, package_name_pattern: '@my-scope/my_package-with-percent-sign-escaped-\%') + end + + let_it_be(:ppr_with_unsupported_regex_characters) do + create(:package_protection_rule, + package_name_pattern: '@my-scope/my_package-with-unsupported-regex-characters.+') + end + + let(:package_name) { package_protection_rule.package_name_pattern } + + subject { described_class.matching_package_name(package_name, described_class.all) } + + context 'with several package protection rule scenarios' do + using RSpec::Parameterized::TableSyntax + + where(:package_name, :expected_package_protection_rules) do + '@my-scope/my_package' | [ref(:package_protection_rule)] + '@my-scope/my2package' | [] + '@my-scope/my_package-2' | [] + + # With wildcard pattern at the start + '@my-scope/my_package-with-wildcard-start' | [ref(:ppr_with_wildcard_start)] + '@my-scope/my_package-with-wildcard-start-any' | [] + 'prefix-@my-scope/my_package-with-wildcard-start' | [ref(:ppr_with_wildcard_start)] + 'prefix-@my-scope/my_package-with-wildcard-start-any' | [] + + # With wildcard pattern at the end + '@my-scope/my_package-with-wildcard-end' | [ref(:ppr_with_wildcard_end)] + '@my-scope/my_package-with-wildcard-end:1234567890' | [ref(:ppr_with_wildcard_end)] + 'prefix-@my-scope/my_package-with-wildcard-end' | [] + 'prefix-@my-scope/my_package-with-wildcard-end:1234567890' | [] + + # With wildcard pattern inbetween + '@my-scope/my_package-with-wildcard-inbetween' | [ref(:ppr_with_wildcard_inbetween)] + '@my-scope/any-my_package-with-wildcard-inbetween' | [ref(:ppr_with_wildcard_inbetween)] + '@my-scope/any-my_package-my_package-wildcard-inbetween-any' | [] + + # With multiple wildcard pattern are used + '@my-scope/my_package-with-wildcard-multiple' | [ref(:ppr_with_wildcard_multiples)] + 'prefix-@my-scope/any-my_package-with-wildcard-multiple-any' | [ref(:ppr_with_wildcard_multiples)] + '****@my-scope/****my_package-with-wildcard-multiple****' | [ref(:ppr_with_wildcard_multiples)] + 'prefix-@other-scope/any-my_package-with-wildcard-multiple-any' | [] + + # With wildcard escaped + '@my-scope/my_package-with-wildcard-escaped-\*' | [ref(:ppr_with_wildcard_escaped)] + '@my-scope/my_package-with-wildcard-escaped-\any-character' | [ref(:ppr_with_wildcard_escaped)] + '@my-scope/my_package-with-wildcard-escaped-\%' | [ref(:ppr_with_wildcard_escaped)] + '@my-scope/my_package-with-wildcard-escaped-any-character' | [] + + # With underscore + '@my-scope/my_package-with_____underscore' | [ref(:ppr_with_underscore)] + '@my-scope/my_package-with_any_underscore' | [] + + # With percent sign + '@my-scope/my_package-with-percent-sign-%' | [ref(:ppr_with_percent_sign)] + '@my-scope/my_package-with-percent-sign-any-character' | [] + + # With percent sign escaped + '@my-scope/my_package-with-percent-sign-escaped-\%' | [ref(:ppr_with_percent_sign_escaped)] + '@my-scope/my_package-with-percent-sign-escaped-\*' | [] + '@my-scope/my_package-with-percent-sign-escaped-\any-character' | [] + '@my-scope/my_package-with-percent-sign-escaped-any-character' | [] + + '@my-scope/my_package-with-unsupported-regex-characters.+' | [ref(:ppr_with_unsupported_regex_characters)] + '@my-scope/my_package-with-unsupported-regex-characters.' | [] + '@my-scope/my_package-with-unsupported-regex-characters' | [] + '@my-scope/my_package-with-unsupported-regex-characters-any' | [] + + # Special cases + nil | [] + '' | [] + 'any_package' | [] + end + + with_them do + it { is_expected.to match_array(expected_package_protection_rules) } + end + end + + context 'with multiple matching package protection rules' do + let!(:package_protection_rule_second_match) do + create(:package_protection_rule, package_name_pattern: "#{package_name}*") + end + + it { is_expected.to contain_exactly(package_protection_rule_second_match, package_protection_rule) } + end + + context 'with scope `none`' do + subject { described_class.matching_package_name(package_name, described_class.none) } + + it { is_expected.to be_empty } + end + end + + describe '.push_protection_applicable_for_access_level' do + let_it_be(:ppr_for_developer) do + create(:package_protection_rule, package_name_pattern: 'package-name-pattern-for-developer', + push_protected_up_to_access_level: Gitlab::Access::DEVELOPER) + end + + let_it_be(:ppr_for_maintainer) do + create(:package_protection_rule, package_name_pattern: 'package-name-pattern-for-maintainer', + push_protected_up_to_access_level: Gitlab::Access::MAINTAINER) + end + + let_it_be(:ppr_for_owner) do + create(:package_protection_rule, package_name_pattern: 'package-name-pattern-for-owner', + push_protected_up_to_access_level: Gitlab::Access::OWNER) + end + + subject { described_class.push_protection_applicable_for_access_level(push_protected_up_to_access_level) } + + context 'with different scenarios' do + let_it_be(:access_level_guest) { Gitlab::Access::GUEST } + let_it_be(:access_level_reporter) { Gitlab::Access::REPORTER } + let_it_be(:access_level_developer) { Gitlab::Access::DEVELOPER } + let_it_be(:access_level_maintainer) { Gitlab::Access::MAINTAINER } + let_it_be(:access_level_owner) { Gitlab::Access::OWNER } + let_it_be(:access_level_admin) { Gitlab::Access::ADMIN } + let_it_be(:access_level_no_access) { Gitlab::Access::NO_ACCESS } + + using RSpec::Parameterized::TableSyntax + + # rubocop:disable Layout/LineLength + where(:push_protected_up_to_access_level, :expected_package_protection_rules) do + ref(:access_level_guest) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] + ref(:access_level_reporter) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] + ref(:access_level_developer) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] + ref(:access_level_maintainer) | [ref(:ppr_for_maintainer), ref(:ppr_for_owner)] + ref(:access_level_owner) | [ref(:ppr_for_owner)] + ref(:access_level_admin) | [] + ref(:access_level_no_access) | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] + + # Special cases + nil | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] + 0 | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] + -1 | [ref(:ppr_for_developer), ref(:ppr_for_maintainer), ref(:ppr_for_owner)] + end + # rubocop:enable Layout/LineLength + + with_them do + it { is_expected.to eq expected_package_protection_rules } + end + end + end + + describe '.for_package_type' do + let_it_be(:ppr_for_npm) do + create(:package_protection_rule, package_type: :npm) + end + + it { expect(described_class.for_package_type(:npm)).to eq [ppr_for_npm] } + it { expect(described_class.for_package_type('npm')).to eq [ppr_for_npm] } + it { expect(described_class.for_package_type(Packages::Package.package_types.fetch(:npm))).to eq [ppr_for_npm] } + + it { expect(described_class.for_package_type('')).to eq [] } + it { expect(described_class.for_package_type(nil)).to eq [] } + it { expect(described_class.for_package_type(Packages::Package.package_types.fetch(:pypi))).to eq [] } + end end diff --git a/spec/support/finder_collection_allowlist.yml b/spec/support/finder_collection_allowlist.yml index e7dd9cea9229f2..bfcf6bddfb7af4 100644 --- a/spec/support/finder_collection_allowlist.yml +++ b/spec/support/finder_collection_allowlist.yml @@ -51,6 +51,7 @@ - Packages::Go::VersionFinder - Packages::PackageFileFinder - Packages::PackageFinder +- Packages::PackageProtectionRulePushFinder - Packages::Pypi::PackageFinder - Projects::Integrations::Jira::ByIdsFinder - Projects::Integrations::Jira::IssuesFinder -- GitLab