Add test infra for auth testing. #26979
Conversation
|
|
||
| // Read the authorization header from client. | ||
| AuthenticationProtocols protocol = AuthenticationProtocols.None; | ||
| string authorizationHeader = response.Contains("Proxy-Authenticate", StringComparison.OrdinalIgnoreCase) ? "Proxy-Authorization" : "Authorization"; |
There was a problem hiding this comment.
Shouldn't this be checking for 'Authorization' request headers from the client? 'Proxy-Authenticate' is not something a client would send to a server. And these changes here are for the Loopback test server. This server isn't a proxy server.
There was a problem hiding this comment.
The string I'm checking here is the response the server sends, not the client. The code below checks for Authorization header from client.
There was a problem hiding this comment.
But I should remove the Proxy stuff, doesn't make sense here. It's left from my first draft.
| } | ||
|
|
||
| public sealed class ManagedHandler_Authentication_Test : HttpClientTestBase | ||
| { |
There was a problem hiding this comment.
Wouldn't we want these tests to be in HttpClientHandlerTest etc. so that they can be exercised by all the handlers?
There was a problem hiding this comment.
I ran these with winhttp handler, and there are a lot of test failures, for one, winhttp handler doesn't support Digest, and there were some failures due to non-conformance with RFC.
There was a problem hiding this comment.
winhttp handler doesn't support Digest
In general WinHttpHandler supports all the auth schemes that WinHTTP supports including Basic, Digest, Negotiate, NTLM. So, if there is a bug with Digest for WinHttpHandler, we should open an issue, etc.
There was a problem hiding this comment.
I ran these with winhttp handler
What about CurlHandler? If they both don't work, maybe the issue is actually with the server implementation rather than the client?
| if (data[currentIndex++] != ',') | ||
| { | ||
| parsedIndex = currentIndex; | ||
| return null; |
There was a problem hiding this comment.
Is there any benefit to making our parsing more strict here? Looking at the spec I agree that it is probably correct not to generate key value pairs that end with spaces or tabs. But accepting them when we do see them might be good.
Priya91
force-pushed
the
newhttpauth
branch
2 times, most recently
from
5efbacc to
ef7e59f
Compare
|
Well, that has a no merge label, I hope to get this in before that. |
| await ReadWriteAcceptedAsync(s, reader, writer, response); | ||
|
|
||
| // Read the request method. | ||
| string line = await reader.ReadLineAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
Just FYI, having it won't hurt anything, but we generally don't use ConfigureAwait in test code.
There was a problem hiding this comment.
Why not? Is this just for the ReadLineAsync() method or generally? Wouldn't we have the same deadlock problems in test code as in product code?
There was a problem hiding this comment.
Is this just for the ReadLineAsync() method or generally?
Generally
Wouldn't we have the same deadlock problems in test code as in product code?
We shouldn't be writing tests that synchronously block on such helpers; if we do, that's a test bug. We use ConfigureAwait(false) in product code for perf but also because we can't control whether someone does synchronously wait on it.
Why not?
Three reasons:
- xunit has a synchronization context in place for a reason, and we generally want to respect that reason, which is to limit concurrency.
- ConfigureAwait(false) is partially for eeking out additional perf, which we don't care about in tests, and it adds clutter that's only worthwhile if it brings value.
- We shouldn't go out of our way in test code to enable other test code to block artificially and introduce deadlock potential.
There was a problem hiding this comment.
This is really good information about testing and ConfigureAwait(). I don't think the rest of the CoreFx community knows this. It would be good to distribute this information more broadly.
cc: @Caesar1995 @rmkerr
| // Read the authorization header from client. | ||
| AuthenticationProtocols protocol = AuthenticationProtocols.None; | ||
| string clientResponse = null; | ||
| while (!string.IsNullOrEmpty(line = await reader.ReadLineAsync().ConfigureAwait(false))) |
There was a problem hiding this comment.
It's ok for now, but related to @geoffkizer's refactoring, we may just want to have a method "ReadHeaders" that returns the list of headers sent by the client. Then code like this can just process the array or dictionary of headers rather than interleaving request processing with test logic.
There was a problem hiding this comment.
And now that I'm reviewing Geoff's PR, I see exactly that:
https://github.com/dotnet/corefx/pull/27088/files#diff-fd50cecf004b510497450abfa7956a09R189
| // Username is a quoted string. | ||
| int startIndex = trimmedValue.IndexOf('"') + 1; | ||
|
|
||
| if (startIndex != -1) |
There was a problem hiding this comment.
I don't understand this logic. In what circumstance will startIndex ever be -1? For that to happen, IndexOf would need to be returning -2, which it will never do.
Same comment for all other occurrences of this below.
There was a problem hiding this comment.
Oh that's old code, I forgot to change this when i made startindex point to the valid char to be taken.
|
@Priya91, where did we land with #26979 (comment)? It looks like these tests are still SocketsHttpHandler-specific. If SocketsHttpHandler is passing the tests but WinHttpHandler, CurlHandler, and desktop's handler are not, and for something as long-standing as digest auth, that makes me think that the problem is actually with the test and SocketsHttpHandler jointly doing the wrong thing rather than it being a bug in WinHttpHandler, CurlHandler, and desktop. I could be wrong, of course, but it's suspect. |
|
@stephentoub I ran the tests only against winhttphandler, with that all the basic test cases passed, but for digest, the winhttphandler threw exception saying unrecognized message from server. From working on auth stuff in NTAuthentication, I found that the NTAuthentication class supports only WDigest, and it is more likely that the win apis, dont support digest but only Wdigest. @davidsh Can confirm.. I'll enable these tests for CurlHandler, and check the results. |
"Wdigest" is digest auth. The name of the binary in Windows that supports the Digest auth scheme is called Wdigest.dll. There are not separate "Wdigest" vs. "digest" protocols. https://technet.microsoft.com/en-us/library/cc778868(v=ws.10).aspx Digest can be disabled on the Windows box. And it frequently is by default. That might be why it is not working in your tests. See this for background on digest auth and why it might be disabled at times: |
|
@stephentoub @geoffkizer Can you please take a look, i refactored code per the latest changes in Loopback server. |
|
@geoffkizer locally I get test failures in sockethttphandler with assertion fail with httpconnectionpool.. |
|
Which tests are you seeing the assert with? It probably means that the server is incorrectly sending an extra CRLF. |
| case 504: return "Gateway Timeout"; | ||
| case 505: return "Http Version Not Supported"; | ||
| case 507: return "Insufficient Storage"; | ||
| case 100: |
There was a problem hiding this comment.
No, it was not, VS automatically formats it this way, it's a pain to fight against it everytime, and revert the formatting. Prefer to keep it to the default VS way.
| content; | ||
| $"Content-Length: {(content == null ? 0 : content.Length)}\r\n" + | ||
| additionalHeaders + "\r\n" + | ||
| content + "\r\n"; |
There was a problem hiding this comment.
"\r\n" after content is wrong. That's probably why you're seeing the asserts.
Also, I prefer seeing the "\r\n" after additionalHeaders on a separate line. This \r\n is actually the empty line to terminate the headers. When they are on the same line, it looks like its the line terminator for additonalHeaders, which is not right.
There was a problem hiding this comment.
Hmm, i assumed the additionalheaders didn't have to specify \r\n. Will revert this part.
| return lines; | ||
| } | ||
|
|
||
| public async Task<string> ReadRequestContentAsync() |
There was a problem hiding this comment.
I'm not following this method. It looks like you find the Content-Length, skip the next line, and then read whatever is after this until you get a new line. Isn't this going to produce weird results in many cases?
There was a problem hiding this comment.
I assumed the stuff after content-length was content, seems like that is wrong. Will revert this.
| await connection.ReadRequestHeaderAndSendResponseAsync(HttpStatusCode.Unauthorized, authenticateHeaders); | ||
|
|
||
| lines = await connection.ReadRequestHeaderAsync(); | ||
| if (lines.Count == 0) |
| return lines; | ||
| } | ||
|
|
||
| public async Task<List<string>> AcceptConnectionPerformAuthenticationAndCloseAsync(string authenticateHeaders) |
There was a problem hiding this comment.
Can we move this code, and the related code like Is*AuthTokenValid, somewhere other than LoopbackServer? Maybe AuthTestHelpers.cs or something like that?
I'd like to keep LoopbackServer itself minimal.
|
Beyond the scope of this PR, but: Can we largely reuse all of this for proxy auth as well? Seems like with a few tweaks to parameterize tests, we could reuse pretty much all of this. |
|
yes, I was planing to do that @geoffkizer once this is in and stable. (so as moving proxy tests to loopback as you asked) |
|
Great, thanks @wfurt. |
|
I'm happy with this PR. Is it ready to merge? @stephentoub are you happy? |
|
Yes, it's ready to merge, there's the tracking linked issue #27113 to resolve the differences on handlers and within OSes.. |
Add test infra for auth testing. Commit migrated from dotnet/corefx@ff123e8
7 participants
fixes #26375
@dotnet/ncl