diff --git a/.github/workflows/format.yml b/.github/workflows/format.yml new file mode 100644 index 0000000..f1fbe50 --- /dev/null +++ b/.github/workflows/format.yml @@ -0,0 +1,35 @@ +name: Check formatting + +on: + pull_request: + branches: + - master + - develop + paths: + - src/** + - .github/workflows/** + +env: + DOTNET_NOLOGO: true + DOTNET_CLI_TELEMETRY_OPTOUT: true + +jobs: + format: + runs-on: ubuntu-latest + steps: + - name: Checkout source + uses: actions/checkout@v3 + - name: Setup .NET SDK + uses: actions/setup-dotnet@v3 + with: + dotnet-version: 7.0.x + source-url: https://nuget.pkg.github.com/graphql-dotnet/index.json + env: + NUGET_AUTH_TOKEN: ${{secrets.GITHUB_TOKEN}} + - name: Install dependencies + working-directory: src + run: dotnet restore + - name: Check formatting + working-directory: src + run: | + dotnet format --no-restore --verify-no-changes --severity warn || (echo "Run 'dotnet format' to fix issues" && exit 1) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 42fe7a9..40f68db 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -46,9 +46,6 @@ jobs: source-url: https://nuget.pkg.github.com/graphql-dotnet/index.json env: NUGET_AUTH_TOKEN: ${{secrets.GITHUB_TOKEN}} - - name: Disable MSVS Nuget Source # temporary step to investigate https://github.com/graphql-dotnet/graphql-dotnet/issues/2422 - if: ${{ startsWith(matrix.os, 'windows') }} - run: dotnet nuget disable source 'Microsoft Visual Studio Offline Packages' - name: Install dependencies with GraphQL version ${{ matrix.graphqlversion }} working-directory: src run: dotnet restore -p:GraphQLTestVersion=${{ matrix.graphqlversion }} diff --git a/README.md b/README.md index 3e3f006..94e999e 100644 --- a/README.md +++ b/README.md @@ -27,7 +27,7 @@ Note that GitHub requires authentication to consume the feed. See [here](https:/ # Usage - Register the authorization classes in your DI container - call `AddAuthorization` on the provided `IGraphQLBuilder` inside `AddGraphQL` extension method. -- Provide a custom `UserContext` class that implements `IProvideClaimsPrincipal`. +- Provide a custom `UserContext` class that implements `IProvideClaimsPrincipal` or provide the `ClaimsPrincipal` through `ExecutionOptions.User`. - Add policies to the `AuthorizationSettings`. - Apply a policy to a GraphType or Field - both implement `IProvideMetadata`: - using `AuthorizeWithPolicy(string policy)` extension method @@ -35,6 +35,16 @@ Note that GitHub requires authentication to consume the feed. See [here](https:/ - The `AuthorizationValidationRule` will run and verify the policies based on the registered policies. - You can write your own `IAuthorizationRequirement`. +# Limitations + +This authorization framework only supports policy-based authorization. It does not support role-based authorization, or the +`[AllowAnonymous]` attribute/extension, or the `[Authorize]` attribute/extension indicating authorization is required +but without specifying a policy. It also does not integrate with ASP.NET Core's authorization framework. + +The [GraphQL.Server](https://www.github.com/graphql-dotnet/server) repository contains an authorization rule which has the above +missing features, intended for use with ASP.NET Core. It may also be tailored with custom authentication code if desired, rather than +relying on ASP.NET Core's authentication framework. + # Examples 1. Fully functional basic [Console sample](src/BasicSample/Program.cs). @@ -74,3 +84,5 @@ public class MutationType # Known Issues - It is currently not possible to add a policy to Input objects using Schema first approach. + +- :warning: Authorization checks are skipped on fragments that are referenced by other fragments :warning: diff --git a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs index 6d7d658..c568189 100644 --- a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs +++ b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs @@ -148,6 +148,19 @@ public void issue5_with_fragment_should_fail() }); } + [Fact(Skip = "This needs to be fixed")] + public void nested_fragment_should_fail() + { + Settings.AddPolicy("AdminPolicy", builder => builder.RequireClaim("admin")); + + ShouldFailRule(config => + { + config.Query = "query a { article { ...frag } } query b { article { ...frag } } fragment frag on Article { ...frag2 } fragment frag2 on Article { content }"; + config.Schema = TypedSchema(); + config.ValidateResult = result => _ = result.Errors.Single(x => x.Message == $"You are not authorized to run this query.\nRequired claim 'admin' is not present."); + }); + } + [Fact] public void nested_type_list_non_null_policy_fail() { diff --git a/src/GraphQL.Authorization.Tests/ValidationTestBase.cs b/src/GraphQL.Authorization.Tests/ValidationTestBase.cs index c8c6598..9248920 100644 --- a/src/GraphQL.Authorization.Tests/ValidationTestBase.cs +++ b/src/GraphQL.Authorization.Tests/ValidationTestBase.cs @@ -64,7 +64,7 @@ private static IValidationResult Validate(ValidationTestConfig config) { Schema = config.Schema, Document = document, - Operation = document.OperationWithName(config.OperationName)!, + Operation = document.OperationWithName(config.OperationName) ?? throw new InvalidOperationException("Could not find specified operation"), Rules = config.Rules, Variables = config.Variables ?? Inputs.Empty, UserContext = userContext diff --git a/src/GraphQL.Authorization.sln b/src/GraphQL.Authorization.sln index 3253122..fd0d7fc 100644 --- a/src/GraphQL.Authorization.sln +++ b/src/GraphQL.Authorization.sln @@ -31,6 +31,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "workflows", "workflows", "{ ProjectSection(SolutionItems) = preProject ..\.github\workflows\build.yml = ..\.github\workflows\build.yml ..\.github\workflows\codeql-analysis.yml = ..\.github\workflows\codeql-analysis.yml + ..\.github\workflows\format.yml = ..\.github\workflows\format.yml ..\.github\workflows\label.yml = ..\.github\workflows\label.yml ..\.github\workflows\publish.yml = ..\.github\workflows\publish.yml ..\.github\workflows\test.yml = ..\.github\workflows\test.yml diff --git a/src/GraphQL.Authorization/AuthorizationValidationRule.cs b/src/GraphQL.Authorization/AuthorizationValidationRule.cs index 5c7df20..02920cd 100644 --- a/src/GraphQL.Authorization/AuthorizationValidationRule.cs +++ b/src/GraphQL.Authorization/AuthorizationValidationRule.cs @@ -12,7 +12,7 @@ namespace GraphQL.Authorization; /// public class AuthorizationValidationRule : IValidationRule { - private readonly IAuthorizationEvaluator _evaluator; + private readonly Visitor _visitor; /// /// Creates an instance of with @@ -20,10 +20,10 @@ public class AuthorizationValidationRule : IValidationRule /// public AuthorizationValidationRule(IAuthorizationEvaluator evaluator) { - _evaluator = evaluator; + _visitor = new(evaluator); } - private bool ShouldBeSkipped(GraphQLOperationDefinition actualOperation, ValidationContext context) + private static async ValueTask ShouldBeSkipped(GraphQLOperationDefinition actualOperation, ValidationContext context) { if (context.Document.OperationsCount() <= 1) { @@ -49,7 +49,7 @@ private bool ShouldBeSkipped(GraphQLOperationDefinition actualOperation, Validat { //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 + await _fragmentBelongsToOperationVisitor.VisitAsync(actualOperation, c).ConfigureAwait(false); return !c.Found; } } while (true); @@ -69,7 +69,7 @@ public FragmentBelongsToOperationVisitorContext(GraphQLFragmentDefinition fragme public CancellationToken CancellationToken => default; } - private static readonly FragmentBelongsToOperationVisitor _visitor = new(); + private static readonly FragmentBelongsToOperationVisitor _fragmentBelongsToOperationVisitor = new(); private sealed class FragmentBelongsToOperationVisitor : ASTVisitor { @@ -88,57 +88,61 @@ public override ValueTask VisitAsync(ASTNode? node, FragmentBelongsToOperationVi /// public async ValueTask ValidateAsync(ValidationContext context) { - var userContext = context.UserContext as IProvideClaimsPrincipal; - await AuthorizeAsync(null, context.Schema, userContext, context, null).ConfigureAwait(false); - var operationType = OperationType.Query; + await _visitor.AuthorizeAsync(null, context.Schema, context).ConfigureAwait(false); // 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((astType, context) => - { - if (context.Document.OperationsCount() > 1 && astType.Name != context.Operation.Name) - { - return; - } + return _visitor; + } + + private class Visitor : INodeVisitor + { + private readonly IAuthorizationEvaluator _evaluator; - operationType = astType.Operation; + public Visitor(IAuthorizationEvaluator evaluator) + { + _evaluator = evaluator; + } + public async ValueTask EnterAsync(ASTNode node, ValidationContext context) + { + if (node is GraphQLOperationDefinition astType && astType == context.Operation) + { var type = context.TypeInfo.GetLastType(); - AuthorizeAsync(astType, type, userContext, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this - }), + await AuthorizeAsync(astType, type, context).ConfigureAwait(false); + } - new MatchingNodeVisitor((objectFieldAst, context) => + if (node is GraphQLObjectField objectFieldAst && + context.TypeInfo.GetArgument()?.ResolvedType?.GetNamedType() is IComplexGraphType argumentType && + !await ShouldBeSkipped(context.Operation, context).ConfigureAwait(false)) { - if (context.TypeInfo.GetArgument()?.ResolvedType?.GetNamedType() is IComplexGraphType argumentType && !ShouldBeSkipped(context.Operation, context)) - { - var fieldType = argumentType.GetField(objectFieldAst.Name); - AuthorizeAsync(objectFieldAst, fieldType, userContext, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this - } - }), + var fieldType = argumentType.GetField(objectFieldAst.Name); + await AuthorizeAsync(objectFieldAst, fieldType, context).ConfigureAwait(false); + } - new MatchingNodeVisitor((fieldAst, context) => + if (node is GraphQLField fieldAst) { var fieldDef = context.TypeInfo.GetFieldDef(); - if (fieldDef == null || ShouldBeSkipped(context.Operation, context)) + if (fieldDef == null || await ShouldBeSkipped(context.Operation, context).ConfigureAwait(false)) return; // check target field - AuthorizeAsync(fieldAst, fieldDef, userContext, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this + await AuthorizeAsync(fieldAst, fieldDef, context).ConfigureAwait(false); // check returned graph type - AuthorizeAsync(fieldAst, fieldDef.ResolvedType?.GetNamedType(), userContext, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this - }), + await AuthorizeAsync(fieldAst, fieldDef.ResolvedType?.GetNamedType(), context).ConfigureAwait(false); + } - new MatchingNodeVisitor((variableRef, context) => + if (node is GraphQLVariable variableRef) { - if (context.TypeInfo.GetArgument()?.ResolvedType?.GetNamedType() is not IComplexGraphType variableType || ShouldBeSkipped(context.Operation, context)) + if (context.TypeInfo.GetArgument()?.ResolvedType?.GetNamedType() is not IComplexGraphType variableType || + await ShouldBeSkipped(context.Operation, context).ConfigureAwait(false)) return; - AuthorizeAsync(variableRef, variableType, userContext, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this + await AuthorizeAsync(variableRef, variableType, context).ConfigureAwait(false); // 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 @@ -152,35 +156,39 @@ public override ValueTask VisitAsync(ASTNode? node, FragmentBelongsToOperationVi { if (fieldsValues.ContainsKey(field.Name)) { - AuthorizeAsync(variableRef, field, userContext, context, operationType).GetAwaiter().GetResult(); // TODO: need to think of something to avoid this + await AuthorizeAsync(variableRef, field, context).ConfigureAwait(false); } } } - }) - ); - } + } + } - private async Task AuthorizeAsync( - ASTNode? node, - IProvideMetadata? provider, - IProvideClaimsPrincipal? userContext, - ValidationContext context, - OperationType? operationType) - { - if (provider == null || !provider.IsAuthorizationRequired()) - return; + public ValueTask LeaveAsync(ASTNode node, ValidationContext context) => default; + + public async ValueTask AuthorizeAsync( + ASTNode? node, + IProvideMetadata? provider, + ValidationContext context) + { + var userContext = context.UserContext as IProvideClaimsPrincipal; + var user = userContext == null ? context.User : userContext.User; + var operationType = context.Operation.Operation; + + if (provider == null || !provider.IsAuthorizationRequired()) + return; - var result = await _evaluator.Evaluate(userContext?.User, context.UserContext, context.Variables, provider.GetPolicies()).ConfigureAwait(false); + var result = await _evaluator.Evaluate(user, context.UserContext, context.Variables, provider.GetPolicies()).ConfigureAwait(false); - if (result.Succeeded) - return; + if (result.Succeeded) + return; - string errors = string.Join("\n", result.Errors); + string errors = string.Join("\n", result.Errors); - context.ReportError(new ValidationError( - context.Document.Source, - "authorization", - $"You are not authorized to run this {operationType.ToString().ToLower()}.\n{errors}", - node == null ? Array.Empty() : new ASTNode[] { node })); + context.ReportError(new ValidationError( + context.Document.Source, + "authorization", + $"You are not authorized to run this {operationType.ToString().ToLower()}.\n{errors}", + node == null ? Array.Empty() : new ASTNode[] { node })); + } } }