Ensure Sync access token is revoked when user is signed out.

Ensure Sync access token is revoked when user is signed out.

This is needed to avoid the same access token to be re-used after the
next sign in, even if the account is different.

BUG=563416

[bzanotti, 2015-12-02 15:27:06]

bzanotti@chromium.org changed reviewers:
+ zea@chromium.org

[bzanotti, 2015-12-02 15:27:06]

Please take a look.

[Nicolas Zea, 2015-12-02 18:57:31]

The CQ bit was checked by zea@chromium.org to run a CQ dry run

[Nicolas Zea, 2015-12-02 18:57:35]

https://codereview.chromium.org/1496573002/diff/1/components/browser_sync/bro...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/1496573002/diff/1/components/browser_sync/bro...
components/browser_sync/browser/profile_sync_service.cc:769:
!signin_->GetOriginal()->IsAuthenticated()) {
Is this the right place to fix this? Why isn't the first condition sufficient?
It seems like the refresh token should be revoked when you switch accounts,
right?

[commit-bot: I haz the power, 2015-12-02 18:58:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1496573002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1496573002/1

[commit-bot: I haz the power, 2015-12-02 19:01:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-02 19:01:27]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bzanotti, 2015-12-02 22:21:46]

https://codereview.chromium.org/1496573002/diff/1/components/browser_sync/bro...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/1496573002/diff/1/components/browser_sync/bro...
components/browser_sync/browser/profile_sync_service.cc:769:
!signin_->GetOriginal()->IsAuthenticated()) {
On 2015/12/02 18:57:35, Nicolas Zea wrote:
> Is this the right place to fix this? Why isn't the first condition sufficient?
> It seems like the refresh token should be revoked when you switch accounts,
> right?

Please note that I'm not super familiar with sync code, I was just trying to
reintroduce the same logic that was removed in http://crrev.com/1354053004
without reintroducing the bug.

Maybe this is not the best place to fix this, but note that due to how signout
works, this notification is received before the OnGoogleSignOut notification
(but SigninManager will already return false to IsAuthenticated).

Maybe the access_token should simply be cleared on signout (I have no idea why
this isn't the case, but I can observe this on iOS). If you have more
information/context on this, I'm very interested.

[Nicolas Zea, 2015-12-02 22:27:54]

https://codereview.chromium.org/1496573002/diff/1/components/browser_sync/bro...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/1496573002/diff/1/components/browser_sync/bro...
components/browser_sync/browser/profile_sync_service.cc:769:
!signin_->GetOriginal()->IsAuthenticated()) {
On 2015/12/02 22:21:46, bzanotti wrote:
> On 2015/12/02 18:57:35, Nicolas Zea wrote:
> > Is this the right place to fix this? Why isn't the first condition
sufficient?
> > It seems like the refresh token should be revoked when you switch accounts,
> > right?
> 
> Please note that I'm not super familiar with sync code, I was just trying to
> reintroduce the same logic that was removed in http://crrev.com/1354053004
> without reintroducing the bug.
> 
> Maybe this is not the best place to fix this, but note that due to how signout
> works, this notification is received before the OnGoogleSignOut notification
> (but SigninManager will already return false to IsAuthenticated).
> 
> Maybe the access_token should simply be cleared on signout (I have no idea why
> this isn't the case, but I can observe this on iOS). If you have more
> information/context on this, I'm very interested.

Yeah, I think that OnGoogleSignedOut should also be clearing the access_token_
(and calling the UpdateAuthErrorState). That seems a more natural fit for this
logic, and I agree it seems wrong that wasn't happening before.

[Nicolas Zea, 2015-12-03 03:39:21]

On 2015/12/02 22:27:54, Nicolas Zea wrote:
>
https://codereview.chromium.org/1496573002/diff/1/components/browser_sync/bro...
> File components/browser_sync/browser/profile_sync_service.cc (right):
> 
>
https://codereview.chromium.org/1496573002/diff/1/components/browser_sync/bro...
> components/browser_sync/browser/profile_sync_service.cc:769:
> !signin_->GetOriginal()->IsAuthenticated()) {
> On 2015/12/02 22:21:46, bzanotti wrote:
> > On 2015/12/02 18:57:35, Nicolas Zea wrote:
> > > Is this the right place to fix this? Why isn't the first condition
> sufficient?
> > > It seems like the refresh token should be revoked when you switch
accounts,
> > > right?
> > 
> > Please note that I'm not super familiar with sync code, I was just trying to
> > reintroduce the same logic that was removed in http://crrev.com/1354053004
> > without reintroducing the bug.
> > 
> > Maybe this is not the best place to fix this, but note that due to how
signout
> > works, this notification is received before the OnGoogleSignOut notification
> > (but SigninManager will already return false to IsAuthenticated).
> > 
> > Maybe the access_token should simply be cleared on signout (I have no idea
why
> > this isn't the case, but I can observe this on iOS). If you have more
> > information/context on this, I'm very interested.
> 
> Yeah, I think that OnGoogleSignedOut should also be clearing the access_token_
> (and calling the UpdateAuthErrorState). That seems a more natural fit for this
> logic, and I agree it seems wrong that wasn't happening before.

FYI, https://codereview.chromium.org/1498663002/ is based off this patch.

[bzanotti, 2015-12-03 09:56:54]

Description was changed from

==========
Ensure Sync access token is revoked when user is signed out.

This is needed to avoid the same access token to be re-used after the
next sign in, even if the account is different.

BUG=563416
==========

to

==========
Ensure Sync access token is revoked when user is signed out.

This is needed to avoid the same access token to be re-used after the
next sign in, even if the account is different.

BUG=563416
==========