[Sync] Ensure nigori node always has encryption keys.

BUG=102526
TEST=sync_unit_tests


Review URL: http://codereview.chromium.org/8438015

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@108409 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 68 insertions, 18 deletions

diff --git a/chrome/browser/sync/internal_api/sync_manager.cc b/chrome/browser/sync/internal_api/sync_manager.cc
index 9046a73..4ad8eaa 100644
--- a/chrome/browser/sync/internal_api/sync_manager.cc
+++ b/chrome/browser/sync/internal_api/sync_manager.cc
@@ -218,11 +218,13 @@ class SyncManager::SyncInternal
   // Whether or not the Nigori node is encrypted using an explicit passphrase.
   bool IsUsingExplicitPassphrase();
 
-  // Update the Cryptographer from the current nigori node.
+  // Update the Cryptographer from the current nigori node and write back any
+  // necessary changes to the nigori node. We also detect missing encryption
+  // keys and write them into the nigori node.
   // Note: opens a transaction and can trigger an ON_PASSPHRASE_REQUIRED, so
   // should only be called after syncapi is fully initialized.
   // Returns true if cryptographer is ready, false otherwise.
-  bool UpdateCryptographerFromNigori();
+  bool UpdateCryptographerAndNigori();
 
   // Updates the nigori node with any new encrypted types and then
   // encrypts the nodes for those new data types as well as other
@@ -835,12 +837,12 @@ bool SyncManager::SyncInternal::Init(
   return signed_in;
 }
 
-bool SyncManager::SyncInternal::UpdateCryptographerFromNigori() {
+bool SyncManager::SyncInternal::UpdateCryptographerAndNigori() {
   DCHECK(initialized_);
   syncable::ScopedDirLookup lookup(dir_manager(), username_for_share());
   if (!lookup.good()) {
     NOTREACHED()
-        << "UpdateCryptographerFromNigori: lookup not good so bailing out";
+        << "UpdateCryptographerAndNigori: lookup not good so bailing out";
     return false;
   }
   if (!lookup->initial_sync_ended_for_type(syncable::NIGORI))
@@ -861,6 +863,13 @@ bool SyncManager::SyncInternal::UpdateCryptographerFromNigori() {
                       OnPassphraseRequired(sync_api::REASON_DECRYPTION));
   }
 
+  // Due to http://crbug.com/102526, we must check if the encryption keys
+  // are present in the nigori node. If they're not, we write the current set of
+  // keys.
+  if (!nigori.has_encrypted() && cryptographer->is_ready()) {
+    cryptographer->GetKeys(nigori.mutable_encrypted());
+  }
+
   // Ensure the nigori node reflects the most recent set of sensitive types
   // and properly sets encrypt_everything. This is a no-op if nothing changes.
   cryptographer->UpdateNigoriFromEncryptedTypes(&nigori);
@@ -2001,7 +2010,7 @@ UserShare* SyncManager::GetUserShare() const {
 
 void SyncManager::RefreshEncryption() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (data_->UpdateCryptographerFromNigori())
+  if (data_->UpdateCryptographerAndNigori())
     data_->RefreshEncryption();
 }
 
diff --git a/chrome/browser/sync/internal_api/syncapi_unittest.cc b/chrome/browser/sync/internal_api/syncapi_unittest.cc
index c9a29e4..d8b9277 100644
--- a/chrome/browser/sync/internal_api/syncapi_unittest.cc
+++ b/chrome/browser/sync/internal_api/syncapi_unittest.cc
@@ -746,14 +746,21 @@ class SyncManagerTest : public testing::Test,
   virtual void OnChangesComplete(syncable::ModelType model_type) OVERRIDE {}
 
   // Helper methods.
-  bool SetUpEncryption() {
+  bool SetUpEncryption(bool write_to_nigori) {
     // Mock the Mac Keychain service. The real Keychain can block on user input.
     #if defined(OS_MACOSX)
       Encryptor::UseMockKeychain(true);
     #endif
 
-    // We need to create the nigori node as if it were an applied server update.
     UserShare* share = sync_manager_.GetUserShare();
+    {
+      syncable::ScopedDirLookup dir(share->dir_manager.get(), share->name);
+      if (!dir.good())
+        return false;
+      dir->set_initial_sync_ended_for_type(syncable::NIGORI, true);
+    }
+
+    // We need to create the nigori node as if it were an applied server update.
     int64 nigori_id = GetIdForDataType(syncable::NIGORI);
     if (nigori_id == kInvalidId)
       return false;
@@ -765,11 +772,13 @@ class SyncManagerTest : public testing::Test,
       return false;
     KeyParams params = {"localhost", "dummy", "foobar"};
     cryptographer->AddKey(params);
-    sync_pb::NigoriSpecifics nigori;
-    cryptographer->GetKeys(nigori.mutable_encrypted());
-    WriteNode node(&trans);
-    EXPECT_TRUE(node.InitByIdLookup(nigori_id));
-    node.SetNigoriSpecifics(nigori);
+    if (write_to_nigori) {
+      sync_pb::NigoriSpecifics nigori;
+      cryptographer->GetKeys(nigori.mutable_encrypted());
+      WriteNode node(&trans);
+      EXPECT_TRUE(node.InitByIdLookup(nigori_id));
+      node.SetNigoriSpecifics(nigori);
+    }
     return cryptographer->is_ready();
   }
 
@@ -1195,12 +1204,23 @@ TEST_F(SyncManagerTest, OnIncomingNotification) {
 }
 
 TEST_F(SyncManagerTest, RefreshEncryptionReady) {
-  EXPECT_TRUE(SetUpEncryption());
+  EXPECT_TRUE(SetUpEncryption(true));
+  EXPECT_CALL(observer_, OnEncryptionComplete());
   sync_manager_.RefreshEncryption();
   syncable::ModelTypeSet encrypted_types =
       sync_manager_.GetEncryptedDataTypesForTest();
   EXPECT_EQ(1U, encrypted_types.count(syncable::PASSWORDS));
   EXPECT_FALSE(sync_manager_.EncryptEverythingEnabledForTest());
+  {
+    ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    ReadNode node(&trans);
+    EXPECT_TRUE(node.InitByIdLookup(GetIdForDataType(syncable::NIGORI)));
+    sync_pb::NigoriSpecifics nigori = node.GetNigoriSpecifics();
+    EXPECT_TRUE(nigori.has_encrypted());
+    Cryptographer* cryptographer = trans.GetCryptographer();
+    EXPECT_TRUE(cryptographer->is_ready());
+    EXPECT_TRUE(cryptographer->CanDecrypt(nigori.encrypted()));
+  }
 }
 
 // Attempt to refresh encryption when nigori not downloaded.
@@ -1213,8 +1233,29 @@ TEST_F(SyncManagerTest, RefreshEncryptionNotReady) {
   EXPECT_FALSE(sync_manager_.EncryptEverythingEnabledForTest());
 }
 
+// Attempt to refresh encryption when nigori is empty.
+TEST_F(SyncManagerTest, RefreshEncryptionEmptyNigori) {
+  EXPECT_TRUE(SetUpEncryption(false));
+  EXPECT_CALL(observer_, OnEncryptionComplete());
+  sync_manager_.RefreshEncryption();  // Should write to nigori.
+  syncable::ModelTypeSet encrypted_types =
+      sync_manager_.GetEncryptedDataTypesForTest();
+  EXPECT_EQ(1U, encrypted_types.count(syncable::PASSWORDS));  // Hardcoded.
+  EXPECT_FALSE(sync_manager_.EncryptEverythingEnabledForTest());
+  {
+    ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    ReadNode node(&trans);
+    EXPECT_TRUE(node.InitByIdLookup(GetIdForDataType(syncable::NIGORI)));
+    sync_pb::NigoriSpecifics nigori = node.GetNigoriSpecifics();
+    EXPECT_TRUE(nigori.has_encrypted());
+    Cryptographer* cryptographer = trans.GetCryptographer();
+    EXPECT_TRUE(cryptographer->is_ready());
+    EXPECT_TRUE(cryptographer->CanDecrypt(nigori.encrypted()));
+  }
+}
+
 TEST_F(SyncManagerTest, EncryptDataTypesWithNoData) {
-  EXPECT_TRUE(SetUpEncryption());
+  EXPECT_TRUE(SetUpEncryption(true));
   EXPECT_CALL(observer_,
               OnEncryptedTypesChanged(GetAllRealModelTypes(), true));
   EXPECT_CALL(observer_, OnEncryptionComplete());
@@ -1224,7 +1265,7 @@ TEST_F(SyncManagerTest, EncryptDataTypesWithNoData) {
 
 TEST_F(SyncManagerTest, EncryptDataTypesWithData) {
   size_t batch_size = 5;
-  EXPECT_TRUE(SetUpEncryption());
+  EXPECT_TRUE(SetUpEncryption(true));
 
   // Create some unencrypted unsynced data.
   int64 folder = MakeFolderWithParent(sync_manager_.GetUserShare(),
@@ -1330,7 +1371,7 @@ TEST_F(SyncManagerTest, EncryptDataTypesWithData) {
 }
 
 TEST_F(SyncManagerTest, SetPassphraseWithPassword) {
-  EXPECT_TRUE(SetUpEncryption());
+  EXPECT_TRUE(SetUpEncryption(true));
   {
     WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
     ReadNode root_node(&trans);
@@ -1359,7 +1400,7 @@ TEST_F(SyncManagerTest, SetPassphraseWithPassword) {
 }
 
 TEST_F(SyncManagerTest, SetPassphraseWithEmptyPasswordNode) {
-  EXPECT_TRUE(SetUpEncryption());
+  EXPECT_TRUE(SetUpEncryption(true));
   int64 node_id = 0;
   std::string tag = "foo";
   {
@@ -1393,7 +1434,7 @@ TEST_F(SyncManagerTest, SetPassphraseWithEmptyPasswordNode) {
 
 // Friended by WriteNode, so can't be in an anonymouse namespace.
 TEST_F(SyncManagerTest, EncryptBookmarksWithLegacyData) {
-  EXPECT_TRUE(SetUpEncryption());
+  EXPECT_TRUE(SetUpEncryption(true));
   std::string title;
   SyncAPINameToServerName("Google", &title);
   std::string url = "http://www.google.com";