diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index 6d7121d32fede834ba400100c34f7a9f8c1b0f6e..442f743f2c7744a9ca03a23725fab9a017157394 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) diff --git a/ee/app/models/group_merge_request_approval_setting.rb b/ee/app/models/group_merge_request_approval_setting.rb index b4277507f7fd8c7c305dfd5cf67bdaa40adae5ba..19eaf11df7ee6ca77c53f11b70ddd7994d74c8c1 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 7cf1e0a05cfac1124538ff204358d4c9789d0084..3b13b4480096facc650e35f3a4a6cbcb5c9aeabb 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, 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 new file mode 100644 index 0000000000000000000000000000000000000000..620ba8be601fc264ce6eae6d5ba53751fee366b3 --- /dev/null +++ b/ee/lib/audit/group_merge_request_approval_setting_changes_auditor.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +module Audit + class GroupMergeRequestApprovalSettingChangesAuditor < ::EE::Audit::BaseChangesAuditor + def initialize(current_user, approval_setting, params) + @group = approval_setting.group + @params = params + + super(current_user, approval_setting) + end + + def execute + ::GroupMergeRequestApprovalSetting::AUDIT_LOG_ALLOWLIST.each do |column, description| + audit_change(column, description) + end + end + + private + + def audit_change(column, description) + if model.previously_new_record? + audit_new_record(column, description) + else + audit_changes(column, as: description, entity: @group, model: model) + end + end + + def audit_new_record(column, description) + return unless should_audit_params_column?(column) + + audit_context = { + name: "group_merge_request_approval_setting_created", + author: @current_user, + scope: @group, + target: @group, + message: "Changed #{description} from false to true" + } + + ::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 + { + from: model.previous_changes[column].first, + to: model.previous_changes[column].last + } + else + # 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'. + { + 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 0000000000000000000000000000000000000000..c1df096f7ac9483617af8ae5d041f5c4c854009f --- /dev/null +++ b/ee/spec/lib/audit/group_merge_request_approval_setting_changes_auditor_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Audit::GroupMergeRequestApprovalSettingChangesAuditor do + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + + context 'when group_merge_request_approval_setting is created' do + let(:params) do + { 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 + + let(:approval_setting) { create(:group_merge_request_approval_setting, group: group, **params) } + + subject { described_class.new(user, approval_setting, params) } + + 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 + + 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 +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 9a63d4f662d2f61cd0100d354a911709e95dce87..28178025d0643c5b3403b9a684e906db05b301ba 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_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 + + 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 + it_behaves_like '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 + + it_behaves_like 'call audit changes service' end context 'when saving fails' do