From c2a7bf73bf1c89a90d95bd1448b8024ba8f4b98e Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Wed, 28 Aug 2024 09:02:55 -0700 Subject: [PATCH] fix(discord): Verify User Bot Installation Permission Take 2 (#76586) https://github.com/getsentry/sentry/pull/76360 broke prod, so trying again. this time, we are just going to check if the user is actually part of the guild. to call this method, we needed to ask for guild info in the oauth scope. --- src/sentry/integrations/discord/client.py | 17 +++ .../integrations/discord/integration.py | 28 ++--- src/sentry/integrations/discord/types.py | 17 +++ src/sentry/options/defaults.py | 3 + .../integrations/discord/test_integration.py | 103 ++++++++++++++---- 5 files changed, 129 insertions(+), 39 deletions(-) create mode 100644 src/sentry/integrations/discord/types.py diff --git a/src/sentry/integrations/discord/client.py b/src/sentry/integrations/discord/client.py index 5807283a86cd2..b6535d95600a2 100644 --- a/src/sentry/integrations/discord/client.py +++ b/src/sentry/integrations/discord/client.py @@ -13,6 +13,7 @@ from sentry.integrations.client import ApiClient from sentry.integrations.discord.message_builder.base.base import DiscordMessageBuilder from sentry.integrations.discord.utils.consts import DISCORD_ERROR_CODES, DISCORD_USER_ERRORS +from sentry.shared_integrations.exceptions import ApiError # to avoid a circular import from sentry.utils import metrics @@ -102,6 +103,22 @@ def get_user_id(self, access_token: str): user_id = response["id"] # type: ignore[index] return user_id + def check_user_bot_installation_permission(self, access_token: str, guild_id: str) -> bool: + headers = {"Authorization": f"Bearer {access_token}"} + + # We only want information about guild_id and check the user's permission in the guild, but we can't currently do that + # https://github.com/discord/discord-api-docs/discussions/6846 + # TODO(iamrajjoshi): Eventually, we should use `/users/@me/guilds/{guild.id}/member` + # Instead, we check if the user in a member of the guild + + try: + self.get(f"/users/@me/guilds/{guild_id}/member", headers=headers) + except ApiError as e: + if e.code == 404: + return False + + return True + def leave_guild(self, guild_id: str) -> None: """ Leave the given guild_id, if the bot is currently a member. diff --git a/src/sentry/integrations/discord/integration.py b/src/sentry/integrations/discord/integration.py index bf8e0c36dd1bd..f23eafb4fd0dc 100644 --- a/src/sentry/integrations/discord/integration.py +++ b/src/sentry/integrations/discord/integration.py @@ -1,7 +1,6 @@ from __future__ import annotations from collections.abc import Mapping, Sequence -from enum import Enum from typing import Any from urllib.parse import urlencode @@ -17,6 +16,7 @@ IntegrationProvider, ) from sentry.integrations.discord.client import DiscordClient +from sentry.integrations.discord.types import DiscordPermissions from sentry.integrations.models.integration import Integration from sentry.organizations.services.organization.model import RpcOrganizationSummary from sentry.pipeline.views.base import PipelineView @@ -111,19 +111,6 @@ def uninstall(self) -> None: return -class DiscordPermissions(Enum): - # https://discord.com/developers/docs/topics/permissions#permissions - VIEW_CHANNEL = 1 << 10 - SEND_MESSAGES = 1 << 11 - SEND_TTS_MESSAGES = 1 << 12 - EMBED_LINKS = 1 << 14 - ATTACH_FILES = 1 << 15 - MANAGE_THREADS = 1 << 34 - CREATE_PUBLIC_THREADS = 1 << 35 - CREATE_PRIVATE_THREADS = 1 << 36 - SEND_MESSAGES_IN_THREADS = 1 << 38 - - class DiscordIntegrationProvider(IntegrationProvider): key = "discord" name = "Discord" @@ -132,7 +119,8 @@ class DiscordIntegrationProvider(IntegrationProvider): features = frozenset([IntegrationFeatures.CHAT_UNFURL, IntegrationFeatures.ALERT_RULE]) # https://discord.com/developers/docs/topics/oauth2#shared-resources-oauth2-scopes - oauth_scopes = frozenset(["applications.commands", "bot", "identify"]) + oauth_scopes = frozenset(["applications.commands", "bot", "identify", "guilds.members.read"]) + access_token = "" bot_permissions = ( DiscordPermissions.VIEW_CHANNEL.value @@ -175,6 +163,12 @@ def build_integration(self, state: Mapping[str, object]) -> Mapping[str, object] auth_code = str(state.get("code")) if auth_code: discord_user_id = self._get_discord_user_id(auth_code, url) + if options.get( + "discord.validate-user" + ) and not self.client.check_user_bot_installation_permission( + access_token=self.access_token, guild_id=guild_id + ): + raise IntegrationError("User does not have permissions to install bot.") else: raise IntegrationError("Missing code from state.") @@ -238,13 +232,13 @@ def _get_discord_user_id(self, auth_code: str, url: str) -> str: """ try: - access_token = self.client.get_access_token(auth_code, url) + self.access_token = self.client.get_access_token(auth_code, url) except ApiError: raise IntegrationError("Failed to get Discord access token from API.") except KeyError: raise IntegrationError("Failed to get Discord access token from key.") try: - user_id = self.client.get_user_id(access_token) + user_id = self.client.get_user_id(self.access_token) except ApiError: raise IntegrationError("Failed to get Discord user ID from API.") except KeyError: diff --git a/src/sentry/integrations/discord/types.py b/src/sentry/integrations/discord/types.py new file mode 100644 index 0000000000000..5cffc7e93e4cf --- /dev/null +++ b/src/sentry/integrations/discord/types.py @@ -0,0 +1,17 @@ +from enum import Enum + + +class DiscordPermissions(Enum): + # https://discord.com/developers/docs/topics/permissions#permissions + VIEW_CHANNEL = 1 << 10 + SEND_MESSAGES = 1 << 11 + SEND_TTS_MESSAGES = 1 << 12 + EMBED_LINKS = 1 << 14 + ATTACH_FILES = 1 << 15 + MANAGE_THREADS = 1 << 34 + CREATE_PUBLIC_THREADS = 1 << 35 + CREATE_PRIVATE_THREADS = 1 << 36 + SEND_MESSAGES_IN_THREADS = 1 << 38 + + MANAGE_GUILD = 1 << 5 + ADMINISTRATOR = 1 << 3 diff --git a/src/sentry/options/defaults.py b/src/sentry/options/defaults.py index 8867bebce44ca..08b4678f00e29 100644 --- a/src/sentry/options/defaults.py +++ b/src/sentry/options/defaults.py @@ -501,6 +501,9 @@ register("slack.verification-token", flags=FLAG_CREDENTIAL | FLAG_PRIORITIZE_DISK) register("slack.signing-secret", flags=FLAG_CREDENTIAL | FLAG_PRIORITIZE_DISK) +# Discord +register("discord.validate-user", default=False, flags=FLAG_AUTOMATOR_MODIFIABLE) + # Codecov Integration register("codecov.client-secret", flags=FLAG_CREDENTIAL | FLAG_PRIORITIZE_DISK) diff --git a/tests/sentry/integrations/discord/test_integration.py b/tests/sentry/integrations/discord/test_integration.py index c08f85c9edebf..a3014e419e184 100644 --- a/tests/sentry/integrations/discord/test_integration.py +++ b/tests/sentry/integrations/discord/test_integration.py @@ -29,6 +29,7 @@ def setUp(self): options.set("discord.public-key", self.public_key) options.set("discord.bot-token", self.bot_token) options.set("discord.client-secret", self.client_secret) + options.set("discord.validate-user", True) @mock.patch("sentry.integrations.discord.client.DiscordClient.set_application_command") def assert_setup_flow( @@ -72,6 +73,12 @@ def assert_setup_flow( json=[] if command_response_empty else COMMANDS, ) + responses.add( + responses.GET, + url=f"{DiscordClient.base_url}/users/@me/guilds/{guild_id}/member", + json={}, + ) + if command_response_empty: for command in COMMANDS: responses.add( @@ -160,6 +167,13 @@ def assert_setup_flow_from_discord( "access_token": "access_token", }, ) + + responses.add( + responses.GET, + url=f"{DiscordClient.base_url}/users/@me/guilds/{guild_id}/member", + json={}, + ) + responses.add( responses.GET, url=f"{DiscordClient.base_url}/users/@me", json={"id": "user_1234"} ) @@ -237,19 +251,22 @@ def test_multiple_integrations(self): class DiscordIntegrationTest(DiscordSetupTestCase): + def setUp(self): + super().setUp() + self.user_id = "user1234" + self.guild_id = "12345" + self.guild_name = "guild_name" + @responses.activate def test_get_guild_name(self): provider = self.provider() - user_id = "user1234" - guild_id = "guild_id" - guild_name = "guild_name" responses.add( responses.GET, - url=f"{DiscordClient.base_url}{GUILD_URL.format(guild_id=guild_id)}", + url=f"{DiscordClient.base_url}{GUILD_URL.format(guild_id=self.guild_id)}", match=[header_matcher({"Authorization": f"Bot {self.bot_token}"})], json={ - "id": guild_id, - "name": guild_name, + "id": self.guild_id, + "name": self.guild_name, }, ) responses.add( @@ -262,21 +279,26 @@ def test_get_guild_name(self): responses.add( responses.GET, url=f"{DiscordClient.base_url}/users/@me", json={"id": "user_1234"} ) - result = provider.build_integration({"guild_id": "guild_id", "code": user_id}) - assert result["name"] == guild_name + + responses.add( + responses.GET, + url=f"{DiscordClient.base_url}/users/@me/guilds/{self.guild_id}/member", + json={}, + ) + + result = provider.build_integration({"guild_id": self.guild_id, "code": self.user_id}) + assert result["name"] == self.guild_name @responses.activate def test_build_integration_no_code_in_state(self): provider = self.provider() - guild_id = "guild_id" - guild_name = "guild_name" responses.add( responses.GET, - url=f"{DiscordClient.base_url}{GUILD_URL.format(guild_id=guild_id)}", + url=f"{DiscordClient.base_url}{GUILD_URL.format(guild_id=self.guild_id)}", match=[header_matcher({"Authorization": f"Bot {self.bot_token}"})], json={ - "id": guild_id, - "name": guild_name, + "id": self.guild_id, + "name": self.guild_name, }, ) with pytest.raises(IntegrationError): @@ -285,9 +307,40 @@ def test_build_integration_no_code_in_state(self): @responses.activate def test_get_guild_name_failure(self): provider = self.provider() - user_id = "user1234" - guild_id = "guild_id" - responses.add(responses.GET, "https://discord.com/api/v10/guilds/guild_name", status=500) + + responses.add(responses.GET, "https://discord.com/api/v10/guilds/guild_name", status=500), + responses.add( + responses.POST, + url="https://discord.com/api/v10/oauth2/token", + json={ + "access_token": "access_token", + }, + ) + responses.add( + responses.GET, url=f"{DiscordClient.base_url}/users/@me", json={"id": self.user_id} + ) + responses.add( + responses.GET, + url=f"{DiscordClient.base_url}/users/@me/guilds/{self.guild_id}/member", + json={}, + ) + + result = provider.build_integration({"guild_id": self.guild_id, "code": self.user_id}) + assert result["name"] == self.guild_id + + @responses.activate + def test_get_user_insufficient_permission(self): + provider = self.provider() + + responses.add( + responses.GET, + url=f"{DiscordClient.base_url}{GUILD_URL.format(guild_id=self.guild_id)}", + match=[header_matcher({"Authorization": f"Bot {self.bot_token}"})], + json={ + "id": self.guild_id, + "name": self.guild_name, + }, + ) responses.add( responses.POST, url="https://discord.com/api/v10/oauth2/token", @@ -296,15 +349,21 @@ def test_get_guild_name_failure(self): }, ) responses.add( - responses.GET, url=f"{DiscordClient.base_url}/users/@me", json={"id": user_id} + responses.GET, url=f"{DiscordClient.base_url}/users/@me", json={"id": self.user_id} + ) + responses.add( + responses.GET, + url=f"{DiscordClient.base_url}/users/@me/guilds/{self.guild_id}/member", + json={"code": 10004, "message": "Unknown guild"}, + status=404, ) - result = provider.build_integration({"guild_id": guild_id, "code": user_id}) - assert result["name"] == guild_id + + with pytest.raises(IntegrationError): + provider.build_integration({"guild_id": self.guild_id, "code": self.user_id}) @responses.activate def test_get_discord_user_id(self): provider = self.provider() - user_id = "user1234" responses.add( responses.POST, @@ -314,12 +373,12 @@ def test_get_discord_user_id(self): }, ) responses.add( - responses.GET, url=f"{DiscordClient.base_url}/users/@me", json={"id": user_id} + responses.GET, url=f"{DiscordClient.base_url}/users/@me", json={"id": self.user_id} ) result = provider._get_discord_user_id("auth_code", "1") - assert result == user_id + assert result == self.user_id @responses.activate def test_get_discord_user_id_oauth_failure(self):