Merge branch '220318-refactor-author_name-from-auditevent-details' into 'master'

Add parallel persistence for author_name on AuditEvent

See merge request gitlab-org/gitlab!35130

app/models/audit_event.rb

@@ -52,7 +52,7 @@ class AuditEvent < ApplicationRecord
   private
 
   def default_author_value
-    ::Gitlab::Audit::NullAuthor.for(author_id, details[:author_name])
+    ::Gitlab::Audit::NullAuthor.for(author_id, (self[:author_name] || details[:author_name]))
   end
 end
 

app/services/audit_event_service.rb

@@ -61,6 +61,7 @@ class AuditEventService
   def base_payload
     {
       author_id: @author.id,
+      author_name: @author.name,
       entity_id: @entity.id,
       entity_type: @entity.class.name
     }

changelogs/unreleased/220318-refactor-author_name-from-auditevent-details.yml

+---
+title: Add parallel persistence for author_name on AuditEvent
+merge_request: 35130
+author:
+type: changed

db/migrate/20200623090030_add_author_name_to_audit_event.rb

+# frozen_string_literal: true
+
+class AddAuthorNameToAuditEvent < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  disable_ddl_transaction!
+
+  def up
+    unless column_exists?(:audit_events, :author_name)
+      with_lock_retries do
+        add_column :audit_events, :author_name, :text
+      end
+    end
+
+    add_text_limit :audit_events, :author_name, 255
+  end
+
+  def down
+    remove_column :audit_events, :author_name
+  end
+end

db/structure.sql

@@ -9366,7 +9366,9 @@ CREATE TABLE public.audit_events (
     details text,
     created_at timestamp without time zone,
     updated_at timestamp without time zone,
-    ip_address inet
+    ip_address inet,
+    author_name text,
+    CONSTRAINT check_83ff8406e2 CHECK ((char_length(author_name) <= 255))
 );
 
 CREATE SEQUENCE public.audit_events_id_seq
@@ -23561,6 +23563,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200622235737
 20200623000148
 20200623000320
+20200623090030
 20200623121135
 20200623141544
 20200623170000

ee/app/services/ee/audit_event_service.rb

@@ -214,17 +214,9 @@ module EE
 
     override :base_payload
     def base_payload
-      payload = {
-        author_id: @author.id,
-        # `@author.respond_to?(:id)` is to support cases where we need to log events
-        # that could take place even when a user is unathenticated, Eg: downloading a public repo.
-        # For such events, it is not mandatory that an author is always present.
-        entity_id: @entity.id,
-        entity_type: @entity.class.name
-      }
-
-      payload[:ip_address] = ip_address if admin_audit_log_enabled?
-      payload
+      super.tap do |payload|
+        payload[:ip_address] = ip_address if admin_audit_log_enabled?
+      end
     end
 
     def for_custom_model(model, key_title)

ee/spec/factories/audit_events.rb

@@ -9,6 +9,7 @@ FactoryBot.define do
     entity_type { 'User' }
     entity_id   { target_user.id }
     ip_address { IPAddr.new '127.0.0.1' }
+    author_name { 'Jane Doe' }
     details do
       {
         change: 'email address',

ee/spec/models/audit_event_spec.rb

@@ -64,12 +64,18 @@ RSpec.describe AuditEvent, type: :model do
     end
 
     context 'when user does not exist anymore' do
-      subject(:event) { described_class.new(author_id: non_existing_record_id) }
+      context 'when database contains author_name' do
+        subject(:event) { described_class.new(author_id: non_existing_record_id, author_name: 'Jane Doe') }
 
-      context 'when details contains author_name' do
         it 'returns author_name' do
-          subject.details = { author_name: 'John Doe' }
+          expect(event.author_name).to eq 'Jane Doe'
+        end
+      end
 
+      context 'when details contains author_name' do
+        subject(:event) { described_class.new(author_id: non_existing_record_id, details: { author_name: 'John Doe' }) }
+
+        it 'returns author_name' do
           expect(event.author_name).to eq 'John Doe'
         end
       end

ee/spec/presenters/audit_event_presenter_spec.rb

@@ -61,9 +61,19 @@ RSpec.describe AuditEventPresenter do
         end
       end
 
-      context 'when `author_name` is included in the details' do
+      context 'when author_name is set in the author_name column' do
+        it 'shows the author name as provided in the database column' do
+          expect(presenter.author_name).to eq('Jane Doe')
+        end
+      end
+
+      context 'when `author_name` is included in the details and not in the author_name column' do
+        before do
+          audit_event.update!(author_name: nil)
+        end
+
         it 'shows the author name as provided in the details' do
-          expect(presenter.author_name).to eq('author')
+          expect(presenter.author_name).to eq(details[:author_name])
         end
       end
     end
@@ -73,7 +83,11 @@ RSpec.describe AuditEventPresenter do
         audit_event.author_id = -1
       end
 
-      context 'when `author_name` is not included in details' do
+      context 'when `author_name` is not included in details and not in the author_name column' do
+        before do
+          audit_event.update!(author_name: nil)
+        end
+
         let(:details) do
           {
             author_name: nil,
@@ -90,7 +104,11 @@ RSpec.describe AuditEventPresenter do
         end
       end
 
-      context 'when `author_name` is included in details' do
+      context 'when `author_name` is included in details and not in the author_name column' do
+        before do
+          audit_event.update!(author_name: nil)
+        end
+
         it 'shows the author name as provided in the details' do
           expect(presenter.author_name).to eq('author')
         end

ee/spec/services/ee/audit_events/impersonation_audit_event_service_spec.rb

@@ -17,6 +17,7 @@ RSpec.describe EE::AuditEvents::ImpersonationAuditEventService do
     it 'creates an event and logs to a file with the provided details' do
       expect(service).to receive(:file_logger).and_return(logger)
       expect(logger).to receive(:info).with(author_id: impersonator.id,
+                                            author_name: impersonator.name,
                                             entity_id: impersonator.id,
                                             entity_type: "User",
                                             action: :custom,

ee/spec/services/ee/audit_events/repository_push_audit_event_service_spec.rb

@@ -20,6 +20,7 @@ RSpec.describe EE::AuditEvents::RepositoryPushAuditEventService do
     let(:attrs) do
       {
         author_id: user.id,
+        author_name: user.name,
         entity_id: entity.id,
         entity_type: entity_type,
         type: 'SecurityEvent',

ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb

@@ -50,6 +50,7 @@ RSpec.shared_examples 'logs the custom audit event' do
   it 'creates an event and logs to a file with the provided details' do
     expect(service).to receive(:file_logger).and_return(logger)
     expect(logger).to receive(:info).with(author_id: user.id,
+                                          author_name: user.name,
                                           entity_id: entity.id,
                                           entity_type: entity_type,
                                           action: :custom,
@@ -87,6 +88,7 @@ RSpec.shared_examples 'logs the release audit event' do
   it 'logs the event to file' do
     expect(service).to receive(:file_logger).and_return(logger)
     expect(logger).to receive(:info).with(author_id: user.id,
+                                          author_name: user.name,
                                           entity_id: entity.id,
                                           entity_type: entity_type,
                                           ip_address: ip_address,

spec/services/audit_event_service_spec.rb

@@ -17,6 +17,7 @@ RSpec.describe AuditEventService do
     it 'creates an event and logs to a file' do
       expect(service).to receive(:file_logger).and_return(logger)
       expect(logger).to receive(:info).with(author_id: user.id,
+                                            author_name: user.name,
                                             entity_id: project.id,
                                             entity_type: "Project",
                                             action: :destroy)
@@ -35,6 +36,7 @@ RSpec.describe AuditEventService do
         })
       expect(service).to receive(:file_logger).and_return(logger)
       expect(logger).to receive(:info).with(author_id: user.id,
+                                            author_name: user.name,
                                             entity_type: 'Project',
                                             entity_id: project.id,
                                             from: 'true',
@@ -56,6 +58,7 @@ RSpec.describe AuditEventService do
     it 'logs security event to file' do
       expect(service).to receive(:file_logger).and_return(logger)
       expect(logger).to receive(:info).with(author_id: user.id,
+                                            author_name: user.name,
                                             entity_type: 'Project',
                                             entity_id: project.id,
                                             action: :destroy)