diff --git a/src/sentry/api/base.py b/src/sentry/api/base.py index 442888a0baa6..9bcec9deecdd 100644 --- a/src/sentry/api/base.py +++ b/src/sentry/api/base.py @@ -29,10 +29,12 @@ audit_logger = logging.getLogger("sentry.audit.api") api_access_logger = logging.getLogger("sentry.access.api") -from sentry import analytics, tsdb +from sentry import analytics, features, tsdb from sentry.analytics.events.release_set_commits import ReleaseSetCommitsLocalEvent from sentry.api.api_owners import ApiOwner from sentry.api.api_publish_status import ApiPublishStatus +from sentry.api.client_kind import FEATURE_FLAG as CLIENT_KIND_FEATURE_FLAG +from sentry.api.client_kind import set_client_kind_attributes from sentry.api.exceptions import ( INSUFFICIENT_SCOPE_ATTR, InsufficientScope, @@ -45,7 +47,9 @@ from sentry.auth.staff import has_staff_option from sentry.hybridcloud.apigateway.cell_request_resolvers import CellRequestResolver from sentry.middleware import is_frontend_request +from sentry.models.organization import Organization from sentry.organizations.absolute_url import generate_organization_url +from sentry.organizations.services.organization import RpcOrganization from sentry.ratelimits.config import DEFAULT_RATE_LIMIT_CONFIG, RateLimitConfig from sentry.seer import agent_token from sentry.silo.base import SiloLimit, SiloMode @@ -493,6 +497,17 @@ def dispatch(self, request: Request, *args, **kwargs) -> Response: (args, kwargs) = self.convert_args(request, *args, **kwargs) self.args = args self.kwargs = kwargs + + # Resolved solely to check the opt-in; everything else is + # derived from the request. Both sources are conventions rather + # than contracts, so the result is type-checked before use. + organization = kwargs.get("organization") or getattr( + request, "organization", None + ) + if isinstance(organization, (Organization, RpcOrganization)) and features.has( + CLIENT_KIND_FEATURE_FLAG, organization, actor=request.user + ): + set_client_kind_attributes(request) else: handler = self.http_method_not_allowed diff --git a/src/sentry/api/bases/organization_events.py b/src/sentry/api/bases/organization_events.py index d74c50e6db5d..23076c817c16 100644 --- a/src/sentry/api/bases/organization_events.py +++ b/src/sentry/api/bases/organization_events.py @@ -21,7 +21,6 @@ from sentry.api.base import CURSOR_LINK_HEADER from sentry.api.bases import NoProjects from sentry.api.bases.organization import FilterParamsDateNotNull, OrganizationEndpoint -from sentry.api.client_kind import set_client_kind_attributes from sentry.api.helpers.error_upsampling import ( are_any_projects_error_upsampled, convert_fields_for_upsampling, @@ -113,19 +112,6 @@ def resolve_axis_column( class OrganizationEventsEndpointBase(OrganizationEndpoint): owner = ApiOwner.DATA_BROWSING - def convert_args( - self, - request: Request, - *args: Any, - **kwargs: Any, - ) -> tuple[tuple[Any, ...], dict[str, Any]]: - (args, kwargs) = super().convert_args(request, *args, **kwargs) - # Runs after authentication, so the credential-based checks in - # `get_client_kind` see the resolved auth. Done here rather than in each - # handler so every events endpoint reports the caller the same way. - set_client_kind_attributes(request, kwargs["organization"]) - return (args, kwargs) - def has_feature(self, organization: Organization, request: Request) -> bool: return ( features.has("organizations:discover-basic", organization, actor=request.user) diff --git a/src/sentry/api/client_kind.py b/src/sentry/api/client_kind.py index d4b466766a5d..4e9d63e95656 100644 --- a/src/sentry/api/client_kind.py +++ b/src/sentry/api/client_kind.py @@ -20,11 +20,9 @@ from rest_framework.request import Request from sentry_conventions.attributes import ATTRIBUTE_NAMES -from sentry import features from sentry.auth.services.auth import AuthenticatedToken from sentry.auth.system import is_system_auth from sentry.middleware import is_frontend_request -from sentry.models.organization import Organization from sentry.seer.agent_token import is_agent_auth from sentry.utils.http import SEER_REFERRER_HEADER, get_mcp_client_family, is_mcp_request from sentry.utils.sdk import get_transaction_name_from_request @@ -101,16 +99,16 @@ def client_kind_scope(kind: ClientKind) -> Generator[None]: ) -def get_client_kind(request: Request, organization: Organization) -> ClientKind | None: +def get_client_kind(request: Request) -> ClientKind: """Classify the caller of an API request. - Returns ``None`` when the org has not opted in, so that a disabled org is - distinguishable from one whose traffic genuinely classifies as ``UNKNOWN``. - Otherwise never raises; unrecognized callers fall back to ``UNKNOWN``. - """ - if not features.has(FEATURE_FLAG, organization, actor=request.user): - return None + Never raises; unrecognized callers fall back to ``UNKNOWN``. + Says nothing about whether the caller's organization opted in -- ``FEATURE_FLAG`` + is checked by the caller, which is what holds the organization. Callers must + check it before reaching here, or a ``client_kind_scope`` declaration becomes a + way around the opt-in. + """ declared = _client_kind_override.get() if declared is not None: return declared @@ -184,16 +182,14 @@ def get_client_kind(request: Request, organization: Organization) -> ClientKind return ClientKind.UNKNOWN -def set_client_kind_attributes(request: Request, organization: Organization) -> None: +def set_client_kind_attributes(request: Request) -> None: """Record who called the endpoint, on a span and on the enclosing transaction. - A no-op when the org has not opted into ``client_kind``. Wired into - ``OrganizationEventsEndpointBase.convert_args`` so every events endpoint - reports the same set of attributes without hand-wiring them per handler. + Called once from ``Endpoint.dispatch``, behind the opt-in check it makes for + whichever organization ``Endpoint.client_kind_organization`` resolves, so every + endpoint reports the same set of attributes without hand-wiring them per handler. """ - client_kind = get_client_kind(request, organization) - if client_kind is None: - return + client_kind = get_client_kind(request) client_host = get_client_host(request) user_agent = get_user_agent(request) diff --git a/tests/sentry/api/test_client_kind.py b/tests/sentry/api/test_client_kind.py index 4e58e1ef8c8d..de84417a5229 100644 --- a/tests/sentry/api/test_client_kind.py +++ b/tests/sentry/api/test_client_kind.py @@ -23,7 +23,7 @@ from sentry.auth.system import SystemToken from sentry.seer.agent_token import AGENT_TOKEN_KIND from sentry.seer.endpoints.seer_rpc import SeerRpcSignatureAuthentication -from sentry.testutils.cases import TestCase +from sentry.testutils.cases import APITestCase, TestCase from sentry.utils.sdk import get_transaction_name_from_request EVENTS_PATH = "/api/0/organizations/my-org/events/" @@ -80,14 +80,8 @@ def api_token(*, application_id: int | None = None) -> AuthenticatedToken: class GetClientKindTest(TestCase): - def classify(self, request: Request) -> ClientKind | None: - with self.feature(FEATURE_FLAG): - return get_client_kind(request, self.organization) - - def test_returns_none_when_feature_is_disabled(self) -> None: - # A disabled org has to stay distinguishable from one that classifies as UNKNOWN. - with self.feature({FEATURE_FLAG: False}): - assert get_client_kind(make_request(), self.organization) is None + def classify(self, request: Request) -> ClientKind: + return get_client_kind(request) def test_session_auth_is_frontend(self) -> None: assert self.classify(make_request(user=session_user())) == ClientKind.FRONTEND @@ -259,25 +253,12 @@ def test_absent_header_is_none(self) -> None: class SetClientKindAttributesTest(TestCase): - def test_noop_when_feature_is_disabled(self) -> None: - request = make_request(auth=api_token(), user_agent="curl/8.7.1") - with ( - self.feature({FEATURE_FLAG: False}), - mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, - mock.patch("sentry.api.client_kind.start_span") as start_span, - ): - set_client_kind_attributes(request, self.organization) - sdk.set_tag.assert_not_called() - sdk.set_attribute.assert_not_called() - start_span.assert_not_called() - def test_records_kind_and_user_agent(self) -> None: request = make_request(auth=api_token(), user_agent="curl/8.7.1") with ( - self.feature(FEATURE_FLAG), mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, ): - set_client_kind_attributes(request, self.organization) + set_client_kind_attributes(request) assert sdk.set_tag.call_args_list == [mock.call("client_kind_test", "script")] assert sdk.set_attribute.call_args_list == [ mock.call("client_kind_test", "script"), @@ -294,20 +275,18 @@ def test_records_client_host_for_mcp(self) -> None: }, ) with ( - self.feature(FEATURE_FLAG), mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, ): - set_client_kind_attributes(request, self.organization) + set_client_kind_attributes(request) assert mock.call("client_host_test", "claude-code") in sdk.set_tag.call_args_list assert mock.call("client_host_test", "claude-code") in sdk.set_attribute.call_args_list def test_omits_user_agent_when_absent(self) -> None: request = make_request(auth=api_token()) with ( - self.feature(FEATURE_FLAG), mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, ): - set_client_kind_attributes(request, self.organization) + set_client_kind_attributes(request) for call in sdk.set_attribute.call_args_list: assert call.args[0] != ATTRIBUTE_NAMES.USER_AGENT_ORIGINAL @@ -318,22 +297,20 @@ def test_records_for_the_internal_api_client_too(self) -> None: request = make_request(auth=api_token(), user_agent="curl/8.7.1") mark_from_api_client(request) with ( - self.feature(FEATURE_FLAG), mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, mock.patch("sentry.api.client_kind.start_span"), ): - set_client_kind_attributes(request, self.organization) + set_client_kind_attributes(request) assert sdk.set_tag.call_args_list == [mock.call("client_kind_test", "script")] class AttributionSpanTest(TestCase): def record(self, request: Request) -> tuple[Any, list[tuple[str, Any]]]: with ( - self.feature(FEATURE_FLAG), mock.patch("sentry.api.client_kind.start_span") as start_span, mock.patch("sentry.api.client_kind.set_span_data") as set_span_data, ): - set_client_kind_attributes(request, self.organization) + set_client_kind_attributes(request) span = start_span.return_value.__enter__.return_value return start_span, [ call.args[1:] for call in set_span_data.call_args_list if call.args[0] is span @@ -396,9 +373,8 @@ def test_an_unmatched_path_collapses_onto_a_catch_all(self) -> None: class ClientKindScopeTest(TestCase): - def classify(self, request: Request) -> ClientKind | None: - with self.feature(FEATURE_FLAG): - return get_client_kind(request, self.organization) + def classify(self, request: Request) -> ClientKind: + return get_client_kind(request) def test_declared_kind_wins_over_a_signal_less_request(self) -> None: request = make_request(cookies=False) @@ -432,18 +408,68 @@ def test_nested_scopes_restore_the_outer_kind(self) -> None: assert self.classify(request) == ClientKind.MCP assert self.classify(request) == ClientKind.SEER - def test_org_opt_in_still_governs(self) -> None: - with client_kind_scope(ClientKind.SEER): - with self.feature({FEATURE_FLAG: False}): - assert get_client_kind(make_request(), self.organization) is None - def test_declared_kind_is_recorded(self) -> None: request = make_request(cookies=False) with ( - self.feature(FEATURE_FLAG), client_kind_scope(ClientKind.SEER), mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, ): - set_client_kind_attributes(request, self.organization) + set_client_kind_attributes(request) assert sdk.set_tag.call_args_list == [mock.call("client_kind_test", "seer")] assert mock.call("client_kind_test", "seer") in sdk.set_attribute.call_args_list + + +class DispatchWiringTest(APITestCase): + """Coverage reaches endpoints beyond the events base this started on. + + Driven through real requests rather than a resolver seam: `Endpoint.dispatch` + reads the organization straight off `kwargs`/`request`, so the only honest way + to pin which endpoint families are covered is to call them. + """ + + def setUp(self) -> None: + super().setUp() + self.login_as(self.user) + + def tags_for(self, url: str, *, enabled: bool = True) -> list[Any]: + with ( + self.feature(FEATURE_FLAG if enabled else {FEATURE_FLAG: False}), + mock.patch("sentry.api.client_kind.sentry_sdk") as sdk, + ): + assert self.client.get(url).status_code == 200 + return sdk.set_tag.call_args_list + + def test_an_organization_endpoint_records_the_caller(self) -> None: + url = f"/api/0/organizations/{self.organization.slug}/" + assert mock.call("client_kind_test", "frontend") in self.tags_for(url) + + def test_a_project_endpoint_records_the_caller(self) -> None: + url = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/" + assert mock.call("client_kind_test", "frontend") in self.tags_for(url) + + def test_a_team_endpoint_records_the_caller(self) -> None: + url = f"/api/0/teams/{self.organization.slug}/{self.team.slug}/" + assert mock.call("client_kind_test", "frontend") in self.tags_for(url) + + def test_an_issue_endpoint_records_the_caller(self) -> None: + # Team and issue endpoints resolve their organization off the related object + # rather than into an `organization` kwarg, so they are the families most + # likely to silently fall out of coverage. + url = f"/api/0/organizations/{self.organization.slug}/issues/{self.group.id}/" + assert mock.call("client_kind_test", "frontend") in self.tags_for(url) + + def test_records_nothing_when_the_organization_has_not_opted_in(self) -> None: + url = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/" + assert self.tags_for(url, enabled=False) == [] + + def test_a_declared_kind_does_not_bypass_the_opt_in(self) -> None: + """A declared caller must not also grant the organization's opt-in. + + The opt-in check moved out of `get_client_kind` and up to the dispatch call + site, so it is the ordering there -- not the function -- that now keeps a + `client_kind_scope` declaration from reporting for an org that never enabled + the feature. + """ + url = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/" + with client_kind_scope(ClientKind.SEER): + assert self.tags_for(url, enabled=False) == [] diff --git a/tests/sentry/seer/endpoints/test_seer_rpc.py b/tests/sentry/seer/endpoints/test_seer_rpc.py index ade3b9f3f97a..1339b3c6bb18 100644 --- a/tests/sentry/seer/endpoints/test_seer_rpc.py +++ b/tests/sentry/seer/endpoints/test_seer_rpc.py @@ -116,13 +116,13 @@ def test_get_organization_projects_registered_on_internal_rpc(self) -> None: def test_dispatch_declares_seer_as_the_client_kind(self) -> None: org = self.create_organization() - captured: list[ClientKind | None] = [] + captured: list[ClientKind] = [] def fake_method(**kwargs: Any) -> dict[str, Any]: nested = Request(APIRequestFactory().get("/")) nested.user = AnonymousUser() nested.auth = None - captured.append(get_client_kind(nested, org)) + captured.append(get_client_kind(nested)) return {"features": []} path = self._get_path("get_organization_features") @@ -151,7 +151,7 @@ def test_client_kind_scope_does_not_outlive_the_dispatch(self) -> None: nested = Request(APIRequestFactory().get("/")) nested.user = AnonymousUser() nested.auth = None - assert get_client_kind(nested, org) == ClientKind.UNKNOWN + assert get_client_kind(nested) == ClientKind.UNKNOWN def test_snuba_rate_limit_returns_429(self) -> None: """Test that SnubaRPCRateLimitExceeded returns 429 to Seer for retry."""