From df98cca0cd87f3cde6d715bd12098a65df73fc7e Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 9 Aug 2023 16:06:02 +0200 Subject: [PATCH 01/37] remove useless line --- packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index e0991350e2..ce3642bad9 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -431,7 +431,6 @@ export default class ProviderView extends View { currentSelection, files: hasInput ? filterItems(files) : files, folders: hasInput ? filterItems(folders) : folders, - username: this.username, getNextFolder: this.getNextFolder, getFolder: this.getFolder, loadAllFiles: this.opts.loadAllFiles, From 9ccf2ae622a46d5f59033b8a844f705a45306944 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 9 Aug 2023 16:13:21 +0200 Subject: [PATCH 02/37] fix broken cookie removal logic related #4426 --- .../@uppy/companion/src/server/helpers/jwt.js | 36 +++++++++++-------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/packages/@uppy/companion/src/server/helpers/jwt.js b/packages/@uppy/companion/src/server/helpers/jwt.js index b1a9b75dfd..38217e157c 100644 --- a/packages/@uppy/companion/src/server/helpers/jwt.js +++ b/packages/@uppy/companion/src/server/helpers/jwt.js @@ -15,7 +15,6 @@ const { encrypt, decrypt } = require('./utils') // there's no way for them to retry their failed files. // With 400 days, there's still a theoretical possibility but very low. const EXPIRY = 60 * 60 * 24 * 400 -const EXPIRY_MS = EXPIRY * 1000 /** * @@ -78,14 +77,15 @@ module.exports.verifyEncryptedAuthToken = (token, secret, providerName) => { return tokens } -const addToCookies = (res, token, companionOptions, authProvider, prefix) => { +function getCommonCookieOptions ({ companionOptions }) { const cookieOptions = { - maxAge: EXPIRY_MS, httpOnly: true, } // Fix to show thumbnails on Chrome // https://community.transloadit.com/t/dropbox-and-box-thumbnails-returning-401-unauthorized/15781/2 + // Note that sameSite cookies also require secure (which needs https), so thumbnails don't work from localhost + // to test locally, you can manually find the URL of the image and open it in a separate browser tab if (companionOptions.server && companionOptions.server.protocol === 'https') { cookieOptions.sameSite = 'none' cookieOptions.secure = true @@ -94,14 +94,27 @@ const addToCookies = (res, token, companionOptions, authProvider, prefix) => { if (companionOptions.cookieDomain) { cookieOptions.domain = companionOptions.cookieDomain } + + return cookieOptions +} + +const getCookieName = (authProvider) => `uppyAuthToken--${authProvider}` + +const addToCookies = (res, token, companionOptions, authProvider) => { + const cookieOptions = { + ...getCommonCookieOptions({ companionOptions }), + maxAge: EXPIRY * 1000, + httpOnly: true, + } + // send signed token to client. - res.cookie(`${prefix}--${authProvider}`, token, cookieOptions) + res.cookie(getCookieName(authProvider), token, cookieOptions) } module.exports.addToCookiesIfNeeded = (req, res, uppyAuthToken) => { // some providers need the token in cookies for thumbnail/image requests if (req.companion.provider.needsCookieAuth) { - addToCookies(res, uppyAuthToken, req.companion.options, req.companion.provider.authProvider, 'uppyAuthToken') + addToCookies(res, uppyAuthToken, req.companion.options, req.companion.provider.authProvider) } } @@ -112,14 +125,9 @@ module.exports.addToCookiesIfNeeded = (req, res, uppyAuthToken) => { * @param {string} authProvider */ module.exports.removeFromCookies = (res, companionOptions, authProvider) => { - const cookieOptions = { - maxAge: EXPIRY_MS, - httpOnly: true, - } - - if (companionOptions.cookieDomain) { - cookieOptions.domain = companionOptions.cookieDomain - } + // options must be identical to those given to res.cookie(), excluding expires and maxAge. + // https://expressjs.com/en/api.html#res.clearCookie + const cookieOptions = getCommonCookieOptions({ companionOptions }) - res.clearCookie(`uppyAuthToken--${authProvider}`, cookieOptions) + res.clearCookie(getCookieName(authProvider), cookieOptions) } From 04128f9404c7a4fca37238c59d6eb8de7d0385f1 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 9 Aug 2023 16:15:03 +0200 Subject: [PATCH 03/37] fix mime type of thumbnails not critical but some browsers might have problems --- packages/@uppy/companion/src/server/controllers/thumbnail.js | 1 + packages/@uppy/companion/src/server/provider/dropbox/index.js | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/@uppy/companion/src/server/controllers/thumbnail.js b/packages/@uppy/companion/src/server/controllers/thumbnail.js index 0994964ed2..cd6c7a7dfa 100644 --- a/packages/@uppy/companion/src/server/controllers/thumbnail.js +++ b/packages/@uppy/companion/src/server/controllers/thumbnail.js @@ -10,6 +10,7 @@ async function thumbnail (req, res, next) { try { const { stream } = await provider.thumbnail({ id, token: accessToken }) + res.set('Content-Type', 'image/jpeg') stream.pipe(res) } catch (err) { if (err.isAuthError) res.sendStatus(401) diff --git a/packages/@uppy/companion/src/server/provider/dropbox/index.js b/packages/@uppy/companion/src/server/provider/dropbox/index.js index 50b4989ea6..287803c6fb 100644 --- a/packages/@uppy/companion/src/server/provider/dropbox/index.js +++ b/packages/@uppy/companion/src/server/provider/dropbox/index.js @@ -100,7 +100,7 @@ class DropBox extends Provider { return this.#withErrorHandling('provider.dropbox.thumbnail.error', async () => { const stream = getClient({ token }).stream.post('files/get_thumbnail_v2', { prefixUrl: 'https://content.dropboxapi.com/2', - headers: { 'Dropbox-API-Arg': httpHeaderSafeJson({ resource: { '.tag': 'path', path: `${id}` }, size: 'w256h256' }) }, + headers: { 'Dropbox-API-Arg': httpHeaderSafeJson({ resource: { '.tag': 'path', path: `${id}` }, size: 'w256h256', format: 'jpeg' }) }, body: Buffer.alloc(0), responseType: 'json', }) From 9ceac7c409a6602af8e24d918a7f6768f6f1c2b6 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 9 Aug 2023 16:39:27 +0200 Subject: [PATCH 04/37] simplify/speedup token generation so we don't have to decode/decrypt/encode/encrypt so many times --- .../src/server/controllers/connect.js | 14 ++++++++------ .../src/server/helpers/oauth-state.js | 19 +++++++------------ .../@uppy/companion/test/mockoauthstate.js | 2 -- 3 files changed, 15 insertions(+), 20 deletions(-) diff --git a/packages/@uppy/companion/src/server/controllers/connect.js b/packages/@uppy/companion/src/server/controllers/connect.js index 1e98927744..f34cab40e9 100644 --- a/packages/@uppy/companion/src/server/controllers/connect.js +++ b/packages/@uppy/companion/src/server/controllers/connect.js @@ -9,23 +9,25 @@ const oAuthState = require('../helpers/oauth-state') */ module.exports = function connect (req, res) { const { secret } = req.companion.options - let state = oAuthState.generateState(secret) + const stateObj = oAuthState.generateState() + if (req.query.state) { - const origin = JSON.parse(atob(req.query.state)) - state = oAuthState.addToState(state, origin, secret) + const { origin } = JSON.parse(atob(req.query.state)) + stateObj.origin = origin } if (req.companion.options.server.oauthDomain) { - state = oAuthState.addToState(state, { companionInstance: req.companion.buildURL('', true) }, secret) + stateObj.companionInstance = req.companion.buildURL('', true) } if (req.companion.clientVersion) { - state = oAuthState.addToState(state, { clientVersion: req.companion.clientVersion }, secret) + stateObj.clientVersion = req.companion.clientVersion } if (req.query.uppyPreAuthToken) { - state = oAuthState.addToState(state, { preAuthToken: req.query.uppyPreAuthToken }, secret) + stateObj.preAuthToken = req.query.uppyPreAuthToken } + const state = oAuthState.encodeState(stateObj, secret) res.redirect(req.companion.buildURL(`/connect/${req.companion.provider.authProvider}?state=${state}`, true)) } diff --git a/packages/@uppy/companion/src/server/helpers/oauth-state.js b/packages/@uppy/companion/src/server/helpers/oauth-state.js index d833376079..bebe9fcf33 100644 --- a/packages/@uppy/companion/src/server/helpers/oauth-state.js +++ b/packages/@uppy/companion/src/server/helpers/oauth-state.js @@ -2,29 +2,24 @@ const crypto = require('node:crypto') const atob = require('atob') const { encrypt, decrypt } = require('./utils') -const setState = (state, secret) => { +module.exports.encodeState = (state, secret) => { const encodedState = Buffer.from(JSON.stringify(state)).toString('base64') return encrypt(encodedState, secret) } -const getState = (state, secret) => { +const decodeState = (state, secret) => { const encodedState = decrypt(state, secret) return JSON.parse(atob(encodedState)) } -module.exports.generateState = (secret) => { - const state = {} - state.id = crypto.randomBytes(10).toString('hex') - return setState(state, secret) -} - -module.exports.addToState = (state, data, secret) => { - const stateObj = getState(state, secret) - return setState(Object.assign(stateObj, data), secret) +module.exports.generateState = () => { + return { + id: crypto.randomBytes(10).toString('hex'), + } } module.exports.getFromState = (state, name, secret) => { - return getState(state, secret)[name] + return decodeState(state, secret)[name] } module.exports.getDynamicStateFromRequest = (req) => { diff --git a/packages/@uppy/companion/test/mockoauthstate.js b/packages/@uppy/companion/test/mockoauthstate.js index 63fe9d8b83..bc1523b740 100644 --- a/packages/@uppy/companion/test/mockoauthstate.js +++ b/packages/@uppy/companion/test/mockoauthstate.js @@ -1,7 +1,5 @@ module.exports = () => { return { - generateState: () => 'some-cool-nice-encrytpion', - addToState: () => 'some-cool-nice-encrytpion', getFromState: (state, key) => { if (state === 'state-with-invalid-instance-url') { return 'http://localhost:3452' From 600b2d0b40839dbc7a5cab9c79b9fef42b7daaad Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 9 Aug 2023 23:29:25 +0200 Subject: [PATCH 05/37] use instanceof instead of prop check --- packages/@uppy/companion-client/src/AuthError.js | 2 +- packages/@uppy/companion-client/src/Provider.js | 5 +++-- packages/@uppy/companion-client/src/RequestClient.js | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/@uppy/companion-client/src/AuthError.js b/packages/@uppy/companion-client/src/AuthError.js index 14517d3400..b985bf0953 100644 --- a/packages/@uppy/companion-client/src/AuthError.js +++ b/packages/@uppy/companion-client/src/AuthError.js @@ -4,7 +4,7 @@ class AuthError extends Error { constructor () { super('Authorization required') this.name = 'AuthError' - this.isAuthError = true + this.isAuthError = true // todo remove in next major and pull AuthError into a shared package } } diff --git a/packages/@uppy/companion-client/src/Provider.js b/packages/@uppy/companion-client/src/Provider.js index 41790b13f3..e72e60a777 100644 --- a/packages/@uppy/companion-client/src/Provider.js +++ b/packages/@uppy/companion-client/src/Provider.js @@ -2,6 +2,7 @@ import RequestClient from './RequestClient.js' import * as tokenStorage from './tokenStorage.js' +import AuthError from './AuthError.js' const getName = (id) => { return id.split('-').map((s) => s.charAt(0).toUpperCase() + s.slice(1)).join(' ') @@ -160,10 +161,10 @@ export default class Provider extends RequestClient { await this.#refreshingTokenPromise try { - // throw Object.assign(new Error(), { isAuthError: true }) // testing simulate access token expired (to refresh token) + // throw new AuthError() // testing simulate access token expired (to refresh token) return await super.request(...args) } catch (err) { - if (!err.isAuthError) throw err // only handle auth errors (401 from provider) + if (!(err instanceof AuthError)) throw err // only handle auth errors (401 from provider) await this.#refreshingTokenPromise diff --git a/packages/@uppy/companion-client/src/RequestClient.js b/packages/@uppy/companion-client/src/RequestClient.js index 69fcc882a6..bbbf61b7b0 100644 --- a/packages/@uppy/companion-client/src/RequestClient.js +++ b/packages/@uppy/companion-client/src/RequestClient.js @@ -164,7 +164,7 @@ export default class RequestClient { if (!skipPostResponse) this.onReceiveResponse(response) return handleJSONResponse(response) } catch (err) { - if (err?.isAuthError) throw err + if (err instanceof AuthError) throw err // auth errors must be passed through throw new ErrorWithCause(`Could not ${method} ${this.#getUrl(path)}`, { cause: err }) } } From da67ac9678664e3a6bb2ae5f86a7f432ffd35f69 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 10 Aug 2023 00:11:45 +0200 Subject: [PATCH 06/37] Implement alternative provider auth New concept "simple auth" - authentication that happens immediately (in one http request) without redirecting to any third party. uppyAuthToken initially used to simply contain an encrypted & json encoded OAuth2 access_token for a specific provider. Then we added refresh tokens as well inside uppyAuthToken #4448. Now we also allow storing other state or parameters needed for that specific provider, like username, password, host name, webdav URL etc... This is needed for providers like webdav, ftp etc, where the user needs to give some more input data while authenticating Companion: - `providerTokens` has been renamed to `providerUserSession` because it now includes not only tokens, but a user's session with a provider. Companion `Provider` class: - New `hasSimpleAuth` static boolean property - whether this provider uses simple auth - uppyAuthToken expiry default 24hr again for providers that don't support refresh tokens - make uppyAuthToken expiry configurable per provider - new `authStateExpiry` static property (defaults to 24hr) - new static property `grantDynamicToUserSession`, allows providers to specify which state from Grant `dynamic` to include into the provider's `providerUserSession`. --- packages/@uppy/box/src/Box.jsx | 1 + .../@uppy/companion-client/src/Provider.js | 22 +++++-- packages/@uppy/companion/src/companion.js | 4 +- .../src/server/controllers/callback.js | 15 +++-- .../src/server/controllers/connect.js | 19 ++++++- .../companion/src/server/controllers/get.js | 5 +- .../companion/src/server/controllers/index.js | 1 + .../companion/src/server/controllers/list.js | 8 ++- .../src/server/controllers/logout.js | 11 ++-- .../src/server/controllers/oauth-redirect.js | 2 +- .../src/server/controllers/refresh-token.js | 23 +++----- .../src/server/controllers/send-token.js | 2 +- .../src/server/controllers/simple-auth.js | 31 ++++++++++ .../src/server/controllers/thumbnail.js | 8 +-- .../@uppy/companion/src/server/helpers/jwt.js | 45 +++++++++------ .../src/server/helpers/oauth-state.js | 6 +- .../@uppy/companion/src/server/middlewares.js | 57 +++++++++++-------- .../companion/src/server/provider/Provider.js | 33 ++++++++++- .../src/server/provider/credentials.js | 4 +- .../src/server/provider/drive/index.js | 5 ++ .../src/server/provider/dropbox/index.js | 5 ++ .../companion/src/server/provider/index.js | 13 +++-- packages/@uppy/dropbox/src/Dropbox.jsx | 1 + packages/@uppy/facebook/src/Facebook.jsx | 1 + .../@uppy/google-drive/src/GoogleDrive.jsx | 1 + packages/@uppy/instagram/src/Instagram.jsx | 1 + packages/@uppy/onedrive/src/OneDrive.jsx | 1 + .../src/ProviderView/AuthView.jsx | 54 +++++++++++------- .../src/ProviderView/ProviderView.jsx | 7 ++- packages/@uppy/zoom/src/Zoom.jsx | 1 + 30 files changed, 267 insertions(+), 120 deletions(-) create mode 100644 packages/@uppy/companion/src/server/controllers/simple-auth.js diff --git a/packages/@uppy/box/src/Box.jsx b/packages/@uppy/box/src/Box.jsx index 93a202a1db..2fd823b383 100644 --- a/packages/@uppy/box/src/Box.jsx +++ b/packages/@uppy/box/src/Box.jsx @@ -30,6 +30,7 @@ export default class Box extends UIPlugin { companionCookiesRule: this.opts.companionCookiesRule, provider: 'box', pluginId: this.id, + supportsRefreshToken: false, }) this.defaultLocale = locale diff --git a/packages/@uppy/companion-client/src/Provider.js b/packages/@uppy/companion-client/src/Provider.js index e72e60a777..cae6b42102 100644 --- a/packages/@uppy/companion-client/src/Provider.js +++ b/packages/@uppy/companion-client/src/Provider.js @@ -40,6 +40,7 @@ export default class Provider extends RequestClient { this.tokenKey = `companion-${this.pluginId}-auth-token` this.companionKeysParams = this.opts.companionKeysParams this.preAuthToken = null + this.supportsRefreshToken = opts.supportsRefreshToken ?? true // todo false in next major } async headers () { @@ -74,7 +75,7 @@ export default class Provider extends RequestClient { return this.uppy.getPlugin(this.pluginId).storage.getItem(this.tokenKey) } - async #removeAuthToken () { + async removeAuthToken () { return this.uppy.getPlugin(this.pluginId).storage.removeItem(this.tokenKey) } @@ -92,11 +93,18 @@ export default class Provider extends RequestClient { } } - authUrl (queries = {}) { + // eslint-disable-next-line class-methods-use-this + authQuery () { + return {} + } + + authUrl ({ formSubmitEvent, query } = {}) { const params = new URLSearchParams({ + ...query, state: btoa(JSON.stringify({ origin: getOrigin() })), - ...queries, + ...this.authQuery({ formSubmitEvent }), }) + if (this.preAuthToken) { params.set('uppyPreAuthToken', this.preAuthToken) } @@ -104,12 +112,13 @@ export default class Provider extends RequestClient { return `${this.hostname}/${this.id}/connect?${params}` } - async login (queries) { + async login ({ uppyVersions, formSubmitEvent }) { await this.ensurePreAuth() return new Promise((resolve, reject) => { - const link = this.authUrl(queries) + const link = this.authUrl({ query: { uppyVersions }, formSubmitEvent }) const authWindow = window.open(link, '_blank') + const handleToken = (e) => { if (e.source !== authWindow) { this.uppy.log.warn('ignoring event from unknown source', e) @@ -164,6 +173,7 @@ export default class Provider extends RequestClient { // throw new AuthError() // testing simulate access token expired (to refresh token) return await super.request(...args) } catch (err) { + if (!this.supportsRefreshToken) throw err if (!(err instanceof AuthError)) throw err // only handle auth errors (401 from provider) await this.#refreshingTokenPromise @@ -205,7 +215,7 @@ export default class Provider extends RequestClient { async logout (options) { const response = await this.get(`${this.id}/logout`, options) - await this.#removeAuthToken() + await this.removeAuthToken() return response } diff --git a/packages/@uppy/companion/src/companion.js b/packages/@uppy/companion/src/companion.js index 3e314e72d0..f1f5a43774 100644 --- a/packages/@uppy/companion/src/companion.js +++ b/packages/@uppy/companion/src/companion.js @@ -126,6 +126,8 @@ module.exports.app = (optionsArg = {}) => { 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.post('/:providerName/simple-auth', express.json(), middlewares.hasSessionAndProvider, middlewares.hasBody, middlewares.hasSimpleAuthProvider, controllers.simpleAuth) + app.get('/:providerName/list/:id?', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list) // backwards compat: app.get('/search/:providerName/list', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list) @@ -136,7 +138,7 @@ module.exports.app = (optionsArg = {}) => { app.get('/:providerName/thumbnail/:id', middlewares.hasSessionAndProvider, middlewares.hasOAuthProvider, middlewares.cookieAuthToken, middlewares.verifyToken, controllers.thumbnail) - app.param('providerName', providerManager.getProviderMiddleware(providers)) + app.param('providerName', providerManager.getProviderMiddleware(providers, grantConfig)) if (app.get('env') !== 'test') { jobs.startCleanUpJob(options.filePath) diff --git a/packages/@uppy/companion/src/server/controllers/callback.js b/packages/@uppy/companion/src/server/controllers/callback.js index dcd7dadd22..220d06e030 100644 --- a/packages/@uppy/companion/src/server/controllers/callback.js +++ b/packages/@uppy/companion/src/server/controllers/callback.js @@ -33,25 +33,28 @@ module.exports = function callback (req, res, next) { // eslint-disable-line no- const grant = req.session.grant || {} + const grantDynamic = oAuthState.getGrantDynamicFromRequest(req) + const origin = grantDynamic.state && oAuthState.getFromState(grantDynamic.state, 'origin', req.companion.options.secret) + if (!grant.response?.access_token) { logger.debug(`Did not receive access token for provider ${providerName}`, null, req.id) logger.debug(grant.response, 'callback.oauth.resp', req.id) - const state = oAuthState.getDynamicStateFromRequest(req) - const origin = state && oAuthState.getFromState(state, 'origin', req.companion.options.secret) return res.status(400).send(closePageHtml(origin)) } - if (!req.companion.allProvidersTokens) req.companion.allProvidersTokens = {} - req.companion.allProvidersTokens[providerName] = { + req.companion.providerUserSession = { accessToken: grant.response.access_token, refreshToken: grant.response.refresh_token, // might be undefined for some providers + ...req.companion.providerClass.grantDynamicToUserSession({ grantDynamic }), } + logger.debug(`Generating auth token for provider ${providerName}`, null, req.id) const uppyAuthToken = tokenService.generateEncryptedAuthToken( - req.companion.allProvidersTokens, req.companion.options.secret, + { [providerName]: req.companion.providerUserSession }, + req.companion.options.secret, req.companion.providerClass.authStateExpiry, ) - tokenService.addToCookiesIfNeeded(req, res, uppyAuthToken) + tokenService.addToCookiesIfNeeded(req, res, uppyAuthToken, req.companion.providerClass.authStateExpiry) return res.redirect(req.companion.buildURL(`/${providerName}/send-token?uppyAuthToken=${uppyAuthToken}`, true)) } diff --git a/packages/@uppy/companion/src/server/controllers/connect.js b/packages/@uppy/companion/src/server/controllers/connect.js index f34cab40e9..3db52193c9 100644 --- a/packages/@uppy/companion/src/server/controllers/connect.js +++ b/packages/@uppy/companion/src/server/controllers/connect.js @@ -1,6 +1,11 @@ const atob = require('atob') const oAuthState = require('../helpers/oauth-state') +const queryString = (params, prefix = '?') => { + const str = new URLSearchParams(params).toString() + return str ? `${prefix}${str}` : '' +} + /** * initializes the oAuth flow for a provider. * @@ -29,5 +34,17 @@ module.exports = function connect (req, res) { } const state = oAuthState.encodeState(stateObj, secret) - res.redirect(req.companion.buildURL(`/connect/${req.companion.provider.authProvider}?state=${state}`, true)) + const { provider, providerGrantConfig } = req.companion + + // pass along grant's dynamic config (if specified for the provider in its grant config `dynamic` section) + const grantDynamicConfig = Object.fromEntries(providerGrantConfig.dynamic?.map(p => [p, req.query[p]]) || []) + + const providerName = provider.authProvider + const qs = queryString({ + ...grantDynamicConfig, + state, + }) + + // Now we redirect to grant's /connect endpoint, see `app.use(Grant(grantConfig))` + res.redirect(req.companion.buildURL(`/connect/${providerName}${qs}`, true)) } diff --git a/packages/@uppy/companion/src/server/controllers/get.js b/packages/@uppy/companion/src/server/controllers/get.js index 64babedbfb..c1a6570d90 100644 --- a/packages/@uppy/companion/src/server/controllers/get.js +++ b/packages/@uppy/companion/src/server/controllers/get.js @@ -3,7 +3,8 @@ const { startDownUpload } = require('../helpers/upload') async function get (req, res) { const { id } = req.params - const { accessToken } = req.companion.providerTokens + const { providerUserSession } = req.companion + const { accessToken } = providerUserSession const { provider } = req.companion async function getSize () { @@ -11,7 +12,7 @@ async function get (req, res) { } async function download () { - const { stream } = await provider.download({ id, token: accessToken, query: req.query }) + const { stream } = await provider.download({ id, token: accessToken, providerUserSession, query: req.query }) return stream } diff --git a/packages/@uppy/companion/src/server/controllers/index.js b/packages/@uppy/companion/src/server/controllers/index.js index 4410eec3b9..90dd93fb12 100644 --- a/packages/@uppy/companion/src/server/controllers/index.js +++ b/packages/@uppy/companion/src/server/controllers/index.js @@ -6,6 +6,7 @@ module.exports = { get: require('./get'), thumbnail: require('./thumbnail'), list: require('./list'), + simpleAuth: require('./simple-auth'), logout: require('./logout'), connect: require('./connect'), preauth: require('./preauth'), diff --git a/packages/@uppy/companion/src/server/controllers/list.js b/packages/@uppy/companion/src/server/controllers/list.js index 284660c1be..43afb0b7c9 100644 --- a/packages/@uppy/companion/src/server/controllers/list.js +++ b/packages/@uppy/companion/src/server/controllers/list.js @@ -1,10 +1,14 @@ const { respondWithError } = require('../provider/error') async function list ({ query, params, companion }, res, next) { - const { accessToken } = companion.providerTokens + const { providerUserSession } = companion + const { accessToken } = providerUserSession try { - const data = await companion.provider.list({ companion, token: accessToken, directory: params.id, query }) + // todo remove backward compat `token` param from all provider methods (because it can be found in providerUserSession) + const data = await companion.provider.list({ + companion, token: accessToken, providerUserSession, directory: params.id, query, + }) res.json(data) } catch (err) { if (respondWithError(err, res)) return diff --git a/packages/@uppy/companion/src/server/controllers/logout.js b/packages/@uppy/companion/src/server/controllers/logout.js index 1ec87e303f..741dc61dda 100644 --- a/packages/@uppy/companion/src/server/controllers/logout.js +++ b/packages/@uppy/companion/src/server/controllers/logout.js @@ -13,20 +13,19 @@ async function logout (req, res, next) { req.session.grant.dynamic = null } } - const { providerName } = req.params const { companion } = req - const tokens = companion.allProvidersTokens ? companion.allProvidersTokens[providerName] : null + const { providerUserSession } = companion - if (!tokens) { + if (!providerUserSession) { cleanSession() res.json({ ok: true, revoked: false }) return } try { - const { accessToken } = tokens - const data = await companion.provider.logout({ token: accessToken, companion }) - delete companion.allProvidersTokens[providerName] + const { accessToken } = providerUserSession + const data = await companion.provider.logout({ token: accessToken, providerUserSession, companion }) + delete companion.providerUserSession tokenService.removeFromCookies(res, companion.options, companion.provider.authProvider) cleanSession() res.json({ ok: true, ...data }) diff --git a/packages/@uppy/companion/src/server/controllers/oauth-redirect.js b/packages/@uppy/companion/src/server/controllers/oauth-redirect.js index a0ef7ba477..ff4f095448 100644 --- a/packages/@uppy/companion/src/server/controllers/oauth-redirect.js +++ b/packages/@uppy/companion/src/server/controllers/oauth-redirect.js @@ -16,7 +16,7 @@ module.exports = function oauthRedirect (req, res) { return } - const state = oAuthState.getDynamicStateFromRequest(req) + const { state } = oAuthState.getGrantDynamicFromRequest(req) if (!state) { res.status(400).send('Cannot find state in session') return diff --git a/packages/@uppy/companion/src/server/controllers/refresh-token.js b/packages/@uppy/companion/src/server/controllers/refresh-token.js index 99203ebe3c..6a8c3162c9 100644 --- a/packages/@uppy/companion/src/server/controllers/refresh-token.js +++ b/packages/@uppy/companion/src/server/controllers/refresh-token.js @@ -10,36 +10,31 @@ async function refreshToken (req, res, next) { const { key: clientId, secret: clientSecret } = req.companion.options.providerOptions[providerName] - const providerTokens = req.companion.allProvidersTokens[providerName] + const { providerUserSession } = req.companion // not all providers have refresh tokens - if (providerTokens.refreshToken == null) { + if (providerUserSession.refreshToken == null) { res.sendStatus(401) return } try { const data = await req.companion.provider.refreshToken({ - clientId, clientSecret, refreshToken: providerTokens.refreshToken, + clientId, clientSecret, refreshToken: providerUserSession.refreshToken, }) - const newAllProvidersTokens = { - ...req.companion.allProvidersTokens, - [providerName]: { - ...providerTokens, - accessToken: data.accessToken, - }, + req.companion.providerUserSession = { + ...providerUserSession, + accessToken: data.accessToken, } - req.companion.allProvidersTokens = newAllProvidersTokens - req.companion.providerTokens = newAllProvidersTokens[providerName] - logger.debug(`Generating refreshed auth token for provider ${providerName}`, null, req.id) const uppyAuthToken = tokenService.generateEncryptedAuthToken( - req.companion.allProvidersTokens, req.companion.options.secret, + { [providerName]: req.companion.providerUserSession }, + req.companion.options.secret, req.companion.providerClass.authStateExpiry, ) - tokenService.addToCookiesIfNeeded(req, res, uppyAuthToken) + tokenService.addToCookiesIfNeeded(req, res, uppyAuthToken, req.companion.providerClass.authStateExpiry) res.send({ uppyAuthToken }) } catch (err) { diff --git a/packages/@uppy/companion/src/server/controllers/send-token.js b/packages/@uppy/companion/src/server/controllers/send-token.js index 17cc4c1601..60bac3474e 100644 --- a/packages/@uppy/companion/src/server/controllers/send-token.js +++ b/packages/@uppy/companion/src/server/controllers/send-token.js @@ -33,7 +33,7 @@ const htmlContent = (token, origin) => { module.exports = function sendToken (req, res, next) { const uppyAuthToken = req.companion.authToken - const state = oAuthState.getDynamicStateFromRequest(req) + const { state } = oAuthState.getGrantDynamicFromRequest(req) if (state) { const origin = oAuthState.getFromState(state, 'origin', req.companion.options.secret) const allowedClients = req.companion.options.clients diff --git a/packages/@uppy/companion/src/server/controllers/simple-auth.js b/packages/@uppy/companion/src/server/controllers/simple-auth.js new file mode 100644 index 0000000000..91d900a152 --- /dev/null +++ b/packages/@uppy/companion/src/server/controllers/simple-auth.js @@ -0,0 +1,31 @@ +const tokenService = require('../helpers/jwt') +const { respondWithError } = require('../provider/error') +const logger = require('../logger') + +async function simpleAuth (req, res, next) { + const { providerName } = req.params + + try { + const simpleAuthResponse = await req.companion.provider.simpleAuth({ requestBody: req.body }) + + req.companion.providerUserSession = { + ...req.companion.providerUserSession, + ...simpleAuthResponse, + } + + logger.debug(`Generating simple auth token for provider ${providerName}`, null, req.id) + const uppyAuthToken = tokenService.generateEncryptedAuthToken( + { [providerName]: req.companion.providerUserSession }, + req.companion.options.secret, req.companion.providerClass.authStateExpiry, + ) + + tokenService.addToCookiesIfNeeded(req, res, uppyAuthToken, req.companion.providerClass.authStateExpiry) + + res.send({ uppyAuthToken }) + } catch (err) { + if (respondWithError(err, res)) return + next(err) + } +} + +module.exports = simpleAuth diff --git a/packages/@uppy/companion/src/server/controllers/thumbnail.js b/packages/@uppy/companion/src/server/controllers/thumbnail.js index cd6c7a7dfa..06de9be9fc 100644 --- a/packages/@uppy/companion/src/server/controllers/thumbnail.js +++ b/packages/@uppy/companion/src/server/controllers/thumbnail.js @@ -4,12 +4,12 @@ * @param {object} res */ async function thumbnail (req, res, next) { - const { providerName, id } = req.params - const { accessToken } = req.companion.allProvidersTokens[providerName] - const { provider } = req.companion + const { id } = req.params + const { provider, providerUserSession } = req.companion + const { accessToken } = providerUserSession try { - const { stream } = await provider.thumbnail({ id, token: accessToken }) + const { stream } = await provider.thumbnail({ id, token: accessToken, providerUserSession }) res.set('Content-Type', 'image/jpeg') stream.pipe(res) } catch (err) { diff --git a/packages/@uppy/companion/src/server/helpers/jwt.js b/packages/@uppy/companion/src/server/helpers/jwt.js index 38217e157c..0db6249a47 100644 --- a/packages/@uppy/companion/src/server/helpers/jwt.js +++ b/packages/@uppy/companion/src/server/helpers/jwt.js @@ -1,10 +1,13 @@ const jwt = require('jsonwebtoken') const { encrypt, decrypt } = require('./utils') -// The Uppy auth token is a (JWT) container around provider OAuth access & refresh tokens. -// Providers themselves will verify these inner tokens. +// The Uppy auth token is an encrypted JWT & JSON encoded container. +// It used to simply contain an OAuth access_token and refresh_token for a specific provider. +// However now we allow more data to be stored in it. This allows for storing other state or parameters needed for that +// specific provider, like username, password, host names etc. +// The different providers APIs themselves will verify these inner tokens through Provider classes. // The expiry of the Uppy auth token should be higher than the expiry of the refresh token. -// Because some refresh tokens never expire, we set the Uppy auth token expiry very high. +// Because some refresh tokens normally never expire, we set the Uppy auth token expiry very high. // Chrome has a maximum cookie expiry of 400 days, so we'll use that (we also store the auth token in a cookie) // // If the Uppy auth token expiry were set too low (e.g. 24hr), we could risk this situation: @@ -14,15 +17,21 @@ const { encrypt, decrypt } = require('./utils') // even though the provider refresh token would still have been accepted and // there's no way for them to retry their failed files. // With 400 days, there's still a theoretical possibility but very low. -const EXPIRY = 60 * 60 * 24 * 400 +const MAX_AGE_REFRESH_TOKEN = 60 * 60 * 24 * 400 + +const MAX_AGE_24H = 60 * 60 * 24 + +module.exports.MAX_AGE_24H = MAX_AGE_24H +module.exports.MAX_AGE_REFRESH_TOKEN = MAX_AGE_REFRESH_TOKEN /** * * @param {*} data * @param {string} secret + * @param {number} maxAge */ -const generateToken = (data, secret) => { - return jwt.sign({ data }, secret, { expiresIn: EXPIRY }) +const generateToken = (data, secret, maxAge) => { + return jwt.sign({ data }, secret, { expiresIn: maxAge }) } /** @@ -40,18 +49,17 @@ const verifyToken = (token, secret) => { * @param {*} payload * @param {string} secret */ -module.exports.generateEncryptedToken = (payload, secret) => { +module.exports.generateEncryptedToken = (payload, secret, maxAge = MAX_AGE_24H) => { // return payload // for easier debugging - return encrypt(generateToken(payload, secret), secret) + return encrypt(generateToken(payload, secret, maxAge), secret) } /** - * * @param {*} payload * @param {string} secret */ -module.exports.generateEncryptedAuthToken = (payload, secret) => { - return module.exports.generateEncryptedToken(JSON.stringify(payload), secret) +module.exports.generateEncryptedAuthToken = (payload, secret, maxAge) => { + return module.exports.generateEncryptedToken(JSON.stringify(payload), secret, maxAge) } /** @@ -100,21 +108,26 @@ function getCommonCookieOptions ({ companionOptions }) { const getCookieName = (authProvider) => `uppyAuthToken--${authProvider}` -const addToCookies = (res, token, companionOptions, authProvider) => { +const addToCookies = ({ res, token, companionOptions, authProvider, maxAge = MAX_AGE_24H * 1000 }) => { const cookieOptions = { ...getCommonCookieOptions({ companionOptions }), - maxAge: EXPIRY * 1000, - httpOnly: true, + maxAge, } // send signed token to client. res.cookie(getCookieName(authProvider), token, cookieOptions) } -module.exports.addToCookiesIfNeeded = (req, res, uppyAuthToken) => { +module.exports.addToCookiesIfNeeded = (req, res, uppyAuthToken, maxAge) => { // some providers need the token in cookies for thumbnail/image requests if (req.companion.provider.needsCookieAuth) { - addToCookies(res, uppyAuthToken, req.companion.options, req.companion.provider.authProvider) + addToCookies({ + res, + token: uppyAuthToken, + companionOptions: req.companion.options, + authProvider: req.companion.provider.authProvider, + maxAge, + }) } } diff --git a/packages/@uppy/companion/src/server/helpers/oauth-state.js b/packages/@uppy/companion/src/server/helpers/oauth-state.js index bebe9fcf33..4a7d1a9b9c 100644 --- a/packages/@uppy/companion/src/server/helpers/oauth-state.js +++ b/packages/@uppy/companion/src/server/helpers/oauth-state.js @@ -22,8 +22,6 @@ module.exports.getFromState = (state, name, secret) => { return decodeState(state, secret)[name] } -module.exports.getDynamicStateFromRequest = (req) => { - const dynamic = (req.session.grant || {}).dynamic || {} - const { state } = dynamic - return state +module.exports.getGrantDynamicFromRequest = (req) => { + return req.session.grant?.dynamic ?? {} } diff --git a/packages/@uppy/companion/src/server/middlewares.js b/packages/@uppy/companion/src/server/middlewares.js index c344ff1602..325a4ff0f2 100644 --- a/packages/@uppy/companion/src/server/middlewares.js +++ b/packages/@uppy/companion/src/server/middlewares.js @@ -24,6 +24,7 @@ exports.hasSessionAndProvider = (req, res, next) => { } const isOAuthProviderReq = (req) => isOAuthProvider(req.companion.providerClass.authProvider) +const isSimpleAuthProviderReq = (req) => !!req.companion.providerClass.hasSimpleAuth /** * Middleware can be used to verify that the current request is to an OAuth provider @@ -38,6 +39,15 @@ exports.hasOAuthProvider = (req, res, next) => { return next() } +exports.hasSimpleAuthProvider = (req, res, next) => { + if (!isSimpleAuthProviderReq(req)) { + logger.debug('Provider does not support simple auth.', 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) @@ -57,7 +67,28 @@ 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) || isSimpleAuthProviderReq(req)) { + // For OAuth / simple auth provider, we find the encrypted auth token from the header: + const token = req.companion.authToken + if (token == null) { + logger.info('cannot auth token', 'token.verify.unset', req.id) + res.sendStatus(401) + return + } + const { providerName } = req.params + try { + const payload = tokenService.verifyEncryptedAuthToken(token, req.companion.options.secret, providerName) + req.companion.providerUserSession = payload[providerName] + } catch (err) { + logger.error(err.message, 'token.verify.error', req.id) + res.sendStatus(401) + return + } + next() + return + } + + // for non auth providers, we just load the static key from options if (!isOAuthProviderReq(req)) { const { providerOptions } = req.companion.options const { providerName } = req.params @@ -67,31 +98,11 @@ exports.verifyToken = (req, res, next) => { return } - req.companion.providerTokens = { + req.companion.providerUserSession = { accessToken: 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) - res.sendStatus(401) - return - } - const { providerName } = req.params - try { - const payload = tokenService.verifyEncryptedAuthToken(token, req.companion.options.secret, providerName) - req.companion.allProvidersTokens = payload - req.companion.providerTokens = payload[providerName] - } catch (err) { - logger.error(err.message, 'token.verify.error', req.id) - res.sendStatus(401) - return } - next() } // does not fail if token is invalid @@ -102,7 +113,7 @@ exports.gentleVerifyToken = (req, res, next) => { const payload = tokenService.verifyEncryptedAuthToken( req.companion.authToken, req.companion.options.secret, providerName, ) - req.companion.allProvidersTokens = payload + req.companion.providerUserSession = payload[providerName] } catch (err) { logger.error(err.message, 'token.gentle.verify.error', req.id) } diff --git a/packages/@uppy/companion/src/server/provider/Provider.js b/packages/@uppy/companion/src/server/provider/Provider.js index 074d18d156..e649cab1a6 100644 --- a/packages/@uppy/companion/src/server/provider/Provider.js +++ b/packages/@uppy/companion/src/server/provider/Provider.js @@ -1,20 +1,24 @@ +const { MAX_AGE_24H } = require('../helpers/jwt') + /** * Provider interface defines the specifications of any provider implementation */ class Provider { /** * - * @param {{providerName: string, allowLocalUrls: boolean}} options + * @param {{providerName: string, allowLocalUrls: boolean, providerGrantConfig?: object}} options */ - constructor ({ allowLocalUrls }) { + constructor ({ allowLocalUrls, providerGrantConfig }) { // Some providers might need cookie auth for the thumbnails fetched via companion this.needsCookieAuth = false this.allowLocalUrls = allowLocalUrls + this.providerGrantConfig = providerGrantConfig return this } /** * config to extend the grant config + * todo major: rename to getExtraGrantConfig */ static getExtraConfig () { return {} @@ -85,13 +89,36 @@ class Provider { } /** - * Name of the OAuth provider. Return empty string if no OAuth provider is needed. + * @param {any} param0 + * @returns {Promise} + */ + // eslint-disable-next-line no-unused-vars, class-methods-use-this + async simpleAuth ({ requestBody }) { + throw new Error('method not implemented') + } + + /** + * Name of the OAuth provider (passed to Grant). Return empty string if no OAuth provider is needed. * * @returns {string} */ + // todo next major: rename authProvider to oauthProvider (we have other non-oauth auth types too now) static get authProvider () { return undefined } + + // eslint-disable-next-line no-unused-vars + static grantDynamicToUserSession ({ grantDynamic }) { + return {} + } + + static get hasSimpleAuth () { + return false + } + + static get authStateExpiry () { + return MAX_AGE_24H + } } module.exports = Provider diff --git a/packages/@uppy/companion/src/server/provider/credentials.js b/packages/@uppy/companion/src/server/provider/credentials.js index fa41dbaef1..54103547a8 100644 --- a/packages/@uppy/companion/src/server/provider/credentials.js +++ b/packages/@uppy/companion/src/server/provider/credentials.js @@ -82,10 +82,10 @@ exports.getCredentialsOverrideMiddleware = (providers, companionOptions) => { return } - const dynamic = oAuthState.getDynamicStateFromRequest(req) + const grantDynamic = oAuthState.getGrantDynamicFromRequest(req) // only use state via session object if user isn't making intial "connect" request. // override param indicates subsequent requests from the oauth flow - const state = override ? dynamic : req.query.state + const state = override ? grantDynamic.state : req.query.state if (!state) { next() return diff --git a/packages/@uppy/companion/src/server/provider/drive/index.js b/packages/@uppy/companion/src/server/provider/drive/index.js index 44fb38d960..c7672af577 100644 --- a/packages/@uppy/companion/src/server/provider/drive/index.js +++ b/packages/@uppy/companion/src/server/provider/drive/index.js @@ -5,6 +5,7 @@ const logger = require('../../logger') const { VIRTUAL_SHARED_DIR, adaptData, isShortcut, isGsuiteFile, getGsuiteExportType } = require('./adapter') const { withProviderErrorHandling } = require('../providerErrors') const { prepareStream } = require('../../helpers/utils') +const { MAX_AGE_REFRESH_TOKEN } = require('../../helpers/jwt') const DRIVE_FILE_FIELDS = 'kind,id,imageMediaMetadata,name,mimeType,ownedByMe,size,modifiedTime,iconLink,thumbnailLink,teamDriveId,videoMediaMetadata,shortcutDetails(targetId,targetMimeType)' const DRIVE_FILES_FIELDS = `kind,nextPageToken,incompleteSearch,files(${DRIVE_FILE_FIELDS})` @@ -49,6 +50,10 @@ class Drive extends Provider { return 'google' } + static get authStateExpiry () { + return MAX_AGE_REFRESH_TOKEN + } + async list (options) { return this.#withErrorHandling('provider.drive.list.error', async () => { const directory = options.directory || 'root' diff --git a/packages/@uppy/companion/src/server/provider/dropbox/index.js b/packages/@uppy/companion/src/server/provider/dropbox/index.js index 287803c6fb..f820581167 100644 --- a/packages/@uppy/companion/src/server/provider/dropbox/index.js +++ b/packages/@uppy/companion/src/server/provider/dropbox/index.js @@ -4,6 +4,7 @@ const Provider = require('../Provider') const adaptData = require('./adapter') const { withProviderErrorHandling } = require('../providerErrors') const { prepareStream } = require('../../helpers/utils') +const { MAX_AGE_REFRESH_TOKEN } = require('../../helpers/jwt') // From https://www.dropbox.com/developers/reference/json-encoding: // @@ -63,6 +64,10 @@ class DropBox extends Provider { return 'dropbox' } + static get authStateExpiry () { + return MAX_AGE_REFRESH_TOKEN + } + /** * * @param {object} options diff --git a/packages/@uppy/companion/src/server/provider/index.js b/packages/@uppy/companion/src/server/provider/index.js index e49aff660d..8de1beaef9 100644 --- a/packages/@uppy/companion/src/server/provider/index.js +++ b/packages/@uppy/companion/src/server/provider/index.js @@ -42,7 +42,7 @@ const providerNameToAuthName = (name, options) => { // eslint-disable-line no-un * * @param {Record} providers */ -module.exports.getProviderMiddleware = (providers) => { +module.exports.getProviderMiddleware = (providers, grantConfig) => { /** * * @param {object} req @@ -54,12 +54,17 @@ module.exports.getProviderMiddleware = (providers) => { const ProviderClass = providers[providerName] if (ProviderClass && validOptions(req.companion.options)) { const { allowLocalUrls } = req.companion.options - req.companion.provider = new ProviderClass({ providerName, allowLocalUrls }) - req.companion.providerClass = ProviderClass + const { authProvider } = ProviderClass - if (isOAuthProvider(ProviderClass.authProvider)) { + let providerGrantConfig + if (isOAuthProvider(authProvider)) { req.companion.getProviderCredentials = getCredentialsResolver(providerName, req.companion.options, req) + providerGrantConfig = grantConfig[authProvider] + req.companion.providerGrantConfig = providerGrantConfig } + + req.companion.provider = new ProviderClass({ providerName, providerGrantConfig, allowLocalUrls }) + req.companion.providerClass = ProviderClass } else { logger.warn('invalid provider options detected. Provider will not be loaded', 'provider.middleware.invalid', req.id) } diff --git a/packages/@uppy/dropbox/src/Dropbox.jsx b/packages/@uppy/dropbox/src/Dropbox.jsx index c591933087..b836ba0d90 100644 --- a/packages/@uppy/dropbox/src/Dropbox.jsx +++ b/packages/@uppy/dropbox/src/Dropbox.jsx @@ -27,6 +27,7 @@ export default class Dropbox extends UIPlugin { companionCookiesRule: this.opts.companionCookiesRule, provider: 'dropbox', pluginId: this.id, + supportsRefreshToken: true, }) this.defaultLocale = locale diff --git a/packages/@uppy/facebook/src/Facebook.jsx b/packages/@uppy/facebook/src/Facebook.jsx index f8ceac492e..30d8c5faba 100644 --- a/packages/@uppy/facebook/src/Facebook.jsx +++ b/packages/@uppy/facebook/src/Facebook.jsx @@ -30,6 +30,7 @@ export default class Facebook extends UIPlugin { companionCookiesRule: this.opts.companionCookiesRule, provider: 'facebook', pluginId: this.id, + supportsRefreshToken: false, }) this.defaultLocale = locale diff --git a/packages/@uppy/google-drive/src/GoogleDrive.jsx b/packages/@uppy/google-drive/src/GoogleDrive.jsx index 96433c33ef..fa857f429f 100644 --- a/packages/@uppy/google-drive/src/GoogleDrive.jsx +++ b/packages/@uppy/google-drive/src/GoogleDrive.jsx @@ -41,6 +41,7 @@ export default class GoogleDrive extends UIPlugin { companionCookiesRule: this.opts.companionCookiesRule, provider: 'drive', pluginId: this.id, + supportsRefreshToken: true, }) this.defaultLocale = locale diff --git a/packages/@uppy/instagram/src/Instagram.jsx b/packages/@uppy/instagram/src/Instagram.jsx index 7ba14fd33e..c0094a86b8 100644 --- a/packages/@uppy/instagram/src/Instagram.jsx +++ b/packages/@uppy/instagram/src/Instagram.jsx @@ -40,6 +40,7 @@ export default class Instagram extends UIPlugin { companionCookiesRule: this.opts.companionCookiesRule, provider: 'instagram', pluginId: this.id, + supportsRefreshToken: false, }) this.onFirstRender = this.onFirstRender.bind(this) diff --git a/packages/@uppy/onedrive/src/OneDrive.jsx b/packages/@uppy/onedrive/src/OneDrive.jsx index 6f4b939f56..b1696a0178 100644 --- a/packages/@uppy/onedrive/src/OneDrive.jsx +++ b/packages/@uppy/onedrive/src/OneDrive.jsx @@ -32,6 +32,7 @@ export default class OneDrive extends UIPlugin { companionCookiesRule: this.opts.companionCookiesRule, provider: 'onedrive', pluginId: this.id, + supportsRefreshToken: false, }) this.defaultLocale = locale diff --git a/packages/@uppy/provider-views/src/ProviderView/AuthView.jsx b/packages/@uppy/provider-views/src/ProviderView/AuthView.jsx index 5cf51f82f6..9d7b02c418 100644 --- a/packages/@uppy/provider-views/src/ProviderView/AuthView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/AuthView.jsx @@ -36,12 +36,38 @@ function GoogleIcon () { ) } -function AuthView (props) { - const { pluginName, pluginIcon, i18nArray, handleAuth } = props +const defaultRenderForm = ({ pluginName, i18nArray }) => { // In order to comply with Google's brand we need to create a different button // for the Google Drive plugin const isGoogleDrive = pluginName === 'Google Drive' + if (isGoogleDrive) { + return ( + + ) + } + + return ( + + ) +} + +function AuthView (props) { + const { pluginName, pluginIcon, i18nArray, handleAuth, renderForm = defaultRenderForm } = props + const pluginNameComponent = ( {pluginName} @@ -56,26 +82,10 @@ function AuthView (props) { pluginName: pluginNameComponent, })} - {isGoogleDrive ? ( - - ) : ( - - )} + +
+ {renderForm({ pluginName, i18nArray })} +
) } diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index ce3642bad9..26f010eeab 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -242,10 +242,12 @@ export default class ProviderView extends View { this.plugin.setPluginState({ filterInput: '' }) } - async handleAuth () { + async handleAuth (formSubmitEvent) { + formSubmitEvent.preventDefault() + const clientVersion = `@uppy/provider-views=${ProviderView.VERSION}` try { - await this.provider.login({ uppyVersions: clientVersion }) + await this.provider.login({ uppyVersions: clientVersion, formSubmitEvent }) this.plugin.setPluginState({ authenticated: true }) this.preFirstRender() } catch (e) { @@ -477,6 +479,7 @@ export default class ProviderView extends View { handleAuth={this.handleAuth} i18n={this.plugin.uppy.i18n} i18nArray={this.plugin.uppy.i18nArray} + renderForm={this.opts.renderAuthForm} /> ) diff --git a/packages/@uppy/zoom/src/Zoom.jsx b/packages/@uppy/zoom/src/Zoom.jsx index c7c3d392ba..dc465e05a3 100644 --- a/packages/@uppy/zoom/src/Zoom.jsx +++ b/packages/@uppy/zoom/src/Zoom.jsx @@ -28,6 +28,7 @@ export default class Zoom extends UIPlugin { companionCookiesRule: this.opts.companionCookiesRule, provider: 'zoom', pluginId: this.id, + supportsRefreshToken: false, }) this.defaultLocale = locale From 7d594891706ae49e2cc6f84e6ef4472bb641104a Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Sun, 13 Aug 2023 20:19:16 +0200 Subject: [PATCH 07/37] refactor --- packages/@uppy/companion-client/src/Provider.js | 4 ++-- packages/@uppy/companion-client/src/RequestClient.js | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/@uppy/companion-client/src/Provider.js b/packages/@uppy/companion-client/src/Provider.js index cae6b42102..31edb322e6 100644 --- a/packages/@uppy/companion-client/src/Provider.js +++ b/packages/@uppy/companion-client/src/Provider.js @@ -1,6 +1,6 @@ 'use strict' -import RequestClient from './RequestClient.js' +import RequestClient, { authErrorStatusCode } from './RequestClient.js' import * as tokenStorage from './tokenStorage.js' import AuthError from './AuthError.js' @@ -62,7 +62,7 @@ export default class Provider extends RequestClient { super.onReceiveResponse(response) const plugin = this.uppy.getPlugin(this.pluginId) const oldAuthenticated = plugin.getPluginState().authenticated - const authenticated = oldAuthenticated ? response.status !== 401 : response.status < 400 + const authenticated = oldAuthenticated ? response.status !== authErrorStatusCode : response.status < 400 plugin.setPluginState({ authenticated }) return response } diff --git a/packages/@uppy/companion-client/src/RequestClient.js b/packages/@uppy/companion-client/src/RequestClient.js index bbbf61b7b0..0323961bce 100644 --- a/packages/@uppy/companion-client/src/RequestClient.js +++ b/packages/@uppy/companion-client/src/RequestClient.js @@ -11,8 +11,10 @@ function stripSlash (url) { return url.replace(/\/$/, '') } +export const authErrorStatusCode = 401 + async function handleJSONResponse (res) { - if (res.status === 401) { + if (res.status === authErrorStatusCode) { throw new AuthError() } From ab06da6d2760e21bd3e70303a0793c5423f118bf Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Sun, 13 Aug 2023 20:24:14 +0200 Subject: [PATCH 08/37] use respondWithError also for thumbnails for consistency --- .../@uppy/companion/src/server/controllers/thumbnail.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/@uppy/companion/src/server/controllers/thumbnail.js b/packages/@uppy/companion/src/server/controllers/thumbnail.js index 06de9be9fc..9cb3b6e68f 100644 --- a/packages/@uppy/companion/src/server/controllers/thumbnail.js +++ b/packages/@uppy/companion/src/server/controllers/thumbnail.js @@ -1,3 +1,5 @@ +const { respondWithError } = require('../provider/error') + /** * * @param {object} req @@ -13,8 +15,8 @@ async function thumbnail (req, res, next) { res.set('Content-Type', 'image/jpeg') stream.pipe(res) } catch (err) { - if (err.isAuthError) res.sendStatus(401) - else next(err) + if (respondWithError(err, res)) return + next(err) } } From 16d4e3ff7d74f32d46c7b5a9b0a3b5484854074c Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Sun, 13 Aug 2023 20:26:56 +0200 Subject: [PATCH 09/37] fix prepareStream it wasn't returning the status code (like `got` does on error) it's needed to respond properly with a http error --- packages/@uppy/companion/src/server/helpers/utils.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/@uppy/companion/src/server/helpers/utils.js b/packages/@uppy/companion/src/server/helpers/utils.js index 63e04e9928..fb5b76f21a 100644 --- a/packages/@uppy/companion/src/server/helpers/utils.js +++ b/packages/@uppy/companion/src/server/helpers/utils.js @@ -160,7 +160,12 @@ module.exports.prepareStream = async (stream) => new Promise((resolve, reject) = if (err?.request?.options?.responseType === 'json' && typeof err?.response?.body === 'string') { try { // todo unit test this - reject(Object.assign(new Error(), { response: { body: JSON.parse(err.response.body) } })) + reject(Object.assign(new Error(), { + response: { + body: JSON.parse(err.response.body), + statusCode: err.response.statusCode, + }, + })) } catch (err2) { reject(err) } From 20923872f0ef7df31cfb41125feb60d2ba19f90e Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Sun, 13 Aug 2023 20:36:41 +0200 Subject: [PATCH 10/37] don't throw when missing i18n key instead log error and show the key this in on par with other i18n frameworks --- packages/@uppy/core/src/BasePlugin.js | 3 ++- packages/@uppy/core/src/Uppy.js | 3 ++- packages/@uppy/utils/src/Translator.js | 20 +++++++++++++------- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/packages/@uppy/core/src/BasePlugin.js b/packages/@uppy/core/src/BasePlugin.js index c802f97b74..9ad2e433c1 100644 --- a/packages/@uppy/core/src/BasePlugin.js +++ b/packages/@uppy/core/src/BasePlugin.js @@ -41,7 +41,8 @@ export default class BasePlugin { } i18nInit () { - const translator = new Translator([this.defaultLocale, this.uppy.locale, this.opts.locale]) + const onMissingKey = (key) => this.uppy.log(`Missing i18n string: ${key}`, 'error') + const translator = new Translator([this.defaultLocale, this.uppy.locale, this.opts.locale], { onMissingKey }) this.i18n = translator.translate.bind(translator) this.i18nArray = translator.translateArray.bind(translator) this.setPluginState() // so that UI re-renders and we see the updated locale diff --git a/packages/@uppy/core/src/Uppy.js b/packages/@uppy/core/src/Uppy.js index 3ed44108a1..06e35d28ff 100644 --- a/packages/@uppy/core/src/Uppy.js +++ b/packages/@uppy/core/src/Uppy.js @@ -198,7 +198,8 @@ class Uppy { } i18nInit () { - const translator = new Translator([this.defaultLocale, this.opts.locale]) + const onMissingKey = (key) => this.log(`Missing i18n string: ${key}`, 'error') + const translator = new Translator([this.defaultLocale, this.opts.locale], { onMissingKey }) this.i18n = translator.translate.bind(translator) this.i18nArray = translator.translateArray.bind(translator) this.locale = translator.locale diff --git a/packages/@uppy/utils/src/Translator.js b/packages/@uppy/utils/src/Translator.js index 8acecb8b3b..fd3a09b759 100644 --- a/packages/@uppy/utils/src/Translator.js +++ b/packages/@uppy/utils/src/Translator.js @@ -1,5 +1,3 @@ -import has from './hasProperty.js' - function insertReplacement (source, rx, replacement) { const newParts = [] source.forEach((chunk) => { @@ -62,6 +60,10 @@ function interpolate (phrase, options) { return interpolated } +const defaultOnMissingKey = (key) => { + throw new Error(`missing string: ${key}`) +} + /** * Translates strings with interpolation & pluralization support. * Extensible with custom dictionaries and pluralization functions. @@ -77,7 +79,7 @@ export default class Translator { /** * @param {object|Array} locales - locale or list of locales. */ - constructor (locales) { + constructor (locales, { onMissingKey = defaultOnMissingKey } = {}) { this.locale = { strings: {}, pluralize (n) { @@ -93,8 +95,12 @@ export default class Translator { } else { this.#apply(locales) } + + this.#onMissingKey = onMissingKey } + #onMissingKey + #apply (locale) { if (!locale?.strings) { return @@ -124,11 +130,11 @@ export default class Translator { * @returns {Array} The translated and interpolated parts, in order. */ translateArray (key, options) { - if (!has(this.locale.strings, key)) { - throw new Error(`missing string: ${key}`) + let string = this.locale.strings[key] + if (string == null) { + this.#onMissingKey(key) + string = key } - - const string = this.locale.strings[key] const hasPluralForms = typeof string === 'object' if (hasPluralForms) { From f575dbc0661ad6157712f22c2e37c1d9333f2fa2 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Sun, 13 Aug 2023 20:42:36 +0200 Subject: [PATCH 11/37] fix bugged try/catch --- packages/@uppy/companion-client/src/RequestClient.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@uppy/companion-client/src/RequestClient.js b/packages/@uppy/companion-client/src/RequestClient.js index 0323961bce..fb3f340564 100644 --- a/packages/@uppy/companion-client/src/RequestClient.js +++ b/packages/@uppy/companion-client/src/RequestClient.js @@ -164,7 +164,7 @@ export default class RequestClient { body: data ? JSON.stringify(data) : null, }) if (!skipPostResponse) this.onReceiveResponse(response) - return handleJSONResponse(response) + return await handleJSONResponse(response) } catch (err) { if (err instanceof AuthError) throw err // auth errors must be passed through throw new ErrorWithCause(`Could not ${method} ${this.#getUrl(path)}`, { cause: err }) From 754e2e06ffe240d88b2d64f4427c301a22fc8384 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Sun, 13 Aug 2023 22:37:45 +0200 Subject: [PATCH 12/37] allow aborting login too and don't replace the whole view with a loader when plugin state loading it will cause auth views to lose state an inter-view loading text looks much more graceful and is how SearchProviderView works too --- .../@uppy/companion-client/src/Provider.js | 17 +++++++-- .../src/ProviderView/ProviderView.jsx | 37 +++++++++---------- 2 files changed, 32 insertions(+), 22 deletions(-) diff --git a/packages/@uppy/companion-client/src/Provider.js b/packages/@uppy/companion-client/src/Provider.js index 31edb322e6..0be722f035 100644 --- a/packages/@uppy/companion-client/src/Provider.js +++ b/packages/@uppy/companion-client/src/Provider.js @@ -112,13 +112,17 @@ export default class Provider extends RequestClient { return `${this.hostname}/${this.id}/connect?${params}` } - async login ({ uppyVersions, formSubmitEvent }) { + async login ({ uppyVersions, formSubmitEvent, signal }) { await this.ensurePreAuth() + signal.throwIfAborted() + return new Promise((resolve, reject) => { const link = this.authUrl({ query: { uppyVersions }, formSubmitEvent }) const authWindow = window.open(link, '_blank') + let cleanup + const handleToken = (e) => { if (e.source !== authWindow) { this.uppy.log.warn('ignoring event from unknown source', e) @@ -148,11 +152,18 @@ export default class Provider extends RequestClient { return } - authWindow.close() - window.removeEventListener('message', handleToken) + cleanup() this.setAuthToken(data.token) resolve() } + + cleanup = () => { + authWindow.close() + window.removeEventListener('message', handleToken) + signal.removeEventListener('abort', cleanup) + } + + signal.addEventListener('abort', cleanup) window.addEventListener('message', handleToken) }) } diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index 26f010eeab..1c60754e6d 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -7,7 +7,6 @@ import { getSafeFileId } from '@uppy/utils/lib/generateFileID' import AuthView from './AuthView.jsx' import Header from './Header.jsx' import Browser from '../Browser.jsx' -import LoaderView from '../Loader.jsx' import CloseWrapper from '../CloseWrapper.js' import View from '../View.js' @@ -69,7 +68,7 @@ export default class ProviderView extends View { // Set default state for the plugin this.plugin.setPluginState({ - authenticated: false, + authenticated: undefined, // we don't know yet files: [], folders: [], breadcrumbs: [], @@ -243,15 +242,21 @@ export default class ProviderView extends View { } async handleAuth (formSubmitEvent) { - formSubmitEvent.preventDefault() - - const clientVersion = `@uppy/provider-views=${ProviderView.VERSION}` try { - await this.provider.login({ uppyVersions: clientVersion, formSubmitEvent }) - this.plugin.setPluginState({ authenticated: true }) - this.preFirstRender() - } catch (e) { - this.plugin.uppy.log(`login failed: ${e.message}`) + await this.#withAbort(async (signal) => { + formSubmitEvent.preventDefault() + + const clientVersion = `@uppy/provider-views=${ProviderView.VERSION}` + + this.setLoading(true) + await this.provider.login({ uppyVersions: clientVersion, formSubmitEvent, signal }) + this.plugin.setPluginState({ authenticated: true }) + this.preFirstRender() + }) + } catch (err) { + this.plugin.uppy.log(`login failed: ${err.message}`) + } finally { + this.setLoading(false) } } @@ -460,17 +465,10 @@ export default class ProviderView extends View { i18n: this.plugin.uppy.i18n, uppyFiles: this.plugin.uppy.getFiles(), validateRestrictions: (...args) => this.plugin.uppy.validateRestrictions(...args), + isLoading: loading, } - if (loading) { - return ( - - - - ) - } - - if (!authenticated) { + if (authenticated === false) { return ( ) From 64411239e3f62154706f85e884081d24c3eefd27 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Sun, 13 Aug 2023 22:40:55 +0200 Subject: [PATCH 13/37] add json http error support add support for passing objects and messages from companion to uppy this allows companion to for example give a more detailed error when authenticating --- .../companion-client/src/RequestClient.js | 21 +++++++++++---- packages/@uppy/companion/src/companion.js | 4 +-- packages/@uppy/companion/src/server/logger.js | 4 +-- .../companion/src/server/provider/error.d.ts | 16 ----------- .../companion/src/server/provider/error.js | 27 ++++++++++++++----- .../src/ProviderView/AuthView.jsx | 4 +-- .../src/ProviderView/ProviderView.jsx | 6 +++++ packages/@uppy/utils/package.json | 3 ++- .../@uppy/utils/src/UserFacingApiError.js | 8 ++++++ 9 files changed, 59 insertions(+), 34 deletions(-) delete mode 100644 packages/@uppy/companion/src/server/provider/error.d.ts create mode 100644 packages/@uppy/utils/src/UserFacingApiError.js diff --git a/packages/@uppy/companion-client/src/RequestClient.js b/packages/@uppy/companion-client/src/RequestClient.js index fb3f340564..293c09a21e 100644 --- a/packages/@uppy/companion-client/src/RequestClient.js +++ b/packages/@uppy/companion-client/src/RequestClient.js @@ -1,5 +1,7 @@ 'use strict' +import UserFacingApiError from '@uppy/utils/lib/UserFacingApiError' + import fetchWithNetworkError from '@uppy/utils/lib/fetchWithNetworkError' import ErrorWithCause from '@uppy/utils/lib/ErrorWithCause' import AuthError from './AuthError.js' @@ -24,11 +26,20 @@ async function handleJSONResponse (res) { } let errMsg = `Failed request with status: ${res.status}. ${res.statusText}` + let errData try { - const errData = await jsonPromise - errMsg = errData.message ? `${errMsg} message: ${errData.message}` : errMsg - errMsg = errData.requestId ? `${errMsg} request-Id: ${errData.requestId}` : errMsg - } catch { /* if the response contains invalid JSON, let's ignore the error */ } + errData = await jsonPromise + } catch { + // if the response contains invalid JSON, let's ignore the error data + throw new Error(errMsg) + } + + if (res.status >= 400 && res.status <= 499 && errData.message) { + throw new UserFacingApiError(errData.message) + } + + errMsg = errData.message ? `${errMsg} message: ${errData.message}` : errMsg + errMsg = errData.requestId ? `${errMsg} request-Id: ${errData.requestId}` : errMsg throw new Error(errMsg) } @@ -166,7 +177,7 @@ export default class RequestClient { if (!skipPostResponse) this.onReceiveResponse(response) return await handleJSONResponse(response) } catch (err) { - if (err instanceof AuthError) throw err // auth errors must be passed through + if (err instanceof AuthError || err instanceof UserFacingApiError) throw err // some errors must be passed through throw new ErrorWithCause(`Could not ${method} ${this.#getUrl(path)}`, { cause: err }) } } diff --git a/packages/@uppy/companion/src/companion.js b/packages/@uppy/companion/src/companion.js index f1f5a43774..f14ddd2db9 100644 --- a/packages/@uppy/companion/src/companion.js +++ b/packages/@uppy/companion/src/companion.js @@ -16,7 +16,7 @@ const jobs = require('./server/jobs') const logger = require('./server/logger') const middlewares = require('./server/middlewares') const { getMaskableSecrets, defaultOptions, validateConfig } = require('./config/companion') -const { ProviderApiError, ProviderAuthError } = require('./server/provider/error') +const { ProviderApiError, ProviderUserError, ProviderAuthError } = require('./server/provider/error') const { getCredentialsOverrideMiddleware } = require('./server/provider/credentials') // @ts-ignore const { version } = require('../package.json') @@ -52,7 +52,7 @@ const interceptGrantErrorResponse = interceptor((req, res) => { }) // make the errors available publicly for custom providers -module.exports.errors = { ProviderApiError, ProviderAuthError } +module.exports.errors = { ProviderApiError, ProviderUserError, ProviderAuthError } module.exports.socket = require('./server/socket') module.exports.setLoggerProcessName = setLoggerProcessName diff --git a/packages/@uppy/companion/src/server/logger.js b/packages/@uppy/companion/src/server/logger.js index 112a35555b..f92f381041 100644 --- a/packages/@uppy/companion/src/server/logger.js +++ b/packages/@uppy/companion/src/server/logger.js @@ -1,7 +1,7 @@ const chalk = require('chalk') const escapeStringRegexp = require('escape-string-regexp') const util = require('node:util') -const { ProviderApiError, ProviderAuthError } = require('./provider/error') +const { ProviderApiError, ProviderUserError, ProviderAuthError } = require('./provider/error') const valuesToMask = [] /** @@ -56,7 +56,7 @@ const log = ({ arg, tag = '', level, traceId = '', color = (message) => message function msgToString () { // We don't need to log stack trace on special errors that we ourselves have produced // (to reduce log noise) - if ((arg instanceof ProviderApiError || arg instanceof ProviderAuthError) && typeof arg.message === 'string') { + if ((arg instanceof ProviderApiError || arg instanceof ProviderUserError || arg instanceof ProviderAuthError) && typeof arg.message === 'string') { return arg.message } if (typeof arg === 'string') return arg diff --git a/packages/@uppy/companion/src/server/provider/error.d.ts b/packages/@uppy/companion/src/server/provider/error.d.ts deleted file mode 100644 index 29ce604bcf..0000000000 --- a/packages/@uppy/companion/src/server/provider/error.d.ts +++ /dev/null @@ -1,16 +0,0 @@ -// We need explicit type declarations for `errors.js` because of a typescript bug when generating declaration files. -// I think it's this one: -// https://github.com/microsoft/TypeScript/issues/37832 -// -// We could try removing this file when we upgrade to 4.1 :) - -export class ProviderApiError extends Error { - constructor(message: string, statusCode: number) -} -export class ProviderAuthError extends ProviderApiError { - constructor() -} - -export function errorToResponse(anyError: Error): { code: number, message: string } - -export function respondWithError(anyError: Error, res: any): boolean diff --git a/packages/@uppy/companion/src/server/provider/error.js b/packages/@uppy/companion/src/server/provider/error.js index cf35932198..7b14cf09a3 100644 --- a/packages/@uppy/companion/src/server/provider/error.js +++ b/packages/@uppy/companion/src/server/provider/error.js @@ -1,3 +1,4 @@ +/* eslint-disable max-classes-per-file */ /** * ProviderApiError is error returned when an adapter encounters * an http error while communication with its corresponding provider @@ -15,6 +16,16 @@ class ProviderApiError extends Error { } } +class ProviderUserError extends Error { + /** + * @param {object} json arbitrary JSON.stringify-able object that will be passed to the client + */ + constructor (json) { + super('User error') + this.json = json + } +} + /** * AuthError is error returned when an adapter encounters * an authorization error while communication with its corresponding provider @@ -33,32 +44,36 @@ class ProviderAuthError extends ProviderApiError { * @param {Error | ProviderApiError} err the error instance to convert to an http json response */ function errorToResponse (err) { - if (err instanceof ProviderAuthError && err.isAuthError) { - return { code: 401, message: err.message } + if (err instanceof ProviderAuthError) { + return { code: 401, json: { message: err.message } } } if (err instanceof ProviderApiError) { if (err.statusCode >= 500) { // bad gateway i.e the provider APIs gateway - return { code: 502, message: err.message } + return { code: 502, json: { message: err.message } } } if (err.statusCode >= 400) { // 424 Failed Dependency - return { code: 424, message: err.message } + return { code: 424, json: { message: err.message } } } } + if (err instanceof ProviderUserError) { + return { code: 400, json: err.json } + } + return undefined } function respondWithError (err, res) { const errResp = errorToResponse(err) if (errResp) { - res.status(errResp.code).json({ message: errResp.message }) + res.status(errResp.code).json(errResp.json) return true } return false } -module.exports = { ProviderAuthError, ProviderApiError, errorToResponse, respondWithError } +module.exports = { ProviderAuthError, ProviderApiError, ProviderUserError, respondWithError } diff --git a/packages/@uppy/provider-views/src/ProviderView/AuthView.jsx b/packages/@uppy/provider-views/src/ProviderView/AuthView.jsx index 9d7b02c418..16fe74e70a 100644 --- a/packages/@uppy/provider-views/src/ProviderView/AuthView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/AuthView.jsx @@ -66,7 +66,7 @@ const defaultRenderForm = ({ pluginName, i18nArray }) => { } function AuthView (props) { - const { pluginName, pluginIcon, i18nArray, handleAuth, renderForm = defaultRenderForm } = props + const { loading, pluginName, pluginIcon, i18nArray, handleAuth, renderForm = defaultRenderForm } = props const pluginNameComponent = ( @@ -84,7 +84,7 @@ function AuthView (props) {
- {renderForm({ pluginName, i18nArray })} + {renderForm({ pluginName, i18nArray, loading })}
) diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index 1c60754e6d..e85f73f746 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -3,6 +3,7 @@ import { h } from 'preact' import PQueue from 'p-queue' import { getSafeFileId } from '@uppy/utils/lib/generateFileID' +import UserFacingApiError from '@uppy/utils/lib/UserFacingApiError' import AuthView from './AuthView.jsx' import Header from './Header.jsx' @@ -254,6 +255,11 @@ export default class ProviderView extends View { this.preFirstRender() }) } catch (err) { + if (err instanceof UserFacingApiError) { + this.plugin.uppy.info({ message: this.plugin.uppy.i18n(err.message) }, 'warning', 5000) + return + } + this.plugin.uppy.log(`login failed: ${err.message}`) } finally { this.setLoading(false) diff --git a/packages/@uppy/utils/package.json b/packages/@uppy/utils/package.json index 2f037fd1dc..665111d7b7 100644 --- a/packages/@uppy/utils/package.json +++ b/packages/@uppy/utils/package.json @@ -65,7 +65,8 @@ "./lib/FOCUSABLE_ELEMENTS.js": "./lib/FOCUSABLE_ELEMENTS.js", "./lib/fileFilters": "./lib/fileFilters.js", "./lib/VirtualList": "./lib/VirtualList.js", - "./src/microtip.scss": "./src/microtip.scss" + "./src/microtip.scss": "./src/microtip.scss", + "./lib/UserFacingApiError": "./lib/UserFacingApiError.js" }, "dependencies": { "lodash": "^4.17.21", diff --git a/packages/@uppy/utils/src/UserFacingApiError.js b/packages/@uppy/utils/src/UserFacingApiError.js new file mode 100644 index 0000000000..68330275c7 --- /dev/null +++ b/packages/@uppy/utils/src/UserFacingApiError.js @@ -0,0 +1,8 @@ +class UserFacingApiError extends Error { + constructor (message) { + super(message) + this.name = 'UserFacingApiError' + } +} + +export default UserFacingApiError From f503d1f82948a0958b07857c9c756e0b7763b263 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Sun, 13 Aug 2023 23:48:00 +0200 Subject: [PATCH 14/37] don't tightly couple auth form with html form don't force the user to use html form and use preact for it, for flexibility --- .../@uppy/companion-client/src/Provider.js | 8 +-- .../src/ProviderView/AuthView.jsx | 60 +++++++++++-------- .../src/ProviderView/ProviderView.jsx | 6 +- 3 files changed, 41 insertions(+), 33 deletions(-) diff --git a/packages/@uppy/companion-client/src/Provider.js b/packages/@uppy/companion-client/src/Provider.js index 0be722f035..3fa2f8e27c 100644 --- a/packages/@uppy/companion-client/src/Provider.js +++ b/packages/@uppy/companion-client/src/Provider.js @@ -98,11 +98,11 @@ export default class Provider extends RequestClient { return {} } - authUrl ({ formSubmitEvent, query } = {}) { + authUrl ({ authFormData, query } = {}) { const params = new URLSearchParams({ ...query, state: btoa(JSON.stringify({ origin: getOrigin() })), - ...this.authQuery({ formSubmitEvent }), + ...this.authQuery({ authFormData }), }) if (this.preAuthToken) { @@ -112,13 +112,13 @@ export default class Provider extends RequestClient { return `${this.hostname}/${this.id}/connect?${params}` } - async login ({ uppyVersions, formSubmitEvent, signal }) { + async login ({ uppyVersions, authFormData, signal }) { await this.ensurePreAuth() signal.throwIfAborted() return new Promise((resolve, reject) => { - const link = this.authUrl({ query: { uppyVersions }, formSubmitEvent }) + const link = this.authUrl({ query: { uppyVersions }, authFormData }) const authWindow = window.open(link, '_blank') let cleanup diff --git a/packages/@uppy/provider-views/src/ProviderView/AuthView.jsx b/packages/@uppy/provider-views/src/ProviderView/AuthView.jsx index 16fe74e70a..c0ec896f4a 100644 --- a/packages/@uppy/provider-views/src/ProviderView/AuthView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/AuthView.jsx @@ -1,4 +1,5 @@ import { h } from 'preact' +import { useCallback } from 'preact/hooks' function GoogleIcon () { return ( @@ -36,37 +37,46 @@ function GoogleIcon () { ) } -const defaultRenderForm = ({ pluginName, i18nArray }) => { +const DefaultForm = ({ pluginName, i18n, onAuth }) => { // In order to comply with Google's brand we need to create a different button // for the Google Drive plugin const isGoogleDrive = pluginName === 'Google Drive' - if (isGoogleDrive) { - return ( - - ) - } + const onSubmit = useCallback((e) => { + e.preventDefault() + onAuth() + }, [onAuth]) return ( - +
+ {isGoogleDrive ? ( + + ) : ( + + )} +
) } +const defaultRenderForm = ({ pluginName, i18n, onAuth }) => ( + +) + function AuthView (props) { - const { loading, pluginName, pluginIcon, i18nArray, handleAuth, renderForm = defaultRenderForm } = props + const { loading, pluginName, pluginIcon, i18n, handleAuth, renderForm = defaultRenderForm } = props const pluginNameComponent = ( @@ -78,14 +88,14 @@ function AuthView (props) {
{pluginIcon()}
- {i18nArray('authenticateWithTitle', { + {i18n('authenticateWithTitle', { pluginName: pluginNameComponent, })}
-
- {renderForm({ pluginName, i18nArray, loading })} -
+
+ {renderForm({ pluginName, i18n, loading, onAuth: handleAuth })} +
) } diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index e85f73f746..84cdbcaed9 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -242,15 +242,13 @@ export default class ProviderView extends View { this.plugin.setPluginState({ filterInput: '' }) } - async handleAuth (formSubmitEvent) { + async handleAuth (authFormData) { try { await this.#withAbort(async (signal) => { - formSubmitEvent.preventDefault() - const clientVersion = `@uppy/provider-views=${ProviderView.VERSION}` this.setLoading(true) - await this.provider.login({ uppyVersions: clientVersion, formSubmitEvent, signal }) + await this.provider.login({ uppyVersions: clientVersion, authFormData, signal }) this.plugin.setPluginState({ authenticated: true }) this.preFirstRender() }) From 272e3d1114f2a81818cca40a837c338c61c7f41e Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 14 Aug 2023 22:15:55 +0200 Subject: [PATCH 15/37] fix i18n --- .../@uppy/provider-views/src/ProviderView/ProviderView.jsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index 84cdbcaed9..4dd13fb91a 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -479,8 +479,7 @@ export default class ProviderView extends View { pluginName={this.plugin.title} pluginIcon={pluginIcon} handleAuth={this.handleAuth} - i18n={this.plugin.uppy.i18n} - i18nArray={this.plugin.uppy.i18nArray} + i18n={this.plugin.uppy.i18nArray} renderForm={this.opts.renderAuthForm} loading={loading} /> From f2e5aa4bba31f04bf509956445d3d17699120fc7 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 14 Aug 2023 22:17:21 +0200 Subject: [PATCH 16/37] make contentType parameterized --- packages/@uppy/companion/src/server/controllers/thumbnail.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@uppy/companion/src/server/controllers/thumbnail.js b/packages/@uppy/companion/src/server/controllers/thumbnail.js index 9cb3b6e68f..1071e381fa 100644 --- a/packages/@uppy/companion/src/server/controllers/thumbnail.js +++ b/packages/@uppy/companion/src/server/controllers/thumbnail.js @@ -11,8 +11,8 @@ async function thumbnail (req, res, next) { const { accessToken } = providerUserSession try { - const { stream } = await provider.thumbnail({ id, token: accessToken, providerUserSession }) - res.set('Content-Type', 'image/jpeg') + const { stream, contentType = 'image/jpeg' } = await provider.thumbnail({ id, token: accessToken, providerUserSession }) + res.set('Content-Type', contentType) stream.pipe(res) } catch (err) { if (respondWithError(err, res)) return From a28fcecf98c742c60897d7b7d9f09d86d03aa03e Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 6 Sep 2023 22:15:59 +0200 Subject: [PATCH 17/37] allow sending certain errors to the user this is useful because: // onedrive gives some errors here that the user might want to know about // e.g. these happen if you try to login to a users in an organization, // without an Office365 licence or OneDrive account setup completed // 400: Tenant does not have a SPO license // 403: You do not have access to create this personal site or you do not have a valid license --- .../@uppy/companion/src/server/provider/providerErrors.js | 8 ++++++-- .../provider-views/src/ProviderView/ProviderView.jsx | 7 +++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/packages/@uppy/companion/src/server/provider/providerErrors.js b/packages/@uppy/companion/src/server/provider/providerErrors.js index bc38093550..4c216837a6 100644 --- a/packages/@uppy/companion/src/server/provider/providerErrors.js +++ b/packages/@uppy/companion/src/server/provider/providerErrors.js @@ -1,7 +1,10 @@ const logger = require('../logger') -const { ProviderApiError, ProviderAuthError } = require('./error') +const { ProviderApiError, ProviderUserError, ProviderAuthError } = require('./error') -async function withProviderErrorHandling ({ fn, tag, providerName, isAuthError = () => false, getJsonErrorMessage }) { +async function withProviderErrorHandling ({ + // eslint-disable-next-line no-unused-vars + fn, tag, providerName, isAuthError = () => false, isUserFacingError = (response) => false, getJsonErrorMessage, +}) { function getErrorMessage (response) { if (typeof response.body === 'object') { const message = getJsonErrorMessage(response.body) @@ -25,6 +28,7 @@ async function withProviderErrorHandling ({ fn, tag, providerName, isAuthError = if (response) { // @ts-ignore if (isAuthError(response)) err2 = new ProviderAuthError() + if (isUserFacingError(response)) err2 = new ProviderUserError({ message: getErrorMessage(response) }) else err2 = new ProviderApiError(getErrorMessage(response), response.statusCode) } diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index 4dd13fb91a..d7986b002a 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -187,6 +187,13 @@ export default class ProviderView extends View { this.plugin.setPluginState({ folders, files, breadcrumbs, filterInput: '' }) }) } catch (err) { + // This is the first call that happens when the provider view loads, after auth, so it's probably nice to show any + // error occurring here to the user. + if (err instanceof UserFacingApiError) { + this.plugin.uppy.info({ message: this.plugin.uppy.i18n(err.message) }, 'warning', 5000) + return + } + this.handleError(err) } finally { this.setLoading(false) From e040c7b76ed52a442dbcb2eddf1caa8825b3dac9 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 6 Sep 2023 23:25:39 +0200 Subject: [PATCH 18/37] make `authProvider` consistent always use the static property ignoring the instance propety fixes #4460 --- packages/@uppy/companion/src/companion.js | 4 ++-- .../src/server/controllers/connect.js | 6 +++--- .../src/server/controllers/logout.js | 2 +- .../src/server/controllers/oauth-redirect.js | 2 +- .../@uppy/companion/src/server/helpers/jwt.js | 2 +- .../@uppy/companion/src/server/middlewares.js | 2 +- .../companion/src/server/provider/Provider.js | 2 +- .../src/server/provider/box/index.js | 4 ++-- .../src/server/provider/drive/index.js | 8 ++------ .../src/server/provider/dropbox/index.js | 4 ++-- .../src/server/provider/facebook/index.js | 8 ++------ .../companion/src/server/provider/index.js | 20 ++++++------------- .../server/provider/instagram/graph/index.js | 8 ++------ .../src/server/provider/onedrive/index.js | 8 ++------ .../src/server/provider/zoom/index.js | 8 ++------ .../test/__tests__/provider-manager.js | 10 +++++----- 16 files changed, 35 insertions(+), 63 deletions(-) diff --git a/packages/@uppy/companion/src/companion.js b/packages/@uppy/companion/src/companion.js index f14ddd2db9..20c8424e15 100644 --- a/packages/@uppy/companion/src/companion.js +++ b/packages/@uppy/companion/src/companion.js @@ -72,13 +72,13 @@ module.exports.app = (optionsArg = {}) => { const providers = providerManager.getDefaultProviders() - providerManager.addProviderOptions(options, grantConfig) - const { customProviders } = options if (customProviders) { providerManager.addCustomProviders(customProviders, providers, grantConfig) } + providerManager.addProviderOptions(options, grantConfig, providers) + // mask provider secrets from log messages logger.setMaskables(getMaskableSecrets(options)) diff --git a/packages/@uppy/companion/src/server/controllers/connect.js b/packages/@uppy/companion/src/server/controllers/connect.js index 3db52193c9..a4b81aa047 100644 --- a/packages/@uppy/companion/src/server/controllers/connect.js +++ b/packages/@uppy/companion/src/server/controllers/connect.js @@ -34,17 +34,17 @@ module.exports = function connect (req, res) { } const state = oAuthState.encodeState(stateObj, secret) - const { provider, providerGrantConfig } = req.companion + const { providerClass, providerGrantConfig } = req.companion // pass along grant's dynamic config (if specified for the provider in its grant config `dynamic` section) const grantDynamicConfig = Object.fromEntries(providerGrantConfig.dynamic?.map(p => [p, req.query[p]]) || []) - const providerName = provider.authProvider + const { authProvider } = providerClass const qs = queryString({ ...grantDynamicConfig, state, }) // Now we redirect to grant's /connect endpoint, see `app.use(Grant(grantConfig))` - res.redirect(req.companion.buildURL(`/connect/${providerName}${qs}`, true)) + res.redirect(req.companion.buildURL(`/connect/${authProvider}${qs}`, true)) } diff --git a/packages/@uppy/companion/src/server/controllers/logout.js b/packages/@uppy/companion/src/server/controllers/logout.js index 741dc61dda..d1fc5597d5 100644 --- a/packages/@uppy/companion/src/server/controllers/logout.js +++ b/packages/@uppy/companion/src/server/controllers/logout.js @@ -26,7 +26,7 @@ async function logout (req, res, next) { const { accessToken } = providerUserSession const data = await companion.provider.logout({ token: accessToken, providerUserSession, companion }) delete companion.providerUserSession - tokenService.removeFromCookies(res, companion.options, companion.provider.authProvider) + tokenService.removeFromCookies(res, companion.options, companion.providerClass.authProvider) cleanSession() res.json({ ok: true, ...data }) } catch (err) { diff --git a/packages/@uppy/companion/src/server/controllers/oauth-redirect.js b/packages/@uppy/companion/src/server/controllers/oauth-redirect.js index ff4f095448..4805da245d 100644 --- a/packages/@uppy/companion/src/server/controllers/oauth-redirect.js +++ b/packages/@uppy/companion/src/server/controllers/oauth-redirect.js @@ -10,7 +10,7 @@ const oAuthState = require('../helpers/oauth-state') */ module.exports = function oauthRedirect (req, res) { const params = qs.stringify(req.query) - const { authProvider } = req.companion.provider + const { authProvider } = req.companion.providerClass if (!req.companion.options.server.oauthDomain) { res.redirect(req.companion.buildURL(`/connect/${authProvider}/callback?${params}`, true)) return diff --git a/packages/@uppy/companion/src/server/helpers/jwt.js b/packages/@uppy/companion/src/server/helpers/jwt.js index 0db6249a47..48d2f1c50a 100644 --- a/packages/@uppy/companion/src/server/helpers/jwt.js +++ b/packages/@uppy/companion/src/server/helpers/jwt.js @@ -125,7 +125,7 @@ module.exports.addToCookiesIfNeeded = (req, res, uppyAuthToken, maxAge) => { res, token: uppyAuthToken, companionOptions: req.companion.options, - authProvider: req.companion.provider.authProvider, + authProvider: req.companion.providerClass.authProvider, maxAge, }) } diff --git a/packages/@uppy/companion/src/server/middlewares.js b/packages/@uppy/companion/src/server/middlewares.js index 325a4ff0f2..aa86115008 100644 --- a/packages/@uppy/companion/src/server/middlewares.js +++ b/packages/@uppy/companion/src/server/middlewares.js @@ -122,7 +122,7 @@ exports.gentleVerifyToken = (req, res, next) => { } exports.cookieAuthToken = (req, res, next) => { - req.companion.authToken = req.cookies[`uppyAuthToken--${req.companion.provider.authProvider}`] + req.companion.authToken = req.cookies[`uppyAuthToken--${req.companion.providerClass.authProvider}`] return next() } diff --git a/packages/@uppy/companion/src/server/provider/Provider.js b/packages/@uppy/companion/src/server/provider/Provider.js index e649cab1a6..b8b8c87f8a 100644 --- a/packages/@uppy/companion/src/server/provider/Provider.js +++ b/packages/@uppy/companion/src/server/provider/Provider.js @@ -122,5 +122,5 @@ class Provider { } module.exports = Provider -// OAuth providers are those that have a `static authProvider` set. It means they require OAuth authentication to work +// OAuth providers are those that have an `authProvider` set. It means they require OAuth authentication to work module.exports.isOAuthProvider = (authProvider) => typeof authProvider === 'string' && authProvider.length > 0 diff --git a/packages/@uppy/companion/src/server/provider/box/index.js b/packages/@uppy/companion/src/server/provider/box/index.js index a05913e435..120c2d0255 100644 --- a/packages/@uppy/companion/src/server/provider/box/index.js +++ b/packages/@uppy/companion/src/server/provider/box/index.js @@ -31,7 +31,6 @@ async function list ({ directory, query, token }) { class Box extends Provider { constructor (options) { super(options) - this.authProvider = Box.authProvider // needed for the thumbnails fetched via companion this.needsCookieAuth = true } @@ -116,11 +115,12 @@ class Box extends Provider { }) } + // eslint-disable-next-line class-methods-use-this async #withErrorHandling (tag, fn) { return withProviderErrorHandling({ fn, tag, - providerName: this.authProvider, + providerName: Box.authProvider, isAuthError: (response) => response.statusCode === 401, getJsonErrorMessage: (body) => body?.message, }) diff --git a/packages/@uppy/companion/src/server/provider/drive/index.js b/packages/@uppy/companion/src/server/provider/drive/index.js index edb3ace710..67e37165c2 100644 --- a/packages/@uppy/companion/src/server/provider/drive/index.js +++ b/packages/@uppy/companion/src/server/provider/drive/index.js @@ -41,11 +41,6 @@ async function getStats ({ id, token }) { * Adapter for API https://developers.google.com/drive/api/v3/ */ class Drive extends Provider { - constructor (options) { - super(options) - this.authProvider = Drive.authProvider - } - static get authProvider () { return 'google' } @@ -181,11 +176,12 @@ class Drive extends Provider { }) } + // eslint-disable-next-line class-methods-use-this async #withErrorHandling (tag, fn) { return withProviderErrorHandling({ fn, tag, - providerName: this.authProvider, + providerName: Drive.authProvider, isAuthError: (response) => ( response.statusCode === 401 || (response.statusCode === 400 && response.body?.error === 'invalid_grant') // Refresh token has expired or been revoked diff --git a/packages/@uppy/companion/src/server/provider/dropbox/index.js b/packages/@uppy/companion/src/server/provider/dropbox/index.js index f820581167..4838c556da 100644 --- a/packages/@uppy/companion/src/server/provider/dropbox/index.js +++ b/packages/@uppy/companion/src/server/provider/dropbox/index.js @@ -56,7 +56,6 @@ async function userInfo ({ token }) { class DropBox extends Provider { constructor (options) { super(options) - this.authProvider = DropBox.authProvider this.needsCookieAuth = true } @@ -136,11 +135,12 @@ class DropBox extends Provider { }) } + // eslint-disable-next-line class-methods-use-this async #withErrorHandling (tag, fn) { return withProviderErrorHandling({ fn, tag, - providerName: this.authProvider, + providerName: DropBox.authProvider, isAuthError: (response) => response.statusCode === 401, getJsonErrorMessage: (body) => body?.error_summary, }) diff --git a/packages/@uppy/companion/src/server/provider/facebook/index.js b/packages/@uppy/companion/src/server/provider/facebook/index.js index 08344b3f76..70a5f3ec63 100644 --- a/packages/@uppy/companion/src/server/provider/facebook/index.js +++ b/packages/@uppy/companion/src/server/provider/facebook/index.js @@ -24,11 +24,6 @@ async function getMediaUrl ({ token, id }) { * Adapter for API https://developers.facebook.com/docs/graph-api/using-graph-api/ */ class Facebook extends Provider { - constructor (options) { - super(options) - this.authProvider = Facebook.authProvider - } - static get authProvider () { return 'facebook' } @@ -86,11 +81,12 @@ class Facebook extends Provider { }) } + // eslint-disable-next-line class-methods-use-this async #withErrorHandling (tag, fn) { return withProviderErrorHandling({ fn, tag, - providerName: this.authProvider, + providerName: Facebook.authProvider, isAuthError: (response) => response.statusCode === 190, // Invalid OAuth 2.0 Access Token getJsonErrorMessage: (body) => body?.error?.message, }) diff --git a/packages/@uppy/companion/src/server/provider/index.js b/packages/@uppy/companion/src/server/provider/index.js index 8de1beaef9..17259ce48d 100644 --- a/packages/@uppy/companion/src/server/provider/index.js +++ b/packages/@uppy/companion/src/server/provider/index.js @@ -25,17 +25,6 @@ const validOptions = (options) => { return options.server.host && options.server.protocol } -/** - * - * @param {string} name of the provider - * @param {{server: object, providerOptions: object}} options - * @returns {string} the authProvider for this provider - */ -const providerNameToAuthName = (name, options) => { // eslint-disable-line no-unused-vars - const providers = exports.getDefaultProviders() - return (providers[name] || {}).authProvider -} - /** * adds the desired provider module to the request object, * based on the providerName parameter specified @@ -97,8 +86,9 @@ module.exports.addCustomProviders = (customProviders, providers, grantConfig) => // eslint-disable-next-line no-param-reassign providers[providerName] = customProvider.module + const { authProvider } = customProvider.module - if (isOAuthProvider(customProvider.module.authProvider)) { + if (isOAuthProvider(authProvider)) { // eslint-disable-next-line no-param-reassign grantConfig[providerName] = { ...customProvider.config, @@ -116,8 +106,9 @@ module.exports.addCustomProviders = (customProviders, providers, grantConfig) => * * @param {{server: object, providerOptions: object}} companionOptions * @param {object} grantConfig + * @param {Record} providers */ -module.exports.addProviderOptions = (companionOptions, grantConfig) => { +module.exports.addProviderOptions = (companionOptions, grantConfig, providers) => { const { server, providerOptions } = companionOptions if (!validOptions({ server })) { logger.warn('invalid provider options detected. Providers will not be loaded', 'provider.options.invalid') @@ -134,7 +125,8 @@ module.exports.addProviderOptions = (companionOptions, grantConfig) => { const { oauthDomain } = server const keys = Object.keys(providerOptions).filter((key) => key !== 'server') keys.forEach((providerName) => { - const authProvider = providerNameToAuthName(providerName, companionOptions) + const authProvider = providers[providerName]?.authProvider + if (isOAuthProvider(authProvider) && grantConfig[authProvider]) { // explicitly add providerOptions so users don't override other providerOptions. // eslint-disable-next-line no-param-reassign diff --git a/packages/@uppy/companion/src/server/provider/instagram/graph/index.js b/packages/@uppy/companion/src/server/provider/instagram/graph/index.js index bcc79caf07..0faa991b87 100644 --- a/packages/@uppy/companion/src/server/provider/instagram/graph/index.js +++ b/packages/@uppy/companion/src/server/provider/instagram/graph/index.js @@ -23,11 +23,6 @@ async function getMediaUrl ({ token, id }) { * Adapter for API https://developers.facebook.com/docs/instagram-api/overview */ class Instagram extends Provider { - constructor (options) { - super(options) - this.authProvider = Instagram.authProvider - } - // for "grant" static getExtraConfig () { return { @@ -86,11 +81,12 @@ class Instagram extends Provider { return { revoked: false, manual_revoke_url: 'https://www.instagram.com/accounts/manage_access/' } } + // eslint-disable-next-line class-methods-use-this async #withErrorHandling (tag, fn) { return withProviderErrorHandling({ fn, tag, - providerName: this.authProvider, + providerName: Instagram.authProvider, isAuthError: (response) => response.statusCode === 190, // Invalid OAuth 2.0 Access Token getJsonErrorMessage: (body) => body?.error?.message, }) diff --git a/packages/@uppy/companion/src/server/provider/onedrive/index.js b/packages/@uppy/companion/src/server/provider/onedrive/index.js index e12a3c5ac2..a653f29f70 100644 --- a/packages/@uppy/companion/src/server/provider/onedrive/index.js +++ b/packages/@uppy/companion/src/server/provider/onedrive/index.js @@ -23,11 +23,6 @@ const getRootPath = (query) => (query.driveId ? `drives/${query.driveId}` : 'me/ * Adapter for API https://docs.microsoft.com/en-us/onedrive/developer/rest-api/ */ class OneDrive extends Provider { - constructor (options) { - super(options) - this.authProvider = OneDrive.authProvider - } - static get authProvider () { return 'microsoft' } @@ -96,11 +91,12 @@ class OneDrive extends Provider { }) } + // eslint-disable-next-line class-methods-use-this async #withErrorHandling (tag, fn) { return withProviderErrorHandling({ fn, tag, - providerName: this.authProvider, + providerName: OneDrive.authProvider, isAuthError: (response) => response.statusCode === 401, getJsonErrorMessage: (body) => body?.error?.message, }) diff --git a/packages/@uppy/companion/src/server/provider/zoom/index.js b/packages/@uppy/companion/src/server/provider/zoom/index.js index 9be59a399d..ea0852dda5 100644 --- a/packages/@uppy/companion/src/server/provider/zoom/index.js +++ b/packages/@uppy/companion/src/server/provider/zoom/index.js @@ -29,11 +29,6 @@ async function findFile ({ client, meetingId, fileId, recordingStart }) { * Adapter for API https://marketplace.zoom.us/docs/api-reference/zoom-api */ class Zoom extends Provider { - constructor (options) { - super(options) - this.authProvider = Zoom.authProvider - } - static get authProvider () { return 'zoom' } @@ -157,6 +152,7 @@ class Zoom extends Provider { }) } + // eslint-disable-next-line class-methods-use-this async #withErrorHandling (tag, fn) { const authErrorCodes = [ 124, // expired token @@ -166,7 +162,7 @@ class Zoom extends Provider { return withProviderErrorHandling({ fn, tag, - providerName: this.authProvider, + providerName: Zoom.authProvider, isAuthError: (response) => authErrorCodes.includes(response.statusCode), getJsonErrorMessage: (body) => body?.message, }) diff --git a/packages/@uppy/companion/test/__tests__/provider-manager.js b/packages/@uppy/companion/test/__tests__/provider-manager.js index de9cf2b2a8..44e7d4d3b4 100644 --- a/packages/@uppy/companion/test/__tests__/provider-manager.js +++ b/packages/@uppy/companion/test/__tests__/provider-manager.js @@ -14,7 +14,7 @@ describe('Test Provider options', () => { }) test('adds provider options', () => { - providerManager.addProviderOptions(companionOptions, grantConfig) + providerManager.addProviderOptions(companionOptions, grantConfig, providerManager.getDefaultProviders()) expect(grantConfig.dropbox.key).toBe('dropbox_key') expect(grantConfig.dropbox.secret).toBe('dropbox_secret') @@ -33,7 +33,7 @@ describe('Test Provider options', () => { test('adds extra provider config', () => { process.env.COMPANION_INSTAGRAM_KEY = '123456' - providerManager.addProviderOptions(getCompanionOptions(), grantConfig) + providerManager.addProviderOptions(getCompanionOptions(), grantConfig, providerManager.getDefaultProviders()) expect(grantConfig.instagram).toEqual({ transport: 'session', callback: '/instagram/callback', @@ -101,7 +101,7 @@ describe('Test Provider options', () => { companionOptions = getCompanionOptions() - providerManager.addProviderOptions(companionOptions, grantConfig) + providerManager.addProviderOptions(companionOptions, grantConfig, providerManager.getDefaultProviders()) expect(grantConfig.dropbox.secret).toBe('xobpord') expect(grantConfig.box.secret).toBe('xwbepqd') @@ -115,7 +115,7 @@ describe('Test Provider options', () => { delete companionOptions.server.host delete companionOptions.server.protocol - providerManager.addProviderOptions(companionOptions, grantConfig) + providerManager.addProviderOptions(companionOptions, grantConfig, providerManager.getDefaultProviders()) expect(grantConfig.dropbox.key).toBeUndefined() expect(grantConfig.dropbox.secret).toBeUndefined() @@ -134,7 +134,7 @@ describe('Test Provider options', () => { test('sets a main redirect uri, if oauthDomain is set', () => { companionOptions.server.oauthDomain = 'domain.com' - providerManager.addProviderOptions(companionOptions, grantConfig) + providerManager.addProviderOptions(companionOptions, grantConfig, providerManager.getDefaultProviders()) expect(grantConfig.dropbox.redirect_uri).toBe('http://domain.com/dropbox/redirect') expect(grantConfig.box.redirect_uri).toBe('http://domain.com/box/redirect') From 408ac3d7cdf41227b2d80efb94ffb6dd55e7cef6 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 6 Sep 2023 23:28:25 +0200 Subject: [PATCH 19/37] fix bug --- packages/@uppy/companion/src/server/provider/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@uppy/companion/src/server/provider/index.js b/packages/@uppy/companion/src/server/provider/index.js index 17259ce48d..f3028011af 100644 --- a/packages/@uppy/companion/src/server/provider/index.js +++ b/packages/@uppy/companion/src/server/provider/index.js @@ -90,7 +90,7 @@ module.exports.addCustomProviders = (customProviders, providers, grantConfig) => if (isOAuthProvider(authProvider)) { // eslint-disable-next-line no-param-reassign - grantConfig[providerName] = { + grantConfig[authProvider] = { ...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 From e98af01324af26a9d23b9b1dd4da8b4d93785258 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 6 Sep 2023 23:46:49 +0200 Subject: [PATCH 20/37] fix test also --- 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 44e7d4d3b4..098a665ec8 100644 --- a/packages/@uppy/companion/test/__tests__/provider-manager.js +++ b/packages/@uppy/companion/test/__tests__/provider-manager.js @@ -157,8 +157,8 @@ describe('Test Custom Provider options', () => { }, }, providers, grantConfig) - expect(grantConfig.foo.key).toBe('foo_key') - expect(grantConfig.foo.secret).toBe('foo_secret') + expect(grantConfig.some_provider.key).toBe('foo_key') + expect(grantConfig.some_provider.secret).toBe('foo_secret') expect(providers.foo).toBeTruthy() }) }) From 3166db989e9706bda0ebca26dbcf79518fb8305c Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 7 Sep 2023 00:10:01 +0200 Subject: [PATCH 21/37] don't have default content-type --- packages/@uppy/companion/src/server/controllers/thumbnail.js | 4 ++-- packages/@uppy/companion/src/server/provider/box/index.js | 2 +- packages/@uppy/companion/src/server/provider/dropbox/index.js | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/@uppy/companion/src/server/controllers/thumbnail.js b/packages/@uppy/companion/src/server/controllers/thumbnail.js index 1071e381fa..b578bbb5c2 100644 --- a/packages/@uppy/companion/src/server/controllers/thumbnail.js +++ b/packages/@uppy/companion/src/server/controllers/thumbnail.js @@ -11,8 +11,8 @@ async function thumbnail (req, res, next) { const { accessToken } = providerUserSession try { - const { stream, contentType = 'image/jpeg' } = await provider.thumbnail({ id, token: accessToken, providerUserSession }) - res.set('Content-Type', contentType) + const { stream, contentType } = await provider.thumbnail({ id, token: accessToken, providerUserSession }) + if (contentType != null) res.set('Content-Type', contentType) stream.pipe(res) } catch (err) { if (respondWithError(err, res)) return diff --git a/packages/@uppy/companion/src/server/provider/box/index.js b/packages/@uppy/companion/src/server/provider/box/index.js index a05913e435..0de108136e 100644 --- a/packages/@uppy/companion/src/server/provider/box/index.js +++ b/packages/@uppy/companion/src/server/provider/box/index.js @@ -88,7 +88,7 @@ class Box extends Provider { }) await prepareStream(stream) - return { stream } + return { stream, contentType: 'image/jpeg' } }) } diff --git a/packages/@uppy/companion/src/server/provider/dropbox/index.js b/packages/@uppy/companion/src/server/provider/dropbox/index.js index f820581167..9eda38bef3 100644 --- a/packages/@uppy/companion/src/server/provider/dropbox/index.js +++ b/packages/@uppy/companion/src/server/provider/dropbox/index.js @@ -111,7 +111,7 @@ class DropBox extends Provider { }) await prepareStream(stream) - return { stream } + return { stream, contentType: 'image/jpeg' } }) } From 5c20186e805453098296eb51437491c13ef0ade1 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 7 Sep 2023 22:31:47 +0200 Subject: [PATCH 22/37] make a loginSimpleAuth api too --- packages/@uppy/companion-client/src/Provider.js | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/@uppy/companion-client/src/Provider.js b/packages/@uppy/companion-client/src/Provider.js index 3afbd703a5..6c02472b4d 100644 --- a/packages/@uppy/companion-client/src/Provider.js +++ b/packages/@uppy/companion-client/src/Provider.js @@ -112,7 +112,14 @@ export default class Provider extends RequestClient { return `${this.hostname}/${this.id}/connect?${params}` } - async login ({ uppyVersions, authFormData, signal }) { + /** @protected */ + async loginSimpleAuth ({ uppyVersions, authFormData, signal }) { + const response = await this.post(`${this.id}/simple-auth`, { form: authFormData }, { qs: { uppyVersions }, signal }) + this.setAuthToken(response.uppyAuthToken) + } + + /** @protected */ + async loginOAuth ({ uppyVersions, authFormData, signal }) { await this.ensurePreAuth() signal.throwIfAborted() @@ -168,6 +175,10 @@ export default class Provider extends RequestClient { }) } + async login ({ uppyVersions, authFormData, signal }) { + return this.loginOAuth({ uppyVersions, authFormData, signal }) + } + refreshTokenUrl () { return `${this.hostname}/${this.id}/refresh-token` } From 7549da4a41f1ea67816e7bf3b936d85021eafca5 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 2 Oct 2023 19:14:48 +0800 Subject: [PATCH 23/37] make removeAuthToken protected (cherry picked from commit 4be2b6fd9136d178966a6cca68204bf7ea38d09e) --- packages/@uppy/companion-client/src/Provider.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/@uppy/companion-client/src/Provider.js b/packages/@uppy/companion-client/src/Provider.js index 84f74a731f..bfd939f076 100644 --- a/packages/@uppy/companion-client/src/Provider.js +++ b/packages/@uppy/companion-client/src/Provider.js @@ -75,7 +75,8 @@ export default class Provider extends RequestClient { return this.uppy.getPlugin(this.pluginId).storage.getItem(this.tokenKey) } - async #removeAuthToken () { + /** @protected */ + async removeAuthToken () { return this.uppy.getPlugin(this.pluginId).storage.removeItem(this.tokenKey) } @@ -213,7 +214,7 @@ export default class Provider extends RequestClient { } catch (refreshTokenErr) { if (refreshTokenErr.isAuthError) { // if refresh-token has failed with auth error, delete token, so we don't keep trying to refresh in future - await this.#removeAuthToken() + await this.removeAuthToken() } throw err } finally { @@ -248,7 +249,7 @@ export default class Provider extends RequestClient { async logout (options) { const response = await this.get(`${this.id}/logout`, options) - await this.#removeAuthToken() + await this.removeAuthToken() return response } From 42f74b262e5fc47c197069a8d8187af2a326784b Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 27 Nov 2023 20:44:50 +0800 Subject: [PATCH 24/37] fix lint --- .../src/server/provider/providerErrors.js | 25 +++++++++++++++---- .../@uppy/utils/src/dataURItoBlob.test.ts | 2 +- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/packages/@uppy/companion/src/server/provider/providerErrors.js b/packages/@uppy/companion/src/server/provider/providerErrors.js index e3c2a3f631..5c9860fbf3 100644 --- a/packages/@uppy/companion/src/server/provider/providerErrors.js +++ b/packages/@uppy/companion/src/server/provider/providerErrors.js @@ -7,12 +7,23 @@ const { StreamHttpJsonError } = require('../helpers/utils') /** * * @param {{ - * fn: () => any, tag: string, providerName: string, isAuthError?: (a: { statusCode: number, body?: object }) => boolean, isUserFacingError?: (a: { statusCode: number, body?: object }) => boolean, + * fn: () => any, + * tag: string, + * providerName: string, + * isAuthError?: (a: { statusCode: number, body?: object }) => boolean, + * isUserFacingError?: (a: { statusCode: number, body?: object }) => boolean, * getJsonErrorMessage: (a: object) => string * }} param0 * @returns */ -async function withProviderErrorHandling ({ fn, tag, providerName, isAuthError = () => false, isUserFacingError = () => false, getJsonErrorMessage }) { +async function withProviderErrorHandling ({ + fn, + tag, + providerName, + isAuthError = () => false, + isUserFacingError = () => false, + getJsonErrorMessage, +}) { function getErrorMessage ({ statusCode, body }) { if (typeof body === 'object') { const message = getJsonErrorMessage(body) @@ -42,9 +53,13 @@ async function withProviderErrorHandling ({ fn, tag, providerName, isAuthError = if (statusCode != null) { let knownErr - if (isAuthError({ statusCode, body })) knownErr = new ProviderAuthError() - else if (isUserFacingError({ statusCode, body })) knownErr = new ProviderUserError({ message: getErrorMessage({ statusCode, body }) }) - else knownErr = new ProviderApiError(getErrorMessage({ statusCode, body }), statusCode) + if (isAuthError({ statusCode, body })) { + knownErr = new ProviderAuthError() + } else if (isUserFacingError({ statusCode, body })) { + knownErr = new ProviderUserError({ message: getErrorMessage({ statusCode, body }) }) + } else { + knownErr = new ProviderApiError(getErrorMessage({ statusCode, body }), statusCode) + } logger.error(knownErr, tag) throw knownErr diff --git a/packages/@uppy/utils/src/dataURItoBlob.test.ts b/packages/@uppy/utils/src/dataURItoBlob.test.ts index a8539f9707..bafbb64800 100644 --- a/packages/@uppy/utils/src/dataURItoBlob.test.ts +++ b/packages/@uppy/utils/src/dataURItoBlob.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest' import dataURItoBlob from './dataURItoBlob.ts' -import sampleImageDataURI from './sampleImageDataURI.js' +import sampleImageDataURI from './sampleImageDataURI.ts' describe('dataURItoBlob', () => { it('should convert a data uri to a blob', () => { From 7a84c8dd403e4526c62e0bbad95b7a52b4e4714f Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 27 Nov 2023 20:53:33 +0800 Subject: [PATCH 25/37] run yarn format --- packages/@uppy/utils/src/Translator.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/@uppy/utils/src/Translator.ts b/packages/@uppy/utils/src/Translator.ts index 866a82ccef..3fcef5991d 100644 --- a/packages/@uppy/utils/src/Translator.ts +++ b/packages/@uppy/utils/src/Translator.ts @@ -100,7 +100,10 @@ const defaultOnMissingKey = (key: string): void => { export default class Translator { protected locale: Locale - constructor(locales: Locale | Locale[], { onMissingKey = defaultOnMissingKey } = {}) { + constructor( + locales: Locale | Locale[], + { onMissingKey = defaultOnMissingKey } = {}, + ) { this.locale = { strings: {}, pluralize(n: number): 0 | 1 { @@ -170,7 +173,7 @@ export default class Translator { } if (typeof string !== 'string') { - throw new Error(`string was not a string`); + throw new Error(`string was not a string`) } return interpolate(string, options) From 38eee7290f17fc4fd300a51520d1fadc57e1d52d Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 29 Nov 2023 11:59:51 +0800 Subject: [PATCH 26/37] Apply suggestions from code review Co-authored-by: Antoine du Hamel --- packages/@uppy/companion-client/src/Provider.js | 2 +- packages/@uppy/companion-client/src/RequestClient.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/@uppy/companion-client/src/Provider.js b/packages/@uppy/companion-client/src/Provider.js index 5bd40e0c0e..06c9e5d9d5 100644 --- a/packages/@uppy/companion-client/src/Provider.js +++ b/packages/@uppy/companion-client/src/Provider.js @@ -171,7 +171,7 @@ export default class Provider extends RequestClient { } cleanup() - this.setAuthToken(data.token).then(() => resolve()).catch(reject) + resolve(this.setAuthToken(data.token)) } cleanup = () => { diff --git a/packages/@uppy/companion-client/src/RequestClient.js b/packages/@uppy/companion-client/src/RequestClient.js index 1c27821151..9b3e92eccc 100644 --- a/packages/@uppy/companion-client/src/RequestClient.js +++ b/packages/@uppy/companion-client/src/RequestClient.js @@ -50,9 +50,9 @@ async function handleJSONResponse (res) { errMsg = errData.requestId ? `${errMsg} request-Id: ${errData.requestId}` : errMsg - } catch { + } catch (cause) { // if the response contains invalid JSON, let's ignore the error data - throw new Error(errMsg) + throw new Error(errMsg, { cause }) } if (res.status >= 400 && res.status <= 499 && errData.message) { From 70a7a489909c65fa38d0bcae146b9cf710685382 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 29 Nov 2023 12:25:20 +0800 Subject: [PATCH 27/37] fix broken merge conflict --- packages/@uppy/companion-client/src/RequestClient.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/@uppy/companion-client/src/RequestClient.js b/packages/@uppy/companion-client/src/RequestClient.js index 9b3e92eccc..8fe377341a 100644 --- a/packages/@uppy/companion-client/src/RequestClient.js +++ b/packages/@uppy/companion-client/src/RequestClient.js @@ -46,10 +46,8 @@ async function handleJSONResponse (res) { try { errData = await res.json() - errMsg = errData.message ? `${errMsg} message: ${errData.message}` : errMsg - errMsg = errData.requestId - ? `${errMsg} request-Id: ${errData.requestId}` - : errMsg + if (errData.message) errMsg = `${errMsg} message: ${errData.message}` + if (errData.requestId) errMsg = `${errMsg} request-Id: ${errData.requestId}` } catch (cause) { // if the response contains invalid JSON, let's ignore the error data throw new Error(errMsg, { cause }) @@ -59,8 +57,6 @@ async function handleJSONResponse (res) { throw new UserFacingApiError(errData.message) } - errMsg = errData.message ? `${errMsg} message: ${errData.message}` : errMsg - errMsg = errData.requestId ? `${errMsg} request-Id: ${errData.requestId}` : errMsg throw new HttpError({ statusCode: res.status, message: errMsg }) } From 0d81a9b567ad7c63b902ddb3606bdd39dd376518 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Fri, 1 Dec 2023 14:41:11 +0800 Subject: [PATCH 28/37] improve inheritance --- packages/@uppy/companion/src/server/logger.js | 4 ++-- packages/@uppy/companion/src/server/provider/error.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/@uppy/companion/src/server/logger.js b/packages/@uppy/companion/src/server/logger.js index f92f381041..77c1d53e53 100644 --- a/packages/@uppy/companion/src/server/logger.js +++ b/packages/@uppy/companion/src/server/logger.js @@ -1,7 +1,7 @@ const chalk = require('chalk') const escapeStringRegexp = require('escape-string-regexp') const util = require('node:util') -const { ProviderApiError, ProviderUserError, ProviderAuthError } = require('./provider/error') +const { ProviderApiError } = require('./provider/error') const valuesToMask = [] /** @@ -56,7 +56,7 @@ const log = ({ arg, tag = '', level, traceId = '', color = (message) => message function msgToString () { // We don't need to log stack trace on special errors that we ourselves have produced // (to reduce log noise) - if ((arg instanceof ProviderApiError || arg instanceof ProviderUserError || arg instanceof ProviderAuthError) && typeof arg.message === 'string') { + if ((arg instanceof ProviderApiError) && typeof arg.message === 'string') { return arg.message } if (typeof arg === 'string') return arg diff --git a/packages/@uppy/companion/src/server/provider/error.js b/packages/@uppy/companion/src/server/provider/error.js index 78ed4d0a8b..30e5e40ce5 100644 --- a/packages/@uppy/companion/src/server/provider/error.js +++ b/packages/@uppy/companion/src/server/provider/error.js @@ -16,12 +16,12 @@ class ProviderApiError extends Error { } } -class ProviderUserError extends Error { +class ProviderUserError extends ProviderApiError { /** * @param {object} json arbitrary JSON.stringify-able object that will be passed to the client */ constructor (json) { - super('User error') + super('User error', undefined) this.json = json } } From 96eb565ead85e8f1ea829030b7715119311e2107 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Fri, 1 Dec 2023 16:19:52 +0800 Subject: [PATCH 29/37] fix bug --- packages/@uppy/companion/src/server/provider/error.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/@uppy/companion/src/server/provider/error.js b/packages/@uppy/companion/src/server/provider/error.js index 30e5e40ce5..4f05adb2fc 100644 --- a/packages/@uppy/companion/src/server/provider/error.js +++ b/packages/@uppy/companion/src/server/provider/error.js @@ -48,6 +48,10 @@ function errorToResponse (err) { return { code: 401, json: { message: err.message } } } + if (err instanceof ProviderUserError) { + return { code: 400, json: err.json } + } + if (err instanceof ProviderApiError) { if (err.statusCode >= 500) { // bad gateway i.e the provider APIs gateway @@ -64,10 +68,6 @@ function errorToResponse (err) { } } - if (err instanceof ProviderUserError) { - return { code: 400, json: err.json } - } - return undefined } From 67d8595902b23395894db24d78e186a841bdb19d Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Sat, 2 Dec 2023 21:51:37 +0800 Subject: [PATCH 30/37] fix bug with dynamic grant config --- packages/@uppy/companion/src/companion.js | 2 +- .../companion/src/server/controllers/connect.js | 14 ++++++++++++-- .../@uppy/companion/src/server/provider/index.js | 2 +- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/packages/@uppy/companion/src/companion.js b/packages/@uppy/companion/src/companion.js index 78f615866f..564a108fb7 100644 --- a/packages/@uppy/companion/src/companion.js +++ b/packages/@uppy/companion/src/companion.js @@ -142,8 +142,8 @@ module.exports.app = (optionsArg = {}) => { if (options.testDynamicOauthCredentials) { app.post('/:providerName/test-dynamic-oauth-credentials', (req, res) => { if (req.query.secret !== options.testDynamicOauthCredentialsSecret) throw new Error('Invalid secret') - logger.info('Returning dynamic OAuth2 credentials') const { providerName } = req.params + logger.info(`Returning dynamic OAuth2 credentials for ${providerName}`) // for simplicity, we just return the normal credentials for the provider, but in a real-world scenario, // we would query based on parameters const { key, secret } = options.providerOptions[providerName] diff --git a/packages/@uppy/companion/src/server/controllers/connect.js b/packages/@uppy/companion/src/server/controllers/connect.js index 3db52193c9..17ae018e7b 100644 --- a/packages/@uppy/companion/src/server/controllers/connect.js +++ b/packages/@uppy/companion/src/server/controllers/connect.js @@ -12,7 +12,7 @@ const queryString = (params, prefix = '?') => { * @param {object} req * @param {object} res */ -module.exports = function connect (req, res) { +module.exports = function connect(req, res) { const { secret } = req.companion.options const stateObj = oAuthState.generateState() @@ -37,7 +37,17 @@ module.exports = function connect (req, res) { const { provider, providerGrantConfig } = req.companion // pass along grant's dynamic config (if specified for the provider in its grant config `dynamic` section) - const grantDynamicConfig = Object.fromEntries(providerGrantConfig.dynamic?.map(p => [p, req.query[p]]) || []) + // this is needed for things like custom oauth domain (e.g. webdav) + const grantDynamicConfig = Object.fromEntries(providerGrantConfig.dynamic?.flatMap((dynamicKey) => { + const queryValue = req.query[dynamicKey]; + + // note: when using credentialsURL (dynamic oauth credentials), dynamic has ['key', 'secret', 'redirect_uri'] + // but in that case, query string is empty, so we need to only fetch these parameters from QS if they exist. + if (!queryValue) return [] + return [[ + dynamicKey, queryValue + ]] + }) || []) const providerName = provider.authProvider const qs = queryString({ diff --git a/packages/@uppy/companion/src/server/provider/index.js b/packages/@uppy/companion/src/server/provider/index.js index ab15888635..e0b927dbc8 100644 --- a/packages/@uppy/companion/src/server/provider/index.js +++ b/packages/@uppy/companion/src/server/provider/index.js @@ -36,7 +36,7 @@ const providerNameToAuthName = (name, options) => { // eslint-disable-line no-un return (providers[name] || {}).authProvider } -function getGrantConfigForProvider ({ providerName, companionOptions, grantConfig }) { +function getGrantConfigForProvider({ providerName, companionOptions, grantConfig }) { const authProvider = providerNameToAuthName(providerName, companionOptions) if (!isOAuthProvider(authProvider)) return undefined From 5025a733018f8b7380b6aced1500c8bac9ed1670 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 5 Dec 2023 15:57:19 +0800 Subject: [PATCH 31/37] use duck typing for error checks see discussion here: https://github.com/transloadit/uppy/pull/4619#discussion_r1406225982 --- .../@uppy/companion-client/src/AuthError.js | 7 +- .../@uppy/companion-client/src/Provider.js | 48 ++++++------ .../companion-client/src/RequestClient.js | 55 +++++++------- .../@uppy/companion/src/server/Uploader.js | 75 +++++++++++-------- .../companion/src/server/helpers/upload.js | 24 +++--- .../companion/src/server/helpers/utils.js | 11 +-- packages/@uppy/companion/src/server/logger.js | 8 +- .../companion/src/server/provider/error.js | 22 ++++-- .../src/server/provider/providerErrors.js | 15 ++-- .../@uppy/companion/src/standalone/index.js | 6 +- .../src/ProviderView/ProviderView.jsx | 5 +- 11 files changed, 146 insertions(+), 130 deletions(-) diff --git a/packages/@uppy/companion-client/src/AuthError.js b/packages/@uppy/companion-client/src/AuthError.js index b985bf0953..6c77e794f0 100644 --- a/packages/@uppy/companion-client/src/AuthError.js +++ b/packages/@uppy/companion-client/src/AuthError.js @@ -1,10 +1,13 @@ 'use strict' class AuthError extends Error { - constructor () { + constructor() { super('Authorization required') this.name = 'AuthError' - this.isAuthError = true // todo remove in next major and pull AuthError into a shared package + + // we use a property because of instanceof is unsafe: + // https://github.com/transloadit/uppy/pull/4619#discussion_r1406225982 + this.isAuthError = true } } diff --git a/packages/@uppy/companion-client/src/Provider.js b/packages/@uppy/companion-client/src/Provider.js index 06c9e5d9d5..fbd3a434aa 100644 --- a/packages/@uppy/companion-client/src/Provider.js +++ b/packages/@uppy/companion-client/src/Provider.js @@ -2,18 +2,18 @@ import RequestClient, { authErrorStatusCode } from './RequestClient.js' import * as tokenStorage from './tokenStorage.js' -import AuthError from './AuthError.js' + const getName = (id) => { return id.split('-').map((s) => s.charAt(0).toUpperCase() + s.slice(1)).join(' ') } -function getOrigin () { +function getOrigin() { // eslint-disable-next-line no-restricted-globals return location.origin } -function getRegex (value) { +function getRegex(value) { if (typeof value === 'string') { return new RegExp(`^${value}$`) } if (value instanceof RegExp) { @@ -22,7 +22,7 @@ function getRegex (value) { return undefined } -function isOriginAllowed (origin, allowedOrigin) { +function isOriginAllowed(origin, allowedOrigin) { const patterns = Array.isArray(allowedOrigin) ? allowedOrigin.map(getRegex) : [getRegex(allowedOrigin)] return patterns .some((pattern) => pattern?.test(origin) || pattern?.test(`${origin}/`)) // allowing for trailing '/' @@ -31,7 +31,7 @@ function isOriginAllowed (origin, allowedOrigin) { export default class Provider extends RequestClient { #refreshingTokenPromise - constructor (uppy, opts) { + constructor(uppy, opts) { super(uppy, opts) this.provider = opts.provider this.id = this.provider @@ -43,7 +43,7 @@ export default class Provider extends RequestClient { this.supportsRefreshToken = opts.supportsRefreshToken ?? true // todo false in next major } - async headers () { + async headers() { const [headers, token] = await Promise.all([super.headers(), this.#getAuthToken()]) const authHeaders = {} if (token) { @@ -58,7 +58,7 @@ export default class Provider extends RequestClient { return { ...headers, ...authHeaders } } - onReceiveResponse (response) { + onReceiveResponse(response) { super.onReceiveResponse(response) const plugin = this.uppy.getPlugin(this.pluginId) const oldAuthenticated = plugin.getPluginState().authenticated @@ -67,16 +67,16 @@ export default class Provider extends RequestClient { return response } - async setAuthToken (token) { + async setAuthToken(token) { return this.uppy.getPlugin(this.pluginId).storage.setItem(this.tokenKey, token) } - async #getAuthToken () { + async #getAuthToken() { return this.uppy.getPlugin(this.pluginId).storage.getItem(this.tokenKey) } /** @protected */ - async removeAuthToken () { + async removeAuthToken() { return this.uppy.getPlugin(this.pluginId).storage.removeItem(this.tokenKey) } @@ -84,7 +84,7 @@ export default class Provider extends RequestClient { * Ensure we have a preauth token if necessary. Attempts to fetch one if we don't, * or rejects if loading one fails. */ - async ensurePreAuth () { + async ensurePreAuth() { if (this.companionKeysParams && !this.preAuthToken) { await this.fetchPreAuthToken() @@ -95,11 +95,11 @@ export default class Provider extends RequestClient { } // eslint-disable-next-line class-methods-use-this - authQuery () { + authQuery() { return {} } - authUrl ({ authFormData, query } = {}) { + authUrl({ authFormData, query } = {}) { const params = new URLSearchParams({ ...query, state: btoa(JSON.stringify({ origin: getOrigin() })), @@ -114,13 +114,13 @@ export default class Provider extends RequestClient { } /** @protected */ - async loginSimpleAuth ({ uppyVersions, authFormData, signal }) { + async loginSimpleAuth({ uppyVersions, authFormData, signal }) { const response = await this.post(`${this.id}/simple-auth`, { form: authFormData }, { qs: { uppyVersions }, signal }) this.setAuthToken(response.uppyAuthToken) } /** @protected */ - async loginOAuth ({ uppyVersions, authFormData, signal }) { + async loginOAuth({ uppyVersions, authFormData, signal }) { await this.ensurePreAuth() signal.throwIfAborted() @@ -185,20 +185,20 @@ export default class Provider extends RequestClient { }) } - async login ({ uppyVersions, authFormData, signal }) { + async login({ uppyVersions, authFormData, signal }) { return this.loginOAuth({ uppyVersions, authFormData, signal }) } - refreshTokenUrl () { + refreshTokenUrl() { return `${this.hostname}/${this.id}/refresh-token` } - fileUrl (id) { + fileUrl(id) { return `${this.hostname}/${this.id}/get/${id}` } /** @protected */ - async request (...args) { + async request(...args) { await this.#refreshingTokenPromise try { @@ -213,7 +213,7 @@ export default class Provider extends RequestClient { if (!this.supportsRefreshToken) throw err // only handle auth errors (401 from provider), and only handle them if we have a (refresh) token const authTokenAfter = await this.#getAuthToken() - if (!(err instanceof AuthError) || !authTokenAfter) throw err + if (!err.isAuthError || !authTokenAfter) throw err if (this.#refreshingTokenPromise == null) { // Many provider requests may be starting at once, however refresh token should only be called once. @@ -242,7 +242,7 @@ export default class Provider extends RequestClient { } } - async fetchPreAuthToken () { + async fetchPreAuthToken() { if (!this.companionKeysParams) { return } @@ -255,17 +255,17 @@ export default class Provider extends RequestClient { } } - list (directory, options) { + list(directory, options) { return this.get(`${this.id}/list/${directory || ''}`, options) } - async logout (options) { + async logout(options) { const response = await this.get(`${this.id}/logout`, options) await this.removeAuthToken() return response } - static initPlugin (plugin, opts, defaultOpts) { + static initPlugin(plugin, opts, defaultOpts) { /* eslint-disable no-param-reassign */ plugin.type = 'acquirer' plugin.files = [] diff --git a/packages/@uppy/companion-client/src/RequestClient.js b/packages/@uppy/companion-client/src/RequestClient.js index 8fe377341a..c0e3160723 100644 --- a/packages/@uppy/companion-client/src/RequestClient.js +++ b/packages/@uppy/companion-client/src/RequestClient.js @@ -14,7 +14,7 @@ import AuthError from './AuthError.js' import packageJson from '../package.json' // Remove the trailing slash so we can always safely append /xyz. -function stripSlash (url) { +function stripSlash(url) { return url.replace(/\/$/, '') } @@ -28,11 +28,12 @@ class HttpError extends Error { constructor({ statusCode, message }) { super(message) + this.name = 'HttpError' this.statusCode = statusCode } } -async function handleJSONResponse (res) { +async function handleJSONResponse(res) { if (res.status === authErrorStatusCode) { throw new AuthError() } @@ -68,28 +69,28 @@ export default class RequestClient { #companionHeaders - constructor (uppy, opts) { + constructor(uppy, opts) { this.uppy = uppy this.opts = opts this.onReceiveResponse = this.onReceiveResponse.bind(this) this.#companionHeaders = opts?.companionHeaders } - setCompanionHeaders (headers) { + setCompanionHeaders(headers) { this.#companionHeaders = headers } - [Symbol.for('uppy test: getCompanionHeaders')] () { + [Symbol.for('uppy test: getCompanionHeaders')]() { return this.#companionHeaders } - get hostname () { + get hostname() { const { companion } = this.uppy.getState() const host = this.opts.companionUrl return stripSlash(companion && companion[host] ? companion[host] : host) } - async headers () { + async headers() { const defaultHeaders = { Accept: 'application/json', 'Content-Type': 'application/json', @@ -102,7 +103,7 @@ export default class RequestClient { } } - onReceiveResponse ({ headers }) { + onReceiveResponse({ headers }) { const state = this.uppy.getState() const companion = state.companion || {} const host = this.opts.companionUrl @@ -115,7 +116,7 @@ export default class RequestClient { } } - #getUrl (url) { + #getUrl(url) { if (/^(https?:|)\/\//.test(url)) { return url } @@ -136,7 +137,7 @@ export default class RequestClient { Subsequent requests use the cached result of the preflight. However if there is an error retrieving the allowed headers, we will try again next time */ - async preflight (path) { + async preflight(path) { const allowedHeadersCached = allowedHeadersCache.get(this.hostname) if (allowedHeadersCached != null) return allowedHeadersCached @@ -181,7 +182,7 @@ export default class RequestClient { return promise } - async preflightAndHeaders (path) { + async preflightAndHeaders(path) { const [allowedHeaders, headers] = await Promise.all([ this.preflight(path), this.headers(), @@ -201,7 +202,7 @@ export default class RequestClient { } /** @protected */ - async request ({ path, method = 'GET', data, skipPostResponse, signal }) { + async request({ path, method = 'GET', data, skipPostResponse, signal }) { try { const headers = await this.preflightAndHeaders(path) const response = await fetchWithNetworkError(this.#getUrl(path), { @@ -216,7 +217,7 @@ export default class RequestClient { return await handleJSONResponse(response) } catch (err) { // pass these through - if (err instanceof AuthError || err instanceof UserFacingApiError || err.name === 'AbortError') throw err + if (err.isAuthError || err.name === 'UserFacingApiError' || err.name === 'AbortError') throw err throw new ErrorWithCause(`Could not ${method} ${this.#getUrl(path)}`, { cause: err, @@ -224,21 +225,21 @@ export default class RequestClient { } } - async get (path, options = undefined) { + async get(path, options = undefined) { // TODO: remove boolean support for options that was added for backward compatibility. // eslint-disable-next-line no-param-reassign if (typeof options === 'boolean') options = { skipPostResponse: options } return this.request({ ...options, path }) } - async post (path, data, options = undefined) { + async post(path, data, options = undefined) { // TODO: remove boolean support for options that was added for backward compatibility. // eslint-disable-next-line no-param-reassign if (typeof options === 'boolean') options = { skipPostResponse: options } return this.request({ ...options, path, method: 'POST', data }) } - async delete (path, data = undefined, options) { + async delete(path, data = undefined, options) { // TODO: remove boolean support for options that was added for backward compatibility. // eslint-disable-next-line no-param-reassign if (typeof options === 'boolean') options = { skipPostResponse: options } @@ -258,7 +259,7 @@ export default class RequestClient { * @param {*} options * @returns */ - async uploadRemoteFile (file, reqBody, options = {}) { + async uploadRemoteFile(file, reqBody, options = {}) { try { const { signal, getQueue } = options @@ -275,7 +276,7 @@ export default class RequestClient { return await this.#requestSocketToken(...args) } catch (outerErr) { // throwing AbortError will cause p-retry to stop retrying - if (outerErr instanceof AuthError) throw new AbortError(outerErr) + if (outerErr.isAuthError) throw new AbortError(outerErr) if (outerErr.cause == null) throw outerErr const err = outerErr.cause @@ -284,8 +285,8 @@ export default class RequestClient { [408, 409, 429, 418, 423].includes(err.statusCode) || (err.statusCode >= 500 && err.statusCode <= 599 && ![501, 505].includes(err.statusCode)) ) - if (err instanceof HttpError && !isRetryableHttpError()) throw new AbortError(err); - + if (err.name === 'HttpError' && !isRetryableHttpError()) throw new AbortError(err); + // p-retry will retry most other errors, // but it will not retry TypeError (except network error TypeErrors) throw err @@ -337,7 +338,7 @@ export default class RequestClient { * * @param {{ file: UppyFile, queue: RateLimitedQueue, signal: AbortSignal }} file */ - async #awaitRemoteFileUpload ({ file, queue, signal }) { + async #awaitRemoteFileUpload({ file, queue, signal }) { let removeEventHandlers const { capabilities } = this.uppy.getState() @@ -364,7 +365,7 @@ export default class RequestClient { socket.send(JSON.stringify({ action, payload: payload ?? {}, - })) + })) }; function sendState() { @@ -384,7 +385,7 @@ export default class RequestClient { socketAbortController?.abort?.() reject(err) } - + // todo instead implement the ability for users to cancel / retry *currently uploading files* in the UI function resetActivityTimeout() { clearTimeout(activityTimeout) @@ -419,7 +420,7 @@ export default class RequestClient { try { const { action, payload } = JSON.parse(e.data) - + switch (action) { case 'progress': { emitSocketProgress(this, payload, file) @@ -435,8 +436,8 @@ export default class RequestClient { const { message } = payload.error throw Object.assign(new Error(message), { cause: payload.error }) } - default: - this.uppy.log(`Companion socket unknown action ${action}`, 'warning') + default: + this.uppy.log(`Companion socket unknown action ${action}`, 'warning') } } catch (err) { onFatalError(err) @@ -449,7 +450,7 @@ export default class RequestClient { if (socket) socket.close() socket = undefined } - + socketAbortController.signal.addEventListener('abort', () => { closeSocket() }) diff --git a/packages/@uppy/companion/src/server/Uploader.js b/packages/@uppy/companion/src/server/Uploader.js index 994580513b..663b0b4cc5 100644 --- a/packages/@uppy/companion/src/server/Uploader.js +++ b/packages/@uppy/companion/src/server/Uploader.js @@ -41,12 +41,12 @@ const PROTOCOLS = Object.freeze({ tus: 'tus', }) -function exceedsMaxFileSize (maxFileSize, size) { +function exceedsMaxFileSize(maxFileSize, size) { return maxFileSize && size && size > maxFileSize } // TODO remove once we migrate away from form-data -function sanitizeMetadata (inputMetadata) { +function sanitizeMetadata(inputMetadata) { if (inputMetadata == null) return {} const outputMetadata = {} @@ -56,16 +56,27 @@ function sanitizeMetadata (inputMetadata) { return outputMetadata } -class AbortError extends Error {} +class AbortError extends Error { + constructor(message) { + super(message) + this.isAbortError = true + } +} + +class ValidationError extends Error { + constructor(message) { + super(message) -class ValidationError extends Error {} + this.name = 'ValidationError' + } +} /** * Validate the options passed down to the uplaoder * * @param {UploaderOptions} options */ -function validateOptions (options) { +function validateOptions(options) { // validate HTTP Method if (options.httpMethod) { if (typeof options.httpMethod !== 'string') { @@ -155,7 +166,7 @@ class Uploader { * * @param {UploaderOptions} options */ - constructor (options) { + constructor(options) { validateOptions(options) this.options = options @@ -200,18 +211,18 @@ class Uploader { this._paused = true if (this.tus) { const shouldTerminate = !!this.tus.url - this.tus.abort(shouldTerminate).catch(() => {}) + this.tus.abort(shouldTerminate).catch(() => { }) } this.abortReadStream(new AbortError()) }) } - abortReadStream (err) { + abortReadStream(err) { this.uploadStopped = true if (this.readStream) this.readStream.destroy(err) } - async _uploadByProtocol () { + async _uploadByProtocol() { // todo a default protocol should not be set. We should ensure that the user specifies their protocol. // after we drop old versions of uppy client we can remove this const protocol = this.options.protocol || PROTOCOLS.multipart @@ -228,7 +239,7 @@ class Uploader { } } - async _downloadStreamAsFile (stream) { + async _downloadStreamAsFile(stream) { this.tmpPath = join(this.options.pathPrefix, this.fileName) logger.debug('fully downloading file', 'uploader.download', this.shortToken) @@ -253,7 +264,7 @@ class Uploader { this.readStream = fileStream } - _needDownloadFirst () { + _needDownloadFirst() { return !this.options.size || !this.options.companionOptions.streamingUpload } @@ -261,7 +272,7 @@ class Uploader { * * @param {import('stream').Readable} stream */ - async uploadStream (stream) { + async uploadStream(stream) { try { if (this.uploadStopped) throw new Error('Cannot upload stream after upload stopped') if (this.readStream) throw new Error('Already uploading') @@ -289,15 +300,15 @@ class Uploader { } } - tryDeleteTmpPath () { - if (this.tmpPath) unlink(this.tmpPath).catch(() => {}) + tryDeleteTmpPath() { + if (this.tmpPath) unlink(this.tmpPath).catch(() => { }) } /** * * @param {import('stream').Readable} stream */ - async tryUploadStream (stream) { + async tryUploadStream(stream) { try { emitter().emit('upload-start', { token: this.token }) @@ -306,7 +317,7 @@ class Uploader { const { url, extraData } = ret this.#emitSuccess(url, extraData) } catch (err) { - if (err instanceof AbortError) { + if (err.isAbortError) { logger.error('Aborted upload', 'uploader.aborted', this.shortToken) return } @@ -328,11 +339,11 @@ class Uploader { * @param {string} token the token to Shorten * @returns {string} */ - static shortenToken (token) { + static shortenToken(token) { return token.substring(0, 8) } - static reqToOptions (req, size) { + static reqToOptions(req, size) { const useFormDataIsSet = Object.prototype.hasOwnProperty.call(req.body, 'useFormData') const useFormData = useFormDataIsSet ? req.body.useFormData : true @@ -365,11 +376,11 @@ class Uploader { * we avoid using the entire token because this is meant to be a short term * access token between uppy client and companion websocket */ - get shortToken () { + get shortToken() { return Uploader.shortenToken(this.token) } - async awaitReady (timeout) { + async awaitReady(timeout) { logger.debug('waiting for socket connection', 'uploader.socket.wait', this.shortToken) // TODO: replace the Promise constructor call when dropping support for Node.js <16 with @@ -379,7 +390,7 @@ class Uploader { let timer let onEvent - function cleanup () { + function cleanup() { emitter().removeListener(eventName, onEvent) clearTimeout(timer) } @@ -407,7 +418,7 @@ class Uploader { * @typedef {{action: string, payload: object}} State * @param {State} state */ - saveState (state) { + saveState(state) { if (!this.storage) return // make sure the keys get cleaned up. // https://github.com/transloadit/uppy/issues/3748 @@ -434,7 +445,7 @@ class Uploader { * @param {number} [bytesUploaded] * @param {number | null} [bytesTotalIn] */ - onProgress (bytesUploaded = 0, bytesTotalIn = 0) { + onProgress(bytesUploaded = 0, bytesTotalIn = 0) { const bytesTotal = bytesTotalIn || this.size || 0 // If fully downloading before uploading, combine downloaded and uploaded bytes @@ -470,7 +481,7 @@ class Uploader { * @param {string} url * @param {object} extraData */ - #emitSuccess (url, extraData) { + #emitSuccess(url, extraData) { const emitData = { action: 'success', payload: { ...extraData, complete: true, url }, @@ -483,7 +494,7 @@ class Uploader { * * @param {Error} err */ - #emitError (err) { + #emitError(err) { // delete stack to avoid sending server info to client // todo remove also extraData from serializedErr in next major, // see PR discussion https://github.com/transloadit/uppy/pull/3832 @@ -502,7 +513,7 @@ class Uploader { * * @param {any} stream */ - async #uploadTus (stream) { + async #uploadTus(stream) { const uploader = this const isFileStream = stream instanceof ReadStream @@ -531,7 +542,7 @@ class Uploader { * * @param {Error} error */ - onError (error) { + onError(error) { logger.error(error, 'uploader.tus.error') // deleting tus originalRequest field because it uses the same http-agent // as companion, and this agent may contain sensitive request details (e.g headers) @@ -550,10 +561,10 @@ class Uploader { * @param {number} [bytesUploaded] * @param {number} [bytesTotal] */ - onProgress (bytesUploaded, bytesTotal) { + onProgress(bytesUploaded, bytesTotal) { uploader.onProgress(bytesUploaded, bytesTotal) }, - onSuccess () { + onSuccess() { resolve({ url: uploader.tus.url }) }, }) @@ -564,12 +575,12 @@ class Uploader { }) } - async #uploadMultipart (stream) { + async #uploadMultipart(stream) { if (!this.options.endpoint) { throw new Error('No multipart endpoint set') } - function getRespObj (response) { + function getRespObj(response) { // remove browser forbidden headers const { 'set-cookie': deleted, 'set-cookie2': deleted2, ...responseHeaders } = response.headers @@ -642,7 +653,7 @@ class Uploader { /** * Upload the file to S3 using a Multipart upload. */ - async #uploadS3Multipart (stream) { + async #uploadS3Multipart(stream) { if (!this.options.s3) { throw new Error('The S3 client is not configured on this companion instance.') } diff --git a/packages/@uppy/companion/src/server/helpers/upload.js b/packages/@uppy/companion/src/server/helpers/upload.js index 31554e4169..5f637c43bf 100644 --- a/packages/@uppy/companion/src/server/helpers/upload.js +++ b/packages/@uppy/companion/src/server/helpers/upload.js @@ -2,9 +2,7 @@ const Uploader = require('../Uploader') const logger = require('../logger') const { respondWithError } = require('../provider/error') -const { ValidationError } = Uploader - -async function startDownUpload ({ req, res, getSize, download }) { +async function startDownUpload({ req, res, getSize, download }) { try { const size = await getSize() const { clientSocketConnectTimeout } = req.companion.options @@ -15,22 +13,22 @@ async function startDownUpload ({ req, res, getSize, download }) { logger.debug('Starting download stream.', null, req.id) const stream = await download() - // "Forking" off the upload operation to background, so we can return the http request: - ;(async () => { - // wait till the client has connected to the socket, before starting - // the download, so that the client can receive all download/upload progress. - logger.debug('Waiting for socket connection before beginning remote download/upload.', null, req.id) - await uploader.awaitReady(clientSocketConnectTimeout) - logger.debug('Socket connection received. Starting remote download/upload.', null, req.id) + // "Forking" off the upload operation to background, so we can return the http request: + ; (async () => { + // wait till the client has connected to the socket, before starting + // the download, so that the client can receive all download/upload progress. + logger.debug('Waiting for socket connection before beginning remote download/upload.', null, req.id) + await uploader.awaitReady(clientSocketConnectTimeout) + logger.debug('Socket connection received. Starting remote download/upload.', null, req.id) - await uploader.tryUploadStream(stream) - })().catch((err) => logger.error(err)) + await uploader.tryUploadStream(stream) + })().catch((err) => logger.error(err)) // Respond the request // NOTE: the Uploader will continue running after the http request is responded res.status(200).json({ token: uploader.token }) } catch (err) { - if (err instanceof ValidationError) { + if (err.name === 'ValidationError') { logger.debug(err.message, 'uploader.validator.fail') res.status(400).json({ message: err.message }) return diff --git a/packages/@uppy/companion/src/server/helpers/utils.js b/packages/@uppy/companion/src/server/helpers/utils.js index fadae31b54..627d87fcb1 100644 --- a/packages/@uppy/companion/src/server/helpers/utils.js +++ b/packages/@uppy/companion/src/server/helpers/utils.js @@ -74,7 +74,7 @@ module.exports.getURLBuilder = (options) => { * * @param {string|Buffer} secret */ -function createSecret (secret) { +function createSecret(secret) { const hash = crypto.createHash('sha256') hash.update(secret) return hash.digest() @@ -85,15 +85,15 @@ function createSecret (secret) { * * @returns {Buffer} */ -function createIv () { +function createIv() { return crypto.randomBytes(16) } -function urlEncode (unencoded) { +function urlEncode(unencoded) { return unencoded.replace(/\+/g, '-').replace(/\//g, '_').replace(/=/g, '~') } -function urlDecode (encoded) { +function urlDecode(encoded) { return encoded.replace(/-/g, '+').replace(/_/g, '/').replace(/~/g, '=') } @@ -157,6 +157,7 @@ class StreamHttpJsonError extends Error { super(`Request failed with status ${statusCode}`) this.statusCode = statusCode this.responseJson = responseJson + this.name = 'StreamHttpJsonError' } } @@ -188,7 +189,7 @@ module.exports.prepareStream = async (stream) => new Promise((resolve, reject) = reject(err) }) - }) +}) module.exports.getBasicAuthHeader = (key, secret) => { const base64 = Buffer.from(`${key}:${secret}`, 'binary').toString('base64') diff --git a/packages/@uppy/companion/src/server/logger.js b/packages/@uppy/companion/src/server/logger.js index 77c1d53e53..f2788ff18f 100644 --- a/packages/@uppy/companion/src/server/logger.js +++ b/packages/@uppy/companion/src/server/logger.js @@ -1,7 +1,6 @@ const chalk = require('chalk') const escapeStringRegexp = require('escape-string-regexp') const util = require('node:util') -const { ProviderApiError } = require('./provider/error') const valuesToMask = [] /** @@ -24,7 +23,7 @@ exports.setMaskables = (maskables) => { * @param {string} msg the message whose content should be masked * @returns {string} */ -function maskMessage (msg) { +function maskMessage(msg) { let out = msg for (const toBeMasked of valuesToMask) { const toBeReplaced = new RegExp(toBeMasked, 'gi') @@ -53,10 +52,11 @@ const log = ({ arg, tag = '', level, traceId = '', color = (message) => message const time = new Date().toISOString() const whitespace = tag && traceId ? ' ' : '' - function msgToString () { + function msgToString() { // We don't need to log stack trace on special errors that we ourselves have produced // (to reduce log noise) - if ((arg instanceof ProviderApiError) && typeof arg.message === 'string') { + // @ts-ignore + if ((arg instanceof Error && arg.name === 'ProviderApiError') && typeof arg.message === 'string') { return arg.message } if (typeof arg === 'string') return arg diff --git a/packages/@uppy/companion/src/server/provider/error.js b/packages/@uppy/companion/src/server/provider/error.js index 4f05adb2fc..758cab4188 100644 --- a/packages/@uppy/companion/src/server/provider/error.js +++ b/packages/@uppy/companion/src/server/provider/error.js @@ -8,7 +8,7 @@ class ProviderApiError extends Error { * @param {string} message error message * @param {number} statusCode the http status code from the provider api */ - constructor (message, statusCode) { + constructor(message, statusCode) { super(`HTTP ${statusCode}: ${message}`) // Include statusCode to make it easier to debug this.name = 'ProviderApiError' this.statusCode = statusCode @@ -20,8 +20,9 @@ class ProviderUserError extends ProviderApiError { /** * @param {object} json arbitrary JSON.stringify-able object that will be passed to the client */ - constructor (json) { + constructor(json) { super('User error', undefined) + this.name = 'ProviderUserError' this.json = json } } @@ -31,7 +32,7 @@ class ProviderUserError extends ProviderApiError { * an authorization error while communication with its corresponding provider */ class ProviderAuthError extends ProviderApiError { - constructor () { + constructor() { super('invalid access token detected by Provider', 401) this.name = 'AuthError' this.isAuthError = true @@ -43,25 +44,30 @@ class ProviderAuthError extends ProviderApiError { * * @param {Error | ProviderApiError} err the error instance to convert to an http json response */ -function errorToResponse (err) { - if (err instanceof ProviderAuthError) { +function errorToResponse(err) { + // @ts-ignore + if (err.isAuthError) { return { code: 401, json: { message: err.message } } } - if (err instanceof ProviderUserError) { + if (err.name === 'ProviderUserError') { + // @ts-ignore return { code: 400, json: err.json } } - if (err instanceof ProviderApiError) { + if (err.name === 'ProviderApiError') { + // @ts-ignore if (err.statusCode >= 500) { // bad gateway i.e the provider APIs gateway return { code: 502, json: { message: err.message } } } + // @ts-ignore if (err.statusCode === 429) { return { code: 429, message: err.message } } + // @ts-ignore if (err.statusCode >= 400) { // 424 Failed Dependency return { code: 424, json: { message: err.message } } @@ -71,7 +77,7 @@ function errorToResponse (err) { return undefined } -function respondWithError (err, res) { +function respondWithError(err, res) { const errResp = errorToResponse(err) if (errResp) { res.status(errResp.code).json(errResp.json) diff --git a/packages/@uppy/companion/src/server/provider/providerErrors.js b/packages/@uppy/companion/src/server/provider/providerErrors.js index 5c9860fbf3..5c77939f4f 100644 --- a/packages/@uppy/companion/src/server/provider/providerErrors.js +++ b/packages/@uppy/companion/src/server/provider/providerErrors.js @@ -1,8 +1,5 @@ -const { HTTPError } = require('got').default - const logger = require('../logger') const { ProviderApiError, ProviderUserError, ProviderAuthError } = require('./error') -const { StreamHttpJsonError } = require('../helpers/utils') /** * @@ -16,7 +13,7 @@ const { StreamHttpJsonError } = require('../helpers/utils') * }} param0 * @returns */ -async function withProviderErrorHandling ({ +async function withProviderErrorHandling({ fn, tag, providerName, @@ -24,7 +21,7 @@ async function withProviderErrorHandling ({ isUserFacingError = () => false, getJsonErrorMessage, }) { - function getErrorMessage ({ statusCode, body }) { + function getErrorMessage({ statusCode, body }) { if (typeof body === 'object') { const message = getJsonErrorMessage(body) if (message != null) return message @@ -43,11 +40,11 @@ async function withProviderErrorHandling ({ let statusCode let body - if (err instanceof HTTPError) { + if (err.name === 'HTTPError') { statusCode = err.response?.statusCode body = err.response?.body - } else if (err instanceof StreamHttpJsonError) { - statusCode = err.statusCode + } else if (err.name === 'StreamHttpJsonError') { + statusCode = err.statusCode body = err.responseJson } @@ -56,7 +53,7 @@ async function withProviderErrorHandling ({ if (isAuthError({ statusCode, body })) { knownErr = new ProviderAuthError() } else if (isUserFacingError({ statusCode, body })) { - knownErr = new ProviderUserError({ message: getErrorMessage({ statusCode, body }) }) + knownErr = new ProviderUserError({ message: getErrorMessage({ statusCode, body }) }) } else { knownErr = new ProviderApiError(getErrorMessage({ statusCode, body }), statusCode) } diff --git a/packages/@uppy/companion/src/standalone/index.js b/packages/@uppy/companion/src/standalone/index.js index d4c926929a..04708a5493 100644 --- a/packages/@uppy/companion/src/standalone/index.js +++ b/packages/@uppy/companion/src/standalone/index.js @@ -17,7 +17,7 @@ const { getCompanionOptions, generateSecret, buildHelpfulStartupMessage } = requ * * @returns {object} */ -module.exports = function server (inputCompanionOptions) { +module.exports = function server(inputCompanionOptions) { const companionOptions = getCompanionOptions(inputCompanionOptions) companion.setLoggerProcessName(companionOptions) @@ -52,7 +52,7 @@ module.exports = function server (inputCompanionOptions) { * censored: boolean * }} */ - function censorQuery (rawQuery) { + function censorQuery(rawQuery) { /** @type {Record} */ const query = {} let censored = false @@ -172,7 +172,7 @@ module.exports = function server (inputCompanionOptions) { if (app.get('env') === 'production') { // if the error is a URIError from the requested URL we only log the error message // to avoid uneccessary error alerts - if (err.status === 400 && err instanceof URIError) { + if (err.status === 400 && err.name === 'URIError') { logger.error(err.message, 'root.error', req.id) } else { logger.error(err, 'root.error', req.id) diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index 665422eac1..c5ba761934 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -2,7 +2,6 @@ import { h } from 'preact' import PQueue from 'p-queue' import { getSafeFileId } from '@uppy/utils/lib/generateFileID' -import UserFacingApiError from '@uppy/utils/lib/UserFacingApiError' import AuthView from './AuthView.jsx' import Header from './Header.jsx' @@ -188,7 +187,7 @@ export default class ProviderView extends View { } catch (err) { // This is the first call that happens when the provider view loads, after auth, so it's probably nice to show any // error occurring here to the user. - if (err instanceof UserFacingApiError) { + if (err.name === 'UserFacingApiError') { this.plugin.uppy.info({ message: this.plugin.uppy.i18n(err.message) }, 'warning', 5000) return } @@ -259,7 +258,7 @@ export default class ProviderView extends View { this.preFirstRender() }) } catch (err) { - if (err instanceof UserFacingApiError) { + if (err.name === 'UserFacingApiError') { this.plugin.uppy.info({ message: this.plugin.uppy.i18n(err.message) }, 'warning', 5000) return } From 761dcdc4a6b96d75841fa4e38b725b62b1cdb3a9 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 5 Dec 2023 19:00:46 +0800 Subject: [PATCH 32/37] Apply suggestions from code review Co-authored-by: Antoine du Hamel --- packages/@uppy/companion/src/server/Uploader.js | 7 ++----- packages/@uppy/companion/src/server/provider/error.js | 6 +++--- .../@uppy/companion/src/server/provider/providerErrors.js | 4 ++-- .../@uppy/provider-views/src/ProviderView/ProviderView.jsx | 2 +- packages/@uppy/utils/src/UserFacingApiError.js | 5 +---- 5 files changed, 9 insertions(+), 15 deletions(-) diff --git a/packages/@uppy/companion/src/server/Uploader.js b/packages/@uppy/companion/src/server/Uploader.js index 663b0b4cc5..dd10fb9718 100644 --- a/packages/@uppy/companion/src/server/Uploader.js +++ b/packages/@uppy/companion/src/server/Uploader.js @@ -57,10 +57,7 @@ function sanitizeMetadata(inputMetadata) { } class AbortError extends Error { - constructor(message) { - super(message) - this.isAbortError = true - } + isAbortError = true } class ValidationError extends Error { @@ -317,7 +314,7 @@ class Uploader { const { url, extraData } = ret this.#emitSuccess(url, extraData) } catch (err) { - if (err.isAbortError) { + if (err?.isAbortError) { logger.error('Aborted upload', 'uploader.aborted', this.shortToken) return } diff --git a/packages/@uppy/companion/src/server/provider/error.js b/packages/@uppy/companion/src/server/provider/error.js index 758cab4188..efb4a53381 100644 --- a/packages/@uppy/companion/src/server/provider/error.js +++ b/packages/@uppy/companion/src/server/provider/error.js @@ -46,16 +46,16 @@ class ProviderAuthError extends ProviderApiError { */ function errorToResponse(err) { // @ts-ignore - if (err.isAuthError) { + if (err?.isAuthError) { return { code: 401, json: { message: err.message } } } - if (err.name === 'ProviderUserError') { + if (err?.name === 'ProviderUserError') { // @ts-ignore return { code: 400, json: err.json } } - if (err.name === 'ProviderApiError') { + if (err?.name === 'ProviderApiError') { // @ts-ignore if (err.statusCode >= 500) { // bad gateway i.e the provider APIs gateway diff --git a/packages/@uppy/companion/src/server/provider/providerErrors.js b/packages/@uppy/companion/src/server/provider/providerErrors.js index 5c77939f4f..715de19592 100644 --- a/packages/@uppy/companion/src/server/provider/providerErrors.js +++ b/packages/@uppy/companion/src/server/provider/providerErrors.js @@ -40,10 +40,10 @@ async function withProviderErrorHandling({ let statusCode let body - if (err.name === 'HTTPError') { + if (err?.name === 'HTTPError') { statusCode = err.response?.statusCode body = err.response?.body - } else if (err.name === 'StreamHttpJsonError') { + } else if (err?.name === 'StreamHttpJsonError') { statusCode = err.statusCode body = err.responseJson } diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx index c5ba761934..68edf1bb88 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx @@ -187,7 +187,7 @@ export default class ProviderView extends View { } catch (err) { // This is the first call that happens when the provider view loads, after auth, so it's probably nice to show any // error occurring here to the user. - if (err.name === 'UserFacingApiError') { + if (err?.name === 'UserFacingApiError') { this.plugin.uppy.info({ message: this.plugin.uppy.i18n(err.message) }, 'warning', 5000) return } diff --git a/packages/@uppy/utils/src/UserFacingApiError.js b/packages/@uppy/utils/src/UserFacingApiError.js index 68330275c7..1bfe9da950 100644 --- a/packages/@uppy/utils/src/UserFacingApiError.js +++ b/packages/@uppy/utils/src/UserFacingApiError.js @@ -1,8 +1,5 @@ class UserFacingApiError extends Error { - constructor (message) { - super(message) - this.name = 'UserFacingApiError' - } + name = 'UserFacingApiError' } export default UserFacingApiError From 64ce4713b258baa368463b8b05e3432d24713909 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 5 Dec 2023 20:53:46 +0800 Subject: [PATCH 33/37] fix broken lint fix script --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index d4b99be78e..a72531a80a 100644 --- a/package.json +++ b/package.json @@ -129,7 +129,7 @@ "contributors:save": "yarn node ./bin/update-contributors.mjs", "dev:with-companion": "npm-run-all --parallel start:companion dev", "dev": "yarn workspace @uppy-dev/dev dev", - "lint:fix": "yarn run lint -- --fix", + "lint:fix": "yarn lint --fix", "lint:markdown": "remark -f -q -i .remarkignore . .github/CONTRIBUTING.md", "lint:staged": "lint-staged", "lint:css": "stylelint ./packages/**/*.scss", From 654da1a5ff13f547bca3758c8c271e83f362f0ff Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 5 Dec 2023 20:54:31 +0800 Subject: [PATCH 34/37] fix broken merge code --- packages/@uppy/companion/test/mockoauthstate.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@uppy/companion/test/mockoauthstate.js b/packages/@uppy/companion/test/mockoauthstate.js index e30f8d5956..8a7db59057 100644 --- a/packages/@uppy/companion/test/mockoauthstate.js +++ b/packages/@uppy/companion/test/mockoauthstate.js @@ -1,8 +1,8 @@ module.exports = () => { return { - getFromState: (state) => { generateState: () => 'some-cool-nice-encrytpion', addToState: () => 'some-cool-nice-encrytpion', + getFromState: (state) => { if (state === 'state-with-invalid-instance-url') { return 'http://localhost:3452' } From a69a1fe8bd58808dc6dfa6fa9ce9c3786a8a1369 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 5 Dec 2023 23:01:00 +0800 Subject: [PATCH 35/37] try to fix flakey tets --- e2e/cypress/integration/dashboard-tus.spec.ts | 3 --- e2e/cypress/integration/dashboard-xhr.spec.ts | 14 +++++------ e2e/cypress/integration/reusable-tests.ts | 25 ++++++++++++++----- 3 files changed, 26 insertions(+), 16 deletions(-) diff --git a/e2e/cypress/integration/dashboard-tus.spec.ts b/e2e/cypress/integration/dashboard-tus.spec.ts index 053089c7df..d6df9a6f79 100644 --- a/e2e/cypress/integration/dashboard-tus.spec.ts +++ b/e2e/cypress/integration/dashboard-tus.spec.ts @@ -1,6 +1,5 @@ import { interceptCompanionUrlRequest, - interceptCompanionUnsplashRequest, runRemoteUrlImageUploadTest, runRemoteUnsplashUploadTest, } from './reusable-tests' @@ -15,8 +14,6 @@ describe('Dashboard with Tus', () => { cy.intercept('/files/*').as('tus') cy.intercept({ method: 'POST', pathname: '/files' }).as('post') cy.intercept({ method: 'PATCH', pathname: '/files/*' }).as('patch') - interceptCompanionUrlRequest() - interceptCompanionUnsplashRequest() }) it('should upload cat image successfully', () => { diff --git a/e2e/cypress/integration/dashboard-xhr.spec.ts b/e2e/cypress/integration/dashboard-xhr.spec.ts index e21ee08caa..f1e6fffd98 100644 --- a/e2e/cypress/integration/dashboard-xhr.spec.ts +++ b/e2e/cypress/integration/dashboard-xhr.spec.ts @@ -1,6 +1,5 @@ import { - interceptCompanionUrlRequest, - interceptCompanionUnsplashRequest, + interceptCompanionUrlMetaRequest, runRemoteUrlImageUploadTest, runRemoteUnsplashUploadTest, } from './reusable-tests' @@ -8,8 +7,6 @@ import { describe('Dashboard with XHR', () => { beforeEach(() => { cy.visit('/dashboard-xhr') - interceptCompanionUrlRequest() - interceptCompanionUnsplashRequest() }) it('should upload remote image with URL plugin', () => { @@ -22,8 +19,9 @@ describe('Dashboard with XHR', () => { cy.get('.uppy-Url-input').type( 'http://localhost:4678/file-with-content-disposition', ) + interceptCompanionUrlMetaRequest() cy.get('.uppy-Url-importButton').click() - cy.wait('@url').then(() => { + cy.wait('@url-meta').then(() => { cy.get('.uppy-Dashboard-Item-name').should('contain', fileName) cy.get('.uppy-Dashboard-Item-status').should('contain', '84 KB') }) @@ -32,8 +30,9 @@ describe('Dashboard with XHR', () => { it('should return correct file name with URL plugin from remote image without Content-Disposition', () => { cy.get('[data-cy="Url"]').click() cy.get('.uppy-Url-input').type('http://localhost:4678/file-no-headers') + interceptCompanionUrlMetaRequest() cy.get('.uppy-Url-importButton').click() - cy.wait('@url').then(() => { + cy.wait('@url-meta').then(() => { cy.get('.uppy-Dashboard-Item-name').should('contain', 'file-no') cy.get('.uppy-Dashboard-Item-status').should('contain', '0') }) @@ -50,8 +49,9 @@ describe('Dashboard with XHR', () => { cy.get('.uppy-Url-input').type( 'http://localhost:4678/file-with-content-disposition', ) + interceptCompanionUrlMetaRequest() cy.get('.uppy-Url-importButton').click() - cy.wait('@url').then(() => { + cy.wait('@url-meta').then(() => { cy.get('.uppy-Dashboard-Item-name').should('contain', 'file-with') cy.get('.uppy-Dashboard-Item-status').should('contain', '123 B') }) diff --git a/e2e/cypress/integration/reusable-tests.ts b/e2e/cypress/integration/reusable-tests.ts index f60d29de53..d311b60e39 100644 --- a/e2e/cypress/integration/reusable-tests.ts +++ b/e2e/cypress/integration/reusable-tests.ts @@ -1,9 +1,13 @@ /* global cy */ -export const interceptCompanionUrlRequest = () => - cy.intercept('http://localhost:3020/url/*').as('url') -export const interceptCompanionUnsplashRequest = () => - cy.intercept('http://localhost:3020/search/unsplash/*').as('unsplash') +const interceptCompanionUrlRequest = () => + cy + .intercept({ method: 'POST', url: 'http://localhost:3020/url/get' }) + .as('url') +export const interceptCompanionUrlMetaRequest = () => + cy + .intercept({ method: 'POST', url: 'http://localhost:3020/url/meta' }) + .as('url-meta') export function runRemoteUrlImageUploadTest() { cy.get('[data-cy="Url"]').click() @@ -11,6 +15,7 @@ export function runRemoteUrlImageUploadTest() { 'https://raw.githubusercontent.com/transloadit/uppy/main/e2e/cypress/fixtures/images/cat.jpg', ) cy.get('.uppy-Url-importButton').click() + interceptCompanionUrlRequest() cy.get('.uppy-StatusBar-actionBtn--upload').click() cy.wait('@url').then(() => { cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Complete') @@ -20,8 +25,12 @@ export function runRemoteUrlImageUploadTest() { export function runRemoteUnsplashUploadTest() { cy.get('[data-cy="Unsplash"]').click() cy.get('.uppy-SearchProvider-input').type('book') + cy.intercept({ + method: 'GET', + url: 'http://localhost:3020/search/unsplash/list?q=book', + }).as('unsplash-list') cy.get('.uppy-SearchProvider-searchButton').click() - cy.wait('@unsplash') + cy.wait('@unsplash-list') // Test that the author link is visible cy.get('.uppy-ProviderBrowserItem') .first() @@ -34,8 +43,12 @@ export function runRemoteUnsplashUploadTest() { cy.get('a').should('have.css', 'display', 'block') }) cy.get('.uppy-c-btn-primary').click() + cy.intercept({ + method: 'POST', + url: 'http://localhost:3020/search/unsplash/get/*', + }).as('unsplash-get') cy.get('.uppy-StatusBar-actionBtn--upload').click() - cy.wait('@unsplash').then(() => { + cy.wait('@unsplash-get').then(() => { cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Complete') }) } From 6d766cb4b65aa78e1dd0806969fe1d4964b75f75 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 5 Dec 2023 23:07:49 +0800 Subject: [PATCH 36/37] fix lint --- e2e/cypress/integration/dashboard-tus.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/e2e/cypress/integration/dashboard-tus.spec.ts b/e2e/cypress/integration/dashboard-tus.spec.ts index d6df9a6f79..8432b969c7 100644 --- a/e2e/cypress/integration/dashboard-tus.spec.ts +++ b/e2e/cypress/integration/dashboard-tus.spec.ts @@ -1,5 +1,4 @@ import { - interceptCompanionUrlRequest, runRemoteUrlImageUploadTest, runRemoteUnsplashUploadTest, } from './reusable-tests' From 7a920f97477a8d62c35d94aaae0879772890b0f5 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 5 Dec 2023 23:24:54 +0800 Subject: [PATCH 37/37] fix merge issue --- packages/@uppy/companion/src/companion.js | 19 ++++++++++++++----- .../companion/src/server/provider/index.js | 16 +++------------- .../test/__tests__/provider-manager.js | 12 +++++++----- 3 files changed, 24 insertions(+), 23 deletions(-) diff --git a/packages/@uppy/companion/src/companion.js b/packages/@uppy/companion/src/companion.js index b96fe1ddd6..683eeb3d1d 100644 --- a/packages/@uppy/companion/src/companion.js +++ b/packages/@uppy/companion/src/companion.js @@ -20,8 +20,10 @@ const { ProviderApiError, ProviderUserError, ProviderAuthError } = require('./se const { getCredentialsOverrideMiddleware } = require('./server/provider/credentials') // @ts-ignore const { version } = require('../package.json') +const { isOAuthProvider } = require('./server/provider/Provider') -function setLoggerProcessName ({ loggerProcessName }) { + +function setLoggerProcessName({ loggerProcessName }) { if (loggerProcessName != null) logger.setProcessName(loggerProcessName) } @@ -77,7 +79,9 @@ module.exports.app = (optionsArg = {}) => { providerManager.addCustomProviders(customProviders, providers, grantConfig) } - providerManager.addProviderOptions(options, grantConfig, providers) + const getAuthProvider = (providerName) => providers[providerName]?.authProvider + + providerManager.addProviderOptions(options, grantConfig, getAuthProvider) // mask provider secrets from log messages logger.setMaskables(getMaskableSecrets(options)) @@ -147,13 +151,18 @@ module.exports.app = (optionsArg = {}) => { // for simplicity, we just return the normal credentials for the provider, but in a real-world scenario, // we would query based on parameters const { key, secret } = options.providerOptions[providerName] + + function getRedirectUri() { + const authProvider = getAuthProvider(providerName) + if (!isOAuthProvider(authProvider)) return undefined + return grantConfig[authProvider]?.redirect_uri + } + res.send({ credentials: { key, secret, - redirect_uri: providerManager.getGrantConfigForProvider({ - providerName, companionOptions: options, grantConfig, - })?.redirect_uri, + redirect_uri: getRedirectUri(), }, }) }) diff --git a/packages/@uppy/companion/src/server/provider/index.js b/packages/@uppy/companion/src/server/provider/index.js index d800d75b3c..fb7d4b1b59 100644 --- a/packages/@uppy/companion/src/server/provider/index.js +++ b/packages/@uppy/companion/src/server/provider/index.js @@ -12,7 +12,6 @@ const zoom = require('./zoom') const { getURLBuilder } = require('../helpers/utils') const logger = require('../logger') const { getCredentialsResolver } = require('./credentials') -// eslint-disable-next-line const Provider = require('./Provider') const { isOAuthProvider } = Provider @@ -25,15 +24,6 @@ const validOptions = (options) => { return options.server.host && options.server.protocol } -function getGrantConfigForProvider({ providerName, companionOptions, grantConfig }) { - const authProvider = providerNameToAuthName(providerName, companionOptions) - - if (!isOAuthProvider(authProvider)) return undefined - return grantConfig[authProvider] -} - -module.exports.getGrantConfigForProvider = getGrantConfigForProvider - /** * adds the desired provider module to the request object, * based on the providerName parameter specified @@ -115,9 +105,9 @@ module.exports.addCustomProviders = (customProviders, providers, grantConfig) => * * @param {{server: object, providerOptions: object}} companionOptions * @param {object} grantConfig - * @param {Record} providers + * @param {(a: string) => string} getAuthProvider */ -module.exports.addProviderOptions = (companionOptions, grantConfig, providers) => { +module.exports.addProviderOptions = (companionOptions, grantConfig, getAuthProvider) => { const { server, providerOptions } = companionOptions if (!validOptions({ server })) { logger.warn('invalid provider options detected. Providers will not be loaded', 'provider.options.invalid') @@ -134,7 +124,7 @@ module.exports.addProviderOptions = (companionOptions, grantConfig, providers) = const { oauthDomain } = server const keys = Object.keys(providerOptions).filter((key) => key !== 'server') keys.forEach((providerName) => { - const authProvider = providers[providerName]?.authProvider + const authProvider = getAuthProvider?.(providerName) if (isOAuthProvider(authProvider) && grantConfig[authProvider]) { // explicitly add providerOptions so users don't override other providerOptions. diff --git a/packages/@uppy/companion/test/__tests__/provider-manager.js b/packages/@uppy/companion/test/__tests__/provider-manager.js index 75d1139983..084e2ec9f1 100644 --- a/packages/@uppy/companion/test/__tests__/provider-manager.js +++ b/packages/@uppy/companion/test/__tests__/provider-manager.js @@ -5,6 +5,8 @@ const { setDefaultEnv } = require('../mockserver') let grantConfig let companionOptions +const getAuthProvider = (providerName) => providerManager.getDefaultProviders()[providerName]?.authProvider + describe('Test Provider options', () => { beforeEach(() => { setDefaultEnv() @@ -14,7 +16,7 @@ describe('Test Provider options', () => { }) test('adds provider options', () => { - providerManager.addProviderOptions(companionOptions, grantConfig, providerManager.getDefaultProviders()) + providerManager.addProviderOptions(companionOptions, grantConfig, getAuthProvider) expect(grantConfig.dropbox.key).toBe('dropbox_key') expect(grantConfig.dropbox.secret).toBe('dropbox_secret') @@ -33,7 +35,7 @@ describe('Test Provider options', () => { test('adds extra provider config', () => { process.env.COMPANION_INSTAGRAM_KEY = '123456' - providerManager.addProviderOptions(getCompanionOptions(), grantConfig, providerManager.getDefaultProviders()) + providerManager.addProviderOptions(getCompanionOptions(), grantConfig, getAuthProvider) expect(grantConfig.instagram).toEqual({ transport: 'session', callback: '/instagram/callback', @@ -102,7 +104,7 @@ describe('Test Provider options', () => { companionOptions = getCompanionOptions() - providerManager.addProviderOptions(companionOptions, grantConfig, providerManager.getDefaultProviders()) + providerManager.addProviderOptions(companionOptions, grantConfig, getAuthProvider) expect(grantConfig.dropbox.secret).toBe('xobpord') expect(grantConfig.box.secret).toBe('xwbepqd') @@ -116,7 +118,7 @@ describe('Test Provider options', () => { delete companionOptions.server.host delete companionOptions.server.protocol - providerManager.addProviderOptions(companionOptions, grantConfig, providerManager.getDefaultProviders()) + providerManager.addProviderOptions(companionOptions, grantConfig, getAuthProvider) expect(grantConfig.dropbox.key).toBeUndefined() expect(grantConfig.dropbox.secret).toBeUndefined() @@ -135,7 +137,7 @@ describe('Test Provider options', () => { test('sets a main redirect uri, if oauthDomain is set', () => { companionOptions.server.oauthDomain = 'domain.com' - providerManager.addProviderOptions(companionOptions, grantConfig, providerManager.getDefaultProviders()) + providerManager.addProviderOptions(companionOptions, grantConfig, getAuthProvider) expect(grantConfig.dropbox.redirect_uri).toBe('http://domain.com/dropbox/redirect') expect(grantConfig.box.redirect_uri).toBe('http://domain.com/box/redirect')