Merge branch '347108-event-streaming-within-transaction' into 'master'

Move event streaming in to after commit queue

See merge request gitlab-org/gitlab!76765

app/models/audit_event.rb

 # frozen_string_literal: true
 
 class AuditEvent < ApplicationRecord
+  include AfterCommitQueue
   include CreatedAtFilterable
   include BulkInsertSafe
   include EachBatch

app/services/audit_event_service.rb

@@ -141,7 +141,7 @@ class AuditEventService
     event.save! if should_save_database?(@save_type)
     stream_event_to_external_destinations(event) if should_save_stream?(@save_type)
   rescue StandardError => e
-    Gitlab::ErrorTracking.track_exception(e, audit_event_type: event.class.to_s)
+    Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e, audit_event_type: event.class.to_s)
   end
 end
 

ee/app/services/ee/audit_event_service.rb

@@ -238,7 +238,9 @@ module EE
     def stream_event_to_external_destinations(event)
       return if event.is_a?(AuthenticationEvent)
 
-      event.stream_to_external_destinations if event.entity_is_group_or_project?
+      if event.entity_is_group_or_project?
+        event.run_after_commit_or_now { event.stream_to_external_destinations }
+      end
     end
 
     override :base_payload

ee/spec/services/audit_event_service_spec.rb

@@ -157,8 +157,7 @@ RSpec.describe AuditEventService, :request_store do
             allow(user).to receive_messages(current_sign_in_ip: 'invalid IP')
 
             expect(Gitlab::ErrorTracking).to(
-              receive(:track_exception)
-                .with(ActiveRecord::RecordInvalid, audit_event_type: 'AuditEvent').and_call_original
+              receive(:track_and_raise_for_dev_exception)
             )
 
             service.security_event
@@ -365,8 +364,7 @@ RSpec.describe AuditEventService, :request_store do
   end
 
   describe '#for_user' do
-    let(:author_name) { 'Administrator' }
-    let(:current_user) { User.new(name: author_name) }
+    let(:current_user) { create(:user) }
     let(:user) { create(:user) }
     let(:custom_message) { 'Some strange event has occurred' }
     let(:options) { { action: action, custom_message: custom_message } }
@@ -384,7 +382,7 @@ RSpec.describe AuditEventService, :request_store do
       it 'sets the details attribute' do
         expect(service.instance_variable_get(:@details)).to eq(
           remove: 'user',
-          author_name: author_name,
+          author_name: current_user.name,
           target_id: user.id,
           target_type: 'User',
           target_details: user.username
@@ -402,7 +400,7 @@ RSpec.describe AuditEventService, :request_store do
       it 'sets the details attribute' do
         expect(service.instance_variable_get(:@details)).to eq(
           add: 'user',
-          author_name: author_name,
+          author_name: current_user.name,
           target_id: user.id,
           target_type: 'User',
           target_details: user.full_path
@@ -420,7 +418,7 @@ RSpec.describe AuditEventService, :request_store do
       it 'sets the details attribute' do
         expect(service.instance_variable_get(:@details)).to eq(
           custom_message: custom_message,
-          author_name: author_name,
+          author_name: current_user.name,
           target_id: user.id,
           target_type: 'User',
           target_details: user.full_path
@@ -653,6 +651,7 @@ RSpec.describe AuditEventService, :request_store do
 
     before do
       stub_licensed_features(external_audit_events: true)
+      stub_feature_flags(ff_external_audit_events_namespace: group.root_ancestor)
     end
 
     subject(:event) { described_class.new(user, project, details, save_type).for_project.security_event }
@@ -660,10 +659,27 @@ RSpec.describe AuditEventService, :request_store do
     context 'with save_type of :database_and_stream' do
       let(:save_type) { :database_and_stream }
 
-      it 'save to database and stream' do
-        expect(AuditEvents::AuditEventStreamingWorker).to receive(:perform_async).once
+      it 'saves to database' do
         expect { event }.to change(AuditEvent, :count).by(1)
       end
+
+      it 'streams the event' do
+        expect_next_instance_of(described_class) do |service|
+          expect(service).to receive(:stream_event_to_external_destinations).once
+        end
+
+        event
+      end
+
+      context 'when the event is created within a transaction' do
+        it 'does not raise an error about a job being enqueued from within a transaction' do
+          RSpec::Expectations.configuration.on_potential_false_positives = :nothing
+
+          ApplicationRecord.transaction do
+            expect { event }.not_to raise_error(Sidekiq::Worker::EnqueueFromTransactionError)
+          end
+        end
+      end
     end
 
     context 'with save_type of :database' do
@@ -678,9 +694,16 @@ RSpec.describe AuditEventService, :request_store do
     context 'with save_type of :stream' do
       let(:save_type) { :stream }
 
-      it 'saves to database and stream' do
-        expect(AuditEvents::AuditEventStreamingWorker).to receive(:perform_async).once
-        expect { event }.to change(AuditEvent, :count).by(0)
+      it 'does not save to database' do
+        expect { event }.not_to change(AuditEvent, :count)
+      end
+
+      it 'streams the event' do
+        expect_next_instance_of(described_class) do |service|
+          expect(service).to receive(:stream_event_to_external_destinations).once
+        end
+
+        event
       end
     end
   end

spec/services/audit_event_service_spec.rb

@@ -60,17 +60,18 @@ RSpec.describe AuditEventService do
           ip_address: user.current_sign_in_ip,
           result: AuthenticationEvent.results[:success],
           provider: 'standard'
-        )
+        ).and_call_original
 
         audit_service.for_authentication.security_event
       end
 
       it 'tracks exceptions when the event cannot be created' do
-        allow(user).to receive_messages(current_sign_in_ip: 'invalid IP')
+        allow_next_instance_of(AuditEvent) do |event|
+          allow(event).to receive(:valid?).and_return(false)
+        end
 
         expect(Gitlab::ErrorTracking).to(
-          receive(:track_exception)
-            .with(ActiveRecord::RecordInvalid, audit_event_type: 'AuthenticationEvent').and_call_original
+          receive(:track_and_raise_for_dev_exception)
         )
 
         audit_service.for_authentication.security_event
@@ -93,7 +94,7 @@ RSpec.describe AuditEventService do
           end
 
           specify do
-            expect(AuthenticationEvent).to receive(:new).with(hash_including(ip_address: output))
+            expect(AuthenticationEvent).to receive(:new).with(hash_including(ip_address: output)).and_call_original
 
             audit_service.for_authentication.security_event
           end

spec/support/shared_examples/lib/gitlab/unique_ip_check_shared_examples.rb

@@ -7,13 +7,13 @@ RSpec.shared_examples 'user login operation with unique ip limit' do
     end
 
     it 'allows user authenticating from the same ip' do
-      expect { operation_from_ip('ip') }.not_to raise_error
-      expect { operation_from_ip('ip') }.not_to raise_error
+      expect { operation_from_ip('111.221.4.3') }.not_to raise_error
+      expect { operation_from_ip('111.221.4.3') }.not_to raise_error
     end
 
     it 'blocks user authenticating from two distinct ips' do
-      expect { operation_from_ip('ip') }.not_to raise_error
-      expect { operation_from_ip('ip2') }.to raise_error(Gitlab::Auth::TooManyIps)
+      expect { operation_from_ip('111.221.4.3') }.not_to raise_error
+      expect { operation_from_ip('1.2.2.3') }.to raise_error(Gitlab::Auth::TooManyIps)
     end
   end
 end
@@ -25,13 +25,13 @@ RSpec.shared_examples 'user login request with unique ip limit' do |success_stat
     end
 
     it 'allows user authenticating from the same ip' do
-      expect(request_from_ip('ip')).to have_gitlab_http_status(success_status)
-      expect(request_from_ip('ip')).to have_gitlab_http_status(success_status)
+      expect(request_from_ip('111.221.4.3')).to have_gitlab_http_status(success_status)
+      expect(request_from_ip('111.221.4.3')).to have_gitlab_http_status(success_status)
     end
 
     it 'blocks user authenticating from two distinct ips' do
-      expect(request_from_ip('ip')).to have_gitlab_http_status(success_status)
-      expect(request_from_ip('ip2')).to have_gitlab_http_status(:forbidden)
+      expect(request_from_ip('111.221.4.3')).to have_gitlab_http_status(success_status)
+      expect(request_from_ip('1.2.2.3')).to have_gitlab_http_status(:forbidden)
     end
   end
 end