Move permission checks to view (and tests)

XanderVertegaal · XanderVertegaal · commit 8b739102d917 · 2026-02-27T11:21:29.000+01:00
diff --git a/backend/problem/serializers.py b/backend/problem/serializers.py
@@ -100,7 +100,7 @@ def create(self, validated_data: dict) -> Problem:
         """
         Create a new Problem instance from validated input data.
         """
-        new_user_problem = Problem(dataset=Problem.Dataset.USER)
+        new_user_problem = Problem(dataset=Problem.Dataset.USER, extra_data={})
 
         return self._update_core_problem_fields(new_user_problem, validated_data)
 
diff --git a/backend/problem/serializers_test.py b/backend/problem/serializers_test.py
@@ -117,76 +117,6 @@ def test_blank_hypothesis_invalid():
     assert "hypothesis" in serializer.errors
 
 
-@pytest.mark.django_db
-def test_kb_create_no_permission(user_problem, visitor):
-    """Test that KB annotations cannot be created without permission."""
-    kb_input = [
-        {
-            "entity1": "cat",
-            "entity2": "feline",
-            "relationship": "equal",
-            "notes": "Test note",
-        }
-    ]
-
-    serializer = input_serializer_with_user(visitor)
-    serializer.handle_kb_annotations(user_problem, kb_input)  # type: ignore
-
-    assert (
-        KnowledgeBaseAnnotation.objects.filter(problem=user_problem).count() == 0
-    ), "No KB annotations should be created without permission."
-
-
-@pytest.mark.django_db
-def test_kb_update_no_permission(user_problem, kb_annotation, visitor):
-    """Test that KB annotations cannot be updated without permission."""
-    kb_annotation.problem = user_problem
-    kb_annotation.save()
-    original_entity1 = kb_annotation.entity1
-
-    updated_entity_1 = "updated_entity"
-
-    kb_input = [
-        {
-            "id": kb_annotation.pk,
-            "entity1": updated_entity_1,
-            "entity2": kb_annotation.entity2,
-            "relationship": kb_annotation.relationship,
-        }
-    ]
-
-    # Preconditions
-    assert original_entity1 != updated_entity_1
-
-    serializer = input_serializer_with_user(visitor)
-    serializer.handle_kb_annotations(user_problem, kb_input)  # type: ignore
-
-    # Verify KB annotation was not updated
-    kb_annotation.refresh_from_db()
-    assert (
-        kb_annotation.entity1 == original_entity1
-    ), "KB annotation should not have been modified without permission."
-    assert updated_entity_1 != original_entity1
-
-
-@pytest.mark.django_db
-def test_kb_mark_removed_no_permission(user_problem, kb_annotation, visitor):
-    """Test that KB annotations cannot be marked as removed without permission."""
-    kb_annotation.problem = user_problem
-    kb_annotation.save()
-
-    kb_input = []  # Empty list should mark any existing KB items as removed.
-
-    serializer = input_serializer_with_user(visitor)
-    serializer.handle_kb_annotations(user_problem, kb_input)  # type: ignore
-
-    # Verify KB annotation was not removed
-    kb_annotation.refresh_from_db()
-    assert (
-        kb_annotation.removed_at is None
-    ), "KB annotation should not have been marked as removed without permission."
-
-
 @pytest.mark.django_db
 def test_create_single_kb_annotation(user_problem, annotator):
     """Test creating a single KB annotation for a problem."""
@@ -200,7 +130,7 @@ def test_create_single_kb_annotation(user_problem, annotator):
     ]
 
     serializer = input_serializer_with_user(annotator)
-    serializer.handle_kb_annotations(user_problem, kb_input)
+    serializer.handle_kb_annotations(user_problem, kb_input, annotator)
 
     kb_annotations = KnowledgeBaseAnnotation.objects.filter(problem=user_problem)
     assert kb_annotations.count() == 1, "One KB annotation should have been created."
@@ -231,7 +161,7 @@ def test_update_single_kb_annotation(user_problem, kb_annotation, annotator):
     ]
 
     serializer = input_serializer_with_user(annotator)
-    serializer.handle_kb_annotations(user_problem, kb_input)
+    serializer.handle_kb_annotations(user_problem, kb_input, annotator)
 
     # Verify KB annotation was updated
     kb_annotation.refresh_from_db()
@@ -251,7 +181,7 @@ def test_mark_kb_annotation_as_removed(user_problem, kb_annotation, annotator):
     kb_input = []  # Empty list should mark existing as removed
 
     serializer = input_serializer_with_user(annotator)
-    serializer.handle_kb_annotations(user_problem, kb_input)
+    serializer.handle_kb_annotations(user_problem, kb_input, annotator)
 
     # Verify KB annotation was marked as removed
     kb_annotation.refresh_from_db()
@@ -287,7 +217,7 @@ def test_create_and_update_multiple_kb_annotations(
     ]
 
     serializer = input_serializer_with_user(annotator)
-    serializer.handle_kb_annotations(user_problem, kb_input)
+    serializer.handle_kb_annotations(user_problem, kb_input, annotator)
 
     kb_annotations = KnowledgeBaseAnnotation.objects.filter(
         problem=user_problem, removed_at__isnull=True
@@ -337,7 +267,6 @@ def test_create_update_and_remove_kb_annotations(
         created_by=annotator,
     )
 
-    # request = Mock(user=annotator)
     kb_input = [
         {
             "id": kb1.pk,
@@ -353,7 +282,7 @@ def test_create_update_and_remove_kb_annotations(
     ]
 
     serializer = input_serializer_with_user(annotator)
-    serializer.handle_kb_annotations(user_problem, kb_input)
+    serializer.handle_kb_annotations(user_problem, kb_input, annotator)
 
     # Verify kb1 was updated.
     kb1.refresh_from_db()
@@ -379,146 +308,3 @@ def test_create_update_and_remove_kb_annotations(
     assert new_annotation.entity2 == "new_e2"
     assert new_annotation.relationship == "superset"
 
-
-@pytest.mark.django_db
-def test_create_problem_with_kb_annotations(annotator):
-    """Test creating a problem with KB annotations through create()."""
-    data = {
-        "premises": ["A cat is running."],
-        "hypothesis": "A cat is moving.",
-        "kbItems": [
-            {
-                "entity1": "cat",
-                "entity2": "feline",
-                "relationship": "equal",
-                "notes": "Test note",
-            },
-            {
-                "entity1": "running",
-                "entity2": "moving",
-                "relationship": "subset",
-            },
-        ],
-    }
-
-    serializer = input_serializer_with_user(annotator, data=data)
-    assert serializer.is_valid(raise_exception=True)
-    problem = serializer.save()
-
-    # Verify problem was created
-    assert problem.pk is not None
-    assert problem.dataset == Problem.Dataset.USER
-    assert problem.hypothesis.text == "A cat is moving."
-    assert problem.premises.count() == 1
-    assert problem.premises.first().text == "A cat is running."
-
-    # Verify KB annotations were created
-    kb_annotations = KnowledgeBaseAnnotation.objects.filter(problem=problem)
-    assert kb_annotations.count() == 2
-
-    kb1 = kb_annotations.get(entity1="cat")
-    assert kb1.entity2 == "feline"
-    assert kb1.relationship == "equal"
-    assert kb1.notes == "Test note"
-    assert kb1.created_by == annotator
-
-    kb2 = kb_annotations.get(entity1="running")
-    assert kb2.entity2 == "moving"
-    assert kb2.relationship == "subset"
-    assert kb2.created_by == annotator
-
-
-@pytest.mark.django_db
-def test_create_problem_with_multiple_premises_and_kb(annotator):
-    """Test creating a problem with multiple premises."""
-    data = {
-        "premises": ["Birds can fly.", "Penguins are birds."],
-        "hypothesis": "Penguins can fly.",
-    }
-
-    serializer = input_serializer_with_user(annotator, data=data)
-    assert serializer.is_valid(raise_exception=True)
-    problem = serializer.save()
-
-    assert problem.premises.count() == 2
-    premise_texts = [p.text for p in problem.premises.all()]
-    assert "Birds can fly." in premise_texts
-    assert "Penguins are birds." in premise_texts
-    assert problem.hypothesis.text == "Penguins can fly."
-
-
-@pytest.mark.django_db
-def test_update_user_problem_with_new_kb_annotations(user_problem, annotator):
-    """Test updating a user problem adds new KB annotations through update()."""
-    data = {
-        "id": user_problem.pk,
-        "premises": ["Updated premise."],
-        "hypothesis": "Updated hypothesis.",
-        "kbItems": [
-            {
-                "entity1": "new_entity1",
-                "entity2": "new_entity2",
-                "relationship": "subset",
-            }
-        ],
-    }
-
-    assert (
-        KnowledgeBaseAnnotation.objects.filter(problem=user_problem).count() == 0
-    ), "Precondition: user_problem should have no KB annotations."
-
-    serializer = input_serializer_with_user(annotator, data=data, instance=user_problem)
-    assert serializer.is_valid(raise_exception=True)
-    updated_problem = serializer.save()
-
-    # Verify problem was updated
-    assert updated_problem.pk == user_problem.pk
-    assert updated_problem.hypothesis.text == "Updated hypothesis."
-    assert updated_problem.premises.first().text == "Updated premise."
-
-    # Verify KB annotation was added
-    kb_annotations = KnowledgeBaseAnnotation.objects.filter(problem=updated_problem)
-    assert kb_annotations.count() == 1
-    kb_annotation = kb_annotations.first()
-    assert kb_annotation is not None
-    assert kb_annotation.entity1 == "new_entity1"
-
-
-@pytest.mark.django_db
-def test_update_non_user_problem_adds_kb_only(non_user_problem, annotator):
-    """Test updating a non-user problem only adds KB annotations, not other fields."""
-    original_hypothesis = non_user_problem.hypothesis.text
-    original_premise_count = non_user_problem.premises.count()
-
-    data = {
-        "id": non_user_problem.pk,
-        "premises": ["This should be ignored."],
-        "hypothesis": "This should also be ignored.",
-        "kbItems": [
-            {
-                "entity1": "entity1",
-                "entity2": "entity2",
-                "relationship": "equal",
-            }
-        ],
-    }
-
-    # Preconditions
-    assert (
-        KnowledgeBaseAnnotation.objects.filter(problem=non_user_problem).count() == 0
-    ), "Non_user_problem should have no KB annotations to begin with."
-    assert original_hypothesis != data["hypothesis"]
-
-    serializer = input_serializer_with_user(
-        annotator, data=data, instance=non_user_problem
-    )
-    assert serializer.is_valid(raise_exception=True)
-    updated_problem = serializer.save()
-
-    # Verify problem fields were not updated
-    assert updated_problem.hypothesis.text == original_hypothesis
-    assert updated_problem.premises.count() == original_premise_count
-
-    # Verify KB annotation was added
-    kb_annotations = KnowledgeBaseAnnotation.objects.filter(problem=updated_problem)
-    assert kb_annotations.count() == 1
diff --git a/backend/problem/views/problem.py b/backend/problem/views/problem.py
@@ -2,7 +2,12 @@
 from rest_framework.request import Request
 from rest_framework.response import Response
 from rest_framework.viewsets import ModelViewSet
-from rest_framework.status import HTTP_201_CREATED, HTTP_200_OK, HTTP_401_UNAUTHORIZED, HTTP_403_FORBIDDEN
+from rest_framework.status import (
+    HTTP_201_CREATED,
+    HTTP_200_OK,
+    HTTP_401_UNAUTHORIZED,
+    HTTP_403_FORBIDDEN,
+)
 from rest_framework.permissions import IsAuthenticated, IsAuthenticatedOrReadOnly
 
 from django.shortcuts import get_object_or_404
@@ -173,13 +178,22 @@ def _handle_update_create_problem(
                 )
             problem = serializer.create(validated_input)  # type: ignore
             status = HTTP_201_CREATED
+
+        elif not (user.can_edit_problem or user.can_edit_kb):
+                return Response(
+                    {
+                        "detail": "User is not authorized to edit problems or knowledge base annotations."
+                    },
+                    status=HTTP_403_FORBIDDEN,
+                )
         
         else:
-            problem_instance = get_object_or_404(Problem, id=problem_id)
-            problem: Problem = serializer.update(problem_instance, validated_input) if user.can_edit_problem else problem_instance
-            
+            problem = get_object_or_404(Problem, id=problem_id)
+            if user.can_edit_problem:
+                problem: Problem = serializer.update(problem, validated_input)  # type: ignore
+
         kb_items = validated_input.get("kbItems", [])
-        if kb_items:
-            serializer.handle_kb_annotations(problem=problem, kb_items=kb_items, user=user)
+        if user.can_edit_kb:
+            serializer.handle_kb_annotations(problem=problem, kb_items=kb_items, user=user)  # type: ignore
 
         return Response({"id": problem.pk}, status=status)
diff --git a/backend/problem/views/problem_test.py b/backend/problem/views/problem_test.py
