Add GraphQL.Server.Authorization.AspNetCore NuGet package #171
Conversation
|
Might wanna just update |
|
Done |
|
Nice. I guess we see what @pekkah and @joemcbride say 👍 |
|
|
||
| <ItemGroup Label="Package References"> | ||
| <PackageReference Include="GraphQL" Version="2.0.0" /> | ||
| <PackageReference Include="Microsoft.AspNetCore.Authorization" Version="2.1.0" /> |
There was a problem hiding this comment.
The other projects in this repository are currently referencing 2.0.x releases of the ASP.NET Core packages. I think this should be consistent with those.
| } | ||
|
|
||
| public static List<string> GetPolicies(this IProvideMetadata type) => | ||
| type.GetMetadata(PolicyKey, new List<string>()); |
There was a problem hiding this comment.
This is going to create a new List<string> every time this method is called. We should just let it default to null.
|
|
||
| public INodeVisitor Validate(ValidationContext context) | ||
| { | ||
| var userContext = context.UserContext as IProvideClaimsPrincipal; |
There was a problem hiding this comment.
Since this is always going to be in ASP.NET Core, why not inject IHttpContextAccessor and use that to get HttpContext.User?
Then there's no special requirement for the UserContext.
There was a problem hiding this comment.
Done. Also removed the IProvideClaimsPrincipal interface as I think it's no longer required.
| public static IGraphQLBuilder AddGraphQLAuthorization(this IGraphQLBuilder builder) | ||
| { | ||
| builder | ||
| .Services |
There was a problem hiding this comment.
We should add this line in these methods to make sure IHttpContextAccessor is registered. It is not by default.
builder.Services.TryAddSingleton<IHttpContextAccessor, HttpContextAccessor>();
|
It should be noted that Ideally I've never written any custom validators for fields apart from this one but it doesn't exactly look simple. Perhaps there should be some abstractions provided to make it simpler. |
Probably won't be adding a direct reference to that anytime soon since I could see wanting to pass a ClaimsPrincipal though and provide a way to retrieve it. Then the end user of the framework can decide how to fetch it. |
|
Woot. Nice one. Can someone bump this once it's released to nuget? Going to swap over to this when I can 👍 |
|
Any word on this? Doesn't seem to be a new nuget release of current package or any new packages that I can see. Currently running a copy of the proj in my sln that I'd love to clean out :) |
5 participants
The current Authorization NuGet package duplicates code from
Microsoft.AspNetCore.Authorizationto work around some issues with .NET 4.6 and binding redirects. This causes all kinds of problems for ASP.NET Core users because the API has been slightly changed.I initially submitted a PR graphql-dotnet/authorization#13 to make use of
Microsoft.AspNetCore.Authorizationdirectly instead in the Authorization repo. After a long discussion in graphql-dotnet/authorization#11 with @joemcbride and others it was decided that this should be a separate project.Some changes were made in the authorization repo by @joemcbride to support argument fields. I have updated the code here to reflect that. This PR is mostly copying the code from the authorization repo but adding in ASP.NET Core idioms. It also copies across calls to
.GetAwaiter().GetResult()which is bad but fixing this requires changes to the core GraphQL project (not looked at how to fix that in too much detail yet) which I think can be done in a separate PR.