Introduce TrustedVaultClient::MarkLocalKeysAsStale()

The API is intended to propagate back to the implementation of
TrustedVaultClient::FetchKeys() that the result of fetching keys led to
keys that did not resolve the encryption issue.

Bug: 1012659
Change-Id: I41eb6fb4ff2744386725faa727be9112dd7e2537
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1970016
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#729690}

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

@@ -45,6 +45,16 @@ public class TrustedVaultClient {
          */
         @Nullable
         Intent createKeyRetrievalIntent();
+
+        /**
+         * Invoked when the result of fetchKeys() represents keys that cannot decrypt Nigori, which
+         * should only be possible if the provided keys are not up-to-date.
+         *
+         * @param gaiaId String representation of the Gaia ID.
+         * @return a promise which indicates completion and also represents whether the operation
+         * took any effect (false positives acceptable).
+         */
+        Promise<Boolean> markKeysAsStale(String gaiaId);
     }
 
     /**
@@ -60,6 +70,11 @@ public class TrustedVaultClient {
         public Intent createKeyRetrievalIntent() {
             return null;
         }
+
+        @Override
+        public Promise<Boolean> markKeysAsStale(String gaiaId) {
+            return Promise.fulfilled(false);
+        }
     };
 
     private static TrustedVaultClient sInstance;
@@ -162,8 +177,35 @@ public class TrustedVaultClient {
                 });
     }
 
+    /**
+     * Forwards calls to Backend.markKeysAsStale() and upon completion invokes native method
+     * markKeysAsStaleCompleted().
+     */
+    @CalledByNative
+    private static void markKeysAsStale(long nativeTrustedVaultClientAndroid, String gaiaId) {
+        assert isNativeRegistered(nativeTrustedVaultClientAndroid);
+        get().mBackend.markKeysAsStale(gaiaId).then(
+                (result)
+                        -> {
+                    if (isNativeRegistered(nativeTrustedVaultClientAndroid)) {
+                        TrustedVaultClientJni.get().markKeysAsStaleCompleted(
+                                nativeTrustedVaultClientAndroid, result);
+                    }
+                },
+                (exception) -> {
+                    if (isNativeRegistered(nativeTrustedVaultClientAndroid)) {
+                        // There's no certainty about whether the operation made any difference so
+                        // let's return true indicating that it might have, since false positives
+                        // are allowed.
+                        TrustedVaultClientJni.get().markKeysAsStaleCompleted(
+                                nativeTrustedVaultClientAndroid, true);
+                    }
+                });
+    }
+
     @NativeMethods
     interface Natives {
         void fetchKeysCompleted(long nativeTrustedVaultClientAndroid, String gaiaId, byte[][] keys);
+        void markKeysAsStaleCompleted(long nativeTrustedVaultClientAndroid, boolean result);
     }
 }

chrome/browser/sync/trusted_vault_client_android.cc

@@ -50,6 +50,18 @@ void TrustedVaultClientAndroid::FetchKeysCompleted(
   std::move(cb).Run(converted_keys);
 }
 
+void TrustedVaultClientAndroid::MarkKeysAsStaleCompleted(JNIEnv* env,
+                                                         jboolean result) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(ongoing_mark_keys_as_stale_) << "No ongoing MarkKeysAsStale() request";
+
+  // Make a copy of the callback and reset |ongoing_mark_keys_as_stale_| before
+  // invoking the callback, in case it has side effects.
+  base::OnceCallback<void(bool)> cb = std::move(ongoing_mark_keys_as_stale_);
+
+  std::move(cb).Run(!!result);
+}
+
 std::unique_ptr<TrustedVaultClientAndroid::Subscription>
 TrustedVaultClientAndroid::AddKeysChangedObserver(
     const base::RepeatingClosure& cb) {
@@ -62,6 +74,8 @@ void TrustedVaultClientAndroid::FetchKeys(
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!ongoing_fetch_keys_)
       << "Only one FetchKeys() request is allowed at any time";
+  DCHECK(!ongoing_mark_keys_as_stale_)
+      << "FetchKeys() not allowed while ongoing MarkKeysAsStale()";
 
   // Store for later completion when Java invokes FetchKeysCompleted().
   ongoing_fetch_keys_ =
@@ -83,3 +97,25 @@ void TrustedVaultClientAndroid::StoreKeys(
   // Not supported on Android, where keys are fetched outside the browser.
   NOTREACHED();
 }
+
+void TrustedVaultClientAndroid::MarkKeysAsStale(
+    const std::string& gaia_id,
+    base::OnceCallback<void(bool)> cb) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(cb);
+  DCHECK(!ongoing_mark_keys_as_stale_)
+      << "Only one MarkKeysAsStale() request is allowed at any time";
+  DCHECK(!ongoing_fetch_keys_)
+      << "MarkKeysAsStale() not allowed while ongoing FetchKeys()";
+
+  // Store for later completion when Java invokes MarkKeysAsStaleCompleted().
+  ongoing_mark_keys_as_stale_ = std::move(cb);
+
+  JNIEnv* const env = base::android::AttachCurrentThread();
+  const base::android::ScopedJavaLocalRef<jstring> java_gaia_id =
+      base::android::ConvertUTF8ToJavaString(env, gaia_id);
+
+  // The Java implementation will eventually call MarkKeysAsStaleCompleted().
+  Java_TrustedVaultClient_markKeysAsStale(env, reinterpret_cast<intptr_t>(this),
+                                          java_gaia_id);
+}

chrome/browser/sync/trusted_vault_client_android.h

@@ -36,6 +36,11 @@ class TrustedVaultClientAndroid : public syncer::TrustedVaultClient {
       const base::android::JavaParamRef<jstring>& gaia_id,
       const base::android::JavaParamRef<jobjectArray>& keys);
 
+  // Called from Java to notify the completion of MarkKeysAsStale()
+  // operation previously initiated from C++. This must correspond to an
+  // ongoing MarkKeysAsStale() request.
+  void MarkKeysAsStaleCompleted(JNIEnv* env, jboolean result);
+
   // TrustedVaultClient implementation.
   std::unique_ptr<Subscription> AddKeysChangedObserver(
       const base::RepeatingClosure& cb) override;
@@ -45,6 +50,8 @@ class TrustedVaultClientAndroid : public syncer::TrustedVaultClient {
       override;
   void StoreKeys(const std::string& gaia_id,
                  const std::vector<std::vector<uint8_t>>& keys) override;
+  void MarkKeysAsStale(const std::string& gaia_id,
+                       base::OnceCallback<void(bool)> cb) override;
 
  private:
   // Struct representing an in-flight FetchKeys() call invoked from C++.
@@ -62,6 +69,10 @@ class TrustedVaultClientAndroid : public syncer::TrustedVaultClient {
   // Null if no in-flight FetchKeys().
   std::unique_ptr<OngoingFetchKeys> ongoing_fetch_keys_;
 
+  // Completion callback of an in-flight MarkKeysAsStale() call invoked from
+  // C++.
+  base::OnceCallback<void(bool)> ongoing_mark_keys_as_stale_;
+
   CallbackList observer_list_;
 };
 

components/sync/driver/file_based_trusted_vault_client.cc

@@ -148,6 +148,13 @@ void FileBasedTrustedVaultClient::StoreKeys(
   observer_list_.Notify();
 }
 
+void FileBasedTrustedVaultClient::MarkKeysAsStale(
+    const std::string& gaia_id,
+    base::OnceCallback<void(bool)> cb) {
+  // Not really supported and not useful for this particular implementation.
+  std::move(cb).Run(false);
+}
+
 void FileBasedTrustedVaultClient::WaitForFlushForTesting(
     base::OnceClosure cb) const {
   backend_task_runner_->PostTaskAndReply(FROM_HERE, base::DoNothing(),

components/sync/driver/file_based_trusted_vault_client.h

@@ -37,6 +37,8 @@ class FileBasedTrustedVaultClient : public TrustedVaultClient {
       override;
   void StoreKeys(const std::string& gaia_id,
                  const std::vector<std::vector<uint8_t>>& keys) override;
+  void MarkKeysAsStale(const std::string& gaia_id,
+                       base::OnceCallback<void(bool)> cb) override;
 
   // Runs |cb| when all requests have completed.
   void WaitForFlushForTesting(base::OnceClosure cb) const;

components/sync/driver/sync_service_crypto.cc

@@ -48,6 +48,11 @@ class EmptyTrustedVaultClient : public TrustedVaultClient {
     // Never invoked by SyncServiceCrypto.
     NOTREACHED();
   }
+
+  void MarkKeysAsStale(const std::string& gaia_id,
+                       base::OnceCallback<void(bool)> cb) override {
+    std::move(cb).Run(false);
+  }
 };
 
 // A SyncEncryptionHandler::Observer implementation that simply posts all calls
@@ -578,11 +583,11 @@ void SyncServiceCrypto::FetchTrustedVaultKeys() {
 
   trusted_vault_client_->FetchKeys(
       state_.account_info.gaia,
-      base::BindOnce(&SyncServiceCrypto::TrustedVaultKeysFetched,
+      base::BindOnce(&SyncServiceCrypto::TrustedVaultKeysFetchedFromClient,
                      weak_factory_.GetWeakPtr()));
 }
 
-void SyncServiceCrypto::TrustedVaultKeysFetched(
+void SyncServiceCrypto::TrustedVaultKeysFetchedFromClient(
     const std::vector<std::vector<uint8_t>>& keys) {
   if (state_.required_user_action !=
           RequiredUserAction::kFetchingTrustedVaultKeys &&
@@ -593,6 +598,14 @@ void SyncServiceCrypto::TrustedVaultKeysFetched(
 
   DCHECK(state_.engine);
 
+  if (keys.empty()) {
+    // Nothing to do if no keys have been fetched from the client (e.g. user
+    // action is required for fetching additional keys). Let's avoid unnecessary
+    // steps like marking keys as stale.
+    FetchTrustedVaultKeysCompletedButInsufficient();
+    return;
+  }
+
   state_.engine->AddTrustedVaultDecryptionKeys(
       keys, base::BindOnce(&SyncServiceCrypto::TrustedVaultKeysAdded,
                            weak_factory_.GetWeakPtr()));
@@ -606,6 +619,35 @@ void SyncServiceCrypto::TrustedVaultKeysAdded() {
     return;
   }
 
+  // Reaching this codepath indicates OnTrustedVaultKeyAccepted() was not
+  // triggered, so the fetched trusted vault keys were insufficient. Let the
+  // trusted vault client know.
+  trusted_vault_client_->MarkKeysAsStale(
+      state_.account_info.gaia,
+      base::BindOnce(&SyncServiceCrypto::TrustedVaultKeysMarkedAsStale,
+                     weak_factory_.GetWeakPtr()));
+}
+
+void SyncServiceCrypto::TrustedVaultKeysMarkedAsStale(bool result) {
+  if (state_.required_user_action !=
+          RequiredUserAction::kFetchingTrustedVaultKeys &&
+      state_.required_user_action !=
+          RequiredUserAction::kTrustedVaultKeyRequiredButFetching) {
+    return;
+  }
+
+  // TODO(crbug.com/1012659): Based on |result|, start a second FetchKeys()
+  // pass.
+
+  FetchTrustedVaultKeysCompletedButInsufficient();
+}
+
+void SyncServiceCrypto::FetchTrustedVaultKeysCompletedButInsufficient() {
+  DCHECK(state_.required_user_action ==
+             RequiredUserAction::kFetchingTrustedVaultKeys ||
+         state_.required_user_action ==
+             RequiredUserAction::kTrustedVaultKeyRequiredButFetching);
+
   // If FetchKeys() was intended to be called during an already existing ongoing
   // FetchKeys(), it needs to be invoked now that it's possible.
   if (state_.deferred_trusted_vault_fetch_keys_pending) {
@@ -614,9 +656,11 @@ void SyncServiceCrypto::TrustedVaultKeysAdded() {
   }
 
   // Reaching this codepath indicates OnTrustedVaultKeyAccepted() was not
-  // triggered, so reconfigure without the encrypted types (excluded implicitly
-  // via the failed datatypes handler).
+  // triggered, so the fetched trusted vault keys were insufficient.
   state_.required_user_action = RequiredUserAction::kTrustedVaultKeyRequired;
+
+  // Reconfigure without the encrypted types (excluded implicitly via the failed
+  // datatypes handler).
   reconfigure_.Run(CONFIGURE_REASON_CRYPTO);
 }
 

components/sync/driver/sync_service_crypto.h

@@ -110,8 +110,11 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer,
 
   // Called at various stages of asynchronously fetching and processing trusted
   // vault encryption keys.
-  void TrustedVaultKeysFetched(const std::vector<std::vector<uint8_t>>& keys);
+  void TrustedVaultKeysFetchedFromClient(
+      const std::vector<std::vector<uint8_t>>& keys);
   void TrustedVaultKeysAdded();
+  void TrustedVaultKeysMarkedAsStale(bool result);
+  void FetchTrustedVaultKeysCompletedButInsufficient();
 
   // Calls SyncServiceBase::NotifyObservers(). Never null.
   const base::RepeatingClosure notify_observers_;

components/sync/driver/sync_service_crypto_unittest.cc

@@ -68,6 +68,10 @@ class TestTrustedVaultClient : public TrustedVaultClient {
   // Exposes the total number of calls to FetchKeys().
   int fetch_count() const { return fetch_count_; }
 
+  // Returns whether MarkKeysAsStale() was called since the last call to
+  // FetchKeys().
+  bool keys_marked_as_stale() const { return keys_marked_as_stale_; }
+
   // Mimics the completion of the next (FIFO) FetchKeys() request.
   bool CompleteFetchKeysRequest() {
     if (pending_responses_.empty()) {
@@ -91,6 +95,7 @@ class TestTrustedVaultClient : public TrustedVaultClient {
       base::OnceCallback<void(const std::vector<std::vector<uint8_t>>&)> cb)
       override {
     ++fetch_count_;
+    keys_marked_as_stale_ = false;
     pending_responses_.push_back(
         base::BindOnce(std::move(cb), gaia_id_to_keys_[gaia_id]));
   }
@@ -101,10 +106,17 @@ class TestTrustedVaultClient : public TrustedVaultClient {
     observer_list_.Notify();
   }
 
+  void MarkKeysAsStale(const std::string& gaia_id,
+                       base::OnceCallback<void(bool)> cb) override {
+    keys_marked_as_stale_ = true;
+    std::move(cb).Run(false);
+  }
+
  private:
   std::map<std::string, std::vector<std::vector<uint8_t>>> gaia_id_to_keys_;
   CallbackList observer_list_;
   int fetch_count_ = 0;
+  bool keys_marked_as_stale_ = false;
   std::list<base::OnceClosure> pending_responses_;
 };
 
@@ -209,6 +221,7 @@ TEST_F(SyncServiceCryptoTest,
   crypto_.OnTrustedVaultKeyAccepted();
   std::move(add_keys_cb).Run();
   EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+  EXPECT_FALSE(trusted_vault_client_.keys_marked_as_stale());
 }
 
 TEST_F(SyncServiceCryptoTest,
@@ -248,6 +261,34 @@ TEST_F(SyncServiceCryptoTest,
   crypto_.OnTrustedVaultKeyAccepted();
   std::move(add_keys_cb).Run();
   EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+  EXPECT_FALSE(trusted_vault_client_.keys_marked_as_stale());
+}
+
+TEST_F(SyncServiceCryptoTest,
+       ShouldReadNoTrustedVaultKeysFromClientAfterInitialization) {
+  const CoreAccountInfo kSyncingAccount =
+      MakeAccountInfoWithGaia("syncingaccount");
+
+  EXPECT_CALL(reconfigure_cb_, Run(_)).Times(0);
+  EXPECT_CALL(engine_, AddTrustedVaultDecryptionKeys(_, _)).Times(0);
+
+  ASSERT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+
+  // Mimic the engine determining that trusted vault keys are required.
+  crypto_.SetSyncEngine(kSyncingAccount, &engine_);
+  ASSERT_THAT(trusted_vault_client_.fetch_count(), Eq(0));
+
+  crypto_.OnTrustedVaultKeyRequired();
+
+  // While there is an ongoing fetch, there should be no user action required.
+  ASSERT_THAT(trusted_vault_client_.fetch_count(), Eq(1));
+  ASSERT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+
+  // Mimic completion of the fetch, which should lead to a reconfiguration.
+  EXPECT_CALL(reconfigure_cb_, Run(CONFIGURE_REASON_CRYPTO));
+  ASSERT_TRUE(trusted_vault_client_.CompleteFetchKeysRequest());
+  EXPECT_TRUE(crypto_.IsTrustedVaultKeyRequired());
+  EXPECT_FALSE(trusted_vault_client_.keys_marked_as_stale());
 }
 
 TEST_F(SyncServiceCryptoTest, ShouldReadInvalidTrustedVaultKeysFromClient) {
@@ -284,6 +325,7 @@ TEST_F(SyncServiceCryptoTest, ShouldReadInvalidTrustedVaultKeysFromClient) {
   EXPECT_CALL(reconfigure_cb_, Run(CONFIGURE_REASON_CRYPTO));
   std::move(add_keys_cb).Run();
   EXPECT_TRUE(crypto_.IsTrustedVaultKeyRequired());
+  EXPECT_TRUE(trusted_vault_client_.keys_marked_as_stale());
 }
 
 // Similar to ShouldReadInvalidTrustedVaultKeysFromClient: the vault
@@ -318,6 +360,7 @@ TEST_F(SyncServiceCryptoTest, ShouldRefetchTrustedVaultKeysWhenChangeObserved) {
   ASSERT_THAT(trusted_vault_client_.fetch_count(), Eq(1));
   ASSERT_TRUE(trusted_vault_client_.CompleteFetchKeysRequest());
   ASSERT_TRUE(crypto_.IsTrustedVaultKeyRequired());
+  ASSERT_TRUE(trusted_vault_client_.keys_marked_as_stale());
 
   // Mimic keys being added to the vault, which triggers a notification to
   // observers (namely |crypto_|), leading to a second fetch.
@@ -326,6 +369,7 @@ TEST_F(SyncServiceCryptoTest, ShouldRefetchTrustedVaultKeysWhenChangeObserved) {
   EXPECT_CALL(reconfigure_cb_, Run(CONFIGURE_REASON_CRYPTO));
   EXPECT_TRUE(trusted_vault_client_.CompleteFetchKeysRequest());
   EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+  EXPECT_FALSE(trusted_vault_client_.keys_marked_as_stale());
 }
 
 // Same as above but the new keys become available during an ongoing FetchKeys()

components/sync/driver/trusted_vault_client.h

@@ -41,6 +41,16 @@ class TrustedVaultClient {
       base::OnceCallback<void(const std::vector<std::vector<uint8_t>>&)>
           cb) = 0;
 
+  // Invoked when the result of FetchKeys() contains keys that cannot decrypt
+  // the pending cryptographer (Nigori) keys, which should only be possible if
+  // the provided keys are not up-to-date. |cb| is run upon completion and
+  // returns false if the call did not make any difference (e.g. the operation
+  // is unsupported) or true if some change may have occurred (which indicates a
+  // second FetchKeys() attempt is worth). Concurrent calls to MarkKeysAsStale()
+  // must not be issued since implementations may not support them.
+  virtual void MarkKeysAsStale(const std::string& gaia_id,
+                               base::OnceCallback<void(bool)> cb) = 0;
+
   // Allows implementations to store encryption keys fetched by other means such
   // as Web interactions. Implementations are free to completely ignore these
   // keys, so callers may not assume that later calls to FetchKeys() would