revoke unused OAuth2 tokens on signout

revoke unused OAuth2 tokens on signout and re-signin

A number of users have been reporting problems with the identity API. Gaia investigations reveal that these users have overflowed a limit on the number of login tokens they can have for Chrome. Revoking tokens we don't need any longer should mitigate the problem. 

BUG=224462

[tim (not reviewing), 2013-03-29 22:41:25]

https://codereview.chromium.org/13249007/diff/1/chrome/browser/signin/signin_...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/13249007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_manager.cc:331: RevokeOAuthLoginToken();
Note that as of M26+ we won't actually go through this
StartSignin/PrepareForSignin flow. This path is only used for client-login. 
(Anecdotally https://codereview.chromium.org/12502017/ will remove it from
chrome os altogether).

OneClickSigninHelper has most of the code responsible for the new flows. A fair
chunk of that should likely be moved to SigninManager since that is now the
primary signin flow, but for now it still lives there.  Roger, do you know where
we should put this revocation code?

[Roger Tawa OOO till Jul 10th, 2013-03-31 18:16:30]

I think the placement of the calls to revoke the token are fine, but I think we
need to keep in mind this is a best effort only.  This won't handle the case of
people who just delete their profiles, re-image a computer, or just uninstall
chrome in the correct manner.  Maybe we should also put code into the delete
profile and the uninstaller to see if a profile is connected and call revoke?

Might also want to speak with Drew about this, I know he spent some time
thinking about this problem.

As for whether to write revoke code in gaia auth fetcher or possibly revive some
code from the following CL, I'm not sure.  Is the revoke method in this CL
equivalent to the revoke removed in 12755010?  Maybe Munjal can say.

https://codereview.chromium.org/12755010/

https://codereview.chromium.org/13249007/diff/1/chrome/browser/signin/signin_...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/13249007/diff/1/chrome/browser/signin/signin_...
chrome/browser/signin/signin_manager.cc:331: RevokeOAuthLoginToken();
On 2013/03/29 22:41:25, timsteele wrote:
> Note that as of M26+ we won't actually go through this
> StartSignin/PrepareForSignin flow. This path is only used for client-login. 
> (Anecdotally https://codereview.chromium.org/12502017/ will remove it from
> chrome os altogether).
> 
> OneClickSigninHelper has most of the code responsible for the new flows. A
fair
> chunk of that should likely be moved to SigninManager since that is now the
> primary signin flow, but for now it still lives there.  Roger, do you know
where
> we should put this revocation code?

All sign in flows go through PrepareForSignin(), so putting code here will run
in M26+.

[Michael Courage, 2013-04-01 22:33:29]

On 2013/03/31 18:16:30, Roger Tawa wrote:
> I think the placement of the calls to revoke the token are fine, but I think
we
> need to keep in mind this is a best effort only.  This won't handle the case
of
> people who just delete their profiles, re-image a computer, or just uninstall
> chrome in the correct manner.  Maybe we should also put code into the delete
> profile and the uninstaller to see if a profile is connected and call revoke?

I'd be happy to have uninstall and profile deletion covered too, but I didn't
know enough about how those paths work to make a simple change. Is there a
straightforward place to insert a revocation call into those paths?

> Might also want to speak with Drew about this, I know he spent some time
> thinking about this problem.
> 
> As for whether to write revoke code in gaia auth fetcher or possibly revive
some
> code from the following CL, I'm not sure.  Is the revoke method in this CL
> equivalent to the revoke removed in 12755010?  Maybe Munjal can say.
> 
> https://codereview.chromium.org/12755010/

LSO says the RevokeToken entry point used in the deleted code is for revoking
all tokens associated with a client, rather than revoking a specific token.

[Roger Tawa OOO till Jul 10th, 2013-04-02 14:14:11]

On 2013/04/01 22:33:29, Michael Courage wrote:
> On 2013/03/31 18:16:30, Roger Tawa wrote:
> > I think the placement of the calls to revoke the token are fine, but I think
> we
> > need to keep in mind this is a best effort only.  This won't handle the case
> of
> > people who just delete their profiles, re-image a computer, or just
uninstall
> > chrome in the correct manner.  Maybe we should also put code into the delete
> > profile and the uninstaller to see if a profile is connected and call
revoke?
> 
> I'd be happy to have uninstall and profile deletion covered too, but I didn't
> know enough about how those paths work to make a simple change. Is there a
> straightforward place to insert a revocation call into those paths?
> 
> > Might also want to speak with Drew about this, I know he spent some time
> > thinking about this problem.
> > 
> > As for whether to write revoke code in gaia auth fetcher or possibly revive
> some
> > code from the following CL, I'm not sure.  Is the revoke method in this CL
> > equivalent to the revoke removed in 12755010?  Maybe Munjal can say.
> > 
> > https://codereview.chromium.org/12755010/
> 
> LSO says the RevokeToken entry point used in the deleted code is for revoking
> all tokens associated with a client, rather than revoking a specific token.

Hi Michael,

I'm OK with doing the extra revoking for profile deletion/uninstall in separate
CLs, but then we should have some bugs open to cover those cases (if not already
present).

The LSO folks also said they prefer that all tokens associated with a client be
revoked rather than only specific tokens.  Chrome also does have a login-scoped
token.  Does this mean the deleted code in CL 12755010 is what should be called?

[Michael Courage, 2013-04-02 19:49:48]

On 2013/04/02 14:14:11, Roger Tawa wrote:
> I'm OK with doing the extra revoking for profile deletion/uninstall in
separate
> CLs, but then we should have some bugs open to cover those cases (if not
already
> present).

OK, I'll file separate bugs for those.

> The LSO folks also said they prefer that all tokens associated with a client
be
> revoked rather than only specific tokens.  Chrome also does have a
login-scoped
> token.  Does this mean the deleted code in CL 12755010 is what should be
called?

After some discussion, LSO's recommendation is to revoke specific tokens.

I don't know what should be done with Chrome's non-OAuth2 tokens. Is it the
login-scoped token the one from from /OAuthLogin or something else?

[Roger Tawa OOO till Jul 10th, 2013-04-02 19:57:10]

On 2013/04/02 19:49:48, Michael Courage wrote:
> On 2013/04/02 14:14:11, Roger Tawa wrote:
> > I'm OK with doing the extra revoking for profile deletion/uninstall in
> separate
> > CLs, but then we should have some bugs open to cover those cases (if not
> already
> > present).
> 
> OK, I'll file separate bugs for those.
> 
> > The LSO folks also said they prefer that all tokens associated with a client
> be
> > revoked rather than only specific tokens.  Chrome also does have a
> login-scoped
> > token.  Does this mean the deleted code in CL 12755010 is what should be
> called?
> 
> After some discussion, LSO's recommendation is to revoke specific tokens.
> 
> I don't know what should be done with Chrome's non-OAuth2 tokens. Is it the
> login-scoped token the one from from /OAuthLogin or something else?

lgtm

OK sounds good for specific tokens.  I don't know either for the non-oauth2
tokens.  Do they cause the same type of problem?

[Michael Courage, 2013-04-04 00:08:50]

On 2013/04/02 19:57:10, Roger Tawa wrote:
> OK sounds good for specific tokens.  I don't know either for the non-oauth2
> tokens.  Do they cause the same type of problem?

Seems like those tokens will automatically do the right thing according to
Breno. (Identity API tokens are not linked the same way to the refresh token,
but they will be handled separately.)