From 38cf29a0054544c575b6bce953f1d433dbb6a3b5 Mon Sep 17 00:00:00 2001 From: Gar Date: Mon, 9 May 2022 07:34:54 -0700 Subject: [PATCH] fix: cleanup star/unstar (#4868) It was querying whoami once for every package you starred/unstarred, and incorrectly trying to determine if you weren't logged in. In fact the function throws a descriptive message if you're not logged in already. The whoami check was also racing with the fetch of the packument for each package you were starring/unstarring meaning you could also get a random 401 for a private package instead of the 'you need to log in' message. unstar was setting an undocumented config item to get the shared code to unstar. The command already has a name attribute that tells us what action we are doing so we can just use that. Finally, the duplicated (and differing) params between the two commands were consolidated. --- docs/content/commands/npm-star.md | 14 ++ lib/commands/star.js | 27 ++- lib/commands/unstar.js | 10 -- .../test/lib/load-all-commands.js.test.cjs | 2 +- .../test/lib/utils/npm-usage.js.test.cjs | 2 +- test/fixtures/mock-registry.js | 14 +- test/lib/commands/deprecate.js | 6 +- test/lib/commands/owner.js | 44 ++--- test/lib/commands/star.js | 154 +++++------------- test/lib/commands/unstar.js | 73 ++++++--- 10 files changed, 159 insertions(+), 187 deletions(-) diff --git a/docs/content/commands/npm-star.md b/docs/content/commands/npm-star.md index bbec7ac5f9263..00ef17a816b4a 100644 --- a/docs/content/commands/npm-star.md +++ b/docs/content/commands/npm-star.md @@ -69,6 +69,20 @@ false, it uses ascii characters instead of unicode glyphs. +#### `otp` + +* Default: null +* Type: null or String + +This is a one-time password from a two-factor authenticator. It's needed +when publishing or changing package permissions with `npm access`. + +If not set, and a registry response fails with a challenge for a one-time +password, npm will prompt on the command line for one. + + + + ### See Also diff --git a/lib/commands/star.js b/lib/commands/star.js index fb3882bcd1c56..7b76be3c1632d 100644 --- a/lib/commands/star.js +++ b/lib/commands/star.js @@ -11,6 +11,7 @@ class Star extends BaseCommand { static params = [ 'registry', 'unicode', + 'otp', ] static ignoreImplicitWorkspace = false @@ -23,26 +24,20 @@ class Star extends BaseCommand { // if we're unstarring, then show an empty star image // otherwise, show the full star image const unicode = this.npm.config.get('unicode') - const unstar = this.npm.config.get('star.unstar') const full = unicode ? '\u2605 ' : '(*)' const empty = unicode ? '\u2606 ' : '( )' - const show = unstar ? empty : full + const show = this.name === 'star' ? full : empty const pkgs = args.map(npa) - for (const pkg of pkgs) { - const [username, fullData] = await Promise.all([ - getIdentity(this.npm, { ...this.npm.flatOptions }), - fetch.json(pkg.escapedName, { - ...this.npm.flatOptions, - spec: pkg, - query: { write: true }, - preferOnline: true, - }), - ]) + const username = await getIdentity(this.npm, this.npm.flatOptions) - if (!username) { - throw new Error('You need to be logged in!') - } + for (const pkg of pkgs) { + const fullData = await fetch.json(pkg.escapedName, { + ...this.npm.flatOptions, + spec: pkg, + query: { write: true }, + preferOnline: true, + }) const body = { _id: fullData._id, @@ -50,7 +45,7 @@ class Star extends BaseCommand { users: fullData.users || {}, } - if (!unstar) { + if (this.name === 'star') { log.info('star', 'starring', body._id) body.users[username] = true log.verbose('star', 'starring', body) diff --git a/lib/commands/unstar.js b/lib/commands/unstar.js index 9a64c843178f3..cbcb73636c638 100644 --- a/lib/commands/unstar.js +++ b/lib/commands/unstar.js @@ -3,15 +3,5 @@ const Star = require('./star.js') class Unstar extends Star { static description = 'Remove an item from your favorite packages' static name = 'unstar' - static params = [ - 'registry', - 'unicode', - 'otp', - ] - - async exec (args) { - this.npm.config.set('star.unstar', true) - return super.exec(args) - } } module.exports = Unstar diff --git a/tap-snapshots/test/lib/load-all-commands.js.test.cjs b/tap-snapshots/test/lib/load-all-commands.js.test.cjs index eccb06580d9f9..67606be07cf7d 100644 --- a/tap-snapshots/test/lib/load-all-commands.js.test.cjs +++ b/tap-snapshots/test/lib/load-all-commands.js.test.cjs @@ -828,7 +828,7 @@ Usage: npm star [...] Options: -[--registry ] [--unicode] +[--registry ] [--unicode] [--otp ] Run "npm help star" for more info ` diff --git a/tap-snapshots/test/lib/utils/npm-usage.js.test.cjs b/tap-snapshots/test/lib/utils/npm-usage.js.test.cjs index d7ec4953d6ab3..63799913419b3 100644 --- a/tap-snapshots/test/lib/utils/npm-usage.js.test.cjs +++ b/tap-snapshots/test/lib/utils/npm-usage.js.test.cjs @@ -862,7 +862,7 @@ All commands: npm star [...] Options: - [--registry ] [--unicode] + [--registry ] [--unicode] [--otp ] Run "npm help star" for more info diff --git a/test/fixtures/mock-registry.js b/test/fixtures/mock-registry.js index 01d43ba3d980b..a62890b72e2f6 100644 --- a/test/fixtures/mock-registry.js +++ b/test/fixtures/mock-registry.js @@ -183,6 +183,15 @@ class MockRegistry { } } + star (manifest, users) { + const spec = npa(manifest.name) + this.nock = this.nock.put(`/${spec.escapedName}`, { + _id: manifest._id, + _rev: manifest._rev, + users, + }).reply(200, { ...manifest, users }) + } + async package ({ manifest, times = 1, query, tarballs }) { let nock = this.nock const spec = npa(manifest.name) @@ -206,7 +215,7 @@ class MockRegistry { // either pass in packuments if you need to set specific attributes besides version, // or an array of versions // the last packument in the packuments or versions array will be tagged latest - manifest ({ name = 'test-package', packuments, versions } = {}) { + manifest ({ name = 'test-package', users, packuments, versions } = {}) { packuments = this.packuments(packuments, name) const latest = packuments.slice(-1)[0] const manifest = { @@ -220,6 +229,9 @@ class MockRegistry { 'dist-tags': { latest: latest.version }, ...latest, } + if (users) { + manifest.users = users + } if (versions) { packuments = versions.map(version => ({ version })) } diff --git a/test/lib/commands/deprecate.js b/test/lib/commands/deprecate.js index 03177cb7be0b9..8a925fc2a6a73 100644 --- a/test/lib/commands/deprecate.js +++ b/test/lib/commands/deprecate.js @@ -85,7 +85,7 @@ t.test('undeprecate', async t => { name: 'foo', versions, }) - registry.package({ manifest, query: { write: true } }) + await registry.package({ manifest, query: { write: true } }) registry.nock.put('/foo', body => { for (const version of versions) { if (body.versions[version].deprecated !== '') { @@ -110,7 +110,7 @@ t.test('deprecates given range', async t => { name: 'foo', versions, }) - registry.package({ manifest, query: { write: true } }) + await registry.package({ manifest, query: { write: true } }) const message = 'test deprecation message' registry.nock.put('/foo', body => { if (body.versions['1.0.1'].deprecated) { @@ -136,7 +136,7 @@ t.test('deprecates all versions when no range is specified', async t => { name: 'foo', versions, }) - registry.package({ manifest, query: { write: true } }) + await registry.package({ manifest, query: { write: true } }) const message = 'test deprecation message' registry.nock.put('/foo', body => { for (const version of versions) { diff --git a/test/lib/commands/owner.js b/test/lib/commands/owner.js index 800d5b96a5876..f8ab7feef5be7 100644 --- a/test/lib/commands/owner.js +++ b/test/lib/commands/owner.js @@ -46,7 +46,7 @@ function registryPackage (t, registry, name) { name, packuments: [{ maintainers, version: '1.0.0' }], }) - mockRegistry.package({ manifest }) + return mockRegistry.package({ manifest }) } t.test('owner no args', async t => { @@ -73,7 +73,7 @@ t.test('owner ls no args', async t => { name: packageName, packuments: [{ maintainers, version: '1.0.0' }], }) - registry.package({ manifest }) + await registry.package({ manifest }) await npm.exec('owner', ['ls']) t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n')) @@ -137,7 +137,7 @@ t.test('owner ls ', async t => { name: packageName, packuments: [{ maintainers, version: '1.0.0' }], }) - registry.package({ manifest }) + await registry.package({ manifest }) await npm.exec('owner', ['ls', packageName]) t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n')) @@ -153,7 +153,7 @@ t.test('owner ls no maintainers', async t => { name: packageName, versions: ['1.0.0'], }) - registry.package({ manifest }) + await registry.package({ manifest }) await npm.exec('owner', ['ls', packageName]) t.equal(joinedOutput(), 'no admin found') @@ -173,7 +173,7 @@ t.test('owner add ', async t => { packuments: [{ maintainers, version: '1.0.0' }], }) registry.couchuser({ username }) - registry.package({ manifest }) + await registry.package({ manifest }) registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`, body => { t.match(body, { _id: manifest._id, @@ -206,7 +206,7 @@ t.test('owner add cwd package', async t => { packuments: [{ maintainers, version: '1.0.0' }], }) registry.couchuser({ username }) - registry.package({ manifest }) + await registry.package({ manifest }) registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`, body => { t.match(body, { _id: manifest._id, @@ -236,7 +236,7 @@ t.test('owner add already an owner', async t => { packuments: [{ maintainers, version: '1.0.0' }], }) registry.couchuser({ username }) - registry.package({ manifest }) + await registry.package({ manifest }) await npm.exec('owner', ['add', username, packageName]) t.equal(joinedOutput(), '') t.match( @@ -273,7 +273,7 @@ t.test('owner add fails to PUT updates', async t => { packuments: [{ maintainers, version: '1.0.0' }], }) registry.couchuser({ username }) - registry.package({ manifest }) + await registry.package({ manifest }) registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`).reply(404, {}) await t.rejects( npm.exec('owner', ['add', username, packageName]), @@ -295,7 +295,7 @@ t.test('owner add no previous maintainers property from server', as packuments: [{ maintainers: undefined, version: '1.0.0' }], }) registry.couchuser({ username }) - registry.package({ manifest }) + await registry.package({ manifest }) registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`, body => { t.match(body, { _id: manifest._id, @@ -351,7 +351,7 @@ t.test('owner rm ', async t => { packuments: [{ maintainers, version: '1.0.0' }], }) registry.couchuser({ username }) - registry.package({ manifest }) + await registry.package({ manifest }) registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`, body => { t.match(body, { _id: manifest._id, @@ -378,7 +378,7 @@ t.test('owner rm not a current owner', async t => { packuments: [{ maintainers, version: '1.0.0' }], }) registry.couchuser({ username }) - registry.package({ manifest }) + await registry.package({ manifest }) await npm.exec('owner', ['rm', username, packageName]) t.match(logs.info, [['owner rm', `Not a package owner: ${username}`]]) }) @@ -400,7 +400,7 @@ t.test('owner rm cwd package', async t => { packuments: [{ maintainers, version: '1.0.0' }], }) registry.couchuser({ username }) - registry.package({ manifest }) + await registry.package({ manifest }) registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`, body => { t.match(body, { _id: manifest._id, @@ -430,7 +430,7 @@ t.test('owner rm only user', async t => { packuments: [{ maintainers: maintainers.slice(0, 1), version: '1.0.0' }], }) registry.couchuser({ username }) - registry.package({ manifest }) + await registry.package({ manifest }) await t.rejects( npm.exec('owner', ['rm', username]), { @@ -486,7 +486,7 @@ t.test('workspaces', async t => { 'process.cwd': () => path.join(prefix, 'workspace-a'), }), }) - registryPackage(t, npm.config.get('registry'), 'workspace-a') + await registryPackage(t, npm.config.get('registry'), 'workspace-a') await npm.exec('owner', ['ls']) t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n')) }) @@ -499,7 +499,7 @@ t.test('workspaces', async t => { }), }) npm.config.set('workspace', ['workspace-a']) - registryPackage(t, npm.config.get('registry'), 'workspace-a') + await registryPackage(t, npm.config.get('registry'), 'workspace-a') await npm.exec('owner', ['ls']) t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n')) }) @@ -511,7 +511,7 @@ t.test('workspaces', async t => { 'process.cwd': () => path.join(prefix, 'workspace-a'), }), }) - registryPackage(t, npm.config.get('registry'), packageName) + await registryPackage(t, npm.config.get('registry'), packageName) await npm.exec('owner', ['ls', packageName]) t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n')) }) @@ -524,7 +524,7 @@ t.test('workspaces', async t => { }), }) npm.config.set('workspace', ['workspace-a']) - registryPackage(t, npm.config.get('registry'), packageName) + await registryPackage(t, npm.config.get('registry'), packageName) await npm.exec('owner', ['ls', packageName]) t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n')) }) @@ -543,7 +543,7 @@ t.test('workspaces', async t => { name: 'workspace-a', packuments: [{ maintainers, version: '1.0.0' }], }) - registry.package({ manifest }) + await registry.package({ manifest }) registry.couchuser({ username }) registry.nock.put(`/workspace-a/-rev/${manifest._rev}`, body => { t.match(body, { @@ -572,7 +572,7 @@ t.test('workspaces', async t => { name: 'workspace-a', packuments: [{ maintainers, version: '1.0.0' }], }) - registry.package({ manifest }) + await registry.package({ manifest }) registry.couchuser({ username }) registry.nock.put(`/workspace-a/-rev/${manifest._rev}`, body => { t.match(body, { @@ -603,7 +603,7 @@ t.test('workspaces', async t => { name: 'workspace-a', packuments: [{ maintainers, version: '1.0.0' }], }) - registry.package({ manifest }) + await registry.package({ manifest }) registry.couchuser({ username }) registry.nock.put(`/workspace-a/-rev/${manifest._rev}`, body => { t.match(body, { @@ -649,7 +649,7 @@ t.test('completion', async t => { name: packageName, packuments: [{ maintainers, version: '1.0.0' }], }) - registry.package({ manifest }) + await registry.package({ manifest }) const res = await owner.completion({ conf: { argv: { remain: ['npm', 'owner', 'rm'] } } }) t.strictSame(res, maintainers.map(m => m.name), 'should return list of current owners') }) @@ -683,7 +683,7 @@ t.test('completion', async t => { name: packageName, packuments: [{ maintainers: [], version: '1.0.0' }], }) - registry.package({ manifest }) + await registry.package({ manifest }) const res = await owner.completion({ conf: { argv: { remain: ['npm', 'owner', 'rm'] } } }) t.strictSame(res, [], 'should return no owners if not found') diff --git a/test/lib/commands/star.js b/test/lib/commands/star.js index 5b79c07769628..ce9d258be1855 100644 --- a/test/lib/commands/star.js +++ b/test/lib/commands/star.js @@ -1,133 +1,61 @@ const t = require('tap') -const { fake: mockNpm } = require('../../fixtures/mock-npm') +const { load: loadMockNpm } = require('../../fixtures/mock-npm.js') +const MockRegistry = require('../../fixtures/mock-registry.js') -let result = '' - -const noop = () => null -const config = { - unicode: false, - 'star.unstar': false, -} -const npm = mockNpm({ - config, - output: (...msg) => { - result += msg.join('\n') - }, -}) -const npmFetch = { json: noop } -const log = { error: noop, info: noop, verbose: noop } -const mocks = { - 'proc-log': log, - 'npm-registry-fetch': npmFetch, - '../../../lib/utils/get-identity.js': async () => 'foo', -} - -const Star = t.mock('../../../lib/commands/star.js', mocks) -const star = new Star(npm) - -t.afterEach(() => { - config.unicode = false - config['star.unstar'] = false - log.info = noop - result = '' -}) +const pkgName = '@npmcli/test-package' +const authToken = 'test-auth-token' +const username = 'test-user' +const auth = { '//registry.npmjs.org/:_authToken': authToken } t.test('no args', async t => { + const { npm } = await loadMockNpm(t) await t.rejects( - star.exec([]), + npm.exec('star', []), { code: 'EUSAGE' }, 'should throw usage error' ) }) -t.test('star a package', async t => { - t.plan(4) - const pkgName = '@npmcli/arborist' - npmFetch.json = async (uri, opts) => { - return { - _id: pkgName, - _rev: 'hash', - users: ( - opts.method === 'PUT' - ? { foo: true } - : {} - ), - } - } - log.info = (title, msg, id) => { - t.equal(title, 'star', 'should use expected title') - t.equal(msg, 'starring', 'should use expected msg') - t.equal(id, pkgName, 'should use expected id') - } - await star.exec([pkgName]) - t.equal( - result, - '(*) @npmcli/arborist', - 'should output starred package msg' - ) -}) +t.test('first person to star a package unicode:false', async t => { + const { npm, joinedOutput } = await loadMockNpm(t, { + config: { unicode: false, ...auth }, + }) + const registry = new MockRegistry({ + tap: t, + registry: npm.config.get('registry'), + authorization: authToken, + }) + const manifest = registry.manifest({ name: pkgName }) + await registry.package({ manifest, query: { write: true } }) + registry.whoami({ username }) + registry.star(manifest, { [username]: true }) -t.test('unstar a package', async t => { - t.plan(4) - const pkgName = '@npmcli/arborist' - config['star.unstar'] = true - npmFetch.json = async (uri, opts) => { - return { - _id: pkgName, - _rev: 'hash', - ...(opts.method === 'PUT' - ? {} - : { foo: true } - ), - } - } - log.info = (title, msg, id) => { - t.equal(title, 'unstar', 'should use expected title') - t.equal(msg, 'unstarring', 'should use expected msg') - t.equal(id, pkgName, 'should use expected id') - } - await star.exec([pkgName]) + await npm.exec('star', [pkgName]) t.equal( - result, - '( ) @npmcli/arborist', - 'should output unstarred package msg' + joinedOutput(), + '(*) @npmcli/test-package', + 'should output starred package msg' ) }) -t.test('unicode', async t => { - t.test('star a package', async t => { - config.unicode = true - npmFetch.json = async (uri, opts) => ({}) - await star.exec(['pkg']) - t.equal( - result, - '\u2605 pkg', - 'should output unicode starred package msg' - ) +t.test('second person to star a package unicode:true', async t => { + const { npm, joinedOutput } = await loadMockNpm(t, { + config: { unicode: true, ...auth }, }) - - t.test('unstar a package', async t => { - config.unicode = true - config['star.unstar'] = true - npmFetch.json = async (uri, opts) => ({}) - await star.exec(['pkg']) - t.equal( - result, - '\u2606 pkg', - 'should output unstarred package msg' - ) + const registry = new MockRegistry({ + tap: t, + registry: npm.config.get('registry'), + authorization: authToken, }) -}) + const manifest = registry.manifest({ name: pkgName, users: { otheruser: true } }) + await registry.package({ manifest, query: { write: true } }) + registry.whoami({ username }) + registry.star(manifest, { otheruser: true, [username]: true }) -t.test('logged out user', async t => { - const Star = t.mock('../../../lib/commands/star.js', { - ...mocks, - '../../../lib/utils/get-identity.js': async () => undefined, - }) - const star = new Star(npm) - await t.rejects( - star.exec(['@npmcli/arborist']), - /You need to be logged in/, - 'should throw login required error' + await npm.exec('star', [pkgName]) + t.equal( + joinedOutput(), + '★ @npmcli/test-package', + 'should output starred package msg' ) }) diff --git a/test/lib/commands/unstar.js b/test/lib/commands/unstar.js index fb3c269b7bbdd..85c33d2793563 100644 --- a/test/lib/commands/unstar.js +++ b/test/lib/commands/unstar.js @@ -1,29 +1,62 @@ const t = require('tap') +const { load: loadMockNpm } = require('../../fixtures/mock-npm.js') +const MockRegistry = require('../../fixtures/mock-registry.js') -t.test('unstar', async t => { - t.plan(3) +const pkgName = '@npmcli/test-package' +const authToken = 'test-auth-token' +const username = 'test-user' +const auth = { '//registry.npmjs.org/:_authToken': authToken } - class Star { - constructor (npm) { - this.npm = npm - } +t.test('no args', async t => { + const { npm } = await loadMockNpm(t) + await t.rejects( + npm.exec('unstar', []), + { code: 'EUSAGE' }, + 'should throw usage error' + ) +}) + +t.test('unstar a package unicode:false', async t => { + const { npm, joinedOutput } = await loadMockNpm(t, { + config: { unicode: false, ...auth }, + }) + const registry = new MockRegistry({ + tap: t, + registry: npm.config.get('registry'), + authorization: authToken, + }) + const manifest = registry.manifest({ name: pkgName, users: { [username]: true } }) + await registry.package({ manifest, query: { write: true } }) + registry.whoami({ username }) + registry.star(manifest, {}) + + await npm.exec('unstar', [pkgName]) + t.equal( + joinedOutput(), + '( ) @npmcli/test-package', + 'should output unstarred package msg' + ) +}) - async exec (args) { - t.same(args, ['pkg'], 'should forward packages') - } - } - const Unstar = t.mock('../../../lib/commands/unstar.js', { - '../../../lib/commands/star.js': Star, +t.test('unstar a package unicode:true', async t => { + const { npm, joinedOutput } = await loadMockNpm(t, { + config: { unicode: true, ...auth }, }) - const unstar = new Unstar({ - config: { - set: (key, value) => { - t.equal(key, 'star.unstar', 'should set unstar config value') - t.equal(value, true, 'should set a truthy value') - }, - }, + const registry = new MockRegistry({ + tap: t, + registry: npm.config.get('registry'), + authorization: authToken, }) + const manifest = registry.manifest({ name: pkgName, users: { [username]: true } }) + await registry.package({ manifest, query: { write: true } }) + registry.whoami({ username }) + registry.star(manifest, {}) - await unstar.exec(['pkg']) + await npm.exec('unstar', [pkgName]) + t.equal( + joinedOutput(), + '☆ @npmcli/test-package', + 'should output unstarred package msg' + ) })