jspm / npm

NPM Location Service
19 stars 34 forks source link

Custom scope authentication #98

Closed guybedford closed 8 years ago

guybedford commented 8 years ago

While we now support scopes which have their own registry settings, we still only check the auth once without any scope context. We should upgrade the auth check to apply for each lookup given the currently installing scope.

glen-84 commented 8 years ago

@guybedford,

With the following .npmrc file:

@x:registry=https://registry.cnpm.example.com

Installation with npm works (npm i @x/api-client-js -SE), but installation with jspm fails:

(node:3336) Warning: a promise was rejected with a non-error: [object String]
(node:3336) Warning: a promise was rejected with a non-error: [object String]

err  Error on lookup for npm:@x/api-client-js
Invalid authentication details. Run jspm registry config npm to reconfigure.
(node:3336) Warning: a promise was rejected with a non-error: [object String]
(node:3336) Warning: a promise was rejected with a non-error: [object String]

err  Error looking up npm:@x/api-client-js.

warn Installation changes not saved.

If I remove the scope from the npmrc file (which I don't really want to do), it still fails:

     Updating registry cache...
     Downloading npm:@x/api-client-js@1.0.0-alpha.0
(node:20428) Warning: a promise was rejected with a non-error: [object String]
(node:20428) Warning: a promise was rejected with a non-error: [object String]
(node:20428) Warning: a promise was rejected with a non-error: [object String]
(node:20428) Warning: a promise was rejected with a non-error: [object String]
(node:20428) Warning: a promise was rejected with a non-error: [object String]
(node:20428) Warning: a promise was created in a handler but was not returned from it
ok   Installed peer buffer to github:jspm/nodelibs-buffer@^0.2.0-alpha (0.2.0-alpha)
(node:20428) Warning: a promise was rejected with a non-error: [object String]
(node:20428) Warning: a promise was rejected with a non-error: [object String]
(node:20428) Warning: a promise was rejected with a non-error: [object String]
(node:20428) Warning: a promise was rejected with a non-error: [object String]
(node:20428) Warning: a promise was rejected with a non-error: [object String]
(node:20428) Warning: a promise was rejected with a non-error: [object String]

err  Repo npm:buffer not found!

warn Installation changes not saved.

This means that I'm unable to install this package from a private repo, which is quite a big issue.

Is this difficult to fix? (jspm 0.17.0-beta.22)

glen-84 commented 8 years ago

I was able to install my package with a temporary hack, but it would really be nice if this was implemented properly.

In npm.js, above this line:

      if (scope) {
        var npmrc = new Npmrc();
        self.auth = npmrc.getAuth(self.registryURL(scope));
      }

If I can help in some way, please let me know.

guybedford commented 8 years ago

@glen-84 a proper implementation for this would be really awesome. If you're keen on working on a PR let me know. The hack sounds about the right direction to me actually, but perhaps we can inline the implementation into getRegistryURL or have getRegistryURL return both auth and url?

glen-84 commented 8 years ago

I would have to get more familiar with the code, so if it's something that you could do in 5 minutes then it's probably not worth me trying to figure this all out. However, it is really important and something that I need to have working within the next couple of business days (maybe a week at most), so if you simply don't have the time then please let me know now and I'll see if I can make a plan.

glen-84 commented 8 years ago

@guybedford,

I don't think that it makes sense for registryURL to set or return auth data, but my understanding of the code is limited, so I could be wrong.

I made some improvements to my code, so it now looks like this:

      var authData;
      if (scope) {
        authData = self.npmrc.getAuth(self.registryURL(scope));
      } else {
        authData = self.auth;
      }

      // This is the existing code, except that `self.auth` is replaced with `authData`.
      return asp(request)(auth.injectRequestOptions({
        uri: self.registryURL(scope) + '/' + repoPath,
        gzip: true,
        strictSSL: self.strictSSL,
        headers: lookupCache ? {
          'if-none-match': lookupCache.eTag
        } : {}
      }, doAuth && authData)).then(function(res) {

The npmrc object is made available from the constructor.

Question: If there is no scope-specific auth, should it fall back to self.auth? I'm not really sure how options.authToken and options.auth fit into this.

It would then be something like this:

      var authData;
      if (scope) {
        authData = self.npmrc.getAuth(self.registryURL(scope));
      }

      if (!authData) {
        authData = self.auth;
      }

      // etc.
guybedford commented 8 years ago

@glen-84 it make make sense to have a more general getAuthAndURL style method with an object containing the URL and auth object to pass to the request.

For non-scope-specific, yes we just fallback to global auth.

options.authToken and options.auth can then be thought of as global auth to apply when this isn't detected perhaps?

glen-84 commented 8 years ago

I just submitted a PR for this.

I couldn't see the benefit of having a method like that. getXAndY() seems like a code smell, but I could be wrong. Maybe something like getRegistryInfo/Details/Config could have worked, but I've kept it simple for now. If you disagree, please LMK exactly what you require.