Merge branch '220322-refactor-target-id' into 'master'

Refactor target_id on AuditEvent

See merge request gitlab-org/gitlab!39949

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
             }