Merge branch 'mc_rocha-audit-schedule-owner-change-335662' into 'master'

Audit dast schedule owner updates

See merge request gitlab-org/gitlab!71764

ee/app/services/app_sec/dast/profile_schedules/audit/update_service.rb

@@ -16,7 +16,7 @@ module AppSec
                 author: current_user,
                 scope: project,
                 target: params[:dast_profile_schedule],
-                message: "Changed DAST profile schedule #{property} from #{old_value} to #{new_value}"
+                message: "Changed DAST profile schedule #{property} from #{old_value || 'nil'} to #{new_value}"
               )
             end
           end

ee/app/services/app_sec/dast/profiles/update_service.rb

@@ -52,9 +52,7 @@ module AppSec
 
         def update_or_create_schedule!
           if schedule
-            attributes = schedule_input_params
-            attributes = attributes.merge(user_id: current_user.id) unless schedule.owner_valid?
-            schedule.update!(attributes)
+            schedule.update!(schedule_input_params)
           else
             ::Dast::ProfileSchedule.new(
               dast_profile: dast_profile,
@@ -93,9 +91,17 @@ module AppSec
         end
 
         def schedule_input_params
+          @schedule_input_params ||= build_schedule_input_params
+        end
+
+        def build_schedule_input_params
+          return unless params[:dast_profile_schedule]
+
           # params[:dast_profile_schedule] is `Types::Dast::ProfileScheduleInputType` object.
           # Using to_h method to convert object into equivalent hash.
-          @schedule_input_params ||= params[:dast_profile_schedule]&.to_h
+          dast_profile_schedule_params = params[:dast_profile_schedule]
+          dast_profile_schedule_params[:user_id] = current_user.id unless schedule&.owner_valid?
+          dast_profile_schedule_params&.to_h
         end
 
         def build_auditors!

ee/spec/services/app_sec/dast/profiles/update_service_spec.rb

@@ -63,18 +63,32 @@ RSpec.describe AppSec::Dast::Profiles::UpdateService do
           project.add_users([user, scheduler_owner], :developer)
         end
 
-        it 'communicates success' do
-          expect(subject.status).to eq(:success)
-        end
+        context 'without dast_profile_schedule param' do
+          it 'communicates success' do
+            expect(subject.status).to eq(:success)
+          end
 
-        it 'updates the dast_profile' do
-          updated_dast_profile = subject.payload[:dast_profile].reload
+          it 'updates the dast_profile' do
+            updated_dast_profile = subject.payload[:dast_profile].reload
 
-          aggregate_failures do
-            expect(updated_dast_profile.dast_site_profile.id).to eq(params[:dast_site_profile_id])
-            expect(updated_dast_profile.dast_scanner_profile.id).to eq(params[:dast_scanner_profile_id])
-            expect(updated_dast_profile.name).to eq(params[:name])
-            expect(updated_dast_profile.description).to eq(params[:description])
+            aggregate_failures do
+              expect(updated_dast_profile.dast_site_profile.id).to eq(params[:dast_site_profile_id])
+              expect(updated_dast_profile.dast_scanner_profile.id).to eq(params[:dast_scanner_profile_id])
+              expect(updated_dast_profile.name).to eq(params[:name])
+              expect(updated_dast_profile.description).to eq(params[:description])
+            end
+          end
+
+          it 'does not try to create or update the dast_profile_schedule' do
+            subject
+
+            expect { subject }.not_to change { dast_profile.reload.dast_profile_schedule }.from(nil)
+          end
+
+          it 'ignores the dast_profile_schedule' do
+            subject
+
+            expect(params[:dast_profile]).not_to receive(:dast_profile_schedule)
           end
         end
 
@@ -130,6 +144,16 @@ RSpec.describe AppSec::Dast::Profiles::UpdateService do
           context 'when associated schedule is present' do
             let_it_be_with_reload(:dast_profile_schedule) { create(:dast_profile_schedule, project: project, dast_profile: dast_profile, owner: scheduler_owner) }
 
+            shared_examples 'audits the owner change' do
+              it 'audits the owner change', :sidekiq_inline do
+                subject
+
+                messages = AuditEvent.where(target_id: dast_profile.dast_profile_schedule.id).pluck(:details).pluck(:custom_message)
+                old_owner = User.find_by(id: scheduler_owner.id)
+                expect(messages).to include("Changed DAST profile schedule user_id from #{old_owner&.id || 'nil'} to #{user.id}")
+              end
+            end
+
             it 'updates the dast profile schedule' do
               subject
 
@@ -155,15 +179,17 @@ RSpec.describe AppSec::Dast::Profiles::UpdateService do
 
             context 'when the owner was deleted' do
               before do
-                scheduler_owner.destroy!
-                subject.payload[:dast_profile_schedule].reload
+                dast_profile_schedule.owner.destroy!
+                dast_profile_schedule.reload
               end
 
               it 'updates the schedule owner' do
                 subject
 
-                expect(dast_profile_schedule.user_id).to eq(user.id)
+                expect(dast_profile_schedule.reload.user_id).to eq(user.id)
               end
+
+              include_examples 'audits the owner change'
             end
 
             context 'when the owner permission was downgraded' do
@@ -176,6 +202,8 @@ RSpec.describe AppSec::Dast::Profiles::UpdateService do
 
                 expect(dast_profile_schedule.user_id).to eq(user.id)
               end
+
+              include_examples 'audits the owner change'
             end
 
             context 'when the owner was removed from the project' do
@@ -189,6 +217,8 @@ RSpec.describe AppSec::Dast::Profiles::UpdateService do
 
                 expect(dast_profile_schedule.user_id).to eq(user.id)
               end
+
+              include_examples 'audits the owner change'
             end
           end
         end