From 45b737c9e3fd29344ae26357eedbb2e47ef3dca0 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Sun, 29 Aug 2021 13:00:26 +0700 Subject: [PATCH 01/15] Close window on auth callback error Because simply receiving a "Bad request" is very bad UX For example if the user cancels the auth flow, or doesn't give companion access it's better to go back --- .../companion/src/server/controllers/callback.js | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/@uppy/companion/src/server/controllers/callback.js b/packages/@uppy/companion/src/server/controllers/callback.js index 23ba3bbc7b..e9d4c1fbab 100644 --- a/packages/@uppy/companion/src/server/controllers/callback.js +++ b/packages/@uppy/companion/src/server/controllers/callback.js @@ -4,6 +4,18 @@ const tokenService = require('../helpers/jwt') const logger = require('../logger') +const closePageHtml = ` + + + + + + + + ` + /** * * @param {object} req @@ -27,5 +39,6 @@ module.exports = function callback (req, res, next) { // eslint-disable-line no- logger.debug(`Did not receive access token for provider ${providerName}`, null, req.id) logger.debug(grant.response, 'callback.oauth.resp', req.id) - return res.sendStatus(400) + + return res.send(closePageHtml) } From 4d168f99a93f4a0d47d04e5bcdc1425565deb5e8 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 30 Aug 2021 21:46:37 +0700 Subject: [PATCH 02/15] Update packages/@uppy/companion/src/server/controllers/callback.js Co-authored-by: Antoine du Hamel --- 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 e9d4c1fbab..aed4504834 100644 --- a/packages/@uppy/companion/src/server/controllers/callback.js +++ b/packages/@uppy/companion/src/server/controllers/callback.js @@ -13,7 +13,7 @@ const closePageHtml = ` window.close() - + Authentication failed. ` /** From fe300c4588efaccc0ffec94ff5fa10514405c0b6 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 30 Aug 2021 22:11:06 +0700 Subject: [PATCH 03/15] Update packages/@uppy/companion/src/server/controllers/callback.js Co-authored-by: Antoine du Hamel --- 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 aed4504834..fa336e1a67 100644 --- a/packages/@uppy/companion/src/server/controllers/callback.js +++ b/packages/@uppy/companion/src/server/controllers/callback.js @@ -39,6 +39,6 @@ module.exports = function callback (req, res, next) { // eslint-disable-line no- logger.debug(`Did not receive access token for provider ${providerName}`, null, req.id) logger.debug(grant.response, 'callback.oauth.resp', req.id) - +res.statusCode = 400 return res.send(closePageHtml) } From 79e8618b344f7ab1f2c37a3833cc0a98e971d401 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 30 Aug 2021 22:17:31 +0700 Subject: [PATCH 04/15] Update packages/@uppy/companion/src/server/controllers/callback.js --- 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 fa336e1a67..4a97f6c107 100644 --- a/packages/@uppy/companion/src/server/controllers/callback.js +++ b/packages/@uppy/companion/src/server/controllers/callback.js @@ -39,6 +39,6 @@ module.exports = function callback (req, res, next) { // eslint-disable-line no- logger.debug(`Did not receive access token for provider ${providerName}`, null, req.id) logger.debug(grant.response, 'callback.oauth.resp', req.id) -res.statusCode = 400 + res.statusCode = 400 return res.send(closePageHtml) } From a6687825c4b5fe87a7af6b303ce54f7c03299204 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 30 Aug 2021 22:31:33 +0700 Subject: [PATCH 05/15] Update callback.js --- packages/@uppy/companion/src/server/controllers/callback.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/@uppy/companion/src/server/controllers/callback.js b/packages/@uppy/companion/src/server/controllers/callback.js index 4a97f6c107..a966b51874 100644 --- a/packages/@uppy/companion/src/server/controllers/callback.js +++ b/packages/@uppy/companion/src/server/controllers/callback.js @@ -39,6 +39,5 @@ module.exports = function callback (req, res, next) { // eslint-disable-line no- logger.debug(`Did not receive access token for provider ${providerName}`, null, req.id) logger.debug(grant.response, 'callback.oauth.resp', req.id) - res.statusCode = 400 - return res.send(closePageHtml) + return res.status(400).send(closePageHtml) } From 7647045488eb638e440529bb7df34f98946b2421 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 30 Aug 2021 23:02:40 +0700 Subject: [PATCH 06/15] pull out reusable code to getDynamicStateFromRequest --- .../companion/src/server/controllers/oauth-redirect.js | 3 +-- .../@uppy/companion/src/server/controllers/send-token.js | 3 +-- packages/@uppy/companion/src/server/helpers/oauth-state.js | 6 ++++++ packages/@uppy/companion/src/server/provider/credentials.js | 4 ++-- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/@uppy/companion/src/server/controllers/oauth-redirect.js b/packages/@uppy/companion/src/server/controllers/oauth-redirect.js index e037bd4da5..dfcfe518be 100644 --- a/packages/@uppy/companion/src/server/controllers/oauth-redirect.js +++ b/packages/@uppy/companion/src/server/controllers/oauth-redirect.js @@ -15,8 +15,7 @@ module.exports = function oauthRedirect (req, res) { return res.redirect(req.companion.buildURL(`/connect/${authProvider}/callback?${params}`, true)) } - const dynamic = (req.session.grant || {}).dynamic || {} - const { state } = dynamic + const state = oAuthState.getDynamicStateFromRequest(req) if (!state) { return res.status(400).send('Cannot find state in session') } diff --git a/packages/@uppy/companion/src/server/controllers/send-token.js b/packages/@uppy/companion/src/server/controllers/send-token.js index 2b27024058..84c83f570e 100644 --- a/packages/@uppy/companion/src/server/controllers/send-token.js +++ b/packages/@uppy/companion/src/server/controllers/send-token.js @@ -38,8 +38,7 @@ module.exports = function sendToken (req, res, next) { tokenService.addToCookies(res, uppyAuthToken, req.companion.options, req.companion.provider.authProvider) } - const dynamic = (req.session.grant || {}).dynamic || {} - const { state } = dynamic + const state = oAuthState.getDynamicStateFromRequest(req) if (state) { const origin = oAuthState.getFromState(state, 'origin', req.companion.options.secret) const allowedClients = req.companion.options.clients diff --git a/packages/@uppy/companion/src/server/helpers/oauth-state.js b/packages/@uppy/companion/src/server/helpers/oauth-state.js index 73231c63f9..150dbd3e7b 100644 --- a/packages/@uppy/companion/src/server/helpers/oauth-state.js +++ b/packages/@uppy/companion/src/server/helpers/oauth-state.js @@ -27,3 +27,9 @@ const getState = (state, secret) => { const encodedState = decrypt(state, secret) return JSON.parse(atob(encodedState)) } + +module.exports.getDynamicStateFromRequest = (req) => { + const dynamic = (req.session.grant || {}).dynamic || {} + const { state } = dynamic + return state +} diff --git a/packages/@uppy/companion/src/server/provider/credentials.js b/packages/@uppy/companion/src/server/provider/credentials.js index 82921f3800..965556898f 100644 --- a/packages/@uppy/companion/src/server/provider/credentials.js +++ b/packages/@uppy/companion/src/server/provider/credentials.js @@ -27,10 +27,10 @@ exports.getCredentialsOverrideMiddleware = (providers, companionOptions) => { return next() } - const dynamic = (req.session.grant || {}).dynamic || {} + const dynamic = oAuthState.getDynamicStateFromRequest(req) // only use state via session object if user isn't making intial "connect" request. // override param indicates subsequent requests from the oauth flow - const state = override ? dynamic.state : req.query.state + const state = override ? dynamic : req.query.state if (!state) { return next() } From 40bbfeddbf55c7f6db95d307aa6de4a6f6f72dec Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 30 Aug 2021 23:05:31 +0700 Subject: [PATCH 07/15] Signal auth error to client --- .../companion/src/server/controllers/callback.js | 12 +++++++++--- packages/@uppy/core/src/index.js | 1 + .../provider-views/src/ProviderView/ProviderView.js | 8 ++++++++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/packages/@uppy/companion/src/server/controllers/callback.js b/packages/@uppy/companion/src/server/controllers/callback.js index a966b51874..763d4d83e5 100644 --- a/packages/@uppy/companion/src/server/controllers/callback.js +++ b/packages/@uppy/companion/src/server/controllers/callback.js @@ -1,16 +1,20 @@ /** * oAuth callback. Encrypts the access token and sends the new token with the response, */ +const serialize = require('serialize-javascript') + const tokenService = require('../helpers/jwt') const logger = require('../logger') +const oAuthState = require('../helpers/oauth-state') -const closePageHtml = ` +const closePageHtml = (origin) => ` Authentication failed. @@ -39,5 +43,7 @@ module.exports = function callback (req, res, next) { // eslint-disable-line no- logger.debug(`Did not receive access token for provider ${providerName}`, null, req.id) logger.debug(grant.response, 'callback.oauth.resp', req.id) - return res.status(400).send(closePageHtml) + const state = oAuthState.getDynamicStateFromRequest(req) + const origin = state && oAuthState.getFromState(state, 'origin', req.companion.options.secret) + return res.status(400).send(closePageHtml(origin)) } diff --git a/packages/@uppy/core/src/index.js b/packages/@uppy/core/src/index.js index 525189d21b..86b17f2049 100644 --- a/packages/@uppy/core/src/index.js +++ b/packages/@uppy/core/src/index.js @@ -88,6 +88,7 @@ class Uppy { noMoreFilesAllowed: 'Cannot add more files', noDuplicates: 'Cannot add the duplicate file \'%{fileName}\', it already exists', companionError: 'Connection with Companion failed', + authAborted: 'Authentication aborted', companionUnauthorizeHint: 'To unauthorize to your %{provider} account, please go to %{url}', failedToUpload: 'Failed to upload %{file}', noInternetConnection: 'No Internet connection', diff --git a/packages/@uppy/provider-views/src/ProviderView/ProviderView.js b/packages/@uppy/provider-views/src/ProviderView/ProviderView.js index f5f63d6613..4618ae256d 100644 --- a/packages/@uppy/provider-views/src/ProviderView/ProviderView.js +++ b/packages/@uppy/provider-views/src/ProviderView/ProviderView.js @@ -305,6 +305,14 @@ module.exports = class ProviderView { // for older Companion versions that used object references const data = typeof e.data === 'string' ? JSON.parse(e.data) : e.data + if (data.error) { + this.plugin.uppy.log('auth aborted') + const { uppy } = this.plugin + const message = uppy.i18n('authAborted') + uppy.info({ message }, 'warning', 5000) + return + } + if (!data.token) { this.plugin.uppy.log('did not receive token from auth window') return From 98263cf08e7e64e75650ee7f4001e6057f26ef4e Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Mon, 30 Aug 2021 23:36:58 +0700 Subject: [PATCH 08/15] fix test mocking --- packages/@uppy/companion/test/__tests__/callback.js | 7 ++++++- packages/@uppy/companion/test/__tests__/companion.js | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/@uppy/companion/test/__tests__/callback.js b/packages/@uppy/companion/test/__tests__/callback.js index 743ab47fda..391a850bc7 100644 --- a/packages/@uppy/companion/test/__tests__/callback.js +++ b/packages/@uppy/companion/test/__tests__/callback.js @@ -1,11 +1,16 @@ /* global jest:false, test:false, expect:false, describe:false */ -jest.mock('../../src/server/helpers/oauth-state', () => require('../mockoauthstate')()) +const mockOauthState = require('../mockoauthstate')() const request = require('supertest') const tokenService = require('../../src/server/helpers/jwt') const { getServer } = require('../mockserver') +jest.mock('../../src/server/helpers/oauth-state', () => ({ + ...jest.requireActual('../../src/server/helpers/oauth-state'), + ...mockOauthState, +})) + const authServer = getServer() const authData = { dropbox: 'token value', diff --git a/packages/@uppy/companion/test/__tests__/companion.js b/packages/@uppy/companion/test/__tests__/companion.js index aaa3ed5530..4c79b82cdb 100644 --- a/packages/@uppy/companion/test/__tests__/companion.js +++ b/packages/@uppy/companion/test/__tests__/companion.js @@ -1,8 +1,13 @@ /* global jest:false, test:false, expect:false, describe:false */ +const mockOauthState = require('../mockoauthstate')() + jest.mock('tus-js-client') jest.mock('purest') -jest.mock('../../src/server/helpers/oauth-state', () => require('../mockoauthstate')()) +jest.mock('../../src/server/helpers/oauth-state', () => ({ + ...jest.requireActual('../../src/server/helpers/oauth-state'), + ...mockOauthState, +})) const request = require('supertest') const tokenService = require('../../src/server/helpers/jwt') From ac13a68719a78cf3a873c024263c2db59a3548a9 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 31 Aug 2021 00:22:35 +0700 Subject: [PATCH 09/15] npm run build:locale-pack --- packages/@uppy/locales/src/en_US.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/@uppy/locales/src/en_US.js b/packages/@uppy/locales/src/en_US.js index fd089050de..945e010424 100644 --- a/packages/@uppy/locales/src/en_US.js +++ b/packages/@uppy/locales/src/en_US.js @@ -14,6 +14,7 @@ en_US.strings = { aspectRatioLandscape: 'Crop landscape (16:9)', aspectRatioPortrait: 'Crop portrait (9:16)', aspectRatioSquare: 'Crop square', + authAborted: 'Authentication aborted', authenticateWith: 'Connect to %{pluginName}', authenticateWithTitle: 'Please authenticate with %{pluginName} to select files', back: 'Back', From c1f66453207aaa9bd4f25f8e249d04924861a764 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 31 Aug 2021 17:36:19 +0700 Subject: [PATCH 10/15] Update packages/@uppy/companion/src/server/controllers/callback.js Co-authored-by: Antoine du Hamel --- packages/@uppy/companion/src/server/controllers/callback.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/@uppy/companion/src/server/controllers/callback.js b/packages/@uppy/companion/src/server/controllers/callback.js index 763d4d83e5..ebc4de141a 100644 --- a/packages/@uppy/companion/src/server/controllers/callback.js +++ b/packages/@uppy/companion/src/server/controllers/callback.js @@ -13,7 +13,10 @@ const closePageHtml = (origin) => ` From 8b2fc952f4a6b9d9a916873e8de8ceabd997591a Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Tue, 31 Aug 2021 18:29:57 +0700 Subject: [PATCH 11/15] fix lint --- packages/@uppy/companion/src/server/controllers/callback.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/@uppy/companion/src/server/controllers/callback.js b/packages/@uppy/companion/src/server/controllers/callback.js index ebc4de141a..b3ac72d309 100644 --- a/packages/@uppy/companion/src/server/controllers/callback.js +++ b/packages/@uppy/companion/src/server/controllers/callback.js @@ -13,10 +13,9 @@ const closePageHtml = (origin) => ` From d528b1939df59b20c0cf7be200079d387db7dcd7 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 30 Sep 2021 17:09:34 +0700 Subject: [PATCH 12/15] add i18n string that got lost during merge --- packages/@uppy/core/src/Uppy.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/@uppy/core/src/Uppy.js b/packages/@uppy/core/src/Uppy.js index bb977e85d3..2a687a61d3 100644 --- a/packages/@uppy/core/src/Uppy.js +++ b/packages/@uppy/core/src/Uppy.js @@ -89,6 +89,7 @@ class Uppy { noMoreFilesAllowed: 'Cannot add more files', noDuplicates: 'Cannot add the duplicate file \'%{fileName}\', it already exists', companionError: 'Connection with Companion failed', + authAborted: 'Authentication aborted', companionUnauthorizeHint: 'To unauthorize to your %{provider} account, please go to %{url}', failedToUpload: 'Failed to upload %{file}', noInternetConnection: 'No Internet connection', From 010129a66d72f649830730b5044129e447bfc651 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 30 Sep 2021 17:21:16 +0700 Subject: [PATCH 13/15] add french translation --- packages/@uppy/locales/src/fr_FR.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/@uppy/locales/src/fr_FR.js b/packages/@uppy/locales/src/fr_FR.js index 1eb0a88fcb..b2650fa258 100644 --- a/packages/@uppy/locales/src/fr_FR.js +++ b/packages/@uppy/locales/src/fr_FR.js @@ -11,6 +11,7 @@ fr_FR.strings = { allFilesFromFolderNamed: 'Tous les fichiers du dossier %{name}', allowAccessDescription: 'Pour prendre des photos ou enregistrer une vidéo avec votre caméra, veuillez autoriser l\'accès à votre caméra pour ce site.', allowAccessTitle: 'Veuillez autoriser l\'accès à votre caméra', + authAborted: 'Authentification interrompue', authenticateWith: 'Se connecter à %{pluginName}', authenticateWithTitle: 'Veuillez vous authentifier avec %{pluginName} pour sélectionner les fichiers', back: 'Retour', From ef62ead05e8ac9b1480f3188ac2b21126a94ec58 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 30 Sep 2021 17:21:51 +0700 Subject: [PATCH 14/15] add dutch translation --- packages/@uppy/locales/src/nl_NL.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/@uppy/locales/src/nl_NL.js b/packages/@uppy/locales/src/nl_NL.js index 526d399cca..9eaf4da627 100644 --- a/packages/@uppy/locales/src/nl_NL.js +++ b/packages/@uppy/locales/src/nl_NL.js @@ -7,6 +7,7 @@ nl_NL.strings = { allowAccessTitle: 'Geef toestemming om je camera te gebruiken', authenticateWith: 'Verbinden met %{pluginName}', authenticateWithTitle: 'Verbind met %{pluginName} om bestanden te selecteren', + authAborted: 'Authenticatie geannuleerd', back: 'Terug', addMore: 'Meer toevoegen', browse: 'blader', From 25a0586bb1ef307a96796de3a33ff2369872c969 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 30 Sep 2021 17:22:35 +0700 Subject: [PATCH 15/15] add norwegian translation --- packages/@uppy/locales/src/nb_NO.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/@uppy/locales/src/nb_NO.js b/packages/@uppy/locales/src/nb_NO.js index 8823675bd8..226972c2c1 100644 --- a/packages/@uppy/locales/src/nb_NO.js +++ b/packages/@uppy/locales/src/nb_NO.js @@ -10,6 +10,7 @@ nb_NO.strings = { addingMoreFiles: 'Legger til flere filer', allowAccessDescription: 'For å kunne ta bilder eller spille inn video må du gi siden tilgang til å bruke ditt kamera.', allowAccessTitle: 'Vennligst gi tilgang til ditt kamera', + authAborted: 'Autentisering avbrutt', aspectRatioLandscape: 'Beskjær landskap (16:9)', aspectRatioPortrait: 'Beskjær portrett (9:16)', aspectRatioSquare: 'Beskjær kvadrat',