Merge branch '301124-merge-request-setting-audit-events' into 'master'

Add audit events for merge request settings

See merge request gitlab-org/gitlab!83922

(cherry picked from commit 8c6f4a33)

859d2a8b Add audit events for merge request settings
e6f7a15c Apply 10 suggestion(s) to 5 file(s)
7b03a8e9 Disable audit on column change from nil to false/blank
3207a415 Move blank to blank logic in auditor

ee/lib/ee/audit/base_changes_auditor.rb

@@ -17,6 +17,17 @@ module EE
       def attributes_from_auditable_model(column)
         raise NotImplementedError
       end
+
+      # Disables auditing when there is unintentional column change done via API params
+      # for example: Project's suggestion_commit_message column is updated from nil to empty string when the user edits
+      # project merge request setting even if user didn't changed the column in the form.
+      def should_audit?(column)
+        return false unless model.previous_changes.has_key?(column)
+
+        from = model.previous_changes[column].first
+        to = model.previous_changes[column].second
+        !(from.blank? && to.blank?)
+      end
     end
   end
 end

ee/lib/ee/audit/project_changes_auditor.rb

@@ -17,12 +17,36 @@ module EE
         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(: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)
+        audit_changes(:remove_source_branch_after_merge, as: 'remove_source_branch_after_merge', model: model)
+        audit_changes(:only_allow_merge_if_pipeline_succeeds, as: 'only_allow_merge_if_pipeline_succeeds', model: model)
+        audit_changes(:only_allow_merge_if_all_discussions_are_resolved, as: 'only_allow_merge_if_all_discussions_are_resolved', model: model)
+        audit_changes(:suggestion_commit_message, as: 'suggestion_commit_message', model: model) if should_audit?(:suggestion_commit_message)
+
+        audit_merge_method
         audit_project_feature_changes
         audit_compliance_framework_changes
+        audit_project_setting_changes
+        audit_project_ci_cd_setting_changes
       end
 
       private
 
+      def audit_merge_method
+        return unless model.previous_changes.has_key?(:merge_requests_ff_only_enabled) ||
+          model.previous_changes.has_key?(:merge_requests_rebase_enabled)
+
+        merge_method_message = _("Changed merge method to %{merge_method}") % { merge_method: model.human_merge_method }
+        audit_context = {
+          author: @current_user,
+          scope: model,
+          target: model,
+          message: merge_method_message
+        }
+        ::Gitlab::Audit::Auditor.audit(audit_context)
+      end
+
       def audit_project_feature_changes
         ::EE::Audit::ProjectFeatureChangesAuditor.new(@current_user, model.project_feature, model).execute
       end
@@ -31,6 +55,14 @@ module EE
         ::EE::Audit::ComplianceFrameworkChangesAuditor.new(@current_user, model.compliance_framework_setting, model).execute
       end
 
+      def audit_project_setting_changes
+        ::EE::Audit::ProjectSettingChangesAuditor.new(@current_user, model.project_setting, model).execute
+      end
+
+      def audit_project_ci_cd_setting_changes
+        ::EE::Audit::ProjectCiCdSettingChangesAuditor.new(@current_user, model.ci_cd_settings, model).execute
+      end
+
       def attributes_from_auditable_model(column)
         case column
         when :name

ee/lib/ee/audit/project_ci_cd_setting_changes_auditor.rb

+# frozen_string_literal: true
+module EE
+  module Audit
+    class ProjectCiCdSettingChangesAuditor < BaseChangesAuditor
+      def initialize(current_user, ci_cd_settings, project)
+        @project = project
+
+        super(current_user, ci_cd_settings)
+      end
+
+      def execute
+        return if model.blank?
+
+        if should_audit?(:merge_pipelines_enabled)
+          audit_changes(:merge_pipelines_enabled, as: 'merge_pipelines_enabled', entity: @project, model: model)
+        end
+
+        if should_audit?(:merge_trains_enabled)
+          audit_changes(:merge_trains_enabled, as: 'merge_trains_enabled', entity: @project, model: model)
+        end
+      end
+
+      def attributes_from_auditable_model(column)
+        {
+          from: model.previous_changes[column].first,
+          to: model.previous_changes[column].last,
+          target_details: @project.full_path
+        }
+      end
+    end
+  end
+end

ee/lib/ee/audit/project_setting_changes_auditor.rb

+# frozen_string_literal: true
+module EE
+  module Audit
+    class ProjectSettingChangesAuditor < BaseChangesAuditor
+      def initialize(current_user, project_setting, project)
+        @project = project
+
+        super(current_user, project_setting)
+      end
+
+      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)
+      end
+
+      def attributes_from_auditable_model(column)
+        {
+          from: model.previous_changes[column].first,
+          to: model.previous_changes[column].last,
+          target_details: @project.full_path
+        }
+      end
+    end
+  end
+end

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

@@ -3,6 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe EE::Audit::ProjectChangesAuditor do
+  using RSpec::Parameterized::TableSyntax
   describe '.audit_changes' do
     let_it_be(:user) { create(:user) }
 
@@ -146,6 +147,100 @@ RSpec.describe EE::Audit::ProjectChangesAuditor do
           )
         end
       end
+
+      context 'when auditable boolean column is changed' do
+        columns = %w[resolve_outdated_diff_discussions printing_merge_request_link_enabled
+                     remove_source_branch_after_merge only_allow_merge_if_pipeline_succeeds
+                     only_allow_merge_if_all_discussions_are_resolved]
+        columns.each do |column|
+          where(:prev_value, :new_value) do
+            true  | false
+            false | true
+          end
+
+          before do
+            project.update_attribute(column, prev_value)
+          end
+
+          with_them do
+            it 'creates an audit event' do
+              project.update_attribute(column, new_value)
+
+              expect { foo_instance.execute }.to change { AuditEvent.count }.by(1)
+              expect(AuditEvent.last.details).to include({
+                                                           change: column,
+                                                           from: prev_value,
+                                                           to: new_value
+                                                         })
+            end
+          end
+        end
+      end
+
+      it 'creates an event when suggestion_commit_message change' do
+        previous_value = project.suggestion_commit_message
+        new_value = "I'm a suggested commit message"
+        project.update!(suggestion_commit_message: new_value)
+
+        expect { foo_instance.execute }.to change { AuditEvent.count }.by(1)
+        expect(AuditEvent.last.details).to include({
+                                                     change: 'suggestion_commit_message',
+                                                     from: previous_value,
+                                                     to: new_value
+                                                   })
+      end
+
+      it 'does not create an event when suggestion_commit_message change from nil to empty string' do
+        project.update!(suggestion_commit_message: "")
+
+        expect { foo_instance.execute }.not_to change { AuditEvent.count }
+      end
+
+      context 'when merge method is changed from Merge' do
+        where(:ff, :rebase, :method) do
+          true  | true  | 'Fast-forward'
+          true  | false | 'Fast-forward'
+          false | true  | 'Rebase merge'
+        end
+
+        before do
+          project.update!(merge_requests_ff_only_enabled: false, merge_requests_rebase_enabled: false)
+        end
+
+        with_them do
+          it 'creates an audit event' do
+            project.update!(merge_requests_ff_only_enabled: ff, merge_requests_rebase_enabled: rebase)
+
+            expect { foo_instance.execute }.to change { AuditEvent.count }.by(1)
+            expect(AuditEvent.last.details).to include({
+                                                         custom_message: "Changed merge method to #{method}"
+                                                       })
+          end
+        end
+      end
+
+      context 'when merge method is changed to Merge' do
+        where(:ff, :rebase) do
+          true  | true
+          true  | false
+          false | true
+        end
+
+        with_them do
+          before do
+            project.update!(merge_requests_ff_only_enabled: ff, merge_requests_rebase_enabled: rebase)
+          end
+
+          it 'creates an Merge method audit event' do
+            project.update!(merge_requests_ff_only_enabled: false, merge_requests_rebase_enabled: false)
+
+            expect { foo_instance.execute }.to change { AuditEvent.count }.by(1)
+            expect(AuditEvent.last.details).to include({
+                                                         custom_message: "Changed merge method to Merge"
+                                                       })
+          end
+        end
+      end
     end
   end
 end

ee/spec/lib/ee/audit/project_ci_cd_setting_changes_auditor_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe EE::Audit::ProjectCiCdSettingChangesAuditor do
+  using RSpec::Parameterized::TableSyntax
+  describe '#execute' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:project) { create(:project) }
+    let_it_be(:ci_cd_settings) { project.ci_cd_settings }
+    let_it_be(:project_ci_cd_setting_changes_auditor) { described_class.new(user, ci_cd_settings, project) }
+
+    before do
+      stub_licensed_features(extended_audit_events: true)
+    end
+
+    context 'when auditable boolean column is changed' do
+      columns = %w[merge_trains_enabled merge_pipelines_enabled]
+      columns.each do |column|
+        context 'when column changes from boolean' do
+          where(:prev_value, :new_value) do
+            true  | false
+            false | true
+          end
+
+          before do
+            project.ci_cd_settings.update_attribute(column, prev_value)
+          end
+
+          with_them do
+            it 'creates an audit event' do
+              project.ci_cd_settings.update_attribute(column, new_value)
+
+              expect { project_ci_cd_setting_changes_auditor.execute }.to change { AuditEvent.count }.by(1)
+              expect(AuditEvent.last.details).to include({
+                                                           change: column,
+                                                           from: prev_value,
+                                                           to: new_value
+                                                         })
+            end
+          end
+        end
+
+        context 'when column changes to false from nil' do
+          before do
+            project.ci_cd_settings.update_attribute(column, nil)
+          end
+
+          it 'does not create an audit event' do
+            project.ci_cd_settings.update_attribute(column, false)
+
+            expect { project_ci_cd_setting_changes_auditor.execute }.not_to change { AuditEvent.count }
+          end
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe EE::Audit::ProjectSettingChangesAuditor do
+  using RSpec::Parameterized::TableSyntax
+  describe '#execute' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:project) { create(:project) }
+    let_it_be(:project_setting_changes_auditor) { described_class.new(user, project.project_setting, project) }
+
+    before do
+      stub_licensed_features(extended_audit_events: true)
+    end
+
+    context 'when project setting is updated' do
+      options = ProjectSetting.squash_options.keys
+      options.each do |prev_value|
+        options.each do |new_value|
+          context 'when squash option is changed' do
+            before do
+              project.project_setting.update!(squash_option: prev_value)
+            end
+
+            next if new_value == prev_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
+                                                         })
+            end
+          end
+        end
+      end
+
+      context 'when allow_merge_on_skipped_pipeline is changed' do
+        where(:prev_value, :new_value) do
+          true  | false
+          false | true
+        end
+
+        before do
+          project.project_setting.update!(allow_merge_on_skipped_pipeline: prev_value)
+        end
+
+        with_them do
+          it 'creates an audit event' do
+            project.project_setting.update!(allow_merge_on_skipped_pipeline: new_value)
+
+            expect { project_setting_changes_auditor.execute }.to change { AuditEvent.count }.by(1)
+            expect(AuditEvent.last.details).to include({
+                                                         change: 'allow_merge_on_skipped_pipeline',
+                                                         from: prev_value,
+                                                         to: new_value
+                                                       })
+          end
+        end
+      end
+    end
+  end
+end

locale/gitlab.pot

@@ -7038,6 +7038,9 @@ msgstr ""
 msgid "Changed assignee(s)."
 msgstr ""
 
+msgid "Changed merge method to %{merge_method}"
+msgstr ""
+
 msgid "Changed reviewer(s)."
 msgstr ""