From 0ed7fd2b1e22516a6a74b6e0b4887f191f82157e Mon Sep 17 00:00:00 2001 From: Jay Montal Date: Tue, 19 Sep 2023 10:17:00 -0600 Subject: [PATCH] Add project push rules changes auditor - Add push rules substructure to audit - Create a base push rules changes auditor - Update group push rules auditor to use base - Add project push rules auditor with single audit event Changelog: changed EE: true --- .../audit_event_types.md | 1 + .../push_rules/create_or_update_service.rb | 4 +- ...h_rules_commit_committer_check_updated.yml | 9 +++ .../audit/group_push_rules_changes_auditor.rb | 56 ------------------- .../base_push_rules_changes_auditor.rb | 33 +++++++++++ .../group_push_rules_changes_auditor.rb | 44 +++++++++++++++ .../project_push_rules_changes_auditor.rb | 25 +++++++++ .../group_push_rules_changes_auditor_spec.rb | 13 +++-- ...project_push_rules_changes_auditor_spec.rb | 56 +++++++++++++++++++ 9 files changed, 180 insertions(+), 61 deletions(-) create mode 100644 ee/config/audit_events/types/project_push_rules_commit_committer_check_updated.yml delete mode 100644 ee/lib/audit/group_push_rules_changes_auditor.rb create mode 100644 ee/lib/audit/push_rules/base_push_rules_changes_auditor.rb create mode 100644 ee/lib/audit/push_rules/group_push_rules_changes_auditor.rb create mode 100644 ee/lib/audit/push_rules/project_push_rules_changes_auditor.rb rename ee/spec/lib/audit/{ => push_rules}/group_push_rules_changes_auditor_spec.rb (93%) create mode 100644 ee/spec/lib/audit/push_rules/project_push_rules_changes_auditor_spec.rb diff --git a/doc/administration/audit_event_streaming/audit_event_types.md b/doc/administration/audit_event_streaming/audit_event_types.md index 86369a6001a9ba..006091bfd38419 100644 --- a/doc/administration/audit_event_streaming/audit_event_types.md +++ b/doc/administration/audit_event_streaming/audit_event_types.md @@ -230,6 +230,7 @@ audit events to external destinations. | [`project_packages_enabled_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/7962) | When the setting that controls packages for a project is toggled, this audit event is created | **{check-circle}** Yes | **{check-circle}** Yes | `groups_and_projects` | GitLab [11.5](https://gitlab.com/gitlab-org/gitlab/-/issues/369288) | | [`project_path_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/100770) | Event triggered on updating a project's path | **{check-circle}** Yes | **{check-circle}** Yes | `groups_and_projects` | GitLab [15.5](https://gitlab.com/gitlab-org/gitlab/-/issues/369271) | | [`project_printing_merge_request_link_enabled_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106652) | Event triggered on updating setting for projects for enabling printing merge request link | **{check-circle}** Yes | **{check-circle}** Yes | `groups_and_projects` | GitLab [15.7](https://gitlab.com/gitlab-org/gitlab/-/issues/369283) | +| [`project_push_rules_commit_committer_check_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/132157) | Triggered when project push rule setting is updated for reject unverified users. | **{check-circle}** Yes | **{check-circle}** Yes | `source_code_management` | GitLab [16.5](https://gitlab.com/gitlab-org/gitlab/-/issues/268116) | | [`project_remove_source_branch_after_merge_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/83922) | Create this audit event whenever a project has its setting to remove branches after merges modified | **{check-circle}** Yes | **{check-circle}** Yes | `code_review_workflow` | GitLab [14.10](https://gitlab.com/gitlab-org/gitlab/-/issues/301124) | | [`project_repository_size_limit_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106652) | Event triggered on updating repository size limit of a project | **{check-circle}** Yes | **{check-circle}** Yes | `groups_and_projects` | GitLab [15.7](https://gitlab.com/gitlab-org/gitlab/-/issues/369274) | | [`project_require_password_to_approve_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106652) | Event triggered on updating project setting for requiring user's password for approval of merge request | **{check-circle}** Yes | **{check-circle}** Yes | `groups_and_projects` | GitLab [15.7](https://gitlab.com/gitlab-org/gitlab/-/issues/369280) | 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 91267c16c74132..69014cbd9d47ab 100644 --- a/ee/app/services/push_rules/create_or_update_service.rb +++ b/ee/app/services/push_rules/create_or_update_service.rb @@ -6,7 +6,9 @@ def execute push_rule = container.push_rule || container.build_push_rule if push_rule.update(params) - ::Audit::GroupPushRulesChangesAuditor.new(current_user, push_rule).execute + ::Audit::PushRules::GroupPushRulesChangesAuditor.new(current_user, push_rule).execute + ::Audit::PushRules::ProjectPushRulesChangesAuditor.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/config/audit_events/types/project_push_rules_commit_committer_check_updated.yml b/ee/config/audit_events/types/project_push_rules_commit_committer_check_updated.yml new file mode 100644 index 00000000000000..259b29b9a7d642 --- /dev/null +++ b/ee/config/audit_events/types/project_push_rules_commit_committer_check_updated.yml @@ -0,0 +1,9 @@ +--- +name: project_push_rules_commit_committer_check_updated +description: Triggered when project push rule setting is updated for reject unverified users. +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/268116 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/132157 +feature_category: source_code_management +milestone: '16.5' +saved_to_database: true +streamed: true diff --git a/ee/lib/audit/group_push_rules_changes_auditor.rb b/ee/lib/audit/group_push_rules_changes_auditor.rb deleted file mode 100644 index 94f80be941c4e0..00000000000000 --- a/ee/lib/audit/group_push_rules_changes_auditor.rb +++ /dev/null @@ -1,56 +0,0 @@ -# frozen_string_literal: true - -module Audit - class GroupPushRulesChangesAuditor < BaseChangesAuditor - STRING_KEYS = [ - :branch_name_regex, :commit_message_regex, :commit_message_negative_regex, :author_email_regex, - :file_name_regex, :max_file_size - ].freeze - - EVENT_TYPE_PER_ATTR = { - max_file_size: 'group_push_rules_max_file_size_updated', - file_name_regex: 'group_push_rules_file_name_regex_updated', - author_email_regex: 'group_push_rules_author_email_regex_updated', - commit_message_negative_regex: 'group_push_rules_commit_message_negative_regex_updated', - commit_message_regex: 'group_push_rules_commit_message_regex_updated', - branch_name_regex: 'group_push_rules_branch_name_regex_updated', - commit_committer_check: 'group_push_rules_commit_committer_check_updated', - reject_unsigned_commits: 'group_push_rules_reject_unsigned_commits_updated', - reject_non_dco_commits: 'group_push_rules_reject_non_dco_commits_updated', - deny_delete_tag: 'group_push_rules_reject_deny_delete_tag_updated', - member_check: 'group_push_rules_reject_member_check_updated', - prevent_secrets: 'group_push_rules_prevent_secrets_updated' - }.freeze - - def execute - return if model.blank? || model.group.nil? - - ::PushRule::AUDIT_LOG_ALLOWLIST.each do |attr, desc| - event_name = EVENT_TYPE_PER_ATTR[attr] || 'audit_operation' - - audit_changes(attr, as: desc, entity: model.group, model: model, - event_type: event_name) - 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/audit/push_rules/base_push_rules_changes_auditor.rb b/ee/lib/audit/push_rules/base_push_rules_changes_auditor.rb new file mode 100644 index 00000000000000..c0043bd6793902 --- /dev/null +++ b/ee/lib/audit/push_rules/base_push_rules_changes_auditor.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Audit + module PushRules + class BasePushRulesChangesAuditor < BaseChangesAuditor + STRING_KEYS = [ + :branch_name_regex, :commit_message_regex, :commit_message_negative_regex, :author_email_regex, + :file_name_regex, :max_file_size + ].freeze + + private + + 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: target_details + } + end + + def audit_required?(column) + should_audit?(column) + end + + def null_value(column) + STRING_KEYS.include?(column) ? nil : false + end + end + end +end diff --git a/ee/lib/audit/push_rules/group_push_rules_changes_auditor.rb b/ee/lib/audit/push_rules/group_push_rules_changes_auditor.rb new file mode 100644 index 00000000000000..8d4b59a36c2f17 --- /dev/null +++ b/ee/lib/audit/push_rules/group_push_rules_changes_auditor.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module Audit + module PushRules + class GroupPushRulesChangesAuditor < BasePushRulesChangesAuditor + EVENT_TYPE_PER_ATTR = { + max_file_size: 'group_push_rules_max_file_size_updated', + file_name_regex: 'group_push_rules_file_name_regex_updated', + author_email_regex: 'group_push_rules_author_email_regex_updated', + commit_message_negative_regex: 'group_push_rules_commit_message_negative_regex_updated', + commit_message_regex: 'group_push_rules_commit_message_regex_updated', + branch_name_regex: 'group_push_rules_branch_name_regex_updated', + commit_committer_check: 'group_push_rules_commit_committer_check_updated', + reject_unsigned_commits: 'group_push_rules_reject_unsigned_commits_updated', + reject_non_dco_commits: 'group_push_rules_reject_non_dco_commits_updated', + deny_delete_tag: 'group_push_rules_reject_deny_delete_tag_updated', + member_check: 'group_push_rules_reject_member_check_updated', + prevent_secrets: 'group_push_rules_prevent_secrets_updated' + }.freeze + + def execute + return if model.blank? || model.group.nil? + + ::PushRule::AUDIT_LOG_ALLOWLIST.each do |attr, desc| + event_name = EVENT_TYPE_PER_ATTR[attr] || 'audit_operation' + + audit_changes( + attr, + as: desc, + entity: model.group, + model: model, + event_type: event_name + ) + end + end + + private + + def target_details + model.group.full_path + end + end + end +end diff --git a/ee/lib/audit/push_rules/project_push_rules_changes_auditor.rb b/ee/lib/audit/push_rules/project_push_rules_changes_auditor.rb new file mode 100644 index 00000000000000..db1b101f319b04 --- /dev/null +++ b/ee/lib/audit/push_rules/project_push_rules_changes_auditor.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Audit + module PushRules + class ProjectPushRulesChangesAuditor < BasePushRulesChangesAuditor + def execute + return if model.blank? || model.project.nil? + + audit_changes( + :commit_committer_check, + as: 'reject unverified users', + entity: model.project, + model: model, + event_type: 'project_push_rules_commit_committer_check_updated' + ) + end + + private + + def target_details + model.project.full_path + end + end + end +end diff --git a/ee/spec/lib/audit/group_push_rules_changes_auditor_spec.rb b/ee/spec/lib/audit/push_rules/group_push_rules_changes_auditor_spec.rb similarity index 93% rename from ee/spec/lib/audit/group_push_rules_changes_auditor_spec.rb rename to ee/spec/lib/audit/push_rules/group_push_rules_changes_auditor_spec.rb index b30d0b2223fcdc..177164c5acdb95 100644 --- a/ee/spec/lib/audit/group_push_rules_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/push_rules/group_push_rules_changes_auditor_spec.rb @@ -2,14 +2,17 @@ require 'spec_helper' -RSpec.describe Audit::GroupPushRulesChangesAuditor, feature_category: :source_code_management do +RSpec.describe Audit::PushRules::GroupPushRulesChangesAuditor, feature_category: :source_code_management do let_it_be(:group) { create(:group) } let_it_be(:current_user) { create(:user) } let(:push_rule) { group.build_push_rule } - before do + before_all do group.add_owner(current_user) + end + + before do stub_licensed_features(audit_events: true, external_audit_events: true) group.external_audit_event_destinations.create!(destination_url: 'http://example.com') end @@ -76,8 +79,10 @@ context 'for EVENT_TYPE_PER_ATTR' do it 'defines audit event types for all the audit log allowlist attributes for group push rule changes' do - expect(PushRule::AUDIT_LOG_ALLOWLIST.keys - Audit::GroupPushRulesChangesAuditor::EVENT_TYPE_PER_ATTR.keys) - .to be_empty + expect( + PushRule::AUDIT_LOG_ALLOWLIST.keys - + Audit::PushRules::GroupPushRulesChangesAuditor::EVENT_TYPE_PER_ATTR.keys + ).to be_empty end end end diff --git a/ee/spec/lib/audit/push_rules/project_push_rules_changes_auditor_spec.rb b/ee/spec/lib/audit/push_rules/project_push_rules_changes_auditor_spec.rb new file mode 100644 index 00000000000000..4d7d26b72cad07 --- /dev/null +++ b/ee/spec/lib/audit/push_rules/project_push_rules_changes_auditor_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Audit::PushRules::ProjectPushRulesChangesAuditor, feature_category: :source_code_management do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:current_user) { create(:user) } + + let(:push_rule) { project.build_push_rule } + + before do + stub_licensed_features(audit_events: true, external_audit_events: true) + group.external_audit_event_destinations.create!(destination_url: 'http://example.com') + end + + subject { described_class.new(current_user, push_rule) } + + context 'when auditing project-level changes in push rules' do + using RSpec::Parameterized::TableSyntax + + # rubocop:disable Layout/LineLength + where(:key, :old_value, :new_value, :event_name) do + :commit_committer_check | false | true | 'project_push_rules_commit_committer_check_updated' + :commit_committer_check | true | false | 'project_push_rules_commit_committer_check_updated' + end + # rubocop:enable Layout/LineLength + + with_them do + before do + push_rule.update!(key => old_value) + push_rule.update!(key => new_value) + end + + it 'audits the change in push rule correctly', :aggregate_failures do + expect do + 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(project) + end + + it 'streams correct audit event', :aggregate_failures do + expect(AuditEvents::AuditEventStreamingWorker).to receive(:perform_async) + .with(event_name, anything, anything) + subject.execute + end + end + end +end -- GitLab