Authorize only actual operation + Added authorization for VariableReference fields (#676)

sungam3r · web-flow · commit 09e92786d061 · 2021-12-29T16:49:22.000+03:00
diff --git a/samples/Samples.Server/Startup.cs b/samples/Samples.Server/Startup.cs
@@ -1,6 +1,7 @@
 using System.Collections.Generic;
 using GraphQL.DataLoader;
 using GraphQL.Execution;
+using System.Threading.Tasks;
 using GraphQL.Samples.Schemas.Chat;
 using GraphQL.Server;
 using GraphQL.Server.Ui.Altair;
@@ -36,11 +37,15 @@ public void ConfigureServices(IServiceCollection services)
             MicrosoftDI.GraphQLBuilderExtensions.AddGraphQL(services)
                 .AddServer(true)
                 .AddSchema<ChatSchema>()
-                .ConfigureExecution(options =>
+                .ConfigureExecutionOptions(options =>
                 {
                     options.EnableMetrics = Environment.IsDevelopment();
                     var logger = options.RequestServices.GetRequiredService<ILogger<Startup>>();
-                    options.UnhandledExceptionDelegate = ctx => logger.LogError("{Error} occurred", ctx.OriginalException.Message);
+                    options.UnhandledExceptionDelegate = ctx =>
+                    {
+                        logger.LogError("{Error} occurred", ctx.OriginalException.Message);
+                        return Task.CompletedTask;
+                    };
                 })
                 .AddSystemTextJson()
                 .AddErrorInfoProvider<CustomErrorInfoProvider>()
diff --git a/samples/Samples.Server/StartupWithRouting.cs b/samples/Samples.Server/StartupWithRouting.cs
@@ -1,6 +1,7 @@
 using System.Collections.Generic;
 using GraphQL.DataLoader;
 using GraphQL.Execution;
+using System.Threading.Tasks;
 using GraphQL.Samples.Schemas.Chat;
 using GraphQL.Server;
 using GraphQL.Server.Ui.Altair;
@@ -37,11 +38,15 @@ public void ConfigureServices(IServiceCollection services)
             MicrosoftDI.GraphQLBuilderExtensions.AddGraphQL(services)
                 .AddServer(true)
                 .AddSchema<ChatSchema>()
-                .ConfigureExecution(options =>
+                .ConfigureExecutionOptions(options =>
                 {
                     options.EnableMetrics = Environment.IsDevelopment();
                     var logger = options.RequestServices.GetRequiredService<ILogger<Startup>>();
-                    options.UnhandledExceptionDelegate = ctx => logger.LogError("{Error} occurred", ctx.OriginalException.Message);
+                    options.UnhandledExceptionDelegate = ctx =>
+                    {
+                        logger.LogError("{Error} occurred", ctx.OriginalException.Message);
+                        return Task.CompletedTask;
+                    };
                 })
                 .AddDefaultEndpointSelectorPolicy()
                 .AddSystemTextJson()
diff --git a/src/All/All.csproj b/src/All/All.csproj
@@ -21,9 +21,9 @@
     <ProjectReference Include="..\Ui.Playground\Ui.Playground.csproj" />
     <ProjectReference Include="..\Ui.Voyager\Ui.Voyager.csproj" />
 
-    <PackageReference Include="GraphQL.MemoryCache" Version="4.6.1" />
-    <PackageReference Include="GraphQL.MicrosoftDI" Version="4.6.1" />
-    <PackageReference Include="GraphQL.SystemReactive" Version="4.6.1" />
+    <PackageReference Include="GraphQL.MemoryCache" Version="5.0.0-preview-397" />
+    <PackageReference Include="GraphQL.MicrosoftDI" Version="5.0.0-preview-397" />
+    <PackageReference Include="GraphQL.SystemReactive" Version="5.0.0-preview-397" />
   </ItemGroup>
 
 </Project>
diff --git a/src/Authorization.AspNetCore/AuthorizationValidationRule.cs b/src/Authorization.AspNetCore/AuthorizationValidationRule.cs
@@ -1,4 +1,5 @@
 using System.Collections.Generic;
+using System.Linq;
 using System.Security.Claims;
 using System.Threading.Tasks;
 using GraphQL.Language.AST;
@@ -28,45 +29,129 @@ public AuthorizationValidationRule(IAuthorizationService authorizationService, I
             _claimsPrincipalAccessor = claimsPrincipalAccessor;
         }
 
+        private bool ShouldBeSkipped(Operation actualOperation, ValidationContext context)
+        {
+            if (context.Document.Operations.Count <= 1)
+            {
+                return false;
+            }
+
+            int i = 0;
+            do
+            {
+                var ancestor = context.TypeInfo.GetAncestor(i++);
+
+                if (ancestor == actualOperation)
+                {
+                    return false;
+                }
+
+                if (ancestor == context.Document)
+                {
+                    return true;
+                }
+
+                if (ancestor is FragmentDefinition fragment)
+                {
+                    return !FragmentBelongsToOperation(fragment, actualOperation);
+                }
+            } while (true);
+        }
+
+        private bool FragmentBelongsToOperation(FragmentDefinition fragment, Operation operation)
+        {
+            bool belongs = false;
+            void Visit(INode node, int _)
+            {
+                if (belongs)
+                {
+                    return;
+                }
+
+                belongs = node is FragmentSpread fragmentSpread && fragmentSpread.Name == fragment.Name;
+
+                if (node != null)
+                {
+                    node.Visit(Visit, 0);
+                }
+            }
+
+            operation.Visit(Visit, 0);
+
+            return belongs;
+        }
+
         /// <inheritdoc />
-        public async Task<INodeVisitor> ValidateAsync(ValidationContext context)
+        public async ValueTask<INodeVisitor?> ValidateAsync(ValidationContext context)
         {
             await AuthorizeAsync(null, context.Schema, context, null);
             var operationType = OperationType.Query;
+            var actualOperation = context.Document.Operations.FirstOrDefault(x => x.Name == context.OperationName) ?? context.Document.Operations.FirstOrDefault();
 
             // 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<Operation>((astType, context) =>
                 {
+                    if (context.Document.Operations.Count > 1 && astType.Name != context.OperationName)
+                    {
+                        return;
+                    }
+
                     operationType = astType.OperationType;
 
                     var type = context.TypeInfo.GetLastType();
                     AuthorizeAsync(astType, type, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this
                 }),
+
                 new MatchingNodeVisitor<ObjectField>((objectFieldAst, context) =>
                 {
-                    if (context.TypeInfo.GetArgument()?.ResolvedType.GetNamedType() is IComplexGraphType argumentType)
+                    if (context.TypeInfo.GetArgument()?.ResolvedType?.GetNamedType() is IComplexGraphType argumentType && !ShouldBeSkipped(actualOperation, context))
                     {
                         var fieldType = argumentType.GetField(objectFieldAst.Name);
                         AuthorizeAsync(objectFieldAst, fieldType, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this
                     }
                 }),
+
                 new MatchingNodeVisitor<Field>((fieldAst, context) =>
                 {
                     var fieldDef = context.TypeInfo.GetFieldDef();
 
-                    if (fieldDef == null)
+                    if (fieldDef == null || ShouldBeSkipped(actualOperation, context))
                         return;
 
                     // check target field
                     AuthorizeAsync(fieldAst, fieldDef, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this
                     // check returned graph type
-                    AuthorizeAsync(fieldAst, fieldDef.ResolvedType.GetNamedType(), context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this
+                    AuthorizeAsync(fieldAst, fieldDef.ResolvedType?.GetNamedType(), context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this
+                }),
+
+                new MatchingNodeVisitor<VariableReference>((variableRef, context) =>
+                {
+                    if (context.TypeInfo.GetArgument()?.ResolvedType?.GetNamedType() is not IComplexGraphType variableType || ShouldBeSkipped(actualOperation, context))
+                        return;
+
+                    AuthorizeAsync(variableRef, variableType, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this;
+
+                    // 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
+                    // validation rule should check that but here we should just ignore that
+                    // "unknown" field.
+                    if (context.Variables != null &&
+                        context.Variables.TryGetValue(variableRef.Name, out object input) &&
+                        input is Dictionary<string, object> fieldsValues)
+                    {
+                        foreach (var field in variableType.Fields)
+                        {
+                            if (fieldsValues.ContainsKey(field.Name))
+                            {
+                                AuthorizeAsync(variableRef, field, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this;
+                            }
+                        }
+                    }
                 })
             );
         }
diff --git a/src/Core/BasicGraphQLExecuter.cs b/src/Core/BasicGraphQLExecuter.cs
@@ -49,7 +49,7 @@ protected virtual ExecutionOptions GetOptions(string operationName, string query
                 Schema = Schema,
                 OperationName = operationName,
                 Query = query,
-                Inputs = variables,
+                Variables = variables,
                 UserContext = context,
                 CancellationToken = cancellationToken,
                 ComplexityConfiguration = _options.ComplexityConfiguration,
diff --git a/src/Core/Core.csproj b/src/Core/Core.csproj
@@ -9,8 +9,8 @@
 
   <ItemGroup>
     <!--TODO: remove SystemReactive in v6 -->
-    <PackageReference Include="GraphQL.SystemReactive" Version="4.6.1" />
-    <PackageReference Include="GraphQL.DataLoader" Version="4.6.1" />
+    <PackageReference Include="GraphQL.SystemReactive" Version="5.0.0-preview-397" />
+    <PackageReference Include="GraphQL.DataLoader" Version="5.0.0-preview-397" />
     <PackageReference Include="Microsoft.Extensions.Options" Version="3.1.0" />
   </ItemGroup>
 
diff --git a/src/Core/DefaultGraphQLExecuter.cs b/src/Core/DefaultGraphQLExecuter.cs
@@ -59,7 +59,7 @@ protected virtual ExecutionOptions GetOptions(string operationName, string query
                 Schema = Schema,
                 OperationName = operationName,
                 Query = query,
-                Inputs = variables,
+                Variables = variables,
                 UserContext = context,
                 CancellationToken = cancellationToken,
                 ComplexityConfiguration = _options.ComplexityConfiguration,
diff --git a/src/Core/GraphQLOptions.cs b/src/Core/GraphQLOptions.cs
@@ -1,4 +1,5 @@
 using System;
+using System.Threading.Tasks;
 using GraphQL.Execution;
 using GraphQL.Validation.Complexity;
 
@@ -16,7 +17,7 @@ public class GraphQLOptions
         /// </summary>
         public bool EnableMetrics { get; set; } = true;
 
-        public Action<UnhandledExceptionContext> UnhandledExceptionDelegate = ctx => { };
+        public Func<UnhandledExceptionContext, Task> UnhandledExceptionDelegate = _ => Task.CompletedTask;
 
         /// <summary>If set, limits the maximum number of nodes executed in parallel</summary>
         public int? MaxParallelExecutionCount { get; set; }
diff --git a/src/Transports.AspNetCore.NewtonsoftJson/Transports.AspNetCore.NewtonsoftJson.csproj b/src/Transports.AspNetCore.NewtonsoftJson/Transports.AspNetCore.NewtonsoftJson.csproj
@@ -8,7 +8,7 @@
 
   <ItemGroup>
     <ProjectReference Include="..\Transports.AspNetCore\Transports.AspNetCore.csproj" />
-    <PackageReference Include="GraphQL.NewtonsoftJson" Version="4.6.1" />
+    <PackageReference Include="GraphQL.NewtonsoftJson" Version="5.0.0-preview-397" />
   </ItemGroup>
 
 </Project>
diff --git a/src/Transports.AspNetCore.SystemTextJson/Transports.AspNetCore.SystemTextJson.csproj b/src/Transports.AspNetCore.SystemTextJson/Transports.AspNetCore.SystemTextJson.csproj
@@ -10,7 +10,7 @@
     <FrameworkReference Include="Microsoft.AspNetCore.App" />
     <ProjectReference Include="..\Transports.AspNetCore\Transports.AspNetCore.csproj" />
     <ProjectReference Include="..\Transports.Subscriptions.Abstractions\Transports.Subscriptions.Abstractions.csproj" />
-    <PackageReference Include="GraphQL.SystemTextJson" Version="4.6.1" />
+    <PackageReference Include="GraphQL.SystemTextJson" Version="5.0.0-preview-397" />
     <PackageReference Include="System.IO.Pipelines" Version="5.0.1" />
   </ItemGroup>
 
diff --git a/src/Transports.AspNetCore/Extensions/GraphQLBuilderUserContextExtensions.cs b/src/Transports.AspNetCore/Extensions/GraphQLBuilderUserContextExtensions.cs
@@ -82,7 +82,7 @@ public static DI.IGraphQLBuilder AddUserContextBuilder<TUserContextBuilder>(this
             where TUserContextBuilder : class, IUserContextBuilder
         {
             builder.Register<IUserContextBuilder, TUserContextBuilder>(DI.ServiceLifetime.Singleton);
-            builder.ConfigureExecution(async options =>
+            builder.ConfigureExecutionOptions(async options =>
             {
                 if (options.UserContext == null || options.UserContext.Count == 0 && options.UserContext.GetType() == typeof(Dictionary<string, object>))
                 {
@@ -106,7 +106,7 @@ public static DI.IGraphQLBuilder AddUserContextBuilder<TUserContext>(this DI.IGr
             where TUserContext : class, IDictionary<string, object>
         {
             builder.Register<IUserContextBuilder>(new UserContextBuilder<TUserContext>(creator));
-            builder.ConfigureExecution(options =>
+            builder.ConfigureExecutionOptions(options =>
             {
                 if (options.UserContext == null || options.UserContext.Count == 0 && options.UserContext.GetType() == typeof(Dictionary<string, object>))
                 {
@@ -129,7 +129,7 @@ public static DI.IGraphQLBuilder AddUserContextBuilder<TUserContext>(this DI.IGr
             where TUserContext : class, IDictionary<string, object>
         {
             builder.Register<IUserContextBuilder>(new UserContextBuilder<TUserContext>(creator));
-            builder.ConfigureExecution(async options =>
+            builder.ConfigureExecutionOptions(async options =>
             {
                 if (options.UserContext == null || options.UserContext.Count == 0 && options.UserContext.GetType() == typeof(Dictionary<string, object>))
                 {
diff --git a/src/Transports.Subscriptions.Abstractions/Transports.Subscriptions.Abstractions.csproj b/src/Transports.Subscriptions.Abstractions/Transports.Subscriptions.Abstractions.csproj
@@ -6,8 +6,8 @@
   </PropertyGroup>
 
   <ItemGroup>
-    <PackageReference Include="GraphQL.SystemReactive" Version="4.6.1" />
-    <PackageReference Include="GraphQL.NewtonsoftJson" Version="4.6.1" />
+    <PackageReference Include="GraphQL.SystemReactive" Version="5.0.0-preview-397" />
+    <PackageReference Include="GraphQL.NewtonsoftJson" Version="5.0.0-preview-397" />
     <PackageReference Include="Microsoft.Extensions.Logging" Version="3.1.0" />
     <PackageReference Include="System.Threading.Tasks.Dataflow" Version="5.0.0" />
   </ItemGroup>
diff --git a/tests/ApiApprovalTests/GraphQL.Server.Authorization.AspNetCore.approved.txt b/tests/ApiApprovalTests/GraphQL.Server.Authorization.AspNetCore.approved.txt
@@ -13,7 +13,7 @@ namespace GraphQL.Server.Authorization.AspNetCore
     {
         public AuthorizationValidationRule(Microsoft.AspNetCore.Authorization.IAuthorizationService authorizationService, GraphQL.Server.Authorization.AspNetCore.IClaimsPrincipalAccessor claimsPrincipalAccessor) { }
         protected virtual void AddValidationError(GraphQL.Language.AST.INode node, GraphQL.Validation.ValidationContext context, GraphQL.Language.AST.OperationType? operationType, Microsoft.AspNetCore.Authorization.AuthorizationResult result) { }
-        public System.Threading.Tasks.Task<GraphQL.Validation.INodeVisitor> ValidateAsync(GraphQL.Validation.ValidationContext context) { }
+        public System.Threading.Tasks.ValueTask<GraphQL.Validation.INodeVisitor?> ValidateAsync(GraphQL.Validation.ValidationContext context) { }
     }
     public class DefaultClaimsPrincipalAccessor : GraphQL.Server.Authorization.AspNetCore.IClaimsPrincipalAccessor
     {
diff --git a/tests/ApiApprovalTests/GraphQL.Server.Core.approved.txt b/tests/ApiApprovalTests/GraphQL.Server.Core.approved.txt
@@ -48,7 +48,7 @@ namespace GraphQL.Server
     }
     public class GraphQLOptions
     {
-        public System.Action<GraphQL.Execution.UnhandledExceptionContext> UnhandledExceptionDelegate;
+        public System.Func<GraphQL.Execution.UnhandledExceptionContext, System.Threading.Tasks.Task> UnhandledExceptionDelegate;
         public GraphQLOptions() { }
         public GraphQL.Validation.Complexity.ComplexityConfiguration ComplexityConfiguration { get; set; }
         public bool EnableMetrics { get; set; }
diff --git a/tests/Samples.Server.Tests/ResponseTests.cs b/tests/Samples.Server.Tests/ResponseTests.cs
@@ -26,7 +26,7 @@ public async Task Single_Query_Should_Return_Single_Result(RequestType requestTy
         }
 
         /// <summary>
-        /// Tests that POST type requests are overridden by query string params. 
+        /// Tests that POST type requests are overridden by query string params.
         /// </summary>
         [Theory]
         [InlineData(RequestType.PostWithJson)]
