From 27b5ed232b769b0269c1f0c610587dae7bf60627 Mon Sep 17 00:00:00 2001 From: Sam Figueroa Date: Thu, 2 Mar 2023 22:44:00 +0000 Subject: [PATCH 1/2] Add event_types for FeatureFlag audit events - Refs: https://gitlab.com/gitlab-org/gitlab/-/issues/374109 Changelog: changed --- app/services/feature_flags/base_service.rb | 34 +++++-------------- app/services/feature_flags/create_service.rb | 10 ++++++ app/services/feature_flags/destroy_service.rb | 10 ++++++ app/services/feature_flags/update_service.rb | 16 +++++++-- .../types/feature_flag_created.yml | 9 +++++ .../types/feature_flag_deleted.yml | 9 +++++ .../types/feature_flag_updated.yml | 9 +++++ .../feature_flags/create_service_spec.rb | 8 +++-- .../feature_flags/destroy_service_spec.rb | 3 +- .../feature_flags/update_service_spec.rb | 5 ++- 10 files changed, 77 insertions(+), 36 deletions(-) create mode 100644 config/audit_events/types/feature_flag_created.yml create mode 100644 config/audit_events/types/feature_flag_deleted.yml create mode 100644 config/audit_events/types/feature_flag_updated.yml diff --git a/app/services/feature_flags/base_service.rb b/app/services/feature_flags/base_service.rb index 59db1a5f12faa7..028906a0b43377 100644 --- a/app/services/feature_flags/base_service.rb +++ b/app/services/feature_flags/base_service.rb @@ -7,42 +7,24 @@ class BaseService < ::BaseService AUDITABLE_ATTRIBUTES = %w(name description active).freeze def success(**args) - audit_event = args.fetch(:audit_event) { audit_event(args[:feature_flag]) } - save_audit_event(audit_event) sync_to_jira(args[:feature_flag]) + + audit_event(args[:feature_flag], args[:audit_context]) super end protected - def update_last_feature_flag_updated_at! - Operations::FeatureFlagsClient.update_last_feature_flag_updated_at!(project) - end - - def audit_event(feature_flag) - message = audit_message(feature_flag) + def audit_event(feature_flag, context = nil) + context ||= audit_context(feature_flag) - return if message.blank? + return if context[:message].blank? - details = - { - custom_message: message, - target_id: feature_flag.id, - target_type: feature_flag.class.name, - target_details: feature_flag.name - } - - ::AuditEventService.new( - current_user, - feature_flag.project, - details - ) + ::Gitlab::Audit::Auditor.audit(context) end - def save_audit_event(audit_event) - return unless audit_event - - audit_event.security_event + def update_last_feature_flag_updated_at! + Operations::FeatureFlagsClient.update_last_feature_flag_updated_at!(project) end def sync_to_jira(feature_flag) diff --git a/app/services/feature_flags/create_service.rb b/app/services/feature_flags/create_service.rb index 6ea40345191ebc..2a3153e6a54b0a 100644 --- a/app/services/feature_flags/create_service.rb +++ b/app/services/feature_flags/create_service.rb @@ -21,6 +21,16 @@ def execute private + def audit_context(feature_flag) + { + name: 'feature_flag_created', + message: audit_message(feature_flag), + author: current_user, + scope: feature_flag.project, + target: feature_flag + } + end + def audit_message(feature_flag) message_parts = ["Created feature flag #{feature_flag.name} with description \"#{feature_flag.description}\"."] diff --git a/app/services/feature_flags/destroy_service.rb b/app/services/feature_flags/destroy_service.rb index 0fdc890b8a39a5..fdcbb802b162e7 100644 --- a/app/services/feature_flags/destroy_service.rb +++ b/app/services/feature_flags/destroy_service.rb @@ -22,6 +22,16 @@ def destroy_feature_flag(feature_flag) end end + def audit_context(feature_flag) + { + name: 'feature_flag_deleted', + message: audit_message(feature_flag), + author: current_user, + scope: feature_flag.project, + target: feature_flag + } + end + def audit_message(feature_flag) "Deleted feature flag #{feature_flag.name}." end diff --git a/app/services/feature_flags/update_service.rb b/app/services/feature_flags/update_service.rb index a465ca1dd5fff5..555b5a93d23ccd 100644 --- a/app/services/feature_flags/update_service.rb +++ b/app/services/feature_flags/update_service.rb @@ -25,13 +25,13 @@ def execute(feature_flag) end end - # We generate the audit event before the feature flag is saved as #changed_strategies_messages depends on the strategies' states before save - audit_event = audit_event(feature_flag) + # We generate the audit context before the feature flag is saved as #changed_strategies_messages depends on the strategies' states before save + saved_audit_context = audit_context feature_flag if feature_flag.save update_last_feature_flag_updated_at! - success(feature_flag: feature_flag, audit_event: audit_event) + success(feature_flag: feature_flag, audit_context: saved_audit_context) else error(feature_flag.errors.full_messages, :bad_request) end @@ -50,6 +50,16 @@ def execute_hooks_after_commit(feature_flag) end end + def audit_context(feature_flag) + { + name: 'feature_flag_updated', + message: audit_message(feature_flag), + author: current_user, + scope: feature_flag.project, + target: feature_flag + } + end + def audit_message(feature_flag) changes = changed_attributes_messages(feature_flag) changes += changed_strategies_messages(feature_flag) diff --git a/config/audit_events/types/feature_flag_created.yml b/config/audit_events/types/feature_flag_created.yml new file mode 100644 index 00000000000000..053580879fdcf4 --- /dev/null +++ b/config/audit_events/types/feature_flag_created.yml @@ -0,0 +1,9 @@ +--- +name: feature_flag_created +description: Triggered when a feature flag is created. +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374109 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113453 +feature_category: feature_flags +milestone: '15.10' +saved_to_database: true +streamed: true diff --git a/config/audit_events/types/feature_flag_deleted.yml b/config/audit_events/types/feature_flag_deleted.yml new file mode 100644 index 00000000000000..3de626409d5227 --- /dev/null +++ b/config/audit_events/types/feature_flag_deleted.yml @@ -0,0 +1,9 @@ +--- +name: feature_flag_deleted +description: Triggered when a feature flag is deleted. +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374109 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113453 +feature_category: feature_flags +milestone: '15.10' +saved_to_database: true +streamed: true diff --git a/config/audit_events/types/feature_flag_updated.yml b/config/audit_events/types/feature_flag_updated.yml new file mode 100644 index 00000000000000..0314684cb48f69 --- /dev/null +++ b/config/audit_events/types/feature_flag_updated.yml @@ -0,0 +1,9 @@ +--- +name: feature_flag_updated +description: Triggered when a feature flag is updated. +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/374109 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113453 +feature_category: feature_flags +milestone: '15.10' +saved_to_database: true +streamed: true diff --git a/spec/services/feature_flags/create_service_spec.rb b/spec/services/feature_flags/create_service_spec.rb index 3ad5287059feef..18c48714ccd593 100644 --- a/spec/services/feature_flags/create_service_spec.rb +++ b/spec/services/feature_flags/create_service_spec.rb @@ -46,6 +46,8 @@ end context 'when feature flag is saved correctly' do + let(:audit_event_details) { AuditEvent.last.details } + let(:audit_event_message) { audit_event_details[:custom_message] } let(:params) do { name: 'feature_flag', @@ -88,9 +90,9 @@ it 'creates audit event', :with_license do expect { subject }.to change { AuditEvent.count }.by(1) - expect(AuditEvent.last.details[:custom_message]).to start_with('Created feature flag feature_flag with description "description".') - expect(AuditEvent.last.details[:custom_message]).to include('Created strategy "default" with scopes "*".') - expect(AuditEvent.last.details[:custom_message]).to include('Created strategy "default" with scopes "production".') + expect(audit_event_message).to start_with('Created feature flag feature_flag with description "description".') + expect(audit_event_message).to include('Created strategy "default" with scopes "*".') + expect(audit_event_message).to include('Created strategy "default" with scopes "production".') end context 'when user is reporter' do diff --git a/spec/services/feature_flags/destroy_service_spec.rb b/spec/services/feature_flags/destroy_service_spec.rb index 7dbd8d53f2c9bc..1ec0ee6e68ce00 100644 --- a/spec/services/feature_flags/destroy_service_spec.rb +++ b/spec/services/feature_flags/destroy_service_spec.rb @@ -20,7 +20,8 @@ describe '#execute' do subject { described_class.new(project, user, params).execute(feature_flag) } - let(:audit_event_message) { AuditEvent.last.details[:custom_message] } + let(:audit_event_details) { AuditEvent.last.details } + let(:audit_event_message) { audit_event_details[:custom_message] } let(:params) { {} } it 'returns status success' do diff --git a/spec/services/feature_flags/update_service_spec.rb b/spec/services/feature_flags/update_service_spec.rb index fe26117537429b..55c09f06f1676f 100644 --- a/spec/services/feature_flags/update_service_spec.rb +++ b/spec/services/feature_flags/update_service_spec.rb @@ -19,9 +19,8 @@ subject { described_class.new(project, user, params).execute(feature_flag) } let(:params) { { name: 'new_name' } } - let(:audit_event_message) do - AuditEvent.last.details[:custom_message] - end + let(:audit_event_details) { AuditEvent.last.details } + let(:audit_event_message) { audit_event_details[:custom_message] } it 'returns success status' do expect(subject[:status]).to eq(:success) -- GitLab From 81e29155deec904cd32c18b95178549373d41f30 Mon Sep 17 00:00:00 2001 From: Sam Figueroa Date: Thu, 9 Mar 2023 09:58:49 +0000 Subject: [PATCH 2/2] Add FeatureFlag changes to permitted FOSS audit events - Refs: https://gitlab.com/gitlab-org/gitlab/-/issues/374109#note_1306152810 Changelog: changed --- lib/gitlab/audit/auditor.rb | 10 +++++++- spec/lib/gitlab/audit/auditor_spec.rb | 33 ++++++++++++++++++++------- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/lib/gitlab/audit/auditor.rb b/lib/gitlab/audit/auditor.rb index fddc1f830aa2bc..e3d2b394404ff0 100644 --- a/lib/gitlab/audit/auditor.rb +++ b/lib/gitlab/audit/auditor.rb @@ -5,6 +5,10 @@ module Audit class Auditor attr_reader :scope, :name + PERMITTED_TARGET_CLASSES = [ + ::Operations::FeatureFlag + ].freeze + # Record audit events # # @param [Hash] context @@ -113,7 +117,11 @@ def log_to_file_and_stream(events) end def audit_enabled? - authentication_event? + authentication_event? || permitted_target? + end + + def permitted_target? + @target.class.in? PERMITTED_TARGET_CLASSES end def authentication_event? diff --git a/spec/lib/gitlab/audit/auditor_spec.rb b/spec/lib/gitlab/audit/auditor_spec.rb index 4b16333d913c7e..2b3c8506440885 100644 --- a/spec/lib/gitlab/audit/auditor_spec.rb +++ b/spec/lib/gitlab/audit/auditor_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Audit::Auditor do +RSpec.describe Gitlab::Audit::Auditor, feature_category: :audit_events do let(:name) { 'audit_operation' } let(:author) { create(:user, :with_sign_ins) } let(:group) { create(:group) } @@ -22,9 +22,9 @@ subject(:auditor) { described_class } describe '.audit' do - context 'when authentication event' do - let(:audit!) { auditor.audit(context) } + let(:audit!) { auditor.audit(context) } + context 'when authentication event' do it 'creates an authentication event' do expect(AuthenticationEvent).to receive(:new).with( { @@ -210,19 +210,38 @@ end context 'when authentication event is false' do + let(:target) { group } let(:context) do { name: name, author: author, scope: group, - target: group, authentication_event: false, message: "sample message" } + target: target, authentication_event: false, message: "sample message" } end it 'does not create an authentication event' do expect { auditor.audit(context) }.not_to change(AuthenticationEvent, :count) end + + context 'with permitted target' do + { feature_flag: :operations_feature_flag }.each do |target_type, factory_name| + context "with #{target_type}" do + let(:target) { build_stubbed factory_name } + + it 'logs audit events to database', :aggregate_failures, :freeze_time do + audit! + audit_event = AuditEvent.last + + expect(audit_event.author_id).to eq(author.id) + expect(audit_event.entity_id).to eq(group.id) + expect(audit_event.entity_type).to eq(group.class.name) + expect(audit_event.created_at).to eq(Time.zone.now) + expect(audit_event.details[:target_id]).to eq(target.id) + expect(audit_event.details[:target_type]).to eq(target.class.name) + end + end + end + end end context 'when authentication event is invalid' do - let(:audit!) { auditor.audit(context) } - before do allow(AuthenticationEvent).to receive(:new).and_raise(ActiveRecord::RecordInvalid) allow(Gitlab::ErrorTracking).to receive(:track_exception) @@ -243,8 +262,6 @@ end context 'when audit events are invalid' do - let(:audit!) { auditor.audit(context) } - before do expect_next_instance_of(AuditEvent) do |instance| allow(instance).to receive(:save!).and_raise(ActiveRecord::RecordInvalid) -- GitLab