Merge branch...

Merge branch '12703-escalated-audit-event-for-changing-access-expiration-should-contain-the-expiration-date' into 'master'

Resolve "ESCALATED: Audit Event for Changing Access Expiration Should Contain the Expiration Date"

See merge request gitlab-org/gitlab!22412

app/services/members/update_service.rb

@@ -7,9 +7,10 @@ module Members
       raise Gitlab::Access::AccessDeniedError unless can?(current_user, action_member_permission(permission, member), member)
 
       old_access_level = member.human_access
+      old_expiry = member.expires_at
 
       if member.update(params)
-        after_execute(action: permission, old_access_level: old_access_level, member: member)
+        after_execute(action: permission, old_access_level: old_access_level, old_expiry: old_expiry, member: member)
 
         # Deletes only confidential issues todos for guests
         enqueue_delete_todos(member) if downgrading_to_guest?

ee/app/helpers/audit_events_helper.rb

@@ -12,6 +12,8 @@ module AuditEventsHelper
       ""
     elsif key.to_s == 'ip_address' && value.blank?
       ""
+    elsif key =~ /^expiry_(from|to)$/ && value.blank?
+      "#{key} <strong>never expires</strong>"
     else
       "#{key} <strong>#{value}</strong>"
     end

ee/app/services/ee/audit_event_service.rb

@@ -44,6 +44,8 @@ module EE
             change: "access_level",
             from: old_access_level,
             to: member.human_access,
+            expiry_from: @details[:old_expiry],
+            expiry_to: member.expires_at,
             author_name: @author.name,
             target_id: user_id,
             target_type: "User",

ee/app/services/ee/members/update_service.rb

@@ -5,20 +5,21 @@ module EE
     module UpdateService
       extend ActiveSupport::Concern
 
-      def after_execute(action:, old_access_level:, member:)
+      def after_execute(action:, old_access_level:, old_expiry:, member:)
         super
 
-        log_audit_event(action: action, old_access_level: old_access_level, member: member)
+        log_audit_event(action: action, old_access_level: old_access_level, old_expiry: old_expiry, member: member)
       end
 
       private
 
-      def log_audit_event(action:, old_access_level:, member:)
+      def log_audit_event(action:, old_access_level:, old_expiry:, member:)
         ::AuditEventService.new(
           current_user,
           member.source,
           action: action,
-          old_access_level: old_access_level
+          old_access_level: old_access_level,
+          old_expiry: old_expiry
         ).for_member(member).security_event
       end
     end

ee/changelogs/unreleased/12703-escalated-audit-event-for-changing-access-expiration-should-cont.yml

+---
+title: Show expiry details in Audit events when changing acesss levels
+merge_request: 22412
+author:
+type: fixed

ee/lib/audit/details.rb

@@ -57,9 +57,43 @@ module Audit
       changed << "from #{@details[:from]}" if @details[:from]
       changed << "to #{@details[:to]}" if @details[:to]
 
+      if access_level_changed?(value) && expiry_details_available?
+        changed << text_for_expiry_change
+      end
+
       changed.join(' ')
     end
 
+    # this check is made in order to not show expiry details for older audit events
+    # that has been logged *without* these keys.
+    def expiry_details_available?
+      @details.has_key?(:expiry_from) && @details.has_key?(:expiry_to)
+    end
+
+    def text_for_expiry_change
+      old_expiry = @details[:expiry_from].presence || never_expires_text
+      new_expiry = @details[:expiry_to].presence || never_expires_text
+
+      if expiry_changed?(old_expiry, new_expiry)
+        _('with expiry changing from %{old_expiry} to %{new_expiry}') %
+          { old_expiry: old_expiry, new_expiry: new_expiry }
+      else
+        _('with expiry remaining unchanged at %{old_expiry}') % { old_expiry: old_expiry }
+      end
+    end
+
+    def never_expires_text
+      _('never expires')
+    end
+
+    def expiry_changed?(old_expiry, new_expiry)
+      new_expiry != old_expiry
+    end
+
+    def access_level_changed?(value)
+      value == 'access level'
+    end
+
     def target_name
       @details[:target_type] == 'Operations::FeatureFlag' ? detail_value : target_name_with_space
     end

ee/spec/helpers/audit_events_helper_spec.rb

@@ -55,5 +55,13 @@ describe AuditEventsHelper do
     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
+
+    it 'returns formatted text with `never expires` if key is expiry_from and the value is blank' do
+      expect(select_keys('expiry_from', nil)).to eq 'expiry_from <strong>never expires</strong>'
+    end
+
+    it 'returns formatted text with `never expires` if key is expiry_to and the value is blank' do
+      expect(select_keys('expiry_to', nil)).to eq 'expiry_to <strong>never expires</strong>'
+    end
   end
 end

ee/spec/lib/audit/details_spec.rb

@@ -27,21 +27,108 @@ describe Audit::Details do
       let(:user_member) { create(:user) }
       let(:project) { create(:project) }
       let(:member) { create(:project_member, :developer, user: user_member, project: project) }
-      let(:member_access_action) do
-        {
-          add: 'user_access',
-          as: Gitlab::Access.options_with_owner.key(member.access_level.to_i),
-          author_name: user.name,
-          target_id: member.id,
-          target_type: 'User',
-          target_details: member.user.name
-        }
+
+      context 'add project member' do
+        let(:member_access_action) do
+          {
+            add: 'user_access',
+            as: Gitlab::Access.options_with_owner.key(member.access_level.to_i),
+            author_name: user.name,
+            target_id: member.id,
+            target_type: 'User',
+            target_details: member.user.name
+          }
+        end
+
+        it 'humanizes add project member access action' do
+          string = described_class.humanize(member_access_action)
+
+          expect(string).to eq('Added user access as Developer')
+        end
       end
 
-      it 'humanizes add project member access action' do
-        string = described_class.humanize(member_access_action)
+      context 'update project member' do
+        context 'access expiry' do
+          context 'when expiry details are present' do
+            let(:member_access_action) do
+              {
+                change: 'access_level',
+                from: 'Maintainer',
+                to: 'Reporter',
+                expiry_from: expiry_from,
+                expiry_to: expiry_to,
+                author_name: user.name,
+                target_id: member.id,
+                target_type: 'User',
+                target_details: member.user.name
+              }
+            end
+
+            context 'when old expiry date is not equal to new expiry date' do
+              let(:expiry_from) { Date.parse('2020-01-03') }
+              let(:expiry_to) { Date.parse('2020-01-04') }
 
-        expect(string).to eq('Added user access as Developer')
+              it 'includes information about change in expiry date' do
+                string = described_class.humanize(member_access_action)
+
+                expect(string).to eq('Changed access level from Maintainer to Reporter with expiry changing from 2020-01-03 to 2020-01-04')
+              end
+            end
+
+            context 'when old expiry date is equal to new expiry date' do
+              let(:expiry_from) { Date.parse('2020-01-03') }
+              let(:expiry_to) { Date.parse('2020-01-03') }
+
+              it 'includes information about expiry date remaining unchanged' do
+                string = described_class.humanize(member_access_action)
+
+                expect(string).to eq('Changed access level from Maintainer to Reporter with expiry remaining unchanged at 2020-01-03')
+              end
+            end
+
+            context 'when the expiry is set to `never expires`' do
+              let(:expiry_from) { Date.parse('2020-01-03') }
+              let(:expiry_to) { nil }
+
+              it 'includes information about expiry date being set to never expires' do
+                string = described_class.humanize(member_access_action)
+
+                expect(string).to eq('Changed access level from Maintainer to Reporter with expiry changing from 2020-01-03 to never expires')
+              end
+            end
+
+            context 'when the expiry is changed from `never expires`' do
+              let(:expiry_from) { nil }
+              let(:expiry_to) { Date.parse('2020-01-04') }
+
+              it 'includes information about expiry date being changed from never expires' do
+                string = described_class.humanize(member_access_action)
+
+                expect(string).to eq('Changed access level from Maintainer to Reporter with expiry changing from never expires to 2020-01-04')
+              end
+            end
+          end
+
+          context 'when expiry details are not present' do
+            let(:member_access_action) do
+              {
+                change: 'access_level',
+                from: 'Maintainer',
+                to: 'Reporter',
+                author_name: user.name,
+                target_id: member.id,
+                target_type: 'User',
+                target_details: member.user.name
+              }
+            end
+
+            it 'does not include any information about expiry date' do
+              string = described_class.humanize(member_access_action)
+
+              expect(string).to eq('Changed access level from Maintainer to Reporter')
+            end
+          end
+        end
       end
     end
 

ee/spec/services/audit_event_service_spec.rb

@@ -38,6 +38,24 @@ describe AuditEventService do
       end
     end
 
+    context 'updating membership' do
+      let(:service) do
+        described_class.new(user, project, {
+          action: :update,
+          old_access_level: 'Reporter',
+          old_expiry: Date.today
+        })
+      end
+
+      it 'records the change in expiry date' do
+        event = service.for_member(project_member).security_event
+
+        expect(event[:details][:change]).to eq('access_level')
+        expect(event[:details][:expiry_from]).to eq(Date.today)
+        expect(event[:details][:expiry_to]).to eq(1.day.from_now.to_date)
+      end
+    end
+
     context 'admin audit log licensed' do
       before do
         stub_licensed_features(admin_audit_log: true)

ee/spec/services/ee/members/update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Members::UpdateService do
+  let(:project) { create(:project, :public) }
+  let(:group) { create(:group, :public) }
+  let(:current_user) { create(:user) }
+  let(:member_user) { create(:user) }
+  let(:permission) { :update }
+  let(:member) { source.members_and_requesters.find_by!(user_id: member_user.id) }
+  let(:params) do
+    { access_level: Gitlab::Access::MAINTAINER, expires_at: Date.parse('2020-01-03') }
+  end
+
+  before do
+    project.add_developer(member_user)
+    group.add_developer(member_user)
+  end
+
+  shared_examples_for 'logs an audit event' do
+    it do
+      expect do
+        described_class.new(current_user, params).execute(member, permission: permission)
+      end.to change { SecurityEvent.count }.by(1)
+    end
+  end
+
+  context 'when current user can update the given member' do
+    before do
+      project.add_maintainer(current_user)
+      group.add_owner(current_user)
+    end
+
+    it_behaves_like 'logs an audit event' do
+      let(:source) { project }
+    end
+
+    it_behaves_like 'logs an audit event' do
+      let(:source) { group }
+    end
+  end
+end

locale/gitlab.pot

@@ -22106,6 +22106,9 @@ msgstr ""
 msgid "needs to be between 10 minutes and 1 month"
 msgstr ""
 
+msgid "never expires"
+msgstr ""
+
 msgid "new merge request"
 msgstr ""
 
@@ -22388,5 +22391,11 @@ msgstr ""
 msgid "with %{additions} additions, %{deletions} deletions."
 msgstr ""
 
+msgid "with expiry changing from %{old_expiry} to %{new_expiry}"
+msgstr ""
+
+msgid "with expiry remaining unchanged at %{old_expiry}"
+msgstr ""
+
 msgid "yaml invalid"
 msgstr ""