Client changes for disabled dasher account

chrome/browser/signin/signin_tracker.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/signin/signin_tracker.h"
 
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/signin/token_service.h"
@@ skipping 153 matching lines @@
 
 // static
 bool SigninTracker::AreServicesSignedIn(Profile* profile) {
   if (!AreServiceTokensLoaded(profile))
     return false;
   // Don't care about the sync state if sync is disabled by policy.
   if (!profile->IsSyncAccessible())
     return true;
   ProfileSyncService* service =
       ProfileSyncServiceFactory::GetForProfile(profile);
+  // Check if sync disabled for domain flag was received from server. Don't care
+  // about the sync state in this case.
+  if (service->IsManaged())

[Andrew T Wilson (Slow), 2013/05/24 15:13:10]

Rather than making every caller of Profile::IsSyncAccessible() also call
service->IsManaged(), it seems cleaner to change IsSyncAccessible() to do this:

bool Profile::IsSyncAccessible() {
  if (ProfileSyncServiceFactory::HasProfileSyncService(this))
    return !ProfileSyncServiceFactory::GetProfileSyncService(this)->IsManaged();

  // No ProfileSyncService created yet - we don't want to create one, so just
infer the accessible state
  // by looking at prefs/command line flags.
  browser_sync::SyncPrefs prefs(GetPrefs());
  return ProfileSyncService::IsSyncEnabled() && !prefs.IsManaged())
}

[pavely, 2013/05/24 22:17:11]

I tried this in my experiments. The issue is that ProfileSyncService transitions

from accessible to not-accessible state while setup is in progress. 
OneClickSigninSyncStarter calls SetSetupInProgress(true) in the beginning, it 
should call SetSetupInProgress(false) at the end, otherwise after user signs out
settings page will disable button and show that setup is in progress. If
GetProfileSyncService returns NULL for disabled user then 
OneClickSigninSyncStarter won't have PSS pointer to call 
SetSetupInProgress(false). This reminds me that I'm not handling case of user
selecting "Choose what to sync" correctly.
Another solution for this problem is to call SetSetupInProgress(false) when 
I set disabled flag in PSS, but I think it hides dependency in implementation 
that shouldn't be there.

[Andrew T Wilson (Slow), 2013/05/27 15:03:34]

Yeah, that's a funky edge case. You could deal with it by having the code that
calls SetSetupInProgress(false) call ProfileSyncServiceFactory::GetForProfile()
to grab the PSS instead of calling IsProfileAccessible() in that specific case.
It could still return null if sync is disabled via a command-line flag so we
have to check for null.

I think it's better to explicitly handle the case of sync changing its
accessibility than to make everyone manually check the disabled status
separately. I just know from hard experience that if you don't do this, people
will forget to do it in their code and we'll get breakages when people disable
sync on the domain (in the same way that sporadically we get crashes when you
run chrome with --sync-disabled because people forget to check for a null PSS).

+    return true;
   return (service->IsSyncEnabledAndLoggedIn() &&
           service->IsSyncTokenAvailable() &&
           service->GetAuthError().state() == GoogleServiceAuthError::NONE &&
           !service->HasUnrecoverableError());
 }
 
 // static
 SigninTracker::LoginState SigninTracker::GetSigninState(
     Profile* profile,
     GoogleServiceAuthError* error) {
   SigninManagerBase* signin = SigninManagerFactory::GetForProfile(profile);
   if (signin->GetAuthenticatedUsername().empty()) {
     // User is signed out, trigger a signin failure.
     if (error)
       *error = GoogleServiceAuthError(GoogleServiceAuthError::REQUEST_CANCELED);
     return WAITING_FOR_GAIA_VALIDATION;
   }
 
   // Wait until all of our services are logged in. For now this just means sync.
   // Long term, we should separate out service auth failures from the signin
   // process, but for the current UI flow we'll validate service signin status
   // also.
   ProfileSyncService* service = profile->IsSyncAccessible() ?
       ProfileSyncServiceFactory::GetForProfile(profile) : NULL;
+  if (service->IsManaged())

[Andrew T Wilson (Slow), 2013/05/24 15:13:10]

See my previous comment - if IsSyncAccessible() calls service->IsManaged() then
we don't have to explicitly call it here.

+    service = NULL;
   if (service && service->waiting_for_auth()) {
     // Still waiting for an auth token to come in so stay in the INITIALIZING
     // state (we do this to avoid triggering an early signin error in the case
     // where there's a previous auth error in the sync service that hasn't
     // been cleared yet).
     return SERVICES_INITIALIZING;
   }
 
   // If we haven't loaded all our service tokens yet, just exit (we'll be called
   // again when another token is loaded, or will transition to SigninFailed if
   // the loading fails).
   if (!AreServiceTokensLoaded(profile))
     return SERVICES_INITIALIZING;
   if (!AreServicesSignedIn(profile)) {
     if (error)
       *error = signin->signin_global_error()->GetLastAuthError();
     return WAITING_FOR_GAIA_VALIDATION;
   }
 
   if (!service || service->sync_initialized())
     return SIGNIN_COMPLETE;
 
   return SERVICES_INITIALIZING;
 }