r137484 - in branches/1132/src/chrome/browser: signin sync ui/webui ui/webui/options2

[atwi...@chromium.org]

Author: atwi...@chromium.org
Date: Wed May 16 12:34:09 2012
New Revision: 137484

Log:
Merge 137359 - Treat SyncCredentialsLost as an auth error
Break up AreCredentialsAvailable() into two routines, IsSyncEnabledAndLoggedIn() and IsSyncTokenAvailable().
ProfileSyncService now will start up the sync backend even if there's no sync token available. GetCredentials() will make up fake/invalid tokens for the sync backend to use, which will eventually yield an auth error for the user.


BUG=121755
TEST=see bug


Review URL: https://chromiumcodereview.appspot.com/10335015

TBR=atwi...@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10388169

Modified:
branches/1132/src/chrome/browser/signin/signin_tracker.cc
branches/1132/src/chrome/browser/signin/signin_tracker_unittest.cc
branches/1132/src/chrome/browser/sync/profile_sync_service.cc
branches/1132/src/chrome/browser/sync/profile_sync_service.h
branches/1132/src/chrome/browser/sync/profile_sync_service_mock.h
branches/1132/src/chrome/browser/sync/sync_ui_util.cc
branches/1132/src/chrome/browser/ui/webui/options2/browser_options_handler2.cc
branches/1132/src/chrome/browser/ui/webui/sync_setup_handler.cc
branches/1132/src/chrome/browser/ui/webui/sync_setup_handler_unittest.cc

Modified: branches/1132/src/chrome/browser/signin/signin_tracker.cc
==============================================================================
--- branches/1132/src/chrome/browser/signin/signin_tracker.cc (original)
+++ branches/1132/src/chrome/browser/signin/signin_tracker.cc Wed May 16 12:34:09 2012
@@ -174,7 +174,8 @@
return false;
ProfileSyncService* service =
ProfileSyncServiceFactory::GetForProfile(profile);
- return (service->AreCredentialsAvailable() &&
+ return (service->IsSyncEnabledAndLoggedIn() &&
+ service->IsSyncTokenAvailable() &&
service->GetAuthError().state() == GoogleServiceAuthError::NONE &&
!service->unrecoverable_error_detected());
}

Modified: branches/1132/src/chrome/browser/signin/signin_tracker_unittest.cc
==============================================================================
--- branches/1132/src/chrome/browser/signin/signin_tracker_unittest.cc (original)
+++ branches/1132/src/chrome/browser/signin/signin_tracker_unittest.cc Wed May 16 12:34:09 2012
@@ -93,7 +93,7 @@
// SIGNIN_SUCCEEDED notification should lead us to get a GaiCredentialsValid()
// callback.
EXPECT_CALL(observer_, GaiaCredentialsValid());
- EXPECT_CALL(*mock_pss_, AreCredentialsAvailable())
+ EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn())
.WillRepeatedly(Return(false));
EXPECT_CALL(*mock_token_service_, HasTokenForService(_))
.WillRepeatedly(Return(false));
@@ -111,7 +111,9 @@
EXPECT_CALL(*token_service, HasTokenForService(_))
.WillRepeatedly(Return(true));
}
- EXPECT_CALL(*sync_service, AreCredentialsAvailable()).WillRepeatedly(
+ EXPECT_CALL(*sync_service, IsSyncEnabledAndLoggedIn()).WillRepeatedly(
+ Return(true));
+ EXPECT_CALL(*sync_service, IsSyncTokenAvailable()).WillRepeatedly(
Return(true));
EXPECT_CALL(*sync_service, waiting_for_auth()).WillRepeatedly(Return(false));
EXPECT_CALL(*sync_service, GetAuthError()).WillRepeatedly(ReturnRef(error));
@@ -217,7 +219,9 @@
EXPECT_CALL(observer_, GaiaCredentialsValid());
EXPECT_CALL(*mock_token_service_, HasTokenForService(_))
.WillRepeatedly(Return(true));
- EXPECT_CALL(*mock_pss_, AreCredentialsAvailable()).WillRepeatedly(
+ EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn()).WillRepeatedly(
+ Return(false));
+ EXPECT_CALL(*mock_pss_, IsSyncTokenAvailable()).WillRepeatedly(
Return(false));
GoogleServiceSigninSuccessDetails details("user...@gmail.com", "password");
content::NotificationService::current()->Notify(
@@ -230,7 +234,7 @@
// Make sure that we don't get a SigninSuccess() callback until after the
// sync service reports that it's signed in.
EXPECT_CALL(observer_, GaiaCredentialsValid());
- EXPECT_CALL(*mock_pss_, AreCredentialsAvailable()).WillOnce(
+ EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn()).WillOnce(
Return(false));
EXPECT_CALL(*mock_token_service_, HasTokenForService(_))
.WillRepeatedly(Return(true));
@@ -251,7 +255,9 @@
// Make sure that we get a SigninFailed() callback if sync gets an error after
// initializaiton.
EXPECT_CALL(observer_, GaiaCredentialsValid());
- EXPECT_CALL(*mock_pss_, AreCredentialsAvailable()).WillRepeatedly(
+ EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn()).WillRepeatedly(
+ Return(false));
+ EXPECT_CALL(*mock_pss_, IsSyncTokenAvailable()).WillRepeatedly(
Return(false));
EXPECT_CALL(*mock_token_service_, HasTokenForService(_))
.WillRepeatedly(Return(true));

Modified: branches/1132/src/chrome/browser/sync/profile_sync_service.cc
==============================================================================
--- branches/1132/src/chrome/browser/sync/profile_sync_service.cc (original)
+++ branches/1132/src/chrome/browser/sync/profile_sync_service.cc Wed May 16 12:34:09 2012
@@ -158,27 +158,19 @@
Shutdown();
}

-bool ProfileSyncService::AreCredentialsAvailable() {
- 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())
+bool ProfileSyncService::IsSyncEnabledAndLoggedIn() {
+ // Exit if sync is disabled.
+ if (IsManaged() || sync_prefs_.IsStartSuppressed())
return false;

- // CrOS user is always logged in. Chrome uses signin_ to check logged in.
- if (signin_->GetAuthenticatedUsername().empty())
- return false;
+ // Sync is logged in if there is a non-empty authenticated username.
+ return !signin_->GetAuthenticatedUsername().empty();
+}

+bool ProfileSyncService::IsSyncTokenAvailable() {
TokenService* token_service = TokenServiceFactory::GetForProfile(profile_);
if (!token_service)
return false;
-
- // TODO(chron): Verify CrOS unit test behavior.
return token_service->HasTokenForService(GaiaConstants::kSyncService);
}

@@ -207,7 +199,15 @@
}

void ProfileSyncService::TryStart() {
- if (!sync_prefs_.IsStartSuppressed() && AreCredentialsAvailable()) {
+ if (!IsSyncEnabledAndLoggedIn())
+ return;
+ TokenService* token_service = TokenServiceFactory::GetForProfile(profile_);
+ if (!token_service)
+ return;
+ // Don't start the backend if the token service hasn't finished loading tokens
+ // yet (if the backend is started before the sync token has been loaded,
+ // GetCredentials() will return bogus credentials).
+ if (IsSyncTokenAvailable() || token_service->TokensLoadedFromDB()) {
if (HasSyncSetupCompleted() || auto_start_enabled_) {
// If sync setup has completed we always start the backend.
// If autostart is enabled, but we haven't completed sync setup, we try to
@@ -217,15 +217,6 @@
// be done by the wizard.
StartUp();
}
- } else if (HasSyncSetupCompleted()) {
- TokenService* token_service = TokenServiceFactory::GetForProfile(profile_);
- if (token_service && token_service->TokensLoadedFromDB() &&
- !AreCredentialsAvailable()) {
- // The token service has lost sync's tokens. We cannot recover from this
- // without signing back in, which is not yet supported. For now, we
- // trigger an unrecoverable error.
- OnUnrecoverableError(FROM_HERE, "Sync credentials lost.");
- }
}
}

@@ -312,8 +303,16 @@
credentials.email = signin_->GetAuthenticatedUsername();
DCHECK(!credentials.email.empty());
TokenService* service = TokenServiceFactory::GetForProfile(profile_);
- credentials.sync_token = service->GetTokenForService(
- GaiaConstants::kSyncService);
+ if (service->HasTokenForService(GaiaConstants::kSyncService)) {
+ credentials.sync_token = service->GetTokenForService(
+ GaiaConstants::kSyncService);
+ UMA_HISTOGRAM_BOOLEAN("Sync.CredentialsLost", false);
+ } else {
+ // We've lost our sync credentials (crbug.com/121755), so just make up some
+ // invalid credentials so the backend will generate an auth error.
+ UMA_HISTOGRAM_BOOLEAN("Sync.CredentialsLost", true);
+ credentials.sync_token = "credentials_lost";
+ }
return credentials;
}

@@ -404,7 +403,7 @@
return;
}

- DCHECK(AreCredentialsAvailable());
+ DCHECK(IsSyncEnabledAndLoggedIn());

last_synced_time_ = sync_prefs_.GetLastSyncedTime();

@@ -1390,7 +1389,10 @@
NotifyObservers();
if (is_sync_managed) {
DisableForUser();
- } else if (HasSyncSetupCompleted() && AreCredentialsAvailable()) {
+ } else if (HasSyncSetupCompleted() &&
+ IsSyncEnabledAndLoggedIn() &&
+ IsSyncTokenAvailable()) {
+ // Previously-configured sync has been re-enabled, so start sync now.
StartUp();
}
}
@@ -1512,8 +1514,8 @@
*(content::Details<const TokenService::TokenRequestFailedDetails>(
details).ptr());
if (IsTokenServiceRelevant(token_details.service()) &&
- !AreCredentialsAvailable()) {
- // The additional check around AreCredentialsAvailable above prevents us
+ !IsSyncTokenAvailable()) {
+ // The additional check around IsSyncTokenAvailable() above prevents us
// sounding the alarm if we actually have a valid token but a refresh
// attempt by TokenService failed for any variety of reasons (e.g. flaky
// network). It's possible the token we do have is also invalid, but in
@@ -1530,10 +1532,11 @@
*(content::Details<const TokenService::TokenAvailableDetails>(
details).ptr());
if (IsTokenServiceRelevant(token_details.service()) &&
- AreCredentialsAvailable()) {
+ IsSyncEnabledAndLoggedIn() &&
+ IsSyncTokenAvailable()) {
if (backend_initialized_)
backend_->UpdateCredentials(GetCredentials());
- else if (!sync_prefs_.IsStartSuppressed())
+ else
StartUp();
}
break;
@@ -1541,20 +1544,14 @@
case chrome::NOTIFICATION_TOKEN_LOADING_FINISHED: {
// This notification gets fired when TokenService loads the tokens
// from storage.
- if (AreCredentialsAvailable()) {
- // Initialize the backend if sync token was loaded.
- if (backend_initialized_) {
+ if (IsSyncEnabledAndLoggedIn()) {
+ // Initialize the backend if sync is enabled. If the sync token was
+ // not loaded, GetCredentials() will generate invalid credentials to
+ // cause the backend to generate an auth error (crbug.com/121755).
+ if (backend_initialized_)
backend_->UpdateCredentials(GetCredentials());
- }
- if (!sync_prefs_.IsStartSuppressed())
+ else
StartUp();
- } else if (!auto_start_enabled_ &&
- !signin_->GetAuthenticatedUsername().empty() &&
- HasSyncSetupCompleted()) {
- // If not in auto-start / Chrome OS mode, and we have a username
- // without tokens, the user will need to signin again. At the moment
- // this is not supported, so we trigger an unrecoverable error.
- OnUnrecoverableError(FROM_HERE, "Sync credentials lost.");
}
break;
}

Modified: branches/1132/src/chrome/browser/sync/profile_sync_service.h
==============================================================================
--- branches/1132/src/chrome/browser/sync/profile_sync_service.h (original)
+++ branches/1132/src/chrome/browser/sync/profile_sync_service.h Wed May 16 12:34:09 2012
@@ -180,9 +180,16 @@

void RegisterAuthNotifications();

+ // Returns true if sync is enabled/not suppressed and the user is logged in.
+ // (being logged in does not mean that tokens are available - tokens may
+ // be missing because they have not loaded yet, or because they were deleted
+ // due to http://crbug.com/121755).
+ // Virtual to enable mocking in tests.
+ virtual bool IsSyncEnabledAndLoggedIn();
+
// Return whether all sync tokens are loaded and available for the backend to
// start up. Virtual to enable mocking in tests.
- virtual bool AreCredentialsAvailable();
+ virtual bool IsSyncTokenAvailable();

// Registers a data type controller with the sync service. This
// makes the data type controller available for use, it does not

Modified: branches/1132/src/chrome/browser/sync/profile_sync_service_mock.h
==============================================================================
--- branches/1132/src/chrome/browser/sync/profile_sync_service_mock.h (original)
+++ branches/1132/src/chrome/browser/sync/profile_sync_service_mock.h Wed May 16 12:34:09 2012
@@ -87,7 +87,8 @@
MOCK_METHOD1(OnActionableError, void(
const browser_sync::SyncProtocolError&));

- MOCK_METHOD0(AreCredentialsAvailable, bool());
+ MOCK_METHOD0(IsSyncEnabledAndLoggedIn, bool());
+ MOCK_METHOD0(IsSyncTokenAvailable, bool());

MOCK_CONST_METHOD0(IsPassphraseRequired, bool());
MOCK_CONST_METHOD0(IsPassphraseRequiredForDecryption, bool());

Modified: branches/1132/src/chrome/browser/sync/sync_ui_util.cc
==============================================================================
--- branches/1132/src/chrome/browser/sync/sync_ui_util.cc (original)
+++ branches/1132/src/chrome/browser/sync/sync_ui_util.cc Wed May 16 12:34:09 2012
@@ -551,7 +551,7 @@
user_state, "Username",
service->signin() ? service->signin()->GetAuthenticatedUsername() : "");
sync_ui_util::AddBoolSyncDetail(
- user_state, "Sync Token Available", service->AreCredentialsAvailable());
+ user_state, "Sync Token Available", service->IsSyncTokenAvailable());

ListValue* local_state = AddSyncDetailsSection(details, "Local State");
sync_ui_util::AddStringSyncDetails(local_state, "Last Synced",

Modified: branches/1132/src/chrome/browser/ui/webui/options2/browser_options_handler2.cc
==============================================================================
--- branches/1132/src/chrome/browser/ui/webui/options2/browser_options_handler2.cc (original)
+++ branches/1132/src/chrome/browser/ui/webui/options2/browser_options_handler2.cc Wed May 16 12:34:09 2012
@@ -1027,9 +1027,10 @@
sync_status->SetBoolean("managed", service->IsManaged());
sync_status->SetBoolean("hasUnrecoverableError",
service->unrecoverable_error_detected());
- sync_status->SetBoolean("autoLoginVisible",
+ sync_status->SetBoolean(
+ "autoLoginVisible",
CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableAutologin) &&
- service->AreCredentialsAvailable());
+ service->IsSyncEnabledAndLoggedIn() && service->IsSyncTokenAvailable());

return sync_status.Pass();
}

Modified: branches/1132/src/chrome/browser/ui/webui/sync_setup_handler.cc
==============================================================================
--- branches/1132/src/chrome/browser/ui/webui/sync_setup_handler.cc (original)
+++ branches/1132/src/chrome/browser/ui/webui/sync_setup_handler.cc Wed May 16 12:34:09 2012
@@ -872,7 +872,7 @@

// There are several different UI flows that can bring the user here:
// 1) Signin promo (passes force_login=true)
- // 2) Normal signin through options page (AreCredentialsAvailable() will
+ // 2) Normal signin through options page (IsSyncEnabledAndLoggedIn() will
// return false).
// 3) Previously working credentials have expired
// (service->GetAuthError() != NONE).
@@ -885,7 +885,7 @@
// 7) ChromeOS re-enable after disabling sync.
#if !defined(OS_CHROMEOS)
if (force_login ||
- !service->AreCredentialsAvailable() ||
+ !service->IsSyncEnabledAndLoggedIn() ||
service->GetAuthError().state() != GoogleServiceAuthError::NONE) {
// User is not logged in, or login has been specially requested - need to
// display login UI (cases 1-4).

Modified: branches/1132/src/chrome/browser/ui/webui/sync_setup_handler_unittest.cc
==============================================================================
--- branches/1132/src/chrome/browser/ui/webui/sync_setup_handler_unittest.cc (original)
+++ branches/1132/src/chrome/browser/ui/webui/sync_setup_handler_unittest.cc Wed May 16 12:34:09 2012
@@ -333,7 +333,9 @@
void SetupInitializedProfileSyncService() {
// An initialized ProfileSyncService will have already completed sync setup
// and will have an initialized sync backend.
- EXPECT_CALL(*mock_pss_, AreCredentialsAvailable())
+ EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn())
+ .WillRepeatedly(Return(true));
+ EXPECT_CALL(*mock_pss_, IsSyncTokenAvailable())
.WillRepeatedly(Return(true));
EXPECT_CALL(*mock_pss_, HasSyncSetupCompleted())
.WillRepeatedly(Return(true));
@@ -373,7 +375,9 @@

#if !defined(OS_CHROMEOS)
TEST_F(SyncSetupHandlerTest, DisplayBasicLogin) {
- EXPECT_CALL(*mock_pss_, AreCredentialsAvailable())
+ EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn())
+ .WillRepeatedly(Return(false));
+ EXPECT_CALL(*mock_pss_, IsSyncTokenAvailable())
.WillRepeatedly(Return(false));
EXPECT_CALL(*mock_pss_, HasSyncSetupCompleted())
.WillRepeatedly(Return(false));
@@ -399,7 +403,9 @@
}

TEST_F(SyncSetupHandlerTest, DisplayForceLogin) {
- EXPECT_CALL(*mock_pss_, AreCredentialsAvailable())
+ EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn())
+ .WillRepeatedly(Return(false));
+ EXPECT_CALL(*mock_pss_, IsSyncTokenAvailable())
.WillRepeatedly(Return(false));
EXPECT_CALL(*mock_pss_, HasSyncSetupCompleted())
.WillRepeatedly(Return(true));
@@ -427,7 +433,9 @@
}

TEST_F(SyncSetupHandlerTest, HandleGaiaAuthFailure) {
- EXPECT_CALL(*mock_pss_, AreCredentialsAvailable())
+ EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn())
+ .WillRepeatedly(Return(false));
+ EXPECT_CALL(*mock_pss_, IsSyncTokenAvailable())
.WillRepeatedly(Return(false));
EXPECT_CALL(*mock_pss_, unrecoverable_error_detected())
.WillRepeatedly(Return(false));
@@ -457,7 +465,9 @@
}

TEST_F(SyncSetupHandlerTest, HandleCaptcha) {
- EXPECT_CALL(*mock_pss_, AreCredentialsAvailable())
+ EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn())
+ .WillRepeatedly(Return(false));
+ EXPECT_CALL(*mock_pss_, IsSyncTokenAvailable())
.WillRepeatedly(Return(false));
EXPECT_CALL(*mock_pss_, unrecoverable_error_detected())
.WillRepeatedly(Return(false));
@@ -488,7 +498,9 @@
}

TEST_F(SyncSetupHandlerTest, HandleFatalError) {
- EXPECT_CALL(*mock_pss_, AreCredentialsAvailable())
+ EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn())
+ .WillRepeatedly(Return(false));
+ EXPECT_CALL(*mock_pss_, IsSyncTokenAvailable())
.WillRepeatedly(Return(false));
EXPECT_CALL(*mock_pss_, HasSyncSetupCompleted())
.WillRepeatedly(Return(false));
@@ -510,7 +522,9 @@
#if !defined(OS_CHROMEOS)
// TODO(kochi): We need equivalent tests for ChromeOS.
TEST_F(SyncSetupHandlerTest, UnrecoverableErrorInitializingSync) {
- EXPECT_CALL(*mock_pss_, AreCredentialsAvailable())
+ EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn())
+ .WillRepeatedly(Return(false));
+ EXPECT_CALL(*mock_pss_, IsSyncTokenAvailable())
.WillRepeatedly(Return(false));
EXPECT_CALL(*mock_pss_, HasSyncSetupCompleted())
.WillRepeatedly(Return(false));
@@ -547,7 +561,9 @@
}

TEST_F(SyncSetupHandlerTest, GaiaErrorInitializingSync) {
- EXPECT_CALL(*mock_pss_, AreCredentialsAvailable())
+ EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn())
+ .WillRepeatedly(Return(false));
+ EXPECT_CALL(*mock_pss_, IsSyncTokenAvailable())
.WillRepeatedly(Return(false));
EXPECT_CALL(*mock_pss_, HasSyncSetupCompleted())
.WillRepeatedly(Return(false));
@@ -794,7 +810,9 @@
GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS);
SetupInitializedProfileSyncService();
mock_signin_->SetAuthenticatedUsername(kTestUser);
- EXPECT_CALL(*mock_pss_, AreCredentialsAvailable())
+ EXPECT_CALL(*mock_pss_, IsSyncEnabledAndLoggedIn())
+ .WillRepeatedly(Return(true));
+ EXPECT_CALL(*mock_pss_, IsSyncTokenAvailable())
.WillRepeatedly(Return(true));
EXPECT_CALL(*mock_pss_, IsPassphraseRequired())
.WillRepeatedly(Return(false));