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

Support authenticating proxies #1

Merged
merged 2 commits into from
Sep 13, 2017

Conversation

CharlieHess
Copy link
Contributor

@CharlieHess CharlieHess commented Sep 13, 2017

Electron's net module supports authenticating proxies out of the box, however, we need to listen to an additional event and invoke a callback, otherwise the proxied request becomes a black-holed request.

This is pretty rudimentary but we'll just check the options passed to the request for user and password fields. If those fields aren't provided we'll abort the request and bubble up an error.

Requires providing user and password options.
@CharlieHess
Copy link
Contributor Author

cc @arantes555 we're using electron-fetch in Slack and this fixes a big issue. Mind taking a look?

@arantes555
Copy link
Owner

@CharlieHess Thanks for the PR ! Glad to see electron-fetch being used ;) I have no trouble with your PR, and spent a bit of time earlier today cleaning up and updating master so the tests pass. However, I wanted to write a few tests for your feature before merging it. I've started in the https://github.com/arantes555/electron-fetch/tree/proxy-auth-test branch, if you wanna take a look. I'll merge and publish a new version as soon as I got them running :)

@CharlieHess
Copy link
Contributor Author

@arantes555 awesome, definitely admire the test coverage here. ✨

@codecov
Copy link

codecov bot commented Sep 13, 2017

Codecov Report

Merging #1 into master will decrease coverage by 0.98%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #1      +/-   ##
==========================================
- Coverage   99.74%   98.76%   -0.99%     
==========================================
  Files           6        6              
  Lines         399      405       +6     
  Branches      131      133       +2     
==========================================
+ Hits          398      400       +2     
- Misses          1        5       +4
Impacted Files Coverage Δ
src/index.js 93.97% <33.33%> (-4.73%) ⬇️
src/fetch-error.js 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05aa23a...b003046. Read the comment docs.

@tex0l
Copy link
Collaborator

tex0l commented Sep 13, 2017

Hey @CharlieHess I adjusted a few things and merged your PR in the test branch https://github.com/arantes555/electron-fetch/tree/proxy-auth-test @arantes555 created.
It turns out that writing those tests pointed out a bug: the session argument was being ignored and replaced by electron.session.fromPartition('electron-fetch'), so that the ses.setProxy didn't do much when testing authenticated and unauthenticated proxies...

It is now solved, and the tests pass.

@arantes555 could you review my commits and publish if it's ok for you ?

@arantes555 arantes555 merged commit b003046 into arantes555:master Sep 13, 2017
@arantes555
Copy link
Owner

Did a tiny bit of cleanup and merged :) publication coming in a few minutes

@CharlieHess CharlieHess deleted the electron-proxy-support branch September 13, 2017 22:38
@arantes555
Copy link
Owner

@CharlieHess v1.1.0 is published :) Thanks ! I'd love to hear more about how you're using it at Slack ;)

@CharlieHess
Copy link
Contributor Author

@arantes555 thanks for the quick turnaround!

Newer versions of Slack (from 3.0 on) will unload some "tabs" / teams based on usage. Those unloaded tabs will still make API calls to Slack from the Node.js process, so we can show notifications / badges. And all those API calls from the main process will use electron-fetch. 😁

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