Revert 59618 - 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/3295026

TBR=tim@chromium.org
Review URL: http://codereview.chromium.org/3419003

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

10 files changed, 26 insertions, 153 deletions

diff --git a/chrome/browser/sync/engine/change_reorder_buffer.cc b/chrome/browser/sync/engine/change_reorder_buffer.cc
index 6af2399..f73d4a7 100644
--- a/chrome/browser/sync/engine/change_reorder_buffer.cc
+++ b/chrome/browser/sync/engine/change_reorder_buffer.cc
@@ -139,8 +139,6 @@ 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);
@@ -172,8 +170,6 @@ 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 e2e091a..0392679 100644
--- a/chrome/browser/sync/engine/change_reorder_buffer.h
+++ b/chrome/browser/sync/engine/change_reorder_buffer.h
@@ -13,7 +13,6 @@
 #include <map>
 #include <vector>
 
-#include "base/linked_ptr.h"
 #include "chrome/browser/sync/engine/syncapi.h"
 #include "chrome/browser/sync/protocol/sync.pb.h"
 
@@ -39,7 +38,6 @@ namespace sync_api {
 class ChangeReorderBuffer {
  public:
   typedef SyncManager::ChangeRecord ChangeRecord;
-  typedef SyncManager::ExtraChangeRecordData ExtraChangeRecordData;
 
   ChangeReorderBuffer();
   ~ChangeReorderBuffer();
@@ -68,10 +66,6 @@ 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;
   }
@@ -102,7 +96,6 @@ 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.
@@ -111,9 +104,6 @@ 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 50305e4..f392941 100644
--- a/chrome/browser/sync/engine/syncapi.cc
+++ b/chrome/browser/sync/engine/syncapi.cc
@@ -173,27 +173,17 @@ 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)) {
-    scoped_ptr<sync_pb::PasswordSpecificsData> data(DecryptPasswordSpecifics(
-        specifics, GetTransaction()->GetCryptographer()));
-    if (!data.get())
+    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()))
       return false;
     password_data_.swap(data);
   }
@@ -1032,7 +1022,6 @@ class SyncManager::SyncInternal
   void SetExtraChangeRecordData(int64 id,
                                 syncable::ModelType type,
                                 ChangeReorderBuffer* buffer,
-                                Cryptographer* cryptographer,
                                 const syncable::EntryKernel& original,
                                 bool existed_before,
                                 bool exists_now);
@@ -1300,8 +1289,6 @@ 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));
@@ -1712,22 +1699,12 @@ void SyncManager::SyncInternal::HandleCalculateChangesChangeEventFromSyncApi(
 
 void SyncManager::SyncInternal::SetExtraChangeRecordData(int64 id,
     syncable::ModelType type, ChangeReorderBuffer* buffer,
-    Cryptographer* cryptographer, const syncable::EntryKernel& original,
-    bool existed_before, bool exists_now) {
+    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));
-    }
   }
 }
 
@@ -1761,8 +1738,7 @@ 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],
-                             dir_manager()->cryptographer(), *i,
+    SetExtraChangeRecordData(id, type, &change_buffers_[type], *i,
                              existed_before, exists_now);
   }
 }
diff --git a/chrome/browser/sync/engine/syncapi.h b/chrome/browser/sync/engine/syncapi.h
index e34b1f9..a57ed32 100644
--- a/chrome/browser/sync/engine/syncapi.h
+++ b/chrome/browser/sync/engine/syncapi.h
@@ -46,7 +46,6 @@
 #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"
@@ -555,15 +554,6 @@ 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
@@ -578,21 +568,6 @@ 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 0a42a94..93143e6 100644
--- a/chrome/browser/sync/glue/password_change_processor.cc
+++ b/chrome/browser/sync/glue/password_change_processor.cc
@@ -141,21 +141,6 @@ 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)) {
@@ -171,19 +156,20 @@ 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 6fbcd9c..56c8b86 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -6,7 +6,6 @@
 
 #include <algorithm>
 
-#include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/task.h"
 #include "base/utf_string_conversions.h"
@@ -22,7 +21,6 @@
 #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"
@@ -104,7 +102,7 @@ void SyncBackendHost::Initialize(
     registrar_.workers[GROUP_PASSWORD] =
         new PasswordModelWorker(password_store);
   } else {
-    LOG(WARNING) << "Password store not initialized, cannot sync passwords";
+    LOG(ERROR) << "Password store not initialized, cannot sync passwords";
   }
 
   // Any datatypes that we want the syncer to pull down must
@@ -115,11 +113,6 @@ void SyncBackendHost::Initialize(
     registrar_.routing_info[(*it)] = GROUP_PASSIVE;
   }
 
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kEnableSyncPasswords)) {
-    registrar_.routing_info[syncable::NIGORI] = GROUP_PASSIVE;
-  }
-
   InitCore(Core::DoInitializeOptions(
       sync_service_url,
       MakeHttpBridgeFactory(baseline_context_getter),
@@ -168,15 +161,6 @@ void SyncBackendHost::StartSyncingWithServer() {
 }
 
 void SyncBackendHost::SetPassphrase(const std::string& 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 (!CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kEnableSyncPasswords)) {
-    LOG(WARNING) << "Silently dropping SetPassphrase request.";
-    return;
-  }
-
   core_thread_.message_loop()->PostTask(FROM_HERE,
       NewRunnableMethod(core_.get(), &SyncBackendHost::Core::DoSetPassphrase,
                         passphrase));
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index 677d788..1196647 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -29,7 +29,6 @@
 #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"
@@ -208,16 +207,16 @@ void ProfileSyncService::Initialize() {
 void ProfileSyncService::RegisterAuthNotifications() {
   registrar_.Add(this,
                  NotificationType::TOKEN_AVAILABLE,
-                 Source<TokenService>(profile_->GetTokenService()));
+                 NotificationService::AllSources());
   registrar_.Add(this,
                  NotificationType::TOKEN_LOADING_FINISHED,
-                 Source<TokenService>(profile_->GetTokenService()));
+                 NotificationService::AllSources());
   registrar_.Add(this,
                  NotificationType::GOOGLE_SIGNIN_SUCCESSFUL,
-                 Source<SigninManager>(&signin_));
+                 NotificationService::AllSources());
   registrar_.Add(this,
                  NotificationType::GOOGLE_SIGNIN_FAILED,
-                 Source<SigninManager>(&signin_));
+                 NotificationService::AllSources());
 }
 
 void ProfileSyncService::RegisterDataTypeController(
@@ -329,8 +328,6 @@ 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() {
@@ -818,11 +815,8 @@ bool ProfileSyncService::IsCryptographerReady() const {
 }
 
 void ProfileSyncService::SetPassphrase(const std::string& passphrase) {
-  if (!sync_initialized()) {
-    cached_passphrase_ = passphrase;
-  } else {
-    backend_->SetPassphrase(passphrase);
-  }
+  DCHECK(backend_.get());
+  backend_->SetPassphrase(passphrase);
 }
 
 void ProfileSyncService::ConfigureDataTypeManager() {
@@ -879,12 +873,6 @@ 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());
@@ -892,16 +880,8 @@ 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.
-      UpdateAuthErrorState(GoogleServiceAuthError(
-          GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS));
+      SetPassphrase("dummy passphrase");
       break;
     }
     case NotificationType::SYNC_DATA_TYPES_UPDATED: {
@@ -935,17 +915,14 @@ void ProfileSyncService::Observe(NotificationType type,
       break;
     }
     case NotificationType::GOOGLE_SIGNIN_SUCCESSFUL: {
-      if (!profile_->GetPrefs()->GetBoolean(
-          prefs::kSyncUsingSecondaryPassphrase)) {
-        const GoogleServiceSigninSuccessDetails* successful =
-            (Details<const GoogleServiceSigninSuccessDetails>(details).ptr());
-        SetPassphrase(successful->password);
-      }
+      LOG(INFO) << "Signin OK. Waiting on tokens.";
+      // TODO(chron): UI update?
+      // TODO(chron): Timeout?
       break;
     }
     case NotificationType::GOOGLE_SIGNIN_FAILED: {
       GoogleServiceAuthError error =
-          *(Details<const GoogleServiceAuthError>(details).ptr());
+          *(Details<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 2b25e0c..c221bfe 100644
--- a/chrome/browser/sync/profile_sync_service.h
+++ b/chrome/browser/sync/profile_sync_service.h
@@ -463,12 +463,6 @@ 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 84a8cc85..d7031ae 100644
--- a/chrome/common/pref_names.cc
+++ b/chrome/common/pref_names.cc
@@ -940,10 +940,6 @@ 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 1bacef4..5f7b957 100644
--- a/chrome/common/pref_names.h
+++ b/chrome/common/pref_names.h
@@ -346,7 +346,6 @@ 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[];