[tor-bugs] #30501 [Applications/Tor Browser]: BridgesList Preferences is an overloaded field

[Tor Bug Tracker & Wiki]

#30501: BridgesList Preferences is an overloaded field
-----------------------------------------------+---------------------------
 Reporter:  sisbell                            |          Owner:  tbb-team
     Type:  defect                             |         Status:
                                               |  needs_revision
 Priority:  Medium                             |      Milestone:
Component:  Applications/Tor Browser           |        Version:
 Severity:  Normal                             |     Resolution:
 Keywords:  tbb-mobile, TorBrowserTeam201910R  |  Actual Points:
Parent ID:  #31280                             |         Points:  2
 Reviewer:                                     |        Sponsor:
                                               |  Sponsor30-can
-----------------------------------------------+---------------------------

Comment (by sisbell):

 Replying to [comment:10 sysrqb]:
 >
 `universal/src/main/java/com/msopentech/thali/toronionproxy/TorConfigBuilder.java`:
 >
 > {{{
 > @@ -92,32 +92,19 @@ public final class TorConfigBuilder {
 > [snip]
 >         if (!pluggableTransportClient.exists() ||
 !pluggableTransportClient.canExecute()) {
 >              throw new IOException("Bridge binary does not exist: " +
 pluggableTransportClient
 >                      .getCanonicalPath());
 >          }
 > }}}
 > Please make this exception's message reflect the correct error - does it
 noe exist or is it not executable?
 Sure, easy enough

 >
 > {{{
 > @@ -153,6 +140,27 @@ public final class TorConfigBuilder {
 >          return
 controlPortWriteToFile(context.config.getControlPortFile().getAbsolutePath());
 >      }
 >
 > +    public TorConfigBuilder customBridges(List<String> bridges) {
 > +        if(bridges.size() > 1) {
 > }}}
 > nit: "if ("
 >
 +1

 > {{{
 > +            Collections.shuffle(bridges, new
 Random(System.nanoTime()));
 > +        }
 > }}}
 > I still don't completely understand this (or above). What's the
 reasoning for shuffling? Tor chooses randomly from the list, in any case.
 I wasn't aware that tor itself does the shuffling. I will remove this
 line.

 >
 > {{{
 > +        for (String bridge : bridges) {
 > +            if(!isNullOrEmpty(bridge)) {
 > +                line("Bridge " + bridge);
 > +            }
 > }}}
 > nit: Can you use `bridge()` instead of `line()` here? I think it'll be
 more readable if `bridge()` is used consistently when we are adding a
 bridge. (see below comment about `bridge()`, as well)

 line method is a general method for just writing out a line. Its also used
 in torrcCustomFromSettings method. I can change this is writeLine to make
 clearer.

 >
 > {{{
 > +    @SettingsConfig
 > +    public TorConfigBuilder customBridgesFromSettings() {
 > +        if (!settings.hasBridges() || !hasCustomBridges()) {
 > +            return this;
 > +        }
 > +        List<String> bridges = settings.getCustomBridges();
 > +        return customBridges(bridges);
 > }}}
 > Can you pass settings.getCustomBridges() directly into customBridges()?
 Sure, its a stylistic preference but easy enough.
 >
 >
 > {{{
 > +    /**
 > +     * Write up two bridges from packaged bridge list if bridges option
 is enabled and if no custom bridges
 > }}}
 > nit: "Write up to"?
 > I also don't understand why it only writes two bridges? Why not write
 all of them?
 I thought this was a requirement but if not, I'll remove it.

 >
 > {{{
 > +    TorConfigBuilder addPredefinedBridgesFromResources(List<BridgeType>
 types, int maxBridges) {
 > }}}
 > Should this have package-level visibility? I realize it was before this
 change, too.

 I'll change it. I left some open for the unit tests but this method is not
 under direct test.
 >
 > {{{
 > +        ArrayList<String> bridgeTypes = new ArrayList<>();
 > +        for(BridgeType bridgeType : types) {
 > }}}
 > nit: "for ("
 >
 > {{{
 > +            bridgeTypes.add(bridgeType.name().toLowerCase());
 > +        }
 > +        if(settings.hasBridges() && hasPredefinedBridges() &&
 !hasCustomBridges()) {
 > }}}
 > nit: "if ("
 >
 > {{{
 > -            if (b.type.equals(bridgeType)) {
 > +            if(bridgeTypes.contains(b.type)) {
 > }}}
 > "if ("
 >
 > {{{
 > public TorConfigBuilder bridge(String type, String config) {
 > }}}
 > Can you change the name of this method to something like
 "addBridgeLine"? "bridge()" is not descriptive.

 The TorConfigBuilder just maps method name to fieldName. If I changed this
 one I would need to do such for all methods, which I think would junk up
 the readability of all the public methods, as they would all be
 add{FieldName}Line.

 >
 > {{{
 > +    private boolean hasPredefinedBridges() {
 > +        return !settings.getBridgeTypes().isEmpty();
 >      }
 > }}}
 > Isn't this only testing that we have pre-defined bridge types, but not
 actual bridges? We'd want to know if the stream for
 `addPredefinedBridgesFromStream()` returns any bridges, right? Maybe
 rename it as `hasPredefinedBridgeTypes()`?
 Makes sense, I'll rename method to be clearer

 It seem like we also want to know if we actually have predefined bridges
 in some cases, too. In particular, `useBridgesFromSettings()` and
 `addPredefinedBridgesFromResources()` should use this, right?

 This would occur if the bridges.txt is empty. This check occurs in
 readBridgesFromStream, which would return an empty list.
 addPredefinedBridgesFromStream invokes readBridgesFromStream so that is
 where the check is currently occurring (an empty list is effectively a
 noop). I can document this to be clearer.

 I'll need to investigate further the case of useBridgesFromSettings if the
 list is empty, in that case it may require explicit check.

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