[bug] check all http response status codes

4 files changed, 87 insertions, 35 deletions

diff --git a/src/leap/soledad/client/_db/blobs.py b/src/leap/soledad/client/_db/blobs.py
index 6d47999a..e997c6ff 100644
--- a/src/leap/soledad/client/_db/blobs.py
+++ b/src/leap/soledad/client/_db/blobs.py
@@ -132,11 +132,13 @@ class ConnectionPool(adbapi.ConnectionPool):
         return handle
 
 
-def check_http_status(code):
+def check_http_status(code, blob_id=None, flags=None):
+    if code == 404:
+        raise BlobNotFoundError(blob_id)
     if code == 409:
-        raise BlobAlreadyExistsError()
+        raise BlobAlreadyExistsError(blob_id)
     elif code == 406:
-        raise InvalidFlagsError()
+        raise InvalidFlagsError((blob_id, flags))
     elif code != 200:
         raise SoledadError("Server Error: %s" % code)
 
@@ -253,8 +255,9 @@ class BlobManager(object):
         :rtype: twisted.internet.defer.Deferred
         """
         uri = urljoin(self.remote, self.user + '/')
-        data = yield self._client.get(uri, params=params)
-        defer.returnValue((yield data.json()))
+        response = yield self._client.get(uri, params=params)
+        check_http_status(response.code)
+        defer.returnValue((yield response.json()))
 
     def local_list(self, namespace='', sync_status=None):
         return self.local.list(namespace, sync_status)
@@ -350,15 +353,10 @@ class BlobManager(object):
         :return: A deferred that fires when the operation finishes.
         :rtype: twisted.internet.defer.Deferred
         """
-        flags = BytesIO(json.dumps(flags))
+        flagsfd = BytesIO(json.dumps(flags))
         uri = urljoin(self.remote, self.user + "/" + blob_id)
-        response = yield self._client.post(uri, data=flags, params=params)
-        if response.code == 404:
-            logger.error("Blob not found during set_flags: %s" % blob_id)
-            msg = "No blob found on server while trying to set flags: %s"
-            raise SoledadError(msg % (blob_id))
-
-        check_http_status(response.code)
+        response = yield self._client.post(uri, data=flagsfd, params=params)
+        check_http_status(response.code, blob_id=blob_id, flags=flags)
 
     @defer.inlineCallbacks
     def get_flags(self, blob_id, **params):
@@ -378,9 +376,7 @@ class BlobManager(object):
         uri = urljoin(self.remote, self.user + "/" + blob_id)
         params.update({'only_flags': True})
         response = yield self._client.get(uri, params=params)
-        if response.code == 404:
-            logger.error("Blob not found in server: %r" % blob_id)
-            raise BlobNotFoundError(blob_id)
+        check_http_status(response.code, blob_id=blob_id)
         defer.returnValue((yield response.json()))
 
     @defer.inlineCallbacks
@@ -435,7 +431,7 @@ class BlobManager(object):
                                 armor=False)
         fd = yield crypter.encrypt()
         response = yield self._client.put(uri, data=fd, params=params)
-        check_http_status(response.code)
+        check_http_status(response.code, blob_id)
         logger.info("Finished upload: %s" % (blob_id,))
 
     @defer.inlineCallbacks
@@ -444,20 +440,20 @@ class BlobManager(object):
         # TODO this needs to be connected in a tube
         uri = urljoin(self.remote, self.user + '/' + blob_id)
         params = {'namespace': namespace} if namespace else None
-        data = yield self._client.get(uri, params=params, namespace=namespace)
-
-        if data.code == 404:
-            logger.warn("Blob not found in server: %s" % blob_id)
-            defer.returnValue(None)
-        elif not data.headers.hasHeader('Tag'):
-            logger.error("Server didn't send a tag header for: %s" % blob_id)
-            defer.returnValue(None)
-        tag = data.headers.getRawHeaders('Tag')[0]
+        response = yield self._client.get(uri, params=params,
+                                          namespace=namespace)
+        check_http_status(response.code, blob_id=blob_id)
+
+        if not response.headers.hasHeader('Tag'):
+            msg = "Server didn't send a tag header for: %s" % blob_id
+            logger.error(msg)
+            raise SoledadError(msg)
+        tag = response.headers.getRawHeaders('Tag')[0]
         tag = base64.urlsafe_b64decode(tag)
         buf = DecrypterBuffer(blob_id, self.secret, tag)
 
         # incrementally collect the body of the response
-        yield treq.collect(data, buf.write)
+        yield treq.collect(response, buf.write)
         fd, size = buf.close()
         logger.info("Finished download: (%s, %d)" % (blob_id, size))
         defer.returnValue((fd, size))
@@ -482,10 +478,13 @@ class BlobManager(object):
         if (yield self.local.exists(blob_id, namespace)):
             yield self.local.delete(blob_id, namespace)
 
+    @defer.inlineCallbacks
     def _delete_from_remote(self, blob_id, **params):
         # TODO this needs to be connected in a tube
         uri = urljoin(self.remote, self.user + '/' + blob_id)
-        return self._client.delete(uri, params=params)
+        response = yield self._client.delete(uri, params=params)
+        check_http_status(response.code, blob_id=blob_id)
+        defer.returnValue(response)
 
 
 class SQLiteBlobBackend(object):
diff --git a/src/leap/soledad/server/_blobs.py b/src/leap/soledad/server/_blobs.py
index 790a9b51..cacfe38d 100644
--- a/src/leap/soledad/server/_blobs.py
+++ b/src/leap/soledad/server/_blobs.py
@@ -124,8 +124,11 @@ class FilesystemBlobsBackend(object):
         with open(path, 'wb') as blobfile:
             yield fbp.startProducing(blobfile)
 
-    def delete_blob(self, user, blob_id, namespace=''):
+    def delete_blob(self, user, blob_id, request, namespace=''):
         blob_path = self._get_path(user, blob_id, namespace)
+        if not os.path.isfile(blob_path):
+            request.setResponseCode(404)
+            return "Blob doesn't exists: %s" % blob_id
         os.unlink(blob_path)
         try:
             os.unlink(blob_path + '.flags')
@@ -175,7 +178,12 @@ class FilesystemBlobsBackend(object):
         return self._get_disk_usage(self._get_path(user))
 
     def add_tag_header(self, user, blob_id, request, namespace=''):
-        with open(self._get_path(user, blob_id, namespace)) as doc_file:
+        blob_path = self._get_path(user, blob_id, namespace)
+        if not os.path.isfile(blob_path):
+            # 404 - Not Found
+            request.setResponseCode(404)
+            return "Blob doesn't exists: %s" % blob_id
+        with open(blob_path) as doc_file:
             doc_file.seek(-16, 2)
             tag = base64.urlsafe_b64encode(doc_file.read())
             request.responseHeaders.setRawHeaders('Tag', [tag])
@@ -202,6 +210,10 @@ class FilesystemBlobsBackend(object):
             raise Exception(err)
         return desired_path
 
+    def exists(self, user, blob_id, namespace):
+        return os.path.isfile(
+            self._get_path(user, blob_id=blob_id, namespace=namespace))
+
     def _get_path(self, user, blob_id='', namespace=''):
         parts = [user]
         if blob_id:
@@ -262,7 +274,7 @@ class BlobsResource(resource.Resource):
     def render_DELETE(self, request):
         logger.info("http put: %s" % request.path)
         user, blob_id, namespace = self._validate(request)
-        self._handler.delete_blob(user, blob_id, namespace)
+        self._handler.delete_blob(user, blob_id, request, namespace=namespace)
         return ''
 
     def render_PUT(self, request):
diff --git a/tests/blobs/test_fs_backend.py b/tests/blobs/test_fs_backend.py
index 53f3127d..07443b8a 100644
--- a/tests/blobs/test_fs_backend.py
+++ b/tests/blobs/test_fs_backend.py
@@ -20,6 +20,7 @@ Tests for blobs backend on server side.
 from twisted.trial import unittest
 from twisted.internet import defer
 from twisted.web.test.test_web import DummyRequest
+from leap.common.files import mkdir_p
 from leap.soledad.server import _blobs
 from io import BytesIO
 from mock import Mock
@@ -38,18 +39,25 @@ class FilesystemBackendTestCase(unittest.TestCase):
         expected_tag = base64.urlsafe_b64encode('B' * 16)
         expected_method = Mock()
         backend = _blobs.FilesystemBlobsBackend()
+        # write a blob...
+        path = backend._get_path('user', 'blob_id', '')
+        mkdir_p(os.path.split(path)[0])
+        with open(path, "w") as f:
+            f.write("bl0b")
+        # ...and get its tag
         request = Mock(responseHeaders=Mock(setRawHeaders=expected_method))
         backend.add_tag_header('user', 'blob_id', request)
 
         expected_method.assert_called_once_with('Tag', [expected_tag])
 
     @mock.patch.object(_blobs.static, 'File')
+    @mock.patch.object(_blobs.FilesystemBlobsBackend, '_get_path',
+                       Mock(return_value='path'))
     def test_read_blob(self, file_mock):
         render_mock = Mock()
         file_mock.return_value = render_mock
         backend = _blobs.FilesystemBlobsBackend()
         request = DummyRequest([''])
-        backend._get_path = Mock(return_value='path')
         backend.read_blob('user', 'blob_id', request)
 
         backend._get_path.assert_called_once_with('user', 'blob_id', '')
@@ -58,11 +66,12 @@ class FilesystemBackendTestCase(unittest.TestCase):
         render_mock.render_GET.assert_called_once_with(request)
 
     @mock.patch.object(os.path, 'isfile')
+    @mock.patch.object(_blobs.FilesystemBlobsBackend, '_get_path',
+                       Mock(return_value='path'))
     @defer.inlineCallbacks
     def test_cannot_overwrite(self, isfile):
         isfile.return_value = True
         backend = _blobs.FilesystemBlobsBackend()
-        backend._get_path = Mock(return_value='path')
         request = DummyRequest([''])
         yield backend.write_blob('user', 'blob_id', request)
         self.assertEquals(request.written[0], "Blob already exists: blob_id")
@@ -154,7 +163,14 @@ class FilesystemBackendTestCase(unittest.TestCase):
     @mock.patch('leap.soledad.server._blobs.os.unlink')
     def test_delete_blob(self, unlink_mock):
         backend = _blobs.FilesystemBlobsBackend(self.tempdir)
-        backend.delete_blob('user', 'blob_id')
+        request = DummyRequest([''])
+        # write a blob...
+        path = backend._get_path('user', 'blob_id', '')
+        mkdir_p(os.path.split(path)[0])
+        with open(path, "w") as f:
+            f.write("bl0b")
+        # ...and delete it
+        backend.delete_blob('user', 'blob_id', request)
         unlink_mock.assert_any_call(backend._get_path('user',
                                                       'blob_id'))
         unlink_mock.assert_any_call(backend._get_path('user',
@@ -164,7 +180,14 @@ class FilesystemBackendTestCase(unittest.TestCase):
     @mock.patch('leap.soledad.server._blobs.os.unlink')
     def test_delete_blob_custom_namespace(self, unlink_mock):
         backend = _blobs.FilesystemBlobsBackend(self.tempdir)
-        backend.delete_blob('user', 'blob_id', namespace='trash')
+        request = DummyRequest([''])
+        # write a blob...
+        path = backend._get_path('user', 'blob_id', 'trash')
+        mkdir_p(os.path.split(path)[0])
+        with open(path, "w") as f:
+            f.write("bl0b")
+        # ...and delete it
+        backend.delete_blob('user', 'blob_id', request, namespace='trash')
         unlink_mock.assert_any_call(backend._get_path('user',
                                                       'blob_id',
                                                       'trash'))
diff --git a/tests/server/test_blobs_server.py b/tests/server/test_blobs_server.py
index 2ee6fda2..c628ae78 100644
--- a/tests/server/test_blobs_server.py
+++ b/tests/server/test_blobs_server.py
@@ -292,3 +292,21 @@ class BlobServerTestCase(unittest.TestCase):
         yield manager._delete_from_remote('blob_id1', namespace=namespace)
         blobs_list = yield manager.remote_list(namespace=namespace)
         self.assertEquals(set(['blob_id2']), set(blobs_list))
+
+    @defer.inlineCallbacks
+    @pytest.mark.usefixtures("method_tmpdir")
+    def test_get_fails_if_no_blob_found(self):
+        manager = BlobManager(self.tempdir, self.uri, self.secret,
+                              self.secret, uuid4().hex)
+        self.addCleanup(manager.close)
+        with pytest.raises(SoledadError):
+            yield manager.get('missing_id')
+
+    @defer.inlineCallbacks
+    @pytest.mark.usefixtures("method_tmpdir")
+    def test_delete_fails_if_no_blob_found(self):
+        manager = BlobManager(self.tempdir, self.uri, self.secret,
+                              self.secret, uuid4().hex)
+        self.addCleanup(manager.close)
+        with pytest.raises(SoledadError):
+            yield manager.delete('missing_id')