Closed Bug 698730 Opened 13 years ago Closed 10 years ago

Multiple http connections with different NTLM authentication get mixed up

Categories

(Core :: Networking: HTTP, defect)

Product:
Core
Component:
Networking: HTTP
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla31

People

(Reporter: info, Assigned: rkent)

Details

Attachments

(1 file, 3 obsolete files)

No error checks for GetUsername, [username] in hash
8.41 KB, patch
mcmanus
: review+
rkent
: checkin+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:7.0.1) Gecko/20100101 Firefox/7.0.1
Build ID: 20110930134335

Steps to reproduce:

Create multiple "xmlhttprequest" connections to same Exchange server (https://exchange2010.exmaple.com/EWS/Exchange.asmx) with different user credentials. Exchange server uses NTLM authentication and Connection: keep-alive (is necessary for NTLM).



Actual results:

Received Exchange errors, not http errors, of trying to access data not authorized for authenticated user.

In the IIS log files on the Exchange server I can see that http request to the Exchange server are received on a connection with different authentication.

How to reproduce:
1. Create xmlhttprequest_1 and authenticate with user John. The connection is created and if we send data the data is send and received ok.
2. Create xmlhttprequest_2 and authentocate with use Jane.  
  -- A. if xmlhttprequest_1 is still busy then a new connection is created and sending and receiving is ok.
  -- B. If xmlhttprequest_1 is idle this idle connection will be reused and request is send of xmlhttprequest_1 authenticated as user John and not Jane. Exchange will respond with 200 but because the request is for a part only accessible by user Jane it will given a webpage with an error.
3. Create xmlhttprequest_3 and user Karel. The result is unknown see case 2.

Every time a new xmlhttprequest is made with one of the users the HttpConnectionMgr will return the first idle connection is has to the server.



Expected results:

The xmlhttprequest should only be send over a connection with the right NTML credentials.
I'm working on a patch/proposal for resolving this problem.

Following is the idea:
- Add extra parameter, username, to object initializer "nsHttpConnectionInfo" in "nsHttpConnectionInfo.h")
- Add Extra parameter, username, to function  nsHttpConnectionInfo::SetOriginServer so the unique key, mHashKey, will have the username in it.
- Change function HttpBaseChannel::Init (in HttpBaseChannel.cpp) to supply the username when creating a new "nsHttpConnectionInfo" object.
- Change function nsHttpConnectionInfo::Clone() in "nsHttpConnectionInfo.h" to suppply username when creating a new "nsHttpConnectionInfo" object.
Keywords: reproducible
To which channel/version should I create the patch?

Problem is at least in version 7 and higher.
(In reply to Michel Verbraak from comment #2)
> To which channel/version should I create the patch?
> 

Hi Michel, thanks for signing up for a patch, that's awesome! If you have questions just let us know - I can help with general networking stuff but I don't have any NTLM specific experience. You should set the assigned-to field of this bug to yourself if you are working on a patch so there aren't duplicated efforts.

> Problem is at least in version 7 and higher.

definitely against mozilla-central. It seems unlikely we would backport it, but if we did it would be after it landed on the nightly (i.e. mozilla-central) so you might as well start there.

Like I said, all I know about ntlm I read on a webpage - but I read that it authenticates the connection so I understand the bug report in comment 0. Can the handshake be redone on an existing connection? Maybe that's not something we want to do - just throwing it out there.

In any event, assuming we want the ntlm username to be part of the connection entry hash key you don't want to change setoriginserver(), rather you want to add a SetNTLMUsername() and then have the new method just update the existing hash key like SetAnonymous() does. You can have it raise an error if setoriginserver() hasn't been called yet. This way you don't need to change the ctor. (just have the clone logic call SetNTLMUsername() too.
Patrick,

Thanks for your reply. I will create the patch against mozilla-central and have it available tonight (CET).

I'm no NTML specialist either but what I read (http://www.innovation.ch/personal/ronald/ntlm.html) it needs a connection keep-alive because the authentication is at least four (4) steps. And yes it authenticates the connection. The handshake cannot be redone for this you need to disconnect the existing connection and create a new one.

I will add an new method as you said and test it.

I have another bug open 698523 which also has the same kind of problem but it is about the hashkey for the AuthCache.
Patrick,

How can I assign it to myself?
(In reply to Michel Verbraak from comment #5)
> Patrick,
> 
> How can I assign it to myself?

click on take in the assigned to field at the top of the bug.. I just assigned this one to you.
Assignee: nobody → info
Should I only add the new functionality for NTLM connections or for all http connections for which in de mURI the Username part is set? (mURI->GetUsername())
(In reply to Michel Verbraak from comment #7)
> Should I only add the new functionality for NTLM connections or for all http
> connections for which in de mURI the Username part is set?
> (mURI->GetUsername())

normal http auth is done on a per transaction basis, so it doesn't need to go into the connection info hash imo
Michel, haven't heard back from you recently so I just wanted to check in and see if I can help (or can find someone to help)?
Patrick. I was busy. Next sunday I will have time again to build and debug this problem.
Like Michel, I also work on Exchange Web Services, and encounter this issue regularly. I'll at least confirm the bug.

Patrick, are you still around, or is there somebody else monitoring this who is close to the http protocol, and could give advice and/or review a patch?

I might do a patch for this, one question though. In nsHttpChannel:BeginConnect(), in order to add the username to the hash only when NTLM is in use as suggested in comment 8, I need to be able to detect when NTLM is being used. Any hints on what I could observe to tell me whether NTLM is being used at that point?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to Kent James (:rkent) from comment #11)
> Like Michel, I also work on Exchange Web Services, and encounter this issue
> regularly. I'll at least confirm the bug.
> 
> Patrick, are you still around, or is there somebody else monitoring this who
> is close to the http protocol, and could give advice and/or review a patch?
> 

I'm here - we need some NTLM expertise. If you've got that and submit patches I'll do my best to get them reviewed.
So I'll be working this in the next few weeks, hoping to land it in Gecko 31 so that it can get into Thunderbird 31.

I've tried something similar to the approach in comment 1 that works, but agreeing with comment 8 I think it would be cleaner if username was not added to connectionInfo. The approach that I am looking at now is to add information to nsHttpConnection that stores whether the connection auth used is CONNECTION_BASED and the username used for that connection. Then in nsConnectionMgr where idle persistent connections are dispatched to connections, it should skip connections of that sort when the transaction has a username attached to it to does not match the connection. The tricky part is all of the cross-thread information exchange.
Status: NEW → ASSIGNED
Attached patch userInConnection.patch (obsolete) — — Splinter Review 
Here's a work in process (currently compiled on mozilla-aurora, not mozilla-central). Seems to work as far as I can tell.
Assignee: info → kent
Attached patch Handle domain, null Username() (obsolete) — — Splinter Review 
This one is patched to mozilla-central, and is cleaned up for review.

There were various issues I had to decide. Let's see if other accept the decisions. Like:

1) I kept the concept of "CONNECTION_BASED" throughout rather than specialize to NTLM

2) Key variables go through the AuthProvider.

3) When either the transaction or the connection has a blank username, the connection may be reused.

4) I introduced a string type into the connection rather than try to do everything with character pointers.

The primary application of this will probably be in web services where the app is passing credentials through the URI. Certainly Exchange Web Services is an important use case.
Attachment #8385152 - Attachment is obsolete: true
Attachment #8385766 - Flags: review?(mcmanus)
Comment on attachment 8385766 [details] [diff] [review]
Handle domain, null Username()

Review of attachment 8385766 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1669,5 @@
> +                conn = nullptr;
> +                continue;
> +              }
> +            }
> +            ent->mIdleConns.RemoveElementAt(i);

Kent. The RemoveElementAt will shorten the list but "i" will still be increased. It will skip a connection when there are multiple connections where some are not AuthIsConnectionBased or do not have matching usernames. If you keep the original while loop and the first two lines of the original while loop, where you always get the first of the connection list, and put the if AuthIsConnectionBased after the RemoveElementAt(0) we will not skip over connections.
not a review yet - just a thank you for taking a shot at this! It certainly isn't trivial.
Comment on attachment 8385766 [details] [diff] [review]
Handle domain, null Username()

Review of attachment 8385766 [details] [diff] [review]:
-----------------------------------------------------------------

the biggest advantage of placing this info into the connectioninfo hash would be that its quota of 6 HTTP/1 connections would be isolated from the connections used with a different username. As this is written, its actually possible to essentially block a new transaction waiting for an idle persistent connection (with a different username) to close. You could wait a very long time.

you've also got problems of other dispatch mechanisms sneaking in on you - assuming that the CI identifies all the necessary properties.. see AddToShortestPipeline for example. That will bypass your new code.

So I think this needs to be added to the connectioninfo hash in some way afterall :( What do you think?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4878,5 @@
>  
>  NS_IMETHODIMP
> +nsHttpChannel::GetIsConnectionBased(bool *isConnectionBased)
> +{
> +    if (mAuthProvider)

we're going to try and embrace the {} all the time syntax in necko (that's a change for us). that comment applies to the whole patch - I won't repeat it

so.. we want it to look like this . mandatory {}, { on EL, } else {
if (foo) {
 blah;
} else {
 blah2;
}

::: netwerk/protocol/http/nsHttpChannel.h
@@ +79,5 @@
>      NS_IMETHOD GetServerResponseHeader(nsACString & aServerResponseHeader);
>      NS_IMETHOD GetProxyChallenges(nsACString & aChallenges);
>      NS_IMETHOD GetWWWChallenges(nsACString & aChallenges);
> +    NS_IMETHOD GetIsConnectionBased(bool *aIsConnectionBased);
> +    NS_IMETHOD GetAuthUsername(nsAString & aAuthUsername);

no space after &

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +314,5 @@
>  
>      if (!trans->IsNullTransaction())
>          mExperienced = true;
>  
> +    if (trans->AuthIsConnectionBased())

is this the right place for this? TODO

::: netwerk/protocol/http/nsHttpConnection.h
@@ +16,5 @@
>  #include "nsIAsyncInputStream.h"
>  #include "nsIAsyncOutputStream.h"
>  #include "nsIInterfaceRequestor.h"
>  #include "nsITimer.h"
> +#include "nsStringFwd.h"

move this to the other ns-not-I includes (nsPrxyRelease)

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1651,5 @@
>      // step 2
>      // consider an idle persistent connection
>      if (caps & NS_HTTP_ALLOW_KEEPALIVE) {
>          nsRefPtr<nsHttpConnection> conn;
> +        for (uint32_t i = 0; !conn && (i < ent->mIdleConns.Length()); i++) {

total nit: ++i is the style we want to move to

@@ +1669,5 @@
> +                conn = nullptr;
> +                continue;
> +              }
> +            }
> +            ent->mIdleConns.RemoveElementAt(i);

echo michel's comment
Attachment #8385766 - Flags: review?(mcmanus) → review-
Sorry to be slow to get back to this. I hope to put a lot of effort into this in the next few days though.

Adding the name to the connectioninfo hash is fairly straightforward if it can be done for all connections, and not just for AuthIsConnectionBased. Basically if the URL includes a username, use that name. else use blank.

Do you think that would be a problem?
(In reply to Kent James (:rkent) from comment #19)
> Sorry to be slow to get back to this. I hope to put a lot of effort into
> this in the next few days though.
> 
> Adding the name to the connectioninfo hash is fairly straightforward if it
> can be done for all connections, and not just for AuthIsConnectionBased.
> Basically if the URL includes a username, use that name. else use blank.
> 
> Do you think that would be a problem?

I think that would be fine - but does that solve this problem? I think I'm missing something.. but NTLM transactions don't normally have the username in the url do they?
"(In reply to Patrick McManus [:mcmanus] from comment #20)

> I think that would be fine - but does that solve this problem? I think I'm
> missing something.. but NTLM transactions don't normally have the username
> in the url do they?

I can really only discuss this confidently after I have embedded myself in the code for a couple of days, but here is what I recall from my last attempt.

It is certainly true that users will be prompted for username and password automatically when the URL does not include username, and that name never gets included in the URL. In my previous code, I tried to capture the username information in the channel and authprovider rather than in the ConnectionInfo hash because it was possible to change those later in the transaction, while the ConnectionInfo hash which determines which CI gets used is established very early. I'm not sure that it will be straightforward to use a different CI after the username is input by the user.

There is also a second issue. In addition to the connection cache, there is also an authentication cache issue with NTLM. I don't recall the details, but specifying the username in the URL gets around that issue as well without any code changes.

So I am reasonably confident that simply adding the username to the CI, when that username is specified in the URL, will provide one viable method for a web service to use NTLM with multiple usernames to the same server. Currently, this is only possible with a very painful workaround that involves tracking all https access to a particular server, detecting when the username is changing, and starting with a fresh connection (after all requests have completed) when it changes. This does not work at all when it is necessary to keep open a long-lived connection though, for example when notifications are streamed over a long-lived connection.

So at least for the purposes of people who are writing programmatic interfaces to an NTLM webservice, adding the username to the CI hash from the URL will provide a viable method of making this work with only a fairly small change to the Mozilla code. That would be sufficient to solve my problems and hopefully those of others with similar issues, even if it does not robustly deal with the issue in all of its possible manifestations.
This patch simply adds the username from the URI to the connection info hash, for all connection types. This is sufficient to solve the problem with NTLM when the username is specified in the URI.

Could I have some feedback on this approach?
Attachment #8405053 - Flags: feedback?(mcmanus)
Comment on attachment 8405053 [details] [diff] [review]
Add username from uri to connection info

Review of attachment 8405053 [details] [diff] [review]:
-----------------------------------------------------------------

I'm sorry I've taken a while to respond to this feedback. I've been thinking a lot about it.

I think we agree that this doesn't really solve the problem - usernames in urls are (at best) a subset of the windows auth cases.

But I'm absolutely *not* a perfect at the cost of the good kind of module owner. I totally encourage partial solutions that are clear steps forward and its true that the real answer to this is a pain.

The problem here is this also impacts non windows auth - different usernames in the url when using something like basic auth will now also use different connections and have to pay for that overhead. So it isn't an unambiguous step forward - its a trade.

In the end I've decided to NAK the approach based on that. I think if it did adequately cover all the windows auth cases then I would be willing to apply it to the basic auth scenario as well if necessary as a cost.. but basing it just on urls seems like cruft for too little gain.

let me know if you think that's unreasonable.
Attachment #8405053 - Flags: feedback?(mcmanus) → feedback-
"The problem here is this also impacts non windows auth - different usernames in the url when using something like basic auth will now also use different connections and have to pay for that overhead"

That is the reason that I initially avoided an approach based on modifying the CI, because it seemed to me that there was no practical way to add a username at that point without affecting other, much more common methods of authentication. So if there are indeed objections to adding username to CI, then I think we have to go back to the previous approach, and try to fix its problems.

Looking again at the issues there, I think the most serious of these was the connection limit. The nsConnectionEntry knows about its connections, so perhaps I could modify the way that the active connection count is returned such that, for connections with connection-based authentication, either the connection count is returned on a per-user basis, or alternatively we could return a username count and multiply the per-server connection limit by that value.

In any case, with the original approach accommodating NTLM can be done with fairly minimal impact on other connections.

So unless you have some objections, I would like to attempt to revise the original patch, fix the nits, address the issue of dealing with connection limits with multiple usernames, but not necessarily attempt to fix the problem of getting it to work for all ways that the username might be specified.
kent, michel - I appreciate your efforts here. I think I need to go all the way to the original use case and clarify a few things. Any help you can provide is appreciated

Let's assume there is no username in the URL for all of this. If usernames in urls are a fundamental thing with NTLM and I just don't know that - tell me :)

(In reply to Michel Verbraak from comment #0)
> How to reproduce:
> 1. Create xmlhttprequest_1 and authenticate with user John. The connection
> is created and if we send data the data is send and received ok.

What really happens here is that we send the xhr on a new pipe and get back a 401 and pop-up a login dialog. the user types in john and password (or inferred from current login) and we retry the xhr a time or two until it is authenticated and a 200 comes back. That connection now applies to john.

Do I have that right/

> 2. Create xmlhttprequest_2 and authentocate with use Jane.  
>   -- A. if xmlhttprequest_1 is still busy then a new connection is created
> and sending and receiving is ok.

2A works just like 1 - except the connection now applies to jane and presumably jane was typed in the dialog (or inferred from the logged in user)

>   -- B. If xmlhttprequest_1 is idle this idle connection will be reused and
> request is send of xmlhttprequest_1 authenticated as user John and not Jane.
> Exchange will respond with 200 but because the request is for a part only
> accessible by user Jane it will given a webpage with an error.

This is where I'm confused. How do we know this is for jane if it isn't in the url? Or is this all supposed to be url based? Upon receiving this xhr on john's connection that is only valid for jane, shouldn't exchange be coming back with a 401?

If this all really does boil down to the username in the url to cover the whole problem, it could change my thinking.
"If usernames in urls are a fundamental thing with NTLM and I just don't know that - tell me"

I was thinking over this issue myself last night.

Consider this case. You are connected to a server with an existing, authenticated NTLM connection #1 authenticated with user #1. Now you want to connect to the server with user #2, making a simple request like "Give me the ID of the root folder for user #2's mailbox". The username to use for this request must be specified to get the correct ID, and that information is not part of the request itself, but is only specified when the connection is authenticated to a particular username.

What that means to me is that you cannot just try a request and go through the typical 401 response followed by entering the correct username, because you have to know, when you initially make the request, what is the correct username. If you do not specify the username, then how will you know to avoid the existing connection? If you use the existing connection you will not get any error, you will just get the wrong result. Doesn't that imply that the username must be part of the URL for the request (or that only one user can be active at any point in time)?

For Basic authentication, we can insert the authentication header in each request manually, and that allows the correct username to be determined. This is how the Firefox OS email apps gets around this problem, BTW. I suspect that they currently break in multiple user cases where the server uses NTLM, though that is probably fairly rare in a phone.

I'm also not sure that this issue is unique to NTLM, but is more an issue that the basic URL does not specify sufficient information to return the correct result. I've mentioned before that there is also an issue with the authentication cache. I think that I'm actually starting to drift into those issues, though that is not the immediate subject of this bug.

So imagine a GET request (which is not actually how EWS works) that is:

https://example.com/EWS/Exchange.asmx?command=RootFolderId

that returns ID1 for user1, and ID2 for user2. This request without a username cannot possibly return the correct result, so the only way this can work is to specify the username as part of the URL.

So I think the situation as I understand it is that we are dealing with really two issues. 1) When the result of the http request is dependent on the username used to authenticate, then the username must be included with the URL (independent of the authentication method). 2) For connection-based authentication, the correct connection must be chosen from existing connections based on the username.

Both 1) and 2) are true in the common case of an NTLM connection to Exchange Web Services (or presumably also to Exchange Active Sync) and so that is the most critical case.

I suppose I could imagine a case where 1) was not true, but 2) was true. In that case, you could imagine a url such as https://example.com/Exchange.asmx?command=getuser2secret that could return a 401 when you tried to access this with user1's credentials. I suspect that this would work with the existing code, but be constantly prompting the user for the correct username unless the username was specified as part of the URL.

How does this work in the non-NTLM case, say if I want to have one tab that shows the GMail page of user1, and a second that shows user2? If you see for example http://www.makeuseof.com/tag/4-ways-simultaneously-manage-multiple-gmail-accounts/ a common recommendation is to actually run two separate browser instances which seems quite tacky. I wonder if Firefox properly handles two tabs, one with https://user1@gmail.com and https://user2@gmail.com?

In the non-ntlm case, it is possible to manage the http headers yourself programmatically and probably get around the issue. Connection caching makes this impossible in the ntlm case, and that is the main point of this bug.
This is what I know for the connections to Exchange through EWS but it will probably also be so for connections to other services:

The NTLM (or Basic and digest) credentials are for the HTTP connection. The username and password do not have to be part of the URL. Only specific thing for NTLM authentication is that the HTTP header needs to have option "Connection: keep-alive" set due to the multiple exchange of nonce and authentication details before the connection is authenticated.

The NTLM (or basic or digest) is for authentication to Exchange (Windows AD). In the body of the POST request we specifiy which exchange folder we would like to read from or write to. So when the NTLM authentication succeeds we will never get a HTTP 401 back from exchange. We will probably get some other value back, might be even a 200, and then the application should process the body to see what really went wrong. This could be something like "You are not allowed to read or write to the selected folder or item". 

@Patrick: The problem I reported was first seen in the following case:
- The EWS add-on for Lightning in Thunderbird is so we are able to manager our Exchange calendar in Thunderbird. With the add-on you can add multiple Exchange calendars to Thundebird/Lightning.
One of our users added one calendar which accessed the default calendar for his username (lets say user john). So he put in the authentication credentials for user john and as calendar folder the default folder for primary SMTP address john@example.com (this is not a username but in EWS his primary SMTP address).
He also added a second calendar which is a group calendar. To access this calendar from OWA (Outlook Web Access) he used a separate username (lets say user Jane) to authenticate and as the folder he specified group@example.com. So for this second calendar in the add-on he specified as authentication username Jane and as primary SMTP address group@example.com
User John has read access to the calendar folder of primary SMTP addres group@example.com but user jane does not have read access to folder of primary smtp address john@example.com
Both access the Exchange server using the same URL: https://exchange.example.com/ews/exchange.asmx

The credentials to authenticate to Exchange for both users are entered ok. But depending on which connection is choosen from the connection pool by Thunderbird (or Firefox) the request to read from calendar with primary address "john@example.com" goes over the connection authenticated by John or the connetion authenticated by jane. Bot will return a HTTP 200 OK but in the body of john's connection it will give results and in the jane's connection it will return an error. 
 
Through this case I did some more testing and found out what I reported in comment 0.  

As an addition to my first comment 0 and some of the other:
- I can get two xmlhttprequest objects connected with different NTLM credentials by just changing one letter in the URL from lower case to upper case for the second xmlhttprequest. In this case the connection info hash is different for both connections.

In the case of EWS (Exchange Web Services) add-ons I think users should only authenticate with one user and grant this user the right permission to read folders for the different primary SMTP addresses. But like in the case of gmail (comment 26), as specified by Kent, you the user are not able to grant others read/write permissions to your mailbox (as far as I know)

Ok back to the problem: Is there any way where we can make the selection from the connection pool being based on URL, authentication type and username? I do not know if authentication type is required and URL and username are enough. 
I think this should also work without having the username specified as part of the URL. 

In the "Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance().open("POST", URL, true, Username, Password);" function we specify the username. This username is also used in building the connection. Can we store this username somewhere so we can use it for the match from the connection pool.

Currently I hardly have any time to dig through the source. But when I have time I will try to look or give feedback on other suggestions and patches.

Thank you all for the work.
Michel Verbraak writes: "In the 'Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance().open("POST", URL, true, Username, Password);' function we specify the username. This username is also used in building the connection. Can we store this username somewhere so we can use it for the match from the connection pool."

In code beginning here http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1661 if the username is passed in through nsXMLHttpRequest::Open then it is set into the uri (an nsIURI object) as userPass, which is then  passed into the channel used for the connection. So XMLHTTPRequest is just a higher-level abstraction of the nsIChannel/nsIURI which is used for the connection. Michel's "somewhere" that we should store the username *is* the uri, so when I propose to grab the username from the uri "for the match from the connection pool", I believe that I am proposing exactly what Michael is also proposing.

Alternatively, code can choose to embed the username into the spec for the string url which is passed to the nxXMLHttpRequest, where it is then made part of the nsIURI object when the uri is created here http://mxr.mozilla.org/comm-central/source/mozilla/content/base/src/nsXMLHttpRequest.cpp#1638.

In either case the result is the same, the username is stored in the uri that is passed to the channel.

The general principle should be (independent of authentication type) that, when the username is passed in as part of the request, then the underlying http protocols should choose to pass appropriate username-dependent authentication to the server when making the request.

In the specific case of NTLM (and more generally in connection-based authentication) which is the subject of this bug, that means keeping track of the username used in particular connections, and not selecting connections that have a non-matching username.
In comment 25 :mcmanus says:

"This is where I'm confused. How do we know this is for jane if it isn't in the url? Or is this all supposed to be url based? Upon receiving this xhr on john's connection that is only valid for jane, shouldn't exchange be coming back with a 401?

If this all really does boil down to the username in the url to cover the whole problem, it could change my thinking."

John's connection is already authenticated and able to accept xhr requests.

If I do my hypothetical GET request https://example.com/EWS/Exchange.asmx?command=RootFolderId asking anonymously for information, I will get information from whatever cached connection currently exists, which happens to be John's. I think that is OK and is what happens now.

But if I wanted to specifically ask for Jane's information, I need some way to do that. It seems to me that the proper way to specify that is https://jane@example.com/EWS/Exchange.asmx?command=RootFolderId (or the xml/PUT equivalent which is what actually occurs). Currently, if you do that, you will get John's connection because of connection caching, with no way to override this. What this bug is moving toward proposing is that specifying the name in the url *should* override this, so that indeed it "really does boil down to the username in the url to cover the whole problem".

Exchange does not come back with a 401 because this is a valid authenticated request, just for the wrong user. But there is nothing in the request itself that says that, as the user is specified by the connection chosen.
Comment on attachment 8405053 [details] [diff] [review]
Add username from uri to connection info

Review of attachment 8405053 [details] [diff] [review]:
-----------------------------------------------------------------

thanks folks - you've convinced me that nsURI.username is a sufficient answer to this problem.

I think that having different basic auth users on different connections as a side effect is unusual enough that I'm ok with it - it would be different if we were a proxy :) The pain is minimal.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4535,5 @@
>          rv = mURI->GetAsciiHost(host);
>      if (NS_SUCCEEDED(rv))
>          rv = mURI->GetPort(&port);
>      if (NS_SUCCEEDED(rv))
> +        rv = mURI->GetUsername(username);

I think we want simply URI->GetUsername(username) here (not rv=). its ok if someone writes an nsIURI that does not provide a working implementation

::: netwerk/protocol/http/nsHttpConnectionInfo.cpp
@@ +76,5 @@
>      mHashKey.AssignLiteral("....");
>      mHashKey.Append(keyHost);
>      mHashKey.Append(':');
>      mHashKey.AppendInt(keyPort);
> +    if (!mUsername.IsEmpty())

brace on this line

@@ +78,5 @@
>      mHashKey.Append(':');
>      mHashKey.AppendInt(keyPort);
> +    if (!mUsername.IsEmpty())
> +    {
> +      mHashKey.Append(':');

let's go with Append(' [') Append(mUsername) Append(']')

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1191,5 @@
>          rv = uri->GetAsciiHost(host);
>      if (NS_SUCCEEDED(rv))
>          rv = uri->GetPort(&port);
> +    if (NS_SUCCEEDED(rv))
> +        rv = uri->GetUsername(username);

again - ignore rv

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1897,5 @@
>      if (NS_FAILED(rv))
>          return rv;
>  
> +    nsAutoCString username;
> +    rv = aURI->GetUsername(username);

again ignore rv
Attachment #8405053 - Flags: feedback- → feedback+
See https://tbpl.mozilla.org/?tree=Try&rev=fe5bd08b27dd
Attachment #8385766 - Attachment is obsolete: true
Attachment #8405053 - Attachment is obsolete: true
Attachment #8409003 - Flags: review?(mcmanus)
mcmanus: I hope you can get to this review in time for Gecko 31, as this is what Thunderbird will have to live with for the next year.
Attachment #8409003 - Flags: review?(mcmanus) → review+
Keywords: reproducible → checkin-needed
Comment on attachment 8409003 [details] [diff] [review]
No error checks for GetUsername, [username] in hash

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/a632ecfff394
Keywords: checkin-needed
Attachment #8409003 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/a632ecfff394
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: