Start moving signin code out of browser/sync.

chrome/browser/sync/profile_sync_service.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/sync/profile_sync_service.h"
 
 #include <cstddef>
 #include <map>
 #include <set>
 #include <utility>
@@ skipping 30 matching lines @@
 #include "chrome/browser/sync/js/js_event_details.h"
 #include "chrome/browser/sync/profile_sync_components_factory_impl.h"
 #include "chrome/browser/sync/sync_global_error.h"
 #include "chrome/browser/sync/util/cryptographer.h"
 #include "chrome/browser/sync/util/oauth.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/global_error_service.h"
 #include "chrome/browser/ui/global_error_service_factory.h"
+#include "chrome/browser/ui/webui/signin/login_ui_service.h"
+#include "chrome/browser/ui/webui/signin/login_ui_service_factory.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/net/gaia/gaia_constants.h"
 #include "chrome/common/time_format.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_source.h"
 #include "grit/generated_resources.h"
 #include "net/base/cookie_monster.h"
@@ skipping 89 matching lines @@
 bool ProfileSyncService::AreCredentialsAvailable() {
   return AreCredentialsAvailable(false);
 }
 
 bool ProfileSyncService::AreCredentialsAvailable(
     bool check_oauth_login_token) {
   if (IsManaged()) {
     return false;
   }
 
+  // If we have start suppressed, then basically just act like we have no
+  // credentials (login is required to fix this, since we need the user's
+  // passphrase to encrypt/decrypt anyway).
+  // TODO(sync): Revisit this when we move to a server-based keystore.
+  if (sync_prefs_.IsStartSuppressed())
+    return false;
+
   // CrOS user is always logged in. Chrome uses signin_ to check logged in.
   if (signin_->GetAuthenticatedUsername().empty())
     return false;
 
   TokenService* token_service = profile()->GetTokenService();
   if (!token_service)
     return false;
 
   // TODO(chron): Verify CrOS unit test behavior.
   if (!token_service->HasTokenForService(browser_sync::SyncServiceName()))
@@ skipping 541 matching lines @@
     if (!to_register.Empty() && HasSyncSetupCompleted() && migrator_.get()) {
       DVLOG(1) << "Dynamically enabling new datatypes: "
                << syncable::ModelTypeSetToString(to_register);
       OnMigrationNeededForTypes(to_register);
     }
   }
 }
 
 void ProfileSyncService::UpdateAuthErrorState(
     const GoogleServiceAuthError& error) {
+  is_auth_in_progress_ = false;
   last_auth_error_ = error;
-  // Protect against the in-your-face dialogs that pop out of nowhere.
-  // Require the user to click somewhere to run the setup wizard in the case
-  // of a steady-state auth failure.
-  if (WizardIsVisible()) {
-    wizard_.Step(last_auth_error_.state() == AuthError::NONE ?
-        SyncSetupWizard::GAIA_SUCCESS : SyncSetupWizard::GetLoginState());
-  } else {
-    auth_error_time_ = base::TimeTicks::Now();
-  }
 
-  if (!auth_start_time_.is_null()) {
-    UMA_HISTOGRAM_TIMES("Sync.AuthorizationTimeInNetwork",
-                    base::TimeTicks::Now() - auth_start_time_);
-    auth_start_time_ = base::TimeTicks();
+  if (WizardIsVisible() && last_auth_error_.state() != AuthError::NONE) {
+    // Got some kind of auth error while the wizard is visible. Exit out of

[tim (not reviewing), 2012/02/14 02:11:21]

err... why is this fatal?  what happens to any common GSAE like invalid gaia?

[Andrew T Wilson (Slow), 2012/02/14 05:23:52]

Remember that this is dealing with a GAIA error *after* login, while the config
screen is visible (not during signin). But, yeah, I guess nothing bad happens if
we leave the config screen up, so I'll remove this (the case of
SERVICE_UNAVAILABLE due to StopSyncingPermanently is already handled below).

+    // the config flow (UI handler can notify the user or bring up the signin
+    // flow as appropriate).
+    wizard_.Step(SyncSetupWizard::FATAL_ERROR);
   }
 
   // Fan the notification out to interested UI-thread components.
   NotifyObservers();
 }
 
 void ProfileSyncService::OnAuthError() {
   UpdateAuthErrorState(backend_->GetAuthError());
 }
 
 void ProfileSyncService::OnStopSyncingPermanently() {
+  UpdateAuthErrorState(
+      GoogleServiceAuthError(GoogleServiceAuthError::SERVICE_UNAVAILABLE));
+
   if (SetupInProgress()) {

[Andrew T Wilson (Slow), 2012/02/14 05:23:52]

BTW, I'm changing this to abort whenever the wizard is visible. If we've stopped
syncing, it doesn't matter if this is initial setup or not, if the wizard is up
we need to close it.

-    wizard_.Step(SyncSetupWizard::SETUP_ABORTED_BY_PENDING_CLEAR);
+    wizard_.Step(SyncSetupWizard::ABORT);
     expect_sync_configuration_aborted_ = true;
   }
   sync_prefs_.SetStartSuppressed(true);
   DisableForUser();
 }
 
 void ProfileSyncService::OnClearServerDataTimeout() {
   if (clear_server_data_state_ != CLEAR_SUCCEEDED &&
       clear_server_data_state_ != CLEAR_FAILED) {
     clear_server_data_state_ = CLEAR_FAILED;
@@ skipping 179 matching lines @@
     case browser_sync::DISABLE_SYNC_ON_CLIENT:
       OnStopSyncingPermanently();
       break;
     default:
       NOTREACHED();
   }
   NotifyObservers();
 }
 
 void ProfileSyncService::ShowLoginDialog() {
-  if (WizardIsVisible()) {
-    wizard_.Focus();
-    // Force the wizard to step to the login screen (which will only actually
-    // happen if the transition is valid).
-    wizard_.Step(SyncSetupWizard::GetLoginState());
-    return;
-  }
-
-  if (!auth_error_time_.is_null()) {
-    UMA_HISTOGRAM_LONG_TIMES("Sync.ReauthorizationTime",
-                             base::TimeTicks::Now() - auth_error_time_);
-    auth_error_time_ = base::TimeTicks();  // Reset auth_error_time_ to null.
-  }
-
-  ShowSyncSetupWithWizard(SyncSetupWizard::GetLoginState());
-
-  NotifyObservers();
+  // TODO(atwilson): Remove this API and have the callers directly call
+  // LoginUIService.
+  LoginUIServiceFactory::GetForProfile(profile_)->ShowLoginUI();
 }
 
 void ProfileSyncService::ShowErrorUI() {
   if (WizardIsVisible()) {
     wizard_.Focus();
     return;
   }
 
-  if (last_auth_error_.state() != AuthError::NONE)
+  // Figure out what kind of error we've encountered. There are only 3 kinds:
+  // 1) auth error.
+  // 2) server-initiated error
+  // 3) passphrase error
+  // Any other errors (such as unrecoverable error) should be handled by the UI
+  // itself and should not result in a call to ShowErrorUI.

[tim (not reviewing), 2012/02/14 02:11:21]

hm. so what about a failure to initialize / load the database in sync_manager?

[Andrew T Wilson (Slow), 2012/02/14 05:23:52]

This change just makes explicit what was previously hidden away in
SyncSetupWizard::GetStepForNonFatalError(). That code assumed that either you
have passphrase required, or else an auth error, anything else hit a NOTREACHED
- so this shouldn't be a change in functionality, it's just clearer now.

The code in sync_ui_util.cc is dense and i'm still wrapping my head around it,
but I believe the intent is that unrecoverable errors like the type you
described don't result in calls to ShowErrorUI().

+  if (last_auth_error_.state() != AuthError::NONE) {
     ShowLoginDialog();
-  else if (ShouldShowActionOnUI(last_actionable_error_))
+  } else if (ShouldShowActionOnUI(last_actionable_error_)) {
     ShowSyncSetup(chrome::kPersonalOptionsSubPage);
-  else
-    ShowSyncSetupWithWizard(SyncSetupWizard::NONFATAL_ERROR);
+  } else {
+    // We should only get here for passphrase error.
+    DCHECK(IsPassphraseRequired());
+    ShowSyncSetupWithWizard(SyncSetupWizard::ENTER_PASSPHRASE);
+  }
 }
 
 void ProfileSyncService::ShowConfigure(bool sync_everything) {
   if (!sync_initialized()) {
     LOG(ERROR) << "Attempted to show sync configure before backend ready.";
     return;
   }
   if (WizardIsVisible()) {
     wizard_.Focus();
     return;
@@ skipping 65 matching lines @@
 
 bool ProfileSyncService::sync_initialized() const {
   return backend_initialized_;
 }
 
 bool ProfileSyncService::unrecoverable_error_detected() const {
   return unrecoverable_error_detected_;
 }
 
 bool ProfileSyncService::UIShouldDepictAuthInProgress() const {
-  return is_auth_in_progress_;
-}
-
-void ProfileSyncService::SetUIShouldDepictAuthInProgress(
-    bool auth_in_progress) {
-  is_auth_in_progress_ = auth_in_progress;
-  // TODO(atwilson): Figure out if we still need to track this or if we should
-  // move this up to the UI (or break it out into two stats that track GAIA
-  // auth and sync auth separately).
-  if (is_auth_in_progress_)
-    auth_start_time_ = base::TimeTicks::Now();
-  NotifyObservers();
+  return signin()->AuthInProgress();
 }
 
 bool ProfileSyncService::IsPassphraseRequired() const {
   return passphrase_required_reason_ !=
       sync_api::REASON_PASSPHRASE_NOT_REQUIRED;
 }
 
 // TODO(zea): Rename this IsPassphraseNeededFromUI and ensure it's used
 // appropriately (see http://crbug.com/91379).
 bool ProfileSyncService::IsPassphraseRequiredForDecryption() const {
@@ skipping 356 matching lines @@
           *(content::Details<const GoogleServiceAuthError>(details).ptr());
       UpdateAuthErrorState(error);
       break;
     }
     case chrome::NOTIFICATION_GOOGLE_SIGNIN_SUCCESSFUL: {
       const GoogleServiceSigninSuccessDetails* successful =
           content::Details<const GoogleServiceSigninSuccessDetails>(
               details).ptr();
       // The user has submitted credentials, which indicates they don't
       // want to suppress start up anymore.
+      // TODO(sync): Remove this when sync is no longer tied to browser signin.

[tim (not reviewing), 2012/02/14 02:11:21]

should tie this to a bug (and mention in the bug to grep for the bug # before
closing)

[Andrew T Wilson (Slow), 2012/02/14 05:23:52]

Done.

       sync_prefs_.SetStartSuppressed(false);
 
       // Because we specify IMPLICIT to SetPassphrase, we know it won'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.
       if (!successful->password.empty())
         SetPassphrase(successful->password, IMPLICIT, INTERNAL);
+
+      if (!sync_initialized() ||

[tim (not reviewing), 2012/02/14 02:11:21]

Hm, so there's no way we can call UpdateAuthErrorState here instead, is there?

[Andrew T Wilson (Slow), 2012/02/14 05:23:52]

We aren't actually changing the state - all we're doing is setting
is_auth_in_progress if we decide we need to wait for auth to complete.

+          GetAuthError().state() != GoogleServiceAuthError::NONE) {
+        // Track the fact that we're still waiting for auth to complete.
+        is_auth_in_progress_ = true;
+      }
       break;
     }
     case chrome::NOTIFICATION_TOKEN_REQUEST_FAILED: {
       const TokenService::TokenRequestFailedDetails& token_details =
           *(content::Details<const TokenService::TokenRequestFailedDetails>(
               details).ptr());
       if (IsTokenServiceRelevant(token_details.service())) {
         GoogleServiceAuthError error(
             GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS);
         UpdateAuthErrorState(error);
@@ skipping 141 matching lines @@
              << "Unrecoverable error.";
   } else {
     DVLOG(0) << "ConfigureDataTypeManager not invoked because backend is not "
              << "initialized";
   }
 }
 
 const FailedDatatypesHandler& ProfileSyncService::failed_datatypes_handler() {
   return failed_datatypes_handler_;
 }