From d1548572d4389bb19848e8bba21ecce858ba86aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Astori?= Date: Tue, 20 Mar 2018 00:52:58 -0400 Subject: [PATCH 1/2] Move the sign out button to the settings and empty local storage on sign out This change improves privacy/security by ensuring all local storage (which includes settings, etc.) is destroyed on sign out or when revoking a remote session. Because signing out is now more "risky", the button has been moved to the settings along with other existing sessions. This commit: - Removes the power/sign-out icon from the sidebar footer (gives additional room for when the admin panel gets added) - Adds a "Sign out" button next to the current session info in the settings session list - Renames "Disconnect" buttons into "Revoke" to better clarify the intent (I will admit that I borrowed the wording from Telegram) - Fixes incorrect `localStorage.remove` method - Uses Sinon.js to mock wrappers for `window.localStorage` and `window.location` (does not mock those themselves, in the "Do not mock what you do not own" fashion, mock our layer instead). I expect we will be able to test a bit more from the UI with this. A good next candidate will be the `mockLogger` things. --- client/css/style.css | 12 ++--- client/index.html.tpl | 1 - client/js/auth.js | 11 +++++ client/js/localStorage.js | 7 ++- client/js/location.js | 9 ++++ client/js/lounge.js | 9 ---- client/js/socket-events/sessions_list.js | 10 +++- client/js/socket-events/sign_out.js | 5 +- client/themes/crypto.css | 1 - client/views/session.tpl | 20 +++++--- package.json | 1 + test/client/js/authTest.js | 31 ++++++++++++ yarn.lock | 62 +++++++++++++++++++++++- 13 files changed, 144 insertions(+), 35 deletions(-) create mode 100644 client/js/auth.js create mode 100644 client/js/location.js create mode 100644 test/client/js/authTest.js diff --git a/client/css/style.css b/client/css/style.css index d05b73f5..9e479251 100644 --- a/client/css/style.css +++ b/client/css/style.css @@ -302,7 +302,6 @@ kbd { #footer .connect::before { content: "\f067"; /* http://fontawesome.io/icon/plus/ */ } #footer .settings::before { content: "\f013"; /* http://fontawesome.io/icon/cog/ */ } #footer .help::before { content: "\f059"; /* http://fontawesome.io/icon/question/ */ } -#footer .sign-out::before { content: "\f011"; /* http://fontawesome.io/icon/power-off/ */ } #form #submit::before { content: "\f1d8"; /* http://fontawesome.io/icon/paper-plane/ */ } @@ -526,7 +525,6 @@ kbd { } #sidebar .chan, -#sidebar .sign-out, #sidebar .empty { color: #99a2b4; font-size: 14px; @@ -537,8 +535,7 @@ kbd { } #sidebar button, -#sidebar .chan, -#sidebar .sign-out { +#sidebar .chan { cursor: pointer; } @@ -798,13 +795,11 @@ kbd { display: inline-block; } -.signed-out #footer .connect, -.signed-out #footer .sign-out { +.signed-out #footer .connect { display: none; } -.public #footer .sign-in, -.public #footer .sign-out { +.public #footer .sign-in { display: none; } @@ -2260,7 +2255,6 @@ part/quit messages where we don't load previews (adds a blank line otherwise) */ #sidebar button, #sidebar .chan, - #sidebar .sign-out, #sidebar .empty, #windows label, #windows .header .topic, diff --git a/client/index.html.tpl b/client/index.html.tpl index 86f9cd7f..40593442 100644 --- a/client/index.html.tpl +++ b/client/index.html.tpl @@ -41,7 +41,6 @@ - diff --git a/client/js/auth.js b/client/js/auth.js new file mode 100644 index 00000000..5d8982fb --- /dev/null +++ b/client/js/auth.js @@ -0,0 +1,11 @@ +"use strict"; + +const localStorage = require("./localStorage"); +const location = require("./location"); + +module.exports = class Auth { + static signout() { + localStorage.clear(); + location.reload(); + } +}; diff --git a/client/js/localStorage.js b/client/js/localStorage.js index 728a6c9f..ebe07b7b 100644 --- a/client/js/localStorage.js +++ b/client/js/localStorage.js @@ -13,7 +13,10 @@ module.exports = { get(key) { return window.localStorage.getItem(key); }, - remove(key, value) { - window.localStorage.removeItem(key, value); + remove(key) { + window.localStorage.removeItem(key); + }, + clear() { + window.localStorage.clear(); }, }; diff --git a/client/js/location.js b/client/js/location.js new file mode 100644 index 00000000..eed5e273 --- /dev/null +++ b/client/js/location.js @@ -0,0 +1,9 @@ +"use strict"; + +// This is a thin wrapper around `window.location`, in order to contain the +// side-effects. Do not add logic to it as it cannot be tested, only mocked. +module.exports = { + reload() { + window.location.reload(); + }, +}; diff --git a/client/js/lounge.js b/client/js/lounge.js index d427b7a4..19582256 100644 --- a/client/js/lounge.js +++ b/client/js/lounge.js @@ -489,15 +489,6 @@ $(function() { $("#help").on("click", "#view-changelog, #back-to-help", openWindow); $("#changelog").on("click", "#back-to-help", openWindow); - sidebar.on("click", "#sign-out", function() { - socket.emit("sign-out"); - storage.remove("token"); - - if (!socket.connected) { - location.reload(); - } - }); - function closeChan(chan) { let cmd = "/close"; diff --git a/client/js/socket-events/sessions_list.js b/client/js/socket-events/sessions_list.js index 9d77df46..91e2a57a 100644 --- a/client/js/socket-events/sessions_list.js +++ b/client/js/socket-events/sessions_list.js @@ -1,6 +1,7 @@ "use strict"; const $ = require("jquery"); +const Auth = require("../auth"); const socket = require("../socket"); const templates = require("../../views"); @@ -25,7 +26,12 @@ socket.on("sessions:list", function(data) { }); $("#settings").on("click", ".remove-session", function() { - socket.emit("sign-out", $(this).data("token")); + const token = $(this).data("token"); - return false; + if (token) { + socket.emit("sign-out", token); + } else { + socket.emit("sign-out"); + Auth.signout(); + } }); diff --git a/client/js/socket-events/sign_out.js b/client/js/socket-events/sign_out.js index df16dbf5..9cb5ebfb 100644 --- a/client/js/socket-events/sign_out.js +++ b/client/js/socket-events/sign_out.js @@ -1,9 +1,8 @@ "use strict"; const socket = require("../socket"); -const storage = require("../localStorage"); +const Auth = require("../auth"); socket.on("sign-out", function() { - storage.remove("token"); - location.reload(); + Auth.signout(); }); diff --git a/client/themes/crypto.css b/client/themes/crypto.css index e34671c3..207709b4 100644 --- a/client/themes/crypto.css +++ b/client/themes/crypto.css @@ -74,7 +74,6 @@ a:hover, #sidebar button, #sidebar .chan, -#sidebar .sign-out, #chat .time, #chat .count::before, #sidebar .empty { diff --git a/client/views/session.tpl b/client/views/session.tpl index d3d5cbd8..7951bfc4 100644 --- a/client/views/session.tpl +++ b/client/views/session.tpl @@ -1,17 +1,25 @@

-{{#if current}} - {{agent}} - {{ip}} -{{else}} - + {{agent}} + {{ip}} + +{{#unless current}}
{{#if active}} Currently active {{else}} Last used on {{/if}} -{{/if}} +{{/unless}}

diff --git a/package.json b/package.json index 290fa134..ff6ee529 100644 --- a/package.json +++ b/package.json @@ -88,6 +88,7 @@ "mousetrap": "1.6.1", "npm-run-all": "4.1.2", "nyc": "11.6.0", + "sinon": "4.4.6", "socket.io-client": "2.0.4", "stylelint": "9.1.3", "stylelint-config-standard": "18.2.0", diff --git a/test/client/js/authTest.js b/test/client/js/authTest.js new file mode 100644 index 00000000..b551d954 --- /dev/null +++ b/test/client/js/authTest.js @@ -0,0 +1,31 @@ +"use strict"; + +const expect = require("chai").expect; +const stub = require("sinon").stub; +const Auth = require("../../../client/js/auth"); +const localStorage = require("../../../client/js/localStorage"); +const location = require("../../../client/js/location"); + +describe("Auth", function() { + describe(".signout", function() { + beforeEach(function() { + stub(localStorage, "clear"); + stub(location, "reload"); + }); + + afterEach(function() { + localStorage.clear.restore(); + location.reload.restore(); + }); + + it("should empty the local storage", function() { + Auth.signout(); + expect(localStorage.clear.calledOnce).to.be.true; + }); + + it("should reload the page", function() { + Auth.signout(); + expect(location.reload.calledOnce).to.be.true; + }); + }); +}); diff --git a/yarn.lock b/yarn.lock index 26f8891c..116c66e5 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6,6 +6,12 @@ version "1.0.4" resolved "https://registry.yarnpkg.com/@fortawesome/fontawesome-free-webfonts/-/fontawesome-free-webfonts-1.0.4.tgz#bac5d89755bf3bc2d2b4deee47d92febf641bb1f" +"@sinonjs/formatio@^2.0.0": + version "2.0.0" + resolved "http://registry.npmjs.org/@sinonjs/formatio/-/formatio-2.0.0.tgz#84db7e9eb5531df18a8c5e0bfb6e449e55e654b2" + dependencies: + samsam "1.3.0" + abbrev@1: version "1.1.1" resolved "https://registry.yarnpkg.com/abbrev/-/abbrev-1.1.1.tgz#f8f2c887ad10bf67f634f005b6987fed3179aac8" @@ -1969,7 +1975,7 @@ detect-node@^2.0.3: version "2.0.3" resolved "https://registry.yarnpkg.com/detect-node/-/detect-node-2.0.3.tgz#a2033c09cc8e158d37748fbde7507832bd6ce127" -diff@3.5.0: +diff@3.5.0, diff@^3.1.0: version "3.5.0" resolved "https://registry.yarnpkg.com/diff/-/diff-3.5.0.tgz#800c0dd1e0a8bfbc95835c202ad220fe317e5a12" @@ -3658,6 +3664,10 @@ is-wsl@^1.1.0: version "1.1.0" resolved "https://registry.yarnpkg.com/is-wsl/-/is-wsl-1.1.0.tgz#1f16e4aa22b04d1336b66188a66af3c600c3a66d" +isarray@0.0.1: + version "0.0.1" + resolved "https://registry.yarnpkg.com/isarray/-/isarray-0.0.1.tgz#8a18acfca9a8f4177e09abfc6038939b05d1eedf" + isarray@1.0.0, isarray@^1.0.0, isarray@~1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/isarray/-/isarray-1.0.0.tgz#bb935d48582cba168c06834957a54a3e07124f11" @@ -3839,6 +3849,10 @@ jsprim@^1.2.2: json-schema "0.2.3" verror "1.10.0" +just-extend@^1.1.27: + version "1.1.27" + resolved "https://registry.yarnpkg.com/just-extend/-/just-extend-1.1.27.tgz#ec6e79410ff914e472652abfa0e603c03d60e905" + jwa@^1.1.4: version "1.1.5" resolved "https://registry.yarnpkg.com/jwa/-/jwa-1.1.5.tgz#a0552ce0220742cd52e153774a32905c30e756e5" @@ -3994,6 +4008,10 @@ lodash.foreach@^4.3.0: version "4.5.0" resolved "https://registry.yarnpkg.com/lodash.foreach/-/lodash.foreach-4.5.0.tgz#1a6a35eace401280c7f06dddec35165ab27e3e53" +lodash.get@^4.4.2: + version "4.4.2" + resolved "https://registry.yarnpkg.com/lodash.get/-/lodash.get-4.4.2.tgz#2d177f652fa31e939b4438d5341499dfa3825e99" + lodash.map@^4.4.0: version "4.6.0" resolved "https://registry.yarnpkg.com/lodash.map/-/lodash.map-4.6.0.tgz#771ec7839e3473d9c4cde28b19394c3562f4f6d3" @@ -4040,6 +4058,10 @@ loglevel@^1.4.1: version "1.6.1" resolved "https://registry.yarnpkg.com/loglevel/-/loglevel-1.6.1.tgz#e0fc95133b6ef276cdc8887cdaf24aa6f156f8fa" +lolex@^2.2.0, lolex@^2.3.2: + version "2.3.2" + resolved "https://registry.yarnpkg.com/lolex/-/lolex-2.3.2.tgz#85f9450425103bf9e7a60668ea25dc43274ca807" + longest-streak@^2.0.1: version "2.0.2" resolved "https://registry.yarnpkg.com/longest-streak/-/longest-streak-2.0.2.tgz#2421b6ba939a443bb9ffebf596585a50b4c38e2e" @@ -4486,6 +4508,16 @@ next-tick@1: version "1.0.0" resolved "https://registry.yarnpkg.com/next-tick/-/next-tick-1.0.0.tgz#ca86d1fe8828169b0120208e3dc8424b9db8342c" +nise@^1.2.0: + version "1.3.2" + resolved "https://registry.yarnpkg.com/nise/-/nise-1.3.2.tgz#fd6fd8dc040dfb3c0a45252feb6ff21832309b14" + dependencies: + "@sinonjs/formatio" "^2.0.0" + just-extend "^1.1.27" + lolex "^2.3.2" + path-to-regexp "^1.7.0" + text-encoding "^0.6.4" + node-fetch@2.0.0: version "2.0.0" resolved "https://registry.yarnpkg.com/node-fetch/-/node-fetch-2.0.0.tgz#982bba43ecd4f2922a29cc186a6bbb0bb73fcba6" @@ -4960,6 +4992,12 @@ path-to-regexp@0.1.7: version "0.1.7" resolved "https://registry.yarnpkg.com/path-to-regexp/-/path-to-regexp-0.1.7.tgz#df604178005f522f15eb4490e7247a1bfaa67f8c" +path-to-regexp@^1.7.0: + version "1.7.0" + resolved "https://registry.yarnpkg.com/path-to-regexp/-/path-to-regexp-1.7.0.tgz#59fde0f435badacba103a84e9d3bc64e96b9937d" + dependencies: + isarray "0.0.1" + path-type@^1.0.0: version "1.1.0" resolved "https://registry.yarnpkg.com/path-type/-/path-type-1.1.0.tgz#59c44f7ee491da704da415da5a4070ba4f8fe441" @@ -5953,6 +5991,10 @@ safe-regex@^1.1.0: dependencies: ret "~0.1.10" +samsam@1.3.0: + version "1.3.0" + resolved "https://registry.yarnpkg.com/samsam/-/samsam-1.3.0.tgz#8d1d9350e25622da30de3e44ba692b5221ab7c50" + sax@^1.2.4, sax@~1.2.1: version "1.2.4" resolved "https://registry.yarnpkg.com/sax/-/sax-1.2.4.tgz#2816234e2378bddc4e5354fab5caa895df7100d9" @@ -6094,6 +6136,18 @@ signal-exit@^3.0.0, signal-exit@^3.0.1, signal-exit@^3.0.2: version "3.0.2" resolved "https://registry.yarnpkg.com/signal-exit/-/signal-exit-3.0.2.tgz#b5fdc08f1287ea1178628e415e25132b73646c6d" +sinon@4.4.6: + version "4.4.6" + resolved "https://registry.yarnpkg.com/sinon/-/sinon-4.4.6.tgz#0b21ce56f1b11015749a82a3bbde2f46b78ec0e1" + dependencies: + "@sinonjs/formatio" "^2.0.0" + diff "^3.1.0" + lodash.get "^4.4.2" + lolex "^2.2.0" + nise "^1.2.0" + supports-color "^5.1.0" + type-detect "^4.0.5" + slash@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/slash/-/slash-1.0.0.tgz#c41f2f6c39fc16d1cd17ad4b5d896114ae470d55" @@ -6684,6 +6738,10 @@ test-exclude@^4.2.0: read-pkg-up "^1.0.1" require-main-filename "^1.0.1" +text-encoding@^0.6.4: + version "0.6.4" + resolved "https://registry.yarnpkg.com/text-encoding/-/text-encoding-0.6.4.tgz#e399a982257a276dae428bb92845cb71bdc26d19" + text-table@~0.2.0: version "0.2.0" resolved "https://registry.yarnpkg.com/text-table/-/text-table-0.2.0.tgz#7f5ee823ae805207c00af2df4a84ec3fcfa570b4" @@ -6837,7 +6895,7 @@ type-check@~0.3.2: dependencies: prelude-ls "~1.1.2" -type-detect@^4.0.0: +type-detect@^4.0.0, type-detect@^4.0.5: version "4.0.8" resolved "https://registry.yarnpkg.com/type-detect/-/type-detect-4.0.8.tgz#7646fb5f18871cfbb7749e69bd39a6388eb7450c" From c86ea9463de3b1f9e96ef9200baa43e80f041bd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Astori?= Date: Tue, 20 Mar 2018 01:54:04 -0400 Subject: [PATCH 2/2] Use Sinon to stub the logger instead of manual stubbing --- src/log.js | 47 ++++++++++++++---------------- test/plugins/packages/indexTest.js | 12 ++++---- test/src/command-line/utilsTest.js | 25 +++++----------- test/util.js | 4 +-- 4 files changed, 36 insertions(+), 52 deletions(-) diff --git a/src/log.js b/src/log.js index 17608572..b627c874 100644 --- a/src/log.js +++ b/src/log.js @@ -13,30 +13,27 @@ function timestamp() { return colors.dim(time); } -/* eslint-disable no-console */ -exports.error = function(...args) { - console.error(timestamp(), colors.red("[ERROR]"), ...args); -}; +module.exports = { + /* eslint-disable no-console */ + error(...args) { + console.error(timestamp(), colors.red("[ERROR]"), ...args); + }, + warn(...args) { + console.error(timestamp(), colors.yellow("[WARN]"), ...args); + }, + info(...args) { + console.log(timestamp(), colors.blue("[INFO]"), ...args); + }, + debug(...args) { + console.log(timestamp(), colors.green("[DEBUG]"), ...args); + }, + raw(...args) { + console.log(...args); + }, + /* eslint-enable no-console */ -exports.warn = function(...args) { - console.error(timestamp(), colors.yellow("[WARN]"), ...args); -}; - -exports.info = function(...args) { - console.log(timestamp(), colors.blue("[INFO]"), ...args); -}; - -exports.debug = function(...args) { - console.log(timestamp(), colors.green("[DEBUG]"), ...args); -}; - -exports.raw = function(...args) { - console.log(...args); -}; - -/* eslint-enable no-console */ - -exports.prompt = (options, callback) => { - options.prompt = [timestamp(), colors.cyan("[PROMPT]"), options.text].join(" "); - read(options, callback); + prompt(options, callback) { + options.prompt = [timestamp(), colors.cyan("[PROMPT]"), options.text].join(" "); + read(options, callback); + }, }; diff --git a/test/plugins/packages/indexTest.js b/test/plugins/packages/indexTest.js index 417d6fa6..0777534f 100644 --- a/test/plugins/packages/indexTest.js +++ b/test/plugins/packages/indexTest.js @@ -1,24 +1,21 @@ "use strict"; const expect = require("chai").expect; +const stub = require("sinon").stub; const TestUtil = require("../../util"); let packages; describe("packages", function() { - let originalLogInfo; - beforeEach(function() { - originalLogInfo = log.info; - - log.info = () => {}; + stub(log, "info"); delete require.cache[require.resolve("../../../src/plugins/packages")]; packages = require("../../../src/plugins/packages"); }); afterEach(function() { - log.info = originalLogInfo; + log.info.restore(); }); describe(".getStylesheets", function() { @@ -51,8 +48,9 @@ describe("packages", function() { describe(".loadPackages", function() { it("should display report about loading packages", function() { // Mock `log.info` to extract its effect into a string + log.info.restore(); let stdout = ""; - log.info = TestUtil.mockLogger((str) => stdout += str); + stub(log, "info").callsFake(TestUtil.sanitizeLog((str) => stdout += str)); packages.loadPackages(); diff --git a/test/src/command-line/utilsTest.js b/test/src/command-line/utilsTest.js index ff36856f..6e1ec680 100644 --- a/test/src/command-line/utilsTest.js +++ b/test/src/command-line/utilsTest.js @@ -1,25 +1,20 @@ "use strict"; const expect = require("chai").expect; +const stub = require("sinon").stub; const TestUtil = require("../../util"); const Utils = require("../../../src/command-line/utils"); describe("Utils", function() { describe(".extraHelp", function() { - let originalRaw; - - beforeEach(function() { - originalRaw = log.raw; - }); - afterEach(function() { - log.raw = originalRaw; + log.raw.restore(); }); it("should start and end with empty lines to display correctly with --help", function() { // Mock `log.raw` to extract its effect into an array const stdout = []; - log.raw = TestUtil.mockLogger((str) => stdout.push(str)); + stub(log, "raw").callsFake(TestUtil.sanitizeLog((str) => stdout.push(str))); Utils.extraHelp(); @@ -36,7 +31,7 @@ describe("Utils", function() { it("should contain information about THELOUNGE_HOME env var", function() { // Mock `log.raw` to extract its effect into a concatenated string let stdout = ""; - log.raw = TestUtil.mockLogger((str) => stdout += str); + stub(log, "raw").callsFake(TestUtil.sanitizeLog((str) => stdout += str)); Utils.extraHelp(); @@ -119,18 +114,12 @@ describe("Utils", function() { }); describe("when given the same key multiple times", function() { - let originalWarn; - - beforeEach(function() { - originalWarn = log.warn; - }); - afterEach(function() { - log.warn = originalWarn; + log.warn.restore(); }); it("should not override options", function() { - log.warn = () => {}; + stub(log, "warn"); expect(Utils.parseConfigOptions("foo=baz", {foo: "bar"})) .to.deep.equal({foo: "bar"}); @@ -138,7 +127,7 @@ describe("Utils", function() { it("should display a warning", function() { let warning = ""; - log.warn = TestUtil.mockLogger((str) => warning += str); + stub(log, "warn").callsFake(TestUtil.sanitizeLog((str) => warning += str)); Utils.parseConfigOptions("foo=bar", {foo: "baz"}); diff --git a/test/util.js b/test/util.js index bf9f3c25..1f433d4b 100644 --- a/test/util.js +++ b/test/util.js @@ -24,7 +24,7 @@ MockClient.prototype.createMessage = function(opts) { return message; }; -function mockLogger(callback) { +function sanitizeLog(callback) { return function(...args) { // Concats and removes ANSI colors. See https://stackoverflow.com/a/29497680 const stdout = args.join(" ").replace( @@ -51,5 +51,5 @@ module.exports = { createWebserver() { return express(); }, - mockLogger, + sanitizeLog, };