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 <duhamelantoine1995@gmail.com> * Update packages/@uppy/companion/src/server/controllers/callback.js Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> * 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 <duhamelantoine1995@gmail.com> * 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 <duhamelantoine1995@gmail.com>
transloadit · Sep 30, 2021 · 56a67de · 56a67de
1 parent 876f833
commit 56a67de
Show file tree

Hide file tree

Showing 13 changed files with 56 additions and 9 deletions.
diff --git 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) => `
+  <!DOCTYPE html>
+  <html>
+  <head>
+      <meta charset="utf-8" />
+      <script>
+      // if window.opener is nullish, we want the following line to throw to avoid
+      // the window closing without informing the user.
+      window.opener.postMessage(${serialize({ error: true })}, ${serialize(origin)})
+      window.close()
+      </script>
+  </head>
+  <body>Authentication failed.</body>
+  </html>`
 
 /**
  *
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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