🤝 Store domain and registry instead of Origin.

The transformation from Origin to Domain and Registry requires native.
If we do this while storing the data in the ClientAppDataRegister when
native will be loaded (native is required for Trusted Web Activities)
we don't need native loaded when reading from the registry.

Bug: 888447
Change-Id: Ibbf94ccbf4dad253752648a45e4c0550edc25dd6
Reviewed-on: https://chromium-review.googlesource.com/c/1292568Reviewed-by: Michael van Ouwerkerk <mvanouwerkerk@chromium.org>
Commit-Queue: Peter Conn <peconn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601558}

chrome/android/java/src/org/chromium/chrome/browser/browserservices/ClearDataBroadcastReceiver.java

@@ -20,9 +20,10 @@ import java.util.Set;
  * corresponding to that app.
  *
  * Trusted Web Activities are registered to an origin (eg https://www.example.com), however because
- * cookies can be scoped more loosely, at TLD+1 level (eg *.example.com) [1], we need to clear data
- * at that level. This unfortunately can lead to too much data getting cleared - for example if the
- * https://peconn.github.io TWA is cleared, you'll loose cookies for https://beverloo.github.io too.
+ * cookies can be scoped more loosely, at 'domain and registry'/TLD+1 level (eg *.example.com) [1],
+ * we need to clear data at that level. This unfortunately can lead to too much data getting cleared
+ * - for example if the https://maps.google.com TWA is cleared, you'll loose cookies for
+ * https://mail.google.com too.
  *
  * We find this acceptable for two reasons:
  * - The alternative is *not* clearing some related data - eg a TWA linked to
@@ -62,12 +63,12 @@ public class ClearDataBroadcastReceiver extends BroadcastReceiver {
         }
 
         String appName = register.getAppNameForRegisteredUid(uid);
-        Set<String> origins = register.getOriginsForRegisteredUid(uid);
+        Set<String> domainAndRegistries = register.getDomainAndRegistriesForRegisteredUid(uid);
 
-        for (String origin : origins) {
+        for (String domainAndRegistry : domainAndRegistries) {
             String action = Intent.ACTION_PACKAGE_FULLY_REMOVED.equals(intent.getAction())
                     ? "been uninstalled" : "had its data cleared";
-            Log.d(TAG, "%s has just %s, it was linked to %s.", appName, action, origin);
+            Log.d(TAG, "%s has just %s, it was linked to %s.", appName, action, domainAndRegistry);
         }
     }
 }

chrome/android/java/src/org/chromium/chrome/browser/browserservices/ClientAppDataRecorder.java

@@ -9,20 +9,23 @@ import android.content.pm.PackageManager;
 import android.text.TextUtils;
 
 import org.chromium.base.Log;
+import org.chromium.chrome.browser.util.UrlUtilities;
 
 import java.util.HashSet;
 import java.util.Set;
 
 /**
  * Takes care of recording that Chrome contains data for the client app in the
- * {@link ClientAppDataRegister}. It performs two main duties:
+ * {@link ClientAppDataRegister}. It performs three main duties:
  * - Holds a cache to deduplicate requests (for performance not correctness).
  * - Transforming the package name into a uid and app label.
+ * - Transforming the origin into a domain and registry (requires native).
  *
  * Lifecycle: There should be a 1-1 relationship between this class and
  * {@link TrustedWebActivityUi}. Having more instances won't effect correctness, but will limit the
  * performance benefits of the cache.
  * Thread safety: All methods on this class should be called from the same thread.
+ * Native: The default {@link UrlTransformer} requires native to be loaded.
  */
 public class ClientAppDataRecorder {
     private static final String TAG = "TWAClientAppData";
@@ -30,23 +33,40 @@ public class ClientAppDataRecorder {
 
     /** Underlying data register. */
     private final ClientAppDataRegister mClientAppDataRegister;
+    private final UrlTransformer mUrlTransformer;
 
     /**
      * Cache so we don't send the same request multiple times. {@link #register} is called on each
-     * navigation and each call to {@link ClientAppDataRegister#registerPackageForOrigin} modifies
-     * SharedPreferences, so we need to cut down on the number of calls.
+     * navigation and each call to {@link ClientAppDataRegister#registerPackageForDomainAndRegistry}
+     * modifies SharedPreferences, so we need to cut down on the number of calls.
      */
     private final Set<String> mCache = new HashSet<>();
 
+    /** Class to allow mocking native calls in unit tests. */
+    /* package */ static class UrlTransformer {
+        /* package */ String getDomainAndRegistry(Origin origin) {
+            return UrlUtilities.getDomainAndRegistry(
+                    origin.toString(), true /* includePrivateRegistries */);
+        }
+    }
+
+    /** Creates a ClientAppDataRecorder with default dependencies. */
+    public ClientAppDataRecorder(PackageManager packageManager) {
+        this(packageManager, new ClientAppDataRegister(), new UrlTransformer());
+    }
+
+    /** Creates a ClientAppDataRecorder providing all dependencies. */
     public ClientAppDataRecorder(PackageManager packageManager,
-            ClientAppDataRegister clientAppDataRegister) {
+            ClientAppDataRegister clientAppDataRegister, UrlTransformer urlTransformer) {
         mPackageManager = packageManager;
         mClientAppDataRegister = clientAppDataRegister;
+        mUrlTransformer = urlTransformer;
     }
 
     /**
-     * Calls {@link ClientAppDataRegister#registerPackageForOrigin}, looking up the uid and
-     * app name for the |packageName| and deduplicating multiple requests with the same parameters.
+     * Calls {@link ClientAppDataRegister#registerPackageForDomainAndRegistry}, looking up the uid
+     * and app name for the |packageName|, extracting the domain and registry from the origin and
+     * deduplicating multiple requests with the same parameters.
      */
     /* package */ void register(String packageName, Origin origin) {
         if (mCache.contains(combine(packageName, origin))) return;
@@ -62,8 +82,11 @@ public class ClientAppDataRecorder {
                 return;
             }
 
-            Log.d(TAG, "Registering %d (%s) for %s", ai.uid, appLabel, origin);
-            mClientAppDataRegister.registerPackageForOrigin(ai.uid, appLabel, origin);
+            String domainAndRegistry = mUrlTransformer.getDomainAndRegistry(origin);
+
+            Log.d(TAG, "Registering %d (%s) for %s", ai.uid, appLabel, domainAndRegistry);
+            mClientAppDataRegister.registerPackageForDomainAndRegistry(
+                    ai.uid, appLabel, domainAndRegistry);
         } catch (PackageManager.NameNotFoundException e) {
             Log.e(TAG, "Couldn't find name for client package %s", packageName);
         }

chrome/android/java/src/org/chromium/chrome/browser/browserservices/ClientAppDataRegister.java

@@ -40,9 +40,10 @@ public class ClientAppDataRegister {
     /**
      * Saves to Preferences that the app with |uid| has the application name |appName| and when it
      * is removed or cleared, we should consider doing the same with Chrome data relevant to
-     * |origin|.
+     * |domainAndRegistry|.
      */
-    /* package */ void registerPackageForOrigin(int uid, String appName, Origin origin) {
+    /* package */ void registerPackageForDomainAndRegistry(
+            int uid, String appName, String domainAndRegistry) {
         // Store the UID in the main Chrome Preferences.
         Set<String> uids = getUids();
         uids.add(String.valueOf(uid));
@@ -51,11 +52,12 @@ public class ClientAppDataRegister {
         // Store the package name for the UID.
         mPreferences.edit().putString(createAppNameKey(uid), appName).apply();
 
-        // Store the origin for the UID.
-        String key = createOriginsKey(uid);
-        Set<String> origins = new HashSet<>(mPreferences.getStringSet(key, Collections.emptySet()));
-        origins.add(origin.toString());
-        mPreferences.edit().putStringSet(key, origins).apply();
+        // Store the domainAndRegistry for the UID.
+        String key = createDomainAndRegistriesKey(uid);
+        Set<String> domainAndRegistries =
+                new HashSet<>(mPreferences.getStringSet(key, Collections.emptySet()));
+        domainAndRegistries.add(domainAndRegistry);
+        mPreferences.edit().putStringSet(key, domainAndRegistries).apply();
     }
 
     private void setUids(Set<String> uids) {
@@ -72,7 +74,7 @@ public class ClientAppDataRegister {
         setUids(uids);
 
         mPreferences.edit().putString(createAppNameKey(uid), null).apply();
-        mPreferences.edit().putStringSet(createOriginsKey(uid), null).apply();
+        mPreferences.edit().putStringSet(createDomainAndRegistriesKey(uid), null).apply();
     }
 
     /* package */ boolean chromeHoldsDataForPackage(int uid) {
@@ -87,11 +89,11 @@ public class ClientAppDataRegister {
     }
 
     /**
-     * Gets all the origins that have been registered for the uid.
+     * Gets all the 'domain and registry's that have been registered for the uid.
      * Do not modify the set returned by this method.
      */
-    /* package */ Set<String> getOriginsForRegisteredUid(int uid) {
-        return mPreferences.getStringSet(createOriginsKey(uid), Collections.emptySet());
+    /* package */ Set<String> getDomainAndRegistriesForRegisteredUid(int uid) {
+        return mPreferences.getStringSet(createDomainAndRegistriesKey(uid), Collections.emptySet());
     }
 
     /**
@@ -103,10 +105,10 @@ public class ClientAppDataRegister {
     }
 
     /**
-     * Creates the Preferences key to access the set of origins for an app.
+     * Creates the Preferences key to access the set of 'domain and registry's for an app.
      * If you modify this you'll have to migrate old data.
      */
-    private static String createOriginsKey(int uid) {
-        return uid + ".origins";
+    private static String createDomainAndRegistriesKey(int uid) {
+        return uid + ".domainAndRegistry";
     }
 }

chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java

@@ -66,7 +66,6 @@ import org.chromium.chrome.browser.autofill_assistant.AutofillAssistantUiControl
 import org.chromium.chrome.browser.browserservices.BrowserSessionContentHandler;
 import org.chromium.chrome.browser.browserservices.BrowserSessionContentUtils;
 import org.chromium.chrome.browser.browserservices.ClientAppDataRecorder;
-import org.chromium.chrome.browser.browserservices.ClientAppDataRegister;
 import org.chromium.chrome.browser.browserservices.TrustedWebActivityUi;
 import org.chromium.chrome.browser.compositor.layouts.LayoutManager;
 import org.chromium.chrome.browser.contextual_suggestions.ContextualSuggestionsModule;
@@ -328,7 +327,7 @@ public class CustomTabActivity extends ChromeActivity<CustomTabActivityComponent
                         return CustomTabActivity.this.getSnackbarManager();
                     }
                 }, getComponent().getTrustedWebActivityDisclosure(),
-                new ClientAppDataRecorder(getPackageManager(), new ClientAppDataRegister()));
+                new ClientAppDataRecorder(getPackageManager()));
     }
 
     /**

chrome/android/junit/src/org/chromium/chrome/browser/browserservices/ClientAppDataRecorderTest.java

@@ -43,6 +43,18 @@ public class ClientAppDataRecorderTest {
     @Mock public PackageManager mPackageManager;
 
     private ClientAppDataRecorder mRecorder;
+    private final ClientAppDataRecorder.UrlTransformer mUrlTransformer =
+            new ClientAppDataRecorder.UrlTransformer() {
+                @Override
+                String getDomainAndRegistry(Origin origin) {
+                    return transform(origin);
+                }
+            };
+
+    private static String transform(Origin origin) {
+        // Just an arbitrary string transformation so we can check it is applied.
+        return origin.toString().toUpperCase();
+    }
 
     @Before
     public void setUp() throws PackageManager.NameNotFoundException {
@@ -60,7 +72,7 @@ public class ClientAppDataRecorderTest {
                 .when(mPackageManager)
                 .getApplicationInfo(eq(MISSING_PACKAGE), anyInt());
 
-        mRecorder = new ClientAppDataRecorder(mPackageManager, mRegister);
+        mRecorder = new ClientAppDataRecorder(mPackageManager, mRegister, mUrlTransformer);
     }
 
 
@@ -68,7 +80,7 @@ public class ClientAppDataRecorderTest {
     @Feature("TrustedWebActivities")
     public void testRegister() {
         mRecorder.register(APP_PACKAGE, ORIGIN);
-        verify(mRegister).registerPackageForOrigin(APP_UID, APP_NAME, ORIGIN);
+        verify(mRegister).registerPackageForDomainAndRegistry(APP_UID, APP_NAME, transform(ORIGIN));
     }
 
     @Test
@@ -76,7 +88,7 @@ public class ClientAppDataRecorderTest {
     public void testDeduplicate() {
         mRecorder.register(APP_PACKAGE, ORIGIN);
         mRecorder.register(APP_PACKAGE, ORIGIN);
-        verify(mRegister).registerPackageForOrigin(APP_UID, APP_NAME, ORIGIN);
+        verify(mRegister).registerPackageForDomainAndRegistry(APP_UID, APP_NAME, transform(ORIGIN));
     }
 
     @Test
@@ -84,8 +96,9 @@ public class ClientAppDataRecorderTest {
     public void testDifferentOrigins() {
         mRecorder.register(APP_PACKAGE, ORIGIN);
         mRecorder.register(APP_PACKAGE, OTHER_ORIGIN);
-        verify(mRegister).registerPackageForOrigin(APP_UID, APP_NAME, ORIGIN);
-        verify(mRegister).registerPackageForOrigin(APP_UID, APP_NAME, OTHER_ORIGIN);
+        verify(mRegister).registerPackageForDomainAndRegistry(APP_UID, APP_NAME, transform(ORIGIN));
+        verify(mRegister).registerPackageForDomainAndRegistry(
+                APP_UID, APP_NAME, transform(OTHER_ORIGIN));
     }
 
     @Test
@@ -93,6 +106,7 @@ public class ClientAppDataRecorderTest {
     public void testMisingPackage() {
         mRecorder.register(MISSING_PACKAGE, ORIGIN);
         // Implicitly checking we don't throw.
-        verify(mRegister, times(0)).registerPackageForOrigin(anyInt(), anyString(), any());
+        verify(mRegister, times(0))
+                .registerPackageForDomainAndRegistry(anyInt(), anyString(), any());
     }
 }

chrome/android/junit/src/org/chromium/chrome/browser/browserservices/ClientAppDataRegisterTest.java

@@ -23,8 +23,8 @@ import java.util.Set;
 public class ClientAppDataRegisterTest {
     private static final int UID = 23;
     private static final String APP_NAME = "Example App";
-    private static final Origin ORIGIN = new Origin("https://www.example.com/");
-    private static final Origin OTHER_ORIGIN = new Origin("https://other.example.com/");
+    private static final String DOMAIN_AND_REGISTRY = "example.com";
+    private static final String OTHER_DOMAIN_AND_REGISTRY = "otherexample.com";
 
     private ClientAppDataRegister mRegister;
 
@@ -35,8 +35,8 @@ public class ClientAppDataRegisterTest {
 
     @Test
     @Feature("TrustedWebActivities")
-    public void testRegistration() {
-        mRegister.registerPackageForOrigin(UID, APP_NAME, ORIGIN);
+    public void registration() {
+        mRegister.registerPackageForDomainAndRegistry(UID, APP_NAME, DOMAIN_AND_REGISTRY);
 
         Assert.assertTrue(mRegister.chromeHoldsDataForPackage(UID));
         Assert.assertEquals(APP_NAME, mRegister.getAppNameForRegisteredUid(UID));
@@ -44,8 +44,8 @@ public class ClientAppDataRegisterTest {
 
     @Test
     @Feature("TrustedWebActivities")
-    public void testDeregistration() {
-        mRegister.registerPackageForOrigin(UID, APP_NAME, ORIGIN);
+    public void deregistration() {
+        mRegister.registerPackageForDomainAndRegistry(UID, APP_NAME, DOMAIN_AND_REGISTRY);
         mRegister.removePackage(UID);
 
         Assert.assertFalse(mRegister.chromeHoldsDataForPackage(UID));
@@ -54,24 +54,31 @@ public class ClientAppDataRegisterTest {
 
     @Test
     @Feature("TrustedWebActivities")
-    public void testGetOrigins() {
-        mRegister.registerPackageForOrigin(UID, APP_NAME, ORIGIN);
-        mRegister.registerPackageForOrigin(UID, APP_NAME, OTHER_ORIGIN);
+    public void getOrigins() {
+        mRegister.registerPackageForDomainAndRegistry(UID, APP_NAME, DOMAIN_AND_REGISTRY);
+        mRegister.registerPackageForDomainAndRegistry(UID, APP_NAME, OTHER_DOMAIN_AND_REGISTRY);
 
-        Set<String> origins = mRegister.getOriginsForRegisteredUid(UID);
+        Set<String> origins = mRegister.getDomainAndRegistriesForRegisteredUid(UID);
         Assert.assertEquals(2, origins.size());
-        Assert.assertTrue(origins.contains(ORIGIN.toString()));
-        Assert.assertTrue(origins.contains(OTHER_ORIGIN.toString()));
+        Assert.assertTrue(origins.contains(DOMAIN_AND_REGISTRY));
+        Assert.assertTrue(origins.contains(OTHER_DOMAIN_AND_REGISTRY));
     }
 
     @Test
     @Feature("TrustedWebActivities")
-    public void testClearOrigins() {
-        mRegister.registerPackageForOrigin(UID, APP_NAME, ORIGIN);
-        mRegister.registerPackageForOrigin(UID, APP_NAME, OTHER_ORIGIN);
+    public void clearOrigins() {
+        mRegister.registerPackageForDomainAndRegistry(UID, APP_NAME, DOMAIN_AND_REGISTRY);
+        mRegister.registerPackageForDomainAndRegistry(UID, APP_NAME, OTHER_DOMAIN_AND_REGISTRY);
         mRegister.removePackage(UID);
 
-        Set<String> origins = mRegister.getOriginsForRegisteredUid(UID);
+        Set<String> origins = mRegister.getDomainAndRegistriesForRegisteredUid(UID);
         Assert.assertTrue(origins.isEmpty());
     }
+
+    @Test
+    @Feature("TrustedWebActivities")
+    public void getAppName() {
+        mRegister.registerPackageForDomainAndRegistry(UID, APP_NAME, DOMAIN_AND_REGISTRY);
+        Assert.assertEquals(APP_NAME, mRegister.getAppNameForRegisteredUid(UID));
+    }
 }