From a1f6bd7789ec93e59a512645788b3cb3e107ba26 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Fri, 8 Jul 2022 15:01:27 +0530 Subject: [PATCH] Event type information in protected branch audit event stream EE: true Changelog: added --- .../protected_branch_audit_event_service.rb | 40 +++++++++---- .../ee/protected_branches/loggable.rb | 5 +- ee/lib/audit/changes.rb | 4 +- .../protected_branches_changes_auditor.rb | 35 +++++++---- ...protected_branches_changes_auditor_spec.rb | 38 ++++++++++-- ...otected_branch_audit_event_service_spec.rb | 60 +++++++++++-------- 6 files changed, 123 insertions(+), 59 deletions(-) diff --git a/ee/app/services/audit_events/protected_branch_audit_event_service.rb b/ee/app/services/audit_events/protected_branch_audit_event_service.rb index 5d2a2fc6850bb9..b7092b83cad068 100644 --- a/ee/app/services/audit_events/protected_branch_audit_event_service.rb +++ b/ee/app/services/audit_events/protected_branch_audit_event_service.rb @@ -1,24 +1,40 @@ # frozen_string_literal: true module AuditEvents - class ProtectedBranchAuditEventService < ::AuditEventService + class ProtectedBranchAuditEventService attr_accessor :protected_branch def initialize(author, protected_branch, action) @action = action @protected_branch = protected_branch + @author = author + end + + def execute + audit_context = { + author: @author, + scope: @protected_branch.project, + target: @protected_branch, + message: message, + name: event_type, + additional_details: { + push_access_levels: @protected_branch.push_access_levels.map(&:humanize), + merge_access_levels: @protected_branch.merge_access_levels.map(&:humanize), + allow_force_push: @protected_branch.allow_force_push, + code_owner_approval_required: @protected_branch.code_owner_approval_required + } + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end - super(author, protected_branch.project, - { author_name: author.name, - custom_message: message, - target_id: protected_branch.id, - target_type: protected_branch.class.name, - target_details: protected_branch.name, - push_access_levels: protected_branch.push_access_levels.map(&:humanize), - merge_access_levels: protected_branch.merge_access_levels.map(&:humanize), - allow_force_push: protected_branch.allow_force_push, - code_owner_approval_required: protected_branch.code_owner_approval_required } - ) + def event_type + case @action + when :add + "protected_branch_created" + when :remove + "protected_branch_removed" + end end def message diff --git a/ee/app/services/ee/protected_branches/loggable.rb b/ee/app/services/ee/protected_branches/loggable.rb index 1328e5ceb4f1e4..792aa8265b417e 100644 --- a/ee/app/services/ee/protected_branches/loggable.rb +++ b/ee/app/services/ee/protected_branches/loggable.rb @@ -5,9 +5,8 @@ module ProtectedBranches module Loggable def log_audit_event(protected_branch_service, action) if protected_branch_service.errors.blank? - ::AuditEvents::ProtectedBranchAuditEventService - .new(current_user, protected_branch_service, action) - .security_event + ::AuditEvents::ProtectedBranchAuditEventService.new(current_user, protected_branch_service, action).execute + end end end diff --git a/ee/lib/audit/changes.rb b/ee/lib/audit/changes.rb index 1fcb44034b2a00..c510cc522317f3 100644 --- a/ee/lib/audit/changes.rb +++ b/ee/lib/audit/changes.rb @@ -89,8 +89,8 @@ def additional_details(options) def build_message(details) message = ["Changed #{details[:change]}"] - message << "from #{details[:from]}" unless details[:from].blank? - message << "to #{details[:to]}" unless details[:to].blank? + message << "from #{details[:from]}" if details[:from].to_s.present? + message << "to #{details[:to]}" if details[:to].to_s.present? message.join(' ') end diff --git a/ee/lib/ee/audit/protected_branches_changes_auditor.rb b/ee/lib/ee/audit/protected_branches_changes_auditor.rb index 64513ae25e2467..f634fb6517a5e4 100644 --- a/ee/lib/ee/audit/protected_branches_changes_auditor.rb +++ b/ee/lib/ee/audit/protected_branches_changes_auditor.rb @@ -10,8 +10,10 @@ def initialize(current_user, model, old_merge_access_levels, old_push_access_lev end def execute - audit_changes(:allow_force_push, as: 'allow force push', entity: model.project, model: model) - audit_changes(:code_owner_approval_required, as: 'code owner approval required', entity: model.project, model: model) + audit_changes(:allow_force_push, as: 'allow force push', entity: model.project, + model: model, event_type: event_type) + audit_changes(:code_owner_approval_required, as: 'code owner approval required', + entity: model.project, model: model, event_type: event_type) audit_access_levels end @@ -23,15 +25,22 @@ def audit_access_levels access_inputs.each do |change, old_access_levels, new_access_levels| unless old_access_levels == new_access_levels - details = { - change: change, - from: old_access_levels.map(&:humanize), - to: new_access_levels.map(&:humanize), - target_id: model.id, - target_type: model.class.name, - target_details: model.name + from = old_access_levels.map(&:humanize) + to = new_access_levels.map(&:humanize) + audit_context = { + author: @current_user, + scope: model.project, + target: model, + message: "Changed #{change} from #{from} to #{to}", + name: event_type, + additional_details: { + change: change, + from: from, + to: to + } } - ::AuditEventService.new(@current_user, model.project, details).security_event + + ::Gitlab::Audit::Auditor.audit(audit_context) end end end @@ -45,6 +54,12 @@ def attributes_from_auditable_model(column) to: new } end + + private + + def event_type + 'protected_branch_updated' + end end end end diff --git a/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb b/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb index 8580a906ec59a7..503b9ee75c91bd 100644 --- a/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb +++ b/ee/spec/lib/ee/audit/protected_branches_changes_auditor_spec.rb @@ -5,13 +5,15 @@ RSpec.describe EE::Audit::ProtectedBranchesChangesAuditor, :request_store do let_it_be(:author) { create(:user, :with_sign_ins) } let_it_be(:user) { create(:user, :with_sign_ins) } - let_it_be(:entity) { create(:project, creator: author) } + let_it_be(:group) { create(:group) } + let_it_be(:destination) { create(:external_audit_event_destination, group: group) } + let_it_be(:entity) { create(:project, creator: author, group: group) } let(:protected_branch) { create(:protected_branch, :no_one_can_push, :no_one_can_merge, allow_force_push: false, code_owner_approval_required: false, project: entity) } let(:ip_address) { '192.168.15.18' } before do - stub_licensed_features(admin_audit_log: true, code_owner_approval_required: true) + stub_licensed_features(admin_audit_log: true, external_audit_events: true, code_owner_approval_required: true) allow(Gitlab::RequestContext.instance).to receive(:client_ip).and_return(ip_address) end @@ -42,13 +44,23 @@ from: false, to: true, ip_address: ip_address, - custom_message: "Changed #{change_text} to true" }) + custom_message: "Changed #{change_text} from false to true" }) expect(event.author_id).to eq(author.id) expect(event.entity_id).to eq(entity.id) expect(event.entity_type).to eq('Project') expect(event.ip_address).to eq(ip_address) end + + it 'streams correct audit event stream' do + protected_branch.update_attribute(setting, true) + + expect(AuditEvents::AuditEventStreamingWorker).to receive(:perform_async).with('protected_branch_updated', + anything, + anything) + + service.execute + end end end @@ -67,21 +79,35 @@ expect { service.execute }.to change { AuditEvent.count }.by(1) event = AuditEvent.last + from = old_access_levels.map(&:humanize) + to = new_access_levels.map(&:humanize) + expect(event.details).to eq({ change: change_text, - from: old_access_levels.map(&:humanize), - to: new_access_levels.map(&:humanize), + from: from, + to: to, target_id: protected_branch.id, target_type: "ProtectedBranch", target_details: protected_branch.name, ip_address: ip_address, entity_path: entity.full_path, - author_name: author.name }) + author_name: author.name, + custom_message: "Changed #{change_text} from #{from} to #{to}" }) expect(event.author_id).to eq(author.id) expect(event.entity_id).to eq(entity.id) expect(event.entity_type).to eq('Project') expect(event.ip_address).to eq(ip_address) end + + it 'streams correct audit event stream' do + new_access_levels.new(user: user) + + expect(AuditEvents::AuditEventStreamingWorker).to receive(:perform_async).with('protected_branch_updated', + anything, + anything) + + service.execute + end end end end diff --git a/ee/spec/services/audit_events/protected_branch_audit_event_service_spec.rb b/ee/spec/services/audit_events/protected_branch_audit_event_service_spec.rb index c07b7d45ba1287..2d05b154352a74 100644 --- a/ee/spec/services/audit_events/protected_branch_audit_event_service_spec.rb +++ b/ee/spec/services/audit_events/protected_branch_audit_event_service_spec.rb @@ -6,24 +6,26 @@ let(:merge_level) { 'Maintainers' } let(:push_level) { 'No one' } let_it_be(:author) { create(:user, :with_sign_ins) } - let_it_be(:entity) { create(:project, creator: author) } + let_it_be(:group) { create(:group) } + let_it_be(:destination) { create(:external_audit_event_destination, group: group) } + let_it_be(:entity) { create(:project, creator: author, group: group) } let_it_be(:protected_branch) { create(:protected_branch, :no_one_can_push, allow_force_push: true, code_owner_approval_required: true, project: entity) } let(:logger) { instance_spy(Gitlab::AuditJsonLogger) } let(:ip_address) { '192.168.15.18' } describe '#security_event' do - shared_examples 'loggable' do |action| + shared_examples 'loggable' do |action, event_type| context "when a protected_branch is #{action}" do let(:service) { described_class.new(author, protected_branch, action) } before do - stub_licensed_features(admin_audit_log: true, code_owner_approval_required: true) + stub_licensed_features(admin_audit_log: true, external_audit_events: true, code_owner_approval_required: true) allow(Gitlab::RequestContext.instance).to receive(:client_ip).and_return(ip_address) end it 'creates an event', :aggregate_failures do - expect { service.security_event }.to change(AuditEvent, :count).by(1) + expect { service.execute }.to change(AuditEvent, :count).by(1) security_event = AuditEvent.last @@ -47,33 +49,39 @@ end it 'logs to a file with the provided details' do - allow(service).to receive(:file_logger).and_return(logger) + expect(::Gitlab::AuditJsonLogger).to receive(:build).and_return(logger) + + service.execute + + expect(logger).to have_received(:info).with(hash_including( + 'author_id' => author.id, + 'author_name' => author.name, + 'entity_id' => entity.id, + 'entity_type' => 'Project', + 'entity_path' => entity.full_path, + 'allow_force_push' => true, + 'code_owner_approval_required' => true, + 'merge_access_levels' => [merge_level], + 'push_access_levels' => [push_level], + 'target_details' => protected_branch.name, + 'target_id' => protected_branch.id, + 'target_type' => 'ProtectedBranch', + 'custom_message' => action == :add ? /Added/ : /Unprotected/, + 'ip_address' => ip_address, + 'created_at' => anything) + ) + end - service.security_event + it_behaves_like 'sends correct event type in audit event stream' do + let(:subject) { service.execute } - expect(logger).to have_received(:info).with( - author_id: author.id, - author_name: author.name, - entity_id: entity.id, - entity_type: 'Project', - entity_path: entity.full_path, - allow_force_push: true, - code_owner_approval_required: true, - merge_access_levels: [merge_level], - push_access_levels: [push_level], - target_details: protected_branch.name, - target_id: protected_branch.id, - target_type: 'ProtectedBranch', - custom_message: action == :add ? /Added/ : /Unprotected/, - ip_address: ip_address, - created_at: anything - ) + let_it_be(:event_type) { event_type } end end end - include_examples 'loggable', :add - include_examples 'loggable', :remove + include_examples 'loggable', :add, 'protected_branch_created' + include_examples 'loggable', :remove, 'protected_branch_removed' context 'when not licensed' do let(:service) { described_class.new(author, protected_branch, :add) } @@ -87,7 +95,7 @@ it "doesn't create an event or log to a file", :aggregate_failures do expect(service).not_to receive(:file_logger) - expect { service.security_event }.not_to change(AuditEvent, :count) + expect { service.execute }.not_to change(AuditEvent, :count) end end end -- GitLab