Merge pull request #5871 from vpereira/verify_authorized_on_packages

Enable verify_authorized in Package controller actions

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

@@ -22,13 +22,15 @@ class Webui::PackageController < Webui::WebuiController
   before_action :require_package, only: [:show, :linking_packages, :dependency, :binary, :binaries,
                                          :requests, :statistics, :commit, :revisions, :submit_request_dialog,
                                          :add_person, :add_group, :rdiff,
-                                         :save, :delete_dialog,
+                                         :save, :save_meta, :delete_dialog,
                                          :remove, :add_file, :save_file, :remove_file, :save_person,
                                          :save_group, :remove_role, :view_file,
                                          :abort_build, :trigger_rebuild, :trigger_services,
                                          :wipe_binaries, :buildresult, :rpmlint_result, :rpmlint_log, :meta,
                                          :attributes, :edit, :files, :users, :binary_download]
 
+  before_action :validate_xml, only: [:save_meta]
+
   before_action :require_repository, only: [:binary, :binary_download]
   before_action :require_architecture, only: [:binary, :binary_download]
 
@@ -45,6 +47,8 @@ class Webui::PackageController < Webui::WebuiController
 
   prepend_before_action :lockout_spiders, only: [:revisions, :dependency, :rdiff, :binary, :binaries, :requests, :binary_download]
 
+  after_action :verify_authorized, only: [:remove_file, :remove, :save_file, :abort_build, :trigger_rebuild, :wipe_binaries, :save_meta, :save, :abort_build]
+
   def show
     if request.bot?
       params.delete(:rev)
@@ -96,6 +100,7 @@ def linking_packages
     render_dialog
   end
 
+  # rubocop:disable Lint/NonLocalExitFromIterator
   def dependency
     dependant_project = Project.find_by_name(params[:dependant_project]) || Project.find_remote_project(params[:dependant_project]).try(:first)
     unless dependant_project
@@ -116,9 +121,7 @@ def dependency
       next if project_repositories.include?(params[repo_key])
       flash[:error] = "Repository '#{params[repo_key]}' is invalid."
       redirect_back(fallback_location: project_show_path(project: @project.name))
-      # rubocop:disable Lint/NonLocalExitFromIterator
       return
-      # rubocop:enable Lint/NonLocalExitFromIterator
     end
 
     @arch = params[:arch]
@@ -133,6 +136,7 @@ def dependency
     redirect_back(fallback_location: { action: :binary, project: params[:project], package: params[:package],
                                        repository: @repository, arch: @arch, filename: @filename })
   end
+  # rubocop:enable Lint/NonLocalExitFromIterator
 
   def statistics
     @arch = params[:arch]
@@ -602,10 +606,7 @@ def branch
   end
 
   def save
-    unless User.current.can_modify?(@package)
-      redirect_to action: :show, project: params[:project], package: params[:package], error: 'No permission to save'
-      return
-    end
+    authorize @package, :update?
     @package.title = params[:title]
     @package.description = params[:description]
     if @package.save
@@ -978,36 +979,23 @@ def meta
   def save_meta
     errors = []
 
-    begin
-      Suse::Validator.validate('package', params[:meta])
-      meta_xml = Xmlhash.parse(params[:meta])
-
-      # That's a valid XML file
-      if Package.exists_by_project_and_name(@project.name, params[:package], follow_project_links: false)
-        @package = Package.get_by_project_and_name(@project.name, params[:package], use_source: false, follow_project_links: false)
-        authorize @package, :update?
+    authorize @package, :save_meta_update?
 
-        if @package && !@package.disabled_for?('sourceaccess', nil, nil) && FlagHelper.xml_disabled_for?(meta_xml, 'sourceaccess')
-          errors << 'admin rights are required to raise the protection level of a package'
-        end
+    if FlagHelper.xml_disabled_for?(@meta_xml, 'sourceaccess')
+      errors << 'admin rights are required to raise the protection level of a package'
+    end
 
-        if meta_xml['project'] && meta_xml['project'] != @project.name
-          errors << 'project name in xml data does not match resource path component'
-        end
+    if @meta_xml['project'] && @meta_xml['project'] != @project.name
+      errors << 'project name in xml data does not match resource path component'
+    end
 
-        if meta_xml['name'] && meta_xml['name'] != @package.name
-          errors << 'package name in xml data does not match resource path component'
-        end
-      else
-        errors << "Package doesn't exists in that project."
-      end
-    rescue Suse::ValidationError => e
-      errors << e.message
+    if @meta_xml['name'] && @meta_xml['name'] != @package.name
+      errors << 'package name in xml data does not match resource path component'
     end
 
     if errors.empty?
       begin
-        @package.update_from_xml(meta_xml)
+        @package.update_from_xml(@meta_xml)
         flash.now[:success] = 'The Meta file has been successfully saved.'
         render layout: false, partial: 'layouts/webui/flash', object: flash
       rescue Backend::Error, NotFoundError => e
@@ -1037,6 +1025,14 @@ def binary_download
 
   private
 
+  def validate_xml
+    Suse::Validator.validate('package', params[:meta])
+    @meta_xml = Xmlhash.parse(params[:meta])
+  rescue Suse::ValidationError => error
+    flash.now[:error] = "Error while saving the Meta file: #{error}."
+    render layout: false, status: 400, partial: 'layouts/webui/flash', object: flash
+  end
+
   def package_files(rev = nil, expand = nil)
     query = {}
     query[:expand]  = expand  if expand

src/api/app/policies/package_policy.rb

@@ -1,7 +1,7 @@
 class PackagePolicy < ApplicationPolicy
   def branch?
     # same as Package.check_source_access!
-    if @record.disabled_for?('sourceaccess', nil, nil) || record.project.disabled_for?('sourceaccess', nil, nil)
+    if source_access? || project_source_access?
       return false unless @user.can_source_access?(@record)
     end
     true
@@ -14,4 +14,16 @@ def update?
   def destroy?
     @user.can_modify?(@record)
   end
+
+  def save_meta_update?
+    update? && !source_access?
+  end
+
+  def project_source_access?
+    @record.project.disabled_for?('sourceaccess', nil, nil)
+  end
+
+  def source_access?
+    @record.disabled_for?('sourceaccess', nil, nil)
+  end
 end

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

@@ -901,8 +901,8 @@ def remove_file_post
         post :save_meta, params: { project: source_project, package: 'blah', meta: valid_meta }
       end
 
-      it { expect(flash[:error]).to eq("Error while saving the Meta file: Package doesn't exists in that project..") }
-      it { expect(response).to have_http_status(:bad_request) }
+      it { expect(flash[:error]).to eq("Package \"blah\" not found in project \"#{source_project.name}\"") }
+      it { expect(response).to redirect_to(project_show_path(project: source_project, nextstatus: 404)) }
     end
 
     context 'when connection with the backend fails' do

src/api/spec/policies/package_policy_spec.rb

@@ -0,0 +1,56 @@
+require 'rails_helper'
+
+RSpec.describe PackagePolicy do
+  let(:anonymous_user) { create(:user_nobody) }
+  let(:user) { create(:confirmed_user) }
+  let(:other_user) { create(:confirmed_user) }
+  let(:admin_user) { create(:admin_user) }
+  let(:project) { user.home_project }
+  let(:package) { create(:package, name: 'my_package', project: project) }
+
+  subject { PackagePolicy }
+
+  context :branch_as_anonymous do
+    permissions :branch? do
+      before do
+        skip('it should fail but its passing')
+      end
+
+      it { expect(subject).not_to permit(anonymous_user, package) }
+    end
+  end
+
+  context :branch_as_other_user do
+    permissions :branch? do
+      it { expect(subject).to permit(other_user, package) }
+    end
+  end
+
+  permissions :update?, :destroy? do
+    it { expect(subject).not_to permit(anonymous_user, package) }
+    it { expect(subject).not_to permit(other_user, package) }
+
+    it { expect(subject).to permit(admin_user, package) }
+    it { expect(subject).to permit(user, package) }
+  end
+
+  context :source_access_enabled do
+    permissions :source_access? do
+      before do
+        allow_any_instance_of(Package).to receive(:disabled_for?).with('sourceaccess', nil, nil).and_return(true)
+      end
+
+      it { expect(subject).to permit(user, package) }
+    end
+  end
+
+  context :source_access_disabled do
+    permissions :source_access? do
+      before do
+        allow_any_instance_of(Package).to receive(:disabled_for?).with('sourceaccess', nil, nil).and_return(false)
+      end
+
+      it { expect(subject).not_to permit(user, package) }
+    end
+  end
+end