From 188ce1c49be39e8d3747c8bdb04148fb361ee4d9 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 9 May 2023 15:41:28 +0200 Subject: [PATCH 01/21] allow storing multiple tokens inside uppy auth token --- .../src/server/controllers/callback.js | 31 +++++++------- .../companion/src/server/controllers/get.js | 6 +-- .../companion/src/server/controllers/list.js | 4 +- .../src/server/controllers/logout.js | 9 ++-- .../src/server/controllers/thumbnail.js | 4 +- .../@uppy/companion/src/server/helpers/jwt.js | 41 ++++++++++++------- .../@uppy/companion/src/server/middlewares.js | 27 +++++++----- .../src/server/provider/credentials.js | 16 +++++--- .../companion/test/__tests__/callback.js | 6 +-- .../companion/test/__tests__/companion.js | 8 ++-- .../companion/test/__tests__/credentials.js | 4 +- .../@uppy/companion/test/__tests__/preauth.js | 8 +--- .../test/__tests__/provider-manager.js | 3 ++ .../companion/test/__tests__/providers.js | 4 +- 14 files changed, 98 insertions(+), 73 deletions(-) diff --git a/packages/@uppy/companion/src/server/controllers/callback.js b/packages/@uppy/companion/src/server/controllers/callback.js index b3ac72d309..82c27c809b 100644 --- a/packages/@uppy/companion/src/server/controllers/callback.js +++ b/packages/@uppy/companion/src/server/controllers/callback.js @@ -31,21 +31,24 @@ 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)) + 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, + ) + return res.redirect(req.companion.buildURL(`/${providerName}/send-token?uppyAuthToken=${uppyAuthToken}`, true)) } diff --git a/packages/@uppy/companion/src/server/controllers/get.js b/packages/@uppy/companion/src/server/controllers/get.js index df11673151..64babedbfb 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/list.js b/packages/@uppy/companion/src/server/controllers/list.js index f539442548..232460fd9f 100644 --- a/packages/@uppy/companion/src/server/controllers/list.js +++ b/packages/@uppy/companion/src/server/controllers/list.js @@ -1,10 +1,10 @@ const { errorToResponse } = 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) diff --git a/packages/@uppy/companion/src/server/controllers/logout.js b/packages/@uppy/companion/src/server/controllers/logout.js index db9bf9a9bd..3c576ae72c 100644 --- a/packages/@uppy/companion/src/server/controllers/logout.js +++ b/packages/@uppy/companion/src/server/controllers/logout.js @@ -15,17 +15,18 @@ 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 }) diff --git a/packages/@uppy/companion/src/server/controllers/thumbnail.js b/packages/@uppy/companion/src/server/controllers/thumbnail.js index be4b4d5070..0994964ed2 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 3d76f46778..cc60c824f3 100644 --- a/packages/@uppy/companion/src/server/helpers/jwt.js +++ b/packages/@uppy/companion/src/server/helpers/jwt.js @@ -5,11 +5,11 @@ const EXPIRY = 60 * 60 * 24 // one day (24 hrs) /** * - * @param {*} payload + * @param {*} data * @param {string} secret */ -module.exports.generateToken = (payload, secret) => { - return jwt.sign({ data: payload }, secret, { expiresIn: EXPIRY }) +module.exports.generateToken = (data, secret) => { + return jwt.sign({ data }, secret, { expiresIn: EXPIRY }) } /** @@ -18,12 +18,8 @@ module.exports.generateToken = (payload, secret) => { * @param {string} secret */ module.exports.verifyToken = (token, secret) => { - try { - // @ts-ignore - return { payload: jwt.verify(token, secret, {}).data } - } catch (err) { - return { err } - } + // @ts-ignore + return jwt.verify(token, secret, {}).data } /** @@ -35,17 +31,34 @@ module.exports.generateEncryptedToken = (payload, secret) => { return encrypt(module.exports.generateToken(payload, secret), secret) } +/** + * + * @param {*} payload + * @param {string} secret + */ +module.exports.generateEncryptedAuthToken = (payload, secret) => { + return module.exports.generateEncryptedToken(JSON.stringify(payload), secret) +} + /** * * @param {string} token * @param {string} secret */ module.exports.verifyEncryptedToken = (token, secret) => { - try { - return module.exports.verifyToken(decrypt(token, secret), secret) - } catch (err) { - return { err } - } + const ret = module.exports.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) => { + const json = module.exports.verifyEncryptedToken(token, secret) + return JSON.parse(json) } const addToCookies = (res, token, companionOptions, authProvider, prefix) => { diff --git a/packages/@uppy/companion/src/server/middlewares.js b/packages/@uppy/companion/src/server/middlewares.js index 995f04e41c..c398bc33cf 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,16 @@ 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) + if (!payload[providerName]) throw new Error(`Missing token payload for provider ${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 +99,12 @@ 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) + if (!payload[providerName]) throw new Error(`Missing token payload for provider ${providerName} (gentle)`) + 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/credentials.js b/packages/@uppy/companion/src/server/provider/credentials.js index 20c24902fb..02139df27d 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/test/__tests__/callback.js b/packages/@uppy/companion/test/__tests__/callback.js index 71d113f4bf..d1361b8f64 100644 --- a/packages/@uppy/companion/test/__tests__/callback.js +++ b/packages/@uppy/companion/test/__tests__/callback.js @@ -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', () => { diff --git a/packages/@uppy/companion/test/__tests__/companion.js b/packages/@uppy/companion/test/__tests__/companion.js index 457bed0a5b..f614c60969 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 0830e4dc35..74b9abb2a4 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 cc638735e8..1c01608545 100644 --- a/packages/@uppy/companion/test/__tests__/preauth.js +++ b/packages/@uppy/companion/test/__tests__/preauth.js @@ -2,12 +2,8 @@ jest.mock('../../src/server/helpers/jwt', () => { return { generateToken: () => {}, verifyToken: () => {}, - generateEncryptedToken: () => { - return 'dummy token' - }, - verifyEncryptedToken: () => { - return { payload: '' } - }, + generateEncryptedToken: () => 'dummy token', + verifyEncryptedToken: () => '', addToCookies: () => {}, removeFromCookies: () => {}, } diff --git a/packages/@uppy/companion/test/__tests__/provider-manager.js b/packages/@uppy/companion/test/__tests__/provider-manager.js index 3da05943f5..6a15f727b2 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({ diff --git a/packages/@uppy/companion/test/__tests__/providers.js b/packages/@uppy/companion/test/__tests__/providers.js index 05b562911f..84cc871967 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) { From 5b7fa1dfa9fdf847546f286cbf2c0a94ec87226d Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 11 May 2023 21:12:23 +0200 Subject: [PATCH 02/21] de-duplicate uploadRemote by creating a new superclass UploaderPlugin --- packages/@uppy/aws-s3-multipart/src/index.js | 33 ++++------------- packages/@uppy/aws-s3/src/MiniXHRUpload.js | 37 ++------------------ packages/@uppy/aws-s3/src/index.js | 22 ++++++++++-- packages/@uppy/tus/src/index.js | 37 ++++---------------- packages/@uppy/xhr-upload/src/index.js | 31 +++------------- 5 files changed, 40 insertions(+), 120 deletions(-) diff --git a/packages/@uppy/aws-s3-multipart/src/index.js b/packages/@uppy/aws-s3-multipart/src/index.js index 1674c6dd7f..338e594e1a 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 EventTracker from '@uppy/utils/lib/EventTracker' import emitSocketProgress from '@uppy/utils/lib/emitSocketProgress' @@ -313,10 +313,10 @@ class HTTPCommunicationQueue { } } -export default class AwsS3Multipart extends BasePlugin { +export default class AwsS3Multipart extends UploaderPlugin { static VERSION = packageJson.version - #queueRequestSocketToken + queueRequestSocketToken #companionCommunicationQueue @@ -369,7 +369,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.queueRequestSocketToken = this.requests.wrapPromiseFunction(this.#requestSocketToken, { priority: -1 }) } [Symbol.for('uppy test: getClient')] () { return this.#client } @@ -683,28 +683,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) { - this.resetUploaderReferences(file.id) - - try { - if (file.serverToken) { - return await this.connectToServerSocket(file) - } - const serverToken = await this.#queueRequestSocketToken(file) - - 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) { - this.uppy.setFileState(file.id, { serverToken: undefined }) - this.uppy.emit('upload-error', file, err) - throw err - } - } - async connectToServerSocket (file) { return new Promise((resolve, reject) => { let queuedRequest @@ -827,7 +805,8 @@ export default class AwsS3Multipart extends BasePlugin { const promises = filesFiltered.map((file) => { if (file.isRemote) { - return this.#uploadRemote(file) + this.resetUploaderReferences(file.id) + return this.uploadRemoteFile(file) } return this.#uploadFile(file) }) diff --git a/packages/@uppy/aws-s3/src/MiniXHRUpload.js b/packages/@uppy/aws-s3/src/MiniXHRUpload.js index c4dc7dfa6e..66adec1ad2 100644 --- a/packages/@uppy/aws-s3/src/MiniXHRUpload.js +++ b/packages/@uppy/aws-s3/src/MiniXHRUpload.js @@ -53,7 +53,7 @@ function createFormDataUpload (file, opts) { const createBareUpload = file => file.data export default class MiniXHRUpload { - #queueRequestSocketToken + queueRequestSocketToken constructor (uppy, opts) { this.uppy = uppy @@ -68,7 +68,7 @@ export default class MiniXHRUpload { this.uploaderEvents = Object.create(null) this.i18n = opts.i18n - this.#queueRequestSocketToken = this.requests.wrapPromiseFunction(this.#requestSocketToken, { priority: -1 }) + this.queueRequestSocketToken = this.requests.wrapPromiseFunction(this.#requestSocketToken, { priority: -1 }) } #getOptions (file) { @@ -89,16 +89,6 @@ export default class MiniXHRUpload { return opts } - uploadFile (id, current, total) { - const file = this.uppy.getFile(id) - if (file.error) { - throw new Error(file.error) - } else if (file.isRemote) { - return this.#uploadRemoteFile(file, current, total) - } - 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) @@ -115,7 +105,7 @@ export default class MiniXHRUpload { }) } - #uploadLocalFile (file, current, total) { + uploadLocalFile (file, current, total) { const opts = this.#getOptions(file) this.uppy.log(`uploading ${current} of ${total}`) @@ -279,27 +269,6 @@ export default class MiniXHRUpload { 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) { - // 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) - - 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) { - this.uppy.setFileState(file.id, { serverToken: undefined }) - this.uppy.emit('upload-error', file, err) - throw err - } - } - async connectToServerSocket (file) { return new Promise((resolve, reject) => { const opts = this.#getOptions(file) diff --git a/packages/@uppy/aws-s3/src/index.js b/packages/@uppy/aws-s3/src/index.js index 5200796372..f8651c5f6a 100644 --- a/packages/@uppy/aws-s3/src/index.js +++ b/packages/@uppy/aws-s3/src/index.js @@ -25,7 +25,7 @@ * 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 { RateLimitedQueue, internalRateLimitedQueue } from '@uppy/utils/lib/RateLimitedQueue' import { RequestClient } from '@uppy/companion-client' import { filterNonFailedFiles, filterFilesToEmitUploadStarted } from '@uppy/utils/lib/fileFilters' @@ -102,7 +102,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 @@ -241,6 +241,24 @@ export default class AwsS3 extends BasePlugin { return Promise.resolve() } + connectToServerSocket (file) { + return this.#uploader.connectToServerSocket(file) + } + + queueRequestSocketToken (file) { + return this.#uploader.queueRequestSocketToken(file) + } + + uploadFile (id, current, total) { + const file = this.uppy.getFile(id) + if (file.error) { + throw new Error(file.error) + } else if (file.isRemote) { + return this.uploadRemoteFile(file, current, total) + } + return this.#uploader.uploadLocalFile(file, current, total) + } + install () { const { uppy } = this uppy.addPreProcessor(this.#setCompanionHeaders) diff --git a/packages/@uppy/tus/src/index.js b/packages/@uppy/tus/src/index.js index 0d254d65e9..5667588580 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,12 +52,12 @@ const tusDefaultOptions = { /** * Tus resumable file uploader */ -export default class Tus extends BasePlugin { +export default class Tus extends UploaderPlugin { static VERSION = packageJson.version #retryDelayIterator - #queueRequestSocketToken + queueRequestSocketToken /** * @param {Uppy} uppy @@ -102,7 +102,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.queueRequestSocketToken = this.requests.wrapPromiseFunction(this.#requestSocketToken, { priority: -1 }) } handleResetProgress () { @@ -453,32 +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) { - this.resetUploaderReferences(file.id) - - try { - if (file.serverToken) { - return await this.connectToServerSocket(file) - } - const serverToken = await this.#queueRequestSocketToken(file) - - 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) { - this.uppy.setFileState(file.id, { serverToken: undefined }) - this.uppy.emit('upload-error', file, err) - throw err - } - } - /** * See the comment on the upload() method. * @@ -732,7 +706,8 @@ export default class Tus extends BasePlugin { const total = files.length if (file.isRemote) { - return this.#uploadRemote(file, current, total) + this.resetUploaderReferences(file.id) + return this.uploadRemoteFile(file, current, total) } return this.#upload(file, current, total) })) diff --git a/packages/@uppy/xhr-upload/src/index.js b/packages/@uppy/xhr-upload/src/index.js index b95d71f195..2dd674df51 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,11 +46,11 @@ 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 + queueRequestSocketToken constructor (uppy, opts) { super(uppy, opts) @@ -129,7 +129,7 @@ export default class XHRUpload extends BasePlugin { } this.uploaderEvents = Object.create(null) - this.#queueRequestSocketToken = this.requests.wrapPromiseFunction(this.#requestSocketToken, { priority: -1 }) + this.queueRequestSocketToken = this.requests.wrapPromiseFunction(this.#requestSocketToken, { priority: -1 }) } getOptions (file) { @@ -367,27 +367,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) { - // 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) - - 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) { - this.uppy.setFileState(file.id, { serverToken: undefined }) - this.uppy.emit('upload-error', file, err) - throw err - } - } - async connectToServerSocket (file) { return new Promise((resolve, reject) => { const opts = this.getOptions(file) @@ -589,7 +568,7 @@ export default class XHRUpload extends BasePlugin { const total = files.length if (file.isRemote) { - return this.#uploadRemote(file, current, total) + return this.uploadRemoteFile(file, current, total) } return this.#upload(file, current, total) })) From 4ae8cce78e6537c10bbfdb445a64547f04e31181 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 11 May 2023 21:33:55 +0200 Subject: [PATCH 03/21] pull out requestSocketToken from MiniXHRUpload --- packages/@uppy/aws-s3/src/MiniXHRUpload.js | 40 +++------------------- packages/@uppy/aws-s3/src/index.js | 32 +++++++++++++++-- 2 files changed, 33 insertions(+), 39 deletions(-) diff --git a/packages/@uppy/aws-s3/src/MiniXHRUpload.js b/packages/@uppy/aws-s3/src/MiniXHRUpload.js index 66adec1ad2..bc011f213a 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 EventTracker from '@uppy/utils/lib/EventTracker' @@ -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 @@ -106,7 +102,7 @@ export default class MiniXHRUpload { } uploadLocalFile (file, current, total) { - const opts = this.#getOptions(file) + const opts = this.getOptions(file) this.uppy.log(`uploading ${current} of ${total}`) return new Promise((resolve, reject) => { @@ -241,37 +237,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 - } - 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 f8651c5f6a..7067b5ee0f 100644 --- a/packages/@uppy/aws-s3/src/index.js +++ b/packages/@uppy/aws-s3/src/index.js @@ -27,7 +27,7 @@ import UploaderPlugin from '@uppy/core/lib/UploaderPlugin.js' 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' @@ -138,6 +138,8 @@ export default class AwsS3 extends UploaderPlugin { this.#client = new RequestClient(uppy, opts) this.#requests = new RateLimitedQueue(this.opts.limit) + + this.queueRequestSocketToken = this.#requests.wrapPromiseFunction(this.#requestSocketToken, { priority: -1 }) } [Symbol.for('uppy test: getClient')] () { return this.#client } @@ -245,8 +247,32 @@ export default class AwsS3 extends UploaderPlugin { return this.#uploader.connectToServerSocket(file) } - queueRequestSocketToken (file) { - return this.#uploader.queueRequestSocketToken(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) { From 7fc0c183bf8dd95c6bc5f7ea2a535b0766bd8b03 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 11 May 2023 23:20:29 +0200 Subject: [PATCH 04/21] add class UploaderPlugin --- packages/@uppy/core/src/UploaderPlugin.js | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 packages/@uppy/core/src/UploaderPlugin.js diff --git a/packages/@uppy/core/src/UploaderPlugin.js b/packages/@uppy/core/src/UploaderPlugin.js new file mode 100644 index 0000000000..b7e465157e --- /dev/null +++ b/packages/@uppy/core/src/UploaderPlugin.js @@ -0,0 +1,22 @@ +import BasePlugin from './BasePlugin.js' + +export default class UploaderPlugin extends BasePlugin { + async uploadRemoteFile (file) { + // 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) + + 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) { + this.uppy.setFileState(file.id, { serverToken: undefined }) + this.uppy.emit('upload-error', file, err) + throw err + } + } +} From d9caa6efd1f566ada618f97d72a92967465f956b Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Fri, 12 May 2023 09:41:58 +0200 Subject: [PATCH 05/21] refactor --- .../src/server/controllers/deauth-callback.js | 8 ++------ .../@uppy/companion/src/server/controllers/list.js | 8 ++------ .../@uppy/companion/src/server/controllers/logout.js | 8 ++------ packages/@uppy/companion/src/server/helpers/upload.js | 8 ++------ .../@uppy/companion/src/server/provider/error.d.ts | 4 ++++ packages/@uppy/companion/src/server/provider/error.js | 11 ++++++++++- 6 files changed, 22 insertions(+), 25 deletions(-) diff --git a/packages/@uppy/companion/src/server/controllers/deauth-callback.js b/packages/@uppy/companion/src/server/controllers/deauth-callback.js index 0157b90673..de5f39483c 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/list.js b/packages/@uppy/companion/src/server/controllers/list.js index 232460fd9f..284660c1be 100644 --- a/packages/@uppy/companion/src/server/controllers/list.js +++ b/packages/@uppy/companion/src/server/controllers/list.js @@ -1,4 +1,4 @@ -const { errorToResponse } = require('../provider/error') +const { respondWithError } = require('../provider/error') async function list ({ query, params, companion }, res, next) { const { accessToken } = companion.providerTokens @@ -7,11 +7,7 @@ async function list ({ query, params, companion }, res, next) { 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 3c576ae72c..1ec87e303f 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') /** * @@ -31,11 +31,7 @@ async function logout (req, res, next) { 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/helpers/upload.js b/packages/@uppy/companion/src/server/helpers/upload.js index 60d16355b1..8e1850b988 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/provider/error.d.ts b/packages/@uppy/companion/src/server/provider/error.d.ts index 45ba78db60..237beefc10 100644 --- a/packages/@uppy/companion/src/server/provider/error.d.ts +++ b/packages/@uppy/companion/src/server/provider/error.d.ts @@ -4,6 +4,8 @@ // // We could try removing this file when we upgrade to 4.1 :) +import { bool } from 'aws-sdk/clients/signer' + export class ProviderApiError extends Error { constructor(message: string, statusCode: number) } @@ -12,3 +14,5 @@ export class ProviderAuthError extends ProviderApiError { } export function errorToResponse(anyError: Error): { code: number, message: string } + +export function respondWithError(anyError: Error, res: any): bool diff --git a/packages/@uppy/companion/src/server/provider/error.js b/packages/@uppy/companion/src/server/provider/error.js index 5d053ab5d9..cf35932198 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 } From e2e5f82735cb19b10dae920fd160e4c821509107 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Fri, 12 May 2023 09:59:38 +0200 Subject: [PATCH 06/21] refactor --- .../src/server/provider/providerErrors.js | 32 ++++++++----------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/packages/@uppy/companion/src/server/provider/providerErrors.js b/packages/@uppy/companion/src/server/provider/providerErrors.js index 6234689a5d..32a61d5c3e 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,23 +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 }) - logger.error(err2, tag) - throw err2 + const { response } = err + + if (response) { + // @ts-ignore + if (isAuthError(response)) return new ProviderAuthError() + + return new ProviderApiError(getErrorMessage(response), response.statusCode) + } + + logger.error(err, tag) + + throw err } } From 30d47353eac887204c4b7f4482481e50d9c89772 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Fri, 12 May 2023 10:01:19 +0200 Subject: [PATCH 07/21] refactor/reuse --- packages/@uppy/companion/src/server/helpers/jwt.js | 6 ++++-- packages/@uppy/companion/src/server/middlewares.js | 8 ++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/@uppy/companion/src/server/helpers/jwt.js b/packages/@uppy/companion/src/server/helpers/jwt.js index cc60c824f3..25c0273164 100644 --- a/packages/@uppy/companion/src/server/helpers/jwt.js +++ b/packages/@uppy/companion/src/server/helpers/jwt.js @@ -56,9 +56,11 @@ module.exports.verifyEncryptedToken = (token, secret) => { * @param {string} token * @param {string} secret */ -module.exports.verifyEncryptedAuthToken = (token, secret) => { +module.exports.verifyEncryptedAuthToken = (token, secret, providerName) => { const json = module.exports.verifyEncryptedToken(token, secret) - return JSON.parse(json) + 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) => { diff --git a/packages/@uppy/companion/src/server/middlewares.js b/packages/@uppy/companion/src/server/middlewares.js index c398bc33cf..c344ff1602 100644 --- a/packages/@uppy/companion/src/server/middlewares.js +++ b/packages/@uppy/companion/src/server/middlewares.js @@ -83,8 +83,7 @@ exports.verifyToken = (req, res, next) => { } const { providerName } = req.params try { - const payload = tokenService.verifyEncryptedAuthToken(token, req.companion.options.secret) - if (!payload[providerName]) throw new Error(`Missing token payload for provider ${providerName}`) + const payload = tokenService.verifyEncryptedAuthToken(token, req.companion.options.secret, providerName) req.companion.allProvidersTokens = payload req.companion.providerTokens = payload[providerName] } catch (err) { @@ -100,8 +99,9 @@ exports.gentleVerifyToken = (req, res, next) => { const { providerName } = req.params if (req.companion.authToken) { try { - const payload = tokenService.verifyEncryptedAuthToken(req.companion.authToken, req.companion.options.secret) - if (!payload[providerName]) throw new Error(`Missing token payload for provider ${providerName} (gentle)`) + 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) From a78efc789796c1a6002355acf2c7e7175495d7c4 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Fri, 12 May 2023 10:50:04 +0200 Subject: [PATCH 08/21] refactor/make getAuthToken private --- packages/@uppy/aws-s3/src/index.js | 2 ++ packages/@uppy/companion-client/src/Provider.js | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/@uppy/aws-s3/src/index.js b/packages/@uppy/aws-s3/src/index.js index 7067b5ee0f..c36c8c4c6b 100644 --- a/packages/@uppy/aws-s3/src/index.js +++ b/packages/@uppy/aws-s3/src/index.js @@ -105,6 +105,8 @@ let warnedSuccessActionStatus = false export default class AwsS3 extends UploaderPlugin { static VERSION = packageJson.version + queueRequestSocketToken + #client #requests diff --git a/packages/@uppy/companion-client/src/Provider.js b/packages/@uppy/companion-client/src/Provider.js index b30b6b4f46..136fc13303 100644 --- a/packages/@uppy/companion-client/src/Provider.js +++ b/packages/@uppy/companion-client/src/Provider.js @@ -20,7 +20,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,11 +43,11 @@ 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) } From cc6ee4c899c6fe6d215f4b398eecbfd28a7a0fe0 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Fri, 12 May 2023 11:15:44 +0200 Subject: [PATCH 09/21] fix bug --- .../companion/src/server/provider/providerErrors.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/@uppy/companion/src/server/provider/providerErrors.js b/packages/@uppy/companion/src/server/provider/providerErrors.js index 32a61d5c3e..bc38093550 100644 --- a/packages/@uppy/companion/src/server/provider/providerErrors.js +++ b/packages/@uppy/companion/src/server/provider/providerErrors.js @@ -20,16 +20,17 @@ async function withProviderErrorHandling ({ fn, tag, providerName, isAuthError = } catch (err) { const { response } = err + let err2 = err + if (response) { // @ts-ignore - if (isAuthError(response)) return new ProviderAuthError() - - return new ProviderApiError(getErrorMessage(response), response.statusCode) + if (isAuthError(response)) err2 = new ProviderAuthError() + else err2 = new ProviderApiError(getErrorMessage(response), response.statusCode) } - logger.error(err, tag) + logger.error(err2, tag) - throw err + throw err2 } } From 8c19821f73073e938987c2426f0e1d629839b6cc Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Fri, 12 May 2023 11:29:23 +0200 Subject: [PATCH 10/21] implement refresh token for dropbox and google drive closes #2721 --- .../@uppy/companion-client/src/Provider.js | 49 ++++++++++++++++--- .../companion-client/src/RequestClient.js | 8 +-- packages/@uppy/companion/src/companion.js | 1 + packages/@uppy/companion/src/config/grant.js | 2 + .../companion/src/server/controllers/index.js | 1 + .../src/server/controllers/refresh-token.js | 49 +++++++++++++++++++ .../companion/src/server/provider/Provider.js | 9 ++++ .../src/server/provider/drive/index.js | 11 +++++ .../src/server/provider/dropbox/index.js | 12 ++++- 9 files changed, 131 insertions(+), 11 deletions(-) create mode 100644 packages/@uppy/companion/src/server/controllers/refresh-token.js diff --git a/packages/@uppy/companion-client/src/Provider.js b/packages/@uppy/companion-client/src/Provider.js index 136fc13303..e12238886a 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 @@ -51,6 +53,10 @@ export default class Provider extends RequestClient { 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,43 @@ 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}` } + 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 +134,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 5e32d83209..96be3a721c 100644 --- a/packages/@uppy/companion-client/src/RequestClient.js +++ b/packages/@uppy/companion-client/src/RequestClient.js @@ -150,7 +150,7 @@ export default class RequestClient { })) } - 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), { @@ -172,20 +172,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 77ba22bdcb..cbaf6a2a72 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 c322b4004d..1e77be22ac 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/index.js b/packages/@uppy/companion/src/server/controllers/index.js index b9a499eecf..4410eec3b9 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/refresh-token.js b/packages/@uppy/companion/src/server/controllers/refresh-token.js new file mode 100644 index 0000000000..ee3789f35f --- /dev/null +++ b/packages/@uppy/companion/src/server/controllers/refresh-token.js @@ -0,0 +1,49 @@ +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, + ) + + res.send({ uppyAuthToken }) + } catch (err) { + if (respondWithError(err, res)) return + next(err) + } +} + +module.exports = refreshToken diff --git a/packages/@uppy/companion/src/server/provider/Provider.js b/packages/@uppy/companion/src/server/provider/Provider.js index 453b0ebadd..2268b0aec4 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/drive/index.js b/packages/@uppy/companion/src/server/provider/drive/index.js index d15a7ed376..903636c9d6 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 848c6b202c..50b4989ea6 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, From 06b96d8c408f11d533e3d286f423ecc2d2f1a596 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Fri, 12 May 2023 11:45:07 +0200 Subject: [PATCH 11/21] fix test --- packages/@uppy/companion/test/__tests__/provider-manager.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/@uppy/companion/test/__tests__/provider-manager.js b/packages/@uppy/companion/test/__tests__/provider-manager.js index 6a15f727b2..de9cf2b2a8 100644 --- a/packages/@uppy/companion/test/__tests__/provider-manager.js +++ b/packages/@uppy/companion/test/__tests__/provider-manager.js @@ -76,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', From 2d77c4b9e8a9d89459ee5a12bf0f205da5e509e2 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Fri, 12 May 2023 23:48:04 +0200 Subject: [PATCH 12/21] also update auth token cookie when refreshing token --- .../src/server/controllers/callback.js | 3 +++ .../src/server/controllers/refresh-token.js | 2 ++ .../src/server/controllers/send-token.js | 5 ----- .../@uppy/companion/src/server/helpers/jwt.js | 15 ++++++--------- .../companion/test/__tests__/callback.js | 19 +++++++++++++++---- .../@uppy/companion/test/__tests__/preauth.js | 2 +- packages/@uppy/companion/test/mockserver.js | 4 +++- 7 files changed, 30 insertions(+), 20 deletions(-) diff --git a/packages/@uppy/companion/src/server/controllers/callback.js b/packages/@uppy/companion/src/server/controllers/callback.js index 82c27c809b..20edc0181a 100644 --- a/packages/@uppy/companion/src/server/controllers/callback.js +++ b/packages/@uppy/companion/src/server/controllers/callback.js @@ -50,5 +50,8 @@ module.exports = function callback (req, res, next) { // eslint-disable-line no- 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/refresh-token.js b/packages/@uppy/companion/src/server/controllers/refresh-token.js index ee3789f35f..99203ebe3c 100644 --- a/packages/@uppy/companion/src/server/controllers/refresh-token.js +++ b/packages/@uppy/companion/src/server/controllers/refresh-token.js @@ -39,6 +39,8 @@ async function refreshToken (req, res, next) { req.companion.allProvidersTokens, req.companion.options.secret, ) + tokenService.addToCookiesIfNeeded(req, res, uppyAuthToken) + res.send({ uppyAuthToken }) } catch (err) { if (respondWithError(err, res)) return diff --git a/packages/@uppy/companion/src/server/controllers/send-token.js b/packages/@uppy/companion/src/server/controllers/send-token.js index 2ec6280942..17cc4c1601 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/helpers/jwt.js b/packages/@uppy/companion/src/server/helpers/jwt.js index 25c0273164..209c37fc41 100644 --- a/packages/@uppy/companion/src/server/helpers/jwt.js +++ b/packages/@uppy/companion/src/server/helpers/jwt.js @@ -28,6 +28,7 @@ module.exports.verifyToken = (token, secret) => { * @param {string} secret */ module.exports.generateEncryptedToken = (payload, secret) => { + // return payload // for easier debugging return encrypt(module.exports.generateToken(payload, secret), secret) } @@ -83,15 +84,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') + } } /** diff --git a/packages/@uppy/companion/test/__tests__/callback.js b/packages/@uppy/companion/test/__tests__/callback.js index d1361b8f64..4eee609aa1 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'), @@ -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__/preauth.js b/packages/@uppy/companion/test/__tests__/preauth.js index 1c01608545..0be49f5575 100644 --- a/packages/@uppy/companion/test/__tests__/preauth.js +++ b/packages/@uppy/companion/test/__tests__/preauth.js @@ -4,7 +4,7 @@ jest.mock('../../src/server/helpers/jwt', () => { verifyToken: () => {}, generateEncryptedToken: () => 'dummy token', verifyEncryptedToken: () => '', - addToCookies: () => {}, + addToCookiesIfNeeded: () => {}, removeFromCookies: () => {}, } }) diff --git a/packages/@uppy/companion/test/mockserver.js b/packages/@uppy/companion/test/mockserver.js index 82b36e2447..7bdf519f8d 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() }) From 0aac0e5850c3da6442c3b0d7ec9680906b30af3b Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Fri, 12 May 2023 23:56:32 +0200 Subject: [PATCH 13/21] fix build error on node 14 --- packages/@uppy/companion/src/server/controllers/callback.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@uppy/companion/src/server/controllers/callback.js b/packages/@uppy/companion/src/server/controllers/callback.js index 20edc0181a..dcd7dadd22 100644 --- a/packages/@uppy/companion/src/server/controllers/callback.js +++ b/packages/@uppy/companion/src/server/controllers/callback.js @@ -41,7 +41,7 @@ module.exports = function callback (req, res, next) { // eslint-disable-line no- return res.status(400).send(closePageHtml(origin)) } - req.companion.allProvidersTokens ||= {} + 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 From 33018dda770b48b2d11462c6f4d25c01058730d6 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 8 Jun 2023 18:57:41 +0100 Subject: [PATCH 14/21] increase auth token expiry to workaround expiry --- .../@uppy/companion/src/server/helpers/jwt.js | 28 ++++++++++++++----- .../@uppy/companion/test/__tests__/preauth.js | 2 -- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/packages/@uppy/companion/src/server/helpers/jwt.js b/packages/@uppy/companion/src/server/helpers/jwt.js index 209c37fc41..b1a9b75dfd 100644 --- a/packages/@uppy/companion/src/server/helpers/jwt.js +++ b/packages/@uppy/companion/src/server/helpers/jwt.js @@ -1,14 +1,28 @@ 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 {*} data * @param {string} secret */ -module.exports.generateToken = (data, secret) => { +const generateToken = (data, secret) => { return jwt.sign({ data }, secret, { expiresIn: EXPIRY }) } @@ -17,7 +31,7 @@ module.exports.generateToken = (data, secret) => { * @param {string} token * @param {string} secret */ -module.exports.verifyToken = (token, secret) => { +const verifyToken = (token, secret) => { // @ts-ignore return jwt.verify(token, secret, {}).data } @@ -29,7 +43,7 @@ module.exports.verifyToken = (token, secret) => { */ module.exports.generateEncryptedToken = (payload, secret) => { // return payload // for easier debugging - return encrypt(module.exports.generateToken(payload, secret), secret) + return encrypt(generateToken(payload, secret), secret) } /** @@ -47,7 +61,7 @@ module.exports.generateEncryptedAuthToken = (payload, secret) => { * @param {string} secret */ module.exports.verifyEncryptedToken = (token, secret) => { - const ret = module.exports.verifyToken(decrypt(token, secret), secret) + const ret = verifyToken(decrypt(token, secret), secret) if (!ret) throw new Error('No payload') return ret } @@ -66,7 +80,7 @@ module.exports.verifyEncryptedAuthToken = (token, secret, providerName) => { const addToCookies = (res, token, companionOptions, authProvider, prefix) => { const cookieOptions = { - maxAge: 1000 * EXPIRY, // would expire after one day (24 hrs) + maxAge: EXPIRY_MS, httpOnly: true, } @@ -99,7 +113,7 @@ module.exports.addToCookiesIfNeeded = (req, res, uppyAuthToken) => { */ 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/test/__tests__/preauth.js b/packages/@uppy/companion/test/__tests__/preauth.js index 0be49f5575..06d8cd2e79 100644 --- a/packages/@uppy/companion/test/__tests__/preauth.js +++ b/packages/@uppy/companion/test/__tests__/preauth.js @@ -1,7 +1,5 @@ jest.mock('../../src/server/helpers/jwt', () => { return { - generateToken: () => {}, - verifyToken: () => {}, generateEncryptedToken: () => 'dummy token', verifyEncryptedToken: () => '', addToCookiesIfNeeded: () => {}, From e3cab0554c943cf4583e9624b700bbaa5a1a3096 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 8 Jun 2023 19:18:03 +0100 Subject: [PATCH 15/21] Update packages/@uppy/companion-client/src/RequestClient.js Co-authored-by: Antoine du Hamel --- packages/@uppy/companion-client/src/RequestClient.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/@uppy/companion-client/src/RequestClient.js b/packages/@uppy/companion-client/src/RequestClient.js index 96be3a721c..69fcc882a6 100644 --- a/packages/@uppy/companion-client/src/RequestClient.js +++ b/packages/@uppy/companion-client/src/RequestClient.js @@ -150,6 +150,7 @@ export default class RequestClient { })) } + /** @protected */ async request ({ path, method = 'GET', data, skipPostResponse, signal }) { try { const headers = await this.preflightAndHeaders(path) From 8ceb9874b79483e5ec9428d90c0b55b87078abc1 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 8 Jun 2023 19:19:47 +0100 Subject: [PATCH 16/21] make queueRequestSocketToken private --- packages/@uppy/aws-s3-multipart/src/index.js | 4 +--- packages/@uppy/aws-s3/src/index.js | 4 +--- packages/@uppy/core/src/UploaderPlugin.js | 9 ++++++++- packages/@uppy/tus/src/index.js | 4 +--- packages/@uppy/xhr-upload/src/index.js | 4 +--- 5 files changed, 12 insertions(+), 13 deletions(-) diff --git a/packages/@uppy/aws-s3-multipart/src/index.js b/packages/@uppy/aws-s3-multipart/src/index.js index 338e594e1a..69d470b60b 100644 --- a/packages/@uppy/aws-s3-multipart/src/index.js +++ b/packages/@uppy/aws-s3-multipart/src/index.js @@ -316,8 +316,6 @@ class HTTPCommunicationQueue { export default class AwsS3Multipart extends UploaderPlugin { static VERSION = packageJson.version - queueRequestSocketToken - #companionCommunicationQueue #client @@ -369,7 +367,7 @@ export default class AwsS3Multipart extends UploaderPlugin { 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 } diff --git a/packages/@uppy/aws-s3/src/index.js b/packages/@uppy/aws-s3/src/index.js index c36c8c4c6b..2083c0c642 100644 --- a/packages/@uppy/aws-s3/src/index.js +++ b/packages/@uppy/aws-s3/src/index.js @@ -105,8 +105,6 @@ let warnedSuccessActionStatus = false export default class AwsS3 extends UploaderPlugin { static VERSION = packageJson.version - queueRequestSocketToken - #client #requests @@ -141,7 +139,7 @@ export default class AwsS3 extends UploaderPlugin { this.#client = new RequestClient(uppy, opts) this.#requests = new RateLimitedQueue(this.opts.limit) - 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 } diff --git a/packages/@uppy/core/src/UploaderPlugin.js b/packages/@uppy/core/src/UploaderPlugin.js index b7e465157e..25da5a0447 100644 --- a/packages/@uppy/core/src/UploaderPlugin.js +++ b/packages/@uppy/core/src/UploaderPlugin.js @@ -1,13 +1,20 @@ import BasePlugin from './BasePlugin.js' export default class UploaderPlugin extends BasePlugin { + #queueRequestSocketToken + + /** @protected */ + setQueueRequestSocketToken (token) { + this.#queueRequestSocketToken = token + } + async uploadRemoteFile (file) { // 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) + const serverToken = await this.#queueRequestSocketToken(file) if (!this.uppy.getState().files[file.id]) return undefined diff --git a/packages/@uppy/tus/src/index.js b/packages/@uppy/tus/src/index.js index 5667588580..f4eea428c4 100644 --- a/packages/@uppy/tus/src/index.js +++ b/packages/@uppy/tus/src/index.js @@ -57,8 +57,6 @@ export default class Tus extends UploaderPlugin { #retryDelayIterator - queueRequestSocketToken - /** * @param {Uppy} uppy * @param {TusOptions} opts @@ -102,7 +100,7 @@ export default class Tus extends UploaderPlugin { 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 () { diff --git a/packages/@uppy/xhr-upload/src/index.js b/packages/@uppy/xhr-upload/src/index.js index 2dd674df51..b88c17f1ad 100644 --- a/packages/@uppy/xhr-upload/src/index.js +++ b/packages/@uppy/xhr-upload/src/index.js @@ -50,8 +50,6 @@ 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 UploaderPlugin { } 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) { From c98cf60bddf0ac92dc21770ed69bda68af3f2fb1 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Fri, 9 Jun 2023 09:02:27 +0100 Subject: [PATCH 17/21] rename arg --- packages/@uppy/core/src/UploaderPlugin.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@uppy/core/src/UploaderPlugin.js b/packages/@uppy/core/src/UploaderPlugin.js index 25da5a0447..e385f241ba 100644 --- a/packages/@uppy/core/src/UploaderPlugin.js +++ b/packages/@uppy/core/src/UploaderPlugin.js @@ -4,8 +4,8 @@ export default class UploaderPlugin extends BasePlugin { #queueRequestSocketToken /** @protected */ - setQueueRequestSocketToken (token) { - this.#queueRequestSocketToken = token + setQueueRequestSocketToken (fn) { + this.#queueRequestSocketToken = fn } async uploadRemoteFile (file) { From e4b645993a85ff9458dc4931d4fba6b117c69fa4 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 20 Jun 2023 14:34:14 +0200 Subject: [PATCH 18/21] fix lint --- packages/@uppy/companion/src/server/provider/error.d.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/@uppy/companion/src/server/provider/error.d.ts b/packages/@uppy/companion/src/server/provider/error.d.ts index 237beefc10..29ce604bcf 100644 --- a/packages/@uppy/companion/src/server/provider/error.d.ts +++ b/packages/@uppy/companion/src/server/provider/error.d.ts @@ -4,8 +4,6 @@ // // We could try removing this file when we upgrade to 4.1 :) -import { bool } from 'aws-sdk/clients/signer' - export class ProviderApiError extends Error { constructor(message: string, statusCode: number) } @@ -15,4 +13,4 @@ export class ProviderAuthError extends ProviderApiError { export function errorToResponse(anyError: Error): { code: number, message: string } -export function respondWithError(anyError: Error, res: any): bool +export function respondWithError(anyError: Error, res: any): boolean From 06d55a51d066a1408fda19ad88b7978ee805edb9 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 21 Jun 2023 12:44:54 +0200 Subject: [PATCH 19/21] log error message --- packages/@uppy/core/src/Uppy.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@uppy/core/src/Uppy.js b/packages/@uppy/core/src/Uppy.js index f9644bb09f..c597c97efb 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]) From e32ff5fc916419965c869f54541947780655cf27 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Wed, 21 Jun 2023 12:45:09 +0200 Subject: [PATCH 20/21] fix s3 --- packages/@uppy/aws-s3/src/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@uppy/aws-s3/src/index.js b/packages/@uppy/aws-s3/src/index.js index 383ca90ed7..20d4437ad7 100644 --- a/packages/@uppy/aws-s3/src/index.js +++ b/packages/@uppy/aws-s3/src/index.js @@ -230,7 +230,7 @@ export default class AwsS3 extends UploaderPlugin { xhrUpload: xhrOpts, }) - return this.#uploader.uploadFile(file.id, index, numberOfFiles) + return this.uploadFile(file.id, index, numberOfFiles) }).catch((error) => { delete paramsPromises[id] From 68905bc928c8b41f3ed4316a76d935e0e1f7f285 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 22 Jun 2023 09:50:21 +0200 Subject: [PATCH 21/21] Update packages/@uppy/companion-client/src/Provider.js Co-authored-by: Antoine du Hamel --- packages/@uppy/companion-client/src/Provider.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/@uppy/companion-client/src/Provider.js b/packages/@uppy/companion-client/src/Provider.js index e12238886a..af50ecd091 100644 --- a/packages/@uppy/companion-client/src/Provider.js +++ b/packages/@uppy/companion-client/src/Provider.js @@ -88,6 +88,7 @@ export default class Provider extends RequestClient { return `${this.hostname}/${this.id}/get/${id}` } + /** @protected */ async request (...args) { await this.#refreshingTokenPromise