From 02fff57ec775c4f06020949e02a4dc699c35c2f5 Mon Sep 17 00:00:00 2001
From: Smriti Garg <sgarg@gitlab.com>
Date: Fri, 28 Jul 2023 06:06:37 +0000
Subject: [PATCH] Review feedback integrated
Encrypted column fix
---
.../audit_event_types.md | 1 +
.../ee/application_settings/update_service.rb | 8 ++
.../types/application_setting_updated.yml | 9 +++
.../application_setting_changes_auditor.rb | 33 ++++++++
...pplication_setting_changes_auditor_spec.rb | 78 +++++++++++++++++++
.../update_service_spec.rb | 31 +++++++-
.../audit_event_logging_shared_examples.rb | 2 +-
.../gems/attr_encrypted/lib/attr_encrypted.rb | 2 +
8 files changed, 162 insertions(+), 2 deletions(-)
create mode 100644 ee/config/audit_events/types/application_setting_updated.yml
create mode 100644 ee/lib/audit/application_setting_changes_auditor.rb
create mode 100644 ee/spec/lib/audit/application_setting_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 a8fa06dbc647..b34fab8f1fa4 100644
--- a/doc/administration/audit_event_streaming/audit_event_types.md
+++ b/doc/administration/audit_event_streaming/audit_event_types.md
@@ -21,6 +21,7 @@ Audit event types are used to [filter streamed audit events](index.md#update-eve
| [`allow_committer_approval_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/102256) | Event triggered on updating prevent merge request approval from committers from group merge request setting | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | [15.6](https://gitlab.com/gitlab-org/gitlab/-/issues/373949) |
| [`allow_merge_on_skipped_pipeline_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/83922) | There is a project setting which toggles the ability to merge when a pipeline is skipped. This audit event tracks changes to that setting. This MR adds a setting to allow this (like previous GitLab versions). | **{check-circle}** Yes | **{check-circle}** Yes | `continuous_integration` | [14.10](https://gitlab.com/gitlab-org/gitlab/-/issues/301124) |
| [`allow_overrides_to_approver_list_per_merge_request_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/102256) | Event triggered on updating prevent users from modifying MR approval rules in merge requests from group merge request setting | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | [15.6](https://gitlab.com/gitlab-org/gitlab/-/issues/373949) |
+| [`application_setting_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124639) | Triggered when Application setting is updated | **{check-circle}** Yes | **{check-circle}** Yes | `system_access` | [16.3](https://gitlab.com/gitlab-org/gitlab/-/issues/282428) |
| [`approval_rule_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/89939) | Triggered when a merge request approval rule is created | **{check-circle}** Yes | **{check-circle}** Yes | `source_code_management` | [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363092) |
| [`approval_rule_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/82297) | Triggered on successful approval rule deletion | **{check-circle}** Yes | **{check-circle}** Yes | `source_code_management` | [14.9](https://gitlab.com/gitlab-org/gitlab/-/issues/329514) |
| [`audit_events_streaming_headers_create`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92068) | Triggered when a streaming header for audit events is created | **{check-circle}** Yes | **{check-circle}** Yes | `audit_events` | [15.3](https://gitlab.com/gitlab-org/gitlab/-/issues/366350) |
diff --git a/ee/app/services/ee/application_settings/update_service.rb b/ee/app/services/ee/application_settings/update_service.rb
index df799d5222b0..810fccb79185 100644
--- a/ee/app/services/ee/application_settings/update_service.rb
+++ b/ee/app/services/ee/application_settings/update_service.rb
@@ -27,6 +27,10 @@ def execute
update_elasticsearch_containers(ElasticsearchIndexedNamespace, elasticsearch_namespace_ids)
update_elasticsearch_containers(ElasticsearchIndexedProject, elasticsearch_project_ids)
update_elasticsearch_index_settings(number_of_replicas: elasticsearch_replicas, number_of_shards: elasticsearch_shards)
+
+ # There are cases when current user is passed as nil like in elastic.rake
+ # we should not log audit events in such cases
+ log_audit_events if current_user
end
result
@@ -79,6 +83,10 @@ def should_auto_approve_blocked_users?
super || user_cap_increased?
end
+ def log_audit_events
+ Audit::ApplicationSettingChangesAuditor.new(current_user, application_setting).execute
+ end
+
def user_cap_increased?
return false unless application_setting.previous_changes.key?(:new_user_signups_cap)
diff --git a/ee/config/audit_events/types/application_setting_updated.yml b/ee/config/audit_events/types/application_setting_updated.yml
new file mode 100644
index 000000000000..0ad85b4562e1
--- /dev/null
+++ b/ee/config/audit_events/types/application_setting_updated.yml
@@ -0,0 +1,9 @@
+---
+name: application_setting_updated
+description: Triggered when Application setting is updated
+introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/282428
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124639
+feature_category: system_access
+milestone: '16.3'
+saved_to_database: true
+streamed: true
diff --git a/ee/lib/audit/application_setting_changes_auditor.rb b/ee/lib/audit/application_setting_changes_auditor.rb
new file mode 100644
index 000000000000..151655192c85
--- /dev/null
+++ b/ee/lib/audit/application_setting_changes_auditor.rb
@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+module Audit
+ class ApplicationSettingChangesAuditor < BaseChangesAuditor
+ def execute
+ return if model.blank?
+
+ changed_columns = model.previous_changes.except!(:updated_at)
+
+ changed_columns.each_key do |column|
+ next if auditable_attribute?(column)
+
+ audit_changes(column, as: column.to_s, model: model,
+ entity: Gitlab::Audit::InstanceScope.new,
+ event_type: "application_setting_updated")
+ end
+ end
+
+ private
+
+ def auditable_attribute?(column)
+ !!(column =~ /_html\Z/) || !!(column =~ /^encrypted_/)
+ end
+
+ def attributes_from_auditable_model(column)
+ {
+ from: model.previous_changes[column].first,
+ to: model.previous_changes[column].last,
+ target_details: column.humanize
+ }
+ end
+ end
+end
diff --git a/ee/spec/lib/audit/application_setting_changes_auditor_spec.rb b/ee/spec/lib/audit/application_setting_changes_auditor_spec.rb
new file mode 100644
index 000000000000..fee14eee164f
--- /dev/null
+++ b/ee/spec/lib/audit/application_setting_changes_auditor_spec.rb
@@ -0,0 +1,78 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Audit::ApplicationSettingChangesAuditor, feature_category: :audit_events do
+ include StubENV
+
+ describe '.audit_changes' do
+ let!(:user) { create(:user) }
+ let!(:application_setting) { ApplicationSetting.create_from_defaults }
+ let!(:application_setting_auditor_instance) { described_class.new(user, application_setting) }
+
+ before do
+ stub_licensed_features(extended_audit_events: true, admin_audit_log: true)
+ end
+
+ shared_examples 'application_setting_audit_events_from_to' do
+ it 'calls auditor' do
+ expect(Gitlab::Audit::Auditor).to receive(:audit).with(
+ {
+ name: "application_setting_updated",
+ author: user,
+ scope: be_an_instance_of(Gitlab::Audit::InstanceScope),
+ target: application_setting,
+ message: "Changed #{change_field} from #{change_from} to #{change_to}",
+ additional_details: {
+ change: change_field.to_s,
+ from: change_from,
+ target_details: change_field.humanize,
+ to: change_to
+ },
+ target_details: change_field.humanize
+ }
+ ).and_call_original
+
+ expect { application_setting_auditor_instance.execute }.to change { AuditEvent.count }.by(1)
+
+ event = AuditEvent.last
+ expect(event.details[:from]).to eq change_from
+ expect(event.details[:to]).to eq change_to
+ expect(event.details[:change]).to eq change_field
+ end
+ end
+
+ context 'when any model change is made' do
+ let(:change_from) { 0 }
+ let(:change_to) { 10 }
+ let(:change_field) { "default_project_visibility" }
+
+ before do
+ application_setting.update!(default_project_visibility: 10)
+ end
+
+ it_behaves_like 'application_setting_audit_events_from_to'
+ end
+
+ context 'when ignored column is updated' do
+ it 'does not create an event for _html columns' do
+ application_setting.update!(after_sign_up_text_html: "test_text")
+
+ expect(AuditEvents::AuditEventStreamingWorker).not_to receive(:perform_async)
+ expect { application_setting_auditor_instance.execute }.not_to change { AuditEvent.count }
+ end
+ end
+
+ context 'when encrypted column is updated' do
+ it 'creates an event for encrypted columns' do
+ application_setting.update!(anthropic_api_key: 'ANTHROPIC_API_KEY')
+
+ expect { application_setting_auditor_instance.execute }.to change { AuditEvent.count }.by(1)
+
+ event = AuditEvent.last
+ expect(event.details[:change]).to eq "anthropic_api_key"
+ expect(event.details[:to]).to eq nil # For encrypted column encrypted_ column contains the cipher
+ end
+ end
+ end
+end
diff --git a/ee/spec/services/application_settings/update_service_spec.rb b/ee/spec/services/application_settings/update_service_spec.rb
index f6d01ad96d50..e5060260e64d 100644
--- a/ee/spec/services/application_settings/update_service_spec.rb
+++ b/ee/spec/services/application_settings/update_service_spec.rb
@@ -3,19 +3,48 @@
require 'spec_helper'
RSpec.describe ApplicationSettings::UpdateService do
- let(:user) { create(:user) }
+ let!(:user) { create(:user) }
let(:setting) { ApplicationSetting.create_from_defaults }
let(:service) { described_class.new(setting, user, opts) }
+ shared_examples 'application_setting_audit_events_from_to' do
+ it 'calls auditor' do
+ expect { service.execute }.to change { AuditEvent.count }.by(1)
+ service.execute
+
+ event = AuditEvent.last
+ expect(event.details[:from]).to eq change_from
+ expect(event.details[:to]).to eq change_to
+ expect(event.details[:change]).to eq change_field
+ end
+
+ context 'when user is nil' do
+ let(:user) { nil }
+
+ it "does not log an event" do
+ expect { service.execute }.to change { AuditEvent.count }.by(0)
+ end
+ end
+ end
+
describe '#execute' do
context 'common params' do
let(:opts) { { home_page_url: 'http://foo.bar' } }
+ let(:change_field) { 'home_page_url' }
+ let(:change_to) { 'http://foo.bar' }
+ let(:change_from) { nil }
+
+ before do
+ stub_licensed_features(extended_audit_events: true, admin_audit_log: true, code_owner_approval_required: true)
+ end
it 'properly updates settings with given params' do
service.execute
expect(setting.home_page_url).to eql(opts[:home_page_url])
end
+
+ it_behaves_like 'application_setting_audit_events_from_to'
end
context 'with valid params' do
diff --git a/ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb b/ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb
index 6b29b6543376..ebd7a72fbc47 100644
--- a/ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb
+++ b/ee/spec/support/shared_examples/services/audit_event_logging_shared_examples.rb
@@ -41,7 +41,7 @@
it 'does not log audit event if operation fails' do
fail_condition!
- expect { operation }.not_to change(AuditEvent, :count)
+ expect { operation }.not_to change { AuditEvent.count }
end
it 'does not log audit event if operation results in no change' do
diff --git a/vendor/gems/attr_encrypted/lib/attr_encrypted.rb b/vendor/gems/attr_encrypted/lib/attr_encrypted.rb
index 82a14086adea..28508e6d2603 100644
--- a/vendor/gems/attr_encrypted/lib/attr_encrypted.rb
+++ b/vendor/gems/attr_encrypted/lib/attr_encrypted.rb
@@ -186,6 +186,8 @@ def attr_encrypted_options
@attr_encrypted_options ||= superclass.attr_encrypted_options.dup
end
+ # Encrypted attributes are ignored as part of ApplicationSetting Audit Event
+ # Log. If the prefix is updated then the corresponding Regex in ApplicationSettingChangesAuditor should also be updated.
def attr_encrypted_default_options
{
prefix: 'encrypted_',
--
GitLab