Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix scoped pkg auth when using npm private pkgs via npm login --scope #5162

Closed
wants to merge 4 commits into from
Closed

Fix scoped pkg auth when using npm private pkgs via npm login --scope #5162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a05fc91
Fix scoped pkg auth when using npm private pkgs via npm login --scope
KidkArolis Jan 6, 2018
93dd2ab
Add some more tests and remove unnecessary code
KidkArolis Jan 8, 2018
9dcbc8f
Refactor authorization flow
KidkArolis Jan 8, 2018
ea3fba0
Readd the custom-host-suffix feature
KidkArolis Jan 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 83 additions & 46 deletions __tests__/registries/npm-registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,22 @@ function createMocks(): Object {
}

describe('request', () => {
// a helper function for creating an instance of npm registry,
// making requests and inspecting request parameters
function createRegistry(config: Object): Object {
const testCwd = '.';
const {mockRequestManager, mockRegistries, mockReporter} = createMocks();
const npmRegistry = new NpmRegistry(testCwd, mockRegistries, mockRequestManager, mockReporter);
npmRegistry.config = config;
return {
request(url: string): Object {
npmRegistry.request(url);
const requestParams = mockRequestManager.request.mock.calls[0][0];
return requestParams;
},
};
}

test('should call requestManager.request with url', () => {
const testCwd = '.';
const {mockRequestManager, mockRegistries, mockReporter} = createMocks();
Expand Down Expand Up @@ -197,69 +213,90 @@ describe('request', () => {

npmRegistry.config = {
'//some.other.registry/:_authToken': 'testScopedAuthToken',
'@testScope:registry': '//some.other.registry/',
'@testScope:registry': 'https://some.other.registry/',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, npm login populates the .npmrc file with the full url, e.g.

@scope:registry=https://some.other.registry/

and not

@scope:registry=//some.other.registry/

I made sure this is not a regression by editing away the https:// in my ~/.npmrc and running npm install which gives me npm ERR! Only absolute URLs are supported

};
npmRegistry.request(url);

const requestParams = mockRequestManager.request.mock.calls[0][0];

expect(requestParams.headers.authorization).toBe('Bearer testScopedAuthToken');
});
});

describe('isRequestToRegistry functional test', () => {
test('request to registry url matching', () => {
const testCwd = '.';
const {mockRequestManager, mockRegistries, mockReporter} = createMocks();
const npmRegistry = new NpmRegistry(testCwd, mockRegistries, mockRequestManager, mockReporter);
test('should add authorization header with token for default registry when using npm login --scope=@foo', () => {
const url = 'https://npmjs.registry.org/@foo%2fyarn.tgz';
const config = {
'//npmjs.registry.org/:_authToken': 'testScopedAuthToken',
'@foo:registry': 'https://npmjs.registry.org/',
};

const validRegistryUrls = [
['http://foo.bar:80/foo/bar/baz', 'http://foo.bar/foo/'],
['http://foo.bar:80/foo/bar/baz', 'https://foo.bar/foo/'],
['http://foo.bar/foo/bar/baz', 'http://foo.bar/foo/'],
['http://foo.bar/foo/00000000-1111-4444-8888-000000000000/baz', 'http://foo.bar/foo/'],
['https://foo.bar:443/foo/bar/baz', 'https://foo.bar/foo/'],
['http://foo.bar/foo/bar/baz', 'https://foo.bar:443/foo/'],
['https://foo.bar/foo/bar/baz', 'https://foo.bar:443/foo/'],
['HTTP://xn--xample-hva.com:80/foo/bar/baz', 'http://êxample.com/foo/bar/baz'],
];

const invalidRegistryUrls = [
['https://wrong.thing/foo/bar/baz', 'https://foo.bar/foo/'],
['https://foo.bar:1337/foo/bar/baz', 'https://foo.bar/foo/'],
];

validRegistryUrls.forEach(([requestUrl, registryUrl]) =>
expect(npmRegistry.isRequestToRegistry(requestUrl, registryUrl)).toBe(true),
);
invalidRegistryUrls.forEach(([requestUrl, registryUrl]) =>
expect(npmRegistry.isRequestToRegistry(requestUrl, registryUrl)).toBe(false),
);
const requestParams = createRegistry(config).request(url);
expect(requestParams.headers.authorization).toBe('Bearer testScopedAuthToken');
});

test('isRequestToRegistry with custom host prefix', () => {
const testCwd = '.';
const {mockRequestManager, mockRegistries, mockReporter} = createMocks();
const npmRegistry = new NpmRegistry(testCwd, mockRegistries, mockRequestManager, mockReporter);
test('should add authorization header with token for yarn registry as default with a scoped package', () => {
const url = 'https://registry.yarnpkg.com/@testScope%2fyarn.tgz';
const config = {
'//registry.yarnpkg.com/:_authToken': 'testScopedAuthToken',
registry: 'https://registry.yarnpkg.com',
};

npmRegistry.config = {
'custom-host-suffix': 'some.host.org',
const requestParams = createRegistry(config).request(url);
expect(requestParams.headers.authorization).toBe('Bearer testScopedAuthToken');
});

test('should add authorization header with token for per scope yarn registry with a scoped package', () => {
const url = 'https://registry.yarnpkg.com/@testScope%2fyarn.tgz';
const config = {
'//registry.yarnpkg.com/:_authToken': 'testScopedAuthToken',
'@testScope:registry': 'https://registry.yarnpkg.com',
};
const requestParams = createRegistry(config).request(url);
expect(requestParams.headers.authorization).toBe('Bearer testScopedAuthToken');
});

expect(npmRegistry.isRequestToRegistry('http://pkgs.host.com:80/foo/bar/baz', 'http://pkgs.host.com/bar/baz')).toBe(
false,
);
test('should not add authorization header if default registry is yarn and npm token exists', () => {
const url = 'https://registry.yarnpkg.com/@testScope%2fyarn.tgz';
const config = {
'//registry.npmjs.com/:_authToken': 'testScopedAuthToken',
registry: 'https://registry.yarnpkg.com/',
};

npmRegistry.config = {
'custom-host-suffix': 'pkgs.host.com',
const requestParams = createRegistry(config).request(url);
expect(requestParams.headers.authorization).toBeUndefined();
});

test('should not add authorization header if request pathname does not match registry pathname', () => {
const url = 'https://custom.registry.com/tarball/path/@testScope%2fyarn.tgz';
const config = {
'//custom.registry.com/meta/path/:_authToken': 'testScopedAuthToken',
'@testScope:registry': 'https://custom.registry.com/meta/path/',
};

expect(npmRegistry.isRequestToRegistry('http://pkgs.host.com:80/foo/bar/baz', 'http://pkgs.host.com/bar/baz')).toBe(
true,
);
expect(npmRegistry.isRequestToRegistry('http://pkgs.host.com:80/foo/bar/baz', '//pkgs.host.com/bar/baz')).toBe(
true,
);
const requestParams = createRegistry(config).request(url);
expect(requestParams.headers.authorization).toBeUndefined();
});

test('should add authorization header if request pathname matches registry pathname', () => {
const url = 'https://custom.registry.com/custom/path/@testScope%2fyarn.tgz';
const config = {
'//custom.registry.com/custom/path/:_authToken': 'testScopedAuthToken',
'@testScope:registry': 'https://custom.registry.com/custom/path/',
};

const requestParams = createRegistry(config).request(url);
expect(requestParams.headers.authorization).toBe('Bearer testScopedAuthToken');
});

test('should add authorization header if pathname does not match but custom-host-suffix is used', () => {
const url = 'https://some.other.registry/tarball/path/@testScope%2fyarn.tgz';
const config = {
'//some.other.registry/some/path/:_authToken': 'testScopedAuthToken',
'@testScope:registry': 'https://some.other.registry/some/path/',
'custom-host-suffix': 'some.other.registry',
};

const requestParams = createRegistry(config).request(url);
expect(requestParams.headers.authorization).toBe('Bearer testScopedAuthToken');
});
});

Expand Down
74 changes: 41 additions & 33 deletions src/registries/npm-registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,48 +99,21 @@ export default class NpmRegistry extends Registry {
}
}

isRequestToRegistry(requestUrl: string, registryUrl: string): boolean {
const normalizedRequestUrl = normalizeUrl(requestUrl);
const normalizedRegistryUrl = normalizeUrl(registryUrl);
const requestParsed = url.parse(normalizedRequestUrl);
const registryParsed = url.parse(normalizedRegistryUrl);
const requestHost = requestParsed.host || '';
const registryHost = registryParsed.host || '';
const requestPath = requestParsed.path || '';
const registryPath = registryParsed.path || '';
const customHostSuffix = this.getRegistryOrGlobalOption(registryUrl, 'custom-host-suffix');

return (
requestHost === registryHost &&
(requestPath.startsWith(registryPath) ||
// For some registries, the package path does not prefix with the registry path
(typeof customHostSuffix === 'string' && requestHost.endsWith(customHostSuffix)))
);
}

request(pathname: string, opts?: RegistryRequestOptions = {}, packageName: ?string): Promise<*> {
// packageName needs to be escaped when if it is passed
const packageIdent = (packageName && NpmRegistry.escapeName(packageName)) || pathname;
const registry = this.getRegistry(packageIdent);
const requestUrl = this.getRequestUrl(registry, pathname);

const alwaysAuth = this.getRegistryOrGlobalOption(registry, 'always-auth');

const headers = Object.assign(
{
Accept: 'application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*',
},
opts.headers,
);

const isToRegistry = this.isRequestToRegistry(requestUrl, registry);

// this.token must be checked to account for publish requests on non-scopped packages
if (this.token || (isToRegistry && (alwaysAuth || this.isScopedPackage(packageIdent)))) {
const authorization = this.getAuth(packageIdent);
if (authorization) {
headers.authorization = authorization;
}
const authorization = this.getAuth(packageIdent);
if (authorization) {
headers.authorization = authorization;
}

return this.requestManager.request({
Expand Down Expand Up @@ -271,6 +244,7 @@ export default class NpmRegistry extends Registry {
if (registry) {
return String(registry);
}
return '';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to make sure that if URL is with a protocol, e.g. https://github.com, we don't "assume" this is registry.npmjs.org. This change is making getRegistry stricter. Since this method is used only for authentication and scope resolving purposes, we basically make sure to be strict and only return registry.npmjs.org if request is really to that registry.

}

for (const scope of [this.getScope(packageIdent), '']) {
Expand All @@ -285,15 +259,41 @@ export default class NpmRegistry extends Registry {
}

getAuth(packageIdent: string): string {
// this.token must be checked to account for publish requests on non-scopped packages
if (this.token) {
return this.token;
}

const baseRegistry = this.getRegistry(packageIdent);
const registries = [baseRegistry];
let registry = this.getRegistry(packageIdent);

// Support for an existing feature: custom-host-suffix
// When custom-host-suffix global option is set, and request url matches
// this custom-host-suffix, we extract scope from the URL. We then use
// this scope to determine which auth token to use for that scope
if (packageIdent.match(REGEX_REGISTRY_PREFIX)) {
const customHostSuffix = this.getRegistryOrGlobalOption(registry || DEFAULT_REGISTRY, 'custom-host-suffix');
if (typeof customHostSuffix === 'string') {
const requestHost = url.parse(normalizeUrl(packageIdent)).host || '';
if (requestHost.endsWith(customHostSuffix)) {
const scope = this.getScope(packageIdent);
registry = this.getRegistry(scope + '/pkg');
}
}
}

if (!registry) {
return '';
}

const alwaysAuth = this.getRegistryOrGlobalOption(registry, 'always-auth');
if (!alwaysAuth && !this.isScopedPackage(packageIdent)) {
return '';
}

const registries = [registry];

// If sending a request to the Yarn registry, we must also send it the auth token for the npm registry
if (baseRegistry === YARN_REGISTRY) {
if (registry === YARN_REGISTRY) {
registries.push(DEFAULT_REGISTRY);
}

Expand Down Expand Up @@ -346,4 +346,12 @@ export default class NpmRegistry extends Registry {
getRegistryOrGlobalOption(registry: string, option: string): mixed {
return this.getRegistryOption(registry, option) || this.getOption(option);
}

getAvailableRegistries(): Array<string> {
const availableRegistries = super.getAvailableRegistries();
if (availableRegistries.indexOf(DEFAULT_REGISTRY) === -1) {
availableRegistries.push(DEFAULT_REGISTRY);
}
return availableRegistries;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DEFAULT_REGISTRY and YARN_REGISTRY are both available registries even if not mentioned in any of the config. Make sure this is reflected as we now rely on this when extracting registry from a request url.

In theory only DEFAULT_REGISTRY should suffice, but I wanted to make sure that if someone sets their default registry for some reason to be the yarn registry, things should work the same.

}