BridgesList Preferences is an overloaded field
BridgesList is an overloaded field, which can cause some confusion. The list can be:
- a filter like obfs4 or meek OR
- can be a custom bridge
For (1), we just pass back all bridges, the filter will occur elsewhere in the library. For (2) we return the bridge list as a raw stream. If length is greater than 5, then we know this is a custom bridge
We should fix this so that we have separate preferences for 1 and 2. We also shouldn't be looking at string length to determine field type.
Activity
Trac:
Parent Ticket: #31280 (moved)
Trac:
Parent: N/A to #31280 (moved)-
Trac:
Parent: #31280 (moved) to #31349 -
Trac:
Parent: #31349 to #31280 (moved) Involves fixes in both TOPL code and tor-android-service
https://github.com/sisbell/tor-android-service/commit/0835e623f802127898df89cfb13685220dbe24b2
https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/ac9ad2295b3dcb312a9aff4f3f7b27343bc4b73d
This will create an incompatibility with Orbot, so I'll need to open another issue for that.
Trac:
Status: new to needs_review
Points: 1 to 2
Keywords: TorBrowserTeam201905 deleted, TorBrowserTeam201910R addedThanks! I'll start with TOPL in one comment and then post tor-android-services in another comment.
I think any incompatibility introduced between tor-android-service and Orbot should be resolved after a larger and more general conversation about where we're going with these libraries and whether it is worth our time maintaining compatibility (and if we should maintain these libraries in the long-term).
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 ofline()here? I think it'll be more readable ifbridge()is used consistently when we are adding a bridge. (see below comment aboutbridge(), 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 ashasPredefinedBridgeTypes()? It seem like we also want to know if we actually have predefined bridges in some cases, too. In particular,useBridgesFromSettings()andaddPredefinedBridgesFromResources()should use this, right?service/src/main/java/org/torproject/android/service/AndroidTorSettings.java:- @Override - public boolean hasDormantCanceledByStartup() { - return true; - }Don't we need this?
service/src/main/java/org/torproject/android/service/util/Prefs.java+ public static void setBridgeTypes(List<String> bridgeTypes) { + if(bridgeTypes == null || bridgeTypes.isEmpty()) {nit: "if ("
+ public static List<String> getBridgeTypes() { + String bridgeTypes = prefs.getString(PREF_BRIDGE_TYPES, null); + if(bridgeTypes == null || bridgeTypes.isEmpty()) {nit: "if (" (and the other instances in the patch).
+ BridgeType defaultBridgeType = (Locale.getDefault().getLanguage().equals("fa")) ? BridgeType.MEEK_LITE : BridgeType.OBFS4;We should check with the anti-censorship team that this is still the correct default. I know we inherited this from Orbot.
+ return Collections.singletonList(defaultBridgeType.name().toLowerCase());I'm not opposed to this, but did you choose a singletonList for a reason?
+ return Arrays.asList(bridgeTypes.split("[|]"));Please create a wrapper-method around
split(), likejoin(), in case we need to change the separate at a later point.+ private static String join(List<String> list) { + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < list.size(); i++) { + sb.append(list.get(i)); + if (i != list.size() - 1) { + sb.append("|");I think
|is a safe divider, but we should confirm with the anti-censorship team. In particular, I think|could be used in the PT arguments, but I don't think any of the current transports use an encoding that could include|.Trac:
Status: needs_review to needs_revisionReplying to 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 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 ofline()here? I think it'll be more readable ifbridge()is used consistently when we are adding a bridge. (see below comment aboutbridge(), 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 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() {
-
} }}} 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
return !settings.getBridgeTypes().isEmpty();addPredefinedBridgesFromStream()returns any bridges, right? Maybe rename it ashasPredefinedBridgeTypes()? 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()andaddPredefinedBridgesFromResources()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.
I collapsed #30552 (moved) into the same commit since it involves same code area. I removed the shuffling, removed max bridge check, renamed methods to make clearer, more tests, more docs. For one case, I check for existence of bridges.txt before setting bridges flag. Various other changes, cleanups.
https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/b72487be7b53d1ffcbd5f3942f2d0f7b1c79c859
Trac:
Status: needs_revision to needs_review
Keywords: TorBrowserTeam201910 deleted, TorBrowserTeam201911R addedReplying to sisbell:
One other thing we will need as part of this issue. We need to have code to migrate from the old format to the new format on upgrade so that we don't wipe out existing user preferences.
I can see this being in either Tor Browser or tor-android-service. I haven't thought hard about which place is better. We'll need a patch for Tor Browser so it uses the new API, in either case.
Replying to sysrqb:
Replying to sisbell:
One other thing we will need as part of this issue. We need to have code to migrate from the old format to the new format on upgrade so that we don't wipe out existing user preferences.
I can see this being in either Tor Browser or tor-android-service. I haven't thought hard about which place is better. We'll need a patch for Tor Browser so it uses the new API, in either case.
We need to do this in tor-android-service. The tor settings interface in topl just has getter methods, so their is no way at a interface level to pull out the values and then to set them back to the new format. Its up to each implementation to do migration like this.
mentioned in issue #31280 (moved)
