Use electron-proxy-agent #878

aenario · 2017-11-23T10:23:01Z

Tested on windows.

codecov-io · 2017-11-29T13:43:08Z

Codecov Report

Merging #878 into master will decrease coverage by 0.33%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #878      +/-   ##
==========================================
- Coverage   67.62%   67.28%   -0.34%     
==========================================
  Files          33       33              
  Lines        2094     2094              
==========================================
- Hits         1416     1409       -7     
- Misses        678      685       +7

Impacted Files	Coverage Δ
cli/src/metadata.js	`82.85% <0%> (-7.15%)`	⬇️
cli/src/ignore.js	`97.67% <0%> (-2.33%)`	⬇️
cli/src/path_restrictions.js	`94.91% <0%> (-1.7%)`	⬇️

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 9f469b4...7a3d980. Read the comment docs.

sebn

👍 Kudos for making this work so fast!

sebn · 2017-11-30T12:08:52Z

gui/src/main/proxy.js

+                                  'Dynamically sets whether to always send credentials for HTTP NTLM or Negotiate authentication.')
+  .describe('login-by-realm', 'comma-separated list of realm:user:password')
+  .help('help')
+  .parse()


Maybe move to main later?

sebn · 2017-11-30T12:15:34Z

gui/src/main/proxy.js

+
+log.warn({config}, 'argv')
+
+const printCertificate = (certif) => `Certificate(${certif.issuerName} ${certif.subjectName})`


Doesn't really print anything.. format*? stringify?

sebn · 2017-11-30T12:15:51Z

gui/src/main/proxy.js

+  .help('help')
+  .parse()
+
+log.warn({config}, 'argv')


log.debug?

(not repeating, there are a few others below)

sebn · 2017-11-30T12:17:41Z

gui/src/main/proxy.js

+const https = require('https')
+
+const log = require('cozy-desktop').default.logger({
+  component: 'GUI:proxy'


Makes me think that CLI won't have proxy support. Not a priority though. We'll just need to make it clear in case we bring it back.

sebn · 2017-11-30T12:20:03Z

gui/src/main/proxy.js

+  component: 'GUI:proxy'
+})
+
+const config = require('yargs')


Why not reuse commander since it was already used in CLI? Is it obsolete in some way? But same a CLI-related comment above: not a priority (just out of curiosity).

It was in the CLI package and I am scared of the dark voodoo between cli & gui if they use the same package 😃

sebn · 2017-11-30T12:21:10Z

gui/src/main/proxy.js

+
+const printCertificate = (certif) => `Certificate(${certif.issuerName} ${certif.subjectName})`
+
+module.exports = (app, session, doneSetup) => {


Do we really export anything?

yep, this cant be initialized until app.on('ready')

Of course, we are exporting a function... Sorry, my bad :)

(My brain was messing up with requirejs-like syntax...)

sebn · 2017-11-30T12:43:39Z

gui/src/main/proxy.js

+    session.defaultSession.allowNTLMCredentialsForDomains(config['proxy-ntlm-domains'])
+  }
+
+  session.defaultSession.setCertificateVerifyProc((request, callback) => {


Is this for debug only?

yep. Hopefully will give us more data for the bug we were speaking about tonight.

sebn · 2017-11-30T12:48:00Z

gui/src/main/proxy.js

+    } else {
+      options = Object.assign({}, options)
+    }
+    options.agent = options.agent || https.globalAgent


Why doesn't https.request need the same options.headers.host patching as in http.request?

https.request calls http.request https://github.com/nodejs/node/blob/edb03cb65d51e336713d6af1134a7394c0776635/lib/https.js#L239

Ok, thanks for the explanation!

sebn · 2017-11-30T12:53:47Z

gui/src/main/proxy.js

+  if (config['login-by-realm']) {
+    config['login-by-realm'].split(',').forEach((lbr) => {
+      const [realm, username, password] = lbr.split(':')
+      loginByRealm[realm] = [username, password]


Make sure auth doesn't break when password contains a colon?

const [realm, username, passwordParts] = lbr.split(':') loginByRealm[realm] = [username, passwordParts.join(':')]

sebn · 2017-11-30T12:54:54Z

gui/src/main/proxy.js

+
+  app.on('certificate-error', (event, webContents, url, error, certificate, callback) => {
+    log.warn({url, error, certificate: printCertificate(certificate)}, 'App Certificate Error')
+    callback(false)


Why false?

Need to callback(false) to not accept any certificate.

sebn · 2017-11-30T18:47:49Z

👍 👌 🙈 ❤️ 🎉 🚢 🚀

aenario force-pushed the electron-proxy-agent branch from 4d9918e to 3b4f424 Compare November 23, 2017 14:31

aenario requested a review from sebn November 23, 2017 14:36

aenario changed the title ~~[WIP] Use electron-proxy-agent~~ Use electron-proxy-agent Nov 23, 2017

aenario force-pushed the electron-proxy-agent branch from 3b4f424 to 9bda66a Compare November 23, 2017 15:23

Use electron-proxy-agent & electron-fetch to support corporate proxies

f4c3404

aenario force-pushed the electron-proxy-agent branch from 9bda66a to f4c3404 Compare November 29, 2017 13:43

lint

d058b51

sebn reviewed Nov 30, 2017

View reviewed changes

human lint

7a3d980

aenario force-pushed the electron-proxy-agent branch from 793e784 to 7a3d980 Compare November 30, 2017 18:04

sebn merged commit 2ff1fc7 into master Nov 30, 2017

sebn deleted the electron-proxy-agent branch November 30, 2017 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use electron-proxy-agent #878

Use electron-proxy-agent #878

aenario commented Nov 23, 2017 •

edited

Loading

codecov-io commented Nov 29, 2017 •

edited

Loading

sebn left a comment

sebn Nov 30, 2017

sebn Nov 30, 2017

sebn Nov 30, 2017

sebn Nov 30, 2017

sebn Nov 30, 2017

sebn Nov 30, 2017

aenario Nov 30, 2017

sebn Nov 30, 2017

aenario Nov 30, 2017

sebn Nov 30, 2017

sebn Nov 30, 2017

sebn Nov 30, 2017

aenario Nov 30, 2017

sebn Nov 30, 2017

aenario Nov 30, 2017

sebn Nov 30, 2017

sebn Nov 30, 2017

sebn Nov 30, 2017

aenario Nov 30, 2017

sebn commented Nov 30, 2017


		log.warn({config}, 'argv')

		const printCertificate = (certif) => `Certificate(${certif.issuerName} ${certif.subjectName})`


		const printCertificate = (certif) => `Certificate(${certif.issuerName} ${certif.subjectName})`

		module.exports = (app, session, doneSetup) => {

Use electron-proxy-agent #878

Use electron-proxy-agent #878

Conversation

aenario commented Nov 23, 2017 • edited Loading

codecov-io commented Nov 29, 2017 • edited Loading

Codecov Report

sebn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebn commented Nov 30, 2017

aenario commented Nov 23, 2017 •

edited

Loading

codecov-io commented Nov 29, 2017 •

edited

Loading