Take 2 - Get password sync to a usable state.

Plumb the gaia password up to the cryptographer to generate an
encryption key.
Add NIGORI to the list of active datatypes to request if
enable-sync-passwords is specified.

Also fixes behavior from bug 55501 (required adding ExtraChangeRecordData to syncapi), as well as an ADD-handling bug in ApplyChangesFromSyncModel.

Add back the call to Cryptographer::Bootstrap which was mistakenly removed in auth refactor.

This patch doesn't have necessary UI to handle secondary passphrases,
password changes, or existing user upgrades. Only unsynced profiles
can currently get password sync working.

BUG=48702, 32410, 55501
TEST=ProfileSyncPasswordTest, upcoming password integration tests

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

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

10 files changed, 154 insertions, 26 deletions

diff --git a/chrome/browser/sync/engine/change_reorder_buffer.cc b/chrome/browser/sync/engine/change_reorder_buffer.cc
index f73d4a7..6af2399 100644
--- a/chrome/browser/sync/engine/change_reorder_buffer.cc
+++ b/chrome/browser/sync/engine/change_reorder_buffer.cc
@@ -139,6 +139,8 @@ void ChangeReorderBuffer::GetAllChangesInTreeOrder(
       record.action = ChangeRecord::ACTION_DELETE;
       if (specifics_.find(record.id) != specifics_.end())
         record.specifics = specifics_[record.id];
+      if (extra_data_.find(record.id) != extra_data_.end())
+        record.extra = extra_data_[record.id];
       changelist->push_back(record);
     } else {
       traversal.ExpandToInclude(trans, i->first);
@@ -170,6 +172,8 @@ void ChangeReorderBuffer::GetAllChangesInTreeOrder(
         record.action = ChangeRecord::ACTION_UPDATE;
       if (specifics_.find(record.id) != specifics_.end())
         record.specifics = specifics_[record.id];
+      if (extra_data_.find(record.id) != extra_data_.end())
+        record.extra = extra_data_[record.id];
       changelist->push_back(record);
     }
 
diff --git a/chrome/browser/sync/engine/change_reorder_buffer.h b/chrome/browser/sync/engine/change_reorder_buffer.h
index 0392679..e2e091a 100644
--- a/chrome/browser/sync/engine/change_reorder_buffer.h
+++ b/chrome/browser/sync/engine/change_reorder_buffer.h
@@ -13,6 +13,7 @@
 #include <map>
 #include <vector>
 
+#include "base/linked_ptr.h"
 #include "chrome/browser/sync/engine/syncapi.h"
 #include "chrome/browser/sync/protocol/sync.pb.h"
 
@@ -38,6 +39,7 @@ namespace sync_api {
 class ChangeReorderBuffer {
  public:
   typedef SyncManager::ChangeRecord ChangeRecord;
+  typedef SyncManager::ExtraChangeRecordData ExtraChangeRecordData;
 
   ChangeReorderBuffer();
   ~ChangeReorderBuffer();
@@ -66,6 +68,10 @@ class ChangeReorderBuffer {
                                          OP_UPDATE_PROPERTIES_ONLY;
   }
 
+  void SetExtraDataForId(int64 id, ExtraChangeRecordData* extra) {
+    extra_data_[id] = make_linked_ptr<ExtraChangeRecordData>(extra);
+  }
+
   void SetSpecificsForId(int64 id, const sync_pb::EntitySpecifics& specifics) {
     specifics_[id] = specifics;
   }
@@ -96,6 +102,7 @@ class ChangeReorderBuffer {
   };
   typedef std::map<int64, Operation> OperationMap;
   typedef std::map<int64, sync_pb::EntitySpecifics> SpecificsMap;
+  typedef std::map<int64, linked_ptr<ExtraChangeRecordData> > ExtraDataMap;
 
   // Stores the items that have been pushed into the buffer, and the type of
   // operation that was associated with them.
@@ -104,6 +111,9 @@ class ChangeReorderBuffer {
   // Stores entity-specific ChangeRecord data per-ID.
   SpecificsMap specifics_;
 
+  // Stores type-specific extra data per-ID.
+  ExtraDataMap extra_data_;
+
   DISALLOW_COPY_AND_ASSIGN(ChangeReorderBuffer);
 };
 
diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc
index cd2d92d..97ab4e8 100644
--- a/chrome/browser/sync/engine/syncapi.cc
+++ b/chrome/browser/sync/engine/syncapi.cc
@@ -173,17 +173,27 @@ std::string BaseNode::GenerateSyncableHash(
   return encode_output;
 }
 
+sync_pb::PasswordSpecificsData* DecryptPasswordSpecifics(
+    const sync_pb::EntitySpecifics& specifics, Cryptographer* crypto) {
+ if (!specifics.HasExtension(sync_pb::password))
+   return NULL;
+  const sync_pb::EncryptedData& encrypted =
+      specifics.GetExtension(sync_pb::password).encrypted();
+  scoped_ptr<sync_pb::PasswordSpecificsData> data(
+      new sync_pb::PasswordSpecificsData);
+  if (!crypto->Decrypt(encrypted, data.get()))
+    return NULL;
+  return data.release();
+}
+
 bool BaseNode::DecryptIfNecessary(Entry* entry) {
   if (GetIsFolder()) return true;  // Ignore the top-level password folder.
   const sync_pb::EntitySpecifics& specifics =
       entry->Get(syncable::SPECIFICS);
   if (specifics.HasExtension(sync_pb::password)) {
-    const sync_pb::EncryptedData& encrypted =
-        specifics.GetExtension(sync_pb::password).encrypted();
-    scoped_ptr<sync_pb::PasswordSpecificsData> data(
-        new sync_pb::PasswordSpecificsData);
-    if (!GetTransaction()->GetCryptographer()->Decrypt(encrypted,
-                                                       data.get()))
+    scoped_ptr<sync_pb::PasswordSpecificsData> data(DecryptPasswordSpecifics(
+        specifics, GetTransaction()->GetCryptographer()));
+    if (!data.get())
       return false;
     password_data_.swap(data);
   }
@@ -1022,6 +1032,7 @@ class SyncManager::SyncInternal
   void SetExtraChangeRecordData(int64 id,
                                 syncable::ModelType type,
                                 ChangeReorderBuffer* buffer,
+                                Cryptographer* cryptographer,
                                 const syncable::EntryKernel& original,
                                 bool existed_before,
                                 bool exists_now);
@@ -1289,6 +1300,8 @@ bool SyncManager::SyncInternal::Init(
   setup_for_test_mode_ = setup_for_test_mode;
 
   share_.dir_manager.reset(new DirectoryManager(database_location));
+  share_.dir_manager->cryptographer()->Bootstrap(
+      restored_key_for_bootstrapping);
 
   connection_manager_.reset(new SyncAPIServerConnectionManager(
       sync_server_and_path, port, use_ssl, user_agent, post_factory));
@@ -1696,12 +1709,22 @@ void SyncManager::SyncInternal::HandleCalculateChangesChangeEventFromSyncApi(
 
 void SyncManager::SyncInternal::SetExtraChangeRecordData(int64 id,
     syncable::ModelType type, ChangeReorderBuffer* buffer,
-    const syncable::EntryKernel& original, bool existed_before,
-    bool exists_now) {
+    Cryptographer* cryptographer, const syncable::EntryKernel& original,
+    bool existed_before, bool exists_now) {
   // If this is a deletion, attach the entity specifics as extra data
   // so that the delete can be processed.
   if (!exists_now && existed_before) {
     buffer->SetSpecificsForId(id, original.ref(SPECIFICS));
+    if (type == syncable::PASSWORDS) {
+      // Need to dig a bit deeper as passwords are encrypted.
+      scoped_ptr<sync_pb::PasswordSpecificsData> data(
+          DecryptPasswordSpecifics(original.ref(SPECIFICS), cryptographer));
+      if (!data.get()) {
+        NOTREACHED();
+        return;
+      }
+      buffer->SetExtraDataForId(id, new ExtraPasswordChangeRecordData(*data));
+    }
   }
 }
 
@@ -1735,7 +1758,8 @@ void SyncManager::SyncInternal::HandleCalculateChangesChangeEventFromSyncer(
     else if (exists_now && existed_before && VisiblePropertiesDiffer(*i, e))
       change_buffers_[type].PushUpdatedItem(id, VisiblePositionsDiffer(*i, e));
 
-    SetExtraChangeRecordData(id, type, &change_buffers_[type], *i,
+    SetExtraChangeRecordData(id, type, &change_buffers_[type],
+                             dir_manager()->cryptographer(), *i,
                              existed_before, exists_now);
   }
 }
diff --git a/chrome/browser/sync/engine/syncapi.h b/chrome/browser/sync/engine/syncapi.h
index a57ed32..e34b1f9 100644
--- a/chrome/browser/sync/engine/syncapi.h
+++ b/chrome/browser/sync/engine/syncapi.h
@@ -46,6 +46,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/scoped_ptr.h"
 #include "build/build_config.h"
+#include "chrome/browser/sync/protocol/password_specifics.pb.h"
 #include "chrome/browser/sync/syncable/model_type.h"
 #include "chrome/browser/sync/util/cryptographer.h"
 #include "chrome/common/net/gaia/google_service_auth_error.h"
@@ -554,6 +555,15 @@ class SyncManager {
   // internal types from clients of the interface.
   class SyncInternal;
 
+  // TODO(tim): Depending on how multi-type encryption pans out, maybe we
+  // should turn ChangeRecord itself into a class.  Or we could template this
+  // wrapper / add a templated method to return unencrypted protobufs.
+  class ExtraChangeRecordData {
+   public:
+    ExtraChangeRecordData() {}
+    virtual ~ExtraChangeRecordData() {}
+  };
+
   // ChangeRecord indicates a single item that changed as a result of a sync
   // operation.  This gives the sync id of the node that changed, and the type
   // of change.  To get the actual property values after an ADD or UPDATE, the
@@ -568,6 +578,21 @@ class SyncManager {
     int64 id;
     Action action;
     sync_pb::EntitySpecifics specifics;
+    linked_ptr<ExtraChangeRecordData> extra;
+  };
+
+  // Since PasswordSpecifics is just an encrypted blob, we extend to provide
+  // access to unencrypted bits.
+  class ExtraPasswordChangeRecordData : public ExtraChangeRecordData {
+   public:
+    ExtraPasswordChangeRecordData(const sync_pb::PasswordSpecificsData& data)
+        : unencrypted_(data) {}
+    virtual ~ExtraPasswordChangeRecordData() {}
+    const sync_pb::PasswordSpecificsData& unencrypted() {
+      return unencrypted_;
+    }
+   private:
+    sync_pb::PasswordSpecificsData unencrypted_;
   };
 
   // Status encapsulates detailed state about the internals of the SyncManager.
diff --git a/chrome/browser/sync/glue/password_change_processor.cc b/chrome/browser/sync/glue/password_change_processor.cc
index 93143e6..0a42a94 100644
--- a/chrome/browser/sync/glue/password_change_processor.cc
+++ b/chrome/browser/sync/glue/password_change_processor.cc
@@ -141,6 +141,21 @@ void PasswordChangeProcessor::ApplyChangesFromSyncModel(
   PasswordModelAssociator::PasswordVector deleted_passwords;
 
   for (int i = 0; i < change_count; ++i) {
+    if (sync_api::SyncManager::ChangeRecord::ACTION_DELETE ==
+        changes[i].action) {
+      DCHECK(changes[i].specifics.HasExtension(sync_pb::password))
+          << "Password specifics data not present on delete!";
+      DCHECK(changes[i].extra.get());
+      sync_api::SyncManager::ExtraPasswordChangeRecordData* extra =
+          static_cast<sync_api::SyncManager::ExtraPasswordChangeRecordData*>(
+              changes[i].extra.get());
+      const sync_pb::PasswordSpecificsData& password = extra->unencrypted();
+      webkit_glue::PasswordForm form;
+      PasswordModelAssociator::CopyPassword(password, &form);
+      deleted_passwords.push_back(form);
+      model_associator_->Disassociate(changes[i].id);
+      continue;
+    }
 
     sync_api::ReadNode sync_node(trans);
     if (!sync_node.InitByIdLookup(changes[i].id)) {
@@ -156,20 +171,19 @@ void PasswordChangeProcessor::ApplyChangesFromSyncModel(
     const sync_pb::PasswordSpecificsData& password_data =
         sync_node.GetPasswordSpecifics();
     webkit_glue::PasswordForm password;
-    PasswordModelAssociator::CopyPassword(password_data,
-                                          &password);
+    PasswordModelAssociator::CopyPassword(password_data, &password);
 
     if (sync_api::SyncManager::ChangeRecord::ACTION_ADD == changes[i].action) {
+      std::string tag(PasswordModelAssociator::MakeTag(password));
+      model_associator_->Associate(&tag, sync_node.GetId());
       new_passwords.push_back(password);
-    } else if (sync_api::SyncManager::ChangeRecord::ACTION_DELETE ==
-               changes[i].action) {
-      deleted_passwords.push_back(password);
     } else {
       DCHECK(sync_api::SyncManager::ChangeRecord::ACTION_UPDATE ==
              changes[i].action);
       updated_passwords.push_back(password);
     }
   }
+
   if (!model_associator_->WriteToPasswordStore(&new_passwords,
                                                &updated_passwords,
                                                &deleted_passwords)) {
diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index 1df0905..149f5c4 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -6,6 +6,7 @@
 
 #include <algorithm>
 
+#include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/task.h"
 #include "base/utf_string_conversions.h"
@@ -21,6 +22,7 @@
 #include "chrome/browser/sync/glue/http_bridge.h"
 #include "chrome/browser/sync/glue/password_model_worker.h"
 #include "chrome/browser/sync/sessions/session_state.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/net/gaia/gaia_constants.h"
 #include "chrome/common/notification_service.h"
@@ -102,7 +104,7 @@ void SyncBackendHost::Initialize(
     registrar_.workers[GROUP_PASSWORD] =
         new PasswordModelWorker(password_store);
   } else {
-    LOG(ERROR) << "Password store not initialized, cannot sync passwords";
+    LOG(WARNING) << "Password store not initialized, cannot sync passwords";
   }
 
   // Any datatypes that we want the syncer to pull down must
@@ -113,6 +115,13 @@ void SyncBackendHost::Initialize(
     registrar_.routing_info[(*it)] = GROUP_PASSIVE;
   }
 
+  // TODO(tim): This should be encryption-specific instead of passwords
+  // specific.  For now we have to do this to avoid NIGORI node lookups when
+  // we haven't downloaded that node.
+  if (profile_->GetPrefs()->GetBoolean(prefs::kSyncPasswords) ) {
+    registrar_.routing_info[syncable::NIGORI] = GROUP_PASSIVE;
+  }
+
   InitCore(Core::DoInitializeOptions(
       sync_service_url,
       MakeHttpBridgeFactory(baseline_context_getter),
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index b049dd8..c344b83 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -29,6 +29,7 @@
 #include "chrome/browser/sync/glue/data_type_manager.h"
 #include "chrome/browser/sync/glue/session_data_type_controller.h"
 #include "chrome/browser/sync/profile_sync_factory.h"
+#include "chrome/browser/sync/signin_manager.h"
 #include "chrome/browser/sync/syncable/directory_manager.h"
 #include "chrome/browser/sync/token_migrator.h"
 #include "chrome/browser/sync/util/user_settings.h"
@@ -195,16 +196,16 @@ void ProfileSyncService::Initialize() {
 void ProfileSyncService::RegisterAuthNotifications() {
   registrar_.Add(this,
                  NotificationType::TOKEN_AVAILABLE,
-                 NotificationService::AllSources());
+                 Source<TokenService>(profile_->GetTokenService()));
   registrar_.Add(this,
                  NotificationType::TOKEN_LOADING_FINISHED,
-                 NotificationService::AllSources());
+                 Source<TokenService>(profile_->GetTokenService()));
   registrar_.Add(this,
                  NotificationType::GOOGLE_SIGNIN_SUCCESSFUL,
-                 NotificationService::AllSources());
+                 Source<SigninManager>(&signin_));
   registrar_.Add(this,
                  NotificationType::GOOGLE_SIGNIN_FAILED,
-                 NotificationService::AllSources());
+                 Source<SigninManager>(&signin_));
 }
 
 void ProfileSyncService::RegisterDataTypeController(
@@ -316,6 +317,8 @@ void ProfileSyncService::RegisterPreferences() {
       enable_by_default);
   pref_service->RegisterBooleanPref(prefs::kSyncManaged, false);
   pref_service->RegisterStringPref(prefs::kEncryptionBootstrapToken, "");
+  pref_service->RegisterBooleanPref(prefs::kSyncUsingSecondaryPassphrase,
+      false);
 }
 
 void ProfileSyncService::ClearPreferences() {
@@ -811,8 +814,19 @@ bool ProfileSyncService::IsCryptographerReady() const {
 }
 
 void ProfileSyncService::SetPassphrase(const std::string& passphrase) {
-  DCHECK(backend_.get());
-  backend_->SetPassphrase(passphrase);
+  // TODO(tim): This should be encryption-specific instead of passwords
+  // specific.  For now we have to do this to avoid NIGORI node lookups when
+  // we haven't downloaded that node.
+  if (!profile_->GetPrefs()->GetBoolean(prefs::kSyncPasswords) ) {
+    LOG(WARNING) << "Silently dropping SetPassphrase request.";
+    return;
+  }
+
+  if (!sync_initialized()) {
+    cached_passphrase_ = passphrase;
+  } else {
+    backend_->SetPassphrase(passphrase);
+  }
 }
 
 void ProfileSyncService::ConfigureDataTypeManager() {
@@ -875,6 +889,12 @@ void ProfileSyncService::Observe(NotificationType type,
         return;
       }
 
+      if (!cached_passphrase_.empty()) {
+        // Don't hold on to the passphrase in raw form longer than needed.
+        SetPassphrase(cached_passphrase_);
+        cached_passphrase_.clear();
+      }
+
       // TODO(sync): Less wizard, more toast.
       wizard_.Step(SyncSetupWizard::DONE);
       FOR_EACH_OBSERVER(Observer, observers_, OnStateChanged());
@@ -882,8 +902,16 @@ void ProfileSyncService::Observe(NotificationType type,
       break;
     }
     case NotificationType::SYNC_PASSPHRASE_REQUIRED: {
+      DCHECK(backend_.get());
+      if (!cached_passphrase_.empty()) {
+        SetPassphrase(cached_passphrase_);
+        cached_passphrase_.clear();
+        break;
+      }
+
       // TODO(sync): Show the passphrase UI here.
-      SetPassphrase("dummy passphrase");
+      UpdateAuthErrorState(GoogleServiceAuthError(
+          GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS));
       break;
     }
     case NotificationType::SYNC_DATA_TYPES_UPDATED: {
@@ -917,14 +945,17 @@ void ProfileSyncService::Observe(NotificationType type,
       break;
     }
     case NotificationType::GOOGLE_SIGNIN_SUCCESSFUL: {
-      LOG(INFO) << "Signin OK. Waiting on tokens.";
-      // TODO(chron): UI update?
-      // TODO(chron): Timeout?
+      if (!profile_->GetPrefs()->GetBoolean(
+          prefs::kSyncUsingSecondaryPassphrase)) {
+        const GoogleServiceSigninSuccessDetails* successful =
+            (Details<const GoogleServiceSigninSuccessDetails>(details).ptr());
+        SetPassphrase(successful->password);
+      }
       break;
     }
     case NotificationType::GOOGLE_SIGNIN_FAILED: {
       GoogleServiceAuthError error =
-          *(Details<GoogleServiceAuthError>(details).ptr());
+          *(Details<const GoogleServiceAuthError>(details).ptr());
       UpdateAuthErrorState(error);
       break;
     }
diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h
index c221bfe..2b25e0c 100644
--- a/chrome/browser/sync/profile_sync_service.h
+++ b/chrome/browser/sync/profile_sync_service.h
@@ -463,6 +463,12 @@ class ProfileSyncService : public browser_sync::SyncFrontend,
 
   scoped_ptr<TokenMigrator> token_migrator_;
 
+  // Sometimes we need to temporarily hold on to a passphrase because we don't
+  // yet have a backend to send it to.  This happens during initialization as
+  // we don't StartUp until we have a valid token, which happens after valid
+  // credentials were provided.
+  std::string cached_passphrase_;
+
   DISALLOW_COPY_AND_ASSIGN(ProfileSyncService);
 };
 
diff --git a/chrome/common/pref_names.cc b/chrome/common/pref_names.cc
index d7031ae..84a8cc85 100644
--- a/chrome/common/pref_names.cc
+++ b/chrome/common/pref_names.cc
@@ -940,6 +940,10 @@ const char kSyncCredentialsMigrated[] = "sync.credentials_migrated";
 // startup so that the user doesn't need to provide credentials on each start.
 const char kEncryptionBootstrapToken[] = "sync.encryption_bootstrap_token";
 
+// Boolean tracking whether the user chose to specify a secondary encryption
+// passphrase.
+const char kSyncUsingSecondaryPassphrase[] = "sync.using_secondary_passphrase";
+
 // String that identifies the user logged into sync and other google services.
 const char kGoogleServicesUsername[] = "google.services.username";
 
diff --git a/chrome/common/pref_names.h b/chrome/common/pref_names.h
index 5f7b957..1bacef4 100644
--- a/chrome/common/pref_names.h
+++ b/chrome/common/pref_names.h
@@ -346,6 +346,7 @@ extern const char kSyncManaged[];
 extern const char kSyncSuppressStart[];
 extern const char kGoogleServicesUsername[];
 extern const char kSyncCredentialsMigrated[];
+extern const char kSyncUsingSecondaryPassphrase[];
 extern const char kEncryptionBootstrapToken[];
 
 extern const char kWebAppCreateOnDesktop[];