Merge branch '213603-include-changes-to-approval-groups-in-audit-log' into 'master'

Improve audit event instrumentation with in-memory queue

See merge request gitlab-org/gitlab!35938

app/models/audit_event.rb

@@ -3,6 +3,7 @@
 class AuditEvent < ApplicationRecord
   include CreatedAtFilterable
   include IgnorableColumns
+  include BulkInsertSafe
 
   ignore_column :updated_at, remove_with: '13.4', remove_after: '2020-09-22'
 

ee/app/models/concerns/auditable.rb

+# frozen_string_literal: true
+
+module Auditable
+  def push_audit_event(event)
+    return unless ::Gitlab::Audit::EventQueue.active?
+
+    ::Gitlab::Audit::EventQueue.current << event
+  end
+end

ee/app/services/audit_events/build_service.rb

+# frozen_string_literal: true
+
+module AuditEvents
+  class BuildService
+    def initialize(author:, scope:, target:, ip_address:, message:)
+      @author = author
+      @scope = scope
+      @target = target
+      @ip_address = ip_address
+      @message = message
+    end
+
+    def execute
+      SecurityEvent.new(payload)
+    end
+
+    private
+
+    def payload
+      if License.feature_available?(:admin_audit_log)
+        base_payload.merge(
+          details: base_details_payload.merge(
+            ip_address: @ip_address,
+            entity_path: @scope.full_path
+          ),
+          ip_address: ip_address
+        )
+      else
+        base_payload.merge(details: base_details_payload)
+      end
+    end
+
+    def base_payload
+      {
+        author_id: @author.id,
+        author_name: @author.name,
+        entity_id: @scope.id,
+        entity_type: @scope.class.name,
+        created_at: DateTime.current
+      }
+    end
+
+    def base_details_payload
+      {
+        author_name: @author.name,
+        target_id: @target.id,
+        target_type: @target.class.name,
+        target_details: @target.name,
+        custom_message: @message
+      }
+    end
+
+    def ip_address
+      @ip_address.presence || @author.current_sign_in_ip
+    end
+  end
+end

ee/lib/gitlab/audit/auditor.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Audit
+    class Auditor
+      # Record audit events for block
+      #
+      # @param [Hash] context
+      # @option context [String] :name the operation name to be audited, used for error tracking
+      # @option context [User] :author the user who authors the change
+      # @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
+      #
+      # @example Wrap operation to be audit logged
+      #
+      #   Gitlab::Audit::Auditor.audit(context) do
+      #     service.execute
+      #   end
+      #
+      # @return result of block execution
+      def self.audit(context)
+        auditor = new(context)
+
+        auditor.audit { yield }
+      end
+
+      def initialize(context)
+        @context = context
+
+        @name = @context.fetch(:name, 'audit_operation')
+        @author = @context.fetch(:author)
+        @scope = @context.fetch(:scope)
+        @target = @context.fetch(:target)
+        @ip_address = @context.fetch(:ip_address, nil)
+      end
+
+      def audit
+        ::Gitlab::Audit::EventQueue.begin!
+
+        return_value = yield
+
+        record
+
+        return_value
+      ensure
+        ::Gitlab::Audit::EventQueue.end!
+      end
+
+      private
+
+      def record
+        events = ::Gitlab::Audit::EventQueue.current.reverse.map(&method(:build_event))
+
+        log_to_database(events)
+        log_to_file(events)
+      end
+
+      def build_event(message)
+        AuditEvents::BuildService.new(
+          author: @author,
+          scope: @scope,
+          target: @target,
+          ip_address: @ip_address,
+          message: message
+        ).execute
+      end
+
+      def log_to_database(events)
+        AuditEvent.bulk_insert!(events)
+      rescue ActiveRecord::RecordInvalid => error
+        ::Gitlab::ErrorTracking.track_exception(error, audit_operation: @name)
+      end
+
+      def log_to_file(events)
+        file_logger = ::Gitlab::AuditJsonLogger.build
+
+        events.each { |event| file_logger.info(event.as_json) }
+      end
+    end
+  end
+end

ee/lib/gitlab/audit/event_queue.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Audit
+    module EventQueue
+      module_function
+
+      def begin!
+        ::Gitlab::SafeRequestStore[:audit_active] = true
+      end
+
+      def current
+        ::Gitlab::SafeRequestStore[:audit] ||= []
+      end
+
+      def push(event)
+        current << event
+      end
+
+      def active?
+        ::Gitlab::SafeRequestStore[:audit_active] || false
+      end
+
+      def end!
+        ::Gitlab::SafeRequestStore.delete(:audit)
+        ::Gitlab::SafeRequestStore.delete(:audit_active)
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Audit::Auditor do
+  let(:name) { 'play_with_project_settings' }
+  let(:author) { build_stubbed(:user) }
+  let(:scope) { build_stubbed(:group) }
+  let(:target) { build_stubbed(:project) }
+  let(:ip_address) { '192.168.8.8' }
+  let(:context) { { name: name, author: author, scope: scope, target: target, ip_address: ip_address } }
+  let(:add_message) { 'Added an interesting field from project Gotham' }
+  let(:remove_message) { 'Removed an interesting field from project Gotham' }
+  let(:operation) do
+    proc do
+      ::Gitlab::Audit::EventQueue.current << add_message
+      ::Gitlab::Audit::EventQueue.current << remove_message
+    end
+  end
+
+  let(:logger) { instance_spy(Gitlab::AuditJsonLogger) }
+
+  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
+
+      auditor.audit(context, &operation)
+
+      expect(Gitlab::Audit::EventQueue).to have_received(:begin!).ordered
+      expect(Gitlab::Audit::EventQueue).to have_received(:end!).ordered
+    end
+
+    it 'records audit events in correct order', :aggregate_failures do
+      expect { auditor.audit(context, &operation) }.to change { AuditEvent.count }.by(2)
+
+      event_messages = AuditEvent.all.order(created_at: :desc).map { |event| event.details[:custom_message] }
+
+      expect(event_messages).to eq([add_message, remove_message])
+    end
+
+    it 'bulk-inserts audit events to database' do
+      allow(AuditEvent).to receive(:bulk_insert!)
+
+      auditor.audit(context, &operation)
+
+      expect(AuditEvent).to have_received(:bulk_insert!)
+    end
+
+    it 'logs audit events to database', :aggregate_failures do
+      auditor.audit(context, &operation)
+
+      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 file' do
+      expect(::Gitlab::AuditJsonLogger).to receive(:build).and_return(logger)
+
+      auditor.audit(context, &operation)
+
+      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
+
+    context 'when audit events are invalid' do
+      before do
+        allow(AuditEvent).to receive(:bulk_insert!).and_raise(ActiveRecord::RecordInvalid)
+        allow(Gitlab::ErrorTracking).to receive(:track_exception)
+      end
+
+      it 'tracks error' do
+        auditor.audit(context, &operation)
+
+        expect(Gitlab::ErrorTracking).to have_received(:track_exception).with(
+          kind_of(ActiveRecord::RecordInvalid),
+          { audit_operation: name }
+        )
+      end
+
+      it 'does not throw exception' do
+        expect { auditor.audit(context, &operation) }.not_to raise_exception
+      end
+    end
+  end
+end

ee/spec/models/concerns/auditable_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Auditable do
+  let(:klazz) { Class.new { include Auditable } }
+
+  subject(:instance) { klazz.new }
+
+  describe '#push_audit_event', :request_store do
+    let(:event) { 'Added a new cat to the house' }
+
+    context 'when audit event queue is active' do
+      before do
+        allow(::Gitlab::Audit::EventQueue).to receive(:active?).and_return(true)
+      end
+
+      it 'add message to audit event queue' do
+        instance.push_audit_event(event)
+
+        expect(::Gitlab::Audit::EventQueue.current).to eq([event])
+      end
+    end
+
+    context 'when audit event queue is not active' do
+      before do
+        allow(::Gitlab::Audit::EventQueue).to receive(:active?).and_return(false)
+      end
+
+      it 'does not add message to audit event queue' do
+        instance.push_audit_event(event)
+
+        expect(::Gitlab::Audit::EventQueue.current).to eq([])
+      end
+    end
+  end
+end

ee/spec/services/audit_events/build_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe AuditEvents::BuildService do
+  let(:author) { build(:author, current_sign_in_ip: '127.0.0.1') }
+  let(:scope) { build(:group) }
+  let(:target) { build(:project) }
+  let(:ip_address) { '192.168.8.8' }
+  let(:message) { 'Added an interesting field from project Gotham' }
+
+  subject(:service) do
+    described_class.new(
+      author: author,
+      scope: scope,
+      target: target,
+      ip_address: ip_address,
+      message: message
+    )
+  end
+
+  describe '#execute' do
+    subject(:event) { service.execute }
+
+    context 'when licensed' do
+      before do
+        stub_licensed_features(admin_audit_log: true)
+      end
+
+      it 'sets correct attributes', :aggregate_failures do
+        freeze_time do
+          expect(event).to have_attributes(
+            author_id: author.id,
+            author_name: author.name,
+            entity_id: scope.id,
+            entity_type: scope.class.name
+          )
+
+          expect(event.details).to eq(
+            author_name: author.name,
+            target_id: target.id,
+            target_type: target.class.name,
+            target_details: target.name,
+            custom_message: message,
+            ip_address: ip_address,
+            entity_path: scope.full_path
+          )
+
+          expect(event.ip_address).to eq(ip_address)
+          expect(event.created_at).to eq(DateTime.current)
+        end
+      end
+
+      context 'when IP address is not provided' do
+        let(:ip_address) { nil }
+
+        it 'uses author current_sign_in_ip' do
+          expect(event.ip_address).to eq(author.current_sign_in_ip)
+        end
+      end
+    end
+
+    context 'when not licensed' do
+      before do
+        stub_licensed_features(admin_audit_log: false)
+      end
+
+      it 'sets correct attributes', :aggregate_failures do
+        freeze_time do
+          expect(event).to have_attributes(
+            author_id: author.id,
+            author_name: author.name,
+            entity_id: scope.id,
+            entity_type: scope.class.name
+          )
+
+          expect(event.details).to eq(
+            author_name: author.name,
+            target_id: target.id,
+            target_type: target.class.name,
+            target_details: target.name,
+            custom_message: message
+          )
+
+          expect(event.ip_address).to be_nil
+          expect(event.created_at).to eq(DateTime.current)
+        end
+      end
+    end
+  end
+end