From a46652c1f71bbc9878d44b58b749bf5257c55db6 Mon Sep 17 00:00:00 2001 From: Greg Pstrucha <875316+gricha@users.noreply.github.com> Date: Wed, 28 Oct 2026 22:48:48 -0700 Subject: [PATCH] ref(auth): Set request.user and request.auth through one helper Adds set_request_principal and routes every place in src that sets request.user or request.auth through it, including DRF authentication. A test fails on new direct assignments. When DRF authentication finds a caller, the viewer context is updated to match, since endpoints with their own authentication classes run them after the viewer-context middleware. A credential with no caller of its own, such as a signed service call, keeps the viewer from the header. The actor now also carries superuser elevation from the credential. ApiClient runs the view in its own viewer-context scope so the view's caller doesn't replace ours. --- src/AGENTS.md | 1 + src/sentry/api/base.py | 8 +- src/sentry/api/client.py | 15 ++- src/sentry/api/endpoints/auth_index.py | 7 +- src/sentry/auth/principal.py | 51 ++++++++-- src/sentry/auth/services/auth/model.py | 8 +- src/sentry/middleware/auth.py | 13 ++- src/sentry/testutils/cases.py | 6 +- src/sentry/testutils/requests.py | 4 +- src/sentry/viewer_context.py | 11 ++ src/sentry/web/frontend/auth_logout.py | 3 +- tests/sentry/api/test_client.py | 20 ++++ tests/sentry/auth/test_principal.py | 134 ++++++++++++++++++++++++- 13 files changed, 246 insertions(+), 35 deletions(-) diff --git a/src/AGENTS.md b/src/AGENTS.md index db3fa11d60cd..2fe2eb62ee8d 100644 --- a/src/AGENTS.md +++ b/src/AGENTS.md @@ -73,6 +73,7 @@ The `ViewerContext` contextvar carries the current viewer's identity. When the v - `acting_user_id(request.user)`: the user behind the call (an agent's is the user it acts for), or None. Handle None. - `has_user(request.user)`: whether there is one; narrows to `User | RpcUser`. - `UserRequired` (403): raise it when code needs a user and there is none. +- `set_request_principal(request, user, auth)`: the only place `request.user` and `request.auth` are set. Personal state (bookmarks, "is this mine?") needs a user; "who did this" fields record None. Never look rows up by a possibly-None user id: `filter(user_id=None)` matches rows with no user. diff --git a/src/sentry/api/base.py b/src/sentry/api/base.py index d8f83f3a17af..ae2508ce16a4 100644 --- a/src/sentry/api/base.py +++ b/src/sentry/api/base.py @@ -43,6 +43,7 @@ ) from sentry.apidocs.hooks import HTTP_METHOD_NAME from sentry.auth import access +from sentry.auth.principal import set_request_principal from sentry.auth.scope_declaration import bind_endpoint_scope_declaration from sentry.auth.staff import has_staff_option from sentry.hybridcloud.apigateway.cell_request_resolvers import CellRequestResolver @@ -439,10 +440,9 @@ def initialize_request(self, request: HttpRequest, *args: Any, **kwargs: Any) -> # If our request is being made via our internal API client, we need to # stitch back on auth and user information if getattr(request, "__from_api_client__", False): - if rv.auth is None: - rv.auth = orig_auth - if rv.user is None: - rv.user = orig_user # type: ignore[unreachable] # the request here is partially initialized + auth = orig_auth if rv.auth is None else rv.auth + user = orig_user if rv.user is None else rv.user + set_request_principal(rv, user, auth) return rv def has_pagination(self, response: Response) -> bool: diff --git a/src/sentry/api/client.py b/src/sentry/api/client.py index 4d28fa0df9c9..81609a666bd2 100644 --- a/src/sentry/api/client.py +++ b/src/sentry/api/client.py @@ -8,7 +8,9 @@ from django.urls import resolve from rest_framework.test import APIRequestFactory, force_authenticate +from sentry.auth.principal import set_request_principal from sentry.auth.superuser import Superuser +from sentry.viewer_context import get_viewer_context, viewer_context_scope __all__ = ("ApiClient", "ApiError", "client") @@ -30,7 +32,12 @@ class ApiClient: ApiError: TypeAlias = ApiError - def request( + def request(self, *args, **kwargs): + # The called view sets its own caller on the viewer context; keep it out of ours. + with viewer_context_scope(get_viewer_context()): + return self._request(*args, **kwargs) + + def _request( self, method, path, @@ -65,8 +72,7 @@ def request( mock_request.__from_api_client__ = True if request: - mock_request.auth = getattr(request, "auth", None) - mock_request.user = request.user + set_request_principal(mock_request, request.user, getattr(request, "auth", None)) if is_sudo is None: mock_request.is_sudo = lambda: request.is_sudo() @@ -79,8 +85,7 @@ def request( else: mock_request.superuser = Superuser(mock_request) else: - mock_request.auth = auth - mock_request.user = user or AnonymousUser() + set_request_principal(mock_request, user or AnonymousUser(), auth) mock_request.is_sudo = lambda: is_sudo mock_request.session = {} mock_request.superuser = Superuser(mock_request) diff --git a/src/sentry/api/endpoints/auth_index.py b/src/sentry/api/endpoints/auth_index.py index b69fd40cef29..ec142e3e9721 100644 --- a/src/sentry/api/endpoints/auth_index.py +++ b/src/sentry/api/endpoints/auth_index.py @@ -21,6 +21,7 @@ from sentry.api.validators import AuthVerifyValidator from sentry.api.validators.auth import MISSING_PASSWORD_OR_U2F_CODE from sentry.auth.authenticators.u2f import U2fInterface +from sentry.auth.principal import set_request_principal from sentry.auth.providers.saml2.provider import handle_saml_single_logout from sentry.auth.services.auth.impl import promote_request_rpc_user from sentry.auth.superuser import SUPERUSER_ORG_ID, SuperuserAccessSerializer @@ -238,7 +239,7 @@ def post(self, request: Request) -> Response: status=status.HTTP_403_FORBIDDEN, ) - request.user = request._request.user + set_request_principal(request, request._request.user, request.auth) return self.get(request) @@ -356,7 +357,7 @@ def put(self, request: Request) -> Response: serializer.validated_data["superuserReason"], ) - request.user = request._request.user + set_request_principal(request, request._request.user, request.auth) return self.get(request) @@ -377,7 +378,7 @@ def delete(self, request: Request, *args, **kwargs) -> Response: # For signals to work here, we must promote the request.user to a full user object logout(request._request) - request.user = AnonymousUser() + set_request_principal(request, AnonymousUser(), request.auth) # Force cookies to be deleted response = Response() diff --git a/src/sentry/auth/principal.py b/src/sentry/auth/principal.py index ad932da4cc73..3cc1792cf2f4 100644 --- a/src/sentry/auth/principal.py +++ b/src/sentry/auth/principal.py @@ -3,10 +3,15 @@ from typing import Any, Protocol, TypeIs from django.contrib.auth.models import AnonymousUser +from django.http.request import HttpRequest +from rest_framework.request import Request +from sentry.auth.services.auth.model import AuthenticatedToken +from sentry.types.superuser import to_viewer_context_superuser from sentry.users.models.user import User from sentry.users.services.user.model import RpcUser -from sentry.viewer_context import Actor, ActorType +from sentry.utils.auth import is_user_from_viewer_context +from sentry.viewer_context import Actor, ActorType, set_viewer_context_actor class RequestPrincipal(Protocol): @@ -54,24 +59,58 @@ def resolve_actor(user: Any, auth: Any) -> Actor | None: """The actor behind an authenticated ``(user, auth)`` pair, or None if there is none.""" from sentry.seer.agent_token import is_agent_auth # circular: imported at app setup + superuser = ( + to_viewer_context_superuser(auth.superuser) + if isinstance(auth, AuthenticatedToken) + else None + ) if is_agent_auth(auth): - return Actor(type=ActorType.AGENT, delegated_user_id=auth.user_id) + return Actor(type=ActorType.AGENT, delegated_user_id=auth.user_id, superuser=superuser) user_id = acting_user_id(user) if user_id is not None: - return Actor(type=ActorType.USER, actor_id=user_id) + return Actor(type=ActorType.USER, actor_id=user_id, superuser=superuser) return None +def set_request_principal(request: HttpRequest | Request, user: Any, auth: Any) -> None: + """ + Set ``request.user`` and ``request.auth``. + + The only place they're set, so what lands on ``request.user`` is decided here. + """ + request.user = user + request.auth = auth + + +_drf_authenticate = vars(Request)["_authenticate"] + + +def _authenticate(self: Request) -> None: + # DRF sets the authenticator's result itself; route it through the chokepoint. + _drf_authenticate(self) + set_request_principal(self, self.user, self.auth) + # Endpoints with their own authentication classes run them after the viewer-context + # middleware, so update the caller it recorded. A credential with no caller of its own, + # e.g. a signed service call, keeps the one from the viewer-context header. + # A user loaded from a signed viewer context keeps its actor too, e.g. an agent. + if is_user_from_viewer_context(self): + return + if (actor := resolve_actor(self.user, self.auth)) is not None: + set_viewer_context_actor(actor) + + def _anonymous_display_name(self: AnonymousUser) -> str: return "" def patch_request_principal() -> None: """ - Give Django's ``AnonymousUser`` the ``RequestPrincipal`` interface. + Route DRF authentication through ``set_request_principal`` and give Django's + ``AnonymousUser`` the ``RequestPrincipal`` interface. - Patched rather than subclassed because Django, DRF and our own code construct it in - many places. ``tools/mypy_helpers/plugin.py`` declares the same members. + Patched rather than subclassed because Django, DRF and our own code construct these + in many places. ``tools/mypy_helpers/plugin.py`` declares the same members. """ + Request._authenticate = _authenticate # type: ignore[attr-defined] AnonymousUser.is_sentry_app = False AnonymousUser.get_display_name = _anonymous_display_name # type: ignore[method-assign] diff --git a/src/sentry/auth/services/auth/model.py b/src/sentry/auth/services/auth/model.py index ab69089f6944..49cf7fcba8d7 100644 --- a/src/sentry/auth/services/auth/model.py +++ b/src/sentry/auth/services/auth/model.py @@ -166,17 +166,17 @@ def applied_to_request[T: HttpRequest](self, request: T) -> Generator[T]: isolated, controlled way. This method allows for a context handling an RPC or inter silo behavior to assume the correct user and auth context provided explicitly in a context. """ + from sentry.auth.principal import set_request_principal # imported at app setup + old_user = request.user old_auth = request.auth - request.user = self._get_user() # type: ignore[assignment] # RpcUser vs. User - request.auth = self.auth + set_request_principal(request, self._get_user(), self.auth) try: yield request finally: - request.user = old_user - request.auth = old_auth + set_request_principal(request, old_user, old_auth) class RpcAuthProviderFlags(RpcModel): diff --git a/src/sentry/middleware/auth.py b/src/sentry/middleware/auth.py index c56e1caedc78..e2fc08d87bf8 100644 --- a/src/sentry/middleware/auth.py +++ b/src/sentry/middleware/auth.py @@ -19,6 +19,7 @@ UserAuthTokenAuthentication, ViewerContextAuthentication, ) +from sentry.auth.principal import set_request_principal from sentry.users.models.userip import UserIP from sentry.utils.auth import AuthUserPasswordExpired, logger @@ -68,7 +69,7 @@ def process_request(self, request: Request) -> None: if request.path.startswith("/api/0/internal/rpc/"): # Avoid doing RPC authentication when we're already # in an RPC request. - request.user, request.auth = AnonymousUser(), None + set_request_principal(request, AnonymousUser(), None) return auth = get_authorization_header(request).split() @@ -83,10 +84,12 @@ def process_request(self, request: Request) -> None: except AuthenticationFailed: result = None if result: - request.user, request.auth = result + set_request_principal(request, *result) else: # default to anonymous user and use IP ratelimit - request.user, request.auth = SimpleLazyObject(lambda: get_user(request)), None # type:ignore[assignment] + set_request_principal( + request, SimpleLazyObject(lambda: get_user(request)), None + ) return # Try viewer context authentication (signed headers from trusted services) @@ -95,11 +98,11 @@ def process_request(self, request: Request) -> None: except AuthenticationFailed: result = None if result: - request.user, request.auth = result + set_request_principal(request, *result) return # default to anonymous user and use IP ratelimit - request.user, request.auth = SimpleLazyObject(lambda: get_user(request)), None # type:ignore[assignment] + set_request_principal(request, SimpleLazyObject(lambda: get_user(request)), None) def process_exception( self, request: HttpRequest, exception: Exception diff --git a/src/sentry/testutils/cases.py b/src/sentry/testutils/cases.py index 95cdfdf63edc..9f5f342c638d 100644 --- a/src/sentry/testutils/cases.py +++ b/src/sentry/testutils/cases.py @@ -63,6 +63,7 @@ from sentry import auth from sentry.api.serializers.models.dashboard import DATASET_SOURCES from sentry.auth.authenticators.totp import TotpInterface +from sentry.auth.principal import set_request_principal from sentry.auth.provider import Provider from sentry.auth.providers.dummy import DummyProvider from sentry.auth.providers.saml2.activedirectory.apps import ( @@ -297,8 +298,7 @@ def make_request( # order matters here, session -> user -> other things request.session = self.session - request.auth = auth - request.user = user or AnonymousUser() + set_request_principal(request, user or AnonymousUser(), auth) # must happen after request.user/request.session is populated request.superuser = Superuser(request) request.staff = Staff(request) @@ -332,7 +332,7 @@ def login_as( request = self.make_request() with assume_test_silo_mode(SiloMode.CONTROL): login(request, user) - request.user = user + set_request_principal(request, user, request.auth) if organization_ids is None: organization_ids = set() diff --git a/src/sentry/testutils/requests.py b/src/sentry/testutils/requests.py index e97b73fbe7aa..7c9a3b4c4527 100644 --- a/src/sentry/testutils/requests.py +++ b/src/sentry/testutils/requests.py @@ -10,6 +10,7 @@ from rest_framework.request import Request from rest_framework.views import APIView +from sentry.auth.principal import set_request_principal from sentry.middleware.auth import AuthenticationMiddleware from sentry.middleware.placeholder import placeholder_get_response from sentry.testutils.factories import Factories @@ -29,8 +30,7 @@ def wrapper(*args, **kwds) -> tuple[HttpRequest, User] | None: login(request, user) AuthenticationMiddleware(placeholder_get_response).process_request(request) else: - request.user = user - request.auth = None + set_request_principal(request, user, None) cache.clear() return result diff --git a/src/sentry/viewer_context.py b/src/sentry/viewer_context.py index d0eae6a448e6..02e96e84e58e 100644 --- a/src/sentry/viewer_context.py +++ b/src/sentry/viewer_context.py @@ -161,6 +161,17 @@ def set_viewer_context_superuser( ) +def set_viewer_context_actor(actor: Actor) -> None: + """Replace the caller, e.g. when an endpoint authenticates after the middleware did.""" + ctx = get_viewer_context() + if ctx is None or ctx.actor == actor: + return + if actor.superuser is None and dataclasses.replace(ctx.actor, superuser=None) == actor: + # Same caller: keep the elevation the access checks granted. + return + _viewer_context_var.set(dataclasses.replace(ctx, actor=actor)) + + def set_viewer_context_organization(organization_id: int) -> None: """Update the current ``ViewerContext`` with a resolved organization id.""" ctx = get_viewer_context() diff --git a/src/sentry/web/frontend/auth_logout.py b/src/sentry/web/frontend/auth_logout.py index e3fbfcddabd6..e1d1c216c379 100644 --- a/src/sentry/web/frontend/auth_logout.py +++ b/src/sentry/web/frontend/auth_logout.py @@ -4,6 +4,7 @@ from django.http.response import HttpResponseBase from django.utils.http import url_has_allowed_host_and_scheme +from sentry.auth.principal import set_request_principal from sentry.utils import auth from sentry.web.frontend.base import BaseView, control_silo_view @@ -23,5 +24,5 @@ def get(self, request: HttpRequest) -> HttpResponseBase: def post(self, request: HttpRequest) -> HttpResponseBase: logout(request) - request.user = AnonymousUser() + set_request_principal(request, AnonymousUser(), request.auth) return self._redirect(request) diff --git a/tests/sentry/api/test_client.py b/tests/sentry/api/test_client.py index 5299ee91325c..819b8e43c360 100644 --- a/tests/sentry/api/test_client.py +++ b/tests/sentry/api/test_client.py @@ -4,6 +4,13 @@ from sentry.api.client import ApiClient from sentry.testutils.cases import TestCase +from sentry.viewer_context import ( + Actor, + ActorType, + ViewerContext, + get_viewer_context, + viewer_context_scope, +) class ClientParameterHandlingTest(TestCase): @@ -42,3 +49,16 @@ def test_mixed_parameters_in_query_string(self, mock_resolve): actual_queries = dict(request.GET.lists()) assert actual_queries == expected_queries + + +class ClientViewerContextTest(TestCase): + @patch("sentry.api.client.resolve") + def test_view_caller_does_not_leak(self, mock_resolve): + mock_view = Mock(return_value=JsonResponse({})) + mock_resolve.return_value = (mock_view, (), {}) + outer = ViewerContext(actor=Actor(type=ActorType.UNKNOWN)) + other = self.create_user() + + with viewer_context_scope(outer): + ApiClient().get(user=other, path="/test/") + assert get_viewer_context() == outer diff --git a/tests/sentry/auth/test_principal.py b/tests/sentry/auth/test_principal.py index 3d7907a2b9b5..6f06c3dacd0c 100644 --- a/tests/sentry/auth/test_principal.py +++ b/tests/sentry/auth/test_principal.py @@ -1,11 +1,34 @@ +import ast +from collections.abc import Iterator +from pathlib import Path +from unittest import mock + from django.contrib.auth.models import AnonymousUser +from django.test import RequestFactory +from rest_framework.authentication import BaseAuthentication +from rest_framework.request import Request -from sentry.auth.principal import RequestPrincipal, acting_user_id, has_user, resolve_actor +from sentry.auth import principal +from sentry.auth.principal import ( + RequestPrincipal, + acting_user_id, + has_user, + resolve_actor, + set_request_principal, +) from sentry.auth.services.auth import AuthenticatedToken from sentry.testutils.cases import TestCase from sentry.testutils.silo import all_silo_test +from sentry.users.models.user import User from sentry.users.services.user.serial import serialize_rpc_user -from sentry.viewer_context import Actor, ActorType +from sentry.viewer_context import ( + Actor, + ActorType, + SuperuserAccess, + ViewerContext, + get_viewer_context, + viewer_context_scope, +) @all_silo_test @@ -50,3 +73,110 @@ def test_agent_token(self) -> None: auth = AuthenticatedToken(kind="agent_token", user_id=self.user.id) actor = resolve_actor(serialize_rpc_user(self.user), auth) assert actor == Actor(type=ActorType.AGENT, delegated_user_id=self.user.id) + + +@all_silo_test +class SetRequestPrincipalTest(TestCase): + def test_sets_user_and_auth(self) -> None: + request = Request(RequestFactory().get("/")) + auth = AuthenticatedToken(kind="org_auth_token") + set_request_principal(request, self.user, auth) + assert request.user is self.user + assert request.auth is auth + + def test_drf_authentication_goes_through_it(self) -> None: + request = Request(RequestFactory().get("/")) + with mock.patch.object( + principal, "set_request_principal", wraps=principal.set_request_principal + ) as set_principal: + assert acting_user_id(request.user) is None + set_principal.assert_called_once_with(request, request.user, request.auth) + + def test_drf_authentication_updates_viewer_context_actor(self) -> None: + ctx = ViewerContext(actor=Actor(type=ActorType.UNKNOWN), organization_id=1) + with viewer_context_scope(ctx): + assert self._drf_request(self.user).user == self.user + assert get_viewer_context() == ViewerContext( + actor=Actor(type=ActorType.USER, actor_id=self.user.id), organization_id=1 + ) + assert get_viewer_context() is None + + def test_drf_authentication_keeps_superuser_for_same_actor(self) -> None: + actor = Actor( + type=ActorType.USER, actor_id=self.user.id, superuser=SuperuserAccess(expires_at=1) + ) + with viewer_context_scope(ViewerContext(actor=actor)): + assert self._drf_request(self.user).user == self.user + assert get_viewer_context() == ViewerContext(actor=actor) + + def test_drf_authentication_without_caller_keeps_actor(self) -> None: + actor = Actor(type=ActorType.USER, actor_id=self.user.id) + with viewer_context_scope(ViewerContext(actor=actor)): + assert self._drf_request(None).user.is_anonymous + assert get_viewer_context() == ViewerContext(actor=actor) + + def test_drf_authentication_from_viewer_context_keeps_its_actor(self) -> None: + actor = Actor(type=ActorType.AGENT, delegated_user_id=self.user.id) + with viewer_context_scope(ViewerContext(actor=actor)): + assert self._drf_request(self.user, from_viewer_context=True).user == self.user + assert get_viewer_context() == ViewerContext(actor=actor) + + def test_does_not_touch_viewer_context(self) -> None: + actor = Actor(type=ActorType.USER, actor_id=self.user.id) + with viewer_context_scope(ViewerContext(actor=actor)): + set_request_principal(RequestFactory().get("/"), AnonymousUser(), None) + assert get_viewer_context() == ViewerContext(actor=actor) + + def _drf_request(self, user: User | None, from_viewer_context: bool = False) -> Request: + class Authentication(BaseAuthentication): + def authenticate(self, request: Request) -> tuple[User, None] | None: + # As ViewerContextAuthentication marks a user it loaded from the signed header. + if from_viewer_context: + setattr(request, "user_from_viewer_context", True) + return (user, None) if user is not None else None + + return Request(RequestFactory().get("/"), authenticators=[Authentication()]) + + +SRC = Path(__file__).parents[3] / "src" / "sentry" + + +def _assign_targets(node: ast.AST) -> Iterator[ast.expr]: + if isinstance(node, ast.Assign): + stack = list(node.targets) + elif isinstance(node, (ast.AnnAssign, ast.AugAssign)): + stack = [node.target] + else: + return + while stack: + target = stack.pop() + if isinstance(target, (ast.Tuple, ast.List)): + stack.extend(target.elts) + else: + yield target + + +def _name(node: ast.expr) -> str: + if isinstance(node, ast.Name): + return node.id + if isinstance(node, ast.Attribute): + return node.attr + return "" + + +def test_request_user_is_only_set_by_set_request_principal() -> None: + offenders = [] + for path in SRC.rglob("*.py"): + if path == SRC / "auth" / "principal.py": + continue + for node in ast.walk(ast.parse(path.read_bytes())): + for target in _assign_targets(node): + if ( + isinstance(target, ast.Attribute) + and target.attr in ("user", "auth") + and _name(target.value).endswith("request") + ): + offenders.append(f"{path.relative_to(SRC)}:{target.lineno}") + assert not offenders, ( + f"Set request.user and request.auth with set_request_principal: {offenders}" + )