Closed sisbell closed 5 years ago
sisbell
commented
5 years ago universal/src/main/java/com/msopentech/thali/toronionproxy/OnionProxyManager.java, line 549 at r1 (raw file):
You have a scenario where you want to manually set the exit node?
Yes, I'm going off the Orbot app which has an Exit Node configuration under its settings.
sisbell
commented
5 years ago universal/src/main/java/com/msopentech/thali/toronionproxy/DefaultSettings.java, line 26 at r1 (raw file):
> ``` > TorSettings > ``` Wouldn't it be easier to just define TorSettings to not include all the APIs we don't have implementations for now and then add them in later when we have implementations?
I do have an Android implementation in a dependent project (which is a modified version of OrbotService using TOPL).
In the case of Android, I'm using it as a wrapper around shared prefs. I could see moving this into the android project at TOPL at some point. For Java, it really will depend on the developer, whether to wrap around a database, flat file config, etc
sisbell
commented
5 years ago universal/src/main/java/com/msopentech/thali/toronionproxy/OnionProxyContext.java, line 158 at r1 (raw file):
Why would we want to hard code in Google's DNS servers?
This is a default one for convenience. I can just pull out the method entirely. The user will just need to know to create the resolve file if they want to set a relay.
sisbell
commented
5 years ago java/src/main/java/com/msopentech/thali/java/toronionproxy/JavaTorInstaller.java, line 99 at r1 (raw file):
Please at least throw an exception in case someone somehow accidentally calls it.
I'll add UnsupportedOperationException
sisbell
commented
5 years ago universal/src/main/java/com/msopentech/thali/toronionproxy/OnionProxyManager.java, line 678 at r1 (raw file):
Just a coding style preference for me. Could you do the increment on a separate line? I recognize that the code is correct I just find it harder to read post increment forms like this inline.
I'll make this change.
sisbell
commented
5 years ago universal/src/main/java/com/msopentech/thali/toronionproxy/TorConfigBuilder.java, line 43 at r1 (raw file):
This is just such a bug waiting to happen. Someone will add another method and forget to add that method here and then the control file will be wrong. Couldn't we use introspection so that if a new method is added to TorConfigBuilder we won't miss it here?
Good Point. I think one way to handle this is to use annotations to annotate the methods we want invoked inside of TorConfigBuilder.updateTorConfig() . I'll need to fix TorConfigBuilder methods that take a param. Specifically I'll add the pluggableTransportFile to the TorConfig and also modify controlPortWriteToFile not to directly use 'String controlPortFile' as a param.
sisbell
commented
5 years ago universal/src/main/java/com/msopentech/thali/toronionproxy/DefaultSettings.java, line 26 at r1 (raw file):
It seems like we should have some default implementation here for managing settings. Even if it's just in memory. Although I vaguely remember that Orbot has a config file somewhere doesn it? (Sorry it's been a REALLY long time)
There is a default settings class in the merge request. It provides some basic hard-coded values.
In the Orbot case, I'm using AndroidTorSettings to wrap a prefs file. A default Java implementation could involve a properties file (we can open this as a separate issue/feature).
sisbell
commented
5 years ago universal/src/main/java/com/msopentech/thali/toronionproxy/OnionProxyContext.java, line 158 at r1 (raw file):
M'eh. Let's make their life easier. But a comment on the method explaining that it's just there as a quick convenience will answer questions. :)
Cool, I'll leave this for now, just adding some documentation. I'll open a a new issue on how to make this more configurable.
sisbell
commented
5 years ago universal/src/main/java/com/msopentech/thali/toronionproxy/OnionProxyContext.java, line 158 at r1 (raw file):
Cool, I'll leave this for now, just adding some documentation. I'll open a a new issue on how to make this more configurable.
Done.
sisbell
commented
5 years ago java/src/main/java/com/msopentech/thali/java/toronionproxy/JavaTorInstaller.java, line 99 at r1 (raw file):
I'll add UnsupportedOperationException
Done.
sisbell
commented
5 years ago universal/src/main/java/com/msopentech/thali/toronionproxy/OnionProxyManager.java, line 678 at r1 (raw file):
Thanks!
Done.
sisbell
commented
5 years ago universal/src/main/java/com/msopentech/thali/toronionproxy/TorConfigBuilder.java, line 43 at r1 (raw file):
+1
Done
sisbell
commented
5 years ago android/src/main/res/drawable-hdpi/ic_launcher.png, line at r3 (raw file):
A library shouldn't need this or the other binary files, right?
Yes, that's true. I pushed those changes a couple of hours ago. I also have a few other Android changes.
sisbell
commented
5 years ago android/src/main/AndroidManifest.xml, line 9 at r3 (raw file):
Given that this is a library do we really need these values? Does something break if we omit them?
We need ACCESS_NETWORK_STATE permission for the ConnectivityManager.getActiveNetworkInfo() call. We need INTERNET permission for the socket connections. We should leave these in so that the app that uses this library will merge in the required permissions into its own manifest.
I'm not seeing any use of WifManager so I think we can safely remove ACCESS_WIFI_STATE. I'll open an issue and submit the change as part of this pull request.
sisbell
commented
5 years ago android/src/main/AndroidManifest.xml, line 9 at r3 (raw file):
We need ACCESS_NETWORK_STATE permission for the ConnectivityManager.getActiveNetworkInfo() call. We need INTERNET permission for the socket connections. We should leave these in so that the app that uses this library will merge in the required permissions into its own manifest. I'm not seeing any use of WifManager so I think we can safely remove ACCESS_WIFI_STATE. I'll open an issue and submit the change as part of this pull request.
Done.
Added core functionality mostly around user settings and event broadcasts. These are abstractions needed to integrate TOPL with applications like Orbot. However the functionality has general use.
This change is