Proxy support and node-request #159

Open
wants to merge 17 commits into
from

Conversation

Projects
None yet
8 participants

Proposed solution for #158

What I did for both Oauth and Oauth2 :

  1. replaced http / https modules by node-request (added in package.json)
  2. added support for HTTPS_PROXY and HTTP_PROXY variables
  3. added a last parameter to the constructor to provide a proxy (or null to use none)
  4. added 5 tests to check those cases :
  • if a proxy is given and env vars are set, the given proxy should be used
  • if NULL is given as a proxy and env vars are set, no proxy should be used
  • if no proxy is given and both HTTPS_PROXY and HTTP_PROXY are set, HTTPS_PROXY should be used
  • if no proxy is given and only HTTP_PROXY is set, HTTP_PROXY should be used
  • if no proxy is given and no env vars are set, no proxy should be used

In Oauth.js, I changed _createClient() method so that it gets a parsed url object as first parameter and changed tests to use it that way.

Since I added proxy as last parameter, it should remain backward compatible. I tested Oauth2 with passport-github, and Oauth with passport-twitter and that worked fine.

That's all for my proposal. Feel free to give it a try and make comments ;)

kerphi commented Nov 8, 2013

@ciaranj is node-oauth still alive ? I hope so 'cause passportjs is a widely used project and is deeply dependant on the node-oauth lib :-(

I'd like to have this http proxy feature integrated in the official source code as soon as possible...

Owner

ciaranj commented Nov 8, 2013

yes, I'm just super busy atm :(

kerphi commented Mar 31, 2014

@ciaranj are you a bit less busy ?

tarator commented Apr 1, 2014

+1 for proxy-support.

kerphi commented Apr 1, 2014

@ciaranj maybe you could delegate "pull request" acceptation to someone else (or team) ?
just an idea ...

mweibel commented Sep 3, 2014

👍, although it might be an even better solution to just allow passing an agent. This agent could then take care of proxying or not (and this would allow setting maxSockets in a non-global way).

Either this or #184/#195. Is there any possibility to get one of them merged (or any other solution which allows setting a proxy & probably an agent)?

+1
@ciaranj are you a bit less busy ?

dmyers commented Jun 18, 2015

👍

Does this just need to be resolved to merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment