Merge branch '12702-no-audit-event-when-access-is-removed-due-to-expiration' into 'master'

Resolve "No Audit Event When Access is Removed Due To Expiration"

See merge request gitlab-org/gitlab!20529

ee/app/helpers/audit_events_helper.rb

@@ -10,6 +10,8 @@ module AuditEventsHelper
   def select_keys(key, value)
     if key =~ /^(author|target)_.*/
       ""
+    elsif key.to_s == 'ip_address' && value.blank?
+      ""
     else
       "#{key} <strong>#{value}</strong>"
     end

ee/app/services/ee/audit_event_service.rb

@@ -7,7 +7,6 @@ module EE
     def for_member(member)
       action = @details[:action]
       old_access_level = @details[:old_access_level]
-      author_name = @author.name
       user_id = member.id
       user_name = member.user ? member.user.name : 'Deleted User'
 
@@ -16,16 +15,26 @@ module EE
         when :destroy
           {
             remove: "user_access",
-            author_name: author_name,
+            author_name: @author.name,
             target_id: user_id,
             target_type: "User",
             target_details: user_name
           }
+        when :expired
+          {
+            remove: "user_access",
+            author_name: member.created_by ? member.created_by.name : 'Deleted User',
+            target_id: user_id,
+            target_type: "User",
+            target_details: user_name,
+            system_event: true,
+            reason: "access expired on #{member.expires_at}"
+          }
         when :create
           {
             add: "user_access",
             as: ::Gitlab::Access.options_with_owner.key(member.access_level.to_i),
-            author_name: author_name,
+            author_name: @author.name,
             target_id: user_id,
             target_type: "User",
             target_details: user_name
@@ -35,7 +44,7 @@ module EE
             change: "access_level",
             from: old_access_level,
             to: member.human_access,
-            author_name: author_name,
+            author_name: @author.name,
             target_id: user_id,
             target_type: "User",
             target_details: user_name

ee/app/services/ee/members/destroy_service.rb

@@ -6,18 +6,30 @@ module EE
       def after_execute(member:)
         super
 
-        log_audit_event(member: member)
+        if system_event? && removed_due_to_expiry?(member)
+          log_audit_event(member: member, author: nil, action: :expired)
+        else
+          log_audit_event(member: member, author: current_user, action: :destroy)
+        end
 
         cleanup_group_identity(member)
       end
 
       private
 
-      def log_audit_event(member:)
+      def removed_due_to_expiry?(member)
+        member.expired?
+      end
+
+      def system_event?
+        current_user.blank?
+      end
+
+      def log_audit_event(member:, author:, action:)
         ::AuditEventService.new(
-          current_user,
+          author,
           member.source,
-          action: :destroy
+          action: action
         ).for_member(member).security_event
       end
 

ee/changelogs/unreleased/12702-no-audit-event-when-access-is-removed-due-to-expiration.yml

+---
+title: Add audit event when member access is removed due to expiration
+merge_request: 20529
+author:
+type: added

ee/lib/audit/details.rb

@@ -15,6 +15,8 @@ module Audit
     def humanize
       if @details[:with]
         "Signed in with #{@details[:with].upcase} authentication"
+      elsif event_created_by_system?
+        "#{action_text} via system job. Reason: #{@details[:reason]}"
       else
         action_text
       end
@@ -22,6 +24,10 @@ module Audit
 
     private
 
+    def event_created_by_system?
+      @details[:system_event]
+    end
+
     def action_text
       action = @details.slice(*ACTIONS)
 

ee/spec/helpers/audit_events_helper_spec.rb

@@ -44,6 +44,14 @@ describe AuditEventsHelper do
       expect(select_keys('target_name', 'John Doe')).to eq ''
     end
 
+    it 'returns empty string if key is ip_address and the value is blank' do
+      expect(select_keys('ip_address', nil)).to eq ''
+    end
+
+    it 'returns formatted text if key is ip_address and the value is not blank' do
+      expect(select_keys('ip_address', '127.0.0.1')).to eq 'ip_address <strong>127.0.0.1</strong>'
+    end
+
     it 'returns formatted text if key does not start with author_, or target_' do
       expect(select_keys('remove', 'user_access')).to eq 'remove <strong>user_access</strong>'
     end

ee/spec/lib/audit/details_spec.rb

@@ -144,5 +144,28 @@ describe Audit::Details do
         expect(string).to eq('Updated ref master from b6bce79c to a7bce79c')
       end
     end
+
+    context 'system event' do
+      let(:user_member) { create(:user) }
+      let(:project) { create(:project) }
+      let(:member) { create(:project_member, :developer, user: user_member, project: project, expires_at: 1.day.from_now) }
+      let(:system_event_action) do
+        {
+          remove: 'user_access',
+          author_name: 'Admin User',
+          target_id: member.id,
+          target_type: 'User',
+          target_details: member.user.name,
+          system_event: true,
+          reason: "access expired on #{member.expires_at}"
+        }
+      end
+
+      it 'humanizes system event' do
+        string = described_class.humanize(system_event_action)
+
+        expect(string).to eq("Removed user access via system job. Reason: access expired on #{member.expires_at}")
+      end
+    end
   end
 end

ee/spec/services/audit_event_service_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 describe AuditEventService do
   let(:project) { create(:project) }
   let(:user) { create(:user) }
-  let(:project_member) { create(:project_member, user: user) }
+  let(:project_member) { create(:project_member, user: user, expires_at: 1.day.from_now) }
   let(:service) { described_class.new(user, project, { action: :destroy }) }
 
   describe '#for_member' do
@@ -26,6 +26,18 @@ describe AuditEventService do
       expect(event[:details][:ip_address]).to eq(user.current_sign_in_ip)
     end
 
+    context 'user access expiry' do
+      let(:service) { described_class.new(nil, project, { action: :expired }) }
+
+      it 'generates a system event' do
+        event = service.for_member(project_member).security_event
+
+        expect(event[:details][:remove]).to eq('user_access')
+        expect(event[:details][:system_event]).to be_truthy
+        expect(event[:details][:reason]).to include('access expired on')
+      end
+    end
+
     context 'admin audit log licensed' do
       before do
         stub_licensed_features(admin_audit_log: true)

ee/spec/services/ee/members/destroy_service_spec.rb

@@ -8,31 +8,75 @@ describe Members::DestroyService do
   let(:group) { create(:group) }
   let(:member) { group.members.find_by(user_id: member_user.id) }
 
-  subject { described_class.new(current_user) }
-
   before do
     group.add_owner(current_user)
     group.add_developer(member_user)
   end
 
-  context 'with group membership via Group SAML' do
-    let!(:saml_provider) { create(:saml_provider, group: group) }
+  shared_examples_for 'logs an audit event' do
+    it do
+      expect { event }.to change { SecurityEvent.count }.by(1)
+    end
+  end
+
+  context 'when current_user is present' do
+    subject { described_class.new(current_user) }
+
+    context 'with group membership via Group SAML' do
+      let!(:saml_provider) { create(:saml_provider, group: group) }
 
-    context 'with a SAML identity' do
-      before do
-        create(:group_saml_identity, user: member_user, saml_provider: saml_provider)
+      context 'with a SAML identity' do
+        before do
+          create(:group_saml_identity, user: member_user, saml_provider: saml_provider)
+        end
+
+        it 'cleans up linked SAML identity' do
+          expect { subject.execute(member, {}) }.to change { member_user.reload.identities.count }.by(-1)
+        end
       end
 
-      it 'cleans up linked SAML identity' do
-        expect { subject.execute(member, {}) }.to change { member_user.reload.identities.count }.by(-1)
+      context 'without a SAML identity' do
+        it 'does not attempt to destroy unrelated identities' do
+          create(:identity, user: member_user)
+
+          expect { subject.execute(member, {}) }.not_to change(Identity, :count)
+        end
       end
     end
 
-    context 'without a SAML identity' do
-      it 'does not attempt to destroy unrelated identities' do
-        create(:identity, user: member_user)
+    context 'audit events' do
+      it_behaves_like 'logs an audit event' do
+        let(:event) { subject.execute(member, {}) }
+      end
+
+      it 'does not log the audit event as a system event' do
+        subject.execute(member, skip_authorization: true)
+        details = AuditEvent.last.details
+
+        expect(details[:system_event]).to be_nil
+        expect(details[:reason]).to be_nil
+      end
+    end
+  end
+
+  context 'when current user is not present' do # ie, when the system initiates the destroy
+    subject { described_class.new(nil) }
+
+    context 'for members with expired access' do
+      let(:member) { create(:project_member, user: member_user, expires_at: 1.day.ago) }
+
+      context 'audit events' do
+        it_behaves_like 'logs an audit event' do
+          let(:event) { subject.execute(member, skip_authorization: true) }
+        end
+
+        it 'logs the audit event as a system event' do
+          subject.execute(member, skip_authorization: true)
+          details = AuditEvent.last.details
 
-        expect { subject.execute(member, {}) }.not_to change(Identity, :count)
+          expect(details[:system_event]).to be_truthy
+          expect(details[:reason]).to include('access expired on')
+        end
       end
     end
   end