Additional changes for v7 compatibility

.github/workflows/format.yml

@@ -0,0 +1,35 @@
+name: Check formatting
+
+on:
+  pull_request:
+    branches:
+      - master
+      - develop
+    paths:
+      - src/**
+      - .github/workflows/**
+
+env:
+  DOTNET_NOLOGO: true
+  DOTNET_CLI_TELEMETRY_OPTOUT: true
+
+jobs:
+  format:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout source
+        uses: actions/checkout@v3
+      - name: Setup .NET SDK
+        uses: actions/setup-dotnet@v3
+        with:
+          dotnet-version: 7.0.x
+          source-url: https://nuget.pkg.github.com/graphql-dotnet/index.json
+        env:
+          NUGET_AUTH_TOKEN: ${{secrets.GITHUB_TOKEN}}
+      - name: Install dependencies
+        working-directory: src
+        run: dotnet restore
+      - name: Check formatting
+        working-directory: src
+        run: |
+          dotnet format --no-restore --verify-no-changes --severity warn || (echo "Run 'dotnet format' to fix issues" && exit 1)

.github/workflows/test.yml

@@ -46,9 +46,6 @@ jobs:
           source-url: https://nuget.pkg.github.com/graphql-dotnet/index.json
         env:
           NUGET_AUTH_TOKEN: ${{secrets.GITHUB_TOKEN}}
-      - name: Disable MSVS Nuget Source # temporary step to investigate https://github.com/graphql-dotnet/graphql-dotnet/issues/2422
-        if: ${{ startsWith(matrix.os, 'windows') }}
-        run: dotnet nuget disable source 'Microsoft Visual Studio Offline Packages'
       - name: Install dependencies with GraphQL version ${{ matrix.graphqlversion }}
         working-directory: src
         run: dotnet restore -p:GraphQLTestVersion=${{ matrix.graphqlversion }}

README.md

@@ -27,14 +27,24 @@ Note that GitHub requires authentication to consume the feed. See [here](https:/
 # Usage
 
 - Register the authorization classes in your DI container - call `AddAuthorization` on the provided `IGraphQLBuilder` inside `AddGraphQL` extension method.
-- Provide a custom `UserContext` class that implements `IProvideClaimsPrincipal`.
+- Provide a custom `UserContext` class that implements `IProvideClaimsPrincipal` or provide the `ClaimsPrincipal` through `ExecutionOptions.User`.
 - Add policies to the `AuthorizationSettings`.
 - Apply a policy to a GraphType or Field - both implement `IProvideMetadata`:
   - using `AuthorizeWithPolicy(string policy)` extension method
   - or with `AuthorizeAttribute` attribute if using Schema + Handler syntax.
 - The `AuthorizationValidationRule` will run and verify the policies based on the registered policies.
 - You can write your own `IAuthorizationRequirement`.
 
+# Limitations
+
+This authorization framework only supports policy-based authorization. It does not support role-based authorization, or the
+`[AllowAnonymous]` attribute/extension, or the `[Authorize]` attribute/extension indicating authorization is required
+but without specifying a policy. It also does not integrate with ASP.NET Core's authorization framework.
+
+The [GraphQL.Server](https://www.github.com/graphql-dotnet/server) repository contains an authorization rule which has the above
+missing features, intended for use with ASP.NET Core. It may also be tailored with custom authentication code if desired, rather than
+relying on ASP.NET Core's authentication framework.
+
 # Examples
 
 1. Fully functional basic [Console sample](src/BasicSample/Program.cs).
@@ -74,3 +84,5 @@ public class MutationType
 # Known Issues
 
 - It is currently not possible to add a policy to Input objects using Schema first approach.
+
+- :warning: Authorization checks are skipped on fragments that are referenced by other fragments :warning:

src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs

@@ -148,6 +148,19 @@ public void issue5_with_fragment_should_fail()
         });
     }
 
+    [Fact(Skip = "This needs to be fixed")]
+    public void nested_fragment_should_fail()
+    {
+        Settings.AddPolicy("AdminPolicy", builder => builder.RequireClaim("admin"));
+
+        ShouldFailRule(config =>
+        {
+            config.Query = "query a { article { ...frag } } query b { article { ...frag } } fragment frag on Article { ...frag2 } fragment frag2 on Article { content }";
+            config.Schema = TypedSchema();
+            config.ValidateResult = result => _ = result.Errors.Single(x => x.Message == $"You are not authorized to run this query.\nRequired claim 'admin' is not present.");
+        });
+    }
+
     [Fact]
     public void nested_type_list_non_null_policy_fail()
     {

src/GraphQL.Authorization.Tests/ValidationTestBase.cs

@@ -64,7 +64,7 @@ private static IValidationResult Validate(ValidationTestConfig config)
         {
             Schema = config.Schema,
             Document = document,
-            Operation = document.OperationWithName(config.OperationName)!,
+            Operation = document.OperationWithName(config.OperationName) ?? throw new InvalidOperationException("Could not find specified operation"),
             Rules = config.Rules,
             Variables = config.Variables ?? Inputs.Empty,
             UserContext = userContext

src/GraphQL.Authorization.sln

@@ -31,6 +31,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "workflows", "workflows", "{
 	ProjectSection(SolutionItems) = preProject
 		..\.github\workflows\build.yml = ..\.github\workflows\build.yml
 		..\.github\workflows\codeql-analysis.yml = ..\.github\workflows\codeql-analysis.yml
+		..\.github\workflows\format.yml = ..\.github\workflows\format.yml
 		..\.github\workflows\label.yml = ..\.github\workflows\label.yml
 		..\.github\workflows\publish.yml = ..\.github\workflows\publish.yml
 		..\.github\workflows\test.yml = ..\.github\workflows\test.yml

src/GraphQL.Authorization/AuthorizationValidationRule.cs

@@ -12,18 +12,18 @@ namespace GraphQL.Authorization;
 /// </summary>
 public class AuthorizationValidationRule : IValidationRule
 {
-    private readonly IAuthorizationEvaluator _evaluator;
+    private readonly Visitor _visitor;
 
     /// <summary>
     /// Creates an instance of <see cref="AuthorizationValidationRule"/> with
     /// the specified authorization evaluator.
     /// </summary>
     public AuthorizationValidationRule(IAuthorizationEvaluator evaluator)
     {
-        _evaluator = evaluator;
+        _visitor = new(evaluator);
     }
 
-    private bool ShouldBeSkipped(GraphQLOperationDefinition actualOperation, ValidationContext context)
+    private static async ValueTask<bool> ShouldBeSkipped(GraphQLOperationDefinition actualOperation, ValidationContext context)
     {
         if (context.Document.OperationsCount() <= 1)
         {
@@ -49,7 +49,7 @@ private bool ShouldBeSkipped(GraphQLOperationDefinition actualOperation, Validat
             {
                 //TODO: may be rewritten completely later
                 var c = new FragmentBelongsToOperationVisitorContext(fragment);
-                _visitor.VisitAsync(actualOperation, c).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this
+                await _fragmentBelongsToOperationVisitor.VisitAsync(actualOperation, c).ConfigureAwait(false);
                 return !c.Found;
             }
         } while (true);
@@ -69,7 +69,7 @@ public FragmentBelongsToOperationVisitorContext(GraphQLFragmentDefinition fragme
         public CancellationToken CancellationToken => default;
     }
 
-    private static readonly FragmentBelongsToOperationVisitor _visitor = new();
+    private static readonly FragmentBelongsToOperationVisitor _fragmentBelongsToOperationVisitor = new();
 
     private sealed class FragmentBelongsToOperationVisitor : ASTVisitor<FragmentBelongsToOperationVisitorContext>
     {
@@ -88,57 +88,61 @@ public override ValueTask VisitAsync(ASTNode? node, FragmentBelongsToOperationVi
     /// <inheritdoc />
     public async ValueTask<INodeVisitor?> ValidateAsync(ValidationContext context)
     {
-        var userContext = context.UserContext as IProvideClaimsPrincipal;
-        await AuthorizeAsync(null, context.Schema, userContext, context, null).ConfigureAwait(false);
-        var operationType = OperationType.Query;
+        await _visitor.AuthorizeAsync(null, context.Schema, context).ConfigureAwait(false);
 
         // this could leak info about hidden fields or types in error messages
         // it would be better to implement a filter on the Schema so it
         // acts as if they just don't exist vs. an auth denied error
         // - filtering the Schema is not currently supported
         // TODO: apply ISchemaFilter - context.Schema.Filter.AllowXXX
-        return new NodeVisitors(
-            new MatchingNodeVisitor<GraphQLOperationDefinition>((astType, context) =>
-            {
-                if (context.Document.OperationsCount() > 1 && astType.Name != context.Operation.Name)
-                {
-                    return;
-                }
+        return _visitor;
+    }
+
+    private class Visitor : INodeVisitor
+    {
+        private readonly IAuthorizationEvaluator _evaluator;
 
-                operationType = astType.Operation;
+        public Visitor(IAuthorizationEvaluator evaluator)
+        {
+            _evaluator = evaluator;
+        }
 
+        public async ValueTask EnterAsync(ASTNode node, ValidationContext context)
+        {
+            if (node is GraphQLOperationDefinition astType && astType == context.Operation)
+            {
                 var type = context.TypeInfo.GetLastType();
-                AuthorizeAsync(astType, type, userContext, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this
-            }),
+                await AuthorizeAsync(astType, type, context).ConfigureAwait(false);
+            }
 
-            new MatchingNodeVisitor<GraphQLObjectField>((objectFieldAst, context) =>
+            if (node is GraphQLObjectField objectFieldAst &&
+                context.TypeInfo.GetArgument()?.ResolvedType?.GetNamedType() is IComplexGraphType argumentType &&
+                !await ShouldBeSkipped(context.Operation, context).ConfigureAwait(false))
             {
-                if (context.TypeInfo.GetArgument()?.ResolvedType?.GetNamedType() is IComplexGraphType argumentType && !ShouldBeSkipped(context.Operation, context))
-                {
-                    var fieldType = argumentType.GetField(objectFieldAst.Name);
-                    AuthorizeAsync(objectFieldAst, fieldType, userContext, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this
-                }
-            }),
+                var fieldType = argumentType.GetField(objectFieldAst.Name);
+                await AuthorizeAsync(objectFieldAst, fieldType, context).ConfigureAwait(false);
+            }
 
-            new MatchingNodeVisitor<GraphQLField>((fieldAst, context) =>
+            if (node is GraphQLField fieldAst)
             {
                 var fieldDef = context.TypeInfo.GetFieldDef();
 
-                if (fieldDef == null || ShouldBeSkipped(context.Operation, context))
+                if (fieldDef == null || await ShouldBeSkipped(context.Operation, context).ConfigureAwait(false))
                     return;
 
                 // check target field
-                AuthorizeAsync(fieldAst, fieldDef, userContext, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this
+                await AuthorizeAsync(fieldAst, fieldDef, context).ConfigureAwait(false);
                 // check returned graph type
-                AuthorizeAsync(fieldAst, fieldDef.ResolvedType?.GetNamedType(), userContext, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this
-            }),
+                await AuthorizeAsync(fieldAst, fieldDef.ResolvedType?.GetNamedType(), context).ConfigureAwait(false);
+            }
 
-            new MatchingNodeVisitor<GraphQLVariable>((variableRef, context) =>
+            if (node is GraphQLVariable variableRef)
             {
-                if (context.TypeInfo.GetArgument()?.ResolvedType?.GetNamedType() is not IComplexGraphType variableType || ShouldBeSkipped(context.Operation, context))
+                if (context.TypeInfo.GetArgument()?.ResolvedType?.GetNamedType() is not IComplexGraphType variableType ||
+                    await ShouldBeSkipped(context.Operation, context).ConfigureAwait(false))
                     return;
 
-                AuthorizeAsync(variableRef, variableType, userContext, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this
+                await AuthorizeAsync(variableRef, variableType, context).ConfigureAwait(false);
 
                 // Check each supplied field in the variable that exists in the variable type.
                 // If some supplied field does not exist in the variable type then some other
@@ -152,35 +156,39 @@ public override ValueTask VisitAsync(ASTNode? node, FragmentBelongsToOperationVi
                     {
                         if (fieldsValues.ContainsKey(field.Name))
                         {
-                            AuthorizeAsync(variableRef, field, userContext, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this
+                            await AuthorizeAsync(variableRef, field, context).ConfigureAwait(false);
                         }
                     }
                 }
-            })
-        );
-    }
+            }
+        }
 
-    private async Task AuthorizeAsync(
-        ASTNode? node,
-        IProvideMetadata? provider,
-        IProvideClaimsPrincipal? userContext,
-        ValidationContext context,
-        OperationType? operationType)
-    {
-        if (provider == null || !provider.IsAuthorizationRequired())
-            return;
+        public ValueTask LeaveAsync(ASTNode node, ValidationContext context) => default;
+
+        public async ValueTask AuthorizeAsync(
+            ASTNode? node,
+            IProvideMetadata? provider,
+            ValidationContext context)
+        {
+            var userContext = context.UserContext as IProvideClaimsPrincipal;
+            var user = userContext == null ? context.User : userContext.User;
+            var operationType = context.Operation.Operation;
+
+            if (provider == null || !provider.IsAuthorizationRequired())
+                return;
 
-        var result = await _evaluator.Evaluate(userContext?.User, context.UserContext, context.Variables, provider.GetPolicies()).ConfigureAwait(false);
+            var result = await _evaluator.Evaluate(user, context.UserContext, context.Variables, provider.GetPolicies()).ConfigureAwait(false);
 
-        if (result.Succeeded)
-            return;
+            if (result.Succeeded)
+                return;
 
-        string errors = string.Join("\n", result.Errors);
+            string errors = string.Join("\n", result.Errors);
 
-        context.ReportError(new ValidationError(
-            context.Document.Source,
-            "authorization",
-            $"You are not authorized to run this {operationType.ToString().ToLower()}.\n{errors}",
-            node == null ? Array.Empty<ASTNode>() : new ASTNode[] { node }));
+            context.ReportError(new ValidationError(
+                context.Document.Source,
+                "authorization",
+                $"You are not authorized to run this {operationType.ToString().ToLower()}.\n{errors}",
+                node == null ? Array.Empty<ASTNode>() : new ASTNode[] { node }));
+        }
     }
 }