Supporting github enterprise

[mrchief]

Of all the github api libs, I find this one to be the most well written. I've been using it for working with gihub enterprise and it works fine for most gets.

However, it fails on POST/PUT where:

• SSL errors crop up due to enterprise being signed by an internal certificate signing authority
• no ability to pass extra headers

Above 2 can be solved if there was a way to pass on additional headers for the request. Currently I'm using request-promise and passing the CA chain as well as custom headers when POST/PUT are needed but this is sub optimal solution.

I'm using this in a Node (v4.4.3 and above) app.

[clayreimann]

Can you provide more input about what you'd like to see added to support your use case?

Adding extra headers should be easy enough (we can add an options param to the GitHub constructor that will take any extra headers you need to send to your Enterprise instance), but I'm not familiar with configuring an XHR (wrapper) to accept unsigned certs. (though you could use this snippet: process.env.NODE_TLS_REJECT_UNAUTHORIZED = 0 to disable your SSL problems)

[mrchief]

Adding extra headers should be easy enough

tl;dr

Something like this would do the trick:

option 1

const options = { 
  headers: { 
    ca: 'abc1234', 
    accept: 'application/vnd.github.raw' 
  },
  json: true    // parse response body as JSON (I think you already do this but this is just to show the possibility of having a way to accept other flags in future)
};
const gh = new GitHub(authopt, apiBaseopt, options);

option 2

const  headers = { 
  ca: 'abc1234', 
  accept: 'application/vnd.github.raw' 
};
const gh = new GitHub(authopt, apiBaseopt, headers);

Detailed

Sounds awesome! I thought so but I wasn't sure of the design principals (since opening up the headers could lead to weird situations). So for my needs, I'm adding:

• a header called ca: '<base64 encoded CA chain>' - since we've custom signed certificates - from request docs
• sometimes (not on all requests) override accept: 'application/vnd.github.raw' header to get the raw response.

The second one is helpful in case of getContents call when I'm trying to get the JSON data for a file. Passing the custom accept header returns raw JSON data instead of the standard github response which include the file contents as base64 encoded string. Getting the raw JSON data then enables me to skip decoding/JSON parsing the contents (since I'm using request which parses the JSON for me automatically).

I did check the raw option but based on source code and GitHub API documentation, I'm not sure if it does the right thing (right thing in this case being able to get a JSON object back without having to do any parsing myself).

Besides my custom needs, I do think the ability to have access to request object opens up all sort of possibilities. Whilst I don't have any other needs at the moment, I can see people needing access to other stuff (like specifying proxy or custom headers for tracking and so on). Users can shoot themselves in the foot but this could be one the advanced options which they may have to use at their own peril.

Hope this makes sense...

[mrchief]

I did check the raw option but based on source code and GitHub API documentation, I'm not sure if it does the right thing (right thing in this case being able to get a JSON object back without having to do any parsing myself).

I think I got a bit overthrown by line149. raw does seem to pass the correct request headers.

So apart from passing the right request headers, the other thing that remains is parsing the response. text is a safe assumption since you don't know what one could be asking. However, I think it'd be nice if one could get parsed JSON (based on json: true flag somehow??).

I think I can live with text if handling raw + json: true introduces too much kludge.

[mrchief]

So apart from passing the right request headers, the other thing that remains is parsing the response.

It seems that even with raw, the JSON data is parsed and accessible at response.data.

However, I found this. Seems like a dead end to me. Even if you did add support for custom headers, you're ultimately limited by axios.

I suppose asking to switch to request-promise would be too big of a change? Just thinking out loud...

Anyway, I'll step aside now and let you come up with the best solution. Thanks for listening.

[mrchief]

By the way, if you consider switching to request-promise, here's what I had to do:

// in _request (requestable.js)

    const config = {
      uri: url,
      method: method,
      headers: headers,
      qs: queryParams,         // params becomes qs
      body: data,                   // data becomes body
      ...this.__options.requestOptions       // this is my hack of passing additional options to 'request'
    };

// instead of response.config.xxx, you access err.options.xxx
function callbackErrorOrThrow(cb, path) {
  return function handler(err, response, body) {
    logger.error(`error making request ${err.options.method} ${err.options.uri} ${response ? JSON.stringify(response):''}`);
    let error = buildError(path, response || {}, err || body);
    if (cb) {
      cb(error);
    } else {
      throw error;
    }
  };
}

And response.data.xxx becomes response.xxx in places where we access response.data.

I've tested Repository functions and they seem fine. I can send a PR if you wish.

[clayreimann]

@mrchief I'm don't think switching to request-promise is the way we want to go (since we need to support the browser too) but I hope to soon fold in #219 which gives you the different media types. So that plus the headers should get you what you need, I think.

• a header called ca: '<base64 encoded CA chain>' - since we've custom signed certificates

Is this a standard HTTP feature, a node feature, something custom to request-promise? I can't seem to find mention of doing something like this anywhere.

[mrchief]

Is this a standard HTTP feature, a node feature, something custom to request-promise? I can't seem to find mention of doing something like this anywhere.

Passing ca is part of Node's https.request options. request-promise (which is just a bluebird wrapper on request) passes it to request which passes it to Node.

I'm not sure if there is an equivalent or de-facto standard for browsers.

I totally forgot about the fact that you support browsers as well. :)

Yeah, #219 and headers would give me what I need.

[divergentdave]

Based on my reading of that doc page, I don't think you need to pass "ca" as a header, instead https.request() expects "ca" to be a property in the options object passed as the first argument.

If you're OK with trusting this CA globally throughout your program, you could do the following. (see axios/axios#284)

https.globalAgent.options.ca = fs.readFileSync('ca.pem');