Skip to content

Commit fa3c8a1

Browse files
fix(asm): improve django login events logic [backport 2.6] (#8704)
Backport 0d681e0 from #8698 to 2.6. ASM: This fix resolves an issue where a failed login attempt could be sent for a valid user if the name of the user was `AnonymousUser`. ## Checklist - [x] Change(s) are motivated and described in the PR description - [x] Testing strategy is described if automated tests are not included in the PR - [x] Risks are described (performance impact, potential for breakage, maintainability) - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. - [x] If change touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from `@DataDog/security-design-and-guidance`. ## Reviewer Checklist - [x] Title is accurate - [x] All changes are related to the pull request's stated goal - [x] Description motivates each change - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [x] Testing strategy adequately addresses listed risks - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] Release note makes sense to a user of the library - [x] Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [x] Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) Co-authored-by: Federico Mon <[email protected]>
1 parent 372b28d commit fa3c8a1

File tree

3 files changed

+31
-16
lines changed

3 files changed

+31
-16
lines changed

ddtrace/appsec/_trace_utils.py

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -296,25 +296,21 @@ def _on_django_login(
296296
return
297297

298298
if user:
299-
if str(user) != "AnonymousUser":
299+
from ddtrace.contrib.django.compat import user_is_authenticated
300+
301+
if user_is_authenticated(user):
300302
user_id, user_extra = info_retriever.get_user_info()
301303

302304
with pin.tracer.trace("django.contrib.auth.login", span_type=SpanTypes.AUTH):
303-
from ddtrace.contrib.django.compat import user_is_authenticated
304-
305-
if user_is_authenticated(user):
306-
session_key = getattr(request, "session_key", None)
307-
track_user_login_success_event(
308-
pin.tracer,
309-
user_id=user_id,
310-
session_id=session_key,
311-
propagate=True,
312-
login_events_mode=mode,
313-
**user_extra,
314-
)
315-
else:
316-
# Login failed but the user exists
317-
track_user_login_failure_event(pin.tracer, user_id=user_id, exists=True, login_events_mode=mode)
305+
session_key = getattr(request, "session_key", None)
306+
track_user_login_success_event(
307+
pin.tracer,
308+
user_id=user_id,
309+
session_id=session_key,
310+
propagate=True,
311+
login_events_mode=mode,
312+
**user_extra,
313+
)
318314
else:
319315
# Login failed and the user is unknown
320316
user_id = info_retriever.get_userid()
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
fixes:
3+
- |
4+
ASM: This fix resolves an issue where a valid user may trigger a failed login event.

tests/contrib/django/test_django_appsec.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,21 @@ def test_django_login_sucess_safe_is_default_if_wrong(client, test_spans, tracer
198198
assert login_span.get_tag(user.ID) == "1"
199199

200200

201+
@pytest.mark.django_db
202+
def test_django_login_sucess_anonymous_username(client, test_spans, tracer):
203+
from django.contrib.auth import get_user
204+
from django.contrib.auth.models import User
205+
206+
with override_global_config(dict(_asm_enabled=True, _automatic_login_events_mode="foobar")):
207+
test_user = User.objects.create(username="AnonymousUser")
208+
test_user.set_password("secret")
209+
test_user.save()
210+
client.login(username="AnonymousUser", password="secret")
211+
assert get_user(client).is_authenticated
212+
login_span = test_spans.find_span(name="django.contrib.auth.login")
213+
assert login_span.get_tag(user.ID) == "1"
214+
215+
201216
@pytest.mark.django_db
202217
def test_django_login_sucess_safe_is_default_if_missing(client, test_spans, tracer):
203218
from django.contrib.auth import get_user

0 commit comments

Comments
 (0)