-
Notifications
You must be signed in to change notification settings - Fork 571
Fix user handling for django ASGI #5282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
cf2d25c to
bd37ba6
Compare
| @pytest_mark_django_db_decorator() | ||
| @pytest.mark.skipif( | ||
| django.VERSION < (3, 0), reason="Django ASGI support shipped in 3.0" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test skipif condition requires newer Django version
Medium Severity
The test test_user_pii_in_asgi_with_auth has a skipif condition checking for django.VERSION < (3, 0), but the test view async_mylogin uses User.objects.acreate_user() (added in Django 4.1) and alogin (added in Django 5.1). This mismatch means the test will fail with an ImportError when run on Django versions 3.0 through 5.0, since the skipif condition won't exclude those versions but the required APIs don't exist.
🔬 Verification Test
Why verification test was not possible: This test failure would manifest only when running the test suite against specific Django versions (3.x, 4.x, 5.0). The test would fail with an ImportError: cannot import name 'alogin' from 'django.contrib.auth' when Django < 5.1 is used. Verifying this would require setting up multiple Django version environments which is outside the scope of this review.
Additional Locations (1)
|
|
||
| def _set_user_info(request: "ASGIRequest", event: "Event") -> None: | ||
| user_info = getattr(request, "_sentry_user_info", {}) | ||
| event.setdefault("user", user_info) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Async user info doesn't merge with existing user data
Low Severity
The new async _set_user_info uses event.setdefault("user", user_info) which only sets the user dict if no "user" key exists. In contrast, the sync version in sentry_sdk/integrations/django/__init__.py uses event.setdefault("user", {}) then user_info.setdefault() on individual fields, which merges request user data into any existing user dict. This means if scope.set_user({"id": "custom"}) is called before the event processor runs, the sync version would still add email and username from the authenticated user, but the async version adds nothing. This behavioral inconsistency could cause missing user fields in async contexts.
🔬 Verification Test
Why verification test was not possible: This requires testing the interaction between scope.set_user() and the event processor in an async Django context, which would need a full integration test environment with Django ASGI running. The behavioral difference is clear from comparing the sync implementation at line 584-605 in __init__.py (which calls setdefault on individual fields) versus the async implementation (which calls setdefault on the entire user dict).
bd37ba6 to
a908d9d
Compare
a908d9d to
6c0c2a7
Compare
Description
For async views in django, we're supposed to use the async version
auserto fetch user info.Issues