Fixes npm auth #3774

arcanis · 2017-06-30T11:00:06Z

This commit should fix #3765. I'm not completely satisfied with hardcoding our registry, maybe that could extended at a later point with an option to "inherit" the auth token from npm. Not sure it would be useful, tho.

bestander · 2017-06-30T18:50:32Z

We definitely should extract the npmrc/yarnrc config parsing from npm-registry and yarn-registry.
There is a lot of confusion that those classes though in inheritance relationship read the config files exclusively.

bestander · 2017-06-30T18:51:23Z

@arcanis, how did you test the PR?
I suppose e2e test will be coming within the follow-up task #3769, right?

bestander · 2017-06-30T19:08:22Z

src/registries/npm-registry.js

-    const authToken = this.getRegistryOrGlobalOption(registry, '_authToken');
-    if (authToken) {
-      return `Bearer ${String(authToken)}`;
+    if (baseRegistry === `https://registry.yarnpkg.com/`) {


we have constants for these values

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

bestander · 2017-06-30T19:10:49Z

src/registries/npm-registry.js


    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] === `@`) {


I don't understand the reason for this change

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

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.

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

bestander

I am not super excited about the fix getting into master this way, let's fix the leaky abstraction that requires us to hardcode domains here and add an automated test.

However I'd like to stabilize 0.27 so I'll cherry-pick this commit there and release a patch today.

BYK · 2017-06-30T21:16:37Z

src/registries/npm-registry.js


    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] === `@`) {


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.

BYK · 2017-06-30T21:18:47Z

src/registries/npm-registry.js


    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] === `@`) {


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

BYK · 2017-06-30T21:20:54Z

src/registries/npm-registry.js

-    if (auth) {
-      return `Basic ${String(auth)}`;
-    }
+    for (const registry of registries) {


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.

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.

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

BYK · 2017-07-05T13:45:04Z

Sup @arcanis? :)

BYK

LGTM, great work!

May be we should plan adding tests for this now? We don't have to do e2e tests for this, right?

BYK · 2017-07-06T10:53:12Z

src/registries/npm-registry.js

-    if (this.token || (alwaysAuth && isRequestToRegistry(requestUrl, registry, customHostSuffix))) {
+
+    const packageIdent = packageName || pathname;
+    const isScoppedPackage = packageIdent.match(/^@|\/@/);


What does the second part cover? (sorry, not very familiar with scoped packages except for the @user/package pattern.

In some cases packageIdent will be @user/package, and in other cases it will be https://example.org/@user/package.tgz

BYK · 2017-07-06T10:55:22Z

src/registries/npm-registry.js

+      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');


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

arcanis · 2017-07-06T11:03:17Z

The main issue with e2e test is that it would require a private package somewhere. I have my own test package on my account, but I obviously cannot use it in the Yarn testsuite 😄

Ideally, we should create a new private package on the yarnpkg organization (we might need to upgrade the plan, cc @bestander, do you know who holds the keys of this account?), create a new yarnpkg-test user, and give to this user the read-only rights to access the test package (npm access). From there we should be able to safely put the auth token for this user inside our tests (unfortunately it will still allow malicious users to publish under this user namespace, but that shouldn't make much difference since they won't have push access to any existing project).

That, or we build a stub registry server.

BYK · 2017-07-06T11:50:01Z

That, or we build a stub registry server.

This. Or even better: we just do integration tests via mocks.

bestander · 2017-07-06T18:35:32Z

I think we should have an end to end installation without stubs at least for one package.
It should take a few minutes to register @yarnpkg test org and create a package there.
The login won't be secure but what are the risks, malicious parties will be able to install that package?

bestander

Approving to unblock but add a test so that this does not drag us.

BYK · 2017-07-07T11:59:41Z

@bestander I'll merge this then since we have filed #3842

daniel-nagy · 2017-07-13T15:28:42Z

This has broken installing private modules from our company's Bitbucket server.

"private-module": "git+ssh://git@git:7999/project/private-module.git"

I rolled back to version 0.27.4 and it correctly installs the package.

bestander · 2017-07-13T15:31:50Z

@daniel-nagy please raise a new issue.
Can you provide an example URL publicly for us to test?

BYK · 2017-07-13T15:33:32Z

@daniel-nagy @bestander #3907

arcanis requested review from BYK and bestander June 30, 2017 11:00

Fixes npm auth

1f0a89d

arcanis force-pushed the auth-fix branch from 301eb07 to 1f0a89d Compare June 30, 2017 11:14

bestander reviewed Jun 30, 2017

View reviewed changes

bestander requested changes Jun 30, 2017

View reviewed changes

bestander mentioned this pull request Jun 30, 2017

[Replace #2507] Support VSTS and other custom registries with unusual feeds #3231

Merged

BYK reviewed Jun 30, 2017

View reviewed changes

Fixes yarn install with private packages

a241fb7

arcanis force-pushed the auth-fix branch from 681c67f to a241fb7 Compare June 30, 2017 23:04

arcanis mentioned this pull request Jul 5, 2017

[email protected] consistently 404s on private packages #3765

Closed

Maël Nison added 2 commits July 6, 2017 11:19

Fixes yarnpkg#3831

d4dcfa7

Uses constants

52e3c8b

BYK approved these changes Jul 6, 2017

View reviewed changes

bestander approved these changes Jul 6, 2017

View reviewed changes

BYK merged commit 5ff6922 into yarnpkg:master Jul 7, 2017

johnf mentioned this pull request Jul 8, 2017

Yarn publish does not work anymore #3831

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes npm auth #3774

Fixes npm auth #3774

arcanis commented Jun 30, 2017 •

edited

Loading

bestander commented Jun 30, 2017

bestander commented Jun 30, 2017

bestander Jun 30, 2017

bestander Jun 30, 2017

bestander Jun 30, 2017

bestander Jun 30, 2017

BYK Jun 30, 2017

BYK Jun 30, 2017

bestander left a comment

BYK Jun 30, 2017

BYK Jun 30, 2017

BYK Jun 30, 2017

arcanis Jun 30, 2017

BYK Jul 3, 2017

BYK commented Jul 5, 2017

BYK left a comment •

edited

Loading

BYK Jul 6, 2017

arcanis Jul 6, 2017 •

edited

Loading

BYK Jul 6, 2017

arcanis commented Jul 6, 2017 •

edited

Loading

BYK commented Jul 6, 2017

bestander commented Jul 6, 2017

bestander left a comment

BYK commented Jul 7, 2017

daniel-nagy commented Jul 13, 2017

bestander commented Jul 13, 2017

BYK commented Jul 13, 2017

Fixes npm auth #3774

Fixes npm auth #3774

Conversation

arcanis commented Jun 30, 2017 • edited Loading

bestander commented Jun 30, 2017

bestander commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bestander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK commented Jul 5, 2017

BYK left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis commented Jul 6, 2017 • edited Loading

BYK commented Jul 6, 2017

bestander commented Jul 6, 2017

bestander left a comment

Choose a reason for hiding this comment

BYK commented Jul 7, 2017

daniel-nagy commented Jul 13, 2017

bestander commented Jul 13, 2017

BYK commented Jul 13, 2017

arcanis commented Jun 30, 2017 •

edited

Loading

BYK left a comment •

edited

Loading

arcanis Jul 6, 2017 •

edited

Loading

arcanis commented Jul 6, 2017 •

edited

Loading