Integrate OpenVPN library

[alalamav]

• Modifies the local openvpn Cordova plugin to use OpenVPN for Android as a library. The plugin's functionality remains.
• Adds instructions to build the openvpn library and include it in the project.
• Removes Colony's dependecy on the OpenVPN app being installed in the device.
• The cordova npm module has been updated to version > 6.0.0 in order to link against the library aar.

---

This change is

[jpevarnek]

Aah, I'm terribly sorry, I forgot this PR was in this repository. Looking at this now.

[jpevarnek]

Hey, a few questions here, mostly around my lack of understanding in this area

---

Reviewed 12 of 12 files at r1. Review status: all files reviewed at latest revision, 13 unresolved discussions.

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 34 [r1] (raw file):

public class OpenVPN extends CordovaPlugin {
  final static String LOG_TAG = "OpenVPNPlugin";

So, adding the disclaimer that I'm not a Java expect, but this seems like it should probably be the class name (or the class name should be this), am I totally wrong there?

---

_client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 36 [r1] (raw file):_

  final static String LOG_TAG = "OpenVPNPlugin";

  private static final int PREPARE_VPN = 0;

This doesn't really give me any information about what the variable represents. All I get from this is that the number 0 has something to do with preparing the VPN. I think it would be good to have the name better reflect what it's actually doing

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 47 [r1] (raw file):

    this.cordova.getActivity().startService(serviceIntent);
    // Bind service.
    Intent intent = new Intent(getBaseContext(), OpenVPNService.class);

Can you add more descriptive names for intent and serviceIntent? I think it would be useful to have these closer reflect what you are using them for rather than just what they are.

---

_client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 55 [r1] (raw file):_

  @Override
  public boolean execute(String action, JSONArray args, CallbackContext callbackContext) throws JSONException {

Can you add a comment here explaining what the return value actually means?

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 56 [r1] (raw file):

  @Override
  public boolean execute(String action, JSONArray args, CallbackContext callbackContext) throws JSONException {
    if (action.equals("startVPN")) {

startVPN and stopVPN might be good things to make constants

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 57 [r1] (raw file):

  public boolean execute(String action, JSONArray args, CallbackContext callbackContext) throws JSONException {
    if (action.equals("startVPN")) {
      String inlineConfig = args.getString(0);

Do you need to validate the number of arguments?

---

_client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 67 [r1] (raw file):_

  }

  private ServiceConnection mConnection = new ServiceConnection() {

Is there an advantage to having the class implementation inline instead of doing it separately?

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 72 [r1] (raw file):

                                   IBinder service) {
      OpenVPNService.LocalBinder binder = (OpenVPNService.LocalBinder)service;
      mService = binder.getService();

I had to go back and check where mService was at this point, might be useful to have it named something closer to mRunningVpnService or something closer to that (I'm actually not exactly sure what it is).

---

_client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 76 [r1] (raw file):_

    @Override
    public void onServiceDisconnected(ComponentName arg0) {

What is arg0 in this case?

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 77 [r1] (raw file):

    @Override
    public void onServiceDisconnected(ComponentName arg0) {
        Log.i(LOG_TAG, "VPN disconnected.");

I think you have an extra level of indentation here

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 83 [r1] (raw file):

  private void prepareVPNService() {
    if (VpnService.prepare(getBaseContext()) == null) {

It looks to me like the android API is saying to launch the intent returned by this if you get one back, why are we not doing that? (can you add a comment)

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 96 [r1] (raw file):

      cp.parseConfig(new StringReader(inlineConfig));
      VpnProfile vp = cp.convertProfile();
      vp.mName = "OpenVPN";

My understanding is that the m means this field is non-public, is there a different way we should be setting it?

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 117 [r1] (raw file):

    int needPassword = vp.needUserPWInput(false);

    if(vpnPermissionIntent != null || needPassword != 0){

It looks to me like both of these got set above, can you at least add a comment explaining this?

---

_Comments from Reviewable_

[alalamav]

Review status: all files reviewed at latest revision, 13 unresolved discussions.

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 34 [r1] (raw file):

Previously, jpevarnek (Jonathan Pevarnek) wrote…

> So, adding the disclaimer that I'm not a Java expect, but this seems like it should probably be the class name (or the class name should be this), am I totally wrong there? >

Done.

---

_client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 36 [r1] (raw file):_

Previously, jpevarnek (Jonathan Pevarnek) wrote…

> This doesn't really give me any information about what the variable represents. All I get from this is that the number 0 has something to do with preparing the VPN. I think it would be good to have the name better reflect what it's actually doing >

Done.

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 47 [r1] (raw file):

Previously, jpevarnek (Jonathan Pevarnek) wrote…

> Can you add more descriptive names for `intent` and `serviceIntent`? I think it would be useful to have these closer reflect what you are using them for rather than just what they are. >

Done.

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 55 [r1] (raw file):

Previously, jpevarnek (Jonathan Pevarnek) wrote…

> Can you add a comment here explaining what the return value actually means? >

Done.

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 56 [r1] (raw file):

Previously, jpevarnek (Jonathan Pevarnek) wrote…

> `startVPN` and `stopVPN` might be good things to make constants >

Done.

---

_client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 57 [r1] (raw file):_

Previously, jpevarnek (Jonathan Pevarnek) wrote…

> Do you need to validate the number of arguments? >

Done.

---

_client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 67 [r1] (raw file):_

Previously, jpevarnek (Jonathan Pevarnek) wrote…

> Is there an advantage to having the class implementation inline instead of doing it separately? >

This seems to be the standard way of defining it - every example I've seen does it like this.

---

_client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 72 [r1] (raw file):_

Previously, jpevarnek (Jonathan Pevarnek) wrote…

> I had to go back and check where `mService` was at this point, might be useful to have it named something closer to `mRunningVpnService` or something closer to that (I'm actually not exactly sure what it is). >

Done.

---

_client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 76 [r1] (raw file):_

Previously, jpevarnek (Jonathan Pevarnek) wrote…

> What is `arg0` in this case? >

Clarified the argument name. https://developer.android.com/reference/android/content/ServiceConnection.html#onServiceDisconnected(android.content.ComponentName)

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 77 [r1] (raw file):

Previously, jpevarnek (Jonathan Pevarnek) wrote…

> I think you have an extra level of indentation here >

Done.

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 83 [r1] (raw file):

Previously, jpevarnek (Jonathan Pevarnek) wrote…

> It looks to me like the android API is saying to launch the intent returned by this if you get one back, why are we not doing that? (can you add a comment) >

I'm removing this method in favor of preparing the VPN service on every start. startVPNWithProfile requests the appropriate permissions from the user if VPN has not been prepared.

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 96 [r1] (raw file):

Previously, jpevarnek (Jonathan Pevarnek) wrote…

> My understanding is that the `m` means this field is non-public, is there a different way we should be setting it? >

I think the m conventionally stands for member, in this case it's a public one.

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 117 [r1] (raw file):

Previously, jpevarnek (Jonathan Pevarnek) wrote…

> It looks to me like both of these got set above, can you at least add a comment explaining this? >

VPN will be prepared here, if user hasn't granted permissions.

---

Comments from Reviewable

[jpevarnek]

Reviewed 2 of 2 files at r2. Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 52 [r2] (raw file):

    if (action.equals(ACTION_START_VPN)) {
      if (args.length() < 1) {
        callbackContext.error("Cannot start VPN without config.");

Stylistic thing: this takes a tiny bit of effort to read and see that we are returning true afterwards. I think it might be easier to understand if you had:

if (args.length() < 1) {
  error(...);
  return true;
}

startVPN(...);
return true;

That's totally personal preference, your choice on whether to consider it though.

Also, does it mean anything if we get more than one argument?

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 121 [r2] (raw file):

    Intent vpnPermissionIntent = VpnService.prepare(getBaseContext());
    int needPassword = vp.needUserPWInput(false);
    if(vpnPermissionIntent != null || needPassword != 0){

nit: looks like indentation here is a bit off from the rest of the code

---

Comments from Reviewable

[alalamav]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 52 [r2] (raw file):

Previously, jpevarnek (Jonathan Pevarnek) wrote…

> Stylistic thing: this takes a tiny bit of effort to read and see that we are returning true afterwards. I think it might be easier to understand if you had: > > ``` > if (args.length() < 1) { > error(...); > return true; > } > > startVPN(...); > return true; > ``` > > That's totally personal preference, your choice on whether to consider it though. > > Also, does it mean anything if we get more than one argument? >

Done. I think your suggestion is clearer.

---

_client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 121 [r2] (raw file):_

Previously, jpevarnek (Jonathan Pevarnek) wrote…

> nit: looks like indentation here is a bit off from the rest of the code >

Done.

---

Comments from Reviewable

[jpevarnek]

:thumbsup:

---

Reviewed 1 of 1 files at r3. Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

client/plugin-src/cordova-plugin-openvpn/src/android/OpenVPN.java, line 121 [r2] (raw file):

Previously, albertolalama (alberto lalama) wrote…

> Done. >

(oops, meant spacing, but thanks for getting what I meant!) :)

---

Comments from Reviewable