Download attachments from mail store instead of querier

- Issue #435
- Improved error handling of attachment resource

7 files changed, 51 insertions, 22 deletions

diff --git a/service/pixelated/adapter/mailstore/leap_mailstore.py b/service/pixelated/adapter/mailstore/leap_mailstore.py
index a3a5e984..993f413c 100644
--- a/service/pixelated/adapter/mailstore/leap_mailstore.py
+++ b/service/pixelated/adapter/mailstore/leap_mailstore.py
@@ -18,7 +18,7 @@ from email.header import decode_header
 import quopri
 import re
 from uuid import uuid4
-from leap.mail.adaptors.soledad import SoledadMailAdaptor
+from leap.mail.adaptors.soledad import SoledadMailAdaptor, ContentDocWrapper
 from twisted.internet import defer
 from pixelated.adapter.mailstore.body_parser import BodyParser
 from pixelated.adapter.mailstore.mailstore import MailStore, underscore_uuid
@@ -182,7 +182,7 @@ class LeapMailStore(MailStore):
     def get_mail_attachment(self, attachment_id):
         results = yield self.soledad.get_from_index('by-type-and-payloadhash', 'cnt', attachment_id) if attachment_id else []
         if len(results):
-            content = results[0]
+            content = ContentDocWrapper(**results[0].content)
             defer.returnValue({'content-type': content.content_type, 'content': self._try_decode(
                 content.raw, content.content_transfer_encoding)})
         else:
diff --git a/service/pixelated/adapter/services/mail_service.py b/service/pixelated/adapter/services/mail_service.py
index 4ff42046..2c576e98 100644
--- a/service/pixelated/adapter/services/mail_service.py
+++ b/service/pixelated/adapter/services/mail_service.py
@@ -65,8 +65,8 @@ class MailService(object):
     def mail(self, mail_id):
         return self.mail_store.get_mail(mail_id, include_body=True)
 
-    def attachment(self, attachment_id, encoding):
-        return self.querier.attachment(attachment_id, encoding)
+    def attachment(self, attachment_id):
+        return self.mail_store.get_mail_attachment(attachment_id)
 
     @defer.inlineCallbacks
     def mail_exists(self, mail_id):
diff --git a/service/pixelated/resources/attachments_resource.py b/service/pixelated/resources/attachments_resource.py
index 0a903cc4..a78022ec 100644
--- a/service/pixelated/resources/attachments_resource.py
+++ b/service/pixelated/resources/attachments_resource.py
@@ -18,7 +18,7 @@ import io
 
 import re
 from twisted.protocols.basic import FileSender
-from twisted.python.log import err
+from twisted.python.log import msg
 from twisted.web import server
 from twisted.web.resource import Resource
 from twisted.internet import defer
@@ -34,23 +34,28 @@ class AttachmentResource(Resource):
         self.mail_service = mail_service
 
     def render_GET(self, request):
+        def error_handler(failure):
+            msg(failure, 'attachment not found')
+            request.code = 404
+            request.finish()
         encoding = request.args.get('encoding', [None])[0]
         filename = request.args.get('filename', [self.attachment_id])[0]
         request.setHeader(b'Content-Type', b'application/force-download')
         request.setHeader(b'Content-Disposition', bytes('attachment; filename=' + filename))
 
         d = self._send_attachment(encoding, filename, request)
-        d.addErrback(err)
+        d.addErrback(error_handler)
 
         return server.NOT_DONE_YET
 
     @defer.inlineCallbacks
     def _send_attachment(self, encoding, filename, request):
-        attachment = yield self.mail_service.attachment(self.attachment_id, encoding)
+        attachment = yield self.mail_service.attachment(self.attachment_id)
 
         bytes_io = io.BytesIO(attachment['content'])
 
         try:
+            request.code = 200
             yield FileSender().beginFileTransfer(bytes_io, request)
         finally:
             bytes_io.close()
diff --git a/service/test/integration/test_retrieve_attachment.py b/service/test/integration/test_retrieve_attachment.py
index 1cf8006f..e7e8670d 100644
--- a/service/test/integration/test_retrieve_attachment.py
+++ b/service/test/integration/test_retrieve_attachment.py
@@ -13,6 +13,9 @@
 #
 # You should have received a copy of the GNU Affero General Public License
 # along with Pixelated. If not, see <http://www.gnu.org/licenses/>.
+from email.mime.application import MIMEApplication
+from email.mime.multipart import MIMEMultipart
+from email.mime.text import MIMEText
 
 from test.support.integration.soledad_test_base import SoledadTestBase
 from twisted.internet import defer
@@ -22,16 +25,26 @@ class RetrieveAttachmentTest(SoledadTestBase):
 
     @defer.inlineCallbacks
     def test_attachment_content_is_retrieved(self):
-        ident = 'F4E99C1CEC4D300A4223A96CCABBE0304BDBC31C550A5A03E207A5E4C3C71A22'
-        attachment_dict = {'content-disposition': 'attachment',
-                           'content-transfer-encoding': '',
-                           'type': 'cnt',
-                           'raw': 'cGVxdWVubyBhbmV4byA6RAo=',
-                           'phash': ident,
-                           'content_type': 'text/plain; charset=US-ASCII; name="attachment_pequeno.txt"'}
+        attachment_id, input_mail = self._create_mail_with_attachment()
+        yield self.mail_store.add_mail('INBOX', input_mail.as_string())
 
-        yield self.add_document_to_soledad(attachment_dict)
+        attachment, req = yield self.get_attachment(attachment_id, 'base64')
 
-        attachment = yield self.get_attachment(ident, 'base64')
+        self.assertEqual(200, req.code)
+        self.assertEquals('pretend to be binary attachment data', attachment)
 
-        self.assertEquals('pequeno anexo :D\n', attachment)
+    def _create_mail_with_attachment(self):
+        input_mail = MIMEMultipart()
+        input_mail.attach(MIMEText(u'a utf8 message', _charset='utf-8'))
+        attachment = MIMEApplication('pretend to be binary attachment data')
+        attachment.add_header('Content-Disposition', 'attachment', filename='filename.txt')
+        input_mail.attach(attachment)
+        attachment_id = 'B5B4ED80AC3B894523D72E375DACAA2FC6606C18EDF680FE95903086C8B5E14A'
+        return attachment_id, input_mail
+
+    @defer.inlineCallbacks
+    def test_attachment_error_returned_if_id_not_found(self):
+        attachment, req = yield self.get_attachment('invalid attachment id', 'base64')
+
+        self.assertEqual(404, req.code)
+        self.assertIsNone(attachment)
diff --git a/service/test/support/integration/app_test_client.py b/service/test/support/integration/app_test_client.py
index fe9eaffd..51fbf483 100644
--- a/service/test/support/integration/app_test_client.py
+++ b/service/test/support/integration/app_test_client.py
@@ -193,9 +193,11 @@ class AppTestClient(object):
         res = yield res
         defer.returnValue([ResponseMail(m) for m in res['mails']])
 
+    @defer.inlineCallbacks
     def get_attachment(self, ident, encoding):
-        res, req = self.get("/attachment/%s" % ident, {'encoding': [encoding]}, as_json=False)
-        return res
+        deferred_result, req = self.get("/attachment/%s" % ident, {'encoding': [encoding]}, as_json=False)
+        res = yield deferred_result
+        defer.returnValue((res, req))
 
     def put_mail(self, data):
         res, req = self.put('/mails', data)
diff --git a/service/test/unit/adapter/mailstore/test_leap_mailstore.py b/service/test/unit/adapter/mailstore/test_leap_mailstore.py
index 7cbd1405..96a862be 100644
--- a/service/test/unit/adapter/mailstore/test_leap_mailstore.py
+++ b/service/test/unit/adapter/mailstore/test_leap_mailstore.py
@@ -137,7 +137,7 @@ class TestLeapMailStore(TestCase):
     @defer.inlineCallbacks
     def test_get_mail_attachment(self):
         attachment_id = 'AAAA9AAD9E153D24265395203C53884506ABA276394B9FEC02B214BF9E77E48E'
-        doc = ContentDocWrapper(content_type='foo/bar', raw='asdf')
+        doc = SoledadDocument(json=json.dumps({'content_type': 'foo/bar', 'raw': 'asdf'}))
         when(self.soledad).get_from_index('by-type-and-payloadhash', 'cnt', attachment_id).thenReturn(defer.succeed([doc]))
         store = LeapMailStore(self.soledad)
 
@@ -153,8 +153,8 @@ class TestLeapMailStore(TestCase):
                              ('quoted-printable', 'äsdf', '=C3=A4sdf')]
 
         for transfer_encoding, data, encoded_data in encoding_examples:
-            doc = ContentDocWrapper(content_type='foo/bar', raw=encoded_data,
-                                    content_transfer_encoding=transfer_encoding)
+            doc = SoledadDocument(json=json.dumps({'content_type': 'foo/bar', 'raw': encoded_data,
+                                                   'content_transfer_encoding': transfer_encoding}))
             when(self.soledad).get_from_index('by-type-and-payloadhash', 'cnt', attachment_id).thenReturn(defer.succeed([doc]))
             store = LeapMailStore(self.soledad)
 
diff --git a/service/test/unit/adapter/test_mail_service.py b/service/test/unit/adapter/test_mail_service.py
index 79161a04..30784769 100644
--- a/service/test/unit/adapter/test_mail_service.py
+++ b/service/test/unit/adapter/test_mail_service.py
@@ -130,3 +130,12 @@ class TestMailService(unittest.TestCase):
         yield self.mail_service.recover_mail(1)
 
         verify(self.mail_store).move_mail_to_mailbox(1, 'INBOX')
+
+    @defer.inlineCallbacks
+    def test_get_attachment(self):
+        attachment_dict = {'content': bytearray('data'), 'content-type': 'text/plain'}
+        when(self.mail_store).get_mail_attachment('some attachment id').thenReturn(defer.succeed(attachment_dict))
+
+        attachment = yield self.mail_service.attachment('some attachment id')
+
+        self.assertEqual(attachment_dict, attachment)