From 3637cd70aed3c2a17ea01b030c2516ebba09c4f5 Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Thu, 9 Apr 2020 17:11:19 +0100 Subject: [PATCH] Add support for redirecting to signed URLs --- services/filestore/app/js/FSPersistor.js | 7 +++ services/filestore/app/js/FileController.js | 49 +++++++++------ services/filestore/app/js/FileHandler.js | 20 +++++++ services/filestore/app/js/GcsPersistor.js | 22 +++++++ .../filestore/app/js/MigrationPersistor.js | 2 + services/filestore/app/js/S3Persistor.js | 7 +++ .../filestore/config/settings.defaults.coffee | 3 + .../test/unit/js/FileControllerTests.js | 48 ++++++++++++++- .../test/unit/js/FileHandlerTests.js | 60 +++++++++++++++++++ .../test/unit/js/GcsPersistorTests.js | 20 ++++++- 10 files changed, 217 insertions(+), 21 deletions(-) diff --git a/services/filestore/app/js/FSPersistor.js b/services/filestore/app/js/FSPersistor.js index 4e514e3350..2e3b4985cb 100644 --- a/services/filestore/app/js/FSPersistor.js +++ b/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, diff --git a/services/filestore/app/js/FileController.js b/services/filestore/app/js/FileController.js index e39afd67bb..d72b4c841c 100644 --- a/services/filestore/app/js/FileController.js +++ b/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) + ) + } + }) }) }) } diff --git a/services/filestore/app/js/FileHandler.js b/services/filestore/app/js/FileHandler.js index 056206b0fe..b76bdff7a2 100644 --- a/services/filestore/app/js/FileHandler.js +++ b/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) } diff --git a/services/filestore/app/js/GcsPersistor.js b/services/filestore/app/js/GcsPersistor.js index 99a8c1a513..c3bf81f45d 100644 --- a/services/filestore/app/js/GcsPersistor.js +++ b/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 diff --git a/services/filestore/app/js/MigrationPersistor.js b/services/filestore/app/js/MigrationPersistor.js index d25ea84ce4..347dd58726 100644 --- a/services/filestore/app/js/MigrationPersistor.js +++ b/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'), diff --git a/services/filestore/app/js/S3Persistor.js b/services/filestore/app/js/S3Persistor.js index 8216c5f7cb..7e9a66a0ab 100644 --- a/services/filestore/app/js/S3Persistor.js +++ b/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 diff --git a/services/filestore/config/settings.defaults.coffee b/services/filestore/config/settings.defaults.coffee index 6867945d10..272230f918 100644 --- a/services/filestore/config/settings.defaults.coffee +++ b/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") diff --git a/services/filestore/test/unit/js/FileControllerTests.js b/services/filestore/test/unit/js/FileControllerTests.js index 16fbb3641c..cd1d19ef02 100644 --- a/services/filestore/test/unit/js/FileControllerTests.js +++ b/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 diff --git a/services/filestore/test/unit/js/FileHandlerTests.js b/services/filestore/test/unit/js/FileHandlerTests.js index 60ee2553c3..f6a0c02fa4 100644 --- a/services/filestore/test/unit/js/FileHandlerTests.js +++ b/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 => { diff --git a/services/filestore/test/unit/js/GcsPersistorTests.js b/services/filestore/test/unit/js/GcsPersistorTests.js index 3c386e5002..07d132fcde 100644 --- a/services/filestore/test/unit/js/GcsPersistorTests.js +++ b/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