From c8a7364ffed4cdbc893fc23a7e8b6e4df6570667 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi <hraghuvanshi@gitlab.com> Date: Tue, 27 Jun 2023 17:13:58 +0530 Subject: [PATCH 1/6] Adding instance scope for audit events Changelog: added EE: true A new scope or entity type for audit events is added for instance level audit events --- .../base.rb | 12 ++++++ .../create.rb | 2 +- ee/app/models/ee/audit_event.rb | 12 +++++- ee/lib/ee/gitlab/audit/auditor.rb | 2 +- ee/lib/gitlab/audit/instance_scope.rb | 22 +++++++++++ ee/spec/lib/gitlab/audit/auditor_spec.rb | 38 ++++++++++++++----- .../lib/gitlab/audit/instance_scope_spec.rb | 29 ++++++++++++++ ee/spec/models/ee/audit_event_spec.rb | 10 +++++ .../create_spec.rb | 17 +++++++++ .../audit_event_streaming_worker_spec.rb | 24 ++++++++++++ spec/factories/audit_events.rb | 23 +++++++++++ 11 files changed, 179 insertions(+), 12 deletions(-) create mode 100644 ee/lib/gitlab/audit/instance_scope.rb create mode 100644 ee/spec/lib/gitlab/audit/instance_scope_spec.rb diff --git a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/base.rb b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/base.rb index 080dae930c075af7..eaccbf168e5d9886 100644 --- a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/base.rb +++ b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/base.rb @@ -25,6 +25,18 @@ def find_object(destination_gid) destination end + + def audit(destination, action:, extra_context: {}) + audit_context = { + name: "#{action}_instance_event_streaming_destination", + author: current_user, + scope: Gitlab::Audit::InstanceScope.new, + target: destination, + message: "#{action.capitalize} instance event streaming destination #{destination.destination_url}" + } + + ::Gitlab::Audit::Auditor.audit(audit_context.merge(extra_context)) + end end end end diff --git a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/create.rb b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/create.rb index a2ce85af7299ac43..4e3920e83fdcc7f7 100644 --- a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/create.rb +++ b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/create.rb @@ -19,7 +19,7 @@ class Create < Base def resolve(destination_url:) destination = ::AuditEvents::InstanceExternalAuditEventDestination.new(destination_url: destination_url) - destination.save + audit(destination, action: :create) if destination.save { instance_external_audit_event_destination: (destination if destination.persisted?), diff --git a/ee/app/models/ee/audit_event.rb b/ee/app/models/ee/audit_event.rb index 9c9e7ef561aab5ed..92f95182413ac44b 100644 --- a/ee/app/models/ee/audit_event.rb +++ b/ee/app/models/ee/audit_event.rb @@ -21,7 +21,17 @@ module AuditEvent attr_accessor :root_group_entity_id def entity - strong_memoize(:entity) { lazy_entity } + strong_memoize(:entity) do + if entity_type == ::Gitlab::Audit::InstanceScope.name + ::Gitlab::Audit::InstanceScope.new + else + lazy_entity + end + end + end + + def default_scope_value + Gitlab::Audit::InstanceScope.new end def root_group_entity diff --git a/ee/lib/ee/gitlab/audit/auditor.rb b/ee/lib/ee/gitlab/audit/auditor.rb index a81fb6af552d3066..7663ee71acf462f1 100644 --- a/ee/lib/ee/gitlab/audit/auditor.rb +++ b/ee/lib/ee/gitlab/audit/auditor.rb @@ -37,7 +37,7 @@ def audit_enabled? return true if ::License.feature_available?(:admin_audit_log) return true if ::License.feature_available?(:extended_audit_events) - scope.respond_to?(:feature_available?) && scope.licensed_feature_available?(:audit_events) + scope.respond_to?(:licensed_feature_available?) && scope.licensed_feature_available?(:audit_events) end end end diff --git a/ee/lib/gitlab/audit/instance_scope.rb b/ee/lib/gitlab/audit/instance_scope.rb new file mode 100644 index 0000000000000000..33e51609268b387a --- /dev/null +++ b/ee/lib/gitlab/audit/instance_scope.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Gitlab + module Audit + class InstanceScope + SCOPE_NAME = "gitlab_instance" + SCOPE_ID = 1 + + attr_reader :id, :name, :full_path + + def initialize + @id = SCOPE_ID + @name = SCOPE_NAME + @full_path = SCOPE_NAME + end + + def licensed_feature_available?(feature) + ::License.feature_available?(feature) + end + end + end +end diff --git a/ee/spec/lib/gitlab/audit/auditor_spec.rb b/ee/spec/lib/gitlab/audit/auditor_spec.rb index 1cc4baf6cc567873..b66219a0e4a27a92 100644 --- a/ee/spec/lib/gitlab/audit/auditor_spec.rb +++ b/ee/spec/lib/gitlab/audit/auditor_spec.rb @@ -304,19 +304,23 @@ } end - it 'logs audit event to database', :aggregate_failures do - expect { audit! }.to change(AuditEvent, :count).by(1) + shared_examples 'logs event to database' do + it 'logs audit event to database', :aggregate_failures do + expect { audit! }.to change(AuditEvent, :count).by(1) - audit_event = AuditEvent.last + audit_event = AuditEvent.last - expect(audit_event.author_id).to eq(author.id) - expect(audit_event.entity_id).to eq(scope.id) - expect(audit_event.entity_type).to eq(scope.class.name) - expect(audit_event.details[:target_id]).to eq(target.id) - expect(audit_event.details[:target_type]).to eq(target.class.name) - expect(audit_event.details[:custom_message]).to eq('Project has been deleted') + expect(audit_event.author_id).to eq(author.id) + expect(audit_event.entity_id).to eq(scope.id) + expect(audit_event.entity_type).to eq(scope.class.name) + expect(audit_event.details[:target_id]).to eq(target.id) + expect(audit_event.details[:target_type]).to eq(target.class.name) + expect(audit_event.details[:custom_message]).to eq('Project has been deleted') + end end + it_behaves_like 'logs event to database' + it 'does not bulk insert and uses save to insert' do expect(AuditEvent).not_to receive(:bulk_insert!) expect_next_instance_of(AuditEvent) do |instance| @@ -359,6 +363,22 @@ it_behaves_like 'only streamed' end + + context 'when the scope of event is instance' do + let(:scope) { Gitlab::Audit::InstanceScope.new } + + let(:context) do + { + name: name, + author: author, + scope: scope, + target: target, + message: 'Project has been deleted' + } + end + + it_behaves_like 'logs event to database' + end end context 'when audit events are invalid' do diff --git a/ee/spec/lib/gitlab/audit/instance_scope_spec.rb b/ee/spec/lib/gitlab/audit/instance_scope_spec.rb new file mode 100644 index 0000000000000000..90c48a7d9e1cef79 --- /dev/null +++ b/ee/spec/lib/gitlab/audit/instance_scope_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Audit::InstanceScope, feature_category: :audit_events do + describe '#initialize' do + it 'sets correct attributes' do + expect(described_class.new) + .to have_attributes(id: 1, name: Gitlab::Audit::InstanceScope::SCOPE_NAME, + full_path: Gitlab::Audit::InstanceScope::SCOPE_NAME) + end + + describe '#licensed_feature_available?' do + subject { described_class.new.licensed_feature_available?(:external_audit_events) } + + context 'when license is available' do + before do + stub_licensed_features(external_audit_events: true) + end + + it { is_expected.to be_truthy } + end + + context 'when license is not available' do + it { is_expected.to be_falsey } + end + end + end +end diff --git a/ee/spec/models/ee/audit_event_spec.rb b/ee/spec/models/ee/audit_event_spec.rb index 50bd2e2145fbae5c..442700ea5c56185e 100644 --- a/ee/spec/models/ee/audit_event_spec.rb +++ b/ee/spec/models/ee/audit_event_spec.rb @@ -292,6 +292,16 @@ expect(event.entity).to be_a(Gitlab::Audit::NullEntity) end end + + context 'when entity is the instance' do + let_it_be(:instance_scope) { Gitlab::Audit::InstanceScope.new } + + subject(:event) { described_class.new(entity_id: instance_scope.id, entity_type: instance_scope.class.name) } + + it 'returns a InstanceScope object' do + expect(event.entity).to be_a(Gitlab::Audit::InstanceScope) + end + end end describe '#root_group_entity' do diff --git a/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/create_spec.rb b/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/create_spec.rb index 21e87d898919f1df..ea5643e607f6e459 100644 --- a/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/create_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/audit_events/instance_external_audit_event_destinations/create_spec.rb @@ -24,6 +24,21 @@ } end + shared_examples 'creates an audit event' do + it 'audits the creation' do + expect(AuditEvents::AuditEventStreamingWorker).to receive(:perform_async).with( + "create_instance_event_streaming_destination", + nil, + anything + ) + + expect { subject } + .to change { AuditEvent.count }.by(1) + + expect(AuditEvent.last.details[:custom_message]).to eq("Create instance event streaming destination https://gitlab.com/example/testendpoint") + end + end + shared_examples 'a mutation that does not create a destination' do subject { post_graphql_mutation(mutation, current_user: current_user) } @@ -61,6 +76,8 @@ expect(mutation_response['instanceExternalAuditEventDestination']['verificationToken']).not_to be_empty end + it_behaves_like 'creates an audit event' + context 'when destination is invalid' do let(:mutation) { graphql_mutation(:instance_external_audit_event_destination_create, invalid_input) } diff --git a/ee/spec/workers/audit_events/audit_event_streaming_worker_spec.rb b/ee/spec/workers/audit_events/audit_event_streaming_worker_spec.rb index 148128a94dc84bc7..4ffb46338847455f 100644 --- a/ee/spec/workers/audit_events/audit_event_streaming_worker_spec.rb +++ b/ee/spec/workers/audit_events/audit_event_streaming_worker_spec.rb @@ -342,6 +342,30 @@ include_context 'audit event stream' end end + + context 'when the entity is InstanceScope' do + let_it_be(:event) { create(:audit_event, :instance_event) } + + subject { worker.perform('audit_operation', nil, event.to_json) } + + context 'when the gitlab instance has an external destination' do + let_it_be(:destination) { create(:instance_external_audit_event_destination) } + + it 'receives HTTP call at destination' do + expect(Gitlab::HTTP).to receive(:post).with(destination.destination_url, anything).once + + subject + end + end + + context 'when the gitlab instance does not have any external destination' do + let_it_be(:event) { create(:audit_event, :instance_event) } + + subject { worker.perform('audit_operation', nil, event.to_json) } + + it_behaves_like 'no HTTP calls are made' + end + end end context 'when connecting to redis fails' do diff --git a/spec/factories/audit_events.rb b/spec/factories/audit_events.rb index 10f60591922147ee..ceb7516441f2aa5d 100644 --- a/spec/factories/audit_events.rb +++ b/spec/factories/audit_events.rb @@ -88,6 +88,29 @@ end end + trait :instance_event do + transient { instance_scope { Gitlab::Audit::InstanceScope.new } } + + entity_type { Gitlab::Audit::InstanceScope.name } + entity_id { instance_scope.id } + entity_path { instance_scope.full_path } + target_details { instance_scope.name } + ip_address { IPAddr.new '127.0.0.1' } + details do + { + change: 'project_creation_level', + from: nil, + to: 'Developers + Maintainers', + author_name: user.name, + target_id: instance_scope.id, + target_type: Gitlab::Audit::InstanceScope.name, + target_details: instance_scope.name, + ip_address: '127.0.0.1', + entity_path: instance_scope.full_path + } + end + end + factory :project_audit_event, traits: [:project_event] factory :group_audit_event, traits: [:group_event] end -- GitLab From 14d139ebdb3ed30cf0437a63796f7526bc52038b Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi <hraghuvanshi@gitlab.com> Date: Fri, 30 Jun 2023 16:12:32 +0530 Subject: [PATCH 2/6] Fixed issue with audit event presenter --- ee/app/models/ee/audit_event.rb | 20 +++++++++---------- ee/app/presenters/audit_event_presenter.rb | 2 +- ...e_instance_event_streaming_destination.yml | 8 ++++++++ ee/lib/gitlab/audit/events/preloader.rb | 4 ++-- .../lib/gitlab/audit/events/preloader_spec.rb | 6 +++--- 5 files changed, 24 insertions(+), 16 deletions(-) create mode 100644 ee/config/audit_events/types/create_instance_event_streaming_destination.yml diff --git a/ee/app/models/ee/audit_event.rb b/ee/app/models/ee/audit_event.rb index 92f95182413ac44b..c409add5ecf95a41 100644 --- a/ee/app/models/ee/audit_event.rb +++ b/ee/app/models/ee/audit_event.rb @@ -67,16 +67,6 @@ def ip_address super&.to_s || details[:ip_address] end - def lazy_entity - BatchLoader.for(entity_id) - .batch( - key: entity_type, default_value: ::Gitlab::Audit::NullEntity.new - ) do |ids, loader, args| - model = Object.const_get(args[:key], false) - model.where(id: ids).find_each { |record| loader.call(record.id, record) } - end - end - def stream_to_external_destinations(use_json: false, event_name: 'audit_operation') return unless can_stream_to_external_destination?(event_name) @@ -90,6 +80,16 @@ def entity_is_group_or_project? private + def lazy_entity + BatchLoader.for(entity_id) + .batch( + key: entity_type, default_value: ::Gitlab::Audit::NullEntity.new + ) do |ids, loader, args| + model = Object.const_get(args[:key], false) + model.where(id: ids).find_each { |record| loader.call(record.id, record) } + end + end + def can_stream_to_external_destination?(event_name) return false if entity.nil? diff --git a/ee/app/presenters/audit_event_presenter.rb b/ee/app/presenters/audit_event_presenter.rb index a2f473ef79d248e2..c1536476f7b9b3dd 100644 --- a/ee/app/presenters/audit_event_presenter.rb +++ b/ee/app/presenters/audit_event_presenter.rb @@ -57,6 +57,6 @@ def author end def entity - @entity ||= audit_event.lazy_entity + @entity ||= audit_event.entity end end diff --git a/ee/config/audit_events/types/create_instance_event_streaming_destination.yml b/ee/config/audit_events/types/create_instance_event_streaming_destination.yml new file mode 100644 index 0000000000000000..988d61ccdd9896da --- /dev/null +++ b/ee/config/audit_events/types/create_instance_event_streaming_destination.yml @@ -0,0 +1,8 @@ +name: create_instance_event_streaming_destination +description: Event triggered when an instance level external audit event destination is created +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/404730 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/123882 +feature_category: audit_events +milestone: "16.2" +saved_to_database: true +streamed: true diff --git a/ee/lib/gitlab/audit/events/preloader.rb b/ee/lib/gitlab/audit/events/preloader.rb index a5762e2d8b9955f4..a8b45eeebad50b1e 100644 --- a/ee/lib/gitlab/audit/events/preloader.rb +++ b/ee/lib/gitlab/audit/events/preloader.rb @@ -8,7 +8,7 @@ def self.preload!(audit_events) audit_events.tap do |audit_events| audit_events.each do |audit_event| audit_event.lazy_author - audit_event.lazy_entity + audit_event.entity end end end @@ -21,7 +21,7 @@ def find_each(&block) @audit_events.each_batch(column: :created_at) do |relation| relation.each do |audit_event| audit_event.lazy_author - audit_event.lazy_entity + audit_event.entity end relation.each do |audit_event| diff --git a/ee/spec/lib/gitlab/audit/events/preloader_spec.rb b/ee/spec/lib/gitlab/audit/events/preloader_spec.rb index dc6f482614f89a4c..a9a7a9f42c5f1499 100644 --- a/ee/spec/lib/gitlab/audit/events/preloader_spec.rb +++ b/ee/spec/lib/gitlab/audit/events/preloader_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Audit::Events::Preloader do +RSpec.describe Gitlab::Audit::Events::Preloader, feature_category: :audit_events do let_it_be(:audit_events) do [ create(:audit_event, created_at: 2.days.ago), @@ -31,7 +31,7 @@ # expect do subject.map do |event| - [event.author_name, event.lazy_entity.name] + [event.author_name, event.entity.name] end end.not_to exceed_query_limit(3) end @@ -59,7 +59,7 @@ # SELECT "users".* FROM "users" WHERE "users"."id" IN (2, 4) ORDER BY "users"."id" ASC LIMIT 1000 expect do preloader.find_each do |event| - [event.author_name, event.lazy_entity.name] + [event.author_name, event.entity.name] end end.not_to exceed_query_limit(5) end -- GitLab From 4be346cbfb4ac32f58717fbf73cc416a989e9648 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi <hraghuvanshi@gitlab.com> Date: Fri, 30 Jun 2023 18:17:16 +0530 Subject: [PATCH 3/6] Removed extra param --- .../instance_external_audit_event_destinations/base.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/base.rb b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/base.rb index eaccbf168e5d9886..d02d97ef3d24445a 100644 --- a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/base.rb +++ b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/base.rb @@ -26,7 +26,7 @@ def find_object(destination_gid) destination end - def audit(destination, action:, extra_context: {}) + def audit(destination, action:) audit_context = { name: "#{action}_instance_event_streaming_destination", author: current_user, -- GitLab From 3bce7f5e9b26cf8dd8c6d53abd1c84c9d359bf23 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi <hraghuvanshi@gitlab.com> Date: Fri, 30 Jun 2023 18:17:22 +0530 Subject: [PATCH 4/6] Removed extra param --- .../instance_external_audit_event_destinations/base.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/base.rb b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/base.rb index d02d97ef3d24445a..fc68738d118024d8 100644 --- a/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/base.rb +++ b/ee/app/graphql/mutations/audit_events/instance_external_audit_event_destinations/base.rb @@ -35,7 +35,7 @@ def audit(destination, action:) message: "#{action.capitalize} instance event streaming destination #{destination.destination_url}" } - ::Gitlab::Audit::Auditor.audit(audit_context.merge(extra_context)) + ::Gitlab::Audit::Auditor.audit(audit_context) end end end -- GitLab From 8f8acf86875c6e0aa515281edb863559c5481e0d Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi <hraghuvanshi@gitlab.com> Date: Tue, 4 Jul 2023 15:15:00 +0530 Subject: [PATCH 5/6] Added feature test cases --- ee/app/presenters/audit_event_presenter.rb | 3 ++- .../features/admin/admin_audit_logs_spec.rb | 23 +++++++++++++++++++ .../presenters/audit_event_presenter_spec.rb | 12 ++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/ee/app/presenters/audit_event_presenter.rb b/ee/app/presenters/audit_event_presenter.rb index c1536476f7b9b3dd..1c2c6ed6bbf36fda 100644 --- a/ee/app/presenters/audit_event_presenter.rb +++ b/ee/app/presenters/audit_event_presenter.rb @@ -36,8 +36,9 @@ def object def object_url return if entity.is_a?(Gitlab::Audit::NullEntity) - url_for(entity) + return Gitlab::Routing.url_helpers.admin_root_url if entity.is_a?(Gitlab::Audit::InstanceScope) + url_for(entity) rescue NoMethodError '' end diff --git a/ee/spec/features/admin/admin_audit_logs_spec.rb b/ee/spec/features/admin/admin_audit_logs_spec.rb index 63d481f549a523c1..8d4a82d2a0872072 100644 --- a/ee/spec/features/admin/admin_audit_logs_spec.rb +++ b/ee/spec/features/admin/admin_audit_logs_spec.rb @@ -103,6 +103,29 @@ end end + describe 'instance events' do + let(:destination) { create(:instance_external_audit_event_destination) } + + before do + audit_context = { + name: "create_instance_event_streaming_destination", + author: admin, + scope: Gitlab::Audit::InstanceScope.new, + target: destination, + message: "Create instance event streaming destination #{destination.destination_url}" + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + + visit admin_audit_logs_path + end + + it 'has instance audit event' do + expect(page).to have_content('gitlab_instance') + expect(page).to have_content('Create instance event streaming destination') + end + end + describe 'filter by date' do let_it_be(:audit_event_1) { create(:user_audit_event, created_at: 5.days.ago) } let_it_be(:audit_event_2) { create(:user_audit_event, created_at: 3.days.ago) } diff --git a/ee/spec/presenters/audit_event_presenter_spec.rb b/ee/spec/presenters/audit_event_presenter_spec.rb index 5c29ab974450d8ed..b69ee88fd71f8cdb 100644 --- a/ee/spec/presenters/audit_event_presenter_spec.rb +++ b/ee/spec/presenters/audit_event_presenter_spec.rb @@ -121,6 +121,18 @@ expect(presenter.object_url).to be_blank end + context 'when object is of type instance scope' do + let_it_be(:audit_event) do + create( + :audit_event, :instance_event + ) + end + + it 'returns the instance admin root url' do + expect(presenter.object_url).to eq(Gitlab::Routing.url_helpers.admin_root_url) + end + end + context 'when a project in a user namespace has been deleted' do let(:project) { build(:project, namespace: create(:user).namespace).destroy! } let(:audit_event) do -- GitLab From e12042f28f3647367908fbc07c8024df17bc1b02 Mon Sep 17 00:00:00 2001 From: Hitesh Raghuvanshi <hraghuvanshi@gitlab.com> Date: Wed, 5 Jul 2023 16:11:07 +0530 Subject: [PATCH 6/6] Removing unused method --- ee/app/models/ee/audit_event.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/ee/app/models/ee/audit_event.rb b/ee/app/models/ee/audit_event.rb index c409add5ecf95a41..19ef1c7459991417 100644 --- a/ee/app/models/ee/audit_event.rb +++ b/ee/app/models/ee/audit_event.rb @@ -30,10 +30,6 @@ def entity end end - def default_scope_value - Gitlab::Audit::InstanceScope.new - end - def root_group_entity strong_memoize(:root_group_entity) do next ::Group.find_by(id: root_group_entity_id) if root_group_entity_id.present? -- GitLab