search

thaliproject / Tor_Onion_Proxy_Library

Provides a JAR and an AAR for embedding the Tor Onion Proxy into a Java or Android Program
Apache License 2.0
175 stars 98 forks source link

Build update #45

Closed sisbell closed 6 years ago

sisbell commented 6 years ago

Issues fixed:

17 cleanup mess with jtorctl-briar.jar

18 Build Android Tor (this is no longer needed since we are dependent of tor-android-library for this)

19 Debugging data

30: Could not find proxy universal library

35: x86 build for Tor Binary

38: Problem installing gradle

Build changes Updated Gradle to 4.5 across project Updated Android Gradle build to 3.0.1 Require JDK version 1.7 Require Android SDK Version 21+ Include tor-android-library for Android install Include jtorctl:02 binary library (deleted jtorctl-briar lib)

Code Changes OnionProxyManager allows an implementing class to decide how to setup Tor. Moved java specific Tor install code from universal to java module. Use tor-android-library for Tor install in android module Some signature changes related to latest jtorctl import Added parameter to allow logging of tor control processing

Todos: Tested on Mac and Android device. Still needs testing on linux/windows/android_emulator Unit tests The threading logic in existing code should move to a publish/subscribe pattern. Need to maintain different versions of Tor binaries per platform version. Currently project is only using latest version which isn’t good for backwards compatibility.

This change is Reviewable

msftclas commented 6 years ago

CLA assistant check
All CLA requirements met.

yaronyg commented 6 years ago

Thank you so much for this! I just had one minor question and then we can commit. Thanks!

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

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

    }

    public final File getGeoIpv6File() {

Why does this need to be final?

Comments from Reviewable

sisbell commented 6 years ago

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

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

Previously, yaronyg (Yaron Y Goland) wrote…
Why does this need to be final?

I was inferring the intent from the original code, where the fields are final. So it would make sense that the method accessors for those fields should also be final.

I could see the case where we would want some configuration file not to be in the working directory. There are couple of solutions that jump out.

1) Marking these method accessors as not final. I would suggest in that case to also remove the final flag on the respective fields so they could be overriden in the subclass.

2) Create a ConfigBuilder class that allows the application to specialize config locations. Then we would have: OnionProxyContext(ConfigBuilder builder). The default builder would be WorkingDirConfigBuilder(File dir)

I’m more in favor of (2), not much work either way. Thoughts?

Comments from Reviewable

yaronyg commented 6 years ago

2 sounds great, thanks!

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

Comments from Reviewable

sisbell commented 6 years ago

Should have the ConfigBuilder complete and ready for review by 2/10

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

Comments from Reviewable

yaronyg commented 6 years ago

Looking forward to it!

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

Comments from Reviewable

sisbell commented 6 years ago

Ready for review and commit.

General:

Android:

yaronyg commented 6 years ago

This is great! I'm happy to ignore my last comment and commit if you would like since the comment is about an issue that predates your code. Just let me know. Thanks!

Reviewed 20 of 20 files at r2. Review status: all files reviewed at latest revision, all discussions resolved.

universal/src/main/java/com/msopentech/thali/toronionproxy/FileUtilities.java, line 83 at r2 (raw file):


    public static boolean setToReadOnlyPermissions(File file) {
        return file.setReadable(false, false) &&

I believe this is actually code that you just moved here but doesn't it look wrong? Why we are setting writable and executable to true for a function that says it is setting read only permissions? And no, I don't remember. :)

Comments from Reviewable

sisbell commented 6 years ago

. The FileUtilities.setToReadOnlyPermissions is correct behavior. It's not clear from Javadocs for the File class, but the first three lines remove all permissions (user/world/group). Then last three set them to user only. Without first three lines, the last three would leave group permissions on top of current file permissions (probably only a *nix issue).

public static boolean setToReadOnlyPermissions(File file) {
    return file.setReadable(false, false) &&
            file.setWritable(false, false) &&
            file.setExecutable(false, false) &&

            file.setReadable(true, true) &&
            file.setWritable(true, true) &&
            file.setExecutable(true, true);
}

If everything else looks good, I'd say its ready for a merge from my side.

Comments from Reviewable