Skip to content

Commit

Permalink
fix: cleanup star/unstar (#4868)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
wraithgar authored May 9, 2022
1 parent 48d2db6 commit 38cf29a
Show file tree
Hide file tree
Showing 10 changed files with 159 additions and 187 deletions.
14 changes: 14 additions & 0 deletions docs/content/commands/npm-star.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,20 @@ false, it uses ascii characters instead of unicode glyphs.
<!-- automatically generated, do not edit manually -->
<!-- see lib/utils/config/definitions.js -->

#### `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.

<!-- automatically generated, do not edit manually -->
<!-- see lib/utils/config/definitions.js -->

<!-- AUTOGENERATED CONFIG DESCRIPTIONS END -->

### See Also
Expand Down
27 changes: 11 additions & 16 deletions lib/commands/star.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ class Star extends BaseCommand {
static params = [
'registry',
'unicode',
'otp',
]

static ignoreImplicitWorkspace = false
Expand All @@ -23,34 +24,28 @@ 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,
_rev: fullData._rev,
users: fullData.users || {},
}

if (!unstar) {
if (this.name === 'star') {
log.info('star', 'starring', body._id)
body.users[username] = true
log.verbose('star', 'starring', body)
Expand Down
10 changes: 0 additions & 10 deletions lib/commands/unstar.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion tap-snapshots/test/lib/load-all-commands.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -828,7 +828,7 @@ Usage:
npm star [<pkg>...]
Options:
[--registry <registry>] [--unicode]
[--registry <registry>] [--unicode] [--otp <otp>]
Run "npm help star" for more info
`
Expand Down
2 changes: 1 addition & 1 deletion tap-snapshots/test/lib/utils/npm-usage.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -862,7 +862,7 @@ All commands:
npm star [<pkg>...]
Options:
[--registry <registry>] [--unicode]
[--registry <registry>] [--unicode] [--otp <otp>]
Run "npm help star" for more info
Expand Down
14 changes: 13 additions & 1 deletion test/fixtures/mock-registry.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 = {
Expand All @@ -220,6 +229,9 @@ class MockRegistry {
'dist-tags': { latest: latest.version },
...latest,
}
if (users) {
manifest.users = users
}
if (versions) {
packuments = versions.map(version => ({ version }))
}
Expand Down
6 changes: 3 additions & 3 deletions test/lib/commands/deprecate.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 !== '') {
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down
44 changes: 22 additions & 22 deletions test/lib/commands/owner.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand All @@ -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'))
Expand Down Expand Up @@ -137,7 +137,7 @@ t.test('owner ls <pkg>', 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'))
Expand All @@ -153,7 +153,7 @@ t.test('owner ls <pkg> 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')
Expand All @@ -173,7 +173,7 @@ t.test('owner add <user> <pkg>', 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,
Expand Down Expand Up @@ -206,7 +206,7 @@ t.test('owner add <user> 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,
Expand Down Expand Up @@ -236,7 +236,7 @@ t.test('owner add <user> <pkg> 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(
Expand Down Expand Up @@ -273,7 +273,7 @@ t.test('owner add <user> <pkg> 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]),
Expand All @@ -295,7 +295,7 @@ t.test('owner add <user> <pkg> 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,
Expand Down Expand Up @@ -351,7 +351,7 @@ t.test('owner rm <user> <pkg>', 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,
Expand All @@ -378,7 +378,7 @@ t.test('owner rm <user> <pkg> 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}`]])
})
Expand All @@ -400,7 +400,7 @@ t.test('owner rm <user> 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,
Expand Down Expand Up @@ -430,7 +430,7 @@ t.test('owner rm <user> 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]),
{
Expand Down Expand Up @@ -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'))
})
Expand All @@ -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'))
})
Expand All @@ -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'))
})
Expand All @@ -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'))
})
Expand All @@ -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, {
Expand Down Expand Up @@ -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, {
Expand Down Expand Up @@ -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, {
Expand Down Expand Up @@ -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')
})
Expand Down Expand Up @@ -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')
Expand Down
Loading

0 comments on commit 38cf29a

Please sign in to comment.