fix(@angular/cli): handle NPM_CONFIG environment variables during ng …

…update and ng add Some organizations are moving away from storing tokens/secrets in an NPM config file in favor of environment variables that only exist for the span of a terminal session. This commit will make sure those variables are read even when there is no NPM config file present.
angular · Jul 21, 2021 · 6b00d12 · 6b00d12
1 parent 02cc9c0
commit 6b00d12
Show file tree

Hide file tree

Showing 6 changed files with 150 additions and 62 deletions.
diff --git a/packages/angular/cli/utilities/package-metadata.ts b/packages/angular/cli/utilities/package-metadata.ts
@@ -131,7 +131,7 @@ function readOptions(
     logger.info(`Locating potential ${baseFilename} files:`);
   }
 
-  const options: PackageManagerOptions = {};
+  let options: PackageManagerOptions = {};
   for (const location of [...defaultConfigLocations, ...projectConfigLocations]) {
     if (existsSync(location)) {
       if (showPotentials) {
@@ -142,58 +142,84 @@ function readOptions(
       // Normalize RC options that are needed by 'npm-registry-fetch'.
       // See: https://github.com/npm/npm-registry-fetch/blob/ebddbe78a5f67118c1f7af2e02c8a22bcaf9e850/index.js#L99-L126
       const rcConfig: PackageManagerOptions = yarn ? lockfile.parse(data) : ini.parse(data);
-      for (const [key, value] of Object.entries(rcConfig)) {
-        let substitutedValue = value;
 
-        // Substitute any environment variable references.
-        if (typeof value === 'string') {
-          substitutedValue = value.replace(/\$\{([^\}]+)\}/, (_, name) => process.env[name] || '');
-        }
+      options = normalizeOptions(rcConfig, location);
+    }
+  }
+
+  for (const [key, value] of Object.entries(process.env)) {
+    if (!value || !key.toLowerCase().startsWith('npm_config_')) {
+      continue;
+    }
+
+    const normalizedName = key
+      .substr(11)
+      .replace(/(?!^)_/g, '-') // don't replace _ at the start of the key
+      .toLowerCase();
+    options[normalizedName] = value;
+  }
 
-        switch (key) {
-          // Unless auth options are scope with the registry url it appears that npm-registry-fetch ignores them,
-          // even though they are documented.
-          // https://github.com/npm/npm-registry-fetch/blob/8954f61d8d703e5eb7f3d93c9b40488f8b1b62ac/README.md
-          // https://github.com/npm/npm-registry-fetch/blob/8954f61d8d703e5eb7f3d93c9b40488f8b1b62ac/auth.js#L45-L91
-          case '_authToken':
-          case 'token':
-          case 'username':
-          case 'password':
-          case '_auth':
-          case 'auth':
-            options['forceAuth'] ??= {};
-            options['forceAuth'][key] = substitutedValue;
-            break;
-          case 'noproxy':
-          case 'no-proxy':
-            options['noProxy'] = substitutedValue;
-            break;
-          case 'maxsockets':
-            options['maxSockets'] = substitutedValue;
-            break;
-          case 'https-proxy':
-          case 'proxy':
-            options['proxy'] = substitutedValue;
-            break;
-          case 'strict-ssl':
-            options['strictSSL'] = substitutedValue;
-            break;
-          case 'local-address':
-            options['localAddress'] = substitutedValue;
-            break;
-          case 'cafile':
-            if (typeof substitutedValue === 'string') {
-              const cafile = path.resolve(path.dirname(location), substitutedValue);
-              try {
-                options['ca'] = readFileSync(cafile, 'utf8').replace(/\r?\n/g, '\n');
-              } catch {}
-            }
-            break;
-          default:
-            options[key] = substitutedValue;
-            break;
+  options = normalizeOptions(options);
+
+  return options;
+}
+
+function normalizeOptions(
+  rawOptions: PackageManagerOptions,
+  location = process.cwd(),
+): PackageManagerOptions {
+  const options: PackageManagerOptions = {};
+
+  for (const [key, value] of Object.entries(rawOptions)) {
+    let substitutedValue = value;
+
+    // Substitute any environment variable references.
+    if (typeof value === 'string') {
+      substitutedValue = value.replace(/\$\{([^\}]+)\}/, (_, name) => process.env[name] || '');
+    }
+
+    switch (key) {
+      // Unless auth options are scope with the registry url it appears that npm-registry-fetch ignores them,
+      // even though they are documented.
+      // https://github.com/npm/npm-registry-fetch/blob/8954f61d8d703e5eb7f3d93c9b40488f8b1b62ac/README.md
+      // https://github.com/npm/npm-registry-fetch/blob/8954f61d8d703e5eb7f3d93c9b40488f8b1b62ac/auth.js#L45-L91
+      case '_authToken':
+      case 'token':
+      case 'username':
+      case 'password':
+      case '_auth':
+      case 'auth':
+        options['forceAuth'] ??= {};
+        options['forceAuth'][key] = substitutedValue;
+        break;
+      case 'noproxy':
+      case 'no-proxy':
+        options['noProxy'] = substitutedValue;
+        break;
+      case 'maxsockets':
+        options['maxSockets'] = substitutedValue;
+        break;
+      case 'https-proxy':
+      case 'proxy':
+        options['proxy'] = substitutedValue;
+        break;
+      case 'strict-ssl':
+        options['strictSSL'] = substitutedValue;
+        break;
+      case 'local-address':
+        options['localAddress'] = substitutedValue;
+        break;
+      case 'cafile':
+        if (typeof substitutedValue === 'string') {
+          const cafile = path.resolve(path.dirname(location), substitutedValue);
+          try {
+            options['ca'] = readFileSync(cafile, 'utf8').replace(/\r?\n/g, '\n');
+          } catch {}
         }
-      }
+        break;
+      default:
+        options[key] = substitutedValue;
+        break;
     }
   }
 

diff --git a/tests/legacy-cli/e2e/tests/commands/add/npm-env-vars.ts b/tests/legacy-cli/e2e/tests/commands/add/npm-env-vars.ts
@@ -0,0 +1,33 @@
+import { expectFileNotToExist, expectFileToExist } from '../../../utils/fs';
+import { getActivePackageManager } from '../../../utils/packages';
+import { git, ng } from '../../../utils/process';
+import {
+  createNpmConfigForAuthentication,
+  setNpmEnvVarsForAuthentication,
+} from '../../../utils/registry';
+
+export default async function () {
+  const packageManager = getActivePackageManager();
+
+  if (packageManager === 'npm') {
+    const originalEnvironment = { ...process.env };
+    try {
+      const command = ['add', '@angular/pwa', '--skip-confirmation'];
+
+      // Environment variables only
+      await expectFileNotToExist('src/manifest.webmanifest');
+      setNpmEnvVarsForAuthentication();
+      await ng(...command);
+      await expectFileToExist('src/manifest.webmanifest');
+      await git('clean', '-dxf');
+
+      // Mix of config file and env vars works
+      await expectFileNotToExist('src/manifest.webmanifest');
+      await createNpmConfigForAuthentication(false, true);
+      await ng(...command);
+      await expectFileToExist('src/manifest.webmanifest');
+    } finally {
+      process.env = originalEnvironment;
+    }
+  }
+}
diff --git a/tests/legacy-cli/e2e/tests/commands/add/registry-option.ts b/tests/legacy-cli/e2e/tests/commands/add/registry-option.ts
@@ -11,15 +11,20 @@ export default async function () {
     '.npmrc': 'registry=http://127.0.0.1:9999',
   });
   // The environment variable has priority over the .npmrc
-  const originalRegistryVariable = process.env['NPM_CONFIG_REGISTRY'];
-  delete process.env['NPM_CONFIG_REGISTRY'];
+  let originalRegistryVariable;
+  if (process.env['NPM_CONFIG_REGISTRY']) {
+    originalRegistryVariable = process.env['NPM_CONFIG_REGISTRY'];
+    delete process.env['NPM_CONFIG_REGISTRY'];
+  }
 
   try {
     await expectToFail(() => ng('add', '@angular/pwa', '--skip-confirmation'));
 
     await ng('add', `--registry=${testRegistry}`, '@angular/pwa', '--skip-confirmation');
     await expectFileToExist('src/manifest.webmanifest');
   } finally {
-    process.env['NPM_CONFIG_REGISTRY'] = originalRegistryVariable;
+    if (originalRegistryVariable) {
+      process.env['NPM_CONFIG_REGISTRY'] = originalRegistryVariable;
+    }
   }
 }
diff --git a/tests/legacy-cli/e2e/tests/commands/add/secure-registry.ts b/tests/legacy-cli/e2e/tests/commands/add/secure-registry.ts
@@ -5,8 +5,11 @@ import { expectToFail } from '../../../utils/utils';
 
 export default async function () {
   // The environment variable has priority over the .npmrc
-  const originalRegistryVariable = process.env['NPM_CONFIG_REGISTRY'];
-  delete process.env['NPM_CONFIG_REGISTRY'];
+  let originalRegistryVariable;
+  if (process.env['NPM_CONFIG_REGISTRY']) {
+    originalRegistryVariable = process.env['NPM_CONFIG_REGISTRY'];
+    delete process.env['NPM_CONFIG_REGISTRY'];
+  }
 
   try {
     const command = ['add', '@angular/pwa', '--skip-confirmation'];
@@ -32,6 +35,8 @@ export default async function () {
     await createNpmConfigForAuthentication(true, true);
     await expectToFail(() => ng(...command));
   } finally {
-    process.env['NPM_CONFIG_REGISTRY'] = originalRegistryVariable;
+    if (originalRegistryVariable) {
+      process.env['NPM_CONFIG_REGISTRY'] = originalRegistryVariable;
+    }
   }
 }
diff --git a/tests/legacy-cli/e2e/tests/update/update-secure-registry.ts b/tests/legacy-cli/e2e/tests/update/update-secure-registry.ts
@@ -4,8 +4,11 @@ import { expectToFail } from '../../utils/utils';
 
 export default async function () {
   // The environment variable has priority over the .npmrc
-  const originalRegistryVariable = process.env['NPM_CONFIG_REGISTRY'];
-  delete process.env['NPM_CONFIG_REGISTRY'];
+  let originalRegistryVariable;
+  if (process.env['NPM_CONFIG_REGISTRY']) {
+    originalRegistryVariable = process.env['NPM_CONFIG_REGISTRY'];
+    delete process.env['NPM_CONFIG_REGISTRY'];
+  }
 
   const worksMessage = 'We analyzed your package.json';
 
@@ -30,6 +33,8 @@ export default async function () {
     await createNpmConfigForAuthentication(true, true);
     await expectToFail(() => ng('update'));
   } finally {
-    process.env['NPM_CONFIG_REGISTRY'] = originalRegistryVariable;
+    if (originalRegistryVariable) {
+      process.env['NPM_CONFIG_REGISTRY'] = originalRegistryVariable;
+    }
   }
 }
diff --git a/tests/legacy-cli/e2e/utils/registry.ts b/tests/legacy-cli/e2e/utils/registry.ts
@@ -19,6 +19,10 @@ export function createNpmRegistry(withAuthentication = false): ChildProcess {
   });
 }
 
+// Token was generated using `echo -n 'testing:s3cret' | openssl base64`.
+const VALID_TOKEN = `dGVzdGluZzpzM2NyZXQ=`;
+const SECURE_REGISTRY = `//localhost:4876/`;
+
 export function createNpmConfigForAuthentication(
   /**
    * When true, the authentication token will be scoped to the registry URL.
@@ -37,9 +41,8 @@ export function createNpmConfigForAuthentication(
   /** When true, an incorrect token is used. Use this to validate authentication failures. */
   invalidToken = false,
 ): Promise<void> {
-  // Token was generated using `echo -n 'testing:s3cret' | openssl base64`.
-  const token = invalidToken ? `invalid=` : `dGVzdGluZzpzM2NyZXQ=`;
-  const registry = `//localhost:4876/`;
+  const token = invalidToken ? `invalid=` : VALID_TOKEN;
+  const registry = SECURE_REGISTRY;
 
   return writeFile(
     '.npmrc',
@@ -54,3 +57,14 @@ export function createNpmConfigForAuthentication(
       `,
   );
 }
+
+export function setNpmEnvVarsForAuthentication(
+  /** When true, an incorrect token is used. Use this to validate authentication failures. */
+  invalidToken = false,
+): void {
+  const token = invalidToken ? `invalid=` : VALID_TOKEN;
+  const registry = SECURE_REGISTRY;
+
+  process.env['NPM_CONFIG_REGISTRY'] = `http:${registry}`;
+  process.env['NPM_CONFIG__AUTH'] = token;
+}