Merge pull request #116 from overleaf/spd-signed-urls

Add support for redirecting to signed URLs

services/filestore/app/js/FSPersistor.js

@@ -74,6 +74,11 @@ async function getFileStream(location, name, opts) {
   return fs.createReadStream(null, opts)
 }
 
+async function getRedirectUrl() {
+  // not implemented
+  return null
+}
+
 async function getFileSize(location, filename) {
   const fullPath = path.join(location, filterName(filename))
 
@@ -211,6 +216,7 @@ module.exports = {
   sendFile: callbackify(sendFile),
   sendStream: callbackify(sendStream),
   getFileStream: callbackify(getFileStream),
+  getRedirectUrl: callbackify(getRedirectUrl),
   getFileSize: callbackify(getFileSize),
   getFileMd5Hash: callbackify(getFileMd5Hash),
   copyFile: callbackify(copyFile),
@@ -222,6 +228,7 @@ module.exports = {
     sendFile,
     sendStream,
     getFileStream,
+    getRedirectUrl,
     getFileSize,
     getFileMd5Hash,
     copyFile,

services/filestore/app/js/FileController.js

@@ -46,31 +46,42 @@ function getFile(req, res, next) {
     }
   }
 
-  FileHandler.getFile(bucket, key, options, function(err, fileStream) {
+  FileHandler.getRedirectUrl(bucket, key, options, function(err, redirectUrl) {
     if (err) {
-      if (err instanceof Errors.NotFoundError) {
-        res.sendStatus(404)
-      } else {
-        next(err)
-      }
-      return
+      metrics.inc('file_redirect_error')
     }
 
-    if (req.query.cacheWarm) {
-      return res.sendStatus(200).end()
+    if (redirectUrl) {
+      metrics.inc('file_redirect')
+      return res.redirect(redirectUrl)
     }
 
-    pipeline(fileStream, res, err => {
-      if (err && err.code === 'ERR_STREAM_PREMATURE_CLOSE') {
-        res.end()
-      } else if (err) {
-        next(
-          new Errors.ReadError({
-            message: 'error transferring stream',
-            info: { bucket, key, format, style }
-          }).withCause(err)
-        )
+    FileHandler.getFile(bucket, key, options, function(err, fileStream) {
+      if (err) {
+        if (err instanceof Errors.NotFoundError) {
+          res.sendStatus(404)
+        } else {
+          next(err)
+        }
+        return
       }
+
+      if (req.query.cacheWarm) {
+        return res.sendStatus(200).end()
+      }
+
+      pipeline(fileStream, res, err => {
+        if (err && err.code === 'ERR_STREAM_PREMATURE_CLOSE') {
+          res.end()
+        } else if (err) {
+          next(
+            new Errors.ReadError({
+              message: 'error transferring stream',
+              info: { bucket, key, format, style }
+            }).withCause(err)
+          )
+        }
+      })
     })
   })
 }

services/filestore/app/js/FileHandler.js

@@ -13,10 +13,12 @@ module.exports = {
   deleteFile: callbackify(deleteFile),
   deleteProject: callbackify(deleteProject),
   getFile: callbackify(getFile),
+  getRedirectUrl: callbackify(getRedirectUrl),
   getFileSize: callbackify(getFileSize),
   getDirectorySize: callbackify(getDirectorySize),
   promises: {
     getFile,
+    getRedirectUrl,
     insertFile,
     deleteFile,
     deleteProject,
@@ -73,6 +75,24 @@ async function getFile(bucket, key, opts) {
   }
 }
 
+async function getRedirectUrl(bucket, key, opts) {
+  // if we're doing anything unusual with options, or the request isn't for
+  // one of the default buckets, return null so that we proxy the file
+  opts = opts || {}
+  if (
+    !opts.start &&
+    !opts.end &&
+    !opts.format &&
+    !opts.style &&
+    Object.values(Settings.filestore.stores).includes(bucket) &&
+    Settings.filestore.allowRedirects
+  ) {
+    return PersistorManager.promises.getRedirectUrl(bucket, key)
+  }
+
+  return null
+}
+
 async function getFileSize(bucket, key) {
   return PersistorManager.promises.getFileSize(bucket, key)
 }

services/filestore/app/js/GcsPersistor.js

@@ -36,6 +36,7 @@ const GcsPersistor = {
   sendFile: callbackify(sendFile),
   sendStream: callbackify(sendStream),
   getFileStream: callbackify(getFileStream),
+  getRedirectUrl: callbackify(getRedirectUrl),
   getFileMd5Hash: callbackify(getFileMd5Hash),
   deleteDirectory: callbackify(deleteDirectory),
   getFileSize: callbackify(getFileSize),
@@ -47,6 +48,7 @@ const GcsPersistor = {
     sendFile,
     sendStream,
     getFileStream,
+    getRedirectUrl,
     getFileMd5Hash,
     deleteDirectory,
     getFileSize,
@@ -141,6 +143,26 @@ async function getFileStream(bucketName, key, _opts = {}) {
   }
 }
 
+async function getRedirectUrl(bucketName, key) {
+  try {
+    const [url] = await storage
+      .bucket(bucketName)
+      .file(key)
+      .getSignedUrl({
+        action: 'read',
+        expires: new Date().getTime() + settings.filestore.signedUrlExpiryInMs
+      })
+    return url
+  } catch (err) {
+    throw PersistorHelper.wrapError(
+      err,
+      'error generating signed url for GCS file',
+      { bucketName, key },
+      ReadError
+    )
+  }
+}
+
 async function getFileSize(bucketName, key) {
   try {
     const [metadata] = await storage

services/filestore/app/js/MigrationPersistor.js

@@ -203,6 +203,7 @@ module.exports = function(primary, fallback) {
     sendFile: primary.sendFile,
     sendStream: primary.sendStream,
     getFileStream: callbackify(getFileStreamWithFallback),
+    getRedirectUrl: primary.getRedirectUrl,
     getFileMd5Hash: callbackify(_wrapFallbackMethod('getFileMd5Hash')),
     deleteDirectory: callbackify(
       _wrapMethodOnBothPersistors('deleteDirectory')
@@ -216,6 +217,7 @@ module.exports = function(primary, fallback) {
       sendFile: primary.promises.sendFile,
       sendStream: primary.promises.sendStream,
       getFileStream: getFileStreamWithFallback,
+      getRedirectUrl: primary.promises.getRedirectUrl,
       getFileMd5Hash: _wrapFallbackMethod('getFileMd5Hash'),
       deleteDirectory: _wrapMethodOnBothPersistors('deleteDirectory'),
       getFileSize: _wrapFallbackMethod('getFileSize'),

services/filestore/app/js/S3Persistor.js

@@ -24,6 +24,7 @@ const S3Persistor = {
   sendFile: callbackify(sendFile),
   sendStream: callbackify(sendStream),
   getFileStream: callbackify(getFileStream),
+  getRedirectUrl: callbackify(getRedirectUrl),
   getFileMd5Hash: callbackify(getFileMd5Hash),
   deleteDirectory: callbackify(deleteDirectory),
   getFileSize: callbackify(getFileSize),
@@ -35,6 +36,7 @@ const S3Persistor = {
     sendFile,
     sendStream,
     getFileStream,
+    getRedirectUrl,
     getFileMd5Hash,
     deleteDirectory,
     getFileSize,
@@ -146,6 +148,11 @@ async function getFileStream(bucketName, key, opts) {
   }
 }
 
+async function getRedirectUrl() {
+  // not implemented
+  return null
+}
+
 async function deleteDirectory(bucketName, key) {
   let response
 

services/filestore/config/settings.defaults.coffee

@@ -70,6 +70,9 @@ settings =
 				buckets: JSON.parse(process.env['FALLBACK_BUCKET_MAPPING'] || '{}')
 				copyOnMiss: process.env['COPY_ON_MISS'] == 'true'
 
+		allowRedirects: if process.env['ALLOW_REDIRECTS'] == 'true' then true else false
+		signedUrlExpiryInMs: parseInt(process.env['LINK_EXPIRY_TIMEOUT'] || 60000)
+
 	path:
 		uploadFolder: Path.resolve(__dirname + "/../uploads")
 

services/filestore/test/unit/js/FileControllerTests.js

@@ -42,7 +42,8 @@ describe('FileController', function() {
       deleteFile: sinon.stub().yields(),
       deleteProject: sinon.stub().yields(),
       insertFile: sinon.stub().yields(),
-      getDirectorySize: sinon.stub().yields(null, fileSize)
+      getDirectorySize: sinon.stub().yields(null, fileSize),
+      getRedirectUrl: sinon.stub().yields(null, null)
     }
 
     LocalFileWriter = {}
@@ -91,6 +92,11 @@ describe('FileController', function() {
   })
 
   describe('getFile', function() {
+    it('should try and get a redirect url first', function() {
+      FileController.getFile(req, res, next)
+      expect(FileHandler.getRedirectUrl).to.have.been.calledWith(bucket, key)
+    })
+
     it('should pipe the stream', function() {
       FileController.getFile(req, res, next)
       expect(stream.pipeline).to.have.been.calledWith(fileStream, res)
@@ -111,6 +117,46 @@ describe('FileController', function() {
       expect(next).to.have.been.calledWith(error)
     })
 
+    describe('with a redirect url', function() {
+      const redirectUrl = 'https://wombat.potato/giraffe'
+
+      beforeEach(function() {
+        FileHandler.getRedirectUrl.yields(null, redirectUrl)
+        res.redirect = sinon.stub()
+      })
+
+      it('should redirect', function() {
+        FileController.getFile(req, res, next)
+        expect(res.redirect).to.have.been.calledWith(redirectUrl)
+      })
+
+      it('should not get a file stream', function() {
+        FileController.getFile(req, res, next)
+        expect(FileHandler.getFile).not.to.have.been.called
+      })
+
+      describe('when there is an error getting the redirect url', function() {
+        beforeEach(function() {
+          FileHandler.getRedirectUrl.yields(new Error('wombat herding error'))
+        })
+
+        it('should not redirect', function() {
+          FileController.getFile(req, res, next)
+          expect(res.redirect).not.to.have.been.called
+        })
+
+        it('should not return an error', function() {
+          FileController.getFile(req, res, next)
+          expect(next).not.to.have.been.called
+        })
+
+        it('should proxy the file', function() {
+          FileController.getFile(req, res, next)
+          expect(FileHandler.getFile).to.have.been.calledWith(bucket, key)
+        })
+      })
+    })
+
     describe('with a range header', function() {
       let expectedOptions
 

services/filestore/test/unit/js/FileHandlerTests.js

@@ -24,6 +24,7 @@ describe('FileHandler', function() {
   const projectKey = `${ObjectId()}/`
   const sourceStream = 'sourceStream'
   const convertedKey = 'convertedKey'
+  const redirectUrl = 'https://wombat.potato/giraffe'
   const readStream = {
     stream: 'readStream',
     on: sinon.stub()
@@ -33,6 +34,7 @@ describe('FileHandler', function() {
     PersistorManager = {
       promises: {
         getFileStream: sinon.stub().resolves(sourceStream),
+        getRedirectUrl: sinon.stub().resolves(redirectUrl),
         checkIfFileExists: sinon.stub().resolves(),
         deleteFile: sinon.stub().resolves(),
         deleteDirectory: sinon.stub().resolves(),
@@ -299,6 +301,64 @@ describe('FileHandler', function() {
     })
   })
 
+  describe('getRedirectUrl', function() {
+    beforeEach(function() {
+      Settings.filestore = {
+        allowRedirects: true,
+        stores: {
+          userFiles: bucket
+        }
+      }
+    })
+
+    it('should return a redirect url', function(done) {
+      FileHandler.getRedirectUrl(bucket, key, (err, url) => {
+        expect(err).not.to.exist
+        expect(url).to.equal(redirectUrl)
+        done()
+      })
+    })
+
+    it('should call the persistor to get a redirect url', function(done) {
+      FileHandler.getRedirectUrl(bucket, key, () => {
+        expect(
+          PersistorManager.promises.getRedirectUrl
+        ).to.have.been.calledWith(bucket, key)
+        done()
+      })
+    })
+
+    it('should return null if options are supplied', function(done) {
+      FileHandler.getRedirectUrl(
+        bucket,
+        key,
+        { start: 100, end: 200 },
+        (err, url) => {
+          expect(err).not.to.exist
+          expect(url).to.be.null
+          done()
+        }
+      )
+    })
+
+    it('should return null if the bucket is not one of the defined ones', function(done) {
+      FileHandler.getRedirectUrl('a_different_bucket', key, (err, url) => {
+        expect(err).not.to.exist
+        expect(url).to.be.null
+        done()
+      })
+    })
+
+    it('should return null if redirects are not enabled', function(done) {
+      Settings.filestore.allowRedirects = false
+      FileHandler.getRedirectUrl(bucket, key, (err, url) => {
+        expect(err).not.to.exist
+        expect(url).to.be.null
+        done()
+      })
+    })
+  })
+
   describe('getDirectorySize', function() {
     it('should call the filestore manager to get directory size', function(done) {
       FileHandler.getDirectorySize(bucket, key, err => {

services/filestore/test/unit/js/GcsPersistorTests.js

@@ -17,6 +17,7 @@ describe('GcsPersistorTests', function() {
   const filesSize = 33
   const md5 = 'ffffffff00000000ffffffff00000000'
   const WriteStream = 'writeStream'
+  const redirectUrl = 'https://wombat.potato/giraffe'
 
   let Metrics,
     Logger,
@@ -97,7 +98,8 @@ describe('GcsPersistorTests', function() {
       getMetadata: sinon.stub().resolves([files[0].metadata]),
       createWriteStream: sinon.stub().returns(WriteStream),
       copy: sinon.stub().resolves(),
-      exists: sinon.stub().resolves([true])
+      exists: sinon.stub().resolves([true]),
+      getSignedUrl: sinon.stub().resolves([redirectUrl])
     }
 
     GcsBucket = {
@@ -260,6 +262,22 @@ describe('GcsPersistorTests', function() {
     })
   })
 
+  describe('getFile', function() {
+    let signedUrl
+
+    beforeEach(async function() {
+      signedUrl = await GcsPersistor.promises.getRedirectUrl(bucket, key)
+    })
+
+    it('should request a signed URL', function() {
+      expect(GcsFile.getSignedUrl).to.have.been.called
+    })
+
+    it('should return the url', function() {
+      expect(signedUrl).to.equal(redirectUrl)
+    })
+  })
+
   describe('getFileSize', function() {
     describe('when called with valid parameters', function() {
       let size