Closed Bug 776171 Opened 8 years ago Closed 7 years ago

XMLHttpRequest authentication doesn't work after the first time

Categories

(Core :: Networking: HTTP, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: squib, Assigned: mayhemer)

Details

Attachments

(3 files, 3 obsolete files)

In the Gaia email app, we need to communicate via XHR with ActiveSync providers (Hotmail, Exchange). To do this, we use the 5-arg form of XHR.open(), passing in the username and password. However, this fails after the first XHR, and causes an authentication dialog to appear. Entering the username/password again makes everything work fine from there on.

Here's a not-so-minimal example (requires a Hotmail account and dom.systemXHR.whitelist to be set appropriately): https://github.com/mozsquib/jsas/blob/master/test/test_eas.html

I can try to whip up a better test case if it would be helpful.

Also, this might be a dupe, but the other XHR auth bugs I saw didn't have a lot of details. Bug 654348 and bug 607128 are likely candidates.
Adding blocking-basecamp? based on chat in #gaia.
blocking-basecamp: --- → ?
Better testcase that I can run would be nice, yes.  ;)

As a start, though, maybe an HTTP log for the case you're hitting?
Attached file HTTP log
Here's the log. I'll try to get a minimal test case working over the weekend.
I guess I should explain the log a bit:

The first and second requests are for the autodiscover page, and it looks like this:

  xhr.open("POST", "https://m.hotmail.com/autodiscover/autodiscover.xml",
           true, this._email, this._password);

After that is a request for the available options:

  xhr.open("OPTIONS", "https://m.hotmail.com/Microsoft-Server-ActiveSync", true);

Finally, I try to run a proper command on the ActiveSync server:

  xhr.open("POST", "https://m.hotmail.com/Microsoft-Server-ActiveSync?Cmd=" + command +
           "&User=" + this._email + "&DeviceId=v140Device&DeviceType=SmartPhone",
           true, this._email, this._password);

In that last command, things *should* just work since I supplied credentials, but I get an authentication dialog. If I skip the autodiscover and OPTIONS steps above, running this last command works fine.
Hmm.  Does it work if you skip the autodiscover but not the OPTIONS?
(In reply to Boris Zbarsky (:bz) from comment #5)
> Hmm.  Does it work if you skip the autodiscover but not the OPTIONS?

It does. It also works if I skip autodiscover the first time, revert that change, then reload and go through autodiscover -> OPTIONS -> command, but I expect that's just because it's remembering the credentials.

One other thing that might be relevant (but might not be): after what happens in the HTTP log, the next step - if you enter your credentials again - is usually (but not always) a redirect to https://col-m.hotmail.com/. I don't think this is causing the problem though, since the Authentication dialog pops up before we get a redirect response.
OK.  I'd still love a testcase I can reproduce on, but also a full HTTP log (not just headers, but the full log that https://developer.mozilla.org/en/HTTP_Logging produces) would be much appreciated....
Do you want all of these options: "NSPR_LOG_MODULES=timestamp,nsHttp:5,nsSocketTransport:5,nsHostResolver:5"?
Attached file Full HTTP log
Here's a log with the above settings (hopefully I sanitized everything).
I just realized that I can inject the Authorization header manually, which works perfectly. I think this is still a bug, but it's not blocking me now.
blocking-basecamp: ? → ---
Hmm.  We're definitely not sending the credentials from the URI on that last post.  The logging in the auth provider isn't detailed enough to say why.  Honza, any idea what's up here?
Taking a look.
Assignee: nobody → honzab.moz
I have written a mochitest that is able to reproduce this problem.  Working on solution now.
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
To describe the problem:

First we auth at /autodiscover/autodiscover.xml - auth path is /autodiscover

Then, we auth at /Microsoft-Server-ActiveSync?Cmd=QUERY-STRING - auth path is /

Realm in both cases is "ActiveSync".

We are looking for an auth node (or entry) under "https + m.hotmail.com + ActiveSync".  During the second load, there already is such a node.

At [1] we delete the ident gained just few lines above from the URL because we believe the entry has just been used (for this request) and it was therefor wrong based on the 401 response.


It is a wrong assumption that the entry is bad.  I created this patch based on:
- mIdent.Empty() is true only when we 
  - either never entered nsHttpChannelAuthProvider::GetCredentialsForChallenge 
  - or if the entry has been deleted on presumption it has been used for request resulting at 401/7 response
- in such case we we prompt for identity
- prompt for identity puts a new entry to the cache and fills mIdent what bypasses to fill mIdent from URL on next entry to GetCredentialsForChallenge 

In other words: mIdent is never empty after we first entered GetCredentialsForChallenge().


[1] http://hg.mozilla.org/mozilla-central/annotate/d78729026fb9/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#l650
Attachment #645047 - Flags: review?(bzbarsky)
Hmm.  This makes this test:

+                else if (!identFromURI ||
+                         nsCRT::strcmp(ident->User(),
+                                       entry->Identity().User()) == 0) {

always true if reached, right?

Honza, does anyone else know this well enough to review?  I'd have to read up on this code to sort it out... :(
(In reply to Boris Zbarsky (:bz) from comment #15)
> Hmm.  This makes this test:
> 
> +                else if (!identFromURI ||
> +                         nsCRT::strcmp(ident->User(),
> +                                       entry->Identity().User()) == 0) {
> 
> always true if reached, right?

Right, what implies, that the patch can be wrong.

I'll rereview it ones more.

> 
> Honza, does anyone else know this well enough to review?  I'd have to read
> up on this code to sort it out... :(

Maybe biesi?  This code is tricky.
Comment on attachment 645047 [details] [diff] [review]
v1

Per comments
Attachment #645047 - Flags: review?(bzbarsky) → review-
Attached patch v2 (obsolete) — Splinter Review
See comment 14 for explanation.
Attachment #645047 - Attachment is obsolete: true
Attachment #667752 - Flags: review?(cbiesinger)
Comment on attachment 667752 [details] [diff] [review]
v2

I attached a wrong patch (the old one instead of the new one).
Attachment #667752 - Attachment is obsolete: true
Attachment #667752 - Flags: review?(cbiesinger)
Attached patch v2 (obsolete) — Splinter Review
This is the correct one.
Attachment #668617 - Flags: review?(cbiesinger)
Attached patch v2 + testSplinter Review
- includes a test
Attachment #668617 - Attachment is obsolete: true
Attachment #668617 - Flags: review?(cbiesinger)
Attachment #669206 - Flags: review?(cbiesinger)
Maybe the tests should be moved to content/base/test where some other xhr tests are lying...
Comment on attachment 669206 [details] [diff] [review]
v2 + test

so many nested ifs :(

why did you test this with a xhr mochitest instead of an nsIChannel xpctest?
(I really think we need a comprehensive set of auth xpctests, but that's rather out of scope for this bug)
https://tbpl.mozilla.org/?tree=Try&rev=04b88dc7eba1


(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #25)
> Comment on attachment 669206 [details] [diff] [review]
> v2 + test
> 
> so many nested ifs :(

Yes, but I don't want to modify this code more then necessary, at least now.  We can do a clean up later if you believe we need it.

> 
> why did you test this with a xhr mochitest instead of an nsIChannel xpctest?

Because this problem is reported as a xhr issue and touches xhr functionality.  I think we just need both kind of tests.

Is any of your comments a review requirement?  Should I somehow try to update the patch?
Comment on attachment 669206 [details] [diff] [review]
v2 + test

I think it would be great if you could also add an xpctest in netwerk/test, unless that's too difficult for some reason. With that r=biesi.
Attachment #669206 - Flags: review?(cbiesinger) → review+
(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #29)
> I think it would be great if you could also add an xpctest in netwerk/test,
> unless that's too difficult for some reason. With that r=biesi.

It would mean to copy (4th time!) code of authenticate.sjs into the xpcshell test request handler.  I don't think it is worth it, since the test would be identical to what the current mochitest is doing.  But if we really want it, then I would like to think of some kind of sharing the authenticate.sjs code among all test harnesses we have first.
https://hg.mozilla.org/mozilla-central/rev/6222e6c573a3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.