From 1bd22b769cb7fdea028f65e0233ed460f0853ed9 Mon Sep 17 00:00:00 2001 From: Austin Fahsl Date: Mon, 18 Jul 2022 16:59:04 -0400 Subject: [PATCH] feat: default verifyAccess to false --- .../__tests__/get-npm-username.test.js | 2 +- .../publish/__tests__/publish-command.test.js | 106 ++++++++++++++---- commands/publish/command.js | 9 +- commands/publish/index.js | 10 +- commands/publish/lib/get-npm-username.js | 2 +- 5 files changed, 100 insertions(+), 29 deletions(-) diff --git a/commands/publish/__tests__/get-npm-username.test.js b/commands/publish/__tests__/get-npm-username.test.js index 6a115d14144..8154f3f985a 100644 --- a/commands/publish/__tests__/get-npm-username.test.js +++ b/commands/publish/__tests__/get-npm-username.test.js @@ -80,7 +80,7 @@ describe("getNpmUsername", () => { const opts = { registry: "https://registry.npmjs.org/" }; await expect(getNpmUsername(opts)).rejects.toThrow( - "Access verification failed. If you are authenticating with a npm automation token, set `command.publish.verifyAccess=false` in your lerna.json to skip this verification, or use a different type of npm access token (https://docs.npmjs.com/creating-and-viewing-access-tokens)." + "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"); }); diff --git a/commands/publish/__tests__/publish-command.test.js b/commands/publish/__tests__/publish-command.test.js index ef6dd31641f..0fada621ae5 100644 --- a/commands/publish/__tests__/publish-command.test.js +++ b/commands/publish/__tests__/publish-command.test.js @@ -124,23 +124,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 @@ -228,12 +213,12 @@ Map { expect(getOneTimePassword).not.toHaveBeenCalled(); }); - it("prompts for OTP when option missing and account-level 2FA enabled", async () => { + it("prompts for OTP when option missing, account-level 2FA enabled, and verify access is true", async () => { const testDir = await initFixture("normal"); getTwoFactorAuthRequired.mockResolvedValueOnce(true); - await lernaPublish(testDir)(); + await lernaPublish(testDir)("--verify-access", true); expect(npmPublish).toHaveBeenCalledWith( expect.objectContaining({ name: "package-1" }), @@ -243,6 +228,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.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", () => { @@ -306,7 +307,74 @@ 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.registry).toMatchInlineSnapshot(` +Set { + "package-1", + "package-3", + "package-4", + "package-2", +} +`); + expect(npmPublish.registry).toMatchInlineSnapshot(` +Map { + "package-1" => "latest", + "package-3" => "latest", + "package-4" => "latest", + "package-2" => "latest", +} +`); + expect(npmPublish.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 } + ); + }); + }); + 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, since skipping access verification is now the default behavior." + ); + }); + it("skips package access verification", async () => { const cwd = await initFixture("normal"); diff --git a/commands/publish/command.js b/commands/publish/command.js index ec2d3e9b0d5..2d2273dadcb 100644 --- a/commands/publish/command.js +++ b/commands/publish/command.js @@ -102,19 +102,14 @@ exports.builder = (yargs) => { 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", }, - // y: { - // describe: "Skip all confirmation prompts.", - // alias: "yes", - // type: "boolean", - // }, }; composeVersionOptions(yargs); diff --git a/commands/publish/index.js b/commands/publish/index.js index 16d12a2c883..9b19f7f86ae 100644 --- a/commands/publish/index.js +++ b/commands/publish/index.js @@ -82,10 +82,11 @@ class PublishCommand extends Command { // 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() { @@ -94,6 +95,13 @@ class PublishCommand extends Command { } initialize() { + if (this.options.verifyAccess === false) { + this.logger.warn( + "verify-access", + "--verify-access=false and --no-verify-access are no longer needed, since skipping access verification is now the default behavior." + ); + } + if (this.options.skipNpm) { // TODO: remove in next major release this.logger.warn("deprecated", "Instead of --skip-npm, call `lerna version` directly"); diff --git a/commands/publish/lib/get-npm-username.js b/commands/publish/lib/get-npm-username.js index 32dedfe07b3..68e81a95f0a 100644 --- a/commands/publish/lib/get-npm-username.js +++ b/commands/publish/lib/get-npm-username.js @@ -57,7 +57,7 @@ function getNpmUsername(options) { if (err.code === "E403") { throw new ValidationError( "ENEEDAUTH", - "Access verification failed. If you are authenticating with a npm automation token, set `command.publish.verifyAccess=false` in your lerna.json to skip this verification, or use a different type of npm access token (https://docs.npmjs.com/creating-and-viewing-access-tokens)." + "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)." ); }