[sync] Add a polling mechanism to CredentialCacheService for on-the-fly sign in / sign out / reconf…

chrome/browser/sync/credential_cache_service_win.cc

Index: chrome/browser/sync/credential_cache_service_win.cc
diff --git a/chrome/browser/sync/credential_cache_service_win.cc b/chrome/browser/sync/credential_cache_service_win.cc
index bd5e8de7aae84c37e04ab1b041cbf279614a0a5a..13440a3951edf78ae27bd26d07bb9df84154d9b7 100644
--- a/chrome/browser/sync/credential_cache_service_win.cc
+++ b/chrome/browser/sync/credential_cache_service_win.cc
@@ -9,6 +9,7 @@
 #include "base/base64.h"
 #include "base/compiler_specific.h"
 #include "base/file_util.h"
+#include "base/time.h"
 #include "base/values.h"
 #include "base/win/windows_version.h"
 #include "chrome/browser/prefs/pref_service.h"
@@ -33,6 +34,12 @@
 
 namespace syncer {
 
+// The time delay (in seconds) between two consecutive polls of the alternate
+// credential cache. A one minute delay seems like a reasonable amount of time
+// in which to propagate changes to signed in state between Metro and Desktop.
+const int kCredentialCachePollIntervalSecs = 60;
+
+using base::TimeDelta;
 using content::BrowserThread;
 
 CredentialCacheService::CredentialCacheService(Profile* profile)
@@ -64,9 +71,14 @@ void CredentialCacheService::Shutdown() {
 
 void CredentialCacheService::OnInitializationCompleted(bool succeeded) {
   DCHECK(succeeded);
-  // When the alternate credential store becomes available, begin consuming its
-  // cached credentials.
-  if (alternate_store_.get() && alternate_store_->IsInitializationComplete()) {
+  // When the local and alternate credential stores become available, begin
+  // consuming the alternate cached credentials. We must also wait for the local
+  // credential store because the credentials read from the alternate cache and
+  // applied locally must eventually get stored in the local cache.
+  if (alternate_store_.get() &&
+      alternate_store_->IsInitializationComplete() &&
+      local_store_.get() &&
+      local_store_->IsInitializationComplete()) {
     ReadCachedCredentialsFromAlternateProfile();
   }
 }
@@ -245,13 +257,7 @@ bool CredentialCacheService::ShouldLookForCachedCredentialsInAlternateProfile()
   // true:
   // 1) Sync is not disabled by policy.
   // 2) Sync startup is not suppressed.
-  // 3) No user is currently signed in to sync.
-  DCHECK(profile_);
-  PrefService* prefs = profile_->GetPrefs();
-  DCHECK(prefs);
-  return !sync_prefs_.IsManaged() &&
-         !sync_prefs_.IsStartSuppressed() &&
-         prefs->GetString(prefs::kGoogleServicesUsername).empty();
+  return !sync_prefs_.IsManaged() && !sync_prefs_.IsStartSuppressed();

[Andrew T Wilson (Slow), 2012/08/03 20:39:56]

Should we document why we want to look for credentials even if we're already
signed in (is this to detect signouts in the alternate profile?)

[Raghu Simha, 2012/08/03 21:57:35]

Yes, it is to detect sign outs in the alternate profile. Comment added.

 }
 
 void CredentialCacheService::InitializeLocalCredentialCacheWriter() {
@@ -259,6 +265,7 @@ void CredentialCacheService::InitializeLocalCredentialCacheWriter() {
       GetCredentialPathInCurrentProfile(),
       content::BrowserThread::GetMessageLoopProxyForThread(
           content::BrowserThread::FILE));
+  local_store_->AddObserver(this);
   local_store_->ReadPrefsAsync(NULL);
 
   // Register for notifications for updates to the sync credentials, which are
@@ -267,11 +274,12 @@ void CredentialCacheService::InitializeLocalCredentialCacheWriter() {
   pref_registrar_.Add(prefs::kSyncEncryptionBootstrapToken, this);
   pref_registrar_.Add(prefs::kGoogleServicesUsername, this);
   pref_registrar_.Add(prefs::kSyncKeepEverythingSynced, this);
-  for (int i = FIRST_REAL_MODEL_TYPE; i < MODEL_TYPE_COUNT; ++i) {
-    if (i == NIGORI)  // The NIGORI preference is not persisted.
+  ModelTypeSet all_types = syncer::ModelTypeSet::All();
+  for (ModelTypeSet::Iterator it = all_types.First(); it.Good(); it.Inc()) {
+    if (it.Get() == NIGORI)  // The NIGORI preference is not persisted.
       continue;
     pref_registrar_.Add(
-        browser_sync::SyncPrefs::GetPrefNameForDataType(ModelTypeFromInt(i)),
+        browser_sync::SyncPrefs::GetPrefNameForDataType(it.Get()),
         this);
   }
 
@@ -289,13 +297,16 @@ void CredentialCacheService::InitializeLocalCredentialCacheWriter() {
 void CredentialCacheService::InitializeAlternateCredentialCacheReader(
     bool* should_initialize) {
   // If |should_initialize| is false, there was no credential cache in the
-  // alternate profile directory, and there is nothing to do.
-  // TODO(rsimha): Add a polling mechanism that periodically examines the
-  // credential file in the alternate profile directory so we can respond to the
-  // user signing in and signing out.
+  // alternate profile directory, and there is nothing to do right now. Schedule
+  // another read in the future and exit.
   DCHECK(should_initialize);
-  if (!*should_initialize)
+  if (!*should_initialize) {
+    ScheduleNextReadFromAlternateCredentialCache();
     return;
+  }
+
+  // A credential cache file was found in the alternate profile. Prepare to
+  // consume its contents.
   alternate_store_ = new JsonPrefStore(
       GetCredentialPathInAlternateProfile(),
       content::BrowserThread::GetMessageLoopProxyForThread(
@@ -316,18 +327,15 @@ bool CredentialCacheService::HasUserSignedOut() {
 
 namespace {
 
-// Determines if credentials should be read from the alternate profile based
-// on the existence of the local and alternate credential files. Returns
-// true via |result| if there is a credential cache file in the alternate
-// profile, but there isn't one in the local profile. Returns false otherwise.
-void ShouldReadFromAlternateCache(
-    const FilePath& credential_path_in_current_profile,
+// Determines if there is a sync credential cache in the alternate profile.
+// Returns true via |result| if there is a credential cache file in the
+// alternate profile. Returns false otherwise.
+void AlternateCredentialCacheExists(
     const FilePath& credential_path_in_alternate_profile,
     bool* result) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
   DCHECK(result);
-  *result = !file_util::PathExists(credential_path_in_current_profile) &&
-            file_util::PathExists(credential_path_in_alternate_profile);
+  *result = file_util::PathExists(credential_path_in_alternate_profile);
 }
 
 }  // namespace
@@ -337,8 +345,7 @@ void CredentialCacheService::LookForCachedCredentialsInAlternateProfile() {
   content::BrowserThread::PostTaskAndReply(
       content::BrowserThread::FILE,
       FROM_HERE,
-      base::Bind(&ShouldReadFromAlternateCache,
-                 GetCredentialPathInCurrentProfile(),
+      base::Bind(&AlternateCredentialCacheExists,
                  GetCredentialPathInAlternateProfile(),
                  should_initialize),
       base::Bind(
@@ -348,16 +355,28 @@ void CredentialCacheService::LookForCachedCredentialsInAlternateProfile() {
 }
 
 void CredentialCacheService::ReadCachedCredentialsFromAlternateProfile() {
+  // If the local user has signed in and signed out, we do not consume cached
+  // credentials from the alternate profile. There is nothing more to do.
+  if (HasUserSignedOut())
+    return;
+
+  // Sanity check the alternate credential cache. If any string credentials
+  // are outright missing even though the file exists, something is awry with
+  // the alternate profile store. There is no sense in flagging an error as the
+  // problem lies in a different profile directory. Silently return as there is
+  // nothing to do.
   DCHECK(alternate_store_.get());
   if (!HasPref(alternate_store_, prefs::kGoogleServicesUsername) ||
       !HasPref(alternate_store_, GaiaConstants::kGaiaLsid) ||
       !HasPref(alternate_store_, GaiaConstants::kGaiaSid) ||
       !HasPref(alternate_store_, prefs::kSyncEncryptionBootstrapToken) ||
       !HasPref(alternate_store_, prefs::kSyncKeepEverythingSynced)) {
-    VLOG(1) << "Could not find cached credentials.";
+    VLOG(1) << "Could not find cached credentials in \""
+            << GetCredentialPathInAlternateProfile().value() << "\".";
     return;
   }
 
+  // Extract cached credentials from the alternate credential cache.
   std::string google_services_username =
       GetAndUnpackStringPref(alternate_store_, prefs::kGoogleServicesUsername);
   std::string lsid =
@@ -367,45 +386,93 @@ void CredentialCacheService::ReadCachedCredentialsFromAlternateProfile() {
   std::string encryption_bootstrap_token =
       GetAndUnpackStringPref(alternate_store_,
                              prefs::kSyncEncryptionBootstrapToken);
-  bool keep_everything_synced =
-      GetBooleanPref(alternate_store_, prefs::kSyncKeepEverythingSynced);
 
-  if (google_services_username.empty() ||
-      lsid.empty() ||
-      sid.empty() ||
-      encryption_bootstrap_token.empty()) {
-    VLOG(1) << "Found empty cached credentials.";
+  // Sign out of sync iff:
+  // 1) The user is signed in to the local profile.
+  // 2) The user has never signed out of the local profile in the past.
+  // 3) We just notice that the user has signed out of the alternate profile.
+  // 4) The user is not already in the process of configuring sync.
+  // There is no need to schedule any more reads of the alternate profile
+  // cache because we only apply cached credentials for first-time sign-ins.
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  if (!sync_prefs_.GetGoogleServicesUsername().empty() &&

[Andrew T Wilson (Slow), 2012/08/03 20:39:56]

Let's change all these references to SigninManager::GetAuthenticatedUsername()
instead of going through sync_prefs.

[Raghu Simha, 2012/08/03 21:57:35]

Done.

+      !HasUserSignedOut() &&
+      google_services_username.empty() &&
+      !service->setup_in_progress()) {

[Andrew T Wilson (Slow), 2012/08/03 20:39:56]

Why do we want to avoid signing out the user if they are configuring sync? How
does the code behave if I do this:

a) Bring up sync settings on Metro
b) on desktop sign out
c) switch to metro, close sync settings

At what point does Metro get signed out? It seems dangerous for us to continue
on in this routine in either case - maybe this case is handled by
MayReconfigureSync below?

[Raghu Simha, 2012/08/03 21:57:35]

If the user is configuring sync, we want to avoid second guessing (and
overriding) their intentions. Signing out a user from underneath when they are
actively configuring sync seems like a bad idea.

 How
In the case you describe, Metro will get signed out during the next poll, after
you close the sync settings.

 It seems dangerous for us to continue
Yes. MayReconfigureSync() will return false if the user is in the process of
configuring sync, and we take no further action in this routine.

+    VLOG(1) << "User has signed out on the other profile. Signing out.";
+    InitiateSignOut();
     return;
   }
 
-  bool datatype_prefs[MODEL_TYPE_COUNT] = { false };
-  for (int i = FIRST_REAL_MODEL_TYPE; i < MODEL_TYPE_COUNT; ++i) {
-    if (i == NIGORI)  // The NIGORI preference is not persisted.
-      continue;
+  // Extract cached sync prefs from the alternate credential cache.
+  bool keep_everything_synced =
+      GetBooleanPref(alternate_store_, prefs::kSyncKeepEverythingSynced);
+  ModelTypeSet preferred_types;
+  ModelTypeSet registered_types = service->GetRegisteredDataTypes();
+  for (ModelTypeSet::Iterator it = registered_types.First();
+       it.Good();
+       it.Inc()) {
     std::string datatype_pref_name =
-        browser_sync::SyncPrefs::GetPrefNameForDataType(ModelTypeFromInt(i));
+        browser_sync::SyncPrefs::GetPrefNameForDataType(it.Get());
     if (!HasPref(alternate_store_, datatype_pref_name)) {
-      VLOG(1) << "Could not find cached datatype prefs.";
-      return;
+      // If there is no cached pref for a specific data type, it means that the
+      // user originally signed in with an older version of Chrome, and then
+      // upgraded to a version with a new datatype. In such cases, we leave the
+      // default initial datatype setting as false while reading cached
+      // credentials, just like we do in SyncPrefs::RegisterPreferences.
+      VLOG(1) << "Could not find cached datatype pref for "
+              << datatype_pref_name << " in "
+              << GetCredentialPathInAlternateProfile().value() << ".";
+      continue;
+    }
+    if (GetBooleanPref(alternate_store_, datatype_pref_name))
+      preferred_types.Put(it.Get());

[Andrew T Wilson (Slow), 2012/08/03 20:39:56]

Is this loop dangerous to execute if the user is configuring sync currently (we
might change settings out from under it?)

[Raghu Simha, 2012/08/03 21:57:35]

I don't think it's dangerous to run this loop. It's merely extracting a set of
preferred sync datatypes from the blob that we read from the alternate
credential cache. We don't actually change any settings in the local pref store
in this loop. And when the prefs are actually applied to the local store, we
make sure that the user is not already configuring sync.

+  }
+
+  // Reconfigure if sync settings or credentials have changed.
+  // Follow this up by scheduling a future read from the alternate profile.
+  if (MayReconfigureSync(google_services_username)) {
+    if (HaveSyncPrefsChanged(keep_everything_synced, preferred_types)) {
+      VLOG(1) << "Sync prefs have changed in other profile. Reconfiguring.";
+      service->OnUserChoseDatatypes(keep_everything_synced, preferred_types);
+    }
+    if (HaveTokenServiceCredentialsChanged(lsid, sid)) {
+      VLOG(1) << "Token service credentials have changed in other profile.";
+      UpdateTokenServiceCredentials(lsid, sid);
     }
-    datatype_prefs[i] = GetBooleanPref(alternate_store_, datatype_pref_name);
+    ScheduleNextReadFromAlternateCredentialCache();
+    return;
   }
 
-  ApplyCachedCredentials(google_services_username,
-                         lsid,
-                         sid,
-                         encryption_bootstrap_token,
-                         keep_everything_synced,
-                         datatype_prefs);
+  // Try signing in with cached credentials from the alternate profile iff:
+  // 1) The user is not currently signed in to the local profile.
+  // 2) The user has never signed out of the local profile in the past.
+  // 3) Valid cached credentials are available in the alternate profile.
+  // 4) The user is not already in the process of configuring sync.
+  // Follow this up by scheduling a future read from the alternate profile.
+  if (sync_prefs_.GetGoogleServicesUsername().empty() &&
+      !HasUserSignedOut() &&
+      !google_services_username.empty() &&
+      !lsid.empty() &&
+      !sid.empty() &&
+      !encryption_bootstrap_token.empty() &&
+      !service->setup_in_progress()) {
+    InitiateSignInWithCachedCredentials(google_services_username,
+                                        encryption_bootstrap_token,
+                                        keep_everything_synced,
+                                        preferred_types);
+    UpdateTokenServiceCredentials(lsid, sid);
+    ScheduleNextReadFromAlternateCredentialCache();
+    return;
+  }
 }
 
-void CredentialCacheService::ApplyCachedCredentials(
+void CredentialCacheService::InitiateSignInWithCachedCredentials(
     const std::string& google_services_username,
-    const std::string& lsid,
-    const std::string& sid,
     const std::string& encryption_bootstrap_token,
     bool keep_everything_synced,
-    const bool datatype_prefs[]) {
+    ModelTypeSet preferred_types) {
   // Update the google username in the SigninManager and PrefStore.
   ProfileSyncService* service =
       ProfileSyncServiceFactory::GetForProfile(profile_);
@@ -418,19 +485,13 @@ void CredentialCacheService::ApplyCachedCredentials(
   sync_prefs_.SetSyncSetupCompleted();
   sync_prefs_.SetEncryptionBootstrapToken(encryption_bootstrap_token);
   sync_prefs_.SetKeepEverythingSynced(keep_everything_synced);
-  syncer::ModelTypeSet registered_types;
-  syncer::ModelTypeSet preferred_types;
-  for (int i = FIRST_REAL_MODEL_TYPE; i < MODEL_TYPE_COUNT; ++i) {
-    if (i == NIGORI)  // The NIGORI preference is not persisted.
-      continue;
-    registered_types.Put(ModelTypeFromInt(i));
-    if (datatype_prefs[i])
-      preferred_types.Put(ModelTypeFromInt(i));
-  }
-  sync_prefs_.SetPreferredDataTypes(registered_types, preferred_types);
+  sync_prefs_.SetPreferredDataTypes(service->GetRegisteredDataTypes(),
+                                    preferred_types);
+}
 
-  // Update the lsid and sid in the TokenService and mint new tokens for all
-  // Chrome services.
+void CredentialCacheService::UpdateTokenServiceCredentials(
+    const std::string& lsid,
+    const std::string& sid) {
   GaiaAuthConsumer::ClientLoginResult login_result;
   login_result.lsid = lsid;
   login_result.sid = sid;
@@ -440,4 +501,63 @@ void CredentialCacheService::ApplyCachedCredentials(
   token_service->StartFetchingTokens();
 }
 
+void CredentialCacheService::InitiateSignOut() {
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  service->DisableForUser();

[Andrew T Wilson (Slow), 2012/08/03 20:39:56]

This is fine, but it's slightly better to use SigninManager::SignOut() instead.

[Raghu Simha, 2012/08/03 21:57:35]

Done.

[Raghu Simha, 2012/08/03 22:12:52]

Actually, PSS doesn't handle the notification sent by SigninManager:SignOut(),
so what you suggest is not going to work. Of course, DisableForUser() implicitly
calls SigninManager:SignOut(), so other parts of the browser that rely on the
notification will end up doing the right thing.

+}
+
+bool CredentialCacheService::HaveSyncPrefsChanged(
+    bool keep_everything_synced,
+    const ModelTypeSet& preferred_types) const {
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  ModelTypeSet local_preferred_types =
+      sync_prefs_.GetPreferredDataTypes(service->GetRegisteredDataTypes());
+  return
+      (keep_everything_synced != sync_prefs_.HasKeepEverythingSynced()) ||
+      !Difference(preferred_types, local_preferred_types).Empty();
+}
+
+bool CredentialCacheService::HaveTokenServiceCredentialsChanged(
+    const std::string& lsid,
+    const std::string& sid) {
+  std::string local_lsid =
+      GetAndUnpackStringPref(local_store_, GaiaConstants::kGaiaLsid);
+  std::string local_sid =
+      GetAndUnpackStringPref(local_store_, GaiaConstants::kGaiaSid);
+  return local_lsid != lsid || local_sid != sid;
+}
+
+bool CredentialCacheService::MayReconfigureSync(
+    const std::string& google_services_username) {
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  // We may attempt to reconfigure sync iff:
+  // 1) The user is signed in to the local profile.
+  // 2) The user has never signed out of the local profile in the past.
+  // 3) The user is signed in to the alternate profile with the same account.

[Andrew T Wilson (Slow), 2012/08/03 20:39:56]

You left out step 4: Profit

[Raghu Simha, 2012/08/03 21:57:35]

Hah! Fixed.

+  // 5) The user is not already in the process of configuring sync.
+  return !sync_prefs_.GetGoogleServicesUsername().empty() &&
+         !HasUserSignedOut() &&
+         google_services_username == sync_prefs_.GetGoogleServicesUsername() &&
+         !service->setup_in_progress();
+}
+
+void CredentialCacheService::ScheduleNextReadFromAlternateCredentialCache() {
+  // We must reinitialize |alternate_store_| here because the underlying
+  // credential file in the alternate profile might have changed, and we must
+  // re-read it afresh.
+  if (alternate_store_.get()) {
+    alternate_store_->RemoveObserver(this);
+    alternate_store_.release();
+  }
+  MessageLoop::current()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(
+          &CredentialCacheService::LookForCachedCredentialsInAlternateProfile,
+          weak_factory_.GetWeakPtr()),
+      TimeDelta::FromSeconds(kCredentialCachePollIntervalSecs));
+}
+
 }  // namespace syncer