-
Notifications
You must be signed in to change notification settings - Fork 561
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable basic auth tests with domainless custom authenticator #1371
Conversation
e4b9e4c
to
79c1f08
Compare
These tests are failing on *nix with the error:
The SSL_Available ConditionalFact was used for all tests that failed with this kind of exception in the past, pending deeper investigation of what is required for automated installation scripts. |
@@ -24,20 +27,18 @@ static Https_ClientCredentialTypeTests() | |||
#if FULLXUNIT_NOTSUPPORTED | |||
[Fact] | |||
#else | |||
[ConditionalFact(nameof(Root_Certificate_Installed), nameof(Basic_Authentication_Available))] | |||
[ConditionalFact(nameof(Root_Certificate_Installed))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove this ConditionalFact? Do we know Basic_Authentication_Available is true for all platforms, both self-host and IIS-host?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The the whole point of this change, it enables Basic authentication by using a custom authenticator on the server side so you don't need to use real credentials to test it. As there's no such thing as ambient credentials with Basic authentication, there's no need to have a test variant which uses a real domain.
@StephenBonikowsky is doing a massive sweep to "version-ize" the tests in PR #1366. I think it would be most efficient if we allowed his PR to be completed and then re-apply the changes in this PR. My own PeerTrust branch is also waiting for that PR to finish. |
@iamjasonp, do you know why this test is getting this exception: |
I wouldn't expect that to pass - root certs aren't installed on CI machines. I'm mildly surprised that Root_Certificate_Installed is evaluating to true. |
Ah, I see - note that CA certificates being used by cURL does not use the x509 stores. So while Root_Certificate_Installed is evaluating to true (because certs are being installed fine in the .NET x509 cert stores), https calls won't work because they depend on the system cert store. |
#1304 will help with getting these tests running in Helix/CI |
@dotnet-bot test all outerloop |
@dotnet-bot test all outerloop |
If the default platforms (Windows_NT, Ubuntu14.04, CentOS7.1, OSX) pass, don't worry about the remaining outerloop CI failures for the new platforms we turned on in CI |
Windows_NT Debug failure was unrelated so merging. |
Fixes #1111