HttpSys => Auth 2.0 #354
HttpSys => Auth 2.0 #354
Conversation
| var auth = authentication.FirstOrDefault(); | ||
| if (auth == null) | ||
| { | ||
| throw new InvalidOperationException("AddAuthentication() is required to use Authentication."); |
There was a problem hiding this comment.
Should this no-op instead of throw?
There was a problem hiding this comment.
This is an interesting one. we know auth is on, and we have the user set on HttpContext.User. You don't really need the AuthN service unless you're mixing windows and anonymous access. How about throwing if AuthenticationSchemes != None && AllowAnonymous? That way you don't need the AuthN service for an app that handles AuthZ at the server level.
| get { return _authHandler; } | ||
| set { _authHandler = value; } | ||
| } | ||
| [Obsolete("See https://go.microsoft.com/fwlink/?linkid=845470")] |
There was a problem hiding this comment.
The obsolete is a bit overkill for an internal property that's implementing an obsolete interface property.
| var auth = authentication.FirstOrDefault(); | ||
| if (auth == null) | ||
| { | ||
| throw new InvalidOperationException("AddAuthentication() is required to use Authentication."); |
There was a problem hiding this comment.
This is an interesting one. we know auth is on, and we have the user set on HttpContext.User. You don't really need the AuthN service unless you're mixing windows and anonymous access. How about throwing if AuthenticationSchemes != None && AllowAnonymous? That way you don't need the AuthN service for an app that handles AuthZ at the server level.
| @@ -26,8 +26,7 @@ public class AuthenticationTests | |||
| [InlineData(AuthenticationSchemes.Negotiate | AuthenticationSchemes.NTLM | /*AuthenticationSchemes.Digest |*/ AuthenticationSchemes.Basic)] | |||
| public async Task AuthTypes_AllowAnonymous_NoChallenge(AuthenticationSchemes authType) | |||
| { | |||
| string address; | |||
| using (Utilities.CreateHttpAuthServer(authType, AllowAnoymous, out address, httpContext => | |||
| using (var server = Utilities.CreateDynamicHost(string.Empty, authType, AllowAnoymous, out var address, out var baseAddress, httpContext => | |||
There was a problem hiding this comment.
Add an overload that doesn't return baseAddress? You're not using it in many (any?) of these tests.
There was a problem hiding this comment.
Nope, removed, also hardcoded the string.Empty basePath in this new overload since it wasn't being set anywhere either.
| @@ -316,20 +243,16 @@ public class AuthenticationTests | |||
| // [InlineData(AuthenticationSchemes.Digest)] | |||
| [InlineData(AuthenticationSchemes.Basic)] | |||
| [InlineData(AuthenticationSchemes.Negotiate | AuthenticationSchemes.NTLM | /*AuthenticationSchemes.Digest |*/ AuthenticationSchemes.Basic)] | |||
| [FrameworkSkipCondition(RuntimeFrameworks.CoreCLR, SkipReason = "HttpClientHandler issue (https://github.com/dotnet/corefx/issues/5045).")] | |||
| //[FrameworkSkipCondition(RuntimeFrameworks.CoreCLR, SkipReason = "HttpClientHandler issue (https://github.com/dotnet/corefx/issues/5045).")] | |||
| { | ||
| context.NotAuthenticated(); | ||
| return Task.FromResult(AuthenticateResult.Success(new AuthenticationTicket(_requestContext.User, properties: null, authenticationScheme: _scheme.Name))); |
There was a problem hiding this comment.
This says the scheme is "Negotiate", but you've registered "Windows". With a combine handler you can skip ListEnabledAuthSchemes and just always return the authenticated user.
| var identity = (ClaimsIdentity)_requestContext.User?.Identity; | ||
| if (identity != null && identity.IsAuthenticated | ||
| && (automaticChallenge || string.Equals(identity.AuthenticationType, context.AuthenticationScheme, StringComparison.Ordinal))) | ||
| _customChallenges |= scheme; |
There was a problem hiding this comment.
_customChallenges = _authSchemes, you don't have any granularity now.
| var identity = (ClaimsIdentity)_requestContext.User?.Identity; | ||
| if (identity != null && identity.IsAuthenticated) | ||
| { | ||
| foreach (var scheme in ListEnabledAuthSchemes()) |
There was a problem hiding this comment.
You can remove this, it will always find a match.
| else | ||
| { | ||
| _requestContext.Response.StatusCode = 401; | ||
| foreach (var scheme in ListEnabledAuthSchemes()) |
| { "AuthenticationScheme", authenticationScheme }, | ||
| }; | ||
| } | ||
|
|
||
| private IEnumerable<AuthenticationSchemes> ListEnabledAuthSchemes() |
There was a problem hiding this comment.
I don't think you need ListEnabledAuthSchemes anywhere now that you only have one handler for all schemes.
|
The auth handler is actually pretty clean now |
| context.NotAuthenticated(); | ||
| } | ||
| } | ||
| return Task.FromResult(AuthenticateResult.Success(new AuthenticationTicket(_requestContext.User, properties: null, authenticationScheme: _scheme.Name))); |
There was a problem hiding this comment.
Since HttpSys pre-sets HttpContext.User, will AuthZ try to merge this Principal with itself?
There was a problem hiding this comment.
Merging only happens when multiple schemes are requested for a policy, it doesn't actually merge with anything that already is on context.User either.
| @@ -12,6 +12,7 @@ | |||
| </PropertyGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <PackageReference Include="Microsoft.AspNetCore.Authentication.Core" Version="$(AspNetCoreVersion)" /> | |||
There was a problem hiding this comment.
Why Core and not Abstractions?
There was a problem hiding this comment.
Moot now that we are going to always AddAuthenticationCore
|
Updated, tests are passing (but quite a few are skipped due to the HttpHandler issue |
| @@ -258,23 +195,14 @@ public class AuthenticationTests | |||
| [InlineData(AuthenticationSchemes.Negotiate | AuthenticationSchemes.NTLM | /*AuthenticationSchemes.Digest |*/ AuthenticationSchemes.Basic)] | |||
| public async Task AuthTypes_AuthenticateWithUser_OneResult(AuthenticationSchemes authType) | |||
| { | |||
| string address; | |||
| var authTypeList = authType.ToString().Split(new char[] { ',', ' ' }, StringSplitOptions.RemoveEmptyEntries); | |||
There was a problem hiding this comment.
authTypeList is no longer used
| @@ -314,17 +241,13 @@ public class AuthenticationTests | |||
| [InlineData(AuthenticationSchemes.Negotiate | AuthenticationSchemes.NTLM | /*AuthenticationSchemes.Digest |*/ AuthenticationSchemes.Basic)] | |||
| public async Task AuthTypes_ChallengeWithAllAuthTypes_AllChallengesSent(AuthenticationSchemes authType) | |||
| { | |||
| string address; | |||
| var authTypeList = authType.ToString().Split(new char[] { ',', ' ' }, StringSplitOptions.RemoveEmptyEntries); | |||
There was a problem hiding this comment.
authTypeList is no longer used
There was a problem hiding this comment.
This one we still use to check the count.
| @@ -340,45 +263,44 @@ public class AuthenticationTests | |||
| [InlineData(AuthenticationSchemes.Basic)] | |||
| public async Task AuthTypes_ChallengeOneAuthType_OneChallengeSent(AuthenticationSchemes authType) | |||
There was a problem hiding this comment.
The input param is no longer used
| @@ -19,7 +20,7 @@ public void OverridingDirectConfigurationWithIServerAddressesFeatureSucceeds() | |||
| var serverAddress = "http://localhost:11001/"; | |||
| var overrideAddress = "http://localhost:11002/"; | |||
|
|
|||
| using (var server = new MessagePump(Options.Create(new HttpSysOptions()), new LoggerFactory())) | |||
| using (var server = new MessagePump(Options.Create(new HttpSysOptions()), new LoggerFactory(), new AuthenticationSchemeProvider(Options.Create(new AuthenticationOptions())))) | |||
There was a problem hiding this comment.
Sure, moved into Utilities.CreatePump static
@Tratcher replaces: #325
Something weird is still going on where the challenge functional tests only fail on core, but pass on 46.