Closed
Bug 422808
Opened 16 years ago
Closed 16 years ago
nsXMLHTTPRequest and nsXMLDocument forwarding requests for nsIAuthPrompt is troublesome
Categories
(Core :: DOM: Core & HTML, defect, P1)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9beta5
People
(Reporter: Gavin, Assigned: Gavin)
References
Details
Attachments
(1 file, 4 obsolete files)
29.52 KB,
patch
|
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
See bug 422135 comment 6. XMLHTTPRequests that require authentication end up using a wrapped nsIAuthPrompt implementation when they could avoid the overhead (and deprecated interface) and just as easily use nsIAuthPrompt2, if only the GetInterface would properly forward requests for nsIAuthPrompt2. This will also work around the issue of the current password manager's nsIAuthPrompt implementation being buggy (which causes bug 422135).
Assignee | ||
Comment 1•16 years ago
|
||
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #309258 -
Flags: superreview?
Attachment #309258 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•16 years ago
|
Attachment #309258 -
Flags: superreview? → superreview?(cbiesinger)
Assignee | ||
Updated•16 years ago
|
Priority: -- → P1
Target Milestone: --- → mozilla1.9beta5
Assignee | ||
Comment 2•16 years ago
|
||
sicking points out in bug 422135 that we can also just remove this code, and let the HTTP channel code fallback to the loadgroup's auth prompter. This also has the advantage that we get the prompter parented to the correct window.
Attachment #309258 -
Attachment is obsolete: true
Attachment #309262 -
Flags: superreview?(cbiesinger)
Attachment #309262 -
Flags: review?(jonas)
Attachment #309258 -
Flags: superreview?(cbiesinger)
Attachment #309258 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 3•16 years ago
|
||
Do we need to worry about other channels not doing the same fallback that the HTTP channel does [1]? [1] http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp&rev=1.328&mark=3064,3068#3054
IMO the correct fix is to rip out the nsIAuthPrompt from XHR and also from the XSLT code. This will make us use the one on the docshell which really is the one we want. Not sure if there are other places like this that we want to rip out too.
Comment 5•16 years ago
|
||
Comment on attachment 309262 [details] [diff] [review] better patch other channels should do the same thing as the http channel.
Attachment #309262 -
Flags: superreview?(cbiesinger) → superreview+
Assignee | ||
Updated•16 years ago
|
Summary: nsXMLHTTPRequest::GetInterface should support nsIAuthPrompt2 → nsXMLHTTPRequest and nsXMLDocument forwarding requests for nsIAuthPrompt is troublesome
Assignee | ||
Comment 6•16 years ago
|
||
Get nsXMLDocument too. I think we can do this for nsXPInstallManager too, but I'll touch that in a separate bug.
Attachment #309262 -
Attachment is obsolete: true
Attachment #309268 -
Flags: review?(jonas)
Attachment #309262 -
Flags: review?(jonas)
Assignee | ||
Comment 7•16 years ago
|
||
Not sure that there's a good way to test this. Any suggestions?
(In reply to comment #3) > Do we need to worry about other channels not doing the same fallback that the > HTTP channel does [1]? No. The other channels should use NS_QueryAuthPrompt2(nsIChannel...) which does the right thing. FTP already does. HTTP should ideally also use that function, not sure why it doesn't. Feel free to file a followup patch to fix that too. Also, could you fix this in nsXMLDocument::GetInterface and txStylesheetSink::GetInterface too?
ugh, sorry, wasn't fast enough with my comment. Mind doing txStylesheetSink too?
Comment 10•16 years ago
|
||
HTTP can't use that function, it uses nsIAuthPromptProvider
Ah, it could use NS_QueryNotificationCallbacks though.
Comment on attachment 309268 [details] [diff] [review] XMLDocument too Unsetting review request. Feel free to rerequest if you don't want to touch the xslt code in this patch.
Attachment #309268 -
Flags: review?(jonas)
Comment on attachment 309268 [details] [diff] [review] XMLDocument too On second thought, i need to muck around with the XSLT code anyway before next beta so i'll fix XSLT then.
Attachment #309268 -
Flags: review+
Assignee | ||
Comment 14•16 years ago
|
||
Filed bug 422849 for the XSLT code.
Comment 15•16 years ago
|
||
(In reply to comment #7) > Not sure that there's a good way to test this. Any suggestions? I recently added tests for the auth prompt in toolkit/components/passwordmgr/test/test_prompt.html... You could add add another testcase in there, except instead of manually invoking the prompt you could trigger it by making an XHR to the mochitest server. You should be able to simulate requesting authentication by adding a dummy "auth.html" file, and a "auth.html^headers^" file containing: HTTP 401 Auth Requested WWW-Authenticate: basic realm="mochitest" I suppose the tricky bit is that since the server always sends those headers you can't actually successfully authenticate, but checking to see if the right dialog appeared and then canceling it should be enough.
Assignee | ||
Comment 16•16 years ago
|
||
Comment on attachment 309268 [details] [diff] [review] XMLDocument too This patch works around bug 422135 (broken XHR authentication prompting) and fixes bug 413249 (inability to save passwords entered for XHR requests). It also avoids the overhead of creating a "prompt wrapper" for XHR/XMLDocument authentication prompting, and makes sure the authentication dialog is parented to the correct window in those cases. I think the risk is pretty low - the possible side effects are that some channels relied on being able to get to an nsIAuthPrompt this way (and would thus fail to prompt for authentication for XMLDocument- or XHR-triggered loads with this patch applied), but HTTP and FTP handle this correctly, and I've briefly looked through all the code in the tree that deals with nsIAuthPrompts and haven't found any other problems.
Attachment #309268 -
Flags: approval1.9?
Assignee | ||
Comment 17•16 years ago
|
||
Dolske has been working on the tests he mentions in comment 15, which should cover the "XHR auth prompting works" aspect - once those are landed I'll add a test to ensure that the dialog parent is correct.
Flags: in-testsuite?
Comment 18•16 years ago
|
||
Microsummaries did some vaguely related stuff involving XHR and authentication, see bug 347310... Would this change have any effect on that?
Assignee | ||
Comment 19•16 years ago
|
||
(In reply to comment #18) > Microsummaries did some vaguely related stuff involving XHR and authentication, > see bug 347310... Would this change have any effect on that? Nope, wouldn't affect that... it will hit the http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsXMLHttpRequest.cpp&rev=1.226#2739 case, since it implements it's own nsIAuthPrompt and sets itself as callbacks on the channel (XHR adds itself as the channel's callbacks object when calling send(), but forwards calls on to the original callback, see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsXMLHttpRequest.cpp&rev=1.226#2263 ).
Assignee | ||
Comment 20•16 years ago
|
||
These are password manager that test the symptoms from bug 422135, and the window parenting problem that would have affected my previous patch, for both XMLHTTPRequests and document.load() (the two things being changed by this patch).
Attachment #309906 -
Flags: review?(dolske)
Comment 21•16 years ago
|
||
Comment on attachment 309906 [details] [diff] [review] tests >Index: toolkit/components/passwordmgr/test/authenticate.sjs >=================================================================== ... >+ if (/plaintext=true/.test(query)) { Test query at the top and set a var, like the others? >Index: toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html >=================================================================== > function runTest(testNum) { > // Seems we need to enable this again, or sendKeyEvent() complaints. > netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect'); > ok(true, "Starting test #" + testNum); >+ window.focus(); Do this later, once someone in bug 399852 can reply about this? Also, now that I look... I'm a bit wary to set the focus in each subtest, lest that mask some other focus bug. If a focus() call ends up being needed at all, it should probably just be done once at the beginning of the test. >+ // Check that the dialog has the correct parent >+ ok(doc.defaultView.opener, "dialog has opener"); >+ // Not using is() because its "expected" text doesn't deal >+ // with window objects very well >+ ok(doc.defaultView.opener == window, "dialog's opener is correct"); What's this testing? >+ // Explicitly cancel the dialog and report a fail in this failure >+ // case, rather than letting the dialog get stuck due to an auth >+ // failure and having the test timeout. >+ if (!username && !password) { >+ ok(false, "No values prefilled"); >+ clickOK = false; >+ } I suppose we should have at least one test somewhere that tests the "no logins stored for host" path... Doesn't need to be for XHR, just add as a TODO in prompt_auth.html if you want. :) >+ makeRequest("authenticate.sjs?user=xmluser1&pass=xmlpass1&realm=xml"); Why testing both plaintext and xml responses? Do they end up going through different paths?
Attachment #309906 -
Flags: review?(dolske) → review+
Assignee | ||
Comment 22•16 years ago
|
||
(In reply to comment #21) > >+ window.focus(); > > Do this later, once someone in bug 399852 can reply about this? Sure. > >+ // Check that the dialog has the correct parent > >+ ok(doc.defaultView.opener, "dialog has opener"); > >+ // Not using is() because its "expected" text doesn't deal > >+ // with window objects very well > >+ ok(doc.defaultView.opener == window, "dialog's opener is correct"); > > What's this testing? See comment 2. One of the things this patch fixes is that previously, the auth dialog would be parented to whichever window was focused when the prompt appears. Now it's parented to the window where the load originated, which this test ensures. > I suppose we should have at least one test somewhere that tests the "no logins > stored for host" path... Doesn't need to be for XHR, just add as a TODO in > prompt_auth.html if you want. :) Added a TODO. > Test query at the top and set a var, like the others? > Why testing both plaintext and xml responses? Do they end up going through > different paths? This plaintext stuff is left over from when I just had the XHR test, and wanted something easy to parse. Since I had to make the output XML for the document.load() test I can just use that for the XHR test now, so I'll just remove all of this.
Assignee | ||
Comment 23•16 years ago
|
||
Attachment #309268 -
Attachment is obsolete: true
Attachment #309906 -
Attachment is obsolete: true
Attachment #309928 -
Flags: approval1.9?
Attachment #309268 -
Flags: approval1.9?
Assignee | ||
Comment 24•16 years ago
|
||
(In reply to comment #16) > (From update of attachment 309268 [details] [diff] [review]) > This patch works around bug 422135 (broken XHR authentication prompting) Update: given the way we've decided to fix bug 422135, this patch is now needed to support XHR password saving and pre-filling. The patch for bug 422135 will only fix the missing prompts, allowing you to log in manually.
Flags: blocking1.9?
Comment 25•16 years ago
|
||
Comment on attachment 309928 [details] [diff] [review] patch+tests a1.9=beltzner
Attachment #309928 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Flags: blocking1.9? → blocking1.9+
Assignee | ||
Comment 26•16 years ago
|
||
mozilla/content/base/src/nsXMLHttpRequest.cpp 1.227 mozilla/toolkit/components/passwordmgr/test/Makefile.in 1.14 mozilla/toolkit/components/passwordmgr/test/authenticate.sjs 1.2 mozilla/toolkit/components/passwordmgr/test/prompt_common.js 1.1 mozilla/toolkit/components/passwordmgr/test/test_prompt.html 1.8 mozilla/toolkit/components/passwordmgr/test/test_xhr.html 1.1 mozilla/toolkit/components/passwordmgr/test/test_xml_load.html 1.1 mozilla/content/xml/document/src/Makefile.in 1.58 mozilla/content/xml/document/src/nsXMLDocument.cpp 1.272
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•