Merge pull request #9552 from dmarcoux/package-policy-create

Authorize package creation with a Pundit policy

src/api/app/controllers/source_controller.rb

@@ -837,7 +837,7 @@ def package_command_undelete
       raise PackageExists, "the package exists already #{@target_project_name} #{@target_package_name}"
     end
     tprj = Project.get_by_name(@target_project_name)
-    unless tprj.is_a?(Project) && User.session!.can_create_package_in?(tprj)
+    unless tprj.is_a?(Project) && Pundit.policy(User.session!, Package.new(project: tprj)).create?
       raise CmdExecutionNoPermission, "no permission to create package in project #{@target_project_name}"
     end
 
@@ -1103,7 +1103,7 @@ def verify_can_modify_target!
 
     if Package.exists_by_project_and_name(@target_project_name, @target_package_name, follow_project_links: false)
       verify_can_modify_target_package!
-    elsif !@project.is_a?(Project) || !User.session!.can_create_package_in?(@project)
+    elsif !@project.is_a?(Project) || !Pundit.policy(User.session!, Package.new(project: @project)).create?
       raise CmdExecutionNoPermission, "no permission to create package in project #{@target_project_name}"
     end
   end

src/api/app/controllers/webui/package_controller.rb

@@ -339,15 +339,12 @@ def rdiff
   end
 
   def new
-    # FIXME: Use the package policy for this
-    authorize @project, :update?
+    authorize Package.new(project: @project), :create?
   end
 
   def create
     @package = @project.packages.build(package_params)
-
-    # FIXME: Use the package policy for this
-    authorize @project, :update?
+    authorize @package, :create?
 
     @package.flags.build(flag: :sourceaccess, status: :disable) if params[:source_protection]
     @package.flags.build(flag: :publish, status: :disable) if params[:disable_publishing]
@@ -356,7 +353,7 @@ def create
       flash[:success] = "Package '#{@package}' was created successfully"
       redirect_to action: :show, project: params[:project], package: @package.name
     else
-      flash[:error] = "Failed to create package '#{@package}'"
+      flash[:error] = "Failed to create package: #{@package.errors.full_messages.join(', ')}"
       redirect_to controller: :project, action: :show, project: params[:project]
     end
   end
@@ -1014,12 +1011,7 @@ def check_package_name_for_new
       redirect_to action: :new, project: @project
       return false
     end
-    # FIXME: This should be a Pundit policy
-    unless User.possibly_nobody.can_create_package_in?(@project)
-      flash[:error] = "You can't create packages in #{@project.name}"
-      redirect_to action: :new, project: @project
-      return false
-    end
+
     true
   end
 

src/api/app/models/bs_request_permission_check.rb

@@ -385,10 +385,10 @@ def set_permissions_for_action(action, new_state = nil)
         @write_permission_in_this_action = true
       end
     else
-      if @target_project && User.session!.can_create_package_in?(@target_project, true)
+      if @target_project && PackagePolicy.new(User.session!, Package.new(project: @target_project), true).create?
         @write_permission_in_target = true
       end
-      if @target_project && User.session!.can_create_package_in?(@target_project, ignore_lock)
+      if @target_project && PackagePolicy.new(User.session!, Package.new(project: @target_project), ignore_lock).create?
         @write_permission_in_this_action = true
       end
     end

src/api/app/models/user.rb

@@ -448,6 +448,7 @@ def has_global_permission?(perm_string)
     end
   end
 
+  # FIXME: This should be a policy
   def can_modify?(object, ignore_lock = nil)
     case object
     when Project
@@ -461,6 +462,7 @@ def can_modify?(object, ignore_lock = nil)
     end
   end
 
+  # FIXME: This should be a policy
   # project is instance of Project
   def can_modify_project?(project, ignore_lock = nil)
     unless project.is_a?(Project)
@@ -475,6 +477,7 @@ def can_modify_project?(project, ignore_lock = nil)
     can_modify_project_internal(project, ignore_lock)
   end
 
+  # FIXME: This should be a policy
   # package is instance of Package
   def can_modify_package?(package, ignore_lock = nil)
     return false if package.nil? # happens with remote packages easily
@@ -488,23 +491,12 @@ def can_modify_package?(package, ignore_lock = nil)
     false
   end
 
+  # FIXME: This should be a policy
   def can_modify_user?(user)
     is_admin? || self == user
   end
 
-  # project is instance of Project
-  def can_create_package_in?(project, ignore_lock = nil)
-    unless project.is_a?(Project)
-      raise ArgumentError, "illegal parameter type to User#can_change?: #{project.class.name}"
-    end
-
-    return false if !ignore_lock && project.is_locked?
-    return true if is_admin?
-    return true if has_global_permission?('create_package')
-    return true if has_local_permission?('create_package', project)
-    false
-  end
-
+  # FIXME: This should be a policy
   # project_name is name of the project
   def can_create_project?(project_name)
     ## special handling for home projects
@@ -518,6 +510,7 @@ def can_create_project?(project_name)
     has_local_permission?('create_project', parent_project)
   end
 
+  # FIXME: This should be a policy
   def can_modify_attribute_definition?(object)
     can_create_attribute_definition?(object)
   end
@@ -528,6 +521,7 @@ def attribute_modifier_rule_matches?(rule)
     true
   end
 
+  # FIXME: This should be a policy
   def can_create_attribute_definition?(object)
     object = object.attrib_namespace if object.is_a?(AttribType)
     unless object.is_a?(AttribNamespace)
@@ -546,6 +540,7 @@ def attribute_modification_rule_matches?(rule, object)
     true
   end
 
+  # FIXME: This should be a policy
   def can_create_attribute_in?(object, atype)
     if !object.is_a?(Project) && !object.is_a?(Package)
       raise ArgumentError, "illegal parameter type to User#can_change?: #{object.class.name}"
@@ -560,14 +555,17 @@ def can_create_attribute_in?(object, atype)
     abies.any? { |rule| attribute_modification_rule_matches?(rule, object) }
   end
 
+  # FIXME: This should be a policy
   def can_download_binaries?(package)
     can?(:download_binaries, package)
   end
 
+  # FIXME: This should be a policy
   def can_source_access?(package)
     can?(:source_access, package)
   end
 
+  # FIXME: This should be a policy
   def can?(key, package)
     is_admin? ||
       has_global_permission?(key.to_s) ||
@@ -933,6 +931,7 @@ def password_validation
     errors.add(:password, 'can\'t be blank')
   end
 
+  # FIXME: This should be a policy
   def can_modify_project_internal(project, ignore_lock)
     # The ordering is important because of the lock status check
     return false if !ignore_lock && project.is_locked?

src/api/app/policies/package_policy.rb

@@ -1,4 +1,18 @@
 class PackagePolicy < ApplicationPolicy
+  # FIXME: Using more than 2 arguments is considered a code smell.
+  def initialize(user, record, ignore_lock = false)
+    super(user, record)
+    @ignore_lock = ignore_lock
+  end
+
+  def create?
+    return false if !@ignore_lock && record.project.is_locked?
+    return true if user.is_admin? ||
+                   user.has_global_permission?('create_package') ||
+                   user.has_local_permission?('create_package', record.project)
+    false
+  end
+
   def branch?
     # same as Package.check_source_access!
     if source_access? || project_source_access?

src/api/app/policies/project_policy.rb

@@ -59,11 +59,7 @@ def local?
     record.is_a?(Project)
   end
 
-  def can_create_package_in?
-    user.can_create_package_in?(record)
-  end
-
   def local_project_and_allowed_to_create_package_in?
-    local? && can_create_package_in?
+    local? && Pundit.policy(user, Package.new(project: record)).create?
   end
 end

src/api/spec/controllers/webui/package_controller_spec.rb

@@ -1378,13 +1378,12 @@ def do_request(params)
 
     context 'Package#save failed' do
       before do
-        allow_any_instance_of(Package).to receive(:save).and_return(false)
         login(my_user)
-        post :create, params: post_params
+        post :create, params: post_params.merge(package: { name: package_name, title: 'a' * 251 })
       end
 
       it { expect(response).to redirect_to(project_show_path(source_project)) }
-      it { expect(flash[:error]).to eq("Failed to create package '#{package_name}'") }
+      it { expect(flash[:error]).to eq('Failed to create package: Title is too long (maximum is 250 characters)') }
     end
 
     context 'package creation' do
@@ -1427,8 +1426,8 @@ def do_request(params)
         let(:package_name) { 'foo' }
         let(:my_user) { create(:confirmed_user, login: 'another_user') }
 
-        it { expect(response).to redirect_to(new_package_path(source_project)) }
-        it { expect(flash[:error]).to eq("You can't create packages in #{source_project.name}") }
+        it { expect(response).to redirect_to(root_path) }
+        it { expect(flash[:error]).to eq('Sorry, you are not authorized to create this Package.') }
       end
     end
   end

src/api/spec/features/webui/packages_spec.rb

@@ -397,7 +397,7 @@
         # Use direct path instead
         visit "/package/new/#{global_project}"
 
-        expect(page).to have_text('Sorry, you are not authorized to update this Project')
+        expect(page).to have_text('Sorry, you are not authorized to create this Package')
         expect(page.current_path).to eq(root_path)
       end
 

src/api/spec/policies/package_policy_spec.rb

@@ -10,6 +10,51 @@
 
   subject { PackagePolicy }
 
+  context :create_in_locked_project_without_ignore_lock do
+    permissions :create? do
+      before do
+        allow(package.project).to receive(:is_locked?).and_return(true)
+        allow(other_user).to receive(:has_global_permission?).with('create_package').and_return(true)
+        allow(user).to receive(:has_local_permission?).with('create_package', package.project).and_return(true)
+      end
+
+      it { is_expected.not_to permit(user, package) }
+      it { is_expected.not_to permit(other_user, package) }
+      it { is_expected.not_to permit(admin_user, package) }
+      it { is_expected.not_to permit(anonymous_user, package) }
+    end
+  end
+
+  context :create_in_locked_project_with_ignore_lock do
+    permissions :create? do
+      before do
+        allow(package.project).to receive(:is_locked?).and_return(true)
+        allow(other_user).to receive(:has_global_permission?).with('create_package').and_return(true)
+        allow(user).to receive(:has_local_permission?).with('create_package', package.project).and_return(true)
+      end
+
+      # We cannot use the `permit` matcher due to the extra argument in `new`
+      it { expect(PackagePolicy.new(admin_user, package, true).create?).to be true }
+      it { expect(PackagePolicy.new(other_user, package, true).create?).to be true }
+      it { expect(PackagePolicy.new(user, package, true).create?).to be true }
+      it { expect(PackagePolicy.new(anonymous_user, package, true).create?).to be false }
+    end
+  end
+
+  context :create_in_unlocked_project do
+    permissions :create? do
+      before do
+        allow(other_user).to receive(:has_global_permission?).with('create_package').and_return(true)
+        allow(user).to receive(:has_local_permission?).with('create_package', package.project).and_return(true)
+      end
+
+      it { is_expected.to permit(admin_user, package) }
+      it { is_expected.to permit(other_user, package) }
+      it { is_expected.to permit(user, package) }
+      it { is_expected.not_to permit(anonymous_user, package) }
+    end
+  end
+
   context :branch_as_anonymous do
     permissions :branch? do
       before do