From f149005026c52ce22e1b5f3e18fc3fa58fcbe1be Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Mon, 27 Apr 2020 17:17:34 -0400 Subject: [PATCH] Add support for remote config v1.1 --- .../jobs/RemoteConfigRefreshJob.java | 2 +- .../logsubmit/LogSectionFeatureFlags.java | 30 ++-- .../securesms/util/FeatureFlags.java | 163 +++++++++++------- .../securesms/util/FeatureFlagsTest.java | 37 +++- .../api/SignalServiceAccountManager.java | 6 +- .../internal/push/RemoteConfigResponse.java | 7 + 6 files changed, 160 insertions(+), 85 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/RemoteConfigRefreshJob.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/RemoteConfigRefreshJob.java index bd31efe1e..4b0faeae3 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/RemoteConfigRefreshJob.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/RemoteConfigRefreshJob.java @@ -50,7 +50,7 @@ public class RemoteConfigRefreshJob extends BaseJob { return; } - Map config = ApplicationDependencies.getSignalServiceAccountManager().getRemoteConfig(); + Map config = ApplicationDependencies.getSignalServiceAccountManager().getRemoteConfig(); FeatureFlags.update(config); } diff --git a/app/src/main/java/org/thoughtcrime/securesms/logsubmit/LogSectionFeatureFlags.java b/app/src/main/java/org/thoughtcrime/securesms/logsubmit/LogSectionFeatureFlags.java index 8f0f7dee1..e1784a252 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/logsubmit/LogSectionFeatureFlags.java +++ b/app/src/main/java/org/thoughtcrime/securesms/logsubmit/LogSectionFeatureFlags.java @@ -20,31 +20,39 @@ public class LogSectionFeatureFlags implements LogSection { @Override public @NonNull CharSequence getContent(@NonNull Context context) { - StringBuilder out = new StringBuilder(); - Map memory = FeatureFlags.getMemoryValues(); - Map disk = FeatureFlags.getDiskValues(); - Map forced = FeatureFlags.getForcedValues(); - int remoteLength = Stream.of(memory.keySet()).map(String::length).max(Integer::compareTo).orElse(0); - int diskLength = Stream.of(disk.keySet()).map(String::length).max(Integer::compareTo).orElse(0); - int forcedLength = Stream.of(forced.keySet()).map(String::length).max(Integer::compareTo).orElse(0); + StringBuilder out = new StringBuilder(); + Map memory = FeatureFlags.getMemoryValues(); + Map disk = FeatureFlags.getDiskValues(); + Map pending = FeatureFlags.getPendingDiskValues(); + Map forced = FeatureFlags.getForcedValues(); + int remoteLength = Stream.of(memory.keySet()).map(String::length).max(Integer::compareTo).orElse(0); + int diskLength = Stream.of(disk.keySet()).map(String::length).max(Integer::compareTo).orElse(0); + int pendingLength = Stream.of(pending.keySet()).map(String::length).max(Integer::compareTo).orElse(0); + int forcedLength = Stream.of(forced.keySet()).map(String::length).max(Integer::compareTo).orElse(0); out.append("-- Memory\n"); - for (Map.Entry entry : memory.entrySet()) { + for (Map.Entry entry : memory.entrySet()) { out.append(Util.rightPad(entry.getKey(), remoteLength)).append(": ").append(entry.getValue()).append("\n"); } out.append("\n"); - out.append("-- Disk\n"); - for (Map.Entry entry : disk.entrySet()) { + out.append("-- Current Disk\n"); + for (Map.Entry entry : disk.entrySet()) { out.append(Util.rightPad(entry.getKey(), diskLength)).append(": ").append(entry.getValue()).append("\n"); } out.append("\n"); + out.append("-- Pending Disk\n"); + for (Map.Entry entry : pending.entrySet()) { + out.append(Util.rightPad(entry.getKey(), pendingLength)).append(": ").append(entry.getValue()).append("\n"); + } + out.append("\n"); + out.append("-- Forced\n"); if (forced.isEmpty()) { out.append("None\n"); } else { - for (Map.Entry entry : forced.entrySet()) { + for (Map.Entry entry : forced.entrySet()) { out.append(Util.rightPad(entry.getKey(), forcedLength)).append(": ").append(entry.getValue()).append("\n"); } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java b/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java index 3c524e91a..afb2fed2e 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java +++ b/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java @@ -29,7 +29,7 @@ import java.util.concurrent.TimeUnit; * * When creating a new flag: * - Create a new string constant. This should almost certainly be prefixed with "android." - * - Add a method to retrieve the value using {@link #getValue(String, boolean)}. You can also add + * - Add a method to retrieve the value using {@link #getBoolean(String, boolean)}. You can also add * other checks here, like requiring other flags. * - If you want to be able to change a flag remotely, place it in {@link #REMOTE_CAPABLE}. * - If you would like to force a value for testing, place an entry in {@link #FORCED_VALUES}. @@ -37,7 +37,7 @@ import java.util.concurrent.TimeUnit; * * Other interesting things you can do: * - Make a flag {@link #HOT_SWAPPABLE} - * - Make a flag {@link #STICKY} + * - Make a flag {@link #STICKY} -- booleans only! * - Register a listener for flag changes in {@link #FLAG_CHANGE_LISTENERS} */ public final class FeatureFlags { @@ -86,7 +86,7 @@ public final class FeatureFlags { * an addition to this map. */ @SuppressWarnings("MismatchedQueryAndUpdateOfCollection") - private static final Map FORCED_VALUES = new HashMap() {{ + private static final Map FORCED_VALUES = new HashMap() {{ }}; /** @@ -124,14 +124,14 @@ public final class FeatureFlags { put(MESSAGE_REQUESTS, (change) -> SignalStore.setMessageRequestEnableTime(change == Change.ENABLED ? System.currentTimeMillis() : 0)); }}; - private static final Map REMOTE_VALUES = new TreeMap<>(); + private static final Map REMOTE_VALUES = new TreeMap<>(); private FeatureFlags() {} public static synchronized void init() { - Map current = parseStoredConfig(SignalStore.remoteConfigValues().getCurrentConfig()); - Map pending = parseStoredConfig(SignalStore.remoteConfigValues().getPendingConfig()); - Map changes = computeChanges(current, pending); + Map current = parseStoredConfig(SignalStore.remoteConfigValues().getCurrentConfig()); + Map pending = parseStoredConfig(SignalStore.remoteConfigValues().getPendingConfig()); + Map changes = computeChanges(current, pending); SignalStore.remoteConfigValues().setCurrentConfig(mapToJson(pending)); REMOTE_VALUES.putAll(pending); @@ -151,10 +151,10 @@ public final class FeatureFlags { } } - public static synchronized void update(@NonNull Map config) { - Map memory = REMOTE_VALUES; - Map disk = parseStoredConfig(SignalStore.remoteConfigValues().getPendingConfig()); - UpdateResult result = updateInternal(config, memory, disk, REMOTE_CAPABLE, HOT_SWAPPABLE, STICKY); + public static synchronized void update(@NonNull Map config) { + Map memory = REMOTE_VALUES; + Map disk = parseStoredConfig(SignalStore.remoteConfigValues().getPendingConfig()); + UpdateResult result = updateInternal(config, memory, disk, REMOTE_CAPABLE, HOT_SWAPPABLE, STICKY); SignalStore.remoteConfigValues().setPendingConfig(mapToJson(result.getDisk())); REMOTE_VALUES.clear(); @@ -171,7 +171,7 @@ public final class FeatureFlags { /** UUID-related stuff that shouldn't be activated until the user-facing launch. */ public static synchronized boolean uuids() { - return getValue(UUIDS, false); + return getBoolean(UUIDS, false); } /** Favoring profile names when displaying contacts. */ @@ -181,12 +181,12 @@ public final class FeatureFlags { /** MessageRequest stuff */ public static synchronized boolean messageRequests() { - return getValue(MESSAGE_REQUESTS, false); + return getBoolean(MESSAGE_REQUESTS, false); } /** Creating usernames, sending messages by username. Requires {@link #uuids()}. */ public static synchronized boolean usernames() { - boolean value = getValue(USERNAMES, false); + boolean value = getBoolean(USERNAMES, false); if (value && !uuids()) throw new MissingFlagRequirementError(); return value; } @@ -202,76 +202,81 @@ public final class FeatureFlags { SignalStore.kbsValues().isV2RegistrationLockEnabled() || SignalStore.kbsValues().hasPin() || pinsForAllMandatory() || - getValue(PINS_FOR_ALL_LEGACY, false) || - getValue(PINS_FOR_ALL, false); + getBoolean(PINS_FOR_ALL_LEGACY, false) || + getBoolean(PINS_FOR_ALL, false); } /** Makes it so the user will eventually see a fullscreen splash requiring them to create a PIN. */ public static boolean pinsForAllMandatory() { - return getValue(PINS_FOR_ALL_MANDATORY, false); + return getBoolean(PINS_FOR_ALL_MANDATORY, false); } /** Safety flag to disable Pins for All Megaphone */ public static boolean pinsForAllMegaphoneKillSwitch() { - return getValue(PINS_MEGAPHONE_KILL_SWITCH, false); + return getBoolean(PINS_MEGAPHONE_KILL_SWITCH, false); } /** Safety switch for disabling profile names megaphone */ public static boolean profileNamesMegaphone() { - return getValue(PROFILE_NAMES_MEGAPHONE, false) && + return getBoolean(PROFILE_NAMES_MEGAPHONE, false) && TextSecurePreferences.getFirstInstallVersion(ApplicationDependencies.getApplication()) < 600; } /** Whether or not we use the attachments v3 form. */ public static boolean attachmentsV3() { - return getValue(ATTACHMENTS_V3, false); + return getBoolean(ATTACHMENTS_V3, false); } /** Send support for remotely deleting a message. */ public static boolean remoteDelete() { - return getValue(REMOTE_DELETE, false); + return getBoolean(REMOTE_DELETE, false); } /** Whether or not profile sharing is required for calling */ public static boolean profileForCalling() { - return messageRequests() && getValue(PROFILE_FOR_CALLING, false); + return messageRequests() && getBoolean(PROFILE_FOR_CALLING, false); } /** Whether or not to display Calling PIP */ public static boolean callingPip() { - return getValue(CALLING_PIP, false); + return getBoolean(CALLING_PIP, false); } /** New group UI elements. */ public static boolean newGroupUI() { - return getValue(NEW_GROUP_UI, false); + return getBoolean(NEW_GROUP_UI, false); } /** Only for rendering debug info. */ - public static synchronized @NonNull Map getMemoryValues() { + public static synchronized @NonNull Map getMemoryValues() { return new TreeMap<>(REMOTE_VALUES); } /** Only for rendering debug info. */ - public static synchronized @NonNull Map getDiskValues() { + public static synchronized @NonNull Map getDiskValues() { return new TreeMap<>(parseStoredConfig(SignalStore.remoteConfigValues().getCurrentConfig())); } /** Only for rendering debug info. */ - public static synchronized @NonNull Map getForcedValues() { + public static synchronized @NonNull Map getPendingDiskValues() { + return new TreeMap<>(parseStoredConfig(SignalStore.remoteConfigValues().getPendingConfig())); + } + + /** Only for rendering debug info. */ + public static synchronized @NonNull Map getForcedValues() { return new TreeMap<>(FORCED_VALUES); } @VisibleForTesting - static @NonNull UpdateResult updateInternal(@NonNull Map remote, - @NonNull Map localMemory, - @NonNull Map localDisk, - @NonNull Set remoteCapable, - @NonNull Set hotSwap, - @NonNull Set sticky) + static @NonNull UpdateResult updateInternal(@NonNull Map remote, + @NonNull Map localMemory, + @NonNull Map localDisk, + @NonNull Set remoteCapable, + @NonNull Set hotSwap, + @NonNull Set sticky) { - Map newMemory = new TreeMap<>(localMemory); - Map newDisk = new TreeMap<>(localDisk); + Map newMemory = new TreeMap<>(localMemory); + Map newDisk = new TreeMap<>(localDisk); Set allKeys = new HashSet<>(); allKeys.addAll(remote.keySet()); @@ -281,12 +286,26 @@ public final class FeatureFlags { Stream.of(allKeys) .filter(remoteCapable::contains) .forEach(key -> { - Boolean remoteValue = remote.get(key); - Boolean diskValue = localDisk.get(key); - Boolean newValue = remoteValue; + Object remoteValue = remote.get(key); + Object diskValue = localDisk.get(key); + Object newValue = remoteValue; - if (sticky.contains(key)) { + if (newValue != null && diskValue != null && newValue.getClass() != diskValue.getClass()) { + Log.w(TAG, "Type mismatch! key: " + key); + + newDisk.remove(key); + + if (hotSwap.contains(key)) { + newMemory.remove(key); + } + + return; + } + + if (sticky.contains(key) && (newValue instanceof Boolean || diskValue instanceof Boolean)) { newValue = diskValue == Boolean.TRUE ? Boolean.TRUE : newValue; + } else if (sticky.contains(key)) { + Log.w(TAG, "Tried to make a non-boolean sticky! Ignoring. (key: " + key + ")"); } if (newValue != null) { @@ -319,7 +338,7 @@ public final class FeatureFlags { } @VisibleForTesting - static @NonNull Map computeChanges(@NonNull Map oldMap, @NonNull Map newMap) { + static @NonNull Map computeChanges(@NonNull Map oldMap, @NonNull Map newMap) { Map changes = new HashMap<>(); Set allKeys = new HashSet<>(); @@ -327,37 +346,59 @@ public final class FeatureFlags { allKeys.addAll(newMap.keySet()); for (String key : allKeys) { - Boolean oldValue = oldMap.get(key); - Boolean newValue = newMap.get(key); + Object oldValue = oldMap.get(key); + Object newValue = newMap.get(key); if (oldValue == null && newValue == null) { throw new AssertionError("Should not be possible."); } else if (oldValue != null && newValue == null) { changes.put(key, Change.REMOVED); + } else if (newValue != oldValue && newValue instanceof Boolean) { + changes.put(key, (boolean) newValue ? Change.ENABLED : Change.DISABLED); } else if (newValue != oldValue) { - changes.put(key, newValue ? Change.ENABLED : Change.DISABLED); + changes.put(key, Change.CHANGED); } } return changes; } - private static boolean getValue(@NonNull String key, boolean defaultValue) { - Boolean forced = FORCED_VALUES.get(key); + private static boolean getBoolean(@NonNull String key, boolean defaultValue) { + Boolean forced = (Boolean) FORCED_VALUES.get(key); if (forced != null) { return forced; } - Boolean remote = REMOTE_VALUES.get(key); - if (remote != null) { - return remote; + Object remote = REMOTE_VALUES.get(key); + if (remote instanceof Boolean) { + return (boolean) remote; + } else if (remote != null) { + Log.w(TAG, "Expected a boolean for key '" + key + "', but got something else! Falling back to the default."); } return defaultValue; } - private static Map parseStoredConfig(String stored) { - Map parsed = new HashMap<>(); + private static int getInteger(@NonNull String key, int defaultValue) { + Integer forced = (Integer) FORCED_VALUES.get(key); + if (forced != null) { + return forced; + } + + String remote = (String) REMOTE_VALUES.get(key); + if (remote != null) { + try { + return Integer.parseInt(remote); + } catch (NumberFormatException e) { + Log.w(TAG, "Expected an int for key '" + key + "', but got something else! Falling back to the default."); + } + } + + return defaultValue; + } + + private static Map parseStoredConfig(String stored) { + Map parsed = new HashMap<>(); if (TextUtils.isEmpty(stored)) { Log.i(TAG, "No remote config stored. Skipping."); @@ -370,7 +411,7 @@ public final class FeatureFlags { while (iter.hasNext()) { String key = iter.next(); - parsed.put(key, root.getBoolean(key)); + parsed.put(key, root.get(key)); } } catch (JSONException e) { throw new AssertionError("Failed to parse! Cleared storage."); @@ -379,12 +420,12 @@ public final class FeatureFlags { return parsed; } - private static @NonNull String mapToJson(@NonNull Map map) { + private static @NonNull String mapToJson(@NonNull Map map) { try { JSONObject json = new JSONObject(); - for (Map.Entry entry : map.entrySet()) { - json.put(entry.getKey(), (boolean) entry.getValue()); + for (Map.Entry entry : map.entrySet()) { + json.put(entry.getKey(), entry.getValue()); } return json.toString(); @@ -409,21 +450,21 @@ public final class FeatureFlags { @VisibleForTesting static final class UpdateResult { - private final Map memory; - private final Map disk; - private final Map memoryChanges; + private final Map memory; + private final Map disk; + private final Map memoryChanges; - UpdateResult(@NonNull Map memory, @NonNull Map disk, @NonNull Map memoryChanges) { + UpdateResult(@NonNull Map memory, @NonNull Map disk, @NonNull Map memoryChanges) { this.memory = memory; this.disk = disk; this.memoryChanges = memoryChanges; } - public @NonNull Map getMemory() { + public @NonNull Map getMemory() { return memory; } - public @NonNull Map getDisk() { + public @NonNull Map getDisk() { return disk; } @@ -438,7 +479,7 @@ public final class FeatureFlags { } enum Change { - ENABLED, DISABLED, REMOVED + ENABLED, DISABLED, CHANGED, REMOVED } /** Read and write versioned profile information. */ diff --git a/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlagsTest.java b/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlagsTest.java index fae628774..10945d395 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlagsTest.java +++ b/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlagsTest.java @@ -1,6 +1,7 @@ package org.thoughtcrime.securesms.util; import org.junit.Test; +import org.thoughtcrime.securesms.BaseUnitTest; import org.thoughtcrime.securesms.util.FeatureFlags.Change; import org.thoughtcrime.securesms.util.FeatureFlags.UpdateResult; @@ -14,23 +15,23 @@ import static junit.framework.TestCase.assertTrue; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -public class FeatureFlagsTest { +public class FeatureFlagsTest extends BaseUnitTest { private static final String A = "A"; private static final String B = "B"; @Test public void updateInternal_newValue_ignoreNotInRemoteCapable() { - UpdateResult result = FeatureFlags.updateInternal(mapOf("A", true, - "B", true), + UpdateResult result = FeatureFlags.updateInternal(mapOf(A, true, + B, true), mapOf(), mapOf(), - setOf("A"), + setOf(A), setOf(), setOf()); assertEquals(mapOf(), result.getMemory()); - assertEquals(mapOf("A", true), result.getDisk()); + assertEquals(mapOf(A, true), result.getDisk()); assertTrue(result.getMemoryChanges().isEmpty()); } @@ -286,11 +287,25 @@ public class FeatureFlagsTest { assertEquals(Change.REMOVED, result.getMemoryChanges().get(A)); } + @Test + public void updateInternal_removeValue_typeMismatch_hotSwap() { + UpdateResult result = FeatureFlags.updateInternal(mapOf(A, "5"), + mapOf(A, true), + mapOf(A, true), + setOf(A), + setOf(A), + setOf()); + + assertEquals(mapOf(), result.getMemory()); + assertEquals(mapOf(), result.getDisk()); + assertEquals(Change.REMOVED, result.getMemoryChanges().get(A)); + } + @Test public void updateInternal_twoNewValues() { UpdateResult result = FeatureFlags.updateInternal(mapOf(A, true, - B, false), - mapOf(), + B, false), + mapOf(), mapOf(), setOf(A, B), setOf(), @@ -320,19 +335,22 @@ public class FeatureFlagsTest { @Test public void computeChanges_generic() { - Map oldMap = new HashMap() {{ + Map oldMap = new HashMap() {{ put("a", true); put("b", false); put("c", true); put("d", false); + put("g", 5); + put("h", 5); }}; - Map newMap = new HashMap() {{ + Map newMap = new HashMap() {{ put("a", true); put("b", true); put("c", false); put("e", true); put("f", false); + put("h", 7); }}; Map changes = FeatureFlags.computeChanges(oldMap, newMap); @@ -343,6 +361,7 @@ public class FeatureFlagsTest { assertEquals(Change.REMOVED, changes.get("d")); assertEquals(Change.ENABLED, changes.get("e")); assertEquals(Change.DISABLED, changes.get("f")); + assertEquals(Change.CHANGED, changes.get("h")); } private static Set setOf(V... values) { diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/SignalServiceAccountManager.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/SignalServiceAccountManager.java index 24bf6d2ba..523d33f3b 100644 --- a/libsignal/service/src/main/java/org/whispersystems/signalservice/api/SignalServiceAccountManager.java +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/api/SignalServiceAccountManager.java @@ -582,12 +582,12 @@ public class SignalServiceAccountManager { } } - public Map getRemoteConfig() throws IOException { + public Map getRemoteConfig() throws IOException { RemoteConfigResponse response = this.pushServiceSocket.getRemoteConfig(); - Map out = new HashMap<>(); + Map out = new HashMap<>(); for (RemoteConfigResponse.Config config : response.getConfig()) { - out.put(config.getName(), config.isEnabled()); + out.put(config.getName(), config.getValue() != null ? config.getValue() : config.isEnabled()); } return out; diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/internal/push/RemoteConfigResponse.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/internal/push/RemoteConfigResponse.java index a7a89d154..41110d9ad 100644 --- a/libsignal/service/src/main/java/org/whispersystems/signalservice/internal/push/RemoteConfigResponse.java +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/internal/push/RemoteConfigResponse.java @@ -19,6 +19,9 @@ public class RemoteConfigResponse { @JsonProperty private boolean enabled; + @JsonProperty + private String value; + public String getName() { return name; } @@ -26,5 +29,9 @@ public class RemoteConfigResponse { public boolean isEnabled() { return enabled; } + + public String getValue() { + return value; + } } }