diff --git a/docs/migration/migration8.md b/docs/migration/migration8.md index 7182dd9a..720b928b 100644 --- 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 deleted file mode 100644 index bf0c6201..00000000 --- a/src/Transports.AspNetCore/AuthorizationVisitorBase.GetRecursivelyReferencedUsedFragments.cs +++ /dev/null @@ -1,102 +0,0 @@ -using GraphQLParser; -using GraphQLParser.Visitors; - -namespace GraphQL.Server.Transports.AspNetCore; - -public partial class AuthorizationVisitorBase -{ - /// - /// Returns all fragments referenced by the selected operation in the document, - /// excluding ones that would be skipped by the @skip or @include directives. - /// - /// - /// is used to determine if the node should be skipped or not. - /// - protected List? GetRecursivelyReferencedUsedFragments(ValidationContext validationContext) - { - var context = new GetRecursivelyReferencedFragmentsVisitorContext(this, validationContext); - var ret = GetRecursivelyReferencedFragmentsVisitor.Instance.VisitAsync(validationContext.Operation, context); - if (!ret.IsCompletedSuccessfully) // should always be true unless an exception occurs within SkipNode - ret.AsTask().GetAwaiter().GetResult(); - return context.FragmentDefinitions; - } - - private sealed class GetRecursivelyReferencedFragmentsVisitorContext : IASTVisitorContext - { - public GetRecursivelyReferencedFragmentsVisitorContext(AuthorizationVisitorBase authorizationVisitor, ValidationContext validationContext) - { - AuthorizationVisitor = authorizationVisitor; - ValidationContext = validationContext; - } - - public AuthorizationVisitorBase AuthorizationVisitor { get; } - - public CancellationToken CancellationToken => default; - - public ValidationContext ValidationContext { get; } - - public List? FragmentDefinitions { get; set; } - } - - private sealed class GetRecursivelyReferencedFragmentsVisitor : ASTVisitor - { - private GetRecursivelyReferencedFragmentsVisitor() { } - - public static readonly GetRecursivelyReferencedFragmentsVisitor Instance = new(); - - public override ValueTask VisitAsync(ASTNode? node, GetRecursivelyReferencedFragmentsVisitorContext context) - { - // check if this node should be skipped or not (check @skip and @include directives) - if (node == null || !context.AuthorizationVisitor.SkipNode(node, context.ValidationContext)) - return base.VisitAsync(node, context); - - return default; - } - - protected override ValueTask VisitOperationDefinitionAsync(GraphQLOperationDefinition operationDefinition, GetRecursivelyReferencedFragmentsVisitorContext context) - => VisitAsync(operationDefinition.SelectionSet, context); - - protected override ValueTask VisitSelectionSetAsync(GraphQLSelectionSet selectionSet, GetRecursivelyReferencedFragmentsVisitorContext context) - => VisitAsync(selectionSet.Selections, context); - - protected override ValueTask VisitFieldAsync(GraphQLField field, GetRecursivelyReferencedFragmentsVisitorContext context) - => VisitAsync(field.SelectionSet, context); - - protected override ValueTask VisitInlineFragmentAsync(GraphQLInlineFragment inlineFragment, GetRecursivelyReferencedFragmentsVisitorContext context) - => VisitAsync(inlineFragment.SelectionSet, context); - - protected override ValueTask VisitFragmentSpreadAsync(GraphQLFragmentSpread fragmentSpread, GetRecursivelyReferencedFragmentsVisitorContext context) - { - // if we have not encountered this fragment before - if (!Contains(fragmentSpread, context)) - { - // find the fragment definition - var fragmentDefinition = context.ValidationContext.Document.FindFragmentDefinition(fragmentSpread.FragmentName.Name); - if (fragmentDefinition != null) - { - // add the fragment definition to our known list - (context.FragmentDefinitions ??= new()).Add(fragmentDefinition); - // walk the fragment definition - return VisitSelectionSetAsync(fragmentDefinition.SelectionSet, context); - } - } - - return default; - } - - private static bool Contains(GraphQLFragmentSpread fragmentSpread, GetRecursivelyReferencedFragmentsVisitorContext context) - { - var fragmentDefinitions = context.FragmentDefinitions; - if (fragmentDefinitions == null) - return false; - - foreach (var fragmentDefinition in fragmentDefinitions) - { - if (fragmentDefinition.FragmentName.Name == fragmentSpread.FragmentName.Name) - return true; - } - - return false; - } - } -} diff --git a/src/Transports.AspNetCore/AuthorizationVisitorBase.cs b/src/Transports.AspNetCore/AuthorizationVisitorBase.cs index e14f4749..12415f3f 100644 --- 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() } } - /// - /// Indicates if the specified node should skip authentication processing. - /// Default implementation looks at @skip and @include directives only. - /// - 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; - } - } - /// /// 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 index 66e0ca97..066e8a44 100644 --- 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 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? 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 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 ValidateAsync(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info) { } public virtual System.Threading.Tasks.ValueTask ValidateSchemaAsync(GraphQL.Validation.ValidationContext context) { } public readonly struct ValidationInfo : System.IEquatable diff --git a/tests/ApiApprovalTests/netcoreapp21+netstandard20/GraphQL.Server.Transports.AspNetCore.approved.txt b/tests/ApiApprovalTests/netcoreapp21+netstandard20/GraphQL.Server.Transports.AspNetCore.approved.txt index 98c889b4..1216fed0 100644 --- 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 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? 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 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 ValidateAsync(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info) { } public virtual System.Threading.Tasks.ValueTask ValidateSchemaAsync(GraphQL.Validation.ValidationContext context) { } public readonly struct ValidationInfo : System.IEquatable diff --git a/tests/ApiApprovalTests/netcoreapp31/GraphQL.Server.Transports.AspNetCore.approved.txt b/tests/ApiApprovalTests/netcoreapp31/GraphQL.Server.Transports.AspNetCore.approved.txt index 08585061..685396c3 100644 --- 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 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? 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 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 ValidateAsync(GraphQL.Server.Transports.AspNetCore.AuthorizationVisitorBase.ValidationInfo info) { } public virtual System.Threading.Tasks.ValueTask ValidateSchemaAsync(GraphQL.Validation.ValidationContext context) { } public readonly struct ValidationInfo : System.IEquatable diff --git a/tests/Transports.AspNetCore.Tests/AuthorizationTests.cs b/tests/Transports.AspNetCore.Tests/AuthorizationTests.cs index 048a1f97..4b394e9a 100644 --- 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(() => new AuthorizationVisitor(null!, _principal, Mock.Of())); - Should.Throw(() => new AuthorizationVisitor(new ValidationContext(), null!, Mock.Of())); - Should.Throw(() => new AuthorizationVisitor(new ValidationContext(), _principal, null!)); + var context = new ValidationContext() { Operation = new(new([])), Document = new([]) }; + Should.Throw(() => new AuthorizationVisitor(context, null!, Mock.Of())); + Should.Throw(() => new AuthorizationVisitor(context, _principal, null!)); } [Theory]