From a1ced9bbb73b7a15fbf22535ab554137ec6d863e Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Tue, 28 Feb 2023 17:51:49 +0530 Subject: [PATCH 1/7] Adds audit event for audit event filter creation EE: true Changelog: added --- .../streaming/event_type_filters/create.rb | 3 ++- .../external_audit_event_destination.rb | 4 ++++ .../event_type_filters/base_service.rb | 17 ++++++++++++++ .../event_type_filters/create_service.rb | 22 +++++++++++------- .../event_type_filters/create_service_spec.rb | 23 ++++++++++++++++++- 5 files changed, 59 insertions(+), 10 deletions(-) create mode 100644 ee/app/services/audit_events/streaming/event_type_filters/base_service.rb diff --git a/ee/app/graphql/mutations/audit_events/streaming/event_type_filters/create.rb b/ee/app/graphql/mutations/audit_events/streaming/event_type_filters/create.rb index 82250892037530..4c4e0d444e1adc 100644 --- a/ee/app/graphql/mutations/audit_events/streaming/event_type_filters/create.rb +++ b/ee/app/graphql/mutations/audit_events/streaming/event_type_filters/create.rb @@ -25,7 +25,8 @@ def resolve(destination_id:, event_type_filters:) response = ::AuditEvents::Streaming::EventTypeFilters::CreateService.new( destination: destination, - event_type_filters: event_type_filters + event_type_filters: event_type_filters, + current_user: current_user ).execute if response.success? diff --git a/ee/app/models/audit_events/external_audit_event_destination.rb b/ee/app/models/audit_events/external_audit_event_destination.rb index 131ccee8af21ad..2d2248d8ac116d 100644 --- a/ee/app/models/audit_events/external_audit_event_destination.rb +++ b/ee/app/models/audit_events/external_audit_event_destination.rb @@ -29,6 +29,10 @@ def headers_hash { STREAMING_TOKEN_HEADER_KEY => verification_token }.merge(headers.map(&:to_hash).inject(:merge).to_h) end + def audit_details + destination_url + end + private def has_fewer_than_20_headers? diff --git a/ee/app/services/audit_events/streaming/event_type_filters/base_service.rb b/ee/app/services/audit_events/streaming/event_type_filters/base_service.rb new file mode 100644 index 00000000000000..aa38f3ebec3b64 --- /dev/null +++ b/ee/app/services/audit_events/streaming/event_type_filters/base_service.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module AuditEvents + module Streaming + module EventTypeFilters + class BaseService + attr_reader :destination, :event_type_filters, :current_user + + def initialize(destination:, event_type_filters:, current_user:) + @destination = destination + @event_type_filters = event_type_filters + @current_user = current_user + end + end + end + end +end diff --git a/ee/app/services/audit_events/streaming/event_type_filters/create_service.rb b/ee/app/services/audit_events/streaming/event_type_filters/create_service.rb index 7c4603c5eacac1..c3d39f4beb2c81 100644 --- a/ee/app/services/audit_events/streaming/event_type_filters/create_service.rb +++ b/ee/app/services/audit_events/streaming/event_type_filters/create_service.rb @@ -3,17 +3,11 @@ module AuditEvents module Streaming module EventTypeFilters - class CreateService - attr_reader :destination, :event_type_filters - - def initialize(destination:, event_type_filters:) - @destination = destination - @event_type_filters = event_type_filters - end - + class CreateService < BaseService def execute begin create_event_type_filters! + log_audit_event rescue ActiveRecord::RecordInvalid => e return ServiceResponse.error(message: e.message) end @@ -30,6 +24,18 @@ def create_event_type_filters! end end end + + def log_audit_event + audit_context = { + name: 'event_type_filters_created', + author: current_user, + scope: destination.group, + target: destination, + message: "Created audit event type filter(s): #{event_type_filters.to_sentence}" + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end end end end diff --git a/ee/spec/services/audit_events/streaming/event_type_filters/create_service_spec.rb b/ee/spec/services/audit_events/streaming/event_type_filters/create_service_spec.rb index af76b17a27be71..1c8d163357f76d 100644 --- a/ee/spec/services/audit_events/streaming/event_type_filters/create_service_spec.rb +++ b/ee/spec/services/audit_events/streaming/event_type_filters/create_service_spec.rb @@ -5,12 +5,14 @@ RSpec.describe AuditEvents::Streaming::EventTypeFilters::CreateService do let_it_be(:destination) { create(:external_audit_event_destination) } let_it_be(:event_type_filters) { ['filter_1'] } + let_it_be(:user) { create(:user) } let(:expected_error) { [] } subject(:response) do described_class.new(destination: destination, - event_type_filters: event_type_filters).execute + event_type_filters: event_type_filters, + current_user: user).execute end describe '#execute' do @@ -21,6 +23,21 @@ expect(response).to be_success expect(response.errors).to match_array(expected_error) end + + it 'creates audit event', :aggregate_failures do + audit_context = { + name: 'event_type_filters_created', + author: user, + scope: destination.group, + target: destination, + message: "Created audit event type filter(s): filter_1" + } + + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context) + .and_call_original + + expect { subject }.to change { AuditEvent.count }.by(1) + end end context 'when record is invalid' do @@ -36,6 +53,10 @@ expect { subject }.not_to change { destination.event_type_filters.count } expect(response.errors).to match_array([expected_error]) end + + it 'does not create audit event' do + expect { subject }.not_to change { AuditEvent.count } + end end end end -- GitLab From f8ceadb2c02370b951bc24170829ad4c91cfa90a Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Wed, 1 Mar 2023 11:03:52 +0530 Subject: [PATCH 2/7] Update test cases for mutation --- .../streaming/event_type_filters/create.rb | 5 +- .../event_type_filters/create_spec.rb | 98 ++++++++----------- .../event_type_filters/create_spec.rb | 91 +++++++++++++++++ 3 files changed, 135 insertions(+), 59 deletions(-) create mode 100644 ee/spec/requests/api/graphql/audit_events/streaming/event_type_filters/create_spec.rb diff --git a/ee/app/graphql/mutations/audit_events/streaming/event_type_filters/create.rb b/ee/app/graphql/mutations/audit_events/streaming/event_type_filters/create.rb index 4c4e0d444e1adc..c31311a3cd8f5d 100644 --- a/ee/app/graphql/mutations/audit_events/streaming/event_type_filters/create.rb +++ b/ee/app/graphql/mutations/audit_events/streaming/event_type_filters/create.rb @@ -14,7 +14,10 @@ class Create < BaseMutation argument :event_type_filters, [GraphQL::Types::String], required: true, - description: 'List of event type filters to add for streaming.' + description: 'List of event type filters to add for streaming.', + prepare: ->(filters, _ctx) do + filters.presence || (raise ::Gitlab::Graphql::Errors::ArgumentError, 'event type filters must be present') + end field :event_type_filters, [GraphQL::Types::String], null: true, diff --git a/ee/spec/graphql/mutations/audit_events/streaming/event_type_filters/create_spec.rb b/ee/spec/graphql/mutations/audit_events/streaming/event_type_filters/create_spec.rb index 9997309e89e500..cce5704f31c124 100644 --- a/ee/spec/graphql/mutations/audit_events/streaming/event_type_filters/create_spec.rb +++ b/ee/spec/graphql/mutations/audit_events/streaming/event_type_filters/create_spec.rb @@ -2,78 +2,60 @@ require 'spec_helper' -RSpec.describe Mutations::AuditEvents::Streaming::EventTypeFilters::Create do - let_it_be(:current_user) { create(:user) } +RSpec.describe AuditEvents::Streaming::EventTypeFilters::CreateService do let_it_be(:destination) { create(:external_audit_event_destination) } + let_it_be(:event_type_filters) { ['filter_1'] } + let_it_be(:user) { create(:user) } - let(:group) { destination.group } - let(:mutation) { described_class.new(object: nil, context: { current_user: current_user }, field: nil) } - let(:params) do - { - destination_id: destination.to_gid, - event_type_filters: %w[filter_1] - } - end + let(:expected_error) { [] } - subject { mutation.resolve(**params) } + subject(:response) do + described_class.new(destination: destination, + event_type_filters: event_type_filters, + current_user: user).execute + end - describe '#resolve' do - context 'when feature is unlicensed' do - before do - stub_licensed_features(external_audit_events: false) + describe '#execute' do + context 'when event type filter is not already present' do + it 'creates event type filter', :aggregate_failures do + expect { subject }.to change { destination.event_type_filters.count }.by 1 + expect(destination.event_type_filters.last.audit_event_type).to eq(event_type_filters.first) + expect(response).to be_success + expect(response.errors).to match_array(expected_error) end - it 'returns useful error messages' do - expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + it 'creates audit event', :aggregate_failures do + audit_context = { + name: 'event_type_filters_created', + author: user, + scope: destination.group, + target: destination, + message: "Created audit event type filter(s): filter_1" + } + + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context) + .and_call_original + + expect { subject }.to change { AuditEvent.count }.by(1) end end - context 'when feature is licensed' do - before do - stub_licensed_features(external_audit_events: true) - end + context 'when record is invalid' do + let(:expected_error) { 'Validation Failed' } - context 'when current_user is not group owner' do - it 'returns useful error messages' do - expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + before do + expect_next_instance_of(::AuditEvents::Streaming::EventTypeFilter) do |filter| + allow(filter).to receive(:save!).and_raise(ActiveRecord::RecordInvalid.new(filter), expected_error) end end - context 'when current_user is group owner' do - before do - group.add_owner(current_user) - end - - context 'and calls create service' do - context 'when response is success' do - let(:response) { ServiceResponse.success } - - before do - allow_next_instance_of(::AuditEvents::Streaming::EventTypeFilters::CreateService) do |instance| - allow(instance).to receive(:execute).and_return(response) - end - end - - it 'returns event type filters' do - expect(subject).to eq({ event_type_filters: destination.event_type_filters, errors: [] }) - end - end - - context 'when response is error' do - let(:response) { ServiceResponse.error(message: 'Something went wrong') } - - before do - allow_next_instance_of(::AuditEvents::Streaming::EventTypeFilters::CreateService) do |instance| - allow(instance).to receive(:execute).and_return(response) - end - end + it 'returns error message', :aggregate_failures do + expect { subject }.not_to change { destination.event_type_filters.count } + expect(response.errors).to match_array([expected_error]) + end - it 'returns error message' do - expect(subject).to eq({ event_type_filters: destination.event_type_filters, - errors: ['Something went wrong'] }) - end - end - end + it 'does not create audit event' do + expect { subject }.not_to change { AuditEvent.count } end end end diff --git a/ee/spec/requests/api/graphql/audit_events/streaming/event_type_filters/create_spec.rb b/ee/spec/requests/api/graphql/audit_events/streaming/event_type_filters/create_spec.rb new file mode 100644 index 00000000000000..bdd1033970fd30 --- /dev/null +++ b/ee/spec/requests/api/graphql/audit_events/streaming/event_type_filters/create_spec.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Create an audit event type filter', feature_category: :audit_events do + include GraphqlHelpers + + let_it_be(:destination) { create(:external_audit_event_destination) } + let_it_be(:event_type_filter) do + create(:audit_events_streaming_event_type_filter, external_audit_event_destination: destination, + audit_event_type: 'filter_1') + end + + let_it_be(:user) { create(:user) } + + let(:current_user) { user } + let(:group) { destination.group } + let(:mutation_name) { :audit_events_streaming_destination_events_add } + let(:mutation) { graphql_mutation(mutation_name, input) } + let(:mutation_response) { graphql_mutation_response(mutation_name) } + let(:input) { { destinationId: destination.to_gid, eventTypeFilters: ['filter_2'] } } + + context 'when unlicensed' do + subject { post_graphql_mutation(mutation, current_user: user) } + + before do + stub_licensed_features(external_audit_events: false) + end + + it_behaves_like 'a mutation on an unauthorized resource' + end + + context 'when licensed' do + subject { post_graphql_mutation(mutation, current_user: user) } + + before do + stub_licensed_features(external_audit_events: true) + end + + context 'when current user is a group maintainer' do + before do + group.add_maintainer(user) + end + + it_behaves_like 'a mutation that returns top-level errors', + errors: [Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR] + end + + context 'when current user is a group developer' do + before do + group.add_developer(user) + end + + it_behaves_like 'a mutation that returns top-level errors', + errors: [Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR] + end + + context 'when current user is a group guest' do + before do + group.add_guest(user) + end + + it_behaves_like 'a mutation that returns top-level errors', + errors: [Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR] + end + + context 'when current user is a group user' do + before do + group.add_owner(user) + end + + it 'returns success response', :aggregate_failures do + subject + + response = mutation_response + expect(response["errors"]).to be_empty + expect(response["eventTypeFilters"]).to eq(destination.event_type_filters.pluck(:audit_event_type)) + end + + context 'when event type filters in input in empty' do + let(:input) { { destinationId: destination.to_gid, eventTypeFilters: [] } } + + it 'returns graphql error' do + subject + + expect(graphql_errors).to include(a_hash_including('message' => 'event type filters must be present')) + end + end + end + end +end -- GitLab From c97eba343c3b192c4ca996da44735ebae6dd9a9d Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Wed, 1 Mar 2023 11:18:26 +0530 Subject: [PATCH 3/7] Update test cases for mutation --- .../event_type_filters/create_spec.rb | 98 +++++++++++-------- .../event_type_filters/create_service_spec.rb | 2 +- 2 files changed, 59 insertions(+), 41 deletions(-) diff --git a/ee/spec/graphql/mutations/audit_events/streaming/event_type_filters/create_spec.rb b/ee/spec/graphql/mutations/audit_events/streaming/event_type_filters/create_spec.rb index cce5704f31c124..a9bd76b9ba78c1 100644 --- a/ee/spec/graphql/mutations/audit_events/streaming/event_type_filters/create_spec.rb +++ b/ee/spec/graphql/mutations/audit_events/streaming/event_type_filters/create_spec.rb @@ -2,60 +2,78 @@ require 'spec_helper' -RSpec.describe AuditEvents::Streaming::EventTypeFilters::CreateService do +RSpec.describe Mutations::AuditEvents::Streaming::EventTypeFilters::Create, feature_category: :audit_events do + let_it_be(:current_user) { create(:user) } let_it_be(:destination) { create(:external_audit_event_destination) } - let_it_be(:event_type_filters) { ['filter_1'] } - let_it_be(:user) { create(:user) } - let(:expected_error) { [] } - - subject(:response) do - described_class.new(destination: destination, - event_type_filters: event_type_filters, - current_user: user).execute + let(:group) { destination.group } + let(:mutation) { described_class.new(object: nil, context: { current_user: current_user }, field: nil) } + let(:params) do + { + destination_id: destination.to_gid, + event_type_filters: %w[filter_1] + } end - describe '#execute' do - context 'when event type filter is not already present' do - it 'creates event type filter', :aggregate_failures do - expect { subject }.to change { destination.event_type_filters.count }.by 1 - expect(destination.event_type_filters.last.audit_event_type).to eq(event_type_filters.first) - expect(response).to be_success - expect(response.errors).to match_array(expected_error) - end - - it 'creates audit event', :aggregate_failures do - audit_context = { - name: 'event_type_filters_created', - author: user, - scope: destination.group, - target: destination, - message: "Created audit event type filter(s): filter_1" - } + subject { mutation.resolve(**params) } - expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context) - .and_call_original + describe '#resolve' do + context 'when feature is unlicensed' do + before do + stub_licensed_features(external_audit_events: false) + end - expect { subject }.to change { AuditEvent.count }.by(1) + it 'returns useful error messages' do + expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) end end - context 'when record is invalid' do - let(:expected_error) { 'Validation Failed' } - + context 'when feature is licensed' do before do - expect_next_instance_of(::AuditEvents::Streaming::EventTypeFilter) do |filter| - allow(filter).to receive(:save!).and_raise(ActiveRecord::RecordInvalid.new(filter), expected_error) - end + stub_licensed_features(external_audit_events: true) end - it 'returns error message', :aggregate_failures do - expect { subject }.not_to change { destination.event_type_filters.count } - expect(response.errors).to match_array([expected_error]) + context 'when current_user is not group owner' do + it 'returns useful error messages' do + expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + end end - it 'does not create audit event' do - expect { subject }.not_to change { AuditEvent.count } + context 'when current_user is group owner' do + before do + group.add_owner(current_user) + end + + context 'and calls create service' do + context 'when response is success' do + let(:response) { ServiceResponse.success } + + before do + allow_next_instance_of(::AuditEvents::Streaming::EventTypeFilters::CreateService) do |instance| + allow(instance).to receive(:execute).and_return(response) + end + end + + it 'returns event type filters' do + expect(subject).to eq({ event_type_filters: destination.event_type_filters, errors: [] }) + end + end + + context 'when response is error' do + let(:response) { ServiceResponse.error(message: 'Something went wrong') } + + before do + allow_next_instance_of(::AuditEvents::Streaming::EventTypeFilters::CreateService) do |instance| + allow(instance).to receive(:execute).and_return(response) + end + end + + it 'returns error message' do + expect(subject).to eq({ event_type_filters: destination.event_type_filters, + errors: ['Something went wrong'] }) + end + end + end end end end diff --git a/ee/spec/services/audit_events/streaming/event_type_filters/create_service_spec.rb b/ee/spec/services/audit_events/streaming/event_type_filters/create_service_spec.rb index 1c8d163357f76d..bc9acd7d4bfc21 100644 --- a/ee/spec/services/audit_events/streaming/event_type_filters/create_service_spec.rb +++ b/ee/spec/services/audit_events/streaming/event_type_filters/create_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe AuditEvents::Streaming::EventTypeFilters::CreateService do +RSpec.describe AuditEvents::Streaming::EventTypeFilters::CreateService, feature_category: :audit_events do let_it_be(:destination) { create(:external_audit_event_destination) } let_it_be(:event_type_filters) { ['filter_1'] } let_it_be(:user) { create(:user) } -- GitLab From 4619f65ba3e66fc481c7a57b3a53c1227c12ad4b Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Wed, 1 Mar 2023 11:20:13 +0530 Subject: [PATCH 4/7] Rubocop offenses fix --- .../audit_events/streaming/event_type_filters/create.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ee/app/graphql/mutations/audit_events/streaming/event_type_filters/create.rb b/ee/app/graphql/mutations/audit_events/streaming/event_type_filters/create.rb index c31311a3cd8f5d..5cdfab4f9383f0 100644 --- a/ee/app/graphql/mutations/audit_events/streaming/event_type_filters/create.rb +++ b/ee/app/graphql/mutations/audit_events/streaming/event_type_filters/create.rb @@ -16,7 +16,8 @@ class Create < BaseMutation required: true, description: 'List of event type filters to add for streaming.', prepare: ->(filters, _ctx) do - filters.presence || (raise ::Gitlab::Graphql::Errors::ArgumentError, 'event type filters must be present') + filters.presence || (raise ::Gitlab::Graphql::Errors::ArgumentError, + 'event type filters must be present') end field :event_type_filters, [GraphQL::Types::String], -- GitLab From 3356f2d5458f526b4f7cee69136f4b682c5c2120 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Wed, 1 Mar 2023 11:29:24 +0530 Subject: [PATCH 5/7] Event filter type definition file --- .../audit_events/types/event_type_filters_created.yml | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 ee/config/audit_events/types/event_type_filters_created.yml diff --git a/ee/config/audit_events/types/event_type_filters_created.yml b/ee/config/audit_events/types/event_type_filters_created.yml new file mode 100644 index 00000000000000..fe93963a2744b4 --- /dev/null +++ b/ee/config/audit_events/types/event_type_filters_created.yml @@ -0,0 +1,9 @@ +--- +name: event_type_filters_created +description: Event triggered when a new audit events streaming event type filter is created +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/344848 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113081 +feature_category: audit_events +milestone: '15.10' +saved_to_database: true +streamed: true -- GitLab From 0db6ab01488726ef902c9b6fc01c77341a0a8e1d Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Wed, 1 Mar 2023 11:45:24 +0530 Subject: [PATCH 6/7] Test case for audit details method --- .../audit_events/external_audit_event_destination_spec.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ee/spec/models/audit_events/external_audit_event_destination_spec.rb b/ee/spec/models/audit_events/external_audit_event_destination_spec.rb index 3ac56baa541243..56577e3c4ea9d2 100644 --- a/ee/spec/models/audit_events/external_audit_event_destination_spec.rb +++ b/ee/spec/models/audit_events/external_audit_event_destination_spec.rb @@ -93,4 +93,10 @@ it_behaves_like 'includes Limitable concern' do subject { build(:external_audit_event_destination, group: create(:group)) } end + + describe '#audit_details' do + it "equals to the user's name" do + expect(destination.audit_details).to eq(destination.destination_url) + end + end end -- GitLab From 21793b8632f729947b1db93e3e22670fe4c1dd83 Mon Sep 17 00:00:00 2001 From: harsimarsandhu Date: Thu, 2 Mar 2023 10:28:27 +0530 Subject: [PATCH 7/7] Update it block description --- .../audit_events/external_audit_event_destination_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/models/audit_events/external_audit_event_destination_spec.rb b/ee/spec/models/audit_events/external_audit_event_destination_spec.rb index 56577e3c4ea9d2..34cc108709dba1 100644 --- a/ee/spec/models/audit_events/external_audit_event_destination_spec.rb +++ b/ee/spec/models/audit_events/external_audit_event_destination_spec.rb @@ -95,7 +95,7 @@ end describe '#audit_details' do - it "equals to the user's name" do + it "equals to the destination url" do expect(destination.audit_details).to eq(destination.destination_url) end end -- GitLab