Remove SkipNode also

graphql-dotnet · Aug 13, 2024 · a7aac5b · a7aac5b
1 parent 651a7a7
commit a7aac5b
Show file tree

Hide file tree

Showing 5 changed files with 3 additions and 70 deletions.
diff --git a/docs/migration/migration8.md b/docs/migration/migration8.md
@@ -37,6 +37,8 @@
 - The `AuthorizationVisitorBase.GetRecursivelyReferencedUsedFragments` method has been removed as
   `ValidationContext` now provides a 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.cs b/src/Transports.AspNetCore/AuthorizationVisitorBase.cs
@@ -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
@@ -41,7 +41,6 @@ namespace GraphQL.Server.Transports.AspNetCore
         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/...pprovalTests/netcoreapp21+netstandard20/GraphQL.Server.Transports.AspNetCore.approved.txt b/...pprovalTests/netcoreapp21+netstandard20/GraphQL.Server.Transports.AspNetCore.approved.txt
@@ -48,7 +48,6 @@ namespace GraphQL.Server.Transports.AspNetCore
         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
@@ -41,7 +41,6 @@ namespace GraphQL.Server.Transports.AspNetCore
         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>