Pre- and Auto-login should not log the user out of an already signed in

Pre- and Auto-login should not log the user out of an already signed in
google account.

BUG=90510
TEST=See bug description.  Also, use the switch account feature and make sure
auto-login does not log you out of any existing sessions.

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

[Rick Campbell, 2011-08-08 21:24:51]

Looks mostly good to me.  There's one typo, and one higher-level issue that I
want to raise.

If you feel strongly about exposing the full power of the flags, I'll defer,
however I think that in a security context it's often better to lock down the
options as much as you can for things where you might instead try to keep as
general as possible in other contexts.

http://codereview.chromium.org/7600003/diff/1/chrome/common/net/gaia/gaia_aut...
File chrome/common/net/gaia/gaia_auth_fetcher.cc (right):

http://codereview.chromium.org/7600003/diff/1/chrome/common/net/gaia/gaia_aut...
chrome/common/net/gaia/gaia_auth_fetcher.cc:386: // know what sessions already
exist in the browser.  The fecther needs to be
TYPO: fetcher

http://codereview.chromium.org/7600003/diff/1/chrome/common/net/gaia/gaia_aut...
chrome/common/net/gaia/gaia_auth_fetcher.cc:394: net::LOAD_NORMAL,
Just looping back to my comment about preferring a boolean "send_cookies" rather
than the more generic flags, I note in passing the use of net::LOAD_NORMAL here.
 This highlights that what is normal for most URL fetchers is abnormal in
auth-related use, and might even warrant a comment calling this out to avoid
reader confusion.

The similar mechanism in GaiaOAuthFetcher tests send_cookies to determine
whether to call set_load_flags.  This has the nice side-effect that it never
uses net::LOAD_NORMAL for that abnormal case and highlights that the cookies are
the only reason that flags are set.

http://codereview.chromium.org/7600003/diff/1/chrome/common/net/gaia/gaia_aut...
File chrome/common/net/gaia/gaia_auth_fetcher.h (right):

http://codereview.chromium.org/7600003/diff/1/chrome/common/net/gaia/gaia_aut...
chrome/common/net/gaia/gaia_auth_fetcher.h:200: static URLFetcher*
CreateGaiaFetcherWithFlags(
In the context of authentication code, I prefer a "tighter" approach as taken in
chrome/browser/net/gaia_oauth_fetcher.{h,cc} which limits the callers options to
a boolean to allow cookie-sending, without opening up the full range of flags.

While I don't think that using flags here is tragic by any means, I think that
being more specific here means reducing the opportunity for future
confusion/mistakes.  See my note in the source file that points to one type of
potential confusion.

[Roger Tawa OOO till Jul 10th, 2011-08-09 20:50:49]

Thanks Rick, changes uploaded.  Make sense to keep this codeconsistent with
oauth fetcher.

http://codereview.chromium.org/7600003/diff/1/chrome/common/net/gaia/gaia_aut...
File chrome/common/net/gaia/gaia_auth_fetcher.cc (right):

http://codereview.chromium.org/7600003/diff/1/chrome/common/net/gaia/gaia_aut...
chrome/common/net/gaia/gaia_auth_fetcher.cc:386: // know what sessions already
exist in the browser.  The fecther needs to be
On 2011/08/08 21:24:51, Rick Campbell wrote:
> TYPO: fetcher

Done.

http://codereview.chromium.org/7600003/diff/1/chrome/common/net/gaia/gaia_aut...
chrome/common/net/gaia/gaia_auth_fetcher.cc:394: net::LOAD_NORMAL,
On 2011/08/08 21:24:51, Rick Campbell wrote:
> Just looping back to my comment about preferring a boolean "send_cookies"
rather
> than the more generic flags, I note in passing the use of net::LOAD_NORMAL
here.
>  This highlights that what is normal for most URL fetchers is abnormal in
> auth-related use, and might even warrant a comment calling this out to avoid
> reader confusion.
> 
> The similar mechanism in GaiaOAuthFetcher tests send_cookies to determine
> whether to call set_load_flags.  This has the nice side-effect that it never
> uses net::LOAD_NORMAL for that abnormal case and highlights that the cookies
are
> the only reason that flags are set.

Done.

http://codereview.chromium.org/7600003/diff/1/chrome/common/net/gaia/gaia_aut...
File chrome/common/net/gaia/gaia_auth_fetcher.h (right):

http://codereview.chromium.org/7600003/diff/1/chrome/common/net/gaia/gaia_aut...
chrome/common/net/gaia/gaia_auth_fetcher.h:200: static URLFetcher*
CreateGaiaFetcherWithFlags(
On 2011/08/08 21:24:51, Rick Campbell wrote:
> In the context of authentication code, I prefer a "tighter" approach as taken
in
> chrome/browser/net/gaia_oauth_fetcher.{h,cc} which limits the callers options
to
> a boolean to allow cookie-sending, without opening up the full range of flags.
> 
> While I don't think that using flags here is tragic by any means, I think that
> being more specific here means reducing the opportunity for future
> confusion/mistakes.  See my note in the source file that points to one type of
> potential confusion.

Done.

[Rick Campbell, 2011-08-09 21:02:52]

LGTM with one comment update.

http://codereview.chromium.org/7600003/diff/6001/chrome/common/net/gaia/gaia_...
File chrome/common/net/gaia/gaia_auth_fetcher.h (right):

http://codereview.chromium.org/7600003/diff/6001/chrome/common/net/gaia/gaia_...
chrome/common/net/gaia/gaia_auth_fetcher.h:200: // net::LOAD_DO_NOT_SEND_COOKIES
are used when creating the fetcher.
Please update the comment, probably either by appending "unless send_cookies is
true" or by just striking the last sentence.

[Roger Tawa OOO till Jul 10th, 2011-08-10 17:26:47]

Good catch, thanks.  Changes uploaded.

http://codereview.chromium.org/7600003/diff/6001/chrome/common/net/gaia/gaia_...
File chrome/common/net/gaia/gaia_auth_fetcher.h (right):

http://codereview.chromium.org/7600003/diff/6001/chrome/common/net/gaia/gaia_...
chrome/common/net/gaia/gaia_auth_fetcher.h:200: // net::LOAD_DO_NOT_SEND_COOKIES
are used when creating the fetcher.
On 2011/08/09 21:02:52, Rick Campbell wrote:
> Please update the comment, probably either by appending "unless send_cookies
is
> true" or by just striking the last sentence.

Done.

[Roger Tawa OOO till Jul 10th, 2011-08-10 19:28:15]

Hi guys,

After some more testing, I noticed that pre-login would not dump the user out of
the original account, so that part of the bug is fixed, but it also would not
login the sync account either.  Going to a gaia login page will still present
the auto-login infobar, and it works correctly.

After some discussion with the gaia team, it turns out its an issue on their end
that will be fixed next week.  So this change is correct as is, but pre-login
will not work iff there is a account already logged in.

I might just wait until next week to commit this change.

[Roger Tawa OOO till Jul 10th, 2011-08-12 15:12:15]

Hi guys,

The gaia fixed it yesterday, and I have confirmed that it works today.  So
assuming all trybots are fine, I'll commit after that.  Thanks,

[Rick Campbell, 2011-08-12 19:52:54]

On 2011/08/12 15:12:15, Roger Tawa wrote:
> Hi guys,
> 
> The gaia fixed it yesterday, and I have confirmed that it works today.  So
> assuming all trybots are fine, I'll commit after that.  Thanks,

Still LGTM :-)

[commit-bot: I haz the power, 2011-08-15 14:07:07]

Presubmit check for 7600003-20013 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/common/url_fetcher.h,content/common/url_fetcher.cc

Presubmit checks took 1.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[jam, 2011-08-15 16:08:25]

content lgtm

http://codereview.chromium.org/7600003/diff/20013/content/common/url_fetcher.h
File content/common/url_fetcher.h (right):

http://codereview.chromium.org/7600003/diff/20013/content/common/url_fetcher....
content/common/url_fetcher.h:224: virtual const GURL& original_url() const;
it's unfortunate that this whole class goes against our style guide, i.e.
virtual functions are named with unix_hacker style. this change didn't add that
though, so I won't hold it against you :) it would be great if someone fixes
this later.

[Roger Tawa OOO till Jul 10th, 2011-08-15 16:47:21]

Thanks John.

I noticed that too, but decided to stick with the file's convention for better
consistency.  I'll open a bug to keep track of making  this file adhere to the
coding standards.

[commit-bot: I haz the power, 2011-08-15 17:48:25]

Commit queue patch verification failed without an error message.
Something went wrong, probably a crash, a hickup or simply
the monkeys went out for diner.
Ping the relevant dude on a on-needed basis.

[commit-bot: I haz the power, 2011-08-15 21:34:47]

Change committed as 96840