From 37bd9b3b1234b59a9fda348e3221208e52691b1b Mon Sep 17 00:00:00 2001 From: Stephen Cefali Date: Mon, 15 Jul 2019 16:00:26 -0700 Subject: [PATCH 1/7] temp --- src/sentry/api/bases/sentryapps.py | 6 ++- .../sentry_app_installation_details.py | 23 ++++++++++- .../models/sentry_app_installation.py | 2 + .../rest_framework/sentry_app_installation.py | 28 ++++++++++++++ .../sentry_app_installations/__init__.py | 1 + .../sentry_app_installations/updater.py | 38 +++++++++++++++++++ 6 files changed, 96 insertions(+), 2 deletions(-) create mode 100644 src/sentry/api/serializers/rest_framework/sentry_app_installation.py create mode 100644 src/sentry/mediators/sentry_app_installations/updater.py diff --git a/src/sentry/api/bases/sentryapps.py b/src/sentry/api/bases/sentryapps.py index 23aa22bd887e31..7f0ade667ac151 100644 --- a/src/sentry/api/bases/sentryapps.py +++ b/src/sentry/api/bases/sentryapps.py @@ -173,7 +173,7 @@ def convert_args(self, request, sentry_app_slug, *args, **kwargs): class SentryAppInstallationsPermission(SentryPermission): scope_map = { 'GET': ('org:read', 'org:integrations', 'org:write', 'org:admin'), - 'POST': ('org:integrations', 'org:write', 'org:admin'), + 'POST': ('org:integrations', 'org:write', 'org:admin') } def has_object_permission(self, request, view, organization): @@ -225,9 +225,11 @@ class SentryAppInstallationPermission(SentryPermission): # nested endpoints will probably need different scopes - figure out how # to deal with that when it happens. 'POST': ('org:integrations', 'event:write', 'event:admin'), + 'PUT': ('org:read') } def has_object_permission(self, request, view, installation): + print ("\n\n check permissions") if not hasattr(request, 'user') or not request.user: return False @@ -246,8 +248,10 @@ def has_object_permission(self, request, view, installation): class SentryAppInstallationBaseEndpoint(IntegrationPlatformEndpoint): + # permission_classes = ( ) permission_classes = (SentryAppInstallationPermission, ) + def convert_args(self, request, uuid, *args, **kwargs): try: installation = SentryAppInstallation.objects.get( diff --git a/src/sentry/api/endpoints/sentry_app_installation_details.py b/src/sentry/api/endpoints/sentry_app_installation_details.py index f580e71baa0e95..9391d103a4bb92 100644 --- a/src/sentry/api/endpoints/sentry_app_installation_details.py +++ b/src/sentry/api/endpoints/sentry_app_installation_details.py @@ -4,7 +4,8 @@ from sentry.api.bases import SentryAppInstallationBaseEndpoint from sentry.api.serializers import serialize -from sentry.mediators.sentry_app_installations import Destroyer +from sentry.mediators.sentry_app_installations import Destroyer, Updater +from sentry.api.serializers.rest_framework import SentryAppInstallationSerializer class SentryAppInstallationDetailsEndpoint(SentryAppInstallationBaseEndpoint): @@ -19,3 +20,23 @@ def delete(self, request, installation): request=request, ) return Response(status=204) + + def put(self, request, installation): + print("\n\n\n hey there") + serializer = SentryAppInstallationSerializer( + installation, + data=request.data, + partial=True, + ) + + if serializer.is_valid(): + result = serializer.validated_data + + updated_installation = Updater.run( + user=request.user, + sentry_app_installation=installation, + status=result.get('status'), + ) + + return Response(serialize(updated_installation, request.user)) + return Response(serializer.errors, status=400) diff --git a/src/sentry/api/serializers/models/sentry_app_installation.py b/src/sentry/api/serializers/models/sentry_app_installation.py index b26c911698837a..f2830df1434280 100644 --- a/src/sentry/api/serializers/models/sentry_app_installation.py +++ b/src/sentry/api/serializers/models/sentry_app_installation.py @@ -8,6 +8,7 @@ @register(SentryAppInstallation) class SentryAppInstallationSerializer(Serializer): def serialize(self, install, attrs, user): + print ("id", install.id) data = { 'app': { 'uuid': install.sentry_app.uuid, @@ -17,6 +18,7 @@ def serialize(self, install, attrs, user): 'slug': install.organization.slug, }, 'uuid': install.uuid, + 'status': install.status, } if install.api_grant: diff --git a/src/sentry/api/serializers/rest_framework/sentry_app_installation.py b/src/sentry/api/serializers/rest_framework/sentry_app_installation.py new file mode 100644 index 00000000000000..f8f553ff5034e6 --- /dev/null +++ b/src/sentry/api/serializers/rest_framework/sentry_app_installation.py @@ -0,0 +1,28 @@ +from __future__ import absolute_import + +from jsonschema.exceptions import ValidationError as SchemaValidationError + +from rest_framework import serializers +from rest_framework.serializers import Serializer, ValidationError + +from django.template.defaultfilters import slugify +from sentry.api.validators.sentry_apps.schema import validate as validate_schema +from sentry.models import ApiScopes, SentryApp +from sentry.models.sentryapp import VALID_EVENT_RESOURCES, REQUIRED_EVENT_PERMISSIONS + + +class SentryAppInstallationSerializer(Serializer): + status = serializers.CharField() + + def validate_status(self, new_status): + last_status = self.instance.status + print ("last status", last_status) + print ("new status", new_status) + if last_status == 'installed': + if not new_status in ['installed']: + raise ValidationError('Cannot change installed integration to %s' % new_status) + elif last_status == 'pending': + if not new_status in ['pending', 'installed']: + raise ValidationError('Cannot change pending integration to %s' % new_status) + + return new_status diff --git a/src/sentry/mediators/sentry_app_installations/__init__.py b/src/sentry/mediators/sentry_app_installations/__init__.py index cd76833604be95..d9c9ab597fc3a1 100644 --- a/src/sentry/mediators/sentry_app_installations/__init__.py +++ b/src/sentry/mediators/sentry_app_installations/__init__.py @@ -2,4 +2,5 @@ from .creator import Creator # NOQA from .destroyer import Destroyer # NOQA +from .updater import Updater # NOQA from .installation_notifier import InstallationNotifier # NOQA diff --git a/src/sentry/mediators/sentry_app_installations/updater.py b/src/sentry/mediators/sentry_app_installations/updater.py new file mode 100644 index 00000000000000..00391ebe518386 --- /dev/null +++ b/src/sentry/mediators/sentry_app_installations/updater.py @@ -0,0 +1,38 @@ +from __future__ import absolute_import + +import six + +from collections import Iterable + +from sentry import analytics +from sentry.coreapi import APIError +from sentry.constants import SentryAppStatus +from sentry.mediators import Mediator, Param +from sentry.mediators import service_hooks +from sentry.mediators.param import if_param +from sentry.models import SentryAppComponent, ServiceHook +from sentry.models.sentryapp import REQUIRED_EVENT_PERMISSIONS + + +class Updater(Mediator): + sentry_app_installation= Param('sentry.models.SentryAppInstallation') + status = Param(six.string_types, required=False) + + def call(self): + self._update_status() + self.sentry_app_installation.save() + return self.sentry_app_installation + + @if_param('status') + def _update_status(self): + print ("new status", self.status) + self.sentry_app_installation.status = self.status + + def record_analytics(self): + pass + # TODO: Add analytics? + # analytics.record( + # 'sentry_app_installation.updated', + # user_id=self.user.id, + # sentry_app_installation=self.sentry_app_installation.id, + # ) From 79bbcb95410be6eaa7a2e3629b80b8776fc97379 Mon Sep 17 00:00:00 2001 From: Stephen Cefali Date: Tue, 16 Jul 2019 15:44:30 -0700 Subject: [PATCH 2/7] initial cut --- src/sentry/api/bases/sentryapps.py | 16 +++-- .../sentry_app_installation_details.py | 1 - .../models/sentry_app_installation.py | 4 +- .../rest_framework/sentry_app_installation.py | 23 ++++--- src/sentry/constants.py | 14 +++-- .../sentry_app_installations/updater.py | 11 ++-- ...ization_sentry_app_installation_details.py | 61 +++++++++++++++++++ 7 files changed, 107 insertions(+), 23 deletions(-) diff --git a/src/sentry/api/bases/sentryapps.py b/src/sentry/api/bases/sentryapps.py index 7f0ade667ac151..575e907ad2d5bd 100644 --- a/src/sentry/api/bases/sentryapps.py +++ b/src/sentry/api/bases/sentryapps.py @@ -2,6 +2,7 @@ from django.http import Http404 from functools import wraps +from django.conf import settings from sentry.utils.sdk import configure_scope from sentry.api.authentication import ClientIdSecretAuthentication @@ -225,11 +226,16 @@ class SentryAppInstallationPermission(SentryPermission): # nested endpoints will probably need different scopes - figure out how # to deal with that when it happens. 'POST': ('org:integrations', 'event:write', 'event:admin'), - 'PUT': ('org:read') } + def has_permission(self, request, *args, **kwargs): + # To let the app mark the installation as installed, we don't care about permissions + if request.user.is_sentry_app: + if request.method == 'PUT': + return True + return super(SentryAppInstallationPermission, self).has_permission(request, *args, **kwargs) + def has_object_permission(self, request, view, installation): - print ("\n\n check permissions") if not hasattr(request, 'user') or not request.user: return False @@ -238,6 +244,10 @@ def has_object_permission(self, request, view, installation): if is_active_superuser(request): return True + # if user is an app, make sure it's for that same app + if request.user.is_sentry_app: + return request.user == installation.sentry_app.proxy_user + if installation.organization not in request.user.get_orgs(): raise Http404 @@ -248,10 +258,8 @@ def has_object_permission(self, request, view, installation): class SentryAppInstallationBaseEndpoint(IntegrationPlatformEndpoint): - # permission_classes = ( ) permission_classes = (SentryAppInstallationPermission, ) - def convert_args(self, request, uuid, *args, **kwargs): try: installation = SentryAppInstallation.objects.get( diff --git a/src/sentry/api/endpoints/sentry_app_installation_details.py b/src/sentry/api/endpoints/sentry_app_installation_details.py index 9391d103a4bb92..699d6de38fd225 100644 --- a/src/sentry/api/endpoints/sentry_app_installation_details.py +++ b/src/sentry/api/endpoints/sentry_app_installation_details.py @@ -22,7 +22,6 @@ def delete(self, request, installation): return Response(status=204) def put(self, request, installation): - print("\n\n\n hey there") serializer = SentryAppInstallationSerializer( installation, data=request.data, diff --git a/src/sentry/api/serializers/models/sentry_app_installation.py b/src/sentry/api/serializers/models/sentry_app_installation.py index f2830df1434280..dda961969c0e96 100644 --- a/src/sentry/api/serializers/models/sentry_app_installation.py +++ b/src/sentry/api/serializers/models/sentry_app_installation.py @@ -3,12 +3,12 @@ from sentry.api.serializers import Serializer, register from sentry.models import SentryAppInstallation +from sentry.constants import SentryAppInstallationStatus @register(SentryAppInstallation) class SentryAppInstallationSerializer(Serializer): def serialize(self, install, attrs, user): - print ("id", install.id) data = { 'app': { 'uuid': install.sentry_app.uuid, @@ -18,7 +18,7 @@ def serialize(self, install, attrs, user): 'slug': install.organization.slug, }, 'uuid': install.uuid, - 'status': install.status, + 'status': SentryAppInstallationStatus.as_str(install.status), } if install.api_grant: diff --git a/src/sentry/api/serializers/rest_framework/sentry_app_installation.py b/src/sentry/api/serializers/rest_framework/sentry_app_installation.py index f8f553ff5034e6..30692d0213dbe0 100644 --- a/src/sentry/api/serializers/rest_framework/sentry_app_installation.py +++ b/src/sentry/api/serializers/rest_framework/sentry_app_installation.py @@ -1,5 +1,7 @@ from __future__ import absolute_import + +from collections import OrderedDict from jsonschema.exceptions import ValidationError as SchemaValidationError from rest_framework import serializers @@ -9,20 +11,25 @@ from sentry.api.validators.sentry_apps.schema import validate as validate_schema from sentry.models import ApiScopes, SentryApp from sentry.models.sentryapp import VALID_EVENT_RESOURCES, REQUIRED_EVENT_PERMISSIONS +from sentry.constants import SentryAppInstallationStatus class SentryAppInstallationSerializer(Serializer): status = serializers.CharField() def validate_status(self, new_status): + # make sure the status in in our defined list + try: + SentryAppInstallationStatus.STATUS_MAP[new_status] + except KeyError as e: + raise ValidationError( + 'Invalid value for status. Valid values: {}'.format( + SentryAppInstallationStatus.STATUS_MAP.keys(), + ), + ) last_status = self.instance.status - print ("last status", last_status) - print ("new status", new_status) - if last_status == 'installed': - if not new_status in ['installed']: - raise ValidationError('Cannot change installed integration to %s' % new_status) - elif last_status == 'pending': - if not new_status in ['pending', 'installed']: - raise ValidationError('Cannot change pending integration to %s' % new_status) + # make sure we don't go from installed to pending + if last_status == SentryAppInstallationStatus.INSTALLED_STR and new_status == SentryAppInstallationStatus.PENDING_STR: + raise ValidationError('Cannot change installed integration to pending') return new_status diff --git a/src/sentry/constants.py b/src/sentry/constants.py index afd7a91f701ffa..a2bddc93ec60af 100644 --- a/src/sentry/constants.py +++ b/src/sentry/constants.py @@ -402,20 +402,26 @@ def as_str(cls, status): class SentryAppInstallationStatus(object): PENDING = 0 INSTALLED = 1 + PENDING_STR = 'pending' + INSTALLED_STR = 'installed' + STATUS_MAP = OrderedDict([ + (INSTALLED_STR, INSTALLED), + (PENDING_STR, PENDING), + ]) @classmethod def as_choices(cls): return ( - (cls.PENDING, 'pending'), - (cls.INSTALLED, 'installed'), + (cls.PENDING, cls.PENDING_STR), + (cls.INSTALLED, cls.INSTALLED_STR), ) @classmethod def as_str(cls, status): if status == cls.PENDING: - return 'pending' + return cls.PENDING_STR elif status == cls.INSTALLED: - return 'installed' + return cls.INSTALLED_STR StatsPeriod = namedtuple('StatsPeriod', ('segments', 'interval')) diff --git a/src/sentry/mediators/sentry_app_installations/updater.py b/src/sentry/mediators/sentry_app_installations/updater.py index 00391ebe518386..b84233327ec66f 100644 --- a/src/sentry/mediators/sentry_app_installations/updater.py +++ b/src/sentry/mediators/sentry_app_installations/updater.py @@ -6,7 +6,7 @@ from sentry import analytics from sentry.coreapi import APIError -from sentry.constants import SentryAppStatus +from sentry.constants import SentryAppInstallationStatus from sentry.mediators import Mediator, Param from sentry.mediators import service_hooks from sentry.mediators.param import if_param @@ -15,7 +15,7 @@ class Updater(Mediator): - sentry_app_installation= Param('sentry.models.SentryAppInstallation') + sentry_app_installation = Param('sentry.models.SentryAppInstallation') status = Param(six.string_types, required=False) def call(self): @@ -25,8 +25,11 @@ def call(self): @if_param('status') def _update_status(self): - print ("new status", self.status) - self.sentry_app_installation.status = self.status + # convert from string to integer + print("update status", self.status) + print("update status 1", SentryAppInstallationStatus.STATUS_MAP[self.status]) + self.sentry_app_installation.status = SentryAppInstallationStatus.STATUS_MAP[self.status] + print("hey") def record_analytics(self): pass diff --git a/tests/sentry/api/endpoints/test_organization_sentry_app_installation_details.py b/tests/sentry/api/endpoints/test_organization_sentry_app_installation_details.py index 8ddbb7ae44c61c..7c297c0e2d53c3 100644 --- a/tests/sentry/api/endpoints/test_organization_sentry_app_installation_details.py +++ b/tests/sentry/api/endpoints/test_organization_sentry_app_installation_details.py @@ -4,6 +4,8 @@ from sentry.testutils import APITestCase import responses +from sentry.mediators.token_exchange import GrantExchanger, Refresher +from sentry.constants import SentryAppInstallationStatus class SentryAppInstallationDetailsTest(APITestCase): @@ -17,6 +19,7 @@ def setUp(self): name='Test', organization=self.super_org, published=True, + scopes=('org:write', 'team:admin'), ) self.installation = self.create_sentry_app_installation( @@ -58,6 +61,7 @@ def test_access_within_installs_organization(self): }, 'uuid': self.installation2.uuid, 'code': self.installation2.api_grant.code, + 'status': 'pending' } def test_no_access_outside_install_organization(self): @@ -95,3 +99,60 @@ def test_member_cannot_delete_install(self): response = self.client.delete(self.url, format='json') assert response.status_code == 403 + + +class MarkInstalledSentryAppInstallationsTest(SentryAppInstallationDetailsTest): + def setUp(self): + super(MarkInstalledSentryAppInstallationsTest, self).setUp() + self.token = GrantExchanger.run( + install=self.installation, + code=self.installation.api_grant.code, + client_id=self.published_app.application.client_id, + user=self.published_app.proxy_user, + ) + + def test_sentry_app_installation_mark_installed(self): + self.url = reverse( + 'sentry-api-0-sentry-app-installation-details', + args=[self.installation.uuid], + ) + response = self.client.put( + self.url, + data={'status': 'installed'}, + HTTP_AUTHORIZATION=u'Bearer {}'.format(self.token.token), + format='json', + ) + assert response.status_code == 200 + assert response.data['status'] == 'installed' + + def test_sentry_app_installation_mark_bad_status(self): + self.url = reverse( + 'sentry-api-0-sentry-app-installation-details', + args=[self.installation.uuid], + ) + response = self.client.put( + self.url, + data={'status': 'other'}, + HTTP_AUTHORIZATION=u'Bearer {}'.format(self.token.token), + format='json', + ) + assert response.status_code == 400 + assert response.data['status'][0] == u"Invalid value for status. Valid values: ['installed', 'pending']" + + def test_sentry_app_installation_already_installed(self): + self.installation.status = SentryAppInstallationStatus.INSTALLED + self.installation.save() + + self.url = reverse( + 'sentry-api-0-sentry-app-installation-details', + args=[self.installation.uuid], + ) + response = self.client.put( + self.url, + data={'status': 'pending'}, + HTTP_AUTHORIZATION=u'Bearer {}'.format(self.token.token), + format='json', + ) + print ("data", response.data) + assert response.status_code == 400 + assert response.data['status'][0] == "Cannot change installed integration to pending" From f197b794cf02c512ec3fd21dc10095221591863c Mon Sep 17 00:00:00 2001 From: Stephen Cefali Date: Tue, 16 Jul 2019 16:06:14 -0700 Subject: [PATCH 3/7] update tests --- src/sentry/api/bases/sentryapps.py | 3 +-- .../rest_framework/sentry_app_installation.py | 13 +++---------- .../mediators/sentry_app_installations/updater.py | 12 ++---------- 3 files changed, 6 insertions(+), 22 deletions(-) diff --git a/src/sentry/api/bases/sentryapps.py b/src/sentry/api/bases/sentryapps.py index 575e907ad2d5bd..e89b1cc0e46852 100644 --- a/src/sentry/api/bases/sentryapps.py +++ b/src/sentry/api/bases/sentryapps.py @@ -2,7 +2,6 @@ from django.http import Http404 from functools import wraps -from django.conf import settings from sentry.utils.sdk import configure_scope from sentry.api.authentication import ClientIdSecretAuthentication @@ -174,7 +173,7 @@ def convert_args(self, request, sentry_app_slug, *args, **kwargs): class SentryAppInstallationsPermission(SentryPermission): scope_map = { 'GET': ('org:read', 'org:integrations', 'org:write', 'org:admin'), - 'POST': ('org:integrations', 'org:write', 'org:admin') + 'POST': ('org:integrations', 'org:write', 'org:admin'), } def has_object_permission(self, request, view, organization): diff --git a/src/sentry/api/serializers/rest_framework/sentry_app_installation.py b/src/sentry/api/serializers/rest_framework/sentry_app_installation.py index 30692d0213dbe0..f1a8b52c97cb14 100644 --- a/src/sentry/api/serializers/rest_framework/sentry_app_installation.py +++ b/src/sentry/api/serializers/rest_framework/sentry_app_installation.py @@ -1,16 +1,8 @@ from __future__ import absolute_import -from collections import OrderedDict -from jsonschema.exceptions import ValidationError as SchemaValidationError - from rest_framework import serializers from rest_framework.serializers import Serializer, ValidationError - -from django.template.defaultfilters import slugify -from sentry.api.validators.sentry_apps.schema import validate as validate_schema -from sentry.models import ApiScopes, SentryApp -from sentry.models.sentryapp import VALID_EVENT_RESOURCES, REQUIRED_EVENT_PERMISSIONS from sentry.constants import SentryAppInstallationStatus @@ -21,13 +13,14 @@ def validate_status(self, new_status): # make sure the status in in our defined list try: SentryAppInstallationStatus.STATUS_MAP[new_status] - except KeyError as e: + except KeyError: raise ValidationError( 'Invalid value for status. Valid values: {}'.format( SentryAppInstallationStatus.STATUS_MAP.keys(), ), ) - last_status = self.instance.status + # convert status to str for comparison + last_status = SentryAppInstallationStatus.as_str(self.instance.status) # make sure we don't go from installed to pending if last_status == SentryAppInstallationStatus.INSTALLED_STR and new_status == SentryAppInstallationStatus.PENDING_STR: raise ValidationError('Cannot change installed integration to pending') diff --git a/src/sentry/mediators/sentry_app_installations/updater.py b/src/sentry/mediators/sentry_app_installations/updater.py index b84233327ec66f..fdf6dea211d629 100644 --- a/src/sentry/mediators/sentry_app_installations/updater.py +++ b/src/sentry/mediators/sentry_app_installations/updater.py @@ -2,16 +2,11 @@ import six -from collections import Iterable -from sentry import analytics -from sentry.coreapi import APIError +# from sentry import analytics from sentry.constants import SentryAppInstallationStatus from sentry.mediators import Mediator, Param -from sentry.mediators import service_hooks from sentry.mediators.param import if_param -from sentry.models import SentryAppComponent, ServiceHook -from sentry.models.sentryapp import REQUIRED_EVENT_PERMISSIONS class Updater(Mediator): @@ -26,14 +21,11 @@ def call(self): @if_param('status') def _update_status(self): # convert from string to integer - print("update status", self.status) - print("update status 1", SentryAppInstallationStatus.STATUS_MAP[self.status]) self.sentry_app_installation.status = SentryAppInstallationStatus.STATUS_MAP[self.status] - print("hey") def record_analytics(self): pass - # TODO: Add analytics? + # TODO: Add analytics # analytics.record( # 'sentry_app_installation.updated', # user_id=self.user.id, From 43a7e2260e722574da2c83fd39858370c63cd505 Mon Sep 17 00:00:00 2001 From: Stephen Cefali Date: Tue, 16 Jul 2019 16:27:43 -0700 Subject: [PATCH 4/7] adds analytics --- .../events/sentry_app_installation_updated.py | 16 +++++++++ .../sentry_app_installations/updater.py | 14 ++++---- ...ization_sentry_app_installation_details.py | 33 +++++++++++++++++-- 3 files changed, 53 insertions(+), 10 deletions(-) create mode 100644 src/sentry/analytics/events/sentry_app_installation_updated.py diff --git a/src/sentry/analytics/events/sentry_app_installation_updated.py b/src/sentry/analytics/events/sentry_app_installation_updated.py new file mode 100644 index 00000000000000..19c6fb8345d187 --- /dev/null +++ b/src/sentry/analytics/events/sentry_app_installation_updated.py @@ -0,0 +1,16 @@ +from __future__ import absolute_import + +from sentry import analytics + + +class SentryAppInstallationUpdatedEvent(analytics.Event): + type = 'sentry_app_installation.updated' + + attributes = ( + analytics.Attribute('sentry_app_installation_id'), + analytics.Attribute('sentry_app_id'), + analytics.Attribute('organization_id'), + ) + + +analytics.register(SentryAppInstallationUpdatedEvent) diff --git a/src/sentry/mediators/sentry_app_installations/updater.py b/src/sentry/mediators/sentry_app_installations/updater.py index fdf6dea211d629..0a7a6ef3caf9fa 100644 --- a/src/sentry/mediators/sentry_app_installations/updater.py +++ b/src/sentry/mediators/sentry_app_installations/updater.py @@ -3,7 +3,7 @@ import six -# from sentry import analytics +from sentry import analytics from sentry.constants import SentryAppInstallationStatus from sentry.mediators import Mediator, Param from sentry.mediators.param import if_param @@ -24,10 +24,10 @@ def _update_status(self): self.sentry_app_installation.status = SentryAppInstallationStatus.STATUS_MAP[self.status] def record_analytics(self): - pass # TODO: Add analytics - # analytics.record( - # 'sentry_app_installation.updated', - # user_id=self.user.id, - # sentry_app_installation=self.sentry_app_installation.id, - # ) + analytics.record( + 'sentry_app_installation.updated', + sentry_app_installation_id=self.sentry_app_installation.id, + sentry_app_id=self.sentry_app_installation.sentry_app.id, + organization_id=self.sentry_app_installation.organization.id + ) diff --git a/tests/sentry/api/endpoints/test_organization_sentry_app_installation_details.py b/tests/sentry/api/endpoints/test_organization_sentry_app_installation_details.py index 7c297c0e2d53c3..b7deae00696574 100644 --- a/tests/sentry/api/endpoints/test_organization_sentry_app_installation_details.py +++ b/tests/sentry/api/endpoints/test_organization_sentry_app_installation_details.py @@ -1,10 +1,11 @@ from __future__ import absolute_import from django.core.urlresolvers import reverse +from mock import patch from sentry.testutils import APITestCase import responses -from sentry.mediators.token_exchange import GrantExchanger, Refresher +from sentry.mediators.token_exchange import GrantExchanger from sentry.constants import SentryAppInstallationStatus @@ -111,7 +112,8 @@ def setUp(self): user=self.published_app.proxy_user, ) - def test_sentry_app_installation_mark_installed(self): + @patch('sentry.analytics.record') + def test_sentry_app_installation_mark_installed(self, record): self.url = reverse( 'sentry-api-0-sentry-app-installation-details', args=[self.installation.uuid], @@ -125,6 +127,13 @@ def test_sentry_app_installation_mark_installed(self): assert response.status_code == 200 assert response.data['status'] == 'installed' + record.assert_called_with( + 'sentry_app_installation.updated', + sentry_app_installation_id=self.installation.id, + sentry_app_id=self.installation.sentry_app.id, + organization_id=self.installation.organization.id + ) + def test_sentry_app_installation_mark_bad_status(self): self.url = reverse( 'sentry-api-0-sentry-app-installation-details', @@ -153,6 +162,24 @@ def test_sentry_app_installation_already_installed(self): HTTP_AUTHORIZATION=u'Bearer {}'.format(self.token.token), format='json', ) - print ("data", response.data) assert response.status_code == 400 assert response.data['status'][0] == "Cannot change installed integration to pending" + + def test_sentry_app_installation_mark_installed_wrong_app(self): + self.token = GrantExchanger.run( + install=self.installation2, + code=self.installation2.api_grant.code, + client_id=self.unpublished_app.application.client_id, + user=self.unpublished_app.proxy_user, + ) + self.url = reverse( + 'sentry-api-0-sentry-app-installation-details', + args=[self.installation.uuid], + ) + response = self.client.put( + self.url, + data={'status': 'installed'}, + HTTP_AUTHORIZATION=u'Bearer {}'.format(self.token.token), + format='json', + ) + assert response.status_code == 403 From 78db9c54ac30d9647da72d41222dfd463109f98b Mon Sep 17 00:00:00 2001 From: Stephen Cefali Date: Wed, 17 Jul 2019 11:01:38 -0700 Subject: [PATCH 5/7] update from PR comments --- src/sentry/api/bases/sentryapps.py | 5 ++--- .../rest_framework/sentry_app_installation.py | 12 +++++++----- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/sentry/api/bases/sentryapps.py b/src/sentry/api/bases/sentryapps.py index e89b1cc0e46852..ef443b4f5465c7 100644 --- a/src/sentry/api/bases/sentryapps.py +++ b/src/sentry/api/bases/sentryapps.py @@ -229,9 +229,8 @@ class SentryAppInstallationPermission(SentryPermission): def has_permission(self, request, *args, **kwargs): # To let the app mark the installation as installed, we don't care about permissions - if request.user.is_sentry_app: - if request.method == 'PUT': - return True + if request.user.is_sentry_app and request.method == 'PUT': + return True return super(SentryAppInstallationPermission, self).has_permission(request, *args, **kwargs) def has_object_permission(self, request, view, installation): diff --git a/src/sentry/api/serializers/rest_framework/sentry_app_installation.py b/src/sentry/api/serializers/rest_framework/sentry_app_installation.py index f1a8b52c97cb14..508ea5a01e8613 100644 --- a/src/sentry/api/serializers/rest_framework/sentry_app_installation.py +++ b/src/sentry/api/serializers/rest_framework/sentry_app_installation.py @@ -19,10 +19,12 @@ def validate_status(self, new_status): SentryAppInstallationStatus.STATUS_MAP.keys(), ), ) - # convert status to str for comparison - last_status = SentryAppInstallationStatus.as_str(self.instance.status) - # make sure we don't go from installed to pending - if last_status == SentryAppInstallationStatus.INSTALLED_STR and new_status == SentryAppInstallationStatus.PENDING_STR: - raise ValidationError('Cannot change installed integration to pending') + + if self.instance: + # convert status to str for comparison + last_status = SentryAppInstallationStatus.as_str(self.instance.status) + # make sure we don't go from installed to pending + if last_status == SentryAppInstallationStatus.INSTALLED_STR and new_status == SentryAppInstallationStatus.PENDING_STR: + raise ValidationError('Cannot change installed integration to pending') return new_status From 2f5d83d1e033baa1ff48a82a4d93706035303ed3 Mon Sep 17 00:00:00 2001 From: Stephen Cefali Date: Wed, 17 Jul 2019 11:49:57 -0700 Subject: [PATCH 6/7] update tests and address PR comments --- .../rest_framework/sentry_app_installation.py | 12 ++++-------- src/sentry/constants.py | 4 ---- .../mediators/sentry_app_installations/updater.py | 3 ++- .../test_organization_sentry_app_installations.py | 3 +++ .../test_installation_notifier.py | 2 ++ 5 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/sentry/api/serializers/rest_framework/sentry_app_installation.py b/src/sentry/api/serializers/rest_framework/sentry_app_installation.py index 508ea5a01e8613..5d89fd7d9e249c 100644 --- a/src/sentry/api/serializers/rest_framework/sentry_app_installation.py +++ b/src/sentry/api/serializers/rest_framework/sentry_app_installation.py @@ -10,15 +10,11 @@ class SentryAppInstallationSerializer(Serializer): status = serializers.CharField() def validate_status(self, new_status): - # make sure the status in in our defined list - try: - SentryAppInstallationStatus.STATUS_MAP[new_status] - except KeyError: + # can only set status to installed + if new_status != SentryAppInstallationStatus.INSTALLED_STR: raise ValidationError( - 'Invalid value for status. Valid values: {}'.format( - SentryAppInstallationStatus.STATUS_MAP.keys(), - ), - ) + u"Invalid value '{}' for status. Valid values: '{}'".format( + new_status, SentryAppInstallationStatus.INSTALLED_STR)) if self.instance: # convert status to str for comparison diff --git a/src/sentry/constants.py b/src/sentry/constants.py index a2bddc93ec60af..dcddf7da54892c 100644 --- a/src/sentry/constants.py +++ b/src/sentry/constants.py @@ -404,10 +404,6 @@ class SentryAppInstallationStatus(object): INSTALLED = 1 PENDING_STR = 'pending' INSTALLED_STR = 'installed' - STATUS_MAP = OrderedDict([ - (INSTALLED_STR, INSTALLED), - (PENDING_STR, PENDING), - ]) @classmethod def as_choices(cls): diff --git a/src/sentry/mediators/sentry_app_installations/updater.py b/src/sentry/mediators/sentry_app_installations/updater.py index 0a7a6ef3caf9fa..8895ef7a9aeada 100644 --- a/src/sentry/mediators/sentry_app_installations/updater.py +++ b/src/sentry/mediators/sentry_app_installations/updater.py @@ -21,7 +21,8 @@ def call(self): @if_param('status') def _update_status(self): # convert from string to integer - self.sentry_app_installation.status = SentryAppInstallationStatus.STATUS_MAP[self.status] + if self.status == SentryAppInstallationStatus.INSTALLED_STR: + self.sentry_app_installation.status = SentryAppInstallationStatus.INSTALLED def record_analytics(self): # TODO: Add analytics diff --git a/tests/sentry/api/endpoints/test_organization_sentry_app_installations.py b/tests/sentry/api/endpoints/test_organization_sentry_app_installations.py index 7f5be4fbdbb491..05af48e586756e 100644 --- a/tests/sentry/api/endpoints/test_organization_sentry_app_installations.py +++ b/tests/sentry/api/endpoints/test_organization_sentry_app_installations.py @@ -58,6 +58,7 @@ def test_superuser_sees_all_installs(self): }, 'uuid': self.installation2.uuid, 'code': self.installation2.api_grant.code, + 'status': 'pending', }] url = reverse( @@ -78,6 +79,7 @@ def test_superuser_sees_all_installs(self): }, 'uuid': self.installation.uuid, 'code': self.installation.api_grant.code, + 'status': 'pending' }] def test_users_only_sees_installs_on_their_org(self): @@ -95,6 +97,7 @@ def test_users_only_sees_installs_on_their_org(self): }, 'uuid': self.installation2.uuid, 'code': self.installation2.api_grant.code, + 'status': 'pending', }] # Org the User is not a part of diff --git a/tests/sentry/mediators/sentry_app_installations/test_installation_notifier.py b/tests/sentry/mediators/sentry_app_installations/test_installation_notifier.py index 27be975ce224e8..a1aed14014e00d 100644 --- a/tests/sentry/mediators/sentry_app_installations/test_installation_notifier.py +++ b/tests/sentry/mediators/sentry_app_installations/test_installation_notifier.py @@ -63,6 +63,7 @@ def test_task_enqueued(self, safe_urlopen): }, 'uuid': self.install.uuid, 'code': self.install.api_grant.code, + 'status': 'pending' }, }, 'actor': { @@ -106,6 +107,7 @@ def test_uninstallation_enqueued(self, safe_urlopen): }, 'uuid': self.install.uuid, 'code': self.install.api_grant.code, + 'status': 'pending' }, }, 'actor': { From c995b5e37623c1dda6ae40db40858dad28196c78 Mon Sep 17 00:00:00 2001 From: Stephen Cefali Date: Wed, 17 Jul 2019 12:46:01 -0700 Subject: [PATCH 7/7] update tests --- .../rest_framework/sentry_app_installation.py | 7 ------ ...ization_sentry_app_installation_details.py | 22 ++----------------- 2 files changed, 2 insertions(+), 27 deletions(-) diff --git a/src/sentry/api/serializers/rest_framework/sentry_app_installation.py b/src/sentry/api/serializers/rest_framework/sentry_app_installation.py index 5d89fd7d9e249c..97c798ac126112 100644 --- a/src/sentry/api/serializers/rest_framework/sentry_app_installation.py +++ b/src/sentry/api/serializers/rest_framework/sentry_app_installation.py @@ -16,11 +16,4 @@ def validate_status(self, new_status): u"Invalid value '{}' for status. Valid values: '{}'".format( new_status, SentryAppInstallationStatus.INSTALLED_STR)) - if self.instance: - # convert status to str for comparison - last_status = SentryAppInstallationStatus.as_str(self.instance.status) - # make sure we don't go from installed to pending - if last_status == SentryAppInstallationStatus.INSTALLED_STR and new_status == SentryAppInstallationStatus.PENDING_STR: - raise ValidationError('Cannot change installed integration to pending') - return new_status diff --git a/tests/sentry/api/endpoints/test_organization_sentry_app_installation_details.py b/tests/sentry/api/endpoints/test_organization_sentry_app_installation_details.py index b7deae00696574..4a61d1e5e97103 100644 --- a/tests/sentry/api/endpoints/test_organization_sentry_app_installation_details.py +++ b/tests/sentry/api/endpoints/test_organization_sentry_app_installation_details.py @@ -6,7 +6,6 @@ from sentry.testutils import APITestCase import responses from sentry.mediators.token_exchange import GrantExchanger -from sentry.constants import SentryAppInstallationStatus class SentryAppInstallationDetailsTest(APITestCase): @@ -134,24 +133,7 @@ def test_sentry_app_installation_mark_installed(self, record): organization_id=self.installation.organization.id ) - def test_sentry_app_installation_mark_bad_status(self): - self.url = reverse( - 'sentry-api-0-sentry-app-installation-details', - args=[self.installation.uuid], - ) - response = self.client.put( - self.url, - data={'status': 'other'}, - HTTP_AUTHORIZATION=u'Bearer {}'.format(self.token.token), - format='json', - ) - assert response.status_code == 400 - assert response.data['status'][0] == u"Invalid value for status. Valid values: ['installed', 'pending']" - - def test_sentry_app_installation_already_installed(self): - self.installation.status = SentryAppInstallationStatus.INSTALLED - self.installation.save() - + def test_sentry_app_installation_mark_pending_status(self): self.url = reverse( 'sentry-api-0-sentry-app-installation-details', args=[self.installation.uuid], @@ -163,7 +145,7 @@ def test_sentry_app_installation_already_installed(self): format='json', ) assert response.status_code == 400 - assert response.data['status'][0] == "Cannot change installed integration to pending" + assert response.data['status'][0] == u"Invalid value 'pending' for status. Valid values: 'installed'" def test_sentry_app_installation_mark_installed_wrong_app(self): self.token = GrantExchanger.run(