Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core Functionality for User Settings and Event Broadcasts #68

Merged
merged 21 commits into from Feb 15, 2019

Conversation

@sisbell
Copy link
Contributor

commented Jan 29, 2019

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 Reviewable

@yaronyg
Copy link
Member

left a comment

createGoogleNameserver

I'm certainly not going to complain if you want to help drag this poor ancient code into the modern world. :) Please see comments.

Reviewed 18 of 18 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @sisbell)


java/src/main/java/com/msopentech/thali/java/toronionproxy/JavaTorInstaller.java, line 99 at r1 (raw file):

    @Override
    public void updateTorConfigCustom(String content) throws IOException, TimeoutException {
        //TODO: needs implementation

Please at least throw an exception in case someone somehow accidentally calls it.


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?


universal/src/main/java/com/msopentech/thali/toronionproxy/OnionProxyContext.java, line 158 at r1 (raw file):

    }

    public final File createGoogleNameserverFile() throws IOException {

Why would we want to hard code in Google's DNS servers?


universal/src/main/java/com/msopentech/thali/toronionproxy/OnionProxyManager.java, line 549 at r1 (raw file):

    /**
     * Sets the exit nodes through the tor control connection

You have a scenario where you want to manually set the exit node?


universal/src/main/java/com/msopentech/thali/toronionproxy/OnionProxyManager.java, line 678 at r1 (raw file):

            } catch (InterruptedException e) {
            }
            if (killAttempts++ > 4)

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.


universal/src/main/java/com/msopentech/thali/toronionproxy/TorConfigBuilder.java, line 43 at r1 (raw file):

            IOException {
        TorConfig config = context.getConfig();
        controlPortWriteToFile(config.getControlPortFile().getCanonicalPath())

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?

@sisbell

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019


universal/src/main/java/com/msopentech/thali/toronionproxy/OnionProxyManager.java, line 549 at r1 (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019


universal/src/main/java/com/msopentech/thali/toronionproxy/DefaultSettings.java, line 26 at r1 (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…
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).

https://github.com/sisbell/tor-android-service/blob/ba6bd51aef5320f198cc276951db65facd92f0f9/service/src/main/java/org/torproject/android/service/AndroidTorSettings.java

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019


universal/src/main/java/com/msopentech/thali/toronionproxy/OnionProxyContext.java, line 158 at r1 (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019


java/src/main/java/com/msopentech/thali/java/toronionproxy/JavaTorInstaller.java, line 99 at r1 (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

Please at least throw an exception in case someone somehow accidentally calls it.

I'll add UnsupportedOperationException

@sisbell

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019


universal/src/main/java/com/msopentech/thali/toronionproxy/OnionProxyManager.java, line 678 at r1 (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019


universal/src/main/java/com/msopentech/thali/toronionproxy/TorConfigBuilder.java, line 43 at r1 (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

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.

@yaronyg
Copy link
Member

left a comment

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @sisbell and @yaronyg)


universal/src/main/java/com/msopentech/thali/toronionproxy/DefaultSettings.java, line 26 at r1 (raw file):

Previously, sisbell (Shane Isbell) wrote…

I do have an Android implementation in a dependent project (which is a modified version of OrbotService using TOPL).

https://github.com/sisbell/tor-android-service/blob/ba6bd51aef5320f198cc276951db65facd92f0f9/service/src/main/java/org/torproject/android/service/AndroidTorSettings.java

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

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)


universal/src/main/java/com/msopentech/thali/toronionproxy/OnionProxyContext.java, line 158 at r1 (raw file):

Previously, sisbell (Shane Isbell) wrote…

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.

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. :)


universal/src/main/java/com/msopentech/thali/toronionproxy/OnionProxyManager.java, line 549 at r1 (raw file):

Previously, sisbell (Shane Isbell) wrote…

Yes, I'm going off the Orbot app which has an Exit Node configuration under its settings.

O.k. so it's not so much that you have a need for this as that you are trying to exhaustively cover all the options Orbot supports. Makes sense.


universal/src/main/java/com/msopentech/thali/toronionproxy/OnionProxyManager.java, line 678 at r1 (raw file):

Previously, sisbell (Shane Isbell) wrote…

I'll make this change.

Thanks!


universal/src/main/java/com/msopentech/thali/toronionproxy/TorConfigBuilder.java, line 43 at r1 (raw file):

Previously, sisbell (Shane Isbell) wrote…

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.

+1

@sisbell

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019


universal/src/main/java/com/msopentech/thali/toronionproxy/DefaultSettings.java, line 26 at r1 (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

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.

https://github.com/sisbell/Tor_Onion_Proxy_Library/blob/a5a78f9da0299fbd345219d0e8822d7fb491e2a7/universal/src/main/java/com/msopentech/thali/toronionproxy/DefaultSettings.java

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019


universal/src/main/java/com/msopentech/thali/toronionproxy/OnionProxyContext.java, line 158 at r1 (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

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 sisbell force-pushed the sisbell:master branch from a5a78f9 to 6126e8e Jan 31, 2019

@sisbell sisbell force-pushed the sisbell:master branch from 6126e8e to 0fae89c Jan 31, 2019

@sisbell

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019


universal/src/main/java/com/msopentech/thali/toronionproxy/OnionProxyContext.java, line 158 at r1 (raw file):

Previously, sisbell (Shane Isbell) wrote…

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019


java/src/main/java/com/msopentech/thali/java/toronionproxy/JavaTorInstaller.java, line 99 at r1 (raw file):

Previously, sisbell (Shane Isbell) wrote…

I'll add UnsupportedOperationException

Done.

@sisbell

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019

@sisbell

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019

@yaronyg
Copy link
Member

left a comment

Reviewed 1 of 18 files at r1, 1 of 1 files at r2, 13 of 16 files at r3.
Reviewable status: 26 of 29 files reviewed, 2 unresolved discussions (waiting on @sisbell and @yaronyg)


android/src/main/AndroidManifest.xml, line 9 at r3 (raw file):

    <application android:allowBackup="false"
        android:label="@string/app_name"

Given that this is a library do we really need these values? Does something break if we omit them?


android/src/main/res/drawable-hdpi/ic_launcher.png, line 0 at r3 (raw file):
A library shouldn't need this or the other binary files, right?

@sisbell

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019


android/src/main/res/drawable-hdpi/ic_launcher.png, line at r3 (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

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.

59cb27c

@sisbell

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019


android/src/main/AndroidManifest.xml, line 9 at r3 (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019


android/src/main/AndroidManifest.xml, line 9 at r3 (raw file):

Previously, sisbell (Shane Isbell) wrote…

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.

@yaronyg
yaronyg approved these changes Feb 4, 2019
Copy link
Member

left a comment

Reviewed 3 of 16 files at r3, 8 of 8 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


android/src/main/AndroidManifest.xml, line 9 at r3 (raw file):

Previously, sisbell (Shane Isbell) wrote…

Done.

I was actually making this comment about the contents of not about the uses-permission statements above it.

But if we can remove that permission, all the better! So I'll take the mistake as a win. :)

@sisbell
Copy link
Contributor Author

left a comment

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@yaronyg yaronyg merged commit f8124f4 into thaliproject:master Feb 15, 2019

2 checks passed

code-review/reviewable 35 files reviewed
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.