Correctly deal with tokens for supervised users in ProfileOAuth2TokenService.

Correctly deal with tokens for supervised users in ProfileOAuth2TokenService.

TBR=antrim@chromium.org
BUG=279307, 312751
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233393

[Bernhard Bauer, 2013-10-29 16:46:12]

Please review.

Roger, Drew: Signin/Sync
Denis: Chrome OS
Michael: Identity API

Thanks!

[Roger Tawa OOO till Jul 10th, 2013-10-29 17:16:32]

lgtm

Thanks Bernhard!

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

chrome/browser/extensions/api/identity/identity_signin_flow.cc also observes the
PO2TS. Should it also have this check? (Maybe it's sufficient to
DCHECK(!account_id.empty()) if it can be assumed that the profile couldn't start
off signed in normally, and then transition to being managed.)

https://codereview.chromium.org/33173005/diff/230001/chrome/browser/extension...
File chrome/browser/extensions/api/identity/account_tracker.cc (right):

https://codereview.chromium.org/33173005/diff/230001/chrome/browser/extension...
chrome/browser/extensions/api/identity/account_tracker.cc:57: if
(token_service->GetPrimaryAccountId().empty())
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]

On 2013/10/29 18:25:35, Michael Courage wrote:
> chrome/browser/extensions/api/identity/identity_signin_flow.cc also observes
the
> PO2TS. Should it also have this check?

Hm... Is the IdentitySigninFlow supposed to return with success as soon as we
have *any* refresh token? That's exactly the kind of thing we want to avoid for
supervised users...

https://codereview.chromium.org/33173005/diff/230001/chrome/browser/extension...
File chrome/browser/extensions/api/identity/account_tracker.cc (right):

https://codereview.chromium.org/33173005/diff/230001/chrome/browser/extension...
chrome/browser/extensions/api/identity/account_tracker.cc:57: if
(token_service->GetPrimaryAccountId().empty())
On 2013/10/29 18:25:35, Michael Courage wrote:
> 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?

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:46]

On 2013/10/29 20:25:08, Bernhard Bauer wrote:
> On 2013/10/29 18:25:35, Michael Courage wrote:
> > chrome/browser/extensions/api/identity/identity_signin_flow.cc also observes
> the
> > PO2TS. Should it also have this check?
> 
> Hm... Is the IdentitySigninFlow supposed to return with success as soon as we
> have *any* refresh token? That's exactly the kind of thing we want to avoid
for
> supervised users...

Fair point. The IdentitySigninFlow is currently not coded to handle multiple
accounts the way AccountTracker is. In fact right now the AccountTracker is the
only part of the Identity API that handles accounts other than the primary
profile account. I will be making the rest of the code multi-account aware soon,
but in the meantime the correct thing to do in
IdentitySigninFlow::OnRefreshTokenAvailable is to ignore all tokens that aren't
for GetPrimaryAccountId().

https://codereview.chromium.org/33173005/diff/230001/chrome/browser/extension...
File chrome/browser/extensions/api/identity/account_tracker.cc (right):

https://codereview.chromium.org/33173005/diff/230001/chrome/browser/extension...
chrome/browser/extensions/api/identity/account_tracker.cc:57: if
(token_service->GetPrimaryAccountId().empty())
On 2013/10/29 20:25:08, Bernhard Bauer wrote:
> On 2013/10/29 18:25:35, Michael Courage wrote:
> > 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?
> 
> 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?

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]

On 2013/10/29 21:13:46, Michael Courage wrote:
> On 2013/10/29 20:25:08, Bernhard Bauer wrote:
> > On 2013/10/29 18:25:35, Michael Courage wrote:
> > > chrome/browser/extensions/api/identity/identity_signin_flow.cc also
observes
> > the
> > > PO2TS. Should it also have this check?
> > 
> > Hm... Is the IdentitySigninFlow supposed to return with success as soon as
we
> > have *any* refresh token? That's exactly the kind of thing we want to avoid
> for
> > supervised users...
> 
> Fair point. The IdentitySigninFlow is currently not coded to handle multiple
> accounts the way AccountTracker is. In fact right now the AccountTracker is
the
> only part of the Identity API that handles accounts other than the primary
> profile account. I will be making the rest of the code multi-account aware
soon,
> but in the meantime the correct thing to do in
> IdentitySigninFlow::OnRefreshTokenAvailable is to ignore all tokens that
aren't
> for GetPrimaryAccountId().

Done.

https://codereview.chromium.org/33173005/diff/230001/chrome/browser/extension...
File chrome/browser/extensions/api/identity/account_tracker.cc (right):

https://codereview.chromium.org/33173005/diff/230001/chrome/browser/extension...
chrome/browser/extensions/api/identity/account_tracker.cc:57: if
(token_service->GetPrimaryAccountId().empty())
On 2013/10/29 21:13:47, Michael Courage wrote:
> On 2013/10/29 20:25:08, Bernhard Bauer wrote:
> > On 2013/10/29 18:25:35, Michael Courage wrote:
> > > 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?
> > 
> > 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?
> 
> 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? 

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).

> 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.

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?

https://codereview.chromium.org/33173005/diff/370001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/33173005/diff/370001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service.cc:116: // Ignore OAuth
tokens for any account but the primary one.
Drew: I changed this. We should here also ignore tokens for any account but the
primary one, right?

[Bernhard Bauer, 2013-10-31 22:05:39]

Ping?

[Bernhard Bauer, 2013-10-31 22:06:45]

To elaborate on the Ping: Michael, would you be okay with landing this while we
think about a better solution long-term?

[Michael Courage, 2013-10-31 22:15:49]

On 2013/10/31 22:06:45, Bernhard Bauer wrote:
> To elaborate on the Ping: Michael, would you be okay with landing this while
we
> think about a better solution long-term?

I was hoping we'd at least have some consensus on what direction to go next, but
ok. Put a TODO on it (in PO2TS somewhere) and lgtm.

[Roger Tawa OOO till Jul 10th, 2013-11-01 00:38:14]

On 2013/10/31 22:15:49, Michael Courage wrote:
> On 2013/10/31 22:06:45, Bernhard Bauer wrote:
> > To elaborate on the Ping: Michael, would you be okay with landing this while
> we
> > think about a better solution long-term?
> 
> I was hoping we'd at least have some consensus on what direction to go next,
but
> ok. Put a TODO on it (in PO2TS somewhere) and lgtm.

Sorry for not responding earlier.  Michael reminded me that there was a question
for me here.

Bernhard is correct that we've been discussing what to do about different
capabilities of the tokens in O2TS.  I don't think adding methods like
is_locally_managed_profile() to O2TS is the right approach.  You should be able
to query for tokens with specific capabilities in a more generic way.  (That
might already be available today, since you can get the scopes of a token by
making an api call.)

Although we have things like managed profiles and kiosk modes that use tokens
with different capabilities, we don't mix them in the same profile.  Services
that depend on having a "signed in user" (i.e. a connected profile), like
identity api, should first check that the profile is connected before looking at
tokens in O2TS.  The check for connectedness is
SigninManager::GetAuthenticatedUsername() returning a non-empty string.  If the
profile is not connected, these services should ignore tokens in O2TS.  Bernhard
addressed this is patchset #5 for now.  Once the identity api is modified to
deal with multiple accounts, it it shouldn't look for the primary account, but
instead check for connectedness as mentioned above.

[Michael Courage, 2013-11-01 02:02:46]

On 2013/11/01 00:38:14, Roger Tawa wrote:
> Although we have things like managed profiles and kiosk modes that use tokens
> with different capabilities, we don't mix them in the same profile.  Services
> that depend on having a "signed in user" (i.e. a connected profile), like
> identity api, should first check that the profile is connected before looking
at
> tokens in O2TS.  The check for connectedness is
> SigninManager::GetAuthenticatedUsername() returning a non-empty string.  If
the
> profile is not connected, these services should ignore tokens in O2TS. 
Bernhard
> addressed this is patchset #5 for now.  Once the identity api is modified to
> deal with multiple accounts, it it shouldn't look for the primary account, but
> instead check for connectedness as mentioned above.

Thanks Roger. Is there a good test to distinguish between cases where the
profile is not connected because of a special mode vs. a profile that could be
connected but isn't at the moment? There are two reasons I ask. One is that some
Identity API calls direct the user to sign-in if the profile is not connected,
which wouldn't be appropriate for such cases. The other reason is that I may
need to expose information about the state of the profile to Chrome Apps. If the
Identity API is not going to work at all because the profile is managed, or in
some other special mode, app developers might want to provide specific messaging
to the user rather than just failing for no apparent reason.

[Roger Tawa OOO till Jul 10th, 2013-11-01 03:11:46]

On 2013/11/01 02:02:46, Michael Courage wrote:
> On 2013/11/01 00:38:14, Roger Tawa wrote:
> > Although we have things like managed profiles and kiosk modes that use
tokens
> > with different capabilities, we don't mix them in the same profile. 
Services
> > that depend on having a "signed in user" (i.e. a connected profile), like
> > identity api, should first check that the profile is connected before
looking
> at
> > tokens in O2TS.  The check for connectedness is
> > SigninManager::GetAuthenticatedUsername() returning a non-empty string.  If
> the
> > profile is not connected, these services should ignore tokens in O2TS. 
> Bernhard
> > addressed this is patchset #5 for now.  Once the identity api is modified to
> > deal with multiple accounts, it it shouldn't look for the primary account,
but
> > instead check for connectedness as mentioned above.
> 
> Thanks Roger. Is there a good test to distinguish between cases where the
> profile is not connected because of a special mode vs. a profile that could be
> connected but isn't at the moment? There are two reasons I ask. One is that
some
> Identity API calls direct the user to sign-in if the profile is not connected,
> which wouldn't be appropriate for such cases. The other reason is that I may
> need to expose information about the state of the profile to Chrome Apps. If
the
> Identity API is not going to work at all because the profile is managed, or in
> some other special mode, app developers might want to provide specific
messaging
> to the user rather than just failing for no apparent reason.

Good point.  I'm not aware of any generic or standard way to determine if a
profile is in one given mode or another.  You can call methods on Profile to
determine things like normal profile, incognito, guest mode, supervised user,
and so on.  But I can't say I know all possible modes of a profile.

[Bernhard Bauer, 2013-11-04 14:48:50]

On 2013/11/01 03:11:46, Roger Tawa wrote:
> On 2013/11/01 02:02:46, Michael Courage wrote:
> > On 2013/11/01 00:38:14, Roger Tawa wrote:
> > > Although we have things like managed profiles and kiosk modes that use
> tokens
> > > with different capabilities, we don't mix them in the same profile. 
> Services
> > > that depend on having a "signed in user" (i.e. a connected profile), like
> > > identity api, should first check that the profile is connected before
> looking
> > at
> > > tokens in O2TS.  The check for connectedness is
> > > SigninManager::GetAuthenticatedUsername() returning a non-empty string. 
If
> > the
> > > profile is not connected, these services should ignore tokens in O2TS. 
> > Bernhard
> > > addressed this is patchset #5 for now.  Once the identity api is modified
to
> > > deal with multiple accounts, it it shouldn't look for the primary account,
> but
> > > instead check for connectedness as mentioned above.
> > 
> > Thanks Roger. Is there a good test to distinguish between cases where the
> > profile is not connected because of a special mode vs. a profile that could
be
> > connected but isn't at the moment? There are two reasons I ask. One is that
> some
> > Identity API calls direct the user to sign-in if the profile is not
connected,
> > which wouldn't be appropriate for such cases. The other reason is that I may
> > need to expose information about the state of the profile to Chrome Apps. If
> the
> > Identity API is not going to work at all because the profile is managed, or
in
> > some other special mode, app developers might want to provide specific
> messaging
> > to the user rather than just failing for no apparent reason.
> 
> Good point.  I'm not aware of any generic or standard way to determine if a
> profile is in one given mode or another.  You can call methods on Profile to
> determine things like normal profile, incognito, guest mode, supervised user,
> and so on.  But I can't say I know all possible modes of a profile.

Yeah, if you know about supervised users etc., you can directly ask the profile.
Maybe it would make sense to expose some method (on PO2TS?) that says whether
the profile is "special" for purposes of tokens (i.e. supervised, or kiosk mode,
without exposing that fact)?

Also, Drew, can I get an OWNERS review for policy and Sync?

[Bernhard Bauer, 2013-11-05 13:19:39]

Drew, ping?

[Andrew T Wilson (Slow), 2013-11-05 14:52:22]

lgtm with some nits/questions.

https://codereview.chromium.org/33173005/diff/570001/chrome/browser/managed_m...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/33173005/diff/570001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_service.cc:566:
token_service->LoadCredentials();
Is this still required (I thought PO2TS already self-initializes)? Or did that
CL not land yet?

https://codereview.chromium.org/33173005/diff/570001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/33173005/diff/570001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service.cc:117: if (username !=
account_id)
I think this should be:

if (account_id != oauth2_token_service_->GetPrimaryAccountId())
  return

...rather than assuming that account IDs are email addresses (Roger has been
threatening to break that assumption).

https://codereview.chromium.org/33173005/diff/570001/chrome/browser/signin/pr...
File chrome/browser/signin/profile_oauth2_token_service.h (right):

https://codereview.chromium.org/33173005/diff/570001/chrome/browser/signin/pr...
chrome/browser/signin/profile_oauth2_token_service.h:57: // Usually it's not
necessary to directly call this method.
Are we moving this back down to private: once PO2TS self-loads in Initialize()?

https://codereview.chromium.org/33173005/diff/570001/chrome/browser/signin/pr...
chrome/browser/signin/profile_oauth2_token_service.h:155: std::string
GetAccountIdForMigratingRefreshToken();
Can you document this? It's not clear what this is.

[Bernhard Bauer, 2013-11-05 16:26:50]

https://codereview.chromium.org/33173005/diff/570001/chrome/browser/managed_m...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/33173005/diff/570001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_service.cc:566:
token_service->LoadCredentials();
On 2013/11/05 14:52:23, Andrew T Wilson wrote:
> Is this still required (I thought PO2TS already self-initializes)? Or did that
> CL not land yet?

I hope it doesn't self-initialize yet, otherwise we'd lose all supervised user
tokens!

https://codereview.chromium.org/33173005/diff/570001/chrome/browser/policy/cl...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/33173005/diff/570001/chrome/browser/policy/cl...
chrome/browser/policy/cloud/user_policy_signin_service.cc:117: if (username !=
account_id)
On 2013/11/05 14:52:23, Andrew T Wilson wrote:
> I think this should be:
> 
> if (account_id != oauth2_token_service_->GetPrimaryAccountId())
>   return
> 
> ...rather than assuming that account IDs are email addresses (Roger has been
> threatening to break that assumption).

Done.

https://codereview.chromium.org/33173005/diff/570001/chrome/browser/signin/pr...
File chrome/browser/signin/profile_oauth2_token_service.h (right):

https://codereview.chromium.org/33173005/diff/570001/chrome/browser/signin/pr...
chrome/browser/signin/profile_oauth2_token_service.h:57: // Usually it's not
necessary to directly call this method.
On 2013/11/05 14:52:23, Andrew T Wilson wrote:
> Are we moving this back down to private: once PO2TS self-loads in
Initialize()?

Yes, that was my plan. I added a TODO to that effect.

https://codereview.chromium.org/33173005/diff/570001/chrome/browser/signin/pr...
chrome/browser/signin/profile_oauth2_token_service.h:155: std::string
GetAccountIdForMigratingRefreshToken();
On 2013/11/05 14:52:23, Andrew T Wilson wrote:
> Can you document this? It's not clear what this is.

Done.

[Bernhard Bauer, 2013-11-05 16:27:47]

Denis: ChromeOS TBR.

[commit-bot: I haz the power, 2013-11-05 16:28:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/33173005/700001

[commit-bot: I haz the power, 2013-11-05 18:08:06]

Retried try job too often on linux_rel for step(s) browser_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-06 14:16:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/33173005/910001

[commit-bot: I haz the power, 2013-11-06 14:20:41]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-06 14:49:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/33173005/910001

[commit-bot: I haz the power, 2013-11-06 18:40:14]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-06 20:43:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/33173005/910001

[commit-bot: I haz the power, 2013-11-06 22:18:53]

Change committed as 233393