From a5061992602a79ac6d9e3c6014c9c3653bccd0d6 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 ++-- test/__tests__/callback.js | 7 ++++++- test/__tests__/companion.js | 7 ++++++- 7 files changed, 43 insertions(+), 9 deletions(-) diff --git a/src/server/controllers/callback.js b/src/server/controllers/callback.js index 23ba3bbc7b..b3ac72d309 100644 --- 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) => ` + + + + + + + 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/src/server/controllers/oauth-redirect.js b/src/server/controllers/oauth-redirect.js index e037bd4da5..dfcfe518be 100644 --- 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 index 2b27024058..84c83f570e 100644 --- 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 index 73231c63f9..150dbd3e7b 100644 --- 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 index 82921f3800..965556898f 100644 --- 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 index 743ab47fda..391a850bc7 100644 --- 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 index aaa3ed5530..4c79b82cdb 100644 --- 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')