Refactor sync passphrase setup flow and fix passphrase tests

chrome/browser/sync/profile_sync_service.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/profile_sync_service.h"
 
 #include <stddef.h>
 #include <map>
 #include <ostream>
 #include <set>
@@ skipping 51 matching lines @@
 const char* ProfileSyncService::kDevServerUrl =
     "https://clients4.google.com/chrome-sync/dev";
 
 static const int kSyncClearDataTimeoutInSeconds = 60;  // 1 minute.
 
 ProfileSyncService::ProfileSyncService(ProfileSyncFactory* factory,
                                        Profile* profile,
                                        const std::string& cros_user)
     : last_auth_error_(AuthError::None()),
       observed_passphrase_required_(false),
-      passphrase_required_for_decryption_(false),
+      passphrase_required_reason_(sync_api::REASON_PASSPHRASE_NOT_REQUIRED),
       passphrase_migration_in_progress_(false),
       factory_(factory),
       profile_(profile),
       cros_user_(cros_user),
       sync_service_url_(kDevServerUrl),
       backend_initialized_(false),
       is_auth_in_progress_(false),
       wizard_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       unrecoverable_error_detected_(false),
       scoped_runnable_method_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
@@ skipping 289 matching lines @@
   scoped_ptr<SyncBackendHost> doomed_backend(backend_.release());
   if (doomed_backend.get()) {
     doomed_backend->Shutdown(sync_disabled);
 
     doomed_backend.reset();
   }
 
   // Clear various flags.
   is_auth_in_progress_ = false;
   backend_initialized_ = false;
   observed_passphrase_required_ = false;

[Nicolas Zea, 2011/04/29 22:59:30]

Add clear of passphrase reason here too?

[Raghu Simha, 2011/04/30 00:43:06]

Done.

   last_attempted_user_email_.clear();
   last_auth_error_ = GoogleServiceAuthError::None();
 }
 
 void ProfileSyncService::ClearServerData() {
   clear_server_data_state_ = CLEAR_CLEARING;
   clear_server_data_timer_.Start(
       base::TimeDelta::FromSeconds(kSyncClearDataTimeoutInSeconds), this,
       &ProfileSyncService::OnClearServerDataTimeout);
   backend_->RequestClearServerData();
@@ skipping 192 matching lines @@
   clear_server_data_timer_.Stop();
 
   // Even if the timout fired, we still transition to the succeeded state as
   // we want UI to update itself and no longer allow the user to press "clear"
   if (clear_server_data_state_ != CLEAR_SUCCEEDED) {
     clear_server_data_state_ = CLEAR_SUCCEEDED;
     NotifyObservers();
   }
 }
 
-void ProfileSyncService::OnPassphraseRequired(bool for_decryption) {
+void ProfileSyncService::OnPassphraseRequired(
+    sync_api::PassphraseRequiredReason reason) {
   DCHECK(backend_.get());
   DCHECK(backend_->IsNigoriEnabled());
 
   // TODO(lipalani) : add this check to other locations as well.
   if (unrecoverable_error_detected_) {
     // When unrecoverable error is detected we post a task to shutdown the
     // backend. The task might not have executed yet.
     return;
   }
   observed_passphrase_required_ = true;

[tim (not reviewing), 2011/04/29 23:02:13]

can we eliminate this and just check passphrase_reason_ != NOT_REQUIRED ?

We can leave the observed_passphrase_required() method and just change it to
mean this under the covers.

[Raghu Simha, 2011/04/30 00:43:06]

Done.

-  passphrase_required_for_decryption_ = for_decryption;
+
+  // Set passphrase_required_reason_ based on whether the passphrase is required
+  // for encryption or for decryption with a cached passphrase. If decryption
+  // failed and a new passphrase is required, the flag will already be set to
+  // REASON_SET_PASSPHRASE_FAILED.

[Nicolas Zea, 2011/04/29 22:59:30]

That's not true right? If the passphrase changes and we can't decrypt, syncapi
will trigger OnPassphraseRequired(reason..decryption)

[Raghu Simha, 2011/04/30 00:43:06]

The comment is outdated, misleading and probably unnecessary. Removed it, since
right now, all we're doing is simply storing the value of |reason|, whatever it
might be.

+  passphrase_required_reason_ = reason;
 
   if (!cached_passphrase_.value.empty()) {
     SetPassphrase(cached_passphrase_.value,
                   cached_passphrase_.is_explicit,
                   cached_passphrase_.is_creation);
     cached_passphrase_ = CachedPassphrase();
     return;
   }
 
-  // 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 prompt.
-  if (!IsEncryptedDatatypeEnabled() && for_decryption) {
+  // 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 prompt.
+  if (!IsEncryptedDatatypeEnabled() &&
+      (reason == sync_api::REASON_DECRYPTION ||
+       reason == sync_api::REASON_SET_PASSPHRASE_FAILED)) {
     OnPassphraseAccepted();
     return;
   }
 
-  if (WizardIsVisible() && for_decryption) {
+  if (WizardIsVisible() &&
+      (reason == sync_api::REASON_DECRYPTION ||
+       reason == sync_api::REASON_SET_PASSPHRASE_FAILED)) {
     wizard_.Step(SyncSetupWizard::ENTER_PASSPHRASE);
   }
 
   NotifyObservers();
 }
 
 void ProfileSyncService::OnPassphraseAccepted() {
   // Make sure the data types that depend on the passphrase are started at
   // this time.
   syncable::ModelTypeSet types;
   GetPreferredDataTypes(&types);
-  // Reset "passphrase_required" flag before configuring the DataTypeManager
-  // since we know we no longer require the passphrase.
+
+  // Reset passphrase flags before configuring the DataTypeManager since we know
+  // we no longer require the passphrase.
   observed_passphrase_required_ = false;
+  passphrase_required_reason_ = sync_api::REASON_PASSPHRASE_NOT_REQUIRED;
+
   if (data_type_manager_.get())
     data_type_manager_->Configure(types);
 
   NotifyObservers();
 
   wizard_.Step(SyncSetupWizard::DONE);
 }
 
 void ProfileSyncService::OnEncryptionComplete(
     const syncable::ModelTypeSet& encrypted_types) {
@@ skipping 324 matching lines @@
   syncable::ModelTypeSet types;
   GetPreferredDataTypes(&types);
   // We set this special case here since it's the only datatype whose encryption
   // status we already know. All others are set after the initial sync
   // completes (for now).
   // TODO(zea): Implement a better way that uses preferences for which types
   // need encryption.
   encrypted_types_.clear();
   if (types.count(syncable::PASSWORDS) > 0)
     encrypted_types_.insert(syncable::PASSWORDS);
-  if (observed_passphrase_required_ && passphrase_required_for_decryption_) {
+  if (observed_passphrase_required_ &&
+      (passphrase_required_reason_ == sync_api::REASON_DECRYPTION ||
+       passphrase_required_reason_ == sync_api::REASON_SET_PASSPHRASE_FAILED)) {
     if (IsEncryptedDatatypeEnabled()) {
       // We need a passphrase still. Prompt the user for a passphrase, and
       // DataTypeManager::Configure() will get called once the passphrase is
       // accepted.
-      OnPassphraseRequired(true);
+      OnPassphraseRequired(passphrase_required_reason_);
       return;
     } else {
       // We've been informed that a passphrase is required for decryption, but
       // now there are no encrypted data types enabled, so clear the flag
       // (NotifyObservers() will be called when configuration completes).
       observed_passphrase_required_ = false;
     }
   }
   data_type_manager_->Configure(types);
 }
@@ skipping 148 matching lines @@
         // 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.
       DCHECK(!(observed_passphrase_required_ &&
-               passphrase_required_for_decryption_ &&
+               (passphrase_required_reason_ == sync_api::REASON_DECRYPTION ||
+                passphrase_required_reason_ ==
+                    sync_api::REASON_SET_PASSPHRASE_FAILED) &&
                !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();
@@ skipping 26 matching lines @@
       // request and shouldn't override an explicit one.  Thus, we either
       // update the implicit passphrase (idempotent if the passphrase didn't
       // actually change), or the user has an explicit passphrase set so this
       // becomes a no-op.
       tried_implicit_gaia_remove_when_bug_62103_fixed_ = true;
       SetPassphrase(successful->password, false, true);
 
       // If this signin was to initiate a passphrase migration (on the
       // first computer, thus not for decryption), continue the migration.
       if (passphrase_migration_in_progress_ &&
-          !passphrase_required_for_decryption_) {
+          passphrase_required_reason_ != sync_api::REASON_DECRYPTION &&
+          passphrase_required_reason_ !=
+              sync_api::REASON_SET_PASSPHRASE_FAILED) {
         wizard_.Step(SyncSetupWizard::PASSPHRASE_MIGRATION);
         passphrase_migration_in_progress_ = false;
       }
 
       break;
     }
     case NotificationType::GOOGLE_SIGNIN_FAILED: {
       GoogleServiceAuthError error =
           *(Details<const GoogleServiceAuthError>(details).ptr());
       UpdateAuthErrorState(error);
@@ skipping 63 matching lines @@
   // is initialized, all enabled data types are consistent with one
   // another, and no unrecoverable error has transpired.
   if (unrecoverable_error_detected_)
     return false;
 
   if (!data_type_manager_.get())
     return false;
 
   return data_type_manager_->state() == DataTypeManager::CONFIGURED;
 }