[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..8171786c2e1ba3f4a2cf2ee7fe4511b8d67c02cc 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,11 @@ 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;
+  }
+

[Andrew T Wilson (Slow), 2013/05/15 15:01:24]

Add a DCHECK(!(sync_nothing && sync_everything)) since it's not valid to set
both?

[Raghu Simha, 2013/05/16 02:13:20]

Done.

   ModelTypeNameMap type_names = GetSelectableTypeNameMap();
 
   for (ModelTypeNameMap::const_iterator it = type_names.begin();
@@ -356,6 +363,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 +449,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 +477,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 +568,7 @@ void SyncSetupHandler::ConfigureSyncDone() {
 
     // We're done configuring, so notify ProfileSyncService that it is OK to
     // start syncing.
+    service->SetSetupInProgress(false);
     service->SetSyncSetupCompleted();
   }
 }
@@ -892,17 +903,22 @@ 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) {

[Andrew T Wilson (Slow), 2013/05/15 15:01:24]

Why would we only do this on INVALID_GAIA_CREDENTIALS but not other errors
(like, account_disabled, etc)?

[Raghu Simha, 2013/05/16 02:13:20]

Good catch. I've added those cases, and we now match the auth failure cases in
signin_ui_util.cc.

+    // The user received an auth error while re-enabling sync after having
+    // chosen to "Sync nothing", and the gaia password has been changed since
+    // the user last signed in. Leave the sync backend initialized, and close
+    // the setup dialog. An error message will be shown on the settings page.
+    web_ui()->CallJavascriptFunction("OptionsPage.closeOverlay");
+  } 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 +983,17 @@ void SyncSetupHandler::HandleConfigure(const ListValue* args) {
     return;
   }
 
+  // Disable sync, but remain signed in if the user selected "Sync nothing" in
+  // the advanced settings dialog.
+  if (configuration.sync_nothing) {
+    ProfileSyncService::SyncEvent(
+        ProfileSyncService::STOP_FROM_ADVANCED_DIALOG);
+    CloseOverlay();
+    service->DisableForUser();
+    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
@@ -1102,7 +1129,9 @@ void SyncSetupHandler::HandleCloseTimeout(const ListValue* args) {
 void SyncSetupHandler::CloseSyncSetup() {
   // TODO(atwilson): Move UMA tracking of signin events out of sync module.
   ProfileSyncService* sync_service = GetSyncService();
-  if (IsActiveLogin()) {
+  // If there was a sign in error, do not log a cancel event.
+  if (IsActiveLogin() &&
+      last_signin_error_.state() == GoogleServiceAuthError::NONE) {
     if (!sync_service || !sync_service->HasSyncSetupCompleted()) {
       if (signin_tracker_.get()) {
         ProfileSyncService::SyncEvent(
@@ -1123,38 +1152,20 @@ 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);
-    }
+  // If the setup dialog is being closed during configuration, and sync setup is
+  // incomplete, and there is no signin error, it's because the user clicked
+  // "Cancel" while setting up sync. In this case, we don't want the sync
+  // backend to remain in the initialized state.
+  // Note: If we get here and there is a signin error, it's because a signed in
+  // user is trying to set up sync, and their gaia password has been changed
+  // since they last signed in. In this case, we leave the backend initialized
+  // and allow the user to respond to the error message via the settings page.
+  if (sync_service &&
+      !sync_service->HasSyncSetupCompleted() &&
+      configuring_sync_ &&

[Andrew T Wilson (Slow), 2013/05/15 15:01:24]

You can't really rely on configuring_sync_ - it's just there to allow UMA
tracking, but it's not correctly set in all cases IIRC. Perhaps you can omit
relying on it and just always disable sync if sync setup has not yet completed?

[Raghu Simha, 2013/05/16 02:13:20]

If the user clicks cancel, we want to disable sync if setup hasn't been
completed. However, if the user tried to re-enable sync and saw an auth error
(say, due to a password that was changed in the interim), we want to close the
setup dialog and show the user an error message on the settings page. In this
case, we don't want to disable sync. Instead, we'd like to leave the backend
initialized (about:sync shows that it's in a deferred state), and give the user
the chance to re-auth and continue enabling sync.

I was using |configuring_sync_| to differentiate between the two cases, but what
you say makes sense. I've moved this block to higher up in CloseSyncSetup, and
we now disable sync only if the user actually canceled sync.

The other case where we want to disable sync is when the user selected "Sync
nothing". That's already taken care of by the handler for syncNothing.

+      last_signin_error_.state() == GoogleServiceAuthError::NONE) {
+    DVLOG(1) << "Sync setup aborted by user action";
+    sync_service->DisableForUser();
     sync_service->SetSetupInProgress(false);
   }
 
@@ -1186,13 +1197,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 +1215,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);
   }