Fix missing attributes on token audit events

The bug is caused by the column value being overwritten will nil value
in the `details` hash. The `after_validation` callback always writes the
column value using the `details` value, regardless whether it has been
specified or not.

This change ensures nil check was performed before setting values in
both column and `detail` hash. The column value takes precedence over
the `details` value.

Changelog: fixed
EE: true

app/models/audit_event.rb

@@ -88,7 +88,12 @@ class AuditEvent < ApplicationRecord
   end
 
   def parallel_persist
-    PARALLEL_PERSISTENCE_COLUMNS.each { |col| self[col] = details[col] }
+    PARALLEL_PERSISTENCE_COLUMNS.each do |name|
+      original = self[name] || self.details[name]
+      next unless original
+
+      self[name] = self.details[name] = original
+    end
   end
 end
 

ee/app/services/ee/resource_access_tokens/create_service.rb

@@ -21,6 +21,8 @@ module EE
         ::AuditEventService.new(
           current_user,
           resource,
+          target_id: token&.id,
+          target_type: token&.class&.name,
           target_details: token&.user&.name,
           action: :custom,
           custom_message: message,

ee/app/services/ee/resource_access_tokens/revoke_service.rb

@@ -13,15 +13,18 @@ module EE
 
       def audit_event_service(token, response)
         message = if response.success?
-                    "Revoked #{resource.class.name.downcase} access token with token_id: #{access_token.id}"
+                    "Revoked #{resource.class.name.downcase} access token with token_id: #{token.id}"
                   else
-                    "Attempted to revoke #{resource.class.name.downcase} access token with token_id: #{access_token.id}, but failed with message: #{response.message}"
+                    "Attempted to revoke #{resource.class.name.downcase} access token with token_id: #{token.id}, " \
+                    "but failed with message: #{response.message}"
                   end
 
         ::AuditEventService.new(
           current_user,
           resource,
-          target_details: access_token.user.name,
+          target_id: token.id,
+          target_type: token.class.name,
+          target_details: token.user.name,
           action: :custom,
           custom_message: message,
           ip_address: current_user.current_sign_in_ip

ee/spec/models/audit_event_spec.rb

@@ -14,26 +14,6 @@ RSpec.describe AuditEvent, type: :model do
   end
 
   describe 'callbacks' do
-    context 'parallel_persist' do
-      let_it_be(:details) do
-        { author_name: 'Kungfu Panda', entity_path: 'gitlab-org/gitlab', target_details: 'Project X', target_type: 'User' }
-      end
-
-      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')
-      end
-
-      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
-
     context 'truncate_fields' do
       shared_examples 'a truncated field' do
         context 'when values are provided' do

ee/spec/presenters/audit_event_presenter_spec.rb

@@ -7,17 +7,21 @@ RSpec.describe AuditEventPresenter do
 
   let(:details) do
     {
-      author_name: 'author',
-      ip_address: '127.0.0.1',
-      target_details: 'target name',
-      entity_path: 'path',
+      change: 'name',
       from: 'a',
-      to: 'b'
+      to: 'b',
+      entity_path: 'path'
     }
   end
 
   let(:audit_event) do
-    create(:audit_event, ip_address: '10.2.1.1', details: details)
+    create(
+      :audit_event,
+      author_name: 'author',
+      target_details: 'target name',
+      ip_address: '10.2.1.1',
+      details: details
+    )
   end
 
   subject(:presenter) do
@@ -36,13 +40,7 @@ RSpec.describe AuditEventPresenter do
     end
 
     context 'event authored by a user that no longer exists' do
-      let(:user) { create(:user) }
-      let(:audit_event) { create(:audit_event, user: user, details: details) }
-
-      before do
-        user.destroy!
-        audit_event.reload
-      end
+      let(:audit_event) { build(:audit_event, user: build(:user), details: details) }
 
       context 'when `author_name` is not included in the details' do
         let(:details) do
@@ -62,52 +60,13 @@ RSpec.describe AuditEventPresenter do
       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
+        let(:audit_event) { build(:audit_event, author_name: nil, details: details) }
 
         it 'shows the author name as provided in the details' do
           expect(presenter.author_name).to eq(details[:author_name])
         end
       end
     end
-
-    context 'event authored by an unauthenticated user' do
-      before do
-        audit_event.author_id = -1
-      end
-
-      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,
-            ip_address: '127.0.0.1',
-            target_details: 'target name',
-            entity_path: 'path',
-            from: 'a',
-            to: 'b'
-          }
-        end
-
-        it 'shows `An unauthenticated user` as the author name' do
-          expect(presenter.author_name).to eq('An unauthenticated user')
-        end
-      end
-
-      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
-      end
-    end
   end
 
   describe '#target' do
@@ -153,6 +112,6 @@ RSpec.describe AuditEventPresenter do
   end
 
   it 'exposes the action' do
-    expect(presenter.action).to eq('Changed author from a to b')
+    expect(presenter.action).to eq('Changed name from a to b')
   end
 end

ee/spec/services/audit_events/impersonation_audit_event_service_spec.rb

@@ -27,9 +27,12 @@ RSpec.describe AuditEvents::ImpersonationAuditEventService do
       expect { service.security_event }.to change(AuditEvent, :count).by(1)
       security_event = AuditEvent.last
 
-      expect(security_event.details).to eq(custom_message: message,
-                                           ip_address: ip_address,
-                                           action: :custom)
+      expect(security_event.details).to eq(
+        author_name: impersonator.name,
+        custom_message: message,
+        ip_address: ip_address,
+        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/resource_access_tokens/create_service_spec.rb

@@ -84,9 +84,17 @@ RSpec.describe ResourceAccessTokens::CreateService do
           response = subject
 
           audit_event = AuditEvent.where(author_id: user.id).last
-
-          expect(audit_event.details[:custom_message]).to eq("Created project access token with token_id: #{response.payload[:access_token].id} with scopes: #{response.payload[:access_token].scopes}")
-          expect(audit_event.details[:target_details]).to match(response.payload[:access_token].user.name)
+          access_token = response.payload[:access_token]
+          custom_message = <<~MESSAGE.squish
+            Created project access token with token_id: #{access_token.id} with scopes: #{access_token.scopes}
+          MESSAGE
+
+          expect(audit_event.details).to include(
+            custom_message: custom_message,
+            target_id: access_token.id,
+            target_type: access_token.class.name,
+            target_details: access_token.user.name
+          )
         end
       end
 
@@ -101,12 +109,24 @@ RSpec.describe ResourceAccessTokens::CreateService do
           it 'logs the permission error message' do
             subject
 
-            expect(AuditEvent.where(author_id: user.id).last.details[:custom_message]).to eq('Attempted to create project access token but failed with message: User does not have permission to create project access token')
+            audit_event = AuditEvent.where(author_id: user.id).last
+            custom_message = <<~MESSAGE.squish
+              Attempted to create project access token but failed with message:
+              User does not have permission to create project access token
+            MESSAGE
+
+            expect(audit_event.details).to include(
+              custom_message: custom_message,
+              target_id: nil,
+              target_type: nil,
+              target_details: nil
+            )
           end
         end
 
         context "when access provisioning fails" do
           let_it_be(:user) { create(:user) }
+
           let(:unpersisted_member) { build(:project_member, source: resource, user: user) }
 
           before do
@@ -123,7 +143,18 @@ RSpec.describe ResourceAccessTokens::CreateService do
           it 'logs the provisioning error message' do
             subject
 
-            expect(AuditEvent.where(author_id: user.id).last.details[:custom_message]).to eq('Attempted to create project access token but failed with message: Could not provision maintainer access to project access token')
+            audit_event = AuditEvent.where(author_id: user.id).last
+            custom_message = <<~MESSAGE.squish
+              Attempted to create project access token but failed with message:
+              Could not provision maintainer access to project access token
+            MESSAGE
+
+            expect(audit_event.details).to include(
+              custom_message: custom_message,
+              target_id: nil,
+              target_type: nil,
+              target_details: nil
+            )
           end
         end
       end

ee/spec/services/resource_access_tokens/revoke_service_spec.rb

@@ -7,6 +7,7 @@ RSpec.describe ResourceAccessTokens::RevokeService do
 
   let_it_be(:user) { create(:user) }
   let_it_be(:resource_bot) { create(:user, :project_bot) }
+
   let(:access_token) { create(:personal_access_token, user: resource_bot) }
 
   shared_examples 'audit event details' do
@@ -40,8 +41,12 @@ RSpec.describe ResourceAccessTokens::RevokeService do
 
         audit_event = AuditEvent.where(author_id: user.id).last
 
-        expect(audit_event.details[:custom_message]).to match(/Revoked project access token with token_id: \d+/)
-        expect(audit_event.details[:target_details]).to eq(access_token.user.name)
+        expect(audit_event.details).to include(
+          custom_message: match(/Revoked project access token with token_id: \d+/),
+          target_id: access_token.id,
+          target_type: access_token.class.name,
+          target_details: access_token.user.name
+        )
       end
     end
 
@@ -56,7 +61,18 @@ RSpec.describe ResourceAccessTokens::RevokeService do
         it 'logs the find error message' do
           subject
 
-          expect(AuditEvent.where(author_id: user.id).last.details[:custom_message]).to match(/Attempted to revoke project access token with token_id: \d+, but failed with message: Failed to find bot user/)
+          audit_event = AuditEvent.where(author_id: user.id).last
+          custom_message = <<~MESSAGE.squish
+            Attempted to revoke project access token with token_id: \\d+, but failed with message:
+            Failed to find bot user
+          MESSAGE
+
+          expect(audit_event.details).to include(
+            custom_message: match(custom_message),
+            target_id: access_token.id,
+            target_type: access_token.class.name,
+            target_details: access_token.user.name
+          )
         end
       end
 
@@ -71,7 +87,18 @@ RSpec.describe ResourceAccessTokens::RevokeService do
         it 'logs the permission error message' do
           subject
 
-          expect(AuditEvent.where(author_id: user.id).last.details[:custom_message]).to match(/Attempted to revoke project access token with token_id: \d+, but failed with message: #{user.name} cannot delete #{access_token.user.name}/)
+          audit_event = AuditEvent.where(author_id: user.id).last
+          custom_message = <<~MESSAGE.squish
+            Attempted to revoke project access token with token_id: \\d+, but failed with message:
+            #{user.name} cannot delete #{access_token.user.name}
+          MESSAGE
+
+          expect(audit_event.details).to include(
+            custom_message: match(custom_message),
+            target_id: access_token.id,
+            target_type: access_token.class.name,
+            target_details: access_token.user.name
+          )
         end
       end
     end

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

@@ -60,9 +60,12 @@ RSpec.shared_examples 'logs the custom audit event' do
     expect { service.security_event }.to change(AuditEvent, :count).by(1)
     security_event = AuditEvent.last
 
-    expect(security_event.details).to eq(custom_message: custom_message,
-                                         ip_address: ip_address,
-                                         action: :custom)
+    expect(security_event.details).to eq(
+      author_name: user.name,
+      custom_message: custom_message,
+      ip_address: ip_address,
+      action: :custom
+    )
     expect(security_event.author_id).to eq(user.id)
     expect(security_event.entity_id).to eq(entity.id)
     expect(security_event.entity_type).to eq(entity_type)
@@ -101,11 +104,14 @@ RSpec.shared_examples 'logs the release audit event' do
 
     security_event = AuditEvent.last
 
-    expect(security_event.details).to eq(custom_message: custom_message,
-                                         ip_address: ip_address,
-                                         target_details: target_details,
-                                         target_id: target_id,
-                                         target_type: target_type)
+    expect(security_event.details).to eq(
+      author_name: user.name,
+      custom_message: custom_message,
+      ip_address: ip_address,
+      target_details: target_details,
+      target_id: target_id,
+      target_type: target_type
+    )
 
     expect(security_event.author_id).to eq(user.id)
     expect(security_event.entity_id).to eq(entity.id)

spec/factories/audit_events.rb

@@ -51,6 +51,7 @@ FactoryBot.define do
 
     trait :unauthenticated do
       author_id { -1 }
+      author_name { 'An unauthenticated user' }
       details do
         {
           custom_message: 'Custom action',

spec/models/audit_event_spec.rb

@@ -3,9 +3,6 @@
 require 'spec_helper'
 
 RSpec.describe AuditEvent do
-  let_it_be(:audit_event) { create(:project_audit_event) }
-  subject { audit_event }
-
   describe 'validations' do
     include_examples 'validates IP address' do
       let(:attribute) { :ip_address }
@@ -13,6 +10,71 @@ RSpec.describe AuditEvent do
     end
   end
 
+  describe 'callbacks' do
+    describe '#parallel_persist' do
+      shared_examples 'a parallel persisted field' do
+        using RSpec::Parameterized::TableSyntax
+
+        where(:column, :details, :expected_value) do
+          :value | nil            | :value
+          nil    | :value         | :value
+          :value | :another_value | :value
+          nil    | nil            | nil
+        end
+
+        with_them do
+          let(:values) { { value: value, another_value: "#{value}88" } }
+
+          let(:audit_event) do
+            build(:audit_event, name => values[column], details: { name => values[details] })
+          end
+
+          it 'sets both values to be the same', :aggregate_failures do
+            audit_event.validate
+
+            expect(audit_event[name]).to eq(values[expected_value])
+            expect(audit_event.details[name]).to eq(values[expected_value])
+          end
+        end
+      end
+
+      context 'wih author_name' do
+        let(:name) { :author_name }
+        let(:value) { 'Mary Poppins' }
+
+        it_behaves_like 'a parallel persisted field'
+      end
+
+      context 'with entity_path' do
+        let(:name) { :entity_path }
+        let(:value) { 'gitlab-org' }
+
+        it_behaves_like 'a parallel persisted field'
+      end
+
+      context 'with target_details' do
+        let(:name) { :target_details }
+        let(:value) { 'gitlab-org/gitlab' }
+
+        it_behaves_like 'a parallel persisted field'
+      end
+
+      context 'with target_type' do
+        let(:name) { :target_type }
+        let(:value) { 'Project' }
+
+        it_behaves_like 'a parallel persisted field'
+      end
+
+      context 'with target_id' do
+        let(:name) { :target_id }
+        let(:value) { 8 }
+
+        it_behaves_like 'a parallel persisted field'
+      end
+    end
+  end
+
   describe '#as_json' do
     context 'ip_address' do
       subject { build(:group_audit_event, ip_address: '192.168.1.1').as_json }