Send a notification when the signed-in user is about to sign out.

Send a notification when the signed-in user is about to sign out.

This notification gives features that rely on authentication the chance to do some best-effort clean up before the token is cleared. This change will be used in chrome-to-mobile; upon the reception of this notification, chrome-to-mobile receiving service will send request to fetch oauth2 access token using the oauth2 refresh token maintained by TokenService to do server side clean up before the oauth2 refresh token is deleted when the signed-in user signs out.

BUG=NONE


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168275

[Chen Yu, 2012-11-07 21:11:48]

Starting with stuartmorgan for initial look. Thanks!

[stuartmorgan, 2012-11-08 12:16:33]

No need to run new code by me, just upstreaming of existing code.

One comment though: please add more information to the CL description explaining
specifically what this code will be used for in the changes that will be
following after this CL; CLs that add new code without callers/observers need
clear, specific motivation.

[Chen Yu, 2012-11-08 12:55:50]

Thanks! cl description updated.

[Andrew T Wilson (Slow), 2012-11-08 13:43:54]

https://codereview.chromium.org/11312124/diff/2001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/11312124/diff/2001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:367: content::Details<const
GoogleServiceSignoutDetails>(&details));
I'm not a huge fan of having this extra notification.

Instead, can we just move the calls to ResetCredentialsInMemory() and
EraseTokensFromDB() below until *after* GOOGLE_SIGNED_OUT is called?

Also, what if we change TokenService to invalidate its oauth login token when
the user logs out (as I think has been discussed) - will that impact services
that now rely on access tokens living past the logout process?

Finally, if that service has a cached access token, won't it do something like
this:

1) Get GOOGLE_SIGNED_OUT
2) Try using the access token (async network call)
3) TokenService blows away tokens
4) Server returns "token expired" error
5) Service tries to mint a new access token, but TokenService no longer has a
login token due to step #3, so it can't.

This attempt to hit the server after logout seems destined to be pretty
unreliable - so much so that I am unconvinced we should be doing it.

I don't see a downside to clearing the tokens post-notification, though, even
though I don't think your intended use is valid.

[Chen Yu, 2012-11-08 15:31:01]

Yes, I agreed it will be problematic if we change TokenService to invalidate its
oauth login token when the user logs out. And in this case, it seems the only
solution is to wait for the services that need authentication to clean up to
finish before oauth token is invalidated. ?

On 2012/11/08 13:43:54, Andrew T Wilson wrote:
>
https://codereview.chromium.org/11312124/diff/2001/chrome/browser/signin/sign...
> File chrome/browser/signin/signin_manager.cc (right):
> 
>
https://codereview.chromium.org/11312124/diff/2001/chrome/browser/signin/sign...
> chrome/browser/signin/signin_manager.cc:367: content::Details<const
> GoogleServiceSignoutDetails>(&details));
> I'm not a huge fan of having this extra notification.
> 
> Instead, can we just move the calls to ResetCredentialsInMemory() and
> EraseTokensFromDB() below until *after* GOOGLE_SIGNED_OUT is called?
> 
> Also, what if we change TokenService to invalidate its oauth login token when
> the user logs out (as I think has been discussed) - will that impact services
> that now rely on access tokens living past the logout process?
> 
> Finally, if that service has a cached access token, won't it do something like
> this:
> 
> 1) Get GOOGLE_SIGNED_OUT
> 2) Try using the access token (async network call)
> 3) TokenService blows away tokens
> 4) Server returns "token expired" error
> 5) Service tries to mint a new access token, but TokenService no longer has a
> login token due to step #3, so it can't.
> 
> This attempt to hit the server after logout seems destined to be pretty
> unreliable - so much so that I am unconvinced we should be doing it.
> 
> I don't see a downside to clearing the tokens post-notification, though, even
> though I don't think your intended use is valid.

[qsr, 2012-11-08 15:32:57]

I really don't think waiting when signing out is a good idea, especially when
waiting mean waiting for a network connection.

[Andrew T Wilson (Slow), 2012-11-08 15:34:51]

On 2012/11/08 15:32:57, qsr wrote:
> I really don't think waiting when signing out is a good idea, especially when
> waiting mean waiting for a network connection.

Yeah, I keep coming back around to the conclusion that "trying to take actions
at logout time is a bad idea".

[Chen Yu, 2012-11-08 15:35:36]

On 2012/11/08 15:32:57, qsr wrote:
> I really don't think waiting when signing out is a good idea, especially when
> waiting mean waiting for a network connection.

Sorry if this question is silly: we do need network connection to invalidate
auth token, right?

[Andrew T Wilson (Slow), 2012-11-08 15:50:19]

On 2012/11/08 15:35:36, Chen Yu wrote:
> Sorry if this question is silly: we do need network connection to invalidate
> auth token, right?
That's not silly at all :) Yes, it seems like we'd need a network connection. I
don't know how munjal was planning to do this (http://crbug.com/152536) - it's
possible he was just planning to do a best-effort attempt (make a network
request to invalidate the token, and if it fails, don't worry about it).

It's possible that this kind of "best-effort" server call is sufficient for
chrome2mobile - my point is that somehow you'd need to coordinate this use with
any upcoming change to invalidate the token, because if the token invalidation
happens before your request is processed then your request will always fail.

[Chen Yu, 2012-11-08 16:03:28]

On 2012/11/08 15:50:19, Andrew T Wilson wrote:
> On 2012/11/08 15:35:36, Chen Yu wrote:
> > Sorry if this question is silly: we do need network connection to invalidate
> > auth token, right?
> That's not silly at all :) Yes, it seems like we'd need a network connection.
I
> don't know how munjal was planning to do this (http://crbug.com/152536) - it's
> possible he was just planning to do a best-effort attempt (make a network
> request to invalidate the token, and if it fails, don't worry about it).
> 
> It's possible that this kind of "best-effort" server call is sufficient for
> chrome2mobile - my point is that somehow you'd need to coordinate this use
with
> any upcoming change to invalidate the token, because if the token invalidation
> happens before your request is processed then your request will always fail.

Thank you very much for the information! 

If there is no concern on clearing the tokens post-notifications, how about we
moving on with this change (and the one on OAuthTokenService), while thinking of
a better solution? My feeling is best-effort should be sufficient for
chrome-to-mobile. (And when we change to invalidate token, can we do it with one
second delay?)

[Roger Tawa OOO till Jul 10th, 2012-11-08 16:04:28]

lgtm

[Andrew T Wilson (Slow), 2012-11-09 07:33:42]

LGTM.

I would recommend that you update http://crbug.com/152536 to note what
chrome2mobile is doing with the tokens after logout, with your suggested
workaround (invalidate the token after a time delay) so Munjal is aware of your
requirement.

[commit-bot: I haz the power, 2012-11-13 14:39:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chenyu@chromium.org/11312124/8001

[commit-bot: I haz the power, 2012-11-13 17:42:23]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-11-14 06:54:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chenyu@chromium.org/11312124/8001

[commit-bot: I haz the power, 2012-11-16 20:19:15]

Change committed as 168275