Refactor target_id to use database column

Currently, target_id which refers to the ID
of the target of an auditable event is stored
within a hash. This makes querying and filtering
expensive and difficult. It also makes type
enforcement much more complicated.

This commit adds a new column (target_id)
to audit_events and writes the value to both
the old details hash and the new database column
in parallel.

It also refactors all paths within the application
that write to this value.

app/models/audit_event.rb

@@ -5,7 +5,13 @@ class AuditEvent < ApplicationRecord
   include IgnorableColumns
   include BulkInsertSafe
 
-  PARALLEL_PERSISTENCE_COLUMNS = [:author_name, :entity_path, :target_details, :target_type].freeze
+  PARALLEL_PERSISTENCE_COLUMNS = [
+    :author_name,
+    :entity_path,
+    :target_details,
+    :target_type,
+    :target_id
+  ].freeze
 
   ignore_column :type, remove_with: '13.6', remove_after: '2020-11-22'
 

ee/app/controllers/ee/admin/users_controller.rb

@@ -28,7 +28,7 @@ module EE
 
       def log_audit_event
         EE::AuditEvents::ImpersonationAuditEventService.new(current_user, request.remote_ip, 'Started Impersonation')
-          .for_user(user.username).security_event
+          .for_user(full_path: user.username, entity_id: user.id).security_event
       end
 
       def allowed_user_params

ee/app/controllers/ee/application_controller.rb

@@ -37,7 +37,7 @@ module EE
 
     def log_audit_event
       EE::AuditEvents::ImpersonationAuditEventService.new(impersonator, request.remote_ip, 'Stopped Impersonation')
-        .for_user(current_user.username).security_event
+        .for_user(full_path: current_user.username, entity_id: current_user.id).security_event
     end
 
     def set_current_ip_address(&block)

ee/app/controllers/ee/passwords_controller.rb

@@ -16,7 +16,7 @@ module EE
                             action: :custom,
                             custom_message: 'Ask for password reset',
                             ip_address: request.remote_ip)
-          .for_user(resource_params[:email]).unauth_security_event
+          .for_user(full_path: resource_params[:email], entity_id: nil).unauth_security_event
     end
   end
 end

ee/app/services/ee/applications/create_service.rb

@@ -8,7 +8,7 @@ module EE
       override :execute
       def execute(request)
         super.tap do |application|
-          audit_event_service(request.remote_ip).for_user(application.name).security_event
+          audit_event_service(request.remote_ip).for_user(full_path: application.name, entity_id: application.id).security_event
         end
       end
 

ee/app/services/ee/audit_event_service.rb

@@ -158,8 +158,8 @@ module EE
     #   all of these incorrect usages are removed.
     #
     # @return [AuditEventService]
-    def for_user(full_path = @entity.full_path)
-      for_custom_model('user', full_path)
+    def for_user(full_path: @entity.full_path, entity_id: @entity.id)
+      for_custom_model(model: 'user', target_details: full_path, target_id: entity_id)
     end
 
     # Builds the @details attribute for project
@@ -168,7 +168,7 @@ module EE
     #
     # @return [AuditEventService]
     def for_project
-      for_custom_model('project', @entity.full_path)
+      for_custom_model(model: 'project', target_details: @entity.full_path, target_id: @entity.id)
     end
 
     # Builds the @details attribute for project variable
@@ -177,7 +177,7 @@ module EE
     #
     # @return [AuditEventService]
     def for_project_variable(project_variable_key)
-      for_custom_model('ci_variable', project_variable_key)
+      for_custom_model(model: 'ci_variable', target_details: project_variable_key, target_id: project_variable_key)
     end
 
     # Builds the @details attribute for group
@@ -186,7 +186,7 @@ module EE
     #
     # @return [AuditEventService]
     def for_group
-      for_custom_model('group', @entity.full_path)
+      for_custom_model(model: 'group', target_details: @entity.full_path, target_id: @entity.id)
     end
 
     # Builds the @details attribute for group variable
@@ -195,7 +195,7 @@ module EE
     #
     # @return [AuditEventService]
     def for_group_variable(group_variable_key)
-      for_custom_model('ci_group_variable', group_variable_key)
+      for_custom_model(model: 'ci_group_variable', target_details: group_variable_key, target_id: group_variable_key)
     end
 
     def enabled?
@@ -220,7 +220,7 @@ module EE
     def method_missing(method_sym, *arguments, &block)
       super(method_sym, *arguments, &block) unless respond_to?(method_sym)
 
-      for_custom_model(method_sym.to_s.split('for_').last, *arguments)
+      for_custom_model(model: method_sym.to_s.split('for_').last, target_details: arguments[0], target_id: arguments[1])
     end
 
     def respond_to?(method, include_private = false)
@@ -236,7 +236,7 @@ module EE
       end
     end
 
-    def for_custom_model(model, key_title)
+    def for_custom_model(model:, target_details:, target_id:)
       action = @details[:action]
       model_class = model.camelize
       custom_message = @details[:custom_message]
@@ -247,25 +247,25 @@ module EE
           {
             remove: model,
             author_name: @author.name,
-            target_id: key_title,
+            target_id: target_id,
             target_type: model_class,
-            target_details: key_title
+            target_details: target_details
           }
         when :create
           {
             add: model,
             author_name: @author.name,
-            target_id: key_title,
+            target_id: target_id,
             target_type: model_class,
-            target_details: key_title
+            target_details: target_details
           }
         when :custom
           {
             custom_message: custom_message,
             author_name: @author&.name,
-            target_id: key_title,
+            target_id: target_id,
             target_type: model_class,
-            target_details: key_title
+            target_details: target_details
           }
         end
 

ee/app/services/ee/keys/create_service.rb

@@ -10,7 +10,7 @@ module EE
       end
 
       def log_audit_event(key)
-        audit_event_service.for_user(key.title).security_event
+        audit_event_service.for_user(full_path: key.title, entity_id: key.id).security_event
       end
 
       def audit_event_service

ee/spec/controllers/passwords_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe PasswordsController do
+  describe '#create' do
+    before do
+      @request.env["devise.mapping"] = Devise.mappings[:user]
+      stub_licensed_features(extended_audit_events: true)
+    end
+
+    let(:user) { create(:user) }
+
+    subject { post :create, params: { user: { email: user.email } } }
+
+    it { expect { subject }.to change { AuditEvent.count }.by(1) }
+  end
+end

ee/spec/services/audit_event_service_spec.rb

@@ -302,10 +302,14 @@ RSpec.describe AuditEventService do
   describe '#for_user' do
     let(:author_name) { 'Administrator' }
     let(:current_user) { User.new(name: author_name) }
-    let(:target_user_full_path) { 'ejohn' }
-    let(:user) { instance_spy(User, full_path: target_user_full_path) }
+    let(:user) { create(:user) }
     let(:custom_message) { 'Some strange event has occurred' }
     let(:options) { { action: action, custom_message: custom_message } }
+    let(:event) { service.security_event }
+
+    before do
+      stub_licensed_features(extended_audit_events: true, admin_audit_log: true)
+    end
 
     subject(:service) { described_class.new(current_user, user, options).for_user }
 
@@ -316,11 +320,15 @@ RSpec.describe AuditEventService do
         expect(service.instance_variable_get(:@details)).to eq(
           remove: 'user',
           author_name: author_name,
-          target_id: target_user_full_path,
+          target_id: user.id,
           target_type: 'User',
-          target_details: target_user_full_path
+          target_details: user.username
         )
       end
+
+      it 'sets the target_id column' do
+        expect(event.target_id).to eq(user.id)
+      end
     end
 
     context 'with create action' do
@@ -330,11 +338,15 @@ RSpec.describe AuditEventService do
         expect(service.instance_variable_get(:@details)).to eq(
           add: 'user',
           author_name: author_name,
-          target_id: target_user_full_path,
+          target_id: user.id,
           target_type: 'User',
-          target_details: target_user_full_path
+          target_details: user.full_path
         )
       end
+
+      it 'sets the target_id column' do
+        expect(event.target_id).to eq(user.id)
+      end
     end
 
     context 'with custom action' do
@@ -344,11 +356,65 @@ RSpec.describe AuditEventService do
         expect(service.instance_variable_get(:@details)).to eq(
           custom_message: custom_message,
           author_name: author_name,
-          target_id: target_user_full_path,
+          target_id: user.id,
           target_type: 'User',
-          target_details: target_user_full_path
+          target_details: user.full_path
+        )
+      end
+
+      it 'sets the target_id column' do
+        expect(event.target_id).to eq(user.id)
+      end
+    end
+  end
+
+  describe '#for_project' do
+    let(:current_user) { create(:user) }
+    let(:project) { create(:project) }
+    let(:options) { { action: action } }
+
+    before do
+      stub_licensed_features(extended_audit_events: true, admin_audit_log: true)
+    end
+
+    let(:event) { service.security_event }
+
+    subject(:service) { described_class.new(current_user, project, options).for_project }
+
+    context 'with destroy action' do
+      let(:action) { :destroy }
+
+      it 'sets the details attribute' do
+        expect(service.instance_variable_get(:@details)).to eq(
+          remove: 'project',
+          author_name: current_user.name,
+          target_id: project.id,
+          target_type: 'Project',
+          target_details: project.full_path
+        )
+      end
+
+      it 'sets the target_id column' do
+        expect(event.target_id).to eq(project.id)
+      end
+    end
+
+    context 'with create action' do
+      let(:action) { :create }
+
+      it 'sets the details attribute' do
+        expect(service.instance_variable_get(:@details)).to eq(
+          add: 'project',
+          author_name: current_user.name,
+          target_id: project.id,
+          target_type: 'Project',
+          target_details: project.full_path
         )
       end
+
+      it 'sets the target_id column' do
+        expect(event.target_id).to eq(project.id)
+      end
     end
   end
 
@@ -385,7 +451,7 @@ RSpec.describe AuditEventService do
       expect(event.details).to eq(
         remove: 'project',
         author_name: 'Test User',
-        target_id: project.full_path,
+        target_id: project.id,
         target_type: 'Project',
         target_details: project.full_path
       )
@@ -409,7 +475,7 @@ RSpec.describe AuditEventService do
       expect(event.details).to eq(
         remove: 'group',
         author_name: 'Test User',
-        target_id: group.full_path,
+        target_id: group.id,
         target_type: 'Group',
         target_details: group.full_path
       )

ee/spec/services/ee/users/create_service_spec.rb

@@ -33,7 +33,7 @@ RSpec.describe Users::CreateService do
             details: {
               add: 'user',
               author_name: current_user.name,
-              target_id: @resource.full_path,
+              target_id: @resource.id,
               target_type: 'User',
               target_details: @resource.full_path
             }

ee/spec/services/ee/users/destroy_service_spec.rb

@@ -50,7 +50,7 @@ RSpec.describe Users::DestroyService do
             details: {
               remove: 'user',
               author_name: current_user.name,
-              target_id: @resource.full_path,
+              target_id: @resource.id,
               target_type: 'User',
               target_details: @resource.full_path
             }

ee/spec/services/groups/create_service_spec.rb

@@ -27,7 +27,7 @@ RSpec.describe Groups::CreateService, '#execute' do
            details: {
              add: 'group',
              author_name: user.name,
-             target_id: @resource.full_path,
+             target_id: @resource.id,
              target_type: 'Group',
              target_details: @resource.full_path
            }

ee/spec/services/groups/destroy_service_spec.rb

@@ -24,7 +24,7 @@ RSpec.describe Groups::DestroyService do
            details: {
              remove: 'group',
              author_name: user.name,
-             target_id: group.full_path,
+             target_id: group.id,
              target_type: 'Group',
              target_details: group.full_path
            }

ee/spec/services/projects/create_service_spec.rb

@@ -348,7 +348,7 @@ RSpec.describe Projects::CreateService, '#execute' do
            details: {
              add: 'project',
              author_name: user.name,
-             target_id: @resource.full_path,
+             target_id: @resource.id,
              target_type: 'Project',
              target_details: @resource.full_path
            }

ee/spec/services/projects/destroy_service_spec.rb

@@ -77,7 +77,7 @@ RSpec.describe Projects::DestroyService do
             details: {
               remove: 'project',
               author_name: user.name,
-              target_id: project.full_path,
+              target_id: project.id,
               target_type: 'Project',
               target_details: project.full_path
             }