graphql-dotnet / authorization

A toolset for authorizing access to graph types for GraphQL .NET.
MIT License
158 stars 38 forks source link

Is this library still relevant with GraphQL 7+ #253

Closed BillBaird closed 1 year ago

BillBaird commented 1 year ago

Is this library still relevant with GraphQL having moved to release 7 (currently 7.2.2)? It has not been updated, and if I update the GraphQL library references to 7.2.1 (current version), it no longer compiles as it appears some types have moved elsewhere.

This also implies that the examples for authorization no longer work since they have not been updated to 7+ either.

I'm updating from GraphQL 4.x to 7.x and this has become the one thorn in my side as I try to figure our what things should look like in 7.x.

Thanks, Bill

Shane32 commented 1 year ago

In my personal opinion, no; this is obsolete -- certainly when used with typical ASP.NET Core authentication. This repo may still be useful for custom authentication scenario, but by now it is lacking in features compared to GraphQL.NET Server v7, not to mention being incompatible.

See https://github.com/graphql-dotnet/server#authorization-configuration for details to set up authentication with GraphQL.NET Server v7.

The authentication rule in the server project now supports many features that this one does not:

GraphQL.NET Server v7 also supports:

However, input types and fields of input types are not validated.

Among other features, GraphQL.NET v7 also supports carrying the ClaimsPrincipal through IResolveFieldContext. So the validation rule present within the server repo does not actually rely on ASP.NET Core at all.

Shane32 commented 1 year ago

GraphQL.NET Server v7 even includes two samples of authentication -- identity and JWT-based -- see links at the bottom of the readme linked above.

BillBaird commented 1 year ago

Hello Shane,

Thanks for the confirmation that this library is not needed, and pointers to where to look. I had previously found most of this, but missed the Samples.JWT, which is closer to what I am doing.

That problem that I am running into is that I am using two different OAuth providers and was making heavy use of the .AuthorizeWith() (now .AuthorizeWithPolicy()) with different policies on different fields, some with a single policy, some with multiple policies, some with none (now .AllowAnonymous()). There is no policy that applies to everything, so I don't know that I should be using .AddAuthorizationRule() with Services.AddGraphQL. When I use it, it seems to fire on all fields. Trying to pare it down to a small reproducible example has proved to be a challenge.

The servers that I am upgrading all use FieldBuilders for specifying the schema. I know that shouldn't versus using .AddAutoSchema<Chat.Query ... as is done in Samples.JWT.

I've been able to run Samples.JWT by creating a minimal client (using the same JWT password), including importing the JwtHelper and SecurityKeyType classes from Samples.Jwt. Here is my client (yes, it uses the older GraphQL.Client, but that shouldn't matter). It would be nice if the Samples came with Clients that could be used to tinker with them.

using var graphQLClient = new GraphQLHttpClient("https://localhost:5001/graphql", new NewtonsoftJsonSerializer());

var password = "q09+6LuMZsaRCQ2J80I+rtaJCbmL9+Cj+5kt3AbRQwY=";
JwtHelper.Instance = new(password, SecurityKeyType.SymmetricSecurityKey);

var (exp, accessToken) = JwtHelper.Instance.CreateSignedToken(new Claim("role", "Administrator"));

graphQLClient.HttpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", accessToken);

var request = new GraphQLRequest
{
    Query = @"query JwtTest{count}",
    OperationName = "JwtTest"
};
var graphQLResponse = await graphQLClient.SendQueryAsync<dynamic>(request);
if (graphQLResponse.Errors != null)
    Console.WriteLine(graphQLResponse.Errors[0].Message);
else
    Console.WriteLine(graphQLResponse.Data);

This appears to work correctly. Now to try to set different IValidationRule-based policies on different query fields.

If I'm unable to get things working with different/multiple policies implementing IValidationRule and AuthorizationHandler<...> applied to different queries (fields) I'll respond, or if I should move this elsewhere, let me know.

Thanks, Bill

Shane32 commented 1 year ago

Can you provide a sample of your Startup showing a couple snippets of how you set up GraphQL, the policies, authorization handler, and authorization-related stuff? Perhaps also a sample GraphQL query, with comments showing which fields or types have metadata. Then I can explain how the authentication rule functions and be sure it lines up to how you are using it.

Shane32 commented 1 year ago

In general:

Also, AllowAnonymous is an exception rule for a FIELD, which exempts the parent type from needing to meet its requirements. It only applies when (a) there are anonymous fields defined on a graph, and (b) only those anonymous fields were selected. It also does not follow up the tree any further, but only exempts the type that the field was defined on.

Authorization requirements will start at the schema, then execute for the type, then the field, then the field's type, and then if the field type is an object type, the child field, and so on. Fragments are honored.

Sample request:


                                 # checks schema requirements
query sample {                   # checks QueryGraphType requirements
  widget (id: "3") {             # checks 'widget' field
                                 # checks 'id' argument on 'widget' field
                                 # does not check IdGraphType used by the id field, as this is used as an input type
                                 # checks 'Widget' type
    id                           # checks 'id' field on the Widget type
                                 # checks the IdGraphType used by the id field on the Widget type
    name                         # checks 'name' field on the Widget type
                                 # checks the StringGraphType
  }
}
Shane32 commented 1 year ago

Just FYI you can review the logic for validating authorization requirements here:

https://github.com/graphql-dotnet/server/blob/master/src/Transports.AspNetCore/AuthorizationHelper.cs

BillBaird commented 1 year ago

So I think part of my confusion is what the IValidationRule interface is used for, as well as where to register things.

I've created a simple implementation of IValidationRule:

public virtual ValueTask<INodeVisitor?> ValidateAsync(ValidationContext context)
{
    var user = context.User
        ?? throw new InvalidOperationException("User could not be retrieved from ValidationContext. Please be sure it is set in ExecutionOptions.User.");

    var claim = user.Claims.FirstOrDefault(c => c.Type == "atLeast5Claim");
    int val = 0;
    if (claim != null)
        _ = int.TryParse(claim.Value, out val);

    if (claim == null || val < 5)
        context.ReportError(new AccessDeniedError(context.Document.Source.Span.ToString(), context.Document.Source));

    return default;
}

Then, I use .AddValidationRule() in service.AddGraphQL.

builder.Services.AddGraphQL(b => b
    .AddAutoSchema<Chat.Query>(s => s
        .WithMutation<Chat.Mutation>()
        .WithSubscription<Chat.Subscription>())
    .AddSystemTextJson()
    // support authorization policies within the schema (although none are set in this sample)
    .AddAuthorizationRule()
    .AddValidationRule<AtLeast5Policy>()

I decorated the Chat Query class:

[Authorize("AtLeast5Policy")]
[Authorize("Policy2")]
public static Message? LastMessage([FromServices] IChat chatService)
    => chatService.LastMessage;

public static IEnumerable<Message> AllMessages([FromServices] IChat chatService, string? from = null)
    => from == null ? chatService.GetAllMessages() : chatService.GetMessageFromUser(from);

[Authorize("AtLeast5Policy")]
public static int Count([FromServices] IChat chatService)
    => chatService.Count;

When I call query{count} with the token CreateSignedToken(new Claim("role", "Administrator"), new Claim("atLeast5Claim", "4")), I get the behavior I expect (AccessDenied). Changing the atLeast5Claim to a value of 5, it works properly. All good so far.

However, when I call query JwtTest{allMessages{message}} with the claim set to 4, I get an Access denied for query JwtTest{allMessages{message}} even though it is not declared on the AllMessages query. If I set a breakpoint in the AtLeast5Policy implementation, sure enough, it gets called when I did not expect it to be.

This leads me to believe that any IValidationRule added with .AddValidationRule() gets executed on every call (along with the 23 predefined validation rules). I can see why this makes sense to use for AuthorizationValidationRule which is added by .AddAuthorizationRule() since it is intended to run on everything.

So if IValidationRule runs on everything, how can I check authorization based on different policies. Is this where I should be using something like

builder.Services.AddAuthorization(opts =>
    opts.AddPolicy("AtLeast5Policy", p => p.AddRequirements(new AtLeast5Policy()))
);

and implement IAuthorizationRequirement rather than IValidationRule?

Again, my goal is to have different authenticators used on different queries (including none or multiple), where those authenticators could be simple (contains a role, for instance), or more complex such my AtLeast5Policy.

I've got to break away likely until tomorrow. Thanks in advance for helping demystify this.

Shane32 commented 1 year ago

This leads me to believe that any IValidationRule added with .AddValidationRule() gets executed on every call (along with the 23 predefined validation rules). I can see why this makes sense to use for AuthorizationValidationRule which is added by .AddAuthorizationRule() since it is intended to run on everything.

Correct. IValidationRule implementations are run on every document regardless of any [Authorize] tags. This really isn't what you're looking for. The code to step through a document looking for authorization metadata, match it up to the schema, follow fragments and handle AllowAnonymous is quite complex.

and implement IAuthorizationRequirement rather than IValidationRule?

Yes. You should not need to implement IValidationRule at all, or call .AddValidationRule<AtLeast5Policy>().

If you find that implementing IAuthorizationRequirement doesn't work for you, and you truly need custom code to execute based on the presence of [Authorize] tags in the schema (without communicating with ASP.NET Core authentication services at all), then what you want to do is to this:

  1. Create a new class that derives from AuthorizationVisitorBase (or if convenient AuthorizationVisitor)

  2. Override AuthorizeAsync, IsInRole and IsAuthenticated methods of this new class. See https://github.com/graphql-dotnet/server/blob/master/src/Transports.AspNetCore/AuthorizationVisitor.cs for the reference implementation (also shown below).

public class MyAuthorizationVisitor : AuthorizationVisitorBase
{
    public AuthorizationVisitor(ValidationContext context, ClaimsPrincipal claimsPrincipal, IAuthorizationService authorizationService)
        : base(context)
    {
        if (context == null) throw new ArgumentNullException(nameof(context));
        ClaimsPrincipal = claimsPrincipal ?? throw new ArgumentNullException(nameof(claimsPrincipal));
        AuthorizationService = authorizationService ?? throw new ArgumentNullException(nameof(authorizationService));
    }
    protected ClaimsPrincipal ClaimsPrincipal { get; }
    protected IAuthorizationService AuthorizationService { get; }
    protected override bool IsAuthenticated => ClaimsPrincipal.Identity?.IsAuthenticated ?? false;
    protected override bool IsInRole(string role) => ClaimsPrincipal.IsInRole(role);
    protected override ValueTask<AuthorizationResult> AuthorizeAsync(string policy) => new(AuthorizationService.AuthorizeAsync(ClaimsPrincipal, policy));
}
  1. Clone AuthorizationValidationRule, creating a new validation rule that uses the new visitor you just created: (see https://github.com/graphql-dotnet/server/blob/master/src/Transports.AspNetCore/AuthorizationValidationRule.cs )
public class MyAuthorizationValidationRule : IValidationRule
{
    /// <inheritdoc/>
    public virtual async ValueTask<INodeVisitor?> ValidateAsync(ValidationContext context)
    {
        var user = context.User
            ?? throw new InvalidOperationException("User could not be retrieved from ValidationContext. Please be sure it is set in ExecutionOptions.User.");
        var provider = context.RequestServices
            ?? throw new MissingRequestServicesException();
        var authService = provider.GetService<IAuthorizationService>()
            ?? throw new InvalidOperationException("An instance of IAuthorizationService could not be pulled from the dependency injection framework.");

        var visitor = new MyAuthorizationVisitor(context, user, authService);
        // if the schema fails authentication, report the error and do not perform any additional authorization checks.
        return await visitor.ValidateSchemaAsync(context) ? visitor : null;
    }
}
  1. Remove AddAuthorizationRule()

  2. Add AddValidationRule<MyAuthorizationValidationRule>()

In this manner you can write a completely custom implementation of any authorization logic, while still honoring the semantics of [Authorize] and [AllowAnonymous] that were previously described. You can use the validation rule's constructor to pull in singleton services from your DI provider, or use context.RequestServices to pull in scoped services.

Also, I notice you wrote this in your sample:

[Authorize("AtLeast5Policy")]
[Authorize("Policy2")]
public static Message? LastMessage([FromServices] IChat chatService)
    => chatService.LastMessage;

You cannot set two different policies on the same field/type. You will need to define a new policy which contains the combined requirements of both policies. This is true even if you choose to write a completely custom authorization implementation as described above. The second [Authorize] attribute will overwrite the metadata previously applied to the graph type or field by the first attribute.

Shane32 commented 1 year ago

One last thought: if you really need multiple policy support, you could use the above code snippets to create a custom AuthorizationVisitor, and only make one minor change: split the policy name as if it was a comma-separated list, and then process each of the policies, only returning successful if they all are successful. You would need to add the attribute in this manner: [Authorize("AtLeast5Policy,Policy2")]. This would remain compatible with the ASP.NET Core authorization framework, where you would still define the requirements of each policy within the call to builder.Services.AddAuthorization(). But now it would support multiple policies for a single graph type or field.

Shane32 commented 1 year ago

I would suggest reading the documentation for ASP.NET Core's authorization framework regarding policy-based authorization:

https://learn.microsoft.com/en-us/aspnet/core/security/authorization/policies?view=aspnetcore-7.0

It is not necessary to use it if you wish to implement custom authorization code as shown previously, but if you want to tie your authorization code into the rest of ASP.NET Core (for example to add an authorization policy to a webhook or legacy API), this may be the documentation you are looking for.

BillBaird commented 1 year ago

Thank you Shane, your guidance has been very helpful. I've played with using my own AuthorizationValidationRule and AuthorizationVisitor and can see how I will be approaching this. Your comments about splitting on a name that had multiple policies gave me an idea that will allow me to deal with an issue I've been mulling over for months.

In program startup, I will be registering (my own registration for use by the InstanceVisitor) each policy name along with a singleton instance of its implementing type. These classes will take the policy name in the constructor and the name/singleton pair will be added to a singleton dictionary. My InstanceVisitor will then be able to pull the implementing class from the dictionary. This will obviously save on having to manage a switch statement every time a new policy is added.

Some policies will be primitives, such as checking that a user is authenticated with a certain provider or a claim meets a certain condition. Others will be instances of an expression policy based on parsing the name on instance creation using a grammar and a tool like Antlr4. That will allow me to create policy names like "(AuthenticatedWithProvider1 and IsAdmin) or AuthenticatedWithProvider2". Assuming that the three names in the policy name expression have primitive implementations then the expression policy can be found by the InstanceVisitor and the ExpressionPolicy (which was initialized with the expression name) can execute the expression, in turn calling the primitive policies as necessary. This will also need to manage errors as if AuthenticatedWithProvider1 fails it would have generated an error, but the "or" logic can throw those errors away if AuthenticatedWithProvider2 succeeds. I'll need my own version of IValidationRule, but only to allow each policy to manage success/error messages separately.

To date, I have been hand creating policies like this. In the future doing this once an having an ExpressionPolicy which was initialized with a policy name which is the expression should be a much easier and safer way of doing it.

I never thought I'd be excited by how to implement policies, but I'm eager to get started.

Thanks again for helping me though this and sparking an approach that should work well for our needs.

Shane32 commented 1 year ago

Your use case demonstrates why this library may still be useful even with GraphQL Server 7; your authentication needs are very unique and not based on traditional ASP.NET Core authentication. It also demonstrates how few changes are necessary to use the new GraphQL authentication validation rule with entirely custom logic.

Your use case also shows that it may be desired to add support for multiple instances of [Authorize] applied to a particular type or field. If this is allowed by ASP.NET Core (I don't know if it is or not), then this would add additional parity to ASP.NET Core for authorization.

Shane32 commented 1 year ago

I suggest closing this issue and opening a new issue in the graphql-dotnet/server repo if you have further questions.

BillBaird commented 1 year ago

Early indications is that things are working nicely. I've got a ways to go, but am encouraged. I'll close this and if necessary, move things to graphql-dotnet/server.

Shane32 commented 1 year ago

FYI, your concept about using language expressions such as "(AuthenticatedWithProvider1 and IsAdmin) or AuthenticatedWithProvider2" for policies is very intriguing. I wonder if you can cache the parsing (but not the result) so that Antlr4 does not need to parse each time it hits the same expression... I wonder if you can make it "generic" so that it works with any role or policy name... In another of my projects (not based on ClaimsPrinicpal), I implemented a fixed "all of set A or all of set B" approach against a list of roles -- so I can certainly see how something more than simply an 'OR' set of roles would be useful.

BillBaird commented 1 year ago

I've implemented it pretty much as described earlier such that it is compiled upon first use of the policy expression and placed in the policy dictionary such that subsequent uses should have the same overhead as if it had been hand coded. The implementation is pretty lightweight and straightforward, yet has all the power of any boolean expression. I'd be happy to make it available if you are interested.

Shane32 commented 1 year ago

Sounds good - I am interested - thanks!!

Shane32 commented 1 year ago

By the way, I was wrong in that GraphQL.NET only supports a single policy configured for a field or type. This is incorrect; it fully supports multiple policies set on a single field or type, and applies them in an 'AND' fashion (unlike roles which are applied in an 'OR' fashion). The relevant code I pointed you to was for transport-level authentication (which only supports a single policy), rather than field/type authentication (which supports multiple policies). The correct code is here:

https://github.com/graphql-dotnet/server/blob/master/src/Transports.AspNetCore/AuthorizationVisitorBase.Validation.cs#L72

BillBaird commented 1 year ago

Thanks, I was aware of how roles could be used, but in my use-case with multiple identity providers that work different ways, found them insufficient. Here is my implementation of policy expressions which I am now using in my work projects:

https://bitbucket.org/Bill-Baird/wlb.graphql.auth.policyexpressions/src/master/

Feedback on it would be appreciated.

sungam3r commented 1 year ago

Using [Authorize("Policy1", "Policy2")] can be thought of as Policy1 and Policy2.

AuthorizeAttribute has ctor with single policy only.

Shane32 commented 1 year ago

Thanks, I was aware of how roles could be used, but in my use-case with multiple identity providers that work different ways, found them insufficient. Here is my implementation of policy expressions which I am now using in my work projects:

https://bitbucket.org/Bill-Baird/wlb.graphql.auth.policyexpressions/src/master/

Feedback on it would be appreciated.

Thanks! It looks really clean and well done!

I think for my own use I'd probably make it compatible with roles somehow, such as adding a role modifier. Maybe an authenticated keyword also. I might also write my own lexer and compiler instead of using antlr, just because. And I'd probably set it up to authenticate directly against ASP.NET Core by default, with custom authorization by deriving from a base class and overriding AuthorizeAsync, IsInRole and IsAuthenticated. As such, it wouldn't need any dependencies, and could potentially be integrated directly into the repo as an optional feature. Maybe I'll add it to GraphQL.AspNetCore3.

BillBaird commented 1 year ago

Great suggestions!

To make this more generic, I can see the utility of a first-class role modifier and authenticated keyword. They handle use-cases that I don't need since I have multiple providers and it matters which provider is used, and since we use claims-based security. Along those lines, making this work directly with ASP.NET authorization would be useful as well - I was trying not to rely on the ASP.NET Core bits, but that would make it useable elsewhere, such as with REST APIs. It would be great to have this, or a derived work, in the repo as an optional feature.

I'll keep these ideas in mind going forward. For now this does what I needed it to do.