Fix OnPassphraseRequired() issued for trusted vault passphrase

During trusted vault passphrase, the existence of pending keys should
be signaled using OnTrustedVaultKeyRequired() instead of
OnPassphraseRequired().

This patch fixes the calling site in NigoriSyncBridgeImpl::Init() and
refactors the code to centralize all calling sites to prevent similar
issues in the future.

Bug: 1010189
Change-Id: I408279ff56c8e21aae05c498cf1238e1827b2ea3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1840632
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Maksim Moskvitin <mmoskvitin@google.com>
Cr-Commit-Position: refs/heads/master@{#703216}

components/sync/engine/sync_encryption_handler.h

@@ -42,6 +42,7 @@ enum BootstrapTokenType {
 // and keeps the nigori node up to date.
 // Implementations of this class must be assumed to be non-thread-safe. All
 // methods must be invoked on the sync thread.
+// TODO(crbug.com/1010397): Rename this class.
 class SyncEncryptionHandler {
  public:
   class NigoriState;
@@ -155,6 +156,8 @@ class SyncEncryptionHandler {
   // attempts to re-encrypt all sync data. Returns false in case of error.
   // Note: This method is expensive (it iterates through all encrypted types),
   // so should only be used sparingly (e.g. on startup).
+  // TODO(crbug.com/): Rename to something like NotifyStateToObservers() or
+  // even delete this API altogether.
   virtual bool Init() = 0;
 
   // Attempts to re-encrypt encrypted data types using the passphrase provided.

components/sync/nigori/nigori_sync_bridge_impl.cc

@@ -559,13 +559,6 @@ bool NigoriSyncBridgeImpl::Init() {
   // completeness of first sync cycle (which happens before Init() call).
   // TODO(crbug.com/922900): try to avoid double notification (second one can
   // happen during UpdateLocalState() call).
-  if (state_.pending_keys.has_value()) {
-    for (auto& observer : observers_) {
-      observer.OnPassphraseRequired(REASON_DECRYPTION,
-                                    GetKeyDerivationParamsForPendingKeys(),
-                                    *state_.pending_keys);
-    }
-  }
   for (auto& observer : observers_) {
     observer.OnEncryptedTypesChanged(
         GetEncryptedTypes(state_.encrypt_everything),
@@ -575,6 +568,9 @@ bool NigoriSyncBridgeImpl::Init() {
     observer.OnCryptographerStateChanged(state_.cryptographer.get(),
                                          state_.pending_keys.has_value());
   }
+
+  MaybeNotifyOfPendingKeys();
+
   if (state_.passphrase_type != NigoriSpecifics::UNKNOWN) {
     // if |passphrase_type| is unknown, it is not yet initialized and we
     // shouldn't expose it.
@@ -950,36 +946,7 @@ base::Optional<ModelError> NigoriSyncBridgeImpl::UpdateLocalState(
                                          state_.pending_keys.has_value());
   }
 
-  if (!state_.pending_keys.has_value()) {
-    return base::nullopt;
-  }
-
-  switch (state_.passphrase_type) {
-    case NigoriSpecifics::UNKNOWN:
-    case NigoriSpecifics::IMPLICIT_PASSPHRASE:
-      NOTREACHED();
-      break;
-    case NigoriSpecifics::KEYSTORE_PASSPHRASE: {
-      // Update with keystore Nigori shouldn't reach this point, since it should
-      // report model error if it has pending keys.
-      NOTREACHED();
-      break;
-    }
-    case NigoriSpecifics::CUSTOM_PASSPHRASE:
-    case NigoriSpecifics::FROZEN_IMPLICIT_PASSPHRASE:
-      for (auto& observer : observers_) {
-        observer.OnPassphraseRequired(REASON_DECRYPTION,
-                                      GetKeyDerivationParamsForPendingKeys(),
-                                      *state_.pending_keys);
-      }
-      break;
-    case NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE:
-      for (auto& observer : observers_) {
-        observer.OnTrustedVaultKeyRequired();
-      }
-      break;
-  }
-
+  MaybeNotifyOfPendingKeys();
   return base::nullopt;
 }
 
@@ -1205,6 +1172,32 @@ KeyDerivationParams NigoriSyncBridgeImpl::GetKeyDerivationParamsForPendingKeys()
   }
 }
 
+void NigoriSyncBridgeImpl::MaybeNotifyOfPendingKeys() const {
+  if (!state_.pending_keys.has_value()) {
+    return;
+  }
+
+  switch (state_.passphrase_type) {
+    case NigoriSpecifics::UNKNOWN:
+    case NigoriSpecifics::IMPLICIT_PASSPHRASE:
+    case NigoriSpecifics::KEYSTORE_PASSPHRASE:
+      return;
+    case NigoriSpecifics::CUSTOM_PASSPHRASE:
+    case NigoriSpecifics::FROZEN_IMPLICIT_PASSPHRASE:
+      for (auto& observer : observers_) {
+        observer.OnPassphraseRequired(REASON_DECRYPTION,
+                                      GetKeyDerivationParamsForPendingKeys(),
+                                      *state_.pending_keys);
+      }
+      break;
+    case NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE:
+      for (auto& observer : observers_) {
+        observer.OnTrustedVaultKeyRequired();
+      }
+      break;
+  }
+}
+
 void NigoriSyncBridgeImpl::MaybeNotifyBootstrapTokenUpdated() const {
   switch (state_.passphrase_type) {
     case NigoriSpecifics::UNKNOWN:

components/sync/nigori/nigori_sync_bridge_impl.h

@@ -117,6 +117,10 @@ class NigoriSyncBridgeImpl : public KeystoreKeysHandler,
   // |passphrase_type_| is an explicit passphrase.
   KeyDerivationParams GetKeyDerivationParamsForPendingKeys() const;
 
+  // If there are pending keys and depending on the passphrase type, it invokes
+  // the appropriate observer methods (if any).
+  void MaybeNotifyOfPendingKeys() const;
+
   // Persists Nigori derived from explicit passphrase into preferences, in case
   // error occurs during serialization/encryption, corresponding preference
   // just won't be updated.

components/sync/nigori/nigori_sync_bridge_impl_unittest.cc

@@ -406,12 +406,18 @@ TEST_F(NigoriSyncBridgeImplTest,
       /*keystore_decryptor_params=*/kKeystoreKeyParams,
       /*keystore_key_params=*/kKeystoreKeyParams);
 
+  EXPECT_CALL(*observer(), OnPassphraseRequired(_, _, _)).Times(0);
+  EXPECT_CALL(*observer(), OnTrustedVaultKeyRequired()).Times(0);
+
   EXPECT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
 
+  // The current implementation issues a redundant notification.
   EXPECT_CALL(*observer(), OnCryptographerStateChanged(
-                               NotNull(), /*has_pending_keys=*/false));
+                               NotNull(), /*has_pending_keys=*/false))
+      .Times(2);
   EXPECT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
               Eq(base::nullopt));
+  EXPECT_TRUE(bridge()->Init());
   EXPECT_THAT(bridge()->GetKeystoreMigrationTime(), Not(NullTime()));
 
   const Cryptographer& cryptographer = bridge()->GetCryptographerForTesting();
@@ -571,18 +577,26 @@ TEST_F(NigoriSyncBridgeImplTest,
   *entity_data.specifics.mutable_nigori() =
       BuildCustomPassphraseNigoriSpecifics(Pbkdf2KeyParams("passphrase"));
 
+  EXPECT_CALL(*observer(), OnTrustedVaultKeyRequired()).Times(0);
+
   ASSERT_TRUE(bridge()->SetKeystoreKeys({"keystore_key"}));
 
+  // The current implementation issues redundant notifications.
   EXPECT_CALL(*observer(), OnEncryptedTypesChanged(
                                /*encrypted_types=*/EncryptableUserTypes(),
-                               /*encrypt_everything=*/true));
-  EXPECT_CALL(*observer(), OnCryptographerStateChanged(
-                               NotNull(), /*has_pending_keys=*/true));
+                               /*encrypt_everything=*/true))
+      .Times(2);
+  EXPECT_CALL(*observer(),
+              OnCryptographerStateChanged(NotNull(), /*has_pending_keys=*/true))
+      .Times(2);
   EXPECT_CALL(*observer(),
               OnPassphraseTypeChanged(PassphraseType::kCustomPassphrase,
-                                      Not(NullTime())));
+                                      Not(NullTime())))
+      .Times(2);
+
   EXPECT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
               Eq(base::nullopt));
+  EXPECT_TRUE(bridge()->Init());
   EXPECT_TRUE(bridge()->HasPendingKeysForTesting());
 }
 
@@ -768,8 +782,9 @@ TEST(NigoriSyncBridgeImplTestWithPackedExplicitPassphrase,
       BuildCustomPassphraseNigoriSpecifics(kKeyParams);
   ASSERT_TRUE(bridge->SetKeystoreKeys({"keystore_key"}));
 
-  EXPECT_CALL(observer, OnCryptographerStateChanged(
-                            NotNull(), /*has_pending_keys=*/false));
+  EXPECT_CALL(observer,
+              OnCryptographerStateChanged(NotNull(),
+                                          /*has_pending_keys=*/false));
   EXPECT_CALL(observer, OnPassphraseRequired(_, _, _)).Times(0);
   ASSERT_THAT(bridge->MergeSyncData(std::move(entity_data)), Eq(base::nullopt));
 
@@ -857,16 +872,23 @@ TEST_F(NigoriSyncBridgeImplTest,
   *entity_data.specifics.mutable_nigori() =
       BuildTrustedVaultNigoriSpecifics({Pbkdf2KeyParams(kTrustedVaultKey)});
 
+  EXPECT_CALL(*observer(), OnPassphraseRequired(_, _, _)).Times(0);
+
   ASSERT_TRUE(bridge()->SetKeystoreKeys({"keystore_key"}));
 
-  EXPECT_CALL(*observer(), OnCryptographerStateChanged(
-                               NotNull(), /*has_pending_keys=*/true));
+  // The current implementation issues redundant notifications.
+  EXPECT_CALL(*observer(),
+              OnCryptographerStateChanged(NotNull(), /*has_pending_keys=*/true))
+      .Times(2);
   EXPECT_CALL(*observer(),
               OnPassphraseTypeChanged(PassphraseType::kTrustedVaultPassphrase,
-                                      NullTime()));
-  EXPECT_CALL(*observer(), OnTrustedVaultKeyRequired());
+                                      NullTime()))
+      .Times(2);
+  EXPECT_CALL(*observer(), OnTrustedVaultKeyRequired()).Times(2);
+
   EXPECT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
               Eq(base::nullopt));
+  EXPECT_TRUE(bridge()->Init());
   EXPECT_THAT(bridge()->GetPassphraseTypeForTesting(),
               Eq(sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE));
   EXPECT_THAT(bridge()->GetEncryptedTypesForTesting(),
@@ -892,11 +914,14 @@ TEST_F(NigoriSyncBridgeImplTest,
   *default_entity_data.specifics.mutable_nigori() =
       sync_pb::NigoriSpecifics::default_instance();
 
+  EXPECT_CALL(*observer(), OnPassphraseRequired(_, _, _)).Times(0);
+
   ASSERT_TRUE(bridge()->SetKeystoreKeys({"keystore_key"}));
   // Note: passing default Nigori to MergeSyncData() leads to instantiation of
   // keystore Nigori.
   ASSERT_THAT(bridge()->MergeSyncData(std::move(default_entity_data)),
               Eq(base::nullopt));
+  ASSERT_TRUE(bridge()->Init());
 
   EntityData new_entity_data;
   *new_entity_data.specifics.mutable_nigori() =
@@ -933,6 +958,8 @@ TEST_F(NigoriSyncBridgeImplTest,
   const std::string kTrustedVaultKey = "trusted_vault_key";
   const std::string kRotatedTrustedVaultKey = "rotated_vault_key";
 
+  EXPECT_CALL(*observer(), OnPassphraseRequired(_, _, _)).Times(0);
+
   EntityData entity_data;
   *entity_data.specifics.mutable_nigori() =
       BuildTrustedVaultNigoriSpecifics({Pbkdf2KeyParams(kTrustedVaultKey)});
@@ -940,6 +967,7 @@ TEST_F(NigoriSyncBridgeImplTest,
   ASSERT_TRUE(bridge()->SetKeystoreKeys({"keystore_key"}));
   ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
               Eq(base::nullopt));
+  ASSERT_TRUE(bridge()->Init());
   ASSERT_TRUE(bridge()->HasPendingKeysForTesting());
   bridge()->AddTrustedVaultDecryptionKeys({kTrustedVaultKey});
   ASSERT_FALSE(bridge()->HasPendingKeysForTesting());
@@ -983,6 +1011,7 @@ TEST_F(NigoriSyncBridgeImplTest,
   ASSERT_TRUE(bridge()->SetKeystoreKeys({"keystore_key"}));
   ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
               Eq(base::nullopt));
+  ASSERT_TRUE(bridge()->Init());
   ASSERT_TRUE(bridge()->HasPendingKeysForTesting());
   bridge()->AddTrustedVaultDecryptionKeys({kTrustedVaultKey});
   ASSERT_FALSE(bridge()->HasPendingKeysForTesting());