From 974dfc57537003626ba10cdab98f394dbdf69ab7 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 13 Mar 2024 17:48:02 +0000 Subject: [PATCH] feat(tags): prevent system tag modifications (#3605) --- api/conftest.py | 25 +++++++--- api/projects/tags/serializers.py | 7 +++ api/projects/tags/views.py | 14 +++++- ...ws.py => test_unit_projects_tags_views.py} | 50 +++++++++++++++++++ 4 files changed, 88 insertions(+), 8 deletions(-) rename api/tests/unit/projects/tags/{test_views.py => test_unit_projects_tags_views.py} (59%) diff --git a/api/conftest.py b/api/conftest.py index df1fd16de462..77ef92533e07 100644 --- a/api/conftest.py +++ b/api/conftest.py @@ -165,13 +165,20 @@ def chargebee_subscription(organisation: Organisation) -> Subscription: @pytest.fixture() -def project(organisation): - return Project.objects.create(name="Test Project", organisation=organisation) +def tag(project): + return Tag.objects.create(label="tag", project=project, color="#000000") @pytest.fixture() -def tag(project): - return Tag.objects.create(label="tag", project=project, color="#000000") +def system_tag(project: Project) -> Tag: + return Tag.objects.create( + label="system-tag", project=project, color="#FFFFFF", is_system_tag=True + ) + + +@pytest.fixture() +def project(organisation): + return Project.objects.create(name="Test Project", organisation=organisation) @pytest.fixture() @@ -229,13 +236,17 @@ def with_project_permissions( """ def _with_project_permissions( - permission_keys: list[str], project_id: typing.Optional[int] = None + permission_keys: list[str] = None, + project_id: typing.Optional[int] = None, + admin: bool = False, ) -> UserProjectPermission: project_id = project_id or project.id upp, __ = UserProjectPermission.objects.get_or_create( - project_id=project_id, user=staff_user + project_id=project_id, user=staff_user, admin=admin ) - upp.permissions.add(*permission_keys) + + if permission_keys: + upp.permissions.add(*permission_keys) return upp diff --git a/api/projects/tags/serializers.py b/api/projects/tags/serializers.py index cdebfd1f735c..c86f4ca09299 100644 --- a/api/projects/tags/serializers.py +++ b/api/projects/tags/serializers.py @@ -1,3 +1,5 @@ +from typing import Any + from rest_framework import serializers from projects.tags.models import Tag @@ -18,3 +20,8 @@ class Meta: "type", ) read_only_fields = ("project", "uuid", "is_system_tag", "type") + + def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: + if self.instance and self.instance.is_system_tag: + raise serializers.ValidationError("Cannot update a system tag.") + return super().validate(attrs) diff --git a/api/projects/tags/views.py b/api/projects/tags/views.py index d87e96a608bc..49b163316240 100644 --- a/api/projects/tags/views.py +++ b/api/projects/tags/views.py @@ -1,4 +1,4 @@ -from rest_framework import viewsets +from rest_framework import status, viewsets from rest_framework.decorators import action from rest_framework.generics import get_object_or_404 from rest_framework.permissions import IsAuthenticated @@ -36,6 +36,18 @@ def perform_update(self, serializer): project_id = int(self.kwargs["project_pk"]) serializer.save(project_id=project_id) + def destroy(self, request: Request, *args, **kwargs): + instance = self.get_object() + + if instance.is_system_tag: + return Response( + status=status.HTTP_400_BAD_REQUEST, + data={"message": "Cannot delete a system tag."}, + ) + + self.perform_destroy(instance) + return Response(status=status.HTTP_204_NO_CONTENT) + @action( detail=False, url_path=r"get-by-uuid/(?P[0-9a-f-]+)", diff --git a/api/tests/unit/projects/tags/test_views.py b/api/tests/unit/projects/tags/test_unit_projects_tags_views.py similarity index 59% rename from api/tests/unit/projects/tags/test_views.py rename to api/tests/unit/projects/tags/test_unit_projects_tags_views.py index 3401f9887265..d1942cdbb6f6 100644 --- a/api/tests/unit/projects/tags/test_views.py +++ b/api/tests/unit/projects/tags/test_unit_projects_tags_views.py @@ -1,3 +1,5 @@ +import json + import pytest from django.urls import reverse from pytest_lazyfixture import lazy_fixture @@ -68,3 +70,51 @@ def test_get_tag_by__uuid_returns_200_for_user_with_view_project_permission( # Then assert response.status_code == status.HTTP_200_OK assert response.json()["uuid"] == str(tag.uuid) + + +def test_cannot_delete_a_system_tag( + staff_client: APIClient, + with_project_permissions: WithProjectPermissionsCallable, + project: Project, + system_tag: Tag, +) -> None: + # Given + url = reverse("api-v1:projects:tags-detail", args=(project.id, system_tag.id)) + + with_project_permissions(admin=True) + + # When + response = staff_client.delete(url) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json() == {"message": "Cannot delete a system tag."} + + +def test_cannot_update_a_system_tag( + staff_client: APIClient, + with_project_permissions: WithProjectPermissionsCallable, + project: Project, + system_tag: Tag, +): + # Given + url = reverse("api-v1:projects:tags-detail", args=(project.id, system_tag.id)) + + with_project_permissions(admin=True) + + updated_label = f"{system_tag.label} updated" + + data = { + "id": system_tag.id, + "color": system_tag.color, + "label": updated_label, + "project": system_tag.project_id, + "description": "", + } + + # When + response = staff_client.put(url, json.dumps(data), content_type="application/json") + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json() == {"non_field_errors": ["Cannot update a system tag."]}