Adjust issuable policy for metric images

- Prevent non members from having destructive permisions

ee/app/policies/ee/issuable_policy.rb

@@ -5,6 +5,10 @@ module EE
     extend ActiveSupport::Concern
 
     prepended do
+      condition(:is_author) do
+        @user && @subject.author_id == @user.id
+      end
+
       rule { can?(:read_issue) }.policy do
         enable :read_issuable_metric_image
       end
@@ -12,6 +16,15 @@ module EE
       rule { can?(:create_issue) & can?(:update_issue) }.policy do
         enable :upload_issuable_metric_image
       end
+
+      rule { is_author | can?(:create_issue) & can?(:update_issue) }.policy do
+        enable :destroy_issuable_metric_image
+      end
+
+      rule { ~is_project_member }.policy do
+        prevent :upload_issuable_metric_image
+        prevent :destroy_issuable_metric_image
+      end
     end
   end
 end

ee/changelogs/security-360-prevent-any-users-from-deleting-metrics-issue-images.yml

+---
+title: Fix permissions for modifying issue metric images
+merge_request:
+author:
+type: security

ee/lib/ee/api/issues.rb

@@ -79,6 +79,9 @@ module EE
             end
             delete ':metric_image_id' do
               issue = find_project_issue(params[:issue_iid])
+
+              authorize!(:destroy_issuable_metric_image, issue)
+
               metric_image = issue.metric_images.find_by_id(params[:metric_image_id])
 
               render_api_error!('Metric image not found', 404) unless metric_image
@@ -93,12 +96,6 @@ module EE
         end
 
         helpers do
-          include ::API::Helpers::Packages::BasicAuthHelpers
-
-          def project
-            authorized_user_project
-          end
-
           def max_file_size_exceeded?
             params[:file].size > ::IssuableMetricImage::MAX_FILE_SIZE
           end

ee/spec/policies/issuable_policy_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe IssuablePolicy, models: true do
+  let(:non_member) { create(:user) }
+  let(:guest) { create(:user) }
+  let(:reporter) { create(:user) }
+
+  let(:guest_issue) { create(:issue, project: project, author: guest) }
+  let(:reporter_issue) { create(:issue, project: project, author: reporter) }
+
+  before do
+    project.add_guest(guest)
+    project.add_reporter(reporter)
+  end
+
+  def permissions(user, issue)
+    described_class.new(user, issue)
+  end
+
+  describe '#rules' do
+    context 'in a public project' do
+      let_it_be(:project) { create(:project, :public) }
+      let_it_be(:issue) { create(:issue, project: project) }
+
+      it 'disallows non-members from creating and deleting metric images' do
+        expect(permissions(non_member, issue)).to be_allowed(:read_issuable_metric_image)
+        expect(permissions(non_member, issue)).to be_disallowed(:upload_issuable_metric_image, :destroy_issuable_metric_image)
+      end
+
+      it 'allows guests to read, create metric images, and delete them in their own issues' do
+        expect(permissions(guest, issue)).to be_allowed(:read_issuable_metric_image)
+        expect(permissions(guest, issue)).to be_disallowed(:upload_issuable_metric_image, :destroy_issuable_metric_image)
+
+        expect(permissions(guest, guest_issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :destroy_issuable_metric_image)
+      end
+
+      it 'allows reporters to create and delete metric images' do
+        expect(permissions(reporter, issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :destroy_issuable_metric_image)
+        expect(permissions(reporter, reporter_issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :destroy_issuable_metric_image)
+      end
+    end
+
+    context 'in a private project' do
+      let_it_be(:project) { create(:project, :private) }
+      let_it_be(:issue) { create(:issue, project: project) }
+
+      it 'disallows non-members from creating and deleting metric images' do
+        expect(permissions(non_member, issue)).to be_disallowed(:read_issuable_metric_image, :upload_issuable_metric_image, :destroy_issuable_metric_image)
+      end
+
+      it 'allows guests to read metric images, and create + delete in their own issues' do
+        expect(permissions(guest, issue)).to be_allowed(:read_issuable_metric_image)
+        expect(permissions(guest, issue)).to be_disallowed(:upload_issuable_metric_image, :destroy_issuable_metric_image)
+
+        expect(permissions(guest, guest_issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :destroy_issuable_metric_image)
+      end
+
+      it 'allows reporters to create and delete metric images' do
+        expect(permissions(reporter, issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :destroy_issuable_metric_image)
+        expect(permissions(reporter, reporter_issue)).to be_allowed(:read_issuable_metric_image, :upload_issuable_metric_image, :destroy_issuable_metric_image)
+      end
+    end
+  end
+end

ee/spec/requests/api/issues_spec.rb

@@ -705,7 +705,7 @@ RSpec.describe API::Issues, :mailer do
     using RSpec::Parameterized::TableSyntax
 
     let_it_be(:project) do
-      create(:project, :private, creator_id: user.id, namespace: user.namespace)
+      create(:project, :public, creator_id: user.id, namespace: user.namespace)
     end
 
     let!(:image) { create(:issuable_metric_image, issue: issue) }
@@ -722,6 +722,15 @@ RSpec.describe API::Issues, :mailer do
     end
 
     shared_examples 'unauthorized_delete' do
+      it 'cannot delete the metric image' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+        expect(image.reload).to eq(image)
+      end
+    end
+
+    shared_examples 'not_found' do
       it 'cannot delete the metric image' do
         subject
 
@@ -734,9 +743,9 @@ RSpec.describe API::Issues, :mailer do
       :not_member | false | false | :unauthorized_delete
       :not_member | true  | false | :unauthorized_delete
       :not_member | true  | true  | :unauthorized_delete
-      :guest      | false | true  | :unauthorized_delete
+      :guest      | false | true  | :not_found
+      :guest      | false | false | :unauthorized_delete
       :guest      | true  | false | :can_delete_metric_image
-      :guest      | false | false | :can_delete_metric_image
       :reporter   | true  | false | :can_delete_metric_image
       :reporter   | false | false | :can_delete_metric_image
     end