Merge branch '230430-truncate-entity-path-on-audit-events' into 'master'

Gracefully truncate entity_path in audit_events

See merge request gitlab-org/gitlab!38155

ee/app/models/ee/audit_event.rb

@@ -5,14 +5,15 @@ module EE
     extend ActiveSupport::Concern
     extend ::Gitlab::Utils::Override
 
-    TEXT_LIMIT = {
+    TRUNCATED_FIELDS = {
+      entity_path: 5_500,
       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
+      before_validation :truncate_fields
     end
 
     def entity
@@ -43,8 +44,13 @@ module EE
 
     private
 
-    def truncate_target_details
-      self.target_details = self.details[:target_details] = target_details&.truncate(TEXT_LIMIT[:target_details])
+    def truncate_fields
+      TRUNCATED_FIELDS.each do |name, limit|
+        original = self[name] || self.details[name]
+        next unless original
+
+        self[name] = self.details[name] = String(original).truncate(limit)
+      end
     end
   end
 end

ee/spec/models/audit_event_spec.rb

@@ -18,7 +18,7 @@ RSpec.describe AuditEvent, type: :model 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) }
+      let_it_be(:event) { create(:project_audit_event, details: details, entity_path: nil, target_details: nil) }
 
       it 'sets author_name' do
         expect(event[:author_name]).to eq('Kungfu Panda')
@@ -33,30 +33,66 @@ RSpec.describe AuditEvent, type: :model do
       end
     end
 
-    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)
+    context 'truncate_fields' do
+      shared_examples 'a truncated field' do
+        context 'when values are provided' do
+          using RSpec::Parameterized::TableSyntax
+
+          where(:database_column, :details_value, :expected_value) do
+            :long  | nil    | :truncated
+            :short | nil    | :short
+            nil    | :long  | :truncated
+            nil    | :short | :short
+            :long  | :short | :truncated
+          end
+
+          with_them do
+            let(:values) do
+              {
+                long: 'a' * (field_limit + 1),
+                short: 'a' * field_limit,
+                truncated: 'a' * (field_limit - 3) + '...'
+              }
+            end
+
+            let(:audit_event) do
+              create(:audit_event,
+                field_name => values[database_column],
+                details: { field_name => values[details_value] }
+              )
+            end
+
+            it 'sets both values to be the same', :aggregate_failures do
+              expect(audit_event.send(field_name)).to eq(values[expected_value])
+              expect(audit_event.details[field_name]).to eq(values[expected_value])
+            end
+          end
         end
+
+        context 'when values are not provided' do
+          let(:audit_event) do
+            create(:audit_event, field_name => nil, details: {})
+          end
+
+          it 'does not set', :aggregate_failures do
+            expect(audit_event.send(field_name)).to be_nil
+            expect(audit_event.details).not_to have_key(field_name)
+          end
+        end
+      end
+
+      context 'entity_path' do
+        let(:field_name) { :entity_path }
+        let(:field_limit) { 5_500 }
+
+        it_behaves_like 'a truncated field'
+      end
+
+      context 'target_details' do
+        let(:field_name) { :target_details }
+        let(:field_limit) { 5_500 }
+
+        it_behaves_like 'a truncated field'
       end
     end
   end

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

@@ -10,7 +10,6 @@ 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,7 +7,6 @@ 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
@@ -30,8 +29,7 @@ RSpec.describe EE::AuditEvents::ImpersonationAuditEventService do
 
       expect(security_event.details).to eq(custom_message: message,
                                            ip_address: ip_address,
-                                           action: :custom,
-                                           target_details: target_details)
+                                           action: :custom)
       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,7 +10,6 @@ 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,7 +62,6 @@ 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)