ref(np): Resolve MSTeams integrations from one lookup list

src/sentry/integrations/msteams/parsing.py

@@ -1,27 +1,15 @@
-import logging
 from collections.abc import Mapping
 from typing import Any
 
 from sentry.constants import ObjectStatus
 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:

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}

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

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:

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

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()