fix(hybridcloud) Don't use integration proxy for slack (#65861)

markstory · web-flow · commit 0daa3869a435 · 2024-03-01T09:57:16.000-08:00
Slack doesn't use refresh tokens and thus doesn't need to go through the
integration proxy as there are no refresh tokens to coordinate.
diff --git a/src/sentry/integrations/slack/client.py b/src/sentry/integrations/slack/client.py
@@ -7,10 +7,10 @@
 from requests import PreparedRequest, Response
 
 from sentry.constants import ObjectStatus
-from sentry.models.integrations.integration import Integration
-from sentry.services.hybrid_cloud.util import control_silo_function
+from sentry.db.postgres.transactions import in_test_hide_transaction_boundary
+from sentry.integrations.client import ApiClient
+from sentry.services.hybrid_cloud.integration import integration_service
 from sentry.shared_integrations.client import BaseApiResponse
-from sentry.shared_integrations.client.proxy import IntegrationProxyClient, infer_org_integration
 from sentry.shared_integrations.exceptions import ApiError
 from sentry.types.integrations import EXTERNAL_PROVIDERS, ExternalProviders
 from sentry.utils import json, metrics
@@ -19,7 +19,7 @@
 logger = logging.getLogger(__name__)
 
 
-class SlackClient(IntegrationProxyClient):
+class SlackClient(ApiClient):
     allow_redirects = False
     integration_name = "slack"
     base_url = "https://slack.com/api"
@@ -28,37 +28,34 @@ class SlackClient(IntegrationProxyClient):
     def __init__(
         self,
         integration_id: int | None = None,
-        org_integration_id: int | None = None,
         verify_ssl: bool = True,
         logging_context: Mapping[str, Any] | None = None,
+        org_integration_id: int | None = None,  # deprecated but used by getsentry
     ) -> None:
         self.integration_id = integration_id
-        if not org_integration_id and integration_id is not None:
-            org_integration_id = infer_org_integration(
-                integration_id=self.integration_id, ctx_logger=logger
-            )
 
         super().__init__(
-            org_integration_id=org_integration_id,
             verify_ssl=verify_ssl,
             integration_id=integration_id,
             logging_context=logging_context,
         )
 
-    @control_silo_function
     def authorize_request(self, prepared_request: PreparedRequest) -> PreparedRequest:
-        integration = None
-        base_qs = {
-            "provider": EXTERNAL_PROVIDERS[ExternalProviders.SLACK],
-            "status": ObjectStatus.ACTIVE,
-        }
-        if self.integration_id:
-            integration = Integration.objects.filter(id=self.integration_id, **base_qs).first()
-        elif self.org_integration_id:
-            integration = Integration.objects.filter(
-                organizationintegration__id=self.org_integration_id, **base_qs
-            ).first()
+        if "Authorization" in prepared_request.headers or not self.integration_id:
+            return prepared_request
 
+        # TODO(hybridcloud) Pass integration into SlackClient.__init__() so
+        # we don't have to workaround watermarks here.
+        # In order to send requests, SlackClient needs to fetch the integration
+        # to get access tokens which trips up rpc method/transaction
+        # boundary detection. Those boundaries are not relevant because
+        # this is a read operation.
+        with in_test_hide_transaction_boundary():
+            integration = integration_service.get_integration(
+                integration_id=self.integration_id,
+                provider=EXTERNAL_PROVIDERS[ExternalProviders.SLACK],
+                status=ObjectStatus.ACTIVE,
+            )
         if not integration:
             logger.info("no_integration", extra={"path_url": prepared_request.path_url})
             return prepared_request
@@ -68,6 +65,12 @@ def authorize_request(self, prepared_request: PreparedRequest) -> PreparedReques
         prepared_request.headers["Authorization"] = f"Bearer {token}"
         return prepared_request
 
+    def finalize_request(self, prepared_request: PreparedRequest) -> PreparedRequest:
+        """
+        Add request authorization headers
+        """
+        return self.authorize_request(prepared_request=prepared_request)
+
     def is_response_fatal(self, response: Response) -> bool:
         try:
             resp_json = response.json()
diff --git a/src/sentry/integrations/slack/integration.py b/src/sentry/integrations/slack/integration.py
@@ -71,9 +71,7 @@
 
 class SlackIntegration(SlackNotifyBasicMixin, IntegrationInstallation):
     def get_client(self) -> SlackClient:
-        if not self.org_integration:
-            raise IntegrationError("Organization Integration does not exist")
-        return SlackClient(org_integration_id=self.org_integration.id, integration_id=self.model.id)
+        return SlackClient(integration_id=self.model.id)
 
     def get_config_data(self) -> Mapping[str, str]:
         metadata_ = self.model.metadata
diff --git a/tests/sentry/integrations/slack/test_client.py b/tests/sentry/integrations/slack/test_client.py
@@ -1,15 +1,12 @@
 import re
 
 import responses
-from django.test import override_settings
 from responses import matchers
 
 from sentry.integrations.slack.client import SlackClient
 from sentry.silo.base import SiloMode
-from sentry.silo.util import PROXY_BASE_PATH, PROXY_OI_HEADER, PROXY_PATH, PROXY_SIGNATURE_HEADER
 from sentry.testutils.cases import TestCase
 from sentry.testutils.silo import assume_test_silo_mode, region_silo_test
-from tests.sentry.integrations.test_helpers import add_control_silo_proxy_response
 
 
 @region_silo_test
@@ -39,14 +36,6 @@ def _add_response(*, json, match):
                 match=match,
             )
 
-            add_control_silo_proxy_response(
-                method=responses.POST,
-                path="chat.postMessage",
-                status=200,
-                json=json,
-                additional_matchers=match,
-            )
-
         _add_response(
             json=self.mock_user_access_token_response,
             match=[
@@ -64,47 +53,6 @@ def _add_response(*, json, match):
             match=[matchers.header_matcher({})],
         )
 
-    def assert_proxy_request(self, request, is_proxy=True):
-        assert (PROXY_BASE_PATH in request.url) == is_proxy
-        assert (PROXY_OI_HEADER in request.headers) == is_proxy
-        assert (PROXY_SIGNATURE_HEADER in request.headers) == is_proxy
-        if is_proxy:
-            assert request.headers[PROXY_OI_HEADER] is not None
-
-    @responses.activate
-    def test_integration_proxy_is_active(self):
-        class SlackProxyTestClient(SlackClient):
-            _use_proxy_url_for_tests = True
-
-        with override_settings(SILO_MODE=SiloMode.MONOLITH):
-            client = SlackProxyTestClient(integration_id=self.integration.id)
-            client.post("/chat.postMessage", data=self.payload)
-            request = responses.calls[0].request
-
-            assert "/chat.postMessage" in request.url
-            assert client.base_url in request.url
-            self.assert_proxy_request(request, is_proxy=False)
-
-        responses.calls.reset()
-        with override_settings(SILO_MODE=SiloMode.CONTROL):
-            client = SlackProxyTestClient(integration_id=self.integration.id)
-            client.post("/chat.postMessage", data=self.payload)
-            request = responses.calls[0].request
-
-            assert "/chat.postMessage" in request.url
-            assert client.base_url in request.url
-            self.assert_proxy_request(request, is_proxy=False)
-
-        responses.calls.reset()
-        with override_settings(SILO_MODE=SiloMode.REGION):
-            client = SlackProxyTestClient(integration_id=self.integration.id)
-            client.post("/chat.postMessage", data=self.payload)
-            request = responses.calls[0].request
-
-            assert request.headers[PROXY_PATH] == "chat.postMessage"
-            assert client.base_url not in request.url
-            self.assert_proxy_request(request, is_proxy=True)
-
     @responses.activate
     def test_authorize_with_no_id_noop(self):
         client = SlackClient()
@@ -121,13 +69,6 @@ def test_authorize_manually(self):
         )
         assert response == self.mock_user_access_token_response
 
-    @responses.activate
-    @assume_test_silo_mode(SiloMode.CONTROL)
-    def test_authorize_with_org_integration_id(self):
-        client = SlackClient(org_integration_id=self.organization_integration.id)
-        response = client.post("/chat.postMessage", data=self.payload)
-        assert response == self.mock_access_token_response
-
     @responses.activate
     @assume_test_silo_mode(SiloMode.CONTROL)
     def test_authorize_with_integration_id(self):
@@ -139,22 +80,18 @@ def test_authorize_with_integration_id(self):
     @assume_test_silo_mode(SiloMode.CONTROL)
     def test_authorize_user_access_token(self):
         self.integration.update(metadata={"user_access_token": self.user_access_token})
-        client = SlackClient(org_integration_id=self.organization_integration.id)
+        client = SlackClient(integration_id=self.integration.id)
         response = client.post("/chat.postMessage", data=self.payload)
         assert response == self.mock_user_access_token_response
 
     @responses.activate
     def test_no_authorization_in_region_mode(self):
-        client = SlackClient(org_integration_id=self.organization_integration.id)
-        response = client.post("/chat.postMessage", data=self.payload)
-        assert response == self.mock_not_authed_response
-
         client = SlackClient(integration_id=self.integration.id)
         response = client.post("/chat.postMessage", data=self.payload)
-        assert response == self.mock_not_authed_response
+        assert response == self.mock_access_token_response
 
         with assume_test_silo_mode(SiloMode.CONTROL):
             self.integration.update(metadata={"user_access_token": self.user_access_token})
-        client = SlackClient(org_integration_id=self.organization_integration.id)
+        client = SlackClient(integration_id=self.integration.id)
         response = client.post("/chat.postMessage", data=self.payload)
-        assert response == self.mock_not_authed_response
+        assert response == self.mock_user_access_token_response
diff --git a/tests/sentry/integrations/slack/webhooks/events/test_message_im.py b/tests/sentry/integrations/slack/webhooks/events/test_message_im.py
@@ -2,7 +2,6 @@
 
 from sentry.models.identity import Identity, IdentityStatus
 from sentry.silo import SiloMode
-from sentry.silo.util import PROXY_BASE_URL_HEADER, PROXY_OI_HEADER, PROXY_SIGNATURE_HEADER
 from sentry.testutils.cases import IntegratedApiTestCase
 from sentry.testutils.helpers import get_response_text
 from sentry.testutils.silo import assume_test_silo_mode, region_silo_test
@@ -66,34 +65,22 @@ def test_identifying_channel_correctly(self):
         data = json.loads(request.body)
         assert data.get("channel") == event_data["channel"]
 
-    def _check_proxying(self) -> None:
-        assert len(responses.calls) == 1
-        request = responses.calls[0].request
-        assert request.headers[PROXY_OI_HEADER] == str(self.organization_integration.id)
-        assert request.headers[PROXY_BASE_URL_HEADER] == "https://slack.com/api"
-        assert PROXY_SIGNATURE_HEADER in request.headers
-
     @responses.activate
     def test_user_message_im_notification_platform(self):
         responses.add(responses.POST, "https://slack.com/api/chat.postMessage", json={"ok": True})
         resp = self.post_webhook(event_data=json.loads(MESSAGE_IM_EVENT))
         assert resp.status_code == 200, resp.content
 
-        if self.should_call_api_without_proxying():
-            assert len(responses.calls) == 1
-            request = responses.calls[0].request
-            assert (
-                request.headers["Authorization"] == "Bearer xoxb-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx"
-            )
-            data = json.loads(request.body)
-            heading, contents = self.get_block_section_text(data)
-            assert heading == "Unknown command: `helloo`"
-            assert (
-                contents
-                == "Here are the commands you can use. Commands not working? Re-install the app!"
-            )
-        else:
-            self._check_proxying()
+        assert len(responses.calls) == 1
+        request = responses.calls[0].request
+        assert request.headers["Authorization"] == "Bearer xoxb-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx"
+        data = json.loads(request.body)
+        heading, contents = self.get_block_section_text(data)
+        assert heading == "Unknown command: `helloo`"
+        assert (
+            contents
+            == "Here are the commands you can use. Commands not working? Re-install the app!"
+        )
 
     @responses.activate
     def test_user_message_link(self):
@@ -107,16 +94,11 @@ def test_user_message_link(self):
         resp = self.post_webhook(event_data=json.loads(MESSAGE_IM_EVENT_LINK))
         assert resp.status_code == 200, resp.content
 
-        if self.should_call_api_without_proxying():
-            assert len(responses.calls) == 1
-            request = responses.calls[0].request
-            assert (
-                request.headers["Authorization"] == "Bearer xoxb-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx"
-            )
-            data = json.loads(request.body)
-            assert "Link your Slack identity" in get_response_text(data)
-        else:
-            self._check_proxying()
+        assert len(responses.calls) == 1
+        request = responses.calls[0].request
+        assert request.headers["Authorization"] == "Bearer xoxb-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx"
+        data = json.loads(request.body)
+        assert "Link your Slack identity" in get_response_text(data)
 
     @responses.activate
     def test_user_message_already_linked(self):
@@ -138,16 +120,11 @@ def test_user_message_already_linked(self):
         resp = self.post_webhook(event_data=json.loads(MESSAGE_IM_EVENT_LINK))
         assert resp.status_code == 200, resp.content
 
-        if self.should_call_api_without_proxying():
-            assert len(responses.calls) == 1
-            request = responses.calls[0].request
-            assert (
-                request.headers["Authorization"] == "Bearer xoxb-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx"
-            )
-            data = json.loads(request.body)
-            assert "You are already linked" in get_response_text(data)
-        else:
-            self._check_proxying()
+        assert len(responses.calls) == 1
+        request = responses.calls[0].request
+        assert request.headers["Authorization"] == "Bearer xoxb-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx"
+        data = json.loads(request.body)
+        assert "You are already linked" in get_response_text(data)
 
     @responses.activate
     def test_user_message_unlink(self):
@@ -168,16 +145,11 @@ def test_user_message_unlink(self):
         resp = self.post_webhook(event_data=json.loads(MESSAGE_IM_EVENT_UNLINK))
         assert resp.status_code == 200, resp.content
 
-        if self.should_call_api_without_proxying():
-            assert len(responses.calls) == 1
-            request = responses.calls[0].request
-            assert (
-                request.headers["Authorization"] == "Bearer xoxb-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx"
-            )
-            data = json.loads(request.body)
-            assert "Click here to unlink your identity" in get_response_text(data)
-        else:
-            self._check_proxying()
+        assert len(responses.calls) == 1
+        request = responses.calls[0].request
+        assert request.headers["Authorization"] == "Bearer xoxb-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx"
+        data = json.loads(request.body)
+        assert "Click here to unlink your identity" in get_response_text(data)
 
     @responses.activate
     def test_user_message_already_unlinked(self):
@@ -192,16 +164,11 @@ def test_user_message_already_unlinked(self):
         resp = self.post_webhook(event_data=json.loads(MESSAGE_IM_EVENT_UNLINK))
         assert resp.status_code == 200, resp.content
 
-        if self.should_call_api_without_proxying():
-            assert len(responses.calls) == 1
-            request = responses.calls[0].request
-            assert (
-                request.headers["Authorization"] == "Bearer xoxb-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx"
-            )
-            data = json.loads(request.body)
-            assert "You do not have a linked identity to unlink" in get_response_text(data)
-        else:
-            self._check_proxying()
+        assert len(responses.calls) == 1
+        request = responses.calls[0].request
+        assert request.headers["Authorization"] == "Bearer xoxb-xxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxx"
+        data = json.loads(request.body)
+        assert "You do not have a linked identity to unlink" in get_response_text(data)
 
     def test_bot_message_im(self):
         resp = self.post_webhook(event_data=json.loads(MESSAGE_IM_BOT_EVENT))
