Merge pull request #1380 from opengisch/fix_range_typing

refactor: fix typing for parsing the `Range` header and serving the files

Author: suricactus

docker-app/qfieldcloud/filestorage/tests/test_range.py

@@ -1,11 +1,12 @@
 import logging
 from io import StringIO
 
-from django.test import override_settings
+from django.test import RequestFactory, override_settings
 from rest_framework import status
 from rest_framework.test import APITransactionTestCase
 
 from qfieldcloud.authentication.models import AuthToken
+from qfieldcloud.core.exceptions import InvalidRangeError
 from qfieldcloud.core.models import (
     Person,
     Project,
@@ -14,7 +15,7 @@
 from qfieldcloud.core.tests.utils import (
     setup_subscription_plans,
 )
-from qfieldcloud.filestorage.utils import parse_range
+from qfieldcloud.filestorage.utils import get_range, parse_range_header
 
 logging.disable(logging.CRITICAL)
 
@@ -39,9 +40,9 @@ def setUp(self):
         )
 
     def test_parsing_range_function_succeeds(self):
-        self.assertEquals(parse_range("bytes=4-8", 10), (4, 8))
+        self.assertEquals(parse_range_header("bytes=4-8", 10), (4, 8))
 
-        start_byte, end_byte = parse_range("bytes=2-", 10)
+        start_byte, end_byte = parse_range_header("bytes=2-", 10)
 
         self.assertEquals(start_byte, 2)
         self.assertIsNone(end_byte)
@@ -50,39 +51,85 @@ def test_parsing_wrong_invalid_range_function_succeeds(self):
         file_size = 1000000
 
         # not starting with 'bytes'
-        self.assertIsNone(parse_range("byte=4-8", file_size))
+        self.assertIsNone(parse_range_header("byte=4-8", file_size))
 
         # start byte can not be negative
-        self.assertIsNone(parse_range("bytes=-1-15", file_size))
+        self.assertIsNone(parse_range_header("bytes=-1-15", file_size))
 
         # start and end bytes can not be negative
-        self.assertIsNone(parse_range("bytes=-10--15", file_size))
+        self.assertIsNone(parse_range_header("bytes=-10--15", file_size))
 
         # start position cannot be greater than the end position
-        self.assertIsNone(parse_range("bytes=9-1", file_size))
+        self.assertIsNone(parse_range_header("bytes=9-1", file_size))
 
         # suffix ranges are not supported (yet), see https://www.rfc-editor.org/rfc/rfc9110.html#rule.suffix-range
-        self.assertIsNone(parse_range("bytes=-5", file_size))
+        self.assertIsNone(parse_range_header("bytes=-5", file_size))
 
         # bytes should be numbers
-        self.assertIsNone(parse_range("bytes=one-two", file_size))
+        self.assertIsNone(parse_range_header("bytes=one-two", file_size))
         # whitespaces are not accepted
-        self.assertIsNone(parse_range("bytes= 1-9", file_size))
-        self.assertIsNone(parse_range("bytes=1 -9", file_size))
-        self.assertIsNone(parse_range("bytes=1- 9", file_size))
-        self.assertIsNone(parse_range("bytes=1-9 ", file_size))
-        self.assertIsNone(parse_range("bytes=1- ", file_size))
-        self.assertIsNone(parse_range(" bytes=1-9", file_size))
+        self.assertIsNone(parse_range_header("bytes= 1-9", file_size))
+        self.assertIsNone(parse_range_header("bytes=1 -9", file_size))
+        self.assertIsNone(parse_range_header("bytes=1- 9", file_size))
+        self.assertIsNone(parse_range_header("bytes=1-9 ", file_size))
+        self.assertIsNone(parse_range_header("bytes=1- ", file_size))
+        self.assertIsNone(parse_range_header(" bytes=1-9", file_size))
         # typos in bytes
-        self.assertIsNone(parse_range("bites=0-9", file_size))
-        self.assertIsNone(parse_range("starting bytes=0-9", file_size))
-        self.assertIsNone(parse_range("bytes=0-9 closing bytes", file_size))
+        self.assertIsNone(parse_range_header("bites=0-9", file_size))
+        self.assertIsNone(parse_range_header("starting bytes=0-9", file_size))
+        self.assertIsNone(parse_range_header("bytes=0-9 closing bytes", file_size))
         # empty range
-        self.assertIsNone(parse_range("bytes=0-0", file_size))
-        self.assertIsNone(parse_range("bytes=1-1", file_size))
+        self.assertIsNone(parse_range_header("bytes=0-0", file_size))
+        self.assertIsNone(parse_range_header("bytes=1-1", file_size))
         # multiple ranges are not supported (yet), see https://www.rfc-editor.org/rfc/rfc9110.html#section-14.1.2-9.4.1
-        self.assertIsNone(parse_range("bytes=1-5, 10-15", file_size))
-        self.assertIsNone(parse_range("bytes=1-5,10-15", file_size))
+        self.assertIsNone(parse_range_header("bytes=1-5, 10-15", file_size))
+        self.assertIsNone(parse_range_header("bytes=1-5,10-15", file_size))
+
+    def test_get_range_function(self):
+        factory = RequestFactory()
+
+        request = factory.get("")
+        range = get_range(request, 10)
+
+        self.assertIsNone(range)
+
+        request = factory.get("", headers={"Range": "bytes=4-8"})
+        range = get_range(request, 10)
+
+        self.assertIsNotNone(range)
+        # typing is not aware that the above function checks for `None`
+        assert range
+
+        self.assertEqual(range.start, 4)
+        self.assertEqual(range.end, 8)
+        self.assertEqual(range.length, 5)
+        self.assertEqual(range.total_size, 10)
+        self.assertEqual(range.header, "bytes=4-8")
+
+        request = factory.get("", headers={"Range": "bytes=4-8"})
+
+        request = factory.get("", headers={"Range": "bytes=2-"})
+        range = get_range(request, 10)
+
+        self.assertIsNotNone(range)
+        assert range
+
+        self.assertEqual(range.start, 2)
+        self.assertEqual(range.end, 9)
+        self.assertEqual(range.length, 8)
+        self.assertEqual(range.total_size, 10)
+        self.assertEqual(range.header, "bytes=2-")
+
+        request = factory.get("", headers={"Range": "bytes= 2 - "})
+
+        with self.assertRaises(InvalidRangeError):
+            get_range(request, 10)
+
+        with override_settings(QFIELDCLOUD_MINIMUM_RANGE_HEADER_LENGTH=3):
+            request = factory.get("", headers={"Range": "bytes=0-1"})
+
+            with self.assertRaises(InvalidRangeError):
+                get_range(request, 10)
 
     def test_upload_file_then_download_range_succeeds(self):
         for project in [self.project_default_storage, self.project_webdav_storage]:

docker-app/qfieldcloud/filestorage/utils.py

@@ -4,12 +4,16 @@
 from pathlib import Path, PurePath
 from typing import Any
 
+from attr import dataclass
 from django.conf import settings
 from django.core.exceptions import ValidationError
 from django.core.files.base import ContentFile
 from django.core.validators import RegexValidator
+from django.http import HttpRequest
 from django.utils.translation import gettext_lazy as _
 
+from qfieldcloud.core.exceptions import InvalidRangeError
+
 filename_validator = RegexValidator(
     settings.STORAGES_FILENAME_VALIDATION_REGEX,
     _(
@@ -158,7 +162,10 @@ def to_uuid(value: Any) -> uuid.UUID | None:
         return None
 
 
-def parse_range(input_range: str, file_size: int) -> tuple[int, int | None] | None:
+def parse_range_header(
+    input_range: str,
+    file_size: int,
+) -> tuple[int, int | None] | None:
     """Parses a range HTTP Header string.
 
     Arguments:
@@ -189,3 +196,54 @@ def parse_range(input_range: str, file_size: int) -> tuple[int, int | None] | No
         range_end = None
 
     return (range_start, range_end)
+
+
+@dataclass
+class RangeForFile:
+    """A range for a file, as parsed from a HTTP Range header."""
+
+    start: int
+    end: int
+    length: int
+    total_size: int
+    header: str
+
+
+def get_range(request: HttpRequest, total_size: int) -> RangeForFile | None:
+    """Get a parsed range for a file, if any.
+
+    Arguments:
+        request: the HTTP request to get the range from.
+        file: the file to get the range for.
+    """
+    range_header = request.headers.get("Range", "")
+
+    if not range_header:
+        return None
+
+    range_match = parse_range_header(range_header, total_size)
+
+    if not range_match:
+        raise InvalidRangeError("The provided HTTP range header is invalid.")
+
+    range_start, range_end = range_match
+
+    if range_end is None:
+        range_end = total_size - 1
+
+    range_length = range_end - range_start + 1
+
+    if range_length < settings.QFIELDCLOUD_MINIMUM_RANGE_HEADER_LENGTH:
+        raise InvalidRangeError(
+            "Requested range too small, expected at least {} but got {} bytes".format(
+                settings.QFIELDCLOUD_MINIMUM_RANGE_HEADER_LENGTH, range_length
+            )
+        )
+
+    return RangeForFile(
+        start=range_start,
+        end=range_end,
+        length=range_length,
+        total_size=total_size,
+        header=range_header,
+    )

docker-app/qfieldcloud/filestorage/view_helpers.py

@@ -19,7 +19,6 @@
 
 from qfieldcloud.core import exceptions, permissions_utils
 from qfieldcloud.core.exceptions import (
-    InvalidRangeError,
     MultipleProjectsError,
     RestrictedProjectModificationError,
 )
@@ -36,9 +35,9 @@
     FileVersion,
 )
 from qfieldcloud.filestorage.utils import (
+    get_range,
     is_admin_restricted_file,
     is_qgis_project_file,
-    parse_range,
     validate_filename,
 )
 
@@ -247,27 +246,7 @@ def download_field_file(
     http_host = request.headers.get("host", "")
     https_port = http_host.split(":")[-1] if ":" in http_host else "443"
 
-    download_range = request.headers.get("Range", "")
-    if download_range:
-        file_size = field_file.size
-        range_match = parse_range(download_range, file_size)
-
-        if not range_match:
-            raise InvalidRangeError("The provided HTTP range header is invalid.")
-
-        range_start, range_end = range_match
-
-        if range_end is None:
-            range_end = file_size - 1
-
-        range_length = range_end - range_start + 1
-
-        if range_length < settings.QFIELDCLOUD_MINIMUM_RANGE_HEADER_LENGTH:
-            raise InvalidRangeError(
-                "Requested range too small, expected at least {} but got {} bytes".format(
-                    settings.QFIELDCLOUD_MINIMUM_RANGE_HEADER_LENGTH, range_length
-                )
-            )
+    range = get_range(request, field_file.size)
 
     if https_port == settings.WEB_HTTPS_PORT and not settings.IN_TEST_SUITE:
         # this is the relative path of the file, including the containing directories.
@@ -321,25 +300,27 @@ def download_field_file(
         response["X-Accel-Redirect"] = "/storage-download/"
         response["redirect_uri"] = url
 
-        if download_range:
-            response["file_range"] = download_range
+        if range:
+            response["file_range"] = range.header
 
         field_file.storage.patch_nginx_download_redirect(response)  # type: ignore
 
         return response
     elif settings.DEBUG or settings.IN_TEST_SUITE:
-        if download_range:
+        if range:
             file = field_file.open()
 
-            file.seek(range_start)
-            content = file.read(range_length)
+            file.seek(range.start)
+            content = file.read(range.length)
 
             response = HttpResponse(
                 content, status=206, content_type="application/octet-stream"
             )
 
-            response["Content-Range"] = f"bytes {range_start}-{range_end}/{file_size}"
-            response["Content-Length"] = str(range_length)
+            response["Content-Range"] = (
+                f"bytes {range.start}-{range.end}/{range.total_size}"
+            )
+            response["Content-Length"] = str(range.length)
             response["Accept-Ranges"] = "bytes"
 
             return response