[sync] Separate sign in from sync on Desktop Chrome

chrome/browser/ui/webui/sync_setup_handler.cc

Index: chrome/browser/ui/webui/sync_setup_handler.cc
diff --git a/chrome/browser/ui/webui/sync_setup_handler.cc b/chrome/browser/ui/webui/sync_setup_handler.cc
index dffb73940cebbb809bbd940a8f6b9da36ce90b6a..eb7f2684a5825717ae383588fed4119381d2e0ef 100644
--- a/chrome/browser/ui/webui/sync_setup_handler.cc
+++ b/chrome/browser/ui/webui/sync_setup_handler.cc
@@ -65,6 +65,7 @@ struct SyncConfigInfo {
 
   bool encrypt_all;
   bool sync_everything;
+  bool sync_nothing;
   syncer::ModelTypeSet data_types;
   std::string passphrase;
   bool passphrase_is_gaia;
@@ -73,6 +74,7 @@ struct SyncConfigInfo {
 SyncConfigInfo::SyncConfigInfo()
     : encrypt_all(false),
       sync_everything(false),
+      sync_nothing(false),
       passphrase_is_gaia(false) {
 }
 
@@ -145,6 +147,14 @@ bool GetConfiguration(const std::string& json, SyncConfigInfo* config) {
     return false;
   }
 
+  if (!result->GetBoolean("syncNothing", &config->sync_nothing)) {
+    DLOG(ERROR) << "GetConfiguration() not passed a syncNothing value";
+    return false;
+  }
+
+  DCHECK(!(config->sync_everything && config->sync_nothing))
+      << "syncAllDataTypes and syncNothing cannot both be true";
+
   ModelTypeNameMap type_names = GetSelectableTypeNameMap();
 
   for (ModelTypeNameMap::const_iterator it = type_names.begin();
@@ -356,6 +366,7 @@ void SyncSetupHandler::GetStaticLocalizedValues(
     { "getOtpURL", IDS_SYNC_GET_OTP_URL },
     { "syncAllDataTypes", IDS_SYNC_EVERYTHING },
     { "chooseDataTypes", IDS_SYNC_CHOOSE_DATATYPES },
+    { "syncNothing", IDS_SYNC_NOTHING },
     { "bookmarks", IDS_SYNC_DATATYPE_BOOKMARKS },
     { "preferences", IDS_SYNC_DATATYPE_PREFERENCES },
     { "autofill", IDS_SYNC_DATATYPE_AUTOFILL },
@@ -441,6 +452,7 @@ void SyncSetupHandler::DisplayConfigureSync(bool show_advanced,
   // Setup args for the sync configure screen:
   //   showSyncEverythingPage: false to skip directly to the configure screen
   //   syncAllDataTypes: true if the user wants to sync everything
+  //   syncNothing: true if the user wants to sync nothing
   //   <data_type>Registered: true if the associated data type is supported
   //   <data_type>Synced: true if the user wants to sync that specific data type
   //   encryptionEnabled: true if sync supports encryption
@@ -468,6 +480,7 @@ void SyncSetupHandler::DisplayConfigureSync(bool show_advanced,
   args.SetBoolean("passphraseFailed", passphrase_failed);
   args.SetBoolean("showSyncEverythingPage", !show_advanced);
   args.SetBoolean("syncAllDataTypes", sync_prefs.HasKeepEverythingSynced());
+  args.SetBoolean("syncNothing", false);  // Always false during initial setup.
   args.SetBoolean("encryptAllData", service->EncryptEverythingEnabled());
 
   // We call IsPassphraseRequired() here, instead of calling
@@ -558,6 +571,7 @@ void SyncSetupHandler::ConfigureSyncDone() {
 
     // We're done configuring, so notify ProfileSyncService that it is OK to
     // start syncing.
+    service->SetSetupInProgress(false);

[Andrew T Wilson (Slow), 2013/05/16 08:39:30]

Where was this done previously (i.e. why is this necessary now)?

[Raghu Simha, 2013/05/17 05:14:17]

Earlier, this was being called in CloseSyncSetup as part of the
sign-out-if-not-fully-setup block, and was being run irrespective of whether the
user canceled sign in or not. The above call is necessary now that the
CloseSyncSetup has been refactored out to disable sync only in the cancel case,
since we want to leave the backend initialized in the user-needs-to-re-auth
case.

     service->SetSyncSetupCompleted();
   }
 }
@@ -892,17 +906,25 @@ void SyncSetupHandler::SigninFailed(const GoogleServiceAuthError& error) {
 
   last_signin_error_ = error;
 
-  // If using web-based sign in flow, don't show the gaia sign in page again
-  // since there is no way to show the user an error message.
-  if (SyncPromoUI::UseWebBasedSigninFlow()) {
+  if (!retry_on_signin_failure_ &&
+      (error.state() == GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS ||
+       error.state() == GoogleServiceAuthError::ACCOUNT_DELETED ||
+       error.state() == GoogleServiceAuthError::ACCOUNT_DISABLED)) {
+    // The user received an auth error while re-enabling sync after having
+    // chosen to "Sync nothing". This can happen if the gaia password has been
+    // changed since the user last signed in, or if the account has been deleted
+    // or disabled. Leave the sync backend initialized, and close the setup
+    // dialog. An error message will be shown on the settings page.
+    CloseOverlay();

[Andrew T Wilson (Slow), 2013/05/16 08:39:30]

It seems dangerous to just call CloseOverlay() - CloseSyncSetup() does lots of
good cleanup like freeing the SigninTracker. I think the code below that calls
CloseOverlay() is probably incorrect. In fact, I think there's lots of code that
calls CloseOverlay() (like DisplayGaiaSuccessAndClose(), etc that are doing the
wrong thing here too - but that's for another CL :)

Can we just fall through to use the existing cases below? Especially since roger
is driving a CL to remove all of the non-web-based-signin-flow code, so all of
this could just turn into a single call to CloseSyncSetup().

If CloseSyncSetup() does something you don't want to happen here, then we should
change it to do the right thing.

[Raghu Simha, 2013/05/17 05:14:17]

As a matter of fact, CloseOverlay() calls CloseSyncSetup(), and therefore, does
all the good cleanup you mentioned. In addition to calling CloseSyncSetup(), it
also closes the overlay window. This has been true since this CL:
https://chromiumcodereview.appspot.com/9295044/diff/38001/chrome/browser/ui/w...

The case I am trying to handle here is when sync has been stopped, and the user
tries to re-enable it after the gaia password has changed. If we merely call
CloseSyncSetup without calling CloseOverlay, all the cleanup is done, but the
spinner window remains visible, and hides the "sign in again" link that shows up
on the main settings page.

I've collapsed the if block I originally added into one of the existing cases.
I've also retained the call to CloseOverlay(), which explicitly calls
CloseSyncSetup(). I've updated the description of CloseOverlay() in the header
file. If you believe CloseOverlay() shouldn't be calling CloseSyncSetup(), let
me know, and I'll change it in another CL.

[Andrew T Wilson (Slow), 2013/05/17 07:26:10]

Hah, apparently I forgot about my own refactorings :)

+  } else if (SyncPromoUI::UseWebBasedSigninFlow()) {
+    // If using web-based sign in flow, don't show the gaia sign in page again
+    // since there is no way to show the user an error message.
     CloseSyncSetup();
   } else if (retry_on_signin_failure_) {
     // Got a failed signin - this is either just a typical auth error, or a
     // sync error (treat sync errors as "fatal errors" - i.e. non-auth errors).
     // On ChromeOS, this condition can happen when auth token is invalid and
     // cannot start sync backend.
-    // If using web-based sign in flow, don't show the gaia sign in page again
-    // since there is no way to show the user an error message.
     ProfileSyncService* service = GetSyncService();
     DisplayGaiaLogin(service && service->HasUnrecoverableError());
   } else {
@@ -967,6 +989,19 @@ void SyncSetupHandler::HandleConfigure(const ListValue* args) {
     return;
   }
 
+  // Disable sync, but remain signed in if the user selected "Sync nothing" in
+  // the advanced settings dialog. Note: In order to disable sync across
+  // restarts on Chrome OS, we must call OnStopSyncingPermanently(), which
+  // suppresses sync startup in addition to disabling it.
+  if (configuration.sync_nothing) {
+    ProfileSyncService::SyncEvent(
+        ProfileSyncService::STOP_FROM_ADVANCED_DIALOG);
+    CloseOverlay();
+    service->OnStopSyncingPermanently();
+    service->SetSetupInProgress(false);
+    return;
+  }
+
   // Note: Data encryption will not occur until configuration is complete
   // (when the PSS receives its CONFIGURE_DONE notification from the sync
   // backend), so the user still has a chance to cancel out of the operation
@@ -1103,7 +1138,10 @@ void SyncSetupHandler::CloseSyncSetup() {
   // TODO(atwilson): Move UMA tracking of signin events out of sync module.
   ProfileSyncService* sync_service = GetSyncService();
   if (IsActiveLogin()) {
-    if (!sync_service || !sync_service->HasSyncSetupCompleted()) {
+    // Don't log a cancel event if the sync setup dialog is being
+    // automatically closed due to an auth error.
+    if ((!sync_service || !sync_service->HasSyncSetupCompleted()) &&
+        last_signin_error_.state() == GoogleServiceAuthError::NONE) {
       if (signin_tracker_.get()) {
         ProfileSyncService::SyncEvent(
             ProfileSyncService::CANCEL_DURING_SIGNON);
@@ -1114,6 +1152,17 @@ void SyncSetupHandler::CloseSyncSetup() {
         ProfileSyncService::SyncEvent(
             ProfileSyncService::CANCEL_FROM_SIGNON_WITHOUT_AUTH);
       }
+
+      // If the user clicked "Cancel" while setting up sync, disable sync
+      // because we don't want the sync backend to remain in the initialized
+      // state. Note: In order to disable sync across restarts on Chrome OS, we
+      // must call OnStopSyncingPermanently(), which suppresses sync startup in
+      // addition to disabling it.
+      if (sync_service) {
+        DVLOG(1) << "Sync setup aborted by user action";
+        sync_service->OnStopSyncingPermanently();
+        sync_service->SetSetupInProgress(false);
+      }
     }
 
     // Let the various services know that we're no longer active.
@@ -1123,41 +1172,6 @@ void SyncSetupHandler::CloseSyncSetup() {
     GetLoginUIService()->LoginUIClosed(this);
   }
 
-  if (sync_service) {
-    // Make sure user isn't left half-logged-in (signed in, but without sync
-    // started up). If the user hasn't finished setting up sync, then sign out
-    // and shut down sync.
-    if (!sync_service->HasSyncSetupCompleted()) {
-      DVLOG(1) << "Signin aborted by user action";
-      // We can get here on Chrome OS (e.g dashboard clear), but "do nothing"
-      // is the correct behavior.
-#if !defined(OS_CHROMEOS)
-      if (signin_tracker_.get() || sync_service->FirstSetupInProgress()) {
-        // User was still in the process of signing in, so sign him out again.
-        // This makes sure that the user isn't left signed in but with sync
-        // un-configured.
-        //
-        // This has the side-effect of signing out the user in the following
-        // scenario:
-        //   * User signs in while sync is disabled by policy.
-        //   * Sync is re-enabled by policy.
-        //   * User brings up sync setup dialog to do initial sync config.
-        //   * User cancels out of the dialog.
-        //
-        // This case is indistinguishable from the "one click signin" case where
-        // the user checks the "advanced setup" checkbox, then cancels out of
-        // the setup box, which is a much more common scenario, so we do the
-        // right thing for the one-click case.
-        SigninManagerFactory::GetForProfile(GetProfile())->SignOut();
-      }
-#endif
-      sync_service->DisableForUser();
-      browser_sync::SyncPrefs sync_prefs(GetProfile()->GetPrefs());
-      sync_prefs.SetStartSuppressed(true);
-    }
-    sync_service->SetSetupInProgress(false);
-  }
-
   // Reset the attempted email address and error, otherwise the sync setup
   // overlay in the settings page will stay in whatever error state it was last
   // when it is reopened.
@@ -1186,13 +1200,10 @@ void SyncSetupHandler::OpenSyncSetup() {
   //    logged in.
   // 6) One-click signin (credentials are already available, so should display
   //    sync configure UI, not login UI).
-  // 7) ChromeOS re-enable after disabling sync.
+  // 7) User re-enables sync after disabling it via advanced settings.
   SigninManagerBase* signin =
       SigninManagerFactory::GetForProfile(GetProfile());
   if (signin->GetAuthenticatedUsername().empty() ||
-#if !defined(OS_CHROMEOS)
-      (GetSyncService() && GetSyncService()->IsStartSuppressed()) ||
-#endif
       signin->signin_global_error()->HasMenuItem()) {
     // User is not logged in, or login has been specially requested - need to
     // display login UI (cases 1-3).
@@ -1207,7 +1218,7 @@ void SyncSetupHandler::OpenSyncSetup() {
 
     // User is already logged in. They must have brought up the config wizard
     // via the "Advanced..." button or through One-Click signin (cases 4-6), or
-    // they are re-enabling sync on Chrome OS.
+    // they are re-enabling sync after having disabled it (case 7).
     DisplayConfigureSync(true, false);
   }