Revert 108530 - Merge 108409 - [Sync] Ensure nigori node always has encryption keys.

BUG=102526
TEST=sync_unit_tests


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

TBR=zea@chromium.org
Review URL: http://codereview.chromium.org/8417052

TBR=zea@chromium.org
Review URL: http://codereview.chromium.org/8430044

git-svn-id: svn://svn.chromium.org/chrome/branches/912/src@108538 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 18 insertions, 69 deletions

diff --git a/chrome/browser/sync/internal_api/sync_manager.cc b/chrome/browser/sync/internal_api/sync_manager.cc
index 5b68289..c8435b6 100644
--- a/chrome/browser/sync/internal_api/sync_manager.cc
+++ b/chrome/browser/sync/internal_api/sync_manager.cc
@@ -217,13 +217,11 @@ 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 and write back any
-  // necessary changes to the nigori node. We also detect missing encryption
-  // keys and write them into the nigori node.
+  // Update the Cryptographer from the current 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 UpdateCryptographerAndNigori();
+  bool UpdateCryptographerFromNigori();
 
   // Set the datatypes we want to encrypt and encrypt any nodes as necessary.
   // Note: |encrypted_types| will be unioned with the current set of encrypted
@@ -845,12 +843,11 @@ void SyncManager::SyncInternal::BootstrapEncryption(
   cryptographer->Bootstrap(restored_key_for_bootstrapping);
 }
 
-bool SyncManager::SyncInternal::UpdateCryptographerAndNigori() {
+bool SyncManager::SyncInternal::UpdateCryptographerFromNigori() {
   DCHECK(initialized_);
   syncable::ScopedDirLookup lookup(dir_manager(), username_for_share());
   if (!lookup.good()) {
-    NOTREACHED()
-        << "UpdateCryptographerAndNigori: lookup not good so bailing out";
+    NOTREACHED() << "BootstrapEncryption: lookup not good so bailing out";
     return false;
   }
   if (!lookup->initial_sync_ended_for_type(syncable::NIGORI))
@@ -871,13 +868,6 @@ bool SyncManager::SyncInternal::UpdateCryptographerAndNigori() {
                       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);
@@ -2013,7 +2003,7 @@ UserShare* SyncManager::GetUserShare() const {
 
 void SyncManager::RefreshEncryption() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (data_->UpdateCryptographerAndNigori())
+  if (data_->UpdateCryptographerFromNigori())
     data_->EncryptDataTypes(syncable::ModelTypeSet());
 }
 
diff --git a/chrome/browser/sync/internal_api/syncapi_unittest.cc b/chrome/browser/sync/internal_api/syncapi_unittest.cc
index 6de3ea5..24dcffa 100644
--- a/chrome/browser/sync/internal_api/syncapi_unittest.cc
+++ b/chrome/browser/sync/internal_api/syncapi_unittest.cc
@@ -743,21 +743,14 @@ class SyncManagerTest : public testing::Test,
   virtual void OnChangesComplete(syncable::ModelType model_type) OVERRIDE {}
 
   // Helper methods.
-  bool SetUpEncryption(bool write_to_nigori) {
+  bool SetUpEncryption() {
     // Mock the Mac Keychain service. The real Keychain can block on user input.
     #if defined(OS_MACOSX)
       Encryptor::UseMockKeychain(true);
     #endif
 
-    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.
+    UserShare* share = sync_manager_.GetUserShare();
     int64 nigori_id = GetIdForDataType(syncable::NIGORI);
     if (nigori_id == kInvalidId)
       return false;
@@ -769,13 +762,11 @@ class SyncManagerTest : public testing::Test,
       return false;
     KeyParams params = {"localhost", "dummy", "foobar"};
     cryptographer->AddKey(params);
-    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);
-    }
+    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();
   }
 
@@ -1201,23 +1192,12 @@ TEST_F(SyncManagerTest, OnIncomingNotification) {
 }
 
 TEST_F(SyncManagerTest, RefreshEncryptionReady) {
-  EXPECT_TRUE(SetUpEncryption(true));
-  EXPECT_CALL(observer_, OnEncryptionComplete());
+  EXPECT_TRUE(SetUpEncryption());
   sync_manager_.RefreshEncryption();
   syncable::ModelTypeSet encrypted_types =
       sync_manager_.GetEncryptedDataTypes();
   EXPECT_EQ(1U, encrypted_types.count(syncable::PASSWORDS));
   EXPECT_FALSE(sync_manager_.EncryptEverythingEnabled());
-  {
-    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.
@@ -1230,29 +1210,8 @@ TEST_F(SyncManagerTest, RefreshEncryptionNotReady) {
   EXPECT_FALSE(sync_manager_.EncryptEverythingEnabled());
 }
 
-// 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(true));
+  EXPECT_TRUE(SetUpEncryption());
   EXPECT_CALL(observer_, OnEncryptionComplete(GetAllRealModelTypes()));
   sync_manager_.EnableEncryptEverything();
   EXPECT_TRUE(sync_manager_.EncryptEverythingEnabled());
@@ -1260,7 +1219,7 @@ TEST_F(SyncManagerTest, EncryptDataTypesWithNoData) {
 
 TEST_F(SyncManagerTest, EncryptDataTypesWithData) {
   size_t batch_size = 5;
-  EXPECT_TRUE(SetUpEncryption(true));
+  EXPECT_TRUE(SetUpEncryption());
 
   // Create some unencrypted unsynced data.
   int64 folder = MakeFolderWithParent(sync_manager_.GetUserShare(),
@@ -1355,7 +1314,7 @@ TEST_F(SyncManagerTest, EncryptDataTypesWithData) {
 }
 
 TEST_F(SyncManagerTest, SetPassphraseWithPassword) {
-  EXPECT_TRUE(SetUpEncryption(true));
+  EXPECT_TRUE(SetUpEncryption());
   {
     WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
     ReadNode root_node(&trans);
@@ -1384,7 +1343,7 @@ TEST_F(SyncManagerTest, SetPassphraseWithPassword) {
 }
 
 TEST_F(SyncManagerTest, SetPassphraseWithEmptyPasswordNode) {
-  EXPECT_TRUE(SetUpEncryption(true));
+  EXPECT_TRUE(SetUpEncryption());
   int64 node_id = 0;
   std::string tag = "foo";
   {
@@ -1418,7 +1377,7 @@ TEST_F(SyncManagerTest, SetPassphraseWithEmptyPasswordNode) {
 
 // Friended by WriteNode, so can't be in an anonymouse namespace.
 TEST_F(SyncManagerTest, EncryptBookmarksWithLegacyData) {
-  EXPECT_TRUE(SetUpEncryption(true));
+  EXPECT_TRUE(SetUpEncryption());
   std::string title;
   SyncAPINameToServerName("Google", &title);
   std::string url = "http://www.google.com";