Notes: API perms for read only users + note history tracking (#14284)

* Add engagement, finding, and test note permissions

* Update note permissions in Engagement, Finding, and Test viewsets

* Add note history tracking to Engagement, Finding, and Test viewsets

* add tests

* Correct test memory issue

* Apply suggestions from code review

Author: Maffooch

dojo/api_v2/permissions.py

@@ -385,6 +385,15 @@ class UserHasEngagementRelatedObjectPermission(BaseRelatedObjectPermission):
     }
 
 
+class UserHasEngagementNotePermission(BaseRelatedObjectPermission):
+    permission_map = {
+        "get_permission": Permissions.Engagement_View,
+        "put_permission": Permissions.Engagement_Edit,
+        "delete_permission": Permissions.Engagement_Edit,
+        "post_permission": Permissions.Engagement_View,
+    }
+
+
 class UserHasRiskAcceptancePermission(permissions.BasePermission):
     def has_permission(self, request, view):
         # The previous implementation only checked for the object permission if the path was
@@ -437,6 +446,15 @@ class UserHasFindingRelatedObjectPermission(BaseRelatedObjectPermission):
     }
 
 
+class UserHasFindingNotePermission(BaseRelatedObjectPermission):
+    permission_map = {
+        "get_permission": Permissions.Finding_View,
+        "put_permission": Permissions.Finding_Edit,
+        "delete_permission": Permissions.Finding_Edit,
+        "post_permission": Permissions.Finding_View,
+    }
+
+
 class UserHasImportPermission(permissions.BasePermission):
     def has_permission(self, request, view):
         # permission check takes place before validation, so we don't have access to serializer.validated_data()
@@ -817,6 +835,15 @@ class UserHasTestRelatedObjectPermission(BaseRelatedObjectPermission):
     }
 
 
+class UserHasTestNotePermission(BaseRelatedObjectPermission):
+    permission_map = {
+        "get_permission": Permissions.Test_View,
+        "put_permission": Permissions.Test_Edit,
+        "delete_permission": Permissions.Test_Edit,
+        "post_permission": Permissions.Test_View,
+    }
+
+
 class UserHasTestImportPermission(permissions.BasePermission):
     def has_permission(self, request, view):
         return check_post_permission(

dojo/api_v2/views.py

@@ -121,6 +121,7 @@
     Languages,
     Network_Locations,
     Note_Type,
+    NoteHistory,
     Notes,
     Notification_Webhooks,
     Notifications,
@@ -504,7 +505,7 @@ def generate_report(self, request, pk=None):
         request=serializers.AddNewNoteOptionSerializer,
         responses={status.HTTP_201_CREATED: serializers.NoteSerializer},
     )
-    @action(detail=True, methods=["get", "post"], permission_classes=[IsAuthenticated, permissions.UserHasEngagementRelatedObjectPermission])
+    @action(detail=True, methods=["get", "post"], permission_classes=[IsAuthenticated, permissions.UserHasEngagementNotePermission])
     def notes(self, request, pk=None):
         engagement = self.get_object()
         if request.method == "POST":
@@ -532,6 +533,10 @@ def notes(self, request, pk=None):
                 note_type=note_type,
             )
             note.save()
+            # Add an entry to the note history
+            history = NoteHistory.objects.create(data=note.entry, time=note.date, current_editor=note.author)
+            note.history.add(history)
+            # Now add the note to the object
             engagement.notes.add(note)
             # Determine if we need to send any notifications for user mentioned
             process_tag_notifications(
@@ -1096,7 +1101,7 @@ def request_response(self, request, pk=None):
         request=serializers.AddNewNoteOptionSerializer,
         responses={status.HTTP_201_CREATED: serializers.NoteSerializer},
     )
-    @action(detail=True, methods=["get", "post"], permission_classes=(IsAuthenticated, permissions.UserHasFindingRelatedObjectPermission))
+    @action(detail=True, methods=["get", "post"], permission_classes=(IsAuthenticated, permissions.UserHasFindingNotePermission))
     def notes(self, request, pk=None):
         finding = self.get_object()
         if request.method == "POST":
@@ -1125,6 +1130,10 @@ def notes(self, request, pk=None):
                 note_type=note_type,
             )
             note.save()
+            # Add an entry to the note history
+            history = NoteHistory.objects.create(data=note.entry, time=note.date, current_editor=note.author)
+            note.history.add(history)
+            # Now add the note to the object
             finding.last_reviewed = note.date
             finding.last_reviewed_by = author
             finding.save(update_fields=["last_reviewed", "last_reviewed_by", "updated"])
@@ -1226,7 +1235,7 @@ def download_file(self, request, file_id, pk=None):
         request=serializers.FindingNoteSerializer,
         responses={status.HTTP_204_NO_CONTENT: ""},
     )
-    @action(detail=True, methods=["patch"], permission_classes=(IsAuthenticated, permissions.UserHasFindingRelatedObjectPermission))
+    @action(detail=True, methods=["patch"], permission_classes=(IsAuthenticated, permissions.UserHasFindingNotePermission))
     def remove_note(self, request, pk=None):
         """Remove Note From Finding Note"""
         finding = self.get_object()
@@ -2162,7 +2171,7 @@ def generate_report(self, request, pk=None):
         request=serializers.AddNewNoteOptionSerializer,
         responses={status.HTTP_201_CREATED: serializers.NoteSerializer},
     )
-    @action(detail=True, methods=["get", "post"], permission_classes=(IsAuthenticated, permissions.UserHasTestRelatedObjectPermission))
+    @action(detail=True, methods=["get", "post"], permission_classes=(IsAuthenticated, permissions.UserHasTestNotePermission))
     def notes(self, request, pk=None):
         test = self.get_object()
         if request.method == "POST":
@@ -2190,6 +2199,10 @@ def notes(self, request, pk=None):
                 note_type=note_type,
             )
             note.save()
+            # Add an entry to the note history
+            history = NoteHistory.objects.create(data=note.entry, time=note.date, current_editor=note.author)
+            note.history.add(history)
+            # Now add the note to the object
             test.notes.add(note)
             # Determine if we need to send any notifications for user mentioned
             process_tag_notifications(

unittests/test_rest_framework.py

@@ -856,6 +856,63 @@ def test_detail_configuration_not_authorized(self):
             response = self.client.get(relative_url)
             self.assertEqual(200, response.status_code, response.content[:1000])
 
+    class RelatedObjectsTest(BaseClassTest):
+        def test_notes_can_be_added_by_users_with_read_only_permissions(self):
+            self.setUp_global_reader()
+            response = self.client.get(self.url, format="json")
+            self.assertEqual(200, response.status_code, response.content[:1000])
+            engagement_id = response.data["results"][0]["id"]
+            # Attempt to add a note with reader permissions
+            relative_url = f"{self.url}{engagement_id}/notes/"
+            response = self.client.post(relative_url, {"entry": "string"})
+            self.assertEqual(201, response.status_code, response.content[:1000])
+
+        @parameterized.expand(
+            [
+                ("files", {"title": "test"}),
+                ("tags", {"tags": ["apple", "banana", "cherry"]}),
+            ],
+        )
+        def test_related_objects(self, related_object_path, payload):
+            """
+            Tests that BaseRelatedObjectPermission enforces the permissions not associated
+            with the base object. For example, even though a request to add a tag to an
+            engagement is a POST, we do not need engagement add permissions, but rather
+            engagement edit permissions since that is what is defined in the
+            UserHasEngagementRelatedObjectPermission class
+            """
+            self.setUp_global_reader()
+            # Skip tags for engagement and tests
+            if related_object_path == "tags" and self.endpoint_model in {Engagement, Test}:
+                return
+            # Get an object
+            response = self.client.get(self.url, format="json")
+            self.assertEqual(200, response.status_code, response.content[:1000])
+            object_id = response.data["results"][0]["id"]
+            # Attempt to add a related object
+            relative_url = f"{self.url}{object_id}/{related_object_path}/"
+            response = self.client.post(relative_url, payload)
+            self.assertEqual(403, response.status_code, response.content[:1000])
+            # Now switch to a user with edit permissions (but not create)
+            self.setUp_global_writer()
+            # Retry adding the related object
+            if related_object_path == "files":
+                # Convert bytes to a mock uploaded file
+                response = self.client.post(
+                    relative_url,
+                    {
+                        "file": SimpleUploadedFile(
+                            name="test_file.txt",
+                            content=b"empty",
+                            content_type="text/plain",
+                        ),
+                        **payload,
+                    },
+                )
+            else:
+                response = self.client.post(relative_url, payload)
+            self.assertIn(response.status_code, [200, 201], response.content[:1000])
+
 
 @versioned_fixtures
 class AppAnalysisTest(BaseClass.BaseClassTest):
@@ -1487,7 +1544,7 @@ def test_update_object_not_authorized(self):
 
 
 @versioned_fixtures
-class EngagementTest(BaseClass.BaseClassTest):
+class EngagementTest(BaseClass.RelatedObjectsTest, BaseClass.BaseClassTest):
     fixtures = ["dojo_testdata.json"]
 
     def __init__(self, *args, **kwargs):
@@ -1517,42 +1574,6 @@ def __init__(self, *args, **kwargs):
         self.deleted_objects = 23
         BaseClass.RESTEndpointTest.__init__(self, *args, **kwargs)
 
-    @parameterized.expand(
-        [
-            ("files", {"title": "test", "file": b"empty"}),
-            ("notes", {"entry": "string"}),
-        ],
-    )
-    def test_related_objects(self, related_object_path, payload):
-        """
-        Tests that BaseRelatedObjectPermission enforces the permissions not associated
-        with the base object. For example, even though a request to add a note to an
-        engagement is a POST, we do not need engagement add permissions, but rather
-        engagement edit permissions since that is what is defined in the
-        UserHasEngagementRelatedObjectPermission class
-        """
-        self.setUp_global_reader()
-        # Get an engagement
-        response = self.client.get(self.url, format="json")
-        self.assertEqual(200, response.status_code, response.content[:1000])
-        engagement_id = response.data["results"][0]["id"]
-        # Attempt to add a related object
-        relative_url = f"{self.url}{engagement_id}/{related_object_path}/"
-        response = self.client.post(relative_url, payload)
-        self.assertEqual(403, response.status_code, response.content[:1000])
-        # Now switch to a user with edit permissions (but not create)
-        self.setUp_global_writer()
-        # Retry adding the related object
-        if related_object_path == "files":
-            # Convert bytes to a mock uploaded file
-            payload["file"] = SimpleUploadedFile(
-                name="test_file.txt",
-                content=payload["file"],  # the b"empty"
-                content_type="text/plain",
-            )
-        response = self.client.post(relative_url, payload)
-        self.assertEqual(201, response.status_code, response.content[:1000])
-
 
 @versioned_fixtures
 class RiskAcceptanceTest(BaseClass.BaseClassTest):
@@ -1726,7 +1747,7 @@ def test_file_with_quoted_name(self):
 
 
 @versioned_fixtures
-class FindingsTest(BaseClass.BaseClassTest):
+class FindingsTest(BaseClass.RelatedObjectsTest, BaseClass.BaseClassTest):
     fixtures = ["dojo_testdata.json"]
 
     def __init__(self, *args, **kwargs):
@@ -2415,7 +2436,7 @@ def test_severity_validation(self):
 
 
 @versioned_fixtures
-class TestsTest(BaseClass.BaseClassTest):
+class TestsTest(BaseClass.RelatedObjectsTest, BaseClass.BaseClassTest):
     fixtures = ["dojo_testdata.json"]
 
     def __init__(self, *args, **kwargs):