Merge branch '220325-target_details_refactor' into 'master'

Refactor target_details from AuditEvent details

See merge request gitlab-org/gitlab!36567

app/models/audit_event.rb

@@ -5,7 +5,7 @@ class AuditEvent < ApplicationRecord
   include IgnorableColumns
   include BulkInsertSafe
 
-  PARALLEL_PERSISTENCE_COLUMNS = [:author_name, :entity_path].freeze
+  PARALLEL_PERSISTENCE_COLUMNS = [:author_name, :entity_path, :target_details].freeze
 
   ignore_column :updated_at, remove_with: '13.4', remove_after: '2020-09-22'
 

ee/app/models/ee/audit_event.rb

@@ -5,8 +5,14 @@ module EE
     extend ActiveSupport::Concern
     extend ::Gitlab::Utils::Override
 
+    TEXT_LIMIT = {
+      target_details: 5_500
+    }.freeze
+
     prepended do
       scope :by_entity, -> (entity_type, entity_id) { by_entity_type(entity_type).by_entity_id(entity_id) }
+
+      before_validation :truncate_target_details
     end
 
     def entity
@@ -21,6 +27,10 @@ module EE
       AuditEventPresenter.new(self)
     end
 
+    def target_details
+      super || details[:target_details]
+    end
+
     def lazy_entity
       BatchLoader.for(entity_id)
         .batch(
@@ -30,5 +40,11 @@ module EE
           model.where(id: ids).find_each { |record| loader.call(record.id, record) }
         end
     end
+
+    private
+
+    def truncate_target_details
+      self.target_details = self.details[:target_details] = target_details&.truncate(TEXT_LIMIT[:target_details])
+    end
   end
 end

ee/app/presenters/audit_event_presenter.rb

@@ -14,7 +14,7 @@ class AuditEventPresenter < Gitlab::View::Presenter::Simple
   end
 
   def target
-    details[:target_details]
+    audit_event.target_details
   end
 
   def ip_address

ee/spec/factories/audit_events.rb

@@ -9,6 +9,7 @@ FactoryBot.define do
     entity_type { 'User' }
     entity_id   { target_user.id }
     entity_path { target_user.full_path }
+    target_details { target_user.name }
     ip_address { IPAddr.new '127.0.0.1' }
     author_name { 'Jane Doe' }
     details do
@@ -31,6 +32,7 @@ FactoryBot.define do
       entity_type { 'Project' }
       entity_id   { target_project.id }
       entity_path { target_project.full_path }
+      target_details { target_project.name }
       ip_address { IPAddr.new '127.0.0.1' }
       details do
         {
@@ -53,6 +55,7 @@ FactoryBot.define do
       entity_type { 'Group' }
       entity_id   { target_group.id }
       entity_path { target_group.full_path }
+      target_details { target_group.name }
       ip_address { IPAddr.new '127.0.0.1' }
       details do
         {

ee/spec/models/audit_event_spec.rb

@@ -14,15 +14,50 @@ RSpec.describe AuditEvent, type: :model do
   end
 
   describe 'callbacks' do
-    let_it_be(:details) { { author_name: 'Kungfu Panda', entity_path: 'gitlab-org/gitlab' } }
-    let_it_be(:event) { create(:project_audit_event, details: details) }
+    context 'parallel_persist' do
+      let_it_be(:details) do
+        { author_name: 'Kungfu Panda', entity_path: 'gitlab-org/gitlab', target_details: 'Project X' }
+      end
+      let_it_be(:event) { create(:project_audit_event, details: details, target_details: nil) }
+
+      it 'sets author_name' do
+        expect(event[:author_name]).to eq('Kungfu Panda')
+      end
 
-    it 'sets author_name' do
-      expect(event[:author_name]).to eq('Kungfu Panda')
+      it 'sets entity_path' do
+        expect(event[:entity_path]).to eq('gitlab-org/gitlab')
+      end
+
+      it 'sets target_details' do
+        expect(event[:target_details]).to eq('Project X')
+      end
     end
 
-    it 'sets entity_path' do
-      expect(event[:entity_path]).to eq('gitlab-org/gitlab')
+    describe '#truncate_target_details' do
+      where(:database_column, :details_value, :expected_value) do
+        text_limit = described_class::TEXT_LIMIT[:target_details]
+        long_value = 'a' * (text_limit + 1)
+        truncated_long_value = long_value.truncate(text_limit)
+        short_value = 'a' * text_limit
+
+        [
+          [nil, nil, nil],
+          [long_value, nil, truncated_long_value],
+          [short_value, nil, short_value],
+          [nil, long_value, truncated_long_value],
+          [nil, short_value, short_value],
+          [long_value, 'something', truncated_long_value]
+        ]
+      end
+
+      with_them do
+        let(:audit_event) { create(:audit_event, target_details: database_column, details: { target_details: details_value }) }
+
+        it 'expects both values to be the same and correct' do
+          expect(audit_event.target_details).to eq(expected_value)
+          expect(audit_event.details[:target_details]).to eq(expected_value)
+        end
+      end
     end
   end
 

ee/spec/presenters/audit_event_presenter_spec.rb

@@ -110,8 +110,10 @@ RSpec.describe AuditEventPresenter do
     end
   end
 
-  it 'exposes the target' do
-    expect(presenter.target).to eq(details[:target_details])
+  describe '#target' do
+    it 'delegates to the model object' do
+      expect(presenter.target).to equal(audit_event.target_details)
+    end
   end
 
   context 'exposes the ip address' do

ee/spec/services/ee/audit_events/custom_audit_event_service_spec.rb

@@ -10,6 +10,7 @@ RSpec.describe EE::AuditEvents::CustomAuditEventService do
       let(:entity) { create(:project) }
       let(:entity_type) { 'Project' }
       let(:custom_message) { 'Custom Event' }
+      let(:target_details) { nil }
       let(:service) { described_class.new(user, entity, ip_address, custom_message) }
     end
   end

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

@@ -7,6 +7,7 @@ RSpec.describe EE::AuditEvents::ImpersonationAuditEventService do
   let(:ip_address) { '127.0.0.1' }
   let(:message) { 'Impersonation Started' }
   let(:logger) { instance_double(Gitlab::AuditJsonLogger) }
+  let(:target_details) { nil }
   let(:service) { described_class.new(impersonator, ip_address, message) }
 
   describe '#security_event' do
@@ -28,8 +29,9 @@ RSpec.describe EE::AuditEvents::ImpersonationAuditEventService do
       security_event = SecurityEvent.last
 
       expect(security_event.details).to eq(custom_message: message,
-                                               ip_address: ip_address,
-                                               action: :custom)
+                                           ip_address: ip_address,
+                                           action: :custom,
+                                           target_details: target_details)
       expect(security_event.author_id).to eq(impersonator.id)
       expect(security_event.entity_id).to eq(impersonator.id)
       expect(security_event.entity_type).to eq('User')

ee/spec/services/ee/audit_events/repository_download_started_audit_event_service_spec.rb

@@ -10,6 +10,7 @@ RSpec.describe EE::AuditEvents::RepositoryDownloadStartedAuditEventService do
       let(:entity) { create(:project) }
       let(:entity_type) { 'Project' }
       let(:custom_message) { 'Repository Download Started' }
+      let(:target_details) { nil }
       let(:service) { described_class.new(user, entity, ip_address) }
     end
   end

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

@@ -62,6 +62,7 @@ RSpec.shared_examples 'logs the custom audit event' do
 
     expect(security_event.details).to eq(custom_message: custom_message,
                                          ip_address: ip_address,
+                                         target_details: target_details,
                                          action: :custom)
     expect(security_event.author_id).to eq(user.id)
     expect(security_event.entity_id).to eq(entity.id)