From cf894d98a580ee6ef5f483bf47be44fe9b0ff6b9 Mon Sep 17 00:00:00 2001 From: Paulo Castro Date: Sat, 1 Aug 2020 01:07:43 +0100 Subject: [PATCH] login: Use any free port number instead of 8989 for web authentication Change-type: minor --- doc/cli.markdown | 4 + lib/actions-oclif/login.ts | 17 ++- lib/auth/index.ts | 34 +++--- lib/auth/server.ts | 164 ++++++++++++++++---------- tests/auth/server.spec.ts | 236 +++++++++++++++++-------------------- 5 files changed, 242 insertions(+), 213 deletions(-) diff --git a/doc/cli.markdown b/doc/cli.markdown index 92d62014..ffc75769 100644 --- a/doc/cli.markdown +++ b/doc/cli.markdown @@ -446,6 +446,10 @@ email password +#### -P, --port PORT + +TCP port number of local HTTP login server (--web auth only) + ## logout Logout from your balena account. diff --git a/lib/actions-oclif/login.ts b/lib/actions-oclif/login.ts index 83992b52..160ace52 100644 --- a/lib/actions-oclif/login.ts +++ b/lib/actions-oclif/login.ts @@ -28,6 +28,7 @@ interface FlagsDef { email?: string; user?: string; password?: string; + port?: number; help: void; } @@ -73,14 +74,17 @@ export default class LoginCmd extends Command { web: flags.boolean({ char: 'w', description: 'web-based login', + exclusive: ['token', 'credentials'], }), token: flags.boolean({ char: 't', description: 'session token or API key', + exclusive: ['web', 'credentials'], }), credentials: flags.boolean({ char: 'c', description: 'credential-based login', + exclusive: ['web', 'token'], }), email: flags.string({ char: 'e', @@ -101,6 +105,12 @@ export default class LoginCmd extends Command { description: 'password', dependsOn: ['credentials'], }), + port: flags.integer({ + char: 'P', + description: + 'TCP port number of local HTTP login server (--web auth only)', + dependsOn: ['web'], + }), help: cf.help, }; @@ -134,11 +144,6 @@ Find out about the available commands by running: $ balena help ${messages.reachingOut}`); - - if (options.web) { - const { shutdownServer } = await import('../auth'); - shutdownServer(); - } } async doLogin(loginOptions: FlagsDef, token?: string): Promise { @@ -166,7 +171,7 @@ ${messages.reachingOut}`); // Web else if (loginOptions.web) { const auth = await import('../auth'); - await auth.login(); + await auth.login({ port: loginOptions.port }); return; } else { const patterns = await import('../utils/patterns'); diff --git a/lib/auth/index.ts b/lib/auth/index.ts index 2df0e2d6..f7d71da1 100644 --- a/lib/auth/index.ts +++ b/lib/auth/index.ts @@ -15,9 +15,7 @@ limitations under the License. */ import { getBalenaSdk } from '../utils/lazy'; -import { awaitForToken } from './server'; - -export { shutdownServer } from './server'; +import { LoginServer } from './server'; /** * @module auth @@ -43,28 +41,26 @@ export { shutdownServer } from './server'; * console.log('I\'m logged in!') * console.log("My session token is: #{sessionToken}") */ -export const login = async () => { +export async function login({ host = '127.0.0.1', port = 0 }) { const utils = await import('./utils'); - const options = { - port: 8989, - path: '/auth', - }; + const loginServer = new LoginServer(); + const { + host: actualHost, + port: actualPort, + urlPath, + } = await loginServer.start({ host, port }); - // Needs to be 127.0.0.1 not localhost, because the ip only is whitelisted - // 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 callbackUrl = `http://${actualHost}:${actualPort}${urlPath}`; 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 () => { - const open = await import('open'); - open(loginUrl, { wait: false }); - }, 1000); + const open = await import('open'); + open(loginUrl, { wait: false }); const balena = getBalenaSdk(); - const token = await awaitForToken(options); + const token = await loginServer.awaitForToken(); await balena.auth.loginWithToken(token); + loginServer.shutdown(); return token; -}; +} diff --git a/lib/auth/server.ts b/lib/auth/server.ts index 68116729..bb7871fe 100644 --- a/lib/auth/server.ts +++ b/lib/auth/server.ts @@ -15,6 +15,7 @@ limitations under the License. */ import * as bodyParser from 'body-parser'; +import { EventEmitter } from 'events'; import * as express from 'express'; import type { Socket } from 'net'; import * as path from 'path'; @@ -22,68 +23,55 @@ import * as path from 'path'; import * as utils from './utils'; import { ExpectedError } from '../errors'; -const serverSockets: Socket[] = []; +export class LoginServer extends EventEmitter { + protected expressApp: express.Express; + protected server: import('net').Server; + protected serverSockets: Socket[] = []; + protected firstError: Error; + protected token: string; -const createServer = ({ port }: { port: number }) => { - const app = express(); - app.use( - bodyParser.urlencoded({ - extended: true, - }), - ); + public readonly loginPath = '/auth'; - app.set('view engine', 'ejs'); - app.set('views', path.join(__dirname, 'pages')); + /** + * Start the HTTP server, listening on the given IP address and port number. + * If the port number is 0, the OS will allocate a free port number. + */ + public async start({ host = '127.0.0.1', port = 0 } = {}): Promise<{ + host: string; + port: number; + urlPath: string; + }> { + this.once('error', (err: Error) => { + this.firstError = err; + }); + this.on('token', (token: string) => { + this.token = token; + }); - const server = app.listen(port); - server.on('connection', (socket) => serverSockets.push(socket)); + const app = (this.expressApp = express()); + app.use( + bodyParser.urlencoded({ + extended: true, + }), + ); - return { app, server }; -}; + app.set('view engine', 'ejs'); + app.set('views', path.join(__dirname, 'pages')); -/** - * 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); -} + this.server = await new Promise((resolve, reject) => { + const server = app.listen(port, host, (err: Error) => { + if (err) { + this.emit('error', err); + reject(err); + } else { + resolve(server); + } + }); + server.on('connection', (socket) => this.serverSockets.push(socket)); + }); -/** - * @summary Await for token - * @function - * @protected - * - * @param {Object} options - options - * @param {String} options.path - callback path - * @param {Number} options.port - http port - * - * @example - * server.awaitForToken - * path: '/auth' - * port: 9001 - * .then (token) -> - * console.log(token) - */ -export const awaitForToken = (options: { - path: string; - port: number; -}): Promise => { - const { app, server } = createServer({ port: options.port }); - - return new Promise((resolve, reject) => { - app.post(options.path, async (request, response) => { - server.close(); // stop listening for new connections + this.expressApp.post(this.loginPath, async (request, response) => { + this.server.close(); // stop listening for new connections try { const token = request.body.token?.trim(); if (!token) { @@ -93,18 +81,68 @@ export const awaitForToken = (options: { if (!loggedIn) { throw new ExpectedError('Invalid token'); } + this.emit('token', token); response.status(200).render('success'); - resolve(token); } catch (error) { + this.emit('error', error); response.status(401).render('error'); - reject(new Error(error.message)); } }); - app.use((_request, response) => { - server.close(); // stop listening for new connections + this.expressApp.use((_request, response) => { + this.server.close(); // stop listening for new connections + this.emit('error', new Error('Unknown path or verb')); response.status(404).send('Not found'); - reject(new Error('Unknown path or verb')); }); - }); -}; + + return this.getAddress(); + } + + public getAddress(): { host: string; port: number; urlPath: string } { + const info = this.server.address() as import('net').AddressInfo; + return { + host: info.address, + port: info.port, + urlPath: this.loginPath, + }; + } + + /** + * Shut the server down. + * Call this method to avoid the process hanging in some situations. + */ + public shutdown() { + // 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 complete. Setting `server.timeout` to some value like + // 3 seconds works, but then the CLI process hangs for "only" 3 seconds. 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 + // + this.serverSockets.forEach((s) => s.unref()); + this.serverSockets.splice(0); + } + + /** + * Await for the user to complete login through a web browser. + * Resolve to the authentication token string. + * + * @return Promise that resolves to the authentication token string + */ + public async awaitForToken(): Promise { + if (this.firstError) { + throw this.firstError; + } + if (this.token) { + return this.token; + } + return new Promise((resolve, reject) => { + this.on('error', reject); + this.on('token', resolve); + }); + } +} diff --git a/tests/auth/server.spec.ts b/tests/auth/server.spec.ts index f178ba55..d7e06bb0 100644 --- a/tests/auth/server.spec.ts +++ b/tests/auth/server.spec.ts @@ -14,7 +14,7 @@ * 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'; @@ -23,7 +23,7 @@ import * as path from 'path'; import * as request from 'request'; import * as sinon from 'sinon'; -import * as server from '../../build/auth/server'; +import { LoginServer } from '../../build/auth/server'; import * as utils from '../../build/auth/utils'; import tokens from './tokens'; @@ -31,11 +31,6 @@ chai.use(chaiAsPromised); const { expect } = chai; -const options = { - port: 3000, - path: '/auth', -}; - async function getPage(name: string): Promise { const pagePath = path.join( __dirname, @@ -51,154 +46,145 @@ async function getPage(name: string): Promise { return compiledTpl(); } -describe('Server:', function () { - it('should get 404 if posting to an unknown path', function (done) { - const promise = server.awaitForToken(options); - expect(promise).to.be.rejectedWith('Unknown path or verb'); +describe('Login server:', function () { + let server: LoginServer; + let addr: { host: string; port: number; urlPath: string }; - return request.post( - `http://localhost:${options.port}/foobarbaz`, - { - form: { - token: tokens.johndoe.token, - }, - }, - function (error, response, body) { - expect(error).to.not.exist; - expect(response.statusCode).to.equal(404); - expect(body).to.equal('Not found'); - return done(); - }, - ); + this.beforeEach(async () => { + server = new LoginServer(); + await server.start(); + addr = server.getAddress(); + expect(addr.host).to.equal('127.0.0.1'); }); - it('should get 404 if not using the correct verb', function (done) { - const promise = server.awaitForToken(options); - expect(promise).to.be.rejectedWith('Unknown path or verb'); + this.afterEach(() => { + server.shutdown(); + }); - return request.get( - `http://localhost:${options.port}${options.path}`, - { - form: { - token: tokens.johndoe.token, + async function testLogin(opt: { + expectedBody: string; + expectedErrorMsg?: string; + expectedStatusCode: number; + expectedToken: string; + urlPath?: string; + verb?: string; + }) { + opt.urlPath = opt.urlPath ?? addr.urlPath; + const post = opt.verb + ? ((request as any)[opt.verb] as typeof request.post) + : request.post; + await new Promise((resolve, reject) => { + post( + `http://${addr.host}:${addr.port}${opt.urlPath}`, + { + form: { + token: opt.expectedToken, + }, }, - }, - function (error, response, body) { - expect(error).to.not.exist; - expect(response.statusCode).to.equal(404); - expect(body).to.equal('Not found'); - return done(); - }, - ); + function (error, response, body) { + try { + expect(error).to.not.exist; + expect(response.statusCode).to.equal(opt.expectedStatusCode); + expect(body).to.equal(opt.expectedBody); + resolve(); + } catch (err) { + reject(err); + } + }, + ); + }); + + try { + const token = await server.awaitForToken(); + if (opt.expectedErrorMsg) { + throw new Error('Error not thrown when expected'); + } else { + expect(token).to.exist; + expect(token).to.equal(opt.expectedToken); + } + } catch (err) { + if (opt.expectedErrorMsg) { + expect(err).to.have.property('message', opt.expectedErrorMsg); + } else { + throw err; + } + } + } + + it('should get 404 if posting to an unknown path', async () => { + await testLogin({ + expectedBody: 'Not found', + expectedStatusCode: 404, + expectedToken: tokens.johndoe.token, + expectedErrorMsg: 'Unknown path or verb', + urlPath: '/foobarbaz', + }); + }); + + it('should get 404 if not using the correct verb', async () => { + await testLogin({ + expectedBody: 'Not found', + expectedStatusCode: 404, + expectedToken: tokens.johndoe.token, + expectedErrorMsg: 'Unknown path or verb', + verb: 'get', + }); }); describe('given the token authenticates with the server', function () { beforeEach(function () { this.loginIfTokenValidStub = sinon.stub(utils, 'loginIfTokenValid'); - return this.loginIfTokenValidStub.returns(Bluebird.resolve(true)); + this.loginIfTokenValidStub.returns(Promise.resolve(true)); }); afterEach(function () { - return this.loginIfTokenValidStub.restore(); + this.loginIfTokenValidStub.restore(); }); - return it('should eventually be the token', function (done) { - const promise = server.awaitForToken(options); - expect(promise).to.eventually.equal(tokens.johndoe.token); - - return request.post( - `http://localhost:${options.port}${options.path}`, - { - form: { - token: tokens.johndoe.token, - }, - }, - function (error, response, body) { - expect(error).to.not.exist; - expect(response.statusCode).to.equal(200); - return getPage('success').then(function (expectedBody) { - expect(body).to.equal(expectedBody); - return done(); - }); - }, - ); + it('should eventually be the token', async () => { + await testLogin({ + expectedBody: await getPage('success'), + expectedStatusCode: 200, + expectedToken: tokens.johndoe.token, + }); }); }); - return describe('given the token does not authenticate with the server', function () { + describe('given the token does not authenticate with the server', function () { beforeEach(function () { this.loginIfTokenValidStub = sinon.stub(utils, 'loginIfTokenValid'); - return this.loginIfTokenValidStub.returns(Bluebird.resolve(false)); + return this.loginIfTokenValidStub.returns(Promise.resolve(false)); }); afterEach(function () { return this.loginIfTokenValidStub.restore(); }); - it('should be rejected', function (done) { - const promise = server.awaitForToken(options); - expect(promise).to.be.rejectedWith('Invalid token'); - - return request.post( - `http://localhost:${options.port}${options.path}`, - { - form: { - token: tokens.johndoe.token, - }, - }, - function (error, response, body) { - expect(error).to.not.exist; - expect(response.statusCode).to.equal(401); - return getPage('error').then(function (expectedBody) { - expect(body).to.equal(expectedBody); - return done(); - }); - }, - ); + it('should be rejected', async () => { + await testLogin({ + expectedBody: await getPage('error'), + expectedStatusCode: 401, + expectedToken: tokens.johndoe.token, + expectedErrorMsg: 'Invalid token', + }); }); - it('should be rejected if no token', function (done) { - const promise = server.awaitForToken(options); - expect(promise).to.be.rejectedWith('No token'); - - return request.post( - `http://localhost:${options.port}${options.path}`, - { - form: { - token: '', - }, - }, - function (error, response, body) { - expect(error).to.not.exist; - expect(response.statusCode).to.equal(401); - return getPage('error').then(function (expectedBody) { - expect(body).to.equal(expectedBody); - return done(); - }); - }, - ); + it('should be rejected if no token', async () => { + await testLogin({ + expectedBody: await getPage('error'), + expectedStatusCode: 401, + expectedToken: '', + expectedErrorMsg: 'No token', + }); }); - return it('should be rejected if token is malformed', function (done) { - const promise = server.awaitForToken(options); - expect(promise).to.be.rejectedWith('Invalid token'); - - return request.post( - `http://localhost:${options.port}${options.path}`, - { - form: { - token: 'asdf', - }, - }, - function (error, response, body) { - expect(error).to.not.exist; - expect(response.statusCode).to.equal(401); - return getPage('error').then(function (expectedBody) { - expect(body).to.equal(expectedBody); - return done(); - }); - }, - ); + it('should be rejected if token is malformed', async () => { + await testLogin({ + expectedBody: await getPage('error'), + expectedStatusCode: 401, + expectedToken: 'asdf', + expectedErrorMsg: 'Invalid token', + }); }); }); });