Merge branch '366623-use-auditor-in-audit-changes-module' into 'master'

Use auditor when auditing using audit changes

See merge request gitlab-org/gitlab!91461

ee/app/services/audit_events/build_service.rb

@@ -10,7 +10,7 @@ class BuildService
     # @return [BuildService]
     def initialize(
       author:, scope:, target:, message:,
-      created_at: DateTime.current, additional_details: {}, ip_address: nil)
+      created_at: DateTime.current, additional_details: {}, ip_address: nil, target_details: nil)
       raise MissingAttributeError if missing_attribute?(author, scope, target, message)
 
       @author = build_author(author)
@@ -20,6 +20,7 @@ def initialize(
       @message = build_message(message)
       @created_at = created_at
       @additional_details = additional_details
+      @target_details = target_details
     end
 
     # Create an instance of AuditEvent
@@ -61,13 +62,13 @@ def base_payload
     end
 
     def base_details_payload
-      {
+      @additional_details.merge({
         author_name: @author.name,
         target_id: @target.id,
         target_type: @target.type,
-        target_details: @target.details,
+        target_details: @target_details || @target.details,
         custom_message: @message
-      }.merge(@additional_details)
+      })
     end
 
     def build_author(author)

ee/lib/audit/changes.rb

@@ -10,7 +10,7 @@ module Changes
     # @option options [User, Project, Group] :target_model scope the event belongs to
     # @option options [Object] :model object being audited
     # @option options [Boolean] :skip_changes whether to record from/to values
-    #
+    # @option options [String] :event_type adds event type in streaming audit event headers and payload
     # @return [AuditEvent, nil] the resulting object or nil if there is no
     #   change detected
     def audit_changes(column, options = {})
@@ -66,8 +66,41 @@ def parse_options(column, options)
     end
 
     def audit_event(options)
-      ::AuditEventService.new(@current_user, entity, options) # rubocop:disable Gitlab/ModuleWithInstanceVariables
-        .for_changes(model).security_event
+      return unless audit_enabled?
+
+      name = options.fetch(:event_type, 'audit_operation')
+      details = additional_details(options)
+      audit_context = {
+        name: name,
+        author: @current_user, # rubocop:disable Gitlab/ModuleWithInstanceVariables
+        scope: entity,
+        target: model,
+        message: build_message(details),
+        additional_details: details,
+        target_details: options[:target_details]
+      }
+
+      ::Gitlab::Audit::Auditor.audit(audit_context)
+    end
+
+    def additional_details(options)
+      { change: options[:as] || options[:column] }.merge(options.slice(:from, :to, :target_details))
+    end
+
+    def build_message(details)
+      message = ["Changed #{details[:change]}"]
+      message << "from #{details[:from]}" unless details[:from].blank?
+      message << "to #{details[:to]}" unless details[:to].blank?
+      message.join(' ')
+    end
+
+    # TODO: Remove this once we implement license feature checks in Auditor.
+    # issue link: https://gitlab.com/gitlab-org/gitlab/-/issues/365441
+    def audit_enabled?
+      return true if ::License.feature_available?(:admin_audit_log)
+      return true if ::License.feature_available?(:extended_audit_events)
+
+      entity.respond_to?(:feature_available?) && entity.licensed_feature_available?(:audit_events)
     end
   end
 end

ee/lib/gitlab/audit/auditor.rb

@@ -63,6 +63,7 @@ def initialize(context = {})
         @message = @context.fetch(:message, '')
         @additional_details = @context.fetch(:additional_details, {})
         @ip_address = @context[:ip_address]
+        @target_details = @context[:target_details]
       end
 
       def multiple_audit
@@ -103,7 +104,8 @@ def build_event(message)
           created_at: @created_at,
           message: message,
           additional_details: @additional_details,
-          ip_address: @ip_address
+          ip_address: @ip_address,
+          target_details: @target_details
         ).execute
       end
 

ee/spec/lib/audit/changes_spec.rb

@@ -39,42 +39,44 @@
     end
 
     describe 'audit changes' do
-      let(:audit_event_service) { instance_spy(AuditEventService) }
+      let(:options) { { model: user, event_type: 'audit_operation' } }
 
-      before do
-        allow(AuditEventService).to receive(:new).and_return(audit_event_service)
-      end
-
-      it 'calls the audit event service' do
+      it 'calls the auditor' do
         user.update!(name: 'Scrooge McDuck')
 
-        audit!
+        expect(::Gitlab::Audit::Auditor).to receive(:audit).with(
+          { additional_details: { change: :name,
+                                  from: "Donald Duck",
+                                  to: "Scrooge McDuck" },
+            name: 'audit_operation',
+            author: current_user,
+            scope: user,
+            target: user,
+            message: "Changed name from Donald Duck to Scrooge McDuck",
+            target_details: nil }
+        )
 
-        aggregate_failures 'audit event service interactions' do
-          expect(AuditEventService).to have_received(:new)
-            .with(
-              current_user, user,
-              model: user,
-              action: :update, column: :name,
-              from: 'Donald Duck', to: 'Scrooge McDuck'
-            )
-          expect(audit_event_service).to have_received(:for_changes)
-          expect(audit_event_service).to have_received(:security_event)
-        end
+        audit!
       end
 
-      context 'when entity is provided' do
-        let(:project) { Project.new }
-        let(:options) { { model: user, entity: project } }
-
-        it 'instantiates audit event service with the given entity' do
-          user.update!(name: 'Scrooge McDuck')
+      it 'creates audit event with correct attributes', :aggregate_failure do
+        user.update!(name: 'Scrooge McDuck')
 
-          audit!
+        audit!
 
-          expect(AuditEventService).to have_received(:new)
-            .with(anything, project, anything)
-        end
+        audit_event = AuditEvent.last
+
+        expect(audit_event.author_id).to eq(current_user.id)
+        expect(audit_event.entity_id).to eq(user.id)
+        expect(audit_event.entity_type).to eq(user.class.name)
+        expect(audit_event.details).to eq({ change: :name,
+                                            author_name: current_user.name,
+                                            from: "Donald Duck",
+                                            to: "Scrooge McDuck",
+                                            target_details: user.name,
+                                            target_id: user.id,
+                                            target_type: user.class.name,
+                                            custom_message: "Changed name from Donald Duck to Scrooge McDuck" })
       end
     end
   end

ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb

@@ -32,7 +32,8 @@
           expect { service.execute }.to change { AuditEvent.count }.by(1)
 
           event = AuditEvent.last
-          expect(event.details).to eq({ change: "#{setting}".humanize(capitalize: false),
+          change_text = setting.to_s.humanize(capitalize: false)
+          expect(event.details).to eq({ change: change_text,
                                         author_name: author.name,
                                         target_id: protected_branch.id,
                                         entity_path: entity.full_path,
@@ -40,7 +41,8 @@
                                         target_details: protected_branch.name,
                                         from: false,
                                         to: true,
-                                        ip_address: ip_address })
+                                        ip_address: ip_address,
+                                        custom_message: "Changed #{change_text} to true" })
 
           expect(event.author_id).to eq(author.id)
           expect(event.entity_id).to eq(entity.id)

ee/spec/lib/gitlab/audit/auditor_spec.rb

@@ -191,6 +191,43 @@
         end
       end
 
+      context 'when overriding the target_details' do
+        target_details = "this is my target details"
+        let(:context) do
+          { name: name,
+            author: author,
+            scope: scope,
+            target: target,
+            created_at: Time.zone.now,
+            target_details: target_details }
+        end
+
+        it 'logs audit events to database' do
+          freeze_time do
+            audit!
+
+            audit_event = AuditEvent.last
+            expect(audit_event.details).to include({ target_details: target_details })
+            expect(audit_event.target_details).to eq(target_details)
+          end
+        end
+
+        it 'logs audit events to file' do
+          freeze_time do
+            expect(::Gitlab::AuditJsonLogger).to receive(:build).and_return(logger)
+
+            audit!
+
+            expect(logger).to have_received(:info).exactly(2).times.with(
+              hash_including(
+                'details' => hash_including('target_details' => target_details),
+                'target_details' => target_details
+              )
+            )
+          end
+        end
+      end
+
       context 'when overriding the ip address' do
         ip_address = '192.168.8.8'
         let(:context) { { name: name, author: author, scope: scope, target: target, ip_address: ip_address } }

ee/spec/services/groups/update_service_spec.rb

@@ -40,7 +40,8 @@
             param[:details].merge!(
               change: 'visibility',
               from: 'Public',
-              to: 'Private'
+              to: 'Private',
+              custom_message: "Changed visibility from Public to Private"
             )
           end
         end

ee/spec/services/projects/transfer_service_spec.rb

@@ -48,7 +48,8 @@
              author_name: user.name,
              target_id: project.id,
              target_type: 'Project',
-             target_details: project.full_path
+             target_details: project.full_path,
+             custom_message: "Changed namespace from #{project.old_path_with_namespace} to #{project.full_path}"
            }
          }
       end

ee/spec/services/projects/update_service_spec.rb

@@ -152,7 +152,8 @@
             param[:details].merge!(
               change: 'name',
               from: old_name,
-              to: project.full_name
+              to: project.full_name,
+              custom_message: "Changed name from #{old_name} to #{project.full_name}"
             )
           end
         end
@@ -171,7 +172,8 @@
             param[:details].merge!(
               change: 'path',
               from: project.old_path_with_namespace,
-              to: project.full_path
+              to: project.full_path,
+              custom_message: "Changed path from #{project.old_path_with_namespace} to #{project.full_path}"
             )
           end
         end
@@ -212,7 +214,8 @@
             param[:details].merge!(
               change: 'visibility',
               from: 'Private',
-              to: 'Internal'
+              to: 'Internal',
+              custom_message: "Changed visibility from Private to Internal"
             )
           end
         end