diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index 214a1f3a009654d423339d11192caacad9c358da..6b7824dfc882c7776379844fa49020abea2b23a2 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -115,6 +115,7 @@ From there, you can see the following actions: - Group deploy token was successfully created, revoked, or deleted. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/353452) in GitLab 14.9. - 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#restrict-group-access-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. 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/push_rule.rb b/ee/app/models/push_rule.rb index 53b32006b5fb2c7627a43658432f080a40e62e92..ff3b8758738fc8bfc5f30deb790350e4aceaf88a 100644 --- a/ee/app/models/push_rule.rb +++ b/ee/app/models/push_rule.rb @@ -19,6 +19,20 @@ class PushRule < ApplicationRecord branch_name_regex ].freeze + AUDIT_LOG_ALLOWLIST = { + commit_committer_check: 'reject unverified users', + reject_unsigned_commits: 'reject unsigned commits', + deny_delete_tag: 'do not allow users to remove Git tags with git push', + member_check: 'check whether the commit author is a GitLab user', + prevent_secrets: 'prevent pushing secret files', + branch_name_regex: 'required branch name regex', + commit_message_regex: 'required commit message regex', + commit_message_negative_regex: 'rejected commit message regex', + author_email_regex: 'required author email regex', + file_name_regex: 'prohibited file name regex', + max_file_size: 'maximum file size (MB)' + }.freeze + belongs_to :project, inverse_of: :push_rule has_one :group, inverse_of: :push_rule, autosave: true diff --git a/ee/app/services/push_rules/create_or_update_service.rb b/ee/app/services/push_rules/create_or_update_service.rb index 56265d51709ef5ecbfd47cc948610f24cea14ac9..91267c16c74132e939da1cf1d9d9ac6b0e6325c6 100644 --- a/ee/app/services/push_rules/create_or_update_service.rb +++ b/ee/app/services/push_rules/create_or_update_service.rb @@ -6,6 +6,7 @@ def execute push_rule = container.push_rule || container.build_push_rule if push_rule.update(params) + ::Audit::GroupPushRulesChangesAuditor.new(current_user, push_rule).execute ServiceResponse.success(payload: { push_rule: push_rule }) else error_message = push_rule.errors.full_messages.to_sentence diff --git a/ee/lib/audit/group_push_rules_changes_auditor.rb b/ee/lib/audit/group_push_rules_changes_auditor.rb new file mode 100644 index 0000000000000000000000000000000000000000..be7396bdfcc243daf31cd778869922efadc791de --- /dev/null +++ b/ee/lib/audit/group_push_rules_changes_auditor.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module Audit + class GroupPushRulesChangesAuditor < EE::Audit::BaseChangesAuditor + STRING_KEYS = [ + :branch_name_regex, :commit_message_regex, :commit_message_negative_regex, :author_email_regex, + :file_name_regex, :max_file_size + ].freeze + + def execute + return if model.blank? || model.group.nil? + + ::PushRule::AUDIT_LOG_ALLOWLIST.each do |attr, desc| + audit_changes(attr, as: desc, entity: model.group, model: model) + end + end + + private + + def audit_required?(column) + should_audit?(column) + end + + def attributes_from_auditable_model(column) + before = model.previous_changes[column].first + after = model.previous_changes[column].last + { + from: before || null_value(column), + to: after || null_value(column), + target_details: model.group.full_path + } + end + + def null_value(column) + STRING_KEYS.include?(column) ? nil : false + end + end +end diff --git a/ee/lib/ee/audit/base_changes_auditor.rb b/ee/lib/ee/audit/base_changes_auditor.rb index bcc18507785d8d877eb5ebe8d4299614a2f4a72a..8c663c27e43f87c7f31281d1f9632ad5cc81121f 100644 --- a/ee/lib/ee/audit/base_changes_auditor.rb +++ b/ee/lib/ee/audit/base_changes_auditor.rb @@ -14,10 +14,6 @@ def parse_options(column, options) super.merge(attributes_from_auditable_model(column)) end - 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. @@ -28,6 +24,12 @@ def should_audit?(column) to = model.previous_changes[column].second !(from.blank? && to.blank?) end + + private + + def attributes_from_auditable_model(column) + raise NotImplementedError + end end end end diff --git a/ee/spec/lib/audit/group_push_rules_changes_auditor_spec.rb b/ee/spec/lib/audit/group_push_rules_changes_auditor_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..ddcaed8747e8b62cf8941f96acb07ed0b20bf7ff --- /dev/null +++ b/ee/spec/lib/audit/group_push_rules_changes_auditor_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Audit::GroupPushRulesChangesAuditor do + let_it_be(:group) { create(:group) } + let_it_be(:current_user) { create(:user) } + + let(:push_rule) { group.build_push_rule } + + before do + group.add_owner(current_user) + end + + subject { described_class.new(current_user, push_rule) } + + context 'auditing group-level changes' do + using RSpec::Parameterized::TableSyntax + + where(:key, :old_value, :new_value) do + :commit_committer_check | false | true + :commit_committer_check | true | false + :reject_unsigned_commits | false | true + :reject_unsigned_commits | true | false + :deny_delete_tag | false | true + :deny_delete_tag | true | false + :member_check | false | true + :member_check | true | false + :prevent_secrets | false | true + :prevent_secrets | true | false + :branch_name_regex | nil | "\\Asecurity-.*\\z" + :branch_name_regex | ".*\\w{2}" | "\\Asecurity-.*\\z" + :commit_message_regex | nil | "\\Asecurity-.*\\z" + :commit_message_regex | ".*\\w{2}" | "\\Asecurity-.*\\z" + :commit_message_negative_regex | nil | "\\Asecurity-.*\\z" + :commit_message_negative_regex | ".*\\w{2}" | "\\Asecurity-.*\\z" + :author_email_regex | nil | "\\Asecurity-.*\\z" + :author_email_regex | ".*\\w{2}" | "\\Asecurity-.*\\z" + :file_name_regex | nil | "\\Asecurity-.*\\z" + :file_name_regex | ".*\\w{2}" | "\\Asecurity-.*\\z" + :max_file_size | 0 | 132 + :max_file_size | 12 | 42 + end + + with_them do + it 'audits the change in push rule correctly', :aggregate_failures do + push_rule.update!(key => old_value) + expect do + push_rule.update!(key => new_value) + subject.execute + end.to change { AuditEvent.count }.by(1) + + event = AuditEvent.last + expect(event.author).to eq(current_user) + expect(event.details[:change]).to eq(::PushRule::AUDIT_LOG_ALLOWLIST[key]) + expect(event.details[:from]).to eq(old_value) + expect(event.details[:to]).to eq(new_value) + expect(event.entity).to eq(group) + end + end + end +end diff --git a/ee/spec/services/push_rules/create_or_update_service_spec.rb b/ee/spec/services/push_rules/create_or_update_service_spec.rb index c8822ffadbbe8c16b3ec5be3dc03ea434a41a03b..009fe8ab75968a635a2adf3515736974d97b6fd5 100644 --- a/ee/spec/services/push_rules/create_or_update_service_spec.rb +++ b/ee/spec/services/push_rules/create_or_update_service_spec.rb @@ -3,12 +3,12 @@ require 'spec_helper' RSpec.describe PushRules::CreateOrUpdateService, '#execute' do - let_it_be_with_reload(:project) { create(:project) } + let_it_be_with_reload(:container) { create(:project) } let_it_be(:user) { create(:user) } let(:params) { { max_file_size: 28 } } - subject { described_class.new(container: project, current_user: user, params: params) } + subject { described_class.new(container: container, current_user: user, params: params) } shared_examples 'a failed update' do let(:params) { { max_file_size: -28 } } @@ -18,12 +18,12 @@ expect(response).to be_error expect(response.message).to eq('Max file size must be greater than or equal to 0') - expect(response.payload).to match(push_rule: project.push_rule) + expect(response.payload).to match(push_rule: container.push_rule) end end context 'with existing push rule' do - let_it_be(:push_rule) { create(:push_rule, project: project) } + let_it_be(:push_rule) { create(:push_rule, project: container) } it 'updates existing push rule' do expect { subject.execute } @@ -38,6 +38,14 @@ expect(response.payload).to match(push_rule: push_rule) end + context 'when container is a group' do + let_it_be(:container) { create(:group) } + + it 'audits the changes' do + expect { subject.execute }.to change { AuditEvent.count }.by(1) + end + end + it_behaves_like 'a failed update' end @@ -45,14 +53,14 @@ it 'creates a new push rule', :aggregate_failures do expect { subject.execute }.to change { PushRule.count }.by(1) - expect(project.push_rule.max_file_size).to eq(28) + expect(container.push_rule.max_file_size).to eq(28) end it 'responds with a successful service response', :aggregate_failures do response = subject.execute expect(response).to be_success - expect(response.payload).to match(push_rule: project.push_rule) + expect(response.payload).to match(push_rule: container.push_rule) end it_behaves_like 'a failed update'