From 6061ca7e589b1625ef455a3686643039b79f102e Mon Sep 17 00:00:00 2001
From: Harsimar Sandhu <hsandhu@gitlab.com>
Date: Mon, 4 Jul 2022 11:38:19 +0000
Subject: [PATCH] Use auditor when auditing using audit changes
EE: true
Changelog: changed
---
ee/app/services/audit_events/build_service.rb | 9 +--
ee/lib/audit/changes.rb | 39 ++++++++++++-
ee/lib/gitlab/audit/auditor.rb | 4 +-
ee/spec/lib/audit/changes_spec.rb | 58 ++++++++++---------
...protected_branches_changes_auditor_spec.rb | 6 +-
ee/spec/lib/gitlab/audit/auditor_spec.rb | 37 ++++++++++++
.../services/groups/update_service_spec.rb | 3 +-
.../projects/transfer_service_spec.rb | 3 +-
.../services/projects/update_service_spec.rb | 9 ++-
9 files changed, 125 insertions(+), 43 deletions(-)
diff --git a/ee/app/services/audit_events/build_service.rb b/ee/app/services/audit_events/build_service.rb
index fc89c9d2b9f9b..c3ac3584cc208 100644
--- a/ee/app/services/audit_events/build_service.rb
+++ b/ee/app/services/audit_events/build_service.rb
@@ -10,7 +10,7 @@ class BuildService
# @return [BuildService]
def initialize(
author:, scope:, target:, message:,
- created_at: DateTime.current, additional_details: {}, ip_address: nil)
+ created_at: DateTime.current, additional_details: {}, ip_address: nil, target_details: nil)
raise MissingAttributeError if missing_attribute?(author, scope, target, message)
@author = build_author(author)
@@ -20,6 +20,7 @@ def initialize(
@message = build_message(message)
@created_at = created_at
@additional_details = additional_details
+ @target_details = target_details
end
# Create an instance of AuditEvent
@@ -61,13 +62,13 @@ def base_payload
end
def base_details_payload
- {
+ @additional_details.merge({
author_name: @author.name,
target_id: @target.id,
target_type: @target.type,
- target_details: @target.details,
+ target_details: @target_details || @target.details,
custom_message: @message
- }.merge(@additional_details)
+ })
end
def build_author(author)
diff --git a/ee/lib/audit/changes.rb b/ee/lib/audit/changes.rb
index fc6ff2937e9a0..1fcb44034b2a0 100644
--- a/ee/lib/audit/changes.rb
+++ b/ee/lib/audit/changes.rb
@@ -10,7 +10,7 @@ module Changes
# @option options [User, Project, Group] :target_model scope the event belongs to
# @option options [Object] :model object being audited
# @option options [Boolean] :skip_changes whether to record from/to values
- #
+ # @option options [String] :event_type adds event type in streaming audit event headers and payload
# @return [AuditEvent, nil] the resulting object or nil if there is no
# change detected
def audit_changes(column, options = {})
@@ -66,8 +66,41 @@ def parse_options(column, options)
end
def audit_event(options)
- ::AuditEventService.new(@current_user, entity, options) # rubocop:disable Gitlab/ModuleWithInstanceVariables
- .for_changes(model).security_event
+ return unless audit_enabled?
+
+ name = options.fetch(:event_type, 'audit_operation')
+ details = additional_details(options)
+ audit_context = {
+ name: name,
+ author: @current_user, # rubocop:disable Gitlab/ModuleWithInstanceVariables
+ scope: entity,
+ target: model,
+ message: build_message(details),
+ additional_details: details,
+ target_details: options[:target_details]
+ }
+
+ ::Gitlab::Audit::Auditor.audit(audit_context)
+ end
+
+ def additional_details(options)
+ { change: options[:as] || options[:column] }.merge(options.slice(:from, :to, :target_details))
+ end
+
+ def build_message(details)
+ message = ["Changed #{details[:change]}"]
+ message << "from #{details[:from]}" unless details[:from].blank?
+ message << "to #{details[:to]}" unless details[:to].blank?
+ message.join(' ')
+ end
+
+ # TODO: Remove this once we implement license feature checks in Auditor.
+ # issue link: https://gitlab.com/gitlab-org/gitlab/-/issues/365441
+ def audit_enabled?
+ return true if ::License.feature_available?(:admin_audit_log)
+ return true if ::License.feature_available?(:extended_audit_events)
+
+ entity.respond_to?(:feature_available?) && entity.licensed_feature_available?(:audit_events)
end
end
end
diff --git a/ee/lib/gitlab/audit/auditor.rb b/ee/lib/gitlab/audit/auditor.rb
index b1ff59d34b633..16f43c7d6eb91 100644
--- a/ee/lib/gitlab/audit/auditor.rb
+++ b/ee/lib/gitlab/audit/auditor.rb
@@ -63,6 +63,7 @@ def initialize(context = {})
@message = @context.fetch(:message, '')
@additional_details = @context.fetch(:additional_details, {})
@ip_address = @context[:ip_address]
+ @target_details = @context[:target_details]
end
def multiple_audit
@@ -103,7 +104,8 @@ def build_event(message)
created_at: @created_at,
message: message,
additional_details: @additional_details,
- ip_address: @ip_address
+ ip_address: @ip_address,
+ target_details: @target_details
).execute
end
diff --git a/ee/spec/lib/audit/changes_spec.rb b/ee/spec/lib/audit/changes_spec.rb
index 1677ec57797df..1c679416dcb6e 100644
--- a/ee/spec/lib/audit/changes_spec.rb
+++ b/ee/spec/lib/audit/changes_spec.rb
@@ -39,42 +39,44 @@
end
describe 'audit changes' do
- let(:audit_event_service) { instance_spy(AuditEventService) }
+ let(:options) { { model: user, event_type: 'audit_operation' } }
- before do
- allow(AuditEventService).to receive(:new).and_return(audit_event_service)
- end
-
- it 'calls the audit event service' do
+ it 'calls the auditor' do
user.update!(name: 'Scrooge McDuck')
- audit!
+ expect(::Gitlab::Audit::Auditor).to receive(:audit).with(
+ { additional_details: { change: :name,
+ from: "Donald Duck",
+ to: "Scrooge McDuck" },
+ name: 'audit_operation',
+ author: current_user,
+ scope: user,
+ target: user,
+ message: "Changed name from Donald Duck to Scrooge McDuck",
+ target_details: nil }
+ )
- aggregate_failures 'audit event service interactions' do
- expect(AuditEventService).to have_received(:new)
- .with(
- current_user, user,
- model: user,
- action: :update, column: :name,
- from: 'Donald Duck', to: 'Scrooge McDuck'
- )
- expect(audit_event_service).to have_received(:for_changes)
- expect(audit_event_service).to have_received(:security_event)
- end
+ audit!
end
- context 'when entity is provided' do
- let(:project) { Project.new }
- let(:options) { { model: user, entity: project } }
-
- it 'instantiates audit event service with the given entity' do
- user.update!(name: 'Scrooge McDuck')
+ it 'creates audit event with correct attributes', :aggregate_failure do
+ user.update!(name: 'Scrooge McDuck')
- audit!
+ audit!
- expect(AuditEventService).to have_received(:new)
- .with(anything, project, anything)
- end
+ audit_event = AuditEvent.last
+
+ expect(audit_event.author_id).to eq(current_user.id)
+ expect(audit_event.entity_id).to eq(user.id)
+ expect(audit_event.entity_type).to eq(user.class.name)
+ expect(audit_event.details).to eq({ change: :name,
+ author_name: current_user.name,
+ from: "Donald Duck",
+ to: "Scrooge McDuck",
+ target_details: user.name,
+ target_id: user.id,
+ target_type: user.class.name,
+ custom_message: "Changed name from Donald Duck to Scrooge McDuck" })
end
end
end
diff --git a/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb
index 1e9fba51684dc..8580a906ec59a 100644
--- a/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb
+++ b/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb
@@ -32,7 +32,8 @@
expect { service.execute }.to change { AuditEvent.count }.by(1)
event = AuditEvent.last
- expect(event.details).to eq({ change: "#{setting}".humanize(capitalize: false),
+ change_text = setting.to_s.humanize(capitalize: false)
+ expect(event.details).to eq({ change: change_text,
author_name: author.name,
target_id: protected_branch.id,
entity_path: entity.full_path,
@@ -40,7 +41,8 @@
target_details: protected_branch.name,
from: false,
to: true,
- ip_address: ip_address })
+ ip_address: ip_address,
+ custom_message: "Changed #{change_text} to true" })
expect(event.author_id).to eq(author.id)
expect(event.entity_id).to eq(entity.id)
diff --git a/ee/spec/lib/gitlab/audit/auditor_spec.rb b/ee/spec/lib/gitlab/audit/auditor_spec.rb
index 87b0996d54a82..9657b886bffa8 100644
--- a/ee/spec/lib/gitlab/audit/auditor_spec.rb
+++ b/ee/spec/lib/gitlab/audit/auditor_spec.rb
@@ -191,6 +191,43 @@
end
end
+ context 'when overriding the target_details' do
+ target_details = "this is my target details"
+ let(:context) do
+ { name: name,
+ author: author,
+ scope: scope,
+ target: target,
+ created_at: Time.zone.now,
+ target_details: target_details }
+ end
+
+ it 'logs audit events to database' do
+ freeze_time do
+ audit!
+
+ audit_event = AuditEvent.last
+ expect(audit_event.details).to include({ target_details: target_details })
+ expect(audit_event.target_details).to eq(target_details)
+ end
+ end
+
+ it 'logs audit events to file' do
+ freeze_time do
+ expect(::Gitlab::AuditJsonLogger).to receive(:build).and_return(logger)
+
+ audit!
+
+ expect(logger).to have_received(:info).exactly(2).times.with(
+ hash_including(
+ 'details' => hash_including('target_details' => target_details),
+ 'target_details' => target_details
+ )
+ )
+ end
+ end
+ end
+
context 'when overriding the ip address' do
ip_address = '192.168.8.8'
let(:context) { { name: name, author: author, scope: scope, target: target, ip_address: ip_address } }
diff --git a/ee/spec/services/groups/update_service_spec.rb b/ee/spec/services/groups/update_service_spec.rb
index 232d3047cc010..ba7d2c97480be 100644
--- a/ee/spec/services/groups/update_service_spec.rb
+++ b/ee/spec/services/groups/update_service_spec.rb
@@ -40,7 +40,8 @@
param[:details].merge!(
change: 'visibility',
from: 'Public',
- to: 'Private'
+ to: 'Private',
+ custom_message: "Changed visibility from Public to Private"
)
end
end
diff --git a/ee/spec/services/projects/transfer_service_spec.rb b/ee/spec/services/projects/transfer_service_spec.rb
index b1482933c292a..c481d83d9cce9 100644
--- a/ee/spec/services/projects/transfer_service_spec.rb
+++ b/ee/spec/services/projects/transfer_service_spec.rb
@@ -48,7 +48,8 @@
author_name: user.name,
target_id: project.id,
target_type: 'Project',
- target_details: project.full_path
+ target_details: project.full_path,
+ custom_message: "Changed namespace from #{project.old_path_with_namespace} to #{project.full_path}"
}
}
end
diff --git a/ee/spec/services/projects/update_service_spec.rb b/ee/spec/services/projects/update_service_spec.rb
index af1eda5fa9c55..951951a596120 100644
--- a/ee/spec/services/projects/update_service_spec.rb
+++ b/ee/spec/services/projects/update_service_spec.rb
@@ -152,7 +152,8 @@
param[:details].merge!(
change: 'name',
from: old_name,
- to: project.full_name
+ to: project.full_name,
+ custom_message: "Changed name from #{old_name} to #{project.full_name}"
)
end
end
@@ -171,7 +172,8 @@
param[:details].merge!(
change: 'path',
from: project.old_path_with_namespace,
- to: project.full_path
+ to: project.full_path,
+ custom_message: "Changed path from #{project.old_path_with_namespace} to #{project.full_path}"
)
end
end
@@ -212,7 +214,8 @@
param[:details].merge!(
change: 'visibility',
from: 'Private',
- to: 'Internal'
+ to: 'Internal',
+ custom_message: "Changed visibility from Private to Internal"
)
end
end
--
GitLab