Merge branch '213603-audit-changes-to-approval-groups' into 'master'

Audit changes to project approval groups

See merge request gitlab-org/gitlab!36393

doc/administration/audit_events.md

@@ -94,6 +94,7 @@ From there, you can see the following actions:
 - Permission to approve merge requests by committers was updated ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/7531) in GitLab 12.9)
 - Permission to approve merge requests by authors was updated ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/7531) in GitLab 12.9)
 - 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 events can also be accessed via the [Project Audit Events API](../api/audit_events.md#project-audit-events-starter)
 

ee/app/models/approval_merge_request_rule.rb

@@ -83,6 +83,16 @@ class ApprovalMergeRequestRule < ApplicationRecord
     end
   end
 
+  def audit_add(_model)
+    # no-op
+    # only audit on project rule
+  end
+
+  def audit_remove(_model)
+    # no-op
+    # only audit on project rule
+  end
+
   def project
     merge_request.target_project
   end

ee/app/models/approval_project_rule.rb

@@ -2,6 +2,7 @@
 
 class ApprovalProjectRule < ApplicationRecord
   include ApprovalRuleLike
+  include Auditable
 
   belongs_to :project
   has_and_belongs_to_many :protected_branches
@@ -51,6 +52,14 @@ class ApprovalProjectRule < ApplicationRecord
     rule
   end
 
+  def audit_add(model)
+    push_audit_event("Added #{model.class.name} #{model.name} to approval group on #{self.name} rule")
+  end
+
+  def audit_remove(model)
+    push_audit_event("Removed #{model.class.name} #{model.name} from approval group on #{self.name} rule")
+  end
+
   private
 
   def report_type_for(rule)

ee/app/models/concerns/approval_rule_like.rb

@@ -14,8 +14,11 @@ module ApprovalRuleLike
   ALL_MEMBERS = 'All Members'
 
   included do
-    has_and_belongs_to_many :users
-    has_and_belongs_to_many :groups, class_name: 'Group', join_table: "#{self.table_name}_groups"
+    has_and_belongs_to_many :users,
+      after_add: :audit_add, after_remove: :audit_remove
+    has_and_belongs_to_many :groups,
+      class_name: 'Group', join_table: "#{self.table_name}_groups",
+      after_add: :audit_add, after_remove: :audit_remove
     has_many :group_users, -> { distinct }, through: :groups, source: :users
 
     validates :name, presence: true
@@ -25,6 +28,14 @@ module ApprovalRuleLike
     scope :regular_or_any_approver, -> { where(rule_type: [:regular, :any_approver]) }
   end
 
+  def audit_add
+    raise NotImplementedError
+  end
+
+  def audit_remove
+    raise NotImplementedError
+  end
+
   # Users who are eligible to approve, including specified group members.
   # @return [Array<User>]
   def approvers

ee/app/services/concerns/approval_rules/updater.rb

@@ -9,7 +9,7 @@ module ApprovalRules
       filter_eligible_groups!
       filter_eligible_protected_branches!
 
-      if rule.update(params)
+      if with_audit_logged { rule.update(params) }
         log_audit_event(rule)
         rule.reset
 
@@ -21,6 +21,17 @@ module ApprovalRules
 
     private
 
+    def with_audit_logged(&block)
+      audit_context = {
+        name: 'update_aproval_rules',
+        author: current_user,
+        scope: rule.project,
+        target: rule
+      }
+
+      ::Gitlab::Audit::Auditor.audit(audit_context, &block)
+    end
+
     def filter_eligible_users!
       return unless params.key?(:user_ids)
 

ee/changelogs/unreleased/213603-audit-changes-to-project-approval-group.yml

+---
+title: Audit changes to project approval groups
+merge_request: 36393
+author:
+type: added

ee/spec/models/approval_project_rule_spec.rb

@@ -209,4 +209,68 @@ RSpec.describe ApprovalProjectRule do
       end
     end
   end
+
+  describe 'callbacks', :request_store do
+    let_it_be(:user) { create(:user, name: 'Batman') }
+    let_it_be(:group) { create(:group, name: 'Justice League') }
+
+    let_it_be(:new_user) { create(:user, name: 'Spiderman') }
+    let_it_be(:new_group) { create(:group, name: 'Avengers') }
+
+    let_it_be(:rule, reload: true) { create(:approval_project_rule, name: 'Security', users: [user], groups: [group]) }
+
+    shared_examples 'auditable' do
+      context 'when audit event queue is active' do
+        before do
+          allow(::Gitlab::Audit::EventQueue).to receive(:active?).and_return(true)
+        end
+
+        it 'adds message to audit event queue' do
+          action!
+
+          expect(::Gitlab::Audit::EventQueue.current).to contain_exactly(message)
+        end
+      end
+
+      context 'when audit event queue is not active' do
+        before do
+          allow(::Gitlab::Audit::EventQueue).to receive(:active?).and_return(false)
+        end
+
+        it 'does not add message to audit event queue' do
+          action!
+
+          expect(::Gitlab::Audit::EventQueue.current).to be_empty
+        end
+      end
+    end
+
+    describe '#audit_add users after :add' do
+      let(:action!) { rule.update(users: [user, new_user]) }
+      let(:message) { 'Added User Spiderman to approval group on Security rule' }
+
+      it_behaves_like 'auditable'
+    end
+
+    describe '#audit_remove users after :remove' do
+      let(:action!) { rule.update(users: []) }
+      let(:message) { 'Removed User Batman from approval group on Security rule' }
+
+      it_behaves_like 'auditable'
+    end
+
+    describe '#audit_add groups after :add' do
+      let(:action!) { rule.update(groups: [group, new_group]) }
+      let(:message) { 'Added Group Avengers to approval group on Security rule' }
+
+      it_behaves_like 'auditable'
+    end
+
+    describe '#audit_remove groups after :remove' do
+      let(:action!) { rule.update(groups: []) }
+      let(:message) { 'Removed Group Justice League from approval group on Security rule' }
+
+      it_behaves_like 'auditable'
+    end
+  end
 end

ee/spec/services/approval_rules/update_service_spec.rb

@@ -192,13 +192,24 @@ RSpec.describe ApprovalRules::UpdateService do
     end
 
     describe 'audit events' do
-      subject(:operation) do
-        described_class.new(
-          approval_rule,
-          user,
-          name: 'developers',
-          approvals_required: 1
-        ).execute
+      let_it_be(:approver) { create(:user, name: 'Batman') }
+      let_it_be(:group) { create(:group, name: 'Justice League') }
+      let_it_be(:new_approver) { create(:user, name: 'Spiderman') }
+      let_it_be(:new_group) { create(:group, name: 'Avengers') }
+
+      let(:approval_rule) do
+        create(:approval_project_rule,
+          name: 'Gotham',
+          project: target,
+          approvals_required: 2,
+          users: [approver],
+          groups: [group]
+        )
+      end
+
+      before do
+        project.add_reporter approver
+        project.add_reporter new_approver
       end
 
       context 'when licensed' do
@@ -206,18 +217,52 @@ RSpec.describe ApprovalRules::UpdateService do
           stub_licensed_features(audit_events: true)
         end
 
-        context 'when rule update operation succeeds' do
+        context 'when rule update operation succeeds', :request_store do
           it 'logs an audit event' do
-            expect { operation }.to change { AuditEvent.count }.by(1)
+            expect do
+              described_class.new(approval_rule, user, approvals_required: 1).execute
+            end.to change { AuditEvent.count }.by(1)
           end
 
-          it 'logs the audit event info' do
-            operation
+          it 'audits the number of required approvals change' do
+            described_class.new(approval_rule, user, approvals_required: 1).execute
 
             expect(AuditEvent.last).to have_attributes(
               details: hash_including(change: 'number of required approvals', from: 2, to: 1)
             )
           end
+
+          it 'audits the group addition to approval group' do
+            described_class.new(approval_rule, user, group_ids: [group.id, new_group.id]).execute
+
+            expect(AuditEvent.last.details[:custom_message]).to eq(
+              "Added Group Avengers to approval group on Gotham rule"
+            )
+          end
+
+          it 'audits the group removal from approval group' do
+            described_class.new(approval_rule, user, group_ids: []).execute
+
+            expect(AuditEvent.last.details[:custom_message]).to eq(
+              "Removed Group Justice League from approval group on Gotham rule"
+            )
+          end
+
+          it 'audits the user addition to approval group' do
+            described_class.new(approval_rule, user, user_ids: [approver.id, new_approver.id]).execute
+
+            expect(AuditEvent.last.details[:custom_message]).to eq(
+              "Added User Spiderman to approval group on Gotham rule"
+            )
+          end
+
+          it 'audits the user removal from approval group' do
+            described_class.new(approval_rule, user, user_ids: []).execute
+
+            expect(AuditEvent.last.details[:custom_message]).to eq(
+              "Removed User Batman from approval group on Gotham rule"
+            )
+          end
         end
 
         context 'when rule update operation fails' do
@@ -226,7 +271,9 @@ RSpec.describe ApprovalRules::UpdateService do
           end
 
           it 'does not log any audit event' do
-            expect { operation }.not_to change { AuditEvent.count }
+            expect do
+              described_class.new(approval_rule, user, approvals_required: 1).execute
+            end.not_to change { AuditEvent.count }
           end
         end
       end
@@ -241,7 +288,9 @@ RSpec.describe ApprovalRules::UpdateService do
         end
 
         it 'does not log any audit event' do
-          expect { operation }.not_to change { AuditEvent.count }
+          expect do
+            described_class.new(approval_rule, user, approvals_required: 1).execute
+          end.not_to change { AuditEvent.count }
         end
       end
     end