[tor-bugs] #30199 [Applications/Tor Browser]: tor-android-service: Review 2019/04/16

[Tor Bug Tracker & Wiki]

#30199: tor-android-service: Review 2019/04/16
-------------------------------------------------+-------------------------
 Reporter:  sysrqb                               |          Owner:  tbb-
                                                 |  team
     Type:  defect                               |         Status:  new
 Priority:  Medium                               |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tbb-mobile, tbb-8.5,                 |  Actual Points:
  TorBrowserTeam201906                           |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------

Comment (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 [ticket:30199 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

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/30199#comment:8>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online