Repository: getsentry/sentry
Pull request: feat(discord): Remove Discord bot during uninstall flow
We will deal with how to handle broken integrations later on, when we are actually trying to use the integration!

Problem statement:
Implement Discord integration uninstall flow
Discord has an API that will allow us to automatically remove the bot from the relevant server when the integration is removed on our end.

If the bot is removed from a server directly, there is no way for us to know this. Instead we will need to look out for problem response codes (401, 403, 404) while trying to interact with a given server. If these are coming up then the installation is likely broken.

Hints from issue discussion:

Commit message: Merge branch 'master' into spalmurray/discord-uninstall

Patch to review:
diff --git a/src/sentry/integrations/discord/client.py b/src/sentry/integrations/discord/client.py
index c85164f7213327..ddf59324714772 100644
--- a/src/sentry/integrations/discord/client.py
+++ b/src/sentry/integrations/discord/client.py
@@ -15,7 +15,10 @@ class DiscordClient(IntegrationProxyClient):
     base_url: str = "https://discord.com/api/v10"
 
     # https://discord.com/developers/docs/resources/guild#get-guild
-    get_guild_url = "/guilds/{guild_id}"
+    guild_url = "/guilds/{guild_id}"
+
+    # https://discord.com/developers/docs/resources/user#leave-guild
+    users_guild_url = "/users/@me/guilds/{guild_id}"
 
     def __init__(
         self,
@@ -42,4 +45,10 @@ def get_guild_name(self, guild_id: str) -> str:
         """
         Normal version of get_guild_name that uses the regular auth flow.
         """
-        return self.get(self.get_guild_url.format(guild_id=guild_id))["name"]  # type:ignore
+        return self.get(self.guild_url.format(guild_id=guild_id))["name"]  # type:ignore
+
+    def leave_guild(self, guild_id: str) -> None:
+        """
+        Leave the given guild_id, if the bot is currently a member.
+        """
+        self.delete(self.users_guild_url.format(guild_id=guild_id))
diff --git a/src/sentry/integrations/discord/integration.py b/src/sentry/integrations/discord/integration.py
index 70b985fb9369e6..4f9108b493e14f 100644
--- a/src/sentry/integrations/discord/integration.py
+++ b/src/sentry/integrations/discord/integration.py
@@ -39,7 +39,37 @@
 
 
 class DiscordIntegration(IntegrationInstallation):
-    pass
+    def get_client(self) -> DiscordClient:
+        org_integration_id = self.org_integration.id if self.org_integration else None
+
+        return DiscordClient(
+            integration_id=self.model.id,  # type:ignore
+            org_integration_id=org_integration_id,
+        )
+
+    def uninstall(self) -> None:
+        # If this is the only org using this Discord server, we should remove
+        # the bot from the server.
+        from sentry.services.hybrid_cloud.integration import integration_service
+
+        installations = integration_service.get_organization_integrations(
+            integration_id=self.model.id,
+            providers=["discord"],
+        )
+
+        # Remove any installations pending deletion
+        active_installations = [i for i in installations if i.status not in (2, 3)]
+
+        if len(active_installations) > 1:
+            return
+
+        client = self.get_client()
+        try:
+            client.leave_guild(str(self.model.external_id))
+        except ApiError:
+            # Most likely the bot has already been removed from the guild.
+            # In any case, we should just continue.
+            return
 
 
 class DiscordIntegrationProvider(IntegrationProvider):
@@ -71,7 +101,7 @@ def build_integration(self, state: Mapping[str, object]) -> Mapping[str, object]
 
     def get_guild_name(self, guild_id: str) -> str:
         bot_token = options.get("discord.bot-token")
-        url = DiscordClient.get_guild_url.format(guild_id=guild_id)
+        url = DiscordClient.guild_url.format(guild_id=guild_id)
         headers = {"Authorization": f"Bot {bot_token}"}
         try:
             response = DiscordClient().get(url, headers=headers)
diff --git a/tests/sentry/integrations/discord/test_client.py b/tests/sentry/integrations/discord/test_client.py
index 689913f0641c00..179980c8e4c42a 100644
--- a/tests/sentry/integrations/discord/test_client.py
+++ b/tests/sentry/integrations/discord/test_client.py
@@ -40,7 +40,7 @@ def test_get_guild_name(self):
 
         responses.add(
             responses.GET,
-            url=f"{DiscordClient.base_url}{DiscordClient.get_guild_url.format(guild_id=guild_id)}",
+            url=f"{DiscordClient.base_url}{DiscordClient.guild_url.format(guild_id=guild_id)}",
             json={
                 "id": guild_id,
                 "name": server_name,
@@ -86,14 +86,14 @@ def assert_proxy_request(self, request, is_proxy=True):
 
         responses.add(
             method=responses.GET,
-            url=f"{DiscordClient.base_url}{DiscordClient.get_guild_url.format(guild_id=self.integration.external_id)}",
+            url=f"{DiscordClient.base_url}{DiscordClient.guild_url.format(guild_id=self.integration.external_id)}",
             json={"guild_id": "1234567890", "name": "Cool server"},
             status=200,
         )
 
         responses.add(
             method=responses.GET,
-            url=f"{control_address}{PROXY_BASE_PATH}{DiscordClient.get_guild_url.format(guild_id=self.integration.external_id)}",
+            url=f"{control_address}{PROXY_BASE_PATH}{DiscordClient.guild_url.format(guild_id=self.integration.external_id)}",
             json={"guild_id": "1234567890", "name": "Cool server"},
             status=200,
         )
@@ -103,7 +103,7 @@ def assert_proxy_request(self, request, is_proxy=True):
             client.get_guild_name(self.integration.external_id)
             request = responses.calls[0].request
 
-            assert client.get_guild_url.format(guild_id=self.integration.external_id) in request.url
+            assert client.guild_url.format(guild_id=self.integration.external_id) in request.url
             assert client.base_url in request.url
             client.assert_proxy_request(request, is_proxy=False)
 
@@ -113,7 +113,7 @@ def assert_proxy_request(self, request, is_proxy=True):
             client.get_guild_name(self.integration.external_id)
             request = responses.calls[0].request
 
-            assert client.get_guild_url.format(guild_id=self.integration.external_id) in request.url
+            assert client.guild_url.format(guild_id=self.integration.external_id) in request.url
             assert client.base_url in request.url
             client.assert_proxy_request(request, is_proxy=False)
 
@@ -123,6 +123,6 @@ def assert_proxy_request(self, request, is_proxy=True):
             client.get_guild_name(self.integration.external_id)
             request = responses.calls[0].request
 
-            assert client.get_guild_url.format(guild_id=self.integration.external_id) in request.url
+            assert client.guild_url.format(guild_id=self.integration.external_id) in request.url
             assert client.base_url not in request.url
             client.assert_proxy_request(request, is_proxy=True)
diff --git a/tests/sentry/integrations/discord/test_integration.py b/tests/sentry/integrations/discord/test_integration.py
index 0c67592c1449ed..4a3b6222f592c9 100644
--- a/tests/sentry/integrations/discord/test_integration.py
+++ b/tests/sentry/integrations/discord/test_integration.py
@@ -43,7 +43,7 @@ def assert_setup_flow(
 
         responses.add(
             responses.GET,
-            url=f"{DiscordClient.base_url}{DiscordClient.get_guild_url.format(guild_id=guild_id)}",
+            url=f"{DiscordClient.base_url}{DiscordClient.guild_url.format(guild_id=guild_id)}",
             json={
                 "id": guild_id,
                 "name": server_name,
@@ -99,7 +99,7 @@ def test_get_guild_name(self):
 
         responses.add(
             responses.GET,
-            url=f"{DiscordClient.base_url}{DiscordClient.get_guild_url.format(guild_id=guild_id)}",
+            url=f"{DiscordClient.base_url}{DiscordClient.guild_url.format(guild_id=guild_id)}",
             json={
                 "id": guild_id,
                 "name": guild_name,
@@ -118,7 +118,7 @@ def test_get_guild_name_failure(self):
 
         responses.add(
             responses.GET,
-            url=f"{DiscordClient.base_url}{DiscordClient.get_guild_url.format(guild_id=guild_id)}",
+            url=f"{DiscordClient.base_url}{DiscordClient.guild_url.format(guild_id=guild_id)}",
             status=500,
         )
 
diff --git a/tests/sentry/integrations/discord/test_uninstall.py b/tests/sentry/integrations/discord/test_uninstall.py
new file mode 100644
index 00000000000000..c204171ac3808f
--- /dev/null
+++ b/tests/sentry/integrations/discord/test_uninstall.py
@@ -0,0 +1,109 @@
+from typing import Any, Optional
+
+import responses
+
+from sentry.constants import ObjectStatus
+from sentry.integrations.discord.client import DiscordClient
+from sentry.models import Integration, Organization, OrganizationIntegration, ScheduledDeletion
+from sentry.services.hybrid_cloud.user.model import RpcUser
+from sentry.silo.base import SiloMode
+from sentry.testutils.cases import APITestCase
+from sentry.testutils.factories import Factories
+from sentry.testutils.silo import assume_test_silo_mode, control_silo_test
+
+GUILD_ID = "guild-id"
+LEAVE_GUILD_URL = (
+    f"{DiscordClient.base_url}{DiscordClient.users_guild_url.format(guild_id=GUILD_ID)}"
+)
+
+
+@control_silo_test
+class DiscordUninstallTest(APITestCase):
+    endpoint = "sentry-api-0-organization-integration-details"
+    method = "delete"
+
+    def setUp(self) -> None:
+        self.integration = self.create_discord_integration(self.organization, self.user)
+        self.login_as(self.user)
+        return
+
+    def create_discord_integration(
+        self,
+        organization: Organization,
+        user: Optional[RpcUser],
+        guild_id: str = GUILD_ID,
+        **kwargs: Any,
+    ):
+        if user is None:
+            with assume_test_silo_mode(SiloMode.REGION):
+                user = organization.get_default_owner()
+
+        integration = Factories.create_integration(
+            provider="discord",
+            name="Cool server",
+            external_id=guild_id,
+            organization=organization,
+            **kwargs,
+        )
+
+        return integration
+
+    def uninstall(self):
+        org_integration = OrganizationIntegration.objects.get(
+            integration=self.integration, organization_id=self.organization.id
+        )
+
+        with self.tasks():
+            self.get_success_response(self.organization.slug, self.integration.id)
+
+        assert Integration.objects.filter(id=self.integration.id).exists()
+
+        assert not OrganizationIntegration.objects.filter(
+            integration=self.integration,
+            organization_id=self.organization.id,
+            status=ObjectStatus.ACTIVE,
+        ).exists()
+
+        assert ScheduledDeletion.objects.filter(
+            model_name="OrganizationIntegration",
+            object_id=org_integration.id,
+        ).exists()
+
+    def mock_discord_guild_leave(self, status: int = 204):
+        responses.add(
+            responses.DELETE,
+            url=f"{DiscordClient.base_url}{DiscordClient.users_guild_url.format(guild_id=GUILD_ID)}",
+            status=status,
+        )
+
+    def assert_leave_guild_api_call_count(self, count: int):
+        assert responses.assert_call_count(count=count, url=LEAVE_GUILD_URL)
+
+    @responses.activate
+    def test_uninstall(self):
+        self.mock_discord_guild_leave()
+        self.uninstall()
+        self.assert_leave_guild_api_call_count(1)
+
+    @responses.activate
+    def test_uninstall_bot_already_left_guild(self):
+        self.mock_discord_guild_leave(status=404)
+        self.uninstall()
+        self.assert_leave_guild_api_call_count(1)
+
+    @responses.activate
+    def test_uninstall_multiple_orgs(self):
+        self.mock_discord_guild_leave()
+
+        other_org = self.create_organization(owner=self.user)
+        self.integration.add_organization(other_org)
+
+        self.uninstall()
+
+        # make sure other org integration persists and we did not make an
+        # attempt to uninstall the bot from the shared server
+        assert Integration.objects.filter(id=self.integration.id).exists()
+        assert OrganizationIntegration.objects.filter(
+            integration=self.integration, organization_id=other_org.id
+        ).exists()
+        self.assert_leave_guild_api_call_count(0)
diff --git a/tests/sentry/integrations/discord/test_webhook.py b/tests/sentry/integrations/discord/test_webhook.py
index 6c0984955b103f..11097d7fae924f 100644
--- a/tests/sentry/integrations/discord/test_webhook.py
+++ b/tests/sentry/integrations/discord/test_webhook.py
@@ -1,7 +1,5 @@
 from unittest import mock
 
-import responses
-
 from sentry.testutils.cases import APITestCase
 from sentry.testutils.silo import region_silo_test
 
@@ -10,7 +8,6 @@
 
 @region_silo_test(stable=True)
 class DiscordWebhookTest(APITestCase):
-    @responses.activate
     @mock.patch("sentry.integrations.discord.requests.base.verify_signature")
     def test_ping_interaction(self, mock_verify_signature):
         mock_verify_signature.return_value = True
@@ -28,7 +25,6 @@ def test_ping_interaction(self, mock_verify_signature):
         assert resp.json()["type"] == 1
         assert mock_verify_signature.call_count == 1
 
-    @responses.activate
     @mock.patch("sentry.integrations.discord.requests.base.verify_signature")
     def test_unknown_interaction(self, mock_verify_signature):
         mock_verify_signature.return_value = True
@@ -44,7 +40,6 @@ def test_unknown_interaction(self, mock_verify_signature):
 
         assert resp.status_code == 200
 
-    @responses.activate
     @mock.patch("sentry.integrations.discord.requests.base.verify_signature")
     def test_unauthorized_interaction(self, mock_verify_signature):
         mock_verify_signature.return_value = False
@@ -60,7 +55,6 @@ def test_unauthorized_interaction(self, mock_verify_signature):
 
         assert resp.status_code == 401
 
-    @responses.activate
     def test_missing_signature(self):
         resp = self.client.post(
             path=WEBHOOK_URL,
@@ -73,7 +67,6 @@ def test_missing_signature(self):
 
         assert resp.status_code == 401
 
-    @responses.activate
     def test_missing_timestamp(self):
         resp = self.client.post(
             path=WEBHOOK_URL,


Code review comments:
diff_hunk: @@ -39,7 +39,37 @@
 
 
 class DiscordIntegration(IntegrationInstallation):
-    pass
+    def get_client(self) -> DiscordClient:
+        org_integration_id = self.org_integration.id if self.org_integration else None
+
+        return DiscordClient(
+            integration_id=self.model.id,  # type:ignore
+            org_integration_id=org_integration_id,
+        )
+
+    def uninstall(self) -> None:
+        # If this is the only org using this Discord server, we should remove
+        # the bot from the server.
+        from sentry.services.hybrid_cloud.integration import integration_service
+
+        installations = integration_service.get_organization_integrations(
+            integration_id=self.model.id,
+            providers=["discord"],
+        )
+
+        # Remove any installations pending deletion
+        active_installations = [i for i in installations if i.status not in (2, 3)]
path: src/sentry/integrations/discord/integration.py
text: @user1:
Is there an enum here that we can use instead of `i.status not in (2, 3)`? Maybe `ObjectStatus.PENDING_DELETION` and `ObjectStatus.DELETION_IN_PROGRESS`?

diff_hunk: @@ -39,7 +39,37 @@
 
 
 class DiscordIntegration(IntegrationInstallation):
-    pass
+    def get_client(self) -> DiscordClient:
+        org_integration_id = self.org_integration.id if self.org_integration else None
+
+        return DiscordClient(
+            integration_id=self.model.id,  # type:ignore
+            org_integration_id=org_integration_id,
+        )
+
+    def uninstall(self) -> None:
+        # If this is the only org using this Discord server, we should remove
+        # the bot from the server.
+        from sentry.services.hybrid_cloud.integration import integration_service
+
+        installations = integration_service.get_organization_integrations(
+            integration_id=self.model.id,
+            providers=["discord"],
+        )
+
+        # Remove any installations pending deletion
+        active_installations = [i for i in installations if i.status not in (2, 3)]
+
+        if len(active_installations) > 1:
+            return
path: src/sentry/integrations/discord/integration.py
text: @user1:
Would it make sense to log something here? Might be helpful to know - or we can raise an error and let the user know why the uninstall didn't work

diff_hunk: @@ -0,0 +1,109 @@
+from typing import Any, Optional
+
+import responses
+
+from sentry.constants import ObjectStatus
+from sentry.integrations.discord.client import DiscordClient
+from sentry.models import Integration, Organization, OrganizationIntegration, ScheduledDeletion
+from sentry.services.hybrid_cloud.user.model import RpcUser
+from sentry.silo.base import SiloMode
+from sentry.testutils.cases import APITestCase
+from sentry.testutils.factories import Factories
+from sentry.testutils.silo import assume_test_silo_mode, control_silo_test
+
+GUILD_ID = "guild-id"
+LEAVE_GUILD_URL = (
+    f"{DiscordClient.base_url}{DiscordClient.users_guild_url.format(guild_id=GUILD_ID)}"
+)
+
+
+@control_silo_test
+class DiscordUninstallTest(APITestCase):
+    endpoint = "sentry-api-0-organization-integration-details"
+    method = "delete"
+
+    def setUp(self) -> None:
+        self.integration = self.create_discord_integration(self.organization, self.user)
+        self.login_as(self.user)
+        return
+
+    def create_discord_integration(
+        self,
+        organization: Organization,
+        user: Optional[RpcUser],
+        guild_id: str = GUILD_ID,
+        **kwargs: Any,
+    ):
+        if user is None:
+            with assume_test_silo_mode(SiloMode.REGION):
+                user = organization.get_default_owner()
+
+        integration = Factories.create_integration(
+            provider="discord",
+            name="Cool server",
+            external_id=guild_id,
+            organization=organization,
+            **kwargs,
+        )
+
+        return integration
+
+    def uninstall(self):
+        org_integration = OrganizationIntegration.objects.get(
+            integration=self.integration, organization_id=self.organization.id
+        )
+
+        with self.tasks():
+            self.get_success_response(self.organization.slug, self.integration.id)
+
+        assert Integration.objects.filter(id=self.integration.id).exists()
+
+        assert not OrganizationIntegration.objects.filter(
+            integration=self.integration,
+            organization_id=self.organization.id,
+            status=ObjectStatus.ACTIVE,
+        ).exists()
+
+        assert ScheduledDeletion.objects.filter(
+            model_name="OrganizationIntegration",
+            object_id=org_integration.id,
+        ).exists()
+
+    def mock_discord_guild_leave(self, status: int = 204):
+        responses.add(
+            responses.DELETE,
+            url=f"{DiscordClient.base_url}{DiscordClient.users_guild_url.format(guild_id=GUILD_ID)}",
+            status=status,
+        )
+
+    def assert_leave_guild_api_call_count(self, count: int):
+        assert responses.assert_call_count(count=count, url=LEAVE_GUILD_URL)
+
+    @responses.activate
+    def test_uninstall(self):
+        self.mock_discord_guild_leave()
+        self.uninstall()
+        self.assert_leave_guild_api_call_count(1)
+
+    @responses.activate
+    def test_uninstall_bot_already_left_guild(self):
+        self.mock_discord_guild_leave(status=404)
+        self.uninstall()
+        self.assert_leave_guild_api_call_count(1)
+
+    @responses.activate
+    def test_uninstall_multiple_orgs(self):
+        self.mock_discord_guild_leave()
+
+        other_org = self.create_organization(owner=self.user)
+        self.integration.add_organization(other_org)
+
+        self.uninstall()
+
+        # make sure other org integration persists and we did not make an
path: tests/sentry/integrations/discord/test_uninstall.py
text: @user1:
```suggestion
        # Make sure other org integration persists and we did not make an
```