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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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 1 commit
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 8 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 @@ -197,69 +197,31 @@ 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', () => {
test('should add authorization header with token for default registry when using npm login --scope=@foo', () => {
const testCwd = '.';
const {mockRequestManager, mockRegistries, mockReporter} = createMocks();
const npmRegistry = new NpmRegistry(testCwd, mockRegistries, mockRequestManager, mockReporter);

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),
);
});

test('isRequestToRegistry with custom host prefix', () => {
const testCwd = '.';
const {mockRequestManager, mockRegistries, mockReporter} = createMocks();
const npmRegistry = new NpmRegistry(testCwd, mockRegistries, mockRequestManager, mockReporter);
const url = 'https://npmjs.registry.org/@foo%2fyarn.tgz';

npmRegistry.config = {
'custom-host-suffix': 'some.host.org',
'//npmjs.registry.org/:_authToken': 'testScopedAuthToken',
'@foo:registry': 'https://npmjs.registry.org/',
};
npmRegistry.request(url);

expect(npmRegistry.isRequestToRegistry('http://pkgs.host.com:80/foo/bar/baz', 'http://pkgs.host.com/bar/baz')).toBe(
false,
);

npmRegistry.config = {
'custom-host-suffix': 'pkgs.host.com',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The custom-host-suffix feature is no longer necessary. If the request is being made to a registry that needs a token - the token will get sent. If the requested registry does no have a token, no token is sent.

Though, looking at this test a bit more now, I might have misunderstood. If someone is using a registry with a full path, e.g. http://pkgs.host.com/bar/baz instead of just http://pkgs.host.com/, removal of this "custom-host-suffix" feature might indeed introduce a regression.

But why not just specify the registry as http://pkgs.host.com/? You basically trust that server.

Copy link
Member

Choose a reason for hiding this comment

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

History shows @soda0289 adding it in #3987.
Do you think the original issue is still fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The custom-host-suffix feature is the only point in this PR that I'm unsure about.

This PR preserves the feature where the authorization token is only sent if it's applicable to that registry (which was one of the intentions of #3987). But I don't look at pathnames.

Fix #3987 made sure that pathname and not just host is also checked when deciding if auth header should be used. But I'm not convinced that's needed. The reasoning is briefly described here: https://github.com/yarnpkg/website/pull/504/files by @lumaxis, but I need to understand that use case better. Are you around to discuss this @lumaxis? In particular how can I setup this VSTS registry to test yarn?

I will dive deeper on this will get back here with my conclusions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @KidkArolis, thanks for the ping!
Unfortunately, I haven't really been keeping up with the custom-host-suffix feature because for unrelated reasons, we're still using NPM in the projects I'm currently working on.
Looking at one of our internal VSTS package feeds though, it appears that they "fixed" my original issue by simply adding both URLs with the slightly different paths to the snippet they have you copy to your ~/.npmrc:

//team-name.pkgs.visualstudio.com/_packaging/ProjectName/npm/registry/:_authToken=$TOKEN
//team-name.pkgs.visualstudio.com/_packaging/ProjectName/npm/:_authToken=$TOKEN

My work in #3231 was really only picking up an existing solution proposed in #2507. Overall, I think my specific problem with VSTS would still be solved if the custom-host-suffix feature were removed. I remember however that other people who were using other private package feeds were looking forward to the feature too. I don't know if they actually ended up using it though.

Does this help you at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the info!

My conclusion is then that one could still use VSTS if the ~/.npmrc looked something like:

registry=https://team-name.pkgs.visualstudio.com/
//team-name.pkgs.visualstudio.com/:_authToken=$TOKEN

or if using scopes

@foo:registry=https://team-name.pkgs.visualstudio.com/
//team-name.pkgs.visualstudio.com/:_authToken=$TOKEN

With config like that, authorization headers would be sent correctly. So it's good to know this would work for that VSTS case.

Next, I'll look into how exactly npm handles the case when registry config contains a path, I found the relevant bit of code: https://github.com/npm/npm/blob/5e426a78ca02d0044f8dd26e0c5f881217081cbd/lib/config/fetch-opts.js#L46-L67. But haven't had a chance to look mode deeply yet.

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

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,
);
expect(requestParams.headers.authorization).toBe('Bearer testScopedAuthToken');
});
});

Expand Down
43 changes: 19 additions & 24 deletions src/registries/npm-registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import envReplace from '../util/env-replace.js';
import Registry from './base-registry.js';
import {addSuffix} from '../util/misc';
import {getPosixPath, resolveWithHome} from '../util/path';
import normalizeUrl from 'normalize-url';
import {default as userHome, home} from '../util/user-home-dir';
import path from 'path';
import url from 'url';
Expand Down Expand Up @@ -99,32 +98,13 @@ 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 alwaysAuth = registry && this.getRegistryOrGlobalOption(registry, 'always-auth');

const headers = Object.assign(
{
Expand All @@ -133,10 +113,8 @@ export default class NpmRegistry extends Registry {
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)))) {
if (this.token || alwaysAuth || this.isScopedPackage(packageIdent)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The key change is basically:

We stop checking if request isToRegistry (which is a strange and slightly complicated thing to understand, of course request is being made to a registry, what else would it be made to?)

And instead we make use of this.getAuth to figure out if this specific request needs a token or not.

In fact, we could push these 3 conditions (this.token, alwaysAuth, scopedPackage) to getAuth making this method much cleaner. getAuth takes a url and figures out if authorization is required or not. Should I do that?

const authorization = this.getAuth(packageIdent);
if (authorization) {
headers.authorization = authorization;
Expand Down Expand Up @@ -271,6 +249,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 @@ -290,6 +269,11 @@ export default class NpmRegistry extends Registry {
}

const baseRegistry = this.getRegistry(packageIdent);

if (!baseRegistry) {
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.

Now that we more strictly check what registry the request is for (e.g. don't conclude a request to https://github.com is to known registry), we can conclude that non registry requests don't need auth. This is basically the replacement for the isRequestToRegistry.


const registries = [baseRegistry];

// If sending a request to the Yarn registry, we must also send it the auth token for the npm registry
Expand Down Expand Up @@ -346,4 +330,15 @@ 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(YARN_REGISTRY) === -1) {
availableRegistries.push(YARN_REGISTRY);
}
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.

}