Closed
Bug 537782
Opened 15 years ago
Closed 14 years ago
e10s HTTP: authentication
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jduell.mcbugs, Assigned: mayhemer)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jduell.mcbugs
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•15 years ago
|
||
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•15 years ago
|
||
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.
Reporter | ||
Comment 3•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
Note that the auth prompt piece of this work overlaps a bit with bug 516749, since the prompts share a common implementation (commonDialog.xul).
Reporter | ||
Comment 7•15 years ago
|
||
So Bugzilla isn't smart enough to update a bug dependency if the dependency is marked as a duplicate? Sad.
Reporter | ||
Comment 8•15 years ago
|
||
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•15 years ago
|
||
(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?
Reporter | ||
Comment 10•15 years ago
|
||
> 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.
Reporter | ||
Comment 11•15 years ago
|
||
The XHR auth use is here: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#3185
Reporter | ||
Comment 12•15 years ago
|
||
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.
Updated•15 years ago
|
Assignee: nobody → dwitte
Reporter | ||
Comment 13•15 years ago
|
||
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.
Reporter | ||
Comment 14•15 years ago
|
||
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
Assignee | ||
Comment 16•15 years ago
|
||
Working patch for http/proxy authentication. Fully depends on (and based on patch of) bug 569227.
Attachment #448716 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Comment 17•14 years ago
|
||
Just a merged patch.
Attachment #448716 -
Attachment is obsolete: true
Attachment #450758 -
Flags: review?(jduell.mcbugs)
Attachment #448716 -
Flags: review?(jduell.mcbugs)
Reporter | ||
Comment 18•14 years ago
|
||
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.
Attachment #450758 -
Flags: review?(jduell.mcbugs) → review+
Reporter | ||
Comment 19•14 years ago
|
||
http://hg.mozilla.org/projects/electrolysis/rev/f0474518bd43 Http Auth prompt is pretty cute on fennec.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 20•14 years ago
|
||
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.
Updated•14 years ago
|
Attachment #450758 -
Flags: superreview+
Reporter | ||
Comment 21•14 years ago
|
||
> 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•