Merge branch 'sy-restrict-issue-creation-to-appropriate-users' into 'master'

Limit creation of issues based on issue type

See merge request gitlab-org/gitlab!61281

app/policies/concerns/readonly_abilities.rb

@@ -13,6 +13,7 @@ module ReadonlyAbilities
     create_merge_request_from
     create_merge_request_in
     award_emoji
+    create_incident
   ].freeze
 
   READONLY_FEATURES = %i[

app/policies/project_policy.rb

@@ -226,6 +226,8 @@ class ProjectPolicy < BasePolicy
     enable :read_insights
   end
 
+  rule { can?(:guest_access) & can?(:create_issue) }.enable :create_incident
+
   # These abilities are not allowed to admins that are not members of the project,
   # that's why they are defined separately.
   rule { guest & can?(:download_code) }.enable :build_download_code

app/services/issues/base_service.rb

@@ -37,6 +37,8 @@ module Issues
     def filter_params(issue)
       super
 
+      params.delete(:issue_type) unless issue_type_allowed?(issue)
+
       moved_issue = params.delete(:moved_issue)
 
       # Setting created_at, updated_at and iid is allowed only for admins and owners or
@@ -75,6 +77,11 @@ module Issues
 
       Milestones::IssuesCountService.new(milestone).delete_cache
     end
+
+    # @param object [Issue, Project]
+    def issue_type_allowed?(object)
+      can?(current_user, :"create_#{params[:issue_type]}", object)
+    end
   end
 end
 

app/services/issues/build_service.rb

@@ -64,20 +64,17 @@ module Issues
 
     private
 
-    def allowed_issue_base_params
-      [:title, :description, :confidential, :issue_type]
-    end
+    def allowed_issue_params
+      allowed_params = [
+        :title,
+        :description,
+        :confidential
+      ]
 
-    def allowed_issue_admin_params
-      [:milestone_id]
-    end
+      allowed_params << :milestone_id if can?(current_user, :admin_issue, project)
+      allowed_params << :issue_type if issue_type_allowed?(project)
 
-    def allowed_issue_params
-      if can?(current_user, :admin_issue, project)
-        params.slice(*(allowed_issue_base_params + allowed_issue_admin_params))
-      else
-        params.slice(*allowed_issue_base_params)
-      end
+      params.slice(*allowed_params)
     end
 
     def build_issue_params

app/services/issues/update_service.rb

@@ -28,7 +28,6 @@ module Issues
       # because we do allow users that cannot admin issues to set confidential flag when creating an issue
       unless can_admin_issuable?(issue)
         params.delete(:confidential)
-        params.delete(:issue_type)
       end
     end
 

changelogs/unreleased/sy-restrict-issue-creation-to-appropriate-users.yml

+---
+title: Restrict issue creation via API by relevant permissions
+merge_request: 61281
+author:
+type: fixed

ee/app/controllers/projects/quality/test_cases_controller.rb

@@ -5,7 +5,7 @@ class Projects::Quality::TestCasesController < Projects::ApplicationController
 
   before_action :check_quality_management_available!
   before_action :authorize_read_issue!
-  before_action :authorize_admin_issue!, only: [:new]
+  before_action :authorize_create_test_case!, only: [:new]
 
   feature_category :quality_management
 

ee/app/graphql/mutations/quality_management/test_cases/create.rb

@@ -8,7 +8,7 @@ module Mutations
 
         graphql_name 'CreateTestCase'
 
-        authorize :admin_issue
+        authorize :create_test_case
 
         argument :title, GraphQL::STRING_TYPE,
                  required: true,

ee/app/policies/ee/project_policy.rb

@@ -18,6 +18,9 @@ module EE
       with_scope :subject
       condition(:requirements_available) { @subject.feature_available?(:requirements) & access_allowed_to?(:requirements) }
 
+      with_scope :subject
+      condition(:quality_management_available) { @subject.feature_available?(:quality_management) }
+
       condition(:compliance_framework_available) { @subject.feature_available?(:compliance_framework, @user) }
 
       with_scope :global
@@ -360,6 +363,8 @@ module EE
 
       rule { requirements_available & owner }.enable :destroy_requirement
 
+      rule { quality_management_available & can?(:reporter_access) & can?(:create_issue) }.enable :create_test_case
+
       rule { compliance_framework_available & can?(:maintainer_access) }.enable :admin_compliance_framework
 
       rule { status_page_available & can?(:owner_access) }.enable :mark_issue_for_publication

ee/app/policies/ee/readonly_abilities.rb

@@ -7,6 +7,7 @@ module EE
     READONLY_ABILITIES_EE = %i[
       admin_software_license_policy
       modify_auto_fix_setting
+      create_test_case
     ].freeze
 
     READONLY_FEATURES_EE = %i[

ee/app/services/ee/issues/build_service.rb

@@ -5,8 +5,6 @@ module EE
     module BuildService
       extend ::Gitlab::Utils::Override
 
-      DISABLED_ISSUE_TYPES = %w[test_case requirement].freeze
-
       def issue_params_from_template
         return {} unless project.feature_available?(:issuable_default_templates)
 
@@ -21,20 +19,6 @@ module EE
       def build_issue_params
         issue_params_from_template.merge(super)
       end
-
-      override :allowed_issue_base_params
-      def allowed_issue_base_params
-        return super - [:issue_type] if DISABLED_ISSUE_TYPES.include?(params[:issue_type])
-
-        super
-      end
-
-      override :allowed_issue_admin_params
-      def allowed_issue_admin_params
-        return super + [:issue_type] if params[:issue_type] == 'test_case'
-
-        super
-      end
     end
   end
 end

ee/app/services/quality_management/test_cases/create_service.rb

@@ -14,8 +14,6 @@ module QualityManagement
       end
 
       def execute
-        return error(_('Test cases are not available for this project')) unless can_create_test_cases?
-
         issue = Issues::CreateService.new(
           project: project,
           current_user: current_user,
@@ -43,10 +41,6 @@ module QualityManagement
       def error(message, issue = nil)
         ServiceResponse.error(payload: { issue: issue }, message: message)
       end
-
-      def can_create_test_cases?
-        project.feature_available?(:quality_management)
-      end
     end
   end
 end

ee/app/views/projects/quality/test_cases/index.html.haml

@@ -2,7 +2,7 @@
 - page_title _('Test Cases')
 - @content_class = 'project-test-cases'
 
-#js-test-cases-list{ data: { can_create_test_case: can?(current_user, :create_issue, @project).to_s,
+#js-test-cases-list{ data: { can_create_test_case: can?(current_user, :create_test_case, @project).to_s,
   initial_state: params[:state],
   page: params[:page],
   prev: params[:prev],

ee/spec/policies/project_policy_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe ProjectPolicy do
   subject { described_class.new(current_user, project) }
 
   before do
-    stub_licensed_features(license_scanning: true)
+    stub_licensed_features(license_scanning: true, quality_management: true)
   end
 
   context 'basic permissions' do
@@ -190,7 +190,7 @@ RSpec.describe ProjectPolicy do
       end
 
       it 'disables boards permissions' do
-        expect_disallowed :admin_issue_board
+        expect_disallowed :admin_issue_board, :create_test_case
       end
     end
   end
@@ -1354,6 +1354,41 @@ RSpec.describe ProjectPolicy do
     let(:resource) { project }
   end
 
+  describe 'Quality Management test case' do
+    using RSpec::Parameterized::TableSyntax
+
+    let(:policy) { :create_test_case }
+
+    where(:role, :admin_mode, :allowed) do
+      :guest      | nil   | false
+      :reporter   | nil   | true
+      :developer  | nil   | true
+      :maintainer | nil   | true
+      :owner      | nil   | true
+      :admin      | false | false
+      :admin      | true  | true
+    end
+
+    before do
+      stub_licensed_features(quality_management: true)
+      enable_admin_mode!(current_user) if admin_mode
+    end
+
+    with_them do
+      let(:current_user) { public_send(role) }
+
+      it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
+
+      context 'with unavailable license' do
+        before do
+          stub_licensed_features(quality_management: false)
+        end
+
+        it { is_expected.to(be_disallowed(policy)) }
+      end
+    end
+  end
+
   describe ':compliance_framework_available' do
     using RSpec::Parameterized::TableSyntax
 
@@ -1563,7 +1598,7 @@ RSpec.describe ProjectPolicy do
     before do
       allow(project.root_namespace).to receive(:over_storage_limit?).and_return(over_storage_limit)
       allow(project).to receive(:design_management_enabled?).and_return(true)
-      stub_licensed_features(security_dashboard: true, license_scanning: true)
+      stub_licensed_features(security_dashboard: true, license_scanning: true, quality_management: true)
     end
 
     context 'when the group has exceeded its storage limit' do

ee/spec/requests/api/graphql/mutations/quality_management/test_cases/create_spec.rb

@@ -45,14 +45,6 @@ RSpec.describe 'Create test case' do
                                'The resource that you are attempting to access does not exist '\
                                'or you don\'t have permission to perform this action'
                               ]
-
-      context 'with authorized logged user', :aggregate_failures do
-        before_all do
-          project.add_reporter(current_user)
-        end
-
-        it_behaves_like 'a mutation that returns errors in the response', errors: ["Test cases are not available for this project"]
-      end
     end
 
     context 'when quality management feature is available' do

ee/spec/services/ee/issues/update_service_spec.rb

@@ -7,11 +7,12 @@ RSpec.describe Issues::UpdateService do
 
   let(:project) { create(:project, group: group) }
   let(:issue) { create(:issue, project: project) }
-  let(:user) { issue.author }
+  let(:author) { issue.author }
+  let(:user) { author }
 
   describe 'execute' do
     before do
-      project.add_reporter(user)
+      project.add_reporter(author)
     end
 
     def update_issue(opts)
@@ -173,12 +174,13 @@ RSpec.describe Issues::UpdateService do
       let!(:sla_setting) { create(:project_incident_management_setting, :sla_enabled, project: project) }
 
       before do
-        stub_licensed_features(incident_sla: true)
+        stub_licensed_features(incident_sla: true, quality_management: true)
       end
 
       context 'from issue to incident' do
         it 'creates an SLA' do
           expect { update_issue(issue_type: 'incident') }.to change(IssuableSla, :count).by(1)
+          expect(issue.reload).to be_incident
           expect(issue.reload.issuable_sla).to be_present
         end
       end
@@ -189,17 +191,36 @@ RSpec.describe Issues::UpdateService do
 
         it 'does not remove the SLA or create a new one' do
           expect { update_issue(issue_type: 'issue') }.not_to change(IssuableSla, :count)
+          expect(issue.reload.issue_type).to eq('issue')
           expect(issue.reload.issuable_sla).to be_present
         end
       end
 
-      # Not an expected scenario, but covers an SLA-agnostic hypothetical
-      context 'from test_case to issue' do
-        let(:issue) { create(:quality_test_case, project: project) }
+      context 'from issue to restricted issue types' do
+        context 'with permissions' do
+          it 'changes the type' do
+            expect { update_issue(issue_type: 'test_case') }
+              .to change { issue.reload.issue_type }
+              .from('issue')
+              .to('test_case')
+          end
 
-        it 'does nothing' do
-          expect { update_issue(issue_type: 'issue') }.not_to change(IssuableSla, :count)
-          expect(issue.reload.issuable_sla).to be_nil
+          it 'does not create or remove an SLA' do
+            expect { update_issue(issue_type: 'test_case') }.not_to change(IssuableSla, :count)
+            expect(issue.issuable_sla).to be_nil
+          end
+        end
+
+        context 'without sufficient permissions' do
+          let(:user) { create(:user) }
+
+          before do
+            project.add_guest(user)
+          end
+
+          it 'excludes the issue type param' do
+            expect { update_issue(issue_type: 'test_case') }.not_to change { issue.reload.issue_type }
+          end
         end
       end
     end

ee/spec/services/issues/build_service_spec.rb

@@ -51,11 +51,17 @@ RSpec.describe Issues::BuildService do
   end
 
   describe '#execute' do
+    before do
+      stub_licensed_features(quality_management: true, requirements: true)
+    end
+
     context 'as developer' do
-      it 'sets test_case' do
-        issue = build_issue(issue_type: 'test_case')
+      Issue.issue_types.each_key do |issue_type|
+        it "sets the issue type to #{issue_type}" do
+          issue = build_issue(issue_type: issue_type)
 
-        expect(issue).to be_test_case
+          expect(issue.issue_type).to eq(issue_type.to_s)
+        end
       end
     end
 
@@ -63,11 +69,11 @@ RSpec.describe Issues::BuildService do
       let(:user) { guest }
 
       context 'setting issue type' do
-        Issues::BuildService::DISABLED_ISSUE_TYPES.each do |type|
-          it "cannot set #{type}" do
-            issue = build_issue(issue_type: type)
+        [:test_case, :requirement].each do |issue_type|
+          it "cannot set the issue type to #{issue_type}" do
+            issue = build_issue(issue_type: issue_type)
 
-            expect(issue).to be_issue
+            expect(issue.issue_type).to eq('issue')
           end
         end
       end

ee/spec/services/quality_management/test_cases/create_service_spec.rb

@@ -14,39 +14,30 @@ RSpec.describe QualityManagement::TestCases::CreateService do
       project.add_reporter(user)
     end
 
+    before do
+      stub_licensed_features(quality_management: true)
+    end
+
     context 'when test has title and description' do
       let(:title) { 'test case title' }
       let(:new_issue) { Issue.last! }
 
-      context 'when quality management license is not available' do
-        it 'responds with errors' do
-          expect(service.execute).to be_error
-          expect(service.execute.message).to eq("Test cases are not available for this project")
-        end
+      it 'responds with success' do
+        expect(service.execute).to be_success
       end
 
-      context 'when quality_management license is available' do
-        before do
-          stub_licensed_features(quality_management: true)
-        end
-
-        it 'responds with success' do
-          expect(service.execute).to be_success
-        end
-
-        it 'creates an test case issue' do
-          expect { service.execute }.to change(Issue, :count).by(1)
-        end
+      it 'creates an test case issue' do
+        expect { service.execute }.to change(Issue, :count).by(1)
+      end
 
-        it 'created issue has correct attributes' do
-          service.execute
-          aggregate_failures do
-            expect(new_issue.title).to eq(title)
-            expect(new_issue.description).to eq(description)
-            expect(new_issue.author).to eq(user)
-            expect(new_issue.issue_type).to eq('test_case')
-            expect(new_issue.labels.map(&:title)).to eq([label.title])
-          end
+      it 'created issue has correct attributes' do
+        service.execute
+        aggregate_failures do
+          expect(new_issue.title).to eq(title)
+          expect(new_issue.description).to eq(description)
+          expect(new_issue.author).to eq(user)
+          expect(new_issue.issue_type).to eq('test_case')
+          expect(new_issue.labels.map(&:title)).to eq([label.title])
         end
       end
     end
@@ -54,10 +45,6 @@ RSpec.describe QualityManagement::TestCases::CreateService do
     context 'when test case has no title' do
       let(:title) { '' }
 
-      before do
-        stub_licensed_features(quality_management: true)
-      end
-
       it 'does not create an issue' do
         expect { service.execute }.not_to change(Issue, :count)
       end

locale/gitlab.pot

@@ -31847,9 +31847,6 @@ msgstr ""
 msgid "Test Cases"
 msgstr ""
 
-msgid "Test cases are not available for this project"
-msgstr ""
-
 msgid "Test coverage parsing"
 msgstr ""
 

spec/policies/project_policy_spec.rb

@@ -60,7 +60,7 @@ RSpec.describe ProjectPolicy do
       end
 
       it 'does not include the issues permissions' do
-        expect_disallowed :read_issue, :read_issue_iid, :create_issue, :update_issue, :admin_issue
+        expect_disallowed :read_issue, :read_issue_iid, :create_issue, :update_issue, :admin_issue, :create_incident
       end
 
       it 'disables boards and lists permissions' do
@@ -72,7 +72,7 @@ RSpec.describe ProjectPolicy do
         it 'does not include the issues permissions' do
           create(:jira_service, project: project)
 
-          expect_disallowed :read_issue, :read_issue_iid, :create_issue, :update_issue, :admin_issue
+          expect_disallowed :read_issue, :read_issue_iid, :create_issue, :update_issue, :admin_issue, :create_incident
         end
       end
     end

spec/services/issues/build_service_spec.rb

@@ -184,9 +184,9 @@ RSpec.describe Issues::BuildService do
         end
 
         it 'cannot set invalid type' do
-          expect do
-            build_issue(issue_type: 'invalid type')
-          end.to raise_error(ArgumentError, "'invalid type' is not a valid issue_type")
+          issue = build_issue(issue_type: 'invalid type')
+
+          expect(issue).to be_issue
         end
       end
     end

spec/support/shared_contexts/policies/project_policy_shared_context.rb

@@ -15,7 +15,7 @@ RSpec.shared_context 'ProjectPolicy context' do
 
   let(:base_guest_permissions) do
     %i[
-      award_emoji create_issue create_merge_request_in create_note
+      award_emoji create_issue create_incident create_merge_request_in create_note
       create_project read_issue_board read_issue read_issue_iid read_issue_link
       read_label read_issue_board_list read_milestone read_note read_project
       read_project_for_iids read_project_member read_release read_snippet

spec/support/shared_examples/policies/project_policy_shared_examples.rb

@@ -57,7 +57,7 @@ RSpec.shared_examples 'project policies as anonymous' do
     context 'when a project has pending invites' do
       let(:group) { create(:group, :public) }
       let(:project) { create(:project, :public, namespace: group) }
-      let(:user_permissions) { [:create_merge_request_in, :create_project, :create_issue, :create_note, :upload_file, :award_emoji] }
+      let(:user_permissions) { [:create_merge_request_in, :create_project, :create_issue, :create_note, :upload_file, :award_emoji, :create_incident] }
       let(:anonymous_permissions) { guest_permissions - user_permissions }
       let(:current_user) { anonymous }