From 9d91457961119d04963de5f1b4f2d8c6bad6c1d1 Mon Sep 17 00:00:00 2001 From: Max Woolf Date: Fri, 29 Apr 2022 11:40:02 +0100 Subject: [PATCH 1/6] Add group push rule changes to audit log When a group owner makes changes to the group push rules, we now audit those changes at the group level. EE: true Changelog: added --- doc/administration/audit_events.md | 1 + .../push_rules/create_or_update_service.rb | 1 + .../audit/group_push_rules_changes_auditor.rb | 50 +++++++++++++++ .../group_push_rules_changes_auditor_spec.rb | 62 +++++++++++++++++++ 4 files changed, 114 insertions(+) create mode 100644 ee/lib/ee/audit/group_push_rules_changes_auditor.rb create mode 100644 ee/spec/lib/ee/audit/group_push_rules_changes_auditor_spec.rb diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index 214a1f3a009654..6b7824dfc882c7 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/services/push_rules/create_or_update_service.rb b/ee/app/services/push_rules/create_or_update_service.rb index 56265d51709ef5..a173c009a9b195 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) + EE::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/ee/audit/group_push_rules_changes_auditor.rb b/ee/lib/ee/audit/group_push_rules_changes_auditor.rb new file mode 100644 index 00000000000000..fca5688b3319fb --- /dev/null +++ b/ee/lib/ee/audit/group_push_rules_changes_auditor.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module EE + module Audit + class GroupPushRulesChangesAuditor < BaseChangesAuditor + 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 + + 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? + + AUDIT_LOG_ALLOWLIST.each do |attr, desc| + audit_changes(attr, as: desc, entity: model.group, model: model) + end + end + + def attributes_from_auditable_model(column) + before = model.previous_changes[column].first + after = model.previous_changes[column].last + { + from: before.nil? ? null_value(column) : before, + to: after.nil? ? null_value(column) : after, + target_details: model.group.full_path + } + end + + private + + def null_value(column) + STRING_KEYS.include?(column) ? nil : false + end + end + end +end diff --git a/ee/spec/lib/ee/audit/group_push_rules_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/group_push_rules_changes_auditor_spec.rb new file mode 100644 index 00000000000000..776e4b17810226 --- /dev/null +++ b/ee/spec/lib/ee/audit/group_push_rules_changes_auditor_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe EE::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(EE::Audit::GroupPushRulesChangesAuditor::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 -- GitLab From 08b1e624d80c996c676a335e4330fe7299ac04fd Mon Sep 17 00:00:00 2001 From: Max Woolf Date: Fri, 29 Apr 2022 13:17:26 +0100 Subject: [PATCH 2/6] Fix non-group containers --- .../push_rules/create_or_update_service.rb | 2 +- .../audit/group_push_rules_changes_auditor.rb | 48 ++++++++++++++++++ .../audit/group_push_rules_changes_auditor.rb | 50 ------------------- .../group_push_rules_changes_auditor_spec.rb | 4 +- 4 files changed, 51 insertions(+), 53 deletions(-) create mode 100644 ee/lib/audit/group_push_rules_changes_auditor.rb delete mode 100644 ee/lib/ee/audit/group_push_rules_changes_auditor.rb rename ee/spec/lib/{ee => }/audit/group_push_rules_changes_auditor_spec.rb (93%) 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 a173c009a9b195..91267c16c74132 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,7 @@ def execute push_rule = container.push_rule || container.build_push_rule if push_rule.update(params) - EE::Audit::GroupPushRulesChangesAuditor.new(current_user, push_rule).execute + ::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 00000000000000..84e26157aaabff --- /dev/null +++ b/ee/lib/audit/group_push_rules_changes_auditor.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module Audit + class GroupPushRulesChangesAuditor < EE::Audit::BaseChangesAuditor + 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 + + 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? + + AUDIT_LOG_ALLOWLIST.each do |attr, desc| + audit_changes(attr, as: desc, entity: model.group, model: model) + end + end + + def attributes_from_auditable_model(column) + before = model.previous_changes[column].first + after = model.previous_changes[column].last + { + from: before.nil? ? null_value(column) : before, + to: after.nil? ? null_value(column) : after, + target_details: model.group.full_path + } + end + + private + + def null_value(column) + STRING_KEYS.include?(column) ? nil : false + end + end +end diff --git a/ee/lib/ee/audit/group_push_rules_changes_auditor.rb b/ee/lib/ee/audit/group_push_rules_changes_auditor.rb deleted file mode 100644 index fca5688b3319fb..00000000000000 --- a/ee/lib/ee/audit/group_push_rules_changes_auditor.rb +++ /dev/null @@ -1,50 +0,0 @@ -# frozen_string_literal: true - -module EE - module Audit - class GroupPushRulesChangesAuditor < BaseChangesAuditor - 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 - - 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? - - AUDIT_LOG_ALLOWLIST.each do |attr, desc| - audit_changes(attr, as: desc, entity: model.group, model: model) - end - end - - def attributes_from_auditable_model(column) - before = model.previous_changes[column].first - after = model.previous_changes[column].last - { - from: before.nil? ? null_value(column) : before, - to: after.nil? ? null_value(column) : after, - target_details: model.group.full_path - } - end - - private - - def null_value(column) - STRING_KEYS.include?(column) ? nil : false - end - end - end -end diff --git a/ee/spec/lib/ee/audit/group_push_rules_changes_auditor_spec.rb b/ee/spec/lib/audit/group_push_rules_changes_auditor_spec.rb similarity index 93% rename from ee/spec/lib/ee/audit/group_push_rules_changes_auditor_spec.rb rename to ee/spec/lib/audit/group_push_rules_changes_auditor_spec.rb index 776e4b17810226..a2c59bced8a508 100644 --- a/ee/spec/lib/ee/audit/group_push_rules_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/group_push_rules_changes_auditor_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe EE::Audit::GroupPushRulesChangesAuditor do +RSpec.describe Audit::GroupPushRulesChangesAuditor do let_it_be(:group) { create(:group) } let_it_be(:current_user) { create(:user) } @@ -52,7 +52,7 @@ event = AuditEvent.last expect(event.author).to eq(current_user) - expect(event.details[:change]).to eq(EE::Audit::GroupPushRulesChangesAuditor::AUDIT_LOG_ALLOWLIST[key]) + expect(event.details[:change]).to eq(::Audit::GroupPushRulesChangesAuditor::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) -- GitLab From 09de28484fcc97fbdb7365d340f873066fa65407 Mon Sep 17 00:00:00 2001 From: Max Woolf Date: Fri, 29 Apr 2022 16:53:20 +0100 Subject: [PATCH 3/6] Fix first-time setting of push rules --- ee/lib/audit/group_push_rules_changes_auditor.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/ee/lib/audit/group_push_rules_changes_auditor.rb b/ee/lib/audit/group_push_rules_changes_auditor.rb index 84e26157aaabff..7da8a23b9b7932 100644 --- a/ee/lib/audit/group_push_rules_changes_auditor.rb +++ b/ee/lib/audit/group_push_rules_changes_auditor.rb @@ -29,6 +29,16 @@ def execute end end + def audit_required?(column) + return false unless should_audit?(column) + + if STRING_KEYS.include?(column) + model.previous_changes[column].any?(&:present?) && changed?(column) + else + changed?(column) + end + end + def attributes_from_auditable_model(column) before = model.previous_changes[column].first after = model.previous_changes[column].last -- GitLab From 61f93c62b9287d7b2292475ab93a617104ce6726 Mon Sep 17 00:00:00 2001 From: Harsimar Sandhu Date: Fri, 6 May 2022 13:07:57 +0000 Subject: [PATCH 4/6] Apply 1 suggestion(s) to 1 file(s) --- ee/lib/audit/group_push_rules_changes_auditor.rb | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/ee/lib/audit/group_push_rules_changes_auditor.rb b/ee/lib/audit/group_push_rules_changes_auditor.rb index 7da8a23b9b7932..e9a6d81d7edf99 100644 --- a/ee/lib/audit/group_push_rules_changes_auditor.rb +++ b/ee/lib/audit/group_push_rules_changes_auditor.rb @@ -30,13 +30,7 @@ def execute end def audit_required?(column) - return false unless should_audit?(column) - - if STRING_KEYS.include?(column) - model.previous_changes[column].any?(&:present?) && changed?(column) - else - changed?(column) - end + should_audit?(column) end def attributes_from_auditable_model(column) -- GitLab From cd7e30b0fa7620741459e6c16e96de62706df4ea Mon Sep 17 00:00:00 2001 From: Harsimar Sandhu Date: Fri, 6 May 2022 13:07:57 +0000 Subject: [PATCH 5/6] Apply 1 suggestion(s) to 1 file(s) --- ee/app/models/push_rule.rb | 14 ++++++++ .../audit/group_push_rules_changes_auditor.rb | 32 ++++--------------- ee/lib/ee/audit/base_changes_auditor.rb | 10 +++--- .../group_push_rules_changes_auditor_spec.rb | 2 +- 4 files changed, 27 insertions(+), 31 deletions(-) diff --git a/ee/app/models/push_rule.rb b/ee/app/models/push_rule.rb index 53b32006b5fb2c..ff3b8758738fc8 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/lib/audit/group_push_rules_changes_auditor.rb b/ee/lib/audit/group_push_rules_changes_auditor.rb index 7da8a23b9b7932..be7396bdfcc243 100644 --- a/ee/lib/audit/group_push_rules_changes_auditor.rb +++ b/ee/lib/audit/group_push_rules_changes_auditor.rb @@ -2,20 +2,6 @@ module Audit class GroupPushRulesChangesAuditor < EE::Audit::BaseChangesAuditor - 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 - STRING_KEYS = [ :branch_name_regex, :commit_message_regex, :commit_message_negative_regex, :author_email_regex, :file_name_regex, :max_file_size @@ -24,33 +10,27 @@ class GroupPushRulesChangesAuditor < EE::Audit::BaseChangesAuditor def execute return if model.blank? || model.group.nil? - AUDIT_LOG_ALLOWLIST.each do |attr, desc| + ::PushRule::AUDIT_LOG_ALLOWLIST.each do |attr, desc| audit_changes(attr, as: desc, entity: model.group, model: model) end end - def audit_required?(column) - return false unless should_audit?(column) + private - if STRING_KEYS.include?(column) - model.previous_changes[column].any?(&:present?) && changed?(column) - else - changed?(column) - end + 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.nil? ? null_value(column) : before, - to: after.nil? ? null_value(column) : after, + from: before || null_value(column), + to: after || null_value(column), target_details: model.group.full_path } end - private - def null_value(column) STRING_KEYS.include?(column) ? nil : false end diff --git a/ee/lib/ee/audit/base_changes_auditor.rb b/ee/lib/ee/audit/base_changes_auditor.rb index bcc18507785d8d..8c663c27e43f87 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 index a2c59bced8a508..ddcaed8747e8b6 100644 --- a/ee/spec/lib/audit/group_push_rules_changes_auditor_spec.rb +++ b/ee/spec/lib/audit/group_push_rules_changes_auditor_spec.rb @@ -52,7 +52,7 @@ event = AuditEvent.last expect(event.author).to eq(current_user) - expect(event.details[:change]).to eq(::Audit::GroupPushRulesChangesAuditor::AUDIT_LOG_ALLOWLIST[key]) + 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) -- GitLab From c67d1e6aa96887c17b1bf89cc2878a643d07ffe5 Mon Sep 17 00:00:00 2001 From: Max Woolf Date: Wed, 11 May 2022 10:11:34 +0100 Subject: [PATCH 6/6] Backend maintainer suggestions --- .../create_or_update_service_spec.rb | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) 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 c8822ffadbbe8c..009fe8ab75968a 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' -- GitLab