Closed Bug 422808 Opened 14 years ago Closed 14 years ago

nsXMLHTTPRequest and nsXMLDocument forwarding requests for nsIAuthPrompt is troublesome

Categories

(Core :: DOM: Core & HTML, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta5

People

(Reporter: Gavin, Assigned: Gavin)

References

Details

Attachments

(1 file, 4 obsolete files)

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).
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #309258 - Flags: superreview?
Attachment #309258 - Flags: review?(bzbarsky)
Attachment #309258 - Flags: superreview? → superreview?(cbiesinger)
Priority: -- → P1
Target Milestone: --- → mozilla1.9beta5
Attached patch better patch (obsolete) — Splinter Review
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)
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 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+
Summary: nsXMLHTTPRequest::GetInterface should support nsIAuthPrompt2 → nsXMLHTTPRequest and nsXMLDocument forwarding requests for nsIAuthPrompt is troublesome
Attached patch XMLDocument too (obsolete) — Splinter Review
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)
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?
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+
Filed bug 422849 for the XSLT code.
(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.
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?
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?
Microsummaries did some vaguely related stuff involving XHR and authentication, see bug 347310... Would this change have any effect on that?
(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 ).
Attached patch tests (obsolete) — Splinter Review
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 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+
(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.
Attached patch patch+testsSplinter Review
Attachment #309268 - Attachment is obsolete: true
Attachment #309906 - Attachment is obsolete: true
Attachment #309928 - Flags: approval1.9?
Attachment #309268 - Flags: approval1.9?
(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 on attachment 309928 [details] [diff] [review]
patch+tests

a1.9=beltzner
Attachment #309928 - Flags: approval1.9? → approval1.9+
Flags: blocking1.9? → blocking1.9+
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: 14 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 425078
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.