From 4e1c5435b84c58ec1c1547806ec9a926dfde97ef Mon Sep 17 00:00:00 2001 From: Samuel Mannehed Date: Sun, 5 Nov 2017 21:40:47 +0100 Subject: [PATCH 1/9] Add missing semicolons --- core/encodings.js | 2 +- core/rfb.js | 2 +- core/util/localization.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/core/encodings.js b/core/encodings.js index 3184ace0..a0551d63 100644 --- a/core/encodings.js +++ b/core/encodings.js @@ -26,7 +26,7 @@ export var encodings = { pseudoEncodingContinuousUpdates: -313, pseudoEncodingCompressLevel9: -247, pseudoEncodingCompressLevel0: -256, -} +}; export function encodingName(num) { switch (num) { diff --git a/core/rfb.js b/core/rfb.js index 3d12ae4b..caa59634 100644 --- a/core/rfb.js +++ b/core/rfb.js @@ -45,7 +45,7 @@ export default function RFB(target, url, options) { this._url = url; // Connection details - options = options || {} + options = options || {}; this._rfb_credentials = options.credentials || {}; this._shared = 'shared' in options ? !!options.shared : true; this._repeaterID = options.repeaterID || ''; diff --git a/core/util/localization.js b/core/util/localization.js index fcebf06f..39c91f68 100644 --- a/core/util/localization.js +++ b/core/util/localization.js @@ -164,7 +164,7 @@ Localizer.prototype = { process(document.body, true); }, -} +}; export var l10n = new Localizer(); export default l10n.get.bind(l10n); From 5b20d338ce258b788c2a2d1782e9718fa15af068 Mon Sep 17 00:00:00 2001 From: Samuel Mannehed Date: Sun, 5 Nov 2017 21:47:06 +0100 Subject: [PATCH 2/9] Remove RFB.notification() This interface was a band aid for poor design. The two cases where it was used was replaced by logging. --- app/ui.js | 5 ----- core/rfb.js | 33 ++++----------------------------- docs/API.md | 21 --------------------- tests/playback-ui.js | 19 +------------------ tests/playback.js | 4 +--- tests/test.rfb.js | 29 ----------------------------- tests/vnc_perf.html | 4 ---- vnc_lite.html | 6 +----- 8 files changed, 7 insertions(+), 114 deletions(-) diff --git a/app/ui.js b/app/ui.js index f6615674..7ab82f5a 100644 --- a/app/ui.js +++ b/app/ui.js @@ -532,10 +532,6 @@ var UI = { document.getElementById('noVNC_status').classList.remove("noVNC_open"); }, - notification: function (e) { - UI.showStatus(e.detail.message, e.detail.level); - }, - activateControlbar: function(event) { clearTimeout(UI.idleControlbarTimeout); // We manipulate the anchor instead of the actual control @@ -1038,7 +1034,6 @@ var UI = { { shared: UI.getSetting('shared'), repeaterID: UI.getSetting('repeaterID'), credentials: { password: password } }); - UI.rfb.addEventListener("notification", UI.notification); UI.rfb.addEventListener("updatestate", UI.updateState); UI.rfb.addEventListener("disconnect", UI.disconnectFinished); UI.rfb.addEventListener("credentialsrequired", UI.credentials); diff --git a/core/rfb.js b/core/rfb.js index caa59634..a95b1326 100644 --- a/core/rfb.js +++ b/core/rfb.js @@ -1,7 +1,7 @@ /* * noVNC: HTML5 VNC client * Copyright (C) 2012 Joel Martin - * Copyright (C) 2016 Samuel Mannehed for Cendio AB + * Copyright (C) 2017 Samuel Mannehed for Cendio AB * Licensed under MPL 2.0 (see LICENSE.txt) * * See README.md for usage and integration instructions. @@ -589,30 +589,6 @@ RFB.prototype = { return false; }, - /* - * Send a notification to the UI. Valid levels are: - * 'normal'|'warn'|'error' - * - * NOTE: If this function is called multiple times, remember that the - * interface could be only showing the latest notification. - */ - _notification: function(msg, level) { - switch (level) { - case 'normal': - case 'warn': - case 'error': - Log.Debug("Notification[" + level + "]:" + msg); - break; - default: - Log.Error("Invalid notification level: " + level); - return; - } - - var event = new CustomEvent("notification", - { detail: { message: msg, level: level } }); - this.dispatchEvent(event); - }, - _setCapability: function (cap, val) { this._capabilities[cap] = val; var event = new CustomEvent("capabilities", @@ -1262,8 +1238,7 @@ RFB.prototype = { switch (xvp_msg) { case 0: // XVP_FAIL - Log.Error("Operation Failed"); - this._notification("XVP Operation Failed", 'error'); + Log.Error("XVP Operation Failed"); break; case 1: // XVP_INIT this._rfb_xvp_ver = xvp_ver; @@ -2350,8 +2325,8 @@ RFB.encodingHandlers = { msg = "Unknown reason"; break; } - this._notification("Server did not accept the resize request: " - + msg, 'normal'); + Log.Warn("Server did not accept the resize request: " + + msg); } else { this._resize(this._FBU.width, this._FBU.height); } diff --git a/docs/API.md b/docs/API.md index 8f565e93..7d34b07b 100644 --- a/docs/API.md +++ b/docs/API.md @@ -67,10 +67,6 @@ protocol stream. - The `updatestate` event is fired when the connection state of the `RFB` object changes. -[`notification`](#notification) - - The `notification` event is fired when the `RFB` usage has a - message to display to the user. - [`disconnect`](#disconnected) - The `disconnect` event is fired when the `RFB` object disconnects. @@ -200,23 +196,6 @@ Note that a `RFB` objects can not transition from the disconnected state in any way, a new instance of the object has to be created for new connections. -#### notification - -The `notification` event is fired when the `RFB` object wants a message -displayed to the user. The `detail` property is an `Object` containing -the following properties: - -| Property | Type | Description -| --------- | ----------- | ----------- -| `message` | `DOMString` | The message to display -| `level` | `DOMString` | The severity of the message - -The following levels are currently defined: - - - `"normal"` - - `"warn"` - - `"error"` - #### disconnect The `disconnect` event is fired when the connection has been diff --git a/tests/playback-ui.js b/tests/playback-ui.js index 5e10d773..03b08fe1 100644 --- a/tests/playback-ui.js +++ b/tests/playback-ui.js @@ -52,10 +52,6 @@ function enableUI() { encoding = VNC_frame_encoding; } -const notification = function (rfb, mesg, level, options) { - document.getElementById('VNC_status').textContent = mesg; -} - function IterationPlayer (iterations, frames, encoding) { this._iterations = iterations; @@ -72,7 +68,6 @@ function IterationPlayer (iterations, frames, encoding) { this.onfinish = function() {}; this.oniterationfinish = function() {}; this.rfbdisconnected = function() {}; - this.rfbnotification = function() {}; } IterationPlayer.prototype = { @@ -87,7 +82,7 @@ IterationPlayer.prototype = { }, _nextIteration: function () { - const player = new RecordingPlayer(this._frames, this._encoding, this._disconnected.bind(this), this._notification.bind(this)); + const player = new RecordingPlayer(this._frames, this._encoding, this._disconnected.bind(this)); player.onfinish = this._iterationFinish.bind(this); if (this._state !== 'running') { return; } @@ -131,15 +126,6 @@ IterationPlayer.prototype = { this.onrfbdisconnected(evt); }, - - _notification: function (rfb, msg, level, options) { - var evt = new Event('rfbnotification'); - evt.message = msg; - evt.level = level; - evt.options = options; - - this.onrfbnotification(evt); - }, }; function start() { @@ -167,9 +153,6 @@ function start() { message(`noVNC sent disconnected during iteration ${evt.iteration} frame ${evt.frame}`); } }; - player.onrfbnotification = function (evt) { - document.getElementById('VNC_status').textContent = evt.message; - }; player.onfinish = function (evt) { const iterTime = parseInt(evt.duration / evt.iterations, 10); message(`${evt.iterations} iterations took ${evt.duration}ms (average ${iterTime}ms / iteration)`); diff --git a/tests/playback.js b/tests/playback.js index 2cd74998..9199eba0 100644 --- a/tests/playback.js +++ b/tests/playback.js @@ -44,12 +44,11 @@ if (setImmediate === undefined) { window.addEventListener("message", _onMessage); } -export default function RecordingPlayer (frames, encoding, disconnected, notification) { +export default function RecordingPlayer (frames, encoding, disconnected) { this._frames = frames; this._encoding = encoding; this._disconnected = disconnected; - this._notification = notification; if (this._encoding === undefined) { let frame = this._frames[0]; @@ -80,7 +79,6 @@ RecordingPlayer.prototype = { this._rfb = new RFB(document.getElementById('VNC_canvas'), 'wss://test'); this._rfb.viewOnly = true; this._rfb.ondisconnected = this._handleDisconnect.bind(this); - this._rfb.onnotification = this._notification; this._enablePlaybackMode(); // reset the frame index and timer diff --git a/tests/test.rfb.js b/tests/test.rfb.js index 4dbe1840..a3746402 100644 --- a/tests/test.rfb.js +++ b/tests/test.rfb.js @@ -400,27 +400,6 @@ describe('Remote Frame Buffer Protocol Client', function() { }); }); - - describe('#_notification', function () { - var client; - beforeEach(function () { client = make_rfb(); }); - - it('should call the notification callback', function () { - var spy = sinon.spy(); - client.addEventListener("notification", spy); - client._notification('notify!', 'warn'); - expect(spy).to.have.been.calledOnce; - expect(spy.args[0][0].detail.message).to.equal('notify!'); - expect(spy.args[0][0].detail.level).to.equal('warn'); - }); - - it('should not call the notification callback when level is invalid', function () { - var spy = sinon.spy(); - client.addEventListener("notification", spy); - client._notification('notify!', 'invalid'); - expect(spy).to.not.have.been.called; - }); - }); }); describe('Connection States', function () { @@ -1589,14 +1568,6 @@ describe('Remote Frame Buffer Protocol Client', function() { }); describe('XVP Message Handling', function () { - it('should send a notification on XVP_FAIL', function () { - var spy = sinon.spy(); - client.addEventListener("notification", spy); - client._sock._websocket._receive_data(new Uint8Array([250, 0, 10, 0])); - expect(spy).to.have.been.calledOnce; - expect(spy.args[0][0].detail.message).to.equal('XVP Operation Failed'); - }); - it('should set the XVP version and fire the callback with the version on XVP_INIT', function () { var spy = sinon.spy(); client.addEventListener("capabilities", spy); diff --git a/tests/vnc_perf.html b/tests/vnc_perf.html index 001ac286..6e4f4a31 100644 --- a/tests/vnc_perf.html +++ b/tests/vnc_perf.html @@ -85,10 +85,6 @@ } } - notification = function (rfb, mesg, level, options) { - document.getElementById('VNC_status').textContent = mesg; - } - function do_test() { document.getElementById('startButton').value = "Running"; document.getElementById('startButton').disabled = true; diff --git a/vnc_lite.html b/vnc_lite.html index d817885d..e93efcfd 100644 --- a/vnc_lite.html +++ b/vnc_lite.html @@ -188,9 +188,6 @@ status(e.detail.reason, "error"); } } - function notification(e) { - status(e.detail.message, e.detail.level); - } window.onresize = function () { // When the window has been resized, wait until the size remains @@ -272,8 +269,7 @@ { repeaterID: WebUtil.getConfigVar('repeaterID', ''), shared: WebUtil.getConfigVar('shared', true), credentials: { password: password } }); - rfb.viewOnly = WebUtil.getConfigVar('view_only', false); - rfb.addEventListener("notification", notification); + rfb.viewOnly = WebUtil.getConfigVar('view_only', false); rfb.addEventListener("updatestate", updateState); rfb.addEventListener("disconnect", disconnect); rfb.addEventListener("capabilities", function () { updatePowerButtons(); initialResize(); }); From 2592c24388d64153380dc452b1c4ccc00d42463c Mon Sep 17 00:00:00 2001 From: Samuel Mannehed Date: Thu, 9 Nov 2017 10:12:44 +0100 Subject: [PATCH 3/9] Translate user visible text And make the log message a bit more detailed --- app/ui.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/ui.js b/app/ui.js index 7ab82f5a..2c9e3961 100644 --- a/app/ui.js +++ b/app/ui.js @@ -423,9 +423,8 @@ var UI = { UI.showStatus(_("Disconnected")); break; default: - msg = "Invalid UI state"; - Log.Error(msg); - UI.showStatus(msg, 'error'); + Log.Error("Invalid visual state: " + event.detail.state); + UI.showStatus(_("Internal error"), 'error'); break; } From 8317524cdae9f0e34fd71467a0e5e248ee4f93dc Mon Sep 17 00:00:00 2001 From: Samuel Mannehed Date: Thu, 9 Nov 2017 10:14:18 +0100 Subject: [PATCH 4/9] Don't translate log messages --- app/ui.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/app/ui.js b/app/ui.js index 2c9e3961..8bc704b2 100644 --- a/app/ui.js +++ b/app/ui.js @@ -1008,9 +1008,8 @@ var UI = { } if (!host) { - var msg = _("Must set host"); - Log.Error(msg); - UI.showStatus(msg, 'error'); + Log.Error("Can't connect when host is: " + host); + UI.showStatus(_("Must set host"), 'error'); return; } @@ -1106,9 +1105,8 @@ var UI = { document.getElementById('noVNC_password_input').focus(); }, 100); - var msg = _("Password is required"); - Log.Warn(msg); - UI.showStatus(msg, "warning"); + Log.Warn("Server asked for a password"); + UI.showStatus(_("Password is required"), "warning"); }, setPassword: function(e) { From ee5cae9fee484794211d89de55e9d454abd4d599 Mon Sep 17 00:00:00 2001 From: Samuel Mannehed Date: Fri, 10 Nov 2017 12:52:39 +0100 Subject: [PATCH 5/9] Replace updatestate event with connect Instead of exposing all the internal connection states, the RFB module will now only send events on connect and on disconnect. This makes it simpler for the application and gets rid of the double events that were being sent on disconnect (previously updatestate and disconnect). --- app/ui.js | 86 +++++++++++++++++++++++++++----------------- core/rfb.js | 31 ++++++++-------- docs/API.md | 27 ++++---------- tests/test.rfb.js | 91 +++++++++++++++++++++++++++++------------------ vnc_lite.html | 52 +++++++++------------------ 5 files changed, 149 insertions(+), 138 deletions(-) diff --git a/app/ui.js b/app/ui.js index 8bc704b2..0ee0c00b 100644 --- a/app/ui.js +++ b/app/ui.js @@ -105,7 +105,7 @@ var UI = { UI.updateViewClip(); - UI.updateVisualState(); + UI.updateVisualState('init'); document.documentElement.classList.remove("noVNC_loading"); @@ -388,53 +388,41 @@ var UI = { * VISUAL * ------v------*/ - updateState: function(event) { - var msg; + // Disable/enable controls depending on connection state + updateVisualState: function(state) { document.documentElement.classList.remove("noVNC_connecting"); document.documentElement.classList.remove("noVNC_connected"); document.documentElement.classList.remove("noVNC_disconnecting"); document.documentElement.classList.remove("noVNC_reconnecting"); - switch (event.detail.state) { + let transition_elem = document.getElementById("noVNC_transition_text"); + switch (state) { + case 'init': + break; case 'connecting': - document.getElementById("noVNC_transition_text").textContent = _("Connecting..."); + transition_elem.textContent = _("Connecting..."); document.documentElement.classList.add("noVNC_connecting"); break; case 'connected': - UI.connected = true; - UI.inhibit_reconnect = false; - UI.doneInitialResize = false; document.documentElement.classList.add("noVNC_connected"); - if (UI.getSetting('encrypt')) { - msg = _("Connected (encrypted) to ") + UI.desktopName; - } else { - msg = _("Connected (unencrypted) to ") + UI.desktopName; - } - UI.showStatus(msg); - document.getElementById('noVNC_canvas').focus(); break; case 'disconnecting': - UI.connected = false; - document.getElementById("noVNC_transition_text").textContent = _("Disconnecting..."); + transition_elem.textContent = _("Disconnecting..."); document.documentElement.classList.add("noVNC_disconnecting"); break; case 'disconnected': - UI.showStatus(_("Disconnected")); + break; + case 'reconnecting': + transition_elem.textContent = _("Reconnecting..."); + document.documentElement.classList.add("noVNC_reconnecting"); break; default: - Log.Error("Invalid visual state: " + event.detail.state); + Log.Error("Invalid visual state: " + state); UI.showStatus(_("Internal error"), 'error'); - break; + return; } - UI.updateVisualState(); - }, - - // Disable/enable controls depending on connection state - updateVisualState: function() { - //Log.Debug(">> updateVisualState"); - UI.enableDisableViewClip(); if (UI.connected) { @@ -480,8 +468,6 @@ var UI = { // State change also closes the password dialog document.getElementById('noVNC_password_dlg') .classList.remove('noVNC_open'); - - //Log.Debug("<< updateVisualState"); }, showStatus: function(text, status_type, time) { @@ -1016,6 +1002,8 @@ var UI = { UI.closeAllPanels(); UI.closeConnectPanel(); + UI.updateVisualState('connecting'); + UI.updateViewOnly(); var url; @@ -1032,7 +1020,7 @@ var UI = { { shared: UI.getSetting('shared'), repeaterID: UI.getSetting('repeaterID'), credentials: { password: password } }); - UI.rfb.addEventListener("updatestate", UI.updateState); + UI.rfb.addEventListener("connect", UI.connectFinished); UI.rfb.addEventListener("disconnect", UI.disconnectFinished); UI.rfb.addEventListener("credentialsrequired", UI.credentials); UI.rfb.addEventListener("capabilities", function () { UI.updatePowerButton(); UI.initialResize(); }); @@ -1046,9 +1034,13 @@ var UI = { UI.closeAllPanels(); UI.rfb.disconnect(); + UI.connected = false; + // Disable automatic reconnecting UI.inhibit_reconnect = true; + UI.updateVisualState('disconnecting'); + // Don't display the connection settings until we're actually disconnected }, @@ -1063,16 +1055,43 @@ var UI = { UI.connect(null, UI.reconnect_password); }, + connectFinished: function (e) { + UI.connected = true; + UI.inhibit_reconnect = false; + UI.doneInitialResize = false; + + let msg; + if (UI.getSetting('encrypt')) { + msg = _("Connected (encrypted) to ") + UI.desktopName; + } else { + msg = _("Connected (unencrypted) to ") + UI.desktopName; + } + UI.showStatus(msg); + UI.updateVisualState('connected'); + + // Do this last because it can only be used on rendered elements + document.getElementById('noVNC_canvas').focus(); + }, + disconnectFinished: function (e) { + // This variable is ideally set when disconnection starts, but + // when the disconnection isn't clean or if it is initiated by + // the server, we need to do it here as well since + // UI.disconnect() won't be used in those cases. + UI.connected = false; + if (typeof e.detail.reason !== 'undefined') { UI.showStatus(e.detail.reason, 'error'); + UI.updateVisualState('disconnected'); } else if (UI.getSetting('reconnect', false) === true && !UI.inhibit_reconnect) { - document.getElementById("noVNC_transition_text").textContent = _("Reconnecting..."); - document.documentElement.classList.add("noVNC_reconnecting"); + UI.updateVisualState('reconnecting'); var delay = parseInt(UI.getSetting('reconnect_delay')); UI.reconnect_callback = setTimeout(UI.reconnect, delay); return; + } else { + UI.updateVisualState('disconnected'); + UI.showStatus(_("Disconnected"), 'normal'); } UI.openControlbar(); @@ -1085,7 +1104,8 @@ var UI = { UI.reconnect_callback = null; } - document.documentElement.classList.remove("noVNC_reconnecting"); + UI.updateVisualState('disconnected'); + UI.openControlbar(); UI.openConnectPanel(); }, diff --git a/core/rfb.js b/core/rfb.js index a95b1326..dced6b16 100644 --- a/core/rfb.js +++ b/core/rfb.js @@ -512,8 +512,6 @@ RFB.prototype = { // State change actions this._rfb_connection_state = state; - var event = new CustomEvent("updatestate", { detail: { state: state } }); - this.dispatchEvent(event); var smsg = "New state '" + state + "', was '" + oldstate + "'."; Log.Debug(smsg); @@ -528,23 +526,15 @@ RFB.prototype = { } switch (state) { - case 'disconnected': - // Fire disconnected event after updatestate event since - // we don't know if the UI only displays the latest message - if (this._rfb_disconnect_reason !== "") { - event = new CustomEvent("disconnect", - { detail: { reason: this._rfb_disconnect_reason } }); - } else { - // No reason means clean disconnect - event = new CustomEvent("disconnect", { detail: {} }); - } - this.dispatchEvent(event); - break; - case 'connecting': this._connect(); break; + case 'connected': + var event = new CustomEvent("connect", { detail: {} }); + this.dispatchEvent(event); + break; + case 'disconnecting': this._disconnect(); @@ -553,6 +543,17 @@ RFB.prototype = { this._updateConnectionState('disconnected'); }.bind(this), DISCONNECT_TIMEOUT * 1000); break; + + case 'disconnected': + if (this._rfb_disconnect_reason !== "") { + event = new CustomEvent("disconnect", + { detail: { reason: this._rfb_disconnect_reason } }); + } else { + // No reason means clean disconnect + event = new CustomEvent("disconnect", { detail: {} }); + } + this.dispatchEvent(event); + break; } }, diff --git a/docs/API.md b/docs/API.md index 7d34b07b..e194e969 100644 --- a/docs/API.md +++ b/docs/API.md @@ -63,9 +63,9 @@ protocol stream. ### Events -[`updatestate`](#updatestate) - - The `updatestate` event is fired when the connection state of the - `RFB` object changes. +[`connect`](#connect) + - The `connect` event is fired when the `RFB` object has completed + the connection and handshaking with the server. [`disconnect`](#disconnected) - The `disconnect` event is fired when the `RFB` object disconnects. @@ -177,24 +177,11 @@ connection to a specified VNC server. - A `DOMString` specifying the ID to provide to any VNC repeater encountered. -#### updatestate +#### connect -The `updatestate` event is fired after the noVNC connection state -changes. The `detail` property is an `Object` containg the property -`state` with the new connection state. - -Here is a list of the states that are reported: - -| connection state | description -| ----------------- | ------------ -| `"connecting"` | starting to connect -| `"connected"` | connected normally -| `"disconnecting"` | starting to disconnect -| `"disconnected"` | disconnected - -Note that a `RFB` objects can not transition from the disconnected -state in any way, a new instance of the object has to be created for -new connections. +The `connect` event is fired after all the handshaking with the server +is completed and the connection is fully established. After this event +the `RFB` object is ready to recieve graphics updates and to send input. #### disconnect diff --git a/tests/test.rfb.js b/tests/test.rfb.js index a3746402..2ef61240 100644 --- a/tests/test.rfb.js +++ b/tests/test.rfb.js @@ -68,11 +68,9 @@ describe('Remote Frame Buffer Protocol Client', function() { describe('#RFB', function () { it('should set the current state to "connecting"', function () { var client = new RFB(document.createElement('canvas'), 'wss://host:8675'); - var spy = sinon.spy(); - client.addEventListener("updatestate", spy); + client._rfb_connection_state = ''; this.clock.tick(); - expect(spy).to.have.been.calledOnce; - expect(spy.args[0][0].detail.state).to.equal('connecting'); + expect(client._rfb_connection_state).to.equal('connecting'); }); it('should actually connect to the websocket', function () { @@ -90,13 +88,15 @@ describe('Remote Frame Buffer Protocol Client', function() { client = make_rfb(); }); - it('should set the current state to "disconnecting"', function () { - var spy = sinon.spy(); - client.addEventListener("updatestate", spy); + it('should go to state "disconnecting" before "disconnected"', function () { + sinon.spy(client, '_updateConnectionState'); client.disconnect(); - expect(spy).to.have.been.calledTwice; - expect(spy.args[0][0].detail.state).to.equal('disconnecting'); - expect(spy.args[1][0].detail.state).to.equal('disconnected'); + expect(client._updateConnectionState).to.have.been.calledTwice; + expect(client._updateConnectionState.getCall(0).args[0]) + .to.equal('disconnecting'); + expect(client._updateConnectionState.getCall(1).args[0]) + .to.equal('disconnected'); + expect(client._rfb_connection_state).to.equal('disconnected'); }); it('should unregister error event handler', function () { @@ -320,13 +320,6 @@ describe('Remote Frame Buffer Protocol Client', function() { expect(client._disconnTimer).to.be.null; }); - it('should call the updateState callback', function () { - var spy = sinon.spy(); - client.addEventListener("updatestate", spy); - client._updateConnectionState('disconnecting'); - expect(spy.args[0][0].detail.state).to.equal('disconnecting'); - }); - it('should set the rfb_connection_state', function () { client._rfb_connection_state = 'disconnecting'; client._updateConnectionState('disconnected'); @@ -340,16 +333,23 @@ describe('Remote Frame Buffer Protocol Client', function() { }); it('should ignore state changes to the same state', function () { - var spy = sinon.spy(); - client.addEventListener("updatestate", spy); - client._rfb_connection_state = 'connecting'; - client._updateConnectionState('connecting'); - expect(spy).to.not.have.been.called; + var connectSpy = sinon.spy(); + var disconnectSpy = sinon.spy(); + client.addEventListener("connect", connectSpy); + client.addEventListener("disconnect", disconnectSpy); + + client._rfb_connection_state = 'connected'; + client._updateConnectionState('connected'); + expect(connectSpy).to.not.have.been.called; + + client._rfb_connection_state = 'disconnected'; + client._updateConnectionState('disconnected'); + expect(disconnectSpy).to.not.have.been.called; }); it('should ignore illegal state changes', function () { var spy = sinon.spy(); - client.addEventListener("updatestate", spy); + client.addEventListener("disconnect", spy); client._rfb_connection_state = 'connected'; client._updateConnectionState('disconnected'); expect(client._rfb_connection_state).to.not.equal('disconnected'); @@ -403,6 +403,39 @@ describe('Remote Frame Buffer Protocol Client', function() { }); describe('Connection States', function () { + describe('connecting', function () { + it('should open the websocket connection', function () { + var client = new RFB(document.createElement('canvas'), + 'ws://HOST:8675/PATH'); + sinon.spy(client._sock, 'open'); + this.clock.tick(); + expect(client._sock.open).to.have.been.calledOnce; + }); + }); + + describe('connected', function () { + var client; + beforeEach(function () { + client = make_rfb(); + }); + + it('should result in a connect event if state becomes connected', function () { + var spy = sinon.spy(); + client.addEventListener("connect", spy); + client._rfb_connection_state = 'connecting'; + client._updateConnectionState('connected'); + expect(spy).to.have.been.calledOnce; + }); + + it('should not result in a connect event if the state is not "connected"', function () { + var spy = sinon.spy(); + client.addEventListener("connect", spy); + client._sock._websocket.open = function () {}; // explicitly don't call onopen + client._updateConnectionState('connecting'); + expect(spy).to.not.have.been.called; + }); + }); + describe('disconnecting', function () { var client; beforeEach(function () { @@ -465,19 +498,7 @@ describe('Remote Frame Buffer Protocol Client', function() { expect(spy).to.have.been.calledOnce; expect(spy.args[0].length).to.equal(1); }); - - it('should call the updateState callback before the disconnect callback', function () { - var updateStateSpy = sinon.spy(); - var disconnectSpy = sinon.spy(); - client.addEventListener("disconnect", disconnectSpy); - client.addEventListener("updatestate", updateStateSpy); - client._rfb_connection_state = 'disconnecting'; - client._updateConnectionState('disconnected'); - expect(updateStateSpy.calledBefore(disconnectSpy)).to.be.true; - }); }); - - // NB(directxman12): Connected does *nothing* in updateConnectionState }); describe('Protocol Initialization States', function () { diff --git a/vnc_lite.html b/vnc_lite.html index e93efcfd..b904b4a5 100644 --- a/vnc_lite.html +++ b/vnc_lite.html @@ -147,45 +147,25 @@ document.getElementById('noVNC_status_bar').className = "noVNC_status_" + level; document.getElementById('noVNC_status').textContent = text; } - function updateState(e) { - var cad = document.getElementById('sendCtrlAltDelButton'); - switch (e.detail.state) { - case 'connecting': - status("Connecting", "normal"); - break; - case 'connected': - doneInitialResize = false; - if (WebUtil.getConfigVar('encrypt', - (window.location.protocol === "https:"))) { - status("Connected (encrypted) to " + - desktopName, "normal"); - } else { - status("Connected (unencrypted) to " + - desktopName, "normal"); - } - break; - case 'disconnecting': - status("Disconnecting", "normal"); - break; - case 'disconnected': - status("Disconnected", "normal"); - break; - default: - status(e.detail.state, "warn"); - break; - } - if (e.detail.state === 'connected') { - cad.disabled = false; + function connected(e) { + document.getElementById('sendCtrlAltDelButton').disabled = false; + doneInitialResize = false; + if (WebUtil.getConfigVar('encrypt', + (window.location.protocol === "https:"))) { + status("Connected (encrypted) to " + desktopName, "normal"); } else { - cad.disabled = true; - updatePowerButtons(); + status("Connected (unencrypted) to " + desktopName, "normal"); } - } - function disconnect(e) { + + function disconnected(e) { + document.getElementById('sendCtrlAltDelButton').disabled = true; + updatePowerButtons(); if (typeof(e.detail.reason) !== 'undefined') { status(e.detail.reason, "error"); + } else { + status("Disconnected", "normal"); } } @@ -246,6 +226,8 @@ (function() { + status("Connecting", "normal"); + if ((!host) || (!port)) { status('Must specify host and port in URL', 'error'); } @@ -270,8 +252,8 @@ shared: WebUtil.getConfigVar('shared', true), credentials: { password: password } }); rfb.viewOnly = WebUtil.getConfigVar('view_only', false); - rfb.addEventListener("updatestate", updateState); - rfb.addEventListener("disconnect", disconnect); + rfb.addEventListener("connect", connected); + rfb.addEventListener("disconnect", disconnected); rfb.addEventListener("capabilities", function () { updatePowerButtons(); initialResize(); }); rfb.addEventListener("credentialsrequired", credentials); rfb.addEventListener("desktopname", updateDesktopName); From d472f3f19ef8cb9000ccfdf854d9f40e587fac0a Mon Sep 17 00:00:00 2001 From: Samuel Mannehed Date: Sat, 11 Nov 2017 01:48:47 +0100 Subject: [PATCH 6/9] Abstract RFB errors to avoid sending strings The API allowed strings to be passed from the RFB module to the application using the disconnect reason. This caused problems since the application didn't have control over translations for these strings. Most of the information being passed using this string was very technical and not helpful to the end user. One exception to this was the security result information regarding for example authentication failures. The protocol allows the VNC server to pass a string directly to the user in the security result. So the disconnect reason is replaced by a boolean saying if the disconnection was clean or not. And for the security result information from the server, a new event has been added. --- app/ui.js | 20 +++- core/rfb.js | 219 ++++++++++++++++++++++++------------------- docs/API.md | 32 ++++++- tests/playback-ui.js | 6 +- tests/playback.js | 7 +- tests/test.rfb.js | 75 ++++++++++----- vnc_lite.html | 6 +- 7 files changed, 232 insertions(+), 133 deletions(-) diff --git a/app/ui.js b/app/ui.js index 0ee0c00b..cadf44b1 100644 --- a/app/ui.js +++ b/app/ui.js @@ -1023,6 +1023,7 @@ var UI = { UI.rfb.addEventListener("connect", UI.connectFinished); UI.rfb.addEventListener("disconnect", UI.disconnectFinished); UI.rfb.addEventListener("credentialsrequired", UI.credentials); + UI.rfb.addEventListener("securityfailure", UI.securityFailed); UI.rfb.addEventListener("capabilities", function () { UI.updatePowerButton(); UI.initialResize(); }); UI.rfb.addEventListener("clipboard", UI.clipboardReceive); UI.rfb.addEventListener("bell", UI.bell); @@ -1080,9 +1081,10 @@ var UI = { // UI.disconnect() won't be used in those cases. UI.connected = false; - if (typeof e.detail.reason !== 'undefined') { - UI.showStatus(e.detail.reason, 'error'); + if (!e.detail.clean) { UI.updateVisualState('disconnected'); + UI.showStatus(_("Something went wrong, connection is closed"), + 'error'); } else if (UI.getSetting('reconnect', false) === true && !UI.inhibit_reconnect) { UI.updateVisualState('reconnecting'); @@ -1098,6 +1100,20 @@ var UI = { UI.openConnectPanel(); }, + securityFailed: function (e) { + let msg = ""; + // On security failures we might get a string with a reason + // directly from the server. Note that we can't control if + // this string is translated or not. + if ('reason' in e.detail) { + msg = _("New connection has been rejected with reason: ") + + e.detail.reason; + } else { + msg = _("New connection has been rejected"); + } + UI.showStatus(msg, 'error'); + }, + cancelReconnect: function() { if (UI.reconnect_callback !== null) { clearTimeout(UI.reconnect_callback); diff --git a/core/rfb.js b/core/rfb.js index dced6b16..aa58c00f 100644 --- a/core/rfb.js +++ b/core/rfb.js @@ -11,7 +11,6 @@ */ import * as Log from './util/logging.js'; -import _ from './util/localization.js'; import { decodeUTF8 } from './util/strings.js'; import { browserSupportsCursorURIs, isTouchDevice } from './util/browsers.js'; import EventTargetMixin from './util/eventtarget.js'; @@ -54,7 +53,7 @@ export default function RFB(target, url, options) { this._rfb_connection_state = ''; this._rfb_init_state = ''; this._rfb_auth_scheme = ''; - this._rfb_disconnect_reason = ""; + this._rfb_clean_disconnect = true; // Server capabilities this._rfb_version = 0; @@ -190,38 +189,40 @@ export default function RFB(target, url, options) { this._rfb_init_state = 'ProtocolVersion'; Log.Debug("Starting VNC handshake"); } else { - this._fail("Unexpected server connection"); + this._fail("Unexpected server connection while " + + this._rfb_connection_state); } }.bind(this)); this._sock.on('close', function (e) { Log.Warn("WebSocket on-close event"); var msg = ""; if (e.code) { - msg = " (code: " + e.code; + msg = "(code: " + e.code; if (e.reason) { msg += ", reason: " + e.reason; } msg += ")"; } switch (this._rfb_connection_state) { - case 'disconnecting': - this._updateConnectionState('disconnected'); - break; case 'connecting': - this._fail('Failed to connect to server', msg); + this._fail("Connection closed " + msg); break; case 'connected': // Handle disconnects that were initiated server-side this._updateConnectionState('disconnecting'); this._updateConnectionState('disconnected'); break; + case 'disconnecting': + // Normal disconnection path + this._updateConnectionState('disconnected'); + break; case 'disconnected': - this._fail("Unexpected server disconnect", - "Already disconnected: " + msg); + this._fail("Unexpected server disconnect " + + "when already disconnected " + msg); break; default: - this._fail("Unexpected server disconnect", - "Not in any state yet: " + msg); + this._fail("Unexpected server disconnect before connecting " + + msg); break; } this._sock.off('close'); @@ -384,9 +385,9 @@ RFB.prototype = { this._sock.open(this._url, ['binary']); } catch (e) { if (e.name === 'SyntaxError') { - this._fail("Invalid host or port value given", e); + this._fail("Invalid host or port (" + e + ")"); } else { - this._fail("Error while connecting", e); + this._fail("Error when opening socket (" + e + ")"); } } @@ -539,19 +540,15 @@ RFB.prototype = { this._disconnect(); this._disconnTimer = setTimeout(function () { - this._rfb_disconnect_reason = _("Disconnect timeout"); + Log.Error("Disconnection timed out."); this._updateConnectionState('disconnected'); }.bind(this), DISCONNECT_TIMEOUT * 1000); break; case 'disconnected': - if (this._rfb_disconnect_reason !== "") { - event = new CustomEvent("disconnect", - { detail: { reason: this._rfb_disconnect_reason } }); - } else { - // No reason means clean disconnect - event = new CustomEvent("disconnect", { detail: {} }); - } + event = new CustomEvent( + "disconnect", { detail: + { clean: this._rfb_clean_disconnect } }); this.dispatchEvent(event); break; } @@ -559,29 +556,25 @@ RFB.prototype = { /* Print errors and disconnect * - * The optional parameter 'details' is used for information that + * The parameter 'details' is used for information that * should be logged but not sent to the user interface. */ - _fail: function (msg, details) { - var fullmsg = msg; - if (typeof details !== 'undefined') { - fullmsg = msg + " (" + details + ")"; - } + _fail: function (details) { switch (this._rfb_connection_state) { case 'disconnecting': - Log.Error("Failed when disconnecting: " + fullmsg); + Log.Error("Failed when disconnecting: " + details); break; case 'connected': - Log.Error("Failed while connected: " + fullmsg); + Log.Error("Failed while connected: " + details); break; case 'connecting': - Log.Error("Failed when connecting: " + fullmsg); + Log.Error("Failed when connecting: " + details); break; default: - Log.Error("RFB failure: " + fullmsg); + Log.Error("RFB failure: " + details); break; } - this._rfb_disconnect_reason = msg; //This is sent to the UI + this._rfb_clean_disconnect = false; //This is sent to the UI // Transition to disconnected without waiting for socket to close this._updateConnectionState('disconnecting'); @@ -693,8 +686,7 @@ RFB.prototype = { _negotiate_protocol_version: function () { if (this._sock.rQlen() < 12) { - return this._fail("Error while negotiating with server", - "Incomplete protocol version"); + return this._fail("Received incomplete protocol version."); } var sversion = this._sock.rQshiftStr(12).substr(4, 7); @@ -719,8 +711,7 @@ RFB.prototype = { this._rfb_version = 3.8; break; default: - return this._fail("Unsupported server", - "Invalid server version: " + sversion); + return this._fail("Invalid server version " + sversion); } if (is_repeater) { @@ -762,10 +753,7 @@ RFB.prototype = { if (this._sock.rQwait("security type", num_types, 1)) { return false; } if (num_types === 0) { - var strlen = this._sock.rQshift32(); - var reason = this._sock.rQshiftStr(strlen); - return this._fail("Error while negotiating with server", - "Security failure: " + reason); + return this._handle_security_failure("no security types"); } var types = this._sock.rQshiftBytes(num_types); @@ -782,8 +770,7 @@ RFB.prototype = { } else if (includes(2, types)) { this._rfb_auth_scheme = 2; // VNC Auth } else { - return this._fail("Unsupported server", - "Unsupported security types: " + types); + return this._fail("Unsupported security types (types: " + types + ")"); } this._sock.send([this._rfb_auth_scheme]); @@ -799,6 +786,59 @@ RFB.prototype = { return this._init_msg(); // jump to authentication }, + /* + * Get the security failure reason if sent from the server and + * send the 'securityfailure' event. + * + * - The optional parameter context can be used to add some extra + * context to the log output. + * + * - The optional parameter security_result_status can be used to + * add a custom status code to the event. + */ + _handle_security_failure: function (context, security_result_status) { + + if (typeof context === 'undefined') { + context = ""; + } else { + context = " on " + context; + } + + if (typeof security_result_status === 'undefined') { + security_result_status = 1; // fail + } + + if (this._sock.rQwait("reason length", 4)) { + return false; + } + let strlen = this._sock.rQshift32(); + let reason = ""; + + if (strlen > 0) { + if (this._sock.rQwait("reason", strlen, 8)) { return false; } + reason = this._sock.rQshiftStr(strlen); + } + + if (reason !== "") { + + let event = new CustomEvent( + "securityfailure", + { detail: { status: security_result_status, reason: reason } }); + this.dispatchEvent(event); + + return this._fail("Security negotiation failed" + context + + " (reason: " + reason + ")"); + } else { + + let event = new CustomEvent( + "securityfailure", + { detail: { status: security_result_status } }); + this.dispatchEvent(event); + + return this._fail("Security negotiation failed" + context); + } + }, + // authentication _negotiate_xvp_auth: function () { if (!this._rfb_credentials.username || @@ -854,15 +894,13 @@ RFB.prototype = { if (serverSupportedTunnelTypes[0]) { if (serverSupportedTunnelTypes[0].vendor != clientSupportedTunnelTypes[0].vendor || serverSupportedTunnelTypes[0].signature != clientSupportedTunnelTypes[0].signature) { - return this._fail("Unsupported server", - "Client's tunnel type had the incorrect " + + return this._fail("Client's tunnel type had the incorrect " + "vendor or signature"); } this._sock.send([0, 0, 0, 0]); // use NOTUNNEL return false; // wait until we receive the sub auth count to continue } else { - return this._fail("Unsupported server", - "Server wanted tunnels, but doesn't support " + + return this._fail("Server wanted tunnels, but doesn't support " + "the notunnel type"); } }, @@ -916,24 +954,19 @@ RFB.prototype = { this._rfb_auth_scheme = 2; return this._init_msg(); default: - return this._fail("Unsupported server", - "Unsupported tiny auth scheme: " + - authType); + return this._fail("Unsupported tiny auth scheme " + + "(scheme: " + authType + ")"); } } } - return this._fail("Unsupported server", - "No supported sub-auth types!"); + return this._fail("No supported sub-auth types!"); }, _negotiate_authentication: function () { switch (this._rfb_auth_scheme) { case 0: // connection failed - if (this._sock.rQwait("auth reason", 4)) { return false; } - var strlen = this._sock.rQshift32(); - var reason = this._sock.rQshiftStr(strlen); - return this._fail("Authentication failure", reason); + return this._handle_security_failure("authentication scheme"); case 1: // no auth if (this._rfb_version >= 3.8) { @@ -953,33 +986,30 @@ RFB.prototype = { return this._negotiate_tight_auth(); default: - return this._fail("Unsupported server", - "Unsupported auth scheme: " + - this._rfb_auth_scheme); + return this._fail("Unsupported auth scheme (scheme: " + + this._rfb_auth_scheme + ")"); } }, _handle_security_result: function () { if (this._sock.rQwait('VNC auth response ', 4)) { return false; } - switch (this._sock.rQshift32()) { - case 0: // OK - this._rfb_init_state = 'ClientInitialisation'; - Log.Debug('Authentication OK'); - return this._init_msg(); - case 1: // failed - if (this._rfb_version >= 3.8) { - var length = this._sock.rQshift32(); - if (this._sock.rQwait("SecurityResult reason", length, 8)) { return false; } - var reason = this._sock.rQshiftStr(length); - return this._fail("Authentication failure", reason); - } else { - return this._fail("Authentication failure"); - } - case 2: - return this._fail("Too many authentication attempts"); - default: - return this._fail("Unsupported server", - "Unknown SecurityResult"); + + let status = this._sock.rQshift32(); + + if (status === 0) { // OK + this._rfb_init_state = 'ClientInitialisation'; + Log.Debug('Authentication OK'); + return this._init_msg(); + } else { + if (this._rfb_version >= 3.8) { + return this._handle_security_failure("security result", status); + } else { + let event = new CustomEvent("securityfailure", + { detail: { status: status } }); + this.dispatchEvent(event); + + return this._fail("Security handshake failed"); + } } }, @@ -1158,15 +1188,15 @@ RFB.prototype = { return this._negotiate_server_init(); default: - return this._fail("Internal error", "Unknown init state: " + - this._rfb_init_state); + return this._fail("Unknown init state (state: " + + this._rfb_init_state + ")"); } }, _handle_set_colour_map_msg: function () { Log.Debug("SetColorMapEntries"); - return this._fail("Protocol error", "Unexpected SetColorMapEntries message"); + return this._fail("Unexpected SetColorMapEntries message"); }, _handle_server_cut_text: function () { @@ -1215,8 +1245,7 @@ RFB.prototype = { */ if (!(flags & (1<<31))) { - return this._fail("Internal error", - "Unexpected fence response"); + return this._fail("Unexpected fence response"); } // Filter out unsupported flags @@ -1247,8 +1276,7 @@ RFB.prototype = { this._setCapability("power", true); break; default: - this._fail("Unexpected server message", - "Illegal server XVP message " + xvp_msg); + this._fail("Illegal server XVP message (msg: " + xvp_msg + ")"); break; } @@ -1306,7 +1334,7 @@ RFB.prototype = { return this._handle_xvp_msg(); default: - this._fail("Unexpected server message", "Type:" + msg_type); + this._fail("Unexpected server message (type " + msg_type + ")"); Log.Debug("sock.rQslice(0, 30): " + this._sock.rQslice(0, 30)); return true; } @@ -1361,9 +1389,8 @@ RFB.prototype = { (hdr[10] << 8) + hdr[11], 10); if (!this._encHandlers[this._FBU.encoding]) { - this._fail("Unexpected server message", - "Unsupported encoding " + - this._FBU.encoding); + this._fail("Unsupported encoding (encoding: " + + this._FBU.encoding + ")"); return false; } } @@ -1857,8 +1884,8 @@ RFB.encodingHandlers = { if (this._sock.rQwait("HEXTILE subencoding", this._FBU.bytes)) { return false; } var subencoding = rQ[rQi]; // Peek if (subencoding > 30) { // Raw - this._fail("Unexpected server message", - "Illegal hextile subencoding: " + subencoding); + this._fail("Illegal hextile subencoding (subencoding: " + + subencoding + ")"); return false; } @@ -2187,9 +2214,8 @@ RFB.encodingHandlers = { else if (ctl === 0x0A) cmode = "png"; else if (ctl & 0x04) cmode = "filter"; else if (ctl < 0x04) cmode = "copy"; - else return this._fail("Unexpected server message", - "Illegal tight compression received, " + - "ctl: " + ctl); + else return this._fail("Illegal tight compression received (ctl: " + + ctl + ")"); switch (cmode) { // fill use depth because TPIXELs drop the padding byte @@ -2249,9 +2275,8 @@ RFB.encodingHandlers = { } else { // Filter 0, Copy could be valid here, but servers don't send it as an explicit filter // Filter 2, Gradient is valid but not use if jpeg is enabled - this._fail("Unexpected server message", - "Unsupported tight subencoding received, " + - "filter: " + filterId); + this._fail("Unsupported tight subencoding received " + + "(filter: " + filterId + ")"); } break; case "copy": diff --git a/docs/API.md b/docs/API.md index e194e969..4d4f4d51 100644 --- a/docs/API.md +++ b/docs/API.md @@ -74,6 +74,10 @@ protocol stream. - The `credentialsrequired` event is fired when more credentials must be given to continue. +[`securityfailure`](#securityfailure) + - The `securityfailure` event is fired when the security negotiation + with the server fails. + [`clipboard`](#clipboard) - The `clipboard` event is fired when clipboard data is received from the server. @@ -186,10 +190,10 @@ the `RFB` object is ready to recieve graphics updates and to send input. #### disconnect The `disconnect` event is fired when the connection has been -terminated. The `detail` property is an `Object` the optionally -contains the property `reason`. `reason` is a `DOMString` specifying -the reason in the event of an unexpected termination. `reason` will be -omitted for a clean termination. +terminated. The `detail` property is an `Object` that contains the +property `clean`. `clean` is a `boolean` indicating if the termination +was clean or not. In the event of an unexpected termination or an error +`clean` will be set to false. #### credentialsrequired @@ -198,6 +202,26 @@ credentials than were specified to [`RFB()`](#rfb-1). The `detail` property is an `Object` containing the property `types` which is an `Array` of `DOMString` listing the credentials that are required. +#### securityfailure + +The `securityfailure` event is fired when the handshaking process with +the server fails during the security negotiation step. The `detail` +property is an `Object` containing the following properties: + +| Property | Type | Description +| -------- | ----------- | ----------- +| `status` | `long` | The failure status code +| `reason` | `DOMString` | The **optional** reason for the failure + +The property `status` corresponds to the +[SecurityResult](https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#securityresult) +status code in cases of failure. A status of zero will not be sent in +this event since that indicates a successful security handshaking +process. The optional property `reason` is provided by the server and +thus the language of the string is not known. However most servers will +probably send English strings. The server can choose to not send a +reason and in these cases the `reason` property will be omitted. + #### clipboard The `clipboard` event is fired when the server has sent clipboard data. diff --git a/tests/playback-ui.js b/tests/playback-ui.js index 03b08fe1..01ad241e 100644 --- a/tests/playback-ui.js +++ b/tests/playback-ui.js @@ -115,13 +115,13 @@ IterationPlayer.prototype = { this._nextIteration(); }, - _disconnected: function (rfb, reason, frame) { - if (reason) { + _disconnected: function (rfb, clean, frame) { + if (!clean) { this._state = 'failed'; } var evt = new Event('rfbdisconnected'); - evt.reason = reason; + evt.clean = clean; evt.frame = frame; this.onrfbdisconnected(evt); diff --git a/tests/playback.js b/tests/playback.js index 9199eba0..c769e88f 100644 --- a/tests/playback.js +++ b/tests/playback.js @@ -78,7 +78,8 @@ RecordingPlayer.prototype = { // initialize a new RFB this._rfb = new RFB(document.getElementById('VNC_canvas'), 'wss://test'); this._rfb.viewOnly = true; - this._rfb.ondisconnected = this._handleDisconnect.bind(this); + this._rfb.addEventListener("disconnect", + this._handleDisconnect.bind(this)); this._enablePlaybackMode(); // reset the frame index and timer @@ -186,8 +187,8 @@ RecordingPlayer.prototype = { } }, - _handleDisconnect(rfb, reason) { + _handleDisconnect(rfb, clean) { this._running = false; - this._disconnected(rfb, reason, this._frame_index); + this._disconnected(rfb, clean, this._frame_index); } }; diff --git a/tests/test.rfb.js b/tests/test.rfb.js index 2ef61240..81ee1dd6 100644 --- a/tests/test.rfb.js +++ b/tests/test.rfb.js @@ -377,26 +377,21 @@ describe('Remote Frame Buffer Protocol Client', function() { expect(client._rfb_connection_state).to.equal('disconnected'); }); - it('should set disconnect_reason', function () { + it('should set clean_disconnect variable', function () { + client._rfb_clean_disconnect = true; client._rfb_connection_state = 'connected'; - client._fail('a reason'); - expect(client._rfb_disconnect_reason).to.equal('a reason'); + client._fail(); + expect(client._rfb_clean_disconnect).to.be.false; }); - it('should not include details in disconnect_reason', function () { - client._rfb_connection_state = 'connected'; - client._fail('a reason', 'details'); - expect(client._rfb_disconnect_reason).to.equal('a reason'); - }); - - it('should result in disconnect callback with message when reason given', function () { + it('should result in disconnect event with clean set to false', function () { client._rfb_connection_state = 'connected'; var spy = sinon.spy(); client.addEventListener("disconnect", spy); - client._fail('a reason'); + client._fail(); this.clock.tick(2000); expect(spy).to.have.been.calledOnce; - expect(spy.args[0][0].detail.reason).to.equal('a reason'); + expect(spy.args[0][0].detail.clean).to.be.false; }); }); @@ -471,17 +466,16 @@ describe('Remote Frame Buffer Protocol Client', function() { var client; beforeEach(function () { client = make_rfb(); }); - it('should call the disconnect callback if the state is "disconnected"', function () { + it('should result in a disconnect event if state becomes "disconnected"', function () { var spy = sinon.spy(); client.addEventListener("disconnect", spy); client._rfb_connection_state = 'disconnecting'; - client._rfb_disconnect_reason = "error"; client._updateConnectionState('disconnected'); expect(spy).to.have.been.calledOnce; - expect(spy.args[0][0].detail.reason).to.equal("error"); + expect(spy.args[0][0].detail.clean).to.be.true; }); - it('should not call the disconnect callback if the state is not "disconnected"', function () { + it('should not result in a disconnect event if the state is not "disconnected"', function () { var spy = sinon.spy(); client.addEventListener("disconnect", spy); client._sock._websocket.close = function () {}; // explicitly don't call onclose @@ -489,7 +483,7 @@ describe('Remote Frame Buffer Protocol Client', function() { expect(spy).to.not.have.been.called; }); - it('should call the disconnect callback without msg when no reason given', function () { + it('should result in a disconnect event without msg when no reason given', function () { var spy = sinon.spy(); client.addEventListener("disconnect", spy); client._rfb_connection_state = 'disconnecting'; @@ -653,7 +647,7 @@ describe('Remote Frame Buffer Protocol Client', function() { expect(client._fail).to.have.been.calledOnce; expect(client._fail).to.have.been.calledWith( - 'Error while negotiating with server','Security failure: whoops'); + 'Security negotiation failed on no security types (reason: whoops)'); }); it('should transition to the Authentication state and continue on successful negotiation', function () { @@ -688,7 +682,7 @@ describe('Remote Frame Buffer Protocol Client', function() { sinon.spy(client, '_fail'); client._sock._websocket._receive_data(new Uint8Array(data)); expect(client._fail).to.have.been.calledWith( - 'Authentication failure', 'Whoopsies'); + 'Security negotiation failed on authentication scheme (reason: Whoopsies)'); }); it('should transition straight to SecurityResult on "no auth" (1) for versions >= 3.8', function () { @@ -909,14 +903,53 @@ describe('Remote Frame Buffer Protocol Client', function() { var failure_data = [0, 0, 0, 1, 0, 0, 0, 6, 119, 104, 111, 111, 112, 115]; client._sock._websocket._receive_data(new Uint8Array(failure_data)); expect(client._fail).to.have.been.calledWith( - 'Authentication failure', 'whoops'); + 'Security negotiation failed on security result (reason: whoops)'); }); it('should fail on an error code of 1 with a standard message for version < 3.8', function () { sinon.spy(client, '_fail'); client._rfb_version = 3.7; client._sock._websocket._receive_data(new Uint8Array([0, 0, 0, 1])); - expect(client._fail).to.have.been.calledWith('Authentication failure'); + expect(client._fail).to.have.been.calledWith( + 'Security handshake failed'); + }); + + it('should result in securityfailure event when receiving a non zero status', function () { + var spy = sinon.spy(); + client.addEventListener("securityfailure", spy); + client._sock._websocket._receive_data(new Uint8Array([0, 0, 0, 2])); + expect(spy).to.have.been.calledOnce; + expect(spy.args[0][0].detail.status).to.equal(2); + }); + + it('should include reason when provided in securityfailure event', function () { + client._rfb_version = 3.8; + var spy = sinon.spy(); + client.addEventListener("securityfailure", spy); + var failure_data = [0, 0, 0, 1, 0, 0, 0, 12, 115, 117, 99, 104, + 32, 102, 97, 105, 108, 117, 114, 101]; + client._sock._websocket._receive_data(new Uint8Array(failure_data)); + expect(spy.args[0][0].detail.status).to.equal(1); + expect(spy.args[0][0].detail.reason).to.equal('such failure'); + }); + + it('should not include reason when length is zero in securityfailure event', function () { + client._rfb_version = 3.9; + var spy = sinon.spy(); + client.addEventListener("securityfailure", spy); + var failure_data = [0, 0, 0, 1, 0, 0, 0, 0]; + client._sock._websocket._receive_data(new Uint8Array(failure_data)); + expect(spy.args[0][0].detail.status).to.equal(1); + expect('reason' in spy.args[0][0].detail).to.be.false; + }); + + it('should not include reason in securityfailure event for version < 3.8', function () { + client._rfb_version = 3.6; + var spy = sinon.spy(); + client.addEventListener("securityfailure", spy); + client._sock._websocket._receive_data(new Uint8Array([0, 0, 0, 2])); + expect(spy.args[0][0].detail.status).to.equal(2); + expect('reason' in spy.args[0][0].detail).to.be.false; }); }); diff --git a/vnc_lite.html b/vnc_lite.html index b904b4a5..762da013 100644 --- a/vnc_lite.html +++ b/vnc_lite.html @@ -162,10 +162,10 @@ function disconnected(e) { document.getElementById('sendCtrlAltDelButton').disabled = true; updatePowerButtons(); - if (typeof(e.detail.reason) !== 'undefined') { - status(e.detail.reason, "error"); - } else { + if (e.detail.clean) { status("Disconnected", "normal"); + } else { + status("Something went wrong, connection is closed", "error"); } } From d623a029d6bc2347781d85a40b67645eec2b8300 Mon Sep 17 00:00:00 2001 From: Samuel Mannehed Date: Sat, 11 Nov 2017 01:59:13 +0100 Subject: [PATCH 7/9] Dont overwrite more severe visible statuses And only show the first error. This means that if UI.showStatus() is called for a new error while one error is already showing, the new error will not be shown. However, if a warning was showing and a new error comes up, the warning will be overwritten. --- app/ui.js | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/app/ui.js b/app/ui.js index cadf44b1..d43df751 100644 --- a/app/ui.js +++ b/app/ui.js @@ -479,21 +479,40 @@ var UI = { status_type = 'normal'; } - statusElem.classList.remove("noVNC_status_normal"); - statusElem.classList.remove("noVNC_status_warn"); - statusElem.classList.remove("noVNC_status_error"); + // Don't overwrite more severe visible statuses and never + // errors. Only shows the first error. + let visible_status_type = 'none'; + if (statusElem.classList.contains("noVNC_open")) { + if (statusElem.classList.contains("noVNC_status_error")) { + visible_status_type = 'error'; + } else if (statusElem.classList.contains("noVNC_status_warn")) { + visible_status_type = 'warn'; + } else { + visible_status_type = 'normal'; + } + } + if (visible_status_type === 'error' || + (visible_status_type === 'warn' && status_type === 'normal')) { + return; + } switch (status_type) { + case 'error': + statusElem.classList.remove("noVNC_status_warn"); + statusElem.classList.remove("noVNC_status_normal"); + statusElem.classList.add("noVNC_status_error"); + break; case 'warning': case 'warn': + statusElem.classList.remove("noVNC_status_error"); + statusElem.classList.remove("noVNC_status_normal"); statusElem.classList.add("noVNC_status_warn"); break; - case 'error': - statusElem.classList.add("noVNC_status_error"); - break; case 'normal': case 'info': default: + statusElem.classList.remove("noVNC_status_error"); + statusElem.classList.remove("noVNC_status_warn"); statusElem.classList.add("noVNC_status_normal"); break; } @@ -993,6 +1012,8 @@ var UI = { password = undefined; } + UI.hideStatus(); + if (!host) { Log.Error("Can't connect when host is: " + host); UI.showStatus(_("Must set host"), 'error'); From 689580381c9f96212213bd3dd65b94b5b4e36222 Mon Sep 17 00:00:00 2001 From: Samuel Mannehed Date: Sat, 11 Nov 2017 02:31:19 +0100 Subject: [PATCH 8/9] Move UI.cancelReconnect() to related functions --- app/ui.js | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/app/ui.js b/app/ui.js index d43df751..08ce449c 100644 --- a/app/ui.js +++ b/app/ui.js @@ -1077,6 +1077,18 @@ var UI = { UI.connect(null, UI.reconnect_password); }, + cancelReconnect: function() { + if (UI.reconnect_callback !== null) { + clearTimeout(UI.reconnect_callback); + UI.reconnect_callback = null; + } + + UI.updateVisualState('disconnected'); + + UI.openControlbar(); + UI.openConnectPanel(); + }, + connectFinished: function (e) { UI.connected = true; UI.inhibit_reconnect = false; @@ -1135,18 +1147,6 @@ var UI = { UI.showStatus(msg, 'error'); }, - cancelReconnect: function() { - if (UI.reconnect_callback !== null) { - clearTimeout(UI.reconnect_callback); - UI.reconnect_callback = null; - } - - UI.updateVisualState('disconnected'); - - UI.openControlbar(); - UI.openConnectPanel(); - }, - /* ------^------- * /CONNECTION * ============== From 7279364c9abf7131c4cc292955e0305e5adca2b7 Mon Sep 17 00:00:00 2001 From: Samuel Mannehed Date: Mon, 13 Nov 2017 12:12:41 +0100 Subject: [PATCH 9/9] Move localization.js to app Since it is no longer used in core. Also splits localization tests into a separate file. --- {core/util => app}/localization.js | 0 app/ui.js | 2 +- karma.conf.js | 2 + tests/test.localization.js | 76 ++++++++++++++++++++++++++++++ tests/test.util.js | 67 -------------------------- 5 files changed, 79 insertions(+), 68 deletions(-) rename {core/util => app}/localization.js (100%) create mode 100644 tests/test.localization.js diff --git a/core/util/localization.js b/app/localization.js similarity index 100% rename from core/util/localization.js rename to app/localization.js diff --git a/app/ui.js b/app/ui.js index 08ce449c..3bd74c88 100644 --- a/app/ui.js +++ b/app/ui.js @@ -12,7 +12,7 @@ /* global window, document.getElementById, Util, WebUtil, RFB, Display */ import * as Log from '../core/util/logging.js'; -import _, { l10n } from '../core/util/localization.js'; +import _, { l10n } from './localization.js'; import { isTouchDevice } from '../core/util/browsers.js'; import { setCapture, getPointerEvent } from '../core/util/events.js'; import KeyTable from "../core/input/keysym.js"; diff --git a/karma.conf.js b/karma.conf.js index 1060ee5f..10bf372a 100644 --- a/karma.conf.js +++ b/karma.conf.js @@ -61,6 +61,7 @@ module.exports = function(config) { files: [ { pattern: 'vendor/sinon.js', included: false }, { pattern: 'node_modules/sinon-chai/lib/sinon-chai.js', included: false }, + { pattern: 'app/localization.js', included: false }, { pattern: 'core/**/*.js', included: false }, { pattern: 'vendor/pako/**/*.js', included: false }, { pattern: 'tests/test.*.js', included: false }, @@ -90,6 +91,7 @@ module.exports = function(config) { // preprocess matching files before serving them to the browser // available preprocessors: https://npmjs.org/browse/keyword/karma-preprocessor preprocessors: { + 'app/localization.js': ['babel'], 'core/**/*.js': ['babel'], 'tests/test.*.js': ['babel'], 'tests/fake.*.js': ['babel'], diff --git a/tests/test.localization.js b/tests/test.localization.js new file mode 100644 index 00000000..339144a5 --- /dev/null +++ b/tests/test.localization.js @@ -0,0 +1,76 @@ +/* jshint expr: true */ + +var assert = chai.assert; +var expect = chai.expect; + +import l10nGet, { l10n } from '../app/localization.js'; + +describe('Localization', function() { + "use strict"; + + describe('language selection', function () { + var origNavigator; + beforeEach(function () { + // window.navigator is a protected read-only property in many + // environments, so we need to redefine it whilst running these + // tests. + origNavigator = Object.getOwnPropertyDescriptor(window, "navigator"); + if (origNavigator === undefined) { + // Object.getOwnPropertyDescriptor() doesn't work + // properly in any version of IE + this.skip(); + } + + Object.defineProperty(window, "navigator", {value: {}}); + if (window.navigator.languages !== undefined) { + // Object.defineProperty() doesn't work properly in old + // versions of Chrome + this.skip(); + } + + window.navigator.languages = []; + }); + afterEach(function () { + Object.defineProperty(window, "navigator", origNavigator); + }); + + it('should use English by default', function() { + expect(l10n.language).to.equal('en'); + }); + it('should use English if no user language matches', function() { + window.navigator.languages = ["nl", "de"]; + l10n.setup(["es", "fr"]); + expect(l10n.language).to.equal('en'); + }); + it('should use the most preferred user language', function() { + window.navigator.languages = ["nl", "de", "fr"]; + l10n.setup(["es", "fr", "de"]); + expect(l10n.language).to.equal('de'); + }); + it('should prefer sub-languages languages', function() { + window.navigator.languages = ["pt-BR"]; + l10n.setup(["pt", "pt-BR"]); + expect(l10n.language).to.equal('pt-BR'); + }); + it('should fall back to language "parents"', function() { + window.navigator.languages = ["pt-BR"]; + l10n.setup(["fr", "pt", "de"]); + expect(l10n.language).to.equal('pt'); + }); + it('should not use specific language when user asks for a generic language', function() { + window.navigator.languages = ["pt", "de"]; + l10n.setup(["fr", "pt-BR", "de"]); + expect(l10n.language).to.equal('de'); + }); + it('should handle underscore as a separator', function() { + window.navigator.languages = ["pt-BR"]; + l10n.setup(["pt_BR"]); + expect(l10n.language).to.equal('pt_BR'); + }); + it('should handle difference in case', function() { + window.navigator.languages = ["pt-br"]; + l10n.setup(["pt-BR"]); + expect(l10n.language).to.equal('pt-BR'); + }); + }); +}); diff --git a/tests/test.util.js b/tests/test.util.js index 12f41d62..1eab1e99 100644 --- a/tests/test.util.js +++ b/tests/test.util.js @@ -4,7 +4,6 @@ var assert = chai.assert; var expect = chai.expect; import * as Log from '../core/util/logging.js'; -import l10nGet, { l10n } from '../core/util/localization.js'; import sinon from '../vendor/sinon.js'; @@ -63,72 +62,6 @@ describe('Utils', function() { }); }); - describe('language selection', function () { - var origNavigator; - beforeEach(function () { - // window.navigator is a protected read-only property in many - // environments, so we need to redefine it whilst running these - // tests. - origNavigator = Object.getOwnPropertyDescriptor(window, "navigator"); - if (origNavigator === undefined) { - // Object.getOwnPropertyDescriptor() doesn't work - // properly in any version of IE - this.skip(); - } - - Object.defineProperty(window, "navigator", {value: {}}); - if (window.navigator.languages !== undefined) { - // Object.defineProperty() doesn't work properly in old - // versions of Chrome - this.skip(); - } - - window.navigator.languages = []; - }); - afterEach(function () { - Object.defineProperty(window, "navigator", origNavigator); - }); - - it('should use English by default', function() { - expect(l10n.language).to.equal('en'); - }); - it('should use English if no user language matches', function() { - window.navigator.languages = ["nl", "de"]; - l10n.setup(["es", "fr"]); - expect(l10n.language).to.equal('en'); - }); - it('should use the most preferred user language', function() { - window.navigator.languages = ["nl", "de", "fr"]; - l10n.setup(["es", "fr", "de"]); - expect(l10n.language).to.equal('de'); - }); - it('should prefer sub-languages languages', function() { - window.navigator.languages = ["pt-BR"]; - l10n.setup(["pt", "pt-BR"]); - expect(l10n.language).to.equal('pt-BR'); - }); - it('should fall back to language "parents"', function() { - window.navigator.languages = ["pt-BR"]; - l10n.setup(["fr", "pt", "de"]); - expect(l10n.language).to.equal('pt'); - }); - it('should not use specific language when user asks for a generic language', function() { - window.navigator.languages = ["pt", "de"]; - l10n.setup(["fr", "pt-BR", "de"]); - expect(l10n.language).to.equal('de'); - }); - it('should handle underscore as a separator', function() { - window.navigator.languages = ["pt-BR"]; - l10n.setup(["pt_BR"]); - expect(l10n.language).to.equal('pt_BR'); - }); - it('should handle difference in case', function() { - window.navigator.languages = ["pt-br"]; - l10n.setup(["pt-BR"]); - expect(l10n.language).to.equal('pt-BR'); - }); - }); - // TODO(directxman12): test the conf_default and conf_defaults methods // TODO(directxman12): test decodeUTF8 // TODO(directxman12): test the event methods (addEvent, removeEvent, stopEvent)