From ee3308a7a08799ec7e86237165ebaf278d9a4f9f Mon Sep 17 00:00:00 2001 From: Gar Date: Wed, 18 May 2022 15:51:43 -0700 Subject: [PATCH] fix: remove dead code from get-identity --- lib/utils/get-identity.js | 45 ++--- .../npm-usage.js.test.cjs => npm.js.test.cjs} | 10 +- test/fixtures/mock-registry.js | 4 + test/lib/commands/deprecate.js | 2 +- test/lib/commands/ping.js | 154 ++++++------------ test/lib/commands/whoami.js | 24 ++- test/lib/npm.js | 58 +++++++ test/lib/utils/get-identity.js | 103 ------------ test/lib/utils/is-windows.js | 39 ----- test/lib/utils/npm-usage.js | 60 ------- test/lib/utils/ping.js | 35 ---- 11 files changed, 159 insertions(+), 375 deletions(-) rename tap-snapshots/test/lib/{utils/npm-usage.js.test.cjs => npm.js.test.cjs} (98%) delete mode 100644 test/lib/utils/get-identity.js delete mode 100644 test/lib/utils/is-windows.js delete mode 100644 test/lib/utils/npm-usage.js delete mode 100644 test/lib/utils/ping.js diff --git a/lib/utils/get-identity.js b/lib/utils/get-identity.js index e77c2eea4c5ab..f4aedb89b3957 100644 --- a/lib/utils/get-identity.js +++ b/lib/utils/get-identity.js @@ -1,39 +1,24 @@ const npmFetch = require('npm-registry-fetch') -const needsAuthError = (msg) => - Object.assign(new Error(msg), { code: 'ENEEDAUTH' }) - -module.exports = async (npm, opts = {}) => { +module.exports = async (npm, opts) => { const { registry } = opts - if (!registry) { - throw Object.assign(new Error('No registry specified.'), { code: 'ENOREGISTRY' }) - } // First, check if we have a user/pass-based auth const creds = npm.config.getCredentialsByURI(registry) - const { username: usernameFromURI, token } = creds + if (creds.username) { + return creds.username + } - if (usernameFromURI) { - // Found username; return it - return usernameFromURI - } else if (token) { - // No username, but we have a token; fetch the username from registry - const registryData = await npmFetch.json('/-/whoami', { - ...opts, - }) - const { username: usernameFromRegistry } = registryData - // Retrieved username from registry; return it - if (usernameFromRegistry) { - return usernameFromRegistry - } else { - // Didn't get username from registry; bad token - throw needsAuthError( - 'Your auth token is no longer valid. Please login again.' - ) - } - } else { - // At this point, if they have a credentials object, it doesn't have a - // token or auth in it. Probably just the default registry. - throw needsAuthError('This command requires you to be logged in.') + // No username, but we have a token; fetch the username from registry + if (creds.token) { + const registryData = await npmFetch.json('/-/whoami', { ...opts }) + return registryData.username } + + // At this point, even if they have a credentials object, it doesn't have a + // valid token. + throw Object.assign( + new Error('This command requires you to be logged in.'), + { code: 'ENEEDAUTH' } + ) } diff --git a/tap-snapshots/test/lib/utils/npm-usage.js.test.cjs b/tap-snapshots/test/lib/npm.js.test.cjs similarity index 98% rename from tap-snapshots/test/lib/utils/npm-usage.js.test.cjs rename to tap-snapshots/test/lib/npm.js.test.cjs index e7660911f1eb9..e71bc8268c495 100644 --- a/tap-snapshots/test/lib/utils/npm-usage.js.test.cjs +++ b/tap-snapshots/test/lib/npm.js.test.cjs @@ -5,7 +5,7 @@ * Make sure to inspect the output below. Do not ignore changes! */ 'use strict' -exports[`test/lib/utils/npm-usage.js TAP usage basic usage > must match snapshot 1`] = ` +exports[`test/lib/npm.js TAP usage basic usage > must match snapshot 1`] = ` npm Usage: @@ -41,7 +41,7 @@ Configuration fields: npm help 7 config npm@{VERSION} {BASEDIR} ` -exports[`test/lib/utils/npm-usage.js TAP usage set process.stdout.columns columns=0 > must match snapshot 1`] = ` +exports[`test/lib/npm.js TAP usage set process.stdout.columns columns=0 > must match snapshot 1`] = ` npm Usage: @@ -77,7 +77,7 @@ Configuration fields: npm help 7 config npm@{VERSION} {BASEDIR} ` -exports[`test/lib/utils/npm-usage.js TAP usage set process.stdout.columns columns=90 > must match snapshot 1`] = ` +exports[`test/lib/npm.js TAP usage set process.stdout.columns columns=90 > must match snapshot 1`] = ` npm Usage: @@ -113,7 +113,7 @@ Configuration fields: npm help 7 config npm@{VERSION} {BASEDIR} ` -exports[`test/lib/utils/npm-usage.js TAP usage with browser > must match snapshot 1`] = ` +exports[`test/lib/npm.js TAP usage with browser > must match snapshot 1`] = ` npm Usage: @@ -149,7 +149,7 @@ Configuration fields: npm help 7 config npm@{VERSION} {BASEDIR} ` -exports[`test/lib/utils/npm-usage.js TAP usage with long > must match snapshot 1`] = ` +exports[`test/lib/npm.js TAP usage with long > must match snapshot 1`] = ` npm Usage: diff --git a/test/fixtures/mock-registry.js b/test/fixtures/mock-registry.js index 61263da216efa..8fb5a055ff2d7 100644 --- a/test/fixtures/mock-registry.js +++ b/test/fixtures/mock-registry.js @@ -192,6 +192,10 @@ class MockRegistry { }).reply(200, { ...manifest, users }) } + ping ({ body = {}, responseCode = 200 } = {}) { + this.nock = this.nock.get('/-/ping?write=true').reply(responseCode, body) + } + async package ({ manifest, times = 1, query, tarballs }) { let nock = this.nock const spec = npa(manifest.name) diff --git a/test/lib/commands/deprecate.js b/test/lib/commands/deprecate.js index 8a925fc2a6a73..3a610a703a2dc 100644 --- a/test/lib/commands/deprecate.js +++ b/test/lib/commands/deprecate.js @@ -44,7 +44,7 @@ t.test('completion', async t => { registry.whoami({ statusCode: 404, body: {} }) - t.rejects(testComp([], []), { code: 'ENEEDAUTH' }) + t.rejects(testComp([], []), { code: 'EINVALIDTYPE' }) }) t.test('no args', async t => { diff --git a/test/lib/commands/ping.js b/test/lib/commands/ping.js index f808e0ac3ba2a..dd2f83de08fc9 100644 --- a/test/lib/commands/ping.js +++ b/test/lib/commands/ping.js @@ -1,113 +1,67 @@ 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') -t.test('pings', async t => { - t.plan(6) - - const registry = 'https://registry.npmjs.org' - let noticeCalls = 0 - const Ping = t.mock('../../../lib/commands/ping.js', { - '../../../lib/utils/ping.js': function (spec) { - t.equal(spec.registry, registry, 'passes flatOptions') - return {} - }, - 'proc-log': { - notice: (type, spec) => { - ++noticeCalls - if (noticeCalls === 1) { - t.equal(type, 'PING', 'should log a PING') - t.equal(spec, registry, 'should log the registry url') - } else { - t.equal(type, 'PONG', 'should log a PONG') - t.match(spec, /\d+ms/, 'should log the elapsed milliseconds') - } - }, - }, - }) - const npm = mockNpm({ - config: { registry }, - flatOptions: { registry }, +t.test('no details', async t => { + const { npm, logs, joinedOutput } = await loadMockNpm(t) + const registry = new MockRegistry({ + tap: t, + registry: npm.config.get('registry'), }) - const ping = new Ping(npm) - - await ping.exec([]) - t.equal(noticeCalls, 2, 'should have logged 2 lines') + registry.ping() + await npm.exec('ping', []) + t.match(logs.notice, [['PING', 'https://registry.npmjs.org/'], ['PONG', /[0-9]+ms/]]) + t.equal(joinedOutput(), '') }) -t.test('pings and logs details', async t => { - t.plan(8) +t.test('with details', async t => { + const { npm, logs, joinedOutput } = await loadMockNpm(t) + const registry = new MockRegistry({ + tap: t, + registry: npm.config.get('registry'), + }) + registry.ping({ body: { test: true } }) + await npm.exec('ping', []) + t.match(logs.notice, [ + ['PING', 'https://registry.npmjs.org/'], + ['PONG', /[0-9]+ms/], + ['PONG', '{\n "test": true\n}'], + ]) + t.match(joinedOutput(), '') +}) - const registry = 'https://registry.npmjs.org' - const details = { extra: 'data' } - let noticeCalls = 0 - const Ping = t.mock('../../../lib/commands/ping.js', { - '../../../lib/utils/ping.js': function (spec) { - t.equal(spec.registry, registry, 'passes flatOptions') - return details - }, - 'proc-log': { - notice: (type, spec) => { - ++noticeCalls - if (noticeCalls === 1) { - t.equal(type, 'PING', 'should log a PING') - t.equal(spec, registry, 'should log the registry url') - } else if (noticeCalls === 2) { - t.equal(type, 'PONG', 'should log a PONG') - t.match(spec, /\d+ms/, 'should log the elapsed milliseconds') - } else { - t.equal(type, 'PONG', 'should log a PONG') - const parsed = JSON.parse(spec) - t.match(parsed, details, 'should log JSON stringified details') - } - }, - }, +t.test('valid json', async t => { + const { npm, logs, joinedOutput } = await loadMockNpm(t, { + config: { json: true }, }) - const npm = mockNpm({ - config: { registry }, - flatOptions: { registry }, + const registry = new MockRegistry({ + tap: t, + registry: npm.config.get('registry'), + }) + registry.ping() + await npm.exec('ping', []) + t.match(logs.notice, [['PING', 'https://registry.npmjs.org/'], ['PONG', /[0-9]+ms/]]) + t.match(JSON.parse(joinedOutput()), { + registry: npm.config.get('registry'), + time: /[0-9]+/, + details: {}, }) - const ping = new Ping(npm) - - await ping.exec([]) - t.equal(noticeCalls, 3, 'should have logged 3 lines') }) -t.test('pings and returns json', async t => { - t.plan(9) - - const registry = 'https://registry.npmjs.org' - const details = { extra: 'data' } - let noticeCalls = 0 - const Ping = t.mock('../../../lib/commands/ping.js', { - '../../../lib/utils/ping.js': function (spec) { - t.equal(spec.registry, registry, 'passes flatOptions') - return details - }, - 'proc-log': { - notice: (type, spec) => { - ++noticeCalls - if (noticeCalls === 1) { - t.equal(type, 'PING', 'should log a PING') - t.equal(spec, registry, 'should log the registry url') - } else { - t.equal(type, 'PONG', 'should log a PONG') - t.match(spec, /\d+ms/, 'should log the elapsed milliseconds') - } - }, - }, +t.test('invalid json', async t => { + const { npm, logs, joinedOutput } = await loadMockNpm(t, { + config: { json: true }, }) - const npm = mockNpm({ - config: { registry, json: true }, - flatOptions: { registry }, - output: function (spec) { - const parsed = JSON.parse(spec) - t.equal(parsed.registry, registry, 'returns the correct registry url') - t.match(parsed.details, details, 'prints returned details') - t.type(parsed.time, 'number', 'returns time as a number') - }, + const registry = new MockRegistry({ + tap: t, + registry: npm.config.get('registry'), + }) + registry.ping({ body: '{not: real"json]' }) + await npm.exec('ping', []) + t.match(logs.notice, [['PING', 'https://registry.npmjs.org/'], ['PONG', /[0-9]+ms/]]) + t.match(JSON.parse(joinedOutput()), { + registry: npm.config.get('registry'), + time: /[0-9]+/, + details: {}, }) - const ping = new Ping(npm) - - await ping.exec([]) - t.equal(noticeCalls, 2, 'should have logged 2 lines') }) diff --git a/test/lib/commands/whoami.js b/test/lib/commands/whoami.js index f483bd46d5c4b..ad7c223888df4 100644 --- a/test/lib/commands/whoami.js +++ b/test/lib/commands/whoami.js @@ -5,7 +5,7 @@ const MockRegistry = require('../../fixtures/mock-registry.js') const username = 'foo' const auth = { '//registry.npmjs.org/:_authToken': 'test-auth-token' } -t.test('npm whoami', async (t) => { +t.test('npm whoami', async t => { const { npm, joinedOutput } = await loadMockNpm(t, { config: auth }) const registry = new MockRegistry({ tap: t, @@ -17,7 +17,7 @@ t.test('npm whoami', async (t) => { t.equal(joinedOutput(), username, 'should print username') }) -t.test('npm whoami --json', async (t) => { +t.test('npm whoami --json', async t => { const { npm, joinedOutput } = await loadMockNpm(t, { config: { json: true, @@ -33,3 +33,23 @@ t.test('npm whoami --json', async (t) => { await npm.exec('whoami', []) t.equal(JSON.parse(joinedOutput()), username, 'should print username') }) + +t.test('credentials from token', async t => { + const { npm, joinedOutput } = await loadMockNpm(t, { + config: { + '//registry.npmjs.org/:username': username, + '//registry.npmjs.org/:_password': 'hunter2', + }, + }) + await npm.exec('whoami', []) + t.equal(joinedOutput(), username, 'should print username') +}) + +t.test('not logged in', async t => { + const { npm } = await loadMockNpm(t, { + config: { + json: true, + }, + }) + await t.rejects(npm.exec('whoami', []), { code: 'ENEEDAUTH' }) +}) diff --git a/test/lib/npm.js b/test/lib/npm.js index 566cbca20f6bf..cd692a93f5077 100644 --- a/test/lib/npm.js +++ b/test/lib/npm.js @@ -654,3 +654,61 @@ t.test('implicit workspace accept', async t => { }) await t.rejects(mock.npm.exec('org', []), /.*Usage/) }) + +t.test('usage', async t => { + const { npm } = await loadMockNpm(t) + t.afterEach(() => { + npm.config.set('viewer', null) + npm.config.set('long', false) + npm.config.set('userconfig', '/some/config/file/.npmrc') + }) + const { dirname } = require('path') + const basedir = dirname(dirname(__dirname)) + t.cleanSnapshot = str => str.split(basedir).join('{BASEDIR}') + .split(require('../../package.json').version).join('{VERSION}') + + npm.config.set('viewer', null) + npm.config.set('long', false) + npm.config.set('userconfig', '/some/config/file/.npmrc') + + t.test('basic usage', async t => { + t.matchSnapshot(await npm.usage) + t.end() + }) + + t.test('with browser', async t => { + npm.config.set('viewer', 'browser') + t.matchSnapshot(await npm.usage) + t.end() + }) + + t.test('with long', async t => { + npm.config.set('long', true) + t.matchSnapshot(await npm.usage) + t.end() + }) + + t.test('set process.stdout.columns', async t => { + const { columns } = process.stdout + t.teardown(() => { + Object.defineProperty(process.stdout, 'columns', { + value: columns, + enumerable: true, + configurable: true, + writable: true, + }) + }) + const cases = [0, 90] + for (const cols of cases) { + t.test(`columns=${cols}`, async t => { + Object.defineProperty(process.stdout, 'columns', { + value: cols, + enumerable: true, + configurable: true, + writable: true, + }) + t.matchSnapshot(await npm.usage) + }) + } + }) +}) diff --git a/test/lib/utils/get-identity.js b/test/lib/utils/get-identity.js deleted file mode 100644 index 5e6de9ca9cfee..0000000000000 --- a/test/lib/utils/get-identity.js +++ /dev/null @@ -1,103 +0,0 @@ -const t = require('tap') - -t.test('throws ENOREGISTRY when no registry option is provided', async (t) => { - t.plan(2) - const getIdentity = t.mock('../../../lib/utils/get-identity.js') - - try { - await getIdentity({}) - } catch (err) { - t.equal(err.code, 'ENOREGISTRY', 'assigns the appropriate error code') - t.equal(err.message, 'No registry specified.', 'returns the correct error message') - } -}) - -t.test('returns username from uri when provided', async (t) => { - t.plan(1) - - const getIdentity = t.mock('../../../lib/utils/get-identity.js') - const npm = { - config: { - getCredentialsByURI: () => { - return { username: 'foo' } - }, - }, - } - - const identity = await getIdentity(npm, { registry: 'https://registry.npmjs.org' }) - t.equal(identity, 'foo', 'returns username from uri') -}) - -t.test('calls registry whoami when token is provided', async (t) => { - t.plan(3) - - const options = { - registry: 'https://registry.npmjs.org', - token: 'thisisnotreallyatoken', - } - - const getIdentity = t.mock('../../../lib/utils/get-identity.js', { - 'npm-registry-fetch': { - json: (path, opts) => { - t.equal(path, '/-/whoami', 'calls whoami') - t.same(opts, options, 'passes through provided options') - return { username: 'foo' } - }, - }, - }) - const npm = { - config: { - getCredentialsByURI: () => options, - }, - } - - const identity = await getIdentity(npm, options) - t.equal(identity, 'foo', 'fetched username from registry') -}) - -t.test('throws ENEEDAUTH when response does not include a username', async (t) => { - t.plan(3) - - const options = { - registry: 'https://registry.npmjs.org', - token: 'thisisnotreallyatoken', - } - - const getIdentity = t.mock('../../../lib/utils/get-identity.js', { - 'npm-registry-fetch': { - json: (path, opts) => { - t.equal(path, '/-/whoami', 'calls whoami') - t.same(opts, options, 'passes through provided options') - return {} - }, - }, - }) - const npm = { - config: { - getCredentialsByURI: () => options, - }, - } - - try { - await getIdentity(npm, options) - } catch (err) { - t.equal(err.code, 'ENEEDAUTH', 'throws correct error code') - } -}) - -t.test('throws ENEEDAUTH when neither username nor token is configured', async (t) => { - t.plan(1) - const getIdentity = t.mock('../../../lib/utils/get-identity.js', { - }) - const npm = { - config: { - getCredentialsByURI: () => ({}), - }, - } - - try { - await getIdentity(npm, { registry: 'https://registry.npmjs.org' }) - } catch (err) { - t.equal(err.code, 'ENEEDAUTH', 'throws correct error code') - } -}) diff --git a/test/lib/utils/is-windows.js b/test/lib/utils/is-windows.js deleted file mode 100644 index a1d520f0629f6..0000000000000 --- a/test/lib/utils/is-windows.js +++ /dev/null @@ -1,39 +0,0 @@ -const t = require('tap') - -const mockGlobals = require('../../fixtures/mock-globals') - -t.test('is not windows', async t => { - mockGlobals(t, { 'process.platform': 'posix' }) - t.match({ - isWindows: false, - isWindowsShell: false, - }, t.mock('../../../lib/utils/is-windows.js')) -}) - -t.test('is windows, shell', async t => { - mockGlobals(t, { - 'process.platform': 'win32', - 'process.env': { - MSYSTEM: 'notmingw', - TERM: 'notcygwin', - }, - }) - t.match({ - isWindows: true, - isWindowsShell: true, - }, t.mock('../../../lib/utils/is-windows.js')) -}) - -t.test('is windows, not shell', async t => { - mockGlobals(t, { - 'process.platform': 'win32', - 'process.env': { - MSYSTEM: 'MINGW32', - TERM: 'cygwin', - }, - }) - t.match({ - isWindows: true, - isWindowsShell: false, - }, t.mock('../../../lib/utils/is-windows.js')) -}) diff --git a/test/lib/utils/npm-usage.js b/test/lib/utils/npm-usage.js deleted file mode 100644 index 035d4bbb21ef7..0000000000000 --- a/test/lib/utils/npm-usage.js +++ /dev/null @@ -1,60 +0,0 @@ -const t = require('tap') -const { load: loadMockNpm } = require('../../fixtures/mock-npm.js') - -t.test('usage', async t => { - const { npm } = await loadMockNpm(t) - t.afterEach(() => { - npm.config.set('viewer', null) - npm.config.set('long', false) - npm.config.set('userconfig', '/some/config/file/.npmrc') - }) - const { dirname } = require('path') - const basedir = dirname(dirname(dirname(__dirname))) - t.cleanSnapshot = str => str.split(basedir).join('{BASEDIR}') - .split(require('../../../package.json').version).join('{VERSION}') - - npm.config.set('viewer', null) - npm.config.set('long', false) - npm.config.set('userconfig', '/some/config/file/.npmrc') - - t.test('basic usage', async t => { - t.matchSnapshot(await npm.usage) - t.end() - }) - - t.test('with browser', async t => { - npm.config.set('viewer', 'browser') - t.matchSnapshot(await npm.usage) - t.end() - }) - - t.test('with long', async t => { - npm.config.set('long', true) - t.matchSnapshot(await npm.usage) - t.end() - }) - - t.test('set process.stdout.columns', async t => { - const { columns } = process.stdout - t.teardown(() => { - Object.defineProperty(process.stdout, 'columns', { - value: columns, - enumerable: true, - configurable: true, - writable: true, - }) - }) - const cases = [0, 90] - for (const cols of cases) { - t.test(`columns=${cols}`, async t => { - Object.defineProperty(process.stdout, 'columns', { - value: cols, - enumerable: true, - configurable: true, - writable: true, - }) - t.matchSnapshot(await npm.usage) - }) - } - }) -}) diff --git a/test/lib/utils/ping.js b/test/lib/utils/ping.js deleted file mode 100644 index 1bebfa69d2b78..0000000000000 --- a/test/lib/utils/ping.js +++ /dev/null @@ -1,35 +0,0 @@ -const t = require('tap') - -t.test('pings', async (t) => { - t.plan(3) - - const options = { fake: 'options' } - const response = { some: 'details' } - const ping = t.mock('../../../lib/utils/ping.js', { - 'npm-registry-fetch': (url, opts) => { - t.equal(url, '/-/ping?write=true', 'calls the correct url') - t.equal(opts, options, 'passes through options') - return { json: () => Promise.resolve(response) } - }, - }) - - const res = await ping(options) - t.match(res, response, 'returns json response') -}) - -t.test('catches errors and returns empty json', async (t) => { - t.plan(3) - - const options = { fake: 'options' } - const response = { some: 'details' } - const ping = t.mock('../../../lib/utils/ping.js', { - 'npm-registry-fetch': (url, opts) => { - t.equal(url, '/-/ping?write=true', 'calls the correct url') - t.equal(opts, options, 'passes through options') - return { json: () => Promise.reject(response) } - }, - }) - - const res = await ping(options) - t.match(res, {}, 'returns empty json response') -})