Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
16 changes: 16 additions & 0 deletions src/sentry/analytics/events/sentry_app_installation_updated.py
Original file line number Diff line number Diff line change
@@ -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)
10 changes: 10 additions & 0 deletions src/sentry/api/bases/sentryapps.py
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,12 @@ class SentryAppInstallationPermission(SentryPermission):
'POST': ('org:integrations', 'event:write', 'event:admin'),
}

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 and request.method == 'PUT':
return True
return super(SentryAppInstallationPermission, self).has_permission(request, *args, **kwargs)

def has_object_permission(self, request, view, installation):
if not hasattr(request, 'user') or not request.user:
return False
Expand All @@ -236,6 +242,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

Expand Down
22 changes: 21 additions & 1 deletion src/sentry/api/endpoints/sentry_app_installation_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -19,3 +20,22 @@ def delete(self, request, installation):
request=request,
)
return Response(status=204)

def put(self, request, installation):
serializer = SentryAppInstallationSerializer(
installation,
data=request.data,
partial=True,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need the partial=True here since we expect the status to be in the request.data and that's the only thing you are validating

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be good to keep it, though. Updating is almost always going to allow only a portion of the required data (opposed to requiring all fields).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put it back

)

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)
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

from sentry.api.serializers import Serializer, register
from sentry.models import SentryAppInstallation
from sentry.constants import SentryAppInstallationStatus


@register(SentryAppInstallation)
Expand All @@ -17,6 +18,7 @@ def serialize(self, install, attrs, user):
'slug': install.organization.slug,
},
'uuid': install.uuid,
'status': SentryAppInstallationStatus.as_str(install.status),
}

if install.api_grant:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
from __future__ import absolute_import


from rest_framework import serializers
from rest_framework.serializers import Serializer, ValidationError
from sentry.constants import SentryAppInstallationStatus


class SentryAppInstallationSerializer(Serializer):
status = serializers.CharField()

def validate_status(self, new_status):
# can only set status to installed
if new_status != SentryAppInstallationStatus.INSTALLED_STR:
raise ValidationError(
u"Invalid value '{}' for status. Valid values: '{}'".format(
new_status, SentryAppInstallationStatus.INSTALLED_STR))

return new_status
10 changes: 6 additions & 4 deletions src/sentry/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -402,20 +402,22 @@ def as_str(cls, status):
class SentryAppInstallationStatus(object):
PENDING = 0
INSTALLED = 1
PENDING_STR = 'pending'
INSTALLED_STR = 'installed'

@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'))
Expand Down
1 change: 1 addition & 0 deletions src/sentry/mediators/sentry_app_installations/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
34 changes: 34 additions & 0 deletions src/sentry/mediators/sentry_app_installations/updater.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
from __future__ import absolute_import

import six


from sentry import analytics
from sentry.constants import SentryAppInstallationStatus
from sentry.mediators import Mediator, Param
from sentry.mediators.param import if_param


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):
# convert from string to integer
if self.status == SentryAppInstallationStatus.INSTALLED_STR:
self.sentry_app_installation.status = SentryAppInstallationStatus.INSTALLED

def record_analytics(self):
# TODO: Add analytics
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
)
Original file line number Diff line number Diff line change
@@ -1,9 +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


class SentryAppInstallationDetailsTest(APITestCase):
Expand All @@ -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(
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -95,3 +99,69 @@ 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,
)

@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],
)
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'

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_pending_status(self):
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',
)
assert response.status_code == 400
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(
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
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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):
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ def test_task_enqueued(self, safe_urlopen):
},
'uuid': self.install.uuid,
'code': self.install.api_grant.code,
'status': 'pending'
},
},
'actor': {
Expand Down Expand Up @@ -106,6 +107,7 @@ def test_uninstallation_enqueued(self, safe_urlopen):
},
'uuid': self.install.uuid,
'code': self.install.api_grant.code,
'status': 'pending'
},
},
'actor': {
Expand Down