From 6cb5a294c501b802904c85009e6a80593d1d2e69 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= <mbajtos@cz.ibm.com>
Date: Thu, 10 Nov 2016 14:31:45 +0100
Subject: [PATCH] Continue middleware chain after download error

Stop sending constructing the error response directly inside the
storage component and send the error back to the application to handle
it.

As a result:

 - `File.afterRemoteError` hook is trigered now

 - App-provided error handling strategy is used to build error
   responses, e.g. using the production mode of strong-error-handler
   to hide sensitive information.
---
 lib/storage-handler.js       | 28 ++++++++++++++++------------
 test/upload-download.test.js | 18 ++++++++++++++++++
 2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/lib/storage-handler.js b/lib/storage-handler.js
index 96080ea..52733a2 100644
--- a/lib/storage-handler.js
+++ b/lib/storage-handler.js
@@ -204,16 +204,6 @@ exports.upload = function(provider, req, res, options, cb) {
   });
 };
 
-function handleError(res, err) {
-  if (err.code === 'ENOENT') {
-    res.type('application/json');
-    res.status(404).send({error: err});
-    return;
-  }
-  res.type('application/json');
-  res.status(500).send({error: err});
-}
-
 /**
  * Handle download from a container/file.
  * @param {Object} provider The storage service provider
@@ -248,7 +238,7 @@ exports.download = function(provider, req, res, container, file, cb) {
 
   provider.getFile(params.container, params.remote, function(err, stats) {
     if (err) {
-      return handleError(res, err);
+      return cb(processError(err, params.remote));
     }
 
     setupPartialDownload(params, stats, res);
@@ -261,11 +251,15 @@ exports.download = function(provider, req, res, container, file, cb) {
     res.type(fileName);
 
     reader.pipe(res);
+
     reader.on('error', function onReaderError(err) {
-      handleError(res, err);
+      cb(processError(err, params.remote));
+      cb = function() {}; // avoid double-callback
     });
+
     reader.on('end', function onReaderEnd() {
       cb();
+      cb = function() {}; // avoid double-callback
     });
   }
 };
@@ -287,3 +281,13 @@ function setupPartialDownload(params, stats, res) {
   res.set('Accept-Ranges', 'bytes');
   res.set('Content-Length', chunksize);
 };
+
+function processError(err, fileName) {
+  if (err.code === 'ENOENT') {
+    err.statusCode = err.status = 404;
+    // Hide the original message reported e.g. by FS provider, as it may
+    // contain sensitive information.
+    err.message = 'File not found: ' + fileName;
+  }
+  return err;
+}
diff --git a/test/upload-download.test.js b/test/upload-download.test.js
index a7d3728..7bc16e6 100644
--- a/test/upload-download.test.js
+++ b/test/upload-download.test.js
@@ -314,6 +314,24 @@ describe('storage service', function() {
       });
   });
 
+  it('should run a function after a download failed', function(done) {
+    var hookCalled = false;
+    var Container = app.models.Container;
+
+    Container.afterRemoteError('download', function(ctx, cb) {
+      hookCalled = true;
+      cb();
+    });
+
+    request('http://localhost:' + app.get('port'))
+      .get('/containers/album1/download/does-not-exist')
+      .expect(404, function(err, res) {
+        if (err) return done(err);
+        assert(hookCalled, 'afterRemoteEror hook was not called');
+        done();
+      });
+  });
+
   it('should delete a file', function(done) {
     request('http://localhost:' + app.get('port'))
       .del('/containers/album1/files/test.jpg')