diff --git a/src/sentry/analytics/events/sentry_app_deleted.py b/src/sentry/analytics/events/sentry_app_deleted.py index e9231189befe..79ea44e1d3d2 100644 --- a/src/sentry/analytics/events/sentry_app_deleted.py +++ b/src/sentry/analytics/events/sentry_app_deleted.py @@ -3,7 +3,7 @@ @analytics.eventclass("sentry_app.deleted") class SentryAppDeletedEvent(analytics.Event): - user_id: int + user_id: int | None organization_id: int sentry_app: str diff --git a/src/sentry/analytics/events/sentry_app_updated.py b/src/sentry/analytics/events/sentry_app_updated.py index 88a5d4cd6d2c..c518e5386ee7 100644 --- a/src/sentry/analytics/events/sentry_app_updated.py +++ b/src/sentry/analytics/events/sentry_app_updated.py @@ -3,7 +3,7 @@ @analytics.eventclass("sentry_app.updated") class SentryAppUpdatedEvent(analytics.Event): - user_id: int + user_id: int | None organization_id: int sentry_app: str created_alert_rule_ui_component: bool | None = None diff --git a/src/sentry/api/endpoints/auth_organization_config.py b/src/sentry/api/endpoints/auth_organization_config.py index dc7ccea437a9..7cd088fa21a5 100644 --- a/src/sentry/api/endpoints/auth_organization_config.py +++ b/src/sentry/api/endpoints/auth_organization_config.py @@ -17,7 +17,7 @@ ) from sentry.auth import access from sentry.auth.exceptions import ProviderNotRegistered -from sentry.auth.principal import acting_user_id, require_user +from sentry.auth.principal import acting_user_id, has_user, require_user from sentry.constants import WARN_SESSION_EXPIRED from sentry.demo_mode.utils import is_demo_mode_enabled, is_demo_org from sentry.models.authprovider import AuthProvider @@ -98,7 +98,7 @@ def get(self, request: Request, organization_id_or_slug: str) -> Response: warnings = [] if ( - request.user.is_authenticated + has_user(request.user) and organization_context.member is None and not organization_access.has_global_access and not is_demo_organization diff --git a/src/sentry/api/endpoints/index.py b/src/sentry/api/endpoints/index.py index c340861fa8ab..8edbc9436e96 100644 --- a/src/sentry/api/endpoints/index.py +++ b/src/sentry/api/endpoints/index.py @@ -4,7 +4,7 @@ from sentry.api.api_publish_status import ApiPublishStatus from sentry.api.base import Endpoint, control_silo_endpoint from sentry.api.serializers import serialize -from sentry.auth.principal import require_user +from sentry.auth.principal import has_user from sentry.users.models.user import User @@ -16,8 +16,8 @@ class IndexEndpoint(Endpoint): permission_classes = () def get(self, request: Request) -> Response: - if request.user.is_authenticated: - user = User.objects.get(id=require_user(request).id) + if has_user(request.user): + user = User.objects.get(id=request.user.id) user = serialize(user, user) else: user = None diff --git a/src/sentry/api/endpoints/organization_api_key_details.py b/src/sentry/api/endpoints/organization_api_key_details.py index a89d0eebb1e0..dc1617f418f6 100644 --- a/src/sentry/api/endpoints/organization_api_key_details.py +++ b/src/sentry/api/endpoints/organization_api_key_details.py @@ -43,6 +43,8 @@ class OrganizationApiKeyDetailsEndpoint(ControlSiloOrganizationEndpoint): "PUT": ApiPublishStatus.PRIVATE, } permission_classes = (OrganizationAdminPermission,) + # Keys show their secret and take any scope, so a service account could exceed its own. + requires_user = True def convert_args( self, request: Request, api_key_id: str, *args: Any, **kwargs: Any diff --git a/src/sentry/api/endpoints/organization_api_key_index.py b/src/sentry/api/endpoints/organization_api_key_index.py index c6fc4d45cbd6..92693832750b 100644 --- a/src/sentry/api/endpoints/organization_api_key_index.py +++ b/src/sentry/api/endpoints/organization_api_key_index.py @@ -24,6 +24,8 @@ class OrganizationApiKeyIndexEndpoint(ControlSiloOrganizationEndpoint): "POST": ApiPublishStatus.PRIVATE, } permission_classes = (OrganizationAdminPermission,) + # Keys show their secret and take any scope, so a service account could exceed its own. + requires_user = True def get(self, request: Request, organization_context, organization) -> Response: """ diff --git a/src/sentry/auth/access.py b/src/sentry/auth/access.py index 23a7aaceb474..dfbdeda45980 100644 --- a/src/sentry/auth/access.py +++ b/src/sentry/auth/access.py @@ -50,7 +50,11 @@ from sentry.seer.agent_token import is_agent_auth from sentry.sentry_apps.models.sentry_app import SentryApp from sentry.service_accounts.auth import is_service_account_auth -from sentry.service_accounts.models import ServiceAccountProjectReplica, ServiceAccountReplica +from sentry.service_accounts.models import ( + ServiceAccount, + ServiceAccountProjectReplica, + ServiceAccountReplica, +) from sentry.silo.base import SiloMode from sentry.types.request import _HttpRequestWithUser from sentry.users.models.user import User @@ -1528,15 +1532,25 @@ def from_service_account_auth(auth: AuthenticatedToken, organization_id: int) -> # Bound to the account's own org, like agent tokens. if auth.entity_id is None or auth.organization_id != organization_id: return DEFAULT - # The token doesn't carry `all_projects`, so read it from the replica, like the - # project links. An account deactivated since authentication gets no access. - all_projects = ( - ServiceAccountReplica.objects.filter( - service_account_id=auth.entity_id, organization_id=organization_id, is_active=True + # The token doesn't carry `all_projects`, so read it from the account: the source + # on control, the replica elsewhere. An account deactivated since authentication + # gets no access. + if SiloMode.get_current_mode() == SiloMode.CONTROL: + all_projects = ( + ServiceAccount.objects.filter( + id=auth.entity_id, organization_id=organization_id, is_active=True + ) + .values_list("all_projects", flat=True) + .first() + ) + else: + all_projects = ( + ServiceAccountReplica.objects.filter( + service_account_id=auth.entity_id, organization_id=organization_id, is_active=True + ) + .values_list("all_projects", flat=True) + .first() ) - .values_list("all_projects", flat=True) - .first() - ) if all_projects is None: return DEFAULT return ServiceAccountAccess( diff --git a/src/sentry/sentry_apps/api/bases/sentryapps.py b/src/sentry/sentry_apps/api/bases/sentryapps.py index b92a2e138b70..d0f8ff1c8027 100644 --- a/src/sentry/sentry_apps/api/bases/sentryapps.py +++ b/src/sentry/sentry_apps/api/bases/sentryapps.py @@ -33,6 +33,7 @@ SentryAppPublicErrorBody, SentryAppSentryError, ) +from sentry.service_accounts.auth import is_service_account_auth from sentry.users.services.user.service import user_service from sentry.utils.strings import to_single_line_str @@ -263,13 +264,16 @@ def has_object_permission(self, request: Request, view, sentry_app: RpcSentryApp if superuser_has_permission(request): return True + user_id = acting_user_id(request.user) organizations = ( - user_service.get_organizations(user_id=request.user.id) - if request.user.id is not None - else () + user_service.get_organizations(user_id=user_id) if user_id is not None else () + ) + is_owner_service_account = ( + is_service_account_auth(request.auth) + and request.auth.organization_id == sentry_app.owner_id ) # if app is unpublished, user must be in the Org who owns the app. - if not sentry_app.is_published: + if not sentry_app.is_published and not is_owner_service_account: if not any(sentry_app.owner_id == org.id for org in organizations): raise SentryAppError( message="User must be in the app owner's organization for unpublished apps", @@ -424,7 +428,15 @@ def has_object_permission(self, request: Request, view, installation): if request.user.is_sentry_app: return request.user.id == installation.sentry_app.proxy_user_id - if not org_context.member or org_context.organization.status != OrganizationStatus.ACTIVE: + # A service account has no membership; it belongs to its own organization. + is_org_service_account = ( + is_service_account_auth(request.auth) + and request.auth.organization_id == installation.organization_id + ) + if ( + not (org_context.member or is_org_service_account) + or org_context.organization.status != OrganizationStatus.ACTIVE + ): raise SentryAppError(message="Given organization is not valid", status_code=404) assert request.method, "method must be present in request to get permissions" diff --git a/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py b/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py index 02df10575949..900abccf8300 100644 --- a/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py +++ b/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py @@ -62,6 +62,8 @@ class SentryAppInstallationExternalIssueActionsEndpoint( SentryAppInstallationExternalIssueBaseEndpoint ): owner = ApiOwner.INTEGRATION_PLATFORM + # The app's request names the acting user. + requires_user = True publish_status = { "POST": ApiPublishStatus.PUBLIC_EXPERIMENTAL, } diff --git a/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py b/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py index e32a751d1038..c0f2adba799b 100644 --- a/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py +++ b/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py @@ -57,6 +57,8 @@ class SentryAppExternalRequestOptionsResponse(TypedDict): @control_silo_endpoint class SentryAppInstallationExternalRequestsEndpoint(SentryAppInstallationBaseEndpoint): owner = ApiOwner.INTEGRATION_PLATFORM + # The app's request names the acting user. + requires_user = True publish_status = { "GET": ApiPublishStatus.PUBLIC_EXPERIMENTAL, } diff --git a/src/sentry/sentry_apps/api/endpoints/sentry_app_details.py b/src/sentry/sentry_apps/api/endpoints/sentry_app_details.py index acbd8d197cfb..bddc9260087b 100644 --- a/src/sentry/sentry_apps/api/endpoints/sentry_app_details.py +++ b/src/sentry/sentry_apps/api/endpoints/sentry_app_details.py @@ -27,7 +27,7 @@ as_validation_errors, ) from sentry.apidocs.utils import inline_sentry_response_serializer -from sentry.auth.principal import acting_user_id +from sentry.auth.principal import acting_user_id, require_user from sentry.auth.staff import is_active_staff from sentry.constants import SentryAppStatus from sentry.deletions.models.scheduleddeletion import ScheduledDeletion @@ -46,8 +46,6 @@ from sentry.sentry_apps.models.sentry_app import SentryApp from sentry.sentry_apps.models.sentry_app_installation import SentryAppInstallation from sentry.sentry_apps.utils.webhooks import has_error_events -from sentry.users.models.user import User -from sentry.users.services.user.model import RpcUser from sentry.utils.audit import create_audit_entry logger = logging.getLogger(__name__) @@ -173,7 +171,7 @@ def put( if serializer.is_valid(): result = serializer.validated_data - assert isinstance(request.user, (User, RpcUser)), ( + assert request.user.is_authenticated, ( "User must be authenticated to update a Sentry App" ) updated_app = SentryAppUpdater( @@ -255,11 +253,11 @@ def delete( for install in sentry_app.installations.all(): try: with transaction.atomic(using=router.db_for_write(SentryAppInstallation)): - assert request.user.is_authenticated, ( - "User must be authenticated to delete installation" - ) + # The uninstall webhook names the acting user. SentryAppInstallationNotifier( - sentry_app_installation=install, user=request.user, action="deleted" + sentry_app_installation=install, + user=require_user(request), + action="deleted", ).run() except RequestException as exc: sentry_sdk.capture_exception(exc) @@ -278,7 +276,7 @@ def delete( if request.user.is_authenticated: analytics.record( SentryAppDeletedEvent( - user_id=request.user.id, + user_id=acting_user_id(request.user), organization_id=sentry_app.owner_id, sentry_app=sentry_app.slug, ) diff --git a/src/sentry/sentry_apps/api/serializers/sentry_app.py b/src/sentry/sentry_apps/api/serializers/sentry_app.py index 29d56ba3c28b..5eea537d1c02 100644 --- a/src/sentry/sentry_apps/api/serializers/sentry_app.py +++ b/src/sentry/sentry_apps/api/serializers/sentry_app.py @@ -7,6 +7,7 @@ from sentry.api.serializers import Serializer, register, serialize from sentry.app import env +from sentry.auth.principal import acting_user_id from sentry.auth.staff import is_active_staff from sentry.auth.superuser import is_active_superuser from sentry.constants import SentryAppStatus @@ -103,7 +104,8 @@ def get_attrs( for app in ApiApplication.objects.filter(id__in=[i.application_id for i in item_list]) } - user_orgs = user_service.get_organizations(user_id=user.id) + user_id = acting_user_id(user) + user_orgs = user_service.get_organizations(user_id=user_id) if user_id is not None else [] user_org_ids = {uo.id for uo in user_orgs} # Fetch organization contexts for unique owner_ids where user is a member. @@ -114,7 +116,7 @@ def get_attrs( unique_owner_ids = {i.owner_id for i in item_list if i.owner_id in user_org_ids} for owner_id in unique_owner_ids: owner_context = organization_service.get_organization_by_id( - id=owner_id, user_id=user.id + id=owner_id, user_id=user_id ) if owner_context: owner_contexts[owner_id] = owner_context diff --git a/src/sentry/sentry_apps/logic.py b/src/sentry/sentry_apps/logic.py index 3e4767343d1b..e562276e2e58 100644 --- a/src/sentry/sentry_apps/logic.py +++ b/src/sentry/sentry_apps/logic.py @@ -19,6 +19,7 @@ from sentry.analytics.events.sentry_app_created import SentryAppCreatedEvent from sentry.analytics.events.sentry_app_updated import SentryAppUpdatedEvent from sentry.api.helpers.slugs import sentry_slugify +from sentry.auth.principal import acting_user_id from sentry.auth.staff import has_staff_option from sentry.constants import SentryAppStatus from sentry.db.postgres.transactions import in_test_hide_transaction_boundary @@ -357,7 +358,7 @@ def record_analytics(self, user: User | RpcUser, new_schema_elements: set[str] | created_alert_rule_ui_component = "alert-rule-action" in (new_schema_elements or set()) analytics.record( SentryAppUpdatedEvent( - user_id=user.id, + user_id=acting_user_id(user), organization_id=self.sentry_app.owner_id, sentry_app=self.sentry_app.slug, created_alert_rule_ui_component=created_alert_rule_ui_component, diff --git a/src/sentry/service_accounts/auth.py b/src/sentry/service_accounts/auth.py index 0ecb818222e4..1130560f9858 100644 --- a/src/sentry/service_accounts/auth.py +++ b/src/sentry/service_accounts/auth.py @@ -5,7 +5,8 @@ from sentry import features from sentry.auth.services.auth import AuthenticatedToken from sentry.models.organization import Organization -from sentry.service_accounts.models import ServiceAccountReplica +from sentry.organizations.services.organization import RpcOrganization, organization_service +from sentry.service_accounts.models import ServiceAccount, ServiceAccountReplica from sentry.service_accounts.principal import ServiceAccountPrincipal from sentry.silo.base import SiloMode from sentry.viewer_context import ActorType, ViewerContext @@ -25,8 +26,9 @@ def authenticate_service_account( organization. Only for organizations with ``organizations:service-accounts``. The token carries no - scopes, so they're read from the cell replica on every request. Narrowing, disabling or - deleting the account applies once its outbox replicates, not when the token expires. + scopes, so they're read on every request: from the account itself on control, and from + the cell replica elsewhere. On a cell, narrowing, disabling or deleting the account + applies once its outbox replicates, not when the token expires. """ actor = vc.actor if ( @@ -35,31 +37,40 @@ def authenticate_service_account( or vc.organization_id is None ): return None - # Only cell endpoints accept these contexts, since the replica is a cell table. - # Control endpoints would need to read the control-side ServiceAccount instead. - if SiloMode.get_current_mode() == SiloMode.CONTROL: - return None - service_account = ServiceAccountReplica.objects.filter( - service_account_id=actor.actor_id, organization_id=vc.organization_id, is_active=True - ).first() - if service_account is None: - return None - try: - organization = Organization.objects.get_from_cache(id=service_account.organization_id) - except Organization.DoesNotExist: - return None - if not features.has("organizations:service-accounts", organization): + service_account: ServiceAccount | ServiceAccountReplica | None + organization: Organization | RpcOrganization | None + if SiloMode.get_current_mode() == SiloMode.CONTROL: + service_account = ServiceAccount.objects.filter( + id=actor.actor_id, organization_id=vc.organization_id, is_active=True + ).first() + if service_account is None: + return None + org_context = organization_service.get_organization_by_id( + id=service_account.organization_id, include_projects=False, include_teams=False + ) + organization = org_context.organization if org_context is not None else None + else: + service_account = ServiceAccountReplica.objects.filter( + service_account_id=actor.actor_id, organization_id=vc.organization_id, is_active=True + ).first() + if service_account is None: + return None + try: + organization = Organization.objects.get_from_cache(id=service_account.organization_id) + except Organization.DoesNotExist: + organization = None + if organization is None or not features.has("organizations:service-accounts", organization): return None principal = ServiceAccountPrincipal( - service_account_id=service_account.service_account_id, + service_account_id=actor.actor_id, organization_id=service_account.organization_id, name=service_account.name, ) token = AuthenticatedToken( kind=SERVICE_ACCOUNT_TOKEN_KIND, - entity_id=service_account.service_account_id, + entity_id=actor.actor_id, organization_id=service_account.organization_id, scopes=sorted(service_account.scope_list), ) diff --git a/tests/sentry/api/endpoints/test_auth_organization_config.py b/tests/sentry/api/endpoints/test_auth_organization_config.py index 8b69a1d64b01..3bebc99d2df0 100644 --- a/tests/sentry/api/endpoints/test_auth_organization_config.py +++ b/tests/sentry/api/endpoints/test_auth_organization_config.py @@ -1,11 +1,15 @@ from unittest import mock +from django.test import override_settings +from django.urls import reverse + from sentry.auth.exceptions import ProviderNotRegistered from sentry.organizations.services.organization import organization_service from sentry.silo.base import SiloMode from sentry.testutils.cases import APITestCase -from sentry.testutils.helpers import override_options +from sentry.testutils.helpers import override_options, with_feature from sentry.testutils.silo import assume_test_silo_mode, control_silo_test +from sentry.viewer_context import Actor, ActorType, ViewerContext, encode_viewer_context @control_silo_test @@ -138,6 +142,30 @@ def test_authenticated_non_member_warning(self) -> None: "Ask an organization admin to invite you, or sign in with a different account." ] + @override_settings(SEER_API_SHARED_SECRET="service-account-secret") + @with_feature("organizations:service-accounts") + def test_service_account_has_no_member_warning(self) -> None: + organization = self.create_organization(name="Acme", slug="acme") + service_account = self.create_service_account( + organization=organization, scope_list=["org:read"] + ) + viewer_context = encode_viewer_context( + ViewerContext( + actor=Actor(type=ActorType.SERVICE_ACCOUNT, actor_id=service_account.id), + organization_id=organization.id, + ), + key="service-account-secret", + ) + + response = self.client.get( + reverse(self.endpoint, args=[organization.slug]), + HTTP_X_VIEWER_CONTEXT=viewer_context, + ) + + assert response.status_code == 200, response.content + assert response.data["authenticated"] is True + assert response.data["warnings"] == [] + def test_authenticated_member_can_access_password_organization(self) -> None: user = self.create_user(email="user@example.com") organization = self.create_organization(owner=user, name="Acme", slug="acme") diff --git a/tests/sentry/api/endpoints/test_index.py b/tests/sentry/api/endpoints/test_index.py index 6b356e349b02..95221d0ac80b 100644 --- a/tests/sentry/api/endpoints/test_index.py +++ b/tests/sentry/api/endpoints/test_index.py @@ -1,9 +1,12 @@ +from django.test import override_settings from django.urls import reverse from sentry.models.apikey import ApiKey from sentry.models.apitoken import ApiToken from sentry.testutils.cases import APITestCase +from sentry.testutils.helpers import with_feature from sentry.testutils.silo import control_silo_test +from sentry.viewer_context import Actor, ActorType, ViewerContext, encode_viewer_context @control_silo_test @@ -55,3 +58,23 @@ def test_token_auth_returns_stored_scopes(self) -> None: assert response.status_code == 200 # Deprecated scope translations (org:read -> dashboard:*) are not reported. assert response.data["auth"]["scopes"] == ["org:read"] + + @override_settings(SEER_API_SHARED_SECRET="service-account-secret") + @with_feature("organizations:service-accounts") + def test_service_account_has_no_user(self) -> None: + service_account = self.create_service_account( + organization=self.organization, scope_list=["org:read"] + ) + viewer_context = encode_viewer_context( + ViewerContext( + actor=Actor(type=ActorType.SERVICE_ACCOUNT, actor_id=service_account.id), + organization_id=self.organization.id, + ), + key="service-account-secret", + ) + response = self.client.get( + reverse("sentry-api-index"), HTTP_X_VIEWER_CONTEXT=viewer_context + ) + assert response.status_code == 200, response.content + assert response.data["user"] is None + assert response.data["auth"]["scopes"] == ["org:read"] diff --git a/tests/sentry/api/endpoints/test_organization_api_key_details.py b/tests/sentry/api/endpoints/test_organization_api_key_details.py index 06831d8e24bd..a6ff307d7ca2 100644 --- a/tests/sentry/api/endpoints/test_organization_api_key_details.py +++ b/tests/sentry/api/endpoints/test_organization_api_key_details.py @@ -1,10 +1,14 @@ from django.db import connections +from django.test import override_settings +from django.urls import reverse from sentry.hybridcloud.models import ApiKeyReplica from sentry.models.apikey import ApiKey from sentry.silo.base import SiloMode from sentry.testutils.cases import APITestCase +from sentry.testutils.helpers.features import with_feature from sentry.testutils.silo import assume_test_silo_mode, control_silo_test +from sentry.viewer_context import Actor, ActorType, ViewerContext, encode_viewer_context DEFAULT_SCOPES = ["project:read", "event:read", "team:read", "org:read", "member:read"] @@ -48,6 +52,31 @@ def test_update_api_key_details(self) -> None: assert api_key.allowed_origins == "sentry.io" assert api_key.get_scopes() == ["org:read", "project:read", "project:write", "team:read"] + @override_settings(SEER_API_SHARED_SECRET="service-account-secret") + @with_feature("organizations:service-accounts") + def test_service_account_cannot_update_key(self) -> None: + service_account = self.create_service_account( + organization=self.organization, scope_list=["org:admin"] + ) + viewer_context = encode_viewer_context( + ViewerContext( + actor=Actor(type=ActorType.SERVICE_ACCOUNT, actor_id=service_account.id), + organization_id=self.organization.id, + ), + key="service-account-secret", + ) + + response = self.client.put( + reverse(self.endpoint, args=[self.organization.slug, self.api_key.id]), + data={"scope_list": ["org:admin", "service_account:write"]}, + format="json", + HTTP_X_VIEWER_CONTEXT=viewer_context, + ) + + assert response.status_code == 403, response.content + assert response.data["detail"]["code"] == "user-required" + assert ApiKey.objects.get(id=self.api_key.id).get_scopes() == sorted(DEFAULT_SCOPES) + def test_update_api_key_rejects_invalid_scopes(self) -> None: data = {"scope_list": ["a", "b", "c", "d"]} response = self.get_error_response( diff --git a/tests/sentry/api/endpoints/test_organization_api_key_index.py b/tests/sentry/api/endpoints/test_organization_api_key_index.py index 02307755d08f..3abc1ba852c2 100644 --- a/tests/sentry/api/endpoints/test_organization_api_key_index.py +++ b/tests/sentry/api/endpoints/test_organization_api_key_index.py @@ -1,5 +1,10 @@ +from django.test import override_settings +from django.urls import reverse + from sentry.testutils.cases import APITestCase +from sentry.testutils.helpers.features import with_feature from sentry.testutils.silo import control_silo_test +from sentry.viewer_context import Actor, ActorType, ViewerContext, encode_viewer_context @control_silo_test @@ -13,6 +18,28 @@ def setUp(self) -> None: def test_org_admin_can_access(self) -> None: self.get_success_response(self.organization.slug) + @override_settings(SEER_API_SHARED_SECRET="service-account-secret") + @with_feature("organizations:service-accounts") + def test_service_account_cannot_list_keys(self) -> None: + service_account = self.create_service_account( + organization=self.organization, scope_list=["org:admin"] + ) + viewer_context = encode_viewer_context( + ViewerContext( + actor=Actor(type=ActorType.SERVICE_ACCOUNT, actor_id=service_account.id), + organization_id=self.organization.id, + ), + key="service-account-secret", + ) + + response = self.client.get( + reverse(self.endpoint, args=[self.organization.slug]), + HTTP_X_VIEWER_CONTEXT=viewer_context, + ) + + assert response.status_code == 403, response.content + assert response.data["detail"]["code"] == "user-required" + def test_member_no_access(self) -> None: user = self.create_user("bar@example.com") self.create_member(organization=self.organization, user=user, role="member") diff --git a/tests/sentry/api/test_authentication.py b/tests/sentry/api/test_authentication.py index db609b01226e..3c13094ae342 100644 --- a/tests/sentry/api/test_authentication.py +++ b/tests/sentry/api/test_authentication.py @@ -1285,7 +1285,53 @@ def test_service_account_in_other_org_returns_none(self) -> None: assert ViewerContextAuthentication().authenticate(request) is None @override_settings(SEER_API_SHARED_SECRET=SHARED_SECRET) - def test_service_account_in_control_silo_returns_none(self) -> None: + @with_feature("organizations:service-accounts") + def test_service_account_authenticates_in_control_silo(self) -> None: + service_account = self.create_service_account( + organization=self.organization, scope_list=["org:integrations", "org:read"] + ) + context = ViewerContext( + actor=Actor(type=ActorType.SERVICE_ACCOUNT, actor_id=service_account.id), + organization_id=self.organization.id, + ) + request = self._make_request( + viewer_context=encode_viewer_context(context, key=self.SHARED_SECRET) + ) + + with assume_test_silo_mode(SiloMode.CONTROL, can_be_monolith=False): + result = ViewerContextAuthentication().authenticate(request) + + assert result is not None + user, auth = result + assert user == ServiceAccountPrincipal( + service_account_id=service_account.id, + organization_id=self.organization.id, + name=service_account.name, + ) + assert is_service_account_auth(auth) + assert auth.entity_id == service_account.id + assert auth.organization_id == self.organization.id + assert auth.scopes == ["org:integrations", "org:read"] + + @override_settings(SEER_API_SHARED_SECRET=SHARED_SECRET) + @with_feature("organizations:service-accounts") + def test_disabled_service_account_in_control_silo_returns_none(self) -> None: + service_account = self.create_service_account( + organization=self.organization, is_active=False + ) + context = ViewerContext( + actor=Actor(type=ActorType.SERVICE_ACCOUNT, actor_id=service_account.id), + organization_id=self.organization.id, + ) + request = self._make_request( + viewer_context=encode_viewer_context(context, key=self.SHARED_SECRET) + ) + + with assume_test_silo_mode(SiloMode.CONTROL, can_be_monolith=False): + assert ViewerContextAuthentication().authenticate(request) is None + + @override_settings(SEER_API_SHARED_SECRET=SHARED_SECRET) + def test_service_account_in_control_silo_without_feature_returns_none(self) -> None: service_account = self.create_service_account(organization=self.organization) context = ViewerContext( actor=Actor(type=ActorType.SERVICE_ACCOUNT, actor_id=service_account.id), diff --git a/tests/sentry/auth/test_access.py b/tests/sentry/auth/test_access.py index 27bd056d5e11..c4f4e6d702c9 100644 --- a/tests/sentry/auth/test_access.py +++ b/tests/sentry/auth/test_access.py @@ -1488,6 +1488,22 @@ def test_no_access_to_other_org(self) -> None: assert self._access(organization=other_org) is access.DEFAULT assert not self._access().has_project_access(other_project) + def test_control_silo_reads_the_account(self) -> None: + self._set_all_projects() + + with assume_test_silo_mode(SiloMode.CONTROL, can_be_monolith=False): + result = self._access() + + assert isinstance(result, access.ServiceAccountAccess) + assert result.has_scope("project:read") + assert result.has_project_access(self.unlinked) + + def test_control_silo_inactive_account_has_no_access(self) -> None: + self.service_account.update(is_active=False) + + with assume_test_silo_mode(SiloMode.CONTROL, can_be_monolith=False): + assert self._access() is access.DEFAULT + @no_silo_test class SystemAccessTest(TestCase): diff --git a/tests/sentry/auth/test_principal.py b/tests/sentry/auth/test_principal.py index 65d8803d4d30..85ba386f634e 100644 --- a/tests/sentry/auth/test_principal.py +++ b/tests/sentry/auth/test_principal.py @@ -274,8 +274,8 @@ def _is_request_user_type_check(node: ast.AST) -> bool: # Lower these as callers move off. Exact, so the baseline can't drift back up. -REQUEST_USER_ID_BASELINE = 347 -REQUEST_USER_TYPE_CHECK_BASELINE = 31 +REQUEST_USER_ID_BASELINE = 345 +REQUEST_USER_TYPE_CHECK_BASELINE = 30 def test_request_user_id_uses_only_go_down() -> None: diff --git a/tests/sentry/seer/endpoints/test_organization_agent_token.py b/tests/sentry/seer/endpoints/test_organization_agent_token.py index 41d36611ab86..4cbaa0385db0 100644 --- a/tests/sentry/seer/endpoints/test_organization_agent_token.py +++ b/tests/sentry/seer/endpoints/test_organization_agent_token.py @@ -598,25 +598,12 @@ def _public_mutation_endpoints() -> tuple[PublicMutationEndpoint, ...]: class ServiceAccountGap(StrEnum): - CONTROL_SILO = "service account auth is cell-only" NO_MEMBERSHIP = "checks the caller's own membership or org role, which a service account lacks" NO_TEAM_ACCESS = "service accounts reach no teams in v1" # Rows that don't match the oracle yet. Strict, so a fixed row fails until removed here. SERVICE_ACCOUNT_KNOWN_GAPS: dict[str, ServiceAccountGap] = { - "OrganizationIntegrationChannelValidate": ServiceAccountGap.CONTROL_SILO, - "OrganizationIntegrationChannels": ServiceAccountGap.CONTROL_SILO, - "OrganizationIntegrationDetails": ServiceAccountGap.CONTROL_SILO, - "OrganizationIntegrationDetails_delete": ServiceAccountGap.CONTROL_SILO, - "OrganizationIntegrations": ServiceAccountGap.CONTROL_SILO, - "OrganizationSentryAppComponents": ServiceAccountGap.CONTROL_SILO, - "OrganizationSentryApps": ServiceAccountGap.CONTROL_SILO, - "SentryAppDetails": ServiceAccountGap.CONTROL_SILO, - "SentryAppDetails_delete": ServiceAccountGap.CONTROL_SILO, - "SentryAppDetails_put": ServiceAccountGap.CONTROL_SILO, - "SentryAppInstallationExternalIssueActions_post": ServiceAccountGap.CONTROL_SILO, - "SentryAppInstallationExternalRequests": ServiceAccountGap.CONTROL_SILO, "EventAttachmentDetails": ServiceAccountGap.NO_MEMBERSHIP, "OrganizationMemberDetails_delete": ServiceAccountGap.NO_MEMBERSHIP, "OrganizationMemberDetails_put": ServiceAccountGap.NO_MEMBERSHIP, @@ -634,8 +621,9 @@ class ServiceAccountGap(StrEnum): "TeamProjects_post": ServiceAccountGap.NO_TEAM_ACCESS, } -# Rows that act on the caller as a person (saved views, stars, a personal team), so a -# service account is denied with user-required instead of matching the oracle. +# Rows that act on the caller as a person (saved views, stars, a personal team, a Sentry +# App request that names the acting user), so a service account is denied with +# user-required instead of matching the oracle. SERVICE_ACCOUNT_USER_REQUIRED = frozenset( { "OrganizationGroupSearchViews_post", @@ -643,6 +631,8 @@ class ServiceAccountGap(StrEnum): "OrganizationProjects_post", "OrganizationStarredServiceSpans_delete", "OrganizationStarredServiceSpans_post", + "SentryAppInstallationExternalIssueActions_post", + "SentryAppInstallationExternalRequests", } ) diff --git a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_details.py b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_details.py index 2970a64f5e45..fc2860a7c50e 100644 --- a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_details.py +++ b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_details.py @@ -1,6 +1,8 @@ from unittest.mock import MagicMock, patch import orjson +from django.test import override_settings +from django.urls import reverse from rest_framework.response import Response from sentry import audit_log, deletions @@ -24,6 +26,7 @@ from sentry.testutils.helpers.options import override_options from sentry.testutils.outbox import outbox_runner from sentry.testutils.silo import assume_test_silo_mode, control_silo_test +from sentry.viewer_context import Actor, ActorType, ViewerContext, encode_viewer_context from sentry.workflow_engine.models.action import Action @@ -1139,6 +1142,60 @@ def test_staff_delete_unpublished_app_with_installs(self) -> None: ).exists() assert not SentryAppInstallation.objects.filter(id=installation.id).exists() + def _service_account_viewer_context(self) -> str: + service_account = self.create_service_account( + organization=self.organization, scope_list=["org:admin"] + ) + return encode_viewer_context( + ViewerContext( + actor=Actor(type=ActorType.SERVICE_ACCOUNT, actor_id=service_account.id), + organization_id=self.organization.id, + ), + key="service-account-secret", + ) + + @override_settings(SEER_API_SHARED_SECRET="service-account-secret") + @with_feature("organizations:service-accounts") + @patch("sentry.analytics.record") + def test_service_account_delete_unpublished_app(self, record: MagicMock) -> None: + self.client.logout() + + response = self.client.delete( + reverse(self.endpoint, args=[self.unpublished_app.slug]), + HTTP_X_VIEWER_CONTEXT=self._service_account_viewer_context(), + ) + + assert response.status_code == 204, response.content + assert_last_analytics_event( + record, + SentryAppDeletedEvent( + user_id=None, + organization_id=self.organization.id, + sentry_app=self.unpublished_app.slug, + ), + ) + + @override_settings(SEER_API_SHARED_SECRET="service-account-secret") + @with_feature("organizations:service-accounts") + def test_service_account_cannot_delete_unpublished_app_with_installs(self) -> None: + installation = self.create_sentry_app_installation( + organization=self.organization, + slug=self.unpublished_app.slug, + user=self.user, + ) + self.client.logout() + + response = self.client.delete( + reverse(self.endpoint, args=[self.unpublished_app.slug]), + HTTP_X_VIEWER_CONTEXT=self._service_account_viewer_context(), + ) + + assert response.status_code == 403, response.content + assert response.data["detail"]["code"] == "user-required" + self.unpublished_app.refresh_from_db() + assert self.unpublished_app.status == SentryAppStatus.UNPUBLISHED + assert SentryAppInstallation.objects.filter(id=installation.id).exists() + @override_options({"staff.ga-rollout": True}) def test_staff_cannot_delete_published_app(self) -> None: response = self.get_error_response(self.published_app.slug, status_code=403) diff --git a/tests/sentry/service_accounts/endpoints/test_organization_service_accounts.py b/tests/sentry/service_accounts/endpoints/test_organization_service_accounts.py index 454910cb4d5e..1b29eb6fad79 100644 --- a/tests/sentry/service_accounts/endpoints/test_organization_service_accounts.py +++ b/tests/sentry/service_accounts/endpoints/test_organization_service_accounts.py @@ -1,6 +1,7 @@ from typing import Any from unittest import mock +from django.test import override_settings from django.urls import reverse from rest_framework.request import Request @@ -22,6 +23,7 @@ from sentry.testutils.cases import APITestCase from sentry.testutils.outbox import outbox_runner from sentry.testutils.silo import assume_test_silo_mode, control_silo_test +from sentry.viewer_context import Actor, ActorType, ViewerContext, encode_viewer_context FEATURE_FLAG = "organizations:service-accounts" @@ -396,6 +398,36 @@ def setUp(self) -> None: def test_flag_off_returns_404(self) -> None: self.get_error_response(self.organization.slug, self.service_account.id, status_code=404) + @override_settings(SEER_API_SHARED_SECRET="service-account-secret") + def test_service_account_cannot_update_service_accounts(self) -> None: + caller = self.create_service_account( + organization=self.organization, name="caller", scope_list=["org:admin", "org:read"] + ) + disabled = self.create_service_account( + organization=self.organization, name="disabled", is_active=False + ) + viewer_context = encode_viewer_context( + ViewerContext( + actor=Actor(type=ActorType.SERVICE_ACCOUNT, actor_id=caller.id), + organization_id=self.organization.id, + ), + key="service-account-secret", + ) + + with self.feature(FEATURE_FLAG): + response = self.client.put( + reverse(self.endpoint, args=[self.organization.slug, disabled.id]), + data={"isActive": True, "scopes": ["org:admin", "org:read"]}, + format="json", + HTTP_X_VIEWER_CONTEXT=viewer_context, + ) + + # Writes need service_account:write, which a service account can't hold. + assert response.status_code == 403, response.content + disabled.refresh_from_db() + assert not disabled.is_active + assert disabled.scope_list == ["org:read"] + def test_get(self) -> None: with self.feature(FEATURE_FLAG): response = self.get_success_response(self.organization.slug, self.service_account.id)