From b90c224a99d3246df1d650a08ae194786966e4ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Astori?= Date: Sat, 9 Dec 2017 02:11:05 -0500 Subject: [PATCH 1/5] Define a raw logger to avoid using `console.log`, use it in extra help for environment variables, and add a test for this This has multiple benefits: - Respects the "Do not mock what you do not own" principle, instead we mock `log.raw` when necessary - Lets us not re-assign `console.log`, which breaks as Mocha uses `console.log` as well - Save and restore initial `log.raw` in test hooks (before/after), otherwise this would break Mocha/Chai --- src/command-line/utils.js | 2 +- src/log.js | 5 ++++ test/src/command-line/utilsTest.js | 46 ++++++++++++++++++++++++++++++ test/util.js | 15 ++++++++++ 4 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 test/src/command-line/utilsTest.js diff --git a/src/command-line/utils.js b/src/command-line/utils.js index f9dd3ce8..0a54cad7 100644 --- a/src/command-line/utils.js +++ b/src/command-line/utils.js @@ -16,7 +16,7 @@ class Utils { "", ` THELOUNGE_HOME Path for all configuration files and folders. Defaults to ${colors.green(Helper.expandHome(Utils.defaultHome()))}.`, "", - ].forEach((e) => console.log(e)); // eslint-disable-line no-console + ].forEach((e) => log.raw(e)); } static defaultHome() { diff --git a/src/log.js b/src/log.js index 584d4e4f..1efbdfe5 100644 --- a/src/log.js +++ b/src/log.js @@ -32,6 +32,11 @@ exports.info = function() { exports.debug = function() { console.log.apply(console, timestamp(colors.green("[DEBUG]"), arguments)); }; + +exports.raw = function() { + console.log.apply(console, arguments); +}; + /* eslint-enable no-console */ exports.prompt = (options, callback) => { diff --git a/test/src/command-line/utilsTest.js b/test/src/command-line/utilsTest.js new file mode 100644 index 00000000..dad5c9a8 --- /dev/null +++ b/test/src/command-line/utilsTest.js @@ -0,0 +1,46 @@ +"use strict"; + +const expect = require("chai").expect; +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; + }); + + 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)); + + Utils.extraHelp(); + + // Starts with 2 empty lines + expect(stdout[0]).to.equal("\n"); + expect(stdout[1]).to.equal("\n"); + expect(stdout[2]).to.not.equal("\n"); + + // Ends with 1 empty line + expect(stdout[stdout.length - 2]).to.not.equal("\n"); + expect(stdout[stdout.length - 1]).to.equal("\n"); + }); + + 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); + + Utils.extraHelp(); + + expect(stdout).to.include("THELOUNGE_HOME"); + }); + }); +}); diff --git a/test/util.js b/test/util.js index 6c13d06e..cf519f73 100644 --- a/test/util.js +++ b/test/util.js @@ -23,6 +23,20 @@ MockClient.prototype.createMessage = function(opts) { return message; }; +function mockLogger(callback) { + return function() { + // TODO: Use ...args with The Lounge v3: add `...args` as function argument + // and replaced the next line with `args.join(", ")` + const stdout = Array.prototype.slice.call(arguments).join(", ") + .replace( // Removes ANSI colors. See https://stackoverflow.com/a/29497680 + /[\u001b\u009b][[()#;?]*(?:[0-9]{1,4}(?:;[0-9]{0,4})*)?[0-9A-ORZcf-nqry=><]/g, + "" + ); + + callback(stdout + "\n"); + }; +} + module.exports = { createClient: function() { return new MockClient(); @@ -38,4 +52,5 @@ module.exports = { createWebserver: function() { return express(); }, + mockLogger, }; From df2787d3e9aac3e3921524f818ca4a9ba9c8f983 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Astori?= Date: Sat, 9 Dec 2017 15:06:41 -0500 Subject: [PATCH 2/5] Add a `--config` / `-c` option to the `start` CLI command to arbitrarily override any configuration key The biggest caveat is that JS code (such as functions) will not be interpreted as such, on purpose, for security precautions. If such thing is needed, then a configuration file must be used. --- src/command-line/index.js | 9 +++ src/command-line/utils.js | 50 ++++++++++++++ test/src/command-line/utilsTest.js | 104 +++++++++++++++++++++++++++++ 3 files changed, 163 insertions(+) diff --git a/src/command-line/index.js b/src/command-line/index.js index 2fe21d1a..0ac9e2eb 100644 --- a/src/command-line/index.js +++ b/src/command-line/index.js @@ -2,6 +2,7 @@ global.log = require("../log.js"); +const _ = require("lodash"); const fs = require("fs"); const path = require("path"); const program = require("commander"); @@ -16,6 +17,11 @@ if (require("semver").lt(process.version, "6.0.0")) { program.version(Helper.getVersion(), "-v, --version") .option("--home ", `${colors.bold("[DEPRECATED]")} Use the ${colors.green("THELOUNGE_HOME")} environment variable instead.`) + .option( + "-c, --config ", + "override entries of the configuration file, must be specified for each entry that needs to be overriden", + Utils.parseConfigOptions + ) .on("--help", Utils.extraHelp) .parseOptions(process.argv); @@ -49,6 +55,9 @@ if (!home) { Helper.setHome(home); +// Merge config key-values passed as CLI options into the main config +_.merge(Helper.config, program.config); + require("./start"); require("./config"); if (!Helper.config.public && !Helper.config.ldap.enable) { diff --git a/src/command-line/utils.js b/src/command-line/utils.js index 0a54cad7..0e5e29ac 100644 --- a/src/command-line/utils.js +++ b/src/command-line/utils.js @@ -1,5 +1,6 @@ "use strict"; +const _ = require("lodash"); const colors = require("colors/safe"); const fs = require("fs"); const Helper = require("../helper"); @@ -51,6 +52,55 @@ class Utils { return home; } + + // Parses CLI options such as `-c public=true`, `-c debug.raw=true`, etc. + static parseConfigOptions(val, memo) { + // Invalid option that is not of format `key=value`, do nothing + if (!val.includes("=")) { + return memo; + } + + const parseValue = (value) => { + if (value === "true") { + return true; + } else if (value === "false") { + return false; + } else if (value === "undefined") { + return undefined; + } else if (value === "null") { + return null; + } else if (/^\[.*\]$/.test(value)) { // Arrays + // Supporting arrays `[a,b]` and `[a, b]` + const array = value.slice(1, -1).split(/,\s*/); + // If [] is given, it will be parsed as `[ "" ]`, so treat this as empty + if (array.length === 1 && array[0] === "") { + return []; + } + return array.map(parseValue); // Re-parses all values of the array + } + return value; + }; + + // First time the option is parsed, memo is not set + if (memo === undefined) { + memo = {}; + } + + // Note: If passed `-c foo="bar=42"` (with single or double quotes), `val` + // will always be passed as `foo=bar=42`, never with quotes. + const position = val.indexOf("="); // Only split on the first = found + const key = val.slice(0, position); + const value = val.slice(position + 1); + const parsedValue = parseValue(value); + + if (_.has(memo, key)) { + log.warn(`Configuration key ${colors.bold(key)} was already specified, ignoring...`); + } else { + memo = _.set(memo, key, parsedValue); + } + + return memo; + } } module.exports = Utils; diff --git a/test/src/command-line/utilsTest.js b/test/src/command-line/utilsTest.js index dad5c9a8..ff36856f 100644 --- a/test/src/command-line/utilsTest.js +++ b/test/src/command-line/utilsTest.js @@ -43,4 +43,108 @@ describe("Utils", function() { expect(stdout).to.include("THELOUNGE_HOME"); }); }); + + describe(".parseConfigOptions", function() { + describe("when it's the first option given", function() { + it("should return nothing when passed an invalid config", function() { + expect(Utils.parseConfigOptions("foo")).to.be.undefined; + }); + + it("should correctly parse boolean values", function() { + expect(Utils.parseConfigOptions("foo=true")).to.deep.equal({foo: true}); + expect(Utils.parseConfigOptions("foo=false")).to.deep.equal({foo: false}); + }); + + it("should correctly parse empty strings", function() { + expect(Utils.parseConfigOptions("foo=")).to.deep.equal({foo: ""}); + }); + + it("should correctly parse null values", function() { + expect(Utils.parseConfigOptions("foo=null")).to.deep.equal({foo: null}); + }); + + it("should correctly parse undefined values", function() { + expect(Utils.parseConfigOptions("foo=undefined")) + .to.deep.equal({foo: undefined}); + }); + + it("should correctly parse array values", function() { + expect(Utils.parseConfigOptions("foo=[bar,true]")) + .to.deep.equal({foo: ["bar", true]}); + + expect(Utils.parseConfigOptions("foo=[bar, true]")) + .to.deep.equal({foo: ["bar", true]}); + }); + + it("should correctly parse empty array values", function() { + expect(Utils.parseConfigOptions("foo=[]")) + .to.deep.equal({foo: []}); + }); + + it("should correctly parse values that contain `=` sign", function() { + expect(Utils.parseConfigOptions("foo=bar=42")) + .to.deep.equal({foo: "bar=42"}); + }); + + it("should correctly parse keys using dot-notation", function() { + expect(Utils.parseConfigOptions("foo.bar=value")) + .to.deep.equal({foo: {bar: "value"}}); + }); + + it("should correctly parse keys using array-notation", function() { + expect(Utils.parseConfigOptions("foo[0]=value")) + .to.deep.equal({foo: ["value"]}); + }); + }); + + describe("when some options have already been parsed", function() { + it("should not modify existing options when passed an invalid config", function() { + const memo = {foo: "bar"}; + expect(Utils.parseConfigOptions("foo", memo)).to.equal(memo); + }); + + it("should combine a new option with previously parsed ones", function() { + expect(Utils.parseConfigOptions("bar=false", {foo: true})) + .to.deep.equal({foo: true, bar: false}); + }); + + it("should maintain existing properties of a nested object", function() { + expect(Utils.parseConfigOptions("foo.bar=true", {foo: {baz: false}})) + .to.deep.equal({foo: {bar: true, baz: false}}); + }); + + it("should maintain existing entries of an array", function() { + expect(Utils.parseConfigOptions("foo[1]=baz", {foo: ["bar"]})) + .to.deep.equal({foo: ["bar", "baz"]}); + }); + + describe("when given the same key multiple times", function() { + let originalWarn; + + beforeEach(function() { + originalWarn = log.warn; + }); + + afterEach(function() { + log.warn = originalWarn; + }); + + it("should not override options", function() { + log.warn = () => {}; + + expect(Utils.parseConfigOptions("foo=baz", {foo: "bar"})) + .to.deep.equal({foo: "bar"}); + }); + + it("should display a warning", function() { + let warning = ""; + log.warn = TestUtil.mockLogger((str) => warning += str); + + Utils.parseConfigOptions("foo=bar", {foo: "baz"}); + + expect(warning).to.include("foo was already specified"); + }); + }); + }); + }); }); From 07a01b054790c2669a37968e93b503d4b2a8baa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Astori?= Date: Sun, 10 Dec 2017 16:04:13 -0500 Subject: [PATCH 3/5] Deprecate existing options of `thelounge start` in favor or `-c, --config` --- src/command-line/start.js | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/src/command-line/start.js b/src/command-line/start.js index 2cbaeb00..aa85a3e3 100644 --- a/src/command-line/start.js +++ b/src/command-line/start.js @@ -10,11 +10,11 @@ const Utils = require("./utils"); program .command("start") - .option("-H, --host ", "set the IP address or hostname for the web server to listen on") - .option("-P, --port ", "set the port to listen on") - .option("-B, --bind ", "set the local IP to bind to for outgoing connections") - .option(" --public", "start in public mode") - .option(" --private", "start in private mode") + .option("-H, --host ", `${colors.bold.red("[DEPRECATED]")} to set the IP address or hostname for the web server to listen on, use ${colors.bold("-c host=")} instead`) + .option("-P, --port ", `${colors.bold.red("[DEPRECATED]")} to set the port to listen on, use ${colors.bold("-c port=")} instead`) + .option("-B, --bind ", `${colors.bold.red("[DEPRECATED]")} to set the local IP to bind to for outgoing connections, use ${colors.bold("-c bind=")} instead`) + .option(" --public", `${colors.bold.red("[DEPRECATED]")} to start in public mode, use ${colors.bold("-c public=true")} instead`) + .option(" --private", `${colors.bold.red("[DEPRECATED]")} to start in private mode, use ${colors.bold("-c public=false")} instead`) .description("Start the server") .on("--help", Utils.extraHelp) .action(function(options) { @@ -22,6 +22,22 @@ program const server = require("../server"); + if (options.host) { + log.warn(`${colors.bold("-H, --host ")} is ${colors.bold.red("deprecated")} and will be removed in The Lounge v3. Use ${colors.bold("-c host=")} instead.`); + } + if (options.port) { + log.warn(`${colors.bold("-P, --port ")} is ${colors.bold.red("deprecated")} and will be removed in The Lounge v3. Use ${colors.bold("-c port=")} instead.`); + } + if (options.bind) { + log.warn(`${colors.bold("-B, --bind ")} is ${colors.bold.red("deprecated")} and will be removed in The Lounge v3. Use ${colors.bold("-c bind=")} instead.`); + } + if (options.public) { + log.warn(`${colors.bold("--public")} is ${colors.bold.red("deprecated")} and will be removed in The Lounge v3. Use ${colors.bold("-c public=true")} instead.`); + } + if (options.private) { + log.warn(`${colors.bold("--private")} is ${colors.bold.red("deprecated")} and will be removed in The Lounge v3. Use ${colors.bold("-c public=false")} instead.`); + } + var mode = Helper.config.public; if (options.public) { mode = true; From 6547d18e7fad901fdc7898184bf23ea5d7265c9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Astori?= Date: Sun, 10 Dec 2017 16:07:23 -0500 Subject: [PATCH 4/5] Color all references to deprecations in bold red --- src/command-line/index.js | 10 +++++----- src/command-line/utils.js | 2 +- src/helper.js | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/command-line/index.js b/src/command-line/index.js index 0ac9e2eb..b0e2c04b 100644 --- a/src/command-line/index.js +++ b/src/command-line/index.js @@ -11,12 +11,12 @@ const Helper = require("../helper"); const Utils = require("./utils"); if (require("semver").lt(process.version, "6.0.0")) { - log.warn(`Support of Node.js v4 is ${colors.bold("deprecated")} and will be removed in The Lounge v3.`); + log.warn(`Support of Node.js v4 is ${colors.bold.red("deprecated")} and will be removed in The Lounge v3.`); log.warn("Please upgrade to Node.js v6 or more recent."); } program.version(Helper.getVersion(), "-v, --version") - .option("--home ", `${colors.bold("[DEPRECATED]")} Use the ${colors.green("THELOUNGE_HOME")} environment variable instead.`) + .option("--home ", `${colors.bold.red("[DEPRECATED]")} Use the ${colors.green("THELOUNGE_HOME")} environment variable instead.`) .option( "-c, --config ", "override entries of the configuration file, must be specified for each entry that needs to be overriden", @@ -26,7 +26,7 @@ program.version(Helper.getVersion(), "-v, --version") .parseOptions(process.argv); if (program.home) { - log.warn(`${colors.green("--home")} is ${colors.bold("deprecated")} and will be removed in The Lounge v3.`); + log.warn(`${colors.green("--home")} is ${colors.bold.red("deprecated")} and will be removed in The Lounge v3.`); log.warn(`Use the ${colors.green("THELOUNGE_HOME")} environment variable instead.`); } @@ -43,7 +43,7 @@ if (!fs.existsSync(path.join( } if (process.env.LOUNGE_HOME) { - log.warn(`${colors.green("LOUNGE_HOME")} is ${colors.bold("deprecated")} and will be removed in The Lounge v3.`); + log.warn(`${colors.green("LOUNGE_HOME")} is ${colors.bold.red("deprecated")} and will be removed in The Lounge v3.`); log.warn(`Use ${colors.green("THELOUNGE_HOME")} instead.`); } @@ -67,7 +67,7 @@ require("./install"); // TODO: Remove this when releasing The Lounge v3 if (process.argv[1].endsWith(`${require("path").sep}lounge`)) { - log.warn(`The ${colors.red("lounge")} CLI is ${colors.bold("deprecated")} and will be removed in v3.`); + log.warn(`The ${colors.red("lounge")} CLI is ${colors.bold.red("deprecated")} and will be removed in v3.`); log.warn(`Use ${colors.green("thelounge")} instead.`); process.argv[1] = "thelounge"; } diff --git a/src/command-line/utils.js b/src/command-line/utils.js index 0e5e29ac..6f838bab 100644 --- a/src/command-line/utils.js +++ b/src/command-line/utils.js @@ -35,7 +35,7 @@ class Utils { ".lounge_home" )); if (fs.existsSync(deprecatedDistConfig)) { - log.warn(`${colors.green(".lounge_home")} is ${colors.bold("deprecated")} and will be ignored as of The Lounge v3.`); + log.warn(`${colors.green(".lounge_home")} is ${colors.bold.red("deprecated")} and will be ignored as of The Lounge v3.`); log.warn(`Use ${colors.green(".thelounge_home")} instead.`); distConfig = deprecatedDistConfig; diff --git a/src/helper.js b/src/helper.js index 20cdebe2..12683c4f 100644 --- a/src/helper.js +++ b/src/helper.js @@ -109,7 +109,7 @@ function setHome(newPath) { // TODO: Remove in future release // Backwards compatibility for old way of specifying themes in settings if (this.config.theme.includes(".css")) { - log.warn(`Referring to CSS files in the ${colors.green("theme")} setting of ${colors.green(configPath)} is ${colors.bold("deprecated")} and will be removed in a future version.`); + log.warn(`Referring to CSS files in the ${colors.green("theme")} setting of ${colors.green(configPath)} is ${colors.bold.red("deprecated")} and will be removed in a future version.`); } else { this.config.theme = `themes/${this.config.theme}.css`; } From d89112173df7de01de3db3334a0f096530025fce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Astori?= Date: Sun, 10 Dec 2017 16:57:26 -0500 Subject: [PATCH 5/5] Fix command line index parsing options (`--home` and `--config`) twice --- src/command-line/index.js | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/command-line/index.js b/src/command-line/index.js index b0e2c04b..beef8bef 100644 --- a/src/command-line/index.js +++ b/src/command-line/index.js @@ -22,12 +22,13 @@ program.version(Helper.getVersion(), "-v, --version") "override entries of the configuration file, must be specified for each entry that needs to be overriden", Utils.parseConfigOptions ) - .on("--help", Utils.extraHelp) - .parseOptions(process.argv); + .on("--help", Utils.extraHelp); + +// Parse options from `argv` returning `argv` void of these options. +const argvWithoutOptions = program.parseOptions(process.argv); if (program.home) { - log.warn(`${colors.green("--home")} is ${colors.bold.red("deprecated")} and will be removed in The Lounge v3.`); - log.warn(`Use the ${colors.green("THELOUNGE_HOME")} environment variable instead.`); + log.warn(`${colors.bold("--home")} is ${colors.bold.red("deprecated")} and will be removed in The Lounge v3. Use the ${colors.bold("THELOUNGE_HOME")} environment variable instead.`); } // Check if the app was built before calling setHome as it wants to load manifest.json from the public folder @@ -72,7 +73,13 @@ if (process.argv[1].endsWith(`${require("path").sep}lounge`)) { process.argv[1] = "thelounge"; } -program.parse(process.argv); +// `parse` expects to be passed `process.argv`, but we need to remove to give it +// a version of `argv` that does not contain options already parsed by +// `parseOptions` above. +// This is done by giving it the updated `argv` that `parseOptions` returned, +// except it returns an object with `args`/`unknown`, so we need to concat them. +// See https://github.com/tj/commander.js/blob/fefda77f463292/index.js#L686-L763 +program.parse(argvWithoutOptions.args.concat(argvWithoutOptions.unknown)); if (!program.args.length) { program.help();