Change X-Auto-Login implementation to use OAuth token.

Change X-Auto-Login implementation to use OAuth token.

 X-Auto-Login implementation was using client login to get an ubertoken. The problem is that SID/LSID are short lived, and that token generated this way have the same timestamp as the original SID/LSID.

 Because TokenService generate also an Oauth refresh token, change the implementation to use this token to generate ubertokens.

BUG=none
TEST=unit

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

[qsr, 2012-01-31 13:57:43]

sky: for browser/ui
 tim: for ownership on signin
 roger: you wrote the initial X-Auto-Login code.

[qsr, 2012-01-31 13:59:47]

Republishing because the first title was wrong, and the message might well have
been lost. If that's not the case sorry for the spam.

[sky, 2012-01-31 16:54:41]

https://chromiumcodereview.appspot.com/9301003/diff/4002/chrome/browser/ui/au...
File chrome/browser/ui/auto_login_info_bar_delegate.cc (right):

https://chromiumcodereview.appspot.com/9301003/diff/4002/chrome/browser/ui/au...
chrome/browser/ui/auto_login_info_bar_delegate.cc:89:
static_cast<Profile*>(navigation_controller_->GetBrowserContext()),
Profile::FromBrowserContext. That said, it's preferable to pass in the Profile.
Is there a reason not to pass in the profile?

https://chromiumcodereview.appspot.com/9301003/diff/4002/chrome/browser/ui/au...
chrome/browser/ui/auto_login_info_bar_delegate.cc:107:
MessageLoop::current()->PostTask(FROM_HERE,
Is there a reason you're not using DeleteSoon on the MessageLoop?

[Roger Tawa OOO till Jul 10th, 2012-01-31 21:38:23]

Hi Benjamin,

A few comments below.

http://r815-84cc73cbe282.chromiumcodereview.appspot.com/9301003/diff/4002/chr...
File chrome/browser/signin/ubertoken_fetcher.cc (right):

http://r815-84cc73cbe282.chromiumcodereview.appspot.com/9301003/diff/4002/chr...
chrome/browser/signin/ubertoken_fetcher.cc:18:
"https://accounts.google.com/OAuthLogin?source=chrome&issueuberauth=1";
i think this url should live in gaia_urls.h

http://r815-84cc73cbe282.chromiumcodereview.appspot.com/9301003/diff/4002/chr...
chrome/browser/signin/ubertoken_fetcher.cc:28:
UbertokenFetcher::~UbertokenFetcher() {}
closing brace on next line

http://r815-84cc73cbe282.chromiumcodereview.appspot.com/9301003/diff/4002/chr...
chrome/browser/signin/ubertoken_fetcher.cc:100: url_fetcher_->Start();
I think it would be better to add this functionality to GaiaAuthFetcher, and
using that instead of a directly creating a URLFetcher here.

http://r815-84cc73cbe282.chromiumcodereview.appspot.com/9301003/diff/4002/chr...
File chrome/browser/signin/ubertoken_fetcher.h (right):

http://r815-84cc73cbe282.chromiumcodereview.appspot.com/9301003/diff/4002/chr...
chrome/browser/signin/ubertoken_fetcher.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
should this file live in chrome/common/net/gaia/?

http://r815-84cc73cbe282.chromiumcodereview.appspot.com/9301003/diff/4002/chr...
chrome/browser/signin/ubertoken_fetcher.h:18: // Allow to retrieves an über-auth
token for the user. This class uses the
Do we allow ü in source code?

http://r815-84cc73cbe282.chromiumcodereview.appspot.com/9301003/diff/4002/chr...
chrome/browser/signin/ubertoken_fetcher.h:46: UbertokenConsumer* consumer);
put on one line

http://r815-84cc73cbe282.chromiumcodereview.appspot.com/9301003/diff/4002/chr...
File chrome/browser/ui/auto_login_info_bar_delegate.cc (right):

http://r815-84cc73cbe282.chromiumcodereview.appspot.com/9301003/diff/4002/chr...
chrome/browser/ui/auto_login_info_bar_delegate.cc:101: this));
'this' should be aligned under &.  same below.

http://r815-84cc73cbe282.chromiumcodereview.appspot.com/9301003/diff/4002/chr...
chrome/browser/ui/auto_login_info_bar_delegate.cc:174: new
AutoLoginRedirector(navigation_controller_, args_);
the token_service_ member does not look likes its used anymore.  If that is the
case, please remove it from AutoLoginInfoBarDelegate.

[qsr, 2012-02-01 14:59:18]

http://codereview.chromium.org/9301003/diff/4002/chrome/browser/signin/uberto...
File chrome/browser/signin/ubertoken_fetcher.cc (right):

http://codereview.chromium.org/9301003/diff/4002/chrome/browser/signin/uberto...
chrome/browser/signin/ubertoken_fetcher.cc:18:
"https://accounts.google.com/OAuthLogin?source=chrome&issueuberauth=1";
On 2012/01/31 21:38:23, Roger Tawa wrote:
> i think this url should live in gaia_urls.h

 Change this to construct the url from urls in GaiaUrls.

http://codereview.chromium.org/9301003/diff/4002/chrome/browser/signin/uberto...
chrome/browser/signin/ubertoken_fetcher.cc:28:
UbertokenFetcher::~UbertokenFetcher() {}
On 2012/01/31 21:38:23, Roger Tawa wrote:
> closing brace on next line

Done.

http://codereview.chromium.org/9301003/diff/4002/chrome/browser/signin/uberto...
chrome/browser/signin/ubertoken_fetcher.cc:100: url_fetcher_->Start();
 If you strongly insist, I'll do it but:
1) GaiaAuthFetcher doesn't seem the right place, because it mainly handles
ClientLogin related things. There is a little bit of OAuth related
functionality, but only in the sense of exchanging a LSO token to a OAuth2
token.
2) GaiaOAuthClient is weird to use for this case -> it takes an URL on its
constructor that will not be used at all as the url to get an uber-token is not
the same one as those used to handle tokens.

 Because Uber token are something quite special, I think that the fetcher must
live here. Now, we might want to move this class next to GaiaAuthFetcher, but
then it would have to take the Oauth2 refresh token as parameter (because it
cannot depends on TokenService anymore), which will make some logic of this
class back to the AutoLoginRedirector and that logic will have to be replicated
if anything else need an uber token.

 Those are the reasons I put all the functionality here. Now, like I said, I can
try to move things a bit, but I don't think they will really feel natural
either.

http://codereview.chromium.org/9301003/diff/4002/chrome/browser/signin/uberto...
File chrome/browser/signin/ubertoken_fetcher.h (right):

http://codereview.chromium.org/9301003/diff/4002/chrome/browser/signin/uberto...
chrome/browser/signin/ubertoken_fetcher.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/01/31 21:38:23, Roger Tawa wrote:
> should this file live in chrome/common/net/gaia/?

 This file depends on TokenService so it cannot live there. If I move it there,
then I need to pass a refresh token to it, which will make the work of all users
more complicated, because getting such a token can only be done reliably by
1) checking if one is already registered on the token service
2) if this is not the case, one must listen to the token service and asks him to
start fetching tokens.

 That makes it a lot harder to use this.

http://codereview.chromium.org/9301003/diff/4002/chrome/browser/signin/uberto...
chrome/browser/signin/ubertoken_fetcher.h:18: // Allow to retrieves an über-auth
token for the user. This class uses the
 Changed to u, just in case.

http://codereview.chromium.org/9301003/diff/4002/chrome/browser/signin/uberto...
chrome/browser/signin/ubertoken_fetcher.h:46: UbertokenConsumer* consumer);
On 2012/01/31 21:38:23, Roger Tawa wrote:
> put on one line

Done.

http://codereview.chromium.org/9301003/diff/4002/chrome/browser/ui/auto_login...
File chrome/browser/ui/auto_login_info_bar_delegate.cc (right):

http://codereview.chromium.org/9301003/diff/4002/chrome/browser/ui/auto_login...
chrome/browser/ui/auto_login_info_bar_delegate.cc:89:
static_cast<Profile*>(navigation_controller_->GetBrowserContext()),
> Profile::FromBrowserContext. That said, it's preferable to pass in the
Profile.
> Is there a reason not to pass in the profile?

 We need the navigation_controller_ anyway. I can add the profile here, but it
would be gotten exactly the same way, unless I change teh signature for the
InfoBarDelegate, but then it is pass one more level. This seems to just make
heavier signature for not a big profit...

http://codereview.chromium.org/9301003/diff/4002/chrome/browser/ui/auto_login...
chrome/browser/ui/auto_login_info_bar_delegate.cc:101: this));
 Not relevant anymore.

http://codereview.chromium.org/9301003/diff/4002/chrome/browser/ui/auto_login...
chrome/browser/ui/auto_login_info_bar_delegate.cc:107:
MessageLoop::current()->PostTask(FROM_HERE,
 The usual one -> ignorance. Done.

http://codereview.chromium.org/9301003/diff/4002/chrome/browser/ui/auto_login...
chrome/browser/ui/auto_login_info_bar_delegate.cc:174: new
AutoLoginRedirector(navigation_controller_, args_);
 Not relevant anymore after your change.

[Roger Tawa OOO till Jul 10th, 2012-02-01 15:29:54]

Hi Benjamin,

Some more comments below.  Thanks.

http://r807-c864efbf45b4.chromiumcodereview.appspot.com/9301003/diff/4002/chr...
File chrome/browser/signin/ubertoken_fetcher.cc (right):

http://r807-c864efbf45b4.chromiumcodereview.appspot.com/9301003/diff/4002/chr...
chrome/browser/signin/ubertoken_fetcher.cc:100: url_fetcher_->Start();
On 2012/02/01 14:59:18, qsr1 wrote:
>  If you strongly insist, I'll do it but:
> 1) GaiaAuthFetcher doesn't seem the right place, because it mainly handles
> ClientLogin related things. There is a little bit of OAuth related
> functionality, but only in the sense of exchanging a LSO token to a OAuth2
> token.
> 2) GaiaOAuthClient is weird to use for this case -> it takes an URL on its
> constructor that will not be used at all as the url to get an uber-token is
not
> the same one as those used to handle tokens.
> 
>  Because Uber token are something quite special, I think that the fetcher must
> live here. Now, we might want to move this class next to GaiaAuthFetcher, but
> then it would have to take the Oauth2 refresh token as parameter (because it
> cannot depends on TokenService anymore), which will make some logic of this
> class back to the AutoLoginRedirector and that logic will have to be
replicated
> if anything else need an uber token.
> 
>  Those are the reasons I put all the functionality here. Now, like I said, I
can
> try to move things a bit, but I don't think they will really feel natural
> either.

Agree that keeping the uber token fetcher class here makes sense.

I do think it makes sense to move the fetching specific code to GaiaAuthFetcher.
 As you say, that class already has some oauth stuff in it.  Also, by moving the
URL to GaiaUrls as you already did, this makes more sense too.  There is also
logic there for setting up the fetch wrt cookie management that you have
duplicated here and I don't think that's a good idea.

The GaiaAuthFetcher class is the one that knows how to format and fetch all GAIA
related URLs.  Today these are mainly clientlogin based URLs, but as I work to
replace clientlogin with oauth, this will change.  GaiaAuthFetcher is a not a
clientlogin wrapper, its a GAIA fetcher.  The GaiaOAuthFetcher was experimental
and its functonality will likely move into GaiaAuthFetcher.

Note that I believe there are files missing from this CL.  You moved the URL to
GaiaUrls, but I don't see the code for that.

http://r807-c864efbf45b4.chromiumcodereview.appspot.com/9301003/diff/4002/chr...
File chrome/browser/signin/ubertoken_fetcher.h (right):

http://r807-c864efbf45b4.chromiumcodereview.appspot.com/9301003/diff/4002/chr...
chrome/browser/signin/ubertoken_fetcher.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/02/01 14:59:18, qsr1 wrote:
> On 2012/01/31 21:38:23, Roger Tawa wrote:
> > should this file live in chrome/common/net/gaia/?
> 
>  This file depends on TokenService so it cannot live there. If I move it
there,
> then I need to pass a refresh token to it, which will make the work of all
users
> more complicated, because getting such a token can only be done reliably by
> 1) checking if one is already registered on the token service
> 2) if this is not the case, one must listen to the token service and asks him
to
> start fetching tokens.
> 
>  That makes it a lot harder to use this.

OK, makes sense.

[qsr, 2012-02-01 18:28:09]

PTAL

http://codereview.chromium.org/9301003/diff/4002/chrome/browser/signin/uberto...
File chrome/browser/signin/ubertoken_fetcher.cc (right):

http://codereview.chromium.org/9301003/diff/4002/chrome/browser/signin/uberto...
chrome/browser/signin/ubertoken_fetcher.cc:100: url_fetcher_->Start();
On 2012/02/01 15:29:55, Roger Tawa wrote:
> On 2012/02/01 14:59:18, qsr1 wrote:
> >  If you strongly insist, I'll do it but:
> > 1) GaiaAuthFetcher doesn't seem the right place, because it mainly handles
> > ClientLogin related things. There is a little bit of OAuth related
> > functionality, but only in the sense of exchanging a LSO token to a OAuth2
> > token.
> > 2) GaiaOAuthClient is weird to use for this case -> it takes an URL on its
> > constructor that will not be used at all as the url to get an uber-token is
> not
> > the same one as those used to handle tokens.
> > 
> >  Because Uber token are something quite special, I think that the fetcher
must
> > live here. Now, we might want to move this class next to GaiaAuthFetcher,
but
> > then it would have to take the Oauth2 refresh token as parameter (because it
> > cannot depends on TokenService anymore), which will make some logic of this
> > class back to the AutoLoginRedirector and that logic will have to be
> replicated
> > if anything else need an uber token.
> > 
> >  Those are the reasons I put all the functionality here. Now, like I said, I
> can
> > try to move things a bit, but I don't think they will really feel natural
> > either.
> 
> Agree that keeping the uber token fetcher class here makes sense.
> 
> I do think it makes sense to move the fetching specific code to
GaiaAuthFetcher.
>  As you say, that class already has some oauth stuff in it.  Also, by moving
the
> URL to GaiaUrls as you already did, this makes more sense too.  There is also
> logic there for setting up the fetch wrt cookie management that you have
> duplicated here and I don't think that's a good idea.
> 
> The GaiaAuthFetcher class is the one that knows how to format and fetch all
GAIA
> related URLs.  Today these are mainly clientlogin based URLs, but as I work to
> replace clientlogin with oauth, this will change.  GaiaAuthFetcher is a not a
> clientlogin wrapper, its a GAIA fetcher.  The GaiaOAuthFetcher was
experimental
> and its functonality will likely move into GaiaAuthFetcher.
> 
> Note that I believe there are files missing from this CL.  You moved the URL
to
> GaiaUrls, but I don't see the code for that.

Done.

[Roger Tawa OOO till Jul 10th, 2012-02-01 21:47:14]

Thanks Benjamin, that looks great.  One more question below.

http://codereview.chromium.org/9301003/diff/13002/chrome/common/net/gaia/gaia...
File chrome/common/net/gaia/gaia_auth_fetcher.cc (right):

http://codereview.chromium.org/9301003/diff/13002/chrome/common/net/gaia/gaia...
chrome/common/net/gaia/gaia_auth_fetcher.cc:86: "issueuberauth=1";
Why use url request parameters instead of putting the arguments into the request
body?  Does this endpoint not support POST requests?

[qsr, 2012-02-02 09:26:55]

tim: PING?

http://codereview.chromium.org/9301003/diff/13002/chrome/common/net/gaia/gaia...
File chrome/common/net/gaia/gaia_auth_fetcher.cc (right):

http://codereview.chromium.org/9301003/diff/13002/chrome/common/net/gaia/gaia...
chrome/common/net/gaia/gaia_auth_fetcher.cc:86: "issueuberauth=1";
On 2012/02/01 21:47:14, Roger Tawa wrote:
> Why use url request parameters instead of putting the arguments into the
request
> body?  Does this endpoint not support POST requests?

 Unfortunately it doesn't. I did try with the POST requests to have something
more coherent with everything else, but it just doesn't work. The server always
answer with an error.

[Roger Tawa OOO till Jul 10th, 2012-02-02 14:27:32]

lgtm

Thanks Benjamin.

[commit-bot: I haz the power, 2012-02-02 16:36:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qsr@chromium.org/9301003/13002

[commit-bot: I haz the power, 2012-02-02 16:36:30]

Presubmit check for 9301003-13002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/ui/auto_login_info_bar_delegate.cc

Presubmit checks took 2.1s to calculate.

[qsr, 2012-02-02 17:27:34]

sky: PING?

[tim (not reviewing), 2012-02-02 18:23:37]

signin/ LGTM

[sky, 2012-02-03 17:50:23]

LGTM

[Peter Kasting, 2012-02-03 18:10:29]

There are no significant changes to usage of the infobar classes/functions so
you don't need my signoff.

[commit-bot: I haz the power, 2012-02-06 08:48:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qsr@chromium.org/9301003/13002

[commit-bot: I haz the power, 2012-02-06 12:50:26]

Try job failure for 9301003-13002 (retry) (retry) on mac_rel for step
"test_shell_tests".
It's a second try, previously, steps "base_unittests, test_shell_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-02-06 15:43:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qsr@chromium.org/9301003/13002

[commit-bot: I haz the power, 2012-02-07 01:54:45]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 120571, job name
was 9301003-13002 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).