Support grandfathered prefixes in ChromePreferenceKeys.

Some SharedPreferences keys used in the past are dynamic. For example,
in PaymentPreferencesUtil, keys of the format
"payment_instrument_use_count_[type]" are generated.

These keys should be usable by SharedPreferencesManager, which will
only allow whitelisted keys to be used. This CL creates a whitelist of
dynamic key prefixes grandfathered in because they do not follow the
format "Chrome.[Feature].[KeyPrefix].*".

Bug: 1022108
Change-Id: Iaf9ebf170c339cc4b6d13705d515c97eef9087cd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1981262
Commit-Queue: Henrique Nakashima <hnakashima@chromium.org>
Reviewed-by: Natalie Chouinard <chouinard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#729864}

chrome/browser/preferences/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceKeyChecker.java

@@ -25,6 +25,7 @@ class ChromePreferenceKeyChecker extends BaseChromePreferenceKeyChecker {
 
     private Set<String> mKeysInUse;
     private Set<String> mGrandfatheredFormatKeys;
+    private List<KeyPrefix> mGrandfatheredPrefixes;
     private Pattern mDynamicPartPattern;
 
     /**
@@ -33,16 +34,19 @@ class ChromePreferenceKeyChecker extends BaseChromePreferenceKeyChecker {
      */
     private ChromePreferenceKeyChecker() {
         this(ChromePreferenceKeys.createKeysInUse(),
-                ChromePreferenceKeys.createGrandfatheredKeysInUse());
+                ChromePreferenceKeys.createGrandfatheredKeysInUse(),
+                ChromePreferenceKeys.createGrandfatheredPrefixesInUse());
     }
 
     /**
      * Generic constructor, dependencies are passed in.
      */
     @VisibleForTesting
-    ChromePreferenceKeyChecker(List<String> keysInUse, List<String> grandfatheredKeys) {
+    ChromePreferenceKeyChecker(List<String> keysInUse, List<String> grandfatheredKeys,
+            List<KeyPrefix> grandfatheredPrefixes) {
         mKeysInUse = new HashSet<>(keysInUse);
         mGrandfatheredFormatKeys = new HashSet<>(grandfatheredKeys);
+        mGrandfatheredPrefixes = grandfatheredPrefixes;
 
         // The dynamic part cannot be empty, but otherwise it is anything that does not contain
         // stars.
@@ -72,11 +76,18 @@ class ChromePreferenceKeyChecker extends BaseChromePreferenceKeyChecker {
      * @return Whether |key| is in use.
      */
     private boolean isKeyInUse(String key) {
-        // Grandfathered keys cannot be dynamic, so a simple map check is enough.
+        // For non-dynamic grandfathered keys, a simple map check is enough.
         if (mGrandfatheredFormatKeys.contains(key)) {
             return true;
         }
 
+        // For dynamic grandfathered keys, each grandfathered prefix has to be checked.
+        for (KeyPrefix prefix : mGrandfatheredPrefixes) {
+            if (prefix.hasGenerated(key)) {
+                return true;
+            }
+        }
+
         // If not a format-grandfathered key, assume it follows the format and find out if it is
         // a prefixed key.
         String[] parts = key.split("\\.", 4);

chrome/browser/preferences/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceKeyCheckerTest.java

@@ -30,6 +30,7 @@ public class ChromePreferenceKeyCheckerTest {
     private static final KeyPrefix KEY_PREFIX3_NOT_IN_USE =
             new KeyPrefix("Chrome.Feature.KeyPrefix3.*");
     private static final String GRANDFATHERED_KEY_IN_USE = "grandfatheredkey";
+    private static final String GRANDFATHERED_PREFIX_IN_USE = "grandfatheredprefix_";
 
     private ChromePreferenceKeyChecker mSubject;
 
@@ -38,7 +39,10 @@ public class ChromePreferenceKeyCheckerTest {
         List<String> keysInUse = Arrays.asList(KEY1_IN_USE, KEY2_IN_USE,
                 KEY_PREFIX1_IN_USE.pattern(), KEY_PREFIX2_IN_USE.pattern());
         List<String> grandfatheredKeys = Arrays.asList(GRANDFATHERED_KEY_IN_USE);
-        mSubject = new ChromePreferenceKeyChecker(keysInUse, grandfatheredKeys);
+        List<KeyPrefix> grandfatheredPrefixes =
+                Arrays.asList(new KeyPrefix(GRANDFATHERED_PREFIX_IN_USE + "*"));
+        mSubject =
+                new ChromePreferenceKeyChecker(keysInUse, grandfatheredKeys, grandfatheredPrefixes);
     }
 
     @Test
@@ -47,6 +51,7 @@ public class ChromePreferenceKeyCheckerTest {
         mSubject.checkIsKeyInUse(KEY1_IN_USE);
         mSubject.checkIsKeyInUse(KEY2_IN_USE);
         mSubject.checkIsKeyInUse(GRANDFATHERED_KEY_IN_USE);
+        mSubject.checkIsKeyInUse(GRANDFATHERED_PREFIX_IN_USE + "restofkey");
     }
 
     @Test(expected = RuntimeException.class)

chrome/browser/preferences/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceKeys.java

@@ -7,6 +7,7 @@ package org.chromium.chrome.browser.preferences;
 import org.chromium.base.annotations.CheckDiscard;
 
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 
 /**
@@ -695,5 +696,15 @@ public final class ChromePreferenceKeys {
         // clang-format on
     }
 
+    @CheckDiscard("Validation is performed in tests and in debug builds.")
+    static List<KeyPrefix> createGrandfatheredPrefixesInUse() {
+        return Collections.EMPTY_LIST;
+    }
+
+    @CheckDiscard("Validation is performed in tests and in debug builds.")
+    static List<KeyPrefix> createDeprecatedPrefixesForTesting() {
+        return Collections.EMPTY_LIST;
+    }
+
     private ChromePreferenceKeys() {}
 }

chrome/browser/preferences/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceKeysTest.java

@@ -16,6 +16,7 @@ import org.junit.runner.RunWith;
 
 import org.chromium.base.test.BaseRobolectricTestRunner;
 
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashSet;
@@ -41,11 +42,14 @@ public class ChromePreferenceKeysTest {
     public void testKeysAreNotReused() {
         doTestKeysAreNotReused(ChromePreferenceKeys.createKeysInUse(),
                 ChromePreferenceKeys.createGrandfatheredKeysInUse(),
-                ChromePreferenceKeys.createDeprecatedKeysForTesting());
+                ChromePreferenceKeys.createDeprecatedKeysForTesting(),
+                ChromePreferenceKeys.createGrandfatheredPrefixesInUse(),
+                ChromePreferenceKeys.createDeprecatedPrefixesForTesting());
     }
 
     private void doTestKeysAreNotReused(List<String> usedList, List<String> grandfatheredUsedList,
-            List<String> deprecatedList) {
+            List<String> deprecatedList, List<KeyPrefix> usedGrandfatheredPrefixList,
+            List<KeyPrefix> deprecatedGrandfatheredPrefixList) {
         // Check for duplicate keys in [keys in use].
         Set<String> usedSet = new HashSet<>(usedList);
         assertEquals(usedList.size(), usedSet.size());
@@ -77,6 +81,16 @@ public class ChromePreferenceKeysTest {
                     + "\" is both in ChromePreferenceKeys' [keys in use] and in "
                     + "[deprecated keys]");
         }
+
+        // Check for keys that match a grandfathered prefix, deprecated or not.
+        List<KeyPrefix> grandfatheredPrefixes = new ArrayList<>(usedGrandfatheredPrefixList);
+        grandfatheredPrefixes.addAll(deprecatedGrandfatheredPrefixList);
+
+        for (String usedKey : usedSet) {
+            for (KeyPrefix grandfatheredPrefix : grandfatheredPrefixes) {
+                assertFalse(grandfatheredPrefix.hasGenerated(usedKey));
+            }
+        }
     }
 
     // Below are tests to ensure that testKeysAreNotReused() works.
@@ -84,7 +98,7 @@ public class ChromePreferenceKeysTest {
     @Test
     @SmallTest
     public void testReuseCheck_emptyLists() {
-        doTestKeysAreNotReused(
+        doTestKeysAreNotReused(Collections.EMPTY_LIST, Collections.EMPTY_LIST,
                 Collections.EMPTY_LIST, Collections.EMPTY_LIST, Collections.EMPTY_LIST);
     }
 
@@ -92,21 +106,23 @@ public class ChromePreferenceKeysTest {
     @SmallTest
     public void testReuseCheck_duplicateKey_used() {
         doTestKeysAreNotReused(Arrays.asList("UsedKey1", "UsedKey1"), Collections.EMPTY_LIST,
-                Collections.EMPTY_LIST);
+                Collections.EMPTY_LIST, Collections.EMPTY_LIST, Collections.EMPTY_LIST);
     }
 
     @Test(expected = AssertionError.class)
     @SmallTest
     public void testReuseCheck_duplicateKey_grandfathered() {
         doTestKeysAreNotReused(Collections.EMPTY_LIST,
-                Arrays.asList("GrandfatheredKey1", "GrandfatheredKey1"), Collections.EMPTY_LIST);
+                Arrays.asList("GrandfatheredKey1", "GrandfatheredKey1"), Collections.EMPTY_LIST,
+                Collections.EMPTY_LIST, Collections.EMPTY_LIST);
     }
 
     @Test(expected = AssertionError.class)
     @SmallTest
     public void testReuseCheck_duplicateKey_deprecated() {
         doTestKeysAreNotReused(Collections.EMPTY_LIST, Collections.EMPTY_LIST,
-                Arrays.asList("DeprecatedKey1", "DeprecatedKey1"));
+                Arrays.asList("DeprecatedKey1", "DeprecatedKey1"), Collections.EMPTY_LIST,
+                Collections.EMPTY_LIST);
     }
 
     @Test
@@ -114,21 +130,27 @@ public class ChromePreferenceKeysTest {
     public void testReuseCheck_noIntersection() {
         doTestKeysAreNotReused(Arrays.asList("UsedKey1", "UsedKey2"),
                 Arrays.asList("GrandfatheredKey1", "GrandfatheredKey2"),
-                Arrays.asList("DeprecatedKey1", "DeprecatedKey2"));
+                Arrays.asList("DeprecatedKey1", "DeprecatedKey2"),
+                Arrays.asList(new KeyPrefix("UsedGrandfatheredFormat1*"),
+                        new KeyPrefix("UsedGrandfatheredFormat2*")),
+                Arrays.asList(
+                        new KeyPrefix("DeprecatedFormat1*"), new KeyPrefix("DeprecatedFormat2*")));
     }
 
     @Test(expected = AssertionError.class)
     @SmallTest
     public void testReuseCheck_intersectionUsedAndGrandfathered() {
         doTestKeysAreNotReused(Arrays.asList("ReusedKey", "UsedKey1"),
-                Arrays.asList("GrandfatheredKey1", "ReusedKey"), Collections.EMPTY_LIST);
+                Arrays.asList("GrandfatheredKey1", "ReusedKey"), Collections.EMPTY_LIST,
+                Collections.EMPTY_LIST, Collections.EMPTY_LIST);
     }
 
     @Test(expected = AssertionError.class)
     @SmallTest
     public void testReuseCheck_intersectionUsedAndDeprecated() {
         doTestKeysAreNotReused(Arrays.asList("UsedKey1", "ReusedKey"), Collections.EMPTY_LIST,
-                Arrays.asList("ReusedKey", "DeprecatedKey1"));
+                Arrays.asList("ReusedKey", "DeprecatedKey1"), Collections.EMPTY_LIST,
+                Collections.EMPTY_LIST);
     }
 
     @Test(expected = AssertionError.class)
@@ -136,7 +158,24 @@ public class ChromePreferenceKeysTest {
     public void testReuseCheck_intersectionGrandfatheredAndDeprecated() {
         doTestKeysAreNotReused(Collections.EMPTY_LIST,
                 Arrays.asList("GrandfatheredKey1", "ReusedKey"),
-                Arrays.asList("ReusedKey", "DeprecatedKey1"));
+                Arrays.asList("ReusedKey", "DeprecatedKey1"), Collections.EMPTY_LIST,
+                Collections.EMPTY_LIST);
+    }
+
+    @Test(expected = AssertionError.class)
+    @SmallTest
+    public void testReuseCheck_intersectionUsedGrandfatheredFormat_prefix() {
+        doTestKeysAreNotReused(Arrays.asList("UsedKey1"), Collections.EMPTY_LIST,
+                Collections.EMPTY_LIST, Arrays.asList(new KeyPrefix("UsedKey*")),
+                Collections.EMPTY_LIST);
+    }
+
+    @Test(expected = AssertionError.class)
+    @SmallTest
+    public void testReuseCheck_intersectionDeprecatedGrandfatheredFormat_prefix() {
+        doTestKeysAreNotReused(Arrays.asList("UsedKey1"), Collections.EMPTY_LIST,
+                Collections.EMPTY_LIST, Collections.EMPTY_LIST,
+                Arrays.asList(new KeyPrefix("Used*")));
     }
 
     /**
@@ -220,14 +259,14 @@ public class ChromePreferenceKeysTest {
 
     @Test(expected = AssertionError.class)
     @SmallTest
-    public void testFormatCheck_MissingFeature() {
+    public void testFormatCheck_missingFeature() {
         doTestKeysConformToFormat(Arrays.asList(
                 TestFormatConstantsClass.NEW1, TestFormatConstantsClass.MISSING_FEATURE));
     }
 
     @Test(expected = AssertionError.class)
     @SmallTest
-    public void testFormatCheck_LowercaseKey() {
+    public void testFormatCheck_lowercaseKey() {
         doTestKeysConformToFormat(Arrays.asList(
                 TestFormatConstantsClass.NEW1, TestFormatConstantsClass.LOWERCASE_KEY));
     }

chrome/browser/preferences/android/java/src/org/chromium/chrome/browser/preferences/KeyPrefix.java

@@ -14,7 +14,7 @@ public class KeyPrefix {
 
     KeyPrefix(String pattern) {
         // More thorough checking is performed in ChromePreferenceKeysTest.
-        assert pattern.endsWith(".*");
+        assert pattern.endsWith("*");
         mPrefix = pattern.substring(0, pattern.length() - 1);
     }
 
@@ -29,4 +29,8 @@ public class KeyPrefix {
     String pattern() {
         return mPrefix + "*";
     }
+
+    boolean hasGenerated(String key) {
+        return key.startsWith(mPrefix);
+    }
 }

chrome/browser/preferences/android/java/src/org/chromium/chrome/browser/preferences/KeyPrefixTest.java

@@ -5,6 +5,8 @@
 package org.chromium.chrome.browser.preferences;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 
 import android.support.test.filters.SmallTest;
 
@@ -24,8 +26,25 @@ public class KeyPrefixTest {
         KeyPrefix prefix = new KeyPrefix("Chrome.Feature.KP.*");
 
         assertEquals(prefix.pattern(), "Chrome.Feature.KP.*");
+
         assertEquals(prefix.createKey("DynamicKey"), "Chrome.Feature.KP.DynamicKey");
         assertEquals(prefix.createKey("Level.DynamicKey"), "Chrome.Feature.KP.Level.DynamicKey");
+
+        assertTrue(prefix.hasGenerated("Chrome.Feature.KP.DynamicKey"));
+        assertTrue(prefix.hasGenerated("Chrome.Feature.KP.Level.DynamicKey"));
+        assertFalse(prefix.hasGenerated("OtherKey"));
+    }
+
+    @Test
+    @SmallTest
+    public void testSuccess_validGrandfatheredPattern() {
+        KeyPrefix prefix = new KeyPrefix("grandfathered_pattern_*");
+
+        assertEquals(prefix.pattern(), "grandfathered_pattern_*");
+        assertEquals(prefix.createKey("DynamicKey"), "grandfathered_pattern_DynamicKey");
+
+        assertTrue(prefix.hasGenerated("grandfathered_pattern_DynamicKey"));
+        assertFalse(prefix.hasGenerated("OtherKey"));
     }
 
     @Test(expected = AssertionError.class)
@@ -39,10 +58,4 @@ public class KeyPrefixTest {
     public void testError_missingStar() {
         new KeyPrefix("Chrome.Feature.KP.");
     }
-
-    @Test(expected = AssertionError.class)
-    @SmallTest
-    public void testError_extraStar() {
-        new KeyPrefix("Chrome.Feature.KP.**");
-    }
 }