Fixes npm auth by arcanis · Pull Request #3774 · yarnpkg/yarn

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

-    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

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

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

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

-    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

-    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

+      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

Conversation

arcanis commented Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bestander commented Jun 30, 2017

Uh oh!

bestander commented Jun 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bestander left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BYK commented Jul 5, 2017

Uh oh!

BYK left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arcanis Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arcanis commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BYK commented Jul 6, 2017

Uh oh!

bestander commented Jul 6, 2017

Uh oh!

bestander left a comment

Choose a reason for hiding this comment

Uh oh!

BYK commented Jul 7, 2017

Uh oh!

daniel-nagy commented Jul 13, 2017

Uh oh!

bestander commented Jul 13, 2017

Uh oh!

BYK commented Jul 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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