diff --git a/ee/config/events/cvs_on_sbom_change.yml b/ee/config/events/cvs_on_sbom_change.yml index 11e0fda59ba8364e07944f889c101817c29ba6d1..c1f3d01e2633e23c4f243a935d5d0d0e2a06f610 100644 --- a/ee/config/events/cvs_on_sbom_change.yml +++ b/ee/config/events/cvs_on_sbom_change.yml @@ -4,9 +4,19 @@ internal_events: true action: cvs_on_sbom_change additional_properties: label: - description: pipeline_info - source of SBOM components + description: Type of the source of the SBOM components (e.g. pipeline_info) property: - description: pipeline id + description: Id of the pipeline where SBOM components have been detected + start_time: + description: Start time of the CVS execution + end_time: + description: End time of the CVS execution + possibly_affected_sbom_occurrences: + description: Number of SBOM occurrences that could possibly be affected by the vulnerability + known_affected_sbom_occurrences: + description: Number of SBOM occurrences that are actually affected by the vulnerability + sbom_occurrences_semver_dialects_errors_count: + description: Number of SBOM occurrences for which CVS returned a SemverDialect Error product_group: composition_analysis milestone: '17.4' introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/163935 diff --git a/ee/spec/requests/api/usage_data_spec.rb b/ee/spec/requests/api/usage_data_spec.rb index ebb20a2ca50fb62be4a898ce2816659508647980..a63eab6b8528f5e4f9864d1a22de7c7d5bd0fadf 100644 --- a/ee/spec/requests/api/usage_data_spec.rb +++ b/ee/spec/requests/api/usage_data_spec.rb @@ -24,6 +24,7 @@ let(:event_name) { 'code_suggestion_shown_in_ide' } it 'triggers AI tracking' do + allow(Gitlab::InternalEvents).to receive(:track_event) expect(Gitlab::Tracking::AiTracking).to receive(:track_event) .with( event_name, diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index c37f34169f3e3896124e6a419e8ad230778d3114..143de249e238096db5488700819521e2b2292f1f 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -740,7 +740,7 @@ def track_event(event_name, user:, send_snowplow_event: true, namespace_id: nil, namespace: namespace, project: project ) - rescue Gitlab::InternalEvents::UnknownEventError => e + rescue Gitlab::Tracking::EventValidator::UnknownEventError => e Gitlab::ErrorTracking.track_exception(e, event_name: event_name) # We want to keep the error silent on production to keep the behavior diff --git a/lib/gitlab/internal_events.rb b/lib/gitlab/internal_events.rb index cec2d17f59bf7c1eae1f7c9a543307298ebcb1f2..dbb8f8e50f305c9892d6c08c9b236f9457c0b21a 100644 --- a/lib/gitlab/internal_events.rb +++ b/lib/gitlab/internal_events.rb @@ -2,17 +2,8 @@ module Gitlab module InternalEvents - UnknownEventError = Class.new(StandardError) - InvalidPropertyError = Class.new(StandardError) - InvalidPropertyTypeError = Class.new(StandardError) - SNOWPLOW_EMITTER_BUFFER_SIZE = 100 DEFAULT_BUFFER_SIZE = 1 - BASE_ADDITIONAL_PROPERTIES = { - label: [String], - property: [String], - value: [Integer, Float] - }.freeze KEY_EXPIRY_LENGTH = Gitlab::UsageDataCounters::HLLRedisCounter::KEY_EXPIRY_LENGTH class << self @@ -24,14 +15,10 @@ def track_event( event_name, category: nil, send_snowplow_event: true, additional_properties: {}, **kwargs) - extra = custom_additional_properties(additional_properties) - additional_properties = additional_properties.slice(*BASE_ADDITIONAL_PROPERTIES.keys) - - unless Gitlab::Tracking::EventDefinition.internal_event_exists?(event_name) - raise UnknownEventError, "Unknown event: #{event_name}" - end + Gitlab::Tracking::EventValidator.new(event_name, additional_properties, kwargs).validate! - validate_properties!(additional_properties, kwargs) + extra = custom_additional_properties(additional_properties) + additional_properties = additional_properties.slice(*base_additional_properties.keys) project = kwargs[:project] kwargs[:namespace] ||= project.namespace if project @@ -59,28 +46,6 @@ def track_event( private - def validate_properties!(additional_properties, kwargs) - validate_property!(kwargs, :user, User) - validate_property!(kwargs, :namespace, Namespaces::UserNamespace, Group) - validate_property!(kwargs, :project, Project) - validate_additional_properties!(additional_properties) - end - - def validate_property!(hash, key, *class_names) - return unless hash.has_key?(key) - return if hash[key].nil? - return if class_names.include?(hash[key].class) - - raise InvalidPropertyTypeError, "#{key} should be an instance of #{class_names.join(', ')}" - end - - def validate_additional_properties!(additional_properties) - BASE_ADDITIONAL_PROPERTIES.keys.intersection(additional_properties.keys).each do |key| - allowed_classes = BASE_ADDITIONAL_PROPERTIES[key] - validate_property!(additional_properties, key, *allowed_classes) - end - end - def update_redis_values(event_name, additional_properties, kwargs) event_definition = Gitlab::Tracking::EventDefinition.find(event_name) @@ -100,7 +65,7 @@ def update_redis_values(event_name, additional_properties, kwargs) end def custom_additional_properties(additional_properties) - additional_properties.except(*BASE_ADDITIONAL_PROPERTIES.keys) + additional_properties.except(*base_additional_properties.keys) end def update_total_counter(event_selection_rule) @@ -194,6 +159,10 @@ def gitlab_sdk_client GitlabSDK::Client.new(app_id: app_id, host: host, buffer_size: buffer_size) end strong_memoize_attr :gitlab_sdk_client + + def base_additional_properties + Gitlab::Tracking::EventValidator::BASE_ADDITIONAL_PROPERTIES + end end end end diff --git a/lib/gitlab/tracking/event_definition_validator.rb b/lib/gitlab/tracking/event_definition_validator.rb index 1f40afda3c3678dbd416ffa14cfc00995c86343b..2ba65a20054b78fa38bc3f4e5a679a2784874622 100644 --- a/lib/gitlab/tracking/event_definition_validator.rb +++ b/lib/gitlab/tracking/event_definition_validator.rb @@ -25,7 +25,7 @@ def validate_additional_properties return unless additional_props.present? - extra_props = additional_props - Gitlab::InternalEvents::BASE_ADDITIONAL_PROPERTIES.keys + extra_props = additional_props - Gitlab::Tracking::EventValidator::BASE_ADDITIONAL_PROPERTIES.keys unused_props = prioritized_properties - additional_props return unless extra_props.present? && unused_props.present? @@ -40,7 +40,7 @@ def validate_additional_properties end def prioritized_properties - Gitlab::InternalEvents::BASE_ADDITIONAL_PROPERTIES.keys - NOT_VALIDATED_PROPERTIES + Gitlab::Tracking::EventValidator::BASE_ADDITIONAL_PROPERTIES.keys - NOT_VALIDATED_PROPERTIES end def validate_schema diff --git a/lib/gitlab/tracking/event_validator.rb b/lib/gitlab/tracking/event_validator.rb new file mode 100644 index 0000000000000000000000000000000000000000..06017097db810dc2f3a34ecb7169809a7982f9ba --- /dev/null +++ b/lib/gitlab/tracking/event_validator.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +module Gitlab + module Tracking + class EventValidator + UnknownEventError = Class.new(StandardError) + InvalidPropertyError = Class.new(StandardError) + InvalidPropertyTypeError = Class.new(StandardError) + BASE_ADDITIONAL_PROPERTIES = { + label: [String], + property: [String], + value: [Integer, Float] + }.freeze + + def initialize(event_name, additional_properties, kwargs) + @event_name = event_name + @additional_properties = additional_properties + @kwargs = kwargs + end + + def validate! + unless Gitlab::Tracking::EventDefinition.internal_event_exists?(event_name) + raise UnknownEventError, "Unknown event: #{event_name}" + end + + validate_properties! + validate_additional_properties! + end + + private + + attr_reader :event_name, :additional_properties, :kwargs + + def validate_properties! + validate_property!(kwargs, :user, User) + validate_property!(kwargs, :namespace, Namespaces::UserNamespace, Group) + validate_property!(kwargs, :project, Project) + end + + def validate_property!(hash, key, *class_names) + return unless hash.has_key?(key) + return if hash[key].nil? + return if class_names.include?(hash[key].class) + + raise InvalidPropertyTypeError, "#{key} should be an instance of #{class_names.join(', ')}" + end + + def validate_additional_properties! + additional_properties.each_key do |property| + next unless BASE_ADDITIONAL_PROPERTIES.has_key?(property) + + allowed_classes = BASE_ADDITIONAL_PROPERTIES[property] + validate_property!(additional_properties, property, *allowed_classes) + end + + # skip base properties validation. To be done in a separate MR as we have some non-compliant definitions + custom_properties = additional_properties.except(*BASE_ADDITIONAL_PROPERTIES.keys) + event_definition_attributes = Gitlab::Tracking::EventDefinition.find(event_name).to_h + custom_properties.each_key do |key| + unless event_definition_attributes[:additional_properties].include?(key) + raise InvalidPropertyError, "Unknown additional property: #{key}" + end + end + end + end + end +end diff --git a/spec/controllers/concerns/product_analytics_tracking_spec.rb b/spec/controllers/concerns/product_analytics_tracking_spec.rb index 9247ece5e74d7f01f34828da9c9d7dc3d9a8efb5..cf5fe7790ede5065a8aa5568a3493eef3e4a13e1 100644 --- a/spec/controllers/concerns/product_analytics_tracking_spec.rb +++ b/spec/controllers/concerns/product_analytics_tracking_spec.rb @@ -64,7 +64,8 @@ def expect_no_internal_tracking allow(Gitlab::Tracking::EventDefinition).to receive(:internal_event_exists?).with('an_event').and_return(true) event_definition = instance_double( Gitlab::Tracking::EventDefinition, - event_selection_rules: [all_time_total_count, time_framed_total_count] + event_selection_rules: [all_time_total_count, time_framed_total_count], + to_h: {} ) allow(Gitlab::Tracking::EventDefinition).to receive(:find).with(event_name).and_return(event_definition) end diff --git a/spec/lib/api/helpers_spec.rb b/spec/lib/api/helpers_spec.rb index 7ce9bc4881219c6abc66cc30347c784dff47d101..27872bd3bdf44c3f06d32caaff2a4c9207a5b250 100644 --- a/spec/lib/api/helpers_spec.rb +++ b/spec/lib/api/helpers_spec.rb @@ -1019,11 +1019,11 @@ def app end it 'tracks an exception and renders 422 for unknown event', :aggregate_failures do - expect(Gitlab::InternalEvents).to receive(:track_event).and_raise(Gitlab::InternalEvents::UnknownEventError, "Unknown event: #{unknown_event}") + # expect(Gitlab::InternalEvents).to receive(:track_event).and_raise(Gitlab::InternalEvents::UnknownEventError, "Unknown event: #{unknown_event}") expect(Gitlab::ErrorTracking).to receive(:track_exception) .with( - instance_of(Gitlab::InternalEvents::UnknownEventError), + instance_of(Gitlab::Tracking::EventValidator::UnknownEventError), event_name: unknown_event ) expect(helper).to receive(:unprocessable_entity!).with("Unknown event: #{unknown_event}") diff --git a/spec/lib/gitlab/internal_events_spec.rb b/spec/lib/gitlab/internal_events_spec.rb index 78c3fd7225f8432838d2826b45810bfe3acd0a62..72cc36adf161c87a5debb2d893e0d639bedd6caf 100644 --- a/spec/lib/gitlab/internal_events_spec.rb +++ b/spec/lib/gitlab/internal_events_spec.rb @@ -19,23 +19,13 @@ allow(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis) allow(Gitlab::Tracking).to receive(:tracker).and_return(fake_snowplow) allow(Gitlab::Tracking::EventDefinition).to receive(:find).and_return(event_definition) + allow_next_instance_of(Gitlab::Tracking::EventValidator) do |instance| + allow(instance).to receive(:validate!) + end allow(event_definition).to receive(:event_selection_rules).and_return(event_selection_rules) allow(fake_snowplow).to receive(:event) end - shared_examples 'an event that logs an error' do - it 'logs an error' do - described_class.track_event(event_name, additional_properties: additional_properties, **event_kwargs) - - expect(Gitlab::ErrorTracking).to have_received(:track_and_raise_for_dev_exception) - .with(error_class, - event_name: event_name, - kwargs: logged_kwargs, - additional_properties: additional_properties - ) - end - end - def expect_redis_hll_tracking(value_override = nil, property_name_override = nil) expected_value = value_override || unique_value expected_property_name = property_name_override || property_name @@ -355,46 +345,18 @@ def validate_service_ping_context(service_ping_context) end end - context 'when arguments are invalid' do - let(:error_class) { described_class::InvalidPropertyTypeError } - - context 'when user is not an instance of User' do - let(:user) { 'a_string' } + it 'calls the event validator' do + fake_validator = instance_double(Gitlab::Tracking::EventValidator, validate!: nil) + additional_properties = { label: 'label_name', value: 16.17, property: "lang" } + kwargs = { user: user, project: project } - it_behaves_like 'an event that logs an error' do - let(:event_kwargs) { { user: user, project: project } } - let(:logged_kwargs) { { user: user, project: project.id } } - end - end + expect(Gitlab::Tracking::EventValidator) + .to receive(:new) + .with(event_name, additional_properties, kwargs) + .and_return(fake_validator) + expect(fake_validator).to receive(:validate!) - context 'when project is not an instance of Project' do - let(:project) { 42 } - - it_behaves_like 'an event that logs an error' do - let(:event_kwargs) { { user: user, project: project } } - let(:logged_kwargs) { { user: user.id, project: project } } - end - end - - context 'when namespace is not an instance of Namespace' do - let(:namespace) { false } - - it_behaves_like 'an event that logs an error' do - let(:event_kwargs) { { user: user, namespace: namespace } } - let(:logged_kwargs) { { user: user.id, namespace: namespace } } - end - end - - %i[label value property].each do |attribute_name| - context "when #{attribute_name} has an invalid value" do - let(:additional_properties) { { "#{attribute_name}": :symbol } } - - it_behaves_like 'an event that logs an error' do - let(:event_kwargs) { { user: user } } - let(:logged_kwargs) { { user: user.id } } - end - end - end + described_class.track_event(event_name, additional_properties: additional_properties, **kwargs) end it 'updates Redis, RedisHLL and Snowplow', :aggregate_failures do @@ -420,13 +382,6 @@ def validate_service_ping_context(service_ping_context) expect { described_class.track_event(event_name, **params) }.not_to raise_error end - it 'logs error on unknown event', :aggregate_failures do - expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception) - .with(described_class::UnknownEventError, event_name: 'unknown_event', kwargs: {}, additional_properties: {}) - - expect { described_class.track_event('unknown_event') }.not_to raise_error - end - it 'logs warning on missing property', :aggregate_failures do expect { described_class.track_event(event_name, merge_request_id: 1) }.not_to raise_error diff --git a/spec/lib/gitlab/tracking/event_validator_spec.rb b/spec/lib/gitlab/tracking/event_validator_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..52f6775d1cd945b6a1533777ed9282b6594ef893 --- /dev/null +++ b/spec/lib/gitlab/tracking/event_validator_spec.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Tracking::EventValidator, feature_category: :service_ping do + let(:event_name) { 'test_event' } + let(:additional_properties) { { label: 'test_label', property: 'test_property', value: 1, lang: "eng" } } + let(:kwargs) { { user: build(:user), project: build(:project) } } + let(:event_definition) { instance_double(Gitlab::Tracking::EventDefinition) } + + subject(:validate) { described_class.new(event_name, additional_properties, kwargs).validate! } + + before do + allow(Gitlab::Tracking::EventDefinition).to receive(:internal_event_exists?).and_return(true) + allow(Gitlab::Tracking::EventDefinition).to receive(:find).and_return(event_definition) + allow(event_definition).to receive(:to_h).and_return({ additional_properties: [:lang] }) + end + + describe '#validate!' do + context 'when event exists and properties are valid' do + it 'does not raise an error' do + expect { validate }.not_to raise_error + end + end + + context 'when event does not exist' do + before do + allow(Gitlab::Tracking::EventDefinition).to receive(:internal_event_exists?).and_return(false) + end + + it 'raises an UnknownEventError' do + expect { validate }.to raise_error(Gitlab::Tracking::EventValidator::UnknownEventError) + end + end + + context 'when properties have invalid types' do + [ + { user: 'invalid_user' }, + { project: 'invalid_project' }, + { namespace: 'invalid_namespace' } + ].each do |invalid_property| + context "when #{invalid_property.each_key.first} is invalid" do + let(:kwargs) { invalid_property } + + it 'raises an InvalidPropertyTypeError' do + property_name = invalid_property.each_key.first + expect { validate }.to raise_error(Gitlab::Tracking::EventValidator::InvalidPropertyTypeError, + /#{property_name} should be an instance of #{property_name.capitalize}/) + end + end + end + end + + context 'when a base additional property is invalid' do + [ + { label: 123 }, + { value: 'test_value' }, + { property: true } + ].each do |invalid_property| + context "when #{invalid_property.each_key.first} is invalid" do + let(:additional_properties) { invalid_property } + + it 'raises an InvalidPropertyTypeError' do + property = invalid_property.each_key.first + expected_type = described_class::BASE_ADDITIONAL_PROPERTIES[property] + expect { validate }.to raise_error(Gitlab::Tracking::EventValidator::InvalidPropertyTypeError, + /#{property} should be an instance of #{expected_type}/) + end + end + end + end + + context 'when custom additional properties are not defined in event definition' do + let(:additional_properties) { { custom_property: 'value' } } + + it 'raises an InvalidPropertyError for unknown properties' do + expect { validate }.to raise_error(Gitlab::Tracking::EventValidator::InvalidPropertyError, + 'Unknown additional property: custom_property') + end + end + end +end diff --git a/spec/lib/gitlab/usage/metrics/instrumentations/total_count_metric_spec.rb b/spec/lib/gitlab/usage/metrics/instrumentations/total_count_metric_spec.rb index df617d0607525e8dda17f63f3a460b998161e584..18e209dd648204a2084aa6c944782174099dd9bd 100644 --- a/spec/lib/gitlab/usage/metrics/instrumentations/total_count_metric_spec.rb +++ b/spec/lib/gitlab/usage/metrics/instrumentations/total_count_metric_spec.rb @@ -9,19 +9,22 @@ event_definition = instance_double( Gitlab::Tracking::EventDefinition, - event_selection_rules: [Gitlab::Usage::EventSelectionRule.new(name: 'my_event', time_framed: false)] + event_selection_rules: [Gitlab::Usage::EventSelectionRule.new(name: 'my_event', time_framed: false)], + to_h: {} ) allow(Gitlab::Tracking::EventDefinition).to receive(:find).with('my_event').and_return(event_definition) event_definition1 = instance_double( Gitlab::Tracking::EventDefinition, - event_selection_rules: [Gitlab::Usage::EventSelectionRule.new(name: 'my_event1', time_framed: false)] + event_selection_rules: [Gitlab::Usage::EventSelectionRule.new(name: 'my_event1', time_framed: false)], + to_h: {} ) allow(Gitlab::Tracking::EventDefinition).to receive(:find).with('my_event1').and_return(event_definition1) event_definition2 = instance_double( Gitlab::Tracking::EventDefinition, - event_selection_rules: [Gitlab::Usage::EventSelectionRule.new(name: 'my_event2', time_framed: false)] + event_selection_rules: [Gitlab::Usage::EventSelectionRule.new(name: 'my_event2', time_framed: false)], + to_h: {} ) allow(Gitlab::Tracking::EventDefinition).to receive(:find).with('my_event2').and_return(event_definition2) end diff --git a/spec/requests/api/usage_data_spec.rb b/spec/requests/api/usage_data_spec.rb index 483137ca1dcdea9a1dec589a19892b478edaac97..ee2b687bdf626897db07aa85669c3ec2f4e94c9f 100644 --- a/spec/requests/api/usage_data_spec.rb +++ b/spec/requests/api/usage_data_spec.rb @@ -414,6 +414,8 @@ end it 'triggers internal events and returns status ok' do + allow(Gitlab::InternalEvents).to receive(:track_event) + post api(endpoint, user), params: params expect(response).to have_gitlab_http_status(:ok) diff --git a/spec/services/wiki_pages/base_service_spec.rb b/spec/services/wiki_pages/base_service_spec.rb index aad8fc4ae7fc29635424ebc2d4974f4be57dcc3c..3dfeacf7bab5df549444c80b82c7b930053ebf23 100644 --- a/spec/services/wiki_pages/base_service_spec.rb +++ b/spec/services/wiki_pages/base_service_spec.rb @@ -24,7 +24,9 @@ def internal_event_name end it 'raises an error on unknown events' do - expect { subject.send(:increment_usage, page) }.to raise_error(Gitlab::InternalEvents::UnknownEventError) + expect do + subject.send(:increment_usage, page) + end.to raise_error(Gitlab::Tracking::EventValidator::UnknownEventError) end end end diff --git a/spec/support/shared_examples/controllers/internal_event_tracking_examples.rb b/spec/support/shared_examples/controllers/internal_event_tracking_examples.rb index b8fe4c2f130405a46ba89f07d3d710f92026e17c..03f669aa5c1b3b19b511852323934dcf49807be9 100644 --- a/spec/support/shared_examples/controllers/internal_event_tracking_examples.rb +++ b/spec/support/shared_examples/controllers/internal_event_tracking_examples.rb @@ -20,7 +20,7 @@ RSpec.shared_examples 'internal event tracking' do let(:all_metrics) do additional_properties = try(:additional_properties) || {} - base_additional_properties = Gitlab::InternalEvents::BASE_ADDITIONAL_PROPERTIES.to_h do |key, _val| + base_additional_properties = Gitlab::Tracking::EventValidator::BASE_ADDITIONAL_PROPERTIES.to_h do |key, _val| [key, try(key)] end diff --git a/spec/support_specs/matchers/internal_events_matchers_spec.rb b/spec/support_specs/matchers/internal_events_matchers_spec.rb index f2be064feebf954864beb2df9793c17cb471f38e..495ff03488209ea2caded9cdeefb3480181d4c47 100644 --- a/spec/support_specs/matchers/internal_events_matchers_spec.rb +++ b/spec/support_specs/matchers/internal_events_matchers_spec.rb @@ -9,6 +9,10 @@ let_it_be(:group_2) { create(:group) } let_it_be(:project_1) { create(:project, namespace: group_1) } + before do + allow(Gitlab::Tracking::EventValidator).to receive_message_chain(:new, :validate!) + end + def track_event(event: nil, user: nil, group: nil) Gitlab::InternalEvents.track_event( event || 'g_edit_by_sfe',