From 36d2dfdcf9339d164f75490f4e6e7bdcc071f09c Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 23 Feb 2023 11:32:28 +0800 Subject: [PATCH 1/6] merge search provider and provider closes #4308 --- packages/@uppy/companion/src/companion.js | 35 ++++++++------- .../@uppy/companion/src/server/middlewares.js | 45 ++++++++++++++----- .../companion/src/server/provider/Provider.js | 22 ++++++--- .../src/server/provider/SearchProvider.js | 36 --------------- .../companion/src/server/provider/index.js | 40 ++++++++--------- .../src/server/provider/unsplash/index.js | 4 +- .../companion/test/__tests__/providers.js | 5 ++- 7 files changed, 93 insertions(+), 94 deletions(-) delete mode 100644 packages/@uppy/companion/src/server/provider/SearchProvider.js diff --git a/packages/@uppy/companion/src/companion.js b/packages/@uppy/companion/src/companion.js index dae53724a1..e65478fc67 100644 --- a/packages/@uppy/companion/src/companion.js +++ b/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 @@ -116,22 +116,23 @@ module.exports.app = (optionsArg = {}) => { app.use('/s3', s3(options.s3)) app.use('/url', url()) - app.post('/:providerName/preauth', middlewares.hasSessionAndProvider, 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', middlewares.hasSessionAndProvider, controllers.deauthorizationCallback) - app.get('/:providerName/logout', middlewares.hasSessionAndProvider, middlewares.gentleVerifyToken, controllers.logout) - app.get('/:providerName/send-token', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.sendToken) - app.get('/:providerName/list/:id?', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list) - app.post('/:providerName/get/:id', middlewares.hasSessionAndProvider, 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', middlewares.loadSearchProviderToken, controllers.get) - - app.param('providerName', providerManager.getProviderMiddleware(providers, true)) - app.param('searchProviderName', providerManager.getProviderMiddleware(searchProviders)) + app.post('/:providerName/preauth', middlewares.hasSessionAndProvider, middlewares.hasCredentialsProvider, controllers.preauth) + app.get('/:providerName/connect', middlewares.hasSessionAndProvider, middlewares.hasCredentialsProvider, controllers.connect) + app.get('/:providerName/redirect', middlewares.hasSessionAndProvider, middlewares.hasCredentialsProvider, controllers.redirect) + app.get('/:providerName/callback', middlewares.hasSessionAndProvider, middlewares.hasCredentialsProvider, controllers.callback) + app.post('/:providerName/deauthorization/callback', middlewares.hasSessionAndProvider, middlewares.hasCredentialsProvider, controllers.deauthorizationCallback) + app.get('/:providerName/logout', middlewares.hasSessionAndProvider, middlewares.hasCredentialsProvider, middlewares.gentleVerifyToken, controllers.logout) + app.get('/:providerName/send-token', middlewares.hasSessionAndProvider, middlewares.hasCredentialsProvider, middlewares.verifyToken, controllers.sendToken) + + app.get('/:providerName/list/:id?', middlewares.hasSessionAndProvider, middlewares.loadSearchProviderToken, middlewares.verifyToken, middlewares.hasSearchQuery, controllers.list) + app.get('/search/:providerName/list', middlewares.hasSessionAndProvider, middlewares.loadSearchProviderToken, middlewares.verifyToken, middlewares.hasSearchQuery, controllers.list) + + app.post('/:providerName/get/:id', middlewares.hasSessionAndProvider, middlewares.loadSearchProviderToken, middlewares.verifyToken, controllers.get) + app.post('/search/:providerName/get/:id', middlewares.hasSessionAndProvider, middlewares.loadSearchProviderToken, middlewares.verifyToken, controllers.get) + + app.get('/:providerName/thumbnail/:id', middlewares.hasCredentialsProvider, middlewares.hasSessionAndProvider, middlewares.cookieAuthToken, middlewares.verifyToken, controllers.thumbnail) + + app.param('providerName', providerManager.getProviderMiddleware(providers)) if (app.get('env') !== 'test') { jobs.startCleanUpJob(options.filePath) diff --git a/packages/@uppy/companion/src/server/middlewares.js b/packages/@uppy/companion/src/server/middlewares.js index 945b8073ee..4ee8f66f72 100644 --- a/packages/@uppy/companion/src/server/middlewares.js +++ b/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 { isSearchProvider } = require('./provider/Provider') exports.hasSessionAndProvider = (req, res, next) => { if (!req.session || !req.body) { @@ -22,20 +23,37 @@ exports.hasSessionAndProvider = (req, res, next) => { return next() } -exports.hasSearchQuery = (req, res, next) => { - if (typeof req.query.q !== 'string') { - logger.debug('search request has no search query', 'search.query.check', req.id) +exports.hasCredentialsProvider = (req, res, next) => { + if (!req.companion.getProviderCredentials) { + logger.debug('Provider is does not use auth.', null, req.id) return res.sendStatus(400) } return next() } +exports.hasSearchQuery = (req, res, next) => { + if (isSearchProvider(req)) { + if (typeof req.query.q !== 'string') { + logger.debug('search request has no search query', 'search.query.check', req.id) + return res.sendStatus(400) + } + } + + return next() +} + exports.verifyToken = (req, res, next) => { + if (isSearchProvider(req)) { + next() + return + } + 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) @@ -43,7 +61,8 @@ exports.verifyToken = (req, res, next) => { 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] @@ -68,14 +87,18 @@ exports.cookieAuthToken = (req, res, 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) + if (isSearchProvider(req)) { + const { providerOptions } = req.companion.options + const { providerName } = req.params + if (!providerOptions[providerName] || !providerOptions[providerName].key) { + logger.info(`unconfigured credentials for ${providerName}`, 'searchtoken.load.unset', req.id) + res.sendStatus(501) + return + } + + req.companion.providerToken = providerOptions[providerName].key } - req.companion.providerToken = providerOptions[providerName].key next() } diff --git a/packages/@uppy/companion/src/server/provider/Provider.js b/packages/@uppy/companion/src/server/provider/Provider.js index 62d35fa445..835a032fcf 100644 --- a/packages/@uppy/companion/src/server/provider/Provider.js +++ b/packages/@uppy/companion/src/server/provider/Provider.js @@ -1,3 +1,5 @@ +const noAuthProvider = '' + /** * Provider interface defines the specifications of any provider implementation */ @@ -24,7 +26,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 +37,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 +48,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 +59,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,7 +70,8 @@ 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') } @@ -73,8 +80,11 @@ class Provider { * @returns {string} */ static get authProvider () { - return '' + return noAuthProvider } } module.exports = Provider +module.exports.noAuthProvider = noAuthProvider + +module.exports.isSearchProvider = (req) => req.companion.providerClass.authProvider === noAuthProvider diff --git a/packages/@uppy/companion/src/server/provider/SearchProvider.js b/packages/@uppy/companion/src/server/provider/SearchProvider.js deleted file mode 100644 index c3a833417b..0000000000 --- a/packages/@uppy/companion/src/server/provider/SearchProvider.js +++ /dev/null @@ -1,36 +0,0 @@ -/** - * SearchProvider interface defines the specifications of any Search provider implementation - */ -class SearchProvider { - /** - * list the files available based on the search query - * - * @param {object} options - * @returns {Promise} - */ - async list (options) { // eslint-disable-line no-unused-vars - throw new Error('method not implemented') - } - - /** - * download a certain file from the provider files - * - * @param {object} options - * @returns {Promise} - */ - async download (options) { // eslint-disable-line no-unused-vars - throw new Error('method not implemented') - } - - /** - * get the size of a certain file in the provider files - * - * @param {object} options - * @returns {Promise} - */ - async size (options) { // eslint-disable-line no-unused-vars - throw new Error('method not implemented') - } -} - -module.exports = SearchProvider diff --git a/packages/@uppy/companion/src/server/provider/index.js b/packages/@uppy/companion/src/server/provider/index.js index acb62df297..0d0e37d40c 100644 --- a/packages/@uppy/companion/src/server/provider/index.js +++ b/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 { noAuthProvider } = 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} providers - * @param {boolean} [needsProviderCredentials] + * @param {Record} providers */ -module.exports.getProviderMiddleware = (providers, needsProviderCredentials) => { +module.exports.getProviderMiddleware = (providers) => { /** * * @param {object} req @@ -55,7 +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 + const needsProviderCredentials = ProviderClass.authProvider !== noAuthProvider if (needsProviderCredentials) { req.companion.getProviderCredentials = getCredentialsResolver(providerName, req.companion.options, req) } @@ -72,18 +73,11 @@ module.exports.getProviderMiddleware = (providers, needsProviderCredentials) => * @returns {Record} */ 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} - */ -module.exports.getSearchProviders = () => { - return { unsplash } -} - /** * * @typedef {{'module': typeof Provider, config: Record}} CustomProvider @@ -97,13 +91,17 @@ 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', + + const needsProviderCredentials = customProvider.module.authProvider !== noAuthProvider + if (needsProviderCredentials) { + 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 +128,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 (authProvider !== noAuthProvider && grantConfig[authProvider]) { // explicitly add providerOptions so users don't override other providerOptions. grantConfig[authProvider].key = providerOptions[providerName].key grantConfig[authProvider].secret = providerOptions[providerName].secret diff --git a/packages/@uppy/companion/src/server/provider/unsplash/index.js b/packages/@uppy/companion/src/server/provider/unsplash/index.js index d240e63767..53b6590a31 100644 --- a/packages/@uppy/companion/src/server/provider/unsplash/index.js +++ b/packages/@uppy/companion/src/server/provider/unsplash/index.js @@ -1,6 +1,6 @@ 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') @@ -20,7 +20,7 @@ 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 } }) { return this.#withErrorHandling('provider.unsplash.list.error', async () => { const qs = { per_page: 40, query: query.q } diff --git a/packages/@uppy/companion/test/__tests__/providers.js b/packages/@uppy/companion/test/__tests__/providers.js index 6754e45d65..526318c6d5 100644 --- a/packages/@uppy/companion/test/__tests__/providers.js +++ b/packages/@uppy/companion/test/__tests__/providers.js @@ -18,6 +18,7 @@ const defaults = require('../fixtures/constants') const tokenService = require('../../src/server/helpers/jwt') const { getServer } = require('../mockserver') +const { noAuthProvider } = require('../../src/server/provider/Provider') // todo don't share server between tests. rewrite to not use env variables const authServer = getServer({ COMPANION_CLIENT_SOCKET_CONNECT_TIMEOUT: '0' }) @@ -373,7 +374,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 === noAuthProvider) return + + request(authServer) .get(`/${providerName}/connect?foo=bar`) .set('uppy-auth-token', token) .expect(302) From 51d0b26e2d203d74f4a4ed790761f6c9c4d36ade Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 23 Feb 2023 14:51:32 +0800 Subject: [PATCH 2/6] simplify and improve naming --- packages/@uppy/companion/src/companion.js | 32 ++++++------ .../@uppy/companion/src/server/middlewares.js | 52 ++++++++----------- .../companion/src/server/provider/Provider.js | 5 +- .../companion/src/server/provider/index.js | 10 ++-- .../src/server/provider/unsplash/index.js | 5 ++ 5 files changed, 51 insertions(+), 53 deletions(-) diff --git a/packages/@uppy/companion/src/companion.js b/packages/@uppy/companion/src/companion.js index e65478fc67..646a633d3e 100644 --- a/packages/@uppy/companion/src/companion.js +++ b/packages/@uppy/companion/src/companion.js @@ -116,21 +116,23 @@ module.exports.app = (optionsArg = {}) => { app.use('/s3', s3(options.s3)) app.use('/url', url()) - app.post('/:providerName/preauth', middlewares.hasSessionAndProvider, middlewares.hasCredentialsProvider, controllers.preauth) - app.get('/:providerName/connect', middlewares.hasSessionAndProvider, middlewares.hasCredentialsProvider, controllers.connect) - app.get('/:providerName/redirect', middlewares.hasSessionAndProvider, middlewares.hasCredentialsProvider, controllers.redirect) - app.get('/:providerName/callback', middlewares.hasSessionAndProvider, middlewares.hasCredentialsProvider, controllers.callback) - app.post('/:providerName/deauthorization/callback', middlewares.hasSessionAndProvider, middlewares.hasCredentialsProvider, controllers.deauthorizationCallback) - app.get('/:providerName/logout', middlewares.hasSessionAndProvider, middlewares.hasCredentialsProvider, middlewares.gentleVerifyToken, controllers.logout) - app.get('/:providerName/send-token', middlewares.hasSessionAndProvider, middlewares.hasCredentialsProvider, middlewares.verifyToken, controllers.sendToken) - - app.get('/:providerName/list/:id?', middlewares.hasSessionAndProvider, middlewares.loadSearchProviderToken, middlewares.verifyToken, middlewares.hasSearchQuery, controllers.list) - app.get('/search/:providerName/list', middlewares.hasSessionAndProvider, middlewares.loadSearchProviderToken, middlewares.verifyToken, middlewares.hasSearchQuery, controllers.list) - - app.post('/:providerName/get/:id', middlewares.hasSessionAndProvider, middlewares.loadSearchProviderToken, middlewares.verifyToken, controllers.get) - app.post('/search/:providerName/get/:id', middlewares.hasSessionAndProvider, middlewares.loadSearchProviderToken, middlewares.verifyToken, controllers.get) - - app.get('/:providerName/thumbnail/:id', middlewares.hasCredentialsProvider, middlewares.hasSessionAndProvider, middlewares.cookieAuthToken, middlewares.verifyToken, controllers.thumbnail) + app.post('/:providerName/preauth', middlewares.hasSessionAndProvider, 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', middlewares.hasSessionAndProvider, 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) + // backwards compat: + app.get('/search/:providerName/list', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list) + + app.post('/:providerName/get/:id', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.get) + // backwards compat: + app.post('/search/:providerName/get/:id', 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)) diff --git a/packages/@uppy/companion/src/server/middlewares.js b/packages/@uppy/companion/src/server/middlewares.js index 4ee8f66f72..e3a4f97599 100644 --- a/packages/@uppy/companion/src/server/middlewares.js +++ b/packages/@uppy/companion/src/server/middlewares.js @@ -7,7 +7,7 @@ const tokenService = require('./helpers/jwt') const logger = require('./logger') const getS3Client = require('./s3-client') const { getURLBuilder } = require('./helpers/utils') -const { isSearchProvider } = require('./provider/Provider') +const { isOAuthProvider } = require('./provider/Provider') exports.hasSessionAndProvider = (req, res, next) => { if (!req.session || !req.body) { @@ -23,32 +23,38 @@ exports.hasSessionAndProvider = (req, res, next) => { return next() } -exports.hasCredentialsProvider = (req, res, next) => { - if (!req.companion.getProviderCredentials) { - logger.debug('Provider is does not use auth.', null, req.id) +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.hasSearchQuery = (req, res, next) => { - if (isSearchProvider(req)) { - if (typeof req.query.q !== 'string') { - logger.debug('search request has no search query', 'search.query.check', req.id) - return res.sendStatus(400) +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 } - } - return next() -} - -exports.verifyToken = (req, res, next) => { - if (isSearchProvider(req)) { + 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) @@ -86,22 +92,6 @@ exports.cookieAuthToken = (req, res, next) => { return next() } -exports.loadSearchProviderToken = (req, res, next) => { - if (isSearchProvider(req)) { - const { providerOptions } = req.companion.options - const { providerName } = req.params - if (!providerOptions[providerName] || !providerOptions[providerName].key) { - logger.info(`unconfigured credentials for ${providerName}`, 'searchtoken.load.unset', req.id) - res.sendStatus(501) - return - } - - 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. diff --git a/packages/@uppy/companion/src/server/provider/Provider.js b/packages/@uppy/companion/src/server/provider/Provider.js index 835a032fcf..fccaef2d75 100644 --- a/packages/@uppy/companion/src/server/provider/Provider.js +++ b/packages/@uppy/companion/src/server/provider/Provider.js @@ -77,6 +77,8 @@ class Provider { } /** + * Name of the OAuth provider. Return empty string if no OAuth provider is needed. + * * @returns {string} */ static get authProvider () { @@ -87,4 +89,5 @@ class Provider { module.exports = Provider module.exports.noAuthProvider = noAuthProvider -module.exports.isSearchProvider = (req) => req.companion.providerClass.authProvider === noAuthProvider +// OAuth providers are those that have a `static authProvider` set. It means they require OAuth authentication to work +module.exports.isOAuthProvider = (authProvider) => authProvider !== noAuthProvider diff --git a/packages/@uppy/companion/src/server/provider/index.js b/packages/@uppy/companion/src/server/provider/index.js index 0d0e37d40c..a9f709c264 100644 --- a/packages/@uppy/companion/src/server/provider/index.js +++ b/packages/@uppy/companion/src/server/provider/index.js @@ -15,7 +15,7 @@ const { getCredentialsResolver } = require('./credentials') // eslint-disable-next-line const Provider = require('./Provider') -const { noAuthProvider } = Provider +const { isOAuthProvider } = Provider /** * @@ -56,8 +56,7 @@ module.exports.getProviderMiddleware = (providers) => { req.companion.provider = new ProviderClass({ providerName }) req.companion.providerClass = ProviderClass - const needsProviderCredentials = ProviderClass.authProvider !== noAuthProvider - if (needsProviderCredentials) { + if (isOAuthProvider(ProviderClass.authProvider)) { req.companion.getProviderCredentials = getCredentialsResolver(providerName, req.companion.options, req) } } else { @@ -92,8 +91,7 @@ module.exports.addCustomProviders = (customProviders, providers, grantConfig) => providers[providerName] = customProvider.module - const needsProviderCredentials = customProvider.module.authProvider !== noAuthProvider - if (needsProviderCredentials) { + if (isOAuthProvider(customProvider.module.authProvider)) { grantConfig[providerName] = { ...customProvider.config, // todo: consider setting these options from a universal point also used @@ -128,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 !== noAuthProvider && 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 diff --git a/packages/@uppy/companion/src/server/provider/unsplash/index.js b/packages/@uppy/companion/src/server/provider/unsplash/index.js index 53b6590a31..1c3147f013 100644 --- a/packages/@uppy/companion/src/server/provider/unsplash/index.js +++ b/packages/@uppy/companion/src/server/provider/unsplash/index.js @@ -5,6 +5,7 @@ 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' @@ -22,6 +23,10 @@ const getPhotoMeta = async (client, id) => client.get(`photos/${id}`, { response */ 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 From 7753c68843dc24f0319d85076bc45c4aa8976d77 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 13 Mar 2023 17:30:18 +0800 Subject: [PATCH 3/6] Apply suggestions from code review --- packages/@uppy/companion/src/server/provider/Provider.js | 8 ++------ packages/@uppy/companion/test/__tests__/providers.js | 3 +-- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/packages/@uppy/companion/src/server/provider/Provider.js b/packages/@uppy/companion/src/server/provider/Provider.js index fccaef2d75..a658c290dd 100644 --- a/packages/@uppy/companion/src/server/provider/Provider.js +++ b/packages/@uppy/companion/src/server/provider/Provider.js @@ -1,5 +1,3 @@ -const noAuthProvider = '' - /** * Provider interface defines the specifications of any provider implementation */ @@ -82,12 +80,10 @@ class Provider { * @returns {string} */ static get authProvider () { - return noAuthProvider + return undefined } } module.exports = Provider -module.exports.noAuthProvider = noAuthProvider - // OAuth providers are those that have a `static authProvider` set. It means they require OAuth authentication to work -module.exports.isOAuthProvider = (authProvider) => authProvider !== noAuthProvider +module.exports.isOAuthProvider = (authProvider) => authProvider != null diff --git a/packages/@uppy/companion/test/__tests__/providers.js b/packages/@uppy/companion/test/__tests__/providers.js index 526318c6d5..05b562911f 100644 --- a/packages/@uppy/companion/test/__tests__/providers.js +++ b/packages/@uppy/companion/test/__tests__/providers.js @@ -18,7 +18,6 @@ const defaults = require('../fixtures/constants') const tokenService = require('../../src/server/helpers/jwt') const { getServer } = require('../mockserver') -const { noAuthProvider } = require('../../src/server/provider/Provider') // todo don't share server between tests. rewrite to not use env variables const authServer = getServer({ COMPANION_CLIENT_SOCKET_CONNECT_TIMEOUT: '0' }) @@ -374,7 +373,7 @@ describe('connect to provider', () => { test.each(providerNames)('connect to %s via grant.js endpoint', (providerName) => { const authProvider = AUTH_PROVIDERS[providerName] || providerName - if (authProvider.authProvider === noAuthProvider) return + if (authProvider.authProvider == null) return request(authServer) .get(`/${providerName}/connect?foo=bar`) From a799191db6006e6725253e715597ab0fdf0addd6 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 13 Mar 2023 17:57:32 +0800 Subject: [PATCH 4/6] 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 --- packages/@uppy/companion/test/__tests__/provider-manager.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@uppy/companion/test/__tests__/provider-manager.js b/packages/@uppy/companion/test/__tests__/provider-manager.js index ca8f9f31c7..e6b8ddb42e 100644 --- a/packages/@uppy/companion/test/__tests__/provider-manager.js +++ b/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) From 646163132d2c136865e9ac78657dcf96da13016d Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 13 Mar 2023 19:06:58 +0800 Subject: [PATCH 5/6] Update packages/@uppy/companion/src/server/provider/Provider.js --- packages/@uppy/companion/src/server/provider/Provider.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@uppy/companion/src/server/provider/Provider.js b/packages/@uppy/companion/src/server/provider/Provider.js index a658c290dd..453b0ebadd 100644 --- a/packages/@uppy/companion/src/server/provider/Provider.js +++ b/packages/@uppy/companion/src/server/provider/Provider.js @@ -86,4 +86,4 @@ class Provider { 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) => authProvider != null +module.exports.isOAuthProvider = (authProvider) => typeof authProvider === 'string' && authProvider.length > 0 From 4306486694d0837baa82f5aec56f1a0a3f9a466a Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 28 Mar 2023 15:14:22 +0900 Subject: [PATCH 6/6] fix merge oops --- packages/@uppy/companion/src/companion.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@uppy/companion/src/companion.js b/packages/@uppy/companion/src/companion.js index 2c8d3f6085..77ba22bdcb 100644 --- a/packages/@uppy/companion/src/companion.js +++ b/packages/@uppy/companion/src/companion.js @@ -122,8 +122,8 @@ module.exports.app = (optionsArg = {}) => { 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', express.json(), middlewares.hasSessionAndProvider, middlewares.hasBody, middlewares.hasOAuthProvider, controllers.callback) - app.post('/:providerName/deauthorization/callback', middlewares.hasSessionAndProvider, middlewares.hasOAuthProvider, controllers.deauthorizationCallback) + 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)