From d39da10251cb794e266f7fd7c288a3709da8eeee Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Thu, 26 Jan 2017 15:20:18 +0000 Subject: [PATCH 1/3] Implement token-based authentication for the WebSocket Supply the access token to the WebSocket via a query param. This method is used to send the token because the WebSocket constructor does not allow setting custom headers. See https://github.com/hypothesis/product-backlog/issues/154 for context. An alternative that was tried initially was embedding a username and password in the URL via `wss://user:password@host/` syntax but that turned out not to be supported by IE/Edge and required the server to fail the initial request with a 401 response. Fixes hypothesis/product-backlog#126 --- src/sidebar/streamer.js | 52 ++++++++---- src/sidebar/test/streamer-test.js | 132 ++++++++++++++++++++---------- 2 files changed, 125 insertions(+), 59 deletions(-) diff --git a/src/sidebar/streamer.js b/src/sidebar/streamer.js index d5ad587bd26..ec4ebc760e5 100644 --- a/src/sidebar/streamer.js +++ b/src/sidebar/streamer.js @@ -1,5 +1,6 @@ 'use strict'; +var queryString = require('query-string'); var uuid = require('node-uuid'); var events = require('./events'); @@ -19,8 +20,8 @@ var Socket = require('./websocket'); * @param settings - Application settings */ // @ngInject -function Streamer($rootScope, annotationMapper, annotationUI, groups, session, - settings) { +function Streamer($rootScope, annotationMapper, annotationUI, auth, + groups, session, settings) { // The randomly generated session UUID var clientId = uuid.v4(); @@ -149,32 +150,49 @@ function Streamer($rootScope, annotationMapper, annotationUI, groups, session, } var _connect = function () { - var url = settings.websocketUrl; - // If we have no URL configured, don't do anything. - if (!url) { - return; + if (!settings.websocketUrl) { + return Promise.resolve(); } - socket = new Socket(url); + return auth.tokenGetter().then(function (token) { + var url; + if (token) { + // Include the access token in the URL via a query param. This method + // is used to send credentials because the `WebSocket` constructor does + // not support setting the `Authorization` header directly as we do for + // other API requests. + var parsedURL = new URL(settings.websocketUrl); + var queryParams = queryString.parse(parsedURL.search); + queryParams.access_token = token; + parsedURL.search = queryString.stringify(queryParams); + url = parsedURL.toString(); + } else { + url = settings.websocketUrl; + } - socket.on('open', sendClientConfig); - socket.on('error', handleSocketOnError); - socket.on('message', handleSocketOnMessage); + socket = new Socket(url); - // Configure the client ID - setConfig('client-id', { - messageType: 'client_id', - value: clientId, + socket.on('open', sendClientConfig); + socket.on('error', handleSocketOnError); + socket.on('message', handleSocketOnMessage); + + // Configure the client ID + setConfig('client-id', { + messageType: 'client_id', + value: clientId, + }); + }).catch(function (err) { + console.error('Failed to fetch token for WebSocket authentication', err); }); }; var connect = function () { if (socket) { - return; + return Promise.resolve(); } - _connect(); + return _connect(); }; var reconnect = function () { @@ -182,7 +200,7 @@ function Streamer($rootScope, annotationMapper, annotationUI, groups, session, socket.close(); } - _connect(); + return _connect(); }; function applyPendingUpdates() { diff --git a/src/sidebar/test/streamer-test.js b/src/sidebar/test/streamer-test.js index a47c935b263..c855a47bcaf 100644 --- a/src/sidebar/test/streamer-test.js +++ b/src/sidebar/test/streamer-test.js @@ -42,9 +42,10 @@ var fixtures = { // the most recently created FakeSocket instance var fakeWebSocket = null; -function FakeSocket() { +function FakeSocket(url) { fakeWebSocket = this; // eslint-disable-line consistent-this + this.url = url; this.messages = []; this.didClose = false; @@ -67,6 +68,7 @@ inherits(FakeSocket, EventEmitter); describe('Streamer', function () { var fakeAnnotationMapper; var fakeAnnotationUI; + var fakeAuth; var fakeGroups; var fakeRootScope; var fakeSession; @@ -79,6 +81,7 @@ describe('Streamer', function () { fakeRootScope, fakeAnnotationMapper, fakeAnnotationUI, + fakeAuth, fakeGroups, fakeSession, fakeSettings @@ -88,6 +91,12 @@ describe('Streamer', function () { beforeEach(function () { var emitter = new EventEmitter(); + fakeAuth = { + tokenGetter: function () { + return Promise.resolve('dummy-access-token'); + }, + }; + fakeRootScope = { $apply: function (callback) { callback(); @@ -132,8 +141,10 @@ describe('Streamer', function () { it('should not create a websocket connection if websocketUrl is not provided', function () { fakeSettings = {}; createDefaultStreamer(); - activeStreamer.connect(); - assert.isNull(fakeWebSocket); + + return activeStreamer.connect().then(function () { + assert.isNull(fakeWebSocket); + }); }); it('should not create a websocket connection', function () { @@ -148,44 +159,79 @@ describe('Streamer', function () { it('should send the client ID on connection', function () { createDefaultStreamer(); - activeStreamer.connect(); - assert.equal(fakeWebSocket.messages.length, 1); - assert.equal(fakeWebSocket.messages[0].messageType, 'client_id'); - assert.equal(fakeWebSocket.messages[0].value, activeStreamer.clientId); + return activeStreamer.connect().then(function () { + assert.equal(fakeWebSocket.messages.length, 1); + assert.equal(fakeWebSocket.messages[0].messageType, 'client_id'); + assert.equal(fakeWebSocket.messages[0].value, activeStreamer.clientId); + }); }); describe('#connect()', function () { it('should create a websocket connection', function () { createDefaultStreamer(); - activeStreamer.connect(); - assert.ok(fakeWebSocket); + return activeStreamer.connect().then(function () { + assert.ok(fakeWebSocket); + }); + }); + + it('should include credentials in the URL if the client has an access token', function () { + createDefaultStreamer(); + return activeStreamer.connect().then(function () { + assert.equal(fakeWebSocket.url, 'ws://example.com/ws?access_token=dummy-access-token'); + }); + }); + + it('should preserve query params when adding access token to URL', function () { + fakeSettings.websocketUrl = 'ws://example.com/ws?foo=bar'; + createDefaultStreamer(); + return activeStreamer.connect().then(function () { + assert.equal(fakeWebSocket.url, 'ws://example.com/ws?access_token=dummy-access-token&foo=bar'); + }); + }); + + it('should not include credentials in the URL if the client has no access token', function () { + fakeAuth.tokenGetter = function () { + return Promise.resolve(null); + }; + + createDefaultStreamer(); + return activeStreamer.connect().then(function () { + assert.equal(fakeWebSocket.url, 'ws://example.com/ws'); + }); }); it('should not close any existing socket', function () { + var oldWebSocket; createDefaultStreamer(); - activeStreamer.connect(); - var oldWebSocket = fakeWebSocket; - activeStreamer.connect(); - assert.ok(!oldWebSocket.didClose); - assert.ok(!fakeWebSocket.didClose); + return activeStreamer.connect().then(function () { + oldWebSocket = fakeWebSocket; + return activeStreamer.connect(); + }).then(function () { + assert.ok(!oldWebSocket.didClose); + assert.ok(!fakeWebSocket.didClose); + }); }); }); describe('#reconnect()', function () { it('should close the existing socket', function () { + var oldWebSocket; createDefaultStreamer(); - activeStreamer.connect(); - var oldWebSocket = fakeWebSocket; - activeStreamer.reconnect(); - assert.ok(oldWebSocket.didClose); - assert.ok(!fakeWebSocket.didClose); + + return activeStreamer.connect().then(function () { + oldWebSocket = fakeWebSocket; + return activeStreamer.reconnect(); + }).then(function () { + assert.ok(oldWebSocket.didClose); + assert.ok(!fakeWebSocket.didClose); + }); }); }); describe('annotation notifications', function () { beforeEach(function () { createDefaultStreamer(); - activeStreamer.connect(); + return activeStreamer.connect(); }); context('when the app is the stream', function () { @@ -271,7 +317,7 @@ describe('Streamer', function () { describe('#applyPendingUpdates', function () { beforeEach(function () { createDefaultStreamer(); - activeStreamer.connect(); + return activeStreamer.connect(); }); it('applies pending updates', function () { @@ -307,7 +353,7 @@ describe('Streamer', function () { beforeEach(function () { createDefaultStreamer(); - activeStreamer.connect(); + return activeStreamer.connect(); }); unroll('discards pending updates when #event occurs', function (testCase) { @@ -330,40 +376,42 @@ describe('Streamer', function () { describe('when the focused group changes', function () { it('clears pending updates and deletions', function () { createDefaultStreamer(); - activeStreamer.connect(); - - fakeWebSocket.notify(fixtures.createNotification); - fakeRootScope.$broadcast(events.GROUP_FOCUSED); + return activeStreamer.connect().then(function () { + fakeWebSocket.notify(fixtures.createNotification); + fakeRootScope.$broadcast(events.GROUP_FOCUSED); - assert.equal(activeStreamer.countPendingUpdates(), 0); + assert.equal(activeStreamer.countPendingUpdates(), 0); + }); }); }); describe('session change notifications', function () { it('updates the session when a notification is received', function () { createDefaultStreamer(); - activeStreamer.connect(); - var model = { - groups: [{ - id: 'new-group', - }], - }; - fakeWebSocket.notify({ - type: 'session-change', - model: model, + return activeStreamer.connect().then(function () { + var model = { + groups: [{ + id: 'new-group', + }], + }; + fakeWebSocket.notify({ + type: 'session-change', + model: model, + }); + assert.ok(fakeSession.update.calledWith(model)); }); - assert.ok(fakeSession.update.calledWith(model)); }); }); describe('reconnections', function () { it('resends configuration messages when a reconnection occurs', function () { createDefaultStreamer(); - activeStreamer.connect(); - fakeWebSocket.messages = []; - fakeWebSocket.emit('open'); - assert.equal(fakeWebSocket.messages.length, 1); - assert.equal(fakeWebSocket.messages[0].messageType, 'client_id'); + return activeStreamer.connect().then(function () { + fakeWebSocket.messages = []; + fakeWebSocket.emit('open'); + assert.equal(fakeWebSocket.messages.length, 1); + assert.equal(fakeWebSocket.messages[0].messageType, 'client_id'); + }); }); }); }); From 039d8670a9f2a48facc1a2d64eefa129f0d66ee1 Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Thu, 26 Jan 2017 15:26:58 +0000 Subject: [PATCH 2/3] Add JSDoc comments to Streamer.{connect, reconnect} --- src/sidebar/streamer.js | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/src/sidebar/streamer.js b/src/sidebar/streamer.js index ec4ebc760e5..91e47deceba 100644 --- a/src/sidebar/streamer.js +++ b/src/sidebar/streamer.js @@ -187,21 +187,38 @@ function Streamer($rootScope, annotationMapper, annotationUI, auth, }); }; - var connect = function () { + /** + * Connect to the Hypothesis real time update service. + * + * If the service has already connected this does nothing. + * + * @return {Promise} Promise which resolves once the WebSocket connection + * process has started. + */ + function connect() { if (socket) { return Promise.resolve(); } return _connect(); - }; + } - var reconnect = function () { + /** + * Connect to the Hypothesis real time update service. + * + * If the service has already connected this closes the existing connection + * and reconnects. + * + * @return {Promise} Promise which resolves once the WebSocket connection + * process has started. + */ + function reconnect() { if (socket) { socket.close(); } return _connect(); - }; + } function applyPendingUpdates() { var updates = Object.values(pendingUpdates); From e76475c43d50f5d1c72316c4aa3d57f15ca7d069 Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Mon, 30 Jan 2017 11:59:01 +0000 Subject: [PATCH 3/3] Request authenticated user ID after connecting WebSocket Send a "whoami" request [1] after connecting to query the authenticated user ID for the WS connection. This makes it easy to check that authentication for the WS worked as expected by inspecting the frames of the connection in the devtools. [1] http://h.readthedocs.io/en/latest/realtime/#whoami --- src/sidebar/streamer.js | 8 ++++++++ src/sidebar/test/streamer-test.js | 27 +++++++++++++++++++++------ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/src/sidebar/streamer.js b/src/sidebar/streamer.js index 91e47deceba..2cfdda60783 100644 --- a/src/sidebar/streamer.js +++ b/src/sidebar/streamer.js @@ -182,6 +182,14 @@ function Streamer($rootScope, annotationMapper, annotationUI, auth, messageType: 'client_id', value: clientId, }); + + // Send a "whoami" message. The server will respond with a "whoyouare" + // reply which is useful for verifying that authentication worked as + // expected. + setConfig('auth-check', { + type: 'whoami', + id: 1, + }); }).catch(function (err) { console.error('Failed to fetch token for WebSocket authentication', err); }); diff --git a/src/sidebar/test/streamer-test.js b/src/sidebar/test/streamer-test.js index c855a47bcaf..5af8087e553 100644 --- a/src/sidebar/test/streamer-test.js +++ b/src/sidebar/test/streamer-test.js @@ -157,12 +157,24 @@ describe('Streamer', function () { assert.ok(activeStreamer.clientId); }); - it('should send the client ID on connection', function () { + it('should send the client ID after connecting', function () { createDefaultStreamer(); return activeStreamer.connect().then(function () { - assert.equal(fakeWebSocket.messages.length, 1); - assert.equal(fakeWebSocket.messages[0].messageType, 'client_id'); - assert.equal(fakeWebSocket.messages[0].value, activeStreamer.clientId); + var clientIdMsg = fakeWebSocket.messages.find(function (msg) { + return msg.messageType === 'client_id'; + }); + assert.ok(clientIdMsg); + assert.equal(clientIdMsg.value, activeStreamer.clientId); + }); + }); + + it('should request the logged-in user ID after connecting', function () { + createDefaultStreamer(); + return activeStreamer.connect().then(function () { + var whoamiMsg = fakeWebSocket.messages.find(function (msg) { + return msg.type === 'whoami'; + }); + assert.ok(whoamiMsg); }); }); @@ -409,8 +421,11 @@ describe('Streamer', function () { return activeStreamer.connect().then(function () { fakeWebSocket.messages = []; fakeWebSocket.emit('open'); - assert.equal(fakeWebSocket.messages.length, 1); - assert.equal(fakeWebSocket.messages[0].messageType, 'client_id'); + + var configMsgTypes = fakeWebSocket.messages.map(function (msg) { + return msg.type || msg.messageType; + }); + assert.deepEqual(configMsgTypes, ['client_id', 'whoami']); }); }); });