[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_review
 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 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?

 {{{
 @@ -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 ("

 {{{
 +            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.

 {{{
 +        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)

 {{{
 +    @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()?


 {{{
 +    /**
 +     * 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?

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

 {{{
 +        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.

 {{{
 +    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()`? 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?

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