Merge branch '298783-log-user-instance-request-in-audit-events-2' into 'master'

Log user instance request and rejection in Audit Events

See merge request gitlab-org/gitlab!52543

app/controllers/registrations_controller.rb

@@ -31,6 +31,8 @@ class RegistrationsController < Devise::RegistrationsController
         NotificationService.new.new_instance_access_request(new_user)
       end
 
+      after_request_hook(new_user)
+
       yield new_user if block_given?
     end
 
@@ -85,6 +87,10 @@ class RegistrationsController < Devise::RegistrationsController
     super
   end
 
+  def after_request_hook(user)
+    # overridden by EE module
+  end
+
   def after_sign_up_path_for(user)
     Gitlab::AppLogger.info(user_created_message(confirmed: user.confirmed?))
 

app/services/users/reject_service.rb

@@ -12,6 +12,8 @@ module Users
 
       user.delete_async(deleted_by: current_user, params: { hard_delete: true })
 
+      after_reject_hook(user)
+
       NotificationService.new.user_admin_rejection(user.name, user.email)
 
       success
@@ -24,5 +26,11 @@ module Users
     def allowed?
       can?(current_user, :reject_user)
     end
+
+    def after_reject_hook(user)
+      # overridden by EE module
+    end
   end
 end
+
+Users::RejectService.prepend_if_ee('EE::Users::RejectService')

doc/administration/audit_events.md

@@ -99,7 +99,6 @@ From there, you can see the following actions:
 - Number of required approvals was updated ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/7531) in GitLab 12.9)
 - Added or removed users and groups from project approval groups ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/213603) in GitLab 13.2)
 - Project CI/CD variable added, removed, or protected status changed ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/30857) in GitLab 13.4)
-- User was approved via Admin Area ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/276250) in GitLab 13.6)
 - Project access token was successfully created or revoked ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/230007) in GitLab 13.9)
 - Failed attempt to create or revoke a project access token ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/230007) in GitLab 13.9)
 
@@ -131,6 +130,9 @@ recorded:
 - Changed username ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/7797) in GitLab 12.8)
 - User was deleted ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/251) in GitLab 12.8)
 - User was added ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/251) in GitLab 12.8)
+- User requests access to an instance ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/298783) in GitLab 13.9)
+- User was approved via Admin Area ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/276250) in GitLab 13.6)
+- User was rejected via Admin Area ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/298783) in GitLab 13.9)
 - User was blocked via Admin Area ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/251) in GitLab 12.8)
 - User was blocked via API ([introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/25872) in GitLab 12.9)
 - Failed second-factor authentication attempt ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/16826) in GitLab 13.5)

ee/app/controllers/ee/registrations_controller.rb

@@ -11,6 +11,13 @@ module EE
 
     private
 
+    override :after_request_hook
+    def after_request_hook(user)
+      super
+
+      log_audit_event(user)
+    end
+
     override :set_blocked_pending_approval?
     def set_blocked_pending_approval?
       super || ::Gitlab::CurrentSettings.should_apply_user_signup_cap?
@@ -23,5 +30,14 @@ module EE
                     alert: s_('Profiles|Account could not be deleted. GitLab was unable to verify your identity.')
       end
     end
+
+    def log_audit_event(user)
+      ::AuditEventService.new(
+        user,
+        user,
+        action: :custom,
+        custom_message: _('Instance access request')
+      ).for_user.security_event
+    end
   end
 end

ee/app/services/ee/users/approve_service.rb

@@ -5,6 +5,8 @@ module EE
     module ApproveService
       extend ::Gitlab::Utils::Override
 
+      private
+
       override :after_approve_hook
       def after_approve_hook(user)
         super
@@ -12,14 +14,12 @@ module EE
         log_audit_event(user)
       end
 
-      private
-
       def log_audit_event(user)
         ::AuditEventService.new(
           current_user,
           user,
           action: :custom,
-          custom_message: 'Approved user'
+          custom_message: _('Instance access request approved')
         ).for_user.security_event
       end
     end

ee/app/services/ee/users/reject_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Users
+    module RejectService
+      extend ::Gitlab::Utils::Override
+
+      private
+
+      override :after_reject_hook
+      def after_reject_hook(user)
+        super
+
+        log_audit_event(user)
+      end
+
+      def log_audit_event(user)
+        ::AuditEventService.new(
+          current_user,
+          user,
+          action: :custom,
+          custom_message: _('Instance access request rejected')
+        ).for_user.security_event
+      end
+    end
+  end
+end

ee/changelogs/unreleased/298783-log-user-instance-request-in-audit-events-2.yml

+---
+title: Log user instance request and rejection in Audit Events
+merge_request: 52543
+author:
+type: added

ee/spec/controllers/ee/registrations_controller_spec.rb

@@ -125,6 +125,31 @@ RSpec.describe RegistrationsController do
         it_behaves_like 'blocked user by default'
       end
     end
+
+    context 'audit events' do
+      context 'when licensed' do
+        before do
+          stub_licensed_features(admin_audit_log: true)
+        end
+
+        context 'when user registers for the instance' do
+          it 'logs an audit event' do
+            expect { subject }.to change { AuditEvent.count }.by(1)
+          end
+
+          it 'logs the audit event info', :aggregate_failures do
+            subject
+
+            created_user = User.find_by(email: new_user_email)
+            audit_event = AuditEvent.where(author_id: created_user.id).last
+
+            expect(audit_event.ip_address).to eq(created_user.current_sign_in_ip)
+            expect(audit_event.details[:target_details]).to eq(created_user.username)
+            expect(audit_event.details[:custom_message]).to eq('Instance access request')
+          end
+        end
+      end
+    end
   end
 
   describe '#destroy' do

ee/spec/services/ee/users/approve_service_spec.rb

@@ -23,12 +23,14 @@ RSpec.describe Users::ApproveService do
             expect { operation }.to change { AuditEvent.count }.by(1)
           end
 
-          it 'logs the audit event info' do
+          it 'logs the audit event info', :aggregate_failures do
             operation
 
-            expect(AuditEvent.last).to have_attributes(
-              details: hash_including(custom_message: 'Approved user')
-            )
+            audit_event = AuditEvent.where(author_id: current_user.id).last
+
+            expect(audit_event.ip_address).to eq(current_user.current_sign_in_ip)
+            expect(audit_event.details[:target_details]).to eq(user.username)
+            expect(audit_event.details[:custom_message]).to eq('Instance access request approved')
           end
         end
 
@@ -42,18 +44,6 @@ RSpec.describe Users::ApproveService do
           end
         end
       end
-
-      context 'when not licensed' do
-        before do
-          stub_licensed_features(
-            admin_audit_log: false
-          )
-        end
-
-        it 'does not log any audit event' do
-          expect { operation }.not_to change(AuditEvent, :count)
-        end
-      end
     end
   end
 end

ee/spec/services/ee/users/reject_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Users::RejectService do
+  let_it_be(:current_user) { create(:admin) }
+
+  describe '#execute', :enable_admin_mode do
+    let_it_be_with_reload(:user) { create(:user, :blocked_pending_approval) }
+
+    subject(:reject_user) { Users::RejectService.new(current_user).execute(user) }
+
+    context 'audit events' do
+      context 'when licensed' do
+        before do
+          stub_licensed_features(admin_audit_log: true)
+        end
+
+        context 'when user is successfully rejected' do
+          it 'logs an audit event' do
+            expect { reject_user }.to change { AuditEvent.count }.by(1)
+          end
+
+          it 'logs the audit event info', :aggregate_failures do
+            reject_user
+            audit_event = AuditEvent.where(author_id: current_user.id).last
+
+            expect(audit_event.ip_address).to eq(current_user.current_sign_in_ip)
+            expect(audit_event.details[:target_details]).to eq(user.username)
+            expect(audit_event.details[:custom_message]).to eq('Instance access request rejected')
+          end
+        end
+
+        context 'when user does not have permission to reject another user' do
+          let(:current_user) { create(:user) }
+
+          it 'does not log any audit event' do
+            expect { reject_user }.not_to change { AuditEvent.count }
+          end
+        end
+      end
+    end
+  end
+end

locale/gitlab.pot

@@ -15269,6 +15269,15 @@ msgstr[1] ""
 msgid "Instance Configuration"
 msgstr ""
 
+msgid "Instance access request"
+msgstr ""
+
+msgid "Instance access request approved"
+msgstr ""
+
+msgid "Instance access request rejected"
+msgstr ""
+
 msgid "Instance administrators group already exists"
 msgstr ""
 

spec/controllers/registrations_controller_spec.rb

@@ -73,6 +73,18 @@ RSpec.describe RegistrationsController do
             end
           end
         end
+
+        context 'audit events' do
+          context 'when not licensed' do
+            before do
+              stub_licensed_features(admin_audit_log: false)
+            end
+
+            it 'does not log any audit event' do
+              expect { subject }.not_to change(AuditEvent, :count)
+            end
+          end
+        end
       end
 
       context 'when the `require_admin_approval_after_user_signup` setting is turned off' do

spec/services/users/approve_service_spec.rb

@@ -82,6 +82,20 @@ RSpec.describe Users::ApproveService do
                 .not_to have_enqueued_mail(DeviseMailer, :confirmation_instructions)
             end
           end
+
+          context 'audit events' do
+            context 'when not licensed' do
+              before do
+                stub_licensed_features(
+                  admin_audit_log: false
+                )
+              end
+
+              it 'does not log any audit event' do
+                expect { subject }.not_to change(AuditEvent, :count)
+              end
+            end
+          end
         end
 
         context 'pending invitations' do

spec/services/users/reject_service_spec.rb

@@ -50,5 +50,17 @@ RSpec.describe Users::RejectService do
         end
       end
     end
+
+    context 'audit events' do
+      context 'when not licensed' do
+        before do
+          stub_licensed_features(admin_audit_log: false)
+        end
+
+        it 'does not log any audit event' do
+          expect { subject }.not_to change(AuditEvent, :count)
+        end
+      end
+    end
   end
 end