From 44834a4d228ffcac2d554e3fe02e292f69002821 Mon Sep 17 00:00:00 2001 From: Yury Lebedev Date: Mon, 12 Aug 2024 16:23:48 +0200 Subject: [PATCH] Make user_id optional for track_login_failure Fixes #3840. `Datadog::Kit::AppSec::Events.track_login_failure` should not raise when `user_id` is not provided and `user_exists` flag set to false. --- lib/datadog/kit/appsec/events.rb | 6 ++---- spec/datadog/kit/appsec/events_spec.rb | 23 ++++++++++++++++------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/lib/datadog/kit/appsec/events.rb b/lib/datadog/kit/appsec/events.rb index 2320509a707..be0b0f049c1 100644 --- a/lib/datadog/kit/appsec/events.rb +++ b/lib/datadog/kit/appsec/events.rb @@ -53,13 +53,11 @@ def track_login_success(trace = nil, span = nil, user:, **others) # @param user_exists [bool] Whether the user id that did a login attempt exists. # @param others [Hash] Additional free-form # event information to attach to the trace. - def track_login_failure(trace = nil, span = nil, user_id:, user_exists:, **others) + def track_login_failure(trace = nil, span = nil, user_exists:, user_id: nil, **others) set_trace_and_span_context('track_login_failure', trace, span) do |active_trace, active_span| - raise ArgumentError, 'user_id cannot be nil' if user_id.nil? - track(LOGIN_FAILURE_EVENT, active_trace, active_span, **others) - active_span.set_tag('appsec.events.users.login.failure.usr.id', user_id) + active_span.set_tag('appsec.events.users.login.failure.usr.id', user_id) if user_id active_span.set_tag('appsec.events.users.login.failure.usr.exists', user_exists) end end diff --git a/spec/datadog/kit/appsec/events_spec.rb b/spec/datadog/kit/appsec/events_spec.rb index 4d34b30a169..5a296104cac 100644 --- a/spec/datadog/kit/appsec/events_spec.rb +++ b/spec/datadog/kit/appsec/events_spec.rb @@ -120,13 +120,6 @@ end end - it 'sets user non-existence on trace' do - trace_op.measure('root') do |span, _trace| - described_class.track_login_failure(trace_op, user_id: '42', user_exists: false) - expect(span.tags).to include('appsec.events.users.login.failure.usr.exists' => 'false') - end - end - it 'sets other keys on trace' do trace_op.measure('root') do |span, _trace| described_class.track_login_failure(trace_op, user_id: '42', user_exists: true, foo: 'bar') @@ -134,6 +127,22 @@ end end + context 'when user does not exist' do + it 'sets user non-existence on trace' do + trace_op.measure('root') do |span, _trace| + described_class.track_login_failure(trace_op, user_exists: false) + expect(span.tags).to include('appsec.events.users.login.failure.usr.exists' => 'false') + end + end + + it 'does not set user id on trace' do + trace_op.measure('root') do |span, _trace| + described_class.track_login_failure(trace_op, user_exists: false) + expect(span.tags).not_to have_key('appsec.events.users.login.failure.usr.id') + end + end + end + it_behaves_like 'uses AppSec scope' do let(:event_tag) { 'appsec.events.users.login.failure.track' } subject(:event) { described_class.track_login_failure(trace_op, user_id: '42', user_exists: true) }