Remove GetRecursivelyReferencedUsedFragments and SkipNode (#1143)

Shane32 · gao-artur · web-flow · commit df124cb2b47e · 2024-08-13T08:41:32.000-04:00
Co-authored-by: Artur Gordashnikov &lt;gao.artur@gmail.com&gt;
diff --git a/docs/migration/migration8.md b/docs/migration/migration8.md
@@ -34,6 +34,11 @@
   in the response prefer the same status code.  For practical purposes, this means that the included
   errors triggered by the authorization validation rule will now return 401 or 403 when appropriate.
 - The `SelectResponseContentType` method now returns a `MediaTypeHeaderValue` instead of a string.
+- The `AuthorizationVisitorBase.GetRecursivelyReferencedUsedFragments` method has been removed as
+  `ValidationContext` now provides an overload to `GetRecursivelyReferencedFragments` which will only
+  return fragments in use by the specified operation.
+- The `AuthorizationVisitorBase.SkipNode` method has been removed as `ValidationContext` now provides
+  a `ShouldIncludeNode` method.
 
 ## Other changes
 
diff --git a/src/Transports.AspNetCore/AuthorizationVisitorBase.GetRecursivelyReferencedUsedFragments.cs b/src/Transports.AspNetCore/AuthorizationVisitorBase.GetRecursivelyReferencedUsedFragments.cs
diff --git a/src/Transports.AspNetCore/AuthorizationVisitorBase.cs b/src/Transports.AspNetCore/AuthorizationVisitorBase.cs
@@ -8,7 +8,7 @@ public AuthorizationVisitorBase(ValidationContext context)
     {
         if (context == null)
             throw new ArgumentNullException(nameof(context));
-        _fragmentDefinitionsToCheck = GetRecursivelyReferencedUsedFragments(context);
+        _fragmentDefinitionsToCheck = context.GetRecursivelyReferencedFragments(context.Operation, true);
     }
 
     private bool _checkTree; // used to skip processing fragments or operations that do not apply
@@ -36,7 +36,7 @@ public virtual async ValueTask EnterAsync(ASTNode node, ValidationContext contex
         else if (_checkTree)
         {
             // if a directive indicates to skip this node, skip authorization checks until Leave() is called for this node
-            if (SkipNode(node, context))
+            if (!context.ShouldIncludeNode(node))
             {
                 _checkTree = false;
                 _checkUntil = node;
@@ -175,72 +175,6 @@ async ValueTask PopAndProcessAsync()
         }
     }
 
-    /// <summary>
-    /// Indicates if the specified node should skip authentication processing.
-    /// Default implementation looks at @skip and @include directives only.
-    /// </summary>
-    protected virtual bool SkipNode(ASTNode node, ValidationContext context)
-    {
-        // according to GraphQL spec, directives with the same name may be defined so long as they cannot be
-        // placed on the same node types as other directives with the same name; so here we verify that the
-        // node is a field, fragment spread, or inline fragment, the only nodes allowed by the built-in @skip
-        // and @include directives
-        if (node is not GraphQLField && node is not GraphQLFragmentSpread && node is not GraphQLInlineFragment)
-            return false;
-
-        var directivesNode = (IHasDirectivesNode)node;
-
-        var skipDirective = directivesNode.Directives?.FirstOrDefault(x => x.Name == "skip");
-        if (skipDirective != null)
-        {
-            var value = GetDirectiveValue(skipDirective, context, false);
-            if (value)
-                return true;
-        }
-
-        var includeDirective = directivesNode.Directives?.FirstOrDefault(x => x.Name == "include");
-        if (includeDirective != null)
-        {
-            var value = GetDirectiveValue(includeDirective, context, true);
-            if (!value)
-                return true;
-        }
-
-        return false;
-
-        static bool GetDirectiveValue(GraphQLDirective directive, ValidationContext context, bool defaultValue)
-        {
-            var ifArg = directive.Arguments?.FirstOrDefault(x => x.Name == "if");
-            if (ifArg != null)
-            {
-                if (ifArg.Value is GraphQLBooleanValue boolValue)
-                {
-                    return boolValue.BoolValue;
-                }
-                else if (ifArg.Value is GraphQLVariable variable)
-                {
-                    if (context.Operation.Variables != null)
-                    {
-                        var varDef = context.Operation.Variables.FirstOrDefault(x => x.Variable.Name == variable.Name);
-                        if (varDef != null && varDef.Type.Name() == "Boolean")
-                        {
-                            if (context.Variables.TryGetValue(variable.Name.StringValue, out var value))
-                            {
-                                if (value is bool boolValue2)
-                                    return boolValue2;
-                            }
-                            if (varDef.DefaultValue is GraphQLBooleanValue boolValue3)
-                            {
-                                return boolValue3.BoolValue;
-                            }
-                        }
-                    }
-                }
-            }
-            return defaultValue;
-        }
-    }
-
     /// <summary>
     /// Runs when a fragment is added or updated; the fragment might not be waiting on any
     /// other fragments, or it still might be.
diff --git a/tests/ApiApprovalTests/net50+net60+net80/GraphQL.Server.Transports.AspNetCore.approved.txt b/tests/ApiApprovalTests/net50+net60+net80/GraphQL.Server.Transports.AspNetCore.approved.txt
@@ -36,13 +36,11 @@ namespace GraphQL.Server.Transports.AspNetCore
         protected abstract System.Threading.Tasks.ValueTask<Microsoft.AspNetCore.Authorization.AuthorizationResult> AuthorizeAsync(string policy);
         public virtual System.Threading.Tasks.ValueTask EnterAsync(GraphQLParser.AST.ASTNode node, GraphQL.Validation.ValidationContext context) { }
         protected virtual string GenerateResourceDescription(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info) { }
-        protected System.Collections.Generic.List<GraphQLParser.AST.GraphQLFragmentDefinition>? GetRecursivelyReferencedUsedFragments(GraphQL.Validation.ValidationContext validationContext) { }
         protected virtual void HandleNodeNotAuthorized(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info) { }
         protected virtual void HandleNodeNotInPolicy(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info, string policy, Microsoft.AspNetCore.Authorization.AuthorizationResult authorizationResult) { }
         protected virtual void HandleNodeNotInRoles(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info, System.Collections.Generic.List<string> roles) { }
         protected abstract bool IsInRole(string role);
         public virtual System.Threading.Tasks.ValueTask LeaveAsync(GraphQLParser.AST.ASTNode node, GraphQL.Validation.ValidationContext context) { }
-        protected virtual bool SkipNode(GraphQLParser.AST.ASTNode node, GraphQL.Validation.ValidationContext context) { }
         protected virtual System.Threading.Tasks.ValueTask<bool> ValidateAsync(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info) { }
         public virtual System.Threading.Tasks.ValueTask<bool> ValidateSchemaAsync(GraphQL.Validation.ValidationContext context) { }
         public readonly struct ValidationInfo : System.IEquatable<GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo>
diff --git a/tests/ApiApprovalTests/netcoreapp21+netstandard20/GraphQL.Server.Transports.AspNetCore.approved.txt b/tests/ApiApprovalTests/netcoreapp21+netstandard20/GraphQL.Server.Transports.AspNetCore.approved.txt
@@ -43,13 +43,11 @@ namespace GraphQL.Server.Transports.AspNetCore
         protected abstract System.Threading.Tasks.ValueTask<Microsoft.AspNetCore.Authorization.AuthorizationResult> AuthorizeAsync(string policy);
         public virtual System.Threading.Tasks.ValueTask EnterAsync(GraphQLParser.AST.ASTNode node, GraphQL.Validation.ValidationContext context) { }
         protected virtual string GenerateResourceDescription(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info) { }
-        protected System.Collections.Generic.List<GraphQLParser.AST.GraphQLFragmentDefinition>? GetRecursivelyReferencedUsedFragments(GraphQL.Validation.ValidationContext validationContext) { }
         protected virtual void HandleNodeNotAuthorized(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info) { }
         protected virtual void HandleNodeNotInPolicy(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info, string policy, Microsoft.AspNetCore.Authorization.AuthorizationResult authorizationResult) { }
         protected virtual void HandleNodeNotInRoles(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info, System.Collections.Generic.List<string> roles) { }
         protected abstract bool IsInRole(string role);
         public virtual System.Threading.Tasks.ValueTask LeaveAsync(GraphQLParser.AST.ASTNode node, GraphQL.Validation.ValidationContext context) { }
-        protected virtual bool SkipNode(GraphQLParser.AST.ASTNode node, GraphQL.Validation.ValidationContext context) { }
         protected virtual System.Threading.Tasks.ValueTask<bool> ValidateAsync(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info) { }
         public virtual System.Threading.Tasks.ValueTask<bool> ValidateSchemaAsync(GraphQL.Validation.ValidationContext context) { }
         public readonly struct ValidationInfo : System.IEquatable<GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo>
diff --git a/tests/ApiApprovalTests/netcoreapp31/GraphQL.Server.Transports.AspNetCore.approved.txt b/tests/ApiApprovalTests/netcoreapp31/GraphQL.Server.Transports.AspNetCore.approved.txt
@@ -36,13 +36,11 @@ namespace GraphQL.Server.Transports.AspNetCore
         protected abstract System.Threading.Tasks.ValueTask<Microsoft.AspNetCore.Authorization.AuthorizationResult> AuthorizeAsync(string policy);
         public virtual System.Threading.Tasks.ValueTask EnterAsync(GraphQLParser.AST.ASTNode node, GraphQL.Validation.ValidationContext context) { }
         protected virtual string GenerateResourceDescription(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info) { }
-        protected System.Collections.Generic.List<GraphQLParser.AST.GraphQLFragmentDefinition>? GetRecursivelyReferencedUsedFragments(GraphQL.Validation.ValidationContext validationContext) { }
         protected virtual void HandleNodeNotAuthorized(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info) { }
         protected virtual void HandleNodeNotInPolicy(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info, string policy, Microsoft.AspNetCore.Authorization.AuthorizationResult authorizationResult) { }
         protected virtual void HandleNodeNotInRoles(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info, System.Collections.Generic.List<string> roles) { }
         protected abstract bool IsInRole(string role);
         public virtual System.Threading.Tasks.ValueTask LeaveAsync(GraphQLParser.AST.ASTNode node, GraphQL.Validation.ValidationContext context) { }
-        protected virtual bool SkipNode(GraphQLParser.AST.ASTNode node, GraphQL.Validation.ValidationContext context) { }
         protected virtual System.Threading.Tasks.ValueTask<bool> ValidateAsync(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info) { }
         public virtual System.Threading.Tasks.ValueTask<bool> ValidateSchemaAsync(GraphQL.Validation.ValidationContext context) { }
         public readonly struct ValidationInfo : System.IEquatable<GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo>
diff --git a/tests/Transports.AspNetCore.Tests/AuthorizationTests.cs b/tests/Transports.AspNetCore.Tests/AuthorizationTests.cs
@@ -545,8 +545,9 @@ private void Apply(IMetadataWriter obj, Mode mode)
     public void Constructors()
     {
         Should.Throw<ArgumentNullException>(() => new AuthorizationVisitor(null!, _principal, Mock.Of<IAuthorizationService>()));
-        Should.Throw<ArgumentNullException>(() => new AuthorizationVisitor(new ValidationContext(), null!, Mock.Of<IAuthorizationService>()));
-        Should.Throw<ArgumentNullException>(() => new AuthorizationVisitor(new ValidationContext(), _principal, null!));
+        var context = new ValidationContext() { Operation = new(new([])), Document = new([]) };
+        Should.Throw<ArgumentNullException>(() => new AuthorizationVisitor(context, null!, Mock.Of<IAuthorizationService>()));
+        Should.Throw<ArgumentNullException>(() => new AuthorizationVisitor(context, _principal, null!));
     }
 
     [Theory]

Original file line number	Diff line number	Diff line change
`@@ -545,8 +545,9 @@ private void Apply(IMetadataWriter obj, Mode mode)`
`545`	`545`	`public void Constructors()`
`546`	`546`	`{`
`547`	`547`	`Should.Throw<ArgumentNullException>(() => new AuthorizationVisitor(null!, _principal, Mock.Of<IAuthorizationService>()));`
`548`		`- Should.Throw<ArgumentNullException>(() => new AuthorizationVisitor(new ValidationContext(), null!, Mock.Of<IAuthorizationService>()));`
`549`		`- Should.Throw<ArgumentNullException>(() => new AuthorizationVisitor(new ValidationContext(), _principal, null!));`
	`548`	`+ var context = new ValidationContext() { Operation = new(new([])), Document = new([]) };`
	`549`	`+ Should.Throw<ArgumentNullException>(() => new AuthorizationVisitor(context, null!, Mock.Of<IAuthorizationService>()));`
	`550`	`+ Should.Throw<ArgumentNullException>(() => new AuthorizationVisitor(context, _principal, null!));`
`550`	`551`	`}`
`551`	`552`
`552`	`553`	`[Theory]`