diff --git a/services/filestore/app.js b/services/filestore/app.js index 232c5b24bc..9256cb0029 100644 --- a/services/filestore/app.js +++ b/services/filestore/app.js @@ -9,7 +9,6 @@ const express = require('express') const bodyParser = require('body-parser') const fileController = require('./app/js/FileController') -const bucketController = require('./app/js/BucketController') const keyBuilder = require('./app/js/KeyBuilder') const healthCheckController = require('./app/js/HealthCheckController') @@ -114,7 +113,11 @@ app.get( fileController.directorySize ) -app.get('/bucket/:bucket/key/*', bucketController.getFile) +app.get( + '/bucket/:bucket/key/*', + keyBuilder.bucketFileKeyMiddleware, + fileController.getFile +) app.get('/heapdump', (req, res, next) => require('heapdump').writeSnapshot( diff --git a/services/filestore/app/js/BucketController.js b/services/filestore/app/js/BucketController.js deleted file mode 100644 index 46f69679aa..0000000000 --- a/services/filestore/app/js/BucketController.js +++ /dev/null @@ -1,48 +0,0 @@ -/* eslint-disable - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -let BucketController -const settings = require('settings-sharelatex') -const logger = require('logger-sharelatex') -const FileHandler = require('./FileHandler') -const metrics = require('metrics-sharelatex') -const Errors = require('./Errors') - -module.exports = BucketController = { - getFile(req, res) { - const { bucket } = req.params - const key = req.params[0] - const credentials = - settings.filestore.s3BucketCreds != null - ? settings.filestore.s3BucketCreds[bucket] - : undefined - const options = { - key, - bucket, - credentials - } - metrics.inc(`${bucket}.getFile`) - logger.log({ key, bucket }, 'receiving request to get file from bucket') - return FileHandler.getFile(bucket, key, options, function(err, fileStream) { - if (err != null) { - logger.err({ err, key, bucket }, 'problem getting file from bucket') - if (err instanceof Errors.NotFoundError) { - return res.send(404) - } else { - return res.send(500) - } - } else { - logger.log({ key, bucket }, 'sending bucket file to response') - return fileStream.pipe(res) - } - }) - } -} diff --git a/services/filestore/app/js/KeyBuilder.js b/services/filestore/app/js/KeyBuilder.js index 8de7c0be2a..66cf563014 100644 --- a/services/filestore/app/js/KeyBuilder.js +++ b/services/filestore/app/js/KeyBuilder.js @@ -6,6 +6,7 @@ module.exports = { userFileKeyMiddleware, publicFileKeyMiddleware, publicProjectKeyMiddleware, + bucketFileKeyMiddleware, templateFileKeyMiddleware } @@ -48,6 +49,12 @@ function publicFileKeyMiddleware(req, res, next) { next() } +function bucketFileKeyMiddleware(req, res, next) { + req.bucket = req.params.bucket + req.key = req.params[0] + next() +} + function templateFileKeyMiddleware(req, res, next) { const { template_id: templateId, diff --git a/services/filestore/app/js/S3PersistorManager.js b/services/filestore/app/js/S3PersistorManager.js index 36c49d35bd..00ed46379b 100644 --- a/services/filestore/app/js/S3PersistorManager.js +++ b/services/filestore/app/js/S3PersistorManager.js @@ -303,5 +303,10 @@ function _clientOptions(bucketCredentials) { options.sslEnabled = endpoint.protocol === 'https' } + // path-style access is only used for acceptance tests + if (settings.filestore.s3.pathStyle) { + options.s3ForcePathStyle = true + } + return options } diff --git a/services/filestore/test/acceptance/js/FilestoreTests.js b/services/filestore/test/acceptance/js/FilestoreTests.js index 9260b1bd62..d7dfbce57c 100644 --- a/services/filestore/test/acceptance/js/FilestoreTests.js +++ b/services/filestore/test/acceptance/js/FilestoreTests.js @@ -7,6 +7,7 @@ const FilestoreApp = require('./FilestoreApp') const rp = require('request-promise-native').defaults({ resolveWithFullResponse: true }) +const S3 = require('aws-sdk/clients/s3') const Stream = require('stream') const request = require('request') const { promisify } = require('util') @@ -43,7 +44,8 @@ if (process.env.AWS_ACCESS_KEY_ID) { s3: { key: process.env.AWS_ACCESS_KEY_ID, secret: process.env.AWS_SECRET_ACCESS_KEY, - endpoint: process.env.AWS_S3_ENDPOINT + endpoint: process.env.AWS_S3_ENDPOINT, + pathStyle: true }, stores: { user_files: process.env.AWS_S3_USER_FILES_BUCKET_NAME, @@ -288,6 +290,48 @@ describe('Filestore', function() { }) }) + if (backend === 'S3Persistor') { + describe('with a file in a specific bucket', function() { + let constantFileContents, fileId, fileUrl, bucketName + + beforeEach(async function() { + constantFileContents = `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}` + + const s3ClientSettings = { + credentials: { + accessKeyId: 'fake', + secretAccessKey: 'fake' + }, + endpoint: process.env.AWS_S3_ENDPOINT, + sslEnabled: false, + s3ForcePathStyle: true + } + + const s3 = new S3(s3ClientSettings) + await s3 + .createBucket({ + Bucket: bucketName + }) + .promise() + await s3 + .upload({ + Bucket: bucketName, + Key: fileId, + Body: constantFileContents + }) + .promise() + }) + + it('should get the file from the specified bucket', async function() { + const response = await rp.get(fileUrl) + expect(response.body).to.equal(constantFileContents) + }) + }) + } + describe('with a pdf file', function() { let fileId, fileUrl, localFileSize const localFileReadPath = Path.resolve( diff --git a/services/filestore/test/unit/js/BucketControllerTests.js b/services/filestore/test/unit/js/BucketControllerTests.js deleted file mode 100644 index ef74b3f6c0..0000000000 --- a/services/filestore/test/unit/js/BucketControllerTests.js +++ /dev/null @@ -1,100 +0,0 @@ -/* eslint-disable - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -const { assert } = require('chai') -const sinon = require('sinon') -const chai = require('chai') -const should = chai.should() -const { expect } = chai -const modulePath = '../../../app/js/BucketController.js' -const SandboxedModule = require('sandboxed-module') - -describe('BucketController', function() { - beforeEach(function() { - this.PersistorManager = { - sendStream: sinon.stub(), - copyFile: sinon.stub(), - deleteFile: sinon.stub() - } - - this.settings = { - s3: { - buckets: { - user_files: 'user_files' - } - }, - filestore: { - backend: 's3', - s3: { - secret: 'secret', - key: 'this_key' - } - } - } - - this.FileHandler = { - getFile: sinon.stub(), - deleteFile: sinon.stub(), - insertFile: sinon.stub(), - getDirectorySize: sinon.stub() - } - this.LocalFileWriter = {} - this.controller = SandboxedModule.require(modulePath, { - requires: { - './LocalFileWriter': this.LocalFileWriter, - './FileHandler': this.FileHandler, - './PersistorManager': this.PersistorManager, - 'settings-sharelatex': this.settings, - 'metrics-sharelatex': { - inc() {} - }, - 'logger-sharelatex': { - log() {}, - err() {} - } - } - }) - this.project_id = 'project_id' - this.file_id = 'file_id' - this.bucket = 'user_files' - this.key = `${this.project_id}/${this.file_id}` - this.req = { - query: {}, - params: { - bucket: this.bucket, - 0: this.key - }, - headers: {} - } - this.res = { setHeader() {} } - return (this.fileStream = {}) - }) - - return describe('getFile', function() { - it('should pipe the stream', function(done) { - this.FileHandler.getFile.callsArgWith(3, null, this.fileStream) - this.fileStream.pipe = res => { - res.should.equal(this.res) - return done() - } - return this.controller.getFile(this.req, this.res) - }) - - return it('should send a 500 if there is a problem', function(done) { - this.FileHandler.getFile.callsArgWith(3, 'error') - this.res.send = code => { - code.should.equal(500) - return done() - } - return this.controller.getFile(this.req, this.res) - }) - }) -})