Skip to content

Commit

Permalink
Make user_id optional for track_login_failure
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
y9v committed Aug 12, 2024
1 parent 46c8c8b commit 44834a4
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 11 deletions.
6 changes: 2 additions & 4 deletions lib/datadog/kit/appsec/events.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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<String || Symbol, String>] 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
Expand Down
23 changes: 16 additions & 7 deletions spec/datadog/kit/appsec/events_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -120,20 +120,29 @@
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')
expect(span.tags).to include('appsec.events.users.login.failure.foo' => 'bar')
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) }
Expand Down

0 comments on commit 44834a4

Please sign in to comment.