[Sync] Fix encryption/passphrase handling.

chrome/browser/sync/profile_sync_service.cc

Index: chrome/browser/sync/profile_sync_service.cc
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index 061ded400da2a3dee79d8849d884c3ee81d938ed..a5772a2abdf4485bedb0a36943a10ba124c0afc6 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -368,6 +368,8 @@ void ProfileSyncService::CreateBackend() {
 }
 
 bool ProfileSyncService::IsEncryptedDatatypeEnabled() const {
+  if (!pending_types_for_encryption_.empty())
+    return true;
   syncable::ModelTypeSet preferred_types;
   GetPreferredDataTypes(&preferred_types);
   syncable::ModelTypeSet encrypted_types;
@@ -686,7 +688,7 @@ void ProfileSyncService::OnPassphraseRequired(
   // passphrase is needed for decryption but the user is not syncing an
   // encrypted data type on this machine. Otherwise we look for one.
   if (!IsEncryptedDatatypeEnabled() && IsPassphraseRequiredForDecryption()) {
-    VLOG(1) << "Not decrypting and no encrypted datatypes enabled"
+    VLOG(1) << "Decrypting and no encrypted datatypes enabled"
             << ", accepted passphrase.";
     OnPassphraseAccepted();
   }
@@ -694,8 +696,10 @@ void ProfileSyncService::OnPassphraseRequired(
   // First try supplying gaia password as the passphrase.
   if (!gaia_password_.empty()) {
     VLOG(1) << "Attempting gaia passphrase.";
-    SetPassphrase(gaia_password_, false, true);
+    std::string pass = gaia_password_;

[tim (not reviewing), 2011/08/04 16:57:56]

Comment why we need to do this move-aside?

[Nicolas Zea, 2011/08/04 18:43:54]

Done.

     gaia_password_ = std::string();
+    // May set gaia_password_ if the syncer isn't ready.
+    SetPassphrase(pass, false, true);
     return;
   }
 
@@ -703,10 +707,12 @@ void ProfileSyncService::OnPassphraseRequired(
   // entered in setup.
   if (!cached_passphrase_.value.empty()) {
     VLOG(1) << "Attempting cached passphrase.";
-    SetPassphrase(cached_passphrase_.value,
-                  cached_passphrase_.is_explicit,
-                  cached_passphrase_.is_creation);
+    std::string pass = cached_passphrase_.value;
+    bool is_explicit = cached_passphrase_.is_explicit;
+    bool is_creation = cached_passphrase_.is_creation;
     cached_passphrase_ = CachedPassphrase();
+    // May set cached_passphrase_ is the syncer isn't ready.
+    SetPassphrase(pass, is_explicit, is_creation);
     return;
   }
 
@@ -722,6 +728,10 @@ void ProfileSyncService::OnPassphraseRequired(
 
 void ProfileSyncService::OnPassphraseAccepted() {
   VLOG(1) << "Received OnPassphraseAccepted.";
+  // Don't hold on to a passphrase in raw form longer than needed.
+  gaia_password_ = std::string();
+  cached_passphrase_ = CachedPassphrase();
+
   // Make sure the data types that depend on the passphrase are started at
   // this time.
   syncable::ModelTypeSet types;
@@ -744,6 +754,12 @@ void ProfileSyncService::OnPassphraseAccepted() {
 
 void ProfileSyncService::OnEncryptionComplete(
     const syncable::ModelTypeSet& encrypted_types) {
+  if (!pending_types_for_encryption_.empty()) {
+    // The user had chosen to encrypt datatypes. This is the last thing to
+    // complete, so now that we're done notify the UI.
+    wizard_.Step(SyncSetupWizard::DONE);
+  }
+  pending_types_for_encryption_.clear();
   NotifyObservers();
 }
 
@@ -1167,7 +1183,7 @@ void ProfileSyncService::SetPassphrase(const std::string& passphrase,
                                        bool is_creation) {
   if (ShouldPushChanges() || IsPassphraseRequired()) {
     VLOG(1) << "Setting " << (is_explicit ? "explicit" : "implicit")
-            << " passphrase " << (is_creation ? " for creation" : "");
+            << " passphrase" << (is_creation ? " for creation" : "");
     backend_->SetPassphrase(passphrase, is_explicit);
   } else {
     if (is_explicit) {
@@ -1182,15 +1198,22 @@ void ProfileSyncService::SetPassphrase(const std::string& passphrase,
 
 void ProfileSyncService::EncryptDataTypes(

[tim (not reviewing), 2011/08/04 16:57:56]

W.r.t your comment below, lets call this set_pending_types_for_encryption() so
it's clear to callers it doesn't do anything fancy aside from set the value.

[Nicolas Zea, 2011/08/04 18:43:54]

Done.

     const syncable::ModelTypeSet& encrypted_types) {
-  if (HasSyncSetupCompleted()) {
-    backend_->EncryptDataTypes(encrypted_types);
+  if (encrypted_types.empty()) {
+    // We can't unencrypt types.
+    VLOG(1) << "No datatypes set for encryption, dropping encryption request.";
     pending_types_for_encryption_.clear();
-  } else {
-    pending_types_for_encryption_ = encrypted_types;
+    return;
   }
+  // We require that every call to EncryptDatatypes be coupled with a
+  // ConfigureDataTypeManager call. The actual call to encrypt the datatypes
+  // happens after SYNC_CONFIGURE_DONE, to ensure we've downloaded any new
+  // datatypes before beginning encryption.
+  // TODO(zea): This is very brittle and relies on ConfigureDataTypes being
+  // called after the encryptdatatypes in sync_setup_flow.cc. Fix this.
+  pending_types_for_encryption_ = encrypted_types;
 }
 
-// This would open a transaction to get the encrypted types. Do not call this
+// This will open a transaction to get the encrypted types. Do not call this
 // if you already have a transaction open.
 void ProfileSyncService::GetEncryptedDataTypes(
     syncable::ModelTypeSet* encrypted_types) const {
@@ -1228,8 +1251,6 @@ void ProfileSyncService::Observe(int type,
         expect_sync_configuration_aborted_ = false;
         return;
       }
-      // Clear out the gaia password if it is already there.
-      gaia_password_ = std::string();
       if (status != DataTypeManager::OK) {
         VLOG(0) << "ProfileSyncService::Observe: Unrecoverable error detected";
         std::string message =
@@ -1241,32 +1262,25 @@ void ProfileSyncService::Observe(int type,
         return;
       }
 
-      // If the user had entered a custom passphrase use it now.
-      if (!cached_passphrase_.value.empty()) {
-        // Don't hold on to the passphrase in raw form longer than needed.
-        SetPassphrase(cached_passphrase_.value,
-                      cached_passphrase_.is_explicit,
-                      cached_passphrase_.is_creation);
-        cached_passphrase_ = CachedPassphrase();
-      }
-
       // We should never get in a state where we have no encrypted datatypes
       // enabled, and yet we still think we require a passphrase for decryption.
       DCHECK(!(IsPassphraseRequiredForDecryption() &&
                !IsEncryptedDatatypeEnabled()));
 
-      // TODO(sync): Less wizard, more toast.
-      wizard_.Step(SyncSetupWizard::DONE);
-      NotifyObservers();
-
       // In the old world, this would be a no-op.  With new syncer thread,
       // this is the point where it is safe to switch from config-mode to
       // normal operation.
       backend_->StartSyncingWithServer();
 
-      if (!pending_types_for_encryption_.empty()) {
-        EncryptDataTypes(pending_types_for_encryption_);
-        pending_types_for_encryption_.clear();
+      if (pending_types_for_encryption_.empty()) {
+        // TODO(sync): Less wizard, more toast.

[tim (not reviewing), 2011/08/04 16:57:56]

nit - just nuke this todo, it's obsolete.

[Nicolas Zea, 2011/08/04 18:43:54]

Done.

+        wizard_.Step(SyncSetupWizard::DONE);
+        NotifyObservers();
+      } else {
+        // Will clear pending_types_for_encryption_ on success (via
+        // OnEncryptionComplete). Has no effect if pending_types_for_encryption_
+        // matches the encrypted types.

[tim (not reviewing), 2011/08/04 16:57:56]

if they match, does it still clear?

[Nicolas Zea, 2011/08/04 18:43:54]

Yes, added comment.

On 2011/08/04 16:57:56, timsteele wrote:

+        backend_->EncryptDataTypes(pending_types_for_encryption_);
       }
       break;
     }