From 859d2a8b552fc6b7a424ee9a0654b064dd4c9a73 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Thu, 10 Mar 2022 16:39:15 +0530 Subject: [PATCH 1/4] Add audit events for merge request settings Changelog: added EE: true --- ee/lib/ee/audit/project_changes_auditor.rb | 32 +++++++ .../project_ci_cd_setting_changes_auditor.rb | 27 ++++++ .../audit/project_setting_changes_auditor.rb | 28 ++++++ .../ee/audit/project_changes_auditor_spec.rb | 85 +++++++++++++++++++ ...ject_ci_cd_setting_changes_auditor_spec.rb | 43 ++++++++++ .../project_setting_changes_auditor_spec.rb | 64 ++++++++++++++ locale/gitlab.pot | 3 + 7 files changed, 282 insertions(+) create mode 100644 ee/lib/ee/audit/project_ci_cd_setting_changes_auditor.rb create mode 100644 ee/lib/ee/audit/project_setting_changes_auditor.rb create mode 100644 ee/spec/lib/ee/audit/project_ci_cd_setting_changes_auditor_spec.rb create mode 100644 ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb diff --git a/ee/lib/ee/audit/project_changes_auditor.rb b/ee/lib/ee/audit/project_changes_auditor.rb index 39f31a254b5ab7..d05d991d7ffd64 100644 --- a/ee/lib/ee/audit/project_changes_auditor.rb +++ b/ee/lib/ee/audit/project_changes_auditor.rb @@ -17,12 +17,36 @@ def execute 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) + + 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 @@ def audit_compliance_framework_changes ::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 diff --git a/ee/lib/ee/audit/project_ci_cd_setting_changes_auditor.rb b/ee/lib/ee/audit/project_ci_cd_setting_changes_auditor.rb new file mode 100644 index 00000000000000..03deb35262ae9d --- /dev/null +++ b/ee/lib/ee/audit/project_ci_cd_setting_changes_auditor.rb @@ -0,0 +1,27 @@ +# 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.nil? + + audit_changes(:merge_pipelines_enabled, as: 'merge_pipelines_enabled', entity: @project, model: model) + audit_changes(:merge_trains_enabled, as: 'merge_trains_enabled', 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 diff --git a/ee/lib/ee/audit/project_setting_changes_auditor.rb b/ee/lib/ee/audit/project_setting_changes_auditor.rb new file mode 100644 index 00000000000000..ae9648b63e6284 --- /dev/null +++ b/ee/lib/ee/audit/project_setting_changes_auditor.rb @@ -0,0 +1,28 @@ +# 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.nil? + + 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 diff --git a/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb index a0bbd54e02dcbb..c1e4c0a08658ba 100644 --- a/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb +++ b/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,90 @@ ) 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 + + 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 diff --git a/ee/spec/lib/ee/audit/project_ci_cd_setting_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/project_ci_cd_setting_changes_auditor_spec.rb new file mode 100644 index 00000000000000..ea21df6b6636dc --- /dev/null +++ b/ee/spec/lib/ee/audit/project_ci_cd_setting_changes_auditor_spec.rb @@ -0,0 +1,43 @@ +# 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(:foo_instance) { 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| + 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 { 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 + end +end diff --git a/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb new file mode 100644 index 00000000000000..65151d6faa3980 --- /dev/null +++ b/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb @@ -0,0 +1,64 @@ +# 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(:foo_instance) { 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 { foo_instance.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 { foo_instance.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 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 3c1ada932471ed..c130d99a3128d6 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -7026,6 +7026,9 @@ msgstr "" msgid "Changed assignee(s)." msgstr "" +msgid "Changed merge method to %{merge_method}" +msgstr "" + msgid "Changed reviewer(s)." msgstr "" -- GitLab From e6f7a15c9d1418afe783fae56880529e5689bf14 Mon Sep 17 00:00:00 2001 From: Huzaifa Iftikhar Date: Wed, 6 Apr 2022 08:53:37 +0000 Subject: [PATCH 2/4] Apply 10 suggestion(s) to 5 file(s) --- .../project_ci_cd_setting_changes_auditor.rb | 2 +- .../ee/audit/project_setting_changes_auditor.rb | 2 +- .../lib/ee/audit/project_changes_auditor_spec.rb | 16 ++++++++++------ ...project_ci_cd_setting_changes_auditor_spec.rb | 7 ++++--- .../project_setting_changes_auditor_spec.rb | 6 +++--- 5 files changed, 19 insertions(+), 14 deletions(-) diff --git a/ee/lib/ee/audit/project_ci_cd_setting_changes_auditor.rb b/ee/lib/ee/audit/project_ci_cd_setting_changes_auditor.rb index 03deb35262ae9d..e8a2d864f69f31 100644 --- a/ee/lib/ee/audit/project_ci_cd_setting_changes_auditor.rb +++ b/ee/lib/ee/audit/project_ci_cd_setting_changes_auditor.rb @@ -9,7 +9,7 @@ def initialize(current_user, ci_cd_settings, project) end def execute - return if model.nil? + return if model.blank? audit_changes(:merge_pipelines_enabled, as: 'merge_pipelines_enabled', entity: @project, model: model) audit_changes(:merge_trains_enabled, as: 'merge_trains_enabled', entity: @project, model: model) diff --git a/ee/lib/ee/audit/project_setting_changes_auditor.rb b/ee/lib/ee/audit/project_setting_changes_auditor.rb index ae9648b63e6284..65a06595ba22e7 100644 --- a/ee/lib/ee/audit/project_setting_changes_auditor.rb +++ b/ee/lib/ee/audit/project_setting_changes_auditor.rb @@ -9,7 +9,7 @@ def initialize(current_user, project_setting, project) end def execute - return if model.nil? + 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, diff --git a/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb index c1e4c0a08658ba..926a8fb4ae7a94 100644 --- a/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb @@ -154,7 +154,7 @@ only_allow_merge_if_all_discussions_are_resolved] columns.each do |column| where(:prev_value, :new_value) do - true | false + true | false false | true end @@ -165,6 +165,7 @@ 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, @@ -180,6 +181,7 @@ 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', @@ -190,9 +192,9 @@ 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' + true | true | 'Fast-forward' + true | false | 'Fast-forward' + false | true | 'Rebase merge' end before do @@ -202,6 +204,7 @@ 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}" @@ -212,8 +215,8 @@ context 'when merge method is changed to Merge' do where(:ff, :rebase) do - true | true - true | false + true | true + true | false false | true end @@ -224,6 +227,7 @@ 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" diff --git a/ee/spec/lib/ee/audit/project_ci_cd_setting_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/project_ci_cd_setting_changes_auditor_spec.rb index ea21df6b6636dc..d484c16e98c4dd 100644 --- a/ee/spec/lib/ee/audit/project_ci_cd_setting_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/project_ci_cd_setting_changes_auditor_spec.rb @@ -8,7 +8,7 @@ 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(:foo_instance) { described_class.new(user, ci_cd_settings, project) } + 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) @@ -18,7 +18,7 @@ columns = %w[merge_trains_enabled merge_pipelines_enabled] columns.each do |column| where(:prev_value, :new_value) do - true | false + true | false false | true end @@ -29,7 +29,8 @@ with_them do it 'creates an audit event' do project.ci_cd_settings.update_attribute(column, new_value) - expect { foo_instance.execute }.to change { AuditEvent.count }.by(1) + + 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, diff --git a/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb index 65151d6faa3980..d571f2ab2b521f 100644 --- a/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb @@ -7,7 +7,7 @@ describe '#execute' do let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project) } - let_it_be(:foo_instance) { described_class.new(user, project.project_setting, 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) @@ -26,7 +26,7 @@ it 'creates an audit event' do project.project_setting.update!(squash_option: new_value) - expect { foo_instance.execute }.to change { AuditEvent.count }.by(1) + expect { project_setting_changes_auditor.execute }.to change { AuditEvent.count }.by(1) expect(AuditEvent.last.details).to include({ change: 'squash_option', from: prev_value, @@ -50,7 +50,7 @@ with_them do it 'creates an audit event' do project.project_setting.update!(allow_merge_on_skipped_pipeline: new_value) - expect { foo_instance.execute }.to change { AuditEvent.count }.by(1) + 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, -- GitLab From 7b03a8e9f34ecc5ab8196357167c4fb14dc9e4cd Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Thu, 7 Apr 2022 14:55:49 +0530 Subject: [PATCH 3/4] Disable audit on column change from nil to false/blank --- ee/lib/ee/audit/base_changes_auditor.rb | 12 +++++ ee/lib/ee/audit/project_changes_auditor.rb | 2 +- .../project_ci_cd_setting_changes_auditor.rb | 11 ++++- .../ee/audit/project_changes_auditor_spec.rb | 5 +++ ...ject_ci_cd_setting_changes_auditor_spec.rb | 44 ++++++++++++------- 5 files changed, 56 insertions(+), 18 deletions(-) diff --git a/ee/lib/ee/audit/base_changes_auditor.rb b/ee/lib/ee/audit/base_changes_auditor.rb index 5753a59de0fc57..9a7a92bc9f03a9 100644 --- a/ee/lib/ee/audit/base_changes_auditor.rb +++ b/ee/lib/ee/audit/base_changes_auditor.rb @@ -17,6 +17,18 @@ def parse_options(column, options) def attributes_from_auditable_model(column) raise NotImplementedError end + + # disables auditing when there is no user intended change + # for example: when project update API call changes columns from nil to false/blank empty string + 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 + return false if from.blank? && to.blank? + + true + end end end end diff --git a/ee/lib/ee/audit/project_changes_auditor.rb b/ee/lib/ee/audit/project_changes_auditor.rb index d05d991d7ffd64..83041a5489f996 100644 --- a/ee/lib/ee/audit/project_changes_auditor.rb +++ b/ee/lib/ee/audit/project_changes_auditor.rb @@ -22,7 +22,7 @@ def execute 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) + audit_changes(:suggestion_commit_message, as: 'suggestion_commit_message', model: model) if should_audit? :suggestion_commit_message audit_merge_method audit_project_feature_changes diff --git a/ee/lib/ee/audit/project_ci_cd_setting_changes_auditor.rb b/ee/lib/ee/audit/project_ci_cd_setting_changes_auditor.rb index e8a2d864f69f31..aa8faf13f58b54 100644 --- a/ee/lib/ee/audit/project_ci_cd_setting_changes_auditor.rb +++ b/ee/lib/ee/audit/project_ci_cd_setting_changes_auditor.rb @@ -11,8 +11,15 @@ def initialize(current_user, ci_cd_settings, project) def execute return if model.blank? - audit_changes(:merge_pipelines_enabled, as: 'merge_pipelines_enabled', entity: @project, model: model) - audit_changes(:merge_trains_enabled, as: 'merge_trains_enabled', entity: @project, model: model) + 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) diff --git a/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb index 926a8fb4ae7a94..87a4d2b406ec89 100644 --- a/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb @@ -190,6 +190,11 @@ }) 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' diff --git a/ee/spec/lib/ee/audit/project_ci_cd_setting_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/project_ci_cd_setting_changes_auditor_spec.rb index d484c16e98c4dd..70a3c5f12998d7 100644 --- a/ee/spec/lib/ee/audit/project_ci_cd_setting_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/project_ci_cd_setting_changes_auditor_spec.rb @@ -17,25 +17,39 @@ context 'when auditable boolean column is changed' do columns = %w[merge_trains_enabled merge_pipelines_enabled] columns.each do |column| - where(:prev_value, :new_value) do - true | false - false | true - end + 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 - before do - project.ci_cd_settings.update_attribute(column, prev_value) + 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 - with_them do - it 'creates an audit event' do - project.ci_cd_settings.update_attribute(column, new_value) + 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 }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details).to include({ - change: column, - from: prev_value, - to: new_value - }) + expect { project_ci_cd_setting_changes_auditor.execute }.not_to change { AuditEvent.count } end end end -- GitLab From 3207a4152a4706b21a01b15c7d3933b627c8de20 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Thu, 7 Apr 2022 17:45:15 +0530 Subject: [PATCH 4/4] Move blank to blank logic in auditor --- ee/lib/audit/changes.rb | 8 +++++++- ee/lib/ee/audit/base_changes_auditor.rb | 12 ------------ ee/lib/ee/audit/project_changes_auditor.rb | 2 +- .../audit/project_ci_cd_setting_changes_auditor.rb | 11 ++--------- ee/spec/lib/ee/audit/project_changes_auditor_spec.rb | 1 + .../ee/audit/project_setting_changes_auditor_spec.rb | 4 +++- 6 files changed, 14 insertions(+), 24 deletions(-) diff --git a/ee/lib/audit/changes.rb b/ee/lib/audit/changes.rb index fc6ff2937e9a0b..640f11b1a66e43 100644 --- a/ee/lib/audit/changes.rb +++ b/ee/lib/audit/changes.rb @@ -46,7 +46,13 @@ def not_recently_created? end def changed?(column) - model.previous_changes.has_key?(column) + return false unless model.previous_changes.has_key?(column) + + from = model.previous_changes[column].first + to = model.previous_changes[column].second + return false if from.blank? && to.blank? + + true end def changes(column) diff --git a/ee/lib/ee/audit/base_changes_auditor.rb b/ee/lib/ee/audit/base_changes_auditor.rb index 9a7a92bc9f03a9..5753a59de0fc57 100644 --- a/ee/lib/ee/audit/base_changes_auditor.rb +++ b/ee/lib/ee/audit/base_changes_auditor.rb @@ -17,18 +17,6 @@ def parse_options(column, options) def attributes_from_auditable_model(column) raise NotImplementedError end - - # disables auditing when there is no user intended change - # for example: when project update API call changes columns from nil to false/blank empty string - 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 - return false if from.blank? && to.blank? - - true - end end end end diff --git a/ee/lib/ee/audit/project_changes_auditor.rb b/ee/lib/ee/audit/project_changes_auditor.rb index 83041a5489f996..d05d991d7ffd64 100644 --- a/ee/lib/ee/audit/project_changes_auditor.rb +++ b/ee/lib/ee/audit/project_changes_auditor.rb @@ -22,7 +22,7 @@ def execute 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_changes(:suggestion_commit_message, as: 'suggestion_commit_message', model: model) audit_merge_method audit_project_feature_changes diff --git a/ee/lib/ee/audit/project_ci_cd_setting_changes_auditor.rb b/ee/lib/ee/audit/project_ci_cd_setting_changes_auditor.rb index aa8faf13f58b54..e8a2d864f69f31 100644 --- a/ee/lib/ee/audit/project_ci_cd_setting_changes_auditor.rb +++ b/ee/lib/ee/audit/project_ci_cd_setting_changes_auditor.rb @@ -11,15 +11,8 @@ def initialize(current_user, ci_cd_settings, project) 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 + audit_changes(:merge_pipelines_enabled, as: 'merge_pipelines_enabled', entity: @project, model: model) + audit_changes(:merge_trains_enabled, as: 'merge_trains_enabled', entity: @project, model: model) end def attributes_from_auditable_model(column) diff --git a/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb index 87a4d2b406ec89..32b37188d672f9 100644 --- a/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/project_changes_auditor_spec.rb @@ -192,6 +192,7 @@ 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 diff --git a/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb index d571f2ab2b521f..fe6af59faf22af 100644 --- a/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/project_setting_changes_auditor_spec.rb @@ -26,6 +26,7 @@ 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', @@ -39,7 +40,7 @@ context 'when allow_merge_on_skipped_pipeline is changed' do where(:prev_value, :new_value) do - true | false + true | false false | true end @@ -50,6 +51,7 @@ 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', -- GitLab