Skip to content

Commit

Permalink
Fix request authentication for scoped private registry tarballs (#1666)
Browse files Browse the repository at this point in the history
* Fix request authentication for scoped private registry tarballs

Yarn correctly sends authorization headers when resolving metadata, but
when it's time to download the tarballs, it calls registry.request
with a full tarball url. This causes the getRegistry() function to
incorrectly return the DEFAULT_REGISTRY url which means the authorization
headers are no longer sent. The response in this case is a JSON error
object instead of a tarball file. Untaring fails with "invalid tar file"

* Don't assume anything about the registry URL, read registries from config

* Add a test for installing private packages requiring authentication

* Change the authed package testing strategy to be more local

* Include non scoped registries in available registries list

* Unnest the authed pkg test, use FlowFixMe for afterEach
  • Loading branch information
KidkArolis authored and bestander committed Nov 19, 2016
1 parent bacf1fb commit 99916d4
Show file tree
Hide file tree
Showing 6 changed files with 58 additions and 1 deletion.
13 changes: 13 additions & 0 deletions __tests__/__mocks__/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ const fs = require('fs');

const CACHE_DIR = path.join(__dirname, '..', 'fixtures', 'request-cache');

let authedRequests = [];

function getRequestAlias(params: Object): string {
const parts = url.parse(params.path);
const pathname = cleanAlias(parts.pathname);
Expand Down Expand Up @@ -45,6 +47,9 @@ module.exports = function(params: Object): Request {

module.exports.Request = Request;

module.exports.__resetAuthedRequests = (): void => { authedRequests = []; };
module.exports.__getAuthedRequests = (): Array<Object> => authedRequests;

const httpMock = {
request(options: Object, callback?: ?Function): ClientRequest {
const alias = getRequestAlias(options);
Expand All @@ -55,6 +60,14 @@ const httpMock = {
// TODO better way to do this
const httpModule = options.uri.href.startsWith('https:') ? https : http;

// expose authorized requests to the tests for assertion
if (options.headers.authorization) {
authedRequests.push({
url: options.uri.href,
headers: options.headers,
});
}

if (allowCache && fs.existsSync(loc)) {
// cached
options.agent = null;
Expand Down
19 changes: 19 additions & 0 deletions __tests__/commands/install.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,14 @@ import os from 'os';
jasmine.DEFAULT_TIMEOUT_INTERVAL = 90000;

const path = require('path');
const request = require('request');

const fixturesLoc = path.join(__dirname, '..', 'fixtures', 'install');

beforeEach(request.__resetAuthedRequests);
// $FlowFixMe
afterEach(request.__resetAuthedRequests);

test.concurrent('properly find and save build artifacts', async () => {
await runInstall({}, 'artifacts-finds-and-saves', async (config): Promise<void> => {
const cacheFolder = path.join(config.cacheFolder, 'npm-dummy-0.0.0');
Expand Down Expand Up @@ -775,3 +780,17 @@ test.concurrent('install uses OS line endings when lockfile doesn\'t exist', asy
assert(lockfile.indexOf(os.EOL) >= 0);
});
});

test('install a scoped module from authed private registry', (): Promise<void> => {
return runInstall({noLockfile: true}, 'install-from-authed-private-registry', async (config) => {
const authedRequests = request.__getAuthedRequests();
assert.equal(authedRequests[0].url, 'https://registry.yarnpkg.com/@types%2flodash');
assert.equal(authedRequests[0].headers.authorization, 'Bearer abc123');
assert.equal(authedRequests[1].url, 'https://registry.yarnpkg.com/@types/lodash/-/lodash-4.14.37.tgz');
assert.equal(authedRequests[1].headers.authorization, 'Bearer abc123');
assert.equal(
(await fs.readFile(path.join(config.cwd, 'node_modules', '@types', 'lodash', 'index.d.ts'))).split('\n')[0],
'// Type definitions for Lo-Dash 4.14',
);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
//registry.yarnpkg.com/:_authToken=abc123
@types:registry=https://registry.yarnpkg.com/
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"dependencies": {
"@types/lodash": "4.14.37"
}
}
10 changes: 10 additions & 0 deletions src/registries/base-registry.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,16 @@ export default class BaseRegistry {
return this.config[key];
}

getAvailableRegistries(): Array<string> {
const config = this.config;
return Object.keys(config).reduce((registries, option) => {
if (option === 'registry' || option.split(':')[1] === 'registry') {
registries.push(config[option]);
}
return registries;
}, []);
}

loadConfig(): Promise<void> {
return Promise.resolve();
}
Expand Down
10 changes: 9 additions & 1 deletion src/registries/npm-registry.js
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,15 @@ export default class NpmRegistry extends Registry {
}

getRegistry(packageName: string): string {
// Try scoped registry, and default registry
// Try extracting registry from the url, then scoped registry, and default registry
if (packageName.match(/^https?:/)) {
const availableRegistries = this.getAvailableRegistries();
const registry = availableRegistries.find((registry) => packageName.startsWith(registry));
if (registry) {
return registry;
}
}

for (const scope of [this.getScope(packageName), '']) {
const registry = this.getScopedOption(scope, 'registry')
|| this.registries.yarn.getScopedOption(scope, 'registry');
Expand Down

0 comments on commit 99916d4

Please sign in to comment.