diff --git a/.github/dependabot.yml b/.github/dependabot.yml index a4ab645..139d094 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -4,6 +4,7 @@ updates: directory: "/" schedule: interval: "daily" + - package-ecosystem: "github-actions" directory: "/" schedule: diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index bda8576..7e2a235 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -32,7 +32,7 @@ jobs: - name: Install dependencies working-directory: src - run: dotnet restore -p:GraphQLTestVersion=5.0.0-preview-411 + run: dotnet restore -p:GraphQLTestVersion=5.1.1 - name: Build solution working-directory: src diff --git a/.github/workflows/label.yml b/.github/workflows/label.yml index 7ac774f..30ef4f7 100644 --- a/.github/workflows/label.yml +++ b/.github/workflows/label.yml @@ -6,13 +6,16 @@ # https://github.com/actions/labeler/blob/master/README.md name: Labeler -on: [pull_request] +on: + pull_request_target: + types: + - opened # when PR is opened jobs: label: runs-on: ubuntu-latest steps: - - uses: actions/labeler@v3 + - uses: actions/labeler@v4 with: sync-labels: "" repo-token: "${{ secrets.GITHUB_TOKEN }}" diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 6afa90f..e1d29f8 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -30,7 +30,7 @@ jobs: - ubuntu-latest - windows-latest graphqlversion: - - 5.0.0-preview-411 + - 5.1.1 steps: - name: Checkout source uses: actions/checkout@v2 @@ -54,10 +54,7 @@ jobs: if: ${{ startsWith(matrix.os, 'ubuntu') }} working-directory: src run: | - dotnet tool install -g dotnet-format --version 6.0.243104 --add-source https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-tools/nuget/v3/index.json - dotnet format --no-restore --verify-no-changes -v diag --severity warn whitespace || (echo "Run 'dotnet format' to fix whitespace issues" && exit 1) - dotnet format --no-restore --verify-no-changes -v diag --severity warn analyzers || (echo "Run 'dotnet format' to fix analyzers issues" && exit 1) - dotnet format --no-restore --verify-no-changes -v diag --severity warn style || (echo "Run 'dotnet format' to fix style issues" && exit 1) + dotnet format --no-restore --verify-no-changes --severity warn || (echo "Run 'dotnet format' to fix issues" && exit 1) - name: Build solution [Release] working-directory: src run: dotnet build --no-restore -c Release -p:GraphQLTestVersion=${{ matrix.graphqlversion }} diff --git a/src/BasicSample/BasicSample.csproj b/src/BasicSample/BasicSample.csproj index 40fad25..4f50bff 100644 --- a/src/BasicSample/BasicSample.csproj +++ b/src/BasicSample/BasicSample.csproj @@ -11,7 +11,7 @@ - + diff --git a/src/BasicSample/Program.cs b/src/BasicSample/Program.cs index b9bb902..1ba372c 100644 --- a/src/BasicSample/Program.cs +++ b/src/BasicSample/Program.cs @@ -74,7 +74,7 @@ public class Query /// /// Resolver for 'Query.viewer' field. /// - [GraphQLAuthorize("AdminPolicy")] + [Authorize("AdminPolicy")] public User Viewer() => new() { Id = Guid.NewGuid().ToString(), Name = "Quinn" }; /// diff --git a/src/GraphQL.Authorization.ApiTests/GraphQL.Authorization.ApiTests.csproj b/src/GraphQL.Authorization.ApiTests/GraphQL.Authorization.ApiTests.csproj index df49e9b..52cf5eb 100644 --- a/src/GraphQL.Authorization.ApiTests/GraphQL.Authorization.ApiTests.csproj +++ b/src/GraphQL.Authorization.ApiTests/GraphQL.Authorization.ApiTests.csproj @@ -6,7 +6,7 @@ - + diff --git a/src/GraphQL.Authorization.Tests/AuthorizationSchemaBuilderTests.cs b/src/GraphQL.Authorization.Tests/AuthorizationSchemaBuilderTests.cs index 95413a7..02fccbc 100644 --- a/src/GraphQL.Authorization.Tests/AuthorizationSchemaBuilderTests.cs +++ b/src/GraphQL.Authorization.Tests/AuthorizationSchemaBuilderTests.cs @@ -30,10 +30,10 @@ type Query { } [GraphQLMetadata("Query")] - [GraphQLAuthorize("ClassPolicy")] + [Authorize("ClassPolicy")] public class QueryWithAttributes { - [GraphQLAuthorize("FieldPolicy")] + [Authorize("FieldPolicy")] [System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0060:Remove unused parameter", Justification = "test")] public string Post(string id) => ""; } diff --git a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs index 45592ce..ef536dc 100644 --- a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs +++ b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs @@ -331,10 +331,10 @@ type Query { } [GraphQLMetadata("Query")] - [GraphQLAuthorize("ClassPolicy")] + [Authorize("ClassPolicy")] public class BasicQueryWithAttributes { - [GraphQLAuthorize("FieldPolicy")] + [Authorize("FieldPolicy")] [System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0060:Remove unused parameter", Justification = "test")] public string Post(string id) => ""; } @@ -374,7 +374,7 @@ public class NestedQueryWithAttributes public string? Comment() => null; } - [GraphQLAuthorize("PostPolicy")] + [Authorize("PostPolicy")] public class Post { public string? Id { get; set; } diff --git a/src/GraphQL.Authorization.Tests/GraphQL.Authorization.Tests.csproj b/src/GraphQL.Authorization.Tests/GraphQL.Authorization.Tests.csproj index 13959ce..331acce 100644 --- a/src/GraphQL.Authorization.Tests/GraphQL.Authorization.Tests.csproj +++ b/src/GraphQL.Authorization.Tests/GraphQL.Authorization.Tests.csproj @@ -3,7 +3,7 @@ net6;net5;netcoreapp3.1 - 5.0.0-preview-411 + 5.1.1 diff --git a/src/GraphQL.Authorization.Tests/ValidationTestBase.cs b/src/GraphQL.Authorization.Tests/ValidationTestBase.cs index 0432cde..2ba31ed 100644 --- a/src/GraphQL.Authorization.Tests/ValidationTestBase.cs +++ b/src/GraphQL.Authorization.Tests/ValidationTestBase.cs @@ -4,6 +4,7 @@ using System.Security.Claims; using GraphQL.Execution; using GraphQL.Validation; +using GraphQLParser; using Shouldly; namespace GraphQL.Authorization.Tests @@ -63,7 +64,15 @@ private static IValidationResult Validate(ValidationTestConfig config) var documentBuilder = new GraphQLDocumentBuilder(); var document = documentBuilder.Build(config.Query); var validator = new DocumentValidator(); - return validator.ValidateAsync(config.Schema, document, document.Operations.First().Variables, config.Rules, userContext, config.Variables, config.OperationName).GetAwaiter().GetResult().validationResult; + return validator.ValidateAsync(new ValidationOptions + { + Schema = config.Schema, + Document = document, + Operation = document.OperationWithName(config.OperationName), + Rules = config.Rules, + Variables = config.Variables ?? Inputs.Empty, + UserContext = userContext + }).GetAwaiter().GetResult().validationResult; } internal static ClaimsPrincipal CreatePrincipal(string? authenticationType = null, IDictionary? claims = null) diff --git a/src/GraphQL.Authorization/AuthorizationValidationRule.cs b/src/GraphQL.Authorization/AuthorizationValidationRule.cs index e72bace..b033c90 100644 --- a/src/GraphQL.Authorization/AuthorizationValidationRule.cs +++ b/src/GraphQL.Authorization/AuthorizationValidationRule.cs @@ -1,9 +1,12 @@ +using System; using System.Collections.Generic; -using System.Linq; +using System.Threading; using System.Threading.Tasks; -using GraphQL.Language.AST; using GraphQL.Types; using GraphQL.Validation; +using GraphQLParser; +using GraphQLParser.AST; +using GraphQLParser.Visitors; namespace GraphQL.Authorization { @@ -24,9 +27,9 @@ public AuthorizationValidationRule(IAuthorizationEvaluator evaluator) _evaluator = evaluator; } - private bool ShouldBeSkipped(Operation actualOperation, ValidationContext context) + private bool ShouldBeSkipped(GraphQLOperationDefinition actualOperation, ValidationContext context) { - if (context.Document.Operations.Count <= 1) + if (context.Document.OperationsCount() <= 1) { return false; } @@ -46,126 +49,132 @@ private bool ShouldBeSkipped(Operation actualOperation, ValidationContext contex return true; } - if (ancestor is FragmentDefinition fragment) + if (ancestor is GraphQLFragmentDefinition fragment) { - return !FragmentBelongsToOperation(fragment, actualOperation); + //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 + return !c.Found; } } while (true); } - private bool FragmentBelongsToOperation(FragmentDefinition fragment, Operation operation) + private sealed class FragmentBelongsToOperationVisitorContext : IASTVisitorContext { - bool belongs = false; - void Visit(INode node, int _) + public FragmentBelongsToOperationVisitorContext(GraphQLFragmentDefinition fragment) { - if (belongs) - { - return; - } + Fragment = fragment; + } - belongs = node is FragmentSpread fragmentSpread && fragmentSpread.Name == fragment.Name; + public GraphQLFragmentDefinition Fragment { get; } - if (node != null) - { - node.Visit(Visit, 0); - } - } + public bool Found { get; set; } + + public CancellationToken CancellationToken => default; + } - operation.Visit(Visit, 0); + private static readonly FragmentBelongsToOperationVisitor _visitor = new(); - return belongs; + private sealed class FragmentBelongsToOperationVisitor : ASTVisitor + { + protected override ValueTask VisitFragmentSpreadAsync(GraphQLFragmentSpread fragmentSpread, FragmentBelongsToOperationVisitorContext context) + { + context.Found = context.Fragment.FragmentName.Name == fragmentSpread.FragmentName.Name; + return default; + } + + public override ValueTask VisitAsync(ASTNode? node, FragmentBelongsToOperationVisitorContext context) + { + return context.Found ? default : base.VisitAsync(node, context); + } } /// - public ValueTask ValidateAsync(ValidationContext context) + public async ValueTask ValidateAsync(ValidationContext context) { var userContext = context.UserContext as IProvideClaimsPrincipal; + await AuthorizeAsync(null, context.Schema, userContext, 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 ValueTask(new NodeVisitors( - new MatchingNodeVisitor((astType, context) => + return new NodeVisitors( + new MatchingNodeVisitor((astType, context) => { - if (context.Document.Operations.Count > 1 && astType.Name != context.OperationName) + if (context.Document.OperationsCount() > 1 && astType.Name != context.Operation.Name) { return; } - operationType = astType.OperationType; + operationType = astType.Operation; var type = context.TypeInfo.GetLastType(); - CheckAuth(astType, type, userContext, context, operationType); + AuthorizeAsync(astType, type, userContext, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this }), - new MatchingNodeVisitor((objectFieldAst, context) => + new MatchingNodeVisitor((objectFieldAst, context) => { - if (context.TypeInfo.GetArgument()?.ResolvedType?.GetNamedType() is IComplexGraphType argumentType && !ShouldBeSkipped(actualOperation, context)) + if (context.TypeInfo.GetArgument()?.ResolvedType?.GetNamedType() is IComplexGraphType argumentType && !ShouldBeSkipped(context.Operation, context)) { var fieldType = argumentType.GetField(objectFieldAst.Name); - CheckAuth(objectFieldAst, fieldType, userContext, context, operationType); + AuthorizeAsync(objectFieldAst, fieldType, userContext, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this } }), - new MatchingNodeVisitor((fieldAst, context) => + new MatchingNodeVisitor((fieldAst, context) => { var fieldDef = context.TypeInfo.GetFieldDef(); - if (fieldDef == null || ShouldBeSkipped(actualOperation, context)) + if (fieldDef == null || ShouldBeSkipped(context.Operation, context)) return; // check target field - CheckAuth(fieldAst, fieldDef, userContext, context, operationType); + AuthorizeAsync(fieldAst, fieldDef, userContext, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this // check returned graph type - CheckAuth(fieldAst, fieldDef.ResolvedType?.GetNamedType(), userContext, context, operationType); + AuthorizeAsync(fieldAst, fieldDef.ResolvedType?.GetNamedType(), userContext, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this }), - new MatchingNodeVisitor((variableRef, context) => + new MatchingNodeVisitor((variableRef, context) => { - if (context.TypeInfo.GetArgument()?.ResolvedType?.GetNamedType() is not IComplexGraphType variableType || ShouldBeSkipped(actualOperation, context)) + if (context.TypeInfo.GetArgument()?.ResolvedType?.GetNamedType() is not IComplexGraphType variableType || ShouldBeSkipped(context.Operation, context)) return; - CheckAuth(variableRef, variableType, userContext, context, operationType); + AuthorizeAsync(variableRef, variableType, userContext, 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) && + context.Variables.TryGetValue(variableRef.Name.StringValue, out object? input) && //ISSUE:allocation input is Dictionary fieldsValues) { foreach (var field in variableType.Fields) { if (fieldsValues.ContainsKey(field.Name)) { - CheckAuth(variableRef, field, userContext, context, operationType); + AuthorizeAsync(variableRef, field, userContext, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this } } } }) - )); + ); } - private void CheckAuth( - INode node, + private async Task AuthorizeAsync( + ASTNode? node, IProvideMetadata? provider, IProvideClaimsPrincipal? userContext, ValidationContext context, OperationType? operationType) { - if (provider == null || !provider.RequiresAuthorization()) + if (provider == null || !provider.IsAuthorizationRequired()) return; - // TODO: async -> sync transition - var result = _evaluator - .Evaluate(userContext?.User, context.UserContext, context.Variables, provider.GetPolicies()) - .GetAwaiter() - .GetResult(); + var result = await _evaluator.Evaluate(userContext?.User, context.UserContext, context.Variables, provider.GetPolicies()); if (result.Succeeded) return; @@ -173,10 +182,10 @@ private void CheckAuth( string errors = string.Join("\n", result.Errors); context.ReportError(new ValidationError( - context.Document.OriginalQuery!, + context.Document.Source, "authorization", $"You are not authorized to run this {operationType.ToString().ToLower()}.\n{errors}", - node)); + node == null ? Array.Empty() : new ASTNode[] { node })); } } } diff --git a/src/GraphQL.Authorization/GraphQL.Authorization.csproj b/src/GraphQL.Authorization/GraphQL.Authorization.csproj index d2b586d..479f04e 100644 --- a/src/GraphQL.Authorization/GraphQL.Authorization.csproj +++ b/src/GraphQL.Authorization/GraphQL.Authorization.csproj @@ -7,7 +7,7 @@ - + diff --git a/src/Harness/GraphQL.cs b/src/Harness/GraphQL.cs index bf1a1ac..3695f9a 100644 --- a/src/Harness/GraphQL.cs +++ b/src/Harness/GraphQL.cs @@ -23,7 +23,7 @@ public class Query /// /// Resolver for 'Query.viewer' field. /// - [GraphQLAuthorize("AdminPolicy")] + [Authorize("AdminPolicy")] public User Viewer() => new() { Id = Guid.NewGuid().ToString(), Name = "Quinn" }; /// diff --git a/src/Harness/Harness.csproj b/src/Harness/Harness.csproj index 47460da..a814a7a 100644 --- a/src/Harness/Harness.csproj +++ b/src/Harness/Harness.csproj @@ -1,4 +1,4 @@ - + net6;net5;netcoreapp3.1 @@ -9,10 +9,11 @@ - - - - + + + + + diff --git a/src/Harness/Startup.cs b/src/Harness/Startup.cs index b2f2920..1ed91df 100644 --- a/src/Harness/Startup.cs +++ b/src/Harness/Startup.cs @@ -1,5 +1,7 @@ using GraphQL; +using GraphQL.MicrosoftDI; using GraphQL.Server; +using GraphQL.SystemTextJson; using GraphQL.Types; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Hosting; @@ -35,7 +37,7 @@ type Query { } "; var schema = Schema.For(definitions, builder => builder.Types.Include()); - schema.AllTypes["User"]!.AuthorizeWith("AdminPolicy"); + schema.AllTypes["User"]!.AuthorizeWithPolicy("AdminPolicy"); return schema; }); @@ -46,10 +48,9 @@ type Query { // claims principal must look something like this to allow access // var user = new ClaimsPrincipal(new ClaimsIdentity(new[] { new Claim("role", "Admin") })); - services - .AddGraphQL() + services.AddGraphQL(builder => builder .AddSystemTextJson() - .AddUserContextBuilder(context => new GraphQLUserContext { User = context.User }); + .AddUserContextBuilder(context => new GraphQLUserContext { User = context.User })); } public void Configure(IApplicationBuilder app, IWebHostEnvironment env) diff --git a/src/Tests.props b/src/Tests.props index 6db0c28..b947515 100644 --- a/src/Tests.props +++ b/src/Tests.props @@ -7,11 +7,11 @@ - + all runtime; build; native; contentfiles; analyzers; buildtransitive - +