Fix various file upload validation omissions

contentcuration/contentcuration/models.py

@@ -1891,6 +1891,8 @@ def mark_complete(self):  # noqa C901
                 errors.append("Missing required copyright holder")
             if self.kind_id != content_kinds.EXERCISE and not self.files.filter(preset__supplementary=False).exists():
                 errors.append("Missing default file")
+            if self.files.exclude(preset__kind_id=self.kind_id).exists():
+                errors.append("Node has files that do not match its kind")
             if self.kind_id == content_kinds.EXERCISE:
                 # Check to see if the exercise has at least one assessment item that has:
                 if not self.assessment_items.filter(

contentcuration/contentcuration/tests/views/test_views_internal.py

@@ -93,7 +93,7 @@ def _make_node_data(self):
                     "source_url": fileobj.source_url,
                 }
             ],
-            "kind": "document",
+            "kind": "video",
             "license": "CC BY",
             "license_description": None,
             "copyright_holder": random_data.copyright_holder,
@@ -276,6 +276,75 @@ def test_invalid_metadata_label_excluded(self):
 
         self.assertEqual(response.status_code, 400, response.content)
 
+    def test_invalid_file_data_excluded(self):
+        invalid_file_data_node = self._make_node_data()
+        invalid_file_data_node["title"] = "invalid file data title"
+        bad_file = invalid_file_data_node["files"][0].copy()
+        bad_file["preset"] = format_presets.DOCUMENT_THUMBNAIL
+        invalid_file_data_node["files"] += [bad_file]
+        test_data = {
+            "root_id": self.root_node.id,
+            "content_data": [
+                invalid_file_data_node,
+            ],
+        }
+
+        response = self.admin_client().post(
+            reverse_lazy("api_add_nodes_to_tree"), data=test_data, format="json"
+        )
+
+        self.assertEqual(response.status_code, 200, response.content)
+        node = ContentNode.objects.get(title=invalid_file_data_node["title"])
+        self.assertEqual(node.files.count(), 1)
+
+    def test_mismatched_kind_and_preset(self):
+        mismatched_kind_and_preset_node = self._make_node_data()
+        mismatched_kind_and_preset_node["title"] = "invalid file data title"
+        mismatched_kind_and_preset_node["kind"] = "document"
+        test_data = {
+            "root_id": self.root_node.id,
+            "content_data": [
+                mismatched_kind_and_preset_node,
+            ],
+        }
+
+        response = self.admin_client().post(
+            reverse_lazy("api_add_nodes_to_tree"), data=test_data, format="json"
+        )
+
+        self.assertEqual(response.status_code, 200, response.content)
+        node = ContentNode.objects.get(title=mismatched_kind_and_preset_node["title"])
+        self.assertFalse(node.complete)
+
+    def test_invalid_file_duration_data_excluded(self):
+        invalid_file_data_node = self._make_node_data()
+        invalid_file_data_node["title"] = "invalid file data title"
+        image_file = create_studio_file("", preset=format_presets.VIDEO_THUMBNAIL, ext="jpg")["db_file"]
+        bad_file = {
+            "size": image_file.file_size,
+            "preset": format_presets.VIDEO_THUMBNAIL,
+            "filename": image_file.filename(),
+            "original_filename": image_file.original_filename,
+            "language": image_file.language,
+            "source_url": image_file.source_url,
+            "duration": 10,
+        }
+        invalid_file_data_node["files"] += [bad_file]
+        test_data = {
+            "root_id": self.root_node.id,
+            "content_data": [
+                invalid_file_data_node,
+            ],
+        }
+
+        response = self.admin_client().post(
+            reverse_lazy("api_add_nodes_to_tree"), data=test_data, format="json"
+        )
+
+        self.assertEqual(response.status_code, 200, response.content)
+        node = ContentNode.objects.get(title=invalid_file_data_node["title"])
+        self.assertEqual(node.files.count(), 1)
+
 
 class ApiAddExerciseNodesToTreeTestCase(StudioTestCase):
     """

contentcuration/contentcuration/tests/viewsets/test_file.py

@@ -386,6 +386,17 @@ def test_duration_missing_but_required(self):
 
         self.assertEqual(response.status_code, 400)
 
+    def test_duration_present_but_not_allowed(self):
+        self.file["file_format"] = file_formats.EPUB
+        self.file["preset"] = format_presets.DOCUMENT
+
+        self.client.force_authenticate(user=self.user)
+        response = self.client.post(
+            reverse("file-upload-url"), self.file, format="json",
+        )
+
+        self.assertEqual(response.status_code, 400)
+
     def test_duration_null(self):
         self.file["duration"] = None
         self.file["file_format"] = file_formats.EPUB
@@ -436,6 +447,16 @@ def test_invalid_preset_upload(self):
         response = self.client.post(reverse("file-upload-url"), file, format="json")
         self.assertEqual(response.status_code, 400)
 
+    def test_mismatched_preset_upload(self):
+        self.file["file_format"] = file_formats.EPUB
+
+        self.client.force_authenticate(user=self.user)
+        response = self.client.post(
+            reverse("file-upload-url"), self.file, format="json",
+        )
+
+        self.assertEqual(response.status_code, 400)
+
     def test_insufficient_storage(self):
         self.file["size"] = 100000000000000
 

contentcuration/contentcuration/utils/nodes.py

@@ -6,7 +6,6 @@
 
 from django.conf import settings
 from django.core.exceptions import ObjectDoesNotExist
-from django.core.exceptions import ValidationError
 from django.core.files.storage import default_storage
 from django.db.models import Count
 from django.db.models import Sum
@@ -21,6 +20,7 @@
 from contentcuration.models import FormatPreset
 from contentcuration.models import generate_object_storage_name
 from contentcuration.models import Language
+from contentcuration.models import MEDIA_PRESETS
 from contentcuration.models import User
 from contentcuration.utils.cache import ResourceSizeCache
 from contentcuration.utils.files import get_thumbnail_encoding
@@ -43,13 +43,20 @@ def map_files_to_node(user, node, data):  # noqa: C901
     # filter out file that are empty
     valid_data = filter_out_nones(data)
 
+    errors = []
+
     for file_data in valid_data:
         filename = file_data["filename"]
         checksum, ext1 = os.path.splitext(filename)
         ext = ext1.lstrip(".")
 
         # Determine a preset if none is given
         kind_preset = FormatPreset.get_preset(file_data["preset"]) or FormatPreset.guess_format_preset(filename)
+        try:
+            kind_preset.allowed_formats.get(extension=ext)
+        except ObjectDoesNotExist:
+            errors.append(f"provided or inferred preset {kind_preset.id} does not allow files with extension {ext}")
+            continue
 
         file_path = generate_object_storage_name(checksum, filename)
         storage = default_storage
@@ -64,7 +71,12 @@ def map_files_to_node(user, node, data):  # noqa: C901
         except ObjectDoesNotExist:
             invalid_lang = file_data.get('language')
             logging.warning("file_data with language {} does not exist.".format(invalid_lang))
-            return ValidationError("file_data given was invalid; expected string, got {}".format(invalid_lang))
+            errors.append("file_data given was invalid; expected string, got {}".format(invalid_lang))
+            continue
+
+        if file_data.get("duration") and kind_preset.id not in MEDIA_PRESETS:
+            errors.append("duration was included for a file with a non-media preset")
+            continue
 
         resource_obj = File(
             checksum=checksum,
@@ -95,6 +107,7 @@ def map_files_to_node(user, node, data):  # noqa: C901
                 'zoom': 0
             })
             node.save()
+    return errors
 
 
 def map_files_to_assessment_item(user, assessment_item, data):
@@ -146,10 +159,6 @@ def map_files_to_slideshow_slide_item(user, node, slides, files):
 
         matching_slide = next((slide for slide in slides if slide.metadata["checksum"] == checksum), None)
 
-        if not matching_slide:
-            # TODO(Jacob) Determine proper error type... raise it.
-            print("NO MATCH")
-
         file_path = generate_object_storage_name(checksum, filename)
         storage = default_storage
 

contentcuration/contentcuration/views/internal.py

@@ -651,7 +651,7 @@ def convert_data_to_nodes(user, content_data, parent_node):  # noqa: C901
                     if "source_channel_id" in node_data:
                         new_node = handle_remote_node(user, node_data, parent_node)
 
-                        map_files_to_node(user, new_node, node_data.get("files", []))
+                        file_errors = map_files_to_node(user, new_node, node_data.get("files", []))
 
                         add_tags(new_node, node_data)
 
@@ -660,7 +660,7 @@ def convert_data_to_nodes(user, content_data, parent_node):  # noqa: C901
                         new_node = create_node(node_data, parent_node, sort_order)
 
                         # Create files associated with node
-                        map_files_to_node(user, new_node, node_data["files"])
+                        file_errors = map_files_to_node(user, new_node, node_data["files"])
 
                         # Create questions associated exercise nodes
                         create_exercises(user, new_node, node_data["questions"])
@@ -686,11 +686,12 @@ def convert_data_to_nodes(user, content_data, parent_node):  # noqa: C901
                     # Wait until after files have been set on the node to check for node completeness
                     # as some node kinds are counted as incomplete if they lack a default file.
                     completion_errors = new_node.mark_complete()
+                    new_node.save()
 
-                    if completion_errors:
+                    if completion_errors or file_errors:
                         try:
                             # we need to raise it to get Python to fill out the stack trace.
-                            raise IncompleteNodeError(new_node, completion_errors)
+                            raise IncompleteNodeError(new_node, completion_errors + file_errors)
                         except IncompleteNodeError as e:
                             report_exception(e)
 

contentcuration/contentcuration/viewsets/file.py

@@ -32,6 +32,9 @@
 from contentcuration.viewsets.sync.utils import generate_update_event
 
 
+PRESET_LOOKUP = {p.id: p for p in format_presets.PRESETLIST}
+
+
 class StrictFloatField(serializers.FloatField):
     def to_internal_value(self, data):
         # If data is a string, reject it even if it represents a number.
@@ -72,6 +75,9 @@ def validate(self, attrs):
         if attrs["file_format"] in {file_formats.MP4, file_formats.WEBM, file_formats.MP3}:
             if "duration" not in attrs or attrs["duration"] is None:
                 raise serializers.ValidationError("Duration is required for audio/video files")
+        preset_obj = PRESET_LOOKUP[attrs["preset"]]
+        if attrs["file_format"] not in preset_obj.allowed_formats:
+            raise serializers.ValidationError(f"File format {attrs['file_format']} is not an allowed format for this preset {attrs['preset']}")
         return attrs