Merge branch '251-record-user-hard-delete-audit-event' into 'master'

Record audit event when user is deleted

See merge request gitlab-org/gitlab!24257

doc/administration/audit_events.md

@@ -106,6 +106,7 @@ recorded:
 - Grant OAuth access
 - Started/stopped user impersonation
 - Changed username
+- User was deleted
 
 It is possible to filter particular actions by choosing an audit data type from
 the filter dropdown box. You can further filter by specific group, project or user

ee/app/services/ee/audit_event_service.rb

@@ -120,6 +120,10 @@ module EE
       )
     end
 
+    def for_user(full_path = @entity.full_path)
+      for_custom_model('user', full_path)
+    end
+
     def for_project
       for_custom_model('project', @entity.full_path)
     end

ee/app/services/ee/users/destroy_service.rb

@@ -7,9 +7,13 @@ module EE
 
       override :execute
       def execute(user, options = {})
-        super(user, options) do |delete_user|
+        result = super(user, options) do |delete_user|
           mirror_cleanup(delete_user)
         end
+
+        log_audit_event(user) if result.try(:destroyed?)
+
+        result
       end
 
       def mirror_cleanup(user)
@@ -31,6 +35,14 @@ module EE
 
         mirror_owners.first
       end
+
+      def log_audit_event(user)
+        ::AuditEventService.new(
+          current_user,
+          user,
+          action: :destroy
+        ).for_user.security_event
+      end
     end
   end
 end

ee/changelogs/unreleased/251-record-user-hard-delete-audit-event.yml

+---
+title: Record audit event when user is deleted
+merge_request: 24257
+author:
+type: added

ee/spec/services/audit_event_service_spec.rb

@@ -214,6 +214,61 @@ describe AuditEventService do
     end
   end
 
+  describe '#for_user' do
+    let(:author_name) { 'Administrator' }
+    let(:current_user) { instance_spy(User, name: author_name) }
+    let(:target_user_full_path) { 'ejohn' }
+    let(:user) { instance_spy(User, full_path: target_user_full_path) }
+    let(:custom_message) { 'Some strange event has occurred' }
+    let(:ip_address) { '127.0.0.1' }
+    let(:options) { { action: action, custom_message: custom_message, ip_address: ip_address } }
+
+    subject(:service) { described_class.new(current_user, user, options).for_user }
+
+    context 'with destroy action' do
+      let(:action) { :destroy }
+
+      it 'sets the details attribute' do
+        expect(service.instance_variable_get(:@details)).to eq(
+          remove: 'user',
+          author_name: author_name,
+          target_id: target_user_full_path,
+          target_type: 'User',
+          target_details: target_user_full_path
+        )
+      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: 'user',
+          author_name: author_name,
+          target_id: target_user_full_path,
+          target_type: 'User',
+          target_details: target_user_full_path
+        )
+      end
+    end
+
+    context 'with custom action' do
+      let(:action) { :custom }
+
+      it 'sets the details attribute' do
+        expect(service.instance_variable_get(:@details)).to eq(
+          custom_message: custom_message,
+          author_name: author_name,
+          target_id: target_user_full_path,
+          target_type: 'User',
+          target_details: target_user_full_path,
+          ip_address: ip_address
+        )
+      end
+    end
+  end
+
   describe 'license' do
     let(:project) { create(:project) }
     let(:user) { create(:user) }

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

@@ -3,6 +3,17 @@
 require 'spec_helper'
 
 describe Users::DestroyService do
+  let(:current_user) { create(:admin) }
+  let(:user) { create(:user) }
+
+  subject(:service) { described_class.new(current_user) }
+
+  it 'returns result' do
+    allow(user).to receive(:destroy).and_return(user)
+
+    expect(service.execute(user)).to eq(user)
+  end
+
   context 'when project is a mirror' do
     it 'assigns mirror_user to a project owner' do
       mirror_user = create(:user)
@@ -16,4 +27,65 @@ describe Users::DestroyService do
       end.to change { project.reload.mirror_user }.from(mirror_user).to(new_mirror_user)
     end
   end
+
+  describe 'audit events' do
+    before do
+      stub_licensed_features(admin_audit_log: true)
+    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
 end

spec/services/users/destroy_service_spec.rb

@@ -121,10 +121,17 @@ describe Users::DestroyService do
 
       before do
         solo_owned.group_members = [member]
-        service.execute(user)
+      end
+
+      it 'returns the user with attached errors' do
+        expect(service.execute(user)).to be(user)
+        expect(user.errors.full_messages).to eq([
+          'You must transfer ownership or delete groups before you can remove user'
+        ])
       end
 
       it 'does not delete the user' do
+        service.execute(user)
         expect(User.find(user.id)).to eq user
       end
     end