From df596bf4c10d6917672579cc38800f5e846002bc Mon Sep 17 00:00:00 2001 From: Gar Date: Tue, 2 Feb 2021 14:53:07 -0800 Subject: [PATCH] fix(publish): follow configs for registry auth The "do you have auth configured" only takes into consideration a hard-coded "registry" config, which means that if you don't have auth configured for the npm registry, but you do for the one you have tied to a scope elsewhere in your npmrc, we would erroneously tell you that to add a token. This uses the same method that the rest of the publish chain uses to determine which registry it would be publishing to, then sees if you have auth for THAT registry. Because that other function does a hard fallback to the npm registry, there is no more need for the exception we throw if you do not have one configured. Also, the npm cli already defaults that config item, so you can't even set it to a falsey value if you wanted --- lib/publish.js | 41 +++++----- test/lib/publish.js | 179 +++++++++++++++++++++++++++++++------------- 2 files changed, 145 insertions(+), 75 deletions(-) diff --git a/lib/publish.js b/lib/publish.js index 49b2088070e7..190d381a8aee 100644 --- a/lib/publish.js +++ b/lib/publish.js @@ -6,6 +6,7 @@ const libpub = require('libnpmpublish').publish const runScript = require('@npmcli/run-script') const pacote = require('pacote') const npa = require('npm-package-arg') +const npmFetch = require('npm-registry-fetch') const npm = require('./npm.js') const output = require('./utils/output.js') @@ -71,27 +72,12 @@ const publish_ = async (arg, opts) => { // you can publish name@version, ./foo.tgz, etc. // even though the default is the 'file:.' cwd. const spec = npa(arg) - const manifest = await getManifest(spec, opts) + + let manifest = await getManifest(spec, opts) if (manifest.publishConfig) Object.assign(opts, publishConfigToOpts(manifest.publishConfig)) - const { registry } = opts - if (!registry) { - throw Object.assign(new Error('No registry specified.'), { - code: 'ENOREGISTRY', - }) - } - - if (!dryRun) { - const creds = npm.config.getCredentialsByURI(registry) - if (!creds.token && !creds.username) { - throw Object.assign(new Error('This command requires you to be logged in.'), { - code: 'ENEEDAUTH', - }) - } - } - // only run scripts for directory type publishes if (spec.type === 'directory') { await runScript({ @@ -105,18 +91,27 @@ const publish_ = async (arg, opts) => { const tarballData = await pack(spec, opts) const pkgContents = await getContents(manifest, tarballData) + // The purpose of re-reading the manifest is in case it changed, + // so that we send the latest and greatest thing to the registry + // note that publishConfig might have changed as well! + manifest = await getManifest(spec, opts) + if (manifest.publishConfig) + Object.assign(opts, publishConfigToOpts(manifest.publishConfig)) + // note that logTar calls npmlog.notice(), so if we ARE in silent mode, // this will do nothing, but we still want it in the debuglog if it fails. if (!json) logTar(pkgContents, { log, unicode }) if (!dryRun) { - // The purpose of re-reading the manifest is in case it changed, - // so that we send the latest and greatest thing to the registry - // note that publishConfig might have changed as well! - const manifest = await getManifest(spec, opts) - if (manifest.publishConfig) - Object.assign(opts, publishConfigToOpts(manifest.publishConfig)) + const resolved = npa.resolve(manifest.name, manifest.version) + const registry = npmFetch.pickRegistry(resolved, opts) + const creds = npm.config.getCredentialsByURI(registry) + if (!creds.token && !creds.username) { + throw Object.assign(new Error('This command requires you to be logged in.'), { + code: 'ENEEDAUTH', + }) + } await otplease(opts, opts => libpub(manifest, tarballData, opts)) } diff --git a/test/lib/publish.js b/test/lib/publish.js index f0ce0b966533..6d5cebf54069 100644 --- a/test/lib/publish.js +++ b/test/lib/publish.js @@ -4,19 +4,40 @@ const requireInject = require('require-inject') // mock config const {defaults} = require('../../lib/utils/config.js') const credentials = { - token: 'asdfasdf', - alwaysAuth: false, + 'https://unauthed.registry': { + email: 'me@example.com', + }, + 'https://scope.specific.registry': { + token: 'some.registry.token', + alwaysAuth: false, + }, + 'https://some.registry': { + token: 'some.registry.token', + alwaysAuth: false, + }, + 'https://registry.npmjs.org/': { + token: 'npmjs.registry.token', + alwaysAuth: false, + }, } const config = { list: [defaults], - getCredentialsByURI: () => credentials, } + +const registryCredentials = (t, registry) => { + return (uri) => { + t.same(uri, registry, 'gets credentials for expected registry') + return credentials[uri] + } +} + const fs = require('fs') t.test('should publish with libnpmpublish, respecting publishConfig', (t) => { - t.plan(5) + t.plan(6) - const publishConfig = { registry: 'https://some.registry' } + const registry = 'https://some.registry' + const publishConfig = { registry } const testDir = t.testdir({ 'package.json': JSON.stringify({ name: 'my-cool-pkg', @@ -30,9 +51,12 @@ t.test('should publish with libnpmpublish, respecting publishConfig', (t) => { flatOptions: { json: true, defaultTag: 'latest', - registry: 'https://registry.npmjs.org', + registry, + }, + config: { + ...config, + getCredentialsByURI: registryCredentials(t, registry), }, - config, }, '../../lib/utils/tar.js': { getContents: () => ({ @@ -71,8 +95,9 @@ t.test('should publish with libnpmpublish, respecting publishConfig', (t) => { }) t.test('re-loads publishConfig if added during script process', (t) => { - t.plan(5) - const publishConfig = { registry: 'https://some.registry' } + t.plan(6) + const registry = 'https://some.registry' + const publishConfig = { registry } const testDir = t.testdir({ 'package.json': JSON.stringify({ name: 'my-cool-pkg', @@ -87,7 +112,10 @@ t.test('re-loads publishConfig if added during script process', (t) => { defaultTag: 'latest', registry: 'https://registry.npmjs.org/', }, - config, + config: { + ...config, + getCredentialsByURI: registryCredentials(t, registry), + }, }, '../../lib/utils/tar.js': { getContents: () => ({ @@ -112,7 +140,7 @@ t.test('re-loads publishConfig if added during script process', (t) => { t.match(manifest, { name: 'my-cool-pkg', version: '1.0.0' }, 'gets manifest') t.isa(tarData, Buffer, 'tarData is a buffer') t.ok(opts, 'gets opts object') - t.same(opts.registry, publishConfig.registry, 'publishConfig is passed through') + t.same(opts.registry, registry, 'publishConfig is passed through') }, }, }) @@ -124,9 +152,10 @@ t.test('re-loads publishConfig if added during script process', (t) => { }) }) -t.test('should not log if silent', (t) => { +t.test('should not log if silent (dry run)', (t) => { t.plan(2) + const registry = 'https://registry.npmjs.org' const testDir = t.testdir({ 'package.json': JSON.stringify({ name: 'my-cool-pkg', @@ -140,9 +169,14 @@ t.test('should not log if silent', (t) => { json: false, defaultTag: 'latest', dryRun: true, - registry: 'https://registry.npmjs.org/', + registry, + }, + config: { + ...config, + getCredentialsByURI: () => { + throw new Error('should not call getCredentialsByURI in dry run') + }, }, - config, }, '../../lib/utils/tar.js': { getContents: () => ({}), @@ -164,7 +198,7 @@ t.test('should not log if silent', (t) => { libnpmpack: async () => '', libnpmpublish: { publish: (manifest, tarData, opts) => { - throw new Error('should not call libnpmpublish!') + throw new Error('should not call libnpmpublish in dry run') }, }, }) @@ -176,8 +210,10 @@ t.test('should not log if silent', (t) => { }) }) -t.test('should log tarball contents', (t) => { +t.test('should log tarball contents (dry run)', (t) => { t.plan(3) + + const registry = 'https://registry.npmjs.org' const testDir = t.testdir({ 'package.json': JSON.stringify({ name: 'my-cool-pkg', @@ -191,12 +227,12 @@ t.test('should log tarball contents', (t) => { json: false, defaultTag: 'latest', dryRun: true, - registry: 'https://registry.npmjs.org/', + registry, }, config: { ...config, getCredentialsByURI: () => { - throw new Error('should not call getCredentialsByURI!') + throw new Error('should not call getCredentialsByURI in dry run') }}, }, '../../lib/utils/tar.js': { @@ -216,7 +252,7 @@ t.test('should log tarball contents', (t) => { libnpmpack: async () => '', libnpmpublish: { publish: () => { - throw new Error('should not call libnpmpublish!') + throw new Error('should not call libnpmpublish in dry run') }, }, }) @@ -246,12 +282,15 @@ t.test('shows usage with wrong set of arguments', (t) => { t.test('throws when invalid tag', (t) => { t.plan(1) + + const registry = 'https://registry.npmjs.org' + const publish = requireInject('../../lib/publish.js', { '../../lib/npm.js': { flatOptions: { json: false, defaultTag: '0.0.13', - registry: 'https://registry.npmjs.org/', + registry, }, config, }, @@ -265,7 +304,9 @@ t.test('throws when invalid tag', (t) => { }) t.test('can publish a tarball', t => { - t.plan(3) + t.plan(4) + + const registry = 'https://registry.npmjs.org/' const testDir = t.testdir({ package: { 'package.json': JSON.stringify({ @@ -291,9 +332,12 @@ t.test('can publish a tarball', t => { flatOptions: { json: true, defaultTag: 'latest', - registry: 'https://registry.npmjs.org/', + registry, + }, + config: { + ...config, + getCredentialsByURI: registryCredentials(t, registry), }, - config, }, '../../lib/utils/tar.js': { getContents: () => ({ @@ -323,39 +367,25 @@ t.test('can publish a tarball', t => { }) }) -t.test('throw if no registry', async t => { - t.plan(1) - const publish = requireInject('../../lib/publish.js', { - '../../lib/npm.js': { - flatOptions: { - json: false, - registry: null, - }, - config, - }, - }) - - return publish([], (err) => { - t.match(err, { - message: 'No registry specified.', - code: 'ENOREGISTRY', - }, 'throws when registry unset') - }) -}) - t.test('throw if not logged in', async t => { - t.plan(1) + t.plan(2) + const registry = 'https://unauthed.registry' + const publish = requireInject('../../lib/publish.js', { + '../../lib/utils/tar.js': { + getContents: () => ({ + id: 'someid', + }), + logTar: () => {}, + }, '../../lib/npm.js': { flatOptions: { json: false, - registry: 'https://registry.npmjs.org/', + registry, }, config: { ...config, - getCredentialsByURI: () => ({ - email: 'me@example.com', - }), + getCredentialsByURI: registryCredentials(t, registry), }, }, }) @@ -369,9 +399,10 @@ t.test('throw if not logged in', async t => { }) t.test('read registry only from publishConfig', t => { - t.plan(3) + t.plan(4) - const publishConfig = { registry: 'https://some.registry' } + const registry = 'https://some.registry' + const publishConfig = { registry } const testDir = t.testdir({ 'package.json': JSON.stringify({ name: 'my-cool-pkg', @@ -385,7 +416,10 @@ t.test('read registry only from publishConfig', t => { flatOptions: { json: false, }, - config, + config: { + ...config, + getCredentialsByURI: registryCredentials(t, registry), + }, }, '../../lib/utils/tar.js': { getContents: () => ({ @@ -397,7 +431,7 @@ t.test('read registry only from publishConfig', t => { libnpmpublish: { publish: (manifest, tarData, opts) => { t.match(manifest, { name: 'my-cool-pkg', version: '1.0.0' }, 'gets manifest') - t.same(opts.registry, publishConfig.registry, 'publishConfig is passed through') + t.same(opts.registry, registry, 'publishConfig is passed through') }, }, }) @@ -408,3 +442,44 @@ t.test('read registry only from publishConfig', t => { t.pass('got to callback') }) }) + +t.test('should check auth for scope specific registry', t => { + const testDir = t.testdir({ + 'package.json': JSON.stringify({ + name: '@npm/my-cool-pkg', + version: '1.0.0', + }, null, 2), + }) + + const registry = 'https://scope.specific.registry' + const publish = requireInject('../../lib/publish.js', { + '../../lib/npm.js': { + flatOptions: { + json: false, + '@npm:registry': registry, + }, + config: { + ...config, + getCredentialsByURI: registryCredentials(t, registry), + }, + }, + '../../lib/utils/tar.js': { + getContents: () => ({ + id: 'someid', + }), + logTar: () => {}, + }, + '../../lib/utils/output.js': () => {}, + '../../lib/utils/otplease.js': (opts, fn) => { + return Promise.resolve().then(() => fn(opts)) + }, + libnpmpublish: { + publish: () => '', + }, + }) + return publish([testDir], (er) => { + if (er) + throw er + t.pass('got to callback') + }) +})