feat(publish): disable legacy verifyAccess behavior by default (#274)

* feat(publish): disable legacy `verifyAccess` behavior by default

* tests: add missing unit test for OTP cache

packages/cli/src/cli-commands/cli-publish-commands.ts

@@ -109,24 +109,19 @@ export default {
         type: 'boolean',
       },
       'no-verify-access': {
+        // proxy for --verify-access
         describe: 'Do not verify package read-write access for current npm user.',
         type: 'boolean',
       },
       'verify-access': {
-        // proxy for --no-verify-access
-        hidden: true,
+        describe: 'Verify package read-write access for current npm user.',
         type: 'boolean',
       },
       'workspace-strict-match': {
         describe:
           'Strict match transform version numbers to an exact range (like "1.2.3") rather than with a caret (like ^1.2.3) when using `workspace:*`.',
         type: 'boolean',
       },
-      // y: {
-      //   describe: 'Skip all confirmation prompts.',
-      //   alias: 'yes',
-      //   type: 'boolean',
-      // },
     };
 
     composeVersionOptions(yargs);

packages/core/src/models/interfaces.ts

@@ -158,7 +158,7 @@ export type NpaResolveResult = (
 /** Passed between concurrent executions */
 export interface OneTimePasswordCache {
   /* The one-time password, passed as an option or received via prompt */
-  otp?: string;
+  otp?: string | number;
 }
 
 export interface LernaConfig {

packages/publish/README.md

@@ -45,8 +45,6 @@ During all publish operations, appropriate [lifecycle scripts](#lifecycle-script
 
 Check out [Per-Package Configuration](#per-package-configuration) for more details about publishing scoped packages, custom registries, and custom dist-tags.
 
-> If you're using [npm automation access token](https://docs.npmjs.com/creating-and-viewing-access-tokens#creating-access-tokens) please remember to [disable lerna access verification feature](#--no-verify-access). Automation token doesn't grant permissions needed for the verification to be successful. [Click here to read more about this issue](https://github.com/lerna/lerna/issues/2788).
-
 ## Positionals
 
 ### semver `--bump from-git`
@@ -84,13 +82,13 @@ This is useful when a previous `lerna publish` failed to publish all packages to
     - [`--legacy-auth`](#--legacy-auth)
     - [`--no-git-reset`](#--no-git-reset)
     - [`--no-granular-pathspec`](#--no-granular-pathspec)
-    - [`--no-verify-access`](#--no-verify-access)
     - [`--otp`](#--otp)
     - [`--preid`](#--preid)
     - [`--pre-dist-tag <tag>`](#--pre-dist-tag-tag)
     - [`--registry <url>`](#--registry-url)
     - [`--tag-version-prefix`](#--tag-version-prefix)
     - [`--temp-tag`](#--temp-tag)
+    - [`--verify-access`](#--verify-access)
     - [`--yes`](#--yes)
   - [`workspace:` protocol](#workspace-protocol)
     - [`--workspace-strict-match (default)`](#with---workspace-strict-match-default)
@@ -235,16 +233,6 @@ This option makes the most sense configured in `lerna.json`, as you really don't
 
 The root-level configuration is intentional, as this also covers the [identically-named option in `lerna version`](https://github.com/lerna/lerna/tree/main/commands/version#--no-granular-pathspec).
 
-### `--no-verify-access`
-
-By default, `lerna` will verify the logged-in npm user's access to the packages about to be published. Passing this flag will disable that check.
-
-If you are using a third-party registry that does not support `npm access ls-packages`, you will need to pass this flag (or set `command.publish.verifyAccess` to `false` in `lerna.json`).
-
-> Please use with caution
-
-> For the time being, use this flag/option always when you're handling NPM authorization with the use of [automation access token](https://docs.npmjs.com/creating-and-viewing-access-tokens#creating-access-tokens). [Click here to read more about this issue](https://github.com/lerna/lerna/issues/2788).
-
 ### `--otp`
 
 When publishing packages that require two-factor authentication, you can specify a [one-time password](https://docs.npmjs.com/about-two-factor-authentication) using `--otp`:
@@ -321,6 +309,17 @@ new version(s) to the dist-tag configured by [`--dist-tag`](#--dist-tag-tag) (de
 This is not generally necessary, as lerna will publish packages in topological
 order (all dependencies before dependents) by default.
 
+### `--verify-access`
+
+Historically, `lerna` attempted to fast-fail on authorization/authentication issues by performing some preemptive npm API requests using the given token. These days, however, there are multiple types of tokens that npm supports and they have varying levels of access rights, so there is no one-size fits all solution for this preemptive check and it is more appropriate to allow requests to npm to simply fail with appropriate errors for the given token. For this reason, the legacy `--verify-access` behavior is disabled by default and will likely be removed in a future major version.
+
+For now, though, if you pass this flag you can opt into the legacy behavior and `lerna` will preemptively perform this verification before it attempts to publish any packages.
+
+You should NOT use this option if:
+
+1.  You are using a third-party registry that does not support `npm access ls-packages`
+2.  You are using an authentication token without read access, such as a [npm automation access token](https://docs.npmjs.com/creating-and-viewing-access-tokens#creating-access-tokens)
+
 ### `--yes`
 
 ```sh
@@ -476,3 +475,13 @@ _you would rarely want to disable the strict match, in fact this option will be
   }
 }
 ```
+
+## Deprecated Options
+
+### `--no-verify-access`
+
+The legacy preemptive access verification is now off by default, so `--no-verify-access` is not needed. Requests will fail with appropriate errors when not authorized correctly. To opt-in to the legacy access verification, use [`--verify-access`](#--verify-access).
+
+### `--skip-npm`
+
+Call [`lerna version`](https://github.com/lerna/lerna/tree/main/commands/version#readme) directly, instead.

packages/publish/src/__tests__/get-npm-username.spec.ts

@@ -67,6 +67,23 @@ describe('getNpmUsername', () => {
     expect(console.error).toHaveBeenCalledWith('third-party whoami fail');
   });
 
+  test('logs failure message when npm returns forbidden response', async () => {
+    fetch.json.mockImplementationOnce(() => {
+      const err = new Error('npm profile fail due to insufficient permissions') as Error & { code: string };
+
+      err.code = 'E403';
+
+      return Promise.reject(err);
+    });
+
+    const opts = { registry: 'https://registry.npmjs.org/' } as FetchConfig;
+
+    await expect(getNpmUsername(opts)).rejects.toThrow(
+      'Access verification failed. Ensure that your npm access token has both read and write access, or remove the verifyAccess option to skip this verification. Note that npm automation tokens do NOT have read access (https://docs.npmjs.com/creating-and-viewing-access-tokens).'
+    );
+    expect(console.error).toHaveBeenCalledWith('npm profile fail due to insufficient permissions');
+  });
+
   test('allows third-party registries to fail with a stern warning', async () => {
     (fetch.json as any).mockImplementationOnce(() => {
       const err = new Error('many third-party registries do not support npm whoami') as Error & { code: string };

packages/publish/src/__tests__/publish-command.spec.ts

@@ -42,6 +42,7 @@ jest.mock('../lib/npm-dist-tag', () => jest.requireActual('../lib/__mocks__/npm-
 jest.mock('../lib/pack-directory', () => jest.requireActual('../lib/__mocks__/pack-directory'));
 jest.mock('../lib/git-checkout');
 
+import { npmConf } from '@lerna-lite/core';
 import fs from 'fs-extra';
 import path from 'path';
 
@@ -60,7 +61,7 @@ import yargParser from 'yargs-parser';
 
 // mocked or stubbed modules
 import { npmPublish } from '../lib/npm-publish';
-import { promptConfirmation } from '@lerna-lite/core';
+import { promptConfirmation, PublishCommandOption } from '@lerna-lite/core';
 import { getOneTimePassword, collectUpdates } from '@lerna-lite/core';
 import { packDirectory } from '../lib/pack-directory';
 import { getNpmUsername } from '../lib/get-npm-username';
@@ -79,7 +80,7 @@ const createArgv = (cwd, ...args) => {
   argv['$0'] = cwd;
   argv['loglevel'] = 'silent';
   argv.composed = 'composed';
-  return argv;
+  return argv as unknown as PublishCommandOption;
 };
 
 (gitCheckout as any).mockImplementation(() => Promise.resolve());
@@ -171,23 +172,8 @@ Map {
       expect(npmDistTag.remove).not.toHaveBeenCalled();
       expect(npmDistTag.add).not.toHaveBeenCalled();
 
-      expect(getNpmUsername).toHaveBeenCalled();
-      expect(getNpmUsername).toHaveBeenLastCalledWith(
-        expect.objectContaining({ registry: 'https://registry.npmjs.org/' })
-      );
-
-      expect(verifyNpmPackageAccess).toHaveBeenCalled();
-      expect(verifyNpmPackageAccess).toHaveBeenLastCalledWith(
-        expect.any(Array),
-        'lerna-test',
-        expect.objectContaining({ registry: 'https://registry.npmjs.org/' })
-      );
-
-      expect(getTwoFactorAuthRequired).toHaveBeenCalled();
-      expect(getTwoFactorAuthRequired).toHaveBeenLastCalledWith(
-        // extra insurance that @lerna/npm-conf is defaulting things correctly
-        expect.objectContaining({ otp: undefined })
-      );
+      expect(getNpmUsername).not.toHaveBeenCalled();
+      expect(verifyNpmPackageAccess).not.toHaveBeenCalled();
 
       expect(gitCheckout).toHaveBeenCalledWith(
         // the list of changed files has been asserted many times already
@@ -257,7 +243,6 @@ Map {
       // cli option skips prompt
       (getTwoFactorAuthRequired as any).mockResolvedValueOnce(true);
 
-      // await lernaPublish(testDir)("--otp", otp);
       await new PublishCommand(createArgv(testDir, '--otp', otp));
 
       expect(npmPublish).toHaveBeenCalledWith(
@@ -269,13 +254,32 @@ Map {
       expect(getOneTimePassword).not.toHaveBeenCalled();
     });
 
-    it('prompts for OTP when option missing and account-level 2FA enabled', async () => {
+    it('skips one-time password prompt when already found in cache', async () => {
       const testDir = await initFixture('normal');
+      const otp = '654321';
 
       (getTwoFactorAuthRequired as any).mockResolvedValueOnce(true);
 
-      // await lernaPublish(testDir)();
-      await new PublishCommand(createArgv(testDir));
+      const command = new PublishCommand(createArgv(testDir, '--verify-access', true));
+      await command;
+      command.conf.set('otp', otp);
+      await command.publishPacked();
+
+      expect(npmPublish).toHaveBeenCalledWith(
+        expect.objectContaining({ name: 'package-1' }),
+        '/TEMP_DIR/package-1-MOCKED.tgz',
+        expect.objectContaining({ otp: undefined }),
+        expect.objectContaining({ otp: '654321' })
+      );
+      expect(getOneTimePassword).toHaveBeenCalledTimes(1);
+    });
+
+    it('prompts for OTP when option missing, account-level 2FA enabled, and verify access is true', async () => {
+      const testDir = await initFixture('normal');
+
+      (getTwoFactorAuthRequired as any).mockResolvedValueOnce(true);
+
+      await new PublishCommand(createArgv(testDir, '--verify-access', true));
 
       expect(npmPublish).toHaveBeenCalledWith(
         expect.objectContaining({ name: 'package-1' }),
@@ -285,6 +289,22 @@ Map {
       );
       expect(getOneTimePassword).toHaveBeenLastCalledWith('Enter OTP:');
     });
+
+    it('defers OTP prompt when option missing, account-level 2FA enabled, and verify access is not true', async () => {
+      const testDir = await initFixture('normal');
+
+      (getTwoFactorAuthRequired as any).mockResolvedValueOnce(true);
+
+      await lernaPublish(testDir)();
+
+      expect(npmPublish).toHaveBeenCalledWith(
+        expect.objectContaining({ name: 'package-1' }),
+        '/TEMP_DIR/package-1-MOCKED.tgz',
+        expect.objectContaining({ otp: undefined }),
+        expect.objectContaining({ otp: undefined })
+      );
+      expect(getOneTimePassword).not.toHaveBeenCalled();
+    });
   });
 
   describe('--legacy-auth', () => {
@@ -352,7 +372,75 @@ Map {
     });
   });
 
+  describe('--verify-access', () => {
+    it("publishes packages after verifying the user's access to each package", async () => {
+      const testDir = await initFixture('normal');
+
+      await lernaPublish(testDir)('--verify-access');
+
+      expect(promptConfirmation).toHaveBeenLastCalledWith('Are you sure you want to publish these packages?');
+      expect((packDirectory as any).registry).toMatchInlineSnapshot(`
+Set {
+  "package-1",
+  "package-3",
+  "package-4",
+  "package-2",
+}
+`);
+      expect((npmPublish as any).registry).toMatchInlineSnapshot(`
+Map {
+  "package-1" => "latest",
+  "package-3" => "latest",
+  "package-4" => "latest",
+  "package-2" => "latest",
+}
+`);
+      expect((npmPublish as any).order()).toEqual([
+        'package-1',
+        'package-3',
+        'package-4',
+        'package-2',
+        // package-5 is private
+      ]);
+      expect(npmDistTag.remove).not.toHaveBeenCalled();
+      expect(npmDistTag.add).not.toHaveBeenCalled();
+
+      expect(getNpmUsername).toHaveBeenCalled();
+      expect(getNpmUsername).toHaveBeenLastCalledWith(
+        expect.objectContaining({ registry: 'https://registry.npmjs.org/' })
+      );
+
+      expect(verifyNpmPackageAccess).toHaveBeenCalled();
+      expect(verifyNpmPackageAccess).toHaveBeenLastCalledWith(
+        expect.any(Array),
+        'lerna-test',
+        expect.objectContaining({ registry: 'https://registry.npmjs.org/' })
+      );
+
+      expect(getTwoFactorAuthRequired).toHaveBeenCalled();
+      expect(getTwoFactorAuthRequired).toHaveBeenLastCalledWith(expect.objectContaining({ otp: undefined }));
+
+      expect(gitCheckout).toHaveBeenCalledWith(
+        expect.any(Array),
+        { granularPathspec: true },
+        { cwd: testDir },
+        undefined
+      );
+    });
+  });
+
   describe('--no-verify-access', () => {
+    it('shows warning that this is the default behavior and that this option is no longer needed', async () => {
+      const cwd = await initFixture('normal');
+
+      await lernaPublish(cwd)('--no-verify-access');
+
+      const logMessages = loggingOutput('warn');
+      expect(logMessages).toContain(
+        '--verify-access=false and --no-verify-access are no longer needed, because the legacy preemptive access verification is now disabled by default. Requests will fail with appropriate errors when not authorized correctly.'
+      );
+    });
+
     it('skips package access verification', async () => {
       const cwd = await initFixture('normal');
 

packages/publish/src/lib/get-npm-username.ts

@@ -50,6 +50,13 @@ export function getNpmUsername(options: FetchConfig): Promise<string | undefined
     opts.log.resume();
 
     if (opts.registry === 'https://registry.npmjs.org/') {
+      if (err.code === 'E403') {
+        throw new ValidationError(
+          'ENEEDAUTH',
+          'Access verification failed. Ensure that your npm access token has both read and write access, or remove the verifyAccess option to skip this verification. Note that npm automation tokens do NOT have read access (https://docs.npmjs.com/creating-and-viewing-access-tokens).'
+        );
+      }
+
       throw new ValidationError('EWHOAMI', 'Authentication error. Use `npm whoami` to troubleshoot.');
     }
 

packages/publish/src/publish-command.ts

@@ -64,7 +64,7 @@ export class PublishCommand extends Command<PublishCommandOption> {
   packagesToBeLicensed?: Package[] = [];
   runPackageLifecycle!: (pkg: Package, stage: string) => Promise<void>;
   runRootLifecycle!: (stage: string) => Promise<void> | void;
-  verifyAccess = false;
+  verifyAccess?: boolean = false;
   twoFactorAuthRequired = false;
   updates: PackageGraphNode[] = [];
   updatesVersions?: Map<string, any>;
@@ -110,10 +110,11 @@ export class PublishCommand extends Command<PublishCommandOption> {
 
     // inverted boolean options are only respected if prefixed with `--no-`, e.g. `--no-verify-access`
     this.gitReset = gitReset !== false;
-    this.verifyAccess = verifyAccess !== false;
 
     // consumed by npm-registry-fetch (via libnpmpublish)
     this.npmSession = crypto.randomBytes(8).toString('hex');
+
+    this.verifyAccess = verifyAccess;
   }
 
   get userAgent() {
@@ -122,6 +123,13 @@ export class PublishCommand extends Command<PublishCommandOption> {
   }
 
   initialize() {
+    if (this.options.verifyAccess === false) {
+      this.logger.warn(
+        'verify-access',
+        '--verify-access=false and --no-verify-access are no longer needed, because the legacy preemptive access verification is now disabled by default. Requests will fail with appropriate errors when not authorized correctly.'
+      );
+    }
+
     if (this.options.canary) {
       this.logger.info('canary', 'enabled');
     }