Fix trusted vault key retrieval notification

The previous implementation of key retrieval notification on Android
didn't notify TrustedVaultClient about changed keys upon completion.

In order to detect completion of key retrieval pending intent, this CL
adds TrustedVaultKeyRetrievalProxyActivity that just launches the real
key retrieval pending intent, detects its completion and notifies
TrustedVaultClient about changed keys. This activity doesn't require
the Chrome browser process to be running when the user taps on
notification.

In order to avoid too many pending intents being created for this
notification it's not allowed to create a new notification if one is
already exists or being created.

To ensure that newest real pending intent used for new notification,
this CL also populates FLAG_UPDATE_CURRENT into PendingIntentProvider
for Sync notifications.

Bug: 1071377
Change-Id: Ibba3c9f02aa0bd233c128d98aa194f781a49d339
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2179547
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: Theresa  <twellington@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#770770}

chrome/android/chrome_java_sources.gni

@@ -1510,6 +1510,7 @@ chrome_java_sources = [
   "java/src/org/chromium/chrome/browser/sync/ui/PassphraseCreationDialogFragment.java",
   "java/src/org/chromium/chrome/browser/sync/ui/PassphraseDialogFragment.java",
   "java/src/org/chromium/chrome/browser/sync/ui/PassphraseTypeDialogFragment.java",
+  "java/src/org/chromium/chrome/browser/sync/ui/TrustedVaultKeyRetrievalProxyActivity.java",
   "java/src/org/chromium/chrome/browser/tab/AccessibilityVisibilityHandler.java",
   "java/src/org/chromium/chrome/browser/tab/AuthenticatorNavigationInterceptorTabHelper.java",
   "java/src/org/chromium/chrome/browser/tab/InterceptNavigationDelegateClientImpl.java",

chrome/android/java/AndroidManifest.xml

@@ -522,6 +522,11 @@ by a child template that "extends" this file.
             {{ self.extra_web_rendering_activity_definitions() }}
         </activity>
 
+        <activity android:name="org.chromium.chrome.browser.sync.ui.TrustedVaultKeyRetrievalProxyActivity"
+            android:theme="@android:style/Theme.NoDisplay"
+            android:excludeFromRecents="true"
+            android:exported="false">
+        </activity>
         <activity android:name="org.chromium.chrome.browser.sync.ui.PassphraseActivity"
             android:theme="@style/Theme.Chromium.Activity"
             android:autoRemoveFromRecents="true">

chrome/android/java/monochrome_public_bundle__base_bundle_module.AndroidManifest.expected

@@ -504,6 +504,11 @@
         android:name="org.chromium.chrome.browser.instantapps.AuthenticatedProxyActivity"
         android:noHistory="true"
         android:theme="@android:style/Theme.NoDisplay"/>
+    <activity
+        android:excludeFromRecents="true"
+        android:exported="false"
+        android:name="org.chromium.chrome.browser.sync.ui.TrustedVaultKeyRetrievalProxyActivity"
+        android:theme="@android:style/Theme.NoDisplay"/>
     <activity
         android:excludeFromRecents="true"
         android:name="org.chromium.chrome.browser.webapps.WebappLauncherActivity"

chrome/android/java/src/org/chromium/chrome/browser/sync/SyncNotificationController.java

@@ -4,14 +4,15 @@
 
 package org.chromium.chrome.browser.sync;
 
+import android.app.PendingIntent;
 import android.content.Context;
 import android.content.Intent;
 import android.os.Build;
-import android.util.Log;
 
 import androidx.annotation.StringRes;
 
 import org.chromium.base.ContextUtils;
+import org.chromium.base.Log;
 import org.chromium.base.ThreadUtils;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.notifications.NotificationBuilderFactory;
@@ -24,6 +25,7 @@ import org.chromium.chrome.browser.signin.IdentityServicesProvider;
 import org.chromium.chrome.browser.sync.GoogleServiceAuthError.State;
 import org.chromium.chrome.browser.sync.settings.SyncAndServicesSettings;
 import org.chromium.chrome.browser.sync.ui.PassphraseActivity;
+import org.chromium.chrome.browser.sync.ui.TrustedVaultKeyRetrievalProxyActivity;
 import org.chromium.components.browser_ui.notifications.ChromeNotification;
 import org.chromium.components.browser_ui.notifications.ChromeNotificationBuilder;
 import org.chromium.components.browser_ui.notifications.NotificationManagerProxy;
@@ -40,9 +42,10 @@ import org.chromium.components.sync.PassphraseType;
  * regarding the user sync status.
  */
 public class SyncNotificationController implements ProfileSyncService.SyncStateChangedListener {
-    private static final String TAG = "SyncNotificationController";
+    private static final String TAG = "SyncUI";
     private final NotificationManagerProxy mNotificationManager;
     private final ProfileSyncService mProfileSyncService;
+    private boolean mTrustedVaultNotificationShownOrCreating = false;
 
     public SyncNotificationController() {
         mNotificationManager =
@@ -60,7 +63,7 @@ public class SyncNotificationController implements ProfileSyncService.SyncStateC
 
         // Auth errors take precedence over passphrase errors.
         if (!AndroidSyncSettings.get().isSyncEnabled()) {
-            mNotificationManager.cancel(NotificationConstants.NOTIFICATION_ID_SYNC);
+            cancelNotifications();
             return;
         }
         if (shouldSyncAuthErrorBeShown()) {
@@ -85,38 +88,25 @@ public class SyncNotificationController implements ProfileSyncService.SyncStateC
                     return;
                 case PassphraseType.KEYSTORE_PASSPHRASE: // Falling through intentionally.
                 default:
-                    mNotificationManager.cancel(NotificationConstants.NOTIFICATION_ID_SYNC);
+                    cancelNotifications();
                     return;
             }
         } else if (mProfileSyncService.isEngineInitialized()
                 && mProfileSyncService.isTrustedVaultKeyRequiredForPreferredDataTypes()) {
-            CoreAccountInfo primaryAccountInfo =
-                    IdentityServicesProvider.get().getIdentityManager().getPrimaryAccountInfo(
-                            ConsentLevel.SYNC);
-            if (primaryAccountInfo != null) {
-                int flags = Intent.FLAG_ACTIVITY_NEW_TASK | Intent.FLAG_ACTIVITY_CLEAR_TOP;
-                // TODO(crbug.com/1012659): Upon intent completion, the new keys should be fetched.
-                TrustedVaultClient.get()
-                        .createKeyRetrievalIntent(primaryAccountInfo)
-                        .then(
-                                (pendingIntent)
-                                        -> {
-                                    showSyncNotificationForPendingIntent(
-                                            mProfileSyncService.isEncryptEverythingEnabled()
-                                                    ? R.string.sync_error_card_title
-                                                    : R.string.sync_passwords_error_card_title,
-                                            new PendingIntentProvider(pendingIntent, flags));
-                                },
-                                (exception) -> {
-                                    Log.w(TAG, "Error creating key retrieval intent: ", exception);
-                                });
-            }
+            maybeCreateKeyRetrievalNotification();
         } else {
-            mNotificationManager.cancel(NotificationConstants.NOTIFICATION_ID_SYNC);
-            return;
+            cancelNotifications();
         }
     }
 
+    /**
+     * Cancels existing notification if there is any.
+     */
+    private void cancelNotifications() {
+        mNotificationManager.cancel(NotificationConstants.NOTIFICATION_ID_SYNC);
+        mTrustedVaultNotificationShownOrCreating = false;
+    }
+
     /**
      * Builds and shows a notification for the |message|.
      *
@@ -173,8 +163,10 @@ public class SyncNotificationController implements ProfileSyncService.SyncStateC
      */
     private void showSyncNotification(@StringRes int message, Intent intent) {
         Context applicationContext = ContextUtils.getApplicationContext();
-        PendingIntentProvider contentIntent =
-                PendingIntentProvider.getActivity(applicationContext, 0, intent, 0);
+        // There might be cached PendingIntent for sync notification and this PendingIntent might
+        // be outdated. FLAG_UPDATE_CURRENT updates cached PendingIntent.
+        PendingIntentProvider contentIntent = PendingIntentProvider.getActivity(
+                applicationContext, 0, intent, PendingIntent.FLAG_UPDATE_CURRENT);
         showSyncNotificationForPendingIntent(message, contentIntent);
     }
 
@@ -223,4 +215,41 @@ public class SyncNotificationController implements ProfileSyncService.SyncStateC
         intent.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP);
         return intent;
     }
+
+    /**
+     * Attempts to asynchronously create and show key retrieval notification if no one is already
+     * created or creating and there is a primary account with SYNC ConsentLevel.
+     */
+    private void maybeCreateKeyRetrievalNotification() {
+        CoreAccountInfo primaryAccountInfo =
+                IdentityServicesProvider.get().getIdentityManager().getPrimaryAccountInfo(
+                        ConsentLevel.SYNC);
+        // Check/set |mTrustedVaultNotificationShownOrCreating| here to ensure notification is not
+        // shown again immediately after cancelling (Sync state might be changed often) and there
+        // is only one asynchronous createKeyRetrievalIntent() attempt at the time triggered by
+        // this function.
+        // TODO(crbug.com/1071377): if the user dismissed the notification, it will reappear only
+        // after browser restart or disable-enable Sync action. This is sub-optimal behavior and
+        // it's better to find a way to show it more often, but not on each Sync state change.
+        if (primaryAccountInfo == null || mTrustedVaultNotificationShownOrCreating) {
+            return;
+        }
+        mTrustedVaultNotificationShownOrCreating = true;
+        TrustedVaultClient.get()
+                .createKeyRetrievalIntent(primaryAccountInfo)
+                .then(
+                        (pendingIntent)
+                                -> {
+                            // TODO(crbug.com/1071377): Sync state might be changed already, so
+                            // this notification won't make sense.
+                            showSyncNotification(mProfileSyncService.isEncryptEverythingEnabled()
+                                            ? R.string.sync_error_card_title
+                                            : R.string.sync_passwords_error_card_title,
+                                    TrustedVaultKeyRetrievalProxyActivity
+                                            .createKeyRetrievalProxyIntent(pendingIntent));
+                        },
+                        (exception) -> {
+                            Log.w(TAG, "Error creating key retrieval intent: ", exception);
+                        });
+    }
 }

chrome/android/java/src/org/chromium/chrome/browser/sync/ui/TrustedVaultKeyRetrievalProxyActivity.java

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.sync.ui;
+
+import android.app.Activity;
+import android.app.PendingIntent;
+import android.content.Intent;
+import android.content.IntentSender;
+import android.os.Bundle;
+
+import org.chromium.base.ContextUtils;
+import org.chromium.base.Log;
+import org.chromium.chrome.browser.init.ChromeBrowserInitializer;
+import org.chromium.chrome.browser.sync.TrustedVaultClient;
+
+/**
+ * {@link TrustedVaultKeyRetrievalProxyActivity} has no own UI and just launches real key retrieval
+ * activity (passed via extra). The reason for using this proxy activity is to detect when real key
+ * retrieval activity finishes and notify TrustedVaultClient about changed keys.
+ */
+public class TrustedVaultKeyRetrievalProxyActivity extends Activity {
+    private static final String KEY_RETRIEVAL_INTENT_NAME = "key_retrieval";
+    private static final String TAG = "SyncUI";
+    private static final int REQUEST_CODE_TRUSTED_VAULT_KEY_RETRIEVAL = 1;
+
+    /**
+     * Creates an intent that launches an TrustedVaultKeyRetrievalProxyActivity.
+     *
+     * @param keyRetrievalIntent Actual key retrieval intent, which will be launched by
+     * TrustedVaultKeyRetrievalProxyActivity.
+     *
+     * @return the intent for launching TrustedVaultKeyRetrievalProxyActivity
+     */
+    public static Intent createKeyRetrievalProxyIntent(PendingIntent keyRetrievalIntent) {
+        Intent intent = new Intent(
+                ContextUtils.getApplicationContext(), TrustedVaultKeyRetrievalProxyActivity.class);
+        intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
+        intent.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP);
+        intent.putExtra(KEY_RETRIEVAL_INTENT_NAME, keyRetrievalIntent);
+        return intent;
+    }
+
+    @Override
+    protected void onCreate(Bundle savedInstanceState) {
+        super.onCreate(savedInstanceState);
+
+        PendingIntent keyRetrievalIntent =
+                getIntent().getParcelableExtra(KEY_RETRIEVAL_INTENT_NAME);
+        assert keyRetrievalIntent != null;
+        try {
+            startIntentSenderForResult(keyRetrievalIntent.getIntentSender(),
+                    REQUEST_CODE_TRUSTED_VAULT_KEY_RETRIEVAL,
+                    /* fillInIntent */ null, /* flagsMask */ 0,
+                    /* flagsValues */ 0, /* extraFlags */ 0,
+                    /* options */ null);
+        } catch (IntentSender.SendIntentException exception) {
+            Log.w(TAG, "Error sending key retrieval intent: ", exception);
+        }
+    }
+
+    @Override
+    protected void onActivityResult(int requestCode, int resultCode, Intent data) {
+        assert requestCode == REQUEST_CODE_TRUSTED_VAULT_KEY_RETRIEVAL;
+
+        // Upon key retrieval completion, the keys in TrustedVaultClient could have changed. This is
+        // done even if the user cancelled the flow (i.e. resultCode != RESULT_OK) because it's
+        // harmless to issue a redundant notifyKeysChanged().
+        // The Chrome browser process might be not alive, there is no need to start it, because new
+        // keys will be fetched during its next start automatically.
+        if (ChromeBrowserInitializer.getInstance().isFullBrowserInitialized()) {
+            TrustedVaultClient.get().notifyKeysChanged();
+        }
+        finish();
+    }
+}
\ No newline at end of file

chrome/android/java/trichrome_chrome_bundle__base_bundle_module.AndroidManifest.expected

@@ -504,6 +504,11 @@
         android:name="org.chromium.chrome.browser.instantapps.AuthenticatedProxyActivity"
         android:noHistory="true"
         android:theme="@android:style/Theme.NoDisplay"/>
+    <activity
+        android:excludeFromRecents="true"
+        android:exported="false"
+        android:name="org.chromium.chrome.browser.sync.ui.TrustedVaultKeyRetrievalProxyActivity"
+        android:theme="@android:style/Theme.NoDisplay"/>
     <activity
         android:excludeFromRecents="true"
         android:name="org.chromium.chrome.browser.webapps.WebappLauncherActivity"