From e556a11d4099a4b49f62bc07b12df84a76d71680 Mon Sep 17 00:00:00 2001 From: Grant Patterson Date: Mon, 21 Sep 2026 16:03:54 -0700 Subject: [PATCH 1/2] ref(np): Resolve MSTeams integrations from one lookup list The request parser walked a card action id, then a team id, then a tenant id to find the integration, while the card action webhook handler only looked at the card action id and can_infer_integration only looked at the first two. Cards built by the notification platform cannot embed an integration id, since the renderer has no access to the target it is being sent to, so every caller needs the full chain. Express the chain once as an ordered list of integration_service filters and derive all three callers from it, so a request identified only by its tenant -- an action submitted from a personal chat -- resolves and is dispatched to a cell rather than falling through to the control silo. Co-authored-by: Cursor --- src/sentry/integrations/msteams/parsing.py | 87 ++++++++--------- src/sentry/integrations/msteams/webhook.py | 4 +- .../integrations/parsers/msteams.py | 6 +- .../msteams/test_action_state_change.py | 49 ++++++++-- .../integrations/msteams/test_parsing.py | 94 +++++++++++++++++++ 5 files changed, 176 insertions(+), 64 deletions(-) create mode 100644 tests/sentry/integrations/msteams/test_parsing.py diff --git a/src/sentry/integrations/msteams/parsing.py b/src/sentry/integrations/msteams/parsing.py index c4b12556aad5..fcc3f3312229 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,49 @@ 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 _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: 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 installs are keyed by team id, personal installs by tenant id. + for external_id in ( + get_path(data, "channelData", "team", "id"), + get_path(data, "channelData", "tenant", "id"), + ): + if external_id is not None: + lookups.append( + {"provider": IntegrationProviderSlug.MSTEAMS.value, "external_id": external_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 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(_integration_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..a7c8d28ce00f --- /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_can_infer_from_tenant_id_alone(self) -> None: + data = self.request_data(tenant_id="m17hr4nd1r") + + assert parsing.can_infer_integration(data=data) is True + + 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 From 68dacd00ce9abdcb7b73179a42fde0c9eeea5d41 Mon Sep 17 00:00:00 2001 From: Grant Patterson Date: Mon, 21 Sep 2026 17:03:18 -0700 Subject: [PATCH 2/2] fix(np): Keep tenant-only MSTeams payloads in the control silo A tenant id identifies a personal install, whose events are served from the control silo, so it cannot stand in as a cell routing key. Co-authored-by: Cursor --- src/sentry/integrations/msteams/parsing.py | 40 +++++++++++++------ .../integrations/msteams/test_parsing.py | 4 +- .../integrations/parsers/test_msteams.py | 3 +- 3 files changed, 31 insertions(+), 16 deletions(-) diff --git a/src/sentry/integrations/msteams/parsing.py b/src/sentry/integrations/msteams/parsing.py index fcc3f3312229..6ea73b7a3dd9 100644 --- a/src/sentry/integrations/msteams/parsing.py +++ b/src/sentry/integrations/msteams/parsing.py @@ -19,10 +19,14 @@ def get_integration_from_channel_data(data: Mapping[str, Any]) -> RpcIntegration ) -def _integration_lookups(data: Mapping[str, Any]) -> list[Mapping[str, Any]]: +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 that could identify the integration a - request belongs to, most specific first. Empty when the request carries no usable identifier. + The `integration_service.get_integration` filters identifying an integration whose events + are served from the cells, most specific first. """ lookups: list[Mapping[str, Any]] = [] @@ -36,21 +40,31 @@ def _integration_lookups(data: Mapping[str, Any]) -> list[Mapping[str, Any]]: if integration_id is not None: lookups.append({"integration_id": integration_id}) - # Team installs are keyed by team id, personal installs by tenant id. - for external_id in ( - get_path(data, "channelData", "team", "id"), - get_path(data, "channelData", "tenant", "id"), - ): - if external_id is not None: - lookups.append( - {"provider": IntegrationProviderSlug.MSTEAMS.value, "external_id": external_id} - ) + team_id = get_path(data, "channelData", "team", "id") + if team_id is not None: + lookups.append(_external_id_lookup(team_id)) + + 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 len(_integration_lookups(data=data)) > 0 + return len(_routable_lookups(data=data)) > 0 def get_integration_from_request_data(data: Mapping[str, Any]) -> RpcIntegration | None: diff --git a/tests/sentry/integrations/msteams/test_parsing.py b/tests/sentry/integrations/msteams/test_parsing.py index a7c8d28ce00f..2d8adff99184 100644 --- a/tests/sentry/integrations/msteams/test_parsing.py +++ b/tests/sentry/integrations/msteams/test_parsing.py @@ -75,10 +75,10 @@ def test_can_infer_from_team_id(self) -> None: assert parsing.can_infer_integration(data=data) is True - def test_can_infer_from_tenant_id_alone(self) -> None: + 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 True + 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 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()