[Sync] Fix encryption/passphrase handling.

We now ensure cached passphrases are always used. In addition, encryption now
happens after configuration, allowing us to make use of the
pending_encrypted_types to know if we're waiting for encryption (which
affects how we prompt for passphrase). This is dependent on sync_setup_flow.cc
always calling EncryptDataTypes before configuring the new datatypes and setting
pending_encrypted_types_ appropriately, which then gets consumed on
SYNC_CONFIGURE_DONE.

BUG=91314
TEST=see bug

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

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

chrome/browser/sync/engine/syncapi.cc

@@ -1641,7 +1641,33 @@ SyncManager::Observer::~Observer() {}
 SyncManager::SyncManager(const std::string& name)
     : data_(new SyncInternal(name)) {}
 
-SyncManager::Status::Status() {
+SyncManager::Status::Status()
+    : summary(INVALID),
+      authenticated(false),
+      server_up(false),
+      server_reachable(false),
+      server_broken(false),
+      notifications_enabled(false),
+      notifications_received(0),
+      notifiable_commits(0),
+      max_consecutive_errors(0),
+      unsynced_count(0),
+      conflicting_count(0),
+      syncing(false),
+      initial_sync_ended(false),
+      syncer_stuck(false),
+      updates_available(0),
+      updates_received(0),
+      tombstone_updates_received(0),
+      disk_full(false),
+      num_local_overwrites_total(0),
+      num_server_overwrites_total(0),
+      nonempty_get_updates(0),
+      empty_get_updates(0),
+      useless_sync_cycles(0),
+      useful_sync_cycles(0),
+      cryptographer_ready(false),
+      crypto_has_pending_keys(false) {
 }
 
 SyncManager::Status::~Status() {
@@ -2096,12 +2122,18 @@ void SyncManager::SyncInternal::EncryptDataTypes(
   Cryptographer* cryptographer = trans.GetCryptographer();
 
   if (!cryptographer->is_initialized()) {
-    NOTREACHED() << "Attempting to encrypt datatypes when cryptographer not "
-                 << "initialized.";
+    VLOG(1) << "Attempting to encrypt datatypes when cryptographer not "
+            << "initialized, prompting for passphrase.";
+    ObserverList<SyncManager::Observer> temp_obs_list;
+    CopyObservers(&temp_obs_list);
+    // TODO(zea): this isn't really decryption, but that's the only way we have
+    // to prompt the user for a passsphrase. See http://crbug.com/91379.
+    FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+                      OnPassphraseRequired(sync_api::REASON_DECRYPTION));
     return;
   }
 
-  // Update the Nigori node set of encrypted datatypes so other machines notice.
+  // Update the Nigori node's set of encrypted datatypes.
   // Note, we merge the current encrypted types with those requested. Once a
   // datatypes is marked as needing encryption, it is never unmarked.
   sync_pb::NigoriSpecifics nigori;
@@ -2113,8 +2145,14 @@ void SyncManager::SyncInternal::EncryptDataTypes(
                  std::inserter(newly_encrypted_types,
                                newly_encrypted_types.begin()));
   allstatus_.SetEncryptedTypes(newly_encrypted_types);
-  if (newly_encrypted_types == current_encrypted_types)
-    return;  // Set of encrypted types did not change.
+  if (newly_encrypted_types == current_encrypted_types) {
+    // Set of encrypted types has not changed, just notify and return.
+    ObserverList<SyncManager::Observer> temp_obs_list;
+    CopyObservers(&temp_obs_list);
+    FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+                      OnEncryptionComplete(current_encrypted_types));
+    return;
+  }
   syncable::FillNigoriEncryptedTypes(newly_encrypted_types, &nigori);
   node.SetNigoriSpecifics(nigori);
 
@@ -2159,7 +2197,7 @@ void SyncManager::SyncInternal::ReEncryptEverything(WriteTransaction* trans) {
       WriteNode child(trans);
       if (!child.InitByIdLookup(child_id)) {
         NOTREACHED();
-        return;
+        continue;
       }
       if (child.GetIsFolder()) {
         to_visit.push(child.GetFirstChildId());
@@ -2178,20 +2216,19 @@ void SyncManager::SyncInternal::ReEncryptEverything(WriteTransaction* trans) {
     ReadNode passwords_root(trans);
     std::string passwords_tag =
         syncable::ModelTypeToRootTag(syncable::PASSWORDS);
-    if (!passwords_root.InitByTagLookup(passwords_tag)) {
-      LOG(WARNING) << "No passwords to reencrypt.";
-      return;
-    }
-
-    int64 child_id = passwords_root.GetFirstChildId();
-    while (child_id != kInvalidId) {
-      WriteNode child(trans);
-      if (!child.InitByIdLookup(child_id)) {
-        NOTREACHED();
-        return;
+    // It's possible we'll have the password routing info and not the password
+    // root if we attempted to SetPassphrase before passwords was enabled.
+    if (passwords_root.InitByTagLookup(passwords_tag)) {
+      int64 child_id = passwords_root.GetFirstChildId();
+      while (child_id != kInvalidId) {
+        WriteNode child(trans);
+        if (!child.InitByIdLookup(child_id)) {
+          NOTREACHED();
+          return;
+        }
+        child.SetPasswordSpecifics(child.GetPasswordSpecifics());
+        child_id = child.GetSuccessorId();
       }
-      child.SetPasswordSpecifics(child.GetPasswordSpecifics());
-      child_id = child.GetSuccessorId();
     }
   }
 

chrome/browser/sync/engine/syncapi_unittest.cc

@@ -1342,7 +1342,7 @@ TEST_F(SyncManagerTest, EncryptDataTypesWithData) {
   // Trigger's a ReEncryptEverything with new passphrase.
   testing::Mock::VerifyAndClearExpectations(&observer_);
   EXPECT_CALL(observer_, OnPassphraseAccepted(_)).Times(1);
-  EXPECT_CALL(observer_, OnEncryptionComplete(_)).Times(1);
+  EXPECT_CALL(observer_, OnEncryptionComplete(encrypted_types)).Times(1);
   sync_manager_.SetPassphrase("new_passphrase", true);
   {
     ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
@@ -1360,11 +1360,12 @@ TEST_F(SyncManagerTest, EncryptDataTypesWithData) {
                                                    syncable::THEMES,
                                                    false /* not encrypted */));
   }
-  // Calling EncryptDataTypes with the same encrypted types should not trigger
-  // a re-encryption.
+  // Calling EncryptDataTypes with an empty encrypted types should not trigger
+  // a reencryption and should just notify immediately.
+  // TODO(zea): add logic to ensure nothing was written.
   testing::Mock::VerifyAndClearExpectations(&observer_);
   EXPECT_CALL(observer_, OnPassphraseAccepted(_)).Times(0);
-  EXPECT_CALL(observer_, OnEncryptionComplete(_)).Times(0);
+  EXPECT_CALL(observer_, OnEncryptionComplete(encrypted_types)).Times(1);
   sync_manager_.EncryptDataTypes(encrypted_types);
 }
 

chrome/browser/sync/profile_sync_service.cc

@@ -376,6 +376,8 @@ void ProfileSyncService::CreateBackend() {
 }
 
 bool ProfileSyncService::IsEncryptedDatatypeEnabled() const {
+  if (HasPendingEncryptedTypes())
+    return true;
   syncable::ModelTypeSet preferred_types;
   GetPreferredDataTypes(&preferred_types);
   syncable::ModelTypeSet encrypted_types;
@@ -691,31 +693,39 @@ void ProfileSyncService::OnPassphraseRequired(
           << sync_api::PassphraseRequiredReasonToString(reason);
   passphrase_required_reason_ = reason;
 
+  // Store any passphrases we have into temps and clear out the originals so
+  // we don't hold on to the passphrases any longer than we have to. We
+  // then use the passphrases if they're present (after OnPassphraseAccepted).
+  std::string gaia_password = gaia_password_;
+  std::string cached_passphrase = cached_passphrase_.value;
+  bool is_explicit = cached_passphrase_.is_explicit;
+  bool is_creation = cached_passphrase_.is_creation;
+  gaia_password_ = std::string();
+  cached_passphrase_ = CachedPassphrase();
+
   // We will skip the passphrase prompt and suppress the warning if the
   // 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();
   }
 
   // First try supplying gaia password as the passphrase.
-  if (!gaia_password_.empty()) {
+  if (!gaia_password.empty()) {
     VLOG(1) << "Attempting gaia passphrase.";
-    SetPassphrase(gaia_password_, false, true);
-    gaia_password_ = std::string();
+    // SetPassphrase will set gaia_password_ if the syncer isn't ready.
+    SetPassphrase(gaia_password, false, true);
     return;
   }
 
   // If the above failed then try the custom passphrase the user might have
   // entered in setup.
-  if (!cached_passphrase_.value.empty()) {
+  if (!cached_passphrase.empty()) {
     VLOG(1) << "Attempting cached passphrase.";
-    SetPassphrase(cached_passphrase_.value,
-                  cached_passphrase_.is_explicit,
-                  cached_passphrase_.is_creation);
-    cached_passphrase_ = CachedPassphrase();
+    // SetPassphrase will set cached_passphrase_ if the syncer isn't ready.
+    SetPassphrase(cached_passphrase, is_explicit, is_creation);
     return;
   }
 
@@ -731,6 +741,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;
@@ -753,6 +767,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();
 }
 
@@ -1182,7 +1202,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) {
@@ -1195,17 +1215,21 @@ void ProfileSyncService::SetPassphrase(const std::string& passphrase,
   }
 }
 
-void ProfileSyncService::EncryptDataTypes(
+void ProfileSyncService::set_pending_types_for_encryption(
     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;
   }
+  // Setting the pending types for encryption doesn't actually trigger
+  // encryption. The actual encryption will occur after the datatype manager
+  // is reconfigured.
+  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 {
@@ -1222,6 +1246,10 @@ void ProfileSyncService::GetEncryptedDataTypes(
   }
 }
 
+bool ProfileSyncService::HasPendingEncryptedTypes() const {
+  return !pending_types_for_encryption_.empty();
+}
+
 void ProfileSyncService::Observe(int type,
                                  const NotificationSource& source,
                                  const NotificationDetails& details) {
@@ -1243,8 +1271,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 =
@@ -1256,32 +1282,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()) {
+        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 (and will clear
+        // pending_types_for_encryption_).
+        backend_->EncryptDataTypes(pending_types_for_encryption_);
       }
       break;
     }

chrome/browser/sync/profile_sync_service.h

@@ -446,13 +446,12 @@ class ProfileSyncService : public browser_sync::SyncFrontend,
                              bool is_explicit,
                              bool is_creation);
 
-  // Changes the set of datatypes that require encryption. This affects all
-  // machines synced to this account and all data belonging to the specified
-  // types.
-  // Note that this is an asynchronous operation (the encryption of data is
-  // performed on SyncBackendHost's core thread) and may not have an immediate
-  // effect.
-  virtual void EncryptDataTypes(
+  // Sets the set of datatypes that are waiting for encryption
+  // (pending_types_for_encryption_).
+  // Note that this does not trigger the actual encryption. The encryption call
+  // is kicked off automatically the next time the datatype manager is
+  // reconfigured.
+  virtual void set_pending_types_for_encryption(
       const syncable::ModelTypeSet& encrypted_types);
 
   // Get the currently encrypted data types.
@@ -461,6 +460,9 @@ class ProfileSyncService : public browser_sync::SyncFrontend,
   virtual void GetEncryptedDataTypes(
       syncable::ModelTypeSet* encrypted_types) const;
 
+  // Returns true if the syncer is waiting for new datatypes to be encrypted.
+  virtual bool HasPendingEncryptedTypes() const;
+
   // Returns whether processing changes is allowed.  Check this before doing
   // any model-modifying operations.
   bool ShouldPushChanges();
@@ -636,9 +638,8 @@ class ProfileSyncService : public browser_sync::SyncFrontend,
   base::OneShotTimer<ProfileSyncService> clear_server_data_timer_;
 
   // The most recently requested set of types to encrypt. Set by the user,
-  // and cached if the syncer was unable to encrypt new types (for example
-  // because we haven't finished initializing). Cleared when we successfully
-  // post a new encrypt task to the sync backend.
+  // and cached until the syncer either finishes encryption
+  // (OnEncryptionComplete) or the user cancels.
   syncable::ModelTypeSet pending_types_for_encryption_;
 
   scoped_ptr<browser_sync::BackendMigrator> migrator_;

chrome/browser/sync/profile_sync_service_harness.cc

@@ -796,7 +796,16 @@ bool ProfileSyncServiceHarness::EnableEncryptionForType(
   if (encrypted_types.count(type) > 0)
     return true;
   encrypted_types.insert(type);
-  service_->EncryptDataTypes(encrypted_types);
+  service_->set_pending_types_for_encryption(encrypted_types);
+
+  // In order to kick off the encryption we have to reconfigure. Just grab the
+  // currently synced types and use them.
+  syncable::ModelTypeSet synced_datatypes;
+  service_->GetPreferredDataTypes(&synced_datatypes);
+  bool sync_everything = (synced_datatypes.size() ==
+      (syncable::MODEL_TYPE_COUNT - syncable::FIRST_REAL_MODEL_TYPE));
+  service_->OnUserChoseDatatypes(sync_everything,
+                                 synced_datatypes);
 
   // Wait some time to let the enryption finish.
   return WaitForTypeEncryption(type);

chrome/browser/sync/sync_setup_flow.cc

@@ -175,6 +175,8 @@ void SyncSetupFlow::GetArgsForConfigure(ProfileSyncService* service,
   service->GetEncryptedDataTypes(&encrypted_types);
   bool encrypt_all =
       encrypted_types.upper_bound(syncable::PASSWORDS) != encrypted_types.end();
+  if (service->HasPendingEncryptedTypes())
+    encrypt_all = true;
   args->SetBoolean("encryptAllData", encrypt_all);
 
   // Load the parameters for the encryption tab.
@@ -267,16 +269,13 @@ void SyncSetupFlow::OnUserConfigured(const SyncConfiguration& configuration) {
   // Go to the "loading..." screen.
   Advance(SyncSetupWizard::SETTING_UP);
 
+  // Note: encryption will not occur until OnUserChoseDatatypes is called.
+  syncable::ModelTypeSet encrypted_types;
   if (configuration.encrypt_all) {
-    syncable::ModelTypeSet data_types;
-    service_->GetRegisteredDataTypes(&data_types);
-    service_->EncryptDataTypes(data_types);
-  }
-
-  // If we are activating the passphrase, we need to have one supplied.
-  DCHECK(service_->IsUsingSecondaryPassphrase() ||
-         !configuration.use_secondary_passphrase ||
-         configuration.secondary_passphrase.length() > 0);
+    // Encrypt all registered types.
+    service_->GetRegisteredDataTypes(&encrypted_types);
+  }  // Else we clear the pending types for encryption.
+  service_->set_pending_types_for_encryption(encrypted_types);
 
   if (!configuration.gaia_passphrase.empty()) {
     // Caller passed a gaia passphrase. This is illegal if we are currently
@@ -285,6 +284,15 @@ void SyncSetupFlow::OnUserConfigured(const SyncConfiguration& configuration) {
     service_->SetPassphrase(configuration.gaia_passphrase, false, false);
   }
 
+  // It's possible the user has to provide a secondary passphrase even when
+  // they have not set one previously. This occurs when the user has changed
+  // their gaia password and then sign in to a new machine for the first time.
+  // The new machine will download data encrypted with their old gaia password,
+  // which their current gaia password will not be able to decrypt, triggering
+  // a prompt for a passphrase. At this point, the user must enter their old
+  // password, which we store as a new secondary passphrase.
+  // TODO(zea): eventually use the above gaia_passphrase instead of the
+  // secondary passphrase in this case.
   if (configuration.use_secondary_passphrase) {
     if (!service_->IsUsingSecondaryPassphrase()) {
       service_->SetPassphrase(configuration.secondary_passphrase, true, true);