From b11debcce2d68d239218920f05dbc70bad8c5c53 Mon Sep 17 00:00:00 2001 From: Evan Purkhiser Date: Mon, 20 May 2019 15:13:02 -0700 Subject: [PATCH] ref(slack): Implement bot app switch (#13184) Introduces as SLACK_INTEGRATION_USER_WST setting that can be enabled to use the legacy workspace token apps (which is what sentry.io uses). - When installing the integration we now use scopes depending on if the workspace token app setting is enabled. Otherwise we use bot tokens. NOTE that the WST app scopes have not changed. - When using bot tokens we must store the bot token _and_ user token to handle unfurling. - We can still pre-link the authorizing user, but we have to make a API request first to receive the users ID. --- src/sentry/conf/server.py | 5 ++ src/sentry/identity/slack/provider.py | 14 +++- .../integrations/slack/event_endpoint.py | 9 +- src/sentry/integrations/slack/integration.py | 48 +++++++++-- .../integrations/slack/test_event_endpoint.py | 3 +- .../integrations/slack/test_integration.py | 83 ++++++++++++++++--- .../integrations/slack/test_notify_action.py | 1 - 7 files changed, 136 insertions(+), 27 deletions(-) diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index 7c897860ce8880..a09c936de398b3 100644 --- a/src/sentry/conf/server.py +++ b/src/sentry/conf/server.py @@ -1606,3 +1606,8 @@ def get_sentry_sdk_config(): 'topic': KAFKA_EVENTS, }, } + +# Enable this to use the legacy Slack Workspace Token apps. You will likely +# never need to switch this unless you created a workspace app before slack +# disabled them. +SLACK_INTEGRATION_USE_WST = False diff --git a/src/sentry/identity/slack/provider.py b/src/sentry/identity/slack/provider.py index c989ae973a2d55..2d08299bf7931f 100644 --- a/src/sentry/identity/slack/provider.py +++ b/src/sentry/identity/slack/provider.py @@ -8,10 +8,9 @@ class SlackIdentityProvider(OAuth2Provider): key = 'slack' name = 'Slack' - # TODO(epurkhiser): This identity provider is actually used for authorizing - # the Slack application through their Workspace Token OAuth flow, not a - # standard user access token flow. - oauth_access_token_url = 'https://slack.com/api/oauth.token' + # This identity provider is used for authorizing the Slack application + # through their Bot token (or legacy Workspace Token if enabled) flow. + oauth_access_token_url = 'https://slack.com/api/oauth.access' oauth_authorize_url = 'https://slack.com/oauth/authorize' oauth_scopes = ( @@ -25,11 +24,18 @@ def get_oauth_client_id(self): def get_oauth_client_secret(self): return options.get('slack.client-secret') + def get_oauth_data(self, payload): + # TODO(epurkhiser): This flow isn't actually used right now in sentry. + # In slack-bot world we would need to make an API call to the 'me' + # endpoint to get their user ID here. + return super(SlackIdentityProvider, self).get_oauth_data(self, payload) + def build_identity(self, data): data = data['data'] return { 'type': 'slack', + # TODO(epurkhiser): See note above 'id': data['user']['id'], 'email': data['user']['email'], 'scopes': sorted(data['scope'].split(',')), diff --git a/src/sentry/integrations/slack/event_endpoint.py b/src/sentry/integrations/slack/event_endpoint.py index 6d57f7234440bf..8c20b7464a58de 100644 --- a/src/sentry/integrations/slack/event_endpoint.py +++ b/src/sentry/integrations/slack/event_endpoint.py @@ -4,6 +4,8 @@ import re import six +from django.conf import settings + from sentry import http from sentry.api.base import Endpoint from sentry.models import Group, Project @@ -58,8 +60,13 @@ def on_link_shared(self, request, integration, token, data): if not results: return + if settings.SLACK_INTEGRATION_USE_WST: + access_token = integration.metadata['access_token'], + else: + access_token = integration.metadata['user_access_token'], + payload = { - 'token': integration.metadata['access_token'], + 'token': access_token, 'channel': data['channel'], 'ts': data['message_ts'], 'unfurls': json.dumps({ diff --git a/src/sentry/integrations/slack/integration.py b/src/sentry/integrations/slack/integration.py index 7646c81656ee4e..0926a1132dd7fe 100644 --- a/src/sentry/integrations/slack/integration.py +++ b/src/sentry/integrations/slack/integration.py @@ -1,6 +1,7 @@ from __future__ import absolute_import from django.utils.translation import ugettext_lazy as _ +from django.conf import settings from sentry import http from sentry.identity.pipeline import IdentityProviderPipeline @@ -68,7 +69,12 @@ class SlackIntegrationProvider(IntegrationProvider): IntegrationFeatures.ALERT_RULE, ]) + # Scopes differ depending on if it's a workspace app identity_oauth_scopes = frozenset([ + 'bot', + 'links:read', + 'links:write', + ]) if not settings.SLACK_INTEGRATION_USE_WST else frozenset([ 'channels:read', 'groups:read', 'users:read', @@ -110,25 +116,51 @@ def get_team_info(self, access_token): return resp['team'] + def get_identity(self, user_token): + payload = { + 'token': user_token, + } + + session = http.build_session() + resp = session.get('https://slack.com/api/auth.test', params=payload) + resp.raise_for_status() + resp = resp.json() + + return resp['user_id'] + def build_integration(self, state): data = state['identity']['data'] assert data['ok'] + if settings.SLACK_INTEGRATION_USE_WST: + access_token = data['access_token'] + user_id_slack = data['authorizing_user_id'] + else: + access_token = data['bot']['bot_access_token'] + user_id_slack = self.get_identity(data['access_token']) + scopes = sorted(self.identity_oauth_scopes) - team_data = self.get_team_info(data['access_token']) + team_data = self.get_team_info(access_token) + + metadata = { + 'access_token': access_token, + 'scopes': scopes, + 'icon': team_data['icon']['image_132'], + 'domain_name': team_data['domain'] + '.slack.com', + } + + # When using bot tokens, we must use the user auth token for URL + # unfurling + if not settings.SLACK_INTEGRATION_USE_WST: + metadata['user_access_token'] = data['access_token'] return { 'name': data['team_name'], 'external_id': data['team_id'], - 'metadata': { - 'access_token': data['access_token'], - 'scopes': scopes, - 'icon': team_data['icon']['image_132'], - 'domain_name': team_data['domain'] + '.slack.com', - }, + 'metadata': metadata, 'user_identity': { 'type': 'slack', - 'external_id': data['authorizing_user_id'], + 'external_id': user_id_slack, 'scopes': [], 'data': {}, }, diff --git a/tests/sentry/integrations/slack/test_event_endpoint.py b/tests/sentry/integrations/slack/test_event_endpoint.py index f68cfd93cb22dd..fc7a7f1b04e8e1 100644 --- a/tests/sentry/integrations/slack/test_event_endpoint.py +++ b/tests/sentry/integrations/slack/test_event_endpoint.py @@ -2,6 +2,7 @@ import json import responses +from django.test.utils import override_settings from sentry import options from sentry.models import Integration, OrganizationIntegration @@ -40,6 +41,7 @@ }""" +@override_settings(SLACK_INTEGRATION_USE_WST=True) class BaseEventTest(APITestCase): def setUp(self): super(BaseEventTest, self).setUp() @@ -50,7 +52,6 @@ def setUp(self): external_id='TXXXXXXX1', metadata={ 'access_token': 'xoxp-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx', - 'bot_access_token': 'xoxb-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx', } ) OrganizationIntegration.objects.create( diff --git a/tests/sentry/integrations/slack/test_integration.py b/tests/sentry/integrations/slack/test_integration.py index 52a077fa3bc578..9613ce8bdc62ef 100644 --- a/tests/sentry/integrations/slack/test_integration.py +++ b/tests/sentry/integrations/slack/test_integration.py @@ -3,6 +3,7 @@ import responses import six +from django.test.utils import override_settings from six.moves.urllib.parse import parse_qs, urlencode, urlparse from sentry.integrations.slack import SlackIntegrationProvider @@ -13,7 +14,8 @@ class SlackIntegrationTest(IntegrationTestCase): provider = SlackIntegrationProvider - def assert_setup_flow(self, team_id='TXXXXXXX1', authorizing_user_id='UXXXXXXX1'): + def assert_setup_flow(self, team_id='TXXXXXXX1', authorizing_user_id='UXXXXXXX1', + access_token='xoxp-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx', access_extras=None): responses.reset() resp = self.client.get(self.init_path) @@ -32,14 +34,27 @@ def assert_setup_flow(self, team_id='TXXXXXXX1', authorizing_user_id='UXXXXXXX1' # easier authorize_params = {k: v[0] for k, v in six.iteritems(params)} + access_json = { + 'ok': True, + 'access_token': access_token, + 'team_id': team_id, + 'team_name': 'Example', + 'authorizing_user_id': authorizing_user_id, + } + + if access_extras is not None: + access_json.update(access_extras) + responses.add( - responses.POST, 'https://slack.com/api/oauth.token', + responses.POST, 'https://slack.com/api/oauth.access', + json=access_json, + ) + + responses.add( + responses.GET, 'https://slack.com/api/auth.test', json={ 'ok': True, - 'access_token': 'xoxp-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx', - 'team_id': team_id, - 'team_name': 'Example', - 'authorizing_user_id': authorizing_user_id, + 'user_id': authorizing_user_id, } ) @@ -74,8 +89,50 @@ def assert_setup_flow(self, team_id='TXXXXXXX1', authorizing_user_id='UXXXXXXX1' self.assertDialogSuccess(resp) @responses.activate - def test_basic_flow(self): - self.assert_setup_flow() + def test_bot_flow(self): + self.assert_setup_flow( + access_token='xoxa-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx', + access_extras={ + 'bot': { + 'bot_access_token': 'xoxb-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx', + } + }) + + integration = Integration.objects.get(provider=self.provider.key) + assert integration.external_id == 'TXXXXXXX1' + assert integration.name == 'Example' + assert integration.metadata == { + 'access_token': 'xoxb-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx', + 'user_access_token': 'xoxa-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx', + 'scopes': sorted(self.provider.identity_oauth_scopes), + 'icon': 'http://example.com/ws_icon.jpg', + 'domain_name': 'test-slack-workspace.slack.com', + } + oi = OrganizationIntegration.objects.get( + integration=integration, + organization=self.organization, + ) + assert oi.config == {} + + idp = IdentityProvider.objects.get( + type='slack', + external_id='TXXXXXXX1', + ) + identity = Identity.objects.get( + idp=idp, + user=self.user, + external_id='UXXXXXXX1', + ) + assert identity.status == IdentityStatus.VALID + + @override_settings(SLACK_INTEGRATION_USE_WST=True) + def assert_wst_setup_flow(self, *args, **kwargs): + self.assert_setup_flow(*args, **kwargs) + + @responses.activate + @override_settings(SLACK_INTEGRATION_USE_WST=True) + def test_wst_flow(self): + self.assert_wst_setup_flow() integration = Integration.objects.get(provider=self.provider.key) assert integration.external_id == 'TXXXXXXX1' @@ -104,9 +161,10 @@ def test_basic_flow(self): assert identity.status == IdentityStatus.VALID @responses.activate + @override_settings(SLACK_INTEGRATION_USE_WST=True) def test_multiple_integrations(self): - self.assert_setup_flow() - self.assert_setup_flow(team_id='TXXXXXXX2', authorizing_user_id='UXXXXXXX2') + self.assert_wst_setup_flow() + self.assert_wst_setup_flow(team_id='TXXXXXXX2', authorizing_user_id='UXXXXXXX2') integrations = Integration.objects.filter(provider=self.provider.key) @@ -131,11 +189,12 @@ def test_multiple_integrations(self): assert identities[0].idp != identities[1].idp @responses.activate + @override_settings(SLACK_INTEGRATION_USE_WST=True) def test_reassign_user(self): - self.assert_setup_flow() + self.assert_wst_setup_flow() identity = Identity.objects.get() assert identity.external_id == 'UXXXXXXX1' - self.assert_setup_flow(authorizing_user_id='UXXXXXXX2') + self.assert_wst_setup_flow(authorizing_user_id='UXXXXXXX2') identity = Identity.objects.get() assert identity.external_id == 'UXXXXXXX2' diff --git a/tests/sentry/integrations/slack/test_notify_action.py b/tests/sentry/integrations/slack/test_notify_action.py index a879cb6ed04f46..127ffa5861239b 100644 --- a/tests/sentry/integrations/slack/test_notify_action.py +++ b/tests/sentry/integrations/slack/test_notify_action.py @@ -22,7 +22,6 @@ def setUp(self): external_id='TXXXXXXX1', metadata={ 'access_token': 'xoxp-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx', - 'bot_access_token': 'xoxb-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx', } ) self.integration.add_organization(event.project.organization, self.user)