Skip to content

Commit 38cf29a

Browse files
wraithgarwraithgar
authored
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.
1 parent 48d2db6 commit 38cf29a

File tree

10 files changed

+159
-187
lines changed

10 files changed

+159
-187
lines changed

‎docs/content/commands/npm-star.md

+14
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,20 @@ false, it uses ascii characters instead of unicode glyphs.
6969
<!-- automatically generated, do not edit manually -->
7070
<!-- see lib/utils/config/definitions.js -->
7171

72+
#### `otp`
73+
74+
* Default: null
75+
* Type: null or String
76+
77+
This is a one-time password from a two-factor authenticator. It's needed
78+
when publishing or changing package permissions with `npm access`.
79+
80+
If not set, and a registry response fails with a challenge for a one-time
81+
password, npm will prompt on the command line for one.
82+
83+
<!-- automatically generated, do not edit manually -->
84+
<!-- see lib/utils/config/definitions.js -->
85+
7286
<!-- AUTOGENERATED CONFIG DESCRIPTIONS END -->
7387

7488
### See Also

‎lib/commands/star.js

+11-16
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ class Star extends BaseCommand {
1111
static params = [
1212
'registry',
1313
'unicode',
14+
'otp',
1415
]
1516

1617
static ignoreImplicitWorkspace = false
@@ -23,34 +24,28 @@ class Star extends BaseCommand {
2324
// if we're unstarring, then show an empty star image
2425
// otherwise, show the full star image
2526
const unicode = this.npm.config.get('unicode')
26-
const unstar = this.npm.config.get('star.unstar')
2727
const full = unicode ? '\u2605 ' : '(*)'
2828
const empty = unicode ? '\u2606 ' : '( )'
29-
const show = unstar ? empty : full
29+
const show = this.name === 'star' ? full : empty
3030

3131
const pkgs = args.map(npa)
32-
for (const pkg of pkgs) {
33-
const [username, fullData] = await Promise.all([
34-
getIdentity(this.npm, { ...this.npm.flatOptions }),
35-
fetch.json(pkg.escapedName, {
36-
...this.npm.flatOptions,
37-
spec: pkg,
38-
query: { write: true },
39-
preferOnline: true,
40-
}),
41-
])
32+
const username = await getIdentity(this.npm, this.npm.flatOptions)
4233

43-
if (!username) {
44-
throw new Error('You need to be logged in!')
45-
}
34+
for (const pkg of pkgs) {
35+
const fullData = await fetch.json(pkg.escapedName, {
36+
...this.npm.flatOptions,
37+
spec: pkg,
38+
query: { write: true },
39+
preferOnline: true,
40+
})
4641

4742
const body = {
4843
_id: fullData._id,
4944
_rev: fullData._rev,
5045
users: fullData.users || {},
5146
}
5247

53-
if (!unstar) {
48+
if (this.name === 'star') {
5449
log.info('star', 'starring', body._id)
5550
body.users[username] = true
5651
log.verbose('star', 'starring', body)

‎lib/commands/unstar.js

-10
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,5 @@ const Star = require('./star.js')
33
class Unstar extends Star {
44
static description = 'Remove an item from your favorite packages'
55
static name = 'unstar'
6-
static params = [
7-
'registry',
8-
'unicode',
9-
'otp',
10-
]
11-
12-
async exec (args) {
13-
this.npm.config.set('star.unstar', true)
14-
return super.exec(args)
15-
}
166
}
177
module.exports = Unstar

‎tap-snapshots/test/lib/load-all-commands.js.test.cjs

+1-1
Original file line numberDiff line numberDiff line change
@@ -828,7 +828,7 @@ Usage:
828828
npm star [<pkg>...]
829829
830830
Options:
831-
[--registry <registry>] [--unicode]
831+
[--registry <registry>] [--unicode] [--otp <otp>]
832832
833833
Run "npm help star" for more info
834834
`

‎tap-snapshots/test/lib/utils/npm-usage.js.test.cjs

+1-1
Original file line numberDiff line numberDiff line change
@@ -862,7 +862,7 @@ All commands:
862862
npm star [<pkg>...]
863863
864864
Options:
865-
[--registry <registry>] [--unicode]
865+
[--registry <registry>] [--unicode] [--otp <otp>]
866866
867867
Run "npm help star" for more info
868868

‎test/fixtures/mock-registry.js

+13-1
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,15 @@ class MockRegistry {
183183
}
184184
}
185185

186+
star (manifest, users) {
187+
const spec = npa(manifest.name)
188+
this.nock = this.nock.put(`/${spec.escapedName}`, {
189+
_id: manifest._id,
190+
_rev: manifest._rev,
191+
users,
192+
}).reply(200, { ...manifest, users })
193+
}
194+
186195
async package ({ manifest, times = 1, query, tarballs }) {
187196
let nock = this.nock
188197
const spec = npa(manifest.name)
@@ -206,7 +215,7 @@ class MockRegistry {
206215
// either pass in packuments if you need to set specific attributes besides version,
207216
// or an array of versions
208217
// the last packument in the packuments or versions array will be tagged latest
209-
manifest ({ name = 'test-package', packuments, versions } = {}) {
218+
manifest ({ name = 'test-package', users, packuments, versions } = {}) {
210219
packuments = this.packuments(packuments, name)
211220
const latest = packuments.slice(-1)[0]
212221
const manifest = {
@@ -220,6 +229,9 @@ class MockRegistry {
220229
'dist-tags': { latest: latest.version },
221230
...latest,
222231
}
232+
if (users) {
233+
manifest.users = users
234+
}
223235
if (versions) {
224236
packuments = versions.map(version => ({ version }))
225237
}

‎test/lib/commands/deprecate.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ t.test('undeprecate', async t => {
8585
name: 'foo',
8686
versions,
8787
})
88-
registry.package({ manifest, query: { write: true } })
88+
await registry.package({ manifest, query: { write: true } })
8989
registry.nock.put('/foo', body => {
9090
for (const version of versions) {
9191
if (body.versions[version].deprecated !== '') {
@@ -110,7 +110,7 @@ t.test('deprecates given range', async t => {
110110
name: 'foo',
111111
versions,
112112
})
113-
registry.package({ manifest, query: { write: true } })
113+
await registry.package({ manifest, query: { write: true } })
114114
const message = 'test deprecation message'
115115
registry.nock.put('/foo', body => {
116116
if (body.versions['1.0.1'].deprecated) {
@@ -136,7 +136,7 @@ t.test('deprecates all versions when no range is specified', async t => {
136136
name: 'foo',
137137
versions,
138138
})
139-
registry.package({ manifest, query: { write: true } })
139+
await registry.package({ manifest, query: { write: true } })
140140
const message = 'test deprecation message'
141141
registry.nock.put('/foo', body => {
142142
for (const version of versions) {

‎test/lib/commands/owner.js

+22-22
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ function registryPackage (t, registry, name) {
4646
name,
4747
packuments: [{ maintainers, version: '1.0.0' }],
4848
})
49-
mockRegistry.package({ manifest })
49+
return mockRegistry.package({ manifest })
5050
}
5151

5252
t.test('owner no args', async t => {
@@ -73,7 +73,7 @@ t.test('owner ls no args', async t => {
7373
name: packageName,
7474
packuments: [{ maintainers, version: '1.0.0' }],
7575
})
76-
registry.package({ manifest })
76+
await registry.package({ manifest })
7777

7878
await npm.exec('owner', ['ls'])
7979
t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n'))
@@ -137,7 +137,7 @@ t.test('owner ls <pkg>', async t => {
137137
name: packageName,
138138
packuments: [{ maintainers, version: '1.0.0' }],
139139
})
140-
registry.package({ manifest })
140+
await registry.package({ manifest })
141141

142142
await npm.exec('owner', ['ls', packageName])
143143
t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n'))
@@ -153,7 +153,7 @@ t.test('owner ls <pkg> no maintainers', async t => {
153153
name: packageName,
154154
versions: ['1.0.0'],
155155
})
156-
registry.package({ manifest })
156+
await registry.package({ manifest })
157157

158158
await npm.exec('owner', ['ls', packageName])
159159
t.equal(joinedOutput(), 'no admin found')
@@ -173,7 +173,7 @@ t.test('owner add <user> <pkg>', async t => {
173173
packuments: [{ maintainers, version: '1.0.0' }],
174174
})
175175
registry.couchuser({ username })
176-
registry.package({ manifest })
176+
await registry.package({ manifest })
177177
registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`, body => {
178178
t.match(body, {
179179
_id: manifest._id,
@@ -206,7 +206,7 @@ t.test('owner add <user> cwd package', async t => {
206206
packuments: [{ maintainers, version: '1.0.0' }],
207207
})
208208
registry.couchuser({ username })
209-
registry.package({ manifest })
209+
await registry.package({ manifest })
210210
registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`, body => {
211211
t.match(body, {
212212
_id: manifest._id,
@@ -236,7 +236,7 @@ t.test('owner add <user> <pkg> already an owner', async t => {
236236
packuments: [{ maintainers, version: '1.0.0' }],
237237
})
238238
registry.couchuser({ username })
239-
registry.package({ manifest })
239+
await registry.package({ manifest })
240240
await npm.exec('owner', ['add', username, packageName])
241241
t.equal(joinedOutput(), '')
242242
t.match(
@@ -273,7 +273,7 @@ t.test('owner add <user> <pkg> fails to PUT updates', async t => {
273273
packuments: [{ maintainers, version: '1.0.0' }],
274274
})
275275
registry.couchuser({ username })
276-
registry.package({ manifest })
276+
await registry.package({ manifest })
277277
registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`).reply(404, {})
278278
await t.rejects(
279279
npm.exec('owner', ['add', username, packageName]),
@@ -295,7 +295,7 @@ t.test('owner add <user> <pkg> no previous maintainers property from server', as
295295
packuments: [{ maintainers: undefined, version: '1.0.0' }],
296296
})
297297
registry.couchuser({ username })
298-
registry.package({ manifest })
298+
await registry.package({ manifest })
299299
registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`, body => {
300300
t.match(body, {
301301
_id: manifest._id,
@@ -351,7 +351,7 @@ t.test('owner rm <user> <pkg>', async t => {
351351
packuments: [{ maintainers, version: '1.0.0' }],
352352
})
353353
registry.couchuser({ username })
354-
registry.package({ manifest })
354+
await registry.package({ manifest })
355355
registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`, body => {
356356
t.match(body, {
357357
_id: manifest._id,
@@ -378,7 +378,7 @@ t.test('owner rm <user> <pkg> not a current owner', async t => {
378378
packuments: [{ maintainers, version: '1.0.0' }],
379379
})
380380
registry.couchuser({ username })
381-
registry.package({ manifest })
381+
await registry.package({ manifest })
382382
await npm.exec('owner', ['rm', username, packageName])
383383
t.match(logs.info, [['owner rm', `Not a package owner: ${username}`]])
384384
})
@@ -400,7 +400,7 @@ t.test('owner rm <user> cwd package', async t => {
400400
packuments: [{ maintainers, version: '1.0.0' }],
401401
})
402402
registry.couchuser({ username })
403-
registry.package({ manifest })
403+
await registry.package({ manifest })
404404
registry.nock.put(`/${spec.escapedName}/-rev/${manifest._rev}`, body => {
405405
t.match(body, {
406406
_id: manifest._id,
@@ -430,7 +430,7 @@ t.test('owner rm <user> only user', async t => {
430430
packuments: [{ maintainers: maintainers.slice(0, 1), version: '1.0.0' }],
431431
})
432432
registry.couchuser({ username })
433-
registry.package({ manifest })
433+
await registry.package({ manifest })
434434
await t.rejects(
435435
npm.exec('owner', ['rm', username]),
436436
{
@@ -486,7 +486,7 @@ t.test('workspaces', async t => {
486486
'process.cwd': () => path.join(prefix, 'workspace-a'),
487487
}),
488488
})
489-
registryPackage(t, npm.config.get('registry'), 'workspace-a')
489+
await registryPackage(t, npm.config.get('registry'), 'workspace-a')
490490
await npm.exec('owner', ['ls'])
491491
t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n'))
492492
})
@@ -499,7 +499,7 @@ t.test('workspaces', async t => {
499499
}),
500500
})
501501
npm.config.set('workspace', ['workspace-a'])
502-
registryPackage(t, npm.config.get('registry'), 'workspace-a')
502+
await registryPackage(t, npm.config.get('registry'), 'workspace-a')
503503
await npm.exec('owner', ['ls'])
504504
t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n'))
505505
})
@@ -511,7 +511,7 @@ t.test('workspaces', async t => {
511511
'process.cwd': () => path.join(prefix, 'workspace-a'),
512512
}),
513513
})
514-
registryPackage(t, npm.config.get('registry'), packageName)
514+
await registryPackage(t, npm.config.get('registry'), packageName)
515515
await npm.exec('owner', ['ls', packageName])
516516
t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n'))
517517
})
@@ -524,7 +524,7 @@ t.test('workspaces', async t => {
524524
}),
525525
})
526526
npm.config.set('workspace', ['workspace-a'])
527-
registryPackage(t, npm.config.get('registry'), packageName)
527+
await registryPackage(t, npm.config.get('registry'), packageName)
528528
await npm.exec('owner', ['ls', packageName])
529529
t.match(joinedOutput(), maintainers.map(m => `${m.name} <${m.email}>`).join('\n'))
530530
})
@@ -543,7 +543,7 @@ t.test('workspaces', async t => {
543543
name: 'workspace-a',
544544
packuments: [{ maintainers, version: '1.0.0' }],
545545
})
546-
registry.package({ manifest })
546+
await registry.package({ manifest })
547547
registry.couchuser({ username })
548548
registry.nock.put(`/workspace-a/-rev/${manifest._rev}`, body => {
549549
t.match(body, {
@@ -572,7 +572,7 @@ t.test('workspaces', async t => {
572572
name: 'workspace-a',
573573
packuments: [{ maintainers, version: '1.0.0' }],
574574
})
575-
registry.package({ manifest })
575+
await registry.package({ manifest })
576576
registry.couchuser({ username })
577577
registry.nock.put(`/workspace-a/-rev/${manifest._rev}`, body => {
578578
t.match(body, {
@@ -603,7 +603,7 @@ t.test('workspaces', async t => {
603603
name: 'workspace-a',
604604
packuments: [{ maintainers, version: '1.0.0' }],
605605
})
606-
registry.package({ manifest })
606+
await registry.package({ manifest })
607607
registry.couchuser({ username })
608608
registry.nock.put(`/workspace-a/-rev/${manifest._rev}`, body => {
609609
t.match(body, {
@@ -649,7 +649,7 @@ t.test('completion', async t => {
649649
name: packageName,
650650
packuments: [{ maintainers, version: '1.0.0' }],
651651
})
652-
registry.package({ manifest })
652+
await registry.package({ manifest })
653653
const res = await owner.completion({ conf: { argv: { remain: ['npm', 'owner', 'rm'] } } })
654654
t.strictSame(res, maintainers.map(m => m.name), 'should return list of current owners')
655655
})
@@ -683,7 +683,7 @@ t.test('completion', async t => {
683683
name: packageName,
684684
packuments: [{ maintainers: [], version: '1.0.0' }],
685685
})
686-
registry.package({ manifest })
686+
await registry.package({ manifest })
687687

688688
const res = await owner.completion({ conf: { argv: { remain: ['npm', 'owner', 'rm'] } } })
689689
t.strictSame(res, [], 'should return no owners if not found')

0 commit comments

Comments
 (0)