[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..bf590bbe149101235ca5af2ec55d5f5a2d7276f6 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,17 @@
 
 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;

[Andrew T Wilson (Slow), 2012/08/03 23:33:24]

Now I wonder if 60 secs is too frequent for us to be reading and re-parsing a
JSON file. What do you think?

[Raghu Simha, 2012/08/04 00:18:02]

I could increase this to 2 minutes, or even 5 minutes. I think increasing the
delay is perfectly fine for the sign-out case. The question is: How fast fo we
want a user to show up as signed in on the other side if both modes are active?
If we up the interval to 5 minutes, would the user be tempted to manually sign
in, thereby defeating the purpose of this feature?

How about making the timeout 2 minutes for now?

+
+// Keeps track of the last time a credential cache was written to. Used to make
+// sure that we only apply changes from newer credential caches to older ones,
+// and not vice versa.
+const char kLastUpdatedTime[] = "last_updated_time";
+
+using base::TimeDelta;
 using content::BrowserThread;
 
 CredentialCacheService::CredentialCacheService(Profile* profile)
@@ -64,9 +76,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();
   }
 }
@@ -176,6 +193,14 @@ std::string CredentialCacheService::UnpackCredential(
   return unencrypted;
 }
 
+void CredentialCacheService::WriteLastUpdatedTime() {
+  DCHECK(local_store_.get());
+  int last_updated_time = base::TimeTicks::Now().ToInternalValue();
+  local_store_->SetValueSilently(
+      kLastUpdatedTime,
+      base::Value::CreateIntegerValue(last_updated_time));
+}
+
 void CredentialCacheService::PackAndUpdateStringPref(
       const std::string& pref_name,
       const std::string& new_value) {
@@ -187,6 +212,7 @@ void CredentialCacheService::PackAndUpdateStringPref(
     // sign-ins.
     local_store_->SetValueSilently(pref_name, PackCredential(std::string()));
   }
+  WriteLastUpdatedTime();
 }
 
 void CredentialCacheService::UpdateBooleanPref(const std::string& pref_name,
@@ -201,6 +227,16 @@ void CredentialCacheService::UpdateBooleanPref(const std::string& pref_name,
     local_store_->SetValueSilently(pref_name,
                                    base::Value::CreateBooleanValue(false));
   }
+  WriteLastUpdatedTime();
+}
+
+int CredentialCacheService::GetLastUpdatedTime(
+    scoped_refptr<JsonPrefStore> store) {
+  const base::Value* last_updated_time_value = NULL;
+  store->GetValue(kLastUpdatedTime, &last_updated_time_value);
+  int last_updated_time;
+  last_updated_time_value->GetAsInteger(&last_updated_time);

[Andrew T Wilson (Slow), 2012/08/03 23:33:24]

Should we add a CHECK here for the return value? Or at least a
DCHECK(IsType(TYPE_INTEGER))?

[Raghu Simha, 2012/08/04 00:18:02]

I've now made this a string pref that's converted from the int64 returned by
Now(). We were losing precision with just an integer pref, which was a bad
thing. Also added a DCHECK to make sure that the conversion worked.

+  return last_updated_time;
 }
 
 std::string CredentialCacheService::GetAndUnpackStringPref(
@@ -245,13 +281,10 @@ 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();
+  // Note that we do want to look for credentials in the alternate profile even
+  // if the local user is signed in, so we can detect a sign out originating
+  // from the alternate profile.
+  return !sync_prefs_.IsManaged() && !sync_prefs_.IsStartSuppressed();
 }
 
 void CredentialCacheService::InitializeLocalCredentialCacheWriter() {
@@ -259,6 +292,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 +301,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 +324,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 +354,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 +372,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 +382,30 @@ 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, now or
+  // later on.
+  if (HasUserSignedOut())

[Andrew T Wilson (Slow), 2012/08/03 23:33:24]

So, if the user logs out in one profile, that should get synced to the remote
profile.

Now, what if they log back in on the other profile - do we not want to sign them
back in here?

[Raghu Simha, 2012/08/04 00:18:02]

As of now, the answer is no, as discussed in the design doc. We don't want to
risk merging in a whole lot of old state to a sync account, and more
importantly, we don't want to inadvertently mix data from two accounts if the
user swaps the account with which they were signed in.

+    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. There is nothing to do now.
+  // We schedule a future read from the alternate credential cache and return.
   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() << "\".";
+    ScheduleNextReadFromAlternateCredentialCache();
     return;
   }
 
+  // Extract cached credentials from the alternate credential cache.
   std::string google_services_username =
       GetAndUnpackStringPref(alternate_store_, prefs::kGoogleServicesUsername);
   std::string lsid =
@@ -367,45 +415,82 @@ 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 if the alternate profile has signed out the same user.
+  // 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.
+  if (ShouldSignOutOfSync(google_services_username)) {
+    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);
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  ModelTypeSet registered_types = service->GetRegisteredDataTypes();
+  ModelTypeSet preferred_types;
+  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;
     }
-    datatype_prefs[i] = GetBooleanPref(alternate_store_, datatype_pref_name);
+    if (GetBooleanPref(alternate_store_, datatype_pref_name))
+      preferred_types.Put(it.Get());
   }
 
-  ApplyCachedCredentials(google_services_username,
+  // Reconfigure if sync settings or credentials have changed in the alternate
+  // profile, but for the same user that is signed in to the local profile.
+  // Follow this up by scheduling a future read from the alternate profile, so
+  // we can detect future reconfigures or sign outs.
+  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);
+    }
+    ScheduleNextReadFromAlternateCredentialCache();
+    return;
+  }
+
+  // Sign in if we notice new cached credentials in the alternate profile.
+  // Follow this up by scheduling a future read from the alternate profile, so
+  // we can detect future reconfigures or sign outs.
+  if (ShouldSignInToSync(google_services_username,
                          lsid,
                          sid,
-                         encryption_bootstrap_token,
-                         keep_everything_synced,
-                         datatype_prefs);
+                         encryption_bootstrap_token)) {
+    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 +503,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 +519,107 @@ void CredentialCacheService::ApplyCachedCredentials(
   token_service->StartFetchingTokens();
 }
 
+void CredentialCacheService::InitiateSignOut() {
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  service->DisableForUser();
+}
+
+bool CredentialCacheService::HaveSyncPrefsChanged(
+    bool keep_everything_synced,
+    const ModelTypeSet& preferred_types) const {

[Andrew T Wilson (Slow), 2012/08/03 23:33:24]

Part of the contract of ModelTypeSet is that you can just pass it around as a
value - no need to pass by reference. See enum_set.h in
sync/internal_api/public/base/

[Raghu Simha, 2012/08/04 00:18:02]

Done.

+  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::ShouldSignOutOfSync(
+    const std::string& google_services_username) {
+  // We must 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 noticed that the user has signed out of the alternate profile.
+  // 4) The user is not already in the process of configuring sync.
+  // 5) The alternate cache was updated more recently than the local cache.
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  return !service->signin()->GetAuthenticatedUsername().empty() &&
+         !HasUserSignedOut() &&
+         google_services_username.empty() &&
+         !service->setup_in_progress() &&
+         (GetLastUpdatedTime(alternate_store_) >
+          GetLastUpdatedTime(local_store_));
+}
+
+bool CredentialCacheService::MayReconfigureSync(
+    const std::string& google_services_username) {
+  // 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.
+  // 4) The user is not already in the process of configuring sync.
+  // 5) The alternate cache was updated more recently than the local cache.
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  return !service->signin()->GetAuthenticatedUsername().empty() &&
+         !HasUserSignedOut() &&
+         (google_services_username ==
+          service->signin()->GetAuthenticatedUsername()) &&
+         !service->setup_in_progress() &&
+         (GetLastUpdatedTime(alternate_store_) >
+          GetLastUpdatedTime(local_store_));
+}
+
+bool CredentialCacheService::ShouldSignInToSync(
+    const std::string& google_services_username,
+    const std::string& lsid,
+    const std::string& sid,
+    const std::string& encryption_bootstrap_token) {
+  // We should sign 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.
+  ProfileSyncService* service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  return service->signin()->GetAuthenticatedUsername().empty() &&
+         !HasUserSignedOut() &&
+         !google_services_username.empty() &&
+         !lsid.empty() &&
+         !sid.empty() &&
+         !encryption_bootstrap_token.empty() &&
+         !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();
+  }
+  next_read_.Reset(base::Bind(
+      &CredentialCacheService::LookForCachedCredentialsInAlternateProfile,
+      weak_factory_.GetWeakPtr()));
+  MessageLoop::current()->PostDelayedTask(
+      FROM_HERE,
+      next_read_.callback(),
+      TimeDelta::FromSeconds(kCredentialCachePollIntervalSecs));
+}
+
 }  // namespace syncer