Opened 3 months ago

Last modified 8 days ago

#30199 needs_review defect

tor-android-service: Review 2019/04/16

Reported by: sysrqb Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, tbb-8.5, TorBrowserTeam201907R
Cc: sisbell Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Overall, nicely done. This review is on master (6a9314aff4418a4edac33ff39fae266b097cf000):

the initial import (36f9873ff075253f4c1c9e394c91031fd4ba9d4a), do you know the git hash of the orbot commit you used?

1d635a925ca1728542067ee7bf34ff532d623a3f - The renaming is Tor Browser specific, so we should probably carry this as a patch in tor-browser-build, instead of hard coding "Tor Browser" within a general purpose "Tor service" library, right?

45244c49fbe382e97655b8f8d8f482e54f95ed07 - I wonder if we should carry a patch for this too. If tor-android-services+TOPL is intended as a general Android Tor library, then we probably shouldn't make it Tor Browser specific.

f3b3df4e66630a68c9987d52d93580549a0acaf4 - this package doesn't exist yet, but it will exist in a few weeks, so leaving it is probably okay.

1a2121e2a7ee8a6c0f3b5edd150b38eaae8bef304 - hasCookieAuthentication() and runAsDaemon() seem out of place in this commit

784919d8eb19083cf761b3e7314c49d8befc00cd - custom tor, the binaries should be compiled as dependencies by tor-browser-build and injected into the build, TorService.TOR_VERSION should be patched at build time

6a9314aff4418a4edac33ff39fae266b097cf000 - In the future, we should take the bridges used on desktop (currently specified in Firefox's pref format and injected at build time by tor-browser-build), and pre-process them for inclusion on Android, too.

Child Tickets

Change History (15)

comment:1 Changed 3 months ago by sysrqb

Keywords: tbb-mobile tbb-8.5 TorBrowserTeam201904 added

comment:2 Changed 3 months ago by gk

Keywords: tbb-8.5-must added; tbb-8.5 removed
Parent ID: #27609

comment:3 Changed 2 months ago by gk

Keywords: tbb-8.5 added; tbb-8.5-must removed

Nice to have but no blocker anymore.

comment:4 Changed 2 months ago by gk

Keywords: TorBrowserTeam201905 added; TorBrowserTeam201904 removed

Moving tickets to May

comment:5 Changed 5 weeks ago by gk

Parent ID: #27609

comment:6 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam201906 added; TorBrowserTeam201905 removed

Moving tickets to June

comment:7 Changed 3 weeks ago by sisbell

The original branch point was from commit: 8ad7668013a7291b98cf8689cd99961db69f8ed3

comment:8 in reply to:  description Changed 2 weeks ago by sisbell

I wasn't able to follow the exact commits in the descriptions (I can't seem to locate the hashes) but I have a general idea of what some of them are addressing:

Replying to sysrqb:

Overall, nicely done. This review is on master (6a9314aff4418a4edac33ff39fae266b097cf000):

the initial import (36f9873ff075253f4c1c9e394c91031fd4ba9d4a), do you know the git hash of the orbot commit you used?

The original branch point was from commit: 8ad7668013a7291b98cf8689cd99961db69f8ed3

1d635a925ca1728542067ee7bf34ff532d623a3f - The renaming is Tor Browser specific, so we should probably carry this as a patch in tor-browser-build, instead of hard coding "Tor Browser" within a general purpose "Tor service" library, right?

I did update the resources to be the latest ones in Orbot. We will need to provide a patch to add 'Tor Browser' text now.

45244c49fbe382e97655b8f8d8f482e54f95ed07 - I wonder if we should carry a patch for this too. If tor-android-services+TOPL is intended as a general Android Tor library, then we probably shouldn't make it Tor Browser specific.

f3b3df4e66630a68c9987d52d93580549a0acaf4 - this package doesn't exist yet, but it will exist in a few weeks, so leaving it is probably okay.

1a2121e2a7ee8a6c0f3b5edd150b38eaae8bef304 - hasCookieAuthentication() and runAsDaemon() seem out of place in this commit

I was not able to locate the 3 commits above to review

784919d8eb19083cf761b3e7314c49d8befc00cd - custom tor, the binaries should be compiled as dependencies by tor-browser-build and injected into the build, TorService.TOR_VERSION should be patched at build time

I'll open an issue for this. I think a good approach would be to place the version in a property file rather than using static fields. Orbot handles things a little differently now as well and pulls a binary artifact that contains the correct version in the meta-data.

6a9314aff4418a4edac33ff39fae266b097cf000 - In the future, we should take the bridges used on desktop (currently specified in Firefox's pref format and injected at build time by tor-browser-build), and pre-process them for inclusion on Android, too.

I did see sysrqb added this in commit d9b049c8cee225b4e7bb6f0191093f543d0f9f65

comment:9 Changed 2 weeks ago by gk

sisbell: you get the hashes if you clone our tor-android-service repo. You might need to add sysrqb's user repo as well (https://github.com/sysrqb/tor-android-service/). 1a2121e2a7ee8a6c0f3b5edd150b38eaae8bef304 is a typo and should be a2121e2a7ee8a6c0f3b5edd150b38eaae8bef304.

comment:10 Changed 9 days ago by gk

Keywords: TorBrowserTeam201907 added; TorBrowserTeam201906 removed

Moving tickets to July

comment:11 Changed 8 days ago by sisbell

45244c49fbe382e97655b8f8d8f482e54f95ed07

Yes, you are right. In tor-android-service/0628 branch I changed these back to ones that Orbot uses. We will now need to add a patch to change to our default ports.

comment:12 Changed 8 days ago by sisbell

f3b3df4e66630a68c9987d52d93580549a0acaf4

Orbot project is no longer using BROWSER_APP_USERNAME. So this will also be eliminated from our code. I'll open an issue for this.

comment:13 Changed 8 days ago by sisbell

a2121e2a7ee8a6c0f3b5edd150b38eaae8bef304
Yes I think they are out of place. We can modify the commit log.

comment:14 Changed 8 days ago by sisbell

Status: newneeds_review

comment:15 Changed 8 days ago by gk

Keywords: TorBrowserTeam201907R added; TorBrowserTeam201907 removed
Note: See TracTickets for help on using tickets.