@uppy/companion: merge Provider/SearchProvider (#4330)

* merge search provider and provider

closes #4308

* simplify and improve naming

* Apply suggestions from code review

* fix test

need to have an authProvider for a grantConfig to be set
previously we didn't care, so this test was a bit wrong

* Update packages/@uppy/companion/src/server/provider/Provider.js

* fix merge oops

packages/@uppy/companion/src/companion.js

@@ -71,7 +71,7 @@ module.exports.app = (optionsArg = {}) => {
   const options = merge({}, defaultOptions, optionsArg)
 
   const providers = providerManager.getDefaultProviders()
-  const searchProviders = providerManager.getSearchProviders()
+
   providerManager.addProviderOptions(options, grantConfig)
 
   const { customProviders } = options
@@ -119,22 +119,25 @@ module.exports.app = (optionsArg = {}) => {
   app.use('/s3', s3(options.s3))
   app.use('/url', url())
 
-  app.post('/:providerName/preauth', express.json(), express.urlencoded({ extended: false }), middlewares.hasSessionAndProvider, middlewares.hasBody, controllers.preauth)
-  app.get('/:providerName/connect', middlewares.hasSessionAndProvider, controllers.connect)
-  app.get('/:providerName/redirect', middlewares.hasSessionAndProvider, controllers.redirect)
-  app.get('/:providerName/callback', middlewares.hasSessionAndProvider, controllers.callback)
-  app.post('/:providerName/deauthorization/callback', express.json(), middlewares.hasSessionAndProvider, middlewares.hasBody, controllers.deauthorizationCallback)
-  app.get('/:providerName/logout', middlewares.hasSessionAndProvider, middlewares.gentleVerifyToken, controllers.logout)
-  app.get('/:providerName/send-token', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.sendToken)
+  app.post('/:providerName/preauth', express.json(), express.urlencoded({ extended: false }), middlewares.hasSessionAndProvider, middlewares.hasBody, middlewares.hasOAuthProvider, controllers.preauth)
+  app.get('/:providerName/connect', middlewares.hasSessionAndProvider, middlewares.hasOAuthProvider, controllers.connect)
+  app.get('/:providerName/redirect', middlewares.hasSessionAndProvider, middlewares.hasOAuthProvider, controllers.redirect)
+  app.get('/:providerName/callback', middlewares.hasSessionAndProvider, middlewares.hasOAuthProvider, controllers.callback)
+  app.post('/:providerName/deauthorization/callback', express.json(), middlewares.hasSessionAndProvider, middlewares.hasBody, middlewares.hasOAuthProvider, controllers.deauthorizationCallback)
+  app.get('/:providerName/logout', middlewares.hasSessionAndProvider, middlewares.hasOAuthProvider, middlewares.gentleVerifyToken, controllers.logout)
+  app.get('/:providerName/send-token', middlewares.hasSessionAndProvider, middlewares.hasOAuthProvider, middlewares.verifyToken, controllers.sendToken)
+
   app.get('/:providerName/list/:id?', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list)
-  app.post('/:providerName/get/:id', express.json(), middlewares.hasSessionAndProvider, middlewares.hasBody, middlewares.verifyToken, controllers.get)
-  app.get('/:providerName/thumbnail/:id', middlewares.hasSessionAndProvider, middlewares.cookieAuthToken, middlewares.verifyToken, controllers.thumbnail)
-  // @ts-ignore Type instantiation is excessively deep and possibly infinite.
-  app.get('/search/:searchProviderName/list', middlewares.hasSearchQuery, middlewares.loadSearchProviderToken, controllers.list)
-  app.post('/search/:searchProviderName/get/:id', express.json(), middlewares.loadSearchProviderToken, controllers.get)
-
-  app.param('providerName', providerManager.getProviderMiddleware(providers, true))
-  app.param('searchProviderName', providerManager.getProviderMiddleware(searchProviders))
+  // backwards compat:
+  app.get('/search/:providerName/list', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list)
+
+  app.post('/:providerName/get/:id', express.json(), middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.get)
+  // backwards compat:
+  app.post('/search/:providerName/get/:id', express.json(), middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.get)
+
+  app.get('/:providerName/thumbnail/:id', middlewares.hasSessionAndProvider, middlewares.hasOAuthProvider, middlewares.cookieAuthToken, middlewares.verifyToken, controllers.thumbnail)
+
+  app.param('providerName', providerManager.getProviderMiddleware(providers))
 
   if (app.get('env') !== 'test') {
     jobs.startCleanUpJob(options.filePath)

packages/@uppy/companion/src/server/middlewares.js

@@ -7,6 +7,7 @@ const tokenService = require('./helpers/jwt')
 const logger = require('./logger')
 const getS3Client = require('./s3-client')
 const { getURLBuilder } = require('./helpers/utils')
+const { isOAuthProvider } = require('./provider/Provider')
 
 exports.hasSessionAndProvider = (req, res, next) => {
   if (!req.session) {
@@ -22,6 +23,21 @@ exports.hasSessionAndProvider = (req, res, next) => {
   return next()
 }
 
+const isOAuthProviderReq = (req) => isOAuthProvider(req.companion.providerClass.authProvider)
+
+/**
+ * Middleware can be used to verify that the current request is to an OAuth provider
+ * This is because not all requests are supported by non-oauth providers (formerly known as SearchProviders)
+ */
+exports.hasOAuthProvider = (req, res, next) => {
+  if (!isOAuthProviderReq(req)) {
+    logger.debug('Provider does not support OAuth.', null, req.id)
+    return res.sendStatus(400)
+  }
+
+  return next()
+}
+
 exports.hasBody = (req, res, next) => {
   if (!req.body) {
     logger.debug('No body attached to req object. Exiting dispatcher.', null, req.id)
@@ -41,18 +57,36 @@ exports.hasSearchQuery = (req, res, next) => {
 }
 
 exports.verifyToken = (req, res, next) => {
+  // for non oauth providers, we just load the static key from options
+  if (!isOAuthProviderReq(req)) {
+    const { providerOptions } = req.companion.options
+    const { providerName } = req.params
+    if (!providerOptions[providerName] || !providerOptions[providerName].key) {
+      logger.info(`unconfigured credentials for ${providerName}`, 'non.oauth.token.load.unset', req.id)
+      res.sendStatus(501)
+      return
+    }
+
+    req.companion.providerToken = providerOptions[providerName].key
+    next()
+    return
+  }
+
+  // Ok, OAuth provider, we fetch the token:
   const token = req.companion.authToken
   if (token == null) {
     logger.info('cannot auth token', 'token.verify.unset', req.id)
-    return res.sendStatus(401)
+    res.sendStatus(401)
+    return
   }
   const { providerName } = req.params
   const { err, payload } = tokenService.verifyEncryptedToken(token, req.companion.options.secret)
   if (err || !payload[providerName]) {
     if (err) {
       logger.error(err.message, 'token.verify.error', req.id)
     }
-    return res.sendStatus(401)
+    res.sendStatus(401)
+    return
   }
   req.companion.providerTokens = payload
   req.companion.providerToken = payload[providerName]
@@ -76,18 +110,6 @@ exports.cookieAuthToken = (req, res, next) => {
   return next()
 }
 
-exports.loadSearchProviderToken = (req, res, next) => {
-  const { providerOptions } = req.companion.options
-  const providerName = req.params.searchProviderName
-  if (!providerOptions[providerName] || !providerOptions[providerName].key) {
-    logger.info(`unconfigured credentials for ${providerName}`, 'searchtoken.load.unset', req.id)
-    return res.sendStatus(501)
-  }
-
-  req.companion.providerToken = providerOptions[providerName].key
-  next()
-}
-
 exports.cors = (options = {}) => (req, res, next) => {
   // HTTP headers are not case sensitive, and express always handles them in lower case, so that's why we lower case them.
   // I believe that HTTP verbs are case sensitive, and should be uppercase.

packages/@uppy/companion/src/server/provider/Provider.js

@@ -24,7 +24,8 @@ class Provider {
    * @param {object} options
    * @returns {Promise}
    */
-  async list (options) { // eslint-disable-line no-unused-vars
+  // eslint-disable-next-line class-methods-use-this,no-unused-vars
+  async list (options) {
     throw new Error('method not implemented')
   }
 
@@ -34,7 +35,8 @@ class Provider {
    * @param {object} options
    * @returns {Promise}
    */
-  async download (options) { // eslint-disable-line no-unused-vars
+  // eslint-disable-next-line class-methods-use-this,no-unused-vars
+  async download (options) {
     throw new Error('method not implemented')
   }
 
@@ -44,7 +46,8 @@ class Provider {
    * @param {object} options
    * @returns {Promise}
    */
-  async thumbnail (options) { // eslint-disable-line no-unused-vars
+  // eslint-disable-next-line class-methods-use-this,no-unused-vars
+  async thumbnail (options) {
     throw new Error('method not implemented')
   }
 
@@ -54,7 +57,8 @@ class Provider {
    * @param {object} options
    * @returns {Promise}
    */
-  async size (options) { // eslint-disable-line no-unused-vars
+  // eslint-disable-next-line class-methods-use-this,no-unused-vars
+  async size (options) {
     throw new Error('method not implemented')
   }
 
@@ -64,17 +68,22 @@ class Provider {
    * @param {object} options
    * @returns {Promise}
    */
-  async deauthorizationCallback (options) { // eslint-disable-line no-unused-vars
+  // eslint-disable-next-line class-methods-use-this,no-unused-vars
+  async deauthorizationCallback (options) {
     // @todo consider doing something like throw new NotImplementedError() instead
     throw new Error('method not implemented')
   }
 
   /**
+   * Name of the OAuth provider. Return empty string if no OAuth provider is needed.
+   *
    * @returns {string}
    */
   static get authProvider () {
-    return ''
+    return undefined
   }
 }
 
 module.exports = Provider
+// OAuth providers are those that have a `static authProvider` set. It means they require OAuth authentication to work
+module.exports.isOAuthProvider = (authProvider) => typeof authProvider === 'string' && authProvider.length > 0

packages/@uppy/companion/src/server/provider/index.js

@@ -14,8 +14,8 @@ const logger = require('../logger')
 const { getCredentialsResolver } = require('./credentials')
 // eslint-disable-next-line
 const Provider = require('./Provider')
-// eslint-disable-next-line
-const SearchProvider = require('./SearchProvider')
+
+const { isOAuthProvider } = Provider
 
 /**
  *
@@ -40,10 +40,9 @@ const providerNameToAuthName = (name, options) => { // eslint-disable-line no-un
  * adds the desired provider module to the request object,
  * based on the providerName parameter specified
  *
- * @param {Record<string, (typeof Provider) | typeof SearchProvider>} providers
- * @param {boolean} [needsProviderCredentials]
+ * @param {Record<string, typeof Provider>} providers
  */
-module.exports.getProviderMiddleware = (providers, needsProviderCredentials) => {
+module.exports.getProviderMiddleware = (providers) => {
   /**
    *
    * @param {object} req
@@ -55,8 +54,9 @@ module.exports.getProviderMiddleware = (providers, needsProviderCredentials) =>
     const ProviderClass = providers[providerName]
     if (ProviderClass && validOptions(req.companion.options)) {
       req.companion.provider = new ProviderClass({ providerName })
+      req.companion.providerClass = ProviderClass
 
-      if (needsProviderCredentials) {
+      if (isOAuthProvider(ProviderClass.authProvider)) {
         req.companion.getProviderCredentials = getCredentialsResolver(providerName, req.companion.options, req)
       }
     } else {
@@ -72,18 +72,11 @@ module.exports.getProviderMiddleware = (providers, needsProviderCredentials) =>
  * @returns {Record<string, typeof Provider>}
  */
 module.exports.getDefaultProviders = () => {
-  const providers = { dropbox, box, drive, facebook, onedrive, zoom, instagram }
+  const providers = { dropbox, box, drive, facebook, onedrive, zoom, instagram, unsplash }
 
   return providers
 }
 
-/**
- * @returns {Record<string, typeof SearchProvider>}
- */
-module.exports.getSearchProviders = () => {
-  return { unsplash }
-}
-
 /**
  *
  * @typedef {{'module': typeof Provider, config: Record<string,unknown>}} CustomProvider
@@ -97,13 +90,16 @@ module.exports.addCustomProviders = (customProviders, providers, grantConfig) =>
     const customProvider = customProviders[providerName]
 
     providers[providerName] = customProvider.module
-    grantConfig[providerName] = {
-      ...customProvider.config,
-      // todo: consider setting these options from a universal point also used
-      // by official providers. It'll prevent these from getting left out if the
-      // requirement changes.
-      callback: `/${providerName}/callback`,
-      transport: 'session',
+
+    if (isOAuthProvider(customProvider.module.authProvider)) {
+      grantConfig[providerName] = {
+        ...customProvider.config,
+        // todo: consider setting these options from a universal point also used
+        // by official providers. It'll prevent these from getting left out if the
+        // requirement changes.
+        callback: `/${providerName}/callback`,
+        transport: 'session',
+      }
     }
   })
 }
@@ -130,7 +126,7 @@ module.exports.addProviderOptions = (companionOptions, grantConfig) => {
   const keys = Object.keys(providerOptions).filter((key) => key !== 'server')
   keys.forEach((providerName) => {
     const authProvider = providerNameToAuthName(providerName, companionOptions)
-    if (authProvider && grantConfig[authProvider]) {
+    if (isOAuthProvider(authProvider) && grantConfig[authProvider]) {
       // explicitly add providerOptions so users don't override other providerOptions.
       grantConfig[authProvider].key = providerOptions[providerName].key
       grantConfig[authProvider].secret = providerOptions[providerName].secret

packages/@uppy/companion/src/server/provider/unsplash/index.js

@@ -1,10 +1,11 @@
 const got = require('got').default
 
-const SearchProvider = require('../SearchProvider')
+const Provider = require('../Provider')
 const { getURLMeta } = require('../../helpers/request')
 const adaptData = require('./adapter')
 const { withProviderErrorHandling } = require('../providerErrors')
 const { prepareStream } = require('../../helpers/utils')
+const { ProviderApiError } = require('../error')
 
 const BASE_URL = 'https://api.unsplash.com'
 
@@ -20,8 +21,12 @@ const getPhotoMeta = async (client, id) => client.get(`photos/${id}`, { response
 /**
  * Adapter for API https://api.unsplash.com
  */
-class Unsplash extends SearchProvider {
+class Unsplash extends Provider {
   async list ({ token, query = { cursor: null, q: null } }) {
+    if (typeof query.q !== 'string') {
+      throw new ProviderApiError('Search query missing', 400)
+    }
+
     return this.#withErrorHandling('provider.unsplash.list.error', async () => {
       const qs = { per_page: 40, query: query.q }
       if (query.cursor) qs.page = query.cursor

packages/@uppy/companion/test/__tests__/provider-manager.js

@@ -1,4 +1,4 @@
-/* global jest:false, test:false, expect:false, describe:false, beforeEach:false */
+/* global test:false, expect:false, describe:false, beforeEach:false */
 
 const providerManager = require('../../src/server/provider')
 const { getCompanionOptions } = require('../../src/standalone/helper')
@@ -148,7 +148,7 @@ describe('Test Custom Provider options', () => {
           key: 'foo_key',
           secret: 'foo_secret',
         },
-        module: jest.mock(),
+        module: { authProvider: 'some_provider' },
       },
     }, providers, grantConfig)
 

packages/@uppy/companion/test/__tests__/providers.js

@@ -373,7 +373,9 @@ describe('connect to provider', () => {
   test.each(providerNames)('connect to %s via grant.js endpoint', (providerName) => {
     const authProvider = AUTH_PROVIDERS[providerName] || providerName
 
-    return request(authServer)
+    if (authProvider.authProvider == null) return
+
+    request(authServer)
       .get(`/${providerName}/connect?foo=bar`)
       .set('uppy-auth-token', token)
       .expect(302)