fix: Validate required parameters after reading config files. (#1928)

src/config.js

@@ -113,6 +113,8 @@ export function applyConfigToArgv({
         `Calling coerce() on configured value for ${option}`);
       newArgv[option] = coerce(newArgv[option]);
     }
+
+    newArgv[decamelizedOptName] = newArgv[option];
   }
   return newArgv;
 }

src/program.js

@@ -153,7 +153,20 @@ export class Program {
   // Retrieve the yargs argv object and apply any further fix needed
   // on the output of the yargs options parsing.
   getArguments(): Object {
+    // To support looking up required parameters via config files, we need to
+    // temporarily disable the requiredArguments validation. Otherwise yargs
+    // would exit early. Validation is enforced by the checkRequiredArguments()
+    // method, after reading configuration files.
+    //
+    // This is an undocumented internal API of yargs! Unit tests to avoid
+    // regressions are located at: tests/functional/test.cli.sign.js
+    //
+    // Replace hack if possible:  https://github.com/mozilla/web-ext/issues/1930
+    const validationInstance = this.yargs.getValidationInstance();
+    const { requiredArguments } = validationInstance;
+    validationInstance.requiredArguments = () => {};
     const argv = this.yargs.argv;
+    validationInstance.requiredArguments = requiredArguments;
 
     // Yargs boolean options doesn't define the no* counterpart
     // with negate-boolean on Yargs 15. Define as expected by the
@@ -178,6 +191,15 @@ export class Program {
     return argv;
   }
 
+  // getArguments() disables validation of required parameters, to allow us to
+  // read parameters from config files first. Before the program continues, it
+  // must call checkRequiredArguments() to ensure that required parameters are
+  // defined (in the CLI or in a config file).
+  checkRequiredArguments(adjustedArgv: Object): void {
+    const validationInstance = this.yargs.getValidationInstance();
+    validationInstance.requiredArguments(adjustedArgv);
+  }
+
   // Remove WEB_EXT_* environment vars that are not a global cli options
   // or an option supported by the current command (See #793).
   cleanupProcessEnvConfigs(systemProcess: typeof process) {
@@ -287,6 +309,8 @@ export class Program {
         this.enableVerboseMode(logStream, version);
       }
 
+      this.checkRequiredArguments(adjustedArgv);
+
       await runCommand(adjustedArgv, {shouldExitProgram});
 
     } catch (error) {

tests/functional/test.cli.sign.js

@@ -1,13 +1,37 @@
 /* @flow */
 import {spawn} from 'child_process';
+import path from 'path';
 
+import {assert} from 'chai';
 import {describe, it, beforeEach, afterEach} from 'mocha';
+import {fs} from 'mz';
 
 import {
   minimalAddonPath, fakeServerPath,
   withTempAddonDir, execWebExt, reportCommandErrors,
 } from './common';
 
+// Put this as "web-ext-config.js" in the current directory, and replace
+// "FAKEAPIKEY" and "FAKEAPISECRET" with the actual values to enable
+// "web-ext sign" without passing those values via the CLI parameters.
+const GOOD_EXAMPLE_OF_WEB_EXT_CONFIG_JS = `
+module.exports = {
+  sign: {
+    apiKey: "FAKEAPIKEY",
+    apiSecret: "FAKEAPISECRET",
+  },
+};
+`;
+
+// Do NOT use this to specify the API key and secret. It won't work.
+const BAD_EXAMPLE_OF_WEB_EXT_CONFIG_JS = `
+module.exports = {
+  // Bad config: those should be under the "sign" key.
+  apiKey: "FAKEAPIKEY",
+  apiSecret: "FAKEAPISECRET",
+};
+`;
+
 describe('web-ext sign', () => {
   let fakeServerProcess;
 
@@ -48,4 +72,77 @@ describe('web-ext sign', () => {
        });
      })
   );
+
+  it('should use config file if required parameters are not in the arguments',
+     () => withTempAddonDir({addonPath: minimalAddonPath}, (srcDir, tmpDir) => {
+       fs.writeFileSync(
+         path.join(tmpDir, 'web-ext-config.js'),
+         GOOD_EXAMPLE_OF_WEB_EXT_CONFIG_JS,
+       );
+
+       fs.writeFileSync(
+         path.join(tmpDir, 'package.json'),
+         JSON.stringify({
+           webExt: {
+             sign: {
+               apiUrlPrefix: 'http://localhost:8989/fake/api/v3',
+             },
+             sourceDir: srcDir,
+           },
+         })
+       );
+
+       const argv = [
+         'sign', '--verbose',
+       ];
+       const cmd = execWebExt(argv, {cwd: tmpDir});
+
+       return cmd.waitForExit.then(({exitCode, stdout, stderr}) => {
+         if (exitCode !== 0) {
+           reportCommandErrors({
+             argv,
+             exitCode,
+             stdout,
+             stderr,
+           });
+         }
+       });
+     })
+  );
+
+  it('should show an error message if the api-key is not set in the config',
+     () => withTempAddonDir({addonPath: minimalAddonPath}, (srcDir, tmpDir) => {
+       const configFilePath = path.join(tmpDir, 'web-ext-config.js');
+       fs.writeFileSync(configFilePath, BAD_EXAMPLE_OF_WEB_EXT_CONFIG_JS);
+       const argv = [
+         'sign', '--verbose', '--no-config-discovery', '-c', configFilePath,
+       ];
+       const cmd = execWebExt(argv, {cwd: tmpDir});
+
+       return cmd.waitForExit.then(({exitCode, stdout}) => {
+         assert.notEqual(exitCode, 0);
+         assert.match(
+           stdout,
+           /web-ext-config.js specified an unknown option: "apiKey"/
+         );
+       });
+     })
+  );
+
+  it('should show an error message if the api-key cannot be found',
+     () => withTempAddonDir({addonPath: minimalAddonPath}, (srcDir, tmpDir) => {
+       const argv = [
+         'sign', '--verbose', '--no-config-discovery',
+       ];
+       const cmd = execWebExt(argv, {cwd: tmpDir});
+
+       return cmd.waitForExit.then(({exitCode, stderr}) => {
+         assert.notEqual(exitCode, 0);
+         assert.match(
+           stderr,
+           /Missing required arguments: api-key, api-secret/
+         );
+       });
+     })
+  );
 });