feat(opsgenie): refactor validation (#53996)

mifu67 · web-flow · commit b77a4bb69f84 · 2023-08-03T16:51:26.000Z
Refactor key validation to allow all types of integration keys. Check
that keys are valid upon alert rule save/update.

Fixes ER-1772
Fixes ER-1773
Fixes ER-1774
diff --git a/src/sentry/incidents/logic.py b/src/sentry/incidents/logic.py
@@ -42,7 +42,7 @@
 from sentry.services.hybrid_cloud.app import RpcSentryAppInstallation, app_service
 from sentry.services.hybrid_cloud.integration import RpcIntegration, integration_service
 from sentry.services.hybrid_cloud.integration.model import RpcOrganizationIntegration
-from sentry.shared_integrations.exceptions import DuplicateDisplayNameError
+from sentry.shared_integrations.exceptions import ApiError, DuplicateDisplayNameError
 from sentry.snuba.dataset import Dataset
 from sentry.snuba.entity_subscription import (
     ENTITY_TIME_COLUMNS,
@@ -1339,7 +1339,8 @@ def get_alert_rule_trigger_action_opsgenie_team(
     use_async_lookup=False,
     input_channel_id=None,
     integrations=None,
-):
+) -> tuple[str, str]:
+    from sentry.integrations.opsgenie.client import OpsgenieClient
     from sentry.integrations.opsgenie.utils import get_team
 
     oi = integration_service.get_organization_integration(
@@ -1348,6 +1349,20 @@ def get_alert_rule_trigger_action_opsgenie_team(
     team = get_team(target_value, oi)
     if not team:
         raise InvalidTriggerActionError("No Opsgenie team found.")
+
+    integration_key = team["integration_key"]
+    integration = integration_service.get_integration(integration_id=integration_id)
+    if integration is None:
+        raise InvalidTriggerActionError("Opsgenie integration not found.")
+    client = OpsgenieClient(
+        integration=integration,
+        integration_key=integration_key,
+        org_integration_id=oi.id,
+    )
+    try:
+        client.authorize_integration(type="sentry")
+    except ApiError:
+        raise InvalidTriggerActionError("Invalid integration key.")
     return team["id"], team["team"]
 
 
diff --git a/src/sentry/integrations/opsgenie/actions/form.py b/src/sentry/integrations/opsgenie/actions/form.py
@@ -5,9 +5,18 @@
 from django import forms
 from django.utils.translation import gettext_lazy as _
 
+from sentry.integrations.opsgenie.client import OpsgenieClient
 from sentry.integrations.opsgenie.utils import get_team
 from sentry.services.hybrid_cloud.integration import integration_service
-from sentry.services.hybrid_cloud.integration.model import RpcOrganizationIntegration
+from sentry.services.hybrid_cloud.integration.model import (
+    RpcIntegration,
+    RpcOrganizationIntegration,
+)
+from sentry.shared_integrations.exceptions import ApiError
+
+INVALID_TEAM = 1
+INVALID_KEY = 2
+VALID_TEAM = 3
 
 
 def _validate_int_field(field: str, cleaned_data: Mapping[str, Any]) -> int | None:
@@ -46,25 +55,67 @@ def __init__(self, *args, **kwargs):
         self.fields["team"].choices = teams
         self.fields["team"].widget.choices = self.fields["team"].choices
 
-    def _team_is_valid(
-        self, team_id: str | None, org_integration: RpcOrganizationIntegration | None
-    ) -> bool:
-        return True if get_team(team_id, org_integration) is not None else False
+    def _get_team_status(
+        self,
+        team_id: str | None,
+        integration: RpcIntegration,
+        org_integration: RpcOrganizationIntegration,
+    ) -> int:
+        team = get_team(team_id, org_integration)
+        if not team:
+            return INVALID_TEAM
+
+        integration_key = team["integration_key"]
+        client = OpsgenieClient(
+            integration=integration,
+            integration_key=integration_key,
+            org_integration_id=org_integration.id,
+        )
+        # the integration should be of type "sentry"
+        # there's no way to authenticate that a key is an integration key
+        # without specifying the type... even though the type is arbitrary
+        # and all integration keys do the same thing
+        try:
+            client.authorize_integration(type="sentry")
+        except ApiError:
+            return INVALID_KEY
+
+        return VALID_TEAM
 
     def _validate_team(self, team_id: str | None, integration_id: int | None) -> None:
         params = {
             "account": dict(self.fields["account"].choices).get(integration_id),
             "team": dict(self.fields["team"].choices).get(team_id),
         }
+        integration = integration_service.get_integration(
+            integration_id=integration_id, provider="opsgenie"
+        )
         org_integration = integration_service.get_organization_integration(
             integration_id=integration_id,
             organization_id=self.org_id,
         )
-
-        if not self._team_is_valid(team_id=team_id, org_integration=org_integration):
+        if integration is None or org_integration is None:
+            raise forms.ValidationError(
+                _("The Opsgenie integration does not exist."),
+                code="invalid_integration",
+                params=params,
+            )
+        team_status = self._get_team_status(
+            team_id=team_id, integration=integration, org_integration=org_integration
+        )
+        if team_status == INVALID_TEAM:
             raise forms.ValidationError(
                 _('The team "%(team)s" does not belong to the %(account)s Opsgenie account.'),
-                code="invalid",
+                code="invalid_team",
+                params=params,
+            )
+        elif team_status == INVALID_KEY:
+            raise forms.ValidationError(
+                _(
+                    'The provided API key is invalid. Please make sure that the Opsgenie API \
+                  key is an integration key of type "Sentry".'
+                ),
+                code="invalid_key",
                 params=params,
             )
 
diff --git a/src/sentry/integrations/opsgenie/client.py b/src/sentry/integrations/opsgenie/client.py
@@ -58,6 +58,12 @@ def get_team_id(self, team_name: str) -> BaseApiResponseX:
         headers = {"Authorization": "GenieKey " + self.integration_key}
         return self.get(path=path, headers=headers, params=params)
 
+    def authorize_integration(self, type: str) -> BaseApiResponseX:
+        body = {"type": type}
+        path = "/integrations/authenticate"
+        headers = {"Authorization": "GenieKey " + self.integration_key}
+        return self.post(path=path, headers=headers, data=body)
+
     def _get_rule_urls(self, group, rules):
         organization = group.project.organization
         rule_urls = []
diff --git a/src/sentry/integrations/opsgenie/integration.py b/src/sentry/integrations/opsgenie/integration.py
@@ -134,31 +134,35 @@ def get_organization_config(self) -> Sequence[Any]:
             {
                 "name": "team_table",
                 "type": "table",
-                "label": "Opsgenie teams with the Sentry integration enabled",
-                "help": "If teams need to be updated, deleted, or added manually please do so here. Alert rules will need to be individually updated for any additions or deletions of teams.",
+                "label": "Opsgenie integrations",
+                "help": "If integration keys need to be updated, deleted, or added manually please do so here. Your keys must be associated with a 'Sentry' Integration in Opsgenie. \
+                Alert rules will need to be individually updated for any key additions or deletions.",
                 "addButtonText": "",
-                "columnLabels": {"team": "Opsgenie Team", "integration_key": "Integration Key"},
+                "columnLabels": {
+                    "team": "Opsgenie Integration",
+                    "integration_key": "Integration Key",
+                },
                 "columnKeys": ["team", "integration_key"],
-                "confirmDeleteMessage": "Any alert rules associated with this team will stop working. The rules will still exist but will show a `removed` team.",
+                "confirmDeleteMessage": "Any alert rules associated with this integration will stop working. The rules will still exist but will show a `removed` team.",
             }
         ]
 
         return fields
 
     def update_organization_config(self, data: MutableMapping[str, Any]) -> None:
-        # get the team ID/test the API key for a newly added row
+        # add the integration ID to a newly added row
+        if not self.org_integration:
+            return
+
         teams = data["team_table"]
         unsaved_teams = [team for team in teams if team["id"] == ""]
+        # this is not instantaneous, so you could add the same team a bunch of times in a row
+        # but I don't anticipate this being too much of an issue
+        added_names = {team["team"] for team in teams if team not in unsaved_teams}
         for team in unsaved_teams:
-            try:
-                client = self.get_client(integration_key=team["integration_key"])
-                resp = client.get_team_id(team_name=team["team"])
-                team["id"] = resp["data"]["id"]
-            except ApiError:
-                raise ValidationError(
-                    {"api_key": ["Could not save due to invalid team name or integration key."]}
-                )
-
+            if team["team"] in added_names:
+                raise ValidationError({"duplicate_name": ["Duplicate team name."]})
+            team["id"] = str(self.org_integration.id) + "-" + team["team"]
         return super().update_organization_config(data)
 
 
diff --git a/tests/sentry/incidents/action_handlers/test_opsgenie.py b/tests/sentry/incidents/action_handlers/test_opsgenie.py
@@ -21,6 +21,7 @@
 
 @freeze_time()
 class OpsgenieActionHandlerTest(FireTest, TestCase):
+    @responses.activate
     def setUp(self):
         self.og_team = {"id": "123-id", "team": "cool-team", "integration_key": "1234-5678"}
         self.integration = Integration.objects.create(
@@ -33,13 +34,25 @@ def setUp(self):
         self.org_integration.config = {"team_table": [self.og_team]}
         self.org_integration.save()
 
+        resp_data = {
+            "result": "Integration [sentry] is valid",
+            "took": 1,
+            "requestId": "hello-world",
+        }
+        responses.add(
+            responses.POST,
+            url="https://api.opsgenie.com/v2/integrations/authenticate",
+            json=resp_data,
+        )
+
         self.action = self.create_alert_rule_trigger_action(
             target_identifier=self.og_team["id"],
             type=AlertRuleTriggerAction.Type.OPSGENIE,
             target_type=AlertRuleTriggerAction.TargetType.SPECIFIC,
             integration=self.integration,
         )
 
+    @responses.activate
     def test_build_incident_attachment(self):
         from sentry.integrations.opsgenie.utils import build_incident_attachment
 
@@ -48,6 +61,16 @@ def test_build_incident_attachment(self):
         update_incident_status(
             incident, IncidentStatus.CRITICAL, status_method=IncidentStatusMethod.RULE_TRIGGERED
         )
+        resp_data = {
+            "result": "Integration [sentry] is valid",
+            "took": 1,
+            "requestId": "hello-world",
+        }
+        responses.add(
+            responses.POST,
+            url="https://api.opsgenie.com/v2/integrations/authenticate",
+            json=resp_data,
+        )
         self.create_alert_rule_trigger_action(
             target_identifier=self.og_team["id"],
             type=AlertRuleTriggerAction.Type.OPSGENIE,
@@ -88,16 +111,19 @@ def run_test(self, incident, method):
             incident, IncidentStatus(incident.status), metric_value
         )
 
+    @responses.activate
     def test_fire_metric_alert(self):
         self.run_fire_test()
 
+    @responses.activate
     def test_fire_metric_alert_multiple_teams(self):
         team2 = {"id": "456-id", "team": "cooler-team", "integration_key": "1234-7890"}
         self.org_integration.config["team_table"].append(team2)
         self.org_integration.save()
 
         self.run_fire_test()
 
+    @responses.activate
     def test_resolve_metric_alert(self):
         self.run_fire_test("resolve")
 
@@ -120,6 +146,7 @@ def test_rule_snoozed(self):
 
         assert len(responses.calls) == 0
 
+    @responses.activate
     @patch("sentry.integrations.opsgenie.utils.logger")
     def test_missing_integration(self, mock_logger):
         alert_rule = self.create_alert_rule()
@@ -138,6 +165,7 @@ def test_missing_integration(self, mock_logger):
             == "Opsgenie integration removed, but the rule is still active."
         )
 
+    @responses.activate
     @patch("sentry.integrations.opsgenie.utils.logger")
     def test_missing_team(self, mock_logger):
         alert_rule = self.create_alert_rule()
diff --git a/tests/sentry/incidents/test_logic.py b/tests/sentry/incidents/test_logic.py
@@ -1630,6 +1630,7 @@ def test_pagerduty_not_existing(self):
                 integration_id=integration.id,
             )
 
+    @responses.activate
     def test_opsgenie(self):
         metadata = {
             "api_key": "1234-ABCD",
@@ -1647,6 +1648,17 @@ def test_opsgenie(self):
         org_integration.config = {"team_table": [team]}
         org_integration.save()
 
+        resp_data = {
+            "result": "Integration [sentry] is valid",
+            "took": 1,
+            "requestId": "hello-world",
+        }
+        responses.add(
+            responses.POST,
+            url="https://api.opsgenie.com/v2/integrations/authenticate",
+            json=resp_data,
+        )
+
         type = AlertRuleTriggerAction.Type.OPSGENIE
         target_type = AlertRuleTriggerAction.TargetType.SPECIFIC
         action = update_alert_rule_trigger_action(
diff --git a/tests/sentry/integrations/opsgenie/test_client.py b/tests/sentry/integrations/opsgenie/test_client.py
@@ -1,6 +1,6 @@
 import responses
 
-from sentry.models import Integration, OrganizationIntegration, Rule
+from sentry.models import Integration, Rule
 from sentry.testutils.cases import APITestCase
 from sentry.utils import json
 
@@ -32,28 +32,27 @@ def test_get_client(self):
         assert client.integration_key == METADATA["api_key"]
 
     @responses.activate
-    def test_get_team_id(self):
+    def test_authorize_integration(self):
         client = self.installation.get_client(integration_key="1234-5678")
 
-        org_integration = OrganizationIntegration.objects.get(
-            organization_id=self.organization.id, integration_id=self.integration.id
-        )
-        org_integration.config = {
-            "team_table": [{"id": "", "team": "cool-team", "integration_key": "1234-5678"}]
+        resp_data = {
+            "result": "Integration [sentry] is valid",
+            "took": 1,
+            "requestId": "hello-world",
         }
-        org_integration.save()
-        resp_data = {"data": {"id": "123-id", "name": "cool-team"}}
-        responses.add(responses.GET, url=f"{client.base_url}/teams/cool-team", json=resp_data)
+        responses.add(
+            responses.POST, url=f"{client.base_url}/integrations/authenticate", json=resp_data
+        )
 
-        resp = client.get_team_id(team_name="cool-team")
+        resp = client.authorize_integration(type="sentry")
         assert resp == resp_data
 
     @responses.activate
     def test_send_notification(self):
         resp_data = {
             "result": "Request will be processed",
-            "took": 0.302,
-            "requestId": "43a29c5c-3dbf-4fa4-9c26-f4f71023e120",
+            "took": 1,
+            "requestId": "hello-world",
         }
         responses.add(responses.POST, url="https://api.opsgenie.com/v2/alerts", json=resp_data)
 
diff --git a/tests/sentry/integrations/opsgenie/test_integration.py b/tests/sentry/integrations/opsgenie/test_integration.py
diff --git a/tests/sentry/integrations/opsgenie/test_notify_action.py b/tests/sentry/integrations/opsgenie/test_notify_action.py
