Duplication with ASP.NET Core about authorization HOT 40 CLOSED

Hi,
I also landed on this issue thinking the same thing. I'm using only asp.net core and there is already an available system for authorization policies, so I don't want to have to register another set of policies that work similarly but not exactly the same as my existing policies. I was considering forking and modifying this project for use specifically in AspNet Core so it uses the same authorization policies as used elsewhere in my apps.

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.
That might be cleaner than trying to make this one project work differently for different frameworks. This one already works for other versions of aspnet, but it would be best for asp.net core to have a different implementation. Whether it should all be in the same project with ifdefs I guess really depends on how much code in common there would be between the different implementations. This doesn't look like a huge code base to begin with.

from authorization.

This is on purpose. The core GraphQL library is server agnostic. This is also server agnostic.

from authorization.

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.

from authorization.

So the intention is that this package can be used without the GraphQL.Server NuGet 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.

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?

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.

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.

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.

from authorization.

This example does use the GraphQL.Server.Transports.AspNetCore library.

https://github.com/graphql-dotnet/examples/tree/master/src/AspNetCore

from authorization.

Microsoft.AspNetCore.Authorization follows .NET Standard 2.0, so you could also use it with full framework 4.6.1 and above.

from authorization.

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.

from authorization.

Understood.

Would you accept a PR that used Microsoft.AspNetCore.Authorization for .NET standard and the duplicate code for .NET full framework? In the past when .NET Core had less API surface area, it was common to use #if NETSTANDARD_2_0 pre-processor directives to support both.

from authorization.

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.

from authorization.

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?

from authorization.

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.

from authorization.

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.

from authorization.

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.

from authorization.

Perhaps it should be configurable using an IOptions<T>, that way you could use IHostingEnvironment if you wanted?

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.

from authorization.

Currently it is exposing role names in the reason message which I would not want to disclose in production.
Just tested and I get a 204 response for error and 200 for success.

from authorization.

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.

from authorization.

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.
Note I'm using Blazor for the client so deserializing the response into a C# model.

from authorization.

You can use the existence of an error property in the JSON to detect an error. I'm not sure what the official GraphQL spec says, but GraphQL.NET recently fixed a "bug" in which they stopped returning 400 Bad Request for an invalid GraphQL request and instead returned 200 OK.

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 IOptions<T> to provide an extensibility point or register some kind of IAuthorizationRequirementErrorProvider<ClaimsAuthorizationRequirement> with IOC. Then I thought it was overkill.

from authorization.

Oops I was wrong, I'm getting 200 for error. That makes it hard to know how to handle the response.

from authorization.

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:

protected override async Task OnInitAsync()
{
    var query = new GraphQLRequest
    {
        Query = @"
        {
          siteList {
            id,
            aliasId,
            siteName,
            siteFolderName
          }
        }"
    };
    if (oidc.CurrentUser != null)
    {
        Http.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", oidc.CurrentUser.Access_Token);
    }

    var queryJson = Json.Serialize(query);
    var endPoint = await AppSettings.Get("graphqlApiUri");
    var response = await Http.PostAsync(endPoint, new StringContent(queryJson, Encoding.UTF8, "application/json"));
    response.EnsureSuccessStatusCode();
    var result = await response.Content.ReadAsStringAsync();
    if(response.StatusCode.Equals(200))
    {
        model = Json.Deserialize<SiteListModel>(result);
    }
    if (response.StatusCode.Equals(204))
    {
        errorModel = Json.Deserialize<ApiErrorModel>(result);
    }


}

from authorization.

as it is I guess I have to inspect the string to detect the error

from authorization.

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.

from authorization.

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.
Really this code is just proof of concept so far, I can tidy it up by writing my own helpers and extensions.

But yes, early days for sure. Still I find it compelling to write my client side code in C# and razor syntax.

from authorization.

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:

SiteListModel model = null;
ErrorModel errorModel = null;

protected override async Task OnInitAsync()
{
    var query = new GraphQLRequest
    {
        Query = @"
        {
          siteList {
            id,
            aliasId,
            siteName,
            siteFolderName
          }
        }"
    };
    if (oidc.CurrentUser != null)
    {
        Http.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", oidc.CurrentUser.Access_Token);
    }
    var endPoint = await AppSettings.Get("graphqlApiUri");

    var result = await Http.SendQuery<SiteListModel>(endPoint, query);
    if(result.SuccessResult != null)
    {
        model = result.SuccessResult;
    }
    else if(result.Errors != null)
    {
        errorModel = result.Errors;
    }
    
}

from authorization.

I would be fine with having two different projects, one specific to ASP.NET Core. There is a little too much #if in #13 for me to feel comfortable pulling it in.

from authorization.

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.

from authorization.

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 AddGraphQLAuthorization to the IGraphQLBuilder interface (I think that is what it was called).

from authorization.

Hi @RehanSaeed and @joemcbride

Yesterday I got my first subscription working but ran into an issue with the AuthorizationValidationRule

The problem is here:

public INodeVisitor Validate(ValidationContext context)
    {
        var userContext = context.UserContext as IProvideClaimsPrincipal;
        ...

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.

if (type == null || !type.RequiresAuthorization())
        {
            return;
        }

How can I make it get my UserContext there so I can implement a subscription that does require authorization?

from authorization.

I'm not sure, I haven't tried auth with subscriptions. I'd be interested in your solution.

from authorization.

I see in the server readme:

UserContext of your resolver will be type of MessageHandlingContext. You can access the properties including your actual UserContext by using the Get("UserContext") method. This will read the context from the properties of MessageHandlingContext. You can add any other properties as to the context in IOperationMessageListeners. See the sample for example of injecting ClaimsPrincipal.

But this doesn't seem to be true and I don't see that in the sample.
I guess I can try implementing an IOperationMessageListener and try to add it myself, but it seems like this should be built in according to the readme.

from authorization.

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

public class GraphQLUserContext : IProvideClaimsPrincipal
{
    public ClaimsPrincipal User { get; set; }
}

public class AddUserContextMessageListener : IOperationMessageListener
{
    public AddUserContextMessageListener(IHttpContextAccessor httpContextAccessor)
    {
        _httpContextAccessor = httpContextAccessor;
    }

    private IHttpContextAccessor _httpContextAccessor;

    public async Task BeforeHandleAsync(MessageHandlingContext context)
    {
        var httpContext = _httpContextAccessor.HttpContext;
        if(httpContext != null)
        {
            var userContext = new GraphQLUserContext
            {
                User = httpContext.User
            };
            // if the default auth scheme is cookies
            // users from remote clients that pass bearer token are not automatically authenticated
            if (!httpContext.User.Identity.IsAuthenticated)
            {
                var result = await httpContext.AuthenticateAsync("Bearer");
                if (result.Succeeded)
                {
                    userContext.User = result.Principal;
                }
            }
            context.Properties.TryAdd("UserContext", userContext);
           
        }
        
    }

    public Task HandleAsync(MessageHandlingContext context)
    {
        return Task.CompletedTask;
    }

    public Task AfterHandleAsync(MessageHandlingContext context)
    {
        return Task.CompletedTask;
    }
}

public INodeVisitor Validate(ValidationContext context)
    {
        var userContext = context.UserContext as IProvideClaimsPrincipal;

        if(userContext == null && context.UserContext is MessageHandlingContext)
        {
            var subscriptionContext = context.UserContext as MessageHandlingContext;
            userContext = subscriptionContext.Get<GraphQLUserContext>("UserContext") as IProvideClaimsPrincipal;
        }
...

from authorization.

I take back the part that it has to know the type of usercontext, it can get it just using the interface like this:

var userContext = context.UserContext as IProvideClaimsPrincipal;

        if(userContext == null && context.UserContext is MessageHandlingContext)
        {
            var subscriptionContext = context.UserContext as MessageHandlingContext;
            userContext = subscriptionContext.Get<IProvideClaimsPrincipal>("UserContext");
        }

using just the interface. But really the GraphQLUserContext is simple and it makes sense to have an implementation in the authorization project I think.

from authorization.

I think another project for asp.net core is good. In terms of adding it to the server repo so you can do AddGraphQLAuthorization from IGraphQLBuilder, I'm not so sure.

Like the aspnet guys do, we could have some kind of lightweight abstraction package. Would be weird to have a dependency to GraphQL.Server.Abstractions from GraphQL.Authorization.AspNetCore if you're not using the server, but since IGraphQLBuilder is essentially empty, maybe it could just go into GraphQL proper or a GraphQL.Abstractions?

Else we just create the extension method in our own projects or we create a GraphQL.Server.Transports.AuthenticatedAspNetCore sort of amalgamation package somewhere.

from authorization.

New PR raised in the server project at graphql-dotnet/server#171

from authorization.

Closing this as the PR in the server project has been merged.

from authorization.

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.

from authorization.

@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.

from authorization.

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:
services.AddGraphQL(options =>
{
options.EnableMetrics = true;
options.ExposeExceptions = _env.IsDevelopment();
}).AddGraphQLAuthorization(options =>
{
options.AddPolicy("Authorized", p => p.RequireAuthenticatedUser());
});
Where do we get executed the authorized attribute, When working with restful end points it get based and returns the results, I really appreciate if some one can share the solution.

In this issue I have seen AddUserContextMessageListener do we need to add this bit? please help

from authorization.

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 .AuthorizeWith on a field creation.

from authorization.

@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

#33

from authorization.