Client changes for disabled dasher account

chrome/common/pref_names.cc

Index: chrome/common/pref_names.cc
diff --git a/chrome/common/pref_names.cc b/chrome/common/pref_names.cc
index 58a075b8f682b742a13e2109583cca86f9236775..671d27e095b6216673ca58d1d817708f1dc559a1 100644
--- a/chrome/common/pref_names.cc
+++ b/chrome/common/pref_names.cc
@@ -1798,6 +1798,8 @@ const char kSyncTypedUrls[] = "sync.typed_urls";
 // sync.
 const char kSyncManaged[] = "sync.managed";
 
+const char kSyncDisabledByAdmin[] = "sync.disabled_by_admin";

[tim (not reviewing), 2013/05/13 04:18:58]

Can eventually replace sync.managed with this?  If so, can you add a TODO here
and reference a bug#?

[Andrew T Wilson (Slow), 2013/05/13 11:51:13]

I don't think this is the right way to approach this - we should not be storing
permanent client-side state that the user has disabled sync on the server.

This is especially true because on some platforms (ChromeOS, enterprise desktop)
you can't sign out. A better solution is to keep some flag in memory in PSS that
says that sync was disabled by the server, which is cleared on restart of
Chrome.

I thought that was the approach we were going with from our previous discussions
- is a permanent pref required for some reason?

[pavely, 2013/05/13 22:37:09]

Flag is stored in prefs to display settings page correctly. Currently values
that affect states of buttons on settings page are kept in prefs and don't
require sync backend to be initialized and attempted to talk to server. I tried
to implement ProfileSyncService to talk to service right after initialization,
but that would mean issuing RPC every time right after browser startup. I think
we shouldn't be doing this and therefore I decided to cache this flag in prefs.
I'll work on the change where we will periodically (infrequently) initiallize
backend to check if account is still disabled.

[pavely, 2013/05/13 22:37:09]

We can replace sync.managed once controlling IsManaged flag through group policy
on windows is deprecated. I don't know if/when this is planned. Will add TODO.

[Andrew T Wilson (Slow), 2013/05/14 12:34:00]

This won't be deprecated - we still need this so enterprise admins for
non-dasher domains still have a way to disable sync.

[Andrew T Wilson (Slow), 2013/05/14 12:34:00]

This is not strictly true - browser_options_handler.cc calls
ProfileSyncService::HasUnrecoverableError() which is not backed by prefs.
Likewise, sync_ui_util::GetStatusLabels() which is also called by
BrowserOptionsHandler queries a bunch of runtime information from
ProfileSyncService (such as the current auth error) which is not backed by
prefs.

I think it's OK if the settings page does not show sync as disabled until after
the sync backend is initialized.
I don't think I can approve this change with the disabled status stored in the
pref - the correct way to do this is to have PSS check is_initialized(), not to
try to cache the data in prefs so you can access the previous value while your
service is initializing (that's not what prefs is for).

Even if you sporadically refresh the prefs value, this is still the wrong
approach.

[tim (not reviewing), 2013/05/16 18:32:48]

Maybe I'm missing something. We store a bit in prefs to tell us whether sync
setup has completed.  How are you proposing we know, when we restart Chrome,
that the reason this bit is false / sync is disabled is because of policy? 

Are you proposing we leave all the sync machinery up and running in a permanent
lame-duck state, where on restart we wait for the user to do something that will
trigger sync initialization (could take a while, especially with my deferred
init work) so that we re-learn the value of this single bit?  That seems like
considerable overkill  / complexity (vs "if enterprise disables sync, disable it
on the client"). What's the motivation for doing that?

+
 // Boolean to prevent sync from automatically starting up.  This is
 // used when sync is disabled by the user via the privacy dashboard.
 const char kSyncSuppressStart[] = "sync.suppress_start";