From 56a67de550c5d774250b476f72a5b26310b32ec5 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 30 Sep 2021 18:07:27 +0700 Subject: [PATCH] Close window on auth callback error and show error to user (#3143) * 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 * Update packages/@uppy/companion/src/server/controllers/callback.js Co-authored-by: Antoine du Hamel * Update packages/@uppy/companion/src/server/controllers/callback.js Co-authored-by: Antoine du Hamel * Update packages/@uppy/companion/src/server/controllers/callback.js * Update callback.js * pull out reusable code to getDynamicStateFromRequest * Signal auth error to client * fix test mocking * npm run build:locale-pack * Update packages/@uppy/companion/src/server/controllers/callback.js Co-authored-by: Antoine du Hamel * fix lint * add i18n string that got lost during merge * add french translation * add dutch translation * add norwegian translation Co-authored-by: Antoine du Hamel --- .../src/server/controllers/callback.js | 22 ++++++++++++++++++- .../src/server/controllers/oauth-redirect.js | 3 +-- .../src/server/controllers/send-token.js | 3 +-- .../src/server/helpers/oauth-state.js | 6 +++++ .../src/server/provider/credentials.js | 4 ++-- .../companion/test/__tests__/callback.js | 7 +++++- .../companion/test/__tests__/companion.js | 7 +++++- packages/@uppy/core/src/Uppy.js | 1 + packages/@uppy/locales/src/en_US.js | 1 + packages/@uppy/locales/src/fr_FR.js | 1 + packages/@uppy/locales/src/nb_NO.js | 1 + packages/@uppy/locales/src/nl_NL.js | 1 + .../src/ProviderView/ProviderView.js | 8 +++++++ 13 files changed, 56 insertions(+), 9 deletions(-) diff --git a/packages/@uppy/companion/src/server/controllers/callback.js b/packages/@uppy/companion/src/server/controllers/callback.js index 23ba3bbc7b..b3ac72d309 100644 --- a/packages/@uppy/companion/src/server/controllers/callback.js +++ b/packages/@uppy/companion/src/server/controllers/callback.js @@ -1,8 +1,26 @@ /** * 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 = (origin) => ` + + + + + + + Authentication failed. + ` /** * @@ -27,5 +45,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.sendStatus(400) + 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/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() } 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') 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', diff --git a/packages/@uppy/locales/src/en_US.js b/packages/@uppy/locales/src/en_US.js index 54ec6ea14b..3517164614 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', 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', 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', 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', 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