Merge branch '217872-audit-dast-profile-actions' into 'master'

Audit DAST profile actions

See merge request gitlab-org/gitlab!62604

doc/administration/audit_events.md

@@ -120,6 +120,7 @@ From there, you can see the following actions:
 - Project access token was successfully created or revoked ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/230007) in GitLab 13.9)
 - Failed attempt to create or revoke a project access token ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/230007) in GitLab 13.9)
 - When default branch changes for a project ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/52339) in GitLab 13.9)
+- Created, updated, or deleted DAST profiles, DAST scanner profiles, and DAST site profiles ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/217872))
 
 Project events can also be accessed via the [Project Audit Events API](../api/audit_events.md#project-audit-events).
 

doc/user/application_security/dast/index.md

@@ -958,6 +958,7 @@ Alternatively, you can use the CI/CD variable `SECURE_ANALYZERS_PREFIX` to overr
 > - The saved scans feature was [introduced](https://gitlab.com/groups/gitlab-org/-/epics/5100) in GitLab 13.9.
 > - The option to select a branch was [introduced](https://gitlab.com/groups/gitlab-org/-/epics/4847) in GitLab 13.10.
 > - DAST branch selection [feature flag removed](https://gitlab.com/gitlab-org/gitlab/-/issues/322672) in GitLab 13.11.
+> - Auditing for DAST profile management was [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/217872) in GitLab 14.1.
 
 An on-demand DAST scan runs outside the DevOps life cycle. Changes in your repository don't trigger
 the scan. You must start it manually.
@@ -1281,6 +1282,13 @@ If a scanner profile is linked to a security policy, a user cannot delete the pr
 page. See [Scan Policies](../policies/index.md)
 for more information.
 
+### Auditing
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/217872) in GitLab 14.1.
+
+The creation, updating, and deletion of DAST profiles, DAST scanner profiles,
+and DAST site profiles are included in the [audit log](../../../administration/audit_events.md).
+
 ## Reports
 
 The DAST tool outputs a report file in JSON format by default. However, this tool can also generate reports in

ee/app/models/dast_scanner_profile.rb

@@ -17,6 +17,12 @@ class DastScannerProfile < ApplicationRecord
     active: 2
   }
 
+  def self.names(scanner_profile_ids)
+    find(*scanner_profile_ids).pluck(:name)
+  rescue ActiveRecord::RecordNotFound
+    []
+  end
+
   def ci_variables
     ::Gitlab::Ci::Variables::Collection.new.tap do |variables|
       variables.append(key: 'DAST_SPIDER_MINS', value: String(spider_timeout)) if spider_timeout

ee/app/models/dast_site_profile.rb

@@ -31,6 +31,12 @@ class DastSiteProfile < ApplicationRecord
 
   delegate :dast_site_validation, to: :dast_site, allow_nil: true
 
+  def self.names(site_profile_ids)
+    find(*site_profile_ids).pluck(:name)
+  rescue ActiveRecord::RecordNotFound
+    []
+  end
+
   def ci_variables
     url = dast_site.url
 

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

+# frozen_string_literal: true
+
+module AppSec
+  module Dast
+    module Profiles
+      module Audit
+        class UpdateService < BaseContainerService
+          def execute
+            params[:new_params].each do |property, new_value|
+              old_value = params[:old_params][property]
+
+              next if old_value == new_value
+
+              ::Gitlab::Audit::Auditor.audit(
+                name: 'dast_profile_update',
+                author: current_user,
+                scope: container,
+                target: params[:dast_profile],
+                message: audit_message(property, new_value, old_value)
+              )
+            end
+          end
+
+          private
+
+          def audit_message(property, new_value, old_value)
+            case property
+            when :dast_scanner_profile_id
+              old_value, new_value = DastScannerProfile.names([old_value, new_value])
+              property = :dast_scanner_profile
+            when :dast_site_profile_id
+              old_value, new_value = DastSiteProfile.names([old_value, new_value])
+              property = :dast_site_profile
+            end
+
+            "Changed DAST profile #{property} from #{old_value} to #{new_value}"
+          end
+        end
+      end
+    end
+  end
+end

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

@@ -16,6 +16,8 @@ module AppSec
             dast_scanner_profile: dast_scanner_profile
           )
 
+          create_audit_event(dast_profile)
+
           return ServiceResponse.success(payload: { dast_profile: dast_profile, pipeline_url: nil }) unless params.fetch(:run_after_create)
 
           response = ::DastOnDemandScans::CreateService.new(
@@ -46,6 +48,16 @@ module AppSec
         def dast_scanner_profile
           @dast_scanner_profile ||= params.fetch(:dast_scanner_profile)
         end
+
+        def create_audit_event(profile)
+          ::Gitlab::Audit::Auditor.audit(
+            name: 'dast_profile_create',
+            author: current_user,
+            scope: container,
+            target: profile,
+            message: "Added DAST profile"
+          )
+        end
       end
     end
   end

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

@@ -9,6 +9,8 @@ module AppSec
           return ServiceResponse.error(message: 'Profile parameter missing') unless dast_profile
           return ServiceResponse.error(message: 'Profile failed to delete') unless dast_profile.destroy
 
+          create_audit_event
+
           ServiceResponse.success(payload: dast_profile)
         end
 
@@ -28,6 +30,16 @@ module AppSec
         def dast_profile
           params[:dast_profile]
         end
+
+        def create_audit_event
+          ::Gitlab::Audit::Auditor.audit(
+            name: 'dast_profile_destroy',
+            author: current_user,
+            scope: container,
+            target: dast_profile,
+            message: "Removed DAST profile"
+          )
+        end
       end
     end
   end

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

@@ -9,8 +9,17 @@ module AppSec
         def execute
           return unauthorized unless allowed?
           return error('Profile parameter missing') unless dast_profile
+
+          auditor = AppSec::Dast::Profiles::Audit::UpdateService.new(container: container, current_user: current_user, params: {
+            dast_profile: dast_profile,
+            new_params: dast_profile_params,
+            old_params: dast_profile.attributes.symbolize_keys
+          })
+
           return error(dast_profile.errors.full_messages) unless dast_profile.update(dast_profile_params)
 
+          auditor.execute
+
           return success(dast_profile: dast_profile, pipeline_url: nil) unless params[:run_after_update]
 
           response = create_scan(dast_profile)

ee/spec/models/dast_scanner_profile_spec.rb

@@ -35,6 +35,25 @@ RSpec.describe DastScannerProfile, type: :model do
     end
   end
 
+  describe '.names' do
+    it 'returns the names for the DAST scanner profiles with the given IDs' do
+      first_profile = create(:dast_scanner_profile, name: 'First profile')
+      second_profile = create(:dast_scanner_profile, name: 'Second profile')
+
+      names = described_class.names([first_profile.id, second_profile.id])
+
+      expect(names).to contain_exactly('First profile', 'Second profile')
+    end
+
+    context 'when a profile is not found' do
+      it 'rescues the error and returns an empty array' do
+        names = described_class.names([0])
+
+        expect(names).to be_empty
+      end
+    end
+  end
+
   describe '#ci_variables' do
     let(:collection) { subject.ci_variables }
 

ee/spec/models/dast_site_profile_spec.rb

@@ -132,6 +132,25 @@ RSpec.describe DastSiteProfile, type: :model do
     it { is_expected.to define_enum_for(:target_type).with_values(**target_types) }
   end
 
+  describe '.names' do
+    it 'returns the names for the DAST site profiles with the given IDs' do
+      first_profile = create(:dast_site_profile, name: 'First profile')
+      second_profile = create(:dast_site_profile, name: 'Second profile')
+
+      names = described_class.names([first_profile.id, second_profile.id])
+
+      expect(names).to contain_exactly('First profile', 'Second profile')
+    end
+
+    context 'when a profile is not found' do
+      it 'rescues the error and returns an empty array' do
+        names = described_class.names([0])
+
+        expect(names).to be_empty
+      end
+    end
+  end
+
   describe 'instance methods' do
     describe '#destroy!' do
       context 'when the associated dast_site has no dast_site_profiles' do

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe AppSec::Dast::Profiles::Audit::UpdateService do
+  let_it_be(:dast_profile) { create(:dast_profile) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
+
+  describe '#execute' do
+    it 'creates audit events for the changed properties', :aggregate_failures do
+      auditor = described_class.new(container: project, current_user: user, params: {
+        dast_profile: dast_profile,
+        new_params: { name: 'New name' },
+        old_params: { name: 'Old name' }
+      })
+
+      auditor.execute
+
+      audit_event = AuditEvent.find_by(author_id: user.id)
+      expect(audit_event.author).to eq(user)
+      expect(audit_event.entity).to eq(project)
+      expect(audit_event.target_id).to eq(dast_profile.id)
+      expect(audit_event.target_type).to eq('Dast::Profile')
+      expect(audit_event.target_details).to eq(dast_profile.name)
+      expect(audit_event.details).to eq({
+        author_name: user.name,
+        custom_message: 'Changed DAST profile name from Old name to New name',
+        target_id: dast_profile.id,
+        target_type: 'Dast::Profile',
+        target_details: dast_profile.name
+      })
+    end
+
+    it 'uses names instead of IDs for the changed scanner and site profile messages' do
+      new_scanner_profile = create(:dast_scanner_profile)
+      old_scanner_profile = create(:dast_scanner_profile)
+      new_site_profile = create(:dast_site_profile)
+      old_site_profile = create(:dast_site_profile)
+
+      auditor = described_class.new(container: project, current_user: user, params: {
+        dast_profile: dast_profile,
+        new_params: { dast_scanner_profile_id: new_scanner_profile.id, dast_site_profile_id: new_site_profile.id },
+        old_params: { dast_scanner_profile_id: old_scanner_profile.id, dast_site_profile_id: old_site_profile.id }
+      })
+
+      auditor.execute
+
+      audit_events = AuditEvent.where(author_id: user.id)
+      messages = audit_events.map(&:details).pluck(:custom_message)
+      expect(messages).to contain_exactly(
+        "Changed DAST profile dast_scanner_profile from #{old_scanner_profile.name} to #{new_scanner_profile.name}",
+        "Changed DAST profile dast_site_profile from #{old_site_profile.name} to #{new_site_profile.name}"
+      )
+    end
+
+    it 'does not exceed the maximum permitted number of queries' do
+      new_scanner_profile = create(:dast_scanner_profile)
+      old_scanner_profile = create(:dast_scanner_profile)
+      new_site_profile = create(:dast_site_profile)
+      old_site_profile = create(:dast_site_profile)
+
+      new_params = {
+        branch_name: 'new-branch',
+        dast_scanner_profile_id: new_scanner_profile.id,
+        dast_site_profile_id: new_site_profile.id,
+        description: 'New description',
+        name: 'New name'
+      }
+      old_params = {
+        branch_name: 'old-branch',
+        dast_scanner_profile_id: old_scanner_profile.id,
+        dast_site_profile_id: old_site_profile.id,
+        description: 'Old description',
+        name: 'Old name'
+      }
+
+      auditor = described_class.new(container: project, current_user: user, params: {
+        dast_profile: dast_profile, new_params: new_params, old_params: old_params
+      })
+
+      recorder = ActiveRecord::QueryRecorder.new do
+        auditor.execute
+      end
+
+      expect(recorder.count).to be <= 18
+    end
+  end
+end

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

@@ -48,6 +48,27 @@ RSpec.describe AppSec::Dast::Profiles::CreateService do
         expect { subject }.to change { Dast::Profile.count }.by(1)
       end
 
+      it 'audits the creation' do
+        profile = subject.payload[:dast_profile]
+
+        audit_event = AuditEvent.find_by(author_id: developer.id)
+
+        aggregate_failures do
+          expect(audit_event.author).to eq(developer)
+          expect(audit_event.entity).to eq(project)
+          expect(audit_event.target_id).to eq(profile.id)
+          expect(audit_event.target_type).to eq('Dast::Profile')
+          expect(audit_event.target_details).to eq(profile.name)
+          expect(audit_event.details).to eq({
+            author_name: developer.name,
+            custom_message: 'Added DAST profile',
+            target_id: profile.id,
+            target_type: 'Dast::Profile',
+            target_details: profile.name
+          })
+        end
+      end
+
       context 'when param run_after_create: true' do
         let(:params) { default_params.merge(run_after_create: true) }
 

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

@@ -60,6 +60,27 @@ RSpec.describe AppSec::Dast::Profiles::DestroyService do
           expect(subject.payload).to be_a(Dast::Profile)
         end
 
+        it 'audits the deletion' do
+          profile = subject.payload
+
+          audit_event = AuditEvent.find_by(author_id: user.id)
+
+          aggregate_failures do
+            expect(audit_event.author).to eq(user)
+            expect(audit_event.entity).to eq(project)
+            expect(audit_event.target_id).to eq(profile.id)
+            expect(audit_event.target_type).to eq('Dast::Profile')
+            expect(audit_event.target_details).to eq(profile.name)
+            expect(audit_event.details).to eq({
+              author_name: user.name,
+              custom_message: 'Removed DAST profile',
+              target_id: profile.id,
+              target_type: 'Dast::Profile',
+              target_details: profile.name
+            })
+          end
+        end
+
         context 'when the dast_profile fails to destroy' do
           it 'communicates failure' do
             allow(dast_profile).to receive(:destroy).and_return(false)

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

@@ -76,6 +76,37 @@ RSpec.describe AppSec::Dast::Profiles::UpdateService do
           end
         end
 
+        it 'audits the update', :aggregate_failures do
+          old_profile_attrs = {
+            description: dast_profile.description,
+            name: dast_profile.name,
+            scanner_profile_name: dast_profile.dast_scanner_profile.name,
+            site_profile_name: dast_profile.dast_site_profile.name
+          }
+
+          subject
+
+          new_profile = dast_profile.reload
+          audit_events = AuditEvent.where(author_id: user.id)
+
+          audit_events.each do |event|
+            expect(event.author).to eq(user)
+            expect(event.entity).to eq(project)
+            expect(event.target_id).to eq(new_profile.id)
+            expect(event.target_type).to eq('Dast::Profile')
+            expect(event.target_details).to eq(new_profile.name)
+          end
+
+          messages = audit_events.map(&:details).pluck(:custom_message)
+          expected_messages = [
+            "Changed DAST profile dast_scanner_profile from #{old_profile_attrs[:scanner_profile_name]} to #{dast_scanner_profile.name}",
+            "Changed DAST profile dast_site_profile from #{old_profile_attrs[:site_profile_name]} to #{dast_site_profile.name}",
+            "Changed DAST profile name from #{old_profile_attrs[:name]} to #{new_profile.name}",
+            "Changed DAST profile description from #{old_profile_attrs[:description]} to #{new_profile.description}"
+          ]
+          expect(messages).to match_array(expected_messages)
+        end
+
         context 'when param run_after_update: true' do
           let(:params) { default_params.merge(run_after_update: true) }