[tbb-bugs] #32516 [Applications/Tor Browser]: Make Write Methods Clearer in TorConfigBuilder

[Tor Bug Tracker & Wiki]

#32516: Make Write Methods Clearer in TorConfigBuilder
-----------------------------------------------+---------------------------
 Reporter:  sisbell                            |          Owner:  tbb-team
     Type:  defect                             |         Status:
                                               |  needs_revision
 Priority:  Medium                             |      Milestone:
Component:  Applications/Tor Browser           |        Version:
 Severity:  Normal                             |     Resolution:
 Keywords:  tbb-mobile, TorBrowserTeam201911R  |  Actual Points:
Parent ID:                                     |         Points:  .25
 Reviewer:                                     |        Sponsor:
-----------------------------------------------+---------------------------
Changes (by sysrqb):

 * status:  needs_review => needs_revision


Comment:

 {{{
      public TorConfigBuilder makeNonExitRelay(String dnsFile, int orPort,
 String nickname) {
 -        buffer.append("ServerDNSResolvConfFile
 ").append(dnsFile).append('\n');
 -        buffer.append("ORPort ").append(orPort).append('\n');
 -        buffer.append("Nickname ").append(nickname).append('\n');
 -        buffer.append("ExitPolicy reject *:*").append('\n');
 -        return this;
 +        writeLine("ServerDNSResolvConfFile", dnsFile);
 +        writeLine("ORPort", String.valueOf(orPort));
 }}}
 nit: ORPort should take an address, as well, and `writeAddress` seems more
 explicit. But don't worry about that right now, we can come back to this
 later.

 {{{
 public TorConfigBuilder proxyOnAllInterfaces() {
 -        buffer.append("SocksListenAddress 0.0.0.0").append('\n');
 -        return this;
 +        return writeLine("SocksListenAddress 0.0.0.0");
      }
 }}}
 nit: This could use `writeAddress("SocksListenAddress", "0.0.0.0", null,
 null)` instead

 {{{
      public TorConfigBuilder transportPluginObfs(String clientPath) {
 -        buffer.append("ClientTransportPlugin obfs3 exec
 ").append(clientPath).append('\n');
 -        buffer.append("ClientTransportPlugin obfs4 exec
 ").append(clientPath).append('\n');
 -        return this;
 +        return writeLine("ClientTransportPlugin obfs3 exec", clientPath)
 +                .writeLine("ClientTransportPlugin obfs4 exec",
 clientPath);
      }
 }}}
 This can be simplified to `ClientTransportPlugin obfs3,obfs4 exec`. Or, if
 we take this two steps further, combine this method and
 `transportPluginMeek` into a single `transportPlugin` and use
 `ClientTransportPlugin meek_lite,obfs2,obfs3,obfs4,scramblesuit exec`


 `writeAddress`
 {{{
 +        if (port != null) {
 +            buffer.append(port <= 0 ? "auto" : port);
 +        } else {
 +            buffer.append("auto");
 +        }
 }}}
 I'm still not sure we should change `port <= 0` into `auto`. `*Port 0` is
 already defined as disabling that type of port. Do we need two ways of
 setting `auto`?
 {{{
 +    public TorConfigBuilder writeLine(String value, String value2) {
 +        return writeLine(value + " " + value2);
 +    }
 }}}
 This seems like a surprising overload. I'm not opposed to having it, but
 it doesn't seem helpful and it is more confusing (less readable) than
 simply passing the string concatenation directly into `writeLine(String)`.

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