Merge branch '201722-exlude-creation-from-audit-changes' into 'master'

Exclude creation event from audit changes

See merge request gitlab-org/gitlab!26140

ee/changelogs/unreleased/201722-exlude-creation-from-audit-changes.yml

+---
+title: Exclude creation event from audit changes
+merge_request: 26140
+author:
+type: fixed

ee/lib/ee/audit/changes.rb

@@ -3,6 +3,17 @@
 module EE
   module Audit
     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 = {})
         column = options[:column] || column
         # rubocop:disable Gitlab/ModuleWithInstanceVariables
@@ -10,7 +21,7 @@ module EE
         @model = options[:model]
         # rubocop:enable Gitlab/ModuleWithInstanceVariables
 
-        return unless changed?(column)
+        return unless audit_required?(column)
 
         audit_event(parse_options(column, options))
       end
@@ -27,6 +38,14 @@ module EE
 
       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)
         model.previous_changes.has_key?(column)
       end

ee/spec/lib/ee/audit/changes_spec.rb

@@ -3,32 +3,78 @@
 require 'spec_helper'
 
 describe EE::Audit::Changes do
+  subject(:foo_instance) { Class.new { include EE::Audit::Changes }.new }
+
   describe '.audit_changes' do
-    let(:user) { create(:user) }
-    let(:foo_instance) { Class.new { include EE::Audit::Changes }.new }
+    let(:current_user) { create(:user, name: 'Mickey Mouse') }
+    let(:user) { create(:user, name: 'Donald Duck') }
+    let(:options) { { model: user } }
+
+    subject(:audit!) { foo_instance.audit_changes(:name, options) }
 
     before do
       stub_licensed_features(extended_audit_events: true)
 
-      foo_instance.instance_variable_set(:@current_user, user)
-      foo_instance.instance_variable_set(:@user, user)
-
-      allow(foo_instance).to receive(:model).and_return(user)
+      foo_instance.instance_variable_set(:@current_user, current_user)
     end
 
     describe 'non audit changes' do
-      it 'does not call the audit event service' do
-        user.update!(name: 'new name')
+      context 'when audited column is not changed' do
+        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
 
     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
-        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

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

@@ -34,79 +34,25 @@ describe Users::DestroyService do
     end
 
     describe 'audit events' do
-      context 'when licensed' do
-        before do
-          stub_licensed_features(admin_audit_log: true)
+      include_examples 'audit event logging' do
+        let(:fail_condition!) do
+          expect_any_instance_of(User)
+            .to receive(:destroy).and_return(false)
         end
 
-        context 'soft delete' do
-          let(:hard_delete) { false }
-
-          context 'when user destroy operation succeeds' do
-            it 'logs audit events for ghost user migration and destroy operation' do
-              service.execute(user, hard_delete: hard_delete)
-
-              expect(AuditEvent.last(3)).to contain_exactly(
-                have_attributes(details: hash_including(change: 'email address')),
-                have_attributes(details: hash_including(change: 'username')),
-                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 '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 }
+        let(:attributes) do
+          {
+            author_id: current_user.id,
+            entity_id: @resource.id,
+            entity_type: 'User',
+            details: {
+              remove: 'user',
+              author_name: current_user.name,
+              target_id: @resource.full_path,
+              target_type: 'User',
+              target_details: @resource.full_path
+            }
+          }
         end
       end
     end