From ecc1d435d0247ea115b4401856e3a08fc579c262 Mon Sep 17 00:00:00 2001 From: Shashank Jarmale Date: Thu, 17 Sep 2026 15:07:31 -0700 Subject: [PATCH] perf(auth): Skip full organization hydration during access checks Fetch project and team IDs lazily for RPC-backed global access, allowing permission contexts to omit full resource collections. Preserve active-resource filtering and request-local caching. Refs ISWF-3361 --- src/sentry/api/permissions.py | 7 +- src/sentry/auth/access.py | 18 ++- tests/sentry/api/bases/test_organization.py | 59 +++++++- tests/sentry/api/test_permissions.py | 24 ++++ tests/sentry/auth/test_access.py | 128 +++++++++++++++++- ...t_organization_sentry_app_installations.py | 16 ++- 6 files changed, 238 insertions(+), 14 deletions(-) diff --git a/src/sentry/api/permissions.py b/src/sentry/api/permissions.py index 9c97359e8446..ce6f8d2a7976 100644 --- a/src/sentry/api/permissions.py +++ b/src/sentry/api/permissions.py @@ -210,7 +210,10 @@ def determine_access( org_context = organization else: org_context = organization_service.get_organization_by_id( - id=extract_id_from(organization), user_id=user_id + id=extract_id_from(organization), + user_id=user_id, + include_projects=False, + include_teams=False, ) if org_context is None: @@ -376,6 +379,8 @@ def determine_access( org_context = organization_service.get_organization_by_id( id=extract_id_from(organization), user_id=request.user.id if request.user else None, + include_projects=False, + include_teams=False, ) assert org_context is not None, "Failed to fetch organization in determine_access" diff --git a/src/sentry/auth/access.py b/src/sentry/auth/access.py index c47ecbf333f6..0538367fe0c4 100644 --- a/src/sentry/auth/access.py +++ b/src/sentry/auth/access.py @@ -27,7 +27,11 @@ from sentry.models.organizationmemberteam import OrganizationMemberTeam from sentry.models.project import Project from sentry.models.team import Team, TeamStatus -from sentry.organizations.services.organization import RpcTeamMember, RpcUserOrganizationContext +from sentry.organizations.services.organization import ( + RpcTeamMember, + RpcUserOrganizationContext, + organization_service, +) from sentry.organizations.services.organization.serial import summarize_member from sentry.roles import organization_roles from sentry.roles.manager import OrganizationRole, TeamRole @@ -773,17 +777,17 @@ def has_project_access(self, project: Project) -> bool: @cached_property def accessible_team_ids(self) -> frozenset[int]: return frozenset( - t.id - for t in self.rpc_user_organization_context.organization.teams - if t.status == TeamStatus.ACTIVE + organization_service.get_active_team_ids( + organization_id=self.rpc_user_organization_context.organization.id + ) ) @cached_property def accessible_project_ids(self) -> frozenset[int]: return frozenset( - p.id - for p in self.rpc_user_organization_context.organization.projects - if p.status == ObjectStatus.ACTIVE + organization_service.get_active_project_ids( + organization_id=self.rpc_user_organization_context.organization.id + ) ) diff --git a/tests/sentry/api/bases/test_organization.py b/tests/sentry/api/bases/test_organization.py index 10bf51ce9c93..5a48c87d12fa 100644 --- a/tests/sentry/api/bases/test_organization.py +++ b/tests/sentry/api/bases/test_organization.py @@ -37,11 +37,12 @@ from sentry.models.organization import Organization from sentry.models.organizationmember import OrganizationMember from sentry.organizations.services.organization import organization_service +from sentry.organizations.services.organization.impl import DatabaseBackedOrganizationService from sentry.silo.base import SiloMode from sentry.testutils.cases import TestCase from sentry.testutils.helpers.datetime import freeze_time from sentry.testutils.requests import drf_request_from_request -from sentry.testutils.silo import assume_test_silo_mode +from sentry.testutils.silo import all_silo_test, assume_test_silo_mode from sentry.users.services.user.serial import serialize_rpc_user from sentry.users.services.user.service import user_service from sentry.utils.security.orgauthtoken_token import hash_token @@ -424,6 +425,62 @@ def build_request(self, user=None, active_superuser=False, **params): return request +@all_silo_test +class DetermineAccessHydrationTest(TestCase): + def test_member_access_omits_full_organization_resources(self) -> None: + user = self.create_user() + org = self.create_organization(flags=0) + team = self.create_team(organization=org) + project = self.create_project(organization=org, teams=[team]) + self.create_member(user=user, organization=org, role="member", teams=[team]) + other_team = self.create_team(organization=org) + other_project = self.create_project(organization=org, teams=[other_team]) + request = drf_request_from_request(self.make_request(user=user)) + + with ( + mock.patch.object( + DatabaseBackedOrganizationService, + "get_organization_by_id", + autospec=True, + side_effect=DatabaseBackedOrganizationService.get_organization_by_id, + ) as get_org, + mock.patch( + "sentry.organizations.services.organization.serial.serialize_project" + ) as serialize_project, + mock.patch( + "sentry.organizations.services.organization.serial.serialize_rpc_team" + ) as serialize_team, + ): + OrganizationPermission().determine_access(request, org) + + get_org.assert_called_once() + assert get_org.call_args.kwargs["id"] == org.id + assert get_org.call_args.kwargs["user_id"] == user.id + assert get_org.call_args.kwargs["include_projects"] is False + assert get_org.call_args.kwargs["include_teams"] is False + serialize_project.assert_not_called() + serialize_team.assert_not_called() + assert request.access.accessible_team_ids == frozenset({team.id}) + assert request.access.accessible_project_ids == frozenset({project.id}) + assert not request.access.has_team_access(other_team) + assert not request.access.has_project_access(other_project) + + def test_existing_context_is_reused(self) -> None: + user = self.create_user() + org = self.create_organization(owner=user) + context = organization_service.get_organization_by_id( + id=org.id, user_id=user.id, include_projects=False, include_teams=False + ) + assert context is not None + request = drf_request_from_request(self.make_request(user=user)) + + with mock.patch.object(organization_service, "get_organization_by_id") as get_org: + OrganizationPermission().determine_access(request, context) + + get_org.assert_not_called() + assert request.access.has_scope("org:read") + + class ControlSiloOrganizationEndpointTest(TestCase): def setUp(self) -> None: super().setUp() diff --git a/tests/sentry/api/test_permissions.py b/tests/sentry/api/test_permissions.py index 4d1f6a72c14b..10f98342c3b2 100644 --- a/tests/sentry/api/test_permissions.py +++ b/tests/sentry/api/test_permissions.py @@ -1,3 +1,5 @@ +from unittest.mock import patch + from rest_framework.views import APIView from sentry.api.bases.organization import OrganizationPermission @@ -296,6 +298,28 @@ def test_determine_access_no_demo_users(self) -> None: assert readonly_rpc_context.member.scopes == list(self.org_member_scopes) + def test_determine_access_omits_full_organization_resources(self) -> None: + team = self.create_team(organization=self.organization) + self.create_project(organization=self.organization, teams=[team]) + request = self.make_request(self.readonly_user) + + with ( + override_options( + {"demo-mode.enabled": True, "demo-mode.users": [self.readonly_user.id]} + ), + patch( + "sentry.organizations.services.organization.serial.serialize_project" + ) as serialize_project, + patch( + "sentry.organizations.services.organization.serial.serialize_rpc_team" + ) as serialize_team, + ): + self.user_permission.determine_access(request=request, organization=self.organization) + + serialize_project.assert_not_called() + serialize_team.assert_not_called() + assert request.access.scopes == frozenset(READONLY_SCOPES) + class InsufficientScopeResponseTest(APITestCase): """End-to-end: a token-scope denial reaches the client as a 403 carrying the RFC 6750 diff --git a/tests/sentry/auth/test_access.py b/tests/sentry/auth/test_access.py index 0c96f8075d37..6d57719ddcf2 100644 --- a/tests/sentry/auth/test_access.py +++ b/tests/sentry/auth/test_access.py @@ -1,6 +1,7 @@ from datetime import timedelta -from unittest.mock import Mock, patch +from unittest.mock import ANY, Mock, patch +import pytest from django.contrib.auth.models import AnonymousUser from django.test import override_settings from django.utils import timezone @@ -13,19 +14,27 @@ update_permission_scope_declaration, ) from sentry.auth.services.access.service import access_service +from sentry.auth.services.auth import AuthenticatedToken from sentry.auth.superuser import SUPERUSER_READONLY_SCOPES, SUPERUSER_SCOPES from sentry.constants import ObjectStatus from sentry.models.apikey import ApiKey from sentry.models.authidentity import AuthIdentity from sentry.models.authprovider import AuthProvider from sentry.models.organization import Organization +from sentry.models.project import Project from sentry.models.team import TeamStatus from sentry.organizations.services.organization import organization_service +from sentry.organizations.services.organization.impl import DatabaseBackedOrganizationService from sentry.silo.base import SiloMode from sentry.testutils.cases import TestCase from sentry.testutils.helpers import with_feature from sentry.testutils.helpers.options import override_options -from sentry.testutils.silo import all_silo_test, assume_test_silo_mode, no_silo_test +from sentry.testutils.silo import ( + all_silo_test, + assume_test_silo_mode, + assume_test_silo_mode_of, + no_silo_test, +) from sentry.users.models.user import User from sentry.users.models.userrole import UserRole @@ -40,7 +49,7 @@ def silo_from_user( rpc_user_org_context = None if organization: rpc_user_org_context = organization_service.get_organization_by_id( - id=organization.id, user_id=user.id + id=organization.id, user_id=user.id, include_projects=False, include_teams=False ) return access.from_user_and_rpc_user_org_context( user=user, @@ -55,7 +64,10 @@ def silo_from_request(request, organization: Organization | None = None, scopes= rpc_user_org_context = None if organization: rpc_user_org_context = organization_service.get_organization_by_id( - id=organization.id, user_id=request.user.id + id=organization.id, + user_id=request.user.id, + include_projects=False, + include_teams=False, ) return access.from_request_org_and_scopes( request=request, rpc_user_org_context=rpc_user_org_context, scopes=scopes @@ -695,6 +707,8 @@ def test_superuser_with_organization_without_membership(self) -> None: assert result.has_team_access(self.team1) assert result.project_ids_with_team_membership == frozenset() assert result.has_project_access(self.project1) + assert result.accessible_team_ids == frozenset({self.team1.id, self.team2.id}) + assert result.accessible_project_ids == frozenset({self.project1.id, self.project2.id}) def test_staff_with_organization_without_membership(self) -> None: request = self.make_request(user=self.staff, is_staff=True) @@ -854,6 +868,12 @@ def test_has_access(self) -> None: assert result.has_project_membership(self.project) assert not result.has_project_access(self.out_of_scope_project) assert not result.permissions + assert result.accessible_team_ids == frozenset({self.team.id}) + full_context = organization_service.get_organization_by_id(id=self.org.id) + assert full_context is not None + expected_projects = frozenset(p.id for p in full_context.organization.projects) + assert result.accessible_project_ids == expected_projects + assert result.project_ids_with_team_membership == expected_projects def test_no_access_due_to_no_app(self) -> None: user = self.create_user("integration2@example.com") @@ -901,6 +921,8 @@ def test_no_deleted_projects(self) -> None: result = self.from_request(request, self.org) assert result.has_project_access(deleted_project) is False assert result.has_project_membership(deleted_project) is False + assert deleted_project.id not in result.accessible_project_ids + assert deleted_project.id not in result.project_ids_with_team_membership def test_no_deleted_teams(self) -> None: deleted_team = self.create_team(organization=self.org, status=TeamStatus.PENDING_DELETION) @@ -910,6 +932,8 @@ def test_no_deleted_teams(self) -> None: request = self.make_request(user=self.proxy_user) result = self.from_request(request, self.org) assert result.has_team_access(deleted_team) is False + assert deleted_team.id not in result.accessible_team_ids + assert deleted_team.id not in result.team_ids_with_membership def test_has_app_scopes(self) -> None: app_with_scopes = self.create_sentry_app(name="ScopeyTheApp", organization=self.org) @@ -927,6 +951,102 @@ def test_has_app_scopes(self) -> None: assert result.has_scope("team:admin") is False +@all_silo_test +class RpcGlobalAccessTest(TestCase): + def setUp(self) -> None: + super().setUp() + self.org = self.create_organization() + self.team = self.create_team(organization=self.org) + self.project = self.create_project(organization=self.org, teams=[]) + context = organization_service.get_organization_by_id( + id=self.org.id, include_projects=False, include_teams=False + ) + assert context is not None + self.context = context + self.token = AuthenticatedToken(kind="org_auth_token", organization_id=self.org.id) + + def test_org_token_resource_ids(self) -> None: + other_org = self.create_organization() + other_project = self.create_project(organization=other_org) + other_team = self.create_team(organization=other_org) + self.create_project(organization=self.org, status=ObjectStatus.PENDING_DELETION) + self.create_project(organization=self.org, status=ObjectStatus.DELETION_IN_PROGRESS) + self.create_project(organization=self.org, status=ObjectStatus.DISABLED) + self.create_team(organization=self.org, status=TeamStatus.PENDING_DELETION) + self.create_team(organization=self.org, status=TeamStatus.DELETION_IN_PROGRESS) + + result = access.from_rpc_auth(self.token, self.context) + + assert result.accessible_project_ids == frozenset({self.project.id}) + assert result.accessible_team_ids == frozenset({self.team.id}) + assert result.has_project_access(self.project) + assert result.has_team_access(self.team) + assert not result.has_project_access(other_project) + assert not result.has_team_access(other_team) + assert result.project_ids_with_team_membership == frozenset() + assert result.team_ids_with_membership == frozenset() + + def test_wrong_org_token_has_no_access(self) -> None: + token = AuthenticatedToken( + kind="org_auth_token", organization_id=self.create_organization().id + ) + + result = access.from_rpc_auth(token, self.context) + + assert isinstance(result, NoAccess) + assert result.accessible_project_ids == frozenset() + assert result.accessible_team_ids == frozenset() + assert not result.has_project_access(self.project) + assert not result.has_team_access(self.team) + + def test_resource_ids_are_independently_lazy_and_cached(self) -> None: + with ( + patch.object( + DatabaseBackedOrganizationService, + "get_active_project_ids", + autospec=True, + side_effect=DatabaseBackedOrganizationService.get_active_project_ids, + ) as project_ids, + patch.object( + DatabaseBackedOrganizationService, + "get_active_team_ids", + autospec=True, + side_effect=DatabaseBackedOrganizationService.get_active_team_ids, + ) as team_ids, + ): + result = access.from_rpc_auth(self.token, self.context) + assert result.has_project_access(self.project) + assert result.has_team_access(self.team) + project_ids.assert_not_called() + team_ids.assert_not_called() + + assert result.accessible_project_ids == frozenset({self.project.id}) + assert result.accessible_project_ids == frozenset({self.project.id}) + project_ids.assert_called_once_with(ANY, organization_id=self.org.id) + team_ids.assert_not_called() + + assert result.accessible_team_ids == frozenset({self.team.id}) + assert result.accessible_team_ids == frozenset({self.team.id}) + team_ids.assert_called_once_with(ANY, organization_id=self.org.id) + + def test_resource_ids_are_refreshed_for_new_access_objects(self) -> None: + result = access.from_rpc_auth(self.token, self.context) + assert result.accessible_project_ids == frozenset({self.project.id}) + with assume_test_silo_mode_of(Project): + self.project.update(status=ObjectStatus.PENDING_DELETION) + + refreshed = access.from_rpc_auth(self.token, self.context) + assert refreshed.accessible_project_ids == frozenset() + + def test_resource_lookup_failure_propagates(self) -> None: + result = access.from_rpc_auth(self.token, self.context) + with ( + patch.object(organization_service, "get_active_project_ids", side_effect=RuntimeError), + pytest.raises(RuntimeError), + ): + _ = result.accessible_project_ids + + @no_silo_test class DefaultAccessTest(TestCase): @patch("sentry.auth.scope_declaration.options.get") diff --git a/tests/sentry/sentry_apps/api/endpoints/test_organization_sentry_app_installations.py b/tests/sentry/sentry_apps/api/endpoints/test_organization_sentry_app_installations.py index e6348a12b354..b52e59a8d033 100644 --- a/tests/sentry/sentry_apps/api/endpoints/test_organization_sentry_app_installations.py +++ b/tests/sentry/sentry_apps/api/endpoints/test_organization_sentry_app_installations.py @@ -1,4 +1,5 @@ from typing import Any +from unittest.mock import patch from django.test import override_settings @@ -101,7 +102,20 @@ def test_superuser_read_and_write_sees_all_installs(self) -> None: def test_users_only_sees_installs_on_their_org(self) -> None: self.login_as(user=self.user) - response = self.get_success_response(self.org.slug, status_code=200) + self.create_project(organization=self.org) + self.create_team(organization=self.org) + with ( + patch( + "sentry.organizations.services.organization.serial.serialize_project" + ) as serialize_project, + patch( + "sentry.organizations.services.organization.serial.serialize_rpc_team" + ) as serialize_team, + ): + response = self.get_success_response(self.org.slug, status_code=200) + + serialize_project.assert_not_called() + serialize_team.assert_not_called() assert response.data == [ {