Allow the user to control which apps can use the VPN (WIP) #56
Conversation
Signed-off-by: Víctor Enríquez <188838+vquicksilver@users.noreply.github.com>
|
Attaching mandatory screenshot.
|
|
Disclaimer, the last time I coded anything was back in 2017 and this is the first time I try to do anything in go + gioui, so guidance to make this work properly is absolutely welcomed. Note that it is working already, though it might still be a little bit clunky. |
Signed-off-by: Víctor Enríquez <188838+vquicksilver@users.noreply.github.com>
| @@ -7,6 +7,8 @@ | |||
| <uses-permission android:name="android.permission.FOREGROUND_SERVICE" /> | |||
| <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" android:maxSdkVersion="28"/> | |||
|
|
|||
| <uses-permission android:name="android.permission.QUERY_ALL_PACKAGES" /> | |||
There was a problem hiding this comment.
Do you know if adding this will trigger a notification to the user when they install it from the Play Store? If you don't know that is ok, I can build a binary on the Internal Testing track and see what it does.
There was a problem hiding this comment.
No idea about this one sorry.
There was a problem hiding this comment.
There are guidelines available with a required form to submit to be accepted into the Play Store with this permission, but other VPN apps are currently active with it declared, so I see no reason for it to be rejected. I recall no notifications or popups when installing or using said apps on Android 13, and the fact that it is tied to approval may mean there is no need for a user-facing warning. Regardless, it unfortunately seems to be necessary in order to add split tunneling as none of the package queries are broad enough.
| Log.v("com.tailscale.ipn", "Loading apps: " + System.currentTimeMillis()/1000L); | ||
| try { | ||
| acfg = new AppsConfig(this, getEncryptedPrefs()); | ||
| //acfg.printConfig(); |
There was a problem hiding this comment.
Recommend removing leftover debugging code.
There was a problem hiding this comment.
Yes I left them there because I was worried of the impact in the starting time on the App depending on the number of apps, as you pointed below loading the apps at the start is far from optimal.
| } catch (Exception e) { | ||
| Log.e("com.tailscale.ipn", "exception", e); | ||
| } | ||
| Log.v("com.tailscale.ipn", "Apps loaded: " + System.currentTimeMillis()/1000L); |
There was a problem hiding this comment.
Admittedly this only appears in adb logcat, but if it was just here for your own development recommend removing it (and the one above)
| @@ -0,0 +1,271 @@ | |||
| // Copyright (c) 2021 Tailscale Inc & AUTHORS All rights reserved. | |||
There was a problem hiding this comment.
I guess I copy&pasted that, I will fix it.
| log.Printf("Checking clicks: %v", len(d.apps)) | ||
| for i < len(d.apps) { | ||
| if(d.apps[i].Changed()){ | ||
| log.Printf("App number %v changed state", i) |
There was a problem hiding this comment.
Recommend removing log message if it was just here for debugging.
| pm.getInstalledApplications(PackageManager.GET_META_DATA); | ||
|
|
||
| // This is done in the OpenVPN code | ||
| int androidSystemUid = 0; |
There was a problem hiding this comment.
This looks like Settings_Allowed_Apps.java from OpenVPN, which is GPL. This app is BSD licensed.
Did most of this file come from OpenVPN?
There was a problem hiding this comment.
Not the whole class, but the logic of that method (getInstalledApps) is doing the same basically, not that there are much more alternative ways of doing it that I am aware of anyway. Also a TODO for me here, I guess it makes sense to filter tailscale from the list of apps itself, it doesn't really makes much sense to keep it in the list.
There was a problem hiding this comment.
What are the implications of this in regards to implementation? Should I be looking to start from scratch to avoid license issues?
| @@ -233,6 +253,9 @@ func main() { | |||
| } | |||
| a.store = newStateStore(a.jvm, a.appCtx) | |||
| interfaces.RegisterInterfaceGetter(a.getInterfaces) | |||
| log.Printf("Loading apps in go: " + time.Now().String()) | |||
| a.loadAndroidApps() | |||
There was a problem hiding this comment.
Unfortunately I don't think loading the list of installed apps at startup is viable. Adware often does this, retrieving the list of installed apps, and the behavior isn't a good look. I think we'd need to retrieve the list of installed apps immediately before using it, when the user opens the menu which would let them exclude apps.
There was a problem hiding this comment.
I fully agree on that, this was the easiest/fastest/"hackyest" solution I could come with (with the time I spent on it), since I think calling those java methods from the UI thread is not that easy (I think there is no context there iirc), and getting the whole data stored in the class made it easier to pass the information.
I will try to explore other ways of doing it.
|
@DentonGentry Extra question, after using the modified app for a week or so, I also realized that probably the option for choosing the apps should be available even if the user hasn't signed it yet, so that it can block certain apps right away if required. Should I also hide that option? (like the one for using a custom server). Thank you very much for your review! I will try to allocate some time to improve this PR between this week and the next one. |
|
I am interested in contributing the necessary changes to this PR. Would the preferable path be to allow write access or create a new branch? |
|
@Myned, you can send a PR from your own fork. That's the typical GitHub workflow. You don't need write access or any permissions. |
This PR allows to control which apps are allowed to connect to the VPN, it adds a new entry to the menu once the user has been connected and display a dialog where the different apps can be enabled/disabled. The user needs to enable/disable the VPN again to apply the changes.
Signed-off-by: Víctor Enríquez 188838+vquicksilver@users.noreply.github.com