Skip to content
This repository has been archived by the owner on Dec 18, 2020. It is now read-only.

Support multiple values for githubOrg #38

Closed
nexdrew opened this issue Nov 3, 2016 · 7 comments
Closed

Support multiple values for githubOrg #38

nexdrew opened this issue Nov 3, 2016 · 7 comments

Comments

@nexdrew
Copy link
Contributor

nexdrew commented Nov 3, 2016

For the benefit of npm Enterprise, it should probably accept both an array and a string value that can be parsed into multiple values.

@nanovazquez
Copy link
Contributor

nanovazquez commented Nov 8, 2016

Any news on this? I'm not aware of all the changes needed in npm enterprise to support this, but to implement it here we can simply query all the orgs until the user is authenticated in one of them, like (credits to @xaka).

authenticator.js (line 84)

  }).then(_this.githubOrg && function(token) {
    return Promise.any([].concat(_this.githubOrg).map(function (githubOrg) {
      return new Promise(function(resolve, reject) {
        github.orgs.getMember({ user: username, org: githubOrg }, function(err, res) {
          if (err) reject(err);
          else resolve(token);
        });
      });
    })).catch(function(err) {
      // handle possible AggregateError, which is a collection of promise errors
      err = err instanceof Promise.AggregateError ? err[0] : err;

      if (err.code === 404) {
        err.code = 401;
        err.message = 'unauthorized';
      } else if (err.code === 500) {
        err.message = 'GitHub enterprise unavailable';
      }

      // this is an error state
      throw err;
    });
  });
};

To support either an array and a string value, you can do the following (tests here):

var items = !Array.isArray(_this.githubOrg) ? _this.githubOrg.split(/,[\s]*/) : value;

But I'm not sure if this is the right place to do it. If so, the line should be added above the return Promise.any([].concat(....

@bcoe
Copy link
Contributor

bcoe commented Mar 15, 2017

@nanovazquez in my QA; #39 doesn't seem to quite do the trick, because at publish time it gets an invalid org error, caused by: https://github.com/npm/npme-auth-github/blob/master/authorizer.js#L83

I think it should be a pretty quick fix; if you get to it before me I can land the feature ASAP.

@nanovazquez
Copy link
Contributor

nanovazquez commented Mar 16, 2017

@bcoe: there you go #40

I'm using the same approach the authenticator.js uses: this.githubOrg could be an Array, a CSV or a single value. We simply check if the parameter received is one of the orgs defined via configuration.

@bcoe
Copy link
Contributor

bcoe commented Mar 16, 2017

@nanovazquez I just released a new version of npm Enterprise with support for multiple organizations (via your patch); please let me know if this does the trick for you.

@bcoe bcoe closed this as completed Mar 16, 2017
@nanovazquez
Copy link
Contributor

nanovazquez commented Mar 20, 2017

@bcoe: Does this version contain the fix?

image

@bcoe
Copy link
Contributor

bcoe commented Mar 21, 2017

@nanovazquez yes! let me know if you run into any issues.

@nanovazquez
Copy link
Contributor

@bcoe: Worked! Thanks for taking care of this :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants