some cleanups in attachment processing and storage

misago/conf/defaults.py

@@ -276,11 +276,12 @@ MISAGO_POSTS_TAIL = 7
 # Number of attachments possible to assign to single post
 MISAGO_POST_ATTACHMENTS_LIMIT = 16
 
+# Max allowed size of image before Misago will generate thumbnail for it
+MISAGO_ATTACHMENT_IMAGE_SIZE_LIMIT = (500, 500)
 
 # Length of secret used for attachments url tokens and filenames
 MISAGO_ATTACHMENT_SECRET_LENGTH = 64
 
-
 # Names of files served when user requests file that doesn't exist or is unavailable
 # Those files will be sought within STATIC_ROOT directory
 MISAGO_404_IMAGE = 'misago/img/error-404.png'

misago/threads/migrations/0001_initial.py

@@ -7,6 +7,7 @@ from django.conf import settings
 from django.contrib.postgres.fields import JSONField
 from django.db import migrations, models
 
+import misago.threads.models.attachment
 from misago.core.pgutils import CreatePartialCompositeIndex, CreatePartialIndex
 
 
@@ -231,9 +232,9 @@ class Migration(migrations.Migration):
                 ('uploader_ip', models.GenericIPAddressField()),
                 ('filename', models.CharField(max_length=255)),
                 ('size', models.PositiveIntegerField(default=0)),
-                ('thumbnail', models.ImageField(blank=True, null=True, upload_to='attachments')),
-                ('image', models.ImageField(blank=True, null=True, upload_to='attachments')),
-                ('file', models.FileField(blank=True, null=True, upload_to='attachments')),
+                ('thumbnail', models.ImageField(blank=True, null=True, upload_to=misago.threads.models.attachment.upload_to)),
+                ('image', models.ImageField(blank=True, null=True, upload_to=misago.threads.models.attachment.upload_to)),
+                ('file', models.FileField(blank=True, null=True, upload_to=misago.threads.models.attachment.upload_to)),
                 ('post', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to='misago_threads.Post')),
             ],
         ),

misago/threads/models/attachment.py

@@ -1,3 +1,5 @@
+import os
+from hashlib import md5
 from io import BytesIO
 
 from django.conf import settings
@@ -9,6 +11,25 @@ from django.utils import timezone
 from django.utils.crypto import get_random_string
 from PIL import Image
 
+from misago.core.utils import slugify
+
+
+def upload_to(instance, filename):
+    spread_path = md5(instance.secret[:16]).hexdigest()
+    secret = Attachment.generate_new_secret()
+
+    filename_lowered = filename.lower().strip()
+    for extension in instance.filetype.extensions_list:
+        if filename_lowered.endswith(extension):
+            break
+
+    filename_clean = u'.'.join((
+        slugify(filename[:(len(extension) + 1) * -1])[:16],
+        extension
+    ))
+
+    return os.path.join('attachments', spread_path[:2], spread_path[2:4], secret, filename_clean)
+
 
 class Attachment(models.Model):
     secret = models.CharField(max_length=64)
@@ -35,9 +56,9 @@ class Attachment(models.Model):
     filename = models.CharField(max_length=255)
     size = models.PositiveIntegerField(default=0)
 
-    thumbnail = models.ImageField(blank=True, null=True, upload_to='attachments')
-    image = models.ImageField(blank=True, null=True, upload_to='attachments')
-    file = models.FileField(blank=True, null=True, upload_to='attachments')
+    thumbnail = models.ImageField(blank=True, null=True, upload_to=upload_to)
+    image = models.ImageField(blank=True, null=True, upload_to=upload_to)
+    file = models.FileField(blank=True, null=True, upload_to=upload_to)
 
     @classmethod
     def generate_new_secret(cls):
@@ -58,7 +79,7 @@ class Attachment(models.Model):
         })
 
     def get_thumbnail_url(self):
-        if self.is_image:
+        if self.thumbnail:
             return reverse('misago:attachment-thumbnail', kwargs={
                 'pk': self.pk,
                 'secret': self.secret,
@@ -67,27 +88,29 @@ class Attachment(models.Model):
             return None
 
     def set_file(self, upload):
-        file_secret = get_random_string(settings.MISAGO_ATTACHMENT_SECRET_LENGTH)
-        self.file = File(upload, '.'.join([file_secret, self.filetype.extensions_list[0]]))
+        self.file = File(upload, upload.name)
 
     def set_image(self, upload):
         fileformat = self.filetype.extensions_list[0]
 
-        image_secret = get_random_string(settings.MISAGO_ATTACHMENT_SECRET_LENGTH)
-        image_filename = '.'.join([image_secret, fileformat])
-        self.image = File(upload, image_filename)
+        self.image = File(upload, upload.name)
 
         thumbnail = Image.open(upload)
-        thumbnail.thumbnail((500, 500))
+        downscale_image = (
+            thumbnail.size[0] > settings.MISAGO_ATTACHMENT_IMAGE_SIZE_LIMIT[0] or
+            thumbnail.size[1] > settings.MISAGO_ATTACHMENT_IMAGE_SIZE_LIMIT[1]
+        )
+        strip_animation = fileformat == 'gif'
 
         thumb_stream = BytesIO()
-        if fileformat == 'jpg':
-            # normalize jpg to jpeg for Pillow
-            thumbnail.save(thumb_stream, 'jpeg')
-        else:
+        if downscale_image:
+            if fileformat == 'jpg':
+                # normalize jpg to jpeg for Pillow
+                thumbnail.save(thumb_stream, 'jpeg')
+            else:
+                thumbnail.save(thumb_stream, fileformat)
+        elif strip_animation:
             thumbnail.save(thumb_stream, fileformat)
 
-        thumb_secret = get_random_string(settings.MISAGO_ATTACHMENT_SECRET_LENGTH)
-        thumb_filename = '.'.join([thumb_secret, fileformat])
-        self.thumbnail = ContentFile(thumb_stream.getvalue(), thumb_filename)
-
+        if downscale_image or strip_animation:
+            self.thumbnail = ContentFile(thumb_stream.getvalue(), upload.name)

misago/threads/tests/test_attachments_api.py

@@ -2,6 +2,7 @@ import json
 import os
 
 from django.core.urlresolvers import reverse
+from django.utils import six
 from django.utils.encoding import smart_str
 
 from misago.acl.models import Role
@@ -15,6 +16,7 @@ TESTFILES_DIR = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'testfi
 TEST_DOCUMENT_PATH = os.path.join(TESTFILES_DIR, 'document.pdf')
 TEST_LARGEPNG_PATH = os.path.join(TESTFILES_DIR, 'large.png')
 TEST_SMALLJPG_PATH = os.path.join(TESTFILES_DIR, 'small.jpg')
+TEST_ANIMATEDGIT_PATH = os.path.join(TESTFILES_DIR, 'animated.gif')
 TEST_CORRUPTEDIMG_PATH = os.path.join(TESTFILES_DIR, 'corrupted.gif')
 
 
@@ -200,12 +202,16 @@ class AttachmentsApiTestCase(AuthenticatedUserTestCase):
         self.assertTrue(not attachment.image)
         self.assertTrue(not attachment.thumbnail)
 
+        self.assertTrue(six.text_type(attachment.file).endswith('document.pdf'))
+
         self.assertIsNone(response_json['post'])
         self.assertEqual(response_json['uploader_name'], self.user.username)
+        self.assertEqual(response_json['url']['index'], attachment.get_absolute_url())
+        self.assertIsNone(response_json['url']['thumb'])
         self.assertEqual(response_json['url']['uploader'], self.user.get_absolute_url())
 
-    def test_image_upload(self):
-        """successful upload creates orphan attachment with thumbnail"""
+    def test_small_image_upload(self):
+        """successful small image upload creates orphan attachment without thumbnail"""
         attachment_type = AttachmentType.objects.create(
             name="Test extension",
             extensions='jpeg,jpg',
@@ -227,8 +233,84 @@ class AttachmentsApiTestCase(AuthenticatedUserTestCase):
 
         self.assertTrue(not attachment.file)
         self.assertIsNotNone(attachment.image)
+        self.assertTrue(not attachment.thumbnail)
+
+        self.assertTrue(six.text_type(attachment.image).endswith('small.jpg'))
+
+        self.assertIsNone(response_json['post'])
+        self.assertEqual(response_json['uploader_name'], self.user.username)
+        self.assertEqual(response_json['url']['index'], attachment.get_absolute_url())
+        self.assertIsNone(response_json['url']['thumb'])
+        self.assertEqual(response_json['url']['uploader'], self.user.get_absolute_url())
+
+    def test_large_image_upload(self):
+        """successful large image upload creates orphan attachment with thumbnail"""
+        self.override_acl({
+            'max_attachment_size': 10 * 1024
+        })
+
+        attachment_type = AttachmentType.objects.create(
+            name="Test extension",
+            extensions='png',
+            mimetypes='image/png'
+        )
+
+        with open(TEST_LARGEPNG_PATH, 'rb') as upload:
+            response = self.client.post(self.api_link, data={
+                'upload': upload
+            })
+        self.assertEqual(response.status_code, 200)
+
+        response_json = json.loads(smart_str(response.content))
+        attachment = Attachment.objects.get(id=response_json['id'])
+
+        self.assertEqual(attachment.filename, 'large.png')
+        self.assertFalse(attachment.is_file)
+        self.assertTrue(attachment.is_image)
+
+        self.assertTrue(not attachment.file)
+        self.assertIsNotNone(attachment.image)
+        self.assertIsNotNone(attachment.thumbnail)
+
+        self.assertTrue(six.text_type(attachment.image).endswith('large.png'))
+        self.assertTrue(six.text_type(attachment.thumbnail).endswith('large.png'))
+
+        self.assertIsNone(response_json['post'])
+        self.assertEqual(response_json['uploader_name'], self.user.username)
+        self.assertEqual(response_json['url']['index'], attachment.get_absolute_url())
+        self.assertEqual(response_json['url']['thumb'], attachment.get_thumbnail_url())
+        self.assertEqual(response_json['url']['uploader'], self.user.get_absolute_url())
+
+    def test_animated_image_upload(self):
+        """successful gif upload creates orphan attachment with thumbnail"""
+        attachment_type = AttachmentType.objects.create(
+            name="Test extension",
+            extensions='gif',
+            mimetypes='image/gif'
+        )
+
+        with open(TEST_ANIMATEDGIT_PATH, 'rb') as upload:
+            response = self.client.post(self.api_link, data={
+                'upload': upload
+            })
+        self.assertEqual(response.status_code, 200)
+
+        response_json = json.loads(smart_str(response.content))
+        attachment = Attachment.objects.get(id=response_json['id'])
+
+        self.assertEqual(attachment.filename, 'animated.gif')
+        self.assertFalse(attachment.is_file)
+        self.assertTrue(attachment.is_image)
+
+        self.assertTrue(not attachment.file)
+        self.assertIsNotNone(attachment.image)
         self.assertIsNotNone(attachment.thumbnail)
 
+        self.assertTrue(six.text_type(attachment.image).endswith('animated.gif'))
+        self.assertTrue(six.text_type(attachment.thumbnail).endswith('animated.gif'))
+
         self.assertIsNone(response_json['post'])
         self.assertEqual(response_json['uploader_name'], self.user.username)
+        self.assertEqual(response_json['url']['index'], attachment.get_absolute_url())
+        self.assertEqual(response_json['url']['thumb'], attachment.get_thumbnail_url())
         self.assertEqual(response_json['url']['uploader'], self.user.get_absolute_url())

misago/threads/tests/test_attachmentview.py

@@ -127,7 +127,10 @@ class AttachmentViewTestCase(AuthenticatedUserTestCase):
         attachment = self.upload_document(is_orphaned=True, by_other_user=True)
 
         response = self.client.get(attachment.get_absolute_url())
-        self.assertIs403(response)
+        self.assertIs404(response)
+
+        response = self.client.get(attachment.get_absolute_url() + '?shva=1')
+        self.assertIs404(response)
 
     def test_document_thumbnail(self):
         """user tries to retrieve thumbnail from non-image attachment"""
@@ -191,6 +194,9 @@ class AttachmentViewTestCase(AuthenticatedUserTestCase):
         self.user.save()
 
         response = self.client.get(attachment.get_absolute_url())
+        self.assertIs404(response)
+
+        response = self.client.get(attachment.get_absolute_url() + '?shva=1')
         self.assertSuccess(response)
 
     def test_orphaned_file_is_uploader(self):
@@ -198,6 +204,9 @@ class AttachmentViewTestCase(AuthenticatedUserTestCase):
         attachment = self.upload_document(is_orphaned=True)
 
         response = self.client.get(attachment.get_absolute_url())
+        self.assertIs404(response)
+
+        response = self.client.get(attachment.get_absolute_url() + '?shva=1')
         self.assertSuccess(response)
 
     def test_has_role(self):
@@ -207,19 +216,19 @@ class AttachmentViewTestCase(AuthenticatedUserTestCase):
         user_roles = self.user.get_roles()
         self.attachment_type_pdf.limit_downloads_to.set(user_roles)
 
-        response = self.client.get(attachment.get_absolute_url())
+        response = self.client.get(attachment.get_absolute_url() + '?shva=1')
         self.assertSuccess(response)
 
     def test_image(self):
         """user retrieves """
         attachment = self.upload_image()
 
-        response = self.client.get(attachment.get_absolute_url())
+        response = self.client.get(attachment.get_absolute_url() + '?shva=1')
         self.assertSuccess(response)
 
     def test_image_thumb(self):
         """user retrieves image's thumbnail"""
         attachment = self.upload_image()
 
-        response = self.client.get(attachment.get_thumbnail_url())
+        response = self.client.get(attachment.get_absolute_url() + '?shva=1')
         self.assertSuccess(response)

misago/threads/tests/test_threads_editor_api.py

@@ -626,7 +626,6 @@ class EditReplyEditorApiTests(EditorApiTestCase):
 
     def test_edit(self):
         """endpoint returns valid configuration for editor"""
-        # TODO: HERE UPLOAD ATTACHMENT OR TWO AND TIE THEM TO EDITED POST
         for i in range(3):
             self.override_acl({
                 'max_attachment_size': 1000,

misago/threads/views/attachment.py

@@ -27,6 +27,11 @@ def serve_file(request, pk, secret, thumbnail):
     queryset = Attachment.objects.select_related('filetype')
     attachment = get_object_or_404(queryset, pk=pk, secret=secret)
 
+    if not attachment.post_id and request.GET.get('shva') != '1':
+        # if attachment is orphaned, don't run acl test unless explictly told so
+        # this saves user suprise of deleted attachment still showing in posts/quotes
+        raise Http404()
+
     if not request.user.is_staff:
         allow_file_download(request, attachment)
 
@@ -45,8 +50,10 @@ def serve_file(request, pk, secret, thumbnail):
 def allow_file_download(request, attachment):
     is_authenticated = request.user.is_authenticated()
 
-    if not attachment.post_id or not request.user.acl['can_download_other_users_attachments']:
-        if not is_authenticated or request.user.id != attachment.uploader_id:
+    if not is_authenticated or request.user.id != attachment.uploader_id:
+        if not attachment.post_id:
+            raise Http404()
+        if not request.user.acl['can_download_other_users_attachments']:
             raise PermissionDenied()
 
     allowed_roles = set(r.pk for r in attachment.filetype.limit_downloads_to.all())