diff --git a/src/sentry/integrations/msteams/parsing.py b/src/sentry/integrations/msteams/parsing.py index c4b12556aad5..6ea73b7a3dd9 100644 --- a/src/sentry/integrations/msteams/parsing.py +++ b/src/sentry/integrations/msteams/parsing.py @@ -1,4 +1,3 @@ -import logging from collections.abc import Mapping from typing import Any @@ -6,22 +5,11 @@ from sentry.integrations.services.integration import integration_service from sentry.integrations.services.integration.model import RpcIntegration from sentry.integrations.types import IntegrationProviderSlug - -logger = logging.getLogger(__name__) - - -def _infer_team_id_from_channel_data(data: Mapping[str, Any]) -> str | None: - try: - channel_data = data["channelData"] - team_id = channel_data["team"]["id"] - return team_id - except Exception: - pass - return None +from sentry.utils.safe import get_path def get_integration_from_channel_data(data: Mapping[str, Any]) -> RpcIntegration | None: - team_id = _infer_team_id_from_channel_data(data=data) + team_id = get_path(data, "channelData", "team", "id") if team_id is None: return None return integration_service.get_integration( @@ -31,50 +19,63 @@ def get_integration_from_channel_data(data: Mapping[str, Any]) -> RpcIntegration ) -def get_integration_for_tenant(data: Mapping[str, Any]) -> RpcIntegration | None: - try: - channel_data = data["channelData"] - tenant_id = channel_data["tenant"]["id"] - return integration_service.get_integration( - provider=IntegrationProviderSlug.MSTEAMS.value, - external_id=tenant_id, - status=ObjectStatus.ACTIVE, - ) - except Exception as err: - logger.info("failed to get tenant id from request data", exc_info=err, extra={"data": data}) - return None +def _external_id_lookup(external_id: str) -> Mapping[str, Any]: + return {"provider": IntegrationProviderSlug.MSTEAMS.value, "external_id": external_id} + +def _routable_lookups(data: Mapping[str, Any]) -> list[Mapping[str, Any]]: + """ + The `integration_service.get_integration` filters identifying an integration whose events + are served from the cells, most specific first. + """ + lookups: list[Mapping[str, Any]] = [] -def _infer_integration_id_from_card_action(data: Mapping[str, Any]) -> int | None: - # The bot builds and sends Adaptive Cards to the channel, and in it will include card actions and context. - # The context will include the "integrationId". - # Whenever a user interacts with the card, MS Teams will send the card action and the context to the bot. - # Here we parse the "integrationId" from the context. + # The bot embeds an "integrationId" in the card action context of the cards it builds, and + # Teams echoes that context back when a user interacts with the card. Cards built by the + # notification platform omit it, since the renderer has no access to the target it is being + # sent to, leaving only the conversation the request arrived on to identify the integration. # # See: https://learn.microsoft.com/en-us/microsoftteams/platform/task-modules-and-cards/cards/cards-actions?tabs=json#actionsubmit - try: - payload = data["value"]["payload"] - integration_id = payload["integrationId"] - return integration_id - except Exception: - pass - return None + integration_id = get_path(data, "value", "payload", "integrationId") + if integration_id is not None: + lookups.append({"integration_id": integration_id}) + team_id = get_path(data, "channelData", "team", "id") + if team_id is not None: + lookups.append(_external_id_lookup(team_id)) -def get_integration_from_card_action(data: Mapping[str, Any]) -> RpcIntegration | None: - integration_id = _infer_integration_id_from_card_action(data=data) - if integration_id is None: - return None - return integration_service.get_integration( - integration_id=integration_id, status=ObjectStatus.ACTIVE - ) + return lookups + + +def _integration_lookups(data: Mapping[str, Any]) -> list[Mapping[str, Any]]: + """ + The `integration_service.get_integration` filters that could identify the integration a + request belongs to, most specific first. Empty when the request carries no usable identifier. + """ + lookups = _routable_lookups(data=data) + + # Personal installs are keyed by tenant id. Their events are handled in the control silo, + # where the identities they operate on live, so a tenant id is not routable on its own. + tenant_id = get_path(data, "channelData", "tenant", "id") + if tenant_id is not None: + lookups.append(_external_id_lookup(tenant_id)) + + return lookups def can_infer_integration(data: Mapping[str, Any]) -> bool: - return ( - _infer_integration_id_from_card_action(data=data) is not None - or _infer_team_id_from_channel_data(data=data) is not None - ) + return len(_routable_lookups(data=data)) > 0 + + +def get_integration_from_request_data(data: Mapping[str, Any]) -> RpcIntegration | None: + for lookup in _integration_lookups(data=data): + integration = integration_service.get_integration( + status=ObjectStatus.ACTIVE, + **lookup, + ) + if integration is not None: + return integration + return None def is_new_integration_installation_event(data: Mapping[str, Any]) -> bool: diff --git a/src/sentry/integrations/msteams/webhook.py b/src/sentry/integrations/msteams/webhook.py index fa6c099ffd81..5d003cd0269e 100644 --- a/src/sentry/integrations/msteams/webhook.py +++ b/src/sentry/integrations/msteams/webhook.py @@ -553,7 +553,7 @@ def _handle_action_submitted(self, request: Request) -> Response: tenant_id = channel_data["tenant"]["id"] payload = data["value"]["payload"] group_id = payload["groupId"] - integration_id = payload["integrationId"] + integration_id = payload.get("integrationId") user_id = data["from"]["id"] activity_id = data["replyToId"] conversation = data["conversation"] @@ -562,7 +562,7 @@ def _handle_action_submitted(self, request: Request) -> Response: else: conversation_id = channel_data["channel"]["id"] - integration = parsing.get_integration_from_card_action(data=data) + integration = parsing.get_integration_from_request_data(data=data) if integration is None: logger.info( "msteams.action.missing-integration", extra={"integration_id": integration_id} diff --git a/src/sentry/middleware/integrations/parsers/msteams.py b/src/sentry/middleware/integrations/parsers/msteams.py index 69cc1129a436..c0d0df2fe0a8 100644 --- a/src/sentry/middleware/integrations/parsers/msteams.py +++ b/src/sentry/middleware/integrations/parsers/msteams.py @@ -41,11 +41,7 @@ def request_data(self) -> Mapping[str, Any]: @control_silo_function def get_integration_from_request(self) -> Integration | None: - integration = parsing.get_integration_from_card_action(data=self.request_data) - if integration is None: - integration = parsing.get_integration_from_channel_data(data=self.request_data) - if integration is None: - integration = parsing.get_integration_for_tenant(data=self.request_data) + integration = parsing.get_integration_from_request_data(data=self.request_data) if integration: return Integration.objects.filter(id=integration.id).first() return None diff --git a/tests/sentry/integrations/msteams/test_action_state_change.py b/tests/sentry/integrations/msteams/test_action_state_change.py index dab411b47277..71150a84d013 100644 --- a/tests/sentry/integrations/msteams/test_action_state_change.py +++ b/tests/sentry/integrations/msteams/test_action_state_change.py @@ -85,6 +85,7 @@ def post_webhook( resolve_input: str | None = None, archive_input: str | None = None, assign_input: str | None = None, + include_integration_id: bool = True, ) -> Response: replyToId = "12345" @@ -103,19 +104,22 @@ def post_webhook( json={}, ) + action_payload: dict[str, Any] = { + "groupId": group_id or self.group1.id, + "eventId": self.event1.event_id, + "actionType": action_type, + "rules": [], + } + if include_integration_id: + action_payload["integrationId"] = self.integration.id + payload = { "type": "message", "from": {"id": user_id}, "channelData": channel_data, "conversation": {"conversationType": conversation_type, "id": conversation_id}, "value": { - "payload": { - "groupId": group_id or self.group1.id, - "eventId": self.event1.event_id, - "actionType": action_type, - "rules": [], - "integrationId": self.integration.id, - }, + "payload": action_payload, "resolveInput": resolve_input, "archiveInput": archive_input, "assignInput": assign_input, @@ -406,6 +410,37 @@ def test_resolve_issue(self, verify: MagicMock) -> None: assert self.group1.get_status() == GroupStatus.RESOLVED assert b"Unresolve" in responses.calls[0].request.body + @responses.activate + @patch("sentry.integrations.msteams.webhook.verify_signature", return_value=True) + def test_resolve_issue_without_integration_id(self, verify: MagicMock) -> None: + resp = self.post_webhook( + action_type=ACTION_TYPE.RESOLVE, + resolve_input="resolved", + include_integration_id=False, + ) + self.group1 = Group.objects.get(id=self.group1.id) + + assert resp.status_code == 200, resp.content + assert self.group1.get_status() == GroupStatus.RESOLVED + + @responses.activate + @patch("sentry.integrations.msteams.webhook.verify_signature", return_value=True) + def test_resolve_issue_without_integration_id_in_personal_chat(self, verify: MagicMock) -> None: + with assume_test_silo_mode(SiloMode.CONTROL): + self.integration.update(external_id="m17hr4nd1r") + self.idp.update(external_id="m17hr4nd1r") + + resp = self.post_webhook( + action_type=ACTION_TYPE.RESOLVE, + resolve_input="resolved", + conversation_type="personal", + include_integration_id=False, + ) + self.group1 = Group.objects.get(id=self.group1.id) + + assert resp.status_code == 200, resp.content + assert self.group1.get_status() == GroupStatus.RESOLVED + @responses.activate @patch("sentry.integrations.msteams.webhook.verify_signature", return_value=True) def test_no_resolve_input(self, verify: MagicMock) -> None: diff --git a/tests/sentry/integrations/msteams/test_parsing.py b/tests/sentry/integrations/msteams/test_parsing.py new file mode 100644 index 000000000000..2d8adff99184 --- /dev/null +++ b/tests/sentry/integrations/msteams/test_parsing.py @@ -0,0 +1,94 @@ +from typing import Any +from unittest import mock + +from sentry.integrations.msteams import parsing +from sentry.integrations.services.integration import integration_service +from sentry.testutils.cases import TestCase +from sentry.testutils.silo import control_silo_test + + +@control_silo_test +class GetIntegrationFromRequestDataTest(TestCase): + def setUp(self) -> None: + super().setUp() + self.integration = self.create_provider_integration( + provider="msteams", + name="Fellowship of the Ring", + external_id="f3ll0wsh1p", + ) + + def request_data( + self, + *, + integration_id: int | None = None, + team_id: str | None = None, + tenant_id: str | None = None, + ) -> dict[str, Any]: + channel_data: dict[str, Any] = {} + if team_id is not None: + channel_data["team"] = {"id": team_id} + if tenant_id is not None: + channel_data["tenant"] = {"id": tenant_id} + + data: dict[str, Any] = {"channelData": channel_data} + if integration_id is not None: + data["value"] = {"payload": {"integrationId": integration_id}} + return data + + def test_resolves_from_card_action(self) -> None: + data = self.request_data(integration_id=self.integration.id) + + integration = parsing.get_integration_from_request_data(data=data) + + assert integration is not None + assert integration.id == self.integration.id + + def test_falls_back_to_channel_data(self) -> None: + data = self.request_data(team_id="f3ll0wsh1p") + + integration = parsing.get_integration_from_request_data(data=data) + + assert integration is not None + assert integration.id == self.integration.id + + def test_falls_back_to_tenant(self) -> None: + self.integration.update(external_id="m17hr4nd1r") + data = self.request_data(tenant_id="m17hr4nd1r") + + integration = parsing.get_integration_from_request_data(data=data) + + assert integration is not None + assert integration.id == self.integration.id + + def test_returns_none_when_nothing_matches(self) -> None: + data = self.request_data(team_id="m0rd0r", tenant_id="s4ur0n") + + assert parsing.get_integration_from_request_data(data=data) is None + + def test_can_infer_from_card_action(self) -> None: + data = self.request_data(integration_id=self.integration.id) + + assert parsing.can_infer_integration(data=data) is True + + def test_can_infer_from_team_id(self) -> None: + data = self.request_data(team_id="f3ll0wsh1p") + + assert parsing.can_infer_integration(data=data) is True + + def test_cannot_infer_from_tenant_id_alone(self) -> None: + data = self.request_data(tenant_id="m17hr4nd1r") + + assert parsing.can_infer_integration(data=data) is False + + def test_cannot_infer_without_any_identifiers(self) -> None: + assert parsing.can_infer_integration(data=self.request_data()) is False + + def test_can_infer_does_not_look_up_the_integration(self) -> None: + # Every webhook request runs through this check before it can be shed, so it has to stay + # a lookup-free inspection of the request body. + data = self.request_data(integration_id=self.integration.id, team_id="f3ll0wsh1p") + + with mock.patch.object(integration_service, "get_integration") as mock_get_integration: + assert parsing.can_infer_integration(data=data) is True + + assert not mock_get_integration.called diff --git a/tests/sentry/middleware/integrations/parsers/test_msteams.py b/tests/sentry/middleware/integrations/parsers/test_msteams.py index d59651dabe46..193428a66579 100644 --- a/tests/sentry/middleware/integrations/parsers/test_msteams.py +++ b/tests/sentry/middleware/integrations/parsers/test_msteams.py @@ -217,8 +217,9 @@ def test_handle_control_silo_payloads(self) -> None: for payload in control_silo_payloads: request = self.factory.post( self.path, - json=payload, + data=payload, HTTP_AUTHORIZATION=f"Bearer {TOKEN}", + content_type="application/json", ) parser = MsTeamsRequestParser(request=request, response_handler=self.get_response) response = parser.get_response()