Update MVC to Auth 2.0 #6131
Update MVC to Auth 2.0 #6131
Conversation
| @@ -103,7 +103,7 @@ public ChallengeResult(IList<string> authenticationSchemes, AuthenticationProper | |||
|
|
|||
| logger.ChallengeResultExecuting(AuthenticationSchemes); | |||
|
|
|||
| var authentication = context.HttpContext.Authentication; | |||
| var authentication = context.HttpContext; | |||
There was a problem hiding this comment.
Rename this or get rid of the variable
| @@ -103,7 +103,7 @@ public ForbidResult(IList<string> authenticationSchemes, AuthenticationPropertie | |||
|
|
|||
| logger.ForbidResultExecuting(AuthenticationSchemes); | |||
|
|
|||
| var authentication = context.HttpContext.Authentication; | |||
| var authentication = context.HttpContext; | |||
There was a problem hiding this comment.
Rename this or get rid of the variable
| @@ -114,7 +114,8 @@ public ForbidResult(IList<string> authenticationSchemes, AuthenticationPropertie | |||
| } | |||
| else | |||
| { | |||
| await authentication.ForbidAsync(Properties); | |||
| // TODO: switch to use sugar in: https://github.com/aspnet/HttpAbstractions/pull/815 | |||
| @@ -21,6 +21,7 @@ Microsoft.AspNetCore.Mvc.RouteAttribute</Description> | |||
| <ItemGroup> | |||
| <ProjectReference Include="..\Microsoft.AspNetCore.Mvc.Abstractions\Microsoft.AspNetCore.Mvc.Abstractions.csproj" /> | |||
|
|
|||
| <PackageReference Include="Microsoft.AspNetCore.Authentication.Core" Version="$(AspNetCoreVersion)" /> | |||
There was a problem hiding this comment.
Is this the right package? is there an abstractions package or something less implementationey?
There was a problem hiding this comment.
Yeah I can switch this to abstractions, but this does pose the question whether AddMVC should be calling AddAuthenticationCore which does live here, otherwise the core services will be missing if no auth was added (maybe ok?)
| @@ -88,7 +88,7 @@ public SignInResult(string authenticationScheme, ClaimsPrincipal principal, Auth | |||
|
|
|||
| logger.SignInResultExecuting(AuthenticationScheme, Principal); | |||
|
|
|||
| var authentication = context.HttpContext.Authentication; | |||
| var authentication = context.HttpContext; | |||
| @@ -106,7 +106,7 @@ public SignOutResult(IList<string> authenticationSchemes, AuthenticationProperti | |||
|
|
|||
| logger.SignOutResultExecuting(AuthenticationSchemes); | |||
|
|
|||
| var authentication = context.HttpContext.Authentication; | |||
| var authentication = context.HttpContext; | |||
| @@ -11,7 +11,7 @@ | |||
| <ProjectReference Include="..\..\..\src\Microsoft.AspNetCore.Mvc.Formatters.Xml\Microsoft.AspNetCore.Mvc.Formatters.Xml.csproj" /> | |||
| <ProjectReference Include="..\Microsoft.AspNetCore.Mvc.TestConfiguration\Microsoft.AspNetCore.Mvc.TestConfiguration.csproj" /> | |||
|
|
|||
| <PackageReference Include="Microsoft.AspNetCore.Authentication" Version="$(AspNetCoreVersion)" /> | |||
| <ProjectReference Include="..\..\..\..\Security\src\Microsoft.AspNetCore.Authentication\Microsoft.AspNetCore.Authentication.csproj" /> | |||
There was a problem hiding this comment.
uhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh
There was a problem hiding this comment.
Yeah you can ignore this, security and MVC will go in in the same push party, and I won't merge this file
| return Task.FromResult(AuthenticateResult.Success(new AuthenticationTicket(principal, | ||
| new AuthenticationProperties(), Options.AuthenticationScheme))); | ||
| new AuthenticationProperties(), Scheme.Name))); | ||
| } |
There was a problem hiding this comment.
Is this test still testing something meaningful?
There was a problem hiding this comment.
Well, its basically a Mock auth scheme for the functional auth filters test, so somewhat meaningful :)
|
This method needs to register the authorization services. If that means MVC Core has to depend on Auth Core, then so be it. Other than that changes look good 👍 |
|
Ok, added Core back to deps, and call AddAuthenticationCore next to AddAuthorization, will merge these changes in as part of the mega security PR push sometime next week |
No description provided.