Browse Source

feat(sentryapps): Fetch projects for both internal and public apps (#34038)

Objective:
Fixes #33938

/projects only accepted Internal Integrations tokens. Because we handle Public and Internal SentryApps authentication differently, I made an abstraction to fetch projects. Future: We will need to make some changes to have a similar level of abstraction for authentication of SentryApps
NisanthanNanthakumar 2 years ago
parent
commit
f105f43f56

+ 3 - 2
src/sentry/api/endpoints/project_index.py

@@ -10,7 +10,8 @@ from sentry.api.paginator import DateTimePaginator
 from sentry.api.serializers import ProjectWithOrganizationSerializer, serialize
 from sentry.auth.superuser import is_active_superuser
 from sentry.db.models.query import in_iexact
-from sentry.models import Project, ProjectPlatform, ProjectStatus, SentryAppInstallationToken
+from sentry.models import Project, ProjectPlatform, ProjectStatus
+from sentry.models.integrations.sentry_app_installation import SentryAppInstallation
 from sentry.search.utils import tokenize_query
 
 
@@ -46,7 +47,7 @@ class ProjectIndexEndpoint(Endpoint):
                 queryset = queryset.none()
         elif not (is_active_superuser(request) and request.GET.get("show") == "all"):
             if request.user.is_sentry_app:
-                queryset = SentryAppInstallationToken.objects.get_projects(request.auth)
+                queryset = SentryAppInstallation.objects.get_projects(request.auth)
                 if isinstance(queryset, EmptyQuerySet):
                     raise AuthenticationFailed("Token not found")
             else:

+ 24 - 2
src/sentry/models/integrations/sentry_app_installation.py

@@ -1,12 +1,14 @@
+from __future__ import annotations
+
 import uuid
 from itertools import chain
-from typing import List
+from typing import TYPE_CHECKING, List
 
 from django.db import models
 from django.db.models import OuterRef, QuerySet, Subquery
 from django.utils import timezone
 
-from sentry.constants import SentryAppInstallationStatus
+from sentry.constants import SentryAppInstallationStatus, SentryAppStatus
 from sentry.db.models import (
     BoundedPositiveIntegerField,
     FlexibleForeignKey,
@@ -14,6 +16,9 @@ from sentry.db.models import (
     ParanoidModel,
 )
 
+if TYPE_CHECKING:
+    from sentry.models import ApiToken, Project
+
 
 def default_uuid():
     return str(uuid.uuid4())
@@ -33,6 +38,23 @@ class SentryAppInstallationForProviderManager(ParanoidManager):
     def get_by_api_token(self, token_id: str) -> QuerySet:
         return self.filter(status=SentryAppInstallationStatus.INSTALLED, api_token_id=token_id)
 
+    def get_projects(self, token: ApiToken) -> QuerySet[Project]:
+        from sentry.models import Project, SentryAppInstallationToken
+
+        try:
+            installation = self.get_by_api_token(token.id).get()
+        except SentryAppInstallation.DoesNotExist:
+            installation = None
+
+        if not installation:
+            return Project.objects.none()
+
+        # TODO(nisanthan): Right now, Internal Integrations can have multiple ApiToken, so we use the join table `SentryAppInstallationToken` to map the one to many relationship. However, for Public Integrations, we can only have 1 ApiToken per installation. So we currently don't use the join table for Public Integrations. We should update to make records in the join table for Public Integrations so that we can have a common abstraction for finding an installation by ApiToken.
+        if installation.sentry_app.status == SentryAppStatus.INTERNAL:
+            return SentryAppInstallationToken.objects.get_projects(token)
+
+        return Project.objects.filter(organization_id=installation.organization_id)
+
     def get_related_sentry_app_components(
         self,
         organization_ids: List[int],

+ 15 - 1
src/sentry/testutils/factories.py

@@ -41,6 +41,7 @@ from sentry.mediators import (
     sentry_app_installations,
     sentry_apps,
     service_hooks,
+    token_exchange,
 )
 from sentry.models import (
     Activity,
@@ -79,6 +80,7 @@ from sentry.models import (
     Repository,
     RepositoryProjectPathConfig,
     Rule,
+    SentryAppInstallation,
     Team,
     User,
     UserEmail,
@@ -748,7 +750,9 @@ class Factories:
         return _kwargs
 
     @staticmethod
-    def create_sentry_app_installation(organization=None, slug=None, user=None, status=None):
+    def create_sentry_app_installation(
+        organization=None, slug=None, user=None, status=None, prevent_token_exchange=False
+    ):
         if not organization:
             organization = Factories.create_organization()
 
@@ -759,8 +763,18 @@ class Factories:
             organization=organization,
             user=(user or Factories.create_user()),
         )
+
         install.status = SentryAppInstallationStatus.INSTALLED if status is None else status
         install.save()
+
+        if not prevent_token_exchange and (install.sentry_app.status != SentryAppStatus.INTERNAL):
+            token_exchange.GrantExchanger.run(
+                install=install,
+                code=install.api_grant.code,
+                client_id=install.sentry_app.application.client_id,
+                user=install.sentry_app.proxy_user,
+            )
+            install = SentryAppInstallation.objects.get(id=install.id)
         return install
 
     @staticmethod

+ 4 - 1
tests/acceptance/test_organization_sentry_app_detailed_view.py

@@ -36,7 +36,10 @@ class OrganizationSentryAppDetailedView(AcceptanceTestCase):
 
     def test_uninstallation(self):
         self.installation = self.create_sentry_app_installation(
-            slug=self.sentry_app.slug, organization=self.organization, user=self.user
+            slug=self.sentry_app.slug,
+            organization=self.organization,
+            user=self.user,
+            prevent_token_exchange=True,
         )
 
         self.load_page(self.sentry_app.slug)

+ 2 - 0
tests/sentry/api/endpoints/test_organization_sentry_app_installation_details.py

@@ -27,6 +27,7 @@ class SentryAppInstallationDetailsTest(APITestCase):
             organization=self.super_org,
             user=self.superuser,
             status=SentryAppInstallationStatus.PENDING,
+            prevent_token_exchange=True,
         )
 
         self.unpublished_app = self.create_sentry_app(name="Testin", organization=self.org)
@@ -36,6 +37,7 @@ class SentryAppInstallationDetailsTest(APITestCase):
             organization=self.org,
             user=self.user,
             status=SentryAppInstallationStatus.PENDING,
+            prevent_token_exchange=True,
         )
 
         self.url = reverse(

+ 8 - 2
tests/sentry/api/endpoints/test_organization_sentry_app_installations.py

@@ -17,11 +17,17 @@ class SentryAppInstallationsTest(APITestCase):
         self.unpublished_app = self.create_sentry_app(name="Testin", organization=self.org)
 
         self.installation = self.create_sentry_app_installation(
-            slug=self.published_app.slug, organization=self.super_org, user=self.superuser
+            slug=self.published_app.slug,
+            organization=self.super_org,
+            user=self.superuser,
+            prevent_token_exchange=True,
         )
 
         self.installation2 = self.create_sentry_app_installation(
-            slug=self.unpublished_app.slug, organization=self.org, user=self.user
+            slug=self.unpublished_app.slug,
+            organization=self.org,
+            user=self.user,
+            prevent_token_exchange=True,
         )
 
         self.url = reverse("sentry-api-0-sentry-app-installations", args=[self.org.slug])

+ 37 - 3
tests/sentry/api/endpoints/test_project_index.py

@@ -1,6 +1,8 @@
 from django.urls import reverse
+from rest_framework import status
 
 from sentry.models import Project, ProjectStatus, SentryAppInstallationToken
+from sentry.models.apitoken import ApiToken
 from sentry.testutils import APITestCase
 
 
@@ -154,7 +156,39 @@ class ProjectsListTest(APITestCase):
 
         # Delete the token
         SentryAppInstallationToken.objects.all().delete()
+        self.get_error_response(
+            extra_headers={"HTTP_AUTHORIZATION": f"Bearer {token}"},
+            status_code=status.HTTP_401_UNAUTHORIZED,
+        )
 
-        path = reverse(self.endpoint)
-        response = self.client.get(path, HTTP_AUTHORIZATION=f"Bearer {token}")
-        assert response.status_code == 401
+    def get_installed_unpublished_sentry_app_access_token(self):
+        self.project = self.create_project(organization=self.organization, teams=[self.team])
+        sentry_app = self.create_sentry_app(
+            scopes=("project:read",),
+            published=False,
+            verify_install=False,
+            name="Super Awesome App",
+        )
+        installation = self.create_sentry_app_installation(
+            slug=sentry_app.slug, organization=self.organization, user=self.user
+        )
+        return installation.api_token.token
+
+    def test_valid_with_public_integration(self):
+        token = self.get_installed_unpublished_sentry_app_access_token()
+
+        # there should only be one record created so just grab the first one
+        response = self.get_success_response(
+            extra_headers={"HTTP_AUTHORIZATION": f"Bearer {token}"}
+        )
+        assert self.project.name.encode("utf-8") in response.content
+
+    def test_deleted_token_with_public_integration(self):
+        token = self.get_installed_unpublished_sentry_app_access_token()
+
+        ApiToken.objects.all().delete()
+
+        self.get_error_response(
+            extra_headers={"HTTP_AUTHORIZATION": f"Bearer {token}"},
+            status_code=status.HTTP_401_UNAUTHORIZED,
+        )

+ 8 - 2
tests/sentry/api/endpoints/test_sentry_app_authorizations.py

@@ -28,7 +28,10 @@ class TestSentryAppAuthorizations(APITestCase):
         )
 
         self.install = self.create_sentry_app_installation(
-            organization=self.organization, slug="nulldb", user=self.user
+            organization=self.organization,
+            slug="nulldb",
+            user=self.user,
+            prevent_token_exchange=True,
         )
 
     def get_response(self, *args, **params):
@@ -64,7 +67,10 @@ class TestSentryAppAuthorizations(APITestCase):
 
     def test_invalid_installation(self):
         self.install = self.create_sentry_app_installation(
-            organization=self.organization, slug="slowdb", user=self.user
+            organization=self.organization,
+            slug="slowdb",
+            user=self.user,
+            prevent_token_exchange=True,
         )
 
         # URL with this new Install's uuid in it

+ 1 - 9
tests/sentry/api/endpoints/test_sentry_internal_app_token_details.py

@@ -1,6 +1,5 @@
 from django.urls import reverse
 
-from sentry.mediators.token_exchange import GrantExchanger
 from sentry.models import ApiToken
 from sentry.testutils import APITestCase
 
@@ -60,16 +59,9 @@ class SentryInternalAppTokenCreationTest(APITestCase):
             slug=sentry_app.slug, organization=self.org, user=self.user
         )
 
-        client_id = install.sentry_app.application.client_id
-        user = install.sentry_app.proxy_user
-
-        api_token = GrantExchanger.run(
-            install=install, code=install.api_grant.code, client_id=client_id, user=user
-        )
-
         url = reverse(
             "sentry-api-0-sentry-internal-app-token-details",
-            args=[install.sentry_app.slug, api_token.token],
+            args=[install.sentry_app.slug, install.api_token.token],
         )
 
         self.login_as(user=self.user)

+ 1 - 1
tests/sentry/mediators/sentry_app_installations/test_installation_notifier.py

@@ -39,7 +39,7 @@ class TestInstallationNotifier(TestCase):
         )
 
         self.install = self.create_sentry_app_installation(
-            slug="foo", organization=self.org, user=self.user
+            slug="foo", organization=self.org, user=self.user, prevent_token_exchange=True
         )
 
     @patch("sentry.tasks.sentry_apps.safe_urlopen", return_value=MockResponseInstance)

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