Merge branch 'mc_rocha-capture-failed-dast-schedule-335662' into 'master'

Capture failed dast schedules

See merge request gitlab-org/gitlab!72024

ee/app/models/dast/profile_schedule.rb

@@ -55,12 +55,12 @@ class Dast::ProfileSchedule < ApplicationRecord
     Ability.allowed?(owner, :create_on_demand_dast_scan, project)
   end
 
-  private
-
   def deactivate!
     update!(active: false)
   end
 
+  private
+
   def cron_timezone
     Time.zone.name
   end

ee/app/workers/app_sec/dast/profile_schedule_worker.rb

@@ -18,13 +18,10 @@ module AppSec
 
         dast_runnable_schedules.find_in_batches do |schedules|
           schedules.each do |schedule|
-            with_context(project: schedule.project, user: schedule.owner) do
-              schedule.schedule_next_run!
-
-              response = service(schedule).execute
-              if response.error?
-                logger.info(structured_payload(message: response.message))
-              end
+            if schedule.owner_valid?
+              execute_schedule(schedule)
+            else
+              schedule.deactivate!
             end
           end
         end
@@ -46,6 +43,17 @@ module AppSec
           }
         )
       end
+
+      def execute_schedule(schedule)
+        with_context(project: schedule.project, user: schedule.owner) do
+          schedule.schedule_next_run!
+
+          response = service(schedule).execute
+          if response.error?
+            logger.info(structured_payload(message: response.message))
+          end
+        end
+      end
     end
   end
 end

ee/spec/workers/app_sec/dast/profile_schedule_worker_spec.rb

@@ -5,7 +5,9 @@ RSpec.describe AppSec::Dast::ProfileScheduleWorker do
   include ExclusiveLeaseHelpers
 
   let_it_be(:plan_limits) { create(:plan_limits, :default_plan) }
-  let_it_be(:schedule) { create(:dast_profile_schedule) }
+  let_it_be(:owner) { create(:user) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:schedule) { create(:dast_profile_schedule, owner: owner, project: project) }
 
   let(:worker) { described_class.new }
   let(:logger) { worker.send(:logger) }
@@ -13,6 +15,8 @@ RSpec.describe AppSec::Dast::ProfileScheduleWorker do
   let(:service_result) { ServiceResponse.success }
 
   before do
+    project.add_developer(owner)
+
     allow(::AppSec::Dast::Scans::CreateService)
       .to receive(:new)
       .and_return(service)
@@ -33,87 +37,128 @@ RSpec.describe AppSec::Dast::ProfileScheduleWorker do
         subject
       end
     end
-
-    context 'when multiple schedules exists' do
+    context 'when feature is licensed' do
       before do
-        schedule.update_column(:next_run_at, 1.minute.from_now)
+        stub_licensed_features(security_on_demand_scans: true)
       end
 
-      def record_preloaded_queries
-        recorder = ActiveRecord::QueryRecorder.new { subject }
-        recorder.data.values.flat_map {|v| v[:occurrences]}.select do |query|
-          ['FROM "projects"', 'FROM "users"', 'FROM "dast_profile"', 'FROM "dast_profile_schedule"'].any? do |s|
-            query.include?(s)
+      context 'when multiple schedules exists' do
+        before do
+          schedule.update_column(:next_run_at, 1.minute.from_now)
+        end
+
+        def record_preloaded_queries
+          recorder = ActiveRecord::QueryRecorder.new { subject }
+          recorder.data.values.flat_map { |v| v[:occurrences] }.select do |query|
+            ['FROM "projects"', 'FROM "users"', 'FROM "dast_profile"', 'FROM "dast_profile_schedule"'].any? do |s|
+              query.include?(s)
+            end
           end
         end
-      end
 
-      it 'preloads configuration, project and owner to avoid N+1 queries' do
-        expected_count = record_preloaded_queries.count
+        it 'preloads configuration, project and owner to avoid N+1 queries' do
+          expected_count = record_preloaded_queries.count
 
-        travel_to(30.minutes.ago) { create_list(:dast_profile_schedule, 5) }
-        actual_count = record_preloaded_queries.count
+          travel_to(30.minutes.ago) { create_list(:dast_profile_schedule, 5) }
+          actual_count = record_preloaded_queries.count
 
-        expect(actual_count).to eq(expected_count)
-      end
-    end
+          expect(actual_count).to eq(expected_count)
+        end
 
-    context 'when schedule exists' do
-      before do
-        schedule.update_column(:next_run_at, 1.minute.ago)
-      end
+        context 'when all of the schedule owners are invalid' do
+          before do
+            travel_to(30.minutes.ago) { create_list(:dast_profile_schedule, 5, owner: nil, active: true) }
+          end
 
-      it 'executes the rule schedule service' do
-        expect_next_found_instance_of(::Dast::ProfileSchedule) do |schedule|
-          expect(schedule).to receive(:schedule_next_run!)
+          it 'sets active to false' do
+            expect { subject }.to change { Dast::ProfileSchedule.where(active: false).count }.to(5)
+          end
         end
 
-        expect(service).to receive(:execute)
+        context 'when some of the schedule owners are invalid' do
+          before do
+            travel_to(30.minutes.ago) do
+              create_list(:dast_profile_schedule, 2, owner: nil, active: true)
+              create_list(:dast_profile_schedule, 3, owner: owner, active: true, project: project)
+            end
+          end
 
-        subject
+          it 'sets active to false' do
+            expect(service).to receive(:execute)
+            subject
+            expect(Dast::ProfileSchedule.where(active: false).count).to eq(2)
+          end
+        end
       end
-    end
 
-    context 'when service returns an error' do
-      before do
-        schedule.update_column(:next_run_at, 1.minute.ago)
-      end
+      context 'when schedule exists' do
+        before do
+          schedule.update_column(:next_run_at, 1.minute.ago)
+        end
 
-      let(:error_message) { 'some message' }
-      let(:service_result) { ServiceResponse.error(message: error_message) }
+        it 'executes the rule schedule service' do
+          expect_next_found_instance_of(::Dast::ProfileSchedule) do |schedule|
+            expect(schedule).to receive(:schedule_next_run!)
+          end
 
-      it 'succeeds and logs the error' do
-        expect(logger)
-          .to receive(:info)
-          .with(a_hash_including('message' => error_message))
+          expect(service).to receive(:execute)
 
-        subject
-      end
-    end
+          subject
+        end
 
-    context 'when schedule does not exist' do
-      before do
-        schedule.update_column(:next_run_at, 1.minute.from_now)
+        context 'when the schedule owner is invalid' do
+          before do
+            schedule.update_column(:user_id, nil)
+            schedule.update_column(:active, true)
+          end
+
+          it 'sets active to false' do
+            expect { subject }.to change { schedule.reload.active }.to(false)
+          end
+        end
       end
 
-      it 'executes the rule schedule service' do
-        expect(::AppSec::Dast::Scans::CreateService).not_to receive(:new)
+      context 'when service returns an error' do
+        before do
+          schedule.update_column(:next_run_at, 1.minute.ago)
+        end
 
-        subject
+        let(:error_message) { 'some message' }
+        let(:service_result) { ServiceResponse.error(message: error_message) }
+
+        it 'succeeds and logs the error' do
+          expect(logger)
+            .to receive(:info)
+                  .with(a_hash_including('message' => error_message))
+
+          subject
+        end
       end
-    end
 
-    context 'when single run schedule exists' do
-      before do
-        schedule.update_columns(next_run_at: 1.minute.ago, cadence: {})
+      context 'when schedule does not exist' do
+        before do
+          schedule.update_column(:next_run_at, 1.minute.from_now)
+        end
+
+        it 'executes the rule schedule service' do
+          expect(::AppSec::Dast::Scans::CreateService).not_to receive(:new)
+
+          subject
+        end
       end
 
-      it 'executes the rule schedule service and deactivate the schedule', :aggregate_failures do
-        expect(schedule.repeat?).to be(false)
+      context 'when single run schedule exists' do
+        before do
+          schedule.update_columns(next_run_at: 1.minute.ago, cadence: {})
+        end
 
-        subject
+        it 'executes the rule schedule service and deactivate the schedule', :aggregate_failures do
+          expect(schedule.repeat?).to be(false)
 
-        expect(schedule.reload.active).to be(false)
+          subject
+
+          expect(schedule.reload.active).to be(false)
+        end
       end
     end
   end