Close window on auth callback error and show error to user (transload…

…it#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>
docsend · Sep 30, 2021 · a506199 · a506199
1 parent ab3cbbf
commit a506199
Show file tree

Hide file tree

Showing 7 changed files with 43 additions and 9 deletions.
diff --git a/src/server/controllers/callback.js b/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/src/server/controllers/oauth-redirect.js b/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/src/server/controllers/send-token.js b/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/src/server/helpers/oauth-state.js b/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/src/server/provider/credentials.js b/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/test/__tests__/callback.js b/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/test/__tests__/companion.js b/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')