stash

backend/files/media_urls.py

@@ -1,8 +1,17 @@
+import io
+import os
+from datetime import timedelta
+
+from django.core.files.base import ContentFile
+from django.core.files.storage import default_storage
 from django.http import HttpResponse
 from django.urls import path
 from django.db.models import Q
 from django.conf import settings
+from django.utils.http import http_date
+from django.utils.timezone import now
 from drf_yasg.utils import swagger_auto_schema
+from PIL import Image
 from rest_framework import status
 from rest_framework.decorators import api_view, permission_classes, authentication_classes
 from rest_framework.permissions import IsAuthenticated
@@ -11,6 +20,29 @@ from rest_framework.response import Response
 from authentication.signature_auth import SignatureAuthentication
 from files.models import File
 
+THUMBNAIL_SIZES = (32, 64, 256)
+
+
+def _accessible_files(request):
+    # Shared by media_urls and thumbnail_urls so both endpoints always agree on who can see
+    # what - a file is visible if the requester is friends-or-self with whatever currently
+    # references it (an inventory item, a profile picture) or it's their own staged photo.
+    return File.objects.filter(
+        Q(connected_items__owner__in=request.user.friends_or_self()) |
+        Q(profile_picture_users__in=request.user.friends_or_self()) |
+        Q(staged_by_workflows__owner__in=request.user.user.all())
+    ).distinct()
+
+
+def _cache_headers(etag):
+    # Content is addressed by its own hash and can never change under a given URL, so caches
+    # (and the conditional-GET checks in both views below) can treat it as immutable forever.
+    return {
+        'ETag': etag,
+        'Cache-Control': 'max-age=31536000, private, immutable',
+        'Expires': http_date((now() + timedelta(days=365)).timestamp()),
+    }
+
 
 @swagger_auto_schema(method='GET', auto_schema=None)
 @api_view(['GET'])
@@ -23,30 +55,112 @@ def media_urls(request, hash_path):
     # is the SERVE_X_ACCEL_REDIRECT path: nginx replaces this response entirely when it
     # follows the X-Accel-Redirect and serves the file itself, so the CORS header for that
     # case has to be configured in nginx's `location /redirect_media/` block instead.
+    #
+    # Looked up by the derived storage path (not by raw hash) because FileSerializer.name
+    # (files/serializers.py) - used everywhere a file URL is handed to the frontend, e.g.
+    # AuthenticatedImage's `src` - already returns this path via Django's FileField.url, and
+    # the existing test suite (files/tests.py MediaUrlTestCase) exercises it this way too.
     try:
-        file = File.objects.filter(
-            Q(connected_items__owner__in=request.user.friends_or_self()) |
-            Q(profile_picture_users__in=request.user.friends_or_self()) |
-            Q(staged_by_workflows__owner__in=request.user.user.all())
-        ).distinct().get(
-            file=hash_path)
+        file = _accessible_files(request).get(file=hash_path)
+
+        # The access-control lookup above must happen before this check - otherwise a bare
+        # hash + If-None-Match would let anyone probe "does a file with this hash exist" for
+        # files they can't actually see.
+        if request.META.get('HTTP_IF_NONE_MATCH') == file.hash:
+            return HttpResponse(status=status.HTTP_304_NOT_MODIFIED)
+
+        cache_headers = _cache_headers(file.hash)
 
         if settings.SERVE_X_ACCEL_REDIRECT:
             return HttpResponse(status=status.HTTP_200_OK,
                                 content_type=file.mime_type,
                                 headers={
                                     'X-Accel-Redirect': f'/redirect_media/{hash_path}',
-                                })  # TODO Expires and Cache-Control
+                                    **cache_headers,
+                                })
         else:
+            # Read via the FieldFile itself (works against whatever storage backend is
+            # actually configured) rather than assuming file.file.path is a real filesystem
+            # path - the test suite swaps in an in-memory backend where that isn't true.
+            with file.file.open('rb') as fh:
+                content = fh.read()
             return HttpResponse(status=status.HTTP_200_OK,
                                 content_type=file.mime_type,
-                                content=open(file.file.path, 'rb').read())
+                                headers=cache_headers,
+                                content=content)
 
+    except File.DoesNotExist:
+        return Response(status=status.HTTP_404_NOT_FOUND)
+
+
+def _thumbnail_rel_path(file_hash, size):
+    # Mirrors files/models.py's hash_upload() sharding, under its own `thumbnails/<size>/`
+    # subtree - reachable through the same nginx `/redirect_media/` alias as originals when
+    # served from real disk, no separate nginx location needed.
+    return os.path.join('thumbnails', str(size), file_hash[:2], file_hash[2:4], file_hash[4:6],
+                         file_hash[6:] + '.jpg')
+
+
+@swagger_auto_schema(method='GET', auto_schema=None)
+@api_view(['GET'])
+@permission_classes([IsAuthenticated])
+@authentication_classes([SignatureAuthentication])
+def thumbnail_urls(request, size, hash_path):
+    if size not in THUMBNAIL_SIZES:
+        return Response(status=status.HTTP_404_NOT_FOUND)
+
+    try:
+        file = _accessible_files(request).get(file=hash_path)
+
+        etag = f'{file.hash}_{size}'
+        if request.META.get('HTTP_IF_NONE_MATCH') == etag:
+            return HttpResponse(status=status.HTTP_304_NOT_MODIFIED)
+
+        # Read/write through the default storage backend, same as File.file itself, rather
+        # than a hand-rolled filesystem path - correct regardless of storage backend (real
+        # disk in production, in-memory under the test runner) and keeps the cache in the
+        # same place originals live.
+        rel_path = _thumbnail_rel_path(file.hash, size)
+        if not default_storage.exists(rel_path):
+            # Thumbnails are always re-encoded as JPEG regardless of the original format -
+            # smaller and simpler than preserving e.g. PNG transparency at this scale.
+            with file.file.open('rb') as fh:
+                image = Image.open(fh)
+                image.thumbnail((size, size))
+                # Flatten through RGBA before dropping to RGB - some modes (grayscale+alpha,
+                # palette-with-transparency, RGBA) store meaningless color/luminance data under
+                # fully transparent pixels (often zeroed out, i.e. black). Converting straight
+                # to RGB reveals that instead of "nothing there"; compositing onto an opaque
+                # background first shows what the image is actually supposed to look like.
+                rgba = image.convert('RGBA')
+                flattened = Image.new('RGB', rgba.size, (255, 255, 255))
+                flattened.paste(rgba, mask=rgba.getchannel('A'))
+                buffer = io.BytesIO()
+                flattened.save(buffer, 'JPEG', quality=90)
+            default_storage.save(rel_path, ContentFile(buffer.getvalue()))
+
+        cache_headers = _cache_headers(etag)
+
+        if settings.SERVE_X_ACCEL_REDIRECT:
+            return HttpResponse(status=status.HTTP_200_OK,
+                                content_type='image/jpeg',
+                                headers={
+                                    'X-Accel-Redirect': f'/redirect_media/{rel_path}',
+                                    **cache_headers,
+                                })
+        else:
+            with default_storage.open(rel_path, 'rb') as fh:
+                content = fh.read()
+            return HttpResponse(status=status.HTTP_200_OK,
+                                content_type='image/jpeg',
+                                headers=cache_headers,
+                                content=content)
 
     except File.DoesNotExist:
         return Response(status=status.HTTP_404_NOT_FOUND)
 
 
 urlpatterns = [
+    path('<int:size>/<path:hash_path>/', thumbnail_urls),
     path('<path:hash_path>', media_urls),
 ]

backend/files/models.py

@@ -1,4 +1,7 @@
+from types import SimpleNamespace
+
 from django.core.files.base import ContentFile
+from django.core.files.storage import default_storage
 from django.db import models, IntegrityError
 from django.db.models import Model
 
@@ -40,6 +43,18 @@ class FileManager(models.Manager):
         else:
             raise ValueError('data must be a base64 encoded string or file and hash must be provided')
         if not self.filter(hash=kwargs['hash']).exists():
+            # The upload path is derived entirely from the hash (hash_upload, above), and hash
+            # is DB-unique - so if no File row owns this hash yet, anything already sitting at
+            # its computed path is necessarily a stale orphan (e.g. left behind by a bug in
+            # cleanup code that deleted a File row without removing its stored bytes, or a
+            # crashed upload). Clear it before saving instead of letting Django's storage layer
+            # invent an alternate filename to avoid the "collision" - a suffixed name would
+            # silently break every part of the app that derives this file's URL purely from its
+            # hash (media serving, thumbnail generation, item/avatar attachment), and no future
+            # caller could ever discover it again.
+            expected_path = hash_upload(SimpleNamespace(hash=kwargs['hash']), '')
+            if default_storage.exists(expected_path):
+                default_storage.delete(expected_path)
             return super().create(**kwargs)
         else:
             raise IntegrityError('File with this hash already exists')

backend/files/tests.py

@@ -1,13 +1,20 @@
+import io
+import os
+import zlib
+
+from django.conf import settings
 from django.core.files.base import ContentFile
-from django.core.files.storage import DefaultStorage
+from django.core.files.storage import DefaultStorage, default_storage
 from django.db import IntegrityError, transaction
 from django.test import Client, override_settings
 from authentication.tests import SignatureAuthClient, ToolshedTestCase, UserTestMixin
 from toolshed.tests import InventoryTestMixin
 from nacl.hash import sha256
 from nacl.encoding import HexEncoder
+from PIL import Image
 import base64
 
+from files.media_urls import THUMBNAIL_SIZES
 from files.models import File
 
 anonymous_client = Client()
@@ -105,6 +112,23 @@ class FilesTestCase(FilesTestMixin, ToolshedTestCase):
         self.assertEqual(File.objects.count(), 3)
         self.assertEqual(countdir(DefaultStorage(), ''), 3)
 
+    def test_file_upload_reclaims_stale_orphan_at_canonical_path(self):
+        # Reproduces a real incident: a File row gets deleted without its underlying stored
+        # bytes being removed (e.g. a bug in some cleanup call site), leaving an orphan sitting
+        # at the exact path hash_upload() would compute for that content. A later upload of the
+        # same content must land back on that canonical path - not get silently suffixed by
+        # Django's default collision-avoidance, which would make it unreachable to every part
+        # of the app that derives a file's URL purely from its hash.
+        expected_path = f"{self.f['hash4'][:2]}/{self.f['hash4'][2:4]}/{self.f['hash4'][4:6]}/{self.f['hash4'][6:]}"
+        default_storage.save(expected_path, ContentFile(self.f['test_content4']))
+        self.assertTrue(default_storage.exists(expected_path))
+        self.assertFalse(File.objects.filter(hash=self.f['hash4']).exists())
+
+        file = File.objects.create(mime_type='text/plain', data=self.f['encoded_content4'])
+
+        self.assertEqual(file.file.name, expected_path)
+        self.assertEqual(file.file.read(), self.f['test_content4'])
+
 
 class MediaUrlTestCase(FilesTestMixin, UserTestMixin, InventoryTestMixin, ToolshedTestCase):
     def setUp(self):
@@ -120,28 +144,29 @@ class MediaUrlTestCase(FilesTestMixin, UserTestMixin, InventoryTestMixin, Toolsh
         self.f['item2'].files.add(self.f['test_file1'])
 
 
-#    def test_file_url(self):
-#        reply = client.get(
-#            f"/media/{self.f['hash1'][:2]}/{self.f['hash1'][2:4]}/{self.f['hash1'][4:6]}/{self.f['hash1'][6:]}",
-#            self.f['local_user1'])
-#        self.assertEqual(reply.status_code, 200)
-#        self.assertEqual(reply.headers['X-Accel-Redirect'],
-#                         f"/redirect_media/{self.f['hash1'][:2]}/{self.f['hash1'][2:4]}/{self.f['hash1'][4:6]}/{self.f['hash1'][6:]}")
-#        self.assertEqual(reply.headers['Content-Type'], self.f['test_file1'].mime_type)
-#        reply = client.get(
-#            f"/media/{self.f['hash2'][:2]}/{self.f['hash2'][2:4]}/{self.f['hash2'][4:6]}/{self.f['hash2'][6:]}",
-#            self.f['local_user1'])
-#        self.assertEqual(reply.status_code, 200)
-#        self.assertEqual(reply.headers['X-Accel-Redirect'],
-#                         f"/redirect_media/{self.f['hash2'][:2]}/{self.f['hash2'][2:4]}/{self.f['hash2'][4:6]}/{self.f['hash2'][6:]}")
-#        self.assertEqual(reply.headers['Content-Type'], self.f['test_file2'].mime_type)
-#        reply = client.get(
-#            f"/media/{self.f['hash2'][:2]}/{self.f['hash2'][2:4]}/{self.f['hash2'][4:6]}/{self.f['hash2'][6:]}",
-#            self.f['local_user2'])
-#        self.assertEqual(reply.status_code, 200)
-#        self.assertEqual(reply.headers['X-Accel-Redirect'],
-#                         f"/redirect_media/{self.f['hash2'][:2]}/{self.f['hash2'][2:4]}/{self.f['hash2'][4:6]}/{self.f['hash2'][6:]}")
-#        self.assertEqual(reply.headers['Content-Type'], self.f['test_file2'].mime_type)
+    @override_settings(SERVE_X_ACCEL_REDIRECT=True)
+    def test_file_url(self):
+        reply = client.get(
+            f"/media/{self.f['hash1'][:2]}/{self.f['hash1'][2:4]}/{self.f['hash1'][4:6]}/{self.f['hash1'][6:]}",
+            self.f['local_user1'])
+        self.assertEqual(reply.status_code, 200)
+        self.assertEqual(reply.headers['X-Accel-Redirect'],
+                         f"/redirect_media/{self.f['hash1'][:2]}/{self.f['hash1'][2:4]}/{self.f['hash1'][4:6]}/{self.f['hash1'][6:]}")
+        self.assertEqual(reply.headers['Content-Type'], self.f['test_file1'].mime_type)
+        reply = client.get(
+            f"/media/{self.f['hash2'][:2]}/{self.f['hash2'][2:4]}/{self.f['hash2'][4:6]}/{self.f['hash2'][6:]}",
+            self.f['local_user1'])
+        self.assertEqual(reply.status_code, 200)
+        self.assertEqual(reply.headers['X-Accel-Redirect'],
+                         f"/redirect_media/{self.f['hash2'][:2]}/{self.f['hash2'][2:4]}/{self.f['hash2'][4:6]}/{self.f['hash2'][6:]}")
+        self.assertEqual(reply.headers['Content-Type'], self.f['test_file2'].mime_type)
+        reply = client.get(
+            f"/media/{self.f['hash2'][:2]}/{self.f['hash2'][2:4]}/{self.f['hash2'][4:6]}/{self.f['hash2'][6:]}",
+            self.f['local_user2'])
+        self.assertEqual(reply.status_code, 200)
+        self.assertEqual(reply.headers['X-Accel-Redirect'],
+                         f"/redirect_media/{self.f['hash2'][:2]}/{self.f['hash2'][2:4]}/{self.f['hash2'][4:6]}/{self.f['hash2'][6:]}")
+        self.assertEqual(reply.headers['Content-Type'], self.f['test_file2'].mime_type)
 
     def test_file_url_fail(self):
         reply = client.get('/media/{}/'.format('nonexistent'), self.f['local_user1'])
@@ -195,3 +220,138 @@ class MediaUrlTestCase(FilesTestMixin, UserTestMixin, InventoryTestMixin, Toolsh
             self.f['ext_user1'])
         self.assertEqual(reply.status_code, 404)
 
+
+class ThumbnailUrlTestCase(FilesTestMixin, UserTestMixin, InventoryTestMixin, ToolshedTestCase):
+    def setUp(self):
+        super().setUp()
+        self.prepare_files()
+        self.prepare_users()
+        self.prepare_categories()
+        self.prepare_tags()
+        self.prepare_properties()
+        self.prepare_inventory()
+
+        # Each test method gets its own distinct image content (and therefore its own content
+        # hash / thumbnail cache path) - InMemoryStorage isn't reset between test methods within
+        # a run, so sharing one fixed image across methods risks one test's cached (or, in
+        # test_thumbnail_served_from_cache_on_second_request's case, deliberately corrupted)
+        # thumbnail leaking into another test's assertions.
+        seed = zlib.crc32(self._testMethodName.encode()) % 256
+        buffer = io.BytesIO()
+        Image.new('RGB', (800, 600), (seed, 255 - seed, 128)).save(buffer, 'PNG')
+        image_bytes = buffer.getvalue()
+        self.f['image_hash'] = sha256(image_bytes, encoder=HexEncoder).decode('utf-8')
+        self.f['image_file'] = File.objects.create(
+            mime_type='image/png', data=base64.b64encode(image_bytes).decode('utf-8'))
+        self.f['item1'].files.add(self.f['image_file'])
+
+    def _thumb_url(self, size, image_hash=None):
+        h = image_hash or self.f['image_hash']
+        return f"/media/{size}/{h[:2]}/{h[2:4]}/{h[4:6]}/{h[6:]}/"
+
+    def _thumb_rel_path(self, size):
+        h = self.f['image_hash']
+        return os.path.join('thumbnails', str(size), h[:2], h[2:4], h[4:6], h[6:] + '.jpg')
+
+    def test_thumbnail_sizes_available(self):
+        # Documents the fixed size allow-list this test suite exercises against - update both
+        # if files/media_urls.py's THUMBNAIL_SIZES ever changes.
+        self.assertEqual(THUMBNAIL_SIZES, (32, 64, 256))
+
+    @override_settings(SERVE_X_ACCEL_REDIRECT=False)
+    def test_thumbnail_generates_resized_jpeg(self):
+        self.assertFalse(default_storage.exists(self._thumb_rel_path(64)))
+
+        reply = client.get(self._thumb_url(64), self.f['local_user1'])
+        self.assertEqual(reply.status_code, 200)
+        self.assertEqual(reply.headers['Content-Type'], 'image/jpeg')
+
+        generated = Image.open(io.BytesIO(reply.content))
+        self.assertEqual(generated.format, 'JPEG')
+        # Aspect-ratio-preserving fit within a 64x64 box, not a crop to exactly 64x64.
+        self.assertLessEqual(max(generated.size), 64)
+        self.assertAlmostEqual(generated.size[0] / generated.size[1], 800 / 600, places=2)
+
+    @override_settings(SERVE_X_ACCEL_REDIRECT=False)
+    def test_thumbnail_flattens_transparency_instead_of_going_black(self):
+        # Reproduces a real incident: an 'LA' (grayscale + alpha) source whose fully-transparent
+        # region has zeroed-out luminance underneath, as many image tools produce. Converting
+        # straight to RGB (dropping alpha without compositing) reveals that zeroed data - the
+        # whole thumbnail comes out solid black even though the visible (opaque) content isn't.
+        half_transparent = Image.new('LA', (200, 200))
+        pixels = half_transparent.load()
+        for x in range(200):
+            for y in range(200):
+                if x < 100:
+                    pixels[x, y] = (0, 0)  # transparent, zeroed-out luminance underneath
+                else:
+                    pixels[x, y] = (255, 255)  # fully opaque, bright content
+
+        buffer = io.BytesIO()
+        half_transparent.save(buffer, 'PNG')
+        image_bytes = buffer.getvalue()
+        image_hash = sha256(image_bytes, encoder=HexEncoder).decode('utf-8')
+        image_file = File.objects.create(
+            mime_type='image/png', data=base64.b64encode(image_bytes).decode('utf-8'))
+        self.f['item1'].files.add(image_file)
+
+        reply = client.get(self._thumb_url(64, image_hash=image_hash), self.f['local_user1'])
+        self.assertEqual(reply.status_code, 200)
+
+        generated = Image.open(io.BytesIO(reply.content)).convert('L')
+        # The opaque (right) half must stay bright; a naive RGB conversion would blacken it too.
+        self.assertGreater(generated.getpixel((generated.width - 1, generated.height // 2)), 200)
+        self.assertNotEqual(generated.getextrema(), (0, 0))
+
+    @override_settings(SERVE_X_ACCEL_REDIRECT=False)
+    def test_thumbnail_served_from_cache_on_second_request(self):
+        client.get(self._thumb_url(64), self.f['local_user1'])
+        rel_path = self._thumb_rel_path(64)
+        with default_storage.open(rel_path, 'rb') as f:
+            cached_bytes = f.read()
+
+        # Overwrite the cached file with a marker so a correct implementation must serve this
+        # exact content back rather than regenerating it from the original.
+        default_storage.delete(rel_path)
+        default_storage.save(rel_path, ContentFile(cached_bytes + b'MARKER'))
+
+        reply = client.get(self._thumb_url(64), self.f['local_user1'])
+        self.assertEqual(reply.status_code, 200)
+        self.assertTrue(reply.content.endswith(b'MARKER'))
+
+    def test_thumbnail_invalid_size(self):
+        reply = client.get(self._thumb_url(100), self.f['local_user1'])
+        self.assertEqual(reply.status_code, 404)
+        self.assertFalse(default_storage.exists(self._thumb_rel_path(100)))
+
+    def test_thumbnail_not_found(self):
+        reply = client.get(self._thumb_url(64, image_hash='0' * 64), self.f['local_user1'])
+        self.assertEqual(reply.status_code, 404)
+
+    def test_thumbnail_anonymous(self):
+        reply = anonymous_client.get(self._thumb_url(64))
+        self.assertEqual(reply.status_code, 403)
+
+    def test_thumbnail_not_friend(self):
+        # local_user1/local_user2 are friends in these fixtures (see prepare_inventory) - the
+        # denied case needs a stranger to that friendship instead.
+        reply = client.get(self._thumb_url(64), self.f['ext_user1'])
+        self.assertEqual(reply.status_code, 404)
+        self.assertFalse(default_storage.exists(self._thumb_rel_path(64)))
+
+    def test_thumbnail_conditional_get(self):
+        reply = client.get(self._thumb_url(64), self.f['local_user1'])
+        etag = reply.headers['ETag']
+        self.assertEqual(etag, f"{self.f['image_hash']}_64")
+
+        reply = client.get(self._thumb_url(64), self.f['local_user1'], HTTP_IF_NONE_MATCH=etag)
+        self.assertEqual(reply.status_code, 304)
+
+    @override_settings(SERVE_X_ACCEL_REDIRECT=True)
+    def test_thumbnail_x_accel_redirect(self):
+        reply = client.get(self._thumb_url(64), self.f['local_user1'])
+        self.assertEqual(reply.status_code, 200)
+        h = self.f['image_hash']
+        self.assertEqual(reply.headers['X-Accel-Redirect'],
+                         f"/redirect_media/thumbnails/64/{h[:2]}/{h[2:4]}/{h[4:6]}/{h[6:]}.jpg")
+