From 3bb5e495a61be466ce1c60aad55e869427bc864a Mon Sep 17 00:00:00 2001 From: Paulo Castro Date: Fri, 15 May 2020 22:21:32 +0100 Subject: [PATCH] Fix 'balena login' web authorization hanging with Google Chrome Resolves: #1404 Change-type: patch --- lib/actions/auth.ts | 8 +++- lib/auth/index.ts | 9 ++-- lib/auth/server.ts | 88 ++++++++++++++------------------------- tests/auth/server.spec.ts | 35 ++++++++++------ 4 files changed, 65 insertions(+), 75 deletions(-) diff --git a/lib/actions/auth.ts b/lib/actions/auth.ts index 24201f99..e6b48d6c 100644 --- a/lib/actions/auth.ts +++ b/lib/actions/auth.ts @@ -1,5 +1,5 @@ /* -Copyright 2016-2017 Balena +Copyright 2016-2020 Balena Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -110,7 +110,6 @@ Examples: } else if (loginOptions.credentials) { return patterns.authenticate(loginOptions); } else if (loginOptions.web) { - console.info('Connecting to the web dashboard'); const auth = await import('../auth'); await auth.login(); return; @@ -143,6 +142,11 @@ Find out about the available commands by running: $ balena help ${messages.reachingOut}`); + + if (options.web) { + const { shutdownServer } = await import('../auth'); + shutdownServer(); + } }, }; diff --git a/lib/auth/index.ts b/lib/auth/index.ts index 90016d9e..beab3e80 100644 --- a/lib/auth/index.ts +++ b/lib/auth/index.ts @@ -1,5 +1,5 @@ /* -Copyright 2016 Balena +Copyright 2016-2020 Balena Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -15,6 +15,9 @@ limitations under the License. */ import { getBalenaSdk } from '../utils/lazy'; +import { awaitForToken, shutdownServer } from './server'; + +export { shutdownServer }; /** * @module auth @@ -52,6 +55,7 @@ export const login = async () => { // from mixed content warnings (as the target of a form in the result page) const callbackUrl = `http://127.0.0.1:${options.port}${options.path}`; const loginUrl = await utils.getDashboardLoginURL(callbackUrl); + console.info(`Opening web browser for URL:\n${loginUrl}`); // Leave a bit of time for the // server to get up and runing setTimeout(async () => { @@ -59,7 +63,6 @@ export const login = async () => { open(loginUrl, { wait: false }); }, 1000); - const server = await import('./server'); const balena = getBalenaSdk(); - return server.awaitForToken(options).tap(balena.auth.loginWithToken); + return awaitForToken(options).tap(balena.auth.loginWithToken); }; diff --git a/lib/auth/server.ts b/lib/auth/server.ts index 559a2af3..5e2b5d50 100644 --- a/lib/auth/server.ts +++ b/lib/auth/server.ts @@ -1,5 +1,5 @@ /* -Copyright 2016 Balena +Copyright 2016-2020 Balena Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -17,10 +17,13 @@ limitations under the License. import * as Promise from 'bluebird'; import * as bodyParser from 'body-parser'; import * as express from 'express'; +import { Socket } from 'net'; import * as path from 'path'; -import { getBalenaSdk } from '../utils/lazy'; + import * as utils from './utils'; +const serverSockets: Socket[] = []; + const createServer = ({ port }: { port: number }) => { const app = express(); app.use( @@ -33,10 +36,29 @@ const createServer = ({ port }: { port: number }) => { app.set('views', path.join(__dirname, 'pages')); const server = app.listen(port); + server.on('connection', socket => serverSockets.push(socket)); return { app, server }; }; +/** + * By design (more like a bug, but they won't admit it), a Node.js `http.server` + * instance prevents the process from exiting for up to 2 minutes (by default) if a + * client keeps a HTTP connection open, and regardless of whether `server.close()` + * was called: the `server.close(callback)` callback takes just as long to be called. + * Setting `server.timeout` to some value like 3 seconds works, but then the CLI + * process hangs for "only" 3 seconds (not good enough). Reducing the timeout to 1 + * second may cause authentication failure if the laptop or CI server are slow for + * any reason. The only reliable way around it seems to be to explicitly unref the + * sockets, so the event loop stops waiting for it. See: + * https://github.com/nodejs/node/issues/2642 + * https://github.com/nodejs/node-v0.x-archive/issues/9066 + */ +export function shutdownServer() { + serverSockets.forEach(s => s.unref()); + serverSockets.splice(0); +} + /** * @summary Await for token * @function @@ -60,45 +82,10 @@ export const awaitForToken = (options: { const { app, server } = createServer({ port: options.port }); return new Promise((resolve, reject) => { - const closeServer = ( - errorMessage: string | undefined, - successPayload?: string, - ) => { - server.close(() => { - if (errorMessage) { - reject(new Error(errorMessage)); - return; - } - - resolve(successPayload); - }); - }; - - const renderAndDone = async ({ - request, - response, - viewName, - errorMessage, - statusCode = 200, - token, - }: { - request: express.Request; - response: express.Response; - viewName: 'success' | 'error'; - errorMessage?: string; - statusCode?: number; - token?: string; - }) => { - const context = await getContext(viewName); - response.status(statusCode).render(viewName, context); - request.connection.destroy(); - closeServer(errorMessage, token); - }; - app.post(options.path, async (request, response) => { + server.close(); // stop listening for new connections try { const token = request.body.token?.trim(); - if (!token) { throw new Error('No token'); } @@ -106,31 +93,18 @@ export const awaitForToken = (options: { if (!loggedIn) { throw new Error('Invalid token'); } - await renderAndDone({ request, response, viewName: 'success', token }); + response.status(200).render('success'); + resolve(token); } catch (error) { - await renderAndDone({ - request, - response, - viewName: 'error', - statusCode: 401, - errorMessage: error.message, - }); + response.status(401).render('error'); + reject(new Error(error.message)); } }); app.use((_request, response) => { + server.close(); // stop listening for new connections response.status(404).send('Not found'); - closeServer('Unknown path or verb'); + reject(new Error('Unknown path or verb')); }); }); }; - -export const getContext = (viewName: 'success' | 'error') => { - if (viewName === 'success') { - return Promise.props({ - dashboardUrl: getBalenaSdk().settings.get('dashboardUrl'), - }); - } - - return Promise.resolve({}); -}; diff --git a/tests/auth/server.spec.ts b/tests/auth/server.spec.ts index e21b3110..6f6465d9 100644 --- a/tests/auth/server.spec.ts +++ b/tests/auth/server.spec.ts @@ -1,4 +1,20 @@ -import * as Promise from 'bluebird'; +/** + * @license + * Copyright 2019-2020 Balena Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import * as Bluebird from 'bluebird'; import * as chai from 'chai'; import chaiAsPromised = require('chai-as-promised'); import * as ejs from 'ejs'; @@ -7,12 +23,7 @@ import * as path from 'path'; import * as request from 'request'; import * as sinon from 'sinon'; -// TODO: Convert server code to Typescript so it can have a declaration file -// @ts-ignore import * as server from '../../build/auth/server'; - -// TODO: Convert utils code to Typescript so it can have a declaration file -// @ts-ignore import * as utils from '../../build/auth/utils'; import tokens from './tokens'; @@ -25,9 +36,7 @@ const options = { path: '/auth', }; -const getPage = function( - name: Parameters[0], -): Promise { +async function getPage(name: string): Promise { const pagePath = path.join( __dirname, '..', @@ -39,8 +48,8 @@ const getPage = function( ); const tpl = fs.readFileSync(pagePath, { encoding: 'utf8' }); const compiledTpl = ejs.compile(tpl); - return server.getContext(name).then((context: any) => compiledTpl(context)); -}; + return compiledTpl(); +} describe('Server:', function() { it('should get 404 if posting to an unknown path', function(done) { @@ -86,7 +95,7 @@ describe('Server:', function() { describe('given the token authenticates with the server', function() { beforeEach(function() { this.loginIfTokenValidStub = sinon.stub(utils, 'loginIfTokenValid'); - return this.loginIfTokenValidStub.returns(Promise.resolve(true)); + return this.loginIfTokenValidStub.returns(Bluebird.resolve(true)); }); afterEach(function() { @@ -119,7 +128,7 @@ describe('Server:', function() { return describe('given the token does not authenticate with the server', function() { beforeEach(function() { this.loginIfTokenValidStub = sinon.stub(utils, 'loginIfTokenValid'); - return this.loginIfTokenValidStub.returns(Promise.resolve(false)); + return this.loginIfTokenValidStub.returns(Bluebird.resolve(false)); }); afterEach(function() {