Fix HTTP auth multi-profile handling.

Fix HTTP auth multi-profile handling.

Convert LoginPromptTest.SupplyRedundantAuths and LoginPromptTest.CancelRedundantAuths to browser_tests.

BUG=95175
TEST=Open same site in two profiles (or normal and incognito window) and try authenticating. The window(s) for the other profile should be unaffected.

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

[mattm, 2011-10-19 23:18:40]

ping.  Let me know if I should get someone else to review.

[tonyg, 2011-10-25 10:08:16]

On 2011/10/19 23:18:40, mattm wrote:
> ping.  Let me know if I should get someone else to review.

Yes, you'll need one of the owners here:
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/OWNERS?view...

[mattm, 2011-10-28 20:23:26]

tonyg: actually that file doesn't have "set noparent" in it, so anyone could
review.  I will ask someone else though.

mirandac: I guess you could just review this?

[Miranda Callahan, 2011-10-28 20:28:25]

On 2011/10/28 20:23:26, mattm wrote:
> tonyg: actually that file doesn't have "set noparent" in it, so anyone could
> review.  I will ask someone else though.
> 
> mirandac: I guess you could just review this?

Matt, it looks good to me, but I'm not sure I am qualified to review this code,
since this is the first I've looked at this class. :-/

[cbentzel, 2011-10-28 20:49:47]

Adding asanka - he's very familiar with this class.

[cbentzel, 2011-10-28 20:51:31]

Also - is this something we actually want to fix?

Let's say you have two windows open such as at session restore, and all profiles
are going through an authenticating proxy.

I would have to reenter the data twice in the login prompt for this case.

It seems annoying, but I suppose you may want each session to be treated as a
different entity.

[mattm, 2011-10-28 21:06:46]

We want to fix the case where you open a page that uses auth in two profiles.  
I can see the argument about proxies, I don't know if there is any signal to
distinguish a login prompt for a page versus for a proxy though?

On 2011/10/28 20:51:31, cbentzel wrote:
> Also - is this something we actually want to fix?
>
> Let's say you have two windows open such as at session restore, and all
profiles
> are going through an authenticating proxy.
> 
> I would have to reenter the data twice in the login prompt for this case.
> 
> It seems annoying, but I suppose you may want each session to be treated as a
> different entity.

[cbentzel, 2011-10-28 21:16:38]

On Fri, Oct 28, 2011 at 5:06 PM, <mattm@chromium.org> wrote:

>
> We want to fix the case where you open a page that uses auth in two
> profiles.
> I can see the argument about proxies, I don't know if there is any signal
> to
> distinguish a login prompt for a page versus for a proxy though?


AuthChallengeInfo has an is_proxy member.

On second thought, though, I think this is the right thing to do since the
profiles should be treated separately. The majority of authenticating
proxies out in the wild use Negotiate/NTLM so the user shouldn't see a
prompt at all. This will only be annoying for users who have multi-profile
enabled and go through a Basic/Digest authenticating proxy which should be
low enough.

In terms of code, the logic seems fine although I'm wondering if we should
just use Profile pointers to do the comparison rather than extracting the
HttpNetworkSession. Also, Asanka's been moving the system-level tests to
browser tests - if it's possible to do this one like that, please try.





>
> On 2011/10/28 20:51:31, cbentzel wrote:
>
>> Also - is this something we actually want to fix?
>>
>
>  Let's say you have two windows open such as at session restore, and all
>>
> profiles
>
>> are going through an authenticating proxy.
>>
>
>  I would have to reenter the data twice in the login prompt for this case.
>>
>
>  It seems annoying, but I suppose you may want each session to be treated
>> as a
>> different entity.
>>
>
>
>
>
http://codereview.chromium.**org/8305004/<http://codereview.chromium.org/8305...
>

[mattm, 2011-10-28 21:43:49]

On 2011/10/28 21:16:38, cbentzel wrote:
> On Fri, Oct 28, 2011 at 5:06 PM, <mailto:mattm@chromium.org> wrote:
> 
> >
> > We want to fix the case where you open a page that uses auth in two
> > profiles.
> > I can see the argument about proxies, I don't know if there is any signal
> > to
> > distinguish a login prompt for a page versus for a proxy though?
> 
> 
> AuthChallengeInfo has an is_proxy member.
> 
> On second thought, though, I think this is the right thing to do since the
> profiles should be treated separately. The majority of authenticating
> proxies out in the wild use Negotiate/NTLM so the user shouldn't see a
> prompt at all. This will only be annoying for users who have multi-profile
> enabled and go through a Basic/Digest authenticating proxy which should be
> low enough.
> 
> In terms of code, the logic seems fine although I'm wondering if we should
> just use Profile pointers to do the comparison rather than extracting the
> HttpNetworkSession. 

I went with HttpNetworkSession since it's accessible here and since it owns the
HttpAuthCache it seems like a good choice.  If it's straightforward to get a
Profile pointer here and you'd prefer using that I could do that instead.  I
didn't see an obvious way to get one.

> Also, Asanka's been moving the system-level tests to
> browser tests - if it's possible to do this one like that, please try.

I think I looked at doing it as a browsertest but I don't remember why I ended
up doing a uitest instead.  I'll try to look at it again.

> 
> 
> 
> 
> >
> > On 2011/10/28 20:51:31, cbentzel wrote:
> >
> >> Also - is this something we actually want to fix?
> >>
> >
> >  Let's say you have two windows open such as at session restore, and all
> >>
> > profiles
> >
> >> are going through an authenticating proxy.
> >>
> >
> >  I would have to reenter the data twice in the login prompt for this case.
> >>
> >
> >  It seems annoying, but I suppose you may want each session to be treated
> >> as a
> >> different entity.
> >>
> >
> >
> >
> >
>
http://codereview.chromium.**org/8305004/%3Chttp://codereview.chromium.org/83...>
> >

[asanka, 2011-10-28 22:37:03]

I'm OK with the code change.  Although I'd prefer a browsertest, as Chris noted.

On 2011/10/28 21:43:49, mattm wrote:
> On 2011/10/28 21:16:38, cbentzel wrote:
> > Also, Asanka's been moving the system-level tests to
> > browser tests - if it's possible to do this one like that, please try.
> 
> I think I looked at doing it as a browsertest but I don't remember why I ended
> up doing a uitest instead.  I'll try to look at it again.

[mattm, 2011-11-23 01:29:28]

On 2011/10/28 22:37:03, asanka wrote:
> I'm OK with the code change.  Although I'd prefer a browsertest, as Chris
noted.

done.  Please take another look.

[asanka, 2011-11-23 18:03:34]

Thanks for adding the browsertests!

http://codereview.chromium.org/8305004/diff/19002/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/8305004/diff/19002/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:772: SetAuthFor(handler_1);
Why call SetAuthFor() after calling CancelAuth()?

http://codereview.chromium.org/8305004/diff/19002/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:853:
ui_test_utils::BROWSER_TEST_WAIT_FOR_TAB);
As you note, the notifications are delivered synchronously.  If the test fails,
it should be evident after the auth_supplied_waiter.Wait() call above. The
'EXPECT_EQ(0, observer_incognito.auth_supplied_count_)' below should be
sufficient to catch this.

Why not just issue a ui_test_utils::RunAllPendingInMessageLoop() here instead?

[mattm, 2011-11-23 19:33:30]

http://codereview.chromium.org/8305004/diff/19002/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/8305004/diff/19002/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:772: SetAuthFor(handler_1);
On 2011/11/23 18:03:34, asanka wrote:
> Why call SetAuthFor() after calling CancelAuth()?

oops, fixed.

http://codereview.chromium.org/8305004/diff/19002/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:853:
ui_test_utils::BROWSER_TEST_WAIT_FOR_TAB);
On 2011/11/23 18:03:34, asanka wrote:
> As you note, the notifications are delivered synchronously.  If the test
fails,
> it should be evident after the auth_supplied_waiter.Wait() call above. The
> 'EXPECT_EQ(0, observer_incognito.auth_supplied_count_)' below should be
> sufficient to catch this.

Yeah, I actually even tested that it did fail if I opened both tabs in the same
browser, but I just felt like I should try something extra to avoid any flake..

> Why not just issue a ui_test_utils::RunAllPendingInMessageLoop() here instead?

done.

[asanka, 2011-11-23 19:59:46]

LGTM. Thanks!

http://codereview.chromium.org/8305004/diff/21002/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/8305004/diff/21002/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:700: // Supply auth in tab
1.
Minor nit: The handler we get below is for whichever tab that put up the auth
dialog first.  Not necessarily tab #1.

http://codereview.chromium.org/8305004/diff/21002/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:766: // Cancel auth in tab
1.
Ditto.

[mattm, 2011-11-23 20:12:15]

http://codereview.chromium.org/8305004/diff/21002/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/8305004/diff/21002/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:700: // Supply auth in tab
1.
On 2011/11/23 19:59:46, asanka wrote:
> Minor nit: The handler we get below is for whichever tab that put up the auth
> dialog first.  Not necessarily tab #1.

Done.

http://codereview.chromium.org/8305004/diff/21002/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:766: // Cancel auth in tab
1.
On 2011/11/23 19:59:46, asanka wrote:
> Ditto.

Done.

[commit-bot: I haz the power, 2011-11-23 20:19:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/8305004/26001

[commit-bot: I haz the power, 2011-11-23 22:22:40]

Change committed as 111428