Merge branch '221259-move-package-permissions-to-core' into 'master'

Move package permission declarations to core [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!36386

app/models/project.rb

@@ -65,6 +65,7 @@ class Project < ApplicationRecord
 
   cache_markdown_field :description, pipeline: :description
 
+  default_value_for :packages_enabled, true
   default_value_for :archived, false
   default_value_for :resolve_outdated_diff_discussions, false
   default_value_for :container_registry_enabled, gitlab_config_features.container_registry
@@ -446,6 +447,7 @@ class Project < ApplicationRecord
   # Sometimes queries (e.g. using CTEs) require explicit disambiguation with table name
   scope :projects_order_id_desc, -> { reorder(self.arel_table['id'].desc) }
 
+  scope :with_packages, -> { joins(:packages) }
   scope :in_namespace, ->(namespace_ids) { where(namespace_id: namespace_ids) }
   scope :personal, ->(user) { where(namespace_id: user.namespace_id) }
   scope :joined, ->(user) { where('namespace_id != ?', user.namespace_id) }
@@ -866,6 +868,15 @@ class Project < ApplicationRecord
     end
   end
 
+  # Because we use default_value_for we need to be sure
+  # packages_enabled= method does exist even if we rollback migration.
+  # Otherwise many tests from spec/migrations will fail.
+  def packages_enabled=(value)
+    if has_attribute?(:packages_enabled)
+      write_attribute(:packages_enabled, value)
+    end
+  end
+
   def cleanup
     @repository = nil
   end

app/policies/project_policy.rb

@@ -154,6 +154,9 @@ class ProjectPolicy < BasePolicy
     ::Feature.enabled?(:build_service_proxy, @subject)
   end
 
+  with_scope :subject
+  condition(:packages_disabled) { !@subject.packages_enabled }
+
   features = %w[
     merge_requests
     issues
@@ -296,12 +299,17 @@ class ProjectPolicy < BasePolicy
     enable :read_metrics_user_starred_dashboard
   end
 
+  rule { packages_disabled | repository_disabled }.policy do
+    prevent(*create_read_update_admin_destroy(:package))
+  end
+
   rule { owner | admin | guest | group_member }.prevent :request_access
   rule { ~request_access_enabled }.prevent :request_access
 
   rule { can?(:developer_access) & can?(:create_issue) }.enable :import_issues
 
   rule { can?(:developer_access) }.policy do
+    enable :create_package
     enable :admin_board
     enable :admin_merge_request
     enable :admin_milestone
@@ -342,6 +350,7 @@ class ProjectPolicy < BasePolicy
   end
 
   rule { can?(:maintainer_access) }.policy do
+    enable :destroy_package
     enable :admin_board
     enable :push_to_delete_protected_branch
     enable :update_snippet

ee/app/models/ee/project.rb

@@ -130,7 +130,6 @@ module EE
       scope :with_active_prometheus_service, -> { joins(:prometheus_service).merge(PrometheusService.active) }
       scope :with_enabled_error_tracking, -> { joins(:error_tracking_setting).where(project_error_tracking_settings: { enabled: true }) }
       scope :with_tracing_enabled, -> { joins(:tracing_setting) }
-      scope :with_packages, -> { joins(:packages) }
       scope :mirrored_with_enabled_pipelines, -> do
         joins(:project_feature).mirror.where(mirror_trigger_builds: true,
                                              project_features: { builds_access_level: ::ProjectFeature::ENABLED })
@@ -179,8 +178,6 @@ module EE
         validates :mirror_user, presence: true
       end
 
-      default_value_for :packages_enabled, true
-
       accepts_nested_attributes_for :tracing_setting, update_only: true, allow_destroy: true
       accepts_nested_attributes_for :status_page_setting, update_only: true, allow_destroy: true
       accepts_nested_attributes_for :compliance_framework_setting, update_only: true, allow_destroy: true
@@ -590,15 +587,6 @@ module EE
       feature_available?(:protected_environments)
     end
 
-    # Because we use default_value_for we need to be sure
-    # packages_enabled= method does exist even if we rollback migration.
-    # Otherwise many tests from spec/migrations will fail.
-    def packages_enabled=(value)
-      if has_attribute?(:packages_enabled)
-        write_attribute(:packages_enabled, value)
-      end
-    end
-
     # Update the default branch querying the remote to determine its HEAD
     def update_root_ref(remote_name)
       root_ref = repository.find_remote_root_ref(remote_name)

ee/app/policies/ee/project_policy.rb

@@ -27,9 +27,6 @@ module EE
       with_scope :subject
       condition(:deploy_board_disabled) { !@subject.feature_available?(:deploy_board) }
 
-      with_scope :subject
-      condition(:packages_disabled) { !@subject.packages_enabled }
-
       with_scope :subject
       condition(:iterations_available) { @subject.feature_available?(:iterations) }
 
@@ -236,7 +233,6 @@ module EE
         enable :create_vulnerability_feedback
         enable :destroy_vulnerability_feedback
         enable :update_vulnerability_feedback
-        enable :create_package
         enable :read_feature_flag
         enable :create_feature_flag
         enable :update_feature_flag
@@ -287,10 +283,6 @@ module EE
 
       rule { deploy_board_disabled & ~is_development }.prevent :read_deploy_board
 
-      rule { packages_disabled | repository_disabled }.policy do
-        prevent(*create_read_update_admin_destroy(:package))
-      end
-
       rule { feature_flags_disabled | repository_disabled }.policy do
         prevent(*create_read_update_admin_destroy(:feature_flag))
         prevent(:admin_feature_flags_user_lists)
@@ -300,7 +292,6 @@ module EE
         enable :push_code_to_protected_branches
         enable :admin_path_locks
         enable :update_approvers
-        enable :destroy_package
         enable :admin_feature_flags_client
         enable :modify_approvers_rules
         enable :modify_approvers_list

ee/spec/models/project_spec.rb

@@ -39,8 +39,6 @@ RSpec.describe Project do
     it { is_expected.to have_many(:approvers).dependent(:destroy) }
     it { is_expected.to have_many(:approver_users).through(:approvers) }
     it { is_expected.to have_many(:approver_groups).dependent(:destroy) }
-    it { is_expected.to have_many(:packages).class_name('Packages::Package') }
-    it { is_expected.to have_many(:package_files).class_name('Packages::PackageFile') }
     it { is_expected.to have_many(:upstream_project_subscriptions) }
     it { is_expected.to have_many(:upstream_projects) }
     it { is_expected.to have_many(:downstream_project_subscriptions) }
@@ -1926,12 +1924,6 @@ RSpec.describe Project do
     end
   end
 
-  describe '#packages_enabled' do
-    subject { create(:project).packages_enabled }
-
-    it { is_expected.to be true }
-  end
-
   describe '#update_root_ref' do
     let(:project) { create(:project, :repository) }
 
@@ -2383,33 +2375,6 @@ RSpec.describe Project do
       .and_return(host: host)
   end
 
-  describe '#package_already_taken?' do
-    let(:namespace) { create(:namespace) }
-    let(:project) { create(:project, :public, namespace: namespace) }
-    let!(:package) { create(:npm_package, project: project, name: "@#{namespace.path}/foo") }
-
-    context 'no package exists with the same name' do
-      it 'returns false' do
-        result = project.package_already_taken?("@#{namespace.path}/bar")
-        expect(result).to be false
-      end
-
-      it 'returns false if it is the project that the package belongs to' do
-        result = project.package_already_taken?("@#{namespace.path}/foo")
-        expect(result).to be false
-      end
-    end
-
-    context 'a package already exists with the same name' do
-      let(:alt_project) { create(:project, :public, namespace: namespace) }
-
-      it 'returns true' do
-        result = alt_project.package_already_taken?("@#{namespace.path}/foo")
-        expect(result).to be true
-      end
-    end
-  end
-
   describe '#ancestor_marked_for_deletion' do
     context 'adjourned deletion feature is not available' do
       before do

ee/spec/policies/project_policy_spec.rb

@@ -611,64 +611,6 @@ RSpec.describe ProjectPolicy do
     end
   end
 
-  describe 'read_package' do
-    context 'with admin' do
-      let(:current_user) { admin }
-
-      it { is_expected.to be_allowed(:read_package) }
-
-      context 'when repository is disabled' do
-        before do
-          project.project_feature.update(repository_access_level: ProjectFeature::DISABLED)
-        end
-
-        it { is_expected.to be_disallowed(:read_package) }
-      end
-    end
-
-    context 'with owner' do
-      let(:current_user) { owner }
-
-      it { is_expected.to be_allowed(:read_package) }
-    end
-
-    context 'with maintainer' do
-      let(:current_user) { maintainer }
-
-      it { is_expected.to be_allowed(:read_package) }
-    end
-
-    context 'with developer' do
-      let(:current_user) { developer }
-
-      it { is_expected.to be_allowed(:read_package) }
-    end
-
-    context 'with reporter' do
-      let(:current_user) { reporter }
-
-      it { is_expected.to be_allowed(:read_package) }
-    end
-
-    context 'with guest' do
-      let(:current_user) { guest }
-
-      it { is_expected.to be_allowed(:read_package) }
-    end
-
-    context 'with non member' do
-      let(:current_user) { create(:user) }
-
-      it { is_expected.to be_allowed(:read_package) }
-    end
-
-    context 'with anonymous' do
-      let(:current_user) { nil }
-
-      it { is_expected.to be_allowed(:read_package) }
-    end
-  end
-
   describe 'remove_project when default_project_deletion_protection is set to true' do
     before do
       allow(Gitlab::CurrentSettings.current_application_settings)

spec/models/project_spec.rb

@@ -119,6 +119,8 @@ RSpec.describe Project do
     it { is_expected.to have_many(:metrics_users_starred_dashboards).inverse_of(:project) }
     it { is_expected.to have_many(:repository_storage_moves) }
     it { is_expected.to have_many(:reviews).inverse_of(:project) }
+    it { is_expected.to have_many(:packages).class_name('Packages::Package') }
+    it { is_expected.to have_many(:package_files).class_name('Packages::PackageFile') }
 
     it_behaves_like 'model with repository' do
       let_it_be(:container) { create(:project, :repository, path: 'somewhere') }
@@ -6167,6 +6169,39 @@ RSpec.describe Project do
     end
   end
 
+  describe '#packages_enabled' do
+    subject { create(:project).packages_enabled }
+
+    it { is_expected.to be true }
+  end
+
+  describe '#package_already_taken?' do
+    let(:namespace) { create(:namespace) }
+    let(:project) { create(:project, :public, namespace: namespace) }
+    let!(:package) { create(:npm_package, project: project, name: "@#{namespace.path}/foo") }
+
+    context 'no package exists with the same name' do
+      it 'returns false' do
+        result = project.package_already_taken?("@#{namespace.path}/bar")
+        expect(result).to be false
+      end
+
+      it 'returns false if it is the project that the package belongs to' do
+        result = project.package_already_taken?("@#{namespace.path}/foo")
+        expect(result).to be false
+      end
+    end
+
+    context 'a package already exists with the same name' do
+      let(:alt_project) { create(:project, :public, namespace: namespace) }
+
+      it 'returns true' do
+        result = alt_project.package_already_taken?("@#{namespace.path}/foo")
+        expect(result).to be true
+      end
+    end
+  end
+
   describe '#design_management_enabled?' do
     let(:project) { build(:project) }
 

spec/policies/project_policy_spec.rb

@@ -941,4 +941,64 @@ RSpec.describe ProjectPolicy do
       it { is_expected.to be_disallowed(:read_build_report_results) }
     end
   end
+
+  describe 'read_package' do
+    subject { described_class.new(current_user, project) }
+
+    context 'with admin' do
+      let(:current_user) { admin }
+
+      it { is_expected.to be_allowed(:read_package) }
+
+      context 'when repository is disabled' do
+        before do
+          project.project_feature.update(repository_access_level: ProjectFeature::DISABLED)
+        end
+
+        it { is_expected.to be_disallowed(:read_package) }
+      end
+    end
+
+    context 'with owner' do
+      let(:current_user) { owner }
+
+      it { is_expected.to be_allowed(:read_package) }
+    end
+
+    context 'with maintainer' do
+      let(:current_user) { maintainer }
+
+      it { is_expected.to be_allowed(:read_package) }
+    end
+
+    context 'with developer' do
+      let(:current_user) { developer }
+
+      it { is_expected.to be_allowed(:read_package) }
+    end
+
+    context 'with reporter' do
+      let(:current_user) { reporter }
+
+      it { is_expected.to be_allowed(:read_package) }
+    end
+
+    context 'with guest' do
+      let(:current_user) { guest }
+
+      it { is_expected.to be_allowed(:read_package) }
+    end
+
+    context 'with non member' do
+      let(:current_user) { create(:user) }
+
+      it { is_expected.to be_allowed(:read_package) }
+    end
+
+    context 'with anonymous' do
+      let(:current_user) { nil }
+
+      it { is_expected.to be_allowed(:read_package) }
+    end
+  end
 end