From 2c75ddacfe33d847cfe18db01509cff77385ea7f Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 22 Jun 2023 11:18:48 +0200 Subject: [PATCH] @uppy/companion: implement refresh for authentication tokens (#4448) * allow storing multiple tokens inside uppy auth token * de-duplicate uploadRemote by creating a new superclass UploaderPlugin * pull out requestSocketToken from MiniXHRUpload * add class UploaderPlugin * refactor * refactor * refactor/reuse * refactor/make getAuthToken private * fix bug * implement refresh token for dropbox and google drive closes #2721 * fix test * also update auth token cookie when refreshing token * fix build error on node 14 * increase auth token expiry to workaround expiry * Update packages/@uppy/companion-client/src/RequestClient.js Co-authored-by: Antoine du Hamel * make queueRequestSocketToken private * rename arg * fix lint * log error message * fix s3 * Update packages/@uppy/companion-client/src/Provider.js Co-authored-by: Antoine du Hamel --------- Co-authored-by: Antoine du Hamel --- packages/@uppy/aws-s3-multipart/src/index.js | 38 +------- packages/@uppy/aws-s3/src/MiniXHRUpload.js | 92 +------------------ packages/@uppy/aws-s3/src/index.js | 68 +++++++++++++- .../@uppy/companion-client/src/Provider.js | 56 +++++++++-- .../companion-client/src/RequestClient.js | 9 +- packages/@uppy/companion/src/companion.js | 1 + packages/@uppy/companion/src/config/grant.js | 2 + .../src/server/controllers/callback.js | 34 ++++--- .../src/server/controllers/deauth-callback.js | 8 +- .../companion/src/server/controllers/get.js | 6 +- .../companion/src/server/controllers/index.js | 1 + .../companion/src/server/controllers/list.js | 12 +-- .../src/server/controllers/logout.js | 17 ++-- .../src/server/controllers/refresh-token.js | 51 ++++++++++ .../src/server/controllers/send-token.js | 5 - .../src/server/controllers/thumbnail.js | 4 +- .../@uppy/companion/src/server/helpers/jwt.js | 82 +++++++++++------ .../companion/src/server/helpers/upload.js | 8 +- .../@uppy/companion/src/server/middlewares.js | 27 +++--- .../companion/src/server/provider/Provider.js | 9 ++ .../src/server/provider/credentials.js | 16 ++-- .../src/server/provider/drive/index.js | 11 +++ .../src/server/provider/dropbox/index.js | 12 ++- .../companion/src/server/provider/error.d.ts | 2 + .../companion/src/server/provider/error.js | 11 ++- .../src/server/provider/providerErrors.js | 29 +++--- .../companion/test/__tests__/callback.js | 25 +++-- .../companion/test/__tests__/companion.js | 8 +- .../companion/test/__tests__/credentials.js | 4 +- .../@uppy/companion/test/__tests__/preauth.js | 12 +-- .../test/__tests__/provider-manager.js | 6 ++ .../companion/test/__tests__/providers.js | 4 +- packages/@uppy/companion/test/mockserver.js | 4 +- packages/@uppy/core/src/UploaderPlugin.js | 34 +++++++ packages/@uppy/core/src/Uppy.js | 4 +- packages/@uppy/tus/src/index.js | 41 +-------- packages/@uppy/xhr-upload/src/index.js | 35 +------ 37 files changed, 440 insertions(+), 348 deletions(-) create mode 100644 packages/@uppy/companion/src/server/controllers/refresh-token.js create mode 100644 packages/@uppy/core/src/UploaderPlugin.js diff --git a/packages/@uppy/aws-s3-multipart/src/index.js b/packages/@uppy/aws-s3-multipart/src/index.js index e9c09ff53..e1f84615a 100644 --- a/packages/@uppy/aws-s3-multipart/src/index.js +++ b/packages/@uppy/aws-s3-multipart/src/index.js @@ -1,4 +1,4 @@ -import BasePlugin from '@uppy/core/lib/BasePlugin.js' +import UploaderPlugin from '@uppy/core/lib/UploaderPlugin.js' import { Socket, Provider, RequestClient } from '@uppy/companion-client' import EventManager from '@uppy/utils/lib/EventManager' import emitSocketProgress from '@uppy/utils/lib/emitSocketProgress' @@ -303,11 +303,9 @@ class HTTPCommunicationQueue { } } -export default class AwsS3Multipart extends BasePlugin { +export default class AwsS3Multipart extends UploaderPlugin { static VERSION = packageJson.version - #queueRequestSocketToken - #companionCommunicationQueue #client @@ -359,7 +357,7 @@ export default class AwsS3Multipart extends BasePlugin { this.uploaderEvents = Object.create(null) this.uploaderSockets = Object.create(null) - this.#queueRequestSocketToken = this.requests.wrapPromiseFunction(this.#requestSocketToken, { priority: -1 }) + this.setQueueRequestSocketToken(this.requests.wrapPromiseFunction(this.#requestSocketToken, { priority: -1 })) } [Symbol.for('uppy test: getClient')] () { return this.#client } @@ -683,32 +681,6 @@ export default class AwsS3Multipart extends BasePlugin { return res.token } - // NOTE! Keep this duplicated code in sync with other plugins - // TODO we should probably abstract this into a common function - async #uploadRemote (file, options) { - this.resetUploaderReferences(file.id) - - try { - if (file.serverToken) { - return await this.connectToServerSocket(file) - } - const serverToken = await this.#queueRequestSocketToken(file, options).abortOn(options?.signal) - - if (!this.uppy.getState().files[file.id]) return undefined - - this.uppy.setFileState(file.id, { serverToken }) - return await this.connectToServerSocket(this.uppy.getFile(file.id)) - } catch (err) { - if (err?.cause?.name !== 'AbortError') { - this.uppy.setFileState(file.id, { serverToken: undefined }) - this.uppy.emit('upload-error', file, err) - throw err - } - // The file upload was aborted, it’s not an error - return undefined - } - } - async connectToServerSocket (file) { return new Promise((resolve, reject) => { let queuedRequest @@ -841,10 +813,10 @@ export default class AwsS3Multipart extends BasePlugin { const removedHandler = (removedFile) => { if (removedFile.id === file.id) controller.abort() } - this.uppy.on('file-removed', removedHandler) - const uploadPromise = this.#uploadRemote(file, { signal: controller.signal }) + this.resetUploaderReferences(file.id) + const uploadPromise = this.uploadRemoteFile(file, { signal: controller.signal }) this.requests.wrapSyncFunction(() => { this.uppy.off('file-removed', removedHandler) diff --git a/packages/@uppy/aws-s3/src/MiniXHRUpload.js b/packages/@uppy/aws-s3/src/MiniXHRUpload.js index 42533d2d6..d6ff26556 100644 --- a/packages/@uppy/aws-s3/src/MiniXHRUpload.js +++ b/packages/@uppy/aws-s3/src/MiniXHRUpload.js @@ -1,5 +1,5 @@ import { nanoid } from 'nanoid/non-secure' -import { Provider, RequestClient, Socket } from '@uppy/companion-client' +import { Socket } from '@uppy/companion-client' import emitSocketProgress from '@uppy/utils/lib/emitSocketProgress' import getSocketHost from '@uppy/utils/lib/getSocketHost' import EventManager from '@uppy/utils/lib/EventManager' @@ -53,8 +53,6 @@ function createFormDataUpload (file, opts) { const createBareUpload = file => file.data export default class MiniXHRUpload { - #queueRequestSocketToken - constructor (uppy, opts) { this.uppy = uppy this.opts = { @@ -67,11 +65,9 @@ export default class MiniXHRUpload { this.requests = opts[internalRateLimitedQueue] this.uploaderEvents = Object.create(null) this.i18n = opts.i18n - - this.#queueRequestSocketToken = this.requests.wrapPromiseFunction(this.#requestSocketToken, { priority: -1 }) } - #getOptions (file) { + getOptions (file) { const { uppy } = this const overrides = uppy.getState().xhrUpload @@ -89,31 +85,6 @@ export default class MiniXHRUpload { return opts } - uploadFile (id, current, total) { - const file = this.uppy.getFile(id) - this.uppy.log(`uploading ${current} of ${total}`) - - if (file.error) { - throw new Error(file.error) - } else if (file.isRemote) { - const controller = new AbortController() - - const removedHandler = (removedFile) => { - if (removedFile.id === file.id) controller.abort() - } - this.uppy.on('file-removed', removedHandler) - - const uploadPromise = this.#uploadRemoteFile(file, { signal: controller.signal }) - - this.requests.wrapSyncFunction(() => { - this.uppy.off('file-removed', removedHandler) - }, { priority: -1 })() - - return uploadPromise - } - return this.#uploadLocalFile(file, current, total) - } - #addEventHandlerForFile (eventName, fileID, eventHandler) { this.uploaderEvents[fileID].on(eventName, (fileOrID) => { // TODO (major): refactor Uppy events to consistently send file objects (or consistently IDs) @@ -130,8 +101,8 @@ export default class MiniXHRUpload { }) } - #uploadLocalFile (file) { - const opts = this.#getOptions(file) + uploadLocalFile (file) { + const opts = this.getOptions(file) return new Promise((resolve, reject) => { // This is done in index.js in the S3 plugin. @@ -265,62 +236,9 @@ export default class MiniXHRUpload { }) } - #requestSocketToken = async (file) => { - const opts = this.#getOptions(file) - const Client = file.remote.providerOptions.provider ? Provider : RequestClient - const client = new Client(this.uppy, file.remote.providerOptions) - const allowedMetaFields = Array.isArray(opts.allowedMetaFields) - ? opts.allowedMetaFields - // Send along all fields by default. - : Object.keys(file.meta) - - if (file.tus) { - // Install file-specific upload overrides. - Object.assign(opts, file.tus) - } - - const res = await client.post(file.remote.url, { - ...file.remote.body, - protocol: 'multipart', - endpoint: opts.endpoint, - size: file.data.size, - fieldname: opts.fieldName, - metadata: Object.fromEntries(allowedMetaFields.map(name => [name, file.meta[name]])), - httpMethod: opts.method, - useFormData: opts.formData, - headers: opts.headers, - }) - return res.token - } - - // NOTE! Keep this duplicated code in sync with other plugins - // TODO we should probably abstract this into a common function - async #uploadRemoteFile (file, options) { - // TODO: we could rewrite this to use server-sent events instead of creating WebSockets. - try { - if (file.serverToken) { - return await this.connectToServerSocket(file) - } - const serverToken = await this.#queueRequestSocketToken(file, options).abortOn(options?.signal) - - if (!this.uppy.getState().files[file.id]) return undefined - - this.uppy.setFileState(file.id, { serverToken }) - return await this.connectToServerSocket(this.uppy.getFile(file.id)) - } catch (err) { - if (err?.cause?.name !== 'AbortError') { - this.uppy.setFileState(file.id, { serverToken: undefined }) - this.uppy.emit('upload-error', file, err) - throw err - } - // The file upload was aborted, it’s not an error - return undefined - } - } - async connectToServerSocket (file) { return new Promise((resolve, reject) => { - const opts = this.#getOptions(file) + const opts = this.getOptions(file) const token = file.serverToken const host = getSocketHost(file.remote.companionUrl) let socket diff --git a/packages/@uppy/aws-s3/src/index.js b/packages/@uppy/aws-s3/src/index.js index 3a425ede7..20d4437ad 100644 --- a/packages/@uppy/aws-s3/src/index.js +++ b/packages/@uppy/aws-s3/src/index.js @@ -25,10 +25,10 @@ * the XHRUpload code, but at least it's not horrifically broken :) */ -import BasePlugin from '@uppy/core/lib/BasePlugin.js' +import UploaderPlugin from '@uppy/core/lib/UploaderPlugin.js' import AwsS3Multipart from '@uppy/aws-s3-multipart' import { RateLimitedQueue, internalRateLimitedQueue } from '@uppy/utils/lib/RateLimitedQueue' -import { RequestClient } from '@uppy/companion-client' +import { RequestClient, Provider } from '@uppy/companion-client' import { filterNonFailedFiles, filterFilesToEmitUploadStarted } from '@uppy/utils/lib/fileFilters' import packageJson from '../package.json' @@ -103,7 +103,7 @@ function defaultGetResponseError (content, xhr) { let warnedSuccessActionStatus = false // TODO deprecate this, will use s3-multipart instead -export default class AwsS3 extends BasePlugin { +export default class AwsS3 extends UploaderPlugin { static VERSION = packageJson.version #client @@ -144,6 +144,8 @@ export default class AwsS3 extends BasePlugin { this.#client = new RequestClient(uppy, opts) this.#requests = new RateLimitedQueue(this.opts.limit) + + this.setQueueRequestSocketToken(this.#requests.wrapPromiseFunction(this.#requestSocketToken, { priority: -1 })) } [Symbol.for('uppy test: getClient')] () { return this.#client } @@ -228,7 +230,7 @@ export default class AwsS3 extends BasePlugin { xhrUpload: xhrOpts, }) - return this.#uploader.uploadFile(file.id, index, numberOfFiles) + return this.uploadFile(file.id, index, numberOfFiles) }).catch((error) => { delete paramsPromises[id] @@ -247,6 +249,64 @@ export default class AwsS3 extends BasePlugin { return Promise.resolve() } + connectToServerSocket (file) { + return this.#uploader.connectToServerSocket(file) + } + + #requestSocketToken = async (file) => { + const opts = this.#uploader.getOptions(file) + const Client = file.remote.providerOptions.provider ? Provider : RequestClient + const client = new Client(this.uppy, file.remote.providerOptions) + const allowedMetaFields = Array.isArray(opts.allowedMetaFields) + ? opts.allowedMetaFields + // Send along all fields by default. + : Object.keys(file.meta) + + if (file.tus) { + // Install file-specific upload overrides. + Object.assign(opts, file.tus) + } + + const res = await client.post(file.remote.url, { + ...file.remote.body, + protocol: 'multipart', + endpoint: opts.endpoint, + size: file.data.size, + fieldname: opts.fieldName, + metadata: Object.fromEntries(allowedMetaFields.map(name => [name, file.meta[name]])), + httpMethod: opts.method, + useFormData: opts.formData, + headers: opts.headers, + }) + return res.token + } + + uploadFile (id, current, total) { + const file = this.uppy.getFile(id) + this.uppy.log(`uploading ${current} of ${total}`) + + if (file.error) throw new Error(file.error) + + if (file.isRemote) { + const controller = new AbortController() + + const removedHandler = (removedFile) => { + if (removedFile.id === file.id) controller.abort() + } + this.uppy.on('file-removed', removedHandler) + + const uploadPromise = this.uploadRemoteFile(file, { signal: controller.signal }) + + this.requests.wrapSyncFunction(() => { + this.uppy.off('file-removed', removedHandler) + }, { priority: -1 })() + + return uploadPromise + } + + return this.#uploader.uploadLocalFile(file, current, total) + } + install () { const { uppy } = this uppy.addPreProcessor(this.#setCompanionHeaders) diff --git a/packages/@uppy/companion-client/src/Provider.js b/packages/@uppy/companion-client/src/Provider.js index b30b6b4f4..af50ecd09 100644 --- a/packages/@uppy/companion-client/src/Provider.js +++ b/packages/@uppy/companion-client/src/Provider.js @@ -8,6 +8,8 @@ const getName = (id) => { } export default class Provider extends RequestClient { + #refreshingTokenPromise + constructor (uppy, opts) { super(uppy, opts) this.provider = opts.provider @@ -20,7 +22,7 @@ export default class Provider extends RequestClient { } async headers () { - const [headers, token] = await Promise.all([super.headers(), this.getAuthToken()]) + const [headers, token] = await Promise.all([super.headers(), this.#getAuthToken()]) const authHeaders = {} if (token) { authHeaders['uppy-auth-token'] = token @@ -43,14 +45,18 @@ export default class Provider extends RequestClient { return response } - setAuthToken (token) { + async setAuthToken (token) { return this.uppy.getPlugin(this.pluginId).storage.setItem(this.tokenKey, token) } - getAuthToken () { + async #getAuthToken () { return this.uppy.getPlugin(this.pluginId).storage.getItem(this.tokenKey) } + async #removeAuthToken () { + return this.uppy.getPlugin(this.pluginId).storage.removeItem(this.tokenKey) + } + /** * Ensure we have a preauth token if necessary. Attempts to fetch one if we don't, * or rejects if loading one fails. @@ -74,10 +80,44 @@ export default class Provider extends RequestClient { return `${this.hostname}/${this.id}/connect?${params}` } + refreshTokenUrl () { + return `${this.hostname}/${this.id}/refresh-token` + } + fileUrl (id) { return `${this.hostname}/${this.id}/get/${id}` } + /** @protected */ + async request (...args) { + await this.#refreshingTokenPromise + + try { + // throw Object.assign(new Error(), { isAuthError: true }) // 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) + + await this.#refreshingTokenPromise + + // Many provider requests may be starting at once, however refresh token should only be called once. + // Once a refresh token operation has started, we need all other request to wait for this operation (atomically) + this.#refreshingTokenPromise = (async () => { + try { + const response = await super.request({ path: this.refreshTokenUrl(), method: 'POST' }) + await this.setAuthToken(response.uppyAuthToken) + } finally { + this.#refreshingTokenPromise = undefined + } + })() + + await this.#refreshingTokenPromise + + // now retry the request with our new refresh token + return super.request(...args) + } + } + async fetchPreAuthToken () { if (!this.companionKeysParams) { return @@ -95,12 +135,10 @@ export default class Provider extends RequestClient { return this.get(`${this.id}/list/${directory || ''}`) } - logout () { - return this.get(`${this.id}/logout`) - .then((response) => Promise.all([ - response, - this.uppy.getPlugin(this.pluginId).storage.removeItem(this.tokenKey), - ])).then(([response]) => response) + async logout () { + const response = await this.get(`${this.id}/logout`) + await this.#removeAuthToken() + return response } static initPlugin (plugin, opts, defaultOpts) { diff --git a/packages/@uppy/companion-client/src/RequestClient.js b/packages/@uppy/companion-client/src/RequestClient.js index 5e32d8320..69fcc882a 100644 --- a/packages/@uppy/companion-client/src/RequestClient.js +++ b/packages/@uppy/companion-client/src/RequestClient.js @@ -150,7 +150,8 @@ export default class RequestClient { })) } - async #request ({ path, method = 'GET', data, skipPostResponse, signal }) { + /** @protected */ + async request ({ path, method = 'GET', data, skipPostResponse, signal }) { try { const headers = await this.preflightAndHeaders(path) const response = await fetchWithNetworkError(this.#getUrl(path), { @@ -172,20 +173,20 @@ export default class RequestClient { // 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 }) + return this.request({ ...options, path }) } 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 }) + return this.request({ ...options, path, method: 'POST', data }) } 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 } - return this.#request({ ...options, path, method: 'DELETE', data }) + return this.request({ ...options, path, method: 'DELETE', data }) } } diff --git a/packages/@uppy/companion/src/companion.js b/packages/@uppy/companion/src/companion.js index 4f88898b5..2f2943648 100644 --- a/packages/@uppy/companion/src/companion.js +++ b/packages/@uppy/companion/src/companion.js @@ -123,6 +123,7 @@ module.exports.app = (optionsArg = {}) => { app.get('/:providerName/connect', middlewares.hasSessionAndProvider, middlewares.hasOAuthProvider, controllers.connect) app.get('/:providerName/redirect', middlewares.hasSessionAndProvider, middlewares.hasOAuthProvider, controllers.redirect) app.get('/:providerName/callback', middlewares.hasSessionAndProvider, middlewares.hasOAuthProvider, controllers.callback) + app.post('/:providerName/refresh-token', middlewares.hasSessionAndProvider, middlewares.hasOAuthProvider, middlewares.verifyToken, controllers.refreshToken) app.post('/:providerName/deauthorization/callback', express.json(), middlewares.hasSessionAndProvider, middlewares.hasBody, middlewares.hasOAuthProvider, controllers.deauthorizationCallback) app.get('/:providerName/logout', middlewares.hasSessionAndProvider, middlewares.hasOAuthProvider, middlewares.gentleVerifyToken, controllers.logout) app.get('/:providerName/send-token', middlewares.hasSessionAndProvider, middlewares.hasOAuthProvider, middlewares.verifyToken, controllers.sendToken) diff --git a/packages/@uppy/companion/src/config/grant.js b/packages/@uppy/companion/src/config/grant.js index c322b4004..1e77be22a 100644 --- a/packages/@uppy/companion/src/config/grant.js +++ b/packages/@uppy/companion/src/config/grant.js @@ -8,12 +8,14 @@ module.exports = () => { 'https://www.googleapis.com/auth/drive.readonly', ], callback: '/drive/callback', + custom_params: { access_type : 'offline' }, }, dropbox: { transport: 'session', authorize_url: 'https://www.dropbox.com/oauth2/authorize', access_url: 'https://api.dropbox.com/oauth2/token', callback: '/dropbox/callback', + custom_params: { token_access_type : 'offline' }, }, box: { transport: 'session', diff --git a/packages/@uppy/companion/src/server/controllers/callback.js b/packages/@uppy/companion/src/server/controllers/callback.js index b3ac72d30..dcd7dadd2 100644 --- a/packages/@uppy/companion/src/server/controllers/callback.js +++ b/packages/@uppy/companion/src/server/controllers/callback.js @@ -31,21 +31,27 @@ const closePageHtml = (origin) => ` module.exports = function callback (req, res, next) { // eslint-disable-line no-unused-vars const { providerName } = req.params - if (!req.companion.providerTokens) { - req.companion.providerTokens = {} - } - const grant = req.session.grant || {} - if (grant.response && grant.response.access_token) { - req.companion.providerTokens[providerName] = grant.response.access_token - logger.debug(`Generating auth token for provider ${providerName}`, null, req.id) - const uppyAuthToken = tokenService.generateEncryptedToken(req.companion.providerTokens, req.companion.options.secret) - return res.redirect(req.companion.buildURL(`/${providerName}/send-token?uppyAuthToken=${uppyAuthToken}`, true)) + + 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)) } - 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] = { + accessToken: grant.response.access_token, + refreshToken: grant.response.refresh_token, // might be undefined for some providers + } + logger.debug(`Generating auth token for provider ${providerName}`, null, req.id) + const uppyAuthToken = tokenService.generateEncryptedAuthToken( + req.companion.allProvidersTokens, req.companion.options.secret, + ) + + tokenService.addToCookiesIfNeeded(req, res, uppyAuthToken) + + return res.redirect(req.companion.buildURL(`/${providerName}/send-token?uppyAuthToken=${uppyAuthToken}`, true)) } diff --git a/packages/@uppy/companion/src/server/controllers/deauth-callback.js b/packages/@uppy/companion/src/server/controllers/deauth-callback.js index 0157b9067..de5f39483 100644 --- a/packages/@uppy/companion/src/server/controllers/deauth-callback.js +++ b/packages/@uppy/companion/src/server/controllers/deauth-callback.js @@ -1,4 +1,4 @@ -const { errorToResponse } = require('../provider/error') +const { respondWithError } = require('../provider/error') async function deauthCallback ({ body, companion, headers }, res, next) { // we need the provider instance to decide status codes because @@ -10,11 +10,7 @@ async function deauthCallback ({ body, companion, headers }, res, next) { res.status(status || 200).json(data) return } catch (err) { - const errResp = errorToResponse(err) - if (errResp) { - res.status(errResp.code).json({ message: errResp.message }) - return - } + if (respondWithError(err, res)) return next(err) } } diff --git a/packages/@uppy/companion/src/server/controllers/get.js b/packages/@uppy/companion/src/server/controllers/get.js index df1167315..64babedbf 100644 --- a/packages/@uppy/companion/src/server/controllers/get.js +++ b/packages/@uppy/companion/src/server/controllers/get.js @@ -3,15 +3,15 @@ const { startDownUpload } = require('../helpers/upload') async function get (req, res) { const { id } = req.params - const token = req.companion.providerToken + const { accessToken } = req.companion.providerTokens const { provider } = req.companion async function getSize () { - return provider.size({ id, token, query: req.query }) + return provider.size({ id, token: accessToken, query: req.query }) } async function download () { - const { stream } = await provider.download({ id, token, query: req.query }) + const { stream } = await provider.download({ id, token: accessToken, 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 b9a499eec..4410eec3b 100644 --- a/packages/@uppy/companion/src/server/controllers/index.js +++ b/packages/@uppy/companion/src/server/controllers/index.js @@ -10,4 +10,5 @@ module.exports = { connect: require('./connect'), preauth: require('./preauth'), redirect: require('./oauth-redirect'), + refreshToken: require('./refresh-token'), } diff --git a/packages/@uppy/companion/src/server/controllers/list.js b/packages/@uppy/companion/src/server/controllers/list.js index f53944254..284660c1b 100644 --- a/packages/@uppy/companion/src/server/controllers/list.js +++ b/packages/@uppy/companion/src/server/controllers/list.js @@ -1,17 +1,13 @@ -const { errorToResponse } = require('../provider/error') +const { respondWithError } = require('../provider/error') async function list ({ query, params, companion }, res, next) { - const token = companion.providerToken + const { accessToken } = companion.providerTokens try { - const data = await companion.provider.list({ companion, token, directory: params.id, query }) + const data = await companion.provider.list({ companion, token: accessToken, directory: params.id, query }) res.json(data) } catch (err) { - const errResp = errorToResponse(err) - if (errResp) { - res.status(errResp.code).json({ message: errResp.message }) - return - } + if (respondWithError(err, res)) return next(err) } } diff --git a/packages/@uppy/companion/src/server/controllers/logout.js b/packages/@uppy/companion/src/server/controllers/logout.js index db9bf9a9b..1ec87e303 100644 --- a/packages/@uppy/companion/src/server/controllers/logout.js +++ b/packages/@uppy/companion/src/server/controllers/logout.js @@ -1,5 +1,5 @@ const tokenService = require('../helpers/jwt') -const { errorToResponse } = require('../provider/error') +const { respondWithError } = require('../provider/error') /** * @@ -15,26 +15,23 @@ async function logout (req, res, next) { } const { providerName } = req.params const { companion } = req - const token = companion.providerTokens ? companion.providerTokens[providerName] : null + const tokens = companion.allProvidersTokens ? companion.allProvidersTokens[providerName] : null - if (!token) { + if (!tokens) { cleanSession() res.json({ ok: true, revoked: false }) return } try { - const data = await companion.provider.logout({ token, companion }) - delete companion.providerTokens[providerName] + const { accessToken } = tokens + const data = await companion.provider.logout({ token: accessToken, companion }) + delete companion.allProvidersTokens[providerName] tokenService.removeFromCookies(res, companion.options, companion.provider.authProvider) cleanSession() res.json({ ok: true, ...data }) } catch (err) { - const errResp = errorToResponse(err) - if (errResp) { - res.status(errResp.code).json({ message: errResp.message }) - return - } + if (respondWithError(err, res)) return next(err) } } diff --git a/packages/@uppy/companion/src/server/controllers/refresh-token.js b/packages/@uppy/companion/src/server/controllers/refresh-token.js new file mode 100644 index 000000000..99203ebe3 --- /dev/null +++ b/packages/@uppy/companion/src/server/controllers/refresh-token.js @@ -0,0 +1,51 @@ +const tokenService = require('../helpers/jwt') +const { respondWithError } = require('../provider/error') +const logger = require('../logger') + +// https://www.dropboxforum.com/t5/Dropbox-API-Support-Feedback/Get-refresh-token-from-access-token/td-p/596739 +// https://developers.dropbox.com/oauth-guide +// https://github.com/simov/grant/issues/149 +async function refreshToken (req, res, next) { + const { providerName } = req.params + + const { key: clientId, secret: clientSecret } = req.companion.options.providerOptions[providerName] + + const providerTokens = req.companion.allProvidersTokens[providerName] + + // not all providers have refresh tokens + if (providerTokens.refreshToken == null) { + res.sendStatus(401) + return + } + + try { + const data = await req.companion.provider.refreshToken({ + clientId, clientSecret, refreshToken: providerTokens.refreshToken, + }) + + const newAllProvidersTokens = { + ...req.companion.allProvidersTokens, + [providerName]: { + ...providerTokens, + 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, + ) + + tokenService.addToCookiesIfNeeded(req, res, uppyAuthToken) + + res.send({ uppyAuthToken }) + } catch (err) { + if (respondWithError(err, res)) return + next(err) + } +} + +module.exports = refreshToken diff --git a/packages/@uppy/companion/src/server/controllers/send-token.js b/packages/@uppy/companion/src/server/controllers/send-token.js index 2ec628094..17cc4c160 100644 --- a/packages/@uppy/companion/src/server/controllers/send-token.js +++ b/packages/@uppy/companion/src/server/controllers/send-token.js @@ -1,7 +1,6 @@ const { URL } = require('node:url') const serialize = require('serialize-javascript') -const tokenService = require('../helpers/jwt') const { hasMatch } = require('../helpers/utils') const oAuthState = require('../helpers/oauth-state') @@ -33,10 +32,6 @@ const htmlContent = (token, origin) => { */ module.exports = function sendToken (req, res, next) { const uppyAuthToken = req.companion.authToken - // some providers need the token in cookies for thumbnail/image requests - if (req.companion.provider.needsCookieAuth) { - tokenService.addToCookies(res, uppyAuthToken, req.companion.options, req.companion.provider.authProvider) - } const state = oAuthState.getDynamicStateFromRequest(req) if (state) { diff --git a/packages/@uppy/companion/src/server/controllers/thumbnail.js b/packages/@uppy/companion/src/server/controllers/thumbnail.js index be4b4d507..0994964ed 100644 --- a/packages/@uppy/companion/src/server/controllers/thumbnail.js +++ b/packages/@uppy/companion/src/server/controllers/thumbnail.js @@ -5,11 +5,11 @@ */ async function thumbnail (req, res, next) { const { providerName, id } = req.params - const token = req.companion.providerTokens[providerName] + const { accessToken } = req.companion.allProvidersTokens[providerName] const { provider } = req.companion try { - const { stream } = await provider.thumbnail({ id, token }) + const { stream } = await provider.thumbnail({ id, token: accessToken }) stream.pipe(res) } catch (err) { if (err.isAuthError) res.sendStatus(401) diff --git a/packages/@uppy/companion/src/server/helpers/jwt.js b/packages/@uppy/companion/src/server/helpers/jwt.js index 3d76f4677..b1a9b75df 100644 --- a/packages/@uppy/companion/src/server/helpers/jwt.js +++ b/packages/@uppy/companion/src/server/helpers/jwt.js @@ -1,15 +1,29 @@ const jwt = require('jsonwebtoken') const { encrypt, decrypt } = require('./utils') -const EXPIRY = 60 * 60 * 24 // one day (24 hrs) +// The Uppy auth token is a (JWT) container around provider OAuth access & refresh tokens. +// Providers themselves will verify these inner tokens. +// 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. +// 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: +// A user starts an upload, thus creating an auth token. They leave the upload running over night. +// The upload finishes after a few hours, but with some failed files. Then the next day (say after 25hr) +// they would retry the failed tiles, however now the Uppy auth token has expired and +// 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 EXPIRY_MS = EXPIRY * 1000 /** * - * @param {*} payload + * @param {*} data * @param {string} secret */ -module.exports.generateToken = (payload, secret) => { - return jwt.sign({ data: payload }, secret, { expiresIn: EXPIRY }) +const generateToken = (data, secret) => { + return jwt.sign({ data }, secret, { expiresIn: EXPIRY }) } /** @@ -17,13 +31,9 @@ module.exports.generateToken = (payload, secret) => { * @param {string} token * @param {string} secret */ -module.exports.verifyToken = (token, secret) => { - try { - // @ts-ignore - return { payload: jwt.verify(token, secret, {}).data } - } catch (err) { - return { err } - } +const verifyToken = (token, secret) => { + // @ts-ignore + return jwt.verify(token, secret, {}).data } /** @@ -32,7 +42,17 @@ module.exports.verifyToken = (token, secret) => { * @param {string} secret */ module.exports.generateEncryptedToken = (payload, secret) => { - return encrypt(module.exports.generateToken(payload, secret), secret) + // return payload // for easier debugging + return encrypt(generateToken(payload, secret), secret) +} + +/** + * + * @param {*} payload + * @param {string} secret + */ +module.exports.generateEncryptedAuthToken = (payload, secret) => { + return module.exports.generateEncryptedToken(JSON.stringify(payload), secret) } /** @@ -41,16 +61,26 @@ module.exports.generateEncryptedToken = (payload, secret) => { * @param {string} secret */ module.exports.verifyEncryptedToken = (token, secret) => { - try { - return module.exports.verifyToken(decrypt(token, secret), secret) - } catch (err) { - return { err } - } + const ret = verifyToken(decrypt(token, secret), secret) + if (!ret) throw new Error('No payload') + return ret +} + +/** + * + * @param {string} token + * @param {string} secret + */ +module.exports.verifyEncryptedAuthToken = (token, secret, providerName) => { + const json = module.exports.verifyEncryptedToken(token, secret) + const tokens = JSON.parse(json) + if (!tokens[providerName]) throw new Error(`Missing token payload for provider ${providerName}`) + return tokens } const addToCookies = (res, token, companionOptions, authProvider, prefix) => { const cookieOptions = { - maxAge: 1000 * EXPIRY, // would expire after one day (24 hrs) + maxAge: EXPIRY_MS, httpOnly: true, } @@ -68,15 +98,11 @@ const addToCookies = (res, token, companionOptions, authProvider, prefix) => { res.cookie(`${prefix}--${authProvider}`, token, cookieOptions) } -/** - * - * @param {object} res - * @param {string} token - * @param {object} companionOptions - * @param {string} authProvider - */ -module.exports.addToCookies = (res, token, companionOptions, authProvider) => { - addToCookies(res, token, companionOptions, authProvider, 'uppyAuthToken') +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') + } } /** @@ -87,7 +113,7 @@ module.exports.addToCookies = (res, token, companionOptions, authProvider) => { */ module.exports.removeFromCookies = (res, companionOptions, authProvider) => { const cookieOptions = { - maxAge: 1000 * EXPIRY, // would expire after one day (24 hrs) + maxAge: EXPIRY_MS, httpOnly: true, } diff --git a/packages/@uppy/companion/src/server/helpers/upload.js b/packages/@uppy/companion/src/server/helpers/upload.js index 60d16355b..8e1850b98 100644 --- a/packages/@uppy/companion/src/server/helpers/upload.js +++ b/packages/@uppy/companion/src/server/helpers/upload.js @@ -1,6 +1,6 @@ const Uploader = require('../Uploader') const logger = require('../logger') -const { errorToResponse } = require('../provider/error') +const { respondWithError } = require('../provider/error') const { ValidationError } = Uploader @@ -36,11 +36,7 @@ async function startDownUpload ({ req, res, getSize, download, onUnhandledError return } - const errResp = errorToResponse(err) - if (errResp) { - res.status(errResp.code).json({ message: errResp.message }) - return - } + if (respondWithError(err, res)) return onUnhandledError(err) } diff --git a/packages/@uppy/companion/src/server/middlewares.js b/packages/@uppy/companion/src/server/middlewares.js index 995f04e41..c344ff160 100644 --- a/packages/@uppy/companion/src/server/middlewares.js +++ b/packages/@uppy/companion/src/server/middlewares.js @@ -67,7 +67,9 @@ exports.verifyToken = (req, res, next) => { return } - req.companion.providerToken = providerOptions[providerName].key + req.companion.providerTokens = { + accessToken: providerOptions[providerName].key, + } next() return } @@ -80,16 +82,15 @@ exports.verifyToken = (req, res, next) => { return } const { providerName } = req.params - const { err, payload } = tokenService.verifyEncryptedToken(token, req.companion.options.secret) - if (err || !payload[providerName]) { - if (err) { - logger.error(err.message, 'token.verify.error', req.id) - } + 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 } - req.companion.providerTokens = payload - req.companion.providerToken = payload[providerName] next() } @@ -97,9 +98,13 @@ exports.verifyToken = (req, res, next) => { exports.gentleVerifyToken = (req, res, next) => { const { providerName } = req.params if (req.companion.authToken) { - const { err, payload } = tokenService.verifyEncryptedToken(req.companion.authToken, req.companion.options.secret) - if (!err && payload[providerName]) { - req.companion.providerTokens = payload + try { + const payload = tokenService.verifyEncryptedAuthToken( + req.companion.authToken, req.companion.options.secret, providerName, + ) + req.companion.allProvidersTokens = payload + } catch (err) { + logger.error(err.message, 'token.gentle.verify.error', req.id) } } next() diff --git a/packages/@uppy/companion/src/server/provider/Provider.js b/packages/@uppy/companion/src/server/provider/Provider.js index 453b0ebad..2268b0aec 100644 --- a/packages/@uppy/companion/src/server/provider/Provider.js +++ b/packages/@uppy/companion/src/server/provider/Provider.js @@ -7,6 +7,7 @@ class Provider { * @param {object} options */ constructor (options) { // eslint-disable-line no-unused-vars + // Some providers might need cookie auth for the thumbnails fetched via companion this.needsCookieAuth = false return this } @@ -74,6 +75,14 @@ class Provider { throw new Error('method not implemented') } + /** + * Generate a new access token based on the refresh token + */ + // eslint-disable-next-line class-methods-use-this,no-unused-vars + async refreshToken (options) { + throw new Error('method not implemented') + } + /** * Name of the OAuth provider. Return empty string if no OAuth provider is needed. * diff --git a/packages/@uppy/companion/src/server/provider/credentials.js b/packages/@uppy/companion/src/server/provider/credentials.js index 631dd1e08..fa41dbaef 100644 --- a/packages/@uppy/companion/src/server/provider/credentials.js +++ b/packages/@uppy/companion/src/server/provider/credentials.js @@ -69,7 +69,7 @@ async function fetchProviderKeys (providerName, companionOptions, credentialRequ * @returns {import('express').RequestHandler} */ exports.getCredentialsOverrideMiddleware = (providers, companionOptions) => { - return (req, res, next) => { + return async (req, res, next) => { const { authProvider, override } = req.params const [providerName] = Object.keys(providers).filter((name) => providers[name].authProvider === authProvider) if (!providerName) { @@ -97,13 +97,17 @@ exports.getCredentialsOverrideMiddleware = (providers, companionOptions) => { return } - const { err, payload } = tokenService.verifyEncryptedToken(preAuthToken, companionOptions.preAuthSecret) - if (err || !payload) { + let payload + try { + payload = tokenService.verifyEncryptedToken(preAuthToken, companionOptions.preAuthSecret) + } catch (err) { next() return } - fetchProviderKeys(providerName, companionOptions, payload).then((credentials) => { + try { + const credentials = await fetchProviderKeys(providerName, companionOptions, payload) + res.locals.grant = { dynamic: { key: credentials.key, @@ -116,7 +120,7 @@ exports.getCredentialsOverrideMiddleware = (providers, companionOptions) => { } next() - }).catch((keyErr) => { + } catch (keyErr) { // TODO we should return an html page here that can communicate the error // back to the Uppy client, just like /send-token does res.send(` @@ -134,7 +138,7 @@ exports.getCredentialsOverrideMiddleware = (providers, companionOptions) => { `) - }) + } } } diff --git a/packages/@uppy/companion/src/server/provider/drive/index.js b/packages/@uppy/companion/src/server/provider/drive/index.js index d15a7ed37..903636c9d 100644 --- a/packages/@uppy/companion/src/server/provider/drive/index.js +++ b/packages/@uppy/companion/src/server/provider/drive/index.js @@ -18,6 +18,10 @@ const getClient = ({ token }) => got.extend({ }, }) +const getOauthClient = () => got.extend({ + prefixUrl: 'https://oauth2.googleapis.com', +}) + async function getStats ({ id, token }) { const client = getClient({ token }) @@ -168,6 +172,13 @@ class Drive extends Provider { }) } + async refreshToken ({ clientId, clientSecret, refreshToken }) { + return this.#withErrorHandling('provider.drive.token.refresh.error', async () => { + const { access_token: accessToken } = await getOauthClient().post('token', { form: { refresh_token: refreshToken, grant_type: 'refresh_token', client_id: clientId, client_secret: clientSecret } }).json() + return { accessToken } + }) + } + async #withErrorHandling (tag, fn) { return withProviderErrorHandling({ fn, diff --git a/packages/@uppy/companion/src/server/provider/dropbox/index.js b/packages/@uppy/companion/src/server/provider/dropbox/index.js index 848c6b202..50b4989ea 100644 --- a/packages/@uppy/companion/src/server/provider/dropbox/index.js +++ b/packages/@uppy/companion/src/server/provider/dropbox/index.js @@ -24,6 +24,10 @@ const getClient = ({ token }) => got.extend({ }, }) +const getOauthClient = () => got.extend({ + prefixUrl: 'https://api.dropboxapi.com/oauth2', +}) + async function list ({ directory, query, token }) { if (query.cursor) { return getClient({ token }).post('files/list_folder/continue', { json: { cursor: query.cursor }, responseType: 'json' }).json() @@ -52,7 +56,6 @@ class DropBox extends Provider { constructor (options) { super(options) this.authProvider = DropBox.authProvider - // needed for the thumbnails fetched via companion this.needsCookieAuth = true } @@ -121,6 +124,13 @@ class DropBox extends Provider { }) } + async refreshToken ({ clientId, clientSecret, refreshToken }) { + return this.#withErrorHandling('provider.dropbox.token.refresh.error', async () => { + const { access_token: accessToken } = await getOauthClient().post('token', { form: { refresh_token: refreshToken, grant_type: 'refresh_token', client_id: clientId, client_secret: clientSecret } }).json() + return { accessToken } + }) + } + async #withErrorHandling (tag, fn) { return withProviderErrorHandling({ fn, diff --git a/packages/@uppy/companion/src/server/provider/error.d.ts b/packages/@uppy/companion/src/server/provider/error.d.ts index 45ba78db6..29ce604bc 100644 --- a/packages/@uppy/companion/src/server/provider/error.d.ts +++ b/packages/@uppy/companion/src/server/provider/error.d.ts @@ -12,3 +12,5 @@ export class ProviderAuthError extends ProviderApiError { } 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 5d053ab5d..cf3593219 100644 --- a/packages/@uppy/companion/src/server/provider/error.js +++ b/packages/@uppy/companion/src/server/provider/error.js @@ -52,4 +52,13 @@ function errorToResponse (err) { return undefined } -module.exports = { ProviderAuthError, ProviderApiError, errorToResponse } +function respondWithError (err, res) { + const errResp = errorToResponse(err) + if (errResp) { + res.status(errResp.code).json({ message: errResp.message }) + return true + } + return false +} + +module.exports = { ProviderAuthError, ProviderApiError, errorToResponse, respondWithError } diff --git a/packages/@uppy/companion/src/server/provider/providerErrors.js b/packages/@uppy/companion/src/server/provider/providerErrors.js index 6234689a5..bc3809355 100644 --- a/packages/@uppy/companion/src/server/provider/providerErrors.js +++ b/packages/@uppy/companion/src/server/provider/providerErrors.js @@ -1,10 +1,8 @@ const logger = require('../logger') const { ProviderApiError, ProviderAuthError } = require('./error') -function convertProviderError ({ err, providerName, isAuthError = () => false, getJsonErrorMessage }) { - const { response } = err - - function getErrorMessage () { +async function withProviderErrorHandling ({ fn, tag, providerName, isAuthError = () => false, getJsonErrorMessage }) { + function getErrorMessage (response) { if (typeof response.body === 'object') { const message = getJsonErrorMessage(response.body) if (message != null) return message @@ -17,22 +15,21 @@ function convertProviderError ({ err, providerName, isAuthError = () => false, g return `request to ${providerName} returned ${response.statusCode}` } - if (response) { - // @ts-ignore - if (isAuthError(response)) return new ProviderAuthError() - - return new ProviderApiError(getErrorMessage(), response.statusCode) - } - - return err -} - -async function withProviderErrorHandling ({ fn, tag, providerName, isAuthError, getJsonErrorMessage }) { try { return await fn() } catch (err) { - const err2 = convertProviderError({ err, providerName, isAuthError, getJsonErrorMessage }) + const { response } = err + + let err2 = err + + if (response) { + // @ts-ignore + if (isAuthError(response)) err2 = new ProviderAuthError() + else err2 = new ProviderApiError(getErrorMessage(response), response.statusCode) + } + logger.error(err2, tag) + throw err2 } } diff --git a/packages/@uppy/companion/test/__tests__/callback.js b/packages/@uppy/companion/test/__tests__/callback.js index 71d113f4b..4eee609aa 100644 --- a/packages/@uppy/companion/test/__tests__/callback.js +++ b/packages/@uppy/companion/test/__tests__/callback.js @@ -3,7 +3,7 @@ const mockOauthState = require('../mockoauthstate')() // eslint-disable-next-line import/order const request = require('supertest') const tokenService = require('../../src/server/helpers/jwt') -const { getServer } = require('../mockserver') +const { getServer, grantToken } = require('../mockserver') jest.mock('../../src/server/helpers/oauth-state', () => ({ ...jest.requireActual('../../src/server/helpers/oauth-state'), @@ -12,10 +12,10 @@ jest.mock('../../src/server/helpers/oauth-state', () => ({ const authServer = getServer() const authData = { - dropbox: 'token value', - drive: 'token value', + dropbox: { accessToken: 'token value' }, + drive: { accessToken: 'token value' }, } -const token = tokenService.generateEncryptedToken(authData, process.env.COMPANION_SECRET) +const token = tokenService.generateEncryptedAuthToken(authData, process.env.COMPANION_SECRET) describe('test authentication callback', () => { test('authentication callback redirects to send-token url', () => { @@ -27,14 +27,25 @@ describe('test authentication callback', () => { }) }) - test('the token gets sent via cookie and html', () => { + test('authentication callback sets cookie', () => { + console.log(process.env.COMPANION_SECRET) + return request(authServer) + .get('/dropbox/callback') + .expect(302) + .expect((res) => { + expect(res.header.location).toContain('http://localhost:3020/dropbox/send-token?uppyAuthToken=') + const authToken = decodeURIComponent(res.header['set-cookie'][0].split(';')[0].split('uppyAuthToken--dropbox=')[1]) + const payload = tokenService.verifyEncryptedAuthToken(authToken, process.env.COMPANION_SECRET, 'dropbox') + expect(payload).toEqual({ dropbox: { accessToken: grantToken } }) + }) + }) + + test('the token gets sent via html', () => { // see mock ../../src/server/helpers/oauth-state above for state values return request(authServer) .get(`/dropbox/send-token?uppyAuthToken=${token}&state=state-with-newer-version`) .expect(200) .expect((res) => { - const authToken = res.header['set-cookie'][0].split(';')[0].split('uppyAuthToken--dropbox=')[1] - expect(authToken).toEqual(token) const body = ` diff --git a/packages/@uppy/companion/test/__tests__/companion.js b/packages/@uppy/companion/test/__tests__/companion.js index 457bed0a5..f614c6096 100644 --- a/packages/@uppy/companion/test/__tests__/companion.js +++ b/packages/@uppy/companion/test/__tests__/companion.js @@ -32,11 +32,11 @@ const { getServer } = require('../mockserver') // todo don't share server between tests. rewrite to not use env variables const authServer = getServer({ COMPANION_CLIENT_SOCKET_CONNECT_TIMEOUT: '0' }) const authData = { - dropbox: 'token value', - box: 'token value', - drive: 'token value', + dropbox: { accessToken: 'token value' }, + box: { accessToken: 'token value' }, + drive: { accessToken: 'token value' }, } -const token = tokenService.generateEncryptedToken(authData, process.env.COMPANION_SECRET) +const token = tokenService.generateEncryptedAuthToken(authData, process.env.COMPANION_SECRET) const OAUTH_STATE = 'some-cool-nice-encrytpion' afterAll(() => { diff --git a/packages/@uppy/companion/test/__tests__/credentials.js b/packages/@uppy/companion/test/__tests__/credentials.js index 0830e4dc3..74b9abb2a 100644 --- a/packages/@uppy/companion/test/__tests__/credentials.js +++ b/packages/@uppy/companion/test/__tests__/credentials.js @@ -8,9 +8,9 @@ const { remoteZoomKey, remoteZoomSecret, remoteZoomVerificationToken } = require const authServer = getServer({ COMPANION_ZOOM_KEYS_ENDPOINT: 'http://localhost:2111/zoom-keys' }) const authData = { - zoom: 'token value', + zoom: { accessToken: 'token value' }, } -const token = tokenService.generateEncryptedToken(authData, process.env.COMPANION_SECRET) +const token = tokenService.generateEncryptedAuthToken(authData, process.env.COMPANION_SECRET) afterAll(() => { nock.cleanAll() diff --git a/packages/@uppy/companion/test/__tests__/preauth.js b/packages/@uppy/companion/test/__tests__/preauth.js index cc638735e..06d8cd2e7 100644 --- a/packages/@uppy/companion/test/__tests__/preauth.js +++ b/packages/@uppy/companion/test/__tests__/preauth.js @@ -1,14 +1,8 @@ jest.mock('../../src/server/helpers/jwt', () => { return { - generateToken: () => {}, - verifyToken: () => {}, - generateEncryptedToken: () => { - return 'dummy token' - }, - verifyEncryptedToken: () => { - return { payload: '' } - }, - addToCookies: () => {}, + generateEncryptedToken: () => 'dummy token', + verifyEncryptedToken: () => '', + addToCookiesIfNeeded: () => {}, removeFromCookies: () => {}, } }) diff --git a/packages/@uppy/companion/test/__tests__/provider-manager.js b/packages/@uppy/companion/test/__tests__/provider-manager.js index 3da05943f..de9cf2b2a 100644 --- a/packages/@uppy/companion/test/__tests__/provider-manager.js +++ b/packages/@uppy/companion/test/__tests__/provider-manager.js @@ -52,6 +52,9 @@ describe('Test Provider options', () => { authorize_url: 'https://www.dropbox.com/oauth2/authorize', access_url: 'https://api.dropbox.com/oauth2/token', callback: '/dropbox/callback', + custom_params: { + token_access_type: 'offline', + }, }) expect(grantConfig.box).toEqual({ @@ -73,6 +76,9 @@ describe('Test Provider options', () => { 'https://www.googleapis.com/auth/drive.readonly', ], callback: '/drive/callback', + custom_params: { + access_type: 'offline', + }, }) expect(grantConfig.zoom).toEqual({ key: 'zoom_key', diff --git a/packages/@uppy/companion/test/__tests__/providers.js b/packages/@uppy/companion/test/__tests__/providers.js index 05b562911..84cc87196 100644 --- a/packages/@uppy/companion/test/__tests__/providers.js +++ b/packages/@uppy/companion/test/__tests__/providers.js @@ -31,9 +31,9 @@ const AUTH_PROVIDERS = { } const authData = {} providerNames.forEach((provider) => { - authData[provider] = 'token value' + authData[provider] = { accessToken: 'token value' } }) -const token = tokenService.generateEncryptedToken(authData, process.env.COMPANION_SECRET) +const token = tokenService.generateEncryptedAuthToken(authData, process.env.COMPANION_SECRET) const thisOrThat = (value1, value2) => { if (value1 !== undefined) { diff --git a/packages/@uppy/companion/test/mockserver.js b/packages/@uppy/companion/test/mockserver.js index 82b36e244..7bdf519f8 100644 --- a/packages/@uppy/companion/test/mockserver.js +++ b/packages/@uppy/companion/test/mockserver.js @@ -50,6 +50,8 @@ function updateEnv (env) { module.exports.setDefaultEnv = () => updateEnv(defaultEnv) +module.exports.grantToken = 'fake token' + module.exports.getServer = (extraEnv) => { const env = { ...defaultEnv, @@ -69,7 +71,7 @@ module.exports.getServer = (extraEnv) => { authServer.use(session({ secret: 'grant', resave: true, saveUninitialized: true })) authServer.all('*/callback', (req, res, next) => { req.session.grant = { - response: { access_token: 'fake token' }, + response: { access_token: module.exports.grantToken }, } next() }) diff --git a/packages/@uppy/core/src/UploaderPlugin.js b/packages/@uppy/core/src/UploaderPlugin.js new file mode 100644 index 000000000..a877bfb41 --- /dev/null +++ b/packages/@uppy/core/src/UploaderPlugin.js @@ -0,0 +1,34 @@ +import BasePlugin from './BasePlugin.js' + +export default class UploaderPlugin extends BasePlugin { + #queueRequestSocketToken + + /** @protected */ + setQueueRequestSocketToken (fn) { + this.#queueRequestSocketToken = fn + } + + async uploadRemoteFile (file, options = {}) { + // TODO: we could rewrite this to use server-sent events instead of creating WebSockets. + try { + if (file.serverToken) { + return await this.connectToServerSocket(file) + } + const serverToken = await this.#queueRequestSocketToken(file).abortOn(options.signal) + + if (!this.uppy.getState().files[file.id]) return undefined + + this.uppy.setFileState(file.id, { serverToken }) + return await this.connectToServerSocket(this.uppy.getFile(file.id)) + } catch (err) { + if (err?.cause?.name === 'AbortError') { + // The file upload was aborted, it’s not an error + return undefined + } + + this.uppy.setFileState(file.id, { serverToken: undefined }) + this.uppy.emit('upload-error', file, err) + throw err + } + } +} diff --git a/packages/@uppy/core/src/Uppy.js b/packages/@uppy/core/src/Uppy.js index f9644bb09..c597c97ef 100644 --- a/packages/@uppy/core/src/Uppy.js +++ b/packages/@uppy/core/src/Uppy.js @@ -1009,13 +1009,13 @@ class Uppy { errorHandler(error, file, response) if (typeof error === 'object' && error.message) { - const newError = new Error(error.message) + this.log(error.message, 'error') + const newError = new Error(this.i18n('failedToUpload', { file: file?.name })) newError.isUserFacing = true // todo maybe don't do this with all errors? newError.details = error.message if (error.details) { newError.details += ` ${error.details}` } - newError.message = this.i18n('failedToUpload', { file: file?.name }) this.#informAndEmit([newError]) } else { this.#informAndEmit([error]) diff --git a/packages/@uppy/tus/src/index.js b/packages/@uppy/tus/src/index.js index 1b053c1e3..39022f3d8 100644 --- a/packages/@uppy/tus/src/index.js +++ b/packages/@uppy/tus/src/index.js @@ -1,4 +1,4 @@ -import BasePlugin from '@uppy/core/lib/BasePlugin.js' +import UploaderPlugin from '@uppy/core/lib/UploaderPlugin.js' import * as tus from 'tus-js-client' import { Provider, RequestClient, Socket } from '@uppy/companion-client' import emitSocketProgress from '@uppy/utils/lib/emitSocketProgress' @@ -52,13 +52,11 @@ const tusDefaultOptions = { /** * Tus resumable file uploader */ -export default class Tus extends BasePlugin { +export default class Tus extends UploaderPlugin { static VERSION = packageJson.version #retryDelayIterator - #queueRequestSocketToken - /** * @param {Uppy} uppy * @param {TusOptions} opts @@ -102,7 +100,7 @@ export default class Tus extends BasePlugin { this.uploaderSockets = Object.create(null) this.handleResetProgress = this.handleResetProgress.bind(this) - this.#queueRequestSocketToken = this.requests.wrapPromiseFunction(this.#requestSocketToken, { priority: -1 }) + this.setQueueRequestSocketToken(this.requests.wrapPromiseFunction(this.#requestSocketToken, { priority: -1 })) } handleResetProgress () { @@ -455,36 +453,6 @@ export default class Tus extends BasePlugin { return res.token } - // NOTE! Keep this duplicated code in sync with other plugins - // TODO we should probably abstract this into a common function - /** - * @param {UppyFile} file for use with upload - * @returns {Promise} - */ - async #uploadRemote (file, options) { - this.resetUploaderReferences(file.id) - - try { - if (file.serverToken) { - return await this.connectToServerSocket(file) - } - const serverToken = await this.#queueRequestSocketToken(file, options).abortOn(options?.signal) - - if (!this.uppy.getState().files[file.id]) return undefined - - this.uppy.setFileState(file.id, { serverToken }) - return await this.connectToServerSocket(this.uppy.getFile(file.id)) - } catch (err) { - if (err?.cause?.name !== 'AbortError') { - this.uppy.setFileState(file.id, { serverToken: undefined }) - this.uppy.emit('upload-error', file, err) - throw err - } - // The file upload was aborted, it’s not an error - return undefined - } - } - /** * See the comment on the upload() method. * @@ -745,7 +713,8 @@ export default class Tus extends BasePlugin { } this.uppy.on('file-removed', removedHandler) - const uploadPromise = this.#uploadRemote(file, { signal: controller.signal }) + this.resetUploaderReferences(file.id) + const uploadPromise = this.uploadRemoteFile(file, { signal: controller.signal }) this.requests.wrapSyncFunction(() => { this.uppy.off('file-removed', removedHandler) diff --git a/packages/@uppy/xhr-upload/src/index.js b/packages/@uppy/xhr-upload/src/index.js index e44fcaf47..a8a331ff7 100644 --- a/packages/@uppy/xhr-upload/src/index.js +++ b/packages/@uppy/xhr-upload/src/index.js @@ -1,4 +1,4 @@ -import BasePlugin from '@uppy/core/lib/BasePlugin.js' +import UploaderPlugin from '@uppy/core/lib/UploaderPlugin.js' import { nanoid } from 'nanoid/non-secure' import { Provider, RequestClient, Socket } from '@uppy/companion-client' import emitSocketProgress from '@uppy/utils/lib/emitSocketProgress' @@ -46,12 +46,10 @@ function setTypeInBlob (file) { return dataWithUpdatedType } -export default class XHRUpload extends BasePlugin { +export default class XHRUpload extends UploaderPlugin { // eslint-disable-next-line global-require static VERSION = packageJson.version - #queueRequestSocketToken - constructor (uppy, opts) { super(uppy, opts) this.type = 'uploader' @@ -129,7 +127,7 @@ export default class XHRUpload extends BasePlugin { } this.uploaderEvents = Object.create(null) - this.#queueRequestSocketToken = this.requests.wrapPromiseFunction(this.#requestSocketToken, { priority: -1 }) + this.setQueueRequestSocketToken(this.requests.wrapPromiseFunction(this.#requestSocketToken, { priority: -1 })) } getOptions (file) { @@ -373,31 +371,6 @@ export default class XHRUpload extends BasePlugin { return res.token } - // NOTE! Keep this duplicated code in sync with other plugins - // TODO we should probably abstract this into a common function - async #uploadRemote (file, options) { - // TODO: we could rewrite this to use server-sent events instead of creating WebSockets. - try { - if (file.serverToken) { - return await this.connectToServerSocket(file) - } - const serverToken = await this.#queueRequestSocketToken(file, options).abortOn(options?.signal) - - if (!this.uppy.getState().files[file.id]) return undefined - - this.uppy.setFileState(file.id, { serverToken }) - return await this.connectToServerSocket(this.uppy.getFile(file.id)) - } catch (err) { - if (err?.cause?.name !== 'AbortError') { - this.uppy.setFileState(file.id, { serverToken: undefined }) - this.uppy.emit('upload-error', file, err) - throw err - } - // The file upload was aborted, it’s not an error - return undefined - } - } - async connectToServerSocket (file) { return new Promise((resolve, reject) => { const opts = this.getOptions(file) @@ -603,7 +576,7 @@ export default class XHRUpload extends BasePlugin { } this.uppy.on('file-removed', removedHandler) - const uploadPromise = this.#uploadRemote(file, { signal: controller.signal }) + const uploadPromise = this.uploadRemoteFile(file, { signal: controller.signal }) this.requests.wrapSyncFunction(() => { this.uppy.off('file-removed', removedHandler)