richard-better / pushbullet.py

A python client for http://pushbullet.com
MIT License
575 stars 110 forks source link

Added proxy support #102

Closed ghost closed 7 years ago

ghost commented 7 years ago

Added a proxy kwarg to init\() and added an if condition to update session headers w/ specified proxy. See #96

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.04%) to 59.33% when pulling e0a8870d59b63736353d470a4eb20d5847a95370 on otlie:master into 73e76267fa43563fb73d94509e12c9311743aca3 on randomchars:master.

kovacsbalu commented 7 years ago

"Note that only HTTPS proxies work with Pushbullet" can you please modify your code to setup only https proxy?

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.3%) to 59.048% when pulling 26b0fa83009a354489c886c600e6b177edfbee64 on otlie:master into 73e76267fa43563fb73d94509e12c9311743aca3 on randomchars:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.3%) to 59.048% when pulling 26b0fa83009a354489c886c600e6b177edfbee64 on otlie:master into 73e76267fa43563fb73d94509e12c9311743aca3 on randomchars:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.3%) to 59.048% when pulling 26b0fa83009a354489c886c600e6b177edfbee64 on otlie:master into 73e76267fa43563fb73d94509e12c9311743aca3 on randomchars:master.

ghost commented 7 years ago

@kovacsbalu Done. Requests will automatically use the HTTPS proxy if suitable, therefore it doesn't matter if they have a HTTP proxy along side the HTTPS, it'll still use the HTTPS one.

troeggla commented 7 years ago

I wonder if the dictionary syntax like here in the example is necessary if only HTTPS proxies are allowed anyway. Or is there a rationale behind this?

simonporter007 commented 7 years ago

Out of interest, where does the HTTPS proxy only requirement come from?

ghost commented 7 years ago

@troeggla: It's explicit, there's multiple types of proxies (SOCKS etc.)

@simonporter007: The API will return a `This server is only accessible over HTTPS.' message if you try to access the API using HTTP.

{"error":{"code":"invalid_request","type":"invalid_request","message":"This server is only accessible over HTTPS.","cat":"(=^‥^=)"},"error_code":"invalid_request"}

troeggla commented 7 years ago

Ah I understand, that makes sense! If there are no more concerns, I will merge this.

simonporter007 commented 7 years ago

Right sorry, misunderstood. You can use both http and https proxy addresses but you have to connect to the https endpoint (quite rightly).

Then maybe it does make sense to remove the dictionary syntax? As whatever is passed in should be handed to the https key?

curl --proxy http://proxy.com:8080 --header 'Access-Token: xxxxxxxxxxxxxxxxxxxxxxx' https://api.pushbullet.com/v2/users/me -k
{"active":true,"iden":"xxxxxxxxxxxxx","created":1.41617401377777e+09,"modified":1.479242967034234e+09,......}
curl --proxy http://proxy.com:8080 --header 'Access-Token: xxxxxxxxxxxxxxxxxxxxxxx' http://api.pushbullet.com/v2/users/me -k
{"error":{"code":"invalid_request","type":"invalid_request","message":"This server is only accessible over HTTPS.","cat":"(=^‥^=)"},"error_code":"invalid_request"}
ghost commented 7 years ago

@simonporter007 Well, I think the current syntax is more explicit and should be kept. As an example, it would suck if someone saw the word `proxy' with no protocol specified and bought a non-HTTP proxy (one could argue this is their own fault for not reading the documentation though), I think it would `do more good than harm' so to speak.

simonporter007 commented 7 years ago

@otlie fair point indeed!