Commit f07e5f2a authored by Tan Le's avatar Tan Le Committed by Dmytro Zaporozhets

Do not audit changes trigger by creation

The model can be either updated/created via Rails `update` API. We want
to distinguish between these cases for auditing purposes. It's incorrect
to show `Change name from <blank> to Y` when user creates new record.
parent 3e2aec80
---
title: Exclude creation event from audit changes
merge_request: 26140
author:
type: fixed
...@@ -3,6 +3,17 @@ ...@@ -3,6 +3,17 @@
module EE module EE
module Audit module Audit
module Changes module Changes
# Records an audit event in DB for model changes
#
# @param [Symbol] column column name to be audited
# @param [Hash] options the options to create an event with
# @option options [Symbol] :column column name to be audited
# @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
#
# @return [SecurityEvent, nil] the resulting object or nil if there is no
# change detected
def audit_changes(column, options = {}) def audit_changes(column, options = {})
column = options[:column] || column column = options[:column] || column
# rubocop:disable Gitlab/ModuleWithInstanceVariables # rubocop:disable Gitlab/ModuleWithInstanceVariables
...@@ -10,7 +21,7 @@ module EE ...@@ -10,7 +21,7 @@ module EE
@model = options[:model] @model = options[:model]
# rubocop:enable Gitlab/ModuleWithInstanceVariables # rubocop:enable Gitlab/ModuleWithInstanceVariables
return unless changed?(column) return unless audit_required?(column)
audit_event(parse_options(column, options)) audit_event(parse_options(column, options))
end end
...@@ -27,6 +38,14 @@ module EE ...@@ -27,6 +38,14 @@ module EE
private private
def audit_required?(column)
not_recently_created? && changed?(column)
end
def not_recently_created?
!model.previous_changes.has_key?(:id)
end
def changed?(column) def changed?(column)
model.previous_changes.has_key?(column) model.previous_changes.has_key?(column)
end end
......
...@@ -3,32 +3,78 @@ ...@@ -3,32 +3,78 @@
require 'spec_helper' require 'spec_helper'
describe EE::Audit::Changes do describe EE::Audit::Changes do
subject(:foo_instance) { Class.new { include EE::Audit::Changes }.new }
describe '.audit_changes' do describe '.audit_changes' do
let(:user) { create(:user) } let(:current_user) { create(:user, name: 'Mickey Mouse') }
let(:foo_instance) { Class.new { include EE::Audit::Changes }.new } let(:user) { create(:user, name: 'Donald Duck') }
let(:options) { { model: user } }
subject(:audit!) { foo_instance.audit_changes(:name, options) }
before do before do
stub_licensed_features(extended_audit_events: true) stub_licensed_features(extended_audit_events: true)
foo_instance.instance_variable_set(:@current_user, user) foo_instance.instance_variable_set(:@current_user, current_user)
foo_instance.instance_variable_set(:@user, user)
allow(foo_instance).to receive(:model).and_return(user)
end end
describe 'non audit changes' do describe 'non audit changes' do
it 'does not call the audit event service' do context 'when audited column is not changed' do
user.update!(name: 'new name') it 'does not call the audit event service' do
user.update!(email: 'scrooge.mcduck@gitlab.com')
expect { audit! }.not_to change { SecurityEvent.count }
end
end
context 'when model is newly created' do
let(:user) { build(:user) }
expect { foo_instance.audit_changes(:email) }.not_to change { SecurityEvent.count } it 'does not call the audit event service' do
user.update!(name: 'Scrooge McDuck')
expect { audit! }.not_to change { SecurityEvent.count }
end
end end
end end
describe 'audit changes' do describe 'audit changes' do
let(:audit_event_service) { instance_spy(AuditEventService) }
before do
allow(AuditEventService).to receive(:new).and_return(audit_event_service)
end
it 'calls the audit event service' do it 'calls the audit event service' do
user.update!(name: 'new name') user.update!(name: 'Scrooge McDuck')
audit!
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
end
context 'when target_model is provided' do
let(:project) { Project.new }
let(:options) { { model: user, target_model: project } }
it 'instantiates audit event service with the given target_model' do
user.update!(name: 'Scrooge McDuck')
audit!
expect { foo_instance.audit_changes(:name) }.to change { SecurityEvent.count }.by(1) expect(AuditEventService).to have_received(:new)
.with(anything, project, anything)
end
end end
end end
end end
......
...@@ -34,79 +34,25 @@ describe Users::DestroyService do ...@@ -34,79 +34,25 @@ describe Users::DestroyService do
end end
describe 'audit events' do describe 'audit events' do
context 'when licensed' do include_examples 'audit event logging' do
before do let(:fail_condition!) do
stub_licensed_features(admin_audit_log: true) expect_any_instance_of(User)
.to receive(:destroy).and_return(false)
end end
context 'soft delete' do let(:attributes) do
let(:hard_delete) { false } {
author_id: current_user.id,
context 'when user destroy operation succeeds' do entity_id: @resource.id,
it 'logs audit events for ghost user migration and destroy operation' do entity_type: 'User',
service.execute(user, hard_delete: hard_delete) details: {
remove: 'user',
expect(AuditEvent.last(3)).to contain_exactly( author_name: current_user.name,
have_attributes(details: hash_including(change: 'email address')), target_id: @resource.full_path,
have_attributes(details: hash_including(change: 'username')), target_type: 'User',
have_attributes(details: hash_including(remove: 'user')) target_details: @resource.full_path
) }
end }
end
context 'when user destroy operation fails' do
before do
allow(user).to receive(:destroy).and_return(false)
end
it 'logs audit events for ghost user migration operation' do
service.execute(user, hard_delete: hard_delete)
expect(AuditEvent.last(2)).to contain_exactly(
have_attributes(details: hash_including(change: 'email address')),
have_attributes(details: hash_including(change: 'username'))
)
end
end
end
context 'hard delete' do
let(:hard_delete) { true }
context 'when user destroy operation succeeds' do
it 'logs audit events for destroy operation' do
service.execute(user, hard_delete: hard_delete)
expect(AuditEvent.last)
.to have_attributes(details: hash_including(remove: 'user'))
end
end
context 'when user destroy operation fails' do
before do
allow(user).to receive(:destroy).and_return(false)
end
it 'does not log any audit event' do
expect { service.execute(user, hard_delete: hard_delete) }
.not_to change { AuditEvent.count }
end
end
end
end
context 'when not licensed' do
before do
stub_licensed_features(
admin_audit_log: false,
audit_events: false,
extended_audit_events: false
)
end
it 'does not log any audit event' do
expect { service.execute(user) }
.not_to change { AuditEvent.count }
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment