Correctly deal with tokens for supervised users in ProfileOAuth2TokenService.

chrome/browser/extensions/api/identity/account_tracker.cc

Index: chrome/browser/extensions/api/identity/account_tracker.cc
diff --git a/chrome/browser/extensions/api/identity/account_tracker.cc b/chrome/browser/extensions/api/identity/account_tracker.cc
index ea09479bd4193855630b4b7a848488f7af900334..f046ff5f4e004593c1eae9a69ef196f08273edca 100644
--- a/chrome/browser/extensions/api/identity/account_tracker.cc
+++ b/chrome/browser/extensions/api/identity/account_tracker.cc
@@ -51,6 +51,12 @@ void AccountTracker::RemoveObserver(Observer* observer) {
 }
 
 void AccountTracker::OnRefreshTokenAvailable(const std::string& account_id) {
+  // Ignore refresh tokens if there is no primary account ID at all.
+  ProfileOAuth2TokenService* token_service =
+      ProfileOAuth2TokenServiceFactory::GetForProfile(profile_);
+  if (token_service->GetPrimaryAccountId().empty())

[Michael Courage, 2013/10/29 18:25:35]

This code doesn't give the reader much hope of understanding the circumstances
where the account_id would be an empty string. In fact, I don't quite get it,
and I have the advantage that I'm looking at the relevant CL + bugs.

Maybe there could be some semantically meaningful query to the PO2TS for this
case, like:

if (token_service->is_locally_managed_profile())
  return;

It seems odd to me that PO2TS would be notifying observers about an account they
are supposed to ignore. Is this whole thing a temporary workaround pending a
larger restructuring of the various token services?

[Bernhard Bauer, 2013/10/29 20:25:08]

Roger can explain more about the ongoing refactoring of the token services. Not
all observers are supposed to ignore refresh tokens if the primary account is
empty; in particular Sync (which can correctly deal with supervised users) needs
to use the token.

What you call a semantically useful query, I call a layering violation ;-)
Ideally, clients of the PO2TS shouldn't have to know about supervised users
directly. Maybe we could add some abstraction though (a profile that *can't* be
signed in, for example). Roger, WDYT?

[Michael Courage, 2013/10/29 21:13:47]

I suspect that this layering violation is already present, only obfuscated by
smuggling the information through a special string rather than having it
directly in the interface. I'm not sure though, since I'm not sure what the
nature of your token is. Should I be able to use it in the Identity API? 

My assumption has been that all login tokens in PO2TS would have the same
capabilities. It's ok if that's changing, but in that case I would like to have
a better way to test those capabilities than checking for magic account names.

[Bernhard Bauer, 2013/10/30 16:23:23]

No, this token should only be used by services that know about supervised users
(i.e. Sync). It's not even a login-scoped token; it's scoped down to a very
narrow scope that only allows talking to Sync (so any other service using it
would receive an error, and consequently show a message to the effect of "Sign
in again", which is not applicable for a supervised user).
Hm, that goes into a direction that Roger has been talking about. I'm also
starting to doubt whether having to explicitly ignore tokens in services that
don't know about supervised users will scale. Could we have something where
PO2TS observers can register for notifications only about certain kinds of
tokens? Roger, WDYT?

+    return;
+
   DVLOG(1) << "AVAILABLE " << account_id;
   ClearAuthError(account_id);
   UpdateSignInState(account_id, true);