Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default proxy from environment #6

Closed
wants to merge 2 commits into from
Closed

Default proxy from environment #6

wants to merge 2 commits into from

Conversation

wolfgangwalther
Copy link

I'm using grant in an environment where I need every request to go through an internal proxy server. The request for profile_url is done with request-compose, which currently does not use the http_proxy / https_proxy environment variables and therefore the OAuth2 / Open ID Connect flow fails.

This PR adds detection of those environment variables to .send(). This needs to be done just before sending (and not in defaults() or something), because the protocol for the request could change at any time before that and the correct env var has to be used.

Added some other test-cases (and a small change to response/status.js) to increase test coverage a bit.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 94

  • 6 of 6 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.2%) to 98.843%

Totals Coverage Status
Change from base Build 92: 1.2%
Covered Lines: 254
Relevant Lines: 254

💛 - Coveralls

@simov
Copy link
Owner

simov commented Jun 7, 2020

Hi @wolfgangwalther, thanks for your work on this PR.

However, it is not going to work for HTTPS. The reason why is because this module does not handle HTTP tunneling internally as request does.

The good news is that I just added support for request options in Grant v5.2.0. Here is how you can use it.

Also added the relevant examples here.


As for the test changes lets discuss this in another PR if you want. Generally speaking I don't see much value in testing how the querystring module works.

Thanks 👍

@simov simov closed this Jun 7, 2020
@wolfgangwalther
Copy link
Author

Thanks for adding those options, I'll have a look. As I am using grant through feathers-oauth, I guess I'll have to add some code to that first.

As for the tests: If you don't want to use those tests, thats fine with we. Some of those probably don't have much more value than contributing to a goal of 100% coverage - something that at least looks nice.
However, going through some of those branches that were not covered, I also made the change to response/status.js. This is in fact a change, that I think is worth doing, because it's about a sensible default in the case of a status code other than 2xx/3xx/4xx/5xx. This might be some custom code like 9xx or something? The current implementation would just do nothing in this case, where handling this the same as 2xx/3xx would make sense, in my opinion.

@wolfgangwalther wolfgangwalther deleted the proxy-from-env branch June 8, 2020 05:47
@simov
Copy link
Owner

simov commented Jun 8, 2020

Thanks @wolfgangwalther, I overlooked that part, have to think about it.

As for Feathers, I've no idea if they moved to the latest major v5 of Grant or not.

Otherwise you can add the environment variable detection before initializing Grant, and configure the tunnel agent accordingly. I tested with a local MITM Proxy, with self signed certificates.

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

Successfully merging this pull request may close these issues.

None yet

3 participants