Allow IAttachmentManipulator to easily check on the uploaded attachment's file data

[Carsten Klein <carsten.klein@…>]

Currently, both the filename and the file data is not available to the manipulator, unless it would reimplement part of the logic found in AttachmentModule#_do_save.

_do_save already does some verification on the uploaded file, which I presume is present in some temporary location on the local filesystem.

My proposal would be to also pass in the upload file object associated with that file to the manipulator so that it is able to verify its contents, for example scanning it with an antivirus scanner and so on.

See also ticket #9280.

[Remy Blank]

Please don't set the milestone arbitrarily. Better leave it empty, we'll take care of it.

[Carsten Klein <carsten.klein@…>]

Will do.

I was not sure whether the unbound method from IAttachmentManipulator could be changed so that it will feature all of the required parameters to the validate_attachment method, hence I simply included it with the doc string.

Here is a proposed patch OTOH and untested:

attachment.py

@@ -34 +34 @@
 from trac.perm import PermissionError, IPermissionPolicy
 from trac.resource import *
 from trac.search import search_to_sql, shorten_result
-from trac.util import get_reporter_id, create_unique_file
+from trac.util import get_reporter_id, create_unique_file, arity
 from trac.util.datefmt import format_datetime, from_utimestamp, \
                               to_datetime, to_utimestamp, utc
 from trac.util.text import exception_to_unicode, pretty_size, print_table, \
@@ -85 +85 @@
         detected. `field` can be any of `description`, `username`, `filename`,
         `content`, or `None` to indicate an overall problem with the
         attachment. Therefore, a return value of `[]` means everything is
-        OK."""
+        OK.
+        
+        New style manipulators can also validate the uploaded file's content.
+        In order to do this, they will have to implement a third parameter
+        to this method, e.g.
+        
+        def validate_attachment(self, req, attachment, upload):
+            ...
+        """
 
 class ILegacyAttachmentPolicyDelegate(Interface):
     """Interface that can be used by plugins to seemlessly participate to the
@@ -637 +645 @@
         attachment.ipnr = req.remote_addr
 
         # Validate attachment
+        def validate(manipulator):
+
+            def validate_old(req, attachment):
+                return manipulator.validate_attachment(req, attachment)
+
+            def validate_new(req, attachment, upload):
+                return manipulator.validate_attachment(req, attachment, 
+                                                      upload)
+            if arity(manipulator.validate_attachment) == 2:
+                return validate_old
+            return validate_new
+
         for manipulator in self.manipulators:
-            for field, message in manipulator.validate_attachment(req,
-                                                                  attachment):
-                if field:
-                    raise InvalidAttachment(_('Attachment field %(field)s is '
-                                              'invalid: %(message)s',
-                                              field=field, message=message))
-                else:
-                    raise InvalidAttachment(_('Invalid attachment: %(message)s',
+                for field, message in validate(manipulator):
+                    if field:
+                        raise InvalidAttachment(_('Attachment field %(field)s '
+                                                  'is invalid: %(message)s',
+                                                  field=field, message=message))
+                    else:
+                        raise InvalidAttachment(_('Invalid attachment: %(message)s',
                                               message=message))
 
         if req.args.get('replace'):

[Carsten Klein <carsten.klein@…>]

Just tested and it turned out that the function was not iterable, of course *sigh*:

attachment.py

@@ -34 +34 @@
 from trac.perm import PermissionError, IPermissionPolicy
 from trac.resource import *
 from trac.search import search_to_sql, shorten_result
-from trac.util import get_reporter_id, create_unique_file
+from trac.util import get_reporter_id, create_unique_file, arity
 from trac.util.datefmt import format_datetime, from_utimestamp, \
                               to_datetime, to_utimestamp, utc
 from trac.util.text import exception_to_unicode, pretty_size, print_table, \
@@ -85 +85 @@
         detected. `field` can be any of `description`, `username`, `filename`,
         `content`, or `None` to indicate an overall problem with the
         attachment. Therefore, a return value of `[]` means everything is
-        OK."""
+        OK.
+        
+        New style manipulators can also validate the uploaded file's content.
+        In order to do this, they will have to implement a third parameter
+        to this method, e.g.
+        
+        def validate_attachment(self, req, attachment, upload):
+            ...
+        """
 
 class ILegacyAttachmentPolicyDelegate(Interface):
     """Interface that can be used by plugins to seemlessly participate to the
@@ -637 +645 @@
         attachment.ipnr = req.remote_addr
 
         # Validate attachment
+        def validate(manipulator):
+
+            def validate_old():
+                return manipulator.validate_attachment(req, attachment)
+
+            def validate_new():
+                return manipulator.validate_attachment(req, attachment, 
+                                                      upload)
+            if arity(manipulator.validate_attachment) == 2:
+                return validate_old
+            return validate_new
+
         for manipulator in self.manipulators:
-            for field, message in manipulator.validate_attachment(req,
-                                                                  attachment):
-                if field:
-                    raise InvalidAttachment(_('Attachment field %(field)s is '
-                                              'invalid: %(message)s',
-                                              field=field, message=message))
-                else:
-                    raise InvalidAttachment(_('Invalid attachment: %(message)s',
+                for field, message in validate(manipulator)():
+                    if field:
+                        raise InvalidAttachment(_('Attachment field %(field)s '
+                                                  'is invalid: %(message)s',
+                                                  field=field, message=message))
+                    else:
+                        raise InvalidAttachment(_('Invalid attachment: %(message)s',
                                               message=message))
 
         if req.args.get('replace'):

[Carsten Klein <carsten.klein@…>]

Dear me, the validate() function is suboptimal in that it should directly call the chosen manipulator and return the result from that call