Closes #2033: Add whitelist to pref key for Telemetry and Firebase An…

…aylitcs

app/src/androidTest/java/org/mozilla/focus/telemetry/TelemetryWrapperTest.java

@@ -20,6 +20,10 @@
 import java.util.Locale;
 
 import static junit.framework.Assert.assertEquals;
+import static junit.framework.Assert.assertNotNull;
+import static junit.framework.Assert.assertNull;
+import static junit.framework.Assert.assertTrue;
+import static org.mozilla.focus.telemetry.TelemetryWrapper.Object.SETTING;
 import static org.mozilla.focus.telemetry.TelemetryWrapper.Value.AUDIO;
 import static org.mozilla.focus.telemetry.TelemetryWrapper.Value.EME;
 import static org.mozilla.focus.telemetry.TelemetryWrapper.Value.MIDI;
@@ -96,17 +100,17 @@ public void launchByExternalAppEvent() {
     public void settingsEvent() {
         SharedPreferences pm = PreferenceManager.getDefaultSharedPreferences(InstrumentationRegistry.getTargetContext());
         for (String key : pm.getAll().keySet()) {
-            // TelemetryWrapper.settingsEvent(key, Boolean.FALSE.toString());
-            assertFirebaseEvent(TelemetryWrapper.Category.ACTION, TelemetryWrapper.Method.CHANGE, TelemetryWrapper.Object.SETTING, key, TelemetryWrapper.Extra.TO, Boolean.FALSE.toString());
+
+            TelemetryWrapper.settingsEvent(key, Boolean.FALSE.toString());
         }
     }
 
     @Test
     public void settingsClickEvent() {
         SharedPreferences pm = PreferenceManager.getDefaultSharedPreferences(InstrumentationRegistry.getTargetContext());
         for (String key : pm.getAll().keySet()) {
-            // TelemetryWrapper.settingsClickEvent(key);
-            assertFirebaseEvent(TelemetryWrapper.Category.ACTION, TelemetryWrapper.Method.CLICK, TelemetryWrapper.Object.SETTING, key);
+            // invalid events should be by pass
+            TelemetryWrapper.settingsClickEvent(key);
         }
     }
 
@@ -577,5 +581,21 @@ public void showPromoteShareDialog() {
         TelemetryWrapper.showPromoteShareDialog();
     }
 
+    @Test
+    public void validPrefKeyFromWhitelistShouldPass() {
+
+        // call lazyInit() to fill the whitelist
+        TelemetryWrapper.EventBuilder.lazyInit();
+
+        // make sure layInit() inits the whitelist successfully.
+        assertTrue(FirebaseEvent.getPrefKeyWhitelist().size() > 0);
+
+        // whitelist-ed pref key should worked with firebaseEvent and telemetryEvent
+        for (String validKey : FirebaseEvent.getPrefKeyWhitelist().values()) {
+            final TelemetryWrapper.EventBuilder builder = new TelemetryWrapper.EventBuilder(TelemetryWrapper.Category.ACTION, TelemetryWrapper.Method.CHANGE, SETTING, validKey);
+            assertNotNull(builder.firebaseEvent);
+            assertNotNull(builder.telemetryEvent);
+        }
+    }
 
 }

app/src/main/java/org/mozilla/focus/settings/SettingsFragment.java

@@ -101,6 +101,7 @@ public void onPause() {
 
     @Override
     public void onSharedPreferenceChanged(SharedPreferences sharedPreferences, String key) {
+        // special handling for locale selection
         if (key.equals(getString(R.string.pref_key_locale))) {
             // Updating the locale leads to onSharedPreferenceChanged being triggered again in some
             // cases. To avoid an infinite loop we won't update the preference a second time. This
@@ -138,9 +139,11 @@ public void onSharedPreferenceChanged(SharedPreferences sharedPreferences, Strin
                     .replace(R.id.container, new SettingsFragment())
                     .commit();
             return;
+        } else {
+            // For other events, we handle them here.
+            TelemetryWrapper.settingsEvent(key, String.valueOf(sharedPreferences.getAll().get(key)));
         }
 
-        TelemetryWrapper.settingsEvent(key, String.valueOf(sharedPreferences.getAll().get(key)));
 
         if (key.equals(getString(R.string.pref_key_storage_clear_browsing_data))) {
             //Clear browsing data Callback function is not here

app/src/main/java/org/mozilla/focus/telemetry/FirebaseEvent.java

@@ -16,9 +16,9 @@
 import org.mozilla.focus.utils.AppConstants;
 import org.mozilla.focus.utils.FirebaseHelper;
 
-import java.util.HashSet;
+import java.util.HashMap;
+import java.util.Map;
 import java.util.Objects;
-import java.util.Set;
 
 class FirebaseEvent {
 
@@ -44,7 +44,7 @@ class FirebaseEvent {
     static final int MAX_LENGTH_PARAM_NAME = 40;
     static final int MAX_LENGTH_PARAM_VALUE = 100;
     private static final String TAG = "FirebaseEvent";
-    private static Set<String> existingPreferenceKey = new HashSet<>();
+    private static HashMap<String, String> prefKeyWhitelist = new HashMap<>();
 
     private String eventName;
     private Bundle eventParam;
@@ -59,21 +59,23 @@ class FirebaseEvent {
 
         final int prefixLength = eventNamePrefix.length();
 
+        // TODO: check eventName should start with[a-zA-Z] , contains only [a-zA-A0-9_], and shouldn't
+        // TODO: start with ^(?!(firebase_|google_|ga_)).*
+        // validate the length
         if (this.eventName.length() > MAX_LENGTH_EVENT_NAME) {
 
+            // we only care about prefixLength if the total length is too long
             if (prefixLength > MAX_LENGTH_EVENT_NAME_PREFIX) {
-                handleError("Event[" + this.eventName + "]'s prefixLength too long  " + prefixLength + " of " + MAX_LENGTH_EVENT_NAME_PREFIX);
+                throwOrWarn("Event[" + this.eventName + "]'s prefixLength too long  " + prefixLength + " of " + MAX_LENGTH_EVENT_NAME_PREFIX);
             }
 
-            // TODO: check eventName should start with[a-zA-Z] , contains only [a-zA-A0-9_], and shouldn't
-            // TODO: start with ^(?!(firebase_|google_|ga_)).*
-            if (value != null && isValueInWhiteList(value)) {
+            throwOrWarn("Event[" + this.eventName + "] exceeds Firebase event name limit " + this.eventName.length() + " of " + MAX_LENGTH_EVENT_NAME);
+
+            // fix the value if we just want to warn
+            if (value != null) {
                 int acceptValueLength = MAX_LENGTH_EVENT_NAME - eventNamePrefix.length();
                 int valueLength = value.length();
                 this.eventName = eventNamePrefix + value.substring(valueLength - acceptValueLength, valueLength);
-            } else {
-                // No matter if value is null nor not, if the length of event name is too long, handle the error hera.
-                handleError("Event[" + this.eventName + "] exceeds Firebase event name limit " + this.eventName.length() + " of " + MAX_LENGTH_EVENT_NAME);
             }
         }
     }
@@ -84,17 +86,13 @@ public static FirebaseEvent create(@NonNull String category, @NonNull String met
 
     }
 
-    @CheckResult
-    public static FirebaseEvent create(@NonNull String category, @NonNull String method, @Nullable String object) {
-        return new FirebaseEvent(category, method, object, null);
-    }
-
     public FirebaseEvent param(String name, String value) {
         if (this.eventParam == null) {
             this.eventParam = new Bundle();
         }
+        // validate the size
         if (this.eventParam.size() >= MAX_PARAM_SIZE) {
-            handleError("Firebase event[" + eventName + "] has too many parameters");
+            throwOrWarn("Firebase event[" + eventName + "] has too many parameters");
         }
 
         this.eventParam.putString(safeParamLength(name, MAX_LENGTH_PARAM_NAME),
@@ -103,12 +101,14 @@ public FirebaseEvent param(String name, String value) {
         return this;
     }
 
+    // TODO: check param name should start with[a-zA-Z] , contains only [a-zA-A0-9_], and shouldn't
+    // TODO: start with ^(?!(firebase_|google_|ga_)).*
     private static String safeParamLength(@NonNull final String str, final int end) {
+        // validate the length
         if (str.length() > end) {
-            handleError("Exceeding limit of param content length:" + str.length() + " of " + end);
+            throwOrWarn("Exceeding limit of param content length:" + str.length() + " of " + end);
         }
-        // TODO: check param name should start with[a-zA-Z] , contains only [a-zA-A0-9_], and shouldn't
-        // TODO: start with ^(?!(firebase_|google_|ga_)).*
+        // fix the value if we just want to warn
         return str.substring(0,
                 Math.min(end, str.length()));
     }
@@ -132,24 +132,32 @@ public void setParam(Bundle bundle) {
     }
 
 
-    private static void handleError(String msg) {
+    private static void throwOrWarn(String msg) {
         if (AppConstants.isReleaseBuild()) {
             Log.e(TAG, msg);
         } else {
             throw new IllegalArgumentException(msg);
         }
     }
 
-    private static boolean isValueInWhiteList(@NonNull String value) {
-        return existingPreferenceKey.contains(value);
+    static String getValidPrefKey(@NonNull String value) {
+        return prefKeyWhitelist.get(value);
+    }
+
+    /**
+     * @return the whitelisted pref key. This may be empty if not initialized.
+     */
+    @VisibleForTesting
+    static Map<String, String> getPrefKeyWhitelist() {
+        return prefKeyWhitelist;
     }
 
-    static void clearValueWhitelist() {
-        existingPreferenceKey.clear();
+    static boolean isInitialized() {
+        return prefKeyWhitelist.size() != 0;
     }
 
-    static void addValueWhitelist(String preferenceKey) {
-        existingPreferenceKey.add(preferenceKey);
+    static void setPrefKeyWhitelist(HashMap<String, String> map) {
+        prefKeyWhitelist = map;
     }
 
     @Override