[sync] Separate sign in from sync on Desktop Chrome

chrome/browser/ui/webui/sync_setup_handler.cc

 // Copyright (c) 2012 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/ui/webui/sync_setup_handler.h"
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
@@ skipping 47 matching lines @@
 
 namespace {
 
 // A structure which contains all the configuration information for sync.
 struct SyncConfigInfo {
   SyncConfigInfo();
   ~SyncConfigInfo();
 
   bool encrypt_all;
   bool sync_everything;
+  bool sync_nothing;
   syncer::ModelTypeSet data_types;
   std::string passphrase;
   bool passphrase_is_gaia;
 };
 
 SyncConfigInfo::SyncConfigInfo()
     : encrypt_all(false),
       sync_everything(false),
+      sync_nothing(false),
       passphrase_is_gaia(false) {
 }
 
 SyncConfigInfo::~SyncConfigInfo() {}
 
 // Note: The order of these types must match the ordering of
 // the respective types in ModelType
 const char* kDataTypeNames[] = {
   "bookmarks",
   "preferences",
@@ skipping 52 matching lines @@
   if (!parsed_value.get() || !parsed_value->GetAsDictionary(&result)) {
     DLOG(ERROR) << "GetConfiguration() not passed a Dictionary";
     return false;
   }
 
   if (!result->GetBoolean("syncAllDataTypes", &config->sync_everything)) {
     DLOG(ERROR) << "GetConfiguration() not passed a syncAllDataTypes value";
     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();
        it != type_names.end(); ++it) {
     std::string key_name = it->second + std::string("Synced");
     bool sync_value;
     if (!result->GetBoolean(key_name, &sync_value)) {
       DLOG(ERROR) << "GetConfiguration() not passed a value for " << key_name;
       return false;
     }
@@ skipping 191 matching lines @@
     { "invalidAccessCode", IDS_SYNC_INVALID_ACCESS_CODE_LABEL },
     { "enterAccessCode", IDS_SYNC_ENTER_ACCESS_CODE_LABEL },
     { "getAccessCodeHelp", IDS_SYNC_ACCESS_CODE_HELP_LABEL },
     { "getAccessCodeURL", IDS_SYNC_GET_ACCESS_CODE_URL },
     { "invalidOtp", IDS_SYNC_INVALID_OTP_LABEL },
     { "enterOtp", IDS_SYNC_ENTER_OTP_LABEL },
     { "getOtpHelp", IDS_SYNC_OTP_HELP_LABEL },
     { "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 },
     { "themes", IDS_SYNC_DATATYPE_THEMES },
     { "passwords", IDS_SYNC_DATATYPE_PASSWORDS },
     { "extensions", IDS_SYNC_DATATYPE_EXTENSIONS },
     { "typedURLs", IDS_SYNC_DATATYPE_TYPED_URLS },
     { "apps", IDS_SYNC_DATATYPE_APPS },
     { "openTabs", IDS_SYNC_DATATYPE_TABS },
     { "syncZeroDataTypesError", IDS_SYNC_ZERO_DATA_TYPES_ERROR },
@@ skipping 65 matching lines @@
   // Should only be called if user is signed in, so no longer need our
   // SigninTracker.
   signin_tracker_.reset();
   configuring_sync_ = true;
   DCHECK(service->sync_initialized()) <<
       "Cannot configure sync until the sync backend is initialized";
 
   // 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
   //   encryptAllData: true if user wants to encrypt all data (not just
   //       passwords)
   //   usePassphrase: true if the data is encrypted with a secondary passphrase
   //   show_passphrase: true if a passphrase is needed to decrypt the sync data
   DictionaryValue args;
 
   // Tell the UI layer which data types are registered/enabled by the user.
   const syncer::ModelTypeSet registered_types =
       service->GetRegisteredDataTypes();
   const syncer::ModelTypeSet preferred_types =
       service->GetPreferredDataTypes();
   ModelTypeNameMap type_names = GetSelectableTypeNameMap();
   for (ModelTypeNameMap::const_iterator it = type_names.begin();
        it != type_names.end(); ++it) {
     syncer::ModelType sync_type = it->first;
     const std::string key_name = it->second;
     args.SetBoolean(key_name + "Registered",
                     registered_types.Has(sync_type));
     args.SetBoolean(key_name + "Synced", preferred_types.Has(sync_type));
   }
   browser_sync::SyncPrefs sync_prefs(GetProfile()->GetPrefs());
   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
   // IsPassphraseRequiredForDecryption(), because we want to show the passphrase
   // UI even if no encrypted data types are enabled.
   args.SetBoolean("showPassphrase", service->IsPassphraseRequired());
   // Keystore encryption is behind a flag. Only show the new encryption settings
   // if keystore encryption is enabled.
   args.SetBoolean("keystoreEncryptionEnabled", IsKeystoreEncryptionEnabled());
 
@@ skipping 70 matching lines @@
 
   ProfileSyncService* service = GetSyncService();
   DCHECK(service);
   if (!service->HasSyncSetupCompleted()) {
     // This is the first time configuring sync, so log it.
     base::FilePath profile_file_path = GetProfile()->GetPath();
     ProfileMetrics::LogProfileSyncSignIn(profile_file_path);
 
     // We're done configuring, so notify ProfileSyncService that it is OK to
     // start syncing.
+    service->SetSetupInProgress(false);
     service->SetSyncSetupCompleted();
   }
 }
 
 bool SyncSetupHandler::IsActiveLogin() const {
   // LoginUIService can be NULL if page is brought up in incognito mode
   // (i.e. if the user is running in guest mode in cros and brings up settings).
   LoginUIService* service = GetLoginUIService();
   return service && (service->current_login_ui() == this);
 }
@@ skipping 314 matching lines @@
   // Gaia credentials are valid - update the UI.
   DisplayGaiaSuccessAndSettingUp();
 }
 
 void SyncSetupHandler::SigninFailed(const GoogleServiceAuthError& error) {
   // Stop a timer to handle timeout in waiting for checking network connection.
   backend_start_timer_.reset();
 
   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 {
     // TODO(peria): Show error dialog for prompting sign in and out on
     // Chrome OS. http://crbug.com/128692
     CloseOverlay();
   }
 }
 
 Profile* SyncSetupHandler::GetProfile() const {
@@ skipping 44 matching lines @@
   // to us from the JS layer.
   ProfileSyncService* service = GetSyncService();
 
   // If the sync engine has shutdown for some reason, just close the sync
   // dialog.
   if (!service || !service->sync_initialized()) {
     CloseOverlay();
     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
   // if (for example) some kind of passphrase error is encountered.
   if (configuration.encrypt_all)
     service->EnableEncryptEverything();
 
   bool passphrase_failed = false;
   if (!configuration.passphrase.empty()) {
     // We call IsPassphraseRequired() here (instead of
@@ skipping 115 matching lines @@
 #endif
 }
 
 void SyncSetupHandler::HandleCloseTimeout(const ListValue* args) {
   CloseSyncSetup();
 }
 
 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(
             ProfileSyncService::CANCEL_DURING_SIGNON);
       } else if (configuring_sync_) {
         ProfileSyncService::SyncEvent(
             ProfileSyncService::CANCEL_DURING_CONFIGURE);
       } else {
         ProfileSyncService::SyncEvent(
             ProfileSyncService::CANCEL_FROM_SIGNON_WITHOUT_AUTH);
       }
     }
 
     // Let the various services know that we're no longer active.
     if (SyncPromoUI::UseWebBasedSigninFlow())
       CloseGaiaSigninPage();
 
     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);
   }
 
   // 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.
   last_attempted_user_email_.clear();
   last_signin_error_ = GoogleServiceAuthError::AuthErrorNone();
 
   configuring_sync_ = false;
@@ skipping 11 matching lines @@
   // 1) Signin promo.
   // 2) Normal signin through settings page (GetAuthenticatedUsername() is
   //    empty).
   // 3) Previously working credentials have expired.
   // 4) User is signed in, but has stopped sync via the google dashboard, and
   //    signout is prohibited by policy so we need to force a re-auth.
   // 5) User clicks [Advanced Settings] button on options page while already
   //    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).
     DisplayGaiaLogin(false);
   } else {
     if (!GetSyncService()) {
       // This can happen if the user directly navigates to /settings/syncSetup.
       DLOG(WARNING) << "Cannot display sync UI when sync is disabled";
       CloseOverlay();
       return;
     }
 
     // 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);
   }
 
   if (!SyncPromoUI::UseWebBasedSigninFlow())
     ShowSetupUI();
 }
 
 void SyncSetupHandler::OpenConfigureSync() {
   if (!PrepareSyncSetup())
     return;
@@ skipping 127 matching lines @@
       *error_message = l10n_util::GetStringUTF16(
           IDS_SYNC_USER_NAME_IN_USE_ERROR);
       return false;
     }
   }
 
   return true;
 }
 
 #endif  // !defined(OS_CHROMEOS)