Skip to content

Fixes npm auth #3774

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

Merged
merged 4 commits into from
Jul 7, 2017
Merged

Fixes npm auth #3774

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1f0a89d
Fixes npm auth
Jun 30, 2017
a241fb7
Fixes yarn install with private packages
arcanis Jun 30, 2017
d4dcfa7
Fixes #3831
Jul 6, 2017
52e3c8b
Uses constants
Jul 6, 2017
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
43 changes: 25 additions & 18 deletions src/registries/npm-registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,15 +76,15 @@ export default class NpmRegistry extends Registry {
const registry = this.getRegistry(packageName || pathname);
const requestUrl = url.resolve(registry, pathname);
const alwaysAuth = this.getRegistryOrGlobalOption(registry, 'always-auth');
const customHostSuffix = this.getRegistryOrGlobalOption(registry, 'custom-host-suffix');

const headers = Object.assign(
{
Accept: 'application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*',
},
opts.headers,
);
if (this.token || (alwaysAuth && isRequestToRegistry(requestUrl, registry, customHostSuffix))) {

if (alwaysAuth || (packageName || pathname)[0] === `@`) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the reason for this change

Copy link
Member

Choose a reason for hiding this comment

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

From @arcanis:

this.token is undefined
So the codepath is not executed if alwaysAuth is not set
The condition I put tries to set an auth token if alwaysAuth is enabled, or if the package is scoped

Copy link
Member

Choose a reason for hiding this comment

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

This should go into the code as a comment. Also a helper function named isScoped or needsAuth (or both?) would make the code more readable.

Copy link
Member

Choose a reason for hiding this comment

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

Also I think the this.token check here was to memoize the token. We may wanna keep that behavior.

const authorization = this.getAuth(packageName || pathname);
if (authorization) {
headers.authorization = authorization;
Expand Down Expand Up @@ -208,26 +208,33 @@ export default class NpmRegistry extends Registry {
return this.token;
}

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

// Check for bearer token.
const authToken = this.getRegistryOrGlobalOption(registry, '_authToken');
if (authToken) {
return `Bearer ${String(authToken)}`;
if (baseRegistry === `https://registry.yarnpkg.com/`) {
Copy link
Member

Choose a reason for hiding this comment

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

we have constants for these values

Copy link
Member

Choose a reason for hiding this comment

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

So if package comes from registry.yarnpkg.com then you want to add registry.npmjs.org to the list of registries in the attempt of getting username and password.

That looks like a lazy patch, it would be better to set up getRegistryOrGlobalOption to return same data for yarnpkg and npmjs rather than patch every callsite

registries.push(`https://registry.npmjs.org/`);
}

// Check for basic auth token.
const auth = this.getRegistryOrGlobalOption(registry, '_auth');
if (auth) {
return `Basic ${String(auth)}`;
}
for (const registry of registries) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this trying to get an auth token for the first matching registry? If so, I find that a bit dangerous. Should we keep a mapping of registry: token pairs and use the appropriate one when communicating. This would also leak tokens to other registries which may be an important security threat.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hence the check to make sure that the npm fallback is only added when the registry is the Yarn registry. The token won't be sent for any other hostname (wich is another issue).

That being said, the whole "multi-registry" logic is flawed, since we only support a single registry implementation (and adding more of them wouldn't make much sense, since it would complexify the codebase for little gain). I'd like to rework it so that we only support a single registry implementation, the npm one, and then make possible to configure what needs to be generic (mostly the hostname). But that's a second step.

Copy link
Member

Choose a reason for hiding this comment

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

I'm +1 to this until we do the refactor. The code can use a short comment explaining this safe-guard.

// Check for bearer token.
const authToken = this.getRegistryOrGlobalOption(registry, '_authToken');
if (authToken) {
return `Bearer ${String(authToken)}`;
}

// Check for basic username/password auth.
const username = this.getRegistryOrGlobalOption(registry, 'username');
const password = this.getRegistryOrGlobalOption(registry, '_password');
if (username && password) {
const pw = new Buffer(String(password), 'base64').toString();
return 'Basic ' + new Buffer(String(username) + ':' + pw).toString('base64');
// Check for basic auth token.
const auth = this.getRegistryOrGlobalOption(registry, '_auth');
if (auth) {
return `Basic ${String(auth)}`;
}

// Check for basic username/password auth.
const username = this.getRegistryOrGlobalOption(registry, 'username');
const password = this.getRegistryOrGlobalOption(registry, '_password');
if (username && password) {
const pw = new Buffer(String(password), 'base64').toString();
return 'Basic ' + new Buffer(String(username) + ':' + pw).toString('base64');
Copy link
Member

Choose a reason for hiding this comment

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

Wow, no idea we did double base64 for the password part.

}
}

return '';
Expand Down