Add audit events for merge request settings

Changelog: added
EE: true

app/models/project_setting.rb

@@ -36,6 +36,15 @@ def target_platforms=(val)
     super(val&.map(&:to_s)&.sort)
   end
 
+  def human_squash_option
+    case squash_option
+    when 'never' then 'Do not allow'
+    when 'always' then 'Require'
+    when 'default_on' then 'Encourage'
+    when 'default_off' then 'Allow'
+    end
+  end
+
   private
 
   def validates_mr_default_target_self

app/models/protected_branch.rb

@@ -68,6 +68,10 @@ def self.by_name(query)
   def allow_multiple?(type)
     type == :push
   end
+
+  def self.downcase_humanized_name
+    name.underscore.humanize.downcase
+  end
 end
 
 ProtectedBranch.prepend_mod_with('ProtectedBranch')

ee/app/models/merge_requests/external_status_check.rb

@@ -4,6 +4,7 @@ module MergeRequests
   class ExternalStatusCheck < ApplicationRecord
     self.table_name = 'external_status_checks'
 
+    include Auditable
     include IgnorableColumns
     ignore_column :external_approval_rule_id, remove_with: '14.3', remove_after: '2021-09-22'
 
@@ -15,8 +16,10 @@ class ExternalStatusCheck < ApplicationRecord
     end
 
     belongs_to :project
-    has_and_belongs_to_many :protected_branches
-
+    has_and_belongs_to_many :protected_branches,
+                            after_add: :audit_protected_branch_add, after_remove: :audit_protected_branch_remove
+    after_create_commit :audit_creation
+    after_destroy_commit :audit_deletion
     validates :external_url, presence: true, uniqueness: { scope: :project_id }, addressable_url: true
     validates :name, uniqueness: { scope: :project_id }, presence: true
     validate :protected_branches_must_belong_to_project
@@ -43,8 +46,43 @@ def to_h
       }
     end
 
+    def audit_protected_branch_add(model)
+      message = "Added #{model.class.downcase_humanized_name} #{model.name} to #{self.name} status check"
+      message += " and removed all other branches from status check" if protected_branches.count == 1
+      push_audit_event(message)
+    end
+
+    def audit_creation
+      message = "Added #{self.name} status check"
+      message += if protected_branches.empty?
+                   " with all branches"
+                 else
+                   " with protected branch(es) #{self.protected_branches_names}"
+                 end
+
+      push_audit_event(message)
+    end
+
+    def audit_deletion
+      push_audit_event("Removed #{self.name} status check")
+    end
+
+    def audit_protected_branch_remove(model)
+      message = if protected_branches.empty?
+                  "Added all branches to #{self.name} status check"
+                else
+                  "Removed #{model.class.downcase_humanized_name} #{model.name} from #{self.name} status check"
+                end
+
+      push_audit_event(message)
+    end
+
     private
 
+    def protected_branches_names
+      self.protected_branches.pluck(:name).join(', ')
+    end
+
     def payload_data(merge_request_hook_data)
       merge_request_hook_data.merge(external_approval_rule: self.to_h)
     end

ee/app/services/external_status_checks/base_service.rb

+# frozen_string_literal: true
+
+module ExternalStatusChecks
+  class BaseService < BaseContainerService
+    private
+
+    def with_audit_logged(rule, name, &block)
+      audit_context = {
+        name: name,
+        author: current_user,
+        scope: rule.project,
+        target: rule
+      }
+
+      ::Gitlab::Audit::Auditor.audit(audit_context, &block)
+    end
+  end
+end

ee/app/services/external_status_checks/create_service.rb

 # frozen_string_literal: true
 
 module ExternalStatusChecks
-  class CreateService < BaseContainerService
+  class CreateService < BaseService
     def execute
       return ServiceResponse.error(message: 'Failed to create rule', payload: { errors: ['Not allowed'] }, http_status: :unauthorized) unless current_user.can?(:admin_project, container)
 
       rule = container.external_status_checks.new(name: params[:name],
-                                                 project: container,
-                                                 external_url: params[:external_url],
-                                                 protected_branch_ids: params[:protected_branch_ids])
+                                                  project: container,
+                                                  external_url: params[:external_url],
+                                                  protected_branch_ids: params[:protected_branch_ids])
 
-      if rule.save
+      if with_audit_logged(rule, 'create_status_check') { rule.save }
         ServiceResponse.success(payload: { rule: rule })
       else
         ServiceResponse.error(message: 'Failed to create rule', payload: { errors: rule.errors.full_messages }, http_status: :unprocessable_entity)

ee/app/services/external_status_checks/destroy_service.rb

 # frozen_string_literal: true
 
 module ExternalStatusChecks
-  class DestroyService < BaseContainerService
+  class DestroyService < BaseService
     def execute(rule)
       return unauthorized_error_response unless current_user.can?(:admin_project, container)
 
-      if rule.destroy
+      if with_audit_logged(rule, 'delete_status_check') { rule.destroy }
         ServiceResponse.success
       else
         ServiceResponse.error(message: 'Failed to destroy rule',

ee/app/services/external_status_checks/update_service.rb

 # frozen_string_literal: true
 
 module ExternalStatusChecks
-  class UpdateService < BaseContainerService
+  class UpdateService < BaseService
     def execute
       return unauthorized_error_response unless current_user.can?(:admin_project, container)
 
-      if rule.update(resource_params)
+      if with_audit_logged(rule, 'update_status_check') { rule.update(resource_params) }
+        log_audit_event
         ServiceResponse.success(payload: { rule: rule })
       else
         ServiceResponse.error(message: 'Failed to update rule',
@@ -21,7 +22,7 @@ def resource_params
     end
 
     def rule
-      container.external_status_checks.find(params[:check_id])
+      @rule ||= container.external_status_checks.find(params[:check_id])
     end
 
     def unauthorized_error_response
@@ -31,5 +32,9 @@ def unauthorized_error_response
         http_status: :unauthorized
       )
     end
+
+    def log_audit_event
+      Audit::ExternalStatusCheckChangesAuditor.new(current_user, rule).execute
+    end
   end
 end

ee/lib/audit/external_status_check_changes_auditor.rb

+# frozen_string_literal: true
+
+module Audit
+  class ExternalStatusCheckChangesAuditor < ::EE::Audit::BaseChangesAuditor
+    def initialize(current_user, external_status_check)
+      @project = external_status_check.project
+
+      super
+    end
+
+    def execute
+      audit_changes(:name, as: 'name', entity: @project, model: model)
+      audit_changes(:external_url, as: 'external url', entity: @project, model: model)
+    end
+
+    def attributes_from_auditable_model(column)
+      {
+        from: model.previous_changes[column].first,
+        to: model.previous_changes[column].last
+      }
+    end
+  end
+end

ee/lib/ee/audit/project_changes_auditor.rb

@@ -16,6 +16,7 @@ def execute
         audit_changes(:reset_approvals_on_push, as: 'require new approvals when new commits are added to an MR', model: model)
         audit_changes(:disable_overriding_approvers_per_merge_request, as: 'prevent users from modifying MR approval rules in merge requests', model: model)
         audit_changes(:require_password_to_approve, as: 'require user password for approvals', model: model)
+        audit_changes(:merge_requests_template, as: 'merge_requests_template', model: model) if should_audit? :merge_requests_template
 
         audit_changes(:resolve_outdated_diff_discussions, as: 'resolve_outdated_diff_discussions', model: model)
         audit_changes(:printing_merge_request_link_enabled, as: 'printing_merge_request_link_enabled', model: model)

ee/lib/ee/audit/project_setting_changes_auditor.rb

@@ -11,9 +11,14 @@ def initialize(current_user, project_setting, project)
       def execute
         return if model.blank?
 
-        audit_changes(:squash_option, as: 'squash_option', entity: @project, model: model)
-        audit_changes(:allow_merge_on_skipped_pipeline, as: 'allow_merge_on_skipped_pipeline', entity: @project,
-                      model: model)
+        if should_audit? :allow_merge_on_skipped_pipeline
+          audit_changes(:allow_merge_on_skipped_pipeline, as: 'allow_merge_on_skipped_pipeline', entity: @project,
+                        model: model)
+        end
+
+        audit_squash_option
+        audit_changes(:merge_commit_template, as: 'merge_commit_template', entity: @project, model: model)
+        audit_changes(:squash_commit_template, as: 'squash_commit_template', entity: @project, model: model)
       end
 
       def attributes_from_auditable_model(column)
@@ -23,6 +28,22 @@ def attributes_from_auditable_model(column)
           target_details: @project.full_path
         }
       end
+
+      private
+
+      def audit_squash_option
+        return unless audit_required? :squash_option
+
+        squash_option_message = _("Changed squash option to %{squash_option}") %
+          { squash_option: model.human_squash_option }
+        audit_context = {
+          author: @current_user,
+          scope: @project,
+          target: @project,
+          message: squash_option_message
+        }
+        ::Gitlab::Audit::Auditor.audit(audit_context)
+      end
     end
   end
 end

ee/spec/lib/audit/external_status_check_changes_auditor_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Audit::ExternalStatusCheckChangesAuditor do
+  describe 'auditing external status check changes' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:project) { create(:project) }
+    let_it_be(:external_status_check) do
+      create(:external_status_check,
+             name: 'QA',
+             external_url: 'http://examplev1.com',
+             project: project)
+    end
+
+    let_it_be(:external_status_check_changes_auditor) { described_class.new(user, external_status_check) }
+
+    before do
+      stub_licensed_features(extended_audit_events: true)
+    end
+
+    let(:subject) { described_class.new(user, project.external_status_checks) }
+
+    context 'when audit change happens' do
+      it 'creates an event when the name changes' do
+        external_status_check.update!(name: 'QAv2')
+
+        expect { external_status_check_changes_auditor.execute }.to change { AuditEvent.count }.by(1)
+        expect(AuditEvent.last.details).to include({
+                                                     change: 'name',
+                                                     from: 'QA',
+                                                     to: 'QAv2'
+                                                   })
+      end
+
+      it 'creates an event when the external url changes' do
+        external_status_check.update!(external_url: 'http://examplev2.com')
+
+        expect { external_status_check_changes_auditor.execute }.to change { AuditEvent.count }.by(1)
+        expect(AuditEvent.last.details).to include({
+                                                     change: 'external url',
+                                                     from: 'http://examplev1.com',
+                                                     to: 'http://examplev2.com'
+                                                   })
+      end
+    end
+  end
+end

ee/spec/lib/ee/audit/project_changes_auditor_spec.rb

@@ -83,6 +83,18 @@
         expect(AuditEvent.last.details[:change]).to eq 'packages_enabled'
       end
 
+      it 'creates an event when the merge requests template changes' do
+        project.update!(merge_requests_template: 'I am a merge request template')
+
+        expect { foo_instance.execute }.to change { AuditEvent.count }.by(1)
+        expect(AuditEvent.last.details[:change]).to eq 'merge_requests_template'
+        expect(AuditEvent.last.details).to include({
+                                                     change: 'merge_requests_template',
+                                                     from: nil,
+                                                     to: 'I am a merge request template'
+                                                   })
+      end
+
       it 'creates an event when the merge requests author approval changes' do
         project.update!(merge_requests_author_approval: true)
 

ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb

@@ -22,17 +22,21 @@
               project.project_setting.update!(squash_option: prev_value)
             end
 
-            next if new_value == prev_value
+            if new_value != prev_value
+              it 'creates an audit event' do
+                project.project_setting.update!(squash_option: new_value)
 
-            it 'creates an audit event' do
-              project.project_setting.update!(squash_option: new_value)
-
-              expect { project_setting_changes_auditor.execute }.to change { AuditEvent.count }.by(1)
-              expect(AuditEvent.last.details).to include({
-                                                           change: 'squash_option',
-                                                           from: prev_value,
-                                                           to: new_value
-                                                         })
+                expect { project_setting_changes_auditor.execute }.to change { AuditEvent.count }.by(1)
+                expect(AuditEvent.last.details).to include(
+                  {
+                    custom_message: "Changed squash option to #{project.project_setting.human_squash_option}"
+                  })
+              end
+            else
+              it 'does not create audit event' do
+                project.project_setting.update!(squash_option: new_value)
+                expect { project_setting_changes_auditor.execute }.to not_change { AuditEvent.count }
+              end
             end
           end
         end

ee/spec/models/approval_project_rule_spec.rb

@@ -246,65 +246,39 @@
 
     let_it_be(:rule, reload: true) { create(:approval_project_rule, name: 'Vulnerability', 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 Vulnerability rule' }
 
-      it_behaves_like 'auditable'
+      it_behaves_like 'audit event queue'
     end
 
     describe '#audit_remove users after :remove' do
       let(:action!) { rule.update!(users: []) }
       let(:message) { 'Removed User Batman from approval group on Vulnerability rule' }
 
-      it_behaves_like 'auditable'
+      it_behaves_like 'audit event queue'
     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 Vulnerability rule' }
 
-      it_behaves_like 'auditable'
+      it_behaves_like 'audit event queue'
     end
 
     describe '#audit_remove groups after :remove' do
       let(:action!) { rule.update!(groups: []) }
       let(:message) { 'Removed Group Justice League from approval group on Vulnerability rule' }
 
-      it_behaves_like 'auditable'
+      it_behaves_like 'audit event queue'
     end
 
     describe "#audit_creation after approval rule is created" do
       let(:action!) { create(:approval_project_rule, approvals_required: 1) }
       let(:message) {'Added approval rule with number of required approvals of 1'}
 
-      it_behaves_like 'auditable'
+      it_behaves_like 'audit event queue'
     end
 
     describe '#vulnerability_states_for_branch' do

ee/spec/models/merge_requests/external_status_check_spec.rb

@@ -187,4 +187,94 @@
       end
     end
   end
+
+  describe 'callbacks', :request_store do
+    let_it_be(:project) { create(:project) }
+    let_it_be(:master_branch) { create(:protected_branch, project: project, name: 'master') }
+    let_it_be(:main_branch) { create(:protected_branch, project: project, name: 'main') }
+    let_it_be(:external_status_check, reload: true) do
+      create(:external_status_check, project: project, name: 'QA', protected_branches: [])
+    end
+
+    describe '#audit_add branches after :add' do
+      context 'when branch is added from zero branches' do
+        let(:action!) { external_status_check.update!(protected_branches: [main_branch]) }
+        let(:message) { 'Added protected branch main to QA status check and removed all other branches from status check' }
+
+        it_behaves_like 'audit event queue'
+      end
+
+      context 'when another branch is added' do
+        before do
+          external_status_check.update!(protected_branches: [main_branch])
+        end
+
+        let(:action!) { external_status_check.update!(protected_branches: [main_branch, master_branch]) }
+        let(:message) { 'Added protected branch master to QA status check' }
+
+        it_behaves_like 'audit event queue'
+      end
+    end
+
+    describe '#audit_remove branches after :remove' do
+      context 'when all the branches are removed' do
+        before do
+          external_status_check.update!(protected_branches: [main_branch])
+        end
+
+        let(:action!) { external_status_check.update!(protected_branches: []) }
+        let(:message) { 'Added all branches to QA status check' }
+
+        it_behaves_like 'audit event queue'
+      end
+
+      context 'when a branch is removed' do
+        before do
+          external_status_check.update!(protected_branches: [main_branch, master_branch])
+        end
+
+        let(:action!) { external_status_check.update!(protected_branches: [master_branch]) }
+        let(:message) { 'Removed protected branch main from QA status check' }
+
+        it_behaves_like 'audit event queue'
+      end
+    end
+
+    describe '#audit_creation external status check after :create' do
+      context 'when protected branches are added' do
+        let_it_be(:external_status_check) do
+          described_class.new(name: 'QAv2',
+                              project: project,
+                              external_url: 'https://www.example.com',
+                              protected_branch_ids: [main_branch.id, master_branch.id])
+        end
+
+        let(:action!) { external_status_check.save! }
+        let(:message) { 'Added QAv2 status check with protected branch(es) main, master' }
+
+        it_behaves_like 'audit event queue'
+      end
+
+      context 'when all branches are added' do
+        let_it_be(:external_status_check) do
+          described_class.new(name: 'QAv2',
+                              project: project,
+                              external_url: 'https://www.example.com',
+                              protected_branch_ids: [])
+        end
+
+        let(:action!) { external_status_check.save! }
+        let(:message) { 'Added QAv2 status check with all branches' }
+
+        it_behaves_like 'audit event queue'
+      end
+    end
+
+    describe '#audit_creation external status check after :create' do
+      let(:action!) { external_status_check.destroy! }
+      let(:message) { 'Removed QA status check' }
+
+      it_behaves_like 'audit event queue'
+    end
+  end
 end

ee/spec/services/external_status_checks/create_service_spec.rb

@@ -66,4 +66,33 @@
       expect(rule.protected_branches).to contain_exactly(protected_branch)
     end
   end
+
+  describe 'audit events' do
+    context 'when licensed' do
+      before do
+        stub_licensed_features(audit_events: true)
+      end
+
+      context 'when external status check save operation succeeds', :request_store do
+        it 'logs an audit event' do
+          expect { subject }.to change { AuditEvent.count }.by(1)
+          expect(AuditEvent.last.details).to include({
+                    custom_message: "Added Test status check with protected branch(es) #{protected_branch.name}"
+                  })
+        end
+      end
+
+      context 'when external status check save operation fails' do
+        before do
+          allow(::MergeRequests::ExternalStatusCheck).to receive(:save).and_return(false)
+        end
+
+        it 'does not log any audit event' do
+          expect { subject }.not_to change { AuditEvent.count }
+        end
+      end
+    end
+
+    it_behaves_like 'does not create audit event when not licensed'
+  end
 end

ee/spec/services/external_status_checks/destroy_service_spec.rb

@@ -4,7 +4,7 @@
 
 RSpec.describe ExternalStatusChecks::DestroyService do
   let_it_be(:project) { create(:project) }
-  let_it_be(:rule) { create(:external_status_check, project: project) }
+  let_it_be(:rule) { create(:external_status_check, name: 'QA', project: project) }
 
   let(:current_user) { project.first_owner }
 
@@ -40,4 +40,34 @@
       expect(subject.payload[:errors]).to contain_exactly('Not allowed')
     end
   end
+
+  describe 'audit events' do
+    context 'when licensed' do
+      before do
+        stub_licensed_features(audit_events: true)
+      end
+
+      context 'when rule destroy operation succeeds', :request_store do
+        it 'logs an audit event' do
+          expect { subject }.to change { AuditEvent.count }.by(1)
+          expect(AuditEvent.last.details).to include({
+                                                       target_type: 'MergeRequests::ExternalStatusCheck',
+                                                       custom_message: 'Removed QA status check'
+                                                     })
+        end
+      end
+
+      context 'when rule destroy operation fails' do
+        before do
+          allow(::MergeRequests::ExternalStatusCheck).to receive(:destroy).and_return(false)
+        end
+
+        it 'does not log any audit event' do
+          expect { subject }.not_to change { AuditEvent.count }
+        end
+      end
+    end
+
+    it_behaves_like 'does not create audit event when not licensed'
+  end
 end

ee/spec/services/external_status_checks/update_service_spec.rb

@@ -48,4 +48,75 @@
       expect(subject.payload[:errors]).to contain_exactly('Not allowed')
     end
   end
+
+  describe 'audit events' do
+    context 'when licensed' do
+      before do
+        stub_licensed_features(audit_events: true)
+      end
+      let_it_be(:master_branch) { create(:protected_branch, project: project, name: 'master') }
+      let_it_be(:main_branch) {create(:protected_branch, project: project, name: 'main')}
+      let_it_be(:external_status_check, reload: true) { create(:external_status_check, name: 'QA', project: project, protected_branches: []) }
+
+      context 'when a branch is added', :request_store do
+        context 'when a new branch is added' do
+          let_it_be(:params) {{ id: project.id, check_id: external_status_check.id, protected_branch_ids: [main_branch.id] } }
+
+          it 'logs an audit event' do
+            expect { subject }.to change { AuditEvent.count }.by(1)
+            expect(AuditEvent.last.details[:custom_message]).to eq "Added protected branch main to QA status check and removed all other branches from status check"
+          end
+        end
+
+        context 'when another branch is added' do
+          before do
+            external_status_check.update!(protected_branches: [main_branch])
+          end
+
+          let_it_be(:params) {{ id: project.id, check_id: external_status_check.id, protected_branch_ids: [main_branch.id, master_branch.id] } }
+
+          it 'logs an audit event' do
+            expect { subject }.to change { AuditEvent.count }.by(1)
+            expect(AuditEvent.last.details[:custom_message]).to eq "Added protected branch master to QA status check"
+          end
+        end
+      end
+
+      context 'when a branch is removed', :request_store do
+        context 'when the only branch is removed' do
+          before do
+            external_status_check.update!(protected_branches: [main_branch])
+          end
+
+          let_it_be(:params) {{ id: project.id, check_id: external_status_check.id, protected_branch_ids: [] } }
+
+          it 'logs an audit event' do
+            expect { subject }.to change { AuditEvent.count }.by(1)
+            expect(AuditEvent.last.details[:custom_message]).to eq "Added all branches to QA status check"
+          end
+        end
+
+        context 'when a branch is removed' do
+          before do
+            external_status_check.update!(protected_branches: [main_branch, master_branch])
+          end
+
+          let_it_be(:params) {{ id: project.id, check_id: external_status_check.id, protected_branch_ids: [main_branch.id] } }
+
+          it 'logs an audit event' do
+            expect { subject }.to change { AuditEvent.count }.by(1)
+            expect(AuditEvent.last.details[:custom_message]).to eq "Removed protected branch master from QA status check"
+          end
+        end
+      end
+    end
+
+    it 'executes ExternalStatusCheckChangesAuditor' do
+      expect(Audit::ExternalStatusCheckChangesAuditor).to receive(:new).with(current_user, check).and_call_original
+
+      subject
+    end
+  end
+
+  it_behaves_like 'does not create audit event when not licensed'
 end

ee/spec/support/shared_contexts/audit_event_not_licensed_shared_context.rb

+# frozen_string_literal: true
+
+RSpec.shared_context 'does not create audit event when not licensed' do
+  before do
+    stub_licensed_features(
+      admin_audit_log: false,
+      audit_events: false,
+      extended_audit_events: false
+    )
+  end
+
+  it 'does not log any audit event' do
+    expect { subject }.not_to change { AuditEvent.count }
+  end
+end

ee/spec/support/shared_contexts/audit_event_queue_shared_context.rb

+# frozen_string_literal: true
+
+RSpec.shared_context 'audit event queue' 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