Proxy support for Android and IOS #1727

marcmcl · 2020-11-09T13:31:04Z

Screenshots

Add any relevant before/after screenshots here

How did you test the change?

iOS Simulator
iOS Device
Android Simulator
Android Device
curl to a dev App Engine server
other, please describe
Tested by manually specifying proxy host and port, as well as with a proxy.pac URL.

Checklist:

Followed the Contributor Guidelines and verified that all contributions are properly licensed pursuant to the LICENSE.

brunobowden · 2020-11-09T20:40:45Z

@marcmcl - have a look at the check failures on GitHub:

pubspec.lock changes are missing. This is breaking most of the CI builds.
Podfile.lock changes are missing. Even though you're not fixing up iOS, you need to include this.

brunobowden · 2020-11-09T20:52:28Z

@marcmcl - two further comments:

Your name shouldn't be in the title. It should be targeted such as "Proxy Support for Android"
PR description should mention associated issue: "Partial fix for Network Proxy Settings aren't used by Flutter #1155"
What other libraries did you look at for http proxies? There seem to be a few on pub.dev. My concern is that by creating a new library, you become responsible for maintaining it long in to the future. I'd first carefully check if there's other good libraries we can use. If we have to build our own, then it might be better to do it under the WHO repo so it has more shared responsibility.
The GitHub example is formatted incorrectly and needs this type of quote:

code example

stale · 2020-11-17T11:21:30Z

This item has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

marcmcl · 2020-11-28T16:31:53Z

@brunobowden I added the missing files and have also included my plugin files as well. Managed to get hostname and port working for proxy on IOS too. Still no support for proxy authentication on both IOS and Android implementations. IOS implementation also doesn't autoconfig proxies specified via PAC yet.

brunobowden

Hi @marcmcl, thanks for contributing this. It'll take me more time to look at this. Here's some initial cursory observations.

docs/legal/apache-2.0-relicensing.txt

client/lib/api/who_service.dart

brunobowden · 2020-11-29T00:17:51Z

@marcmcl - there's also a conflict in client/ios/Podfile.lock. Not sure what to do in GitHub desktop but from the command line you'll need to:

git fetch upstream
git rebase -i upstream/master

Fix for WorldHealthOrganization#1155

- Added get_proxy plugin files which includes some IOS support

marcmcl · 2020-12-02T20:31:42Z

@brunobowden going to retest tomorrow then I will mark the PR as ready for review.

brunobowden

@marcmcl - thanks for pressing on with this. Sorry to ask you to rename lots of file as that's a painful process. Feel free to discuss / push back on the name. We should be thoughtful about that so we don't need to change it later on.

There's also two further client fetches using webHelper.downloadFile in caching.dart. Though should also use the proxy but I'm not quite sure how to do it:
https://github.com/WorldHealthOrganization/app/blob/master/client/lib/api/content/caching.dart#L44

brunobowden · 2020-12-02T21:52:53Z

client/packages/get_proxy/README.md

+
+var client = new HttpClient();
+String proxy = await GetProxy.getProxy;
+if(proxy!="")


missing space after if

brunobowden · 2020-12-02T22:25:10Z

client/lib/api/who_service.dart

@@ -4,7 +4,8 @@ import 'package:who_app/main.dart';
 import 'package:who_app/api/user_preferences.dart';
 import 'package:http/http.dart' as http;
 import 'dart:io' as io;
-
+import 'package:get_proxy/get_proxy.dart';


Instead of get_proxy, should it be http_proxy? You're not getting a proxy but using it for requests. It should match the http.post API as much as possible. The used class can then be httpProxy which more closely matches.

Just a heads up that get_proxy was literally just getting the proxy information. But I'll move the http.post alternative into this package and rename it as you suggest @brunobowden .

brunobowden · 2020-12-02T22:25:14Z

client/lib/api/who_service.dart

@@ -24,7 +25,7 @@ class WhoService {
    final postBody = jsonEncode(req.toProto3Json());

    final url = '$serviceUrl/putDeviceToken';
-    final response = await http.post(url, headers: headers, body: postBody);
+    final response = await httpPost(url, headers, postBody);


General thought is that the http.post APIs will have a lot of thought that's gone in to them. So my suggestion was to use the same API for httpPost. It also benefits users as they can use the same syntax and it benefits reviewers as there's fewer changes switching from one to the other. So you'd expect the new line to be:

Suggested change

final response = await httpPost(url, headers, postBody);

final response = await await httpProxy.post(url, headers: headers, body: postBody);

stale · 2020-12-09T22:43:36Z

This item has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2020-12-21T14:12:14Z

This item has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

marcmcl requested review from britannio, brunobowden, theswerd and yukuairoy as code owners November 9, 2020 13:31

marcmcl changed the title ~~Marcmcl proxysupport~~ Proxy support for Android Nov 10, 2020

marcmcl marked this pull request as draft November 10, 2020 11:04

stale bot added the resolved:stale No recent activity on the issue or PR label Nov 17, 2020

marcmcl changed the title ~~Proxy support for Android~~ Proxy support for Android and IOS Nov 28, 2020

stale bot removed the resolved:stale No recent activity on the issue or PR label Nov 28, 2020

brunobowden reviewed Nov 29, 2020

View reviewed changes

docs/legal/apache-2.0-relicensing.txt Outdated Show resolved Hide resolved

client/lib/api/who_service.dart Outdated Show resolved Hide resolved

marcmcl added 4 commits November 30, 2020 20:27

fix: Added proxy support for calls to WHO API endpoints

c56984a

Fix for WorldHealthOrganization#1155

fix: Minor typo when reentering code on Mac

08be1d6

- Added missing pubspec and Podfile changes

8940612

- Added get_proxy plugin files which includes some IOS support

- Minor styling change based on PR feedback

eaabc6f

marcmcl force-pushed the marcmcl-proxysupport branch from 05dcc57 to eaabc6f Compare November 30, 2020 20:35

marcmcl added 7 commits December 2, 2020 15:01

- Various fixes to pass pre-commit checks

e811412

- Fix formatting to pass Prettier Formatting check

42bc6e4

- Fix policy violations checks

1cc16d5

- Fix incorrect version specified in Podfile.lock

ffb15cc

- Fixed formatting in who_service.dart

2fca3fc

- Fix incorrect version in Podfile.lock

668914d

- Corrected version problem for get_proxy plugin

e391d55

brunobowden requested changes Dec 2, 2020

View reviewed changes

stale bot added resolved:stale No recent activity on the issue or PR and removed resolved:stale No recent activity on the issue or PR labels Dec 9, 2020

stale bot added the resolved:stale No recent activity on the issue or PR label Dec 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proxy support for Android and IOS #1727

Proxy support for Android and IOS #1727

marcmcl commented Nov 9, 2020 •

edited

Loading

brunobowden commented Nov 9, 2020

brunobowden commented Nov 9, 2020 •

edited

Loading

stale bot commented Nov 17, 2020

marcmcl commented Nov 28, 2020

brunobowden left a comment

brunobowden commented Nov 29, 2020

marcmcl commented Dec 2, 2020

brunobowden left a comment

brunobowden Dec 2, 2020

brunobowden Dec 2, 2020

marcmcl Dec 14, 2020

brunobowden Dec 2, 2020

stale bot commented Dec 9, 2020

stale bot commented Dec 21, 2020

	final response = await httpPost(url, headers, postBody);
	final response = await await httpProxy.post(url, headers: headers, body: postBody);

Proxy support for Android and IOS #1727

Are you sure you want to change the base?

Proxy support for Android and IOS #1727

Conversation

marcmcl commented Nov 9, 2020 • edited Loading

Screenshots

How did you test the change?

Checklist:

brunobowden commented Nov 9, 2020

brunobowden commented Nov 9, 2020 • edited Loading

stale bot commented Nov 17, 2020

marcmcl commented Nov 28, 2020

brunobowden left a comment

Choose a reason for hiding this comment

brunobowden commented Nov 29, 2020

marcmcl commented Dec 2, 2020

brunobowden left a comment

Choose a reason for hiding this comment

brunobowden Dec 2, 2020

Choose a reason for hiding this comment

brunobowden Dec 2, 2020

Choose a reason for hiding this comment

marcmcl Dec 14, 2020

Choose a reason for hiding this comment

brunobowden Dec 2, 2020

Choose a reason for hiding this comment

stale bot commented Dec 9, 2020

stale bot commented Dec 21, 2020

marcmcl commented Nov 9, 2020 •

edited

Loading

brunobowden commented Nov 9, 2020 •

edited

Loading