From 2c579d7f4c26d60597ccd261eb1014b2a9795c15 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Wed, 18 May 2022 14:50:17 +0530 Subject: [PATCH 01/14] Audit event for group level merge request settings EE: true Changelog: added --- .../group_merge_request_approval_setting.rb | 7 +++ .../update_service.rb | 5 +++ ...equest_approval_setting_changes_auditor.rb | 33 ++++++++++++++ ...t_approval_setting_changes_auditor_spec.rb | 45 +++++++++++++++++++ .../update_service_spec.rb | 13 ++++++ 5 files changed, 103 insertions(+) create mode 100644 ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb create mode 100644 ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb diff --git a/ee/app/models/group_merge_request_approval_setting.rb b/ee/app/models/group_merge_request_approval_setting.rb index b4277507f7fd8c..19eaf11df7ee6c 100644 --- a/ee/app/models/group_merge_request_approval_setting.rb +++ b/ee/app/models/group_merge_request_approval_setting.rb @@ -14,4 +14,11 @@ class GroupMergeRequestApprovalSetting < ApplicationRecord inclusion: { in: [true, false], message: _('must be a boolean value') } scope :find_or_initialize_by_group, ->(group) { find_or_initialize_by(group: group) } + + AUDIT_LOG_ALLOWLIST = { allow_author_approval: 'prevent merge request approval from authors', + allow_committer_approval: 'prevent merge request approval from committers', + allow_overrides_to_approver_list_per_merge_request: + 'prevent users from modifying MR approval rules in merge requests', + retain_approvals_on_push: 'require new approvals when new commits are added to an MR', + require_password_to_approve: 'require user password for approvals' }.freeze end diff --git a/ee/app/services/merge_request_approval_settings/update_service.rb b/ee/app/services/merge_request_approval_settings/update_service.rb index 7cf1e0a05cfac1..897f88927f834f 100644 --- a/ee/app/services/merge_request_approval_settings/update_service.rb +++ b/ee/app/services/merge_request_approval_settings/update_service.rb @@ -10,6 +10,7 @@ def execute setting.assign_attributes(params) if setting.save + log_audit_event(setting) ServiceResponse.success(payload: setting) else ServiceResponse.error(message: setting.errors.messages) @@ -37,5 +38,9 @@ def execute def allowed? can?(current_user, :admin_merge_request_approval_settings, container) end + + def log_audit_event(setting) + Audit::GroupMergeRequestApprovalSettingChangesAuditor.new(current_user, setting).execute + end end end diff --git a/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb b/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb new file mode 100644 index 00000000000000..7fa9cbbbace066 --- /dev/null +++ b/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Audit + class GroupMergeRequestApprovalSettingChangesAuditor < ::EE::Audit::BaseChangesAuditor + def initialize(current_user, approval_setting) + @group = approval_setting.group + + super + end + + def execute + ::GroupMergeRequestApprovalSetting::AUDIT_LOG_ALLOWLIST.each do |column, desc| + audit_changes(column, as: desc, entity: @group, model: model) + end + end + + private + + def attributes_from_auditable_model(column) + if column == :require_password_to_approve + { + from: model.previous_changes[column].first, + to: model.previous_changes[column].last + } + else + { + from: !model.previous_changes[column].first, + to: !model.previous_changes[column].last + } + end.merge(target_details: @group.full_path) + end + end +end diff --git a/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb new file mode 100644 index 00000000000000..898f2b74b1b8e4 --- /dev/null +++ b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Audit::GroupMergeRequestApprovalSettingChangesAuditor do + describe 'auditing group merge request approval setting changes' do + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be(:approval_setting) do + create(:group_merge_request_approval_setting, + group: group, + allow_author_approval: false, + allow_committer_approval: false, + allow_overrides_to_approver_list_per_merge_request: false, + retain_approvals_on_push: false, + require_password_to_approve: false) + end + + let(:subject) { described_class.new(user, approval_setting) } + + context 'when audit change happens' do + ::GroupMergeRequestApprovalSetting::AUDIT_LOG_ALLOWLIST.each do |column, desc| + it 'creates an audit event' do + approval_setting.update_attribute(column, true) + + expect { subject.execute }.to change { AuditEvent.count }.by(1) + + if column == :require_password_to_approve + expect(AuditEvent.last.details).to include({ + change: desc, + from: false, + to: true + }) + else + expect(AuditEvent.last.details).to include({ + change: desc, + from: true, + to: false + }) + end + end + end + end + end +end diff --git a/ee/spec/services/merge_request_approval_settings/update_service_spec.rb b/ee/spec/services/merge_request_approval_settings/update_service_spec.rb index 9a63d4f662d2f6..921c888f7739a4 100644 --- a/ee/spec/services/merge_request_approval_settings/update_service_spec.rb +++ b/ee/spec/services/merge_request_approval_settings/update_service_spec.rb @@ -55,6 +55,15 @@ describe 'execute with a Group as container' do let(:container) { group } + shared_context 'call audit changes service' do + it 'executes GroupMergeRequestApprovalSettingChangesAuditor' do + expect(Audit::GroupMergeRequestApprovalSettingChangesAuditor).to receive(:new).with(user, + instance_of(GroupMergeRequestApprovalSetting)).and_call_original + + subject.execute + end + end + context 'user does not have permissions' do before do allow(service).to receive(:can?).with(user, :admin_merge_request_approval_settings, group).and_return(false) @@ -86,6 +95,8 @@ expect(response.payload.allow_author_approval).to be(false) end + include_context 'call audit changes service' + context 'when group has an existing setting' do let_it_be(:group) { create(:group) } let_it_be(:existing_setting) { create(:group_merge_request_approval_setting, group: group) } @@ -100,6 +111,8 @@ expect(response).to be_success expect(response.payload.allow_author_approval).to be(false) end + + include_context 'call audit changes service' end context 'when saving fails' do -- GitLab From 1365f247fc30b77436bd37d7cbb517cce9fb1514 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Fri, 20 May 2022 16:02:32 +0530 Subject: [PATCH 02/14] Documentation for group level merge request approval audit events --- doc/administration/audit_events.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index 6d7121d32fede8..e9b1a8e807cbc4 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -117,6 +117,12 @@ From there, you can see the following actions: - Failed attempt to create a group deploy token. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/353452) in GitLab 14.9. - [IP restrictions](../user/group/index.md#group-access-restriction-by-ip-address) changed. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/358986) in GitLab 15.0. - Changes to push rules. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/227629) in GitLab 15.0. +- [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/356152) in GitLab 15.1, changes to the following merge request approvals settings: + - Prevent approval by author + - Prevent approvals by users who add commits + - Prevent editing approval rules in projects and merge requests. + - Require user password to approve + - Remove all approvals when commits are added to the source branch Group events can also be accessed via the [Group Audit Events API](../api/audit_events.md#group-audit-events) -- GitLab From 58c45f7758df19d2ce24a35eea637eab19173032 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Fri, 20 May 2022 17:23:56 +0530 Subject: [PATCH 03/14] Fix markdown lint --- doc/administration/audit_events.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index e9b1a8e807cbc4..3915b1bd53ad7b 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -118,11 +118,11 @@ From there, you can see the following actions: - [IP restrictions](../user/group/index.md#group-access-restriction-by-ip-address) changed. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/358986) in GitLab 15.0. - Changes to push rules. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/227629) in GitLab 15.0. - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/356152) in GitLab 15.1, changes to the following merge request approvals settings: - - Prevent approval by author - - Prevent approvals by users who add commits - - Prevent editing approval rules in projects and merge requests. - - Require user password to approve - - Remove all approvals when commits are added to the source branch + - Prevent approval by author + - Prevent approvals by users who add commits + - Prevent editing approval rules in projects and merge requests. + - Require user password to approve + - Remove all approvals when commits are added to the source branch Group events can also be accessed via the [Group Audit Events API](../api/audit_events.md#group-audit-events) -- GitLab From 79d72d12a63c5b2d8b7a356cd1cd363519f87778 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Mon, 23 May 2022 09:32:47 +0530 Subject: [PATCH 04/14] Add period to all the entries --- doc/administration/audit_events.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index 3915b1bd53ad7b..442f743f2c7744 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -118,11 +118,11 @@ From there, you can see the following actions: - [IP restrictions](../user/group/index.md#group-access-restriction-by-ip-address) changed. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/358986) in GitLab 15.0. - Changes to push rules. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/227629) in GitLab 15.0. - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/356152) in GitLab 15.1, changes to the following merge request approvals settings: - - Prevent approval by author - - Prevent approvals by users who add commits + - Prevent approval by author. + - Prevent approvals by users who add commits. - Prevent editing approval rules in projects and merge requests. - - Require user password to approve - - Remove all approvals when commits are added to the source branch + - Require user password to approve. + - Remove all approvals when commits are added to the source branch. Group events can also be accessed via the [Group Audit Events API](../api/audit_events.md#group-audit-events) -- GitLab From af0389ade589473200dab92675f80aefbdbe44cb Mon Sep 17 00:00:00 2001 From: Evan Read Date: Mon, 23 May 2022 04:14:58 +0000 Subject: [PATCH 05/14] Apply 1 suggestion(s) to 1 file(s) --- doc/administration/audit_events.md | 2 +- .../update_service.rb | 2 +- ...equest_approval_setting_changes_auditor.rb | 29 +++++- ...t_approval_setting_changes_auditor_spec.rb | 90 +++++++++++++------ .../update_service_spec.rb | 2 +- 5 files changed, 90 insertions(+), 35 deletions(-) diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index 442f743f2c7744..0e49314d0aad72 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -268,7 +268,7 @@ on adding these events into GitLab: Don't see the event you want in any of the epics linked above? You can either: - Use the **Audit Event Proposal** issue template to - [create an issue](https://gitlab.com/gitlab-org/gitlab/-/issues/new?issuable_template=Audit%20Event%20Proposal) to + [create an issue](https://gitlab.com/gitlab-org/gitlab/-/issues/new?issuable_template=gAudit%20Event%20Proposal) to request it. - [Add it yourself](../development/audit_event_guide/). diff --git a/ee/app/services/merge_request_approval_settings/update_service.rb b/ee/app/services/merge_request_approval_settings/update_service.rb index 897f88927f834f..3b13b4480096fa 100644 --- a/ee/app/services/merge_request_approval_settings/update_service.rb +++ b/ee/app/services/merge_request_approval_settings/update_service.rb @@ -40,7 +40,7 @@ def allowed? end def log_audit_event(setting) - Audit::GroupMergeRequestApprovalSettingChangesAuditor.new(current_user, setting).execute + Audit::GroupMergeRequestApprovalSettingChangesAuditor.new(current_user, setting, params).execute end end end diff --git a/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb b/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb index 7fa9cbbbace066..ec72ecb83f674b 100644 --- a/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb +++ b/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb @@ -2,20 +2,43 @@ module Audit class GroupMergeRequestApprovalSettingChangesAuditor < ::EE::Audit::BaseChangesAuditor - def initialize(current_user, approval_setting) + def initialize(current_user, approval_setting, params) @group = approval_setting.group + @params = params - super + super(current_user, approval_setting) end def execute ::GroupMergeRequestApprovalSetting::AUDIT_LOG_ALLOWLIST.each do |column, desc| - audit_changes(column, as: desc, entity: @group, model: model) + audit_change(column, desc) end end private + def audit_change(column, desc) + if model.previously_new_record? + audit_new_record(column, desc) + else + audit_changes(column, as: desc, entity: @group, model: model) + end + end + + def audit_new_record(column, desc) + return unless @params.include? column + + audit_context = { + name: "update_group_merge_request_approval_setting", + author: @current_user, + scope: @group, + target: @group, + message: "Changed #{desc} from false to true" + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + def attributes_from_auditable_model(column) if column == :require_password_to_approve { diff --git a/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb index 898f2b74b1b8e4..cb9e49dda2ca14 100644 --- a/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb @@ -6,37 +6,69 @@ describe 'auditing group merge request approval setting changes' do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } - let_it_be(:approval_setting) do - create(:group_merge_request_approval_setting, - group: group, - allow_author_approval: false, - allow_committer_approval: false, - allow_overrides_to_approver_list_per_merge_request: false, - retain_approvals_on_push: false, - require_password_to_approve: false) - end - - let(:subject) { described_class.new(user, approval_setting) } context 'when audit change happens' do - ::GroupMergeRequestApprovalSetting::AUDIT_LOG_ALLOWLIST.each do |column, desc| - it 'creates an audit event' do - approval_setting.update_attribute(column, true) - - expect { subject.execute }.to change { AuditEvent.count }.by(1) - - if column == :require_password_to_approve - expect(AuditEvent.last.details).to include({ - change: desc, - from: false, - to: true - }) - else - expect(AuditEvent.last.details).to include({ - change: desc, - from: true, - to: false - }) + context 'when group_merge_request_approval_setting is created' do + let_it_be(:approval_setting) do + create(:group_merge_request_approval_setting, + group: group) + end + + params = { allow_author_approval: true, + allow_committer_approval: true, + allow_overrides_to_approver_list_per_merge_request: true, + retain_approvals_on_push: true, + require_password_to_approve: true } + let_it_be(:subject) { described_class.new(user, approval_setting, params) } + + before do + allow_next_instance_of(::GroupMergeRequestApprovalSetting) do |instance| + allow(instance).to receive(:previously_new_record?).and_return(true) + end + end + it 'creates audit events' do + expect { subject.execute }.to change { AuditEvent.count }.by(5) + expect(AuditEvent.last(5).map { |e| e.details[:custom_message] }) + .to match_array ["Changed prevent merge request approval from committers from false to true", + "Changed prevent users from modifying MR approval rules in merge requests from false to true", # rubocop:disable Layout/LineLength + "Changed prevent merge request approval from authors from false to true", + "Changed require new approvals when new commits are added to an MR from false to true", + "Changed require user password for approvals from false to true"] + end + end + + context 'when group_merge_request_approval_setting is updated' do + let_it_be(:approval_setting) do + create(:group_merge_request_approval_setting, + group: group, + allow_author_approval: false, + allow_committer_approval: false, + allow_overrides_to_approver_list_per_merge_request: false, + retain_approvals_on_push: false, + require_password_to_approve: false) + end + + let_it_be(:subject) { described_class.new(user, approval_setting, {}) } + + ::GroupMergeRequestApprovalSetting::AUDIT_LOG_ALLOWLIST.each do |column, desc| + it 'creates an audit event' do + approval_setting.update_attribute(column, true) + + expect { subject.execute }.to change { AuditEvent.count }.by(1) + + if column == :require_password_to_approve + expect(AuditEvent.last.details).to include({ + change: desc, + from: false, + to: true + }) + else + expect(AuditEvent.last.details).to include({ + change: desc, + from: true, + to: false + }) + end end end end diff --git a/ee/spec/services/merge_request_approval_settings/update_service_spec.rb b/ee/spec/services/merge_request_approval_settings/update_service_spec.rb index 921c888f7739a4..7bcec448afce2f 100644 --- a/ee/spec/services/merge_request_approval_settings/update_service_spec.rb +++ b/ee/spec/services/merge_request_approval_settings/update_service_spec.rb @@ -58,7 +58,7 @@ shared_context 'call audit changes service' do it 'executes GroupMergeRequestApprovalSettingChangesAuditor' do expect(Audit::GroupMergeRequestApprovalSettingChangesAuditor).to receive(:new).with(user, - instance_of(GroupMergeRequestApprovalSetting)).and_call_original + instance_of(GroupMergeRequestApprovalSetting), params).and_call_original subject.execute end -- GitLab From 05e0038c0003fe4f8f3d2c5e0704e360ae7c0ada Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Wed, 8 Jun 2022 12:19:11 +0530 Subject: [PATCH 06/14] Remove extra letter from doc --- doc/administration/audit_events.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index 0e49314d0aad72..442f743f2c7744 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -268,7 +268,7 @@ on adding these events into GitLab: Don't see the event you want in any of the epics linked above? You can either: - Use the **Audit Event Proposal** issue template to - [create an issue](https://gitlab.com/gitlab-org/gitlab/-/issues/new?issuable_template=gAudit%20Event%20Proposal) to + [create an issue](https://gitlab.com/gitlab-org/gitlab/-/issues/new?issuable_template=Audit%20Event%20Proposal) to request it. - [Add it yourself](../development/audit_event_guide/). -- GitLab From de877ff0efe1b65fa990ab3a79c2ee28fd95dae4 Mon Sep 17 00:00:00 2001 From: Huzaifa Iftikhar Date: Sun, 12 Jun 2022 17:37:06 +0000 Subject: [PATCH 07/14] Apply 3 suggestion(s) to 1 file(s) --- ...quest_approval_setting_changes_auditor_spec.rb | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb index cb9e49dda2ca14..a7b0920cdb213e 100644 --- a/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb @@ -30,7 +30,8 @@ expect { subject.execute }.to change { AuditEvent.count }.by(5) expect(AuditEvent.last(5).map { |e| e.details[:custom_message] }) .to match_array ["Changed prevent merge request approval from committers from false to true", - "Changed prevent users from modifying MR approval rules in merge requests from false to true", # rubocop:disable Layout/LineLength + "Changed prevent users from modifying MR approval rules in merge requests "\ + "from false to true", "Changed prevent merge request approval from authors from false to true", "Changed require new approvals when new commits are added to an MR from false to true", "Changed require user password for approvals from false to true"] @@ -57,17 +58,9 @@ expect { subject.execute }.to change { AuditEvent.count }.by(1) if column == :require_password_to_approve - expect(AuditEvent.last.details).to include({ - change: desc, - from: false, - to: true - }) + expect(AuditEvent.last.details).to include({ change: desc, from: false, to: true }) else - expect(AuditEvent.last.details).to include({ - change: desc, - from: true, - to: false - }) + expect(AuditEvent.last.details).to include({ change: desc, from: true, to: false }) end end end -- GitLab From e2824b2b8177d4b8637f9359855905ef602474bf Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Sun, 12 Jun 2022 23:18:30 +0530 Subject: [PATCH 08/14] Apply review suggestions --- ...equest_approval_setting_changes_auditor.rb | 7 +- ...t_approval_setting_changes_auditor_spec.rb | 98 +++++++++---------- .../update_service_spec.rb | 6 +- 3 files changed, 55 insertions(+), 56 deletions(-) diff --git a/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb b/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb index ec72ecb83f674b..ff1be82411f49b 100644 --- a/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb +++ b/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb @@ -10,8 +10,8 @@ def initialize(current_user, approval_setting, params) end def execute - ::GroupMergeRequestApprovalSetting::AUDIT_LOG_ALLOWLIST.each do |column, desc| - audit_change(column, desc) + ::GroupMergeRequestApprovalSetting::AUDIT_LOG_ALLOWLIST.each do |column, description| + audit_change(column, description) end end @@ -46,6 +46,9 @@ def attributes_from_auditable_model(column) to: model.previous_changes[column].last } else + # we are negating here because descriptions shown in UI and audit events + # are opposite of column meanings for example + # allow_author_approval column has description 'prevent merge request approval from authors'. { from: !model.previous_changes[column].first, to: !model.previous_changes[column].last diff --git a/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb index a7b0920cdb213e..65d1b73f5900fd 100644 --- a/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb @@ -3,66 +3,62 @@ require 'spec_helper' RSpec.describe Audit::GroupMergeRequestApprovalSettingChangesAuditor do - describe 'auditing group merge request approval setting changes' do - let_it_be(:user) { create(:user) } - let_it_be(:group) { create(:group) } + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } - context 'when audit change happens' do - context 'when group_merge_request_approval_setting is created' do - let_it_be(:approval_setting) do - create(:group_merge_request_approval_setting, - group: group) - end + context 'when group_merge_request_approval_setting is created' do + let_it_be(:approval_setting) do + create(:group_merge_request_approval_setting, + group: group) + end - params = { allow_author_approval: true, - allow_committer_approval: true, - allow_overrides_to_approver_list_per_merge_request: true, - retain_approvals_on_push: true, - require_password_to_approve: true } - let_it_be(:subject) { described_class.new(user, approval_setting, params) } + params = { allow_author_approval: true, + allow_committer_approval: true, + allow_overrides_to_approver_list_per_merge_request: true, + retain_approvals_on_push: true, + require_password_to_approve: true } + let_it_be(:subject) { described_class.new(user, approval_setting, params) } - before do - allow_next_instance_of(::GroupMergeRequestApprovalSetting) do |instance| - allow(instance).to receive(:previously_new_record?).and_return(true) - end - end - it 'creates audit events' do - expect { subject.execute }.to change { AuditEvent.count }.by(5) - expect(AuditEvent.last(5).map { |e| e.details[:custom_message] }) - .to match_array ["Changed prevent merge request approval from committers from false to true", - "Changed prevent users from modifying MR approval rules in merge requests "\ - "from false to true", - "Changed prevent merge request approval from authors from false to true", - "Changed require new approvals when new commits are added to an MR from false to true", - "Changed require user password for approvals from false to true"] - end + before do + allow_next_instance_of(::GroupMergeRequestApprovalSetting) do |instance| + allow(instance).to receive(:previously_new_record?).and_return(true) end + end + it 'creates audit events' do + expect { subject.execute }.to change { AuditEvent.count }.by(5) + expect(AuditEvent.last(5).map { |e| e.details[:custom_message] }) + .to match_array ["Changed prevent merge request approval from committers from false to true", + "Changed prevent users from modifying MR approval rules in merge requests "\ + "from false to true", + "Changed prevent merge request approval from authors from false to true", + "Changed require new approvals when new commits are added to an MR from false to true", + "Changed require user password for approvals from false to true"] + end + end - context 'when group_merge_request_approval_setting is updated' do - let_it_be(:approval_setting) do - create(:group_merge_request_approval_setting, - group: group, - allow_author_approval: false, - allow_committer_approval: false, - allow_overrides_to_approver_list_per_merge_request: false, - retain_approvals_on_push: false, - require_password_to_approve: false) - end + context 'when group_merge_request_approval_setting is updated' do + let_it_be(:approval_setting) do + create(:group_merge_request_approval_setting, + group: group, + allow_author_approval: false, + allow_committer_approval: false, + allow_overrides_to_approver_list_per_merge_request: false, + retain_approvals_on_push: false, + require_password_to_approve: false) + end - let_it_be(:subject) { described_class.new(user, approval_setting, {}) } + let_it_be(:subject) { described_class.new(user, approval_setting, {}) } - ::GroupMergeRequestApprovalSetting::AUDIT_LOG_ALLOWLIST.each do |column, desc| - it 'creates an audit event' do - approval_setting.update_attribute(column, true) + ::GroupMergeRequestApprovalSetting::AUDIT_LOG_ALLOWLIST.each do |column, desc| + it 'creates an audit event' do + approval_setting.update_attribute(column, true) - expect { subject.execute }.to change { AuditEvent.count }.by(1) + expect { subject.execute }.to change { AuditEvent.count }.by(1) - if column == :require_password_to_approve - expect(AuditEvent.last.details).to include({ change: desc, from: false, to: true }) - else - expect(AuditEvent.last.details).to include({ change: desc, from: true, to: false }) - end - end + if column == :require_password_to_approve + expect(AuditEvent.last.details).to include({ change: desc, from: false, to: true }) + else + expect(AuditEvent.last.details).to include({ change: desc, from: true, to: false }) end end end diff --git a/ee/spec/services/merge_request_approval_settings/update_service_spec.rb b/ee/spec/services/merge_request_approval_settings/update_service_spec.rb index 7bcec448afce2f..28178025d0643c 100644 --- a/ee/spec/services/merge_request_approval_settings/update_service_spec.rb +++ b/ee/spec/services/merge_request_approval_settings/update_service_spec.rb @@ -55,7 +55,7 @@ describe 'execute with a Group as container' do let(:container) { group } - shared_context 'call audit changes service' do + shared_examples 'call audit changes service' do it 'executes GroupMergeRequestApprovalSettingChangesAuditor' do expect(Audit::GroupMergeRequestApprovalSettingChangesAuditor).to receive(:new).with(user, instance_of(GroupMergeRequestApprovalSetting), params).and_call_original @@ -95,7 +95,7 @@ expect(response.payload.allow_author_approval).to be(false) end - include_context 'call audit changes service' + it_behaves_like 'call audit changes service' context 'when group has an existing setting' do let_it_be(:group) { create(:group) } @@ -112,7 +112,7 @@ expect(response.payload.allow_author_approval).to be(false) end - include_context 'call audit changes service' + it_behaves_like 'call audit changes service' end context 'when saving fails' do -- GitLab From 1b1d57fb9568c86d9a7112be1160adce62cf24a1 Mon Sep 17 00:00:00 2001 From: Huzaifa Iftikhar Date: Mon, 13 Jun 2022 09:09:39 +0000 Subject: [PATCH 09/14] Apply 2 suggestion(s) to 1 file(s) --- .../group_merge_request_approval_setting_changes_auditor.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb b/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb index ff1be82411f49b..31ebdd6267004b 100644 --- a/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb +++ b/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb @@ -47,8 +47,8 @@ def attributes_from_auditable_model(column) } else # we are negating here because descriptions shown in UI and audit events - # are opposite of column meanings for example - # allow_author_approval column has description 'prevent merge request approval from authors'. + # are opposite of column names. For example + # `allow_author_approval` column has description 'prevent merge request approval from authors'. { from: !model.previous_changes[column].first, to: !model.previous_changes[column].last -- GitLab From c7055b229e403883a1a5cd2117005bfb32b2b6d2 Mon Sep 17 00:00:00 2001 From: Huzaifa Iftikhar Date: Mon, 13 Jun 2022 09:09:44 +0000 Subject: [PATCH 10/14] Apply 1 suggestion(s) to 1 file(s) --- .../group_merge_request_approval_setting_changes_auditor.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb b/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb index 31ebdd6267004b..48a914eced3bd5 100644 --- a/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb +++ b/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb @@ -46,7 +46,7 @@ def attributes_from_auditable_model(column) to: model.previous_changes[column].last } else - # we are negating here because descriptions shown in UI and audit events + # we are negating here because descriptions shown in the UI and audit events # are opposite of column names. For example # `allow_author_approval` column has description 'prevent merge request approval from authors'. { -- GitLab From f5efb5809c038fb569211b402559f93f39377f42 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Mon, 13 Jun 2022 15:08:36 +0530 Subject: [PATCH 11/14] Rename desc to description --- ...p_merge_request_approval_setting_changes_auditor.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb b/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb index 48a914eced3bd5..43f6a42d904d99 100644 --- a/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb +++ b/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb @@ -17,15 +17,15 @@ def execute private - def audit_change(column, desc) + def audit_change(column, description) if model.previously_new_record? - audit_new_record(column, desc) + audit_new_record(column, description) else - audit_changes(column, as: desc, entity: @group, model: model) + audit_changes(column, as: description, entity: @group, model: model) end end - def audit_new_record(column, desc) + def audit_new_record(column, description) return unless @params.include? column audit_context = { @@ -33,7 +33,7 @@ def audit_new_record(column, desc) author: @current_user, scope: @group, target: @group, - message: "Changed #{desc} from false to true" + message: "Changed #{description} from false to true" } ::Gitlab::Audit::Auditor.audit(audit_context) -- GitLab From 781835571fa568d329ecd4ec3f74cb05c9cc21d5 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Tue, 14 Jun 2022 09:57:18 +0000 Subject: [PATCH 12/14] Apply 1 suggestion(s) to 1 file(s) --- ...group_merge_request_approval_setting_changes_auditor_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb index 65d1b73f5900fd..7bf43a2071dfd4 100644 --- a/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb @@ -17,7 +17,7 @@ allow_overrides_to_approver_list_per_merge_request: true, retain_approvals_on_push: true, require_password_to_approve: true } - let_it_be(:subject) { described_class.new(user, approval_setting, params) } + subject { described_class.new(user, approval_setting, params) } before do allow_next_instance_of(::GroupMergeRequestApprovalSetting) do |instance| -- GitLab From 434dfa8df1bba080e5cf3a981d96b5f4c2cb1e20 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Tue, 14 Jun 2022 15:57:22 +0530 Subject: [PATCH 13/14] Apply review suggestions for test case --- ...t_approval_setting_changes_auditor_spec.rb | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb index 7bf43a2071dfd4..b7b8057788eaad 100644 --- a/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb @@ -7,23 +7,18 @@ let_it_be(:group) { create(:group) } context 'when group_merge_request_approval_setting is created' do - let_it_be(:approval_setting) do - create(:group_merge_request_approval_setting, - group: group) + let(:params) do + { allow_author_approval: true, + allow_committer_approval: true, + allow_overrides_to_approver_list_per_merge_request: true, + retain_approvals_on_push: true, + require_password_to_approve: true } end - params = { allow_author_approval: true, - allow_committer_approval: true, - allow_overrides_to_approver_list_per_merge_request: true, - retain_approvals_on_push: true, - require_password_to_approve: true } + let(:approval_setting) { create(:group_merge_request_approval_setting, group: group, **params) } + subject { described_class.new(user, approval_setting, params) } - before do - allow_next_instance_of(::GroupMergeRequestApprovalSetting) do |instance| - allow(instance).to receive(:previously_new_record?).and_return(true) - end - end it 'creates audit events' do expect { subject.execute }.to change { AuditEvent.count }.by(5) expect(AuditEvent.last(5).map { |e| e.details[:custom_message] }) -- GitLab From 1135c58946c2245b3624804aec6c0bfa7560e0d8 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Wed, 15 Jun 2022 00:22:31 +0530 Subject: [PATCH 14/14] Handle new record audit events case --- ...ge_request_approval_setting_changes_auditor.rb | 15 +++++++++++++-- ...quest_approval_setting_changes_auditor_spec.rb | 8 ++++---- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb b/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb index 43f6a42d904d99..620ba8be601fc2 100644 --- a/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb +++ b/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb @@ -26,10 +26,10 @@ def audit_change(column, description) end def audit_new_record(column, description) - return unless @params.include? column + return unless should_audit_params_column?(column) audit_context = { - name: "update_group_merge_request_approval_setting", + name: "group_merge_request_approval_setting_created", author: @current_user, scope: @group, target: @group, @@ -39,6 +39,17 @@ def audit_new_record(column, description) ::Gitlab::Audit::Auditor.audit(audit_context) end + def should_audit_params_column?(column) + if column == :require_password_to_approve + @params[column] + else + # we are comparing with false here because on UI we show negative statements. + # where as in tables in store opposite named columns. + # for example `allow_author_approval` column is shown as Prevent approval by author. + @params[column] == false + end + end + def attributes_from_auditable_model(column) if column == :require_password_to_approve { diff --git a/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb index b7b8057788eaad..c1df096f7ac948 100644 --- a/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb @@ -8,10 +8,10 @@ context 'when group_merge_request_approval_setting is created' do let(:params) do - { allow_author_approval: true, - allow_committer_approval: true, - allow_overrides_to_approver_list_per_merge_request: true, - retain_approvals_on_push: true, + { allow_author_approval: false, + allow_committer_approval: false, + allow_overrides_to_approver_list_per_merge_request: false, + retain_approvals_on_push: false, require_password_to_approve: true } end -- GitLab