Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(hc): Remove get_orgs() #54635

Merged
merged 8 commits into from
Aug 12, 2023
Merged
Show file tree
Hide file tree
Changes from 5 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
52 changes: 32 additions & 20 deletions src/sentry/api/bases/sentryapps.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,14 @@
from sentry.auth.superuser import is_active_superuser
from sentry.coreapi import APIError
from sentry.middleware.stats import add_request_metric_tags
from sentry.models import Organization
from sentry.models.integrations.sentry_app import SentryApp
from sentry.models.organization import OrganizationStatus
from sentry.services.hybrid_cloud.app import RpcSentryApp, app_service
from sentry.services.hybrid_cloud.organization import organization_service
from sentry.services.hybrid_cloud.organization import (
RpcUserOrganizationContext,
organization_service,
)
from sentry.services.hybrid_cloud.user import RpcUser
from sentry.services.hybrid_cloud.user.service import user_service
from sentry.utils.sdk import configure_scope
from sentry.utils.strings import to_single_line_str
Expand Down Expand Up @@ -77,17 +80,17 @@ class SentryAppsPermission(SentryPermission):
"POST": ("org:write", "org:admin"),
}

def has_object_permission(self, request: Request, view, organization):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SentryApps will be control silo, which means they won't have direct access to organizations. the context here represents an RPC lookup of org details.

def has_object_permission(self, request: Request, view, context: RpcUserOrganizationContext):
if not hasattr(request, "user") or not request.user:
return False

self.determine_access(request, organization)
self.determine_access(request, context)

if is_active_superuser(request):
return True

# User must be a part of the Org they're trying to create the app in.
if organization not in request.user.get_orgs():
if context.organization.status != OrganizationStatus.ACTIVE or not context.member:
raise Http404

return ensure_scoped_permission(request, self.scope_map.get(request.method))
Expand All @@ -109,27 +112,36 @@ def _get_organization_slug(self, request: Request):
raise ValidationError({"organization": to_single_line_str(error_message)})
return organization_slug

def _get_organization_for_superuser(self, organization_slug):
try:
return Organization.objects.get(slug=organization_slug)
except Organization.DoesNotExist:
def _get_organization_for_superuser(
self, user: RpcUser, organization_slug: str
) -> RpcUserOrganizationContext:
context = organization_service.get_organization_by_slug(
slug=organization_slug, only_visible=False, user_id=user.id
)

if context is None:
error_message = f"Organization '{organization_slug}' does not exist."
raise ValidationError({"organization": to_single_line_str(error_message)})

def _get_organization_for_user(self, user, organization_slug):
try:
return user.get_orgs().get(slug=organization_slug)
except Organization.DoesNotExist:
return context

def _get_organization_for_user(
self, user: RpcUser, organization_slug: str
) -> RpcUserOrganizationContext:
context = organization_service.get_organization_by_slug(
slug=organization_slug, only_visible=True, user_id=user.id
)
if context is None or context.member is None:
error_message = f"User does not belong to the '{organization_slug}' organization."
raise PermissionDenied(to_single_line_str(error_message))
return context

def _get_organization(self, request: Request):
def _get_org_context(self, request: Request) -> RpcUserOrganizationContext:
organization_slug = self._get_organization_slug(request)
if is_active_superuser(request):
return self._get_organization_for_superuser(organization_slug)
return self._get_organization_for_superuser(request.user, organization_slug)
else:
user = request.user
return self._get_organization_for_user(user, organization_slug)
return self._get_organization_for_user(request.user, organization_slug)

def convert_args(self, request: Request, *args, **kwargs):
"""
Expand All @@ -154,9 +166,9 @@ def convert_args(self, request: Request, *args, **kwargs):
if not request.json_body:
return (args, kwargs)

organization = self._get_organization(request)
self.check_object_permissions(request, organization)
kwargs["organization"] = organization
context = self._get_org_context(request)
self.check_object_permissions(request, context)
kwargs["organization"] = context.organization

return (args, kwargs)

Expand Down
5 changes: 4 additions & 1 deletion src/sentry/api/bases/user.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from __future__ import annotations

from rest_framework.request import Request

from sentry.api.base import Endpoint
Expand All @@ -6,10 +8,11 @@
from sentry.auth.superuser import is_active_superuser
from sentry.auth.system import is_system_auth
from sentry.models import Organization, OrganizationStatus, User
from sentry.services.hybrid_cloud.user import RpcUser


class UserPermission(SentryPermission):
def has_object_permission(self, request: Request, view, user=None):
def has_object_permission(self, request: Request, view, user: User | RpcUser | None = None):
if user is None:
user = request.user
if request.user.id == user.id:
Expand Down
19 changes: 17 additions & 2 deletions src/sentry/api/endpoints/integrations/sentry_apps/index.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from sentry.constants import SentryAppStatus
from sentry.models import SentryApp
from sentry.sentry_apps.apps import SentryAppCreator
from sentry.services.hybrid_cloud.user.service import user_service
from sentry.utils import json

logger = logging.getLogger(__name__)
Expand All @@ -30,11 +31,25 @@ def get(self, request: Request) -> Response:
elif status == "unpublished":
queryset = SentryApp.objects.filter(status=SentryAppStatus.UNPUBLISHED)
if not is_active_superuser(request):
queryset = queryset.filter(owner_id__in=[o.id for o in request.user.get_orgs()])
queryset = queryset.filter(
owner_id__in=[
o.organization_id
for o in user_service.get_organizations(
user_id=request.user.id, only_visible=True
)
]
)
elif status == "internal":
queryset = SentryApp.objects.filter(status=SentryAppStatus.INTERNAL)
if not is_active_superuser(request):
queryset = queryset.filter(owner_id__in=[o.id for o in request.user.get_orgs()])
queryset = queryset.filter(
owner_id__in=[
o.organization_id
for o in user_service.get_organizations(
user_id=request.user.id, only_visible=True
)
]
)
else:
if is_active_superuser(request):
queryset = SentryApp.objects.all()
Expand Down
45 changes: 16 additions & 29 deletions src/sentry/api/endpoints/user_notification_fine_tuning.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from typing import Any, Mapping

from django.db import router, transaction
from rest_framework import status
from rest_framework.request import Request
Expand All @@ -13,6 +15,7 @@
get_option_value_from_int,
get_type_from_fine_tuning_key,
)
from sentry.services.hybrid_cloud.user.service import user_service
from sentry.types.integrations import ExternalProviders

INVALID_EMAIL_MSG = (
Expand Down Expand Up @@ -80,7 +83,8 @@ def put(self, request: Request, user, notification_type) -> Response:

# Validate that all of the IDs are integers.
try:
ids_to_update = {int(i) for i in request.data.keys()}
for k in request.data.keys():
int(k)
except ValueError:
return Response(
{"detail": "Invalid id value provided. Id values should be integers."},
Expand All @@ -89,28 +93,11 @@ def put(self, request: Request, user, notification_type) -> Response:

# Make sure that the IDs we are going to update are a subset of the
# user's list of organizations or projects.
parents = (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requiring validation of project is and organization ids forces a fan out across all regions for a user. In reality, these values are not foreign keys. We also frequently "leave behind" notification settings without clearing them in all cases, so letting arbitrary values from a user isn't particularly problematic. Having notification settings for a project or org you don't belong to doesn't grant any access or necessarily grant any notifications.

user.get_orgs() if notification_type == FineTuningAPIKey.DEPLOY else user.get_projects()
)
parent_ids = {parent.id for parent in parents}
if not ids_to_update.issubset(parent_ids):
bad_ids = ids_to_update - parent_ids
return Response(
{
"detail": "At least one of the requested projects is not \
available to this user, because the user does not belong \
to the necessary teams. (ids of unavailable projects: %s)"
% bad_ids
},
status=status.HTTP_403_FORBIDDEN,
)

if notification_type == FineTuningAPIKey.EMAIL:
return self._handle_put_emails(user, request.data)

return self._handle_put_notification_settings(
user, notification_type, parents, request.data
)
return self._handle_put_notification_settings(user, notification_type, request.data)

@staticmethod
def _handle_put_reports(user, data):
Expand All @@ -122,7 +109,7 @@ def _handle_put_reports(user, data):
value = set(user_option.value or [])

# The set of IDs of the organizations of which the user is a member.
org_ids = {organization.id for organization in user.get_orgs()}
org_ids = {o.id for o in user_service.get_organizations(user_id=user.id, only_visible=True)}
for org_id, enabled in data.items():
org_id = int(org_id)
# We want "0" to be falsey
Expand Down Expand Up @@ -167,26 +154,26 @@ def _handle_put_emails(user, data):
return Response(status=status.HTTP_204_NO_CONTENT)

@staticmethod
def _handle_put_notification_settings(user, notification_type: FineTuningAPIKey, parents, data):
def _handle_put_notification_settings(
user, notification_type: FineTuningAPIKey, data: Mapping[str, Any]
):
with transaction.atomic(using=router.db_for_write(NotificationSetting)):
for parent in parents:
# We fetched every available parent but only care about the ones in `request.data`.
if str(parent.id) not in data:
continue
for setting_obj_id_str, value_str in data.items():
setting_obj_id_int = int(setting_obj_id_str)

# This partitioning always does the same thing because notification_type stays constant.
project_option, organization_option = (
(None, parent)
(None, setting_obj_id_int)
if notification_type == FineTuningAPIKey.DEPLOY
else (parent, None)
else (setting_obj_id_int, None)
)

type = get_type_from_fine_tuning_key(notification_type)
value = int(data[str(parent.id)])
value_int = int(value_str)
NotificationSetting.objects.update_settings(
ExternalProviders.EMAIL,
type,
get_option_value_from_int(type, value),
get_option_value_from_int(type, value_int),
user_id=user.id,
project=project_option,
organization=organization_option,
Expand Down
40 changes: 34 additions & 6 deletions src/sentry/api/endpoints/user_organizations.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,45 @@
from __future__ import annotations

from django.db.models import Q
from rest_framework.request import Request
from rest_framework.response import Response
from typing_extensions import override

from sentry.api.base import control_silo_endpoint
from sentry.api.bases.user import UserEndpoint
from sentry.api.base import Endpoint, region_silo_endpoint
from sentry.api.bases.user import UserPermission
from sentry.api.exceptions import ResourceDoesNotExist
from sentry.api.paginator import OffsetPaginator
from sentry.api.serializers import serialize
from sentry.models import Organization, User
from sentry.services.hybrid_cloud.user import RpcUser
from sentry.services.hybrid_cloud.user.service import user_service


@region_silo_endpoint
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swapping this to a region endpoint. In the future, we plan to have the frontend query a user's organization settings per non single tenant region they belong to. Pushing this to the frontend makes sense in the long term.

class UserOrganizationsEndpoint(Endpoint):
permission_classes = (UserPermission,)

@override
def convert_args(self, request: Request, user_id: str | None = None, *args, **kwargs):
user: RpcUser | User | None = None

if user_id == "me":
if not request.user.is_authenticated:
raise ResourceDoesNotExist
user = request.user
elif user_id is not None:
user = user_service.get_user(user_id=int(user_id))

if not user:
raise ResourceDoesNotExist

self.check_object_permissions(request, user)

kwargs["user"] = user
return args, kwargs

@control_silo_endpoint
class UserOrganizationsEndpoint(UserEndpoint):
def get(self, request: Request, user) -> Response:
queryset = user.get_orgs()
def get(self, request: Request, user: RpcUser) -> Response:
queryset = Organization.objects.get_for_user_ids({user.id})

query = request.GET.get("query")
if query:
Expand Down
4 changes: 1 addition & 3 deletions src/sentry/api/serializers/models/sentry_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,7 @@ def serialize(self, obj, attrs, user, access):
user_org_ids = attrs["user_org_ids"]

if owner:
if (env.request and is_active_superuser(env.request)) or (
hasattr(user, "get_orgs") and owner.id in user_org_ids
):
if (env.request and is_active_superuser(env.request)) or owner.id in user_org_ids:
client_secret = (
obj.application.client_secret if obj.show_auth_info(access) else MASKED_VALUE
)
Expand Down
4 changes: 3 additions & 1 deletion src/sentry/features/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ def wrapped(self: Any, request: Request, *args: Any, **kwargs: Any) -> Response:
# flag enabled.
if any_org:
if not any_organization_has_feature(
feature, request.user.get_orgs(), actor=request.user
feature,
Organization.objects.get_for_user_ids({request.user.id}),
actor=request.user,
):
return Response(status=404)

Expand Down
10 changes: 0 additions & 10 deletions src/sentry/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -356,16 +356,6 @@ def refresh_session_nonce(self, request=None):
if request is not None:
request.session["_nonce"] = self.session_nonce

def get_orgs(self):
from sentry.models import Organization

return Organization.objects.get_for_user_ids({self.id})

def get_projects(self):
from sentry.models import Project

return Project.objects.get_for_user_ids({self.id})

def get_orgs_require_2fa(self):
from sentry.models import Organization, OrganizationStatus

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ class UserNotificationFineTuningTest(UserNotificationFineTuningTestBase):
method = "put"

def test_update_invalid_project(self):
self.get_error_response("me", "alerts", status_code=403, **{"123": 1})
# We do not validate project / organization ids! Due to the silo split we accept these at face value rather than fan out
# across all silos to validate them.
self.get_response("me", "alerts", **{"123": 1})

def test_invalid_id_value(self):
self.get_error_response("me", "alerts", status_code=400, **{"nope": 1})
Expand Down Expand Up @@ -271,9 +273,6 @@ def test_permissions(self):
user=self.user, organization_id=new_org.id, key="reports"
).exists()

data = {str(new_project.id): 1}
self.get_error_response("me", "alerts", status_code=403, **data)

value = NotificationSetting.objects.get_settings(
ExternalProviders.EMAIL,
NotificationSettingTypes.ISSUE_ALERTS,
Expand Down
9 changes: 7 additions & 2 deletions tests/sentry/features/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from sentry import features
from sentry.features.base import OrganizationFeature
from sentry.features.helpers import any_organization_has_feature, requires_feature
from sentry.models import Organization
from sentry.testutils.cases import TestCase


Expand All @@ -22,9 +23,13 @@
features.add("foo", OrganizationFeature)

def test_any_organization_has_feature(self):
assert not any_organization_has_feature("foo", self.user.get_orgs())
assert not any_organization_has_feature(
"foo", list(Organization.objects.get_for_user_ids({self.user.id}).all())
)
with org_with_feature(self.org, "foo"):
assert any_organization_has_feature("foo", self.user.get_orgs())
assert any_organization_has_feature(
"foo", list(Organization.objects.get_for_user_ids({self.user.id}))
)

def test_org_missing_from_request_fails(self):
@requires_feature("foo")
Expand All @@ -41,7 +46,7 @@
assert get(None, self.request, organization=self.org).status_code == 404

def test_org_with_feature_flag_succeeds(self):
# The Org in scope of the request has the flag.

Check failure on line 49 in tests/sentry/features/test_helpers.py

View workflow job for this annotation

GitHub Actions / backend test (3, 14)

TestFeatureHelpers.test_any_org_true_when_users_other_org_has_flag_succeeds NameError: name 'Organization' is not defined
with org_with_feature(self.org, "foo"):

@requires_feature("foo")
Expand Down
Loading
Loading