Browse Source

chore(hybrid-cloud): Refactor Organization ORM out of views and auth (#40362)

For hybrid cloud, the organization and related models will not exist in the control silo, but will be necessary for certain auth related flows.  This change is the first of many to make the core auth flows compatible with a split silo world by introducing a service object that captures existing needs for an organization arond the `get_active_organization` method.  Behavior should remain identical, except that the pure ORM object is not available in many places.  Those places have been updated to use a new thinner model object that corresponds with future control silo's data availability.

Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
Zach Collins 2 years ago
parent
commit
a882713d1b

+ 5 - 2
src/sentry/api/endpoints/auth_config.py

@@ -24,6 +24,10 @@ class AuthConfigEndpoint(Endpoint, OrganizationMixin):
     # Disable authentication and permission requirements.
     permission_classes = []
 
+    def dispatch(self, request: Request, *args, **kwargs) -> Response:
+        self.determine_active_organization(request)
+        return super().dispatch(request, *args, **kwargs)
+
     def get(self, request: Request, *args, **kwargs) -> Response:
         """
         Get context required to show a login page. Registration is handled elsewhere.
@@ -57,8 +61,7 @@ class AuthConfigEndpoint(Endpoint, OrganizationMixin):
         next_uri = self.get_next_uri(request)
 
         if not is_valid_redirect(next_uri, allowed_hosts=(request.get_host(),)):
-            active_org = self.get_active_organization(request)
-            next_uri = get_org_redirect_url(request, active_org)
+            next_uri = get_org_redirect_url(request, self.active_organization)
 
         return Response({"nextUri": next_uri})
 

+ 11 - 3
src/sentry/api/endpoints/auth_login.py

@@ -1,3 +1,5 @@
+from typing import Optional
+
 from rest_framework.request import Request
 from rest_framework.response import Response
 
@@ -5,6 +7,7 @@ from sentry import ratelimits as ratelimiter
 from sentry.api.base import Endpoint, control_silo_endpoint
 from sentry.api.serializers.base import serialize
 from sentry.api.serializers.models.user import DetailedSelfUserSerializer
+from sentry.models import Organization
 from sentry.utils import auth, metrics
 from sentry.utils.hashlib import md5_text
 from sentry.web.forms.accounts import AuthenticationForm
@@ -16,7 +19,13 @@ class AuthLoginEndpoint(Endpoint, OrganizationMixin):
     # Disable authentication and permission requirements.
     permission_classes = []
 
-    def post(self, request: Request, organization=None, *args, **kwargs) -> Response:
+    def dispatch(self, request: Request, *args, **kwargs) -> Response:
+        self.determine_active_organization(request)
+        return super().dispatch(request, *args, **kwargs)
+
+    def post(
+        self, request: Request, organization: Optional[Organization] = None, *args, **kwargs
+    ) -> Response:
         """
         Process a login request via username/password. SSO login is handled
         elsewhere.
@@ -57,8 +66,7 @@ class AuthLoginEndpoint(Endpoint, OrganizationMixin):
                 }
             )
 
-        active_org = self.get_active_organization(request)
-        redirect_url = auth.get_org_redirect_url(request, active_org)
+        redirect_url = auth.get_org_redirect_url(request, self.active_organization)
 
         return Response(
             {

+ 2 - 2
src/sentry/api/serializers/models/auth_provider.py

@@ -1,7 +1,7 @@
 from django.db.models import F
 
 from sentry.api.serializers import Serializer, register
-from sentry.models import AuthProvider, OrganizationMember
+from sentry.models import AuthProvider, Organization, OrganizationMember
 from sentry.utils.http import absolute_uri
 
 
@@ -14,7 +14,7 @@ class AuthProviderSerializer(Serializer):
             flags=F("flags").bitand(~OrganizationMember.flags["sso:linked"]),
         ).count()
 
-        login_url = organization.get_url()
+        login_url = Organization.get_url(organization.slug)
 
         return {
             "id": str(obj.id),

+ 11 - 9
src/sentry/integrations/github/integration.py

@@ -321,15 +321,17 @@ class GitHubInstallationRedirect(PipelineView):
             pipeline.bind_state("reinstall_id", request.GET["reinstall_id"])
 
         if "installation_id" in request.GET:
-            organization = self.get_active_organization(request)
-
-            # We want to wait until the scheduled deletions finish or else the
-            # post install to migrate repos do not work.
-            integration_pending_deletion_exists = OrganizationIntegration.objects.filter(
-                integration__provider=GitHubIntegrationProvider.key,
-                organization=organization,
-                status=ObjectStatus.PENDING_DELETION,
-            ).exists()
+            self.determine_active_organization(request)
+
+            integration_pending_deletion_exists = False
+            if self.active_organization:
+                # We want to wait until the scheduled deletions finish or else the
+                # post install to migrate repos do not work.
+                integration_pending_deletion_exists = OrganizationIntegration.objects.filter(
+                    integration__provider=GitHubIntegrationProvider.key,
+                    organization_id=self.active_organization.id,
+                    status=ObjectStatus.PENDING_DELETION,
+                ).exists()
 
             if integration_pending_deletion_exists:
                 return render_to_response(

+ 2 - 2
src/sentry/models/lostpasswordhash.py

@@ -5,12 +5,12 @@ from django.db import models
 from django.urls import reverse
 from django.utils import timezone
 
-from sentry.db.models import FlexibleForeignKey, Model, region_silo_only_model, sane_repr
+from sentry.db.models import FlexibleForeignKey, Model, control_silo_only_model, sane_repr
 from sentry.utils.http import absolute_uri
 from sentry.utils.security import get_secure_token
 
 
-@region_silo_only_model
+@control_silo_only_model
 class LostPasswordHash(Model):
     __include_in_export__ = False
 

+ 5 - 3
src/sentry/models/organization.py

@@ -511,11 +511,13 @@ class Organization(Model, SnowflakeIdMixin):
                 request, remove_email_verification_non_compliant_members
             )
 
-    def get_url_viewname(self):
+    @staticmethod
+    def get_url_viewname():
         return "sentry-organization-issue-list"
 
-    def get_url(self):
-        return reverse(self.get_url_viewname(), args=[self.slug])
+    @staticmethod
+    def get_url(slug: str):
+        return reverse(Organization.get_url_viewname(), args=[slug])
 
     def get_scopes(self, role: Role) -> FrozenSet[str]:
         if role.priority > 0:

+ 2 - 2
src/sentry/models/user.py

@@ -19,7 +19,7 @@ from sentry.db.models import (
     BaseModel,
     BoundedAutoField,
     FlexibleForeignKey,
-    region_silo_only_model,
+    control_silo_only_model,
     sane_repr,
 )
 from sentry.models import LostPasswordHash
@@ -115,7 +115,7 @@ class UserManager(BaseManager, DjangoUserManager):
         return self.filter(emails__is_verified=True, is_active=True, **kwargs)
 
 
-@region_silo_only_model
+@control_silo_only_model
 class User(BaseModel, AbstractBaseUser):
     __include_in_export__ = True
 

+ 65 - 16
src/sentry/services/hybrid_cloud/__init__.py

@@ -3,7 +3,7 @@ import logging
 from abc import ABC, abstractmethod
 from dataclasses import dataclass
 from enum import Enum
-from typing import Dict, Generic, List, Mapping, Optional, Type, TypeVar, cast
+from typing import Dict, Generic, Iterable, List, Mapping, Optional, Type, TypeVar, cast
 
 from sentry.models import Organization, OrganizationMember, OrganizationStatus
 
@@ -34,10 +34,20 @@ class ApiProjectKey:
     dsn_public: str = ""
 
 
+@dataclass
+class ApiOrganizationMember:
+    # This can be null when the user is deleted.
+    user_id: Optional[int]
+    pass
+
+
 @dataclass
 class ApiOrganization:
     slug: str = ""
     id: int = -1
+    # exists iff the organization was queried with a user_id context, and that user_id
+    # was confirmed to be a member.
+    member: Optional[ApiOrganizationMember] = None
 
 
 class InterfaceWithLifecycle(ABC):
@@ -105,7 +115,7 @@ class ProjectKeyService(InterfaceWithLifecycle):
 class OrganizationService(InterfaceWithLifecycle):
     @abstractmethod
     def get_organizations(
-        self, user_id: Optional[id], scope: Optional[str], only_visible: bool
+        self, user_id: Optional[int], scope: Optional[str], only_visible: bool
     ) -> List[ApiOrganization]:
         """
         This method is expected to follow the optionally given user_id, scope, and only_visible options to filter
@@ -119,23 +129,56 @@ class OrganizationService(InterfaceWithLifecycle):
         """
         pass
 
+    @abstractmethod
+    def check_membership_by_email(
+        self, organization_id: int, email: str
+    ) -> Optional[ApiOrganizationMember]:
+        """
+        Used to look up an organization membership by an email, used in very specific edge cases.
+        """
+        pass
+
     @abstractmethod
     def get_organization_by_slug(
-        self, slug: str, only_visible: bool, allow_stale: bool
+        self, *, user_id: Optional[int], slug: str, only_visible: bool, allow_stale: bool
     ) -> Optional[ApiOrganization]:
         pass
 
-    def _serialize_organization(self, org: Organization) -> ApiOrganization:
-        return ApiOrganization(
-            slug=org.slug,
-            id=org.id,
-        )
+    def _serialize_member(self, member: OrganizationMember) -> ApiOrganizationMember:
+        return ApiOrganizationMember(user_id=member.user.id if member.user is not None else None)
+
+    def _serialize_organization(
+        self, org: Organization, membership: Iterable[OrganizationMember] = tuple()
+    ) -> ApiOrganization:
+        org = ApiOrganization(slug=org.slug, id=org.id)
+
+        for member in membership:
+            if member.organization.id == org.id:
+                org.member = self._serialize_member(member)
+                break
+
+        return org
 
 
 class DatabaseBackedOrganizationService(OrganizationService):
+    def check_membership_by_email(
+        self, organization_id: int, email: str
+    ) -> Optional[ApiOrganizationMember]:
+        try:
+            member = OrganizationMember.objects.get(organization_id=organization_id, email=email)
+        except OrganizationMember.DoesNotExist:
+            return None
+
+        return self._serialize_member(member)
+
     def get_organization_by_slug(
-        self, slug: str, only_visible: bool, allow_stale: bool
+        self, *, user_id: Optional[int], slug: str, only_visible: bool, allow_stale: bool
     ) -> Optional[ApiOrganization]:
+        membership: List[OrganizationMember]
+        if user_id is not None:
+            membership = OrganizationMember.objects.filter(user_id=user_id)
+        else:
+            membership = []
         try:
             if allow_stale:
                 org = Organization.objects.get_from_cache(slug=slug)
@@ -144,7 +187,7 @@ class DatabaseBackedOrganizationService(OrganizationService):
 
             if only_visible and org.status != OrganizationStatus.VISIBLE:
                 raise Organization.DoesNotExist
-            return self._serialize_organization(org)
+            return self._serialize_organization(org, membership)
         except Organization.DoesNotExist:
             logger.info("Active organization [%s] not found", slug)
 
@@ -154,8 +197,17 @@ class DatabaseBackedOrganizationService(OrganizationService):
         pass
 
     def get_organizations(
-        self, user_id: Optional[id], scope: Optional[str], only_visible: bool
+        self, user_id: Optional[int], scope: Optional[str], only_visible: bool
     ) -> List[ApiOrganization]:
+        if user_id is None:
+            return []
+        organizations = self._query_organizations(user_id, scope, only_visible)
+        membership = OrganizationMember.objects.filter(user_id=user_id)
+        return [self._serialize_organization(o, membership) for o in organizations]
+
+    def _query_organizations(
+        self, user_id: int, scope: Optional[str], only_visible: bool
+    ) -> List[Organization]:
         from django.conf import settings
 
         if settings.SENTRY_PUBLIC and scope is None:
@@ -164,10 +216,7 @@ class DatabaseBackedOrganizationService(OrganizationService):
             else:
                 return list(Organization.objects.filter())
 
-        if user_id is not None:
-            qs = OrganizationMember.objects.filter(user_id=user_id)
-        else:
-            qs = OrganizationMember.objects.filter()
+        qs = OrganizationMember.objects.filter(user_id=user_id)
 
         qs = qs.select_related("organization")
         if only_visible:
@@ -178,7 +227,7 @@ class DatabaseBackedOrganizationService(OrganizationService):
         if scope is not None:
             return [r.organization for r in results if scope in r.get_scopes()]
 
-        return [self._serialize_organization(r.organization) for r in results]
+        return [r.organization for r in results]
 
 
 class DatabaseBackedProjectKeyService(ProjectKeyService):

+ 4 - 3
src/sentry/utils/auth.py

@@ -15,7 +15,8 @@ from django.urls import resolve, reverse
 from django.utils.http import is_safe_url
 from rest_framework.request import Request
 
-from sentry.models import Authenticator, User
+from sentry.models import Authenticator, Organization, User
+from sentry.services.hybrid_cloud import ApiOrganization
 from sentry.utils import metrics
 from sentry.utils.http import absolute_uri
 
@@ -139,12 +140,12 @@ def initiate_login(request, next_url=None):
         request.session["_next"] = next_url
 
 
-def get_org_redirect_url(request, active_organization):
+def get_org_redirect_url(request, active_organization: Optional[ApiOrganization]):
     from sentry import features
 
     # TODO(dcramer): deal with case when the user cannot create orgs
     if active_organization:
-        return active_organization.get_url()
+        return Organization.get_url(active_organization.slug)
     if not features.has("organizations:create"):
         return "/auth/login/"
     return "/organizations/new/"

+ 28 - 18
src/sentry/web/frontend/auth_login.py

@@ -1,3 +1,5 @@
+from typing import Optional
+
 from django.conf import settings
 from django.contrib import messages
 from django.contrib.auth import REDIRECT_FIELD_NAME
@@ -14,6 +16,7 @@ from sentry.auth.superuser import is_active_superuser
 from sentry.constants import WARN_SESSION_EXPIRED
 from sentry.http import get_server_hostname
 from sentry.models import AuthProvider, Organization, OrganizationMember, OrganizationStatus
+from sentry.services.hybrid_cloud import organization_service
 from sentry.signals import join_request_link_viewed, user_signup
 from sentry.utils import auth, json, metrics
 from sentry.utils.auth import (
@@ -117,6 +120,12 @@ class AuthLoginView(BaseView):
     def respond_login(self, request: Request, context, **kwargs):
         return self.respond("sentry/login.html", context)
 
+    def _handle_login(self, request: Request, user, organization: Optional[Organization]):
+        login(request, user, organization_id=organization.id if organization else None)
+        self.determine_active_organization(
+            request,
+        )
+
     def handle_basic_auth(self, request: Request, **kwargs):
         can_register = self.can_register(request)
 
@@ -150,7 +159,7 @@ class AuthLoginView(BaseView):
             # HACK: grab whatever the first backend is and assume it works
             user.backend = settings.AUTHENTICATION_BACKENDS[0]
 
-            login(request, user, organization_id=organization.id if organization else None)
+            self._handle_login(request, user, organization)
 
             # can_register should only allow a single registration
             request.session.pop("can_register", None)
@@ -203,7 +212,7 @@ class AuthLoginView(BaseView):
             elif login_form.is_valid():
                 user = login_form.get_user()
 
-                login(request, user, organization_id=organization.id if organization else None)
+                self._handle_login(request, user, organization)
                 metrics.incr(
                     "login.attempt", instance="success", skip_internal=True, sample_rate=1.0
                 )
@@ -211,30 +220,31 @@ class AuthLoginView(BaseView):
                 if not user.is_active:
                     return self.redirect(reverse("sentry-reactivate-account"))
                 if organization:
-                    if (
-                        self._is_org_member(user, organization)
-                        and request.user
-                        and not is_active_superuser(request)
-                    ):
+                    # Refresh the organization we fetched prior to login in order to check its login state.
+                    organization = organization_service.get_organization_by_slug(
+                        user_id=request.user.id,
+                        slug=organization.slug,
+                        only_visible=False,
+                        allow_stale=False,
+                    )
+                    if organization.member and request.user and not is_active_superuser(request):
                         auth.set_active_org(request, organization.slug)
 
                     if settings.SENTRY_SINGLE_ORGANIZATION:
-                        try:
-                            om = OrganizationMember.objects.get(
-                                organization=organization, email=user.email
-                            )
-                            # XXX(jferge): if user is removed / invited but has an acct,
-                            # pop _next so they aren't in infinite redirect on Single Org Mode
-                        except OrganizationMember.DoesNotExist:
+                        om = organization_service.check_membership_by_email(
+                            organization.id, user.email
+                        )
+                        if om is None:
                             request.session.pop("_next", None)
                         else:
-                            if om.user is None:
+                            if om.user_id is None:
                                 request.session.pop("_next", None)
 
                 # On login, redirect to onboarding
-                active_org = self.get_active_organization(request)
-                if active_org:
-                    onboarding_redirect = get_client_state_redirect_uri(active_org.slug, None)
+                if self.active_organization:
+                    onboarding_redirect = get_client_state_redirect_uri(
+                        self.active_organization.slug, None
+                    )
                     if onboarding_redirect:
                         request.session["_next"] = onboarding_redirect
 

Some files were not shown because too many files changed in this diff