Merge branch '329268-add-support-for-standard-method-call-interface-on-auditor' into 'master'

Add support for standard method call interface on Auditor

See merge request gitlab-org/gitlab!60591

ee/app/services/audit_events/build_service.rb

@@ -2,6 +2,8 @@
 
 module AuditEvents
   class BuildService
+    MissingAttributeError = Class.new(StandardError)
+
     def initialize(author:, scope:, target:, ip_address:, message:)
       @author = author
       @scope = scope
@@ -10,17 +12,28 @@ module AuditEvents
       @message = message
     end
 
+    # Create an instance of AuditEvent
+    #
+    # @raise [MissingAttributeError] when required attributes are blank
+    #
+    # @return [AuditEvent]
     def execute
+      raise MissingAttributeError if missing_attribute?
+
       AuditEvent.new(payload)
     end
 
     private
 
+    def missing_attribute?
+      @author.blank? || @scope.blank? || @target.blank? || @message.blank?
+    end
+
     def payload
       if License.feature_available?(:admin_audit_log)
         base_payload.merge(
           details: base_details_payload.merge(
-            ip_address: @ip_address,
+            ip_address: ip_address,
             entity_path: @scope.full_path
           ),
           ip_address: ip_address

ee/lib/gitlab/audit/auditor.rb

@@ -3,7 +3,7 @@
 module Gitlab
   module Audit
     class Auditor
-      # Record audit events for block
+      # Record audit events
       #
       # @param [Hash] context
       # @option context [String] :name the operation name to be audited, used for error tracking
@@ -11,21 +11,47 @@ module Gitlab
       # @option context [User, Project, Group] :scope the scope which audit event belongs to
       # @option context [Object] :target the target object being audited
       # @option context [Object] :ip_address the request IP address
+      # @option context [String] :message the message describing the action
       #
-      # @example Wrap operation to be audit logged
+      # @example Using block (useful when events are emitted deep in the call stack)
+      #   i.e. multiple audit events
       #
-      #   Gitlab::Audit::Auditor.audit(context) do
+      #   audit_context = {
+      #     name: 'merge_approval_rule_updated',
+      #     author: current_user,
+      #     scope: project_alpha,
+      #     target: merge_approval_rule,
+      #     ip_address: request.remote_ip
+      #     message: 'a user has attempted to update an approval rule'
+      #   }
+      #
+      #   # in the initiating service
+      #   Gitlab::Audit::Auditor.audit(audit_context) do
       #     service.execute
       #   end
       #
+      #   # in the model
+      #   Auditable.push_audit_event('an approver has been added')
+      #   Auditable.push_audit_event('an approval group has been removed')
+      #
+      # @example Using standard method call
+      #   i.e. single audit event
+      #
+      #   merge_approval_rule.save
+      #   Gitlab::Audit::Auditor.audit(audit_context)
+      #
       # @return result of block execution
-      def self.audit(context)
+      def self.audit(context, &block)
         auditor = new(context)
 
-        auditor.audit { yield }
+        if block
+          auditor.multiple_audit(&block)
+        else
+          auditor.single_audit
+        end
       end
 
-      def initialize(context)
+      def initialize(context = {})
         @context = context
 
         @name = @context.fetch(:name, 'audit_operation')
@@ -33,25 +59,29 @@ module Gitlab
         @scope = @context.fetch(:scope)
         @target = @context.fetch(:target)
         @ip_address = @context.fetch(:ip_address, nil)
+        @message = @context.fetch(:message, '')
       end
 
-      def audit
+      def multiple_audit
         ::Gitlab::Audit::EventQueue.begin!
 
         return_value = yield
 
-        record
+        ::Gitlab::Audit::EventQueue.current
+          .map { |message| build_event(message) }
+          .then { |events| record(events) }
 
         return_value
       ensure
         ::Gitlab::Audit::EventQueue.end!
       end
 
-      private
-
-      def record
-        events = ::Gitlab::Audit::EventQueue.current.map { |message| build_event(message) }
+      def single_audit
+        events = [build_event(@message)]
+        record(events)
+      end
 
+      def record(events)
         log_to_database(events)
         log_to_file(events)
       end

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

@@ -22,59 +22,102 @@ RSpec.describe Gitlab::Audit::Auditor do
 
   subject(:auditor) { described_class }
 
-  describe '.audit', :request_store do
-    it 'interacts with the event queue in correct order', :aggregate_failures do
-      allow(Gitlab::Audit::EventQueue).to receive(:begin!).and_call_original
-      allow(Gitlab::Audit::EventQueue).to receive(:end!).and_call_original
+  describe '.audit' do
+    context 'when recording multiple events', :request_store do
+      let(:audit!) { auditor.audit(context, &operation) }
 
-      auditor.audit(context, &operation)
+      it 'interacts with the event queue in correct order', :aggregate_failures do
+        allow(Gitlab::Audit::EventQueue).to receive(:begin!).and_call_original
+        allow(Gitlab::Audit::EventQueue).to receive(:end!).and_call_original
 
-      expect(Gitlab::Audit::EventQueue).to have_received(:begin!).ordered
-      expect(Gitlab::Audit::EventQueue).to have_received(:end!).ordered
-    end
+        audit!
 
-    it 'records audit events in correct order', :aggregate_failures do
-      expect { auditor.audit(context, &operation) }.to change(AuditEvent, :count).by(2)
+        expect(Gitlab::Audit::EventQueue).to have_received(:begin!).ordered
+        expect(Gitlab::Audit::EventQueue).to have_received(:end!).ordered
+      end
 
-      event_messages = AuditEvent.all.map { |event| event.details[:custom_message] }
+      it 'bulk-inserts audit events to database' do
+        allow(AuditEvent).to receive(:bulk_insert!)
 
-      expect(event_messages).to eq([add_message, remove_message])
-    end
+        audit!
 
-    it 'bulk-inserts audit events to database' do
-      allow(AuditEvent).to receive(:bulk_insert!)
+        expect(AuditEvent).to have_received(:bulk_insert!)
+      end
 
-      auditor.audit(context, &operation)
+      it 'records audit events in correct order', :aggregate_failures do
+        expect { audit! }.to change(AuditEvent, :count).by(2)
 
-      expect(AuditEvent).to have_received(:bulk_insert!)
-    end
+        event_messages = AuditEvent.all.map { |event| event.details[:custom_message] }
+
+        expect(event_messages).to eq([add_message, remove_message])
+      end
+
+      it 'logs audit events to database', :aggregate_failures do
+        audit!
+
+        audit_event = AuditEvent.last
+
+        expect(audit_event.author_id).to eq(author.id)
+        expect(audit_event.entity_id).to eq(scope.id)
+        expect(audit_event.entity_type).to eq(scope.class.name)
+        expect(audit_event.details[:target_id]).to eq(target.id)
+        expect(audit_event.details[:target_type]).to eq(target.class.name)
+      end
 
-    it 'logs audit events to database', :aggregate_failures do
-      auditor.audit(context, &operation)
+      it 'logs audit events to file' do
+        expect(::Gitlab::AuditJsonLogger).to receive(:build).and_return(logger)
 
-      audit_event = AuditEvent.last
+        audit!
 
-      expect(audit_event.author_id).to eq(author.id)
-      expect(audit_event.entity_id).to eq(scope.id)
-      expect(audit_event.entity_type).to eq(scope.class.name)
-      expect(audit_event.details[:target_id]).to eq(target.id)
-      expect(audit_event.details[:target_type]).to eq(target.class.name)
+        expect(logger).to have_received(:info).exactly(2).times.with(
+          hash_including(
+            'author_id' => author.id,
+            'author_name' => author.name,
+            'entity_id' => scope.id,
+            'entity_type' => scope.class.name,
+            'details' => kind_of(Hash)
+          )
+        )
+      end
     end
 
-    it 'logs audit events to file' do
-      expect(::Gitlab::AuditJsonLogger).to receive(:build).and_return(logger)
+    context 'when recording single event' do
+      let(:audit!) { auditor.audit(context) }
+      let(:context) do
+        {
+          name: name, author: author, scope: scope, target: target, ip_address: ip_address,
+          message: 'Project has been deleted'
+        }
+      end
+
+      it 'logs audit event to database', :aggregate_failures do
+        expect { audit! }.to change(AuditEvent, :count).by(1)
 
-      auditor.audit(context, &operation)
+        audit_event = AuditEvent.last
 
-      expect(logger).to have_received(:info).exactly(2).times.with(
-        hash_including(
-          'author_id' => author.id,
-          'author_name' => author.name,
-          'entity_id' => scope.id,
-          'entity_type' => scope.class.name,
-          'details' => kind_of(Hash)
+        expect(audit_event.author_id).to eq(author.id)
+        expect(audit_event.entity_id).to eq(scope.id)
+        expect(audit_event.entity_type).to eq(scope.class.name)
+        expect(audit_event.details[:target_id]).to eq(target.id)
+        expect(audit_event.details[:target_type]).to eq(target.class.name)
+        expect(audit_event.details[:custom_message]).to eq('Project has been deleted')
+      end
+
+      it 'logs audit events to file' do
+        expect(::Gitlab::AuditJsonLogger).to receive(:build).and_return(logger)
+
+        audit!
+
+        expect(logger).to have_received(:info).once.with(
+          hash_including(
+            'author_id' => author.id,
+            'author_name' => author.name,
+            'entity_id' => scope.id,
+            'entity_type' => scope.class.name,
+            'details' => kind_of(Hash)
+          )
         )
-      )
+      end
     end
 
     context 'when audit events are invalid' do

ee/spec/services/audit_events/build_service_spec.rb

@@ -87,5 +87,31 @@ RSpec.describe AuditEvents::BuildService do
         end
       end
     end
+
+    context 'when attributes are missing' do
+      context 'when author is missing' do
+        let(:author) { nil }
+
+        it { expect { event }.to raise_error(described_class::MissingAttributeError) }
+      end
+
+      context 'when scope is missing' do
+        let(:scope) { nil }
+
+        it { expect { event }.to raise_error(described_class::MissingAttributeError) }
+      end
+
+      context 'when target is missing' do
+        let(:target) { nil }
+
+        it { expect { event }.to raise_error(described_class::MissingAttributeError) }
+      end
+
+      context 'when message is missing' do
+        let(:message) { nil }
+
+        it { expect { event }.to raise_error(described_class::MissingAttributeError) }
+      end
+    end
   end
 end