537782 - e10s HTTP: authentication

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

Right now I believe proxy and HTTP authentication work by QI-ing the channel to get the right frob for popping up a username/password window.   We will probably want to rig up the HttpChannelParent to return the appropriate thing.

We should wait for the websockets patch (Bug 472529) to land before working on this, as it extensively refactors a lot of the HTTP auth code.

Comment 1

[Honza Bambas (:mayhemer)]

The prompt (if needed, i.e. when there are no credentials cached) is invoked here:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpChannel.cpp#3671

We get the instance in a very complicated way (bug 475053 comment 37) through QI and GI on notification callbacks. Window associated with the channel is involved as well. 

The prompt implementation itself lies in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

There were some discussions in bug 475053 comment 30 where Justin suggest some of his work in the past on refactoring the prompt system: 
"Our prompting is ridiculously complicated... I wrote up some notes at
https://wiki.mozilla.org/User:Dolske/PromptRework a ways back, and it's hard
keeping things straight!" Also interesting are comments 33 to 37 on that bug. 

I think it's time to rework prompting from scratch then to adapt what we have to e10s.

Comment 2

[Benjamin Smedberg]

The chrome process is going to be responsible for actually presenting the auth prompt. If we can associate network channels with a tab/prompting context in the chrome process (and I think we can) we can skip the round trip to content which would then just send messages back to chrome.

Comment 3

[Jason Duell]

Right.  But I haven't a clue how to get an appropriate tab/prompting context within the chrome process.  I know where to put it once I've got it (in HttpChannelParent).  Anyone know offhand?  Are there possible issues with knowing the correct location (if content processes are on different X desktops, etc.) for the prompt to pop up?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Are you mirroring loadgroups between the chrome and content process at all?  If so, then loadgroup setup would presumably just put the right context on the chrome part of the loadgroup...

Comment 5

[Benjamin Smedberg]

The current plan is not to mirror load groups (the load group exists entirely in the content process). jduell, I think we might be able to send a reference to a PIFrameEmbedding or some similar protocol when the channel is initialized, and then get the prompt context off of that.

Comment 6

[Justin Dolske [:Dolske]]

Note that the auth prompt piece of this work overlaps a bit with bug 516749, since the prompts share a common implementation (commonDialog.xul).

Comment 7

[Jason Duell]

So Bugzilla isn't smart enough to update a bug dependency if the dependency is marked as a duplicate?  Sad.

Comment 8

[Jason Duell]

I'm not convinced this bug needs to depend on either websockets or forwarding model dialogs from content/chrome.  We're not changing any of nsHttpChannel's auth code, so websocket's changes to auth shouldn't matter.  And the dialog will be called in chrome, so no cross-process forwarding.

AFAICT we just need to point HttpChannelParent at an nsIAuthPromptProvider somehow, so it can pass it off to the nsHttpChannel in nsHttpChannel::GetAuthPrompt (the nsHttpChannel's mCallbacks variable will point to the HttpChannelParent once bug 536292 lands).

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #8)
> And the dialog
> will be called in chrome, so no cross-process forwarding.

So how are we going to handle tab-modal dialogs?

Comment 10

[Jason Duell]

> how are we going to handle tab-modal dialogs?

I don't know dialogs at all, so I'm not one to say.  bz/biesi/bsmedberg all seem to think things ought to work ok, so long as we can have chrome identify which tab process a channel belongs to, and use that to get the appropriate prompt.  I still have no idea about the specifics of how best to make that happen.  

I believe that patch for bug 536292 has all the code needed to hand off the appropriate nsIAuthPromptProvider (or nsIAuthPrompt2 or nsIAuthPrompt), once we figure out how to get it.

Other notes:

Bz noted that some channels don't hand out auth prompts, and want auth to silently fail (see nsDocument::LoadGroup).  So we'll need to check the HttpChannelChild during AsyncOpen to see if the channel's callbacks/loadgroup supports providing any of the three prompt classes, and set a flag or something during SendAsyncOpen.

We have also identified at least two providers of HTTP auth prompts: docshell, and XHR.  Haven't looked to see if they differ in the type of prompt provided.

Comment 11

[Jason Duell]

The XHR auth use is here:

  http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#3185

Comment 12

[Jason Duell]

So bsmedberg believes that we may be able to get a prompt by adding an PIFrameEmbedding object to the PHttpChannel constructor (i.e. pass an IPDL handle as one of the arguments, and store it).  Chrome should be able to hook up an auth prompt from that.  But he also warned that extensions might need to get involved, and that we might want a more complicated, hierarchical model.  I'll let him provide the details.

Comment 13

[Jason Duell]

bsmedberg says he looked at the fennec prompt code, and it simply puts a prompt up in the last window used.   Biesi suggests we may be able to just use the "@mozilla.org/network/default-auth-prompt;1" contract id  (see  http://mxr.mozilla.org/mozilla-central/source/netwerk/build/nsNetCID.h#463)  to get an auth prompt.

Comment 14

[Jason Duell]

Heard on IRC...

<jduell>: biesi: so the "@mozilla.org/network/default-auth-prompt;1" contract id comments says "it is almost always wrong to get an auth prompt via this interface. use nsIWindowWatcher instead whenever possible."
<biesi> jduell_, well, yes
<biesi> jduell_, I meant that as an intermediate, fennec-only fix
<jduell_> ok
<jduell_> biesi: Ok. Was just wondering if the nsIWindowWatcher was easy to implement
<biesi> jduell_, what do you mean?
<jduell_> Would that be a way to get the "last window used", like bsmedberg was talking about?
<biesi> I thought bsmedberg was saying that the implementation of the authprompt was getting the last window used
<jduell_> oh, ok, then maybe the default prompt will just work
<bsmedberg> this is the fennec authprompt: http://mxr.mozilla.org/mobile-browser/source/components/PromptService.js
<bsmedberg> I'm not sure how it's hooked up currently, it doesn't look like it's using the default contractid
<biesi> bsmedberg, it's the other way, the default contract ID would use the prompt service
<biesi> passing it a null aParent
<biesi> jduell_, bsmedberg: maybe as an explanation: the default auth prompt exists so that the load of the PAC file can prompt for a user/pw; PAC loads aren't really associated with a specific window/tab

Comment 15

[Honza Bambas (:mayhemer)]

-> me

Comment 16

[Honza Bambas (:mayhemer)]

Attachment: v1

Working patch for http/proxy authentication.  Fully depends on (and based on patch of) bug 569227.

Comment 17

[Honza Bambas (:mayhemer)]

Attachment: v1 - merged

Just a merged patch.

Comment 18

[Jason Duell]

Comment on attachment 450758 [details] [diff] [review]
v1 - merged

Looks good, and works on e10s and fennec, once rebased.

I will land my rebased patch:  these comment are just FYI.

>-NS_IMPL_ISUPPORTS4(TabParent, nsITabParent, nsIWebProgress, nsISecureBrowserUI, nsISSLStatusProvider)
>+NS_IMPL_ISUPPORTS5(TabParent, nsITabParent, nsIWebProgress, nsIAuthPromptProvider, nsISecureBrowserUI, nsISSLStatusProvider)

Took out nsISSLStatusProvider, nsISecureBrowserUI.


>+// nsIAuthPromptProvider
>+NS_IMETHODIMP
>+TabParent::GetAuthPrompt(PRUint32 aPromptReason, const nsIID& iid,
>+                          void** aResult)
>+{
>+    // a priority prompt request will override a false mAllowAuth setting
>+    PRBool priorityPrompt = (aPromptReason == PROMPT_PROXY);
>+
>+    // we're either allowing auth, or it's a proxy request
>+    nsresult rv;
>+    nsCOMPtr<nsIPromptFactory> wwatch =
>+      do_GetService(NS_WINDOWWATCHER_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsCOMPtr<nsIDOMWindow> window;
>+    nsCOMPtr<nsIContent> frame = do_QueryInterface(mFrameElement);
>+    if (frame)
>+      window = do_QueryInterface(frame->GetOwnerDoc()->GetWindow());
>+
>+    // Get the an auth prompter for our window so that the parenting
>+    // of the dialogs works as it should when using tabs.
>+    return wwatch->GetPrompt(window, iid,
>+                             reinterpret_cast<void**>(aResult));
>+}

Indents need to be 2-space in this files.  I changed the vi modeline to reflect that (emacs was already 2).

>diff --git a/netwerk/protocol/http/HttpChannelParent.cpp b/netwerk/protocol/http/HttpChannelParent.cpp
>+  if (aIID.Equals(NS_GET_IID(nsIAuthPromptProvider)))
>+  {
>+    if (!mTabParent)
>+      return NS_NOINTERFACE;
>+
>+    return mTabParent->QueryInterface(aIID, result);
>+  }

Changed brace to be on same line as if, got rid of blank line before last return statement.

Comment 19

[Jason Duell]

http://hg.mozilla.org/projects/electrolysis/rev/f0474518bd43

Http Auth prompt is pretty cute on fennec.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 450758 [details] [diff] [review]
v1 - merged

s/frame/frameContent/ please.  The other sounds like an nsIFrame.  :(

There are either tabs or incorrect indentation or both in this patch.  Please fix that, and add modelines to TabParent.cpp to prevent recurrence as needed.

sr=me with that.

Comment 21

[Jason Duell]

> There are either tabs or incorrect indentation or both in this patch.  Please
fix that, and add modelines to TabParent.cpp

One step ahead of you, bz (for once!):  I already made those changes in the patch that I landed.