-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
Duplication with ASP.NET Core #11
Comments
This is on purpose. The core GraphQL library is server agnostic. This is also server agnostic. |
So the intention is that this package can be used without the GraphQL.Server NuGet package? The downsides to this approach is that it's extra code you have to maintain, things get out of sync as in this case some methods I've listed are missing and you miss out on code that people have already written. Surely, the most likely scenario for developers using GraphQL.NET is that they would host it using the GraphQL.Server NuGet packages in conjunction with ASP.NET Core? Is my assumption not true? I feel like my assumption is probably doubly true for people using the GraphQL.Authorization package. The GraphQL.NET documentation barely mentions GraphQL.Server and all of the examples assume that you are only using GraphQL on it's own. I'm not sure what the best solution is but certainly it would be useful to the community to give more of a focus to the server package. |
It can be used in combination with it or without it. This project just provides an Authorization layer for your Graph types. What transport server you're using does not matter.
The GraphQL project targets both .NET Core and .NET Classic. So providing a lib that only worked with ASP.NET Core would exclude those who would need a solution for .NET Classic.
I do not disagree, though this solution is built so you can use it with any server that is using the GraphQL project. That may be ASP.NET Core. That may be ASP.NET Web API or ASP.NET MVC Classic. That may be a TCP server. |
This example does use the https://github.com/graphql-dotnet/examples/tree/master/src/AspNetCore |
|
Have you tried to integrate a .NET Standard nuget into an existing ASP.NET MVC or WPF project? Its a mess. Especially if that project is using paket and has both NetStandard 1.x and NetStandard 2.x references. We have done so at my work. It took hours to resolve version conflicts and binding redirects due to the dependency chain. Other developers just gave up and I had to jump in to get it working. This duplication is fine by me to help avoid that. |
Understood. Would you accept a PR that used |
Raised a PR #13 with my ideas around this. Makes life for ASP.NET Core devs simpler while maintaining the existing code for .NET 4.6 devs. |
Hi, I understand that this library is intended to support more than just .net core. Maybe we can make a separate project/library that is specific to asp.net core and name it GraphQL.Authorization.AspNetCore. |
Hi @joeaudette, glad to see you working on GraphQL. Yes, perhaps splitting this into two projects is a cleaner approach. As you say there is not much code. You could pretty much take my PR and paste it into a new project. We may need to rebase it, as I think there has been a PR accepted since I submitted this one. What are your thoughts @joemcbride? |
Hi @RehanSaeed just looking at your branch now, it looks good to me and I see for aspnetcore the number of needed files is reduced a lot vs full framework. I think I will paste the files into a new project to try it out but hope it can be an official version one way or another once @joemcbride has a chance to review it. I think it does make sense to move it to a separate project so it can be a separate nuget since it doesn't have many files in common with the other implementation. |
Hi @joeaudette I just started looking into this as well. I'd love to hear a report back on how it goes creating your own project and pasting in the solution. |
Hi @RehanSaeed and @ciwchris I did copy the files into a new project and got it working yesterday. I have a repository here where I'm playing with GraphQL and Blazor. The authorization project is here Nice work by @RehanSaeed , the authorization project works great and there are only 5 files including the .csproj file. The only thing I was thinking about changing is the AuthorizationValidationRule class currently gives a lot of details about failed authorization including role names etc. I think we should inject IHostingEnvironment into that class and only give the details in dev environment, for produciton it is sufficient to say something generic like "User did not meet the authorization requirements" so as not to leak security details in production. I think it would be good to move the authorization project back into this repo as a separate project and then release a nuget. |
Perhaps it should be configurable using an I don't think it's a security concern to expose the reason authentication/authorization failed but I could be wrong. In GraphQL it's useful to know whether you failed authentication or authorization as you don't get 401 or 403 response codes as you would in a REST API, the error response in GraphQL is 200 OK I think. |
Currently it is exposing role names in the reason message which I would not want to disclose in production. |
In that case we could provide some kind of generic error message mode as you suggest. A 204 for error seems like a bug, since GraphQL provides a response body. |
I'm glad it returns a different response code for error otherwise I would try to deserialize the response into my model, this way I can deserialize it into a different model for error that matches the json response. So I don't see that as a bug but a feature. |
You can use the existence of an At the time I wrote this PR, I was also thinking that it would be nice to provide a way for a custom authorization requirement to provide some kind of error message, so we don't have to write so much code. I was thinking about using |
Oops I was wrong, I'm getting 200 for error. That makes it hard to know how to handle the response. |
See in my case I'm not dealing directly with the json, I'm deserializing it into a c# object in the blazor client. I would have preferred a different status code to know, ie code like this in blazor:
|
as it is I guess I have to inspect the string to detect the error |
While I think Blazor is super cool and would love to do more with it, this is one major reason I've been using Vue with Vue-Apollo. The GraphQL.NET HttpClient does nothing very useful and you have to hand code things as you've done. That said, it's early days, for Blazor and GraphQL.NET. |
I tried to use the GraphQLHttpClient in Blazor but got errors, I think it is using newtonsoft.json which doesn't currently work in Blazor but they have a more simple util for serialize/deserialize json in Blazor. But yes, early days for sure. Still I find it compelling to write my client side code in C# and razor syntax. |
I just cleaned up my code with an extension method to handle the boilerplate code for graphql api requests, so now it looks like this in Blazor:
|
I would be fine with having two different projects, one specific to ASP.NET Core. There is a little too much |
This is great! @RehanSaeed will you make a new pull request to add it as a separate project? Btw, I updated my copy to use IOptions of T to pass in a setting whether to include error details as you suggested. |
If this is now ASP.NET Core specific, I wonder if I should just submit the PR to the server repo where all the other ASP.NET Core stuff lives, then I can take a dependency on their core project and add a |
Hi @RehanSaeed and @joemcbride Yesterday I got my first subscription working but ran into an issue with the AuthorizationValidationRule The problem is here:
The IProvideClaimsPrincipal is null when there is a subscription request because the context.UserContext is a MessageHandlingContext I got it working only with a subscription that does not require authorization, and to get that working I had to modify the AuthorizeAsync method because it was throwing an error if userContext is null, I had to move this above the code that was throwing the error: so that if no authorization required it doesn't throw.
How can I make it get my UserContext there so I can implement a subscription that does require authorization? |
I'm not sure, I haven't tried auth with subscriptions. I'd be interested in your solution. |
I see in the server readme:
But this doesn't seem to be true and I don't see that in the sample. |
Ok, I did get this working. But it required adding the user context directly in the Authorization project because it has to know the type to get it from properties and I had to implement a IOperationMessageListener to add the usercontext to the properties of the MessageHandlingContext
|
I take back the part that it has to know the type of usercontext, it can get it just using the interface like this:
using just the interface. But really the GraphQLUserContext is simple and it makes sense to have an implementation in the authorization project I think. |
I think another project for asp.net core is good. In terms of adding it to the server repo so you can do Like the aspnet guys do, we could have some kind of lightweight abstraction package. Would be weird to have a dependency to Else we just create the extension method in our own projects or we create a |
New PR raised in the server project at graphql-dotnet/server#171 |
Closing this as the PR in the server project has been merged. |
Any chance of a nuget release? I'm struggling with this right now, as we have a lot of policies defined and it's a pain to recreate them. |
@BalassaMarton , I've pinged the maintainer on the server project PR. The PR has been merged back in Oct but the bits haven't been released to nuget yet. In the meantime, you can just drop the project files that were added over there into your local sln, uninstall this auth pkg and swap over. |
Guys, I'm trying to jwt bearer authentication, when I add authorization validation rule with default "Authorized" policy but getting Error: "You are not authorized to run this query. The current user must be authenticated." in Start up class: In this issue I have seen AddUserContextMessageListener do we need to add this bit? please help |
Have you also added the Startup requirements for the JWT parsing, etc. Auth0 has good examples on that if you're using it. Something like: https://auth0.com/blog/securing-asp-dot-net-core-2-applications-with-jwts/ Then you'd need to use that policy somewhere. I'm surprised that if you haven't done that it is restricting you, but basically add an |
@benmccallum with .net inbuilt authorize attribute under the hood it will cover the authenticateasync method and after adding that method to JwtTokenAuthorizationEvaluator in GraphQL and then register in startup with policy and able to use in graphql queries |
The following code looked a lot like ASP.NET Core to me but to my surprise it lived in the GraphQL namespace:
The
AuthorizationPolicyBuilder
in ASP.NET Core has methods forRequireAuthenticatedUser
andRequireAssertion
which can be very useful. I was considering submitting a PR for these perhaps this project should just be usingMicrosoft.AspNetCore.Authorization
in the first place?Thoughts @joemcbride?
The text was updated successfully, but these errors were encountered: