Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 46 additions & 32 deletions sentry_sdk/integrations/django/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
capture_internal_exceptions,
ensure_integration_enabled,
event_from_exception,
has_data_collection_enabled,
logger,
transaction_from_function,
walk_exception_chain,
Expand Down Expand Up @@ -457,6 +458,39 @@ def _attempt_resolve_again(
_set_transaction_name_and_source(scope, transaction_style, request)


def _get_user_from_request_and_set_on_scope(request: "WSGIRequest") -> None:
user = getattr(request, "user", None)

# Evaluating a SimpleLazyObject in an async view can raise django.core.exceptions.SynchronousOnlyOperation.
# Exit early if the user has not been materialized yet.
is_lazy = isinstance(user, SimpleLazyObject)
if is_lazy and hasattr(request, "_cached_user"):
user = request._cached_user
elif is_lazy:
return

if user is None or not is_authenticated(user):
return

user_info = {}
try:
user_info["id"] = str(user.pk)
except Exception:
pass

try:
user_info["email"] = user.email
except Exception:
pass

try:
user_info["username"] = user.get_username()
except Exception:
pass

sentry_sdk.set_user(user_info)


def _after_get_response(request: "WSGIRequest") -> None:
client = sentry_sdk.get_client()
integration = client.get_integration(DjangoIntegration)
Expand All @@ -468,37 +502,12 @@ def _after_get_response(request: "WSGIRequest") -> None:
_attempt_resolve_again(request, scope, integration.transaction_style)

span_streaming = has_span_streaming_enabled(client.options)
if span_streaming and should_send_default_pii():
user = getattr(request, "user", None)

# Evaluating a SimpleLazyObject in an async view can raise django.core.exceptions.SynchronousOnlyOperation.
# Exit early if the user has not been materialized yet.
is_lazy = isinstance(user, SimpleLazyObject)
if is_lazy and hasattr(request, "_cached_user"):
user = request._cached_user
elif is_lazy:
return

if user is None or not is_authenticated(user):
return

user_info = {}
try:
user_info["id"] = str(user.pk)
except Exception:
pass

try:
user_info["email"] = user.email
except Exception:
pass

try:
user_info["username"] = user.get_username()
except Exception:
pass

sentry_sdk.set_user(user_info)
if span_streaming:
if has_data_collection_enabled(client.options):
if client.options["data_collection"]["user_info"]:
_get_user_from_request_and_set_on_scope(request)
elif should_send_default_pii():
_get_user_from_request_and_set_on_scope(request)


def _patch_get_response() -> None:
Expand Down Expand Up @@ -544,7 +553,12 @@ def wsgi_request_event_processor(event: "Event", hint: "dict[str, Any]") -> "Eve
with capture_internal_exceptions():
DjangoRequestExtractor(request).extract_into_event(event)

if should_send_default_pii():
client_options = sentry_sdk.get_client().options
if has_data_collection_enabled(client_options):
if client_options["data_collection"]["user_info"]:
with capture_internal_exceptions():
_set_user_info(request, event)
elif should_send_default_pii():
with capture_internal_exceptions():
_set_user_info(request, event)

Expand Down
8 changes: 7 additions & 1 deletion sentry_sdk/integrations/django/asgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from sentry_sdk.utils import (
capture_internal_exceptions,
ensure_integration_enabled,
has_data_collection_enabled,
)

if TYPE_CHECKING:
Expand Down Expand Up @@ -70,7 +71,12 @@ def asgi_request_event_processor(event: "Event", hint: "dict[str, Any]") -> "Eve
with capture_internal_exceptions():
DjangoRequestExtractor(request).extract_into_event(event)

if should_send_default_pii():
client_options = sentry_sdk.get_client().options
if has_data_collection_enabled(client_options):
if client_options["data_collection"]["user_info"]:
with capture_internal_exceptions():
_set_user_info(request, event)
elif should_send_default_pii():
with capture_internal_exceptions():
_set_user_info(request, event)

Expand Down
34 changes: 34 additions & 0 deletions tests/integrations/django/asgi/test_asgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@
from sentry_sdk.integrations.django import DjangoIntegration
from sentry_sdk.integrations.django.asgi import _asgi_middleware_mixin_factory
from tests.integrations.django.myapp.asgi import channels_application
from tests.integrations.django.utils import (
USER_INFO_INIT_KWARGS,
pytest_mark_django_db_decorator,
)

try:
from django.urls import reverse
Expand Down Expand Up @@ -1096,3 +1100,33 @@ async def test_async_middleware_process_exception_is_awaited(

assert response["status"] == 200
assert response["body"] == b"handled by async process_exception"


@pytest.mark.forked

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the @pytest.mark.forked here? It tends to take longer and it sometimes causes hard to debug problems with tests, so if it's not necessary, would be better to omit it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the @pytest.mark.forked here?

I think we might - this is a copy/paste from database-related tests in the test_data_scrubbing file and my understanding from it is that any time a test touches a database, we need this decorator to isolate state.

I might've come to an incorrect understanding though, so please let me know if that's not quite right.

@pytest.mark.parametrize("application", APPS)
@pytest.mark.asyncio
@pytest.mark.skipif(
django.VERSION < (3, 0), reason="Django ASGI support shipped in 3.0"
)
@pytest.mark.parametrize("init_kwargs, expect_user", USER_INFO_INIT_KWARGS)
@pytest_mark_django_db_decorator()
async def test_user_identity_error_event_data_collection(
sentry_init, capture_events, application, init_kwargs, expect_user
):
sentry_init(integrations=[DjangoIntegration()], **init_kwargs)
events = capture_events()

comm = HttpCommunicator(application, "GET", "/mylogin-with-exception")
await comm.get_response()
await comm.wait()

event = events[-1]

if expect_user:
assert event["user"]["id"] == "1"
assert event["user"]["email"] == "lennon@thebeatles.com"
assert event["user"]["username"] == "john"
Comment thread
sentry-warden[bot] marked this conversation as resolved.
else:
assert "id" not in event.get("user", {})
assert "email" not in event.get("user", {})
assert "username" not in event.get("user", {})
5 changes: 5 additions & 0 deletions tests/integrations/django/myapp/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ def path(path, *args, **kwargs):
path("nomessage", views.nomessage, name="nomessage"),
path("view-with-signal", views.view_with_signal, name="view_with_signal"),
path("mylogin", views.mylogin, name="mylogin"),
path(
"mylogin-with-exception",
views.mylogin_with_exception,
name="mylogin_with_exception",
),
path("classbased", views.ClassBasedView.as_view(), name="classbased"),
path("sentryclass", views.SentryClassBasedView(), name="sentryclass"),
path(
Expand Down
10 changes: 10 additions & 0 deletions tests/integrations/django/myapp/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,16 @@ def mylogin(request):
return HttpResponse("ok")


@csrf_exempt
def mylogin_with_exception(request):
user, _ = User.objects.get_or_create(
username="john", defaults={"email": "lennon@thebeatles.com"}
)
user.backend = "django.contrib.auth.backends.ModelBackend"
login(request, user)
1 / 0


@csrf_exempt
def handler500(request):
return HttpResponseServerError("Sentry error.")
Expand Down
99 changes: 64 additions & 35 deletions tests/integrations/django/test_data_scrubbing.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@
from sentry_sdk.integrations.django import DjangoIntegration
from tests.conftest import unpack_werkzeug_response, werkzeug_set_cookie
from tests.integrations.django.myapp.wsgi import application
from tests.integrations.django.utils import pytest_mark_django_db_decorator
from tests.integrations.django.utils import (
USER_INFO_INIT_KWARGS,
pytest_mark_django_db_decorator,
)

try:
from django.urls import reverse
Expand Down Expand Up @@ -392,40 +395,6 @@ def test_empty_query_string_is_dropped_with_data_collection(
assert "query_string" not in event["request"]


USER_INFO_INIT_KWARGS = [
pytest.param({"send_default_pii": True}, True, id="legacy_send_default_pii_true"),
pytest.param(
{"send_default_pii": False}, False, id="legacy_send_default_pii_false"
),
pytest.param(
{"_experiments": {"data_collection": {"user_info": True}}},
True,
id="data_collection_user_info_true",
),
pytest.param(
{"_experiments": {"data_collection": {"user_info": False}}},
False,
id="data_collection_user_info_false",
),
pytest.param(
{
"send_default_pii": True,
"_experiments": {"data_collection": {"user_info": False}},
},
False,
id="data_collection_wins_over_send_default_pii_true",
),
pytest.param(
{
"send_default_pii": False,
"_experiments": {"data_collection": {"user_info": True}},
},
True,
id="data_collection_wins_over_send_default_pii_false",
),
]


@pytest.mark.forked
@pytest_mark_django_db_decorator()
@pytest.mark.parametrize("init_kwargs, expect_ip", USER_INFO_INIT_KWARGS)
Expand Down Expand Up @@ -462,6 +431,42 @@ def test_user_info_span_attributes_data_collection(
assert "client.address" not in root_span["attributes"]


@pytest.mark.forked
@pytest_mark_django_db_decorator()
@pytest.mark.parametrize("init_kwargs, expect_user", USER_INFO_INIT_KWARGS)
def test_user_identity_span_attributes_data_collection(
sentry_init, client, capture_items, init_kwargs, expect_user
):
init_kwargs = dict(init_kwargs) # shallow copy so we can mutate
experiments = init_kwargs.pop("_experiments", {})

sentry_init(
integrations=[DjangoIntegration()],
traces_sample_rate=1.0,
trace_lifecycle="stream",
_experiments=experiments,
**init_kwargs,
)

unpack_werkzeug_response(client.get(reverse("mylogin")))

items = capture_items("span")
unpack_werkzeug_response(client.get(reverse("template_test")))
sentry_sdk.flush()

spans = [item.payload for item in items]
(span,) = (s for s in spans if s["name"] == "/template-test")

if expect_user:
assert span["attributes"][SPANDATA.USER_ID] == "1"
assert span["attributes"][SPANDATA.USER_EMAIL] == "lennon@thebeatles.com"
assert span["attributes"][SPANDATA.USER_NAME] == "john"
else:
assert SPANDATA.USER_ID not in span["attributes"]
assert SPANDATA.USER_EMAIL not in span["attributes"]
assert SPANDATA.USER_NAME not in span["attributes"]


@pytest.mark.forked
@pytest_mark_django_db_decorator()
@pytest.mark.parametrize("init_kwargs, expect_ip", USER_INFO_INIT_KWARGS)
Expand All @@ -483,6 +488,30 @@ def test_user_info_error_event_data_collection(
assert "REMOTE_ADDR" not in event["request"]["env"]


@pytest.mark.forked
@pytest_mark_django_db_decorator()
@pytest.mark.parametrize("init_kwargs, expect_user", USER_INFO_INIT_KWARGS)
def test_user_identity_error_event_data_collection(
sentry_init, client, capture_events, init_kwargs, expect_user
):
sentry_init(integrations=[DjangoIntegration()], **init_kwargs)
events = capture_events()

client.get(reverse("mylogin"))
client.get(reverse("view_exc"))

event = events[-1]

if expect_user:
assert event["user"]["id"] == "1"
assert event["user"]["email"] == "lennon@thebeatles.com"
assert event["user"]["username"] == "john"
else:
assert "id" not in event.get("user", {})
assert "email" not in event.get("user", {})
assert "username" not in event.get("user", {})


@pytest.mark.forked
@pytest_mark_django_db_decorator()
def test_error_event_no_user_ip_address_without_remote_addr(
Expand Down
37 changes: 37 additions & 0 deletions tests/integrations/django/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,40 @@
)
except AttributeError:
pass


# Shared parametrization test matrix exercising the precedence between the legacy
# ``send_default_pii`` boolean and the ``data_collection.user_info`` setting.
# The second value indicates whether user info is expected to be collected.
USER_INFO_INIT_KWARGS = [
pytest.param({"send_default_pii": True}, True, id="legacy_send_default_pii_true"),
pytest.param(
{"send_default_pii": False}, False, id="legacy_send_default_pii_false"
),
pytest.param(
{"_experiments": {"data_collection": {"user_info": True}}},
True,
id="data_collection_user_info_true",
),
pytest.param(
{"_experiments": {"data_collection": {"user_info": False}}},
False,
id="data_collection_user_info_false",
),
pytest.param(
{
"send_default_pii": True,
"_experiments": {"data_collection": {"user_info": False}},
},
False,
id="data_collection_wins_over_send_default_pii_true",
),
pytest.param(
{
"send_default_pii": False,
"_experiments": {"data_collection": {"user_info": True}},
},
True,
id="data_collection_wins_over_send_default_pii_false",
),
]
Loading