Companion: remove searchProviders wrapper & move s3 options (#3781)

transloadit · May 30, 2022 · c0a9e21 · c0a9e21
1 parent 8b06289
commit c0a9e21
Show file tree

Hide file tree

Showing 21 changed files with 144 additions and 123 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -308,8 +308,19 @@ module.exports = {
     {
       files: ['./packages/@uppy/companion/**/*.js'],
       rules: {
-        'no-restricted-syntax': 'warn',
         'no-underscore-dangle': 'off',
+
+        // transloadit rules we would like to enforce in the future
+        // but will require separate PRs to gradually get there
+        // and so the meantime: just warn
+        'class-methods-use-this': 'warn',
+        'consistent-return': 'warn',
+        'global-require': 'warn',
+        'import/order': 'warn',
+        'no-param-reassign': 'warn',
+        'no-redeclare': 'warn',
+        'no-shadow': 'warn',
+        'no-use-before-define': 'warn',
       },
     },
     {

diff --git a/packages/@uppy/companion/src/companion.js b/packages/@uppy/companion/src/companion.js
@@ -77,7 +77,7 @@ module.exports.app = (optionsArg = {}) => {
 
   // create singleton redis client
   if (options.redisUrl) {
-    redis.client(merge({ url: options.redisUrl }, options.redisOptions || {}))
+    redis.client(options)
   }
   const emitter = createEmitter(options.redisUrl, options.redisPubSubScope)
 
@@ -117,7 +117,7 @@ module.exports.app = (optionsArg = {}) => {
 
   // add uppy options to the request object so it can be accessed by subsequent handlers.
   app.use('*', middlewares.getCompanionMiddleware(options))
-  app.use('/s3', s3(options.providerOptions.s3))
+  app.use('/s3', s3(options.s3))
   app.use('/url', url())
 
   app.post('/:providerName/preauth', middlewares.hasSessionAndProvider, controllers.preauth)

diff --git a/packages/@uppy/companion/src/config/companion.js b/packages/@uppy/companion/src/config/companion.js
@@ -8,15 +8,14 @@ const defaultOptions = {
     protocol: 'http',
     path: '',
   },
-  providerOptions: {
-    s3: {
-      acl: 'public-read', // todo default to no ACL in next major
-      endpoint: 'https://{service}.{region}.amazonaws.com',
-      conditions: [],
-      useAccelerateEndpoint: false,
-      getKey: (req, filename) => filename,
-      expires: ms('5 minutes') / 1000,
-    },
+  providerOptions: {},
+  s3: {
+    acl: 'public-read', // todo default to no ACL in next major
+    endpoint: 'https://{service}.{region}.amazonaws.com',
+    conditions: [],
+    useAccelerateEndpoint: false,
+    getKey: (req, filename) => filename,
+    expires: ms('5 minutes') / 1000,
   },
   allowLocalUrls: false,
   logClientVersion: true,
@@ -30,7 +29,8 @@ const defaultOptions = {
  */
 function getMaskableSecrets (companionOptions) {
   const secrets = []
-  const { providerOptions, customProviders } = companionOptions
+  const { providerOptions, customProviders, s3 } = companionOptions
+
   Object.keys(providerOptions).forEach((provider) => {
     if (providerOptions[provider].secret) {
       secrets.push(providerOptions[provider].secret)
@@ -45,6 +45,10 @@ function getMaskableSecrets (companionOptions) {
     })
   }
 
+  if (s3?.secret) {
+    secrets.push(s3.secret)
+  }
+
   return secrets
 }
 

diff --git a/packages/@uppy/companion/src/server/Uploader.js b/packages/@uppy/companion/src/server/Uploader.js
@@ -345,7 +345,7 @@ class Uploader {
       storage: redis.client(),
       s3: req.companion.s3Client ? {
         client: req.companion.s3Client,
-        options: req.companion.options.providerOptions.s3,
+        options: req.companion.options.s3,
       } : null,
       chunkSize: req.companion.options.chunkSize,
     }
@@ -400,7 +400,11 @@ class Uploader {
    */
   saveState (state) {
     if (!this.storage) return
-    this.storage.set(`${Uploader.STORAGE_PREFIX}:${this.token}`, jsonStringify(state))
+    // make sure the keys get cleaned up.
+    // https://github.com/transloadit/uppy/issues/3748
+    const keyExpirySec = 60 * 60 * 24
+    const redisKey = `${Uploader.STORAGE_PREFIX}:${this.token}`
+    this.storage.set(redisKey, jsonStringify(state), 'EX', keyExpirySec)
   }
 
   /**

diff --git a/packages/@uppy/companion/src/server/helpers/utils.js b/packages/@uppy/companion/src/server/helpers/utils.js
@@ -43,7 +43,7 @@ module.exports.getURLBuilder = (options) => {
    *
    * @param {string} path the tail path of the url
    * @param {boolean} isExternal if the url is for the external world
-   * @param {boolean=} excludeHost if the server domain and protocol should be included
+   * @param {boolean} [excludeHost] if the server domain and protocol should be included
    */
   const buildURL = (path, isExternal, excludeHost) => {
     let url = path

diff --git a/packages/@uppy/companion/src/server/logger.js b/packages/@uppy/companion/src/server/logger.js
@@ -39,8 +39,8 @@ function maskMessage (msg) {
  * @param {string | Error} arg the message or error to log
  * @param {string} tag a unique tag to easily search for this message
  * @param {string} level error | info | debug
- * @param {string=} id a unique id to easily trace logs tied to a request
- * @param {Function=} color function to display the log in appropriate color
+ * @param {string} [id] a unique id to easily trace logs tied to a request
+ * @param {Function} [color] function to display the log in appropriate color
  */
 const log = (arg, tag = '', level, id = '', color = (message) => message) => {
   const time = new Date().toISOString()
@@ -66,8 +66,8 @@ const log = (arg, tag = '', level, id = '', color = (message) => message) => {
  * INFO level log
  *
  * @param {string} msg the message to log
- * @param {string=} tag a unique tag to easily search for this message
- * @param {string=} traceId a unique id to easily trace logs tied to a request
+ * @param {string} [tag] a unique tag to easily search for this message
+ * @param {string} [traceId] a unique id to easily trace logs tied to a request
  */
 exports.info = (msg, tag, traceId) => {
   log(msg, tag, 'info', traceId)
@@ -77,8 +77,8 @@ exports.info = (msg, tag, traceId) => {
  * WARN level log
  *
  * @param {string} msg the message to log
- * @param {string=} tag a unique tag to easily search for this message
- * @param {string=} traceId a unique id to easily trace logs tied to a request
+ * @param {string} [tag] a unique tag to easily search for this message
+ * @param {string} [traceId] a unique id to easily trace logs tied to a request
  */
 exports.warn = (msg, tag, traceId) => {
   // @ts-ignore
@@ -89,8 +89,8 @@ exports.warn = (msg, tag, traceId) => {
  * ERROR level log
  *
  * @param {string | Error} msg the message to log
- * @param {string=} tag a unique tag to easily search for this message
- * @param {string=} traceId a unique id to easily trace logs tied to a request
+ * @param {string} [tag] a unique tag to easily search for this message
+ * @param {string} [traceId] a unique id to easily trace logs tied to a request
  */
 exports.error = (msg, tag, traceId) => {
   // @ts-ignore
@@ -101,8 +101,8 @@ exports.error = (msg, tag, traceId) => {
  * DEBUG level log
  *
  * @param {string} msg the message to log
- * @param {string=} tag a unique tag to easily search for this message
- * @param {string=} traceId a unique id to easily trace logs tied to a request
+ * @param {string} [tag] a unique tag to easily search for this message
+ * @param {string} [traceId] a unique id to easily trace logs tied to a request
  */
 exports.debug = (msg, tag, traceId) => {
   if (process.env.NODE_ENV !== 'production') {

diff --git a/packages/@uppy/companion/src/server/middlewares.js b/packages/@uppy/companion/src/server/middlewares.js
@@ -69,14 +69,14 @@ exports.cookieAuthToken = (req, res, next) => {
 }
 
 exports.loadSearchProviderToken = (req, res, next) => {
-  const { searchProviders } = req.companion.options.providerOptions
+  const { providerOptions } = req.companion.options
   const providerName = req.params.searchProviderName
-  if (!searchProviders || !searchProviders[providerName] || !searchProviders[providerName].key) {
+  if (!providerOptions[providerName] || !providerOptions[providerName].key) {
     logger.info(`unconfigured credentials for ${providerName}`, 'searchtoken.load.unset', req.id)
     return res.sendStatus(501)
   }
 
-  req.companion.providerToken = searchProviders[providerName].key
+  req.companion.providerToken = providerOptions[providerName].key
   next()
 }
 

diff --git a/packages/@uppy/companion/src/server/provider/box/index.js b/packages/@uppy/companion/src/server/provider/box/index.js
@@ -41,6 +41,10 @@ class Box extends Provider {
    * Lists files and folders from Box API
    *
    * @param {object} options
+   * @param {string} options.directory
+   * @param {any} options.query
+   * @param {string} options.token
+   * @param {unknown} options.companion
    * @param {Function} done
    */
   _list ({ directory, token, query, companion }, done) {

diff --git a/packages/@uppy/companion/src/server/provider/index.js b/packages/@uppy/companion/src/server/provider/index.js
@@ -188,8 +188,6 @@ module.exports.addProviderOptions = (companionOptions, grantConfig) => {
       } else if (server.path) {
         grantConfig[authProvider].callback = `${server.path}${grantConfig[authProvider].callback}`
       }
-    } else if (!['s3', 'searchProviders'].includes(providerName)) {
-      logger.warn(`skipping one found unsupported provider "${providerName}".`, 'provider.options.skip')
     }
   })
 }
diff --git a/packages/@uppy/companion/src/server/provider/onedrive/index.js b/packages/@uppy/companion/src/server/provider/onedrive/index.js
@@ -37,6 +37,9 @@ class OneDrive extends Provider {
    * it then waits till both requests are done before proceeding with the callback
    *
    * @param {object} options
+   * @param {string} options.directory
+   * @param {any} options.query
+   * @param {string} options.token
    * @param {Function} done
    */
   _list ({ directory, query, token }, done) {

diff --git a/packages/@uppy/companion/src/server/provider/unsplash/adapter.js b/packages/@uppy/companion/src/server/provider/unsplash/adapter.js
@@ -40,7 +40,7 @@ exports.getItemThumbnailUrl = (item) => {
 }
 
 exports.getNextPageQuery = (currentQuery) => {
-  const newCursor = parseInt(currentQuery.cursor || 1) + 1
+  const newCursor = Number.parseInt(currentQuery.cursor || 1, 10) + 1
   const query = {
     ...currentQuery,
     cursor: newCursor,

diff --git a/packages/@uppy/companion/src/server/redis.js b/packages/@uppy/companion/src/server/redis.js
@@ -1,21 +1,26 @@
 const redis = require('redis')
+const merge = require('lodash.merge')
 
 let redisClient
 
 /**
- * A Singleton module that provides only on redis client through out
+ * A Singleton module that provides a single redis client through out
  * the lifetime of the server
  *
- * @param {object=} opts node-redis client options
+ * @param {Record<string, unknown>} [opts] node-redis client options
  */
-module.exports.client = (opts) => {
-  if (!opts) {
-    return redisClient
-  }
-
+function createClient (opts) {
   if (!redisClient) {
     redisClient = redis.createClient(opts)
   }
 
   return redisClient
 }
+
+module.exports.client = (companionOptions) => {
+  if (!companionOptions) {
+    return redisClient
+  }
+
+  return createClient(merge({ url: companionOptions.redisUrl }, companionOptions.redisOptions))
+}
diff --git a/packages/@uppy/companion/src/server/s3-client.js b/packages/@uppy/companion/src/server/s3-client.js
@@ -8,35 +8,35 @@ const AWS = require('aws-sdk')
  */
 module.exports = (companionOptions) => {
   let s3Client = null
-  if (companionOptions.providerOptions.s3) {
-    const s3ProviderOptions = companionOptions.providerOptions.s3
+  if (companionOptions.s3) {
+    const { s3 } = companionOptions
 
-    if (s3ProviderOptions.accessKeyId || s3ProviderOptions.secretAccessKey) {
+    if (s3.accessKeyId || s3.secretAccessKey) {
       throw new Error('Found `providerOptions.s3.accessKeyId` or `providerOptions.s3.secretAccessKey` configuration, but Companion requires `key` and `secret` option names instead. Please use the `key` property instead of `accessKeyId` and the `secret` property instead of `secretAccessKey`.')
     }
 
-    const rawClientOptions = s3ProviderOptions.awsClientOptions
+    const rawClientOptions = s3.awsClientOptions
     if (rawClientOptions && (rawClientOptions.accessKeyId || rawClientOptions.secretAccessKey)) {
       throw new Error('Found unsupported `providerOptions.s3.awsClientOptions.accessKeyId` or `providerOptions.s3.awsClientOptions.secretAccessKey` configuration. Please use the `providerOptions.s3.key` and `providerOptions.s3.secret` options instead.')
     }
 
     const s3ClientOptions = {
       signatureVersion: 'v4',
-      endpoint: s3ProviderOptions.endpoint,
-      region: s3ProviderOptions.region,
+      endpoint: s3.endpoint,
+      region: s3.region,
       // backwards compat
-      useAccelerateEndpoint: s3ProviderOptions.useAccelerateEndpoint,
+      useAccelerateEndpoint: s3.useAccelerateEndpoint,
       ...rawClientOptions,
     }
 
     // Use credentials to allow assumed roles to pass STS sessions in.
     // If the user doesn't specify key and secret, the default credentials (process-env)
     // will be used by S3 in calls below.
-    if (s3ProviderOptions.key && s3ProviderOptions.secret && !s3ClientOptions.credentials) {
+    if (s3.key && s3.secret && !s3ClientOptions.credentials) {
       s3ClientOptions.credentials = new AWS.Credentials(
-        s3ProviderOptions.key,
-        s3ProviderOptions.secret,
-        s3ProviderOptions.sessionToken,
+        s3.key,
+        s3.secret,
+        s3.sessionToken,
       )
     }
     s3Client = new S3(s3ClientOptions)

diff --git a/packages/@uppy/companion/src/standalone/helper.js b/packages/@uppy/companion/src/standalone/helper.js
@@ -64,26 +64,22 @@ const getConfigFromEnv = () => {
         verificationToken: getSecret('COMPANION_ZOOM_VERIFICATION_TOKEN'),
         credentialsURL: process.env.COMPANION_ZOOM_KEYS_ENDPOINT,
       },
-      // TODO: remove the redundant searchProviders warpper in next major version
-      searchProviders: {
-        unsplash: {
-          key: process.env.COMPANION_UNSPLASH_KEY,
-          secret: process.env.COMPANION_UNSPLASH_SECRET,
-        },
-      },
-      // TODO: move s3 out of providerOptions, it's a destination, not a source
-      s3: {
-        key: process.env.COMPANION_AWS_KEY,
-        secret: getSecret('COMPANION_AWS_SECRET'),
-        bucket: process.env.COMPANION_AWS_BUCKET,
-        endpoint: process.env.COMPANION_AWS_ENDPOINT,
-        region: process.env.COMPANION_AWS_REGION,
-        useAccelerateEndpoint:
-          process.env.COMPANION_AWS_USE_ACCELERATE_ENDPOINT === 'true',
-        expires: parseInt(process.env.COMPANION_AWS_EXPIRES || '300', 10),
-        acl: process.env.COMPANION_AWS_DISABLE_ACL === 'true' ? null : (process.env.COMPANION_AWS_ACL || 'public-read'), // todo default to no ACL in next major and remove COMPANION_AWS_DISABLE_ACL
+      unsplash: {
+        key: process.env.COMPANION_UNSPLASH_KEY,
+        secret: process.env.COMPANION_UNSPLASH_SECRET,
       },
     },
+    s3: {
+      key: process.env.COMPANION_AWS_KEY,
+      secret: getSecret('COMPANION_AWS_SECRET'),
+      bucket: process.env.COMPANION_AWS_BUCKET,
+      endpoint: process.env.COMPANION_AWS_ENDPOINT,
+      region: process.env.COMPANION_AWS_REGION,
+      useAccelerateEndpoint:
+      process.env.COMPANION_AWS_USE_ACCELERATE_ENDPOINT === 'true',
+      expires: parseInt(process.env.COMPANION_AWS_EXPIRES || '300', 10),
+      acl: process.env.COMPANION_AWS_DISABLE_ACL === 'true' ? null : (process.env.COMPANION_AWS_ACL || 'public-read'), // todo default to no ACL in next major and remove COMPANION_AWS_DISABLE_ACL
+    },
     server: {
       host: process.env.COMPANION_DOMAIN,
       protocol: process.env.COMPANION_PROTOCOL,
@@ -190,11 +186,6 @@ exports.buildHelpfulStartupMessage = (companionOptions) => {
   const buildURL = utils.getURLBuilder(companionOptions)
   const callbackURLs = []
   Object.keys(companionOptions.providerOptions).forEach((providerName) => {
-    // s3 does not need redirect_uris
-    if (providerName === 's3') {
-      return
-    }
-
     callbackURLs.push(buildURL(`/connect/${providerName}/redirect`, true))
   })
 

diff --git a/packages/@uppy/companion/src/standalone/index.js b/packages/@uppy/companion/src/standalone/index.js
@@ -4,16 +4,16 @@ const helmet = require('helmet')
 const morgan = require('morgan')
 const bodyParser = require('body-parser')
 const { URL } = require('url')
-const merge = require('lodash.merge')
 const session = require('express-session')
 const addRequestId = require('express-request-id')()
+const connectRedis = require('connect-redis')
+
 const logger = require('../server/logger')
 const redis = require('../server/redis')
 const companion = require('../companion')
 const helper = require('./helper')
 const middlewares = require('../server/middlewares')
 const { getURLBuilder } = require('../server/helpers/utils')
-const connectRedis = require('connect-redis')
 
 /**
  * Configures an Express app for running Companion standalone
@@ -141,9 +141,7 @@ module.exports = function server (inputCompanionOptions = {}) {
 
   if (companionOptions.redisUrl) {
     const RedisStore = connectRedis(session)
-    const redisClient = redis.client(
-      merge({ url: companionOptions.redisUrl }, companionOptions.redisOptions),
-    )
+    const redisClient = redis.client(companionOptions)
     sessionOptions.store = new RedisStore({ client: redisClient })
   }
 

diff --git a/packages/@uppy/dashboard/src/Dashboard.jsx b/packages/@uppy/dashboard/src/Dashboard.jsx
@@ -380,7 +380,7 @@ export default class Dashboard extends UIPlugin {
       meta: {
         // path of the file relative to the ancestor directory the user selected.
         // e.g. 'docs/Old Prague/airbnb.pdf'
-        relativePath: file.relativePath || null,
+        relativePath: file.relativePath || file.webkitRelativePath || null,
       },
     }))