Merge pull request #1056 from Benbb96/fix-1054

Better error handling when attachment extension is invalid

helpdesk/lib.py

@@ -9,6 +9,7 @@ lib.py - Common functions (eg multipart e-mail)
 
 from datetime import date, datetime, time
 from django.conf import settings
+from django.core.exceptions import ValidationError
 from django.utils.encoding import smart_str
 from helpdesk.settings import CUSTOMFIELD_DATE_FORMAT, CUSTOMFIELD_DATETIME_FORMAT, CUSTOMFIELD_TIME_FORMAT
 import logging
@@ -132,6 +133,7 @@ def process_attachments(followup, attached_files):
     max_email_attachment_size = getattr(
         settings, 'HELPDESK_MAX_EMAIL_ATTACHMENT_SIZE', 512000)
     attachments = []
+    errors = set()
 
     for attached in attached_files:
 
@@ -148,14 +150,21 @@ def process_attachments(followup, attached_files):
                 'application/octet-stream',
                 size=attached.size,
             )
-            att.full_clean()
-            att.save()
+            try:
+                att.full_clean()
+            except ValidationError as e:
+                errors.add(e)
+            else:
+                att.save()
 
-            if attached.size < max_email_attachment_size:
-                # Only files smaller than 512kb (or as defined in
-                # settings.HELPDESK_MAX_EMAIL_ATTACHMENT_SIZE) are sent via
-                # email.
-                attachments.append([filename, att.file])
+                if attached.size < max_email_attachment_size:
+                    # Only files smaller than 512kb (or as defined in
+                    # settings.HELPDESK_MAX_EMAIL_ATTACHMENT_SIZE) are sent via
+                    # email.
+                    attachments.append([filename, att.file])
+
+    if errors:
+        raise ValidationError(list(errors))
 
     return attachments
 

helpdesk/tests/test_get_email.py

@@ -186,13 +186,51 @@ class GetEmailCommonTests(TestCase):
             object_from_message(message.as_string(), self.queue_public, self.logger)
 
             self.assertIn(
-                "ERROR:helpdesk:{'file': ['Unsupported file extension: .jpg']}",
+                "ERROR:helpdesk:['Unsupported file extension: .jpg']",
                 cm.output
             )
 
         self.assertEqual(len(mail.outbox), 1)
         self.assertEqual(f'[test-1] {message.get("subject")} (Opened)', mail.outbox[0].subject)
 
+    def test_multiple_attachments(self):
+        """
+        Tests the saving of multiple attachments
+        """
+        message, _, _ = utils.generate_multipart_email(type_list=['plain', 'file', 'image'])
+
+        self.assertEqual(len(mail.outbox), 0)
+
+        object_from_message(message.as_string(), self.queue_public, self.logger)
+
+        self.assertEqual(len(mail.outbox), 1)
+        self.assertEqual(f'[test-1] {message.get("subject")} (Opened)', mail.outbox[0].subject)
+
+        ticket = Ticket.objects.get()
+        followup = ticket.followup_set.get()
+        self.assertEqual(2, followup.followupattachment_set.count())
+
+    @override_settings(VALID_EXTENSIONS=['.txt'])
+    def test_multiple_attachments_with_wrong_extension(self):
+        """
+        Tests that a wrong extension won't stop from saving other valid attachment
+        """
+        message, _, _ = utils.generate_multipart_email(type_list=['plain', 'image', 'file', 'image'])
+
+        self.assertEqual(len(mail.outbox), 0)
+
+        with self.assertLogs(logger='helpdesk', level='ERROR') as cm:
+            object_from_message(message.as_string(), self.queue_public, self.logger)
+
+            self.assertIn(
+                "ERROR:helpdesk:['Unsupported file extension: .jpg']",
+                cm.output
+            )
+
+        ticket = Ticket.objects.get()
+        followup = ticket.followup_set.get()
+        self.assertEqual(1, followup.followupattachment_set.count())
+
         
 class GetEmailParametricTemplate(object):
     """TestCase that checks basic email functionality across methods and socks configs."""

helpdesk/tests/utils.py

@@ -205,16 +205,14 @@ def add_simple_email_headers(message: Message, locale: str="en_US",
 
 def generate_mime_part(locale: str="en_US",
         part_type: str="plain",
-        use_short_email: bool=False
         ) -> typing.Optional[Message]:
     """
     Generates amime part of the sepecified type
     
     :param locale: change this to generate locale specific strings
     :param text_type: options are plain, html, image (attachment), file (attachment)
-    :param use_short_email: produces a "To" or "From" that is only the email address if True
     """
-    if  "plain" == part_type:
+    if "plain" == part_type:
         body = get_fake("text", locale=locale, min_length=1024)
         msg = MIMEText(body)
     elif "html" == part_type:
@@ -241,7 +239,7 @@ def generate_multipart_email(locale: str="en_US",
     """    
     msg = MIMEMultipart()
     for part_type in type_list:
-        msg.attach(generate_mime_part(locale=locale, part_type=part_type, use_short_email=use_short_email))
+        msg.attach(generate_mime_part(locale=locale, part_type=part_type))
     from_meta, to_meta = add_simple_email_headers(msg, locale=locale, use_short_email=use_short_email)
     return msg, from_meta, to_meta