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

Audit DAST site profile actions

See merge request gitlab-org/gitlab!62465

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

+# frozen_string_literal: true
+
+module AppSec
+  module Dast
+    module SiteProfiles
+      module Audit
+        class UpdateService < BaseService
+          def execute
+            new_params.each do |property, new_value|
+              old_value = old_params[property]
+
+              if new_value.is_a?(Array)
+                next if old_value.sort == new_value.sort
+              else
+                next if old_value == new_value
+              end
+
+              ::Gitlab::Audit::Auditor.audit(
+                name: 'dast_site_profile_update',
+                author: current_user,
+                scope: project,
+                target: dast_site_profile,
+                message: audit_message(property, new_value, old_value)
+              )
+            end
+          end
+
+          private
+
+          def dast_site_profile
+            params[:dast_site_profile]
+          end
+
+          def new_params
+            params[:new_params]
+          end
+
+          def old_params
+            params[:old_params]
+          end
+
+          def audit_message(property, new_value, old_value)
+            case property
+            when :auth_password, :request_headers
+              "Changed DAST site profile #{property} (secret value omitted)"
+            when :excluded_urls
+              "Changed DAST site profile #{property} (long value omitted)"
+            else
+              "Changed DAST site profile #{property} from #{old_value} to #{new_value}"
+            end
+          end
+        end
+      end
+    end
+  end
+end

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

@@ -28,6 +28,8 @@ module AppSec
             create_secret_variable!(::Dast::SiteProfileSecretVariable::PASSWORD, params[:auth_password])
             create_secret_variable!(::Dast::SiteProfileSecretVariable::REQUEST_HEADERS, params[:request_headers])
 
+            create_audit_event
+
             ServiceResponse.success(payload: dast_site_profile)
           end
         rescue Rollback => e
@@ -68,6 +70,16 @@ module AppSec
             url_base: url_base
           ).execute.first
         end
+
+        def create_audit_event
+          ::Gitlab::Audit::Auditor.audit(
+            name: 'dast_site_profile_create',
+            author: current_user,
+            scope: project,
+            target: dast_site_profile,
+            message: 'Added DAST site profile'
+          )
+        end
       end
     end
   end

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

@@ -14,6 +14,8 @@ module AppSec
           return ServiceResponse.error(message: _('Cannot delete %{profile_name} referenced in security policy') % { profile_name: dast_site_profile.name }) if referenced_in_security_policy?(dast_site_profile)
 
           if dast_site_profile.destroy
+            create_audit_event(dast_site_profile)
+
             ServiceResponse.success(payload: dast_site_profile)
           else
             ServiceResponse.error(message: _('Site profile failed to delete'))
@@ -37,6 +39,16 @@ module AppSec
         def find_dast_site_profile(id)
           project.dast_site_profiles.id_in(id).first
         end
+
+        def create_audit_event(profile)
+          ::Gitlab::Audit::Auditor.audit(
+            name: 'dast_site_profile_destroy',
+            author: current_user,
+            scope: project,
+            target: profile,
+            message: 'Removed DAST site profile'
+          )
+        end
       end
     end
   end

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

@@ -22,6 +22,14 @@ module AppSec
           return ServiceResponse.error(message: _('Cannot modify %{profile_name} referenced in security policy') % { profile_name: dast_site_profile.name }) if referenced_in_security_policy?
 
           ActiveRecord::Base.transaction do
+            auditor = Audit::UpdateService.new(project, current_user, {
+              dast_site_profile: dast_site_profile,
+              new_params: params.dup,
+              old_params: dast_site_profile.attributes.symbolize_keys.merge(
+                target_url: dast_site_profile.dast_site.url
+              )
+            })
+
             if target_url = params.delete(:target_url)
               params[:dast_site] = DastSites::FindOrCreateService.new(project, current_user).execute!(url: target_url)
             end
@@ -30,8 +38,8 @@ module AppSec
             handle_secret_variable!(params, :auth_password, ::Dast::SiteProfileSecretVariable::PASSWORD)
 
             params.compact!
-
             dast_site_profile.update!(params)
+            auditor.execute
 
             ServiceResponse.success(payload: dast_site_profile)
           end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe AppSec::Dast::SiteProfiles::Audit::UpdateService do
+  let_it_be(:profile) { create(:dast_site_profile) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
+
+  describe '#execute' do
+    it 'audits the changes in the given properties', :aggregate_failures do
+      auditor = described_class.new(project, user, {
+        dast_site_profile: profile,
+        new_params: { name: 'Updated DAST profile' },
+        old_params: { name: 'Old DAST profile' }
+      })
+
+      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(profile.id)
+      expect(audit_event.target_type).to eq('DastSiteProfile')
+      expect(audit_event.details[:custom_message]).to eq(
+        'Changed DAST site profile name from Old DAST profile to Updated DAST profile'
+      )
+    end
+
+    it 'omits the values for secret properties' do
+      auditor = described_class.new(project, user, {
+        dast_site_profile: profile,
+        new_params: { auth_password: 'newpassword', request_headers: 'A new header' },
+        old_params: { auth_password: 'oldpassword', request_headers: 'An old header' }
+      })
+
+      auditor.execute
+
+      audit_events = AuditEvent.where(author_id: user.id)
+      audit_events_messages = audit_events.map(&:details).pluck(:custom_message)
+      expect(audit_events_messages).to contain_exactly(
+        'Changed DAST site profile auth_password (secret value omitted)',
+        'Changed DAST site profile request_headers (secret value omitted)'
+      )
+    end
+
+    it 'omits the values for properties too long to be displayed' do
+      auditor = described_class.new(project, user, {
+        dast_site_profile: profile,
+        new_params: { excluded_urls: ['https://target.test/signout'] },
+        old_params: { excluded_urls: ['https://target.test/signin'] }
+      })
+
+      auditor.execute
+
+      audit_event = AuditEvent.find_by(author_id: user.id)
+      expect(audit_event.details[:custom_message]).to eq(
+        'Changed DAST site profile excluded_urls (long value omitted)'
+      )
+    end
+
+    it 'sorts properties that are arrays before comparing them' do
+      auditor = described_class.new(project, user, {
+        dast_site_profile: profile,
+        new_params: { excluded_urls: ['https://target.test/signin', 'https://target.test/signout'] },
+        old_params: { excluded_urls: ['https://target.test/signout', 'https://target.test/signin'] }
+      })
+
+      expect { auditor.execute }.not_to change { AuditEvent.count }
+    end
+  end
+end

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

@@ -76,6 +76,27 @@ RSpec.describe AppSec::Dast::SiteProfiles::CreateService do
         expect(payload).to be_a(DastSiteProfile)
       end
 
+      it 'audits the creation' do
+        profile = 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('DastSiteProfile')
+          expect(audit_event.target_details).to eq(profile.name)
+          expect(audit_event.details).to eq({
+            author_name: user.name,
+            custom_message: 'Added DAST site profile',
+            target_id: profile.id,
+            target_type: 'DastSiteProfile',
+            target_details: profile.name
+          })
+        end
+      end
+
       context 'when the dast_site already exists' do
         before do
           create(:dast_site, project: project, url: target_url)

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

@@ -51,6 +51,27 @@ RSpec.describe AppSec::Dast::SiteProfiles::DestroyService do
         expect(payload).to be_a(DastSiteProfile)
       end
 
+      it 'audits the deletion' do
+        profile = 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('DastSiteProfile')
+          expect(audit_event.target_details).to eq(profile.name)
+          expect(audit_event.details).to eq({
+            author_name: user.name,
+            custom_message: 'Removed DAST site profile',
+            target_id: profile.id,
+            target_type: 'DastSiteProfile',
+            target_details: profile.name
+          })
+        end
+      end
+
       context 'when the dast_site_profile does not exist' do
         let(:dast_site_profile_id) do
           Gitlab::GlobalId.build(nil, model_name: 'DastSiteProfile', id: 'does_not_exist')

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

@@ -17,6 +17,7 @@ RSpec.describe AppSec::Dast::SiteProfiles::UpdateService do
   let_it_be(:new_request_headers) { "Authorization: Bearer #{SecureRandom.hex}" }
   let_it_be(:new_auth_url) { "#{new_target_url}/login" }
   let_it_be(:new_auth_password) { SecureRandom.hex }
+  let_it_be(:new_auth_username) { generate(:email) }
 
   let(:default_params) do
     {
@@ -29,7 +30,7 @@ RSpec.describe AppSec::Dast::SiteProfiles::UpdateService do
       auth_url: new_auth_url,
       auth_username_field: 'login[username]',
       auth_password_field: 'login[password]',
-      auth_username: generate(:email),
+      auth_username: new_auth_username,
       auth_password: new_auth_password
     }
   end
@@ -81,6 +82,37 @@ RSpec.describe AppSec::Dast::SiteProfiles::UpdateService do
         expect(payload).to be_a(DastSiteProfile)
       end
 
+      it 'audits the update' do
+        profile = payload.reload
+        audit_events = AuditEvent.where(author_id: user.id)
+
+        aggregate_failures do
+          expect(audit_events.count).to be(9)
+
+          audit_events.each do |event|
+            expect(event.author).to eq(user)
+            expect(event.entity).to eq(project)
+            expect(event.target_id).to eq(profile.id)
+            expect(event.target_type).to eq('DastSiteProfile')
+            expect(event.target_details).to eq(profile.name)
+          end
+
+          custom_messages = audit_events.map(&:details).pluck(:custom_message)
+          expected_custom_messages = [
+            "Changed DAST site profile name from #{dast_profile.name} to #{new_profile_name}",
+            "Changed DAST site profile target_url from #{dast_profile.dast_site.url} to #{new_target_url}",
+            'Changed DAST site profile excluded_urls (long value omitted)',
+            "Changed DAST site profile auth_url from #{dast_profile.auth_url} to #{new_auth_url}",
+            "Changed DAST site profile auth_username_field from #{dast_profile.auth_username_field} to login[username]",
+            "Changed DAST site profile auth_password_field from #{dast_profile.auth_password_field} to login[password]",
+            "Changed DAST site profile auth_username from #{dast_profile.auth_username} to #{new_auth_username}",
+            "Changed DAST site profile auth_password (secret value omitted)",
+            "Changed DAST site profile request_headers (secret value omitted)"
+          ]
+          expect(custom_messages).to match_array(expected_custom_messages)
+        end
+      end
+
       context 'when the target url is localhost' do
         let(:new_target_url) { 'http://localhost:3000/hello-world' }