From f877f51775c56949970811dbc7ef1650b50c7477 Mon Sep 17 00:00:00 2001
From: Simon Detheridge <s@sd.ai>
Date: Tue, 14 Jan 2020 12:18:56 +0000
Subject: [PATCH 01/12] Rename *PersistorManager to *Persistor

---
 .../{FSPersistorManager.js => FSPersistor.js} |  0
 services/filestore/app/js/PersistorManager.js |  4 +-
 .../{S3PersistorManager.js => S3Persistor.js} |  0
 ...torManagerTests.js => FSPersistorTests.js} | 89 +++++++------------
 .../test/unit/js/PersistorManagerTests.js     | 24 +++--
 ...torManagerTests.js => S3PersistorTests.js} | 89 ++++++++-----------
 6 files changed, 80 insertions(+), 126 deletions(-)
 rename services/filestore/app/js/{FSPersistorManager.js => FSPersistor.js} (100%)
 rename services/filestore/app/js/{S3PersistorManager.js => S3Persistor.js} (100%)
 rename services/filestore/test/unit/js/{FSPersistorManagerTests.js => FSPersistorTests.js} (75%)
 rename services/filestore/test/unit/js/{S3PersistorManagerTests.js => S3PersistorTests.js} (87%)

diff --git a/services/filestore/app/js/FSPersistorManager.js b/services/filestore/app/js/FSPersistor.js
similarity index 100%
rename from services/filestore/app/js/FSPersistorManager.js
rename to services/filestore/app/js/FSPersistor.js
diff --git a/services/filestore/app/js/PersistorManager.js b/services/filestore/app/js/PersistorManager.js
index cca0cf0f36..bd944a03f7 100644
--- a/services/filestore/app/js/PersistorManager.js
+++ b/services/filestore/app/js/PersistorManager.js
@@ -14,10 +14,10 @@ if (!settings.filestore.backend) {
 switch (settings.filestore.backend) {
   case 'aws-sdk':
   case 's3':
-    module.exports = require('./S3PersistorManager')
+    module.exports = require('./S3Persistor')
     break
   case 'fs':
-    module.exports = require('./FSPersistorManager')
+    module.exports = require('./FSPersistor')
     break
   default:
     throw new Error(`unknown filestore backend: ${settings.filestore.backend}`)
diff --git a/services/filestore/app/js/S3PersistorManager.js b/services/filestore/app/js/S3Persistor.js
similarity index 100%
rename from services/filestore/app/js/S3PersistorManager.js
rename to services/filestore/app/js/S3Persistor.js
diff --git a/services/filestore/test/unit/js/FSPersistorManagerTests.js b/services/filestore/test/unit/js/FSPersistorTests.js
similarity index 75%
rename from services/filestore/test/unit/js/FSPersistorManagerTests.js
rename to services/filestore/test/unit/js/FSPersistorTests.js
index 3b3b4bf417..ba343c548c 100644
--- a/services/filestore/test/unit/js/FSPersistorManagerTests.js
+++ b/services/filestore/test/unit/js/FSPersistorTests.js
@@ -7,9 +7,9 @@ const Errors = require('../../../app/js/Errors')
 chai.use(require('sinon-chai'))
 chai.use(require('chai-as-promised'))
 
-const modulePath = '../../../app/js/FSPersistorManager.js'
+const modulePath = '../../../app/js/FSPersistor.js'
 
-describe('FSPersistorManagerTests', function() {
+describe('FSPersistorTests', function() {
   const stat = { size: 4, isFile: sinon.stub().returns(true) }
   const fd = 1234
   const readStream = 'readStream'
@@ -22,7 +22,7 @@ describe('FSPersistorManagerTests', function() {
   const files = ['animals/wombat.tex', 'vegetables/potato.tex']
   const globs = [`${location}/${files[0]}`, `${location}/${files[1]}`]
   const filteredFilenames = ['animals_wombat.tex', 'vegetables_potato.tex']
-  let fs, rimraf, stream, LocalFileWriter, FSPersistorManager, glob
+  let fs, rimraf, stream, LocalFileWriter, FSPersistor, glob
 
   beforeEach(function() {
     fs = {
@@ -41,7 +41,7 @@ describe('FSPersistorManagerTests', function() {
         deleteFile: sinon.stub().resolves()
       }
     }
-    FSPersistorManager = SandboxedModule.require(modulePath, {
+    FSPersistor = SandboxedModule.require(modulePath, {
       requires: {
         './LocalFileWriter': LocalFileWriter,
         './Errors': Errors,
@@ -57,7 +57,7 @@ describe('FSPersistorManagerTests', function() {
   describe('sendFile', function() {
     const localFilesystemPath = '/path/to/local/file'
     it('should copy the file', async function() {
-      await FSPersistorManager.promises.sendFile(
+      await FSPersistor.promises.sendFile(
         location,
         files[0],
         localFilesystemPath
@@ -72,33 +72,21 @@ describe('FSPersistorManagerTests', function() {
     it('should return an error if the file cannot be stored', async function() {
       stream.pipeline.yields(error)
       await expect(
-        FSPersistorManager.promises.sendFile(
-          location,
-          files[0],
-          localFilesystemPath
-        )
+        FSPersistor.promises.sendFile(location, files[0], localFilesystemPath)
       ).to.eventually.be.rejected.and.have.property('cause', error)
     })
   })
 
   describe('sendStream', function() {
     it('should send the stream to LocalFileWriter', async function() {
-      await FSPersistorManager.promises.sendStream(
-        location,
-        files[0],
-        remoteStream
-      )
+      await FSPersistor.promises.sendStream(location, files[0], remoteStream)
       expect(LocalFileWriter.promises.writeStream).to.have.been.calledWith(
         remoteStream
       )
     })
 
     it('should delete the temporary file', async function() {
-      await FSPersistorManager.promises.sendStream(
-        location,
-        files[0],
-        remoteStream
-      )
+      await FSPersistor.promises.sendStream(location, files[0], remoteStream)
       expect(LocalFileWriter.promises.deleteFile).to.have.been.calledWith(
         tempFile
       )
@@ -107,30 +95,26 @@ describe('FSPersistorManagerTests', function() {
     it('should return the error from LocalFileWriter', async function() {
       LocalFileWriter.promises.writeStream.rejects(error)
       await expect(
-        FSPersistorManager.promises.sendStream(location, files[0], remoteStream)
+        FSPersistor.promises.sendStream(location, files[0], remoteStream)
       ).to.eventually.be.rejectedWith(error)
     })
 
     it('should send the temporary file to the filestore', async function() {
-      await FSPersistorManager.promises.sendStream(
-        location,
-        files[0],
-        remoteStream
-      )
+      await FSPersistor.promises.sendStream(location, files[0], remoteStream)
       expect(fs.createReadStream).to.have.been.calledWith(tempFile)
     })
   })
 
   describe('getFileStream', function() {
     it('should use correct file location', async function() {
-      await FSPersistorManager.promises.getFileStream(location, files[0], {})
+      await FSPersistor.promises.getFileStream(location, files[0], {})
       expect(fs.open).to.have.been.calledWith(
         `${location}/${filteredFilenames[0]}`
       )
     })
 
     it('should pass the options to createReadStream', async function() {
-      await FSPersistorManager.promises.getFileStream(location, files[0], {
+      await FSPersistor.promises.getFileStream(location, files[0], {
         start: 0,
         end: 8
       })
@@ -146,18 +130,14 @@ describe('FSPersistorManagerTests', function() {
       err.code = 'ENOENT'
       fs.open.yields(err)
 
-      await expect(
-        FSPersistorManager.promises.getFileStream(location, files[0], {})
-      )
+      await expect(FSPersistor.promises.getFileStream(location, files[0], {}))
         .to.eventually.be.rejected.and.be.an.instanceOf(Errors.NotFoundError)
         .and.have.property('cause', err)
     })
 
     it('should wrap any other error', async function() {
       fs.open.yields(error)
-      await expect(
-        FSPersistorManager.promises.getFileStream(location, files[0], {})
-      )
+      await expect(FSPersistor.promises.getFileStream(location, files[0], {}))
         .to.eventually.be.rejectedWith('failed to open file for streaming')
         .and.be.an.instanceOf(Errors.ReadError)
         .and.have.property('cause', error)
@@ -181,18 +161,18 @@ describe('FSPersistorManagerTests', function() {
 
     it('should return the file size', async function() {
       expect(
-        await FSPersistorManager.promises.getFileSize(location, files[0])
+        await FSPersistor.promises.getFileSize(location, files[0])
       ).to.equal(size)
     })
 
     it('should throw a NotFoundError if the file does not exist', async function() {
       await expect(
-        FSPersistorManager.promises.getFileSize(location, badFilename)
+        FSPersistor.promises.getFileSize(location, badFilename)
       ).to.eventually.be.rejected.and.be.an.instanceOf(Errors.NotFoundError)
     })
 
     it('should wrap any other error', async function() {
-      await expect(FSPersistorManager.promises.getFileSize(location, 'raccoon'))
+      await expect(FSPersistor.promises.getFileSize(location, 'raccoon'))
         .to.eventually.be.rejected.and.be.an.instanceOf(Errors.ReadError)
         .and.have.property('cause', error)
     })
@@ -200,28 +180,28 @@ describe('FSPersistorManagerTests', function() {
 
   describe('copyFile', function() {
     it('Should open the source for reading', async function() {
-      await FSPersistorManager.promises.copyFile(location, files[0], files[1])
+      await FSPersistor.promises.copyFile(location, files[0], files[1])
       expect(fs.createReadStream).to.have.been.calledWith(
         `${location}/${filteredFilenames[0]}`
       )
     })
 
     it('Should open the target for writing', async function() {
-      await FSPersistorManager.promises.copyFile(location, files[0], files[1])
+      await FSPersistor.promises.copyFile(location, files[0], files[1])
       expect(fs.createWriteStream).to.have.been.calledWith(
         `${location}/${filteredFilenames[1]}`
       )
     })
 
     it('Should pipe the source to the target', async function() {
-      await FSPersistorManager.promises.copyFile(location, files[0], files[1])
+      await FSPersistor.promises.copyFile(location, files[0], files[1])
       expect(stream.pipeline).to.have.been.calledWith(readStream, writeStream)
     })
   })
 
   describe('deleteFile', function() {
     it('Should call unlink with correct options', async function() {
-      await FSPersistorManager.promises.deleteFile(location, files[0])
+      await FSPersistor.promises.deleteFile(location, files[0])
       expect(fs.unlink).to.have.been.calledWith(
         `${location}/${filteredFilenames[0]}`
       )
@@ -230,14 +210,14 @@ describe('FSPersistorManagerTests', function() {
     it('Should propagate the error', async function() {
       fs.unlink.yields(error)
       await expect(
-        FSPersistorManager.promises.deleteFile(location, files[0])
+        FSPersistor.promises.deleteFile(location, files[0])
       ).to.eventually.be.rejected.and.have.property('cause', error)
     })
   })
 
   describe('deleteDirectory', function() {
     it('Should call rmdir(rimraf) with correct options', async function() {
-      await FSPersistorManager.promises.deleteDirectory(location, files[0])
+      await FSPersistor.promises.deleteDirectory(location, files[0])
       expect(rimraf).to.have.been.calledWith(
         `${location}/${filteredFilenames[0]}`
       )
@@ -246,7 +226,7 @@ describe('FSPersistorManagerTests', function() {
     it('Should propagate the error', async function() {
       rimraf.yields(error)
       await expect(
-        FSPersistorManager.promises.deleteDirectory(location, files[0])
+        FSPersistor.promises.deleteDirectory(location, files[0])
       ).to.eventually.be.rejected.and.have.property('cause', error)
     })
   })
@@ -266,7 +246,7 @@ describe('FSPersistorManagerTests', function() {
     })
 
     it('Should call stat with correct options', async function() {
-      await FSPersistorManager.promises.checkIfFileExists(location, files[0])
+      await FSPersistor.promises.checkIfFileExists(location, files[0])
       expect(fs.stat).to.have.been.calledWith(
         `${location}/${filteredFilenames[0]}`
       )
@@ -274,23 +254,18 @@ describe('FSPersistorManagerTests', function() {
 
     it('Should return true for existing files', async function() {
       expect(
-        await FSPersistorManager.promises.checkIfFileExists(location, files[0])
+        await FSPersistor.promises.checkIfFileExists(location, files[0])
       ).to.equal(true)
     })
 
     it('Should return false for non-existing files', async function() {
       expect(
-        await FSPersistorManager.promises.checkIfFileExists(
-          location,
-          badFilename
-        )
+        await FSPersistor.promises.checkIfFileExists(location, badFilename)
       ).to.equal(false)
     })
 
     it('should wrap the error if there is a problem', async function() {
-      await expect(
-        FSPersistorManager.promises.checkIfFileExists(location, 'llama')
-      )
+      await expect(FSPersistor.promises.checkIfFileExists(location, 'llama'))
         .to.eventually.be.rejected.and.be.an.instanceOf(Errors.ReadError)
         .and.have.property('cause', error)
     })
@@ -299,9 +274,7 @@ describe('FSPersistorManagerTests', function() {
   describe('directorySize', function() {
     it('should wrap the error', async function() {
       glob.yields(error)
-      await expect(
-        FSPersistorManager.promises.directorySize(location, files[0])
-      )
+      await expect(FSPersistor.promises.directorySize(location, files[0]))
         .to.eventually.be.rejected.and.be.an.instanceOf(Errors.ReadError)
         .and.include({ cause: error })
         .and.have.property('info')
@@ -309,7 +282,7 @@ describe('FSPersistorManagerTests', function() {
     })
 
     it('should filter the directory name', async function() {
-      await FSPersistorManager.promises.directorySize(location, files[0])
+      await FSPersistor.promises.directorySize(location, files[0])
       expect(glob).to.have.been.calledWith(
         `${location}/${filteredFilenames[0]}_*`
       )
@@ -317,7 +290,7 @@ describe('FSPersistorManagerTests', function() {
 
     it('should sum directory files size', async function() {
       expect(
-        await FSPersistorManager.promises.directorySize(location, files[0])
+        await FSPersistor.promises.directorySize(location, files[0])
       ).to.equal(stat.size * files.length)
     })
   })
diff --git a/services/filestore/test/unit/js/PersistorManagerTests.js b/services/filestore/test/unit/js/PersistorManagerTests.js
index 0ecbb22078..cdc9de0f92 100644
--- a/services/filestore/test/unit/js/PersistorManagerTests.js
+++ b/services/filestore/test/unit/js/PersistorManagerTests.js
@@ -6,18 +6,14 @@ const SandboxedModule = require('sandboxed-module')
 const modulePath = '../../../app/js/PersistorManager.js'
 
 describe('PersistorManager', function() {
-  let PersistorManager,
-    FSPersistorManager,
-    S3PersistorManager,
-    settings,
-    requires
+  let PersistorManager, FSPersistor, S3Persistor, settings, requires
 
   beforeEach(function() {
-    FSPersistorManager = {
-      wrappedMethod: sinon.stub().returns('FSPersistorManager')
+    FSPersistor = {
+      wrappedMethod: sinon.stub().returns('FSPersistor')
     }
-    S3PersistorManager = {
-      wrappedMethod: sinon.stub().returns('S3PersistorManager')
+    S3Persistor = {
+      wrappedMethod: sinon.stub().returns('S3Persistor')
     }
 
     settings = {
@@ -25,8 +21,8 @@ describe('PersistorManager', function() {
     }
 
     requires = {
-      './S3PersistorManager': S3PersistorManager,
-      './FSPersistorManager': FSPersistorManager,
+      './S3Persistor': S3Persistor,
+      './FSPersistor': FSPersistor,
       'settings-sharelatex': settings,
       'logger-sharelatex': {
         log() {},
@@ -40,7 +36,7 @@ describe('PersistorManager', function() {
     PersistorManager = SandboxedModule.require(modulePath, { requires })
 
     expect(PersistorManager).to.respondTo('wrappedMethod')
-    expect(PersistorManager.wrappedMethod()).to.equal('S3PersistorManager')
+    expect(PersistorManager.wrappedMethod()).to.equal('S3Persistor')
   })
 
   it("should implement the S3 wrapped method when 'aws-sdk' is configured", function() {
@@ -48,7 +44,7 @@ describe('PersistorManager', function() {
     PersistorManager = SandboxedModule.require(modulePath, { requires })
 
     expect(PersistorManager).to.respondTo('wrappedMethod')
-    expect(PersistorManager.wrappedMethod()).to.equal('S3PersistorManager')
+    expect(PersistorManager.wrappedMethod()).to.equal('S3Persistor')
   })
 
   it('should implement the FS wrapped method when FS is configured', function() {
@@ -56,7 +52,7 @@ describe('PersistorManager', function() {
     PersistorManager = SandboxedModule.require(modulePath, { requires })
 
     expect(PersistorManager).to.respondTo('wrappedMethod')
-    expect(PersistorManager.wrappedMethod()).to.equal('FSPersistorManager')
+    expect(PersistorManager.wrappedMethod()).to.equal('FSPersistor')
   })
 
   it('should throw an error when the backend is not configured', function() {
diff --git a/services/filestore/test/unit/js/S3PersistorManagerTests.js b/services/filestore/test/unit/js/S3PersistorTests.js
similarity index 87%
rename from services/filestore/test/unit/js/S3PersistorManagerTests.js
rename to services/filestore/test/unit/js/S3PersistorTests.js
index daeac66d3f..7a945b4d19 100644
--- a/services/filestore/test/unit/js/S3PersistorManagerTests.js
+++ b/services/filestore/test/unit/js/S3PersistorTests.js
@@ -1,12 +1,12 @@
 const sinon = require('sinon')
 const chai = require('chai')
 const { expect } = chai
-const modulePath = '../../../app/js/S3PersistorManager.js'
+const modulePath = '../../../app/js/S3Persistor.js'
 const SandboxedModule = require('sandboxed-module')
 
 const Errors = require('../../../app/js/Errors')
 
-describe('S3PersistorManagerTests', function() {
+describe('S3PersistorTests', function() {
   const defaultS3Key = 'frog'
   const defaultS3Secret = 'prince'
   const defaultS3Credentials = {
@@ -33,7 +33,7 @@ describe('S3PersistorManagerTests', function() {
     Meter,
     MeteredStream,
     ReadStream,
-    S3PersistorManager,
+    S3Persistor,
     S3Client,
     S3ReadStream,
     S3NotFoundError,
@@ -115,7 +115,7 @@ describe('S3PersistorManagerTests', function() {
     }
     S3 = sinon.stub().returns(S3Client)
 
-    S3PersistorManager = SandboxedModule.require(modulePath, {
+    S3Persistor = SandboxedModule.require(modulePath, {
       requires: {
         'aws-sdk/clients/s3': S3,
         'settings-sharelatex': settings,
@@ -133,7 +133,7 @@ describe('S3PersistorManagerTests', function() {
       let stream
 
       beforeEach(async function() {
-        stream = await S3PersistorManager.promises.getFileStream(bucket, key)
+        stream = await S3Persistor.promises.getFileStream(bucket, key)
       })
 
       it('returns a stream', function() {
@@ -164,7 +164,7 @@ describe('S3PersistorManagerTests', function() {
       let stream
 
       beforeEach(async function() {
-        stream = await S3PersistorManager.promises.getFileStream(bucket, key, {
+        stream = await S3Persistor.promises.getFileStream(bucket, key, {
           start: 5,
           end: 10
         })
@@ -201,7 +201,7 @@ describe('S3PersistorManagerTests', function() {
           auth_secret: alternativeSecret
         }
 
-        stream = await S3PersistorManager.promises.getFileStream(bucket, key)
+        stream = await S3Persistor.promises.getFileStream(bucket, key)
       })
 
       it('returns a stream', function() {
@@ -220,16 +220,13 @@ describe('S3PersistorManagerTests', function() {
       })
 
       it('caches the credentials', async function() {
-        stream = await S3PersistorManager.promises.getFileStream(bucket, key)
+        stream = await S3Persistor.promises.getFileStream(bucket, key)
 
         expect(S3).to.have.been.calledOnceWith(alternativeS3Credentials)
       })
 
       it('uses the default credentials for an unknown bucket', async function() {
-        stream = await S3PersistorManager.promises.getFileStream(
-          'anotherBucket',
-          key
-        )
+        stream = await S3Persistor.promises.getFileStream('anotherBucket', key)
 
         expect(S3).to.have.been.calledTwice
         expect(S3.firstCall).to.have.been.calledWith(alternativeS3Credentials)
@@ -237,14 +234,8 @@ describe('S3PersistorManagerTests', function() {
       })
 
       it('caches the default credentials', async function() {
-        stream = await S3PersistorManager.promises.getFileStream(
-          'anotherBucket',
-          key
-        )
-        stream = await S3PersistorManager.promises.getFileStream(
-          'anotherBucket',
-          key
-        )
+        stream = await S3Persistor.promises.getFileStream('anotherBucket', key)
+        stream = await S3Persistor.promises.getFileStream('anotherBucket', key)
 
         expect(S3).to.have.been.calledTwice
         expect(S3.firstCall).to.have.been.calledWith(alternativeS3Credentials)
@@ -256,7 +247,7 @@ describe('S3PersistorManagerTests', function() {
         delete settings.filestore.s3.secret
 
         await expect(
-          S3PersistorManager.promises.getFileStream('anotherBucket', key)
+          S3Persistor.promises.getFileStream('anotherBucket', key)
         ).to.eventually.be.rejected.and.be.an.instanceOf(Errors.SettingsError)
       })
     })
@@ -268,7 +259,7 @@ describe('S3PersistorManagerTests', function() {
         S3ReadStream.on = sinon.stub()
         S3ReadStream.on.withArgs('error').yields(S3NotFoundError)
         try {
-          stream = await S3PersistorManager.promises.getFileStream(bucket, key)
+          stream = await S3Persistor.promises.getFileStream(bucket, key)
         } catch (err) {
           error = err
         }
@@ -298,7 +289,7 @@ describe('S3PersistorManagerTests', function() {
         S3ReadStream.on = sinon.stub()
         S3ReadStream.on.withArgs('error').yields(S3AccessDeniedError)
         try {
-          stream = await S3PersistorManager.promises.getFileStream(bucket, key)
+          stream = await S3Persistor.promises.getFileStream(bucket, key)
         } catch (err) {
           error = err
         }
@@ -328,7 +319,7 @@ describe('S3PersistorManagerTests', function() {
         S3ReadStream.on = sinon.stub()
         S3ReadStream.on.withArgs('error').yields(genericError)
         try {
-          stream = await S3PersistorManager.promises.getFileStream(bucket, key)
+          stream = await S3Persistor.promises.getFileStream(bucket, key)
         } catch (err) {
           error = err
         }
@@ -357,7 +348,7 @@ describe('S3PersistorManagerTests', function() {
       let size
 
       beforeEach(async function() {
-        size = await S3PersistorManager.promises.getFileSize(bucket, key)
+        size = await S3Persistor.promises.getFileSize(bucket, key)
       })
 
       it('should return the object size', function() {
@@ -380,7 +371,7 @@ describe('S3PersistorManagerTests', function() {
           promise: sinon.stub().rejects(S3NotFoundError)
         })
         try {
-          await S3PersistorManager.promises.getFileSize(bucket, key)
+          await S3Persistor.promises.getFileSize(bucket, key)
         } catch (err) {
           error = err
         }
@@ -403,7 +394,7 @@ describe('S3PersistorManagerTests', function() {
           promise: sinon.stub().rejects(genericError)
         })
         try {
-          await S3PersistorManager.promises.getFileSize(bucket, key)
+          await S3Persistor.promises.getFileSize(bucket, key)
         } catch (err) {
           error = err
         }
@@ -422,7 +413,7 @@ describe('S3PersistorManagerTests', function() {
   describe('sendStream', function() {
     describe('with valid parameters', function() {
       beforeEach(async function() {
-        return S3PersistorManager.promises.sendStream(bucket, key, ReadStream)
+        return S3Persistor.promises.sendStream(bucket, key, ReadStream)
       })
 
       it('should upload the stream', function() {
@@ -449,7 +440,7 @@ describe('S3PersistorManagerTests', function() {
           promise: sinon.stub().rejects(genericError)
         })
         try {
-          await S3PersistorManager.promises.sendStream(bucket, key, ReadStream)
+          await S3Persistor.promises.sendStream(bucket, key, ReadStream)
         } catch (err) {
           error = err
         }
@@ -464,7 +455,7 @@ describe('S3PersistorManagerTests', function() {
   describe('sendFile', function() {
     describe('with valid parameters', function() {
       beforeEach(async function() {
-        return S3PersistorManager.promises.sendFile(bucket, key, filename)
+        return S3Persistor.promises.sendFile(bucket, key, filename)
       })
 
       it('should create a read stream for the file', function() {
@@ -486,7 +477,7 @@ describe('S3PersistorManagerTests', function() {
       beforeEach(async function() {
         Fs.createReadStream = sinon.stub().throws(FileNotFoundError)
         try {
-          await S3PersistorManager.promises.sendFile(bucket, key, filename)
+          await S3Persistor.promises.sendFile(bucket, key, filename)
         } catch (err) {
           error = err
         }
@@ -507,7 +498,7 @@ describe('S3PersistorManagerTests', function() {
       beforeEach(async function() {
         Fs.createReadStream = sinon.stub().throws(genericError)
         try {
-          await S3PersistorManager.promises.sendFile(bucket, key, filename)
+          await S3Persistor.promises.sendFile(bucket, key, filename)
         } catch (err) {
           error = err
         }
@@ -526,7 +517,7 @@ describe('S3PersistorManagerTests', function() {
   describe('copyFile', function() {
     describe('with valid parameters', function() {
       beforeEach(async function() {
-        return S3PersistorManager.promises.copyFile(bucket, key, destKey)
+        return S3Persistor.promises.copyFile(bucket, key, destKey)
       })
 
       it('should copy the object', function() {
@@ -546,7 +537,7 @@ describe('S3PersistorManagerTests', function() {
           promise: sinon.stub().rejects(S3NotFoundError)
         })
         try {
-          await S3PersistorManager.promises.copyFile(bucket, key, destKey)
+          await S3Persistor.promises.copyFile(bucket, key, destKey)
         } catch (err) {
           error = err
         }
@@ -561,7 +552,7 @@ describe('S3PersistorManagerTests', function() {
   describe('deleteFile', function() {
     describe('with valid parameters', function() {
       beforeEach(async function() {
-        return S3PersistorManager.promises.deleteFile(bucket, key)
+        return S3Persistor.promises.deleteFile(bucket, key)
       })
 
       it('should delete the object', function() {
@@ -580,7 +571,7 @@ describe('S3PersistorManagerTests', function() {
           promise: sinon.stub().rejects(S3NotFoundError)
         })
         try {
-          await S3PersistorManager.promises.deleteFile(bucket, key)
+          await S3Persistor.promises.deleteFile(bucket, key)
         } catch (err) {
           error = err
         }
@@ -595,7 +586,7 @@ describe('S3PersistorManagerTests', function() {
   describe('deleteDirectory', function() {
     describe('with valid parameters', function() {
       beforeEach(async function() {
-        return S3PersistorManager.promises.deleteDirectory(bucket, key)
+        return S3Persistor.promises.deleteDirectory(bucket, key)
       })
 
       it('should list the objects in the directory', function() {
@@ -621,7 +612,7 @@ describe('S3PersistorManagerTests', function() {
         S3Client.listObjects = sinon
           .stub()
           .returns({ promise: sinon.stub().resolves({ Contents: [] }) })
-        return S3PersistorManager.promises.deleteDirectory(bucket, key)
+        return S3Persistor.promises.deleteDirectory(bucket, key)
       })
 
       it('should list the objects in the directory', function() {
@@ -644,7 +635,7 @@ describe('S3PersistorManagerTests', function() {
           .stub()
           .returns({ promise: sinon.stub().rejects(genericError) })
         try {
-          await S3PersistorManager.promises.deleteDirectory(bucket, key)
+          await S3Persistor.promises.deleteDirectory(bucket, key)
         } catch (err) {
           error = err
         }
@@ -671,7 +662,7 @@ describe('S3PersistorManagerTests', function() {
           .stub()
           .returns({ promise: sinon.stub().rejects(genericError) })
         try {
-          await S3PersistorManager.promises.deleteDirectory(bucket, key)
+          await S3Persistor.promises.deleteDirectory(bucket, key)
         } catch (err) {
           error = err
         }
@@ -692,7 +683,7 @@ describe('S3PersistorManagerTests', function() {
       let size
 
       beforeEach(async function() {
-        size = await S3PersistorManager.promises.directorySize(bucket, key)
+        size = await S3Persistor.promises.directorySize(bucket, key)
       })
 
       it('should list the objects in the directory', function() {
@@ -714,7 +705,7 @@ describe('S3PersistorManagerTests', function() {
         S3Client.listObjects = sinon
           .stub()
           .returns({ promise: sinon.stub().resolves({ Contents: [] }) })
-        size = await S3PersistorManager.promises.directorySize(bucket, key)
+        size = await S3Persistor.promises.directorySize(bucket, key)
       })
 
       it('should list the objects in the directory', function() {
@@ -737,7 +728,7 @@ describe('S3PersistorManagerTests', function() {
           .stub()
           .returns({ promise: sinon.stub().rejects(genericError) })
         try {
-          await S3PersistorManager.promises.directorySize(bucket, key)
+          await S3Persistor.promises.directorySize(bucket, key)
         } catch (err) {
           error = err
         }
@@ -758,10 +749,7 @@ describe('S3PersistorManagerTests', function() {
       let exists
 
       beforeEach(async function() {
-        exists = await S3PersistorManager.promises.checkIfFileExists(
-          bucket,
-          key
-        )
+        exists = await S3Persistor.promises.checkIfFileExists(bucket, key)
       })
 
       it('should get the object header', function() {
@@ -783,10 +771,7 @@ describe('S3PersistorManagerTests', function() {
         S3Client.headObject = sinon
           .stub()
           .returns({ promise: sinon.stub().rejects(S3NotFoundError) })
-        exists = await S3PersistorManager.promises.checkIfFileExists(
-          bucket,
-          key
-        )
+        exists = await S3Persistor.promises.checkIfFileExists(bucket, key)
       })
 
       it('should get the object header', function() {
@@ -809,7 +794,7 @@ describe('S3PersistorManagerTests', function() {
           .stub()
           .returns({ promise: sinon.stub().rejects(genericError) })
         try {
-          await S3PersistorManager.promises.checkIfFileExists(bucket, key)
+          await S3Persistor.promises.checkIfFileExists(bucket, key)
         } catch (err) {
           error = err
         }

From 2625e03a31d8bb766dde35aee40aa98f48f61c87 Mon Sep 17 00:00:00 2001
From: Simon Detheridge <s@sd.ai>
Date: Thu, 16 Jan 2020 16:25:12 +0000
Subject: [PATCH 02/12] Add MigrationPersistor for sending 404 requests to a
 fallback persistor

---
 services/filestore/app/js/FSPersistor.js      |   7 +-
 .../filestore/app/js/MigrationPersistor.js    | 113 +++++
 services/filestore/app/js/PersistorManager.js |  35 +-
 services/filestore/app/js/S3Persistor.js      |   7 +-
 .../filestore/config/settings.defaults.coffee |  61 +--
 services/filestore/npm-shrinkwrap.json        |   6 +
 services/filestore/package.json               |   3 +-
 .../test/acceptance/js/FilestoreApp.js        |   1 +
 .../test/acceptance/js/FilestoreTests.js      | 405 ++++++++++++++-
 .../test/unit/js/MigrationPersistorTests.js   | 463 ++++++++++++++++++
 10 files changed, 1038 insertions(+), 63 deletions(-)
 create mode 100644 services/filestore/app/js/MigrationPersistor.js
 create mode 100644 services/filestore/test/unit/js/MigrationPersistorTests.js

diff --git a/services/filestore/app/js/FSPersistor.js b/services/filestore/app/js/FSPersistor.js
index 862acb9bcb..2ba65f06d2 100644
--- a/services/filestore/app/js/FSPersistor.js
+++ b/services/filestore/app/js/FSPersistor.js
@@ -103,12 +103,17 @@ async function deleteFile(location, name) {
   try {
     await fsUnlink(`${location}/${filteredName}`)
   } catch (err) {
-    throw _wrapError(
+    const wrappedError = _wrapError(
       err,
       'failed to delete file',
       { location, filteredName },
       WriteError
     )
+    if (!(wrappedError instanceof NotFoundError)) {
+      // S3 doesn't give us a 404 when a file wasn't there to be deleted, so we
+      // should be consistent here as well
+      throw wrappedError
+    }
   }
 }
 
diff --git a/services/filestore/app/js/MigrationPersistor.js b/services/filestore/app/js/MigrationPersistor.js
new file mode 100644
index 0000000000..9f7a834f31
--- /dev/null
+++ b/services/filestore/app/js/MigrationPersistor.js
@@ -0,0 +1,113 @@
+const metrics = require('metrics-sharelatex')
+const Settings = require('settings-sharelatex')
+const logger = require('logger-sharelatex')
+const { callbackify } = require('util')
+const { NotFoundError } = require('./Errors')
+
+// Persistor that wraps two other persistors. Talks to the 'primary' by default,
+// but will fall back to an older persistor in the case of a not-found error.
+// If `Settings.filestore.fallback.copyOnMiss` is set, this will copy files from the fallback
+// to the primary, in the event that they are missing.
+//
+// It is unlikely that the bucket/location name will be the same on the fallback
+// as the primary. The bucket names should be overridden in `Settings.filestore.fallback.buckets`
+// e.g.
+// Settings.filestore.fallback.buckets = {
+//   myBucketOnS3: 'myBucketOnGCS'
+// }s
+
+module.exports = function(primary, fallback) {
+  function _wrapMethodOnBothPersistors(method) {
+    return async function(bucket, key, ...moreArgs) {
+      const fallbackBucket = _getFallbackBucket(bucket)
+
+      await Promise.all([
+        primary.promises[method](bucket, key, ...moreArgs),
+        fallback.promises[method](fallbackBucket, key, ...moreArgs)
+      ])
+    }
+  }
+
+  async function copyFileWithFallback(bucket, sourceKey, destKey) {
+    try {
+      return await primary.promises.copyFile(bucket, sourceKey, destKey)
+    } catch (err) {
+      if (err instanceof NotFoundError) {
+        const fallbackBucket = _getFallbackBucket(bucket)
+        return _copyFileFromFallback(fallbackBucket, bucket, sourceKey, destKey)
+      }
+    }
+  }
+
+  function _getFallbackBucket(bucket) {
+    return (
+      Settings.filestore.fallback.buckets &&
+      Settings.filestore.fallback.buckets[bucket]
+    )
+  }
+
+  function _wrapFallbackMethod(method, enableCopy = true) {
+    return async function(bucket, key, ...moreArgs) {
+      try {
+        return await primary.promises[method](bucket, key, ...moreArgs)
+      } catch (err) {
+        if (err instanceof NotFoundError) {
+          const fallbackBucket = _getFallbackBucket(bucket)
+          if (Settings.filestore.fallback.copyOnMiss && enableCopy) {
+            // run in background
+            _copyFileFromFallback(fallbackBucket, bucket, key, key).catch(
+              err => {
+                logger.warn({ err }, 'failed to copy file from fallback')
+              }
+            )
+          }
+          return fallback.promises[method](fallbackBucket, key, ...moreArgs)
+        }
+        throw err
+      }
+    }
+  }
+
+  async function _copyFileFromFallback(
+    sourceBucket,
+    destBucket,
+    sourceKey,
+    destKey
+  ) {
+    const sourceStream = await fallback.promises.getFileStream(
+      sourceBucket,
+      sourceKey,
+      {}
+    )
+
+    await primary.promises.sendStream(destBucket, destKey, sourceStream)
+    metrics.inc('fallback.copy')
+  }
+
+  return {
+    primaryPersistor: primary,
+    fallbackPersistor: fallback,
+    sendFile: primary.sendFile,
+    sendStream: primary.sendStream,
+    getFileStream: callbackify(_wrapFallbackMethod('getFileStream')),
+    deleteDirectory: callbackify(
+      _wrapMethodOnBothPersistors('deleteDirectory')
+    ),
+    getFileSize: callbackify(_wrapFallbackMethod('getFileSize')),
+    deleteFile: callbackify(_wrapMethodOnBothPersistors('deleteFile')),
+    copyFile: callbackify(copyFileWithFallback),
+    checkIfFileExists: callbackify(_wrapFallbackMethod('checkIfFileExists')),
+    directorySize: callbackify(_wrapFallbackMethod('directorySize', false)),
+    promises: {
+      sendFile: primary.promises.sendFile,
+      sendStream: primary.promises.sendStream,
+      getFileStream: _wrapFallbackMethod('getFileStream'),
+      deleteDirectory: _wrapMethodOnBothPersistors('deleteDirectory'),
+      getFileSize: _wrapFallbackMethod('getFileSize'),
+      deleteFile: _wrapMethodOnBothPersistors('deleteFile'),
+      copyFile: copyFileWithFallback,
+      checkIfFileExists: _wrapFallbackMethod('checkIfFileExists'),
+      directorySize: _wrapFallbackMethod('directorySize', false)
+    }
+  }
+}
diff --git a/services/filestore/app/js/PersistorManager.js b/services/filestore/app/js/PersistorManager.js
index bd944a03f7..32f6cd41f8 100644
--- a/services/filestore/app/js/PersistorManager.js
+++ b/services/filestore/app/js/PersistorManager.js
@@ -3,7 +3,8 @@ const logger = require('logger-sharelatex')
 
 logger.log(
   {
-    backend: settings.filestore.backend
+    backend: settings.filestore.backend,
+    fallback: settings.filestore.fallback && settings.filestore.fallback.backend
   },
   'Loading backend'
 )
@@ -11,14 +12,26 @@ if (!settings.filestore.backend) {
   throw new Error('no backend specified - config incomplete')
 }
 
-switch (settings.filestore.backend) {
-  case 'aws-sdk':
-  case 's3':
-    module.exports = require('./S3Persistor')
-    break
-  case 'fs':
-    module.exports = require('./FSPersistor')
-    break
-  default:
-    throw new Error(`unknown filestore backend: ${settings.filestore.backend}`)
+function getPersistor(backend) {
+  switch (backend) {
+    case 'aws-sdk':
+    case 's3':
+      return require('./S3Persistor')
+    case 'fs':
+      return require('./FSPersistor')
+    default:
+      throw new Error(`unknown filestore backend: ${backend}`)
+  }
 }
+
+let persistor = getPersistor(settings.filestore.backend)
+
+if (settings.filestore.fallback && settings.filestore.fallback.backend) {
+  const migrationPersistor = require('./MigrationPersistor')
+  persistor = migrationPersistor(
+    persistor,
+    getPersistor(settings.filestore.fallback.backend)
+  )
+}
+
+module.exports = persistor
diff --git a/services/filestore/app/js/S3Persistor.js b/services/filestore/app/js/S3Persistor.js
index 52cadfbfbd..6d22823401 100644
--- a/services/filestore/app/js/S3Persistor.js
+++ b/services/filestore/app/js/S3Persistor.js
@@ -173,6 +173,7 @@ async function deleteFile(bucketName, key) {
       .deleteObject({ Bucket: bucketName, Key: key })
       .promise()
   } catch (err) {
+    // s3 does not give us a NotFoundError here
     throw _wrapError(
       err,
       'failed to delete file in S3',
@@ -232,8 +233,12 @@ async function directorySize(bucketName, key) {
 }
 
 function _wrapError(error, message, params, ErrorType) {
+  // the AWS client can return one of 'NoSuchKey', 'NotFound' or 404 (integer)
+  // when something is not found, depending on the endpoint
   if (
-    ['NoSuchKey', 'NotFound', 'AccessDenied', 'ENOENT'].includes(error.code)
+    ['NoSuchKey', 'NotFound', 404, 'AccessDenied', 'ENOENT'].includes(
+      error.code
+    )
   ) {
     return new NotFoundError({
       message: 'no such file',
diff --git a/services/filestore/config/settings.defaults.coffee b/services/filestore/config/settings.defaults.coffee
index 206f932a76..a4a2df2d24 100644
--- a/services/filestore/config/settings.defaults.coffee
+++ b/services/filestore/config/settings.defaults.coffee
@@ -7,6 +7,19 @@ if process.env['AWS_KEY'] && !process.env['AWS_ACCESS_KEY_ID']
 if process.env['AWS_SECRET'] && !process.env['AWS_SECRET_ACCESS_KEY']
 	process.env['AWS_SECRET_ACCESS_KEY'] = process.env['AWS_SECRET']
 
+# pre-backend setting, fall back to old behaviour
+unless process.env['BACKEND']?
+	if process.env['AWS_ACCESS_KEY_ID']? or process.env['S3_BUCKET_CREDENTIALS']?
+		process.env['BACKEND'] = "s3"
+		process.env['USER_FILES_BUCKET_NAME'] = process.env['AWS_S3_USER_FILES_BUCKET_NAME']
+		process.env['TEMPLATE_FILES_BUCKET_NAME'] = process.env['AWS_S3_TEMPLATE_FILES_BUCKET_NAME']
+		process.env['PUBLIC_FILES_BUCKET_NAME'] = process.env['AWS_S3_PUBLIC_FILES_BUCKET_NAME']
+	else
+		process.env['BACKEND'] = "fs"
+		process.env['USER_FILES_BUCKET_NAME'] = Path.resolve(__dirname + "/../user_files")
+		process.env['TEMPLATE_FILES_BUCKET_NAME'] = Path.resolve(__dirname + "/../public_files")
+		process.env['PUBLIC_FILES_BUCKET_NAME'] = Path.resolve(__dirname + "/../template_files")
+
 settings =
 	internal:
 		filestore:
@@ -18,38 +31,28 @@ settings =
 		# Choices are
 		# s3 - Amazon S3
 		# fs - local filesystem
-		if process.env['AWS_ACCESS_KEY_ID']? or process.env['S3_BUCKET_CREDENTIALS']?
-			backend: "s3"
-			s3:
+		backend: process.env['BACKEND']
+
+		s3:
+			if process.env['AWS_ACCESS_KEY_ID']? or process.env['S3_BUCKET_CREDENTIALS']?
 				key: process.env['AWS_ACCESS_KEY_ID']
 				secret: process.env['AWS_SECRET_ACCESS_KEY']
 				endpoint: process.env['AWS_S3_ENDPOINT']
-			stores:
-				user_files: process.env['AWS_S3_USER_FILES_BUCKET_NAME']
-				template_files: process.env['AWS_S3_TEMPLATE_FILES_BUCKET_NAME']
-				public_files: process.env['AWS_S3_PUBLIC_FILES_BUCKET_NAME']
-			# if you are using S3, then fill in your S3 details below,
-			# or use env var with the same structure.
-			# s3:
-			# 	key: ""     # default
-			# 	secret: ""  # default
-			#
-			# s3BucketCreds:
-			#   bucketname1: # secrets for bucketname1
-			#     auth_key: ""
-			#     auth_secret: ""
-			#  bucketname2: # secrets for bucketname2...
-			s3BucketCreds: JSON.parse process.env['S3_BUCKET_CREDENTIALS'] if process.env['S3_BUCKET_CREDENTIALS']?
-		else
-			backend: "fs"
-			stores:
-				#
-				# For local filesystem this is the directory to store the files in.
-				# Must contain full path, e.g. "/var/lib/sharelatex/data".
-				# This path must exist, not be tmpfs and be writable to by the user sharelatex is run as.
-				user_files: Path.resolve(__dirname + "/../user_files")
-				public_files: Path.resolve(__dirname + "/../public_files")
-				template_files: Path.resolve(__dirname + "/../template_files")
+
+		stores:
+			user_files: process.env['USER_FILES_BUCKET_NAME']
+			template_files: process.env['TEMPLATE_FILES_BUCKET_NAME']
+			public_files: process.env['PUBLIC_FILES_BUCKET_NAME']
+
+		s3BucketCreds: JSON.parse process.env['S3_BUCKET_CREDENTIALS'] if process.env['S3_BUCKET_CREDENTIALS']?
+
+		fallback:
+			if process.env['FALLBACK_BACKEND']?
+				backend: process.env['FALLBACK_BACKEND']
+				# mapping of bucket names on the fallback, to bucket names on the primary.
+				# e.g. { myS3UserFilesBucketName: 'myGoogleUserFilesBucketName' }
+				buckets: JSON.parse process.env['FALLBACK_BUCKET_MAPPING'] if process.env['FALLBACK_BUCKET_MAPPING']?
+				copyOnMiss: if process.env['COPY_ON_MISS'] == 'true' then true else false
 
 	path:
 		uploadFolder: Path.resolve(__dirname + "/../uploads")
diff --git a/services/filestore/npm-shrinkwrap.json b/services/filestore/npm-shrinkwrap.json
index 8d78271caa..b343d6ad2c 100644
--- a/services/filestore/npm-shrinkwrap.json
+++ b/services/filestore/npm-shrinkwrap.json
@@ -5055,6 +5055,12 @@
       "resolved": "https://registry.npmjs.org/stream-shift/-/stream-shift-1.0.0.tgz",
       "integrity": "sha1-1cdSgl5TZ+eG944Y5EXqIjoVWVI="
     },
+    "streamifier": {
+      "version": "0.1.1",
+      "resolved": "https://registry.npmjs.org/streamifier/-/streamifier-0.1.1.tgz",
+      "integrity": "sha1-l+mNj6TRBdYqJpHR3AfoINuN/E8=",
+      "dev": true
+    },
     "string-width": {
       "version": "2.1.1",
       "resolved": "https://registry.npmjs.org/string-width/-/string-width-2.1.1.tgz",
diff --git a/services/filestore/package.json b/services/filestore/package.json
index 14e35cd8a2..303393bd56 100644
--- a/services/filestore/package.json
+++ b/services/filestore/package.json
@@ -68,6 +68,7 @@
     "prettier-eslint-cli": "^5.0.0",
     "sandboxed-module": "2.0.3",
     "sinon": "7.1.1",
-    "sinon-chai": "^3.3.0"
+    "sinon-chai": "^3.3.0",
+    "streamifier": "^0.1.1"
   }
 }
diff --git a/services/filestore/test/acceptance/js/FilestoreApp.js b/services/filestore/test/acceptance/js/FilestoreApp.js
index 718d53bcf8..20564e2d40 100644
--- a/services/filestore/test/acceptance/js/FilestoreApp.js
+++ b/services/filestore/test/acceptance/js/FilestoreApp.js
@@ -56,6 +56,7 @@ class FilestoreApp {
     }
 
     this.initing = false
+    this.persistor = require('../../../app/js/PersistorManager')
   }
 
   async waitForInit() {
diff --git a/services/filestore/test/acceptance/js/FilestoreTests.js b/services/filestore/test/acceptance/js/FilestoreTests.js
index d7dfbce57c..5a0de3abd8 100644
--- a/services/filestore/test/acceptance/js/FilestoreTests.js
+++ b/services/filestore/test/acceptance/js/FilestoreTests.js
@@ -11,6 +11,7 @@ const S3 = require('aws-sdk/clients/s3')
 const Stream = require('stream')
 const request = require('request')
 const { promisify } = require('util')
+const streamifier = require('streamifier')
 chai.use(require('chai-as-promised'))
 
 const fsWriteFile = promisify(fs.writeFile)
@@ -25,6 +26,19 @@ async function getMetric(filestoreUrl, metric) {
   return parseInt(found ? found[1] : 0) || 0
 }
 
+if (!process.env.AWS_ACCESS_KEY_ID) {
+  throw new Error('please provide credentials for the AWS S3 test server')
+}
+
+function streamToString(stream) {
+  const chunks = []
+  return new Promise((resolve, reject) => {
+    stream.on('data', chunk => chunks.push(chunk))
+    stream.on('error', reject)
+    stream.on('end', () => resolve(Buffer.concat(chunks).toString('utf8')))
+  })
+}
+
 // store settings for multiple backends, so that we can test each one.
 // fs will always be available - add others if they are configured
 const BackendSettings = {
@@ -35,11 +49,8 @@ const BackendSettings = {
       public_files: Path.resolve(__dirname, '../../../public_files'),
       template_files: Path.resolve(__dirname, '../../../template_files')
     }
-  }
-}
-
-if (process.env.AWS_ACCESS_KEY_ID) {
-  BackendSettings.S3Persistor = {
+  },
+  S3Persistor: {
     backend: 's3',
     s3: {
       key: process.env.AWS_ACCESS_KEY_ID,
@@ -52,6 +63,62 @@ if (process.env.AWS_ACCESS_KEY_ID) {
       template_files: process.env.AWS_S3_TEMPLATE_FILES_BUCKET_NAME,
       public_files: process.env.AWS_S3_PUBLIC_FILES_BUCKET_NAME
     }
+  },
+  FallbackS3ToFSPersistor: {
+    backend: 's3',
+    s3: {
+      key: process.env.AWS_ACCESS_KEY_ID,
+      secret: process.env.AWS_SECRET_ACCESS_KEY,
+      endpoint: process.env.AWS_S3_ENDPOINT,
+      pathStyle: true
+    },
+    stores: {
+      user_files: process.env.AWS_S3_USER_FILES_BUCKET_NAME,
+      template_files: process.env.AWS_S3_TEMPLATE_FILES_BUCKET_NAME,
+      public_files: process.env.AWS_S3_PUBLIC_FILES_BUCKET_NAME
+    },
+    fallback: {
+      backend: 'fs',
+      buckets: {
+        [process.env.AWS_S3_USER_FILES_BUCKET_NAME]: Path.resolve(
+          __dirname,
+          '../../../user_files'
+        ),
+        [process.env.AWS_S3_TEMPLATE_FILES_BUCKET_NAME]: Path.resolve(
+          __dirname,
+          '../../../public_files'
+        ),
+        [process.env.AWS_S3_PUBLIC_FILES_BUCKET_NAME]: Path.resolve(
+          __dirname,
+          '../../../template_files'
+        )
+      }
+    }
+  },
+  FallbackFSToS3Persistor: {
+    backend: 'fs',
+    s3: {
+      key: process.env.AWS_ACCESS_KEY_ID,
+      secret: process.env.AWS_SECRET_ACCESS_KEY,
+      endpoint: process.env.AWS_S3_ENDPOINT,
+      pathStyle: true
+    },
+    stores: {
+      user_files: Path.resolve(__dirname, '../../../user_files'),
+      public_files: Path.resolve(__dirname, '../../../public_files'),
+      template_files: Path.resolve(__dirname, '../../../template_files')
+    },
+    fallback: {
+      backend: 's3',
+      buckets: {
+        [Path.resolve(__dirname, '../../../user_files')]: process.env
+          .AWS_S3_USER_FILES_BUCKET_NAME,
+        [Path.resolve(__dirname, '../../../public_files')]: process.env
+          .AWS_S3_TEMPLATE_FILES_BUCKET_NAME,
+        [Path.resolve(__dirname, '../../../template_files')]: process.env
+          .AWS_S3_PUBLIC_FILES_BUCKET_NAME
+      }
+    }
   }
 }
 
@@ -100,23 +167,21 @@ describe('Filestore', function() {
       })
 
       describe('with a file on the server', function() {
-        let fileId, fileUrl
+        let fileId, fileUrl, constantFileContent
 
         const localFileReadPath =
           '/tmp/filestore_acceptance_tests_file_read.txt'
-        const constantFileContent = [
-          'hello world',
-          `line 2 goes here ${Math.random()}`,
-          'there are 3 lines in all'
-        ].join('\n')
-
-        before(async function() {
-          await fsWriteFile(localFileReadPath, constantFileContent)
-        })
 
         beforeEach(async function() {
           fileId = Math.random()
           fileUrl = `${filestoreUrl}/project/acceptance_tests/file/${directoryName}%2F${fileId}`
+          constantFileContent = [
+            'hello world',
+            `line 2 goes here ${Math.random()}`,
+            'there are 3 lines in all'
+          ].join('\n')
+
+          await fsWriteFile(localFileReadPath, constantFileContent)
 
           const writeStream = request.post(fileUrl)
           const readStream = fs.createReadStream(localFileReadPath)
@@ -177,7 +242,7 @@ describe('Filestore', function() {
         })
 
         it('should be able to copy files', async function() {
-          const newProjectID = 'acceptance_tests_copyied_project'
+          const newProjectID = 'acceptance_tests_copied_project'
           const newFileId = Math.random()
           const newFileUrl = `${filestoreUrl}/project/${newProjectID}/file/${directoryName}%2F${newFileId}`
           const opts = {
@@ -198,6 +263,18 @@ describe('Filestore', function() {
           expect(response.body).to.equal(constantFileContent)
         })
 
+        it('should be able to overwrite the file', async function() {
+          const newContent = `here is some different content, ${Math.random()}`
+          const writeStream = request.post(fileUrl)
+          const readStream = streamifier.createReadStream(newContent)
+          // hack to consume the result to ensure the http request has been fully processed
+          const resultStream = fs.createWriteStream('/dev/null')
+          await pipeline(readStream, writeStream, resultStream)
+
+          const response = await rp.get(fileUrl)
+          expect(response.body).to.equal(newContent)
+        })
+
         if (backend === 'S3Persistor') {
           it('should record an egress metric for the upload', async function() {
             const metric = await getMetric(filestoreUrl, 's3_egress')
@@ -292,10 +369,10 @@ describe('Filestore', function() {
 
       if (backend === 'S3Persistor') {
         describe('with a file in a specific bucket', function() {
-          let constantFileContents, fileId, fileUrl, bucketName
+          let constantFileContent, fileId, fileUrl, bucketName
 
           beforeEach(async function() {
-            constantFileContents = `This is a file in a different S3 bucket ${Math.random()}`
+            constantFileContent = `This is a file in a different S3 bucket ${Math.random()}`
             fileId = Math.random().toString()
             bucketName = Math.random().toString()
             fileUrl = `${filestoreUrl}/bucket/${bucketName}/key/${fileId}`
@@ -320,14 +397,302 @@ describe('Filestore', function() {
               .upload({
                 Bucket: bucketName,
                 Key: fileId,
-                Body: constantFileContents
+                Body: constantFileContent
               })
               .promise()
           })
 
           it('should get the file from the specified bucket', async function() {
             const response = await rp.get(fileUrl)
-            expect(response.body).to.equal(constantFileContents)
+            expect(response.body).to.equal(constantFileContent)
+          })
+        })
+      }
+
+      if (BackendSettings[backend].fallback) {
+        describe('with a fallback', function() {
+          async function uploadStringToPersistor(
+            persistor,
+            bucket,
+            key,
+            content
+          ) {
+            const fileStream = streamifier.createReadStream(content)
+            await persistor.promises.sendStream(bucket, key, fileStream)
+          }
+
+          async function getStringFromPersistor(persistor, bucket, key) {
+            const stream = await persistor.promises.getFileStream(
+              bucket,
+              key,
+              {}
+            )
+            return streamToString(stream)
+          }
+
+          async function expectPersistorToHaveFile(
+            persistor,
+            bucket,
+            key,
+            content
+          ) {
+            const foundContent = await getStringFromPersistor(
+              persistor,
+              bucket,
+              key
+            )
+            expect(foundContent).to.equal(content)
+          }
+
+          async function expectPersistorNotToHaveFile(persistor, bucket, key) {
+            await expect(
+              getStringFromPersistor(persistor, bucket, key)
+            ).to.eventually.have.been.rejected.with.property(
+              'name',
+              'NotFoundError'
+            )
+          }
+
+          let constantFileContent,
+            fileId,
+            fileKey,
+            fileUrl,
+            bucket,
+            fallbackBucket
+          const projectId = 'acceptance_tests'
+
+          beforeEach(function() {
+            constantFileContent = `This is yet more file content ${Math.random()}`
+            fileId = Math.random().toString()
+            fileKey = `${projectId}/${directoryName}/${fileId}`
+            fileUrl = `${filestoreUrl}/project/${projectId}/file/${directoryName}%2F${fileId}`
+
+            bucket = Settings.filestore.stores.user_files
+            fallbackBucket = Settings.filestore.fallback.buckets[bucket]
+          })
+
+          describe('with a file in the fallback bucket', function() {
+            beforeEach(async function() {
+              await uploadStringToPersistor(
+                app.persistor.fallbackPersistor,
+                fallbackBucket,
+                fileKey,
+                constantFileContent
+              )
+            })
+
+            it('should not find file in the primary', async function() {
+              await expectPersistorNotToHaveFile(
+                app.persistor.primaryPersistor,
+                bucket,
+                fileKey
+              )
+            })
+
+            it('should find the file in the fallback', async function() {
+              await expectPersistorToHaveFile(
+                app.persistor.fallbackPersistor,
+                fallbackBucket,
+                fileKey,
+                constantFileContent
+              )
+            })
+
+            it('should fetch the file', async function() {
+              const res = await rp.get(fileUrl)
+              expect(res.body).to.equal(constantFileContent)
+            })
+
+            it('should not copy the file to the primary', async function() {
+              await rp.get(fileUrl)
+
+              await expectPersistorNotToHaveFile(
+                app.persistor.primaryPersistor,
+                bucket,
+                fileKey
+              )
+            })
+
+            describe('when copyOnMiss is enabled', function() {
+              beforeEach(function() {
+                Settings.filestore.fallback.copyOnMiss = true
+              })
+
+              it('copies the file to the primary', async function() {
+                await rp.get(fileUrl)
+                // wait for the file to copy in the background
+                await promisify(setTimeout)(1000)
+
+                await expectPersistorToHaveFile(
+                  app.persistor.primaryPersistor,
+                  bucket,
+                  fileKey,
+                  constantFileContent
+                )
+              })
+            })
+
+            describe('when copying a file', function() {
+              let newFileId, newFileUrl, newFileKey
+              const newProjectID = 'acceptance_tests_copied_project'
+
+              beforeEach(async function() {
+                newFileId = Math.random()
+                newFileUrl = `${filestoreUrl}/project/${newProjectID}/file/${directoryName}%2F${newFileId}`
+                newFileKey = `${newProjectID}/${directoryName}/${newFileId}`
+
+                const opts = {
+                  method: 'put',
+                  uri: newFileUrl,
+                  json: {
+                    source: {
+                      project_id: 'acceptance_tests',
+                      file_id: `${directoryName}/${fileId}`
+                    }
+                  }
+                }
+
+                const response = await rp(opts)
+                expect(response.statusCode).to.equal(200)
+              })
+
+              it('should leave the old file in the old bucket', async function() {
+                await expectPersistorToHaveFile(
+                  app.persistor.fallbackPersistor,
+                  fallbackBucket,
+                  fileKey,
+                  constantFileContent
+                )
+              })
+
+              it('should not create a new file in the old bucket', async function() {
+                await expectPersistorNotToHaveFile(
+                  app.persistor.fallbackPersistor,
+                  fallbackBucket,
+                  newFileKey
+                )
+              })
+
+              it('should not copy the old file to the new bucket', async function() {
+                await expectPersistorNotToHaveFile(
+                  app.persistor.primaryPersistor,
+                  bucket,
+                  fileKey
+                )
+              })
+
+              it('should create a new file in the new bucket', async function() {
+                await expectPersistorToHaveFile(
+                  app.persistor.primaryPersistor,
+                  bucket,
+                  newFileKey,
+                  constantFileContent
+                )
+              })
+            })
+          })
+
+          describe('when sending a file', function() {
+            beforeEach(async function() {
+              const writeStream = request.post(fileUrl)
+              const readStream = streamifier.createReadStream(
+                constantFileContent
+              )
+              // hack to consume the result to ensure the http request has been fully processed
+              const resultStream = fs.createWriteStream('/dev/null')
+              await pipeline(readStream, writeStream, resultStream)
+            })
+
+            it('should store the file on the primary', async function() {
+              await expectPersistorToHaveFile(
+                app.persistor.primaryPersistor,
+                bucket,
+                fileKey,
+                constantFileContent
+              )
+            })
+
+            it('should not store the file on the fallback', async function() {
+              await expectPersistorNotToHaveFile(
+                app.persistor.fallbackPersistor,
+                fallbackBucket,
+                `acceptance_tests/${directoryName}/${fileId}`
+              )
+            })
+          })
+
+          describe('when deleting a file', function() {
+            describe('when the file exists on the primary', function() {
+              beforeEach(async function() {
+                await uploadStringToPersistor(
+                  app.persistor.primaryPersistor,
+                  bucket,
+                  fileKey,
+                  constantFileContent
+                )
+              })
+
+              it('should delete the file', async function() {
+                const response = await rp.del(fileUrl)
+                expect(response.statusCode).to.equal(204)
+                await expect(
+                  rp.get(fileUrl)
+                ).to.eventually.be.rejected.and.have.property('statusCode', 404)
+              })
+            })
+
+            describe('when the file exists on the fallback', function() {
+              beforeEach(async function() {
+                await uploadStringToPersistor(
+                  app.persistor.fallbackPersistor,
+                  fallbackBucket,
+                  fileKey,
+                  constantFileContent
+                )
+              })
+
+              it('should delete the file', async function() {
+                const response = await rp.del(fileUrl)
+                expect(response.statusCode).to.equal(204)
+                await expect(
+                  rp.get(fileUrl)
+                ).to.eventually.be.rejected.and.have.property('statusCode', 404)
+              })
+            })
+
+            describe('when the file exists on both the primary and the fallback', function() {
+              beforeEach(async function() {
+                await uploadStringToPersistor(
+                  app.persistor.primaryPersistor,
+                  bucket,
+                  fileKey,
+                  constantFileContent
+                )
+                await uploadStringToPersistor(
+                  app.persistor.fallbackPersistor,
+                  fallbackBucket,
+                  fileKey,
+                  constantFileContent
+                )
+              })
+
+              it('should delete the files', async function() {
+                const response = await rp.del(fileUrl)
+                expect(response.statusCode).to.equal(204)
+                await expect(
+                  rp.get(fileUrl)
+                ).to.eventually.be.rejected.and.have.property('statusCode', 404)
+              })
+            })
+
+            describe('when the file does not exist', function() {
+              it('should return return 204', async function() {
+                // S3 doesn't give us a 404 when the object doesn't exist, so to stay
+                // consistent we merrily return 204 ourselves here as well
+                const response = await rp.del(fileUrl)
+                expect(response.statusCode).to.equal(204)
+              })
+            })
           })
         })
       }
diff --git a/services/filestore/test/unit/js/MigrationPersistorTests.js b/services/filestore/test/unit/js/MigrationPersistorTests.js
new file mode 100644
index 0000000000..1cc8324d46
--- /dev/null
+++ b/services/filestore/test/unit/js/MigrationPersistorTests.js
@@ -0,0 +1,463 @@
+const sinon = require('sinon')
+const chai = require('chai')
+const { expect } = chai
+const modulePath = '../../../app/js/MigrationPersistor.js'
+const SandboxedModule = require('sandboxed-module')
+
+const Errors = require('../../../app/js/Errors')
+
+// Not all methods are tested here, but a method with each type of wrapping has
+// tests. Specifically, the following wrapping methods are tested here:
+// getFileStream: _wrapFallbackMethod
+// sendStream: forward-to-primary
+// deleteFile: _wrapMethodOnBothPersistors
+// copyFile: copyFileWithFallback
+
+describe('MigrationPersistorTests', function() {
+  const bucket = 'womBucket'
+  const fallbackBucket = 'bucKangaroo'
+  const key = 'monKey'
+  const destKey = 'donKey'
+  const genericError = new Error('guru meditation error')
+  const notFoundError = new Errors.NotFoundError('not found')
+  const size = 33
+  const fileStream = 'fileStream'
+
+  function newPersistor(hasFile) {
+    return {
+      promises: {
+        sendFile: sinon.stub().resolves(),
+        sendStream: sinon.stub().resolves(),
+        getFileStream: hasFile
+          ? sinon.stub().resolves(fileStream)
+          : sinon.stub().rejects(notFoundError),
+        deleteDirectory: sinon.stub().resolves(),
+        getFileSize: hasFile
+          ? sinon.stub().resolves(size)
+          : sinon.stub().rejects(notFoundError),
+        deleteFile: sinon.stub().resolves(),
+        copyFile: hasFile
+          ? sinon.stub().resolves()
+          : sinon.stub().rejects(notFoundError),
+        checkIfFileExists: sinon.stub().resolves(hasFile),
+        directorySize: hasFile
+          ? sinon.stub().resolves(size)
+          : sinon.stub().rejects(notFoundError)
+      }
+    }
+  }
+
+  let Metrics, Settings, Logger, MigrationPersistor
+
+  beforeEach(function() {
+    Settings = {
+      filestore: {
+        fallback: {
+          buckets: {
+            [bucket]: fallbackBucket
+          }
+        }
+      }
+    }
+
+    Metrics = {
+      inc: sinon.stub()
+    }
+
+    Logger = {
+      warn: sinon.stub()
+    }
+
+    MigrationPersistor = SandboxedModule.require(modulePath, {
+      requires: {
+        'settings-sharelatex': Settings,
+        './Errors': Errors,
+        'metrics-sharelatex': Metrics,
+        'logger-sharelatex': Logger
+      },
+      globals: { console }
+    })
+  })
+
+  describe('getFileStream', function() {
+    const options = { wombat: 'potato' }
+    describe('when the primary persistor has the file', function() {
+      let primaryPersistor, fallbackPersistor, migrationPersistor, response
+      beforeEach(async function() {
+        primaryPersistor = newPersistor(true)
+        fallbackPersistor = newPersistor(false)
+        migrationPersistor = MigrationPersistor(
+          primaryPersistor,
+          fallbackPersistor
+        )
+        response = await migrationPersistor.promises.getFileStream(
+          bucket,
+          key,
+          options
+        )
+      })
+
+      it('should return the file stream', function() {
+        expect(response).to.equal(fileStream)
+      })
+
+      it('should fetch the file from the primary persistor, with the correct options', function() {
+        expect(
+          primaryPersistor.promises.getFileStream
+        ).to.have.been.calledWithExactly(bucket, key, options)
+      })
+
+      it('should not query the fallback persistor', function() {
+        expect(fallbackPersistor.promises.getFileStream).not.to.have.been.called
+      })
+    })
+
+    describe('when the fallback persistor has the file', function() {
+      let primaryPersistor, fallbackPersistor, migrationPersistor, response
+      beforeEach(async function() {
+        primaryPersistor = newPersistor(false)
+        fallbackPersistor = newPersistor(true)
+        migrationPersistor = MigrationPersistor(
+          primaryPersistor,
+          fallbackPersistor
+        )
+        response = await migrationPersistor.promises.getFileStream(
+          bucket,
+          key,
+          options
+        )
+      })
+
+      it('should return the file stream', function() {
+        expect(response).to.equal(fileStream)
+      })
+
+      it('should fetch the file from the primary persistor with the correct options', function() {
+        expect(
+          primaryPersistor.promises.getFileStream
+        ).to.have.been.calledWithExactly(bucket, key, options)
+      })
+
+      it('should fetch the file from the fallback persistor with the fallback bucket with the correct options', function() {
+        expect(
+          fallbackPersistor.promises.getFileStream
+        ).to.have.been.calledWithExactly(fallbackBucket, key, options)
+      })
+
+      it('should only create one stream', function() {
+        expect(fallbackPersistor.promises.getFileStream).to.have.been.calledOnce
+      })
+
+      it('should not send the file to the primary', function() {
+        expect(primaryPersistor.promises.sendStream).not.to.have.been.called
+      })
+    })
+
+    describe('when the file should be copied to the primary', function() {
+      let primaryPersistor, fallbackPersistor, migrationPersistor
+      beforeEach(async function() {
+        primaryPersistor = newPersistor(false)
+        fallbackPersistor = newPersistor(true)
+        migrationPersistor = MigrationPersistor(
+          primaryPersistor,
+          fallbackPersistor
+        )
+        Settings.filestore.fallback.copyOnMiss = true
+        return migrationPersistor.promises.getFileStream(bucket, key, options)
+      })
+
+      it('should create two streams', function() {
+        expect(fallbackPersistor.promises.getFileStream).to.have.been
+          .calledTwice
+      })
+
+      it('should send one of the streams to the primary', function() {
+        expect(
+          primaryPersistor.promises.sendStream
+        ).to.have.been.calledWithExactly(bucket, key, fileStream)
+      })
+    })
+
+    describe('when neither persistor has the file', function() {
+      it('rejects with a NotFoundError', async function() {
+        const migrationPersistor = MigrationPersistor(
+          newPersistor(false),
+          newPersistor(false)
+        )
+        return expect(
+          migrationPersistor.promises.getFileStream(bucket, key)
+        ).to.eventually.be.rejected.and.be.an.instanceOf(Errors.NotFoundError)
+      })
+    })
+
+    describe('when the primary persistor throws an unexpected error', function() {
+      let primaryPersistor, fallbackPersistor, migrationPersistor, error
+      beforeEach(async function() {
+        primaryPersistor = newPersistor(false)
+        fallbackPersistor = newPersistor(true)
+        primaryPersistor.promises.getFileStream = sinon
+          .stub()
+          .rejects(genericError)
+        migrationPersistor = MigrationPersistor(
+          primaryPersistor,
+          fallbackPersistor
+        )
+        try {
+          await migrationPersistor.promises.getFileStream(bucket, key, options)
+        } catch (err) {
+          error = err
+        }
+      })
+
+      it('rejects with the error', function() {
+        expect(error).to.equal(genericError)
+      })
+
+      it('does not call the fallback', function() {
+        expect(fallbackPersistor.promises.getFileStream).not.to.have.been.called
+      })
+    })
+
+    describe('when the fallback persistor throws an unexpected error', function() {
+      let primaryPersistor, fallbackPersistor, migrationPersistor, error
+      beforeEach(async function() {
+        primaryPersistor = newPersistor(false)
+        fallbackPersistor = newPersistor(false)
+        fallbackPersistor.promises.getFileStream = sinon
+          .stub()
+          .rejects(genericError)
+        migrationPersistor = MigrationPersistor(
+          primaryPersistor,
+          fallbackPersistor
+        )
+        try {
+          await migrationPersistor.promises.getFileStream(bucket, key, options)
+        } catch (err) {
+          error = err
+        }
+      })
+
+      it('rejects with the error', function() {
+        expect(error).to.equal(genericError)
+      })
+
+      it('should have called the fallback', function() {
+        expect(
+          fallbackPersistor.promises.getFileStream
+        ).to.have.been.calledWith(fallbackBucket, key)
+      })
+    })
+  })
+
+  describe('sendStream', function() {
+    let primaryPersistor, fallbackPersistor, migrationPersistor
+    beforeEach(function() {
+      primaryPersistor = newPersistor(false)
+      fallbackPersistor = newPersistor(false)
+      migrationPersistor = MigrationPersistor(
+        primaryPersistor,
+        fallbackPersistor
+      )
+    })
+
+    describe('when it works', function() {
+      beforeEach(async function() {
+        return migrationPersistor.promises.sendStream(bucket, key, fileStream)
+      })
+
+      it('should send the file to the primary persistor', function() {
+        expect(
+          primaryPersistor.promises.sendStream
+        ).to.have.been.calledWithExactly(bucket, key, fileStream)
+      })
+
+      it('should not send the file to the fallback persistor', function() {
+        expect(fallbackPersistor.promises.sendStream).not.to.have.been.called
+      })
+    })
+
+    describe('when the primary persistor throws an error', function() {
+      it('returns the error', async function() {
+        primaryPersistor.promises.sendStream.rejects(notFoundError)
+        return expect(
+          migrationPersistor.promises.sendStream(bucket, key, fileStream)
+        ).to.eventually.be.rejected.and.be.an.instanceOf(Errors.NotFoundError)
+      })
+    })
+  })
+
+  describe('deleteFile', function() {
+    let primaryPersistor, fallbackPersistor, migrationPersistor
+    beforeEach(function() {
+      primaryPersistor = newPersistor(false)
+      fallbackPersistor = newPersistor(false)
+      migrationPersistor = MigrationPersistor(
+        primaryPersistor,
+        fallbackPersistor
+      )
+    })
+
+    describe('when it works', function() {
+      beforeEach(async function() {
+        return migrationPersistor.promises.deleteFile(bucket, key)
+      })
+
+      it('should delete the file from the primary', function() {
+        expect(
+          primaryPersistor.promises.deleteFile
+        ).to.have.been.calledWithExactly(bucket, key)
+      })
+
+      it('should delete the file from the fallback', function() {
+        expect(
+          fallbackPersistor.promises.deleteFile
+        ).to.have.been.calledWithExactly(fallbackBucket, key)
+      })
+    })
+
+    describe('when the primary persistor throws an error', function() {
+      let error
+      beforeEach(async function() {
+        primaryPersistor.promises.deleteFile.rejects(genericError)
+        try {
+          await migrationPersistor.promises.deleteFile(bucket, key)
+        } catch (err) {
+          error = err
+        }
+      })
+
+      it('should return the error', function() {
+        expect(error).to.equal(genericError)
+      })
+
+      it('should delete the file from the primary', function() {
+        expect(
+          primaryPersistor.promises.deleteFile
+        ).to.have.been.calledWithExactly(bucket, key)
+      })
+
+      it('should delete the file from the fallback', function() {
+        expect(
+          fallbackPersistor.promises.deleteFile
+        ).to.have.been.calledWithExactly(fallbackBucket, key)
+      })
+    })
+
+    describe('when the fallback persistor throws an error', function() {
+      let error
+      beforeEach(async function() {
+        fallbackPersistor.promises.deleteFile.rejects(genericError)
+        try {
+          await migrationPersistor.promises.deleteFile(bucket, key)
+        } catch (err) {
+          error = err
+        }
+      })
+
+      it('should return the error', function() {
+        expect(error).to.equal(genericError)
+      })
+
+      it('should delete the file from the primary', function() {
+        expect(
+          primaryPersistor.promises.deleteFile
+        ).to.have.been.calledWithExactly(bucket, key)
+      })
+
+      it('should delete the file from the fallback', function() {
+        expect(
+          fallbackPersistor.promises.deleteFile
+        ).to.have.been.calledWithExactly(fallbackBucket, key)
+      })
+    })
+  })
+
+  describe('copyFile', function() {
+    describe('when the file exists on the primary', function() {
+      let primaryPersistor, fallbackPersistor, migrationPersistor
+      beforeEach(async function() {
+        primaryPersistor = newPersistor(true)
+        fallbackPersistor = newPersistor(false)
+        migrationPersistor = MigrationPersistor(
+          primaryPersistor,
+          fallbackPersistor
+        )
+        return migrationPersistor.promises.copyFile(bucket, key, destKey)
+      })
+
+      it('should call copyFile to copy the file', function() {
+        expect(
+          primaryPersistor.promises.copyFile
+        ).to.have.been.calledWithExactly(bucket, key, destKey)
+      })
+
+      it('should not try to read from the fallback', function() {
+        expect(fallbackPersistor.promises.getFileStream).not.to.have.been.called
+      })
+    })
+
+    describe('when the file does not exist on the primary', function() {
+      let primaryPersistor, fallbackPersistor, migrationPersistor
+      beforeEach(async function() {
+        primaryPersistor = newPersistor(false)
+        fallbackPersistor = newPersistor(true)
+        migrationPersistor = MigrationPersistor(
+          primaryPersistor,
+          fallbackPersistor
+        )
+        return migrationPersistor.promises.copyFile(bucket, key, destKey)
+      })
+
+      it('should call copyFile to copy the file', function() {
+        expect(
+          primaryPersistor.promises.copyFile
+        ).to.have.been.calledWithExactly(bucket, key, destKey)
+      })
+
+      it('should fetch the file from the fallback', function() {
+        expect(
+          fallbackPersistor.promises.getFileStream
+        ).not.to.have.been.calledWithExactly(fallbackBucket, key)
+      })
+
+      it('should send the file to the primary', function() {
+        expect(
+          primaryPersistor.promises.sendStream
+        ).to.have.been.calledWithExactly(bucket, destKey, fileStream)
+      })
+    })
+
+    describe('when the file does not exist on the fallback', function() {
+      let primaryPersistor, fallbackPersistor, migrationPersistor, error
+      beforeEach(async function() {
+        primaryPersistor = newPersistor(false)
+        fallbackPersistor = newPersistor(false)
+        migrationPersistor = MigrationPersistor(
+          primaryPersistor,
+          fallbackPersistor
+        )
+        try {
+          await migrationPersistor.promises.copyFile(bucket, key, destKey)
+        } catch (err) {
+          error = err
+        }
+      })
+
+      it('should call copyFile to copy the file', function() {
+        expect(
+          primaryPersistor.promises.copyFile
+        ).to.have.been.calledWithExactly(bucket, key, destKey)
+      })
+
+      it('should fetch the file from the fallback', function() {
+        expect(
+          fallbackPersistor.promises.getFileStream
+        ).not.to.have.been.calledWithExactly(fallbackBucket, key)
+      })
+
+      it('should return a not-found error', function() {
+        expect(error).to.be.an.instanceOf(Errors.NotFoundError)
+      })
+    })
+  })
+})

From b4b7fd226e01921c9f2ffa0c3b3a5979c58f6956 Mon Sep 17 00:00:00 2001
From: Simon Detheridge <s@sd.ai>
Date: Mon, 27 Jan 2020 11:26:37 +0000
Subject: [PATCH 03/12] Add mechanisms to transfer files with md5-based
 integrity checks

Fix error in settings and tidy up tests

Remove unused variable declaration

Remove .only from tests and update eslint rules to catch it in future

Use  to catch errors more safely getting md5 hash

Avoid unnecessary call to S3 to get md5 response
---
 services/filestore/.eslintrc                  |   3 +-
 services/filestore/app/js/FSPersistor.js      |  41 +++++-
 .../filestore/app/js/MigrationPersistor.js    | 136 ++++++++++++++++--
 services/filestore/app/js/S3Persistor.js      | 114 ++++++++++++++-
 .../filestore/config/settings.defaults.coffee |   8 +-
 services/filestore/npm-shrinkwrap.json        |  15 ++
 services/filestore/package.json               |   1 +
 .../test/acceptance/js/FilestoreTests.js      |  55 +++----
 .../test/unit/js/FSPersistorTests.js          |  57 +++++++-
 .../test/unit/js/MigrationPersistorTests.js   | 125 +++++++++++-----
 .../test/unit/js/S3PersistorTests.js          |  77 +++++++++-
 11 files changed, 535 insertions(+), 97 deletions(-)

diff --git a/services/filestore/.eslintrc b/services/filestore/.eslintrc
index 42a4b5cace..73103de7f6 100644
--- a/services/filestore/.eslintrc
+++ b/services/filestore/.eslintrc
@@ -23,7 +23,8 @@
   "rules": {
     // Swap the no-unused-expressions rule with a more chai-friendly one
     "no-unused-expressions": 0,
-    "chai-friendly/no-unused-expressions": "error"
+    "chai-friendly/no-unused-expressions": "error",
+    "no-console": "error"
   },
   "overrides": [
     {
diff --git a/services/filestore/app/js/FSPersistor.js b/services/filestore/app/js/FSPersistor.js
index 2ba65f06d2..3f54e2d091 100644
--- a/services/filestore/app/js/FSPersistor.js
+++ b/services/filestore/app/js/FSPersistor.js
@@ -1,6 +1,7 @@
 const fs = require('fs')
 const glob = require('glob')
 const path = require('path')
+const crypto = require('crypto')
 const rimraf = require('rimraf')
 const Stream = require('stream')
 const { promisify, callbackify } = require('util')
@@ -36,11 +37,22 @@ async function sendFile(location, target, source) {
   }
 }
 
-async function sendStream(location, target, sourceStream) {
+async function sendStream(location, target, sourceStream, sourceMd5) {
   const fsPath = await LocalFileWriter.writeStream(sourceStream)
+  if (!sourceMd5) {
+    sourceMd5 = await _getFileMd5HashForPath(fsPath)
+  }
 
   try {
     await sendFile(location, target, fsPath)
+    const destMd5 = await getFileMd5Hash(location, target)
+    if (sourceMd5 !== destMd5) {
+      await LocalFileWriter.deleteFile(`${location}/${filterName(target)}`)
+      throw new WriteError({
+        message: 'md5 hash mismatch',
+        info: { sourceMd5, destMd5, location, target }
+      })
+    }
   } finally {
     await LocalFileWriter.deleteFile(fsPath)
   }
@@ -80,6 +92,31 @@ async function getFileSize(location, filename) {
   }
 }
 
+async function getFileMd5Hash(location, filename) {
+  const fullPath = path.join(location, filterName(filename))
+  try {
+    return await _getFileMd5HashForPath(fullPath)
+  } catch (err) {
+    throw new ReadError({
+      message: 'unable to get md5 hash from file',
+      info: { location, filename }
+    }).withCause(err)
+  }
+}
+
+async function _getFileMd5HashForPath(fullPath) {
+  return new Promise((resolve, reject) => {
+    const readStream = fs.createReadStream(fullPath)
+    const hash = crypto.createHash('md5')
+    hash.setEncoding('hex')
+    readStream.on('end', () => {
+      hash.end()
+      resolve(hash.read())
+    })
+    pipeline(readStream, hash).catch(reject)
+  })
+}
+
 async function copyFile(location, fromName, toName) {
   const filteredFromName = filterName(fromName)
   const filteredToName = filterName(toName)
@@ -202,6 +239,7 @@ module.exports = {
   sendStream: callbackify(sendStream),
   getFileStream: callbackify(getFileStream),
   getFileSize: callbackify(getFileSize),
+  getFileMd5Hash: callbackify(getFileMd5Hash),
   copyFile: callbackify(copyFile),
   deleteFile: callbackify(deleteFile),
   deleteDirectory: callbackify(deleteDirectory),
@@ -212,6 +250,7 @@ module.exports = {
     sendStream,
     getFileStream,
     getFileSize,
+    getFileMd5Hash,
     copyFile,
     deleteFile,
     deleteDirectory,
diff --git a/services/filestore/app/js/MigrationPersistor.js b/services/filestore/app/js/MigrationPersistor.js
index 9f7a834f31..fdc31368a3 100644
--- a/services/filestore/app/js/MigrationPersistor.js
+++ b/services/filestore/app/js/MigrationPersistor.js
@@ -1,8 +1,9 @@
 const metrics = require('metrics-sharelatex')
 const Settings = require('settings-sharelatex')
 const logger = require('logger-sharelatex')
+const Minipass = require('minipass')
 const { callbackify } = require('util')
-const { NotFoundError } = require('./Errors')
+const { NotFoundError, WriteError } = require('./Errors')
 
 // Persistor that wraps two other persistors. Talks to the 'primary' by default,
 // but will fall back to an older persistor in the case of a not-found error.
@@ -14,7 +15,7 @@ const { NotFoundError } = require('./Errors')
 // e.g.
 // Settings.filestore.fallback.buckets = {
 //   myBucketOnS3: 'myBucketOnGCS'
-// }s
+// }
 
 module.exports = function(primary, fallback) {
   function _wrapMethodOnBothPersistors(method) {
@@ -40,10 +41,7 @@ module.exports = function(primary, fallback) {
   }
 
   function _getFallbackBucket(bucket) {
-    return (
-      Settings.filestore.fallback.buckets &&
-      Settings.filestore.fallback.buckets[bucket]
-    )
+    return Settings.filestore.fallback.buckets[bucket]
   }
 
   function _wrapFallbackMethod(method, enableCopy = true) {
@@ -68,20 +66,130 @@ module.exports = function(primary, fallback) {
     }
   }
 
-  async function _copyFileFromFallback(
+  async function _getFileStreamAndCopyIfRequired(bucketName, key, opts) {
+    const shouldCopy =
+      Settings.filestore.fallback.copyOnMiss && !opts.start && !opts.end
+
+    try {
+      return await primary.promises.getFileStream(bucketName, key, opts)
+    } catch (err) {
+      if (err instanceof NotFoundError) {
+        const fallbackBucket = _getFallbackBucket(bucketName)
+        if (shouldCopy) {
+          return _copyFileFromFallback(
+            fallbackBucket,
+            bucketName,
+            key,
+            key,
+            true
+          )
+        } else {
+          return fallback.promises.getFileStream(fallbackBucket, key, opts)
+        }
+      }
+      throw err
+    }
+  }
+
+  async function _copyFromFallbackStreamAndVerify(
+    stream,
     sourceBucket,
     destBucket,
     sourceKey,
     destKey
   ) {
+    try {
+      let sourceMd5
+      try {
+        sourceMd5 = await fallback.promises.getFileMd5Hash(
+          sourceBucket,
+          sourceKey
+        )
+      } catch (err) {
+        logger.warn(err, 'error getting md5 hash from fallback persistor')
+      }
+
+      await primary.promises.sendStream(destBucket, destKey, stream, sourceMd5)
+    } catch (err) {
+      let error = err
+      metrics.inc('fallback.copy.failure')
+
+      try {
+        await primary.promises.deleteFile(destBucket, destKey)
+      } catch (err) {
+        error = new WriteError({
+          message: 'unable to clean up destination copy artifact',
+          info: {
+            destBucket,
+            destKey
+          }
+        }).withCause(err)
+      }
+
+      error = new WriteError({
+        message: 'unable to copy file to destination persistor',
+        info: {
+          sourceBucket,
+          destBucket,
+          sourceKey,
+          destKey
+        }
+      }).withCause(error)
+
+      logger.warn({ error }, 'failed to copy file from fallback')
+      throw error
+    }
+  }
+
+  async function _copyFileFromFallback(
+    sourceBucket,
+    destBucket,
+    sourceKey,
+    destKey,
+    returnStream = false
+  ) {
+    metrics.inc('fallback.copy')
     const sourceStream = await fallback.promises.getFileStream(
       sourceBucket,
       sourceKey,
       {}
     )
 
-    await primary.promises.sendStream(destBucket, destKey, sourceStream)
-    metrics.inc('fallback.copy')
+    if (!returnStream) {
+      return _copyFromFallbackStreamAndVerify(
+        sourceStream,
+        sourceBucket,
+        destBucket,
+        sourceKey,
+        destKey
+      )
+    }
+
+    const tee = new Minipass()
+    const clientStream = new Minipass()
+    const copyStream = new Minipass()
+
+    tee.pipe(clientStream)
+    tee.pipe(copyStream)
+
+    // copy the file in the background
+    _copyFromFallbackStreamAndVerify(
+      copyStream,
+      sourceBucket,
+      destBucket,
+      sourceKey,
+      destKey
+    ).catch(
+      // the error handler in this method will log a metric and a warning, so
+      // we don't need to do anything extra here, but catching it will prevent
+      // unhandled promise rejection warnings
+      () => {}
+    )
+
+    // start piping the source stream into the tee after everything is set up,
+    // otherwise one stream may consume bytes that don't arrive at the other
+    sourceStream.pipe(tee)
+    return clientStream
   }
 
   return {
@@ -89,7 +197,8 @@ module.exports = function(primary, fallback) {
     fallbackPersistor: fallback,
     sendFile: primary.sendFile,
     sendStream: primary.sendStream,
-    getFileStream: callbackify(_wrapFallbackMethod('getFileStream')),
+    getFileStream: callbackify(_getFileStreamAndCopyIfRequired),
+    getFileMd5Hash: callbackify(_wrapFallbackMethod('getFileMd5Hash')),
     deleteDirectory: callbackify(
       _wrapMethodOnBothPersistors('deleteDirectory')
     ),
@@ -97,17 +206,18 @@ module.exports = function(primary, fallback) {
     deleteFile: callbackify(_wrapMethodOnBothPersistors('deleteFile')),
     copyFile: callbackify(copyFileWithFallback),
     checkIfFileExists: callbackify(_wrapFallbackMethod('checkIfFileExists')),
-    directorySize: callbackify(_wrapFallbackMethod('directorySize', false)),
+    directorySize: callbackify(_wrapFallbackMethod('directorySize')),
     promises: {
       sendFile: primary.promises.sendFile,
       sendStream: primary.promises.sendStream,
-      getFileStream: _wrapFallbackMethod('getFileStream'),
+      getFileStream: _getFileStreamAndCopyIfRequired,
+      getFileMd5Hash: _wrapFallbackMethod('getFileMd5Hash'),
       deleteDirectory: _wrapMethodOnBothPersistors('deleteDirectory'),
       getFileSize: _wrapFallbackMethod('getFileSize'),
       deleteFile: _wrapMethodOnBothPersistors('deleteFile'),
       copyFile: copyFileWithFallback,
       checkIfFileExists: _wrapFallbackMethod('checkIfFileExists'),
-      directorySize: _wrapFallbackMethod('directorySize', false)
+      directorySize: _wrapFallbackMethod('directorySize')
     }
   }
 }
diff --git a/services/filestore/app/js/S3Persistor.js b/services/filestore/app/js/S3Persistor.js
index 6d22823401..ef465da25c 100644
--- a/services/filestore/app/js/S3Persistor.js
+++ b/services/filestore/app/js/S3Persistor.js
@@ -5,8 +5,11 @@ https.globalAgent.maxSockets = 300
 
 const settings = require('settings-sharelatex')
 const metrics = require('metrics-sharelatex')
+const logger = require('logger-sharelatex')
 
+const Minipass = require('minipass')
 const meter = require('stream-meter')
+const crypto = require('crypto')
 const fs = require('fs')
 const S3 = require('aws-sdk/clients/s3')
 const { URL } = require('url')
@@ -22,6 +25,7 @@ module.exports = {
   sendFile: callbackify(sendFile),
   sendStream: callbackify(sendStream),
   getFileStream: callbackify(getFileStream),
+  getFileMd5Hash: callbackify(getFileMd5Hash),
   deleteDirectory: callbackify(deleteDirectory),
   getFileSize: callbackify(getFileSize),
   deleteFile: callbackify(deleteFile),
@@ -32,6 +36,7 @@ module.exports = {
     sendFile,
     sendStream,
     getFileStream,
+    getFileMd5Hash,
     deleteDirectory,
     getFileSize,
     deleteFile,
@@ -41,6 +46,10 @@ module.exports = {
   }
 }
 
+function hexToBase64(hex) {
+  return Buffer.from(hex, 'hex').toString('base64')
+}
+
 async function sendFile(bucketName, key, fsPath) {
   let readStream
   try {
@@ -56,20 +65,79 @@ async function sendFile(bucketName, key, fsPath) {
   return sendStream(bucketName, key, readStream)
 }
 
-async function sendStream(bucketName, key, readStream) {
+async function sendStream(bucketName, key, readStream, sourceMd5) {
   try {
+    // if there is no supplied md5 hash, we calculate the hash as the data passes through
+    const passthroughStream = new Minipass()
+    let hashPromise
+    let b64Hash
+
+    if (sourceMd5) {
+      b64Hash = hexToBase64(sourceMd5)
+    } else {
+      const hash = crypto.createHash('md5')
+      hash.setEncoding('hex')
+      passthroughStream.pipe(hash)
+      hashPromise = new Promise((resolve, reject) => {
+        passthroughStream.on('end', () => {
+          hash.end()
+          resolve(hash.read())
+        })
+        passthroughStream.on('error', err => {
+          reject(err)
+        })
+      })
+    }
+
     const meteredStream = meter()
+    passthroughStream.pipe(meteredStream)
     meteredStream.on('finish', () => {
       metrics.count('s3.egress', meteredStream.bytes)
     })
 
-    await _getClientForBucket(bucketName)
-      .upload({
-        Bucket: bucketName,
-        Key: key,
-        Body: readStream.pipe(meteredStream)
-      })
+    // pipe the readstream through minipass, which can write to both the metered
+    // stream (which goes on to S3) and the md5 generator if necessary
+    // - we do this last so that a listener streams does not consume data meant
+    // for both destinations
+    readStream.pipe(passthroughStream)
+
+    // if we have an md5 hash, pass this to S3 to verify the upload
+    const uploadOptions = {
+      Bucket: bucketName,
+      Key: key,
+      Body: meteredStream
+    }
+    if (b64Hash) {
+      uploadOptions.ContentMD5 = b64Hash
+    }
+
+    const response = await _getClientForBucket(bucketName)
+      .upload(uploadOptions)
       .promise()
+    const destMd5 = _md5FromResponse(response)
+
+    // if we didn't have an md5 hash, compare our computed one with S3's
+    if (hashPromise) {
+      sourceMd5 = await hashPromise
+
+      if (sourceMd5 !== destMd5) {
+        try {
+          await deleteFile(bucketName, key)
+        } catch (err) {
+          logger.warn(err, 'error deleting file for invalid upload')
+        }
+
+        throw new WriteError({
+          message: 'source and destination hashes do not match',
+          info: {
+            sourceMd5,
+            destMd5,
+            bucketName,
+            key
+          }
+        })
+      }
+    }
   } catch (err) {
     throw _wrapError(
       err,
@@ -167,6 +235,23 @@ async function getFileSize(bucketName, key) {
   }
 }
 
+async function getFileMd5Hash(bucketName, key) {
+  try {
+    const response = await _getClientForBucket(bucketName)
+      .headObject({ Bucket: bucketName, Key: key })
+      .promise()
+    const md5 = _md5FromResponse(response)
+    return md5
+  } catch (err) {
+    throw _wrapError(
+      err,
+      'error getting hash of s3 object',
+      { bucketName, key },
+      ReadError
+    )
+  }
+}
+
 async function deleteFile(bucketName, key) {
   try {
     await _getClientForBucket(bucketName)
@@ -314,3 +399,18 @@ function _buildClientOptions(bucketCredentials) {
 
   return options
 }
+
+function _md5FromResponse(response) {
+  const md5 = (response.ETag || '').replace(/[ "]/g, '')
+  if (!md5.match(/^[a-f0-9]{32}$/)) {
+    throw new ReadError({
+      message: 's3 etag not in md5-hash format',
+      info: {
+        md5,
+        eTag: response.ETag
+      }
+    })
+  }
+
+  return md5
+}
diff --git a/services/filestore/config/settings.defaults.coffee b/services/filestore/config/settings.defaults.coffee
index a4a2df2d24..bb124ae8e0 100644
--- a/services/filestore/config/settings.defaults.coffee
+++ b/services/filestore/config/settings.defaults.coffee
@@ -17,8 +17,8 @@ unless process.env['BACKEND']?
 	else
 		process.env['BACKEND'] = "fs"
 		process.env['USER_FILES_BUCKET_NAME'] = Path.resolve(__dirname + "/../user_files")
-		process.env['TEMPLATE_FILES_BUCKET_NAME'] = Path.resolve(__dirname + "/../public_files")
-		process.env['PUBLIC_FILES_BUCKET_NAME'] = Path.resolve(__dirname + "/../template_files")
+		process.env['TEMPLATE_FILES_BUCKET_NAME'] = Path.resolve(__dirname + "/../template_files")
+		process.env['PUBLIC_FILES_BUCKET_NAME'] = Path.resolve(__dirname + "/../public_files")
 
 settings =
 	internal:
@@ -51,8 +51,8 @@ settings =
 				backend: process.env['FALLBACK_BACKEND']
 				# mapping of bucket names on the fallback, to bucket names on the primary.
 				# e.g. { myS3UserFilesBucketName: 'myGoogleUserFilesBucketName' }
-				buckets: JSON.parse process.env['FALLBACK_BUCKET_MAPPING'] if process.env['FALLBACK_BUCKET_MAPPING']?
-				copyOnMiss: if process.env['COPY_ON_MISS'] == 'true' then true else false
+				buckets: JSON.parse(process.env['FALLBACK_BUCKET_MAPPING'] || '{}')
+				copyOnMiss: process.env['COPY_ON_MISS'] == 'true'
 
 	path:
 		uploadFolder: Path.resolve(__dirname + "/../uploads")
diff --git a/services/filestore/npm-shrinkwrap.json b/services/filestore/npm-shrinkwrap.json
index b343d6ad2c..a4206a94e0 100644
--- a/services/filestore/npm-shrinkwrap.json
+++ b/services/filestore/npm-shrinkwrap.json
@@ -3129,6 +3129,21 @@
       "resolved": "https://registry.npmjs.org/minimist/-/minimist-0.0.8.tgz",
       "integrity": "sha1-hX/Kv8M5fSYluCKCYuhqp6ARsF0="
     },
+    "minipass": {
+      "version": "3.1.1",
+      "resolved": "https://registry.npmjs.org/minipass/-/minipass-3.1.1.tgz",
+      "integrity": "sha512-UFqVihv6PQgwj8/yTGvl9kPz7xIAY+R5z6XYjRInD3Gk3qx6QGSD6zEcpeG4Dy/lQnv1J6zv8ejV90hyYIKf3w==",
+      "requires": {
+        "yallist": "^4.0.0"
+      },
+      "dependencies": {
+        "yallist": {
+          "version": "4.0.0",
+          "resolved": "https://registry.npmjs.org/yallist/-/yallist-4.0.0.tgz",
+          "integrity": "sha512-3wdGidZyq5PB084XLES5TpOSRA3wjXAlIWMhum2kRcv/41Sn2emQ0dycQW4uZXLejwKvg6EsvbdlVL+FYEct7A=="
+        }
+      }
+    },
     "mkdirp": {
       "version": "0.5.1",
       "resolved": "https://registry.npmjs.org/mkdirp/-/mkdirp-0.5.1.tgz",
diff --git a/services/filestore/package.json b/services/filestore/package.json
index 303393bd56..2e9cef8aa0 100644
--- a/services/filestore/package.json
+++ b/services/filestore/package.json
@@ -31,6 +31,7 @@
     "knox": "~0.9.1",
     "logger-sharelatex": "^1.7.0",
     "metrics-sharelatex": "^2.2.0",
+    "minipass": "^3.1.1",
     "mocha": "5.2.0",
     "node-transloadit": "0.0.4",
     "node-uuid": "~1.4.1",
diff --git a/services/filestore/test/acceptance/js/FilestoreTests.js b/services/filestore/test/acceptance/js/FilestoreTests.js
index 5a0de3abd8..1c96445a3a 100644
--- a/services/filestore/test/acceptance/js/FilestoreTests.js
+++ b/services/filestore/test/acceptance/js/FilestoreTests.js
@@ -84,11 +84,11 @@ const BackendSettings = {
           __dirname,
           '../../../user_files'
         ),
-        [process.env.AWS_S3_TEMPLATE_FILES_BUCKET_NAME]: Path.resolve(
+        [process.env.AWS_S3_PUBLIC_FILES_BUCKET_NAME]: Path.resolve(
           __dirname,
           '../../../public_files'
         ),
-        [process.env.AWS_S3_PUBLIC_FILES_BUCKET_NAME]: Path.resolve(
+        [process.env.AWS_S3_TEMPLATE_FILES_BUCKET_NAME]: Path.resolve(
           __dirname,
           '../../../template_files'
         )
@@ -114,9 +114,9 @@ const BackendSettings = {
         [Path.resolve(__dirname, '../../../user_files')]: process.env
           .AWS_S3_USER_FILES_BUCKET_NAME,
         [Path.resolve(__dirname, '../../../public_files')]: process.env
-          .AWS_S3_TEMPLATE_FILES_BUCKET_NAME,
+          .AWS_S3_PUBLIC_FILES_BUCKET_NAME,
         [Path.resolve(__dirname, '../../../template_files')]: process.env
-          .AWS_S3_PUBLIC_FILES_BUCKET_NAME
+          .AWS_S3_TEMPLATE_FILES_BUCKET_NAME
       }
     }
   }
@@ -130,7 +130,7 @@ describe('Filestore', function() {
   // redefine the test suite for every available backend
   Object.keys(BackendSettings).forEach(backend => {
     describe(backend, function() {
-      let app, previousEgress, previousIngress
+      let app, previousEgress, previousIngress, projectId
 
       before(async function() {
         // create the app with the relevant filestore settings
@@ -151,6 +151,7 @@ describe('Filestore', function() {
             getMetric(filestoreUrl, 's3_ingress')
           ])
         }
+        projectId = `acceptance_tests_${Math.random()}`
       })
 
       it('should send a 200 for the status endpoint', async function() {
@@ -174,7 +175,7 @@ describe('Filestore', function() {
 
         beforeEach(async function() {
           fileId = Math.random()
-          fileUrl = `${filestoreUrl}/project/acceptance_tests/file/${directoryName}%2F${fileId}`
+          fileUrl = `${filestoreUrl}/project/${projectId}/file/${directoryName}%2F${fileId}`
           constantFileContent = [
             'hello world',
             `line 2 goes here ${Math.random()}`,
@@ -242,7 +243,7 @@ describe('Filestore', function() {
         })
 
         it('should be able to copy files', async function() {
-          const newProjectID = 'acceptance_tests_copied_project'
+          const newProjectID = `acceptance_tests_copied_project_${Math.random()}`
           const newFileId = Math.random()
           const newFileUrl = `${filestoreUrl}/project/${newProjectID}/file/${directoryName}%2F${newFileId}`
           const opts = {
@@ -250,7 +251,7 @@ describe('Filestore', function() {
             uri: newFileUrl,
             json: {
               source: {
-                project_id: 'acceptance_tests',
+                project_id: projectId,
                 file_id: `${directoryName}/${fileId}`
               }
             }
@@ -304,7 +305,7 @@ describe('Filestore', function() {
       })
 
       describe('with multiple files', function() {
-        let fileIds, fileUrls, project
+        let fileIds, fileUrls
         const directoryName = 'directory'
         const localFileReadPaths = [
           '/tmp/filestore_acceptance_tests_file_read_1.txt',
@@ -331,11 +332,10 @@ describe('Filestore', function() {
         })
 
         beforeEach(async function() {
-          project = `acceptance_tests_${Math.random()}`
           fileIds = [Math.random(), Math.random()]
           fileUrls = [
-            `${filestoreUrl}/project/${project}/file/${directoryName}%2F${fileIds[0]}`,
-            `${filestoreUrl}/project/${project}/file/${directoryName}%2F${fileIds[1]}`
+            `${filestoreUrl}/project/${projectId}/file/${directoryName}%2F${fileIds[0]}`,
+            `${filestoreUrl}/project/${projectId}/file/${directoryName}%2F${fileIds[1]}`
           ]
 
           const writeStreams = [
@@ -359,7 +359,7 @@ describe('Filestore', function() {
 
         it('should get the directory size', async function() {
           const response = await rp.get(
-            `${filestoreUrl}/project/${project}/size`
+            `${filestoreUrl}/project/${projectId}/size`
           )
           expect(parseInt(JSON.parse(response.body)['total bytes'])).to.equal(
             constantFileContents[0].length + constantFileContents[1].length
@@ -459,7 +459,6 @@ describe('Filestore', function() {
             fileUrl,
             bucket,
             fallbackBucket
-          const projectId = 'acceptance_tests'
 
           beforeEach(function() {
             constantFileContent = `This is yet more file content ${Math.random()}`
@@ -503,14 +502,20 @@ describe('Filestore', function() {
               expect(res.body).to.equal(constantFileContent)
             })
 
-            it('should not copy the file to the primary', async function() {
-              await rp.get(fileUrl)
+            describe('when copyOnMiss is disabled', function() {
+              beforeEach(function() {
+                Settings.filestore.fallback.copyOnMiss = false
+              })
 
-              await expectPersistorNotToHaveFile(
-                app.persistor.primaryPersistor,
-                bucket,
-                fileKey
-              )
+              it('should not copy the file to the primary', async function() {
+                await rp.get(fileUrl)
+
+                await expectPersistorNotToHaveFile(
+                  app.persistor.primaryPersistor,
+                  bucket,
+                  fileKey
+                )
+              })
             })
 
             describe('when copyOnMiss is enabled', function() {
@@ -534,9 +539,9 @@ describe('Filestore', function() {
 
             describe('when copying a file', function() {
               let newFileId, newFileUrl, newFileKey
-              const newProjectID = 'acceptance_tests_copied_project'
 
               beforeEach(async function() {
+                const newProjectID = `acceptance_tests_copied_project_${Math.random()}`
                 newFileId = Math.random()
                 newFileUrl = `${filestoreUrl}/project/${newProjectID}/file/${directoryName}%2F${newFileId}`
                 newFileKey = `${newProjectID}/${directoryName}/${newFileId}`
@@ -546,7 +551,7 @@ describe('Filestore', function() {
                   uri: newFileUrl,
                   json: {
                     source: {
-                      project_id: 'acceptance_tests',
+                      project_id: projectId,
                       file_id: `${directoryName}/${fileId}`
                     }
                   }
@@ -616,7 +621,7 @@ describe('Filestore', function() {
               await expectPersistorNotToHaveFile(
                 app.persistor.fallbackPersistor,
                 fallbackBucket,
-                `acceptance_tests/${directoryName}/${fileId}`
+                `${projectId}/${directoryName}/${fileId}`
               )
             })
           })
@@ -706,7 +711,7 @@ describe('Filestore', function() {
 
         beforeEach(async function() {
           fileId = Math.random()
-          fileUrl = `${filestoreUrl}/project/acceptance_tests/file/${directoryName}%2F${fileId}`
+          fileUrl = `${filestoreUrl}/project/${projectId}/file/${directoryName}%2F${fileId}`
           const stat = await fsStat(localFileReadPath)
           localFileSize = stat.size
           const writeStream = request.post(fileUrl)
diff --git a/services/filestore/test/unit/js/FSPersistorTests.js b/services/filestore/test/unit/js/FSPersistorTests.js
index ba343c548c..1be8eea3e2 100644
--- a/services/filestore/test/unit/js/FSPersistorTests.js
+++ b/services/filestore/test/unit/js/FSPersistorTests.js
@@ -12,19 +12,32 @@ const modulePath = '../../../app/js/FSPersistor.js'
 describe('FSPersistorTests', function() {
   const stat = { size: 4, isFile: sinon.stub().returns(true) }
   const fd = 1234
-  const readStream = 'readStream'
   const writeStream = 'writeStream'
   const remoteStream = 'remoteStream'
   const tempFile = '/tmp/potato.txt'
   const location = '/foo'
   const error = new Error('guru meditation error')
+  const md5 = 'ffffffff'
 
   const files = ['animals/wombat.tex', 'vegetables/potato.tex']
   const globs = [`${location}/${files[0]}`, `${location}/${files[1]}`]
   const filteredFilenames = ['animals_wombat.tex', 'vegetables_potato.tex']
-  let fs, rimraf, stream, LocalFileWriter, FSPersistor, glob
+  let fs,
+    rimraf,
+    stream,
+    LocalFileWriter,
+    FSPersistor,
+    glob,
+    readStream,
+    crypto,
+    Hash
 
   beforeEach(function() {
+    readStream = {
+      name: 'readStream',
+      on: sinon.stub().yields(),
+      pipe: sinon.stub()
+    }
     fs = {
       createReadStream: sinon.stub().returns(readStream),
       createWriteStream: sinon.stub().returns(writeStream),
@@ -41,6 +54,14 @@ describe('FSPersistorTests', function() {
         deleteFile: sinon.stub().resolves()
       }
     }
+    Hash = {
+      end: sinon.stub(),
+      read: sinon.stub().returns(md5),
+      setEncoding: sinon.stub()
+    }
+    crypto = {
+      createHash: sinon.stub().returns(Hash)
+    }
     FSPersistor = SandboxedModule.require(modulePath, {
       requires: {
         './LocalFileWriter': LocalFileWriter,
@@ -48,7 +69,8 @@ describe('FSPersistorTests', function() {
         fs,
         glob,
         rimraf,
-        stream
+        stream,
+        crypto
       },
       globals: { console }
     })
@@ -103,6 +125,35 @@ describe('FSPersistorTests', function() {
       await FSPersistor.promises.sendStream(location, files[0], remoteStream)
       expect(fs.createReadStream).to.have.been.calledWith(tempFile)
     })
+
+    describe('when the md5 hash does not match', function() {
+      it('should return a write error', async function() {
+        await expect(
+          FSPersistor.promises.sendStream(
+            location,
+            files[0],
+            remoteStream,
+            '00000000'
+          )
+        )
+          .to.eventually.be.rejected.and.be.an.instanceOf(Errors.WriteError)
+          .and.have.property('message', 'md5 hash mismatch')
+      })
+
+      it('deletes the copied file', async function() {
+        try {
+          await FSPersistor.promises.sendStream(
+            location,
+            files[0],
+            remoteStream,
+            '00000000'
+          )
+        } catch (_) {}
+        expect(LocalFileWriter.promises.deleteFile).to.have.been.calledWith(
+          `${location}/${filteredFilenames[0]}`
+        )
+      })
+    })
   })
 
   describe('getFileStream', function() {
diff --git a/services/filestore/test/unit/js/MigrationPersistorTests.js b/services/filestore/test/unit/js/MigrationPersistorTests.js
index 1cc8324d46..83159f38ad 100644
--- a/services/filestore/test/unit/js/MigrationPersistorTests.js
+++ b/services/filestore/test/unit/js/MigrationPersistorTests.js
@@ -21,35 +21,53 @@ describe('MigrationPersistorTests', function() {
   const genericError = new Error('guru meditation error')
   const notFoundError = new Errors.NotFoundError('not found')
   const size = 33
-  const fileStream = 'fileStream'
+  const md5 = 'ffffffff'
 
-  function newPersistor(hasFile) {
-    return {
-      promises: {
-        sendFile: sinon.stub().resolves(),
-        sendStream: sinon.stub().resolves(),
-        getFileStream: hasFile
-          ? sinon.stub().resolves(fileStream)
-          : sinon.stub().rejects(notFoundError),
-        deleteDirectory: sinon.stub().resolves(),
-        getFileSize: hasFile
-          ? sinon.stub().resolves(size)
-          : sinon.stub().rejects(notFoundError),
-        deleteFile: sinon.stub().resolves(),
-        copyFile: hasFile
-          ? sinon.stub().resolves()
-          : sinon.stub().rejects(notFoundError),
-        checkIfFileExists: sinon.stub().resolves(hasFile),
-        directorySize: hasFile
-          ? sinon.stub().resolves(size)
-          : sinon.stub().rejects(notFoundError)
-      }
-    }
-  }
-
-  let Metrics, Settings, Logger, MigrationPersistor
+  let Metrics,
+    Settings,
+    Logger,
+    MigrationPersistor,
+    Minipass,
+    fileStream,
+    newPersistor
 
   beforeEach(function() {
+    fileStream = {
+      name: 'fileStream',
+      on: sinon
+        .stub()
+        .withArgs('end')
+        .yields(),
+      pipe: sinon.stub()
+    }
+
+    newPersistor = function(hasFile) {
+      return {
+        promises: {
+          sendFile: sinon.stub().resolves(),
+          sendStream: sinon.stub().resolves(),
+          getFileStream: hasFile
+            ? sinon.stub().resolves(fileStream)
+            : sinon.stub().rejects(notFoundError),
+          deleteDirectory: sinon.stub().resolves(),
+          getFileSize: hasFile
+            ? sinon.stub().resolves(size)
+            : sinon.stub().rejects(notFoundError),
+          deleteFile: sinon.stub().resolves(),
+          copyFile: hasFile
+            ? sinon.stub().resolves()
+            : sinon.stub().rejects(notFoundError),
+          checkIfFileExists: sinon.stub().resolves(hasFile),
+          directorySize: hasFile
+            ? sinon.stub().resolves(size)
+            : sinon.stub().rejects(notFoundError),
+          getFileMd5Hash: hasFile
+            ? sinon.stub().resolves(md5)
+            : sinon.stub().rejects(notFoundError)
+        }
+      }
+    }
+
     Settings = {
       filestore: {
         fallback: {
@@ -68,12 +86,20 @@ describe('MigrationPersistorTests', function() {
       warn: sinon.stub()
     }
 
+    Minipass = sinon.stub()
+    Minipass.prototype.on = sinon
+      .stub()
+      .withArgs('end')
+      .yields()
+    Minipass.prototype.pipe = sinon.stub()
+
     MigrationPersistor = SandboxedModule.require(modulePath, {
       requires: {
         'settings-sharelatex': Settings,
         './Errors': Errors,
         'metrics-sharelatex': Metrics,
-        'logger-sharelatex': Logger
+        'logger-sharelatex': Logger,
+        minipass: Minipass
       },
       globals: { console }
     })
@@ -144,7 +170,7 @@ describe('MigrationPersistorTests', function() {
         ).to.have.been.calledWithExactly(fallbackBucket, key, options)
       })
 
-      it('should only create one stream', function() {
+      it('should create one read stream', function() {
         expect(fallbackPersistor.promises.getFileStream).to.have.been.calledOnce
       })
 
@@ -154,7 +180,10 @@ describe('MigrationPersistorTests', function() {
     })
 
     describe('when the file should be copied to the primary', function() {
-      let primaryPersistor, fallbackPersistor, migrationPersistor
+      let primaryPersistor,
+        fallbackPersistor,
+        migrationPersistor,
+        returnedStream
       beforeEach(async function() {
         primaryPersistor = newPersistor(false)
         fallbackPersistor = newPersistor(true)
@@ -163,18 +192,36 @@ describe('MigrationPersistorTests', function() {
           fallbackPersistor
         )
         Settings.filestore.fallback.copyOnMiss = true
-        return migrationPersistor.promises.getFileStream(bucket, key, options)
+        returnedStream = await migrationPersistor.promises.getFileStream(
+          bucket,
+          key,
+          options
+        )
       })
 
-      it('should create two streams', function() {
-        expect(fallbackPersistor.promises.getFileStream).to.have.been
-          .calledTwice
+      it('should create one read stream', function() {
+        expect(fallbackPersistor.promises.getFileStream).to.have.been.calledOnce
       })
 
-      it('should send one of the streams to the primary', function() {
+      it('should get the md5 hash from the source', function() {
+        expect(
+          fallbackPersistor.promises.getFileMd5Hash
+        ).to.have.been.calledWith(fallbackBucket, key)
+      })
+
+      it('should send a stream to the primary', function() {
         expect(
           primaryPersistor.promises.sendStream
-        ).to.have.been.calledWithExactly(bucket, key, fileStream)
+        ).to.have.been.calledWithExactly(
+          bucket,
+          key,
+          sinon.match.instanceOf(Minipass),
+          md5
+        )
+      })
+
+      it('should send a stream to the client', function() {
+        expect(returnedStream).to.be.an.instanceOf(Minipass)
       })
     })
 
@@ -420,10 +467,16 @@ describe('MigrationPersistorTests', function() {
         ).not.to.have.been.calledWithExactly(fallbackBucket, key)
       })
 
+      it('should get the md5 hash from the source', function() {
+        expect(
+          fallbackPersistor.promises.getFileMd5Hash
+        ).to.have.been.calledWith(fallbackBucket, key)
+      })
+
       it('should send the file to the primary', function() {
         expect(
           primaryPersistor.promises.sendStream
-        ).to.have.been.calledWithExactly(bucket, destKey, fileStream)
+        ).to.have.been.calledWithExactly(bucket, destKey, fileStream, md5)
       })
     })
 
diff --git a/services/filestore/test/unit/js/S3PersistorTests.js b/services/filestore/test/unit/js/S3PersistorTests.js
index 7a945b4d19..4f700c8797 100644
--- a/services/filestore/test/unit/js/S3PersistorTests.js
+++ b/services/filestore/test/unit/js/S3PersistorTests.js
@@ -26,8 +26,10 @@ describe('S3PersistorTests', function() {
     { Key: 'hippo', Size: 22 }
   ]
   const filesSize = 33
+  const md5 = 'ffffffff00000000ffffffff00000000'
 
   let Metrics,
+    Logger,
     S3,
     Fs,
     Meter,
@@ -40,7 +42,10 @@ describe('S3PersistorTests', function() {
     S3AccessDeniedError,
     FileNotFoundError,
     EmptyPromise,
-    settings
+    settings,
+    Minipass,
+    Hash,
+    crypto
 
   beforeEach(function() {
     settings = {
@@ -100,7 +105,8 @@ describe('S3PersistorTests', function() {
       }),
       headObject: sinon.stub().returns({
         promise: sinon.stub().resolves({
-          ContentLength: objectSize
+          ContentLength: objectSize,
+          ETag: md5
         })
       }),
       listObjects: sinon.stub().returns({
@@ -108,21 +114,46 @@ describe('S3PersistorTests', function() {
           Contents: files
         })
       }),
-      upload: sinon.stub().returns(EmptyPromise),
+      upload: sinon
+        .stub()
+        .returns({ promise: sinon.stub().resolves({ ETag: `"${md5}"` }) }),
       copyObject: sinon.stub().returns(EmptyPromise),
       deleteObject: sinon.stub().returns(EmptyPromise),
       deleteObjects: sinon.stub().returns(EmptyPromise)
     }
     S3 = sinon.stub().returns(S3Client)
 
+    Hash = {
+      end: sinon.stub(),
+      read: sinon.stub().returns(md5),
+      setEncoding: sinon.stub()
+    }
+    crypto = {
+      createHash: sinon.stub().returns(Hash)
+    }
+
+    Minipass = sinon.stub()
+    Minipass.prototype.on = sinon
+      .stub()
+      .withArgs('end')
+      .yields()
+    Minipass.prototype.pipe = sinon.stub()
+
+    Logger = {
+      warn: sinon.stub()
+    }
+
     S3Persistor = SandboxedModule.require(modulePath, {
       requires: {
         'aws-sdk/clients/s3': S3,
         'settings-sharelatex': settings,
+        'logger-sharelatex': Logger,
         './Errors': Errors,
         fs: Fs,
         'stream-meter': Meter,
-        'metrics-sharelatex': Metrics
+        'metrics-sharelatex': Metrics,
+        minipass: Minipass,
+        crypto
       },
       globals: { console }
     })
@@ -420,17 +451,49 @@ describe('S3PersistorTests', function() {
         expect(S3Client.upload).to.have.been.calledWith({
           Bucket: bucket,
           Key: key,
-          Body: 'readStream'
+          Body: MeteredStream
         })
       })
 
       it('should meter the stream', function() {
-        expect(ReadStream.pipe).to.have.been.calledWith(MeteredStream)
+        expect(Minipass.prototype.pipe).to.have.been.calledWith(MeteredStream)
       })
 
       it('should record an egress metric', function() {
         expect(Metrics.count).to.have.been.calledWith('s3.egress', objectSize)
       })
+
+      it('calculates the md5 hash of the file', function() {
+        expect(Minipass.prototype.pipe).to.have.been.calledWith(Hash)
+      })
+    })
+
+    describe('when a hash is supploed', function() {
+      beforeEach(async function() {
+        return S3Persistor.promises.sendStream(
+          bucket,
+          key,
+          ReadStream,
+          'aaaaaaaabbbbbbbbaaaaaaaabbbbbbbb'
+        )
+      })
+
+      it('should not calculate the md5 hash of the file', function() {
+        expect(Minipass.prototype.pipe).not.to.have.been.calledWith(Hash)
+      })
+
+      it('sends the hash in base64', function() {
+        expect(S3Client.upload).to.have.been.calledWith({
+          Bucket: bucket,
+          Key: key,
+          Body: MeteredStream,
+          ContentMD5: 'qqqqqru7u7uqqqqqu7u7uw=='
+        })
+      })
+
+      it('does not fetch the md5 hash of the uploaded file', function() {
+        expect(S3Client.headObject).not.to.have.been.called
+      })
     })
 
     describe('when the upload fails', function() {
@@ -466,7 +529,7 @@ describe('S3PersistorTests', function() {
         expect(S3Client.upload).to.have.been.calledWith({
           Bucket: bucket,
           Key: key,
-          Body: 'readStream'
+          Body: MeteredStream
         })
       })
     })

From 5d5d325691cf3c1b4f13294a1bf3003c956ed9a3 Mon Sep 17 00:00:00 2001
From: Simon Detheridge <s@sd.ai>
Date: Wed, 29 Jan 2020 11:09:20 +0000
Subject: [PATCH 04/12] Preserve all error information when cleanup of copied
 file fails

---
 .../filestore/app/js/MigrationPersistor.js    | 32 +++++++++----------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/services/filestore/app/js/MigrationPersistor.js b/services/filestore/app/js/MigrationPersistor.js
index fdc31368a3..3a4789693b 100644
--- a/services/filestore/app/js/MigrationPersistor.js
+++ b/services/filestore/app/js/MigrationPersistor.js
@@ -111,22 +111,7 @@ module.exports = function(primary, fallback) {
 
       await primary.promises.sendStream(destBucket, destKey, stream, sourceMd5)
     } catch (err) {
-      let error = err
-      metrics.inc('fallback.copy.failure')
-
-      try {
-        await primary.promises.deleteFile(destBucket, destKey)
-      } catch (err) {
-        error = new WriteError({
-          message: 'unable to clean up destination copy artifact',
-          info: {
-            destBucket,
-            destKey
-          }
-        }).withCause(err)
-      }
-
-      error = new WriteError({
+      const error = new WriteError({
         message: 'unable to copy file to destination persistor',
         info: {
           sourceBucket,
@@ -134,7 +119,20 @@ module.exports = function(primary, fallback) {
           sourceKey,
           destKey
         }
-      }).withCause(error)
+      }).withCause(err)
+      metrics.inc('fallback.copy.failure')
+
+      try {
+        await primary.promises.deleteFile(destBucket, destKey)
+      } catch (err) {
+        error.info.cleanupError = new WriteError({
+          message: 'unable to clean up destination copy artifact',
+          info: {
+            destBucket,
+            destKey
+          }
+        }).withCause(err)
+      }
 
       logger.warn({ error }, 'failed to copy file from fallback')
       throw error

From 9e0b378948d12c5bd137fc41d2baafe445a10f53 Mon Sep 17 00:00:00 2001
From: Simon Detheridge <s@sd.ai>
Date: Mon, 3 Feb 2020 15:55:17 +0000
Subject: [PATCH 05/12] Remove minipass as dependency and refactor to make
 things clearer

---
 .../filestore/app/js/MigrationPersistor.js    | 185 +++++++++---------
 services/filestore/app/js/S3Persistor.js      |  20 +-
 .../test/acceptance/js/FilestoreTests.js      |  71 +++++--
 .../test/unit/js/MigrationPersistorTests.js   |  31 +--
 .../test/unit/js/S3PersistorTests.js          |  33 ++--
 5 files changed, 194 insertions(+), 146 deletions(-)

diff --git a/services/filestore/app/js/MigrationPersistor.js b/services/filestore/app/js/MigrationPersistor.js
index 3a4789693b..3ddc762922 100644
--- a/services/filestore/app/js/MigrationPersistor.js
+++ b/services/filestore/app/js/MigrationPersistor.js
@@ -1,10 +1,12 @@
 const metrics = require('metrics-sharelatex')
 const Settings = require('settings-sharelatex')
 const logger = require('logger-sharelatex')
-const Minipass = require('minipass')
-const { callbackify } = require('util')
+const Stream = require('stream')
+const { callbackify, promisify } = require('util')
 const { NotFoundError, WriteError } = require('./Errors')
 
+const pipeline = promisify(Stream.pipeline)
+
 // Persistor that wraps two other persistors. Talks to the 'primary' by default,
 // but will fall back to an older persistor in the case of a not-found error.
 // If `Settings.filestore.fallback.copyOnMiss` is set, this will copy files from the fallback
@@ -29,14 +31,86 @@ module.exports = function(primary, fallback) {
     }
   }
 
+  async function getFileStreamWithFallback(bucket, key, opts) {
+    const shouldCopy =
+      Settings.filestore.fallback.copyOnMiss && !opts.start && !opts.end
+
+    try {
+      return await primary.promises.getFileStream(bucket, key, opts)
+    } catch (err) {
+      if (err instanceof NotFoundError) {
+        const fallbackBucket = _getFallbackBucket(bucket)
+        const fallbackStream = await fallback.promises.getFileStream(
+          fallbackBucket,
+          key,
+          opts
+        )
+        // tee the stream to the client, and as a copy to the primary (if necessary)
+        // start listening on both straight away so that we don't consume bytes
+        // in one place before the other
+        const returnStream = new Stream.PassThrough()
+        pipeline(fallbackStream, returnStream)
+
+        if (shouldCopy) {
+          const copyStream = new Stream.PassThrough()
+          pipeline(fallbackStream, copyStream)
+
+          _copyStreamFromFallbackAndVerify(
+            copyStream,
+            fallbackBucket,
+            bucket,
+            key,
+            key
+          ).catch(() => {
+            // swallow errors, as this runs in the background and will log a warning
+          })
+        }
+        return returnStream
+      }
+      throw err
+    }
+  }
+
   async function copyFileWithFallback(bucket, sourceKey, destKey) {
     try {
       return await primary.promises.copyFile(bucket, sourceKey, destKey)
     } catch (err) {
       if (err instanceof NotFoundError) {
         const fallbackBucket = _getFallbackBucket(bucket)
-        return _copyFileFromFallback(fallbackBucket, bucket, sourceKey, destKey)
+        const fallbackStream = await fallback.promises.getFileStream(
+          fallbackBucket,
+          sourceKey,
+          {}
+        )
+
+        const copyStream = new Stream.PassThrough()
+        pipeline(fallbackStream, copyStream)
+
+        if (Settings.filestore.fallback.copyOnMiss) {
+          const missStream = new Stream.PassThrough()
+          pipeline(fallbackStream, missStream)
+
+          // copy from sourceKey -> sourceKey
+          _copyStreamFromFallbackAndVerify(
+            missStream,
+            fallbackBucket,
+            bucket,
+            sourceKey,
+            sourceKey
+          ).then(() => {
+            // swallow errors, as this runs in the background and will log a warning
+          })
+        }
+        // copy from sourceKey -> destKey
+        return _copyStreamFromFallbackAndVerify(
+          copyStream,
+          fallbackBucket,
+          bucket,
+          sourceKey,
+          destKey
+        )
       }
+      throw err
     }
   }
 
@@ -44,20 +118,29 @@ module.exports = function(primary, fallback) {
     return Settings.filestore.fallback.buckets[bucket]
   }
 
-  function _wrapFallbackMethod(method, enableCopy = true) {
+  function _wrapFallbackMethod(method) {
     return async function(bucket, key, ...moreArgs) {
       try {
         return await primary.promises[method](bucket, key, ...moreArgs)
       } catch (err) {
         if (err instanceof NotFoundError) {
           const fallbackBucket = _getFallbackBucket(bucket)
-          if (Settings.filestore.fallback.copyOnMiss && enableCopy) {
-            // run in background
-            _copyFileFromFallback(fallbackBucket, bucket, key, key).catch(
-              err => {
-                logger.warn({ err }, 'failed to copy file from fallback')
-              }
+          if (Settings.filestore.fallback.copyOnMiss) {
+            const fallbackStream = await fallback.promises.getFileStream(
+              fallbackBucket,
+              key,
+              {}
             )
+            // run in background
+            _copyStreamFromFallbackAndVerify(
+              fallbackStream,
+              fallbackBucket,
+              bucket,
+              key,
+              key
+            ).catch(err => {
+              logger.warn({ err }, 'failed to copy file from fallback')
+            })
           }
           return fallback.promises[method](fallbackBucket, key, ...moreArgs)
         }
@@ -66,32 +149,7 @@ module.exports = function(primary, fallback) {
     }
   }
 
-  async function _getFileStreamAndCopyIfRequired(bucketName, key, opts) {
-    const shouldCopy =
-      Settings.filestore.fallback.copyOnMiss && !opts.start && !opts.end
-
-    try {
-      return await primary.promises.getFileStream(bucketName, key, opts)
-    } catch (err) {
-      if (err instanceof NotFoundError) {
-        const fallbackBucket = _getFallbackBucket(bucketName)
-        if (shouldCopy) {
-          return _copyFileFromFallback(
-            fallbackBucket,
-            bucketName,
-            key,
-            key,
-            true
-          )
-        } else {
-          return fallback.promises.getFileStream(fallbackBucket, key, opts)
-        }
-      }
-      throw err
-    }
-  }
-
-  async function _copyFromFallbackStreamAndVerify(
+  async function _copyStreamFromFallbackAndVerify(
     stream,
     sourceBucket,
     destBucket,
@@ -139,63 +197,12 @@ module.exports = function(primary, fallback) {
     }
   }
 
-  async function _copyFileFromFallback(
-    sourceBucket,
-    destBucket,
-    sourceKey,
-    destKey,
-    returnStream = false
-  ) {
-    metrics.inc('fallback.copy')
-    const sourceStream = await fallback.promises.getFileStream(
-      sourceBucket,
-      sourceKey,
-      {}
-    )
-
-    if (!returnStream) {
-      return _copyFromFallbackStreamAndVerify(
-        sourceStream,
-        sourceBucket,
-        destBucket,
-        sourceKey,
-        destKey
-      )
-    }
-
-    const tee = new Minipass()
-    const clientStream = new Minipass()
-    const copyStream = new Minipass()
-
-    tee.pipe(clientStream)
-    tee.pipe(copyStream)
-
-    // copy the file in the background
-    _copyFromFallbackStreamAndVerify(
-      copyStream,
-      sourceBucket,
-      destBucket,
-      sourceKey,
-      destKey
-    ).catch(
-      // the error handler in this method will log a metric and a warning, so
-      // we don't need to do anything extra here, but catching it will prevent
-      // unhandled promise rejection warnings
-      () => {}
-    )
-
-    // start piping the source stream into the tee after everything is set up,
-    // otherwise one stream may consume bytes that don't arrive at the other
-    sourceStream.pipe(tee)
-    return clientStream
-  }
-
   return {
     primaryPersistor: primary,
     fallbackPersistor: fallback,
     sendFile: primary.sendFile,
     sendStream: primary.sendStream,
-    getFileStream: callbackify(_getFileStreamAndCopyIfRequired),
+    getFileStream: callbackify(getFileStreamWithFallback),
     getFileMd5Hash: callbackify(_wrapFallbackMethod('getFileMd5Hash')),
     deleteDirectory: callbackify(
       _wrapMethodOnBothPersistors('deleteDirectory')
@@ -208,7 +215,7 @@ module.exports = function(primary, fallback) {
     promises: {
       sendFile: primary.promises.sendFile,
       sendStream: primary.promises.sendStream,
-      getFileStream: _getFileStreamAndCopyIfRequired,
+      getFileStream: getFileStreamWithFallback,
       getFileMd5Hash: _wrapFallbackMethod('getFileMd5Hash'),
       deleteDirectory: _wrapMethodOnBothPersistors('deleteDirectory'),
       getFileSize: _wrapFallbackMethod('getFileSize'),
diff --git a/services/filestore/app/js/S3Persistor.js b/services/filestore/app/js/S3Persistor.js
index ef465da25c..a10251a642 100644
--- a/services/filestore/app/js/S3Persistor.js
+++ b/services/filestore/app/js/S3Persistor.js
@@ -7,13 +7,13 @@ const settings = require('settings-sharelatex')
 const metrics = require('metrics-sharelatex')
 const logger = require('logger-sharelatex')
 
-const Minipass = require('minipass')
 const meter = require('stream-meter')
+const Stream = require('stream')
 const crypto = require('crypto')
 const fs = require('fs')
 const S3 = require('aws-sdk/clients/s3')
 const { URL } = require('url')
-const { callbackify } = require('util')
+const { callbackify, promisify } = require('util')
 const {
   WriteError,
   ReadError,
@@ -46,6 +46,8 @@ module.exports = {
   }
 }
 
+const pipeline = promisify(Stream.pipeline)
+
 function hexToBase64(hex) {
   return Buffer.from(hex, 'hex').toString('base64')
 }
@@ -68,7 +70,6 @@ async function sendFile(bucketName, key, fsPath) {
 async function sendStream(bucketName, key, readStream, sourceMd5) {
   try {
     // if there is no supplied md5 hash, we calculate the hash as the data passes through
-    const passthroughStream = new Minipass()
     let hashPromise
     let b64Hash
 
@@ -77,29 +78,24 @@ async function sendStream(bucketName, key, readStream, sourceMd5) {
     } else {
       const hash = crypto.createHash('md5')
       hash.setEncoding('hex')
-      passthroughStream.pipe(hash)
+      pipeline(readStream, hash)
       hashPromise = new Promise((resolve, reject) => {
-        passthroughStream.on('end', () => {
+        readStream.on('end', () => {
           hash.end()
           resolve(hash.read())
         })
-        passthroughStream.on('error', err => {
+        readStream.on('error', err => {
           reject(err)
         })
       })
     }
 
     const meteredStream = meter()
-    passthroughStream.pipe(meteredStream)
     meteredStream.on('finish', () => {
       metrics.count('s3.egress', meteredStream.bytes)
     })
 
-    // pipe the readstream through minipass, which can write to both the metered
-    // stream (which goes on to S3) and the md5 generator if necessary
-    // - we do this last so that a listener streams does not consume data meant
-    // for both destinations
-    readStream.pipe(passthroughStream)
+    pipeline(readStream, meteredStream)
 
     // if we have an md5 hash, pass this to S3 to verify the upload
     const uploadOptions = {
diff --git a/services/filestore/test/acceptance/js/FilestoreTests.js b/services/filestore/test/acceptance/js/FilestoreTests.js
index 1c96445a3a..1d927618e5 100644
--- a/services/filestore/test/acceptance/js/FilestoreTests.js
+++ b/services/filestore/test/acceptance/js/FilestoreTests.js
@@ -497,16 +497,16 @@ describe('Filestore', function() {
               )
             })
 
-            it('should fetch the file', async function() {
-              const res = await rp.get(fileUrl)
-              expect(res.body).to.equal(constantFileContent)
-            })
-
             describe('when copyOnMiss is disabled', function() {
               beforeEach(function() {
                 Settings.filestore.fallback.copyOnMiss = false
               })
 
+              it('should fetch the file', async function() {
+                const res = await rp.get(fileUrl)
+                expect(res.body).to.equal(constantFileContent)
+              })
+
               it('should not copy the file to the primary', async function() {
                 await rp.get(fileUrl)
 
@@ -523,6 +523,11 @@ describe('Filestore', function() {
                 Settings.filestore.fallback.copyOnMiss = true
               })
 
+              it('should fetch the file', async function() {
+                const res = await rp.get(fileUrl)
+                expect(res.body).to.equal(constantFileContent)
+              })
+
               it('copies the file to the primary', async function() {
                 await rp.get(fileUrl)
                 // wait for the file to copy in the background
@@ -578,21 +583,51 @@ describe('Filestore', function() {
                 )
               })
 
-              it('should not copy the old file to the new bucket', async function() {
-                await expectPersistorNotToHaveFile(
-                  app.persistor.primaryPersistor,
-                  bucket,
-                  fileKey
-                )
+              describe('when copyOnMiss is false', function() {
+                beforeEach(function() {
+                  Settings.filestore.fallback.copyOnMiss = false
+                })
+
+                it('should create a new file in the new bucket', async function() {
+                  await expectPersistorToHaveFile(
+                    app.persistor.primaryPersistor,
+                    bucket,
+                    newFileKey,
+                    constantFileContent
+                  )
+                })
+
+                it('should not copy the old file to the primary with the old key', async function() {
+                  await expectPersistorNotToHaveFile(
+                    app.persistor.primaryPersistor,
+                    bucket,
+                    fileKey
+                  )
+                })
               })
 
-              it('should create a new file in the new bucket', async function() {
-                await expectPersistorToHaveFile(
-                  app.persistor.primaryPersistor,
-                  bucket,
-                  newFileKey,
-                  constantFileContent
-                )
+              describe('when copyOnMiss is true', function() {
+                beforeEach(function() {
+                  Settings.filestore.fallback.copyOnMiss = true
+                })
+
+                it('should create a new file in the new bucket', async function() {
+                  await expectPersistorToHaveFile(
+                    app.persistor.primaryPersistor,
+                    bucket,
+                    newFileKey,
+                    constantFileContent
+                  )
+                })
+
+                it('should copy the old file to the primary with the old key', async function() {
+                  await expectPersistorToHaveFile(
+                    app.persistor.primaryPersistor,
+                    bucket,
+                    fileKey,
+                    constantFileContent
+                  )
+                })
               })
             })
           })
diff --git a/services/filestore/test/unit/js/MigrationPersistorTests.js b/services/filestore/test/unit/js/MigrationPersistorTests.js
index 83159f38ad..db8401c78c 100644
--- a/services/filestore/test/unit/js/MigrationPersistorTests.js
+++ b/services/filestore/test/unit/js/MigrationPersistorTests.js
@@ -26,8 +26,8 @@ describe('MigrationPersistorTests', function() {
   let Metrics,
     Settings,
     Logger,
+    Stream,
     MigrationPersistor,
-    Minipass,
     fileStream,
     newPersistor
 
@@ -82,24 +82,22 @@ describe('MigrationPersistorTests', function() {
       inc: sinon.stub()
     }
 
+    Stream = {
+      pipeline: sinon.stub().yields(),
+      PassThrough: sinon.stub()
+    }
+
     Logger = {
       warn: sinon.stub()
     }
 
-    Minipass = sinon.stub()
-    Minipass.prototype.on = sinon
-      .stub()
-      .withArgs('end')
-      .yields()
-    Minipass.prototype.pipe = sinon.stub()
-
     MigrationPersistor = SandboxedModule.require(modulePath, {
       requires: {
         'settings-sharelatex': Settings,
+        stream: Stream,
         './Errors': Errors,
         'metrics-sharelatex': Metrics,
-        'logger-sharelatex': Logger,
-        minipass: Minipass
+        'logger-sharelatex': Logger
       },
       globals: { console }
     })
@@ -155,7 +153,7 @@ describe('MigrationPersistorTests', function() {
       })
 
       it('should return the file stream', function() {
-        expect(response).to.equal(fileStream)
+        expect(response).to.be.an.instanceOf(Stream.PassThrough)
       })
 
       it('should fetch the file from the primary persistor with the correct options', function() {
@@ -215,13 +213,13 @@ describe('MigrationPersistorTests', function() {
         ).to.have.been.calledWithExactly(
           bucket,
           key,
-          sinon.match.instanceOf(Minipass),
+          sinon.match.instanceOf(Stream.PassThrough),
           md5
         )
       })
 
       it('should send a stream to the client', function() {
-        expect(returnedStream).to.be.an.instanceOf(Minipass)
+        expect(returnedStream).to.be.an.instanceOf(Stream.PassThrough)
       })
     })
 
@@ -476,7 +474,12 @@ describe('MigrationPersistorTests', function() {
       it('should send the file to the primary', function() {
         expect(
           primaryPersistor.promises.sendStream
-        ).to.have.been.calledWithExactly(bucket, destKey, fileStream, md5)
+        ).to.have.been.calledWithExactly(
+          bucket,
+          destKey,
+          sinon.match.instanceOf(Stream.PassThrough),
+          md5
+        )
       })
     })
 
diff --git a/services/filestore/test/unit/js/S3PersistorTests.js b/services/filestore/test/unit/js/S3PersistorTests.js
index 4f700c8797..b9711572c2 100644
--- a/services/filestore/test/unit/js/S3PersistorTests.js
+++ b/services/filestore/test/unit/js/S3PersistorTests.js
@@ -35,6 +35,7 @@ describe('S3PersistorTests', function() {
     Meter,
     MeteredStream,
     ReadStream,
+    Stream,
     S3Persistor,
     S3Client,
     S3ReadStream,
@@ -43,7 +44,6 @@ describe('S3PersistorTests', function() {
     FileNotFoundError,
     EmptyPromise,
     settings,
-    Minipass,
     Hash,
     crypto
 
@@ -61,6 +61,10 @@ describe('S3PersistorTests', function() {
       }
     }
 
+    Stream = {
+      pipeline: sinon.stub().yields()
+    }
+
     EmptyPromise = {
       promise: sinon.stub().resolves()
     }
@@ -70,7 +74,11 @@ describe('S3PersistorTests', function() {
     }
 
     ReadStream = {
-      pipe: sinon.stub().returns('readStream')
+      pipe: sinon.stub().returns('readStream'),
+      on: sinon
+        .stub()
+        .withArgs('end')
+        .yields()
     }
 
     FileNotFoundError = new Error('File not found')
@@ -132,13 +140,6 @@ describe('S3PersistorTests', function() {
       createHash: sinon.stub().returns(Hash)
     }
 
-    Minipass = sinon.stub()
-    Minipass.prototype.on = sinon
-      .stub()
-      .withArgs('end')
-      .yields()
-    Minipass.prototype.pipe = sinon.stub()
-
     Logger = {
       warn: sinon.stub()
     }
@@ -151,8 +152,8 @@ describe('S3PersistorTests', function() {
         './Errors': Errors,
         fs: Fs,
         'stream-meter': Meter,
+        stream: Stream,
         'metrics-sharelatex': Metrics,
-        minipass: Minipass,
         crypto
       },
       globals: { console }
@@ -456,7 +457,10 @@ describe('S3PersistorTests', function() {
       })
 
       it('should meter the stream', function() {
-        expect(Minipass.prototype.pipe).to.have.been.calledWith(MeteredStream)
+        expect(Stream.pipeline).to.have.been.calledWith(
+          ReadStream,
+          MeteredStream
+        )
       })
 
       it('should record an egress metric', function() {
@@ -464,7 +468,7 @@ describe('S3PersistorTests', function() {
       })
 
       it('calculates the md5 hash of the file', function() {
-        expect(Minipass.prototype.pipe).to.have.been.calledWith(Hash)
+        expect(Stream.pipeline).to.have.been.calledWith(ReadStream, Hash)
       })
     })
 
@@ -479,7 +483,10 @@ describe('S3PersistorTests', function() {
       })
 
       it('should not calculate the md5 hash of the file', function() {
-        expect(Minipass.prototype.pipe).not.to.have.been.calledWith(Hash)
+        expect(Stream.pipeline).not.to.have.been.calledWith(
+          sinon.match.any,
+          Hash
+        )
       })
 
       it('sends the hash in base64', function() {

From 42a5d168dfce029e1978811a84c9f51093936c0b Mon Sep 17 00:00:00 2001
From: Simon Detheridge <s@sd.ai>
Date: Mon, 3 Feb 2020 15:55:28 +0000
Subject: [PATCH 06/12] Remove unused packages

---
 services/filestore/npm-shrinkwrap.json | 291 -------------------------
 services/filestore/package.json        |   6 -
 2 files changed, 297 deletions(-)

diff --git a/services/filestore/npm-shrinkwrap.json b/services/filestore/npm-shrinkwrap.json
index a4206a94e0..bdc836e237 100644
--- a/services/filestore/npm-shrinkwrap.json
+++ b/services/filestore/npm-shrinkwrap.json
@@ -606,11 +606,6 @@
         "event-target-shim": "^5.0.0"
       }
     },
-    "accept-encoding": {
-      "version": "0.1.0",
-      "resolved": "https://registry.npmjs.org/accept-encoding/-/accept-encoding-0.1.0.tgz",
-      "integrity": "sha1-XdiLjfcfHcLlzGuVZezOHjmaMz4="
-    },
     "accepts": {
       "version": "1.3.5",
       "resolved": "https://registry.npmjs.org/accepts/-/accepts-1.3.5.tgz",
@@ -790,11 +785,6 @@
         }
       }
     },
-    "aws-sign": {
-      "version": "0.2.1",
-      "resolved": "https://registry.npmjs.org/aws-sign/-/aws-sign-0.2.1.tgz",
-      "integrity": "sha1-uWGyLwuqTxXsJBFA83dtbBQoVtA="
-    },
     "aws-sign2": {
       "version": "0.7.0",
       "resolved": "https://registry.npmjs.org/aws-sign2/-/aws-sign2-0.7.0.tgz",
@@ -857,14 +847,6 @@
         "tweetnacl": "^0.14.3"
       }
     },
-    "best-encoding": {
-      "version": "0.1.1",
-      "resolved": "https://registry.npmjs.org/best-encoding/-/best-encoding-0.1.1.tgz",
-      "integrity": "sha1-GVIT2rysBFgYuAe3ox+Dn63cl04=",
-      "requires": {
-        "accept-encoding": "~0.1.0"
-      }
-    },
     "bignumber.js": {
       "version": "7.2.1",
       "resolved": "https://registry.npmjs.org/bignumber.js/-/bignumber.js-7.2.1.tgz",
@@ -883,14 +865,6 @@
       "resolved": "https://registry.npmjs.org/bintrees/-/bintrees-1.0.1.tgz",
       "integrity": "sha1-DmVcm5wkNeqraL9AJyJtK1WjRSQ="
     },
-    "bl": {
-      "version": "0.7.0",
-      "resolved": "https://registry.npmjs.org/bl/-/bl-0.7.0.tgz",
-      "integrity": "sha1-P7BnBgKsKHjrdw3CA58YNr5irls=",
-      "requires": {
-        "readable-stream": "~1.0.2"
-      }
-    },
     "body-parser": {
       "version": "1.18.3",
       "resolved": "https://registry.npmjs.org/body-parser/-/body-parser-1.18.3.tgz",
@@ -914,14 +888,6 @@
       "integrity": "sha1-tcCeF8rNET0Rt7s+04TMASmU2Gs=",
       "dev": true
     },
-    "boom": {
-      "version": "0.3.8",
-      "resolved": "https://registry.npmjs.org/boom/-/boom-0.3.8.tgz",
-      "integrity": "sha1-yM2wQUNZEnQWKMBE7Mcy0dF8Ceo=",
-      "requires": {
-        "hoek": "0.7.x"
-      }
-    },
     "brace-expansion": {
       "version": "1.1.11",
       "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-1.1.11.tgz",
@@ -995,11 +961,6 @@
         "quick-lru": "^4.0.1"
       }
     },
-    "caseless": {
-      "version": "0.3.0",
-      "resolved": "https://registry.npmjs.org/caseless/-/caseless-0.3.0.tgz",
-      "integrity": "sha1-U06XkWOH07cGtk/eu6xGQ4RQk08="
-    },
     "chai": {
       "version": "4.2.0",
       "resolved": "https://registry.npmjs.org/chai/-/chai-4.2.0.tgz",
@@ -1117,14 +1078,6 @@
       "integrity": "sha1-p9BVi9icQveV3UIyj3QIMcpTvCU=",
       "dev": true
     },
-    "combined-stream": {
-      "version": "0.0.7",
-      "resolved": "https://registry.npmjs.org/combined-stream/-/combined-stream-0.0.7.tgz",
-      "integrity": "sha1-ATfmV7qlp1QcV6w3rF/AfXO03B8=",
-      "requires": {
-        "delayed-stream": "0.0.5"
-      }
-    },
     "common-tags": {
       "version": "1.8.0",
       "resolved": "https://registry.npmjs.org/common-tags/-/common-tags-1.8.0.tgz",
@@ -1171,11 +1124,6 @@
       "resolved": "https://registry.npmjs.org/cookie/-/cookie-0.3.1.tgz",
       "integrity": "sha1-5+Ch+e9DtMi6klxcWpboBtFoc7s="
     },
-    "cookie-jar": {
-      "version": "0.2.0",
-      "resolved": "https://registry.npmjs.org/cookie-jar/-/cookie-jar-0.2.0.tgz",
-      "integrity": "sha1-ZOzAasl423leS1KQy+SLo3gUAPo="
-    },
     "cookie-signature": {
       "version": "1.0.6",
       "resolved": "https://registry.npmjs.org/cookie-signature/-/cookie-signature-1.0.6.tgz",
@@ -1213,14 +1161,6 @@
         }
       }
     },
-    "cryptiles": {
-      "version": "0.1.3",
-      "resolved": "https://registry.npmjs.org/cryptiles/-/cryptiles-0.1.3.tgz",
-      "integrity": "sha1-GlVnNPBtJLo0hirpy55wmjr7/xw=",
-      "requires": {
-        "boom": "0.3.x"
-      }
-    },
     "dashdash": {
       "version": "1.14.1",
       "resolved": "https://registry.npmjs.org/dashdash/-/dashdash-1.14.1.tgz",
@@ -1272,11 +1212,6 @@
       "resolved": "https://registry.npmjs.org/delay/-/delay-4.3.0.tgz",
       "integrity": "sha1-7+6/uPVFV5yzlrOnIkQ+yW0UxQ4="
     },
-    "delayed-stream": {
-      "version": "0.0.5",
-      "resolved": "https://registry.npmjs.org/delayed-stream/-/delayed-stream-0.0.5.tgz",
-      "integrity": "sha1-1LH0OpPoKW3+AmlPRoC8N6MTxz8="
-    },
     "depd": {
       "version": "1.1.2",
       "resolved": "https://registry.npmjs.org/depd/-/depd-1.1.2.tgz",
@@ -2078,28 +2013,6 @@
         }
       }
     },
-    "forever-agent": {
-      "version": "0.2.0",
-      "resolved": "https://registry.npmjs.org/forever-agent/-/forever-agent-0.2.0.tgz",
-      "integrity": "sha1-4cJcetROCcOPIzh2x2/MJP+EOx8="
-    },
-    "form-data": {
-      "version": "0.0.10",
-      "resolved": "https://registry.npmjs.org/form-data/-/form-data-0.0.10.tgz",
-      "integrity": "sha1-2zRaU3jYau6x7V1VO4aawZLS9e0=",
-      "requires": {
-        "async": "~0.2.7",
-        "combined-stream": "~0.0.4",
-        "mime": "~1.2.2"
-      },
-      "dependencies": {
-        "mime": {
-          "version": "1.2.11",
-          "resolved": "https://registry.npmjs.org/mime/-/mime-1.2.11.tgz",
-          "integrity": "sha1-WCA+7Ybjpe8XrtK32evUfwpg3RA="
-        }
-      }
-    },
     "forwarded": {
       "version": "0.1.2",
       "resolved": "https://registry.npmjs.org/forwarded/-/forwarded-0.1.2.tgz",
@@ -2323,17 +2236,6 @@
       "integrity": "sha512-PLcsoqu++dmEIZB+6totNFKq/7Do+Z0u4oT0zKOJNl3lYK6vGwwu2hjHs+68OEZbTjiUE9bgOABXbP/GvrS0Kg==",
       "dev": true
     },
-    "hawk": {
-      "version": "0.10.2",
-      "resolved": "https://registry.npmjs.org/hawk/-/hawk-0.10.2.tgz",
-      "integrity": "sha1-mzYd7pWpMWQObVBOBWCaj8OsRdI=",
-      "requires": {
-        "boom": "0.3.x",
-        "cryptiles": "0.1.x",
-        "hoek": "0.7.x",
-        "sntp": "0.1.x"
-      }
-    },
     "he": {
       "version": "1.1.1",
       "resolved": "https://registry.npmjs.org/he/-/he-1.1.1.tgz",
@@ -2349,11 +2251,6 @@
       "resolved": "https://registry.npmjs.org/hex2dec/-/hex2dec-1.1.2.tgz",
       "integrity": "sha1-jhzkvvNqdPfVcjw/swkMKGAHczg="
     },
-    "hoek": {
-      "version": "0.7.6",
-      "resolved": "https://registry.npmjs.org/hoek/-/hoek-0.7.6.tgz",
-      "integrity": "sha1-YPvZBFV1Qc0rh5Wr8wihs3cOFVo="
-    },
     "hosted-git-info": {
       "version": "2.8.5",
       "resolved": "https://registry.npmjs.org/hosted-git-info/-/hosted-git-info-2.8.5.tgz",
@@ -2701,28 +2598,6 @@
         "graceful-fs": "^4.1.9"
       }
     },
-    "knox": {
-      "version": "0.9.2",
-      "resolved": "https://registry.npmjs.org/knox/-/knox-0.9.2.tgz",
-      "integrity": "sha1-NzZZNmniTwJP2vcjtqHcSv2DmnE=",
-      "requires": {
-        "debug": "^1.0.2",
-        "mime": "*",
-        "once": "^1.3.0",
-        "stream-counter": "^1.0.0",
-        "xml2js": "^0.4.4"
-      },
-      "dependencies": {
-        "debug": {
-          "version": "1.0.5",
-          "resolved": "https://registry.npmjs.org/debug/-/debug-1.0.5.tgz",
-          "integrity": "sha1-9yQSF0MPmd7EwrRz6rkiKOh0wqw=",
-          "requires": {
-            "ms": "2.0.0"
-          }
-        }
-      }
-    },
     "levn": {
       "version": "0.3.0",
       "resolved": "https://registry.npmjs.org/levn/-/levn-0.3.0.tgz",
@@ -3129,21 +3004,6 @@
       "resolved": "https://registry.npmjs.org/minimist/-/minimist-0.0.8.tgz",
       "integrity": "sha1-hX/Kv8M5fSYluCKCYuhqp6ARsF0="
     },
-    "minipass": {
-      "version": "3.1.1",
-      "resolved": "https://registry.npmjs.org/minipass/-/minipass-3.1.1.tgz",
-      "integrity": "sha512-UFqVihv6PQgwj8/yTGvl9kPz7xIAY+R5z6XYjRInD3Gk3qx6QGSD6zEcpeG4Dy/lQnv1J6zv8ejV90hyYIKf3w==",
-      "requires": {
-        "yallist": "^4.0.0"
-      },
-      "dependencies": {
-        "yallist": {
-          "version": "4.0.0",
-          "resolved": "https://registry.npmjs.org/yallist/-/yallist-4.0.0.tgz",
-          "integrity": "sha512-3wdGidZyq5PB084XLES5TpOSRA3wjXAlIWMhum2kRcv/41Sn2emQ0dycQW4uZXLejwKvg6EsvbdlVL+FYEct7A=="
-        }
-      }
-    },
     "mkdirp": {
       "version": "0.5.1",
       "resolved": "https://registry.npmjs.org/mkdirp/-/mkdirp-0.5.1.tgz",
@@ -3368,55 +3228,6 @@
       "resolved": "https://registry.npmjs.org/node-forge/-/node-forge-0.8.4.tgz",
       "integrity": "sha1-1nOGYrZhvhnicR7wGqOxghLxMDA="
     },
-    "node-transloadit": {
-      "version": "0.0.4",
-      "resolved": "https://registry.npmjs.org/node-transloadit/-/node-transloadit-0.0.4.tgz",
-      "integrity": "sha1-4ZoHheON94NblO2AANHjXmg7zsE=",
-      "requires": {
-        "request": "~2.16.6",
-        "underscore": "1.2.1"
-      },
-      "dependencies": {
-        "json-stringify-safe": {
-          "version": "3.0.0",
-          "resolved": "https://registry.npmjs.org/json-stringify-safe/-/json-stringify-safe-3.0.0.tgz",
-          "integrity": "sha1-nbew5TDH8onF6MhDKvGRwv91pbM="
-        },
-        "mime": {
-          "version": "1.2.11",
-          "resolved": "https://registry.npmjs.org/mime/-/mime-1.2.11.tgz",
-          "integrity": "sha1-WCA+7Ybjpe8XrtK32evUfwpg3RA="
-        },
-        "qs": {
-          "version": "0.5.6",
-          "resolved": "https://registry.npmjs.org/qs/-/qs-0.5.6.tgz",
-          "integrity": "sha1-MbGtBYVnZRxSaSFQa5qHk5EaA4Q="
-        },
-        "request": {
-          "version": "2.16.6",
-          "resolved": "https://registry.npmjs.org/request/-/request-2.16.6.tgz",
-          "integrity": "sha1-hy/kRa5y3iZrN4edatfclI+gHK0=",
-          "requires": {
-            "aws-sign": "~0.2.0",
-            "cookie-jar": "~0.2.0",
-            "forever-agent": "~0.2.0",
-            "form-data": "~0.0.3",
-            "hawk": "~0.10.2",
-            "json-stringify-safe": "~3.0.0",
-            "mime": "~1.2.7",
-            "node-uuid": "~1.4.0",
-            "oauth-sign": "~0.2.0",
-            "qs": "~0.5.4",
-            "tunnel-agent": "~0.2.0"
-          }
-        },
-        "underscore": {
-          "version": "1.2.1",
-          "resolved": "https://registry.npmjs.org/underscore/-/underscore-1.2.1.tgz",
-          "integrity": "sha1-/FxrB2VnPZKi1KyLTcCqiHAuK9Q="
-        }
-      }
-    },
     "node-uuid": {
       "version": "1.4.8",
       "resolved": "https://registry.npmjs.org/node-uuid/-/node-uuid-1.4.8.tgz",
@@ -3442,11 +3253,6 @@
         }
       }
     },
-    "oauth-sign": {
-      "version": "0.2.0",
-      "resolved": "https://registry.npmjs.org/oauth-sign/-/oauth-sign-0.2.0.tgz",
-      "integrity": "sha1-oOahcV2u0GLzIrYit/5a/RA1tuI="
-    },
     "object-inspect": {
       "version": "1.7.0",
       "resolved": "https://registry.npmjs.org/object-inspect/-/object-inspect-1.7.0.tgz",
@@ -4461,29 +4267,6 @@
         "read-pkg": "^2.0.0"
       }
     },
-    "readable-stream": {
-      "version": "1.0.34",
-      "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-1.0.34.tgz",
-      "integrity": "sha1-Elgg40vIQtLyqq+v5MKRbuMsFXw=",
-      "requires": {
-        "core-util-is": "~1.0.0",
-        "inherits": "~2.0.1",
-        "isarray": "0.0.1",
-        "string_decoder": "~0.10.x"
-      },
-      "dependencies": {
-        "isarray": {
-          "version": "0.0.1",
-          "resolved": "https://registry.npmjs.org/isarray/-/isarray-0.0.1.tgz",
-          "integrity": "sha1-ihis/Kmo9Bd+Cav8YDiTmwXR7t8="
-        }
-      }
-    },
-    "recluster": {
-      "version": "0.3.7",
-      "resolved": "https://registry.npmjs.org/recluster/-/recluster-0.3.7.tgz",
-      "integrity": "sha1-aKRx3ZC2obl3ZjTPdpZAWutWeJU="
-    },
     "regexpp": {
       "version": "2.0.1",
       "resolved": "https://registry.npmjs.org/regexpp/-/regexpp-2.0.1.tgz",
@@ -4663,24 +4446,6 @@
       "integrity": "sha512-pb/MYmXstAkysRFx8piNI1tGFNQIFA3vkE3Gq4EuA1dF6gHp/+vgZqsCGJapvy8N3Q+4o7FwvquPJcnZ7RYy4g==",
       "dev": true
     },
-    "response": {
-      "version": "0.14.0",
-      "resolved": "https://registry.npmjs.org/response/-/response-0.14.0.tgz",
-      "integrity": "sha1-BmNS/z5rAm0EdYCUB2Y7Rob9JpY=",
-      "requires": {
-        "best-encoding": "^0.1.1",
-        "bl": "~0.7.0",
-        "caseless": "^0.3.0",
-        "mime": "~1.2.11"
-      },
-      "dependencies": {
-        "mime": {
-          "version": "1.2.11",
-          "resolved": "https://registry.npmjs.org/mime/-/mime-1.2.11.tgz",
-          "integrity": "sha1-WCA+7Ybjpe8XrtK32evUfwpg3RA="
-        }
-      }
-    },
     "restore-cursor": {
       "version": "2.0.0",
       "resolved": "https://registry.npmjs.org/restore-cursor/-/restore-cursor-2.0.0.tgz",
@@ -4895,14 +4660,6 @@
         }
       }
     },
-    "sntp": {
-      "version": "0.1.4",
-      "resolved": "https://registry.npmjs.org/sntp/-/sntp-0.1.4.tgz",
-      "integrity": "sha1-XvSBuVGnspr/30r9fyaDj8ESD4Q=",
-      "requires": {
-        "hoek": "0.7.x"
-      }
-    },
     "source-map": {
       "version": "0.6.1",
       "resolved": "https://registry.npmjs.org/source-map/-/source-map-0.6.1.tgz",
@@ -4990,49 +4747,11 @@
       "resolved": "https://registry.npmjs.org/stealthy-require/-/stealthy-require-1.1.1.tgz",
       "integrity": "sha1-NbCYdbT/SfJqd35QmzCQoyJr8ks="
     },
-    "stream-browserify": {
-      "version": "2.0.1",
-      "resolved": "https://registry.npmjs.org/stream-browserify/-/stream-browserify-2.0.1.tgz",
-      "integrity": "sha1-ZiZu5fm9uZQKTkUUyvtDu3Hlyds=",
-      "requires": {
-        "inherits": "~2.0.1",
-        "readable-stream": "^2.0.2"
-      },
-      "dependencies": {
-        "readable-stream": {
-          "version": "2.3.6",
-          "resolved": "http://registry.npmjs.org/readable-stream/-/readable-stream-2.3.6.tgz",
-          "integrity": "sha1-sRwn2IuP8fvgcGQ8+UsMea4bCq8=",
-          "requires": {
-            "core-util-is": "~1.0.0",
-            "inherits": "~2.0.3",
-            "isarray": "~1.0.0",
-            "process-nextick-args": "~2.0.0",
-            "safe-buffer": "~5.1.1",
-            "string_decoder": "~1.1.1",
-            "util-deprecate": "~1.0.1"
-          }
-        },
-        "string_decoder": {
-          "version": "1.1.1",
-          "resolved": "https://registry.npmjs.org/string_decoder/-/string_decoder-1.1.1.tgz",
-          "integrity": "sha1-nPFhG6YmhdcDCunkujQUnDrwP8g=",
-          "requires": {
-            "safe-buffer": "~5.1.0"
-          }
-        }
-      }
-    },
     "stream-buffers": {
       "version": "0.2.6",
       "resolved": "https://registry.npmjs.org/stream-buffers/-/stream-buffers-0.2.6.tgz",
       "integrity": "sha1-GBwI1bs2kARfaUAbmuanoM8zE/w="
     },
-    "stream-counter": {
-      "version": "1.0.0",
-      "resolved": "https://registry.npmjs.org/stream-counter/-/stream-counter-1.0.0.tgz",
-      "integrity": "sha1-kc8lac5NxQYf6816yyY5SloRR1E="
-    },
     "stream-meter": {
       "version": "1.0.4",
       "resolved": "https://registry.npmjs.org/stream-meter/-/stream-meter-1.0.4.tgz",
@@ -5117,11 +4836,6 @@
         "function-bind": "^1.1.1"
       }
     },
-    "string_decoder": {
-      "version": "0.10.31",
-      "resolved": "https://registry.npmjs.org/string_decoder/-/string_decoder-0.10.31.tgz",
-      "integrity": "sha1-YuIDvEF2bGwoyfyEMB2rHFMQ+pQ="
-    },
     "strip-ansi": {
       "version": "5.2.0",
       "resolved": "https://registry.npmjs.org/strip-ansi/-/strip-ansi-5.2.0.tgz",
@@ -5321,11 +5035,6 @@
       "integrity": "sha512-qOebF53frne81cf0S9B41ByenJ3/IuH8yJKngAX35CmiZySA0khhkovshKK+jGCaMnVomla7gVlIcc3EvKPbTQ==",
       "dev": true
     },
-    "tunnel-agent": {
-      "version": "0.2.0",
-      "resolved": "https://registry.npmjs.org/tunnel-agent/-/tunnel-agent-0.2.0.tgz",
-      "integrity": "sha1-aFPCr7GyEJ5FYp5JK9419Fnqaeg="
-    },
     "tweetnacl": {
       "version": "0.14.5",
       "resolved": "https://registry.npmjs.org/tweetnacl/-/tweetnacl-0.14.5.tgz",
diff --git a/services/filestore/package.json b/services/filestore/package.json
index 2e9cef8aa0..6f1dde0e8a 100644
--- a/services/filestore/package.json
+++ b/services/filestore/package.json
@@ -28,22 +28,16 @@
     "fs-extra": "^1.0.0",
     "glob": "^7.1.6",
     "heapdump": "^0.3.2",
-    "knox": "~0.9.1",
     "logger-sharelatex": "^1.7.0",
     "metrics-sharelatex": "^2.2.0",
-    "minipass": "^3.1.1",
     "mocha": "5.2.0",
-    "node-transloadit": "0.0.4",
     "node-uuid": "~1.4.1",
     "pngcrush": "0.0.3",
     "range-parser": "^1.0.2",
-    "recluster": "^0.3.7",
     "request": "^2.88.0",
     "request-promise-native": "^1.0.8",
-    "response": "0.14.0",
     "rimraf": "2.2.8",
     "settings-sharelatex": "^1.1.0",
-    "stream-browserify": "^2.0.1",
     "stream-buffers": "~0.2.5",
     "stream-meter": "^1.0.4",
     "underscore": "~1.5.2"

From f4a16cd97230127ca5decf82fdd6763403b74f74 Mon Sep 17 00:00:00 2001
From: Simon Detheridge <s@sd.ai>
Date: Mon, 3 Feb 2020 16:10:10 +0000
Subject: [PATCH 07/12] Update tests to properly check for copied files

---
 .../test/acceptance/js/FilestoreTests.js      | 74 +++++++++++++------
 1 file changed, 50 insertions(+), 24 deletions(-)

diff --git a/services/filestore/test/acceptance/js/FilestoreTests.js b/services/filestore/test/acceptance/js/FilestoreTests.js
index 1d927618e5..54e9d457ce 100644
--- a/services/filestore/test/acceptance/js/FilestoreTests.js
+++ b/services/filestore/test/acceptance/js/FilestoreTests.js
@@ -543,7 +543,7 @@ describe('Filestore', function() {
             })
 
             describe('when copying a file', function() {
-              let newFileId, newFileUrl, newFileKey
+              let newFileId, newFileUrl, newFileKey, opts
 
               beforeEach(async function() {
                 const newProjectID = `acceptance_tests_copied_project_${Math.random()}`
@@ -551,7 +551,7 @@ describe('Filestore', function() {
                 newFileUrl = `${filestoreUrl}/project/${newProjectID}/file/${directoryName}%2F${newFileId}`
                 newFileKey = `${newProjectID}/${directoryName}/${newFileId}`
 
-                const opts = {
+                opts = {
                   method: 'put',
                   uri: newFileUrl,
                   json: {
@@ -561,31 +561,31 @@ describe('Filestore', function() {
                     }
                   }
                 }
-
-                const response = await rp(opts)
-                expect(response.statusCode).to.equal(200)
-              })
-
-              it('should leave the old file in the old bucket', async function() {
-                await expectPersistorToHaveFile(
-                  app.persistor.fallbackPersistor,
-                  fallbackBucket,
-                  fileKey,
-                  constantFileContent
-                )
-              })
-
-              it('should not create a new file in the old bucket', async function() {
-                await expectPersistorNotToHaveFile(
-                  app.persistor.fallbackPersistor,
-                  fallbackBucket,
-                  newFileKey
-                )
               })
 
               describe('when copyOnMiss is false', function() {
-                beforeEach(function() {
+                beforeEach(async function() {
                   Settings.filestore.fallback.copyOnMiss = false
+
+                  const response = await rp(opts)
+                  expect(response.statusCode).to.equal(200)
+                })
+
+                it('should leave the old file in the old bucket', async function() {
+                  await expectPersistorToHaveFile(
+                    app.persistor.fallbackPersistor,
+                    fallbackBucket,
+                    fileKey,
+                    constantFileContent
+                  )
+                })
+
+                it('should not create a new file in the old bucket', async function() {
+                  await expectPersistorNotToHaveFile(
+                    app.persistor.fallbackPersistor,
+                    fallbackBucket,
+                    newFileKey
+                  )
                 })
 
                 it('should create a new file in the new bucket', async function() {
@@ -598,6 +598,9 @@ describe('Filestore', function() {
                 })
 
                 it('should not copy the old file to the primary with the old key', async function() {
+                  // wait for the file to copy in the background
+                  await promisify(setTimeout)(1000)
+
                   await expectPersistorNotToHaveFile(
                     app.persistor.primaryPersistor,
                     bucket,
@@ -607,8 +610,28 @@ describe('Filestore', function() {
               })
 
               describe('when copyOnMiss is true', function() {
-                beforeEach(function() {
+                beforeEach(async function() {
                   Settings.filestore.fallback.copyOnMiss = true
+
+                  const response = await rp(opts)
+                  expect(response.statusCode).to.equal(200)
+                })
+
+                it('should leave the old file in the old bucket', async function() {
+                  await expectPersistorToHaveFile(
+                    app.persistor.fallbackPersistor,
+                    fallbackBucket,
+                    fileKey,
+                    constantFileContent
+                  )
+                })
+
+                it('should not create a new file in the old bucket', async function() {
+                  await expectPersistorNotToHaveFile(
+                    app.persistor.fallbackPersistor,
+                    fallbackBucket,
+                    newFileKey
+                  )
                 })
 
                 it('should create a new file in the new bucket', async function() {
@@ -621,6 +644,9 @@ describe('Filestore', function() {
                 })
 
                 it('should copy the old file to the primary with the old key', async function() {
+                  // wait for the file to copy in the background
+                  await promisify(setTimeout)(1000)
+
                   await expectPersistorToHaveFile(
                     app.persistor.primaryPersistor,
                     bucket,

From 6dcf35137786f8191158756553685d5c030168d8 Mon Sep 17 00:00:00 2001
From: Simon Detheridge <s@sd.ai>
Date: Mon, 3 Feb 2020 16:11:48 +0000
Subject: [PATCH 08/12] Remove unnecessary 'async'

---
 services/filestore/test/acceptance/js/FilestoreTests.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/services/filestore/test/acceptance/js/FilestoreTests.js b/services/filestore/test/acceptance/js/FilestoreTests.js
index 54e9d457ce..e8aac829a6 100644
--- a/services/filestore/test/acceptance/js/FilestoreTests.js
+++ b/services/filestore/test/acceptance/js/FilestoreTests.js
@@ -545,7 +545,7 @@ describe('Filestore', function() {
             describe('when copying a file', function() {
               let newFileId, newFileUrl, newFileKey, opts
 
-              beforeEach(async function() {
+              beforeEach(function() {
                 const newProjectID = `acceptance_tests_copied_project_${Math.random()}`
                 newFileId = Math.random()
                 newFileUrl = `${filestoreUrl}/project/${newProjectID}/file/${directoryName}%2F${newFileId}`

From 304fdfd35cdd63023454c7ec4b33086d0dee953c Mon Sep 17 00:00:00 2001
From: Simon Detheridge <s@sd.ai>
Date: Tue, 11 Feb 2020 16:39:03 +0000
Subject: [PATCH 09/12] Explicitly resume stream after adding listener

---
 services/filestore/test/acceptance/js/FilestoreTests.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/services/filestore/test/acceptance/js/FilestoreTests.js b/services/filestore/test/acceptance/js/FilestoreTests.js
index e8aac829a6..fd1baed474 100644
--- a/services/filestore/test/acceptance/js/FilestoreTests.js
+++ b/services/filestore/test/acceptance/js/FilestoreTests.js
@@ -36,6 +36,7 @@ function streamToString(stream) {
     stream.on('data', chunk => chunks.push(chunk))
     stream.on('error', reject)
     stream.on('end', () => resolve(Buffer.concat(chunks).toString('utf8')))
+    stream.resume()
   })
 }
 

From 93cd55fb79f4b595b3ea948444080688fe439fa3 Mon Sep 17 00:00:00 2001
From: Simon Detheridge <s@sd.ai>
Date: Wed, 29 Jan 2020 12:23:31 +0000
Subject: [PATCH 10/12] Refactor persistors to use a helper for common things

---
 services/filestore/app/js/FSPersistor.js      |  31 ++--
 services/filestore/app/js/PersistorHelper.js  | 114 ++++++++++++++
 services/filestore/app/js/S3Persistor.js      | 141 +++++++-----------
 .../test/unit/js/FSPersistorTests.js          |   5 +-
 .../test/unit/js/S3PersistorTests.js          |  38 ++---
 5 files changed, 203 insertions(+), 126 deletions(-)
 create mode 100644 services/filestore/app/js/PersistorHelper.js

diff --git a/services/filestore/app/js/FSPersistor.js b/services/filestore/app/js/FSPersistor.js
index 3f54e2d091..a5b1a35c8c 100644
--- a/services/filestore/app/js/FSPersistor.js
+++ b/services/filestore/app/js/FSPersistor.js
@@ -8,6 +8,7 @@ const { promisify, callbackify } = require('util')
 
 const LocalFileWriter = require('./LocalFileWriter').promises
 const { NotFoundError, ReadError, WriteError } = require('./Errors')
+const PersistorHelper = require('./PersistorHelper')
 
 const pipeline = promisify(Stream.pipeline)
 const fsUnlink = promisify(fs.unlink)
@@ -28,7 +29,7 @@ async function sendFile(location, target, source) {
     const targetStream = fs.createWriteStream(`${location}/${filteredTarget}`)
     await pipeline(sourceStream, targetStream)
   } catch (err) {
-    throw _wrapError(
+    throw PersistorHelper.wrapError(
       err,
       'failed to copy the specified file',
       { location, target, source },
@@ -65,7 +66,7 @@ async function getFileStream(location, name, opts) {
   try {
     opts.fd = await fsOpen(`${location}/${filteredName}`, 'r')
   } catch (err) {
-    throw _wrapError(
+    throw PersistorHelper.wrapError(
       err,
       'failed to open file for streaming',
       { location, filteredName, opts },
@@ -83,7 +84,7 @@ async function getFileSize(location, filename) {
     const stat = await fsStat(fullPath)
     return stat.size
   } catch (err) {
-    throw _wrapError(
+    throw PersistorHelper.wrapError(
       err,
       'failed to stat file',
       { location, filename },
@@ -126,7 +127,7 @@ async function copyFile(location, fromName, toName) {
     const targetStream = fs.createWriteStream(`${location}/${filteredToName}`)
     await pipeline(sourceStream, targetStream)
   } catch (err) {
-    throw _wrapError(
+    throw PersistorHelper.wrapError(
       err,
       'failed to copy file',
       { location, filteredFromName, filteredToName },
@@ -140,7 +141,7 @@ async function deleteFile(location, name) {
   try {
     await fsUnlink(`${location}/${filteredName}`)
   } catch (err) {
-    const wrappedError = _wrapError(
+    const wrappedError = PersistorHelper.wrapError(
       err,
       'failed to delete file',
       { location, filteredName },
@@ -161,7 +162,7 @@ async function deleteDirectory(location, name) {
   try {
     await rmrf(`${location}/${filteredName}`)
   } catch (err) {
-    throw _wrapError(
+    throw PersistorHelper.wrapError(
       err,
       'failed to delete directory',
       { location, filteredName },
@@ -179,7 +180,7 @@ async function checkIfFileExists(location, name) {
     if (err.code === 'ENOENT') {
       return false
     }
-    throw _wrapError(
+    throw PersistorHelper.wrapError(
       err,
       'failed to stat file',
       { location, filteredName },
@@ -209,7 +210,7 @@ async function directorySize(location, name) {
       }
     }
   } catch (err) {
-    throw _wrapError(
+    throw PersistorHelper.wrapError(
       err,
       'failed to get directory size',
       { location, name },
@@ -220,20 +221,6 @@ async function directorySize(location, name) {
   return size
 }
 
-function _wrapError(error, message, params, ErrorType) {
-  if (error.code === 'ENOENT') {
-    return new NotFoundError({
-      message: 'no such file or directory',
-      info: params
-    }).withCause(error)
-  } else {
-    return new ErrorType({
-      message: message,
-      info: params
-    }).withCause(error)
-  }
-}
-
 module.exports = {
   sendFile: callbackify(sendFile),
   sendStream: callbackify(sendStream),
diff --git a/services/filestore/app/js/PersistorHelper.js b/services/filestore/app/js/PersistorHelper.js
new file mode 100644
index 0000000000..d8beb4a0a9
--- /dev/null
+++ b/services/filestore/app/js/PersistorHelper.js
@@ -0,0 +1,114 @@
+const crypto = require('crypto')
+const meter = require('stream-meter')
+const Stream = require('stream')
+const logger = require('logger-sharelatex')
+const { WriteError, ReadError, NotFoundError } = require('./Errors')
+const { promisify } = require('util')
+
+const pipeline = promisify(Stream.pipeline)
+
+module.exports = {
+  calculateStreamMd5,
+  verifyMd5,
+  getMeteredStream,
+  waitForStreamReady,
+  wrapError
+}
+
+// returns a promise which resolves with the md5 hash of the stream
+function calculateStreamMd5(stream) {
+  const hash = crypto.createHash('md5')
+  hash.setEncoding('hex')
+
+  return new Promise((resolve, reject) => {
+    pipeline(stream, hash)
+      .then(() => {
+        hash.end()
+        resolve(hash.read())
+      })
+      .catch(err => {
+        reject(err)
+      })
+  })
+}
+
+// verifies the md5 hash of a file against the supplied md5 or the one stored in
+// storage if not supplied - deletes the new file if the md5 does not match and
+// throws an error
+async function verifyMd5(persistor, bucket, key, sourceMd5, destMd5 = null) {
+  if (!destMd5) {
+    destMd5 = await persistor.promises.getFileMd5Hash(bucket, key)
+  }
+
+  if (sourceMd5 !== destMd5) {
+    try {
+      await persistor.promises.deleteFile(bucket, key)
+    } catch (err) {
+      logger.warn(err, 'error deleting file for invalid upload')
+    }
+
+    throw new WriteError({
+      message: 'source and destination hashes do not match',
+      info: {
+        sourceMd5,
+        destMd5,
+        bucket,
+        key
+      }
+    })
+  }
+}
+
+// returns the next stream in the pipeline, and calls the callback with the byte count
+// when the stream finishes or receives an error
+function getMeteredStream(stream, callback) {
+  const meteredStream = meter()
+
+  pipeline(stream, meteredStream)
+    .then(() => {
+      callback(null, meteredStream.bytes)
+    })
+    .catch(err => {
+      // on error, just send how many bytes we received before the stream stopped
+      callback(err, meteredStream.bytes)
+    })
+
+  return meteredStream
+}
+
+// resolves when a stream is 'readable', or rejects if the stream throws an error
+// before that happens - this lets us handle protocol-level errors before trying
+// to read them
+function waitForStreamReady(stream) {
+  return new Promise((resolve, reject) => {
+    const onError = function(err) {
+      reject(wrapError(err, 'error before stream became ready', {}, ReadError))
+    }
+    const onStreamReady = function() {
+      stream.removeListener('readable', onStreamReady)
+      stream.removeListener('error', onError)
+      resolve(stream)
+    }
+    stream.on('readable', onStreamReady)
+    stream.on('error', onError)
+  })
+}
+
+function wrapError(error, message, params, ErrorType) {
+  if (
+    error instanceof NotFoundError ||
+    ['NoSuchKey', 'NotFound', 404, 'AccessDenied', 'ENOENT'].includes(
+      error.code
+    )
+  ) {
+    return new NotFoundError({
+      message: 'no such file',
+      info: params
+    }).withCause(error)
+  } else {
+    return new ErrorType({
+      message: message,
+      info: params
+    }).withCause(error)
+  }
+}
diff --git a/services/filestore/app/js/S3Persistor.js b/services/filestore/app/js/S3Persistor.js
index a10251a642..196d2aecda 100644
--- a/services/filestore/app/js/S3Persistor.js
+++ b/services/filestore/app/js/S3Persistor.js
@@ -5,11 +5,11 @@ https.globalAgent.maxSockets = 300
 
 const settings = require('settings-sharelatex')
 const metrics = require('metrics-sharelatex')
-const logger = require('logger-sharelatex')
+
+const PersistorHelper = require('./PersistorHelper')
 
 const meter = require('stream-meter')
 const Stream = require('stream')
-const crypto = require('crypto')
 const fs = require('fs')
 const S3 = require('aws-sdk/clients/s3')
 const { URL } = require('url')
@@ -21,7 +21,7 @@ const {
   SettingsError
 } = require('./Errors')
 
-module.exports = {
+const S3Persistor = {
   sendFile: callbackify(sendFile),
   sendStream: callbackify(sendStream),
   getFileStream: callbackify(getFileStream),
@@ -46,6 +46,8 @@ module.exports = {
   }
 }
 
+module.exports = S3Persistor
+
 const pipeline = promisify(Stream.pipeline)
 
 function hexToBase64(hex) {
@@ -57,7 +59,7 @@ async function sendFile(bucketName, key, fsPath) {
   try {
     readStream = fs.createReadStream(fsPath)
   } catch (err) {
-    throw _wrapError(
+    throw PersistorHelper.wrapError(
       err,
       'error reading file from disk',
       { bucketName, key, fsPath },
@@ -76,27 +78,14 @@ async function sendStream(bucketName, key, readStream, sourceMd5) {
     if (sourceMd5) {
       b64Hash = hexToBase64(sourceMd5)
     } else {
-      const hash = crypto.createHash('md5')
-      hash.setEncoding('hex')
-      pipeline(readStream, hash)
-      hashPromise = new Promise((resolve, reject) => {
-        readStream.on('end', () => {
-          hash.end()
-          resolve(hash.read())
-        })
-        readStream.on('error', err => {
-          reject(err)
-        })
-      })
+      hashPromise = PersistorHelper.calculateStreamMd5(readStream)
     }
 
-    const meteredStream = meter()
-    meteredStream.on('finish', () => {
-      metrics.count('s3.egress', meteredStream.bytes)
+    const meteredStream = PersistorHelper.getMeteredStream(readStream, (_, byteCount) => {
+      // ignore the error parameter and just log the byte count
+      metrics.count('s3.egress', byteCount)
     })
 
-    pipeline(readStream, meteredStream)
-
     // if we have an md5 hash, pass this to S3 to verify the upload
     const uploadOptions = {
       Bucket: bucketName,
@@ -112,30 +101,21 @@ async function sendStream(bucketName, key, readStream, sourceMd5) {
       .promise()
     const destMd5 = _md5FromResponse(response)
 
-    // if we didn't have an md5 hash, compare our computed one with S3's
+    // if we didn't have an md5 hash, we should compare our computed one with S3's
+    // as we couldn't tell S3 about it beforehand
     if (hashPromise) {
       sourceMd5 = await hashPromise
-
-      if (sourceMd5 !== destMd5) {
-        try {
-          await deleteFile(bucketName, key)
-        } catch (err) {
-          logger.warn(err, 'error deleting file for invalid upload')
-        }
-
-        throw new WriteError({
-          message: 'source and destination hashes do not match',
-          info: {
-            sourceMd5,
-            destMd5,
-            bucketName,
-            key
-          }
-        })
-      }
+      // throws on mismatch
+      await PersistorHelper.verifyMd5(
+        S3Persistor,
+        bucketName,
+        key,
+        sourceMd5,
+        destMd5
+      )
     }
   } catch (err) {
-    throw _wrapError(
+    throw PersistorHelper.wrapError(
       err,
       'upload to S3 failed',
       { bucketName, key },
@@ -155,25 +135,29 @@ async function getFileStream(bucketName, key, opts) {
     params.Range = `bytes=${opts.start}-${opts.end}`
   }
 
-  return new Promise((resolve, reject) => {
-    const stream = _getClientForBucket(bucketName)
-      .getObject(params)
-      .createReadStream()
+  const stream = _getClientForBucket(bucketName)
+    .getObject(params)
+    .createReadStream()
 
-    const meteredStream = meter()
-    meteredStream.on('finish', () => {
-      metrics.count('s3.ingress', meteredStream.bytes)
-    })
-
-    const onStreamReady = function() {
-      stream.removeListener('readable', onStreamReady)
-      resolve(stream.pipe(meteredStream))
+  const meteredStream = PersistorHelper.getMeteredStream(
+    stream,
+    (_, byteCount) => {
+      // ignore the error parameter and just log the byte count
+      metrics.count('s3.ingress', byteCount)
     }
-    stream.on('readable', onStreamReady)
-    stream.on('error', err => {
-      reject(_wrapError(err, 'error reading from S3', params, ReadError))
-    })
-  })
+  )
+
+  try {
+    await PersistorHelper.waitForStreamReady(stream)
+    return meteredStream
+  } catch (err) {
+    throw PersistorHelper.wrapError(
+      err,
+      'error reading file from S3',
+      { bucketName, key, opts },
+      ReadError
+    )
+  }
 }
 
 async function deleteDirectory(bucketName, key) {
@@ -184,7 +168,7 @@ async function deleteDirectory(bucketName, key) {
       .listObjects({ Bucket: bucketName, Prefix: key })
       .promise()
   } catch (err) {
-    throw _wrapError(
+    throw PersistorHelper.wrapError(
       err,
       'failed to list objects in S3',
       { bucketName, key },
@@ -205,7 +189,7 @@ async function deleteDirectory(bucketName, key) {
         })
         .promise()
     } catch (err) {
-      throw _wrapError(
+      throw PersistorHelper.wrapError(
         err,
         'failed to delete objects in S3',
         { bucketName, key },
@@ -222,7 +206,7 @@ async function getFileSize(bucketName, key) {
       .promise()
     return response.ContentLength
   } catch (err) {
-    throw _wrapError(
+    throw PersistorHelper.wrapError(
       err,
       'error getting size of s3 object',
       { bucketName, key },
@@ -239,7 +223,7 @@ async function getFileMd5Hash(bucketName, key) {
     const md5 = _md5FromResponse(response)
     return md5
   } catch (err) {
-    throw _wrapError(
+    throw PersistorHelper.wrapError(
       err,
       'error getting hash of s3 object',
       { bucketName, key },
@@ -255,7 +239,7 @@ async function deleteFile(bucketName, key) {
       .promise()
   } catch (err) {
     // s3 does not give us a NotFoundError here
-    throw _wrapError(
+    throw PersistorHelper.wrapError(
       err,
       'failed to delete file in S3',
       { bucketName, key },
@@ -275,7 +259,12 @@ async function copyFile(bucketName, sourceKey, destKey) {
       .copyObject(params)
       .promise()
   } catch (err) {
-    throw _wrapError(err, 'failed to copy file in S3', params, WriteError)
+    throw PersistorHelper.wrapError(
+      err,
+      'failed to copy file in S3',
+      params,
+      WriteError
+    )
   }
 }
 
@@ -287,7 +276,7 @@ async function checkIfFileExists(bucketName, key) {
     if (err instanceof NotFoundError) {
       return false
     }
-    throw _wrapError(
+    throw PersistorHelper.wrapError(
       err,
       'error checking whether S3 object exists',
       { bucketName, key },
@@ -304,7 +293,7 @@ async function directorySize(bucketName, key) {
 
     return response.Contents.reduce((acc, item) => item.Size + acc, 0)
   } catch (err) {
-    throw _wrapError(
+    throw PersistorHelper.wrapError(
       err,
       'error getting directory size in S3',
       { bucketName, key },
@@ -313,26 +302,6 @@ async function directorySize(bucketName, key) {
   }
 }
 
-function _wrapError(error, message, params, ErrorType) {
-  // the AWS client can return one of 'NoSuchKey', 'NotFound' or 404 (integer)
-  // when something is not found, depending on the endpoint
-  if (
-    ['NoSuchKey', 'NotFound', 404, 'AccessDenied', 'ENOENT'].includes(
-      error.code
-    )
-  ) {
-    return new NotFoundError({
-      message: 'no such file',
-      info: params
-    }).withCause(error)
-  } else {
-    return new ErrorType({
-      message: message,
-      info: params
-    }).withCause(error)
-  }
-}
-
 const _clients = new Map()
 let _defaultClient
 
diff --git a/services/filestore/test/unit/js/FSPersistorTests.js b/services/filestore/test/unit/js/FSPersistorTests.js
index 1be8eea3e2..0a09869bc0 100644
--- a/services/filestore/test/unit/js/FSPersistorTests.js
+++ b/services/filestore/test/unit/js/FSPersistorTests.js
@@ -70,7 +70,10 @@ describe('FSPersistorTests', function() {
         glob,
         rimraf,
         stream,
-        crypto
+        crypto,
+        // imported by PersistorHelper but otherwise unused here
+        'stream-meter': {},
+        'logger-sharelatex': {}
       },
       globals: { console }
     })
diff --git a/services/filestore/test/unit/js/S3PersistorTests.js b/services/filestore/test/unit/js/S3PersistorTests.js
index b9711572c2..9686deed5f 100644
--- a/services/filestore/test/unit/js/S3PersistorTests.js
+++ b/services/filestore/test/unit/js/S3PersistorTests.js
@@ -89,6 +89,7 @@ describe('S3PersistorTests', function() {
     }
 
     MeteredStream = {
+      type: 'metered',
       on: sinon.stub(),
       bytes: objectSize
     }
@@ -103,7 +104,7 @@ describe('S3PersistorTests', function() {
 
     S3ReadStream = {
       on: sinon.stub(),
-      pipe: sinon.stub().returns('s3Stream'),
+      pipe: sinon.stub(),
       removeListener: sinon.stub()
     }
     S3ReadStream.on.withArgs('readable').yields()
@@ -168,8 +169,8 @@ describe('S3PersistorTests', function() {
         stream = await S3Persistor.promises.getFileStream(bucket, key)
       })
 
-      it('returns a stream', function() {
-        expect(stream).to.equal('s3Stream')
+      it('returns a metered stream', function() {
+        expect(stream).to.equal(MeteredStream)
       })
 
       it('sets the AWS client up with credentials from settings', function() {
@@ -184,7 +185,10 @@ describe('S3PersistorTests', function() {
       })
 
       it('pipes the stream through the meter', function() {
-        expect(S3ReadStream.pipe).to.have.been.calledWith(MeteredStream)
+        expect(Stream.pipeline).to.have.been.calledWith(
+          S3ReadStream,
+          MeteredStream
+        )
       })
 
       it('records an ingress metric', function() {
@@ -202,8 +206,8 @@ describe('S3PersistorTests', function() {
         })
       })
 
-      it('returns a stream', function() {
-        expect(stream).to.equal('s3Stream')
+      it('returns a metered stream', function() {
+        expect(stream).to.equal(MeteredStream)
       })
 
       it('passes the byte range on to S3', function() {
@@ -236,8 +240,8 @@ describe('S3PersistorTests', function() {
         stream = await S3Persistor.promises.getFileStream(bucket, key)
       })
 
-      it('returns a stream', function() {
-        expect(stream).to.equal('s3Stream')
+      it('returns a metered stream', function() {
+        expect(stream).to.equal(MeteredStream)
       })
 
       it('sets the AWS client up with the alternative credentials', function() {
@@ -305,12 +309,12 @@ describe('S3PersistorTests', function() {
         expect(error).to.be.an.instanceOf(Errors.NotFoundError)
       })
 
-      it('wraps the error from S3', function() {
-        expect(error.cause).to.equal(S3NotFoundError)
+      it('wraps the error', function() {
+        expect(error.cause).to.exist
       })
 
       it('stores the bucket and key in the error', function() {
-        expect(error.info).to.deep.equal({ Bucket: bucket, Key: key })
+        expect(error.info).to.include({ bucketName: bucket, key: key })
       })
     })
 
@@ -335,12 +339,12 @@ describe('S3PersistorTests', function() {
         expect(error).to.be.an.instanceOf(Errors.NotFoundError)
       })
 
-      it('wraps the error from S3', function() {
-        expect(error.cause).to.equal(S3AccessDeniedError)
+      it('wraps the error', function() {
+        expect(error.cause).to.exist
       })
 
       it('stores the bucket and key in the error', function() {
-        expect(error.info).to.deep.equal({ Bucket: bucket, Key: key })
+        expect(error.info).to.include({ bucketName: bucket, key: key })
       })
     })
 
@@ -365,12 +369,12 @@ describe('S3PersistorTests', function() {
         expect(error).to.be.an.instanceOf(Errors.ReadError)
       })
 
-      it('wraps the error from S3', function() {
-        expect(error.cause).to.equal(genericError)
+      it('wraps the error', function() {
+        expect(error.cause).to.exist
       })
 
       it('stores the bucket and key in the error', function() {
-        expect(error.info).to.deep.equal({ Bucket: bucket, Key: key })
+        expect(error.info).to.include({ bucketName: bucket, key: key })
       })
     })
   })

From 49ad408b30d1497da456439a8ae3b5fca996b909 Mon Sep 17 00:00:00 2001
From: Simon Detheridge <s@sd.ai>
Date: Wed, 12 Feb 2020 10:32:26 +0000
Subject: [PATCH 11/12] Remove unused imports and format correctly

---
 services/filestore/app/js/S3Persistor.js | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/services/filestore/app/js/S3Persistor.js b/services/filestore/app/js/S3Persistor.js
index 196d2aecda..891d7be68e 100644
--- a/services/filestore/app/js/S3Persistor.js
+++ b/services/filestore/app/js/S3Persistor.js
@@ -8,12 +8,10 @@ const metrics = require('metrics-sharelatex')
 
 const PersistorHelper = require('./PersistorHelper')
 
-const meter = require('stream-meter')
-const Stream = require('stream')
 const fs = require('fs')
 const S3 = require('aws-sdk/clients/s3')
 const { URL } = require('url')
-const { callbackify, promisify } = require('util')
+const { callbackify } = require('util')
 const {
   WriteError,
   ReadError,
@@ -48,8 +46,6 @@ const S3Persistor = {
 
 module.exports = S3Persistor
 
-const pipeline = promisify(Stream.pipeline)
-
 function hexToBase64(hex) {
   return Buffer.from(hex, 'hex').toString('base64')
 }
@@ -81,10 +77,13 @@ async function sendStream(bucketName, key, readStream, sourceMd5) {
       hashPromise = PersistorHelper.calculateStreamMd5(readStream)
     }
 
-    const meteredStream = PersistorHelper.getMeteredStream(readStream, (_, byteCount) => {
-      // ignore the error parameter and just log the byte count
-      metrics.count('s3.egress', byteCount)
-    })
+    const meteredStream = PersistorHelper.getMeteredStream(
+      readStream,
+      (_, byteCount) => {
+        // ignore the error parameter and just log the byte count
+        metrics.count('s3.egress', byteCount)
+      }
+    )
 
     // if we have an md5 hash, pass this to S3 to verify the upload
     const uploadOptions = {

From 3b011258d207102620d0388d8f0098c77062648c Mon Sep 17 00:00:00 2001
From: Simon Detheridge <s@sd.ai>
Date: Wed, 12 Feb 2020 12:02:06 +0000
Subject: [PATCH 12/12] Tidy up md5 hash generation

---
 services/filestore/app/js/FSPersistor.js     | 19 +++++--------------
 services/filestore/app/js/PersistorHelper.js | 11 +----------
 2 files changed, 6 insertions(+), 24 deletions(-)

diff --git a/services/filestore/app/js/FSPersistor.js b/services/filestore/app/js/FSPersistor.js
index a5b1a35c8c..973c670efd 100644
--- a/services/filestore/app/js/FSPersistor.js
+++ b/services/filestore/app/js/FSPersistor.js
@@ -1,7 +1,6 @@
 const fs = require('fs')
 const glob = require('glob')
 const path = require('path')
-const crypto = require('crypto')
 const rimraf = require('rimraf')
 const Stream = require('stream')
 const { promisify, callbackify } = require('util')
@@ -105,19 +104,6 @@ async function getFileMd5Hash(location, filename) {
   }
 }
 
-async function _getFileMd5HashForPath(fullPath) {
-  return new Promise((resolve, reject) => {
-    const readStream = fs.createReadStream(fullPath)
-    const hash = crypto.createHash('md5')
-    hash.setEncoding('hex')
-    readStream.on('end', () => {
-      hash.end()
-      resolve(hash.read())
-    })
-    pipeline(readStream, hash).catch(reject)
-  })
-}
-
 async function copyFile(location, fromName, toName) {
   const filteredFromName = filterName(fromName)
   const filteredToName = filterName(toName)
@@ -245,3 +231,8 @@ module.exports = {
     directorySize
   }
 }
+
+async function _getFileMd5HashForPath(fullPath) {
+  const stream = fs.createReadStream(fullPath)
+  return PersistorHelper.calculateStreamMd5(stream)
+}
diff --git a/services/filestore/app/js/PersistorHelper.js b/services/filestore/app/js/PersistorHelper.js
index d8beb4a0a9..ea8132a9c9 100644
--- a/services/filestore/app/js/PersistorHelper.js
+++ b/services/filestore/app/js/PersistorHelper.js
@@ -20,16 +20,7 @@ function calculateStreamMd5(stream) {
   const hash = crypto.createHash('md5')
   hash.setEncoding('hex')
 
-  return new Promise((resolve, reject) => {
-    pipeline(stream, hash)
-      .then(() => {
-        hash.end()
-        resolve(hash.read())
-      })
-      .catch(err => {
-        reject(err)
-      })
-  })
+  return pipeline(stream, hash).then(() => hash.read())
 }
 
 // verifies the md5 hash of a file against the supplied md5 or the one stored in