Restrict integration name uniqueness per team (#3863)

Related to https://github.com/grafana/oncall/issues/3858 We still need to improve error messages in the UI, [to be discussed](https://raintank-corp.slack.com/archives/C04JCU51NF8/p1707486122947369).
2024-02-12 08:40:08 -03:00 · 2024-02-12 08:40:08 -03:00 · 0007fc69b7
commit 0007fc69b7
parent 2a46152477
5 changed files with 193 additions and 15 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Allow mobile app to access escalation options endpoints @imtoori ([#3847](https://github.com/grafana/oncall/pull/3847))
 - Enable templating for alert escalation mobile app push notifications by @joeyorlando ([#3845](https://github.com/grafana/oncall/pull/3845))
 - Change email notification template to not wrap user template @mderynck ([#3862](https://github.com/grafana/oncall/pull/3862))
+- Update integration name uniqueness check to be per team ([#3863](https://github.com/grafana/oncall/pull/3863))

 ### Fixed

--- a/engine/apps/api/serializers/alert_receive_channel.py
+++ b/engine/apps/api/serializers/alert_receive_channel.py
@ -287,6 +287,27 @@ class AlertReceiveChannelSerializer(
        ]
        extra_kwargs = {"integration": {"required": True}}

+    def validate(self, data):
+        validated_data = super().validate(data)
+        organization = self.context["request"].auth.organization
+        verbal_name = validated_data.get("verbal_name")
+        team = validated_data.get("team")
+        try:
+            obj = AlertReceiveChannel.objects.get(organization=organization, team=team, verbal_name=verbal_name)
+        except AlertReceiveChannel.DoesNotExist:
+            pass
+        except AlertReceiveChannel.MultipleObjectsReturned:
+            raise serializers.ValidationError(
+                {"verbal_name": "An integration with this name already exists for this team"}
+            )
+        else:
+            if self.instance is None or obj.id != self.instance.id:
+                raise serializers.ValidationError(
+                    {"verbal_name": "An integration with this name already exists for this team"}
+                )
+
+        return validated_data
+
    def create(self, validated_data):
        organization = self.context["request"].auth.organization
        integration = validated_data.get("integration")
@ -356,19 +377,6 @@ class AlertReceiveChannelSerializer(

        return integration

-    def validate_verbal_name(self, verbal_name):
-        organization = self.context["request"].auth.organization
-        if verbal_name is None or (self.instance and verbal_name == self.instance.verbal_name):
-            return verbal_name
-        try:
-            obj = AlertReceiveChannel.objects.get(organization=organization, verbal_name=verbal_name)
-        except AlertReceiveChannel.DoesNotExist:
-            return verbal_name
-        if self.instance and obj.id == self.instance.id:
-            return verbal_name
-        else:
-            raise serializers.ValidationError(detail="Integration with this name already exists")
-
    def get_allow_delete(self, obj: "AlertReceiveChannel") -> bool:
        # don't allow deleting direct paging integrations
        return obj.integration != AlertReceiveChannel.INTEGRATION_DIRECT_PAGING
--- a/engine/apps/api/tests/test_alert_receive_channel.py
+++ b/engine/apps/api/tests/test_alert_receive_channel.py
@ -150,6 +150,100 @@ def test_create_alert_receive_channel(alert_receive_channel_internal_api_setup,
    assert response.status_code == status.HTTP_201_CREATED


+@pytest.mark.django_db
+def test_alert_receive_channel_name_uniqueness(
+    alert_receive_channel_internal_api_setup,
+    make_team,
+    make_user_auth_headers,
+):
+    user, token, alert_receive_channel = alert_receive_channel_internal_api_setup
+    client = APIClient()
+
+    url = reverse("api-internal:alert_receive_channel-list")
+    data = {
+        "integration": AlertReceiveChannel.INTEGRATION_GRAFANA,
+        "team": alert_receive_channel.team.public_primary_key if alert_receive_channel.team else None,
+        "verbal_name": alert_receive_channel.verbal_name,
+    }
+    response = client.post(url, data, format="json", **make_user_auth_headers(user, token))
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
+
+    # name can be reused in a different team
+    another_team = make_team(alert_receive_channel.organization)
+    data = {
+        "integration": AlertReceiveChannel.INTEGRATION_GRAFANA,
+        "team": another_team.public_primary_key,
+        "verbal_name": alert_receive_channel.verbal_name,
+    }
+    response = client.post(url, data, format="json", **make_user_auth_headers(user, token))
+    assert response.status_code == status.HTTP_201_CREATED
+
+    # update works
+    url = reverse("api-internal:alert_receive_channel-detail", kwargs={"pk": alert_receive_channel.public_primary_key})
+    response = client.put(
+        url,
+        data=json.dumps(
+            {
+                "team": alert_receive_channel.team,
+                "verbal_name": alert_receive_channel.verbal_name,
+                "description": "update description",
+            }
+        ),
+        content_type="application/json",
+        **make_user_auth_headers(user, token),
+    )
+    assert response.status_code == status.HTTP_200_OK
+
+    # but updating team will fail if name exists
+    response = client.put(
+        url,
+        data=json.dumps(
+            {
+                "team": another_team.public_primary_key,
+                "verbal_name": alert_receive_channel.verbal_name,
+            }
+        ),
+        content_type="application/json",
+        **make_user_auth_headers(user, token),
+    )
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
+
+
+@pytest.mark.django_db
+def test_alert_receive_channel_name_duplicated(
+    alert_receive_channel_internal_api_setup,
+    make_alert_receive_channel,
+    make_user_auth_headers,
+):
+    # this could happen in case a team is removed and integrations are set to have "no team"
+    user, token, alert_receive_channel = alert_receive_channel_internal_api_setup
+    # another integration with the same verbal name
+    make_alert_receive_channel(
+        alert_receive_channel.organization,
+        verbal_name=alert_receive_channel.verbal_name,
+    )
+
+    client = APIClient()
+
+    # updating team will require changing the name or the team
+    url = reverse("api-internal:alert_receive_channel-detail", kwargs={"pk": alert_receive_channel.public_primary_key})
+    response = client.put(
+        url,
+        data=json.dumps({"verbal_name": alert_receive_channel.verbal_name}),
+        content_type="application/json",
+        **make_user_auth_headers(user, token),
+    )
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
+
+    response = client.put(
+        url,
+        data=json.dumps({"verbal_name": "a new name"}),
+        content_type="application/json",
+        **make_user_auth_headers(user, token),
+    )
+    assert response.status_code == status.HTTP_200_OK
+
+
@pytest.mark.django_db
 def test_create_invalid_alert_receive_channel(alert_receive_channel_internal_api_setup, make_user_auth_headers):
    user, token, _ = alert_receive_channel_internal_api_setup
--- a/engine/apps/public_api/serializers/integrations.py
+++ b/engine/apps/public_api/serializers/integrations.py
@ -149,16 +149,24 @@ class IntegrationSerializer(EagerLoadingMixin, serializers.ModelSerializer, Main
    def validate(self, attrs):
        organization = self.context["request"].auth.organization
        verbal_name = attrs.get("verbal_name", None)
+        team = attrs.get("team", None)
        if verbal_name is None:
            return attrs
        try:
-            obj = AlertReceiveChannel.objects.get(organization=organization, verbal_name=verbal_name)
+            obj = AlertReceiveChannel.objects.get(
+                organization=organization,
+                team=team,
+                verbal_name=verbal_name,
+            )
        except AlertReceiveChannel.DoesNotExist:
            return attrs
+        except AlertReceiveChannel.MultipleObjectsReturned:
+            raise BadRequest(detail="An integration with this name already exists for this team")
+
        if self.instance and obj.id == self.instance.id:
            return attrs
        else:
-            raise BadRequest(detail="Integration with this name already exists")
+            raise BadRequest(detail="An integration with this name already exists for this team")

    def validate_templates(self, templates):
        if not isinstance(templates, dict):
--- a/engine/apps/public_api/tests/test_integrations.py
+++ b/engine/apps/public_api/tests/test_integrations.py
@ -104,6 +104,73 @@ def test_create_integration(
    assert response.status_code == status.HTTP_201_CREATED


+@pytest.mark.django_db
+def test_integration_name_uniqueness(
+    make_organization_and_user_with_token,
+    make_team,
+):
+    organization, _, token = make_organization_and_user_with_token()
+
+    client = APIClient()
+    data = {
+        "type": "grafana",
+        "name": "grafana_created",
+        "team_id": None,
+    }
+    url = reverse("api-public:integrations-list")
+    response = client.post(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}")
+    assert response.status_code == status.HTTP_201_CREATED
+    integration_pk = response.data["id"]
+
+    # cannot create another one with the same name
+    response = client.post(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}")
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
+
+    # but name can be reused in a different team
+    another_team = make_team(organization)
+    data["team_id"] = another_team.public_primary_key
+    response = client.post(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}")
+    assert response.status_code == status.HTTP_201_CREATED
+
+    # update works
+    url = reverse("api-public:integrations-detail", args=[integration_pk])
+    data["team_id"] = None
+    data["description"] = "some description"
+    response = client.put(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}")
+    assert response.status_code == status.HTTP_200_OK
+
+    # but updating team will fail if name exists
+    data["team_id"] = another_team.public_primary_key
+    response = client.put(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}")
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
+
+
+@pytest.mark.django_db
+def test_integration_name_duplicated(
+    make_organization_and_user_with_token,
+    make_alert_receive_channel,
+    make_user_auth_headers,
+):
+    # this could happen in case a team is removed and integrations are set to have "no team"
+    organization, _, token = make_organization_and_user_with_token()
+    integration = make_alert_receive_channel(organization)
+    # duplicated name integration
+    make_alert_receive_channel(organization, verbal_name=integration.verbal_name)
+
+    client = APIClient()
+
+    # updating team will require changing the name or the team
+    data = {"name": integration.verbal_name}
+    url = reverse("api-public:integrations-detail", args=[integration.public_primary_key])
+    response = client.put(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}")
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
+
+    # but updating team will fail if name exists
+    data["name"] = "a new name"
+    response = client.put(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}")
+    assert response.status_code == status.HTTP_200_OK
+
+
@pytest.mark.django_db
 def test_create_integrations_with_none_templates(
    make_organization_and_user_with_token,