Merge branch...

Merge branch '338717-the-processrequirementsreportsworker-retries-on-an-unrecoverable-error' into 'master'

Check requirements access before scheduling ProcessRequirementsReportsWorker

See merge request gitlab-org/gitlab!69810

ee/app/workers/ee/ci/build_finished_worker.rb

@@ -9,7 +9,7 @@ module EE
         # and enqueueing duplicate jobs.
         super
 
-        unless build.project.requirements.empty?
+        if requirements_available?(build) && !test_report_already_generated?(build)
           RequirementsManagement::ProcessRequirementsReportsWorker.perform_async(build.id)
         end
 
@@ -17,6 +17,18 @@ module EE
           ::Security::TrackSecureScansWorker.perform_async(build.id)
         end
       end
+
+      private
+
+      def test_report_already_generated?(build)
+        RequirementsManagement::TestReport.for_user_build(build.user_id, build.id).exists?
+      end
+
+      def requirements_available?(build)
+        build.project.feature_available?(:requirements, build.user) &&
+          !build.project.requirements.empty? &&
+          Ability.allowed?(build.user, :create_requirement_test_report, build.project)
+      end
     end
   end
 end

ee/app/workers/requirements_management/process_requirements_reports_worker.rb

@@ -15,6 +15,11 @@ module RequirementsManagement
       ::Ci::Build.find_by_id(build_id).try do |build|
         RequirementsManagement::ProcessTestReportsService.new(build).execute
       end
+    rescue Gitlab::Access::AccessDeniedError
+      Gitlab::AppLogger.error(
+        "RequirementsManagement::ProcessRequirementsReportsWorker: Insufficient permissions to " \
+        "create tests reports for build #{build_id}, skipping"
+      )
     end
   end
 end

ee/spec/workers/ee/ci/build_finished_worker_spec.rb

@@ -3,10 +3,10 @@
 require 'spec_helper'
 
 RSpec.describe Ci::BuildFinishedWorker do
-  let(:ci_runner) { create(:ci_runner) }
-  let(:build) { create(:ee_ci_build, :sast, :success, runner: ci_runner) }
-  let(:project) { build.project }
-  let(:namespace) { project.shared_runners_limit_namespace }
+  let_it_be(:ci_runner) { create(:ci_runner) }
+  let_it_be(:build) { create(:ee_ci_build, :sast, :success, runner: ci_runner) }
+  let_it_be(:project) { build.project }
+  let_it_be(:namespace) { project.shared_runners_limit_namespace }
 
   subject do
     described_class.new.perform(build.id)
@@ -76,31 +76,73 @@ RSpec.describe Ci::BuildFinishedWorker do
       end
     end
 
+    context 'when token revocation is disabled' do
+      before do
+        allow_next_instance_of(described_class) do |build_finished_worker|
+          allow(build_finished_worker).to receive(:revoke_secret_detection_token?) { false }
+        end
+      end
+
+      it 'does not scan security reports for token revocation' do
+        expect(ScanSecurityReportSecretsWorker).not_to receive(:perform_async)
+
+        subject
+      end
+    end
+
     it 'does not schedule processing of requirement reports by default' do
       expect(RequirementsManagement::ProcessRequirementsReportsWorker).not_to receive(:perform_async)
 
       subject
     end
 
-    it 'schedules processing of requirement reports if project has requirements' do
-      create(:requirement, project: project)
+    context 'with requirements' do
+      let_it_be(:requirement) { create(:requirement, project: project) }
+      let_it_be(:user) { create(:user) }
 
-      expect(RequirementsManagement::ProcessRequirementsReportsWorker).to receive(:perform_async)
+      before do
+        build.update!(user: user)
+        project.add_reporter(user)
+      end
 
-      subject
-    end
+      shared_examples 'schedules processing of requirement reports' do
+        it do
+          expect(RequirementsManagement::ProcessRequirementsReportsWorker).to receive(:perform_async)
 
-    context 'when token revocation is disabled' do
-      before do
-        allow_next_instance_of(described_class) do |build_finished_worker|
-          allow(build_finished_worker).to receive(:revoke_secret_detection_token?) { false }
+          subject
         end
       end
 
-      it 'does not scan security reports for token revocation' do
-        expect(ScanSecurityReportSecretsWorker).not_to receive(:perform_async)
+      shared_examples 'does not schedule processing of requirement reports' do
+        it do
+          expect(RequirementsManagement::ProcessRequirementsReportsWorker).not_to receive(:perform_async)
 
-        subject
+          subject
+        end
+      end
+
+      context 'when requirements feature is available' do
+        before do
+          stub_licensed_features(requirements: true)
+        end
+
+        it_behaves_like 'schedules processing of requirement reports'
+
+        context 'when user has insufficient permissions to create test reports' do
+          before do
+            project.add_guest(user)
+          end
+
+          it_behaves_like 'does not schedule processing of requirement reports'
+        end
+      end
+
+      context 'when requirements feature is not available' do
+        before do
+          stub_licensed_features(requirements: false)
+        end
+
+        it_behaves_like 'does not schedule processing of requirement reports'
       end
     end
   end

ee/spec/workers/requirements_management/process_requirements_reports_worker_spec.rb

@@ -4,22 +4,44 @@ require 'spec_helper'
 
 RSpec.describe RequirementsManagement::ProcessRequirementsReportsWorker do
   describe '#perform' do
+    subject { described_class.new.perform(build_id) }
+
     context 'build exists' do
-      let(:build) { create(:ci_build) }
+      let_it_be(:build) { create(:ci_build) }
+
+      let(:build_id) { build.id }
+
+      it_behaves_like 'an idempotent worker' do
+        let(:job_args) { [build.id] }
+      end
 
       it 'processes requirements reports' do
         service_double = instance_double(RequirementsManagement::ProcessTestReportsService, execute: true)
         expect(RequirementsManagement::ProcessTestReportsService).to receive(:new).and_return(service_double)
 
-        described_class.new.perform(build.id)
+        subject
+      end
+
+      context 'when the service raises a Gitlab::Access::AccessDeniedError' do
+        before do
+          allow_next_instance_of(RequirementsManagement::ProcessTestReportsService) do |instance|
+            allow(instance).to receive(:execute).and_raise(Gitlab::Access::AccessDeniedError)
+          end
+        end
+
+        it 'rescues the error' do
+          expect { subject }.not_to raise_error
+        end
       end
     end
 
     context 'build does not exist' do
+      let(:build_id) { non_existing_record_id }
+
       it 'does not store requirements reports' do
         expect(RequirementsManagement::ProcessTestReportsService).not_to receive(:new)
 
-        described_class.new.perform(non_existing_record_id)
+        subject
       end
     end
   end