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
3 changes: 2 additions & 1 deletion src/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ 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.
- `require_user(request)`: the user, or `UserRequired` (403) if there is none. For one user-only path in a shared endpoint.
- `requires_user = True` on an endpoint: the same check for the whole endpoint.
- `set_request_principal(request, user, auth)`: the only place `request.user` and `request.auth` are set.

A service account is a `ServiceAccountPrincipal`: no user; its access is `request.access`. 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.
Expand Down
18 changes: 17 additions & 1 deletion src/sentry/api/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,11 @@
InsufficientScope,
StaffRequired,
SuperuserRequired,
UserRequired,
)
from sentry.apidocs.hooks import HTTP_METHOD_NAME
from sentry.auth import access
from sentry.auth.principal import set_request_principal
from sentry.auth.principal import RequestPrincipal, has_user, 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
Expand All @@ -55,6 +56,7 @@
from sentry.seer import agent_token
from sentry.silo.base import SiloLimit, SiloMode
from sentry.snuba.query_sources import QuerySource
from sentry.utils import metrics
from sentry.utils.audit import create_audit_entry
from sentry.utils.cursors import Cursor
from sentry.utils.dates import to_datetime
Expand Down Expand Up @@ -275,6 +277,11 @@ class Endpoint(APIView):
rate_limits: RateLimitConfig | Callable[..., RateLimitConfig] = DEFAULT_RATE_LIMIT_CONFIG
enforce_rate_limit: bool = settings.SENTRY_RATELIMITER_ENABLED
servers: list[dict[str, Any]] | None = None
# For endpoints people may call with any credential, API tokens included, but a caller
# with no user (e.g. a service account) may not: it gets a 403 user-required, counted
# in api.user_required. Browser-only endpoints use SessionNoAuthTokenAuthentication,
# which refuses every token; for one user-only path, call require_user(request).
requires_user: bool = False

@classmethod
def as_view(cls, **initkwargs: Any) -> AsView[GenericView]:
Expand Down Expand Up @@ -310,6 +317,12 @@ def build_cursor_link(self, request: HttpRequest, name: str, cursor: Cursor) ->
has_results="true" if bool(cursor) else "false",
)

def initial(self, request: Request, *args: Any, **kwargs: Any) -> None:
super().initial(request, *args, **kwargs)
principal: RequestPrincipal = request.user
if self.requires_user and principal.is_authenticated and not has_user(principal):
raise UserRequired()

def convert_args(self, request: Request, *args, **kwargs):
return (args, kwargs)

Expand Down Expand Up @@ -401,6 +414,9 @@ def handle_exception_with_details(

:returns: A 500 response including the event id of the captured Sentry event.
"""
if isinstance(exc, UserRequired):
# Shows which endpoints callers with no user behind them still can't use.
metrics.incr("api.user_required", tags={"endpoint": type(self).__name__})
try:
# Django REST Framework's built-in exception handler. If `settings.EXCEPTION_HANDLER`
# exists and returns a response, that's used. Otherwise, `exc` is just re-raised
Expand Down
17 changes: 17 additions & 0 deletions src/sentry/auth/principal.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@

from django.contrib.auth.models import AnonymousUser
from django.http.request import HttpRequest
from rest_framework.exceptions import NotAuthenticated
from rest_framework.request import Request

from sentry.api.exceptions import UserRequired
from sentry.auth.services.auth.model import AuthenticatedToken
from sentry.types.superuser import to_viewer_context_superuser
from sentry.users.models.user import User
Expand Down Expand Up @@ -76,6 +78,21 @@ def resolve_actor(user: Any, auth: Any) -> Actor | None:
return None


def require_user(request: HttpRequest | Request) -> User:
"""
The user behind the caller, for code that only works for one.

Raises ``NotAuthenticated`` for anonymous callers and ``UserRequired`` (403) for
callers with no user behind them, e.g. service accounts. For a whole endpoint, set
``requires_user`` instead.
"""
user = request.user
if not has_user(user):
principal: RequestPrincipal = user
raise UserRequired() if principal.is_authenticated else NotAuthenticated()
return user
Comment thread
cursor[bot] marked this conversation as resolved.
Resolved


def set_request_principal(request: HttpRequest | Request, user: Any, auth: Any) -> None:
"""
Set ``request.user`` and ``request.auth``.
Expand Down
3 changes: 3 additions & 0 deletions src/sentry/core/endpoints/organization_projects.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
)
from sentry.apidocs.response_types import DetailResponse
from sentry.apidocs.utils import inline_sentry_response_serializer
from sentry.auth.principal import require_user
from sentry.constants import ObjectStatus
from sentry.core.endpoints.team_projects import (
AuditData,
Expand Down Expand Up @@ -335,6 +336,8 @@ def post(

if not serializer.is_valid():
raise ValidationError(serializer.errors)
# The new project's default team is the creator's own, which needs a person.
require_user(request)
if not self.should_add_creator_to_team(request.user):
raise NotAuthenticated("User is not authenticated")

Expand Down
2 changes: 2 additions & 0 deletions src/sentry/users/api/bases/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ class UserEndpoint(Endpoint):
"""

permission_classes: tuple[type[BasePermission], ...] = (UserPermission,)
requires_user = True

def convert_args(
self, request: Request, user_id: int | str | None = None, *args: Any, **kwargs: Any
Expand Down Expand Up @@ -200,6 +201,7 @@ class RegionSiloUserEndpoint(Endpoint):
"""

permission_classes = (UserPermission,)
requires_user = True

def convert_args(
self, request: Request, user_id: int | str | None = None, *args: Any, **kwargs: Any
Expand Down
31 changes: 31 additions & 0 deletions tests/sentry/api/test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@
from django.http import QueryDict, StreamingHttpResponse
from django.test import override_settings
from pytest import raises
from rest_framework.authentication import BaseAuthentication
from rest_framework.exceptions import ParseError
from rest_framework.permissions import AllowAny, BasePermission
from rest_framework.request import Request
from rest_framework.response import Response
from sentry_sdk import Scope

Expand All @@ -18,8 +20,11 @@
from sentry.api.permissions import SuperuserPermission
from sentry.api.serializers import Serializer, serialize
from sentry.auth import access
from sentry.auth.services.auth import AuthenticatedToken
from sentry.deletions.tasks.hybrid_cloud import schedule_hybrid_cloud_foreign_key_jobs
from sentry.models.apikey import ApiKey
from sentry.service_accounts.auth import SERVICE_ACCOUNT_TOKEN_KIND
from sentry.service_accounts.principal import ServiceAccountPrincipal
from sentry.silo.base import FunctionSiloLimit, SiloMode
from sentry.testutils.cases import APITestCase
from sentry.testutils.helpers.options import override_options
Expand All @@ -44,6 +49,18 @@ def get(self, request):
return Response({"ok": True})


class ServiceAccountAuthentication(BaseAuthentication):
def authenticate(self, request: Request) -> tuple[Any, AuthenticatedToken]:
principal = ServiceAccountPrincipal(service_account_id=1, organization_id=1, name="sa")
token = AuthenticatedToken(kind=SERVICE_ACCOUNT_TOKEN_KIND, entity_id=1, organization_id=1)
return (principal, token)


class DummyUserOnlyEndpoint(DummyEndpoint):
authentication_classes = (ServiceAccountAuthentication,)
requires_user = True


class DummyCsrfProtectedEndpoint(DummyEndpoint):
csrf_protect = True

Expand Down Expand Up @@ -165,6 +182,20 @@ def data_fn(offset, limit):

@all_silo_test
class EndpointTest(APITestCase):
@mock.patch("sentry.api.base.metrics.incr")
def test_requires_user_rejects_service_account(self, mock_incr: MagicMock) -> None:
response = DummyUserOnlyEndpoint.as_view()(self.make_request(method="GET"))
assert response.status_code == 403
assert response.data["detail"]["code"] == "user-required"
mock_incr.assert_any_call("api.user_required", tags={"endpoint": "DummyUserOnlyEndpoint"})

def test_requires_user_allows_user(self) -> None:
class Endpoint(DummyEndpoint):
requires_user = True

request = self.make_request(user=self.user, method="GET")
assert Endpoint.as_view()(request).status_code == 200

def test_csrf_protection_defaults_to_disabled(self) -> None:
assert DummyEndpoint.as_view().csrf_exempt is True

Expand Down
24 changes: 24 additions & 0 deletions tests/sentry/auth/test_principal.py
Original file line number Diff line number Diff line change
@@ -1,24 +1,29 @@
import ast
from collections.abc import Iterator
from pathlib import Path
from typing import Any
from unittest import mock

import pytest
from django.contrib.auth.models import AnonymousUser
from django.test import RequestFactory
from django.utils.functional import SimpleLazyObject
from rest_framework.authentication import BaseAuthentication
from rest_framework.exceptions import NotAuthenticated
from rest_framework.request import Request

from sentry.api.exceptions import UserRequired
from sentry.auth import principal
from sentry.auth.principal import (
RequestPrincipal,
acting_user_id,
has_user,
require_user,
resolve_actor,
set_request_principal,
)
from sentry.auth.services.auth import AuthenticatedToken
from sentry.service_accounts.principal import ServiceAccountPrincipal
from sentry.testutils.cases import TestCase
from sentry.testutils.silo import all_silo_test
from sentry.users.models.user import User
Expand Down Expand Up @@ -77,6 +82,25 @@ def test_agent_token(self) -> None:
assert actor == Actor(type=ActorType.AGENT, delegated_user_id=self.user.id)


class RequireUserTest(TestCase):
def test_user(self) -> None:
request = RequestFactory().get("/")
request.user = self.user
assert require_user(request) == self.user

def test_anonymous(self) -> None:
request = RequestFactory().get("/")
request.user = AnonymousUser()
with pytest.raises(NotAuthenticated):
require_user(request)

def test_service_account(self) -> None:
request: Any = RequestFactory().get("/")
request.user = ServiceAccountPrincipal(service_account_id=1, organization_id=1, name="sa")
with pytest.raises(UserRequired):
require_user(request)


@all_silo_test
class SetRequestPrincipalTest(TestCase):
def test_sets_user_and_auth(self) -> None:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -597,7 +597,6 @@ def _public_mutation_endpoints() -> tuple[PublicMutationEndpoint, ...]:


class ServiceAccountGap(StrEnum):
REQUIRES_USER = "should answer user-required, which needs requires_user / require_user"
USER_ID_FIX = "reads request.user.id; fixed by the per-team acting_user_id(request.user) PRs"
AUDIT_ACTOR = "the audit entry records request.user as its actor"
VIEWER_LOOKUP = "a serializer looks up the viewer's own state by request.user.id"
Expand All @@ -608,7 +607,6 @@ class ServiceAccountGap(StrEnum):

# Rows that don't match the oracle yet. Strict, so a fixed row fails until removed here.
SERVICE_ACCOUNT_KNOWN_GAPS: dict[str, ServiceAccountGap] = {
"OrganizationProjects_post": ServiceAccountGap.REQUIRES_USER,
"DiscoverSavedQueries": ServiceAccountGap.USER_ID_FIX,
"DiscoverSavedQueries_post": ServiceAccountGap.USER_ID_FIX,
"GroupExternalIssueDetails_delete": ServiceAccountGap.USER_ID_FIX,
Expand Down
Loading