From feca8933f1ac4241242691bc74c05cac685f565a Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Wed, 4 Jul 2018 11:18:55 +0100 Subject: [PATCH 01/13] Add endpoint for arbitrary bucket fetch Add `/bucket/:bucket/key/*`, which fetches the file from the given bucket at the given path. Uses auth stored at `settings.filestore.s3.{{bucketName}}` if present, and otherwise default auth. --- services/filestore/app.coffee | 7 +- .../app/coffee/BucketController.coffee | 36 ++++++++++ .../app/coffee/S3PersistorManager.coffee | 4 +- .../unit/coffee/BucketControllerTests.coffee | 68 +++++++++++++++++++ .../coffee/S3PersistorManagerTests.coffee | 35 ++++++++++ 5 files changed, 145 insertions(+), 5 deletions(-) create mode 100644 services/filestore/app/coffee/BucketController.coffee create mode 100644 services/filestore/test/unit/coffee/BucketControllerTests.coffee diff --git a/services/filestore/app.coffee b/services/filestore/app.coffee index eb97ad48dd..ce10d2c91e 100644 --- a/services/filestore/app.coffee +++ b/services/filestore/app.coffee @@ -4,6 +4,7 @@ logger.initialize("filestore") settings = require("settings-sharelatex") request = require("request") fileController = require("./app/js/FileController") +bucketController = require("./app/js/BucketController") keyBuilder = require("./app/js/KeyBuilder") healthCheckController = require("./app/js/HealthCheckController") domain = require("domain") @@ -18,7 +19,7 @@ Metrics.memory.monitor(logger) app.configure -> app.use Metrics.http.monitor(logger) - + app.configure 'development', -> console.log "Development Enviroment" app.use express.errorHandler({ dumpExceptions: true, showStack: true }) @@ -86,6 +87,8 @@ app.del "/project/:project_id/public/:public_file_id", keyBuilder.publicFileKey, app.get "/project/:project_id/size", keyBuilder.publicProjectKey, fileController.directorySize +app.get "/bucket/:bucket/key/*", bucketController.getFile + app.get "/heapdump", (req, res)-> require('heapdump').writeSnapshot '/tmp/' + Date.now() + '.filestore.heapsnapshot', (err, filename)-> res.send filename @@ -103,8 +106,6 @@ app.get '/status', (req, res)-> app.get "/health_check", healthCheckController.check - - app.get '*', (req, res)-> diff --git a/services/filestore/app/coffee/BucketController.coffee b/services/filestore/app/coffee/BucketController.coffee new file mode 100644 index 0000000000..cc1ff03c45 --- /dev/null +++ b/services/filestore/app/coffee/BucketController.coffee @@ -0,0 +1,36 @@ +PersistorManager = require("./PersistorManager") +settings = require("settings-sharelatex") +logger = require("logger-sharelatex") +FileHandler = require("./FileHandler") +metrics = require("metrics-sharelatex") +parseRange = require('range-parser') +Errors = require('./Errors') + +oneDayInSeconds = 60 * 60 * 24 +maxSizeInBytes = 1024 * 1024 * 1024 # 1GB + +module.exports = BucketController = + + getFile: (req, res)-> + {bucket} = req + key = req[0] + {format, style} = req.query + credentials = settings.filestore.s3&[bucket] + options = { + key: key, + bucket: bucket, + credentials: credentials + } + metrics.inc "getFile" + logger.log key:key, bucket:bucket, "receiving request to get file from bucket" + FileHandler.getFile bucket, key, options, (err, fileStream)-> + if err? + logger.err err:err, key:key, bucket:bucket, format:format, style:style, "problem getting file from bucket" + if err instanceof Errors.NotFoundError + return res.send 404 + else + return res.send 500 + else + logger.log key:key, bucket:bucket, format:format, style:style, "sending bucket file to response" + fileStream.pipe res + diff --git a/services/filestore/app/coffee/S3PersistorManager.coffee b/services/filestore/app/coffee/S3PersistorManager.coffee index b1a03fb4f4..2bd6eb0e9b 100644 --- a/services/filestore/app/coffee/S3PersistorManager.coffee +++ b/services/filestore/app/coffee/S3PersistorManager.coffee @@ -68,8 +68,8 @@ module.exports = callback = _.once callback logger.log bucketName:bucketName, key:key, "getting file from s3" s3Client = knox.createClient - key: settings.filestore.s3.key - secret: settings.filestore.s3.secret + key: opts.credentials?.auth_key || settings.filestore.s3.key + secret: opts.credentials?.auth_secret || settings.filestore.s3.secret bucket: bucketName s3Stream = s3Client.get(key, headers) s3Stream.end() diff --git a/services/filestore/test/unit/coffee/BucketControllerTests.coffee b/services/filestore/test/unit/coffee/BucketControllerTests.coffee new file mode 100644 index 0000000000..fc91d08793 --- /dev/null +++ b/services/filestore/test/unit/coffee/BucketControllerTests.coffee @@ -0,0 +1,68 @@ +assert = require("chai").assert +sinon = require('sinon') +chai = require('chai') +should = chai.should() +expect = chai.expect +modulePath = "../../../app/js/BucketController.js" +SandboxedModule = require('sandboxed-module') + +describe "BucketController", -> + + beforeEach -> + @PersistorManager = + sendStream: sinon.stub() + copyFile: sinon.stub() + deleteFile:sinon.stub() + + @settings = + s3: + buckets: + user_files:"user_files" + filestore: + backend: "s3" + s3: + secret: "secret" + key: "this_key" + + @FileHandler = + getFile: sinon.stub() + deleteFile: sinon.stub() + insertFile: sinon.stub() + getDirectorySize: sinon.stub() + @LocalFileWriter = {} + @controller = SandboxedModule.require modulePath, requires: + "./LocalFileWriter":@LocalFileWriter + "./FileHandler": @FileHandler + "./PersistorManager":@PersistorManager + "settings-sharelatex": @settings + "logger-sharelatex": + log:-> + err:-> + @project_id = "project_id" + @file_id = "file_id" + @bucket = "user_files" + @key = "#{@project_id}/#{@file_id}" + @req = + bucket:@bucket + 0:@key + query:{} + headers: {} + @res = + setHeader: -> + @fileStream = {} + + describe "getFile", -> + + it "should pipe the stream", (done)-> + @FileHandler.getFile.callsArgWith(3, null, @fileStream) + @fileStream.pipe = (res)=> + res.should.equal @res + done() + @controller.getFile @req, @res + + it "should send a 500 if there is a problem", (done)-> + @FileHandler.getFile.callsArgWith(3, "error") + @res.send = (code)=> + code.should.equal 500 + done() + @controller.getFile @req, @res diff --git a/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee b/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee index 3a3e7b0d86..b48fde7820 100644 --- a/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee +++ b/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee @@ -55,6 +55,41 @@ describe "S3PersistorManagerTests", -> @stubbedKnoxClient.get.calledWith(@key).should.equal true done() + it "should use default auth", (done)-> + @stubbedKnoxClient.get.returns( + on:-> + end:-> + ) + @S3PersistorManager.getFileStream @bucketName, @key, @opts, (err)=> # empty callback + clientParams = + key: @settings.filestore.s3.key + secret: @settings.filestore.s3.secret + bucket: @bucketName + @knox.createClient.calledWith(clientParams).should.equal true + done() + + describe "with supplied auth", -> + beforeEach -> + @S3PersistorManager = SandboxedModule.require modulePath, requires: @requires + @credentials = + auth_key: "that_key" + auth_secret: "that_secret" + @opts = + credentials: @credentials + + it "should use supplied auth", (done)-> + @stubbedKnoxClient.get.returns( + on:-> + end:-> + ) + @S3PersistorManager.getFileStream @bucketName, @key, @opts, (err)=> # empty callback + clientParams = + key: @credentials.auth_key + secret: @credentials.auth_secret + bucket: @bucketName + @knox.createClient.calledWith(clientParams).should.equal true + done() + describe "with start and end options", -> beforeEach -> @opts = From ece650741a4017e4a960eef556b67d15e410e61c Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Wed, 4 Jul 2018 12:02:09 +0100 Subject: [PATCH 02/13] Amend per several review comments - Removed unused vars - Label the metric with the bucket name --- services/filestore/app/coffee/BucketController.coffee | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/services/filestore/app/coffee/BucketController.coffee b/services/filestore/app/coffee/BucketController.coffee index cc1ff03c45..9192bd147e 100644 --- a/services/filestore/app/coffee/BucketController.coffee +++ b/services/filestore/app/coffee/BucketController.coffee @@ -1,14 +1,9 @@ -PersistorManager = require("./PersistorManager") settings = require("settings-sharelatex") logger = require("logger-sharelatex") FileHandler = require("./FileHandler") metrics = require("metrics-sharelatex") -parseRange = require('range-parser') Errors = require('./Errors') -oneDayInSeconds = 60 * 60 * 24 -maxSizeInBytes = 1024 * 1024 * 1024 # 1GB - module.exports = BucketController = getFile: (req, res)-> @@ -21,7 +16,7 @@ module.exports = BucketController = bucket: bucket, credentials: credentials } - metrics.inc "getFile" + metrics.inc "#{bucket}.getFile" logger.log key:key, bucket:bucket, "receiving request to get file from bucket" FileHandler.getFile bucket, key, options, (err, fileStream)-> if err? @@ -33,4 +28,3 @@ module.exports = BucketController = else logger.log key:key, bucket:bucket, format:format, style:style, "sending bucket file to response" fileStream.pipe res - From cfbf0d81bab33e59d2596589ed45d0b41ca967d1 Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Wed, 4 Jul 2018 12:11:09 +0100 Subject: [PATCH 03/13] Amend: fix params retrieval --- services/filestore/app/coffee/BucketController.coffee | 4 ++-- .../filestore/test/unit/coffee/BucketControllerTests.coffee | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/services/filestore/app/coffee/BucketController.coffee b/services/filestore/app/coffee/BucketController.coffee index 9192bd147e..9392090e83 100644 --- a/services/filestore/app/coffee/BucketController.coffee +++ b/services/filestore/app/coffee/BucketController.coffee @@ -7,8 +7,8 @@ Errors = require('./Errors') module.exports = BucketController = getFile: (req, res)-> - {bucket} = req - key = req[0] + {bucket} = req.params + key = req.params[0] {format, style} = req.query credentials = settings.filestore.s3&[bucket] options = { diff --git a/services/filestore/test/unit/coffee/BucketControllerTests.coffee b/services/filestore/test/unit/coffee/BucketControllerTests.coffee index fc91d08793..461f3f03d6 100644 --- a/services/filestore/test/unit/coffee/BucketControllerTests.coffee +++ b/services/filestore/test/unit/coffee/BucketControllerTests.coffee @@ -43,9 +43,10 @@ describe "BucketController", -> @bucket = "user_files" @key = "#{@project_id}/#{@file_id}" @req = - bucket:@bucket - 0:@key query:{} + params: + bucket: @bucket + 0: @key headers: {} @res = setHeader: -> From 600ab3ce67f5635cc7e95f05bb348abfae19d863 Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Wed, 4 Jul 2018 16:39:41 +0100 Subject: [PATCH 04/13] Amend: remove problematic ampersand --- services/filestore/app/coffee/BucketController.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/filestore/app/coffee/BucketController.coffee b/services/filestore/app/coffee/BucketController.coffee index 9392090e83..2b983e77a3 100644 --- a/services/filestore/app/coffee/BucketController.coffee +++ b/services/filestore/app/coffee/BucketController.coffee @@ -10,7 +10,7 @@ module.exports = BucketController = {bucket} = req.params key = req.params[0] {format, style} = req.query - credentials = settings.filestore.s3&[bucket] + credentials = settings.filestore.s3[bucket] options = { key: key, bucket: bucket, From 336a38ec1e435d3bc504c1d08dc8f855b9a673dd Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Wed, 4 Jul 2018 16:41:31 +0100 Subject: [PATCH 05/13] Amend: scrub secrets from logs Calls to `getFile` can now include S3 credentials in `opts`, so sanitize before writing to opts to log. --- services/filestore/app/coffee/FileHandler.coffee | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/services/filestore/app/coffee/FileHandler.coffee b/services/filestore/app/coffee/FileHandler.coffee index 93cad984dd..2eeb09bc74 100644 --- a/services/filestore/app/coffee/FileHandler.coffee +++ b/services/filestore/app/coffee/FileHandler.coffee @@ -7,7 +7,7 @@ KeyBuilder = require("./KeyBuilder") async = require("async") ImageOptimiser = require("./ImageOptimiser") -module.exports = +module.exports = FileHandler = insertFile: (bucket, key, stream, callback)-> convertedKey = KeyBuilder.getConvertedFolderKey key @@ -23,7 +23,8 @@ module.exports = ], callback getFile: (bucket, key, opts = {}, callback)-> - logger.log bucket:bucket, key:key, opts:opts, "getting file" + # In this call, opts can contain credentials + logger.log bucket:bucket, key:key, opts:@_scrubSecrets(opts), "getting file" if !opts.format? and !opts.style? @_getStandardFile bucket, key, opts, callback else @@ -32,7 +33,7 @@ module.exports = _getStandardFile: (bucket, key, opts, callback)-> PersistorManager.getFileStream bucket, key, opts, (err, fileStream)-> if err? - logger.err bucket:bucket, key:key, opts:opts, "error getting fileStream" + logger.err bucket:bucket, key:key, opts:FileHandler._scrubSecrets(opts), "error getting fileStream" callback err, fileStream _getConvertedFile: (bucket, key, opts, callback)-> @@ -71,7 +72,7 @@ module.exports = return callback(err) done = (err, destPath)-> if err? - logger.err err:err, bucket:bucket, originalKey:originalKey, opts:opts, "error converting file" + logger.err err:err, bucket:bucket, originalKey:originalKey, opts:FileHandler._scrubSecrets(opts), "error converting file" return callback(err) LocalFileWriter.deleteFile originalFsPath, -> callback(err, destPath, originalFsPath) @@ -98,3 +99,8 @@ module.exports = if err? logger.err bucket:bucket, project_id:project_id, "error getting size" callback err, size + + _scrubSecrets: (opts)-> + safe = Object.assign {}, opts + delete safe.credentials + safe From 03033409c6811720e1f3f2673a5c5fcef71f2b54 Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Fri, 6 Jul 2018 09:12:59 +0100 Subject: [PATCH 06/13] Amend: remove unused params --- services/filestore/app/coffee/BucketController.coffee | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/services/filestore/app/coffee/BucketController.coffee b/services/filestore/app/coffee/BucketController.coffee index 2b983e77a3..ed7781fdda 100644 --- a/services/filestore/app/coffee/BucketController.coffee +++ b/services/filestore/app/coffee/BucketController.coffee @@ -9,7 +9,6 @@ module.exports = BucketController = getFile: (req, res)-> {bucket} = req.params key = req.params[0] - {format, style} = req.query credentials = settings.filestore.s3[bucket] options = { key: key, @@ -20,11 +19,11 @@ module.exports = BucketController = logger.log key:key, bucket:bucket, "receiving request to get file from bucket" FileHandler.getFile bucket, key, options, (err, fileStream)-> if err? - logger.err err:err, key:key, bucket:bucket, format:format, style:style, "problem getting file from bucket" + logger.err err:err, key:key, bucket:bucket, "problem getting file from bucket" if err instanceof Errors.NotFoundError return res.send 404 else return res.send 500 else - logger.log key:key, bucket:bucket, format:format, style:style, "sending bucket file to response" + logger.log key:key, bucket:bucket, "sending bucket file to response" fileStream.pipe res From 7feafccf31b830a3cf39a0d3c3413623178c45bf Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Fri, 6 Jul 2018 09:28:09 +0100 Subject: [PATCH 07/13] Amend: safely navigate to bucket credentials --- services/filestore/app/coffee/BucketController.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/filestore/app/coffee/BucketController.coffee b/services/filestore/app/coffee/BucketController.coffee index ed7781fdda..81c660ec69 100644 --- a/services/filestore/app/coffee/BucketController.coffee +++ b/services/filestore/app/coffee/BucketController.coffee @@ -9,7 +9,7 @@ module.exports = BucketController = getFile: (req, res)-> {bucket} = req.params key = req.params[0] - credentials = settings.filestore.s3[bucket] + credentials = settings.filestore.s3?[bucket] options = { key: key, bucket: bucket, From 2da15f2eb301002a656d53ad8fa3520f1da018c0 Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Fri, 6 Jul 2018 10:28:02 +0100 Subject: [PATCH 08/13] Amend - improve documentation of settings --- services/filestore/config/settings.defaults.coffee | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/services/filestore/config/settings.defaults.coffee b/services/filestore/config/settings.defaults.coffee index 92c4a7ec8d..ef44bbb604 100644 --- a/services/filestore/config/settings.defaults.coffee +++ b/services/filestore/config/settings.defaults.coffee @@ -25,12 +25,16 @@ module.exports = template_files: Path.resolve(__dirname + "/../template_files") # if you are using S3, then fill in your S3 details below # s3: - # key: "" - # secret: "" + # key: "" # default + # secret: "" # default + # bucketname1: # secrets for bucketname1 + # auth_key: "" + # auth_secret: "" + # bucketname2: # secrets for bucketname2... path: uploadFolder: Path.resolve(__dirname + "/../uploads") - + commands: # Any commands to wrap the convert utility in, for example ["nice"], or ["firejail", "--profile=/etc/firejail/convert.profile"] convertCommandPrefix: [] From 3e1ef3af635bad5d9e40352426a4257b3aa2c4d5 Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Fri, 6 Jul 2018 11:05:19 +0100 Subject: [PATCH 09/13] Populate S3 settings from environment variable --- services/filestore/config/settings.defaults.coffee | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/services/filestore/config/settings.defaults.coffee b/services/filestore/config/settings.defaults.coffee index ef44bbb604..62d42cf9e3 100644 --- a/services/filestore/config/settings.defaults.coffee +++ b/services/filestore/config/settings.defaults.coffee @@ -23,7 +23,8 @@ module.exports = user_files: Path.resolve(__dirname + "/../user_files") public_files: Path.resolve(__dirname + "/../public_files") template_files: Path.resolve(__dirname + "/../template_files") - # if you are using S3, then fill in your S3 details below + # 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 @@ -31,6 +32,7 @@ module.exports = # auth_key: "" # auth_secret: "" # bucketname2: # secrets for bucketname2... + s3: JSON.parse process.env['S3_CREDENTIALS'] if process.env['S3_CREDENTIALS'] path: uploadFolder: Path.resolve(__dirname + "/../uploads") From c4e3f9eb02924d53d3a32e58324c2b5ac34aad49 Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Fri, 6 Jul 2018 11:05:40 +0100 Subject: [PATCH 10/13] Amend: tests for populating S3 settings from environment variable --- .../test/unit/coffee/SettingsTests.coffee | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 services/filestore/test/unit/coffee/SettingsTests.coffee diff --git a/services/filestore/test/unit/coffee/SettingsTests.coffee b/services/filestore/test/unit/coffee/SettingsTests.coffee new file mode 100644 index 0000000000..95278622ca --- /dev/null +++ b/services/filestore/test/unit/coffee/SettingsTests.coffee @@ -0,0 +1,22 @@ +assert = require("chai").assert +sinon = require('sinon') +chai = require('chai') +should = chai.should() +expect = chai.expect +modulePath = "../../../app/js/BucketController.js" +SandboxedModule = require('sandboxed-module') + +describe "Settings", -> + describe "s3", -> + it "should use JSONified env var if present", (done)-> + s3_settings = + key: 'default_key' + secret: 'default_secret' + bucket1: + auth_key: 'bucket1_key' + auth_secret: 'bucket1_secret' + process.env['S3_CREDENTIALS'] = JSON.stringify s3_settings + + settings =require('settings-sharelatex') + expect(settings.filestore.s3).to.deep.equal s3_settings + done() From c14476c0c1e346a721cb3ba7ea514e9f061e430d Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Wed, 4 Jul 2018 11:18:55 +0100 Subject: [PATCH 11/13] Add endpoint for arbitrary bucket fetch Add `/bucket/:bucket/key/*`, which fetches the file from the given bucket at the given path. Uses auth stored at `settings.filestore.s3.{{bucketName}}` if present, and otherwise default auth. --- services/filestore/app.coffee | 7 +- .../app/coffee/BucketController.coffee | 29 ++++++++ .../filestore/app/coffee/FileHandler.coffee | 14 ++-- .../app/coffee/S3PersistorManager.coffee | 4 +- .../filestore/config/settings.defaults.coffee | 10 ++- .../unit/coffee/BucketControllerTests.coffee | 69 +++++++++++++++++++ .../coffee/S3PersistorManagerTests.coffee | 35 ++++++++++ 7 files changed, 156 insertions(+), 12 deletions(-) create mode 100644 services/filestore/app/coffee/BucketController.coffee create mode 100644 services/filestore/test/unit/coffee/BucketControllerTests.coffee diff --git a/services/filestore/app.coffee b/services/filestore/app.coffee index eb97ad48dd..ce10d2c91e 100644 --- a/services/filestore/app.coffee +++ b/services/filestore/app.coffee @@ -4,6 +4,7 @@ logger.initialize("filestore") settings = require("settings-sharelatex") request = require("request") fileController = require("./app/js/FileController") +bucketController = require("./app/js/BucketController") keyBuilder = require("./app/js/KeyBuilder") healthCheckController = require("./app/js/HealthCheckController") domain = require("domain") @@ -18,7 +19,7 @@ Metrics.memory.monitor(logger) app.configure -> app.use Metrics.http.monitor(logger) - + app.configure 'development', -> console.log "Development Enviroment" app.use express.errorHandler({ dumpExceptions: true, showStack: true }) @@ -86,6 +87,8 @@ app.del "/project/:project_id/public/:public_file_id", keyBuilder.publicFileKey, app.get "/project/:project_id/size", keyBuilder.publicProjectKey, fileController.directorySize +app.get "/bucket/:bucket/key/*", bucketController.getFile + app.get "/heapdump", (req, res)-> require('heapdump').writeSnapshot '/tmp/' + Date.now() + '.filestore.heapsnapshot', (err, filename)-> res.send filename @@ -103,8 +106,6 @@ app.get '/status', (req, res)-> app.get "/health_check", healthCheckController.check - - app.get '*', (req, res)-> diff --git a/services/filestore/app/coffee/BucketController.coffee b/services/filestore/app/coffee/BucketController.coffee new file mode 100644 index 0000000000..81c660ec69 --- /dev/null +++ b/services/filestore/app/coffee/BucketController.coffee @@ -0,0 +1,29 @@ +settings = require("settings-sharelatex") +logger = require("logger-sharelatex") +FileHandler = require("./FileHandler") +metrics = require("metrics-sharelatex") +Errors = require('./Errors') + +module.exports = BucketController = + + getFile: (req, res)-> + {bucket} = req.params + key = req.params[0] + credentials = settings.filestore.s3?[bucket] + options = { + key: key, + bucket: bucket, + credentials: credentials + } + metrics.inc "#{bucket}.getFile" + logger.log key:key, bucket:bucket, "receiving request to get file from bucket" + FileHandler.getFile bucket, key, options, (err, fileStream)-> + if err? + logger.err err:err, key:key, bucket:bucket, "problem getting file from bucket" + if err instanceof Errors.NotFoundError + return res.send 404 + else + return res.send 500 + else + logger.log key:key, bucket:bucket, "sending bucket file to response" + fileStream.pipe res diff --git a/services/filestore/app/coffee/FileHandler.coffee b/services/filestore/app/coffee/FileHandler.coffee index 93cad984dd..2eeb09bc74 100644 --- a/services/filestore/app/coffee/FileHandler.coffee +++ b/services/filestore/app/coffee/FileHandler.coffee @@ -7,7 +7,7 @@ KeyBuilder = require("./KeyBuilder") async = require("async") ImageOptimiser = require("./ImageOptimiser") -module.exports = +module.exports = FileHandler = insertFile: (bucket, key, stream, callback)-> convertedKey = KeyBuilder.getConvertedFolderKey key @@ -23,7 +23,8 @@ module.exports = ], callback getFile: (bucket, key, opts = {}, callback)-> - logger.log bucket:bucket, key:key, opts:opts, "getting file" + # In this call, opts can contain credentials + logger.log bucket:bucket, key:key, opts:@_scrubSecrets(opts), "getting file" if !opts.format? and !opts.style? @_getStandardFile bucket, key, opts, callback else @@ -32,7 +33,7 @@ module.exports = _getStandardFile: (bucket, key, opts, callback)-> PersistorManager.getFileStream bucket, key, opts, (err, fileStream)-> if err? - logger.err bucket:bucket, key:key, opts:opts, "error getting fileStream" + logger.err bucket:bucket, key:key, opts:FileHandler._scrubSecrets(opts), "error getting fileStream" callback err, fileStream _getConvertedFile: (bucket, key, opts, callback)-> @@ -71,7 +72,7 @@ module.exports = return callback(err) done = (err, destPath)-> if err? - logger.err err:err, bucket:bucket, originalKey:originalKey, opts:opts, "error converting file" + logger.err err:err, bucket:bucket, originalKey:originalKey, opts:FileHandler._scrubSecrets(opts), "error converting file" return callback(err) LocalFileWriter.deleteFile originalFsPath, -> callback(err, destPath, originalFsPath) @@ -98,3 +99,8 @@ module.exports = if err? logger.err bucket:bucket, project_id:project_id, "error getting size" callback err, size + + _scrubSecrets: (opts)-> + safe = Object.assign {}, opts + delete safe.credentials + safe diff --git a/services/filestore/app/coffee/S3PersistorManager.coffee b/services/filestore/app/coffee/S3PersistorManager.coffee index b1a03fb4f4..2bd6eb0e9b 100644 --- a/services/filestore/app/coffee/S3PersistorManager.coffee +++ b/services/filestore/app/coffee/S3PersistorManager.coffee @@ -68,8 +68,8 @@ module.exports = callback = _.once callback logger.log bucketName:bucketName, key:key, "getting file from s3" s3Client = knox.createClient - key: settings.filestore.s3.key - secret: settings.filestore.s3.secret + key: opts.credentials?.auth_key || settings.filestore.s3.key + secret: opts.credentials?.auth_secret || settings.filestore.s3.secret bucket: bucketName s3Stream = s3Client.get(key, headers) s3Stream.end() diff --git a/services/filestore/config/settings.defaults.coffee b/services/filestore/config/settings.defaults.coffee index 92c4a7ec8d..ef44bbb604 100644 --- a/services/filestore/config/settings.defaults.coffee +++ b/services/filestore/config/settings.defaults.coffee @@ -25,12 +25,16 @@ module.exports = template_files: Path.resolve(__dirname + "/../template_files") # if you are using S3, then fill in your S3 details below # s3: - # key: "" - # secret: "" + # key: "" # default + # secret: "" # default + # bucketname1: # secrets for bucketname1 + # auth_key: "" + # auth_secret: "" + # bucketname2: # secrets for bucketname2... path: uploadFolder: Path.resolve(__dirname + "/../uploads") - + commands: # Any commands to wrap the convert utility in, for example ["nice"], or ["firejail", "--profile=/etc/firejail/convert.profile"] convertCommandPrefix: [] diff --git a/services/filestore/test/unit/coffee/BucketControllerTests.coffee b/services/filestore/test/unit/coffee/BucketControllerTests.coffee new file mode 100644 index 0000000000..461f3f03d6 --- /dev/null +++ b/services/filestore/test/unit/coffee/BucketControllerTests.coffee @@ -0,0 +1,69 @@ +assert = require("chai").assert +sinon = require('sinon') +chai = require('chai') +should = chai.should() +expect = chai.expect +modulePath = "../../../app/js/BucketController.js" +SandboxedModule = require('sandboxed-module') + +describe "BucketController", -> + + beforeEach -> + @PersistorManager = + sendStream: sinon.stub() + copyFile: sinon.stub() + deleteFile:sinon.stub() + + @settings = + s3: + buckets: + user_files:"user_files" + filestore: + backend: "s3" + s3: + secret: "secret" + key: "this_key" + + @FileHandler = + getFile: sinon.stub() + deleteFile: sinon.stub() + insertFile: sinon.stub() + getDirectorySize: sinon.stub() + @LocalFileWriter = {} + @controller = SandboxedModule.require modulePath, requires: + "./LocalFileWriter":@LocalFileWriter + "./FileHandler": @FileHandler + "./PersistorManager":@PersistorManager + "settings-sharelatex": @settings + "logger-sharelatex": + log:-> + err:-> + @project_id = "project_id" + @file_id = "file_id" + @bucket = "user_files" + @key = "#{@project_id}/#{@file_id}" + @req = + query:{} + params: + bucket: @bucket + 0: @key + headers: {} + @res = + setHeader: -> + @fileStream = {} + + describe "getFile", -> + + it "should pipe the stream", (done)-> + @FileHandler.getFile.callsArgWith(3, null, @fileStream) + @fileStream.pipe = (res)=> + res.should.equal @res + done() + @controller.getFile @req, @res + + it "should send a 500 if there is a problem", (done)-> + @FileHandler.getFile.callsArgWith(3, "error") + @res.send = (code)=> + code.should.equal 500 + done() + @controller.getFile @req, @res diff --git a/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee b/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee index 3a3e7b0d86..b48fde7820 100644 --- a/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee +++ b/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee @@ -55,6 +55,41 @@ describe "S3PersistorManagerTests", -> @stubbedKnoxClient.get.calledWith(@key).should.equal true done() + it "should use default auth", (done)-> + @stubbedKnoxClient.get.returns( + on:-> + end:-> + ) + @S3PersistorManager.getFileStream @bucketName, @key, @opts, (err)=> # empty callback + clientParams = + key: @settings.filestore.s3.key + secret: @settings.filestore.s3.secret + bucket: @bucketName + @knox.createClient.calledWith(clientParams).should.equal true + done() + + describe "with supplied auth", -> + beforeEach -> + @S3PersistorManager = SandboxedModule.require modulePath, requires: @requires + @credentials = + auth_key: "that_key" + auth_secret: "that_secret" + @opts = + credentials: @credentials + + it "should use supplied auth", (done)-> + @stubbedKnoxClient.get.returns( + on:-> + end:-> + ) + @S3PersistorManager.getFileStream @bucketName, @key, @opts, (err)=> # empty callback + clientParams = + key: @credentials.auth_key + secret: @credentials.auth_secret + bucket: @bucketName + @knox.createClient.calledWith(clientParams).should.equal true + done() + describe "with start and end options", -> beforeEach -> @opts = From 836ff145b863c28b55130cd12f9b449847e46faa Mon Sep 17 00:00:00 2001 From: Michael Mazour Date: Fri, 6 Jul 2018 11:05:19 +0100 Subject: [PATCH 12/13] Populate S3 settings from environment variable --- .../filestore/config/settings.defaults.coffee | 4 +++- .../test/unit/coffee/SettingsTests.coffee | 22 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 services/filestore/test/unit/coffee/SettingsTests.coffee diff --git a/services/filestore/config/settings.defaults.coffee b/services/filestore/config/settings.defaults.coffee index ef44bbb604..62d42cf9e3 100644 --- a/services/filestore/config/settings.defaults.coffee +++ b/services/filestore/config/settings.defaults.coffee @@ -23,7 +23,8 @@ module.exports = user_files: Path.resolve(__dirname + "/../user_files") public_files: Path.resolve(__dirname + "/../public_files") template_files: Path.resolve(__dirname + "/../template_files") - # if you are using S3, then fill in your S3 details below + # 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 @@ -31,6 +32,7 @@ module.exports = # auth_key: "" # auth_secret: "" # bucketname2: # secrets for bucketname2... + s3: JSON.parse process.env['S3_CREDENTIALS'] if process.env['S3_CREDENTIALS'] path: uploadFolder: Path.resolve(__dirname + "/../uploads") diff --git a/services/filestore/test/unit/coffee/SettingsTests.coffee b/services/filestore/test/unit/coffee/SettingsTests.coffee new file mode 100644 index 0000000000..95278622ca --- /dev/null +++ b/services/filestore/test/unit/coffee/SettingsTests.coffee @@ -0,0 +1,22 @@ +assert = require("chai").assert +sinon = require('sinon') +chai = require('chai') +should = chai.should() +expect = chai.expect +modulePath = "../../../app/js/BucketController.js" +SandboxedModule = require('sandboxed-module') + +describe "Settings", -> + describe "s3", -> + it "should use JSONified env var if present", (done)-> + s3_settings = + key: 'default_key' + secret: 'default_secret' + bucket1: + auth_key: 'bucket1_key' + auth_secret: 'bucket1_secret' + process.env['S3_CREDENTIALS'] = JSON.stringify s3_settings + + settings =require('settings-sharelatex') + expect(settings.filestore.s3).to.deep.equal s3_settings + done() From 089bf3e0846bc2195300d762ae90a8499177718e Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Tue, 10 Jul 2018 14:17:37 +0100 Subject: [PATCH 13/13] seperate the standard s3 creds from the bucket s3 creds --- services/filestore/app/coffee/BucketController.coffee | 2 +- services/filestore/config/settings.defaults.coffee | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/services/filestore/app/coffee/BucketController.coffee b/services/filestore/app/coffee/BucketController.coffee index 81c660ec69..bb7bd4544b 100644 --- a/services/filestore/app/coffee/BucketController.coffee +++ b/services/filestore/app/coffee/BucketController.coffee @@ -9,7 +9,7 @@ module.exports = BucketController = getFile: (req, res)-> {bucket} = req.params key = req.params[0] - credentials = settings.filestore.s3?[bucket] + credentials = settings.filestore.s3BucketCreds?[bucket] options = { key: key, bucket: bucket, diff --git a/services/filestore/config/settings.defaults.coffee b/services/filestore/config/settings.defaults.coffee index 62d42cf9e3..59f5d8a0a6 100644 --- a/services/filestore/config/settings.defaults.coffee +++ b/services/filestore/config/settings.defaults.coffee @@ -28,11 +28,14 @@ module.exports = # s3: # key: "" # default # secret: "" # default + # + # s3BucketCreds: # bucketname1: # secrets for bucketname1 # auth_key: "" # auth_secret: "" # bucketname2: # secrets for bucketname2... - s3: JSON.parse process.env['S3_CREDENTIALS'] if process.env['S3_CREDENTIALS'] + + s3BucketCreds: JSON.parse process.env['S3_BUCKET_CREDENTIALS'] if process.env['S3_BUCKET_CREDENTIALS'] path: uploadFolder: Path.resolve(__dirname + "/../uploads")