Setup Sync to use OAuth token for managed users.

Setup Sync to use OAuth token for managed users.

BUG=226464
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207501

[Bernhard Bauer, 2013-06-12 14:30:06]

Please review.

Tim: Sync/GAIA constants
Joao: Cloud policy

Thanks!

[Bernhard Bauer, 2013-06-12 14:30:46]

Oh yes, note that this depends on Pavel's
https://codereview.chromium.org/15421011/.

[Joao da Silva, 2013-06-12 15:30:29]

https://codereview.chromium.org/15780020/diff/45001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/15780020/diff/45001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_policy_signin_service.cc:13: #include
"chrome/browser/managed_mode/managed_user_service.h"
#ifdef managed mode

https://codereview.chromium.org/15780020/diff/45001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_policy_signin_service.cc:232: 
The managed mode check should be here, and return early if the profile is
managed.

https://codereview.chromium.org/15780020/diff/45001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_policy_signin_service.cc:404:
!ManagedUserService::ProfileIsManaged(profile_)) {
#ifdef after moving

https://codereview.chromium.org/15780020/diff/45001/google_apis/gaia/gaia_con...
File google_apis/gaia/gaia_constants.cc (right):

https://codereview.chromium.org/15780020/diff/45001/google_apis/gaia/gaia_con...
google_apis/gaia/gaia_constants.cc:45: 
Why this newline? :-)

[Bernhard Bauer, 2013-06-12 19:01:39]

https://codereview.chromium.org/15780020/diff/45001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/15780020/diff/45001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_policy_signin_service.cc:13: #include
"chrome/browser/managed_mode/managed_user_service.h"
On 2013/06/12 15:30:29, Joao da Silva wrote:
> #ifdef managed mode

Done.

https://codereview.chromium.org/15780020/diff/45001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_policy_signin_service.cc:232: 
On 2013/06/12 15:30:29, Joao da Silva wrote:
> The managed mode check should be here, and return early if the profile is
> managed.

Done.

https://codereview.chromium.org/15780020/diff/45001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_policy_signin_service.cc:404:
!ManagedUserService::ProfileIsManaged(profile_)) {
On 2013/06/12 15:30:29, Joao da Silva wrote:
> #ifdef after moving

Done.

https://codereview.chromium.org/15780020/diff/45001/google_apis/gaia/gaia_con...
File google_apis/gaia/gaia_constants.cc (right):

https://codereview.chromium.org/15780020/diff/45001/google_apis/gaia/gaia_con...
google_apis/gaia/gaia_constants.cc:45: 
On 2013/06/12 15:30:29, Joao da Silva wrote:
> Why this newline? :-)

Excellent question! Removed.

[Joao da Silva, 2013-06-12 19:56:13]

lgtm to unblock you, but why did you move the IsManagedProfile check just a bit
earlier in the same function? It should be in the ctor, see inline.

https://codereview.chromium.org/15780020/diff/50001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/15780020/diff/50001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_policy_signin_service.cc:235: 
Really, the check should be here :-) Note the check right above, it's meant for
a similar purpose and they should be together.

[Bernhard Bauer, 2013-06-13 09:45:05]

https://codereview.chromium.org/15780020/diff/50001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/15780020/diff/50001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_policy_signin_service.cc:235: 
On 2013/06/12 19:56:13, Joao da Silva wrote:
> Really, the check should be here :-) Note the check right above, it's meant
for
> a similar purpose and they should be together.

Urr, yes, sorry. I didn't pay much attention and mixed up the return above here
with the one below.

[Bernhard Bauer, 2013-06-17 12:18:47]

Tim, can you review the Sync and GAIA constant parts?

[Bernhard Bauer, 2013-06-17 14:23:53]

https://codereview.chromium.org/15780020/diff/78001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/15780020/diff/78001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_policy_signin_service.cc:237: if
(ManagedUserService::ProfileIsManaged(profile_))
Hm, turns out this doesn't work with newly created profiles, which are only
marked managed after construction. Joao, would be be okay if I moved this back
to Observe()?

[Joao da Silva, 2013-06-17 14:26:02]

https://codereview.chromium.org/15780020/diff/78001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/15780020/diff/78001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_policy_signin_service.cc:237: if
(ManagedUserService::ProfileIsManaged(profile_))
On 2013/06/17 14:23:53, Bernhard Bauer wrote:
> Hm, turns out this doesn't work with newly created profiles, which are only
> marked managed after construction. Joao, would be be okay if I moved this back
> to Observe()?

sgtm. Also registrar_.RemoveAll() then.

[Bernhard Bauer, 2013-06-17 14:36:10]

https://codereview.chromium.org/15780020/diff/78001/chrome/browser/policy/clo...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://codereview.chromium.org/15780020/diff/78001/chrome/browser/policy/clo...
chrome/browser/policy/cloud/user_policy_signin_service.cc:237: if
(ManagedUserService::ProfileIsManaged(profile_))
On 2013/06/17 14:26:02, Joao da Silva wrote:
> On 2013/06/17 14:23:53, Bernhard Bauer wrote:
> > Hm, turns out this doesn't work with newly created profiles, which are only
> > marked managed after construction. Joao, would be be okay if I moved this
back
> > to Observe()?
> 
> sgtm. Also registrar_.RemoveAll() then.

Done.

[tim (not reviewing), 2013-06-17 18:10:21]

+Pavel is a good reviewer for this code (I'm also sheriff today so it should be
faster :p)

[pavely, 2013-06-17 22:36:30]

https://codereview.chromium.org/15780020/diff/91001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/15780020/diff/91001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:1936: 
Currently access token is passed to both sync server and xmpp server. I suspect
chromesync_playpen scope will not work with xmpp server and it will return
CREDENTIALS_REJECTED status which will cause ProfileSyncService to request
access token again. I didn't try it yet.
Could you try this scenario before committing.

If it is true then there should also be a change in
SyncManagerImpl::OnInvalidatorStateChange not to propagate CREDENTIALS_REJECTED
when running under managed user. 
Alternatively there could be change to disable notifications for MU completely
through InvalidatorFactory, but i don't have specific ideas on how to do it as
I'm not yet familiar with that code.

[Bernhard Bauer, 2013-06-18 13:44:35]

https://codereview.chromium.org/15780020/diff/91001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/15780020/diff/91001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:1936: 
On 2013/06/17 22:36:31, pavely wrote:
> Currently access token is passed to both sync server and xmpp server. I
suspect
> chromesync_playpen scope will not work with xmpp server and it will return
> CREDENTIALS_REJECTED status which will cause ProfileSyncService to request
> access token again. I didn't try it yet.
> Could you try this scenario before committing.

Hm, true. I did notice this when testing and worked around this by running
against the Python server (which accepts any token), but obviously that won't
work for production.

> If it is true then there should also be a change in
> SyncManagerImpl::OnInvalidatorStateChange not to propagate
CREDENTIALS_REJECTED
> when running under managed user. 
> Alternatively there could be change to disable notifications for MU completely
> through InvalidatorFactory, but i don't have specific ideas on how to do it as
> I'm not yet familiar with that code.

Me neither, but that doesn't stop me ;-)

I went with the latter approach, as it seems more dependency-inject-y to me to
not create the invalidator in the first place then to find out whether the
profile is managed deep in the bowels of the SyncManagerImpl or
ServerConnectionManager (both of which can't depend on Profile, AFAICT). So,
PTAL?

[pavely, 2013-06-18 17:31:54]

https://codereview.chromium.org/15780020/diff/102001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/15780020/diff/102001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host.cc:1275: invalidator.Pass(),
I thought about this, but found that SyncManagerImpl dereferences invalidator in
a few places without checking for NULL.
If this change works and you can load MU account and see about:sync in
initialized state then I must be wrong and the change is fine.

[pavely, 2013-06-18 17:44:14]

https://codereview.chromium.org/15780020/diff/102001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/15780020/diff/102001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host.cc:1275: invalidator.Pass(),
On 2013/06/18 17:31:55, pavely wrote:
> I thought about this, but found that SyncManagerImpl dereferences invalidator
in
> a few places without checking for NULL.
> If this change works and you can load MU account and see about:sync in
> initialized state then I must be wrong and the change is fine.

I just chatted with Tim. Better approach would be to write empty implementation
of Invalidator interface ad pass it into SyncManager. I believe you can leave
all functions empty, only make GetInvalidatorState return INVALIDATIONS_ENABLED.

[tim (not reviewing), 2013-06-18 19:08:49]

That, or changing the SyncManager contract to permit null Invalidators, with
careful null-checking everywhere in SyncManagerImpl, and a well-known "disabled"
invalidator ID.  

Basically the issue is we don't support this "happily disable notifications"
case today.  If notifications are disabled, I think it's exclusively due to auth
or connectivity / proxy issues, but a valid Invalidator component exists. 

+Richard and Fred fyi

[Bernhard Bauer, 2013-06-18 19:11:40]

On 2013/06/18 19:08:49, timsteele wrote:
> That, or changing the SyncManager contract to permit null Invalidators, with
> careful null-checking everywhere in SyncManagerImpl, and a well-known
"disabled"
> invalidator ID.  
> 
> Basically the issue is we don't support this "happily disable notifications"
> case today.  If notifications are disabled, I think it's exclusively due to
auth
> or connectivity / proxy issues, but a valid Invalidator component exists. 

OK, in that case I think it's less intrusive to use a fake invalidator. If we
get eventually get invalidations working, we just need to replace it with the
real one at one point.

> +Richard and Fred fyi

[rlarocque, 2013-06-18 20:05:12]

In addition to my comments, I think I should mention that there is an
alternative solution, though I'm not sure it would be acceptable.

I have a patch that's almost ready which would make invalidations a stand-alone
service.  It wouldn't have to share auth tokens with sync.  It could mint its
own access token and use it to connect to the XMPP server, which would allow
other invalidations clients (Cloud print, push messaging, etc.) to work.

(link: https://codereview.chromium.org/15580002/)

https://codereview.chromium.org/15780020/diff/102001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/15780020/diff/102001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host.cc:1257: if
(options.create_invalidator) {
I think this is the wrong approach.  

You probably could get away with creating the invalidator as usual.  It would
fail to authenticate, of course, but that's fine.  Dasher accounts with Google
Talk disabled by the admin get into this state all the time.  The only downside
I can see is that we'd be sending pointless requests to Buzz.  Not great, but
probably tolerable.  

Pavel's suggestion of making a fake Invalidator is an even better solution, if
you feel like implementing and testing it.

https://codereview.chromium.org/15780020/diff/102001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host.cc:1275: invalidator.Pass(),
On 2013/06/18 17:44:14, pavely wrote:
> On 2013/06/18 17:31:55, pavely wrote:
> > I thought about this, but found that SyncManagerImpl dereferences
invalidator
> in
> > a few places without checking for NULL.
> > If this change works and you can load MU account and see about:sync in
> > initialized state then I must be wrong and the change is fine.
> 
> I just chatted with Tim. Better approach would be to write empty
implementation
> of Invalidator interface ad pass it into SyncManager. I believe you can leave
> all functions empty, only make GetInvalidatorState return
INVALIDATIONS_ENABLED.

Maybe the state should be TRANSIENT_ERROR, since the invalidator will be
effectively "broken"?

[Bernhard Bauer, 2013-06-18 20:59:22]

Updated the CL, also to wait for sync initialization before configuring sync
data types in ManagedUserService, as discussed with Pavel. PTAL?

On 2013/06/18 20:05:12, rlarocque wrote:
> In addition to my comments, I think I should mention that there is an
> alternative solution, though I'm not sure it would be acceptable.
> 
> I have a patch that's almost ready which would make invalidations a
stand-alone
> service.  It wouldn't have to share auth tokens with sync.  It could mint its
> own access token and use it to connect to the XMPP server, which would allow
> other invalidations clients (Cloud print, push messaging, etc.) to work.

That sounds promising (in particular to actually get invalidations working for
us), but for now just to unblock this I'd prefer the simple solution with the
fake invalidator.

https://codereview.chromium.org/15780020/diff/102001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/15780020/diff/102001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host.cc:1257: if
(options.create_invalidator) {
On 2013/06/18 20:05:13, rlarocque wrote:
> I think this is the wrong approach.  
> 
> You probably could get away with creating the invalidator as usual.  It would
> fail to authenticate, of course, but that's fine.  Dasher accounts with Google
> Talk disabled by the admin get into this state all the time.  The only
downside
> I can see is that we'd be sending pointless requests to Buzz.  Not great, but
> probably tolerable.  
> 
> Pavel's suggestion of making a fake Invalidator is an even better solution, if
> you feel like implementing and testing it.

Yes, that's what I ended up doing.

https://codereview.chromium.org/15780020/diff/102001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host.cc:1275: invalidator.Pass(),
On 2013/06/18 20:05:13, rlarocque wrote:
> On 2013/06/18 17:44:14, pavely wrote:
> > On 2013/06/18 17:31:55, pavely wrote:
> > > I thought about this, but found that SyncManagerImpl dereferences
> invalidator
> > in
> > > a few places without checking for NULL.
> > > If this change works and you can load MU account and see about:sync in
> > > initialized state then I must be wrong and the change is fine.
> > 
> > I just chatted with Tim. Better approach would be to write empty
> implementation
> > of Invalidator interface ad pass it into SyncManager. I believe you can
leave
> > all functions empty, only make GetInvalidatorState return
> INVALIDATIONS_ENABLED.
> 
> Maybe the state should be TRANSIENT_ERROR, since the invalidator will be
> effectively "broken"?

Done.

[pavely, 2013-06-18 21:04:55]

lgtm

[tim (not reviewing), 2013-06-18 21:10:04]

https://codereview.chromium.org/15780020/diff/118002/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/15780020/diff/118002/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host.cc:1266: invalidator.reset(new
DummyInvalidator());
Can't this conditional construction type be computed using the #define inside
the factory?

[Bernhard Bauer, 2013-06-18 21:21:29]

https://codereview.chromium.org/15780020/diff/118002/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/15780020/diff/118002/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host.cc:1266: invalidator.reset(new
DummyInvalidator());
On 2013/06/18 21:10:04, timsteele wrote:
> Can't this conditional construction type be computed using the #define inside
> the factory?

No, the #define is true when the feature itself is enabled (i.e. a user *can*
create a managed user), but we only want to use the dummy when the profile is
managed.

[tim (not reviewing), 2013-06-18 21:36:12]

On 2013/06/18 21:21:29, Bernhard Bauer wrote:
>
https://codereview.chromium.org/15780020/diff/118002/chrome/browser/sync/glue...
> File chrome/browser/sync/glue/sync_backend_host.cc (right):
> 
>
https://codereview.chromium.org/15780020/diff/118002/chrome/browser/sync/glue...
> chrome/browser/sync/glue/sync_backend_host.cc:1266: invalidator.reset(new
> DummyInvalidator());
> On 2013/06/18 21:10:04, timsteele wrote:
> > Can't this conditional construction type be computed using the #define
inside
> > the factory?
> 
> No, the #define is true when the feature itself is enabled (i.e. a user *can*
> create a managed user), but we only want to use the dummy when the profile is
> managed.

I see.  Well I was thinking we could run the same code we use here to determine
whether to create a dummy or not,
but it looks like we'd have to actually store it in a bool and pass the bool in
to the factory, which you could argue
is more convoluted than it needs to be... so ok. 

OWNER stamp for Pavel's LGTM

[commit-bot: I haz the power, 2013-06-18 22:17:07]

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

[commit-bot: I haz the power, 2013-06-18 22:49:39]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

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

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

[commit-bot: I haz the power, 2013-06-19 08:22:02]

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

[commit-bot: I haz the power, 2013-06-19 14:36:37]

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

[commit-bot: I haz the power, 2013-06-19 14:44:51]

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

[commit-bot: I haz the power, 2013-06-19 16:40:14]

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

[commit-bot: I haz the power, 2013-06-19 16:44:01]

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=mac&number...

[commit-bot: I haz the power, 2013-06-20 11:57:16]

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

[commit-bot: I haz the power, 2013-06-20 18:37:58]

Change committed as 207501

[Andrew T Wilson (Slow), 2013-06-20 20:54:43]

https://chromiumcodereview.appspot.com/15780020/diff/156001/chrome/browser/po...
File chrome/browser/policy/cloud/user_policy_signin_service.cc (right):

https://chromiumcodereview.appspot.com/15780020/diff/156001/chrome/browser/po...
chrome/browser/policy/cloud/user_policy_signin_service.cc:372: if
(ManagedUserService::ProfileIsManaged(profile_)) {
I would have liked to have seen a comment for what this code is doing (why not
put this code up in the constructor instead and avoid registering for things in
the first place)? Putting it here is fragile as someone may register as an
observer for other classes (such as OAuth2TokenService) and won't handle the
case of a managed profile.