From 29e67640c170c48aba2eb75c732c4aff50a76334 Mon Sep 17 00:00:00 2001 From: SlavaUtesinov Date: Tue, 7 Dec 2021 08:50:35 +0300 Subject: [PATCH 01/12] Authorize only actual operation --- src/BasicSample/BasicSample.csproj | 2 +- .../GraphQL.Authorization.approved.txt | 2 +- .../AuthorizationValidationRuleTests.cs | 117 ++++++++++-------- .../GraphQL.Authorization.Tests.csproj | 2 +- .../ValidationTestBase.cs | 2 +- .../ValidationTestConfig.cs | 2 + .../AuthorizationValidationRule.cs | 47 ++++++- .../GraphQL.Authorization.csproj | 2 +- 8 files changed, 116 insertions(+), 60 deletions(-) diff --git a/src/BasicSample/BasicSample.csproj b/src/BasicSample/BasicSample.csproj index 82eddcb..33fd571 100644 --- a/src/BasicSample/BasicSample.csproj +++ b/src/BasicSample/BasicSample.csproj @@ -11,7 +11,7 @@ - + diff --git a/src/GraphQL.Authorization.ApiTests/GraphQL.Authorization.approved.txt b/src/GraphQL.Authorization.ApiTests/GraphQL.Authorization.approved.txt index e532611..2c38741 100644 --- a/src/GraphQL.Authorization.ApiTests/GraphQL.Authorization.approved.txt +++ b/src/GraphQL.Authorization.ApiTests/GraphQL.Authorization.approved.txt @@ -55,7 +55,7 @@ namespace GraphQL.Authorization public class AuthorizationValidationRule : GraphQL.Validation.IValidationRule { public AuthorizationValidationRule(GraphQL.Authorization.IAuthorizationEvaluator evaluator) { } - public System.Threading.Tasks.Task ValidateAsync(GraphQL.Validation.ValidationContext context) { } + public System.Threading.Tasks.ValueTask ValidateAsync(GraphQL.Validation.ValidationContext context) { } } public class ClaimAuthorizationRequirement : GraphQL.Authorization.IAuthorizationRequirement { diff --git a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs index 1619f2d..cbfd4ee 100644 --- a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs +++ b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs @@ -13,11 +13,11 @@ public void class_policy_success() Settings.AddPolicy("ClassPolicy", builder => builder.RequireClaim("admin")); Settings.AddPolicy("FieldPolicy", builder => builder.RequireClaim("admin")); - ShouldPassRule(_ => + ShouldPassRule(config => { - _.Query = @"query { post }"; - _.Schema = BasicSchema(); - _.User = CreatePrincipal(claims: new Dictionary + config.Query = @"query { post }"; + config.Schema = BasicSchema(); + config.User = CreatePrincipal(claims: new Dictionary { { "Admin", "true" } }); @@ -29,10 +29,10 @@ public void class_policy_fail() { Settings.AddPolicy("ClassPolicy", builder => builder.RequireClaim("admin")); - ShouldFailRule(_ => + ShouldFailRule(config => { - _.Query = @"query { post }"; - _.Schema = BasicSchema(); + config.Query = @"query { post }"; + config.Schema = BasicSchema(); }); } @@ -42,11 +42,11 @@ public void field_policy_success() Settings.AddPolicy("ClassPolicy", builder => builder.RequireClaim("admin")); Settings.AddPolicy("FieldPolicy", builder => builder.RequireClaim("admin")); - ShouldPassRule(_ => + ShouldPassRule(config => { - _.Query = @"query { post }"; - _.Schema = BasicSchema(); - _.User = CreatePrincipal(claims: new Dictionary + config.Query = @"query { post }"; + config.Schema = BasicSchema(); + config.User = CreatePrincipal(claims: new Dictionary { { "Admin", "true" } }); @@ -58,10 +58,10 @@ public void field_policy_fail() { Settings.AddPolicy("FieldPolicy", builder => builder.RequireClaim("admin")); - ShouldFailRule(_ => + ShouldFailRule(config => { - _.Query = @"query { post }"; - _.Schema = BasicSchema(); + config.Query = @"query { post }"; + config.Schema = BasicSchema(); }); } @@ -70,11 +70,11 @@ public void nested_type_policy_success() { Settings.AddPolicy("PostPolicy", builder => builder.RequireClaim("admin")); - ShouldPassRule(_ => + ShouldPassRule(config => { - _.Query = @"query { post }"; - _.Schema = NestedSchema(); - _.User = CreatePrincipal(claims: new Dictionary + config.Query = @"query { post }"; + config.Schema = NestedSchema(); + config.User = CreatePrincipal(claims: new Dictionary { { "Admin", "true" } }); @@ -86,10 +86,10 @@ public void nested_type_policy_fail() { Settings.AddPolicy("PostPolicy", builder => builder.RequireClaim("admin")); - ShouldFailRule(_ => + ShouldFailRule(config => { - _.Query = @"query { post }"; - _.Schema = NestedSchema(); + config.Query = @"query { post }"; + config.Schema = NestedSchema(); }); } @@ -98,10 +98,24 @@ public void nested_type_list_policy_fail() { Settings.AddPolicy("PostPolicy", builder => builder.RequireClaim("admin")); - ShouldFailRule(_ => + ShouldFailRule(config => { - _.Query = @"query { posts }"; - _.Schema = NestedSchema(); + config.Query = @"query { posts }"; + config.Schema = NestedSchema(); + }); + } + + [Fact] + // https://github.com/graphql-dotnet/authorization/issues/5 + public void issue5() + { + Settings.AddPolicy("PostPolicy", builder => builder.RequireClaim("admin")); + + ShouldPassRule(config => + { + config.OperationName = "c"; + config.Query = @"query p { posts } query c { comment }"; + config.Schema = NestedSchema(); }); } @@ -110,10 +124,10 @@ public void nested_type_list_non_null_policy_fail() { Settings.AddPolicy("PostPolicy", builder => builder.RequireClaim("admin")); - ShouldFailRule(_ => + ShouldFailRule(config => { - _.Query = @"query { postsNonNull }"; - _.Schema = NestedSchema(); + config.Query = @"query { postsNonNull }"; + config.Schema = NestedSchema(); }); } @@ -122,11 +136,11 @@ public void passes_with_claim_on_input_type() { Settings.AddPolicy("FieldPolicy", builder => builder.RequireClaim("admin")); - ShouldPassRule(_ => + ShouldPassRule(config => { - _.Query = @"query { author(input: { name: ""Quinn"" }) }"; - _.Schema = TypedSchema(); - _.User = CreatePrincipal(claims: new Dictionary + config.Query = @"query { author(input: { name: ""Quinn"" }) }"; + config.Schema = TypedSchema(); + config.User = CreatePrincipal(claims: new Dictionary { { "Admin", "true" } }); @@ -138,10 +152,10 @@ public void fails_on_missing_claim_on_input_type() { Settings.AddPolicy("FieldPolicy", builder => builder.RequireClaim("admin")); - ShouldFailRule(_ => + ShouldFailRule(config => { - _.Query = @"query { author(input: { name: ""Quinn"" }) }"; - _.Schema = TypedSchema(); + config.Query = @"query { author(input: { name: ""Quinn"" }) }"; + config.Schema = TypedSchema(); }); } @@ -152,11 +166,11 @@ public void passes_with_multiple_policies_on_field_and_single_on_input_type() Settings.AddPolicy("AdminPolicy", builder => builder.RequireClaim("admin")); Settings.AddPolicy("ConfidentialPolicy", builder => builder.RequireClaim("admin")); - ShouldPassRule(_ => + ShouldPassRule(config => { - _.Query = @"query { author(input: { name: ""Quinn"" }) project(input: { name: ""TEST"" }) }"; - _.Schema = TypedSchema(); - _.User = CreatePrincipal(claims: new Dictionary + config.Query = @"query { author(input: { name: ""Quinn"" }) project(input: { name: ""TEST"" }) }"; + config.Schema = TypedSchema(); + config.User = CreatePrincipal(claims: new Dictionary { { "Admin", "true" } }); @@ -166,11 +180,11 @@ public void passes_with_multiple_policies_on_field_and_single_on_input_type() [Fact] public void Issue61() { - ShouldPassRule(_ => + ShouldPassRule(config => { - _.Query = @"query { unknown(obj: {id: 7}) }"; - _.Schema = TypedSchema(); - _.User = CreatePrincipal(claims: new Dictionary + config.Query = @"query { unknown(obj: {id: 7}) }"; + config.Schema = TypedSchema(); + config.User = CreatePrincipal(claims: new Dictionary { { "Admin", "true" } }); @@ -182,11 +196,11 @@ public void passes_with_policy_on_connection_type() { Settings.AddPolicy("ConnectionPolicy", _ => _.RequireClaim("admin")); - ShouldPassRule(_ => + ShouldPassRule(config => { - _.Query = @"query { posts { items { id } } }"; - _.Schema = TypedSchema(); - _.User = CreatePrincipal(claims: new Dictionary + config.Query = @"query { posts { items { id } } }"; + config.Schema = TypedSchema(); + config.User = CreatePrincipal(claims: new Dictionary { { "Admin", "true" } }); @@ -198,11 +212,11 @@ public void fails_on_missing_claim_on_connection_type() { Settings.AddPolicy("ConnectionPolicy", _ => _.RequireClaim("admin")); - ShouldFailRule(_ => + ShouldFailRule(config => { - _.Query = @"query { posts { items { id } } }"; - _.Schema = TypedSchema(); - _.User = CreatePrincipal(); + config.Query = @"query { posts { items { id } } }"; + config.Schema = TypedSchema(); + config.User = CreatePrincipal(); }); } @@ -233,6 +247,7 @@ type Query { post(id: ID!): Post posts: [Post] postsNonNull: [Post!]! + comment: String } type Post { @@ -256,6 +271,8 @@ public class NestedQueryWithAttributes public IEnumerable Posts() => null; public IEnumerable PostsNonNull() => null; + + public string Comment() => null; } [GraphQLAuthorize("PostPolicy")] diff --git a/src/GraphQL.Authorization.Tests/GraphQL.Authorization.Tests.csproj b/src/GraphQL.Authorization.Tests/GraphQL.Authorization.Tests.csproj index f7a6c63..abc4a6f 100644 --- a/src/GraphQL.Authorization.Tests/GraphQL.Authorization.Tests.csproj +++ b/src/GraphQL.Authorization.Tests/GraphQL.Authorization.Tests.csproj @@ -3,7 +3,7 @@ net5;netcoreapp3.1 - 4.2.0 + 5.0.0-preview-362 diff --git a/src/GraphQL.Authorization.Tests/ValidationTestBase.cs b/src/GraphQL.Authorization.Tests/ValidationTestBase.cs index 10efddf..4e09d9c 100644 --- a/src/GraphQL.Authorization.Tests/ValidationTestBase.cs +++ b/src/GraphQL.Authorization.Tests/ValidationTestBase.cs @@ -63,7 +63,7 @@ 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.Inputs).GetAwaiter().GetResult().validationResult; + return validator.ValidateAsync(config.Schema, document, document.Operations.First().Variables, config.Rules, userContext, config.Inputs, config.OperationName).GetAwaiter().GetResult().validationResult; } internal static ClaimsPrincipal CreatePrincipal(string authenticationType = null, IDictionary claims = null) diff --git a/src/GraphQL.Authorization.Tests/ValidationTestConfig.cs b/src/GraphQL.Authorization.Tests/ValidationTestConfig.cs index 9dec7b4..ea34dbc 100644 --- a/src/GraphQL.Authorization.Tests/ValidationTestConfig.cs +++ b/src/GraphQL.Authorization.Tests/ValidationTestConfig.cs @@ -8,6 +8,8 @@ namespace GraphQL.Authorization.Tests { public class ValidationTestConfig { + public string OperationName { get; set; } + public string Query { get; set; } public ISchema Schema { get; set; } diff --git a/src/GraphQL.Authorization/AuthorizationValidationRule.cs b/src/GraphQL.Authorization/AuthorizationValidationRule.cs index 892c542..d0c9353 100644 --- a/src/GraphQL.Authorization/AuthorizationValidationRule.cs +++ b/src/GraphQL.Authorization/AuthorizationValidationRule.cs @@ -1,3 +1,5 @@ +using System; +using System.Linq; using System.Threading.Tasks; using GraphQL.Language.AST; using GraphQL.Types; @@ -22,8 +24,39 @@ public AuthorizationValidationRule(IAuthorizationEvaluator evaluator) _evaluator = evaluator; } + private bool ShouldBeSkipped(ValidationContext context) + { + if (context.Document.Operations.Count <= 1) + { + return false; + } + + var actualOperation = context.Document.Operations.FirstOrDefault(x => x.Name == context.OperationName); + + if (actualOperation == null) + { + return false; + } + + var i = 0; + do + { + try + { + if (context.TypeInfo.GetAncestor(i++) == actualOperation) + { + return false; + } + } + catch (InvalidOperationException) + { + return true; + } + } while (true); + } + /// - public Task ValidateAsync(ValidationContext context) + public ValueTask ValidateAsync(ValidationContext context) { var userContext = context.UserContext as IProvideClaimsPrincipal; var operationType = OperationType.Query; @@ -33,10 +66,14 @@ public Task ValidateAsync(ValidationContext context) // 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 Task.FromResult((INodeVisitor)new NodeVisitors( + return new ValueTask(new NodeVisitors( new MatchingNodeVisitor((astType, context) => { + if (context.Document.Operations.Count > 1 && astType.Name != context.OperationName) + { + return; + } + operationType = astType.OperationType; var type = context.TypeInfo.GetLastType(); @@ -45,7 +82,7 @@ public Task ValidateAsync(ValidationContext context) new MatchingNodeVisitor((objectFieldAst, context) => { - if (context.TypeInfo.GetArgument()?.ResolvedType.GetNamedType() is IComplexGraphType argumentType) + if (context.TypeInfo.GetArgument()?.ResolvedType.GetNamedType() is IComplexGraphType argumentType && !ShouldBeSkipped(context)) { var fieldType = argumentType.GetField(objectFieldAst.Name); CheckAuth(objectFieldAst, fieldType, userContext, context, operationType); @@ -56,7 +93,7 @@ public Task ValidateAsync(ValidationContext context) { var fieldDef = context.TypeInfo.GetFieldDef(); - if (fieldDef == null) + if (fieldDef == null || ShouldBeSkipped(context)) return; // check target field diff --git a/src/GraphQL.Authorization/GraphQL.Authorization.csproj b/src/GraphQL.Authorization/GraphQL.Authorization.csproj index ccd33b9..8ad5c6e 100644 --- a/src/GraphQL.Authorization/GraphQL.Authorization.csproj +++ b/src/GraphQL.Authorization/GraphQL.Authorization.csproj @@ -7,7 +7,7 @@ - + From f2c00309830332152418decf3d9efad26b06d343 Mon Sep 17 00:00:00 2001 From: Ivan Maximov Date: Tue, 7 Dec 2021 12:29:32 +0300 Subject: [PATCH 02/12] Update src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs --- .../AuthorizationValidationRuleTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs index cbfd4ee..a403cb2 100644 --- a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs +++ b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs @@ -105,8 +105,8 @@ public void nested_type_list_policy_fail() }); } - [Fact] // https://github.com/graphql-dotnet/authorization/issues/5 + [Fact] public void issue5() { Settings.AddPolicy("PostPolicy", builder => builder.RequireClaim("admin")); From b9afccad7d18e7943c0158670044c424daea3a06 Mon Sep 17 00:00:00 2001 From: SlavaUtesinov Date: Tue, 7 Dec 2021 12:50:35 +0300 Subject: [PATCH 03/12] major graphqlversion --- .github/workflows/test.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 6c10299..8f99326 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -30,8 +30,7 @@ jobs: - ubuntu-latest - windows-latest graphqlversion: - - 4.2.0 - - 4.4.0 + - 5.0.0-preview-362 steps: - name: Checkout source uses: actions/checkout@v2 From 09c601f4d17b1891f8ffe6e476191afe88ed4b78 Mon Sep 17 00:00:00 2001 From: SlavaUtesinov Date: Tue, 7 Dec 2021 13:28:01 +0300 Subject: [PATCH 04/12] actualOperation fix --- .../AuthorizationValidationRuleTests.cs | 10 ++++++---- .../AuthorizationValidationRule.cs | 7 +------ 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs index a403cb2..0acf4d9 100644 --- a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs +++ b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs @@ -106,15 +106,17 @@ public void nested_type_list_policy_fail() } // https://github.com/graphql-dotnet/authorization/issues/5 - [Fact] - public void issue5() + [Theory] + [InlineData("c", "query p { posts } query c { comment }")] + [InlineData(null, "query c { comment } query p { posts }")] + public void issue5(string operationName, string query) { Settings.AddPolicy("PostPolicy", builder => builder.RequireClaim("admin")); ShouldPassRule(config => { - config.OperationName = "c"; - config.Query = @"query p { posts } query c { comment }"; + config.OperationName = operationName; + config.Query = query; config.Schema = NestedSchema(); }); } diff --git a/src/GraphQL.Authorization/AuthorizationValidationRule.cs b/src/GraphQL.Authorization/AuthorizationValidationRule.cs index d0c9353..04e1d51 100644 --- a/src/GraphQL.Authorization/AuthorizationValidationRule.cs +++ b/src/GraphQL.Authorization/AuthorizationValidationRule.cs @@ -31,12 +31,7 @@ private bool ShouldBeSkipped(ValidationContext context) return false; } - var actualOperation = context.Document.Operations.FirstOrDefault(x => x.Name == context.OperationName); - - if (actualOperation == null) - { - return false; - } + var actualOperation = context.Document.Operations.FirstOrDefault(x => x.Name == context.OperationName) ?? context.Document.Operations.FirstOrDefault(); var i = 0; do From defed21197f4a78e70549a123f2baba4278948cf Mon Sep 17 00:00:00 2001 From: SlavaUtesinov Date: Tue, 7 Dec 2021 15:17:45 +0300 Subject: [PATCH 05/12] codecov.yaml --- src/GraphQL.Authorization.sln | 1 + src/codecov.yaml | 3 +++ 2 files changed, 4 insertions(+) create mode 100644 src/codecov.yaml diff --git a/src/GraphQL.Authorization.sln b/src/GraphQL.Authorization.sln index 8f48539..b38a1aa 100644 --- a/src/GraphQL.Authorization.sln +++ b/src/GraphQL.Authorization.sln @@ -21,6 +21,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = ".github", ".github", "{84514A09-9BB4-4C85-8A8E-92AF5AA26445}" ProjectSection(SolutionItems) = preProject + codecov.yaml = codecov.yaml ..\.github\dependabot.yml = ..\.github\dependabot.yml ..\.github\FUNDING.yml = ..\.github\FUNDING.yml ..\.github\labeler.yml = ..\.github\labeler.yml diff --git a/src/codecov.yaml b/src/codecov.yaml new file mode 100644 index 0000000..877c249 --- /dev/null +++ b/src/codecov.yaml @@ -0,0 +1,3 @@ +# https://docs.codecov.com/docs/codecov-yaml +comment: + behavior: new From 14734f7f559b6bbd9a803e12616f94822609cbde Mon Sep 17 00:00:00 2001 From: SlavaUtesinov Date: Tue, 7 Dec 2021 15:19:45 +0300 Subject: [PATCH 06/12] codecov.yaml location fix --- {src => .github}/codecov.yaml | 0 src/GraphQL.Authorization.sln | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename {src => .github}/codecov.yaml (100%) diff --git a/src/codecov.yaml b/.github/codecov.yaml similarity index 100% rename from src/codecov.yaml rename to .github/codecov.yaml diff --git a/src/GraphQL.Authorization.sln b/src/GraphQL.Authorization.sln index b38a1aa..b29d818 100644 --- a/src/GraphQL.Authorization.sln +++ b/src/GraphQL.Authorization.sln @@ -21,7 +21,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = ".github", ".github", "{84514A09-9BB4-4C85-8A8E-92AF5AA26445}" ProjectSection(SolutionItems) = preProject - codecov.yaml = codecov.yaml + ..\.github\codecov.yaml = ..\.github\codecov.yaml ..\.github\dependabot.yml = ..\.github\dependabot.yml ..\.github\FUNDING.yml = ..\.github\FUNDING.yml ..\.github\labeler.yml = ..\.github\labeler.yml From 4a16e8470904c4c8f7308b7f89d46430ab551b3b Mon Sep 17 00:00:00 2001 From: Ivan Maximov Date: Tue, 7 Dec 2021 15:36:14 +0300 Subject: [PATCH 07/12] Update src/GraphQL.Authorization/AuthorizationValidationRule.cs --- src/GraphQL.Authorization/AuthorizationValidationRule.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/GraphQL.Authorization/AuthorizationValidationRule.cs b/src/GraphQL.Authorization/AuthorizationValidationRule.cs index 04e1d51..f09da15 100644 --- a/src/GraphQL.Authorization/AuthorizationValidationRule.cs +++ b/src/GraphQL.Authorization/AuthorizationValidationRule.cs @@ -38,6 +38,7 @@ private bool ShouldBeSkipped(ValidationContext context) { try { + // TODO: rewrite if (context.TypeInfo.GetAncestor(i++) == actualOperation) { return false; From 589e95179c482caa2158a455f3be301fdf75339a Mon Sep 17 00:00:00 2001 From: SlavaUtesinov Date: Tue, 7 Dec 2021 17:38:45 +0300 Subject: [PATCH 08/12] codeql-analysis.yml fix --- .github/workflows/codeql-analysis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index c22c60a..51bf5c3 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=4.4.0 + run: dotnet restore -p:GraphQLTestVersion=5.0.0-preview-362 - name: Build solution working-directory: src From 83b3fd9d30d80f0019913cf00a904d65731f1a89 Mon Sep 17 00:00:00 2001 From: SlavaUtesinov Date: Wed, 8 Dec 2021 08:23:51 +0300 Subject: [PATCH 09/12] fragment --- .../AuthorizationValidationRuleTests.cs | 51 ++++++++++++++++++- .../AuthorizationValidationRule.cs | 42 ++++++++++++--- 2 files changed, 84 insertions(+), 9 deletions(-) diff --git a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs index 0acf4d9..aedaa67 100644 --- a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs +++ b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs @@ -1,4 +1,5 @@ using System.Collections.Generic; +using System.Linq; using GraphQL.Types; using GraphQL.Types.Relay.DataObjects; using Xunit; @@ -109,7 +110,7 @@ public void nested_type_list_policy_fail() [Theory] [InlineData("c", "query p { posts } query c { comment }")] [InlineData(null, "query c { comment } query p { posts }")] - public void issue5(string operationName, string query) + public void issue5_should_pass(string operationName, string query) { Settings.AddPolicy("PostPolicy", builder => builder.RequireClaim("admin")); @@ -121,6 +122,33 @@ public void issue5(string operationName, string query) }); } + // https://github.com/graphql-dotnet/authorization/issues/5 + [Fact] + public void issue5_with_fragment_should_pass() + { + Settings.AddPolicy("PostPolicy", builder => builder.RequireClaim("admin")); + + ShouldPassRule(config => + { + config.Query = "query a { article { id } } query b { article { ...frag } } fragment frag on Article { content }"; + config.Schema = TypedSchema(); + }); + } + + // https://github.com/graphql-dotnet/authorization/issues/5 + [Fact] + public void issue5_with_fragment_should_fail() + { + Settings.AddPolicy("PostPolicy", builder => builder.RequireClaim("admin")); + + ShouldFailRule(config => + { + config.Query = "query a { article { ...frag } } query b { article { ...frag } } fragment frag on Article { content }"; + config.Schema = TypedSchema(); + config.ValidateResult = result => result.Errors.Single(x => x.Message == $"You are not authorized to run this query.\nRequired policy 'AdminPolicy' is not present."); + }); + } + [Fact] public void nested_type_list_non_null_policy_fail() { @@ -291,6 +319,22 @@ public PostGraphType() } } + public class Article + { + public string Id { get; set; } + + public string Content { get; set; } + } + + public class ArticleGraphType : ObjectGraphType
+ { + public ArticleGraphType() + { + Field(p => p.Id); + Field(p => p.Content).AuthorizeWith("AdminPolicy"); + } + } + public class Author { public string Name { get; set; } @@ -316,6 +360,11 @@ private static ISchema TypedSchema() resolve: context => "testing" ).AuthorizeWith("AdminPolicy").AuthorizeWith("ConfidentialPolicy"); + query.Field( + "article", + resolve: context => null + ); + return new Schema { Query = query }; } diff --git a/src/GraphQL.Authorization/AuthorizationValidationRule.cs b/src/GraphQL.Authorization/AuthorizationValidationRule.cs index f09da15..5dac479 100644 --- a/src/GraphQL.Authorization/AuthorizationValidationRule.cs +++ b/src/GraphQL.Authorization/AuthorizationValidationRule.cs @@ -1,4 +1,3 @@ -using System; using System.Linq; using System.Threading.Tasks; using GraphQL.Language.AST; @@ -36,21 +35,48 @@ private bool ShouldBeSkipped(ValidationContext context) var i = 0; do { - try + var ancestor = context.TypeInfo.GetAncestor(i++); + + if (ancestor == actualOperation) { - // TODO: rewrite - if (context.TypeInfo.GetAncestor(i++) == actualOperation) - { - return false; - } + return false; } - catch (InvalidOperationException) + + if (ancestor == context.Document) { return true; } + + if (ancestor is FragmentDefinition fragment) + { + return !FragmentBelongsToOperation(fragment, actualOperation); + } } while (true); } + private bool FragmentBelongsToOperation(FragmentDefinition fragment, Operation operation) + { + var belongs = false; + void Visit(INode node, int _) + { + if (belongs) + { + return; + } + + belongs = node is FragmentSpread fragmentSpread && fragmentSpread.Name == fragment.Name; + + if (node != null) + { + node.Visit(Visit, 0); + } + } + + operation.Visit(Visit, 0); + + return belongs; + } + /// public ValueTask ValidateAsync(ValidationContext context) { From ce2d24a32a9399057fa3c9fa81c3633f5e193f09 Mon Sep 17 00:00:00 2001 From: SlavaUtesinov Date: Wed, 8 Dec 2021 08:47:10 +0300 Subject: [PATCH 10/12] new test --- .../AuthorizationValidationRuleTests.cs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs index aedaa67..34faf8f 100644 --- a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs +++ b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs @@ -123,14 +123,16 @@ public void issue5_should_pass(string operationName, string query) } // https://github.com/graphql-dotnet/authorization/issues/5 - [Fact] - public void issue5_with_fragment_should_pass() + [Theory] + [InlineData("query a { article { id } } query b { article { ...frag } } fragment frag on Article { content }")] + [InlineData("query a { article { ...frag1 author } } query b { article { ...frag2 } } fragment frag1 on Article { id } fragment frag2 on Article { content }")] + public void issue5_with_fragment_should_pass(string query) { Settings.AddPolicy("PostPolicy", builder => builder.RequireClaim("admin")); ShouldPassRule(config => { - config.Query = "query a { article { id } } query b { article { ...frag } } fragment frag on Article { content }"; + config.Query = query; config.Schema = TypedSchema(); }); } @@ -323,6 +325,8 @@ public class Article { public string Id { get; set; } + public string Author { get; set; } + public string Content { get; set; } } @@ -331,6 +335,7 @@ public class ArticleGraphType : ObjectGraphType
public ArticleGraphType() { Field(p => p.Id); + Field(p => p.Author); Field(p => p.Content).AuthorizeWith("AdminPolicy"); } } From e78de8c036cae24ba59bf92fe87646b50e0db981 Mon Sep 17 00:00:00 2001 From: SlavaUtesinov Date: Wed, 8 Dec 2021 14:32:55 +0300 Subject: [PATCH 11/12] test fix --- .../AuthorizationValidationRuleTests.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs index 34faf8f..fbd36a9 100644 --- a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs +++ b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs @@ -128,7 +128,7 @@ public void issue5_should_pass(string operationName, string query) [InlineData("query a { article { ...frag1 author } } query b { article { ...frag2 } } fragment frag1 on Article { id } fragment frag2 on Article { content }")] public void issue5_with_fragment_should_pass(string query) { - Settings.AddPolicy("PostPolicy", builder => builder.RequireClaim("admin")); + Settings.AddPolicy("AdminPolicy", builder => builder.RequireClaim("admin")); ShouldPassRule(config => { @@ -141,13 +141,13 @@ public void issue5_with_fragment_should_pass(string query) [Fact] public void issue5_with_fragment_should_fail() { - Settings.AddPolicy("PostPolicy", builder => builder.RequireClaim("admin")); + Settings.AddPolicy("AdminPolicy", builder => builder.RequireClaim("admin")); ShouldFailRule(config => { config.Query = "query a { article { ...frag } } query b { article { ...frag } } fragment frag on Article { content }"; config.Schema = TypedSchema(); - config.ValidateResult = result => result.Errors.Single(x => x.Message == $"You are not authorized to run this query.\nRequired policy 'AdminPolicy' is not present."); + config.ValidateResult = result => result.Errors.Single(x => x.Message == $"You are not authorized to run this query.\nRequired claim 'admin' is not present."); }); } From 5c766a7ec323debe28841db590c3fe408cf4c338 Mon Sep 17 00:00:00 2001 From: SlavaUtesinov Date: Wed, 8 Dec 2021 15:05:22 +0300 Subject: [PATCH 12/12] actualOperation --- src/GraphQL.Authorization/AuthorizationValidationRule.cs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/GraphQL.Authorization/AuthorizationValidationRule.cs b/src/GraphQL.Authorization/AuthorizationValidationRule.cs index 5dac479..a9dc909 100644 --- a/src/GraphQL.Authorization/AuthorizationValidationRule.cs +++ b/src/GraphQL.Authorization/AuthorizationValidationRule.cs @@ -23,15 +23,13 @@ public AuthorizationValidationRule(IAuthorizationEvaluator evaluator) _evaluator = evaluator; } - private bool ShouldBeSkipped(ValidationContext context) + private bool ShouldBeSkipped(Operation actualOperation, ValidationContext context) { if (context.Document.Operations.Count <= 1) { return false; } - var actualOperation = context.Document.Operations.FirstOrDefault(x => x.Name == context.OperationName) ?? context.Document.Operations.FirstOrDefault(); - var i = 0; do { @@ -82,6 +80,7 @@ public ValueTask ValidateAsync(ValidationContext context) { var userContext = context.UserContext as IProvideClaimsPrincipal; 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 @@ -104,7 +103,7 @@ public ValueTask ValidateAsync(ValidationContext context) new MatchingNodeVisitor((objectFieldAst, context) => { - if (context.TypeInfo.GetArgument()?.ResolvedType.GetNamedType() is IComplexGraphType argumentType && !ShouldBeSkipped(context)) + if (context.TypeInfo.GetArgument()?.ResolvedType.GetNamedType() is IComplexGraphType argumentType && !ShouldBeSkipped(actualOperation, context)) { var fieldType = argumentType.GetField(objectFieldAst.Name); CheckAuth(objectFieldAst, fieldType, userContext, context, operationType); @@ -115,7 +114,7 @@ public ValueTask ValidateAsync(ValidationContext context) { var fieldDef = context.TypeInfo.GetFieldDef(); - if (fieldDef == null || ShouldBeSkipped(context)) + if (fieldDef == null || ShouldBeSkipped(actualOperation, context)) return; // check target field