From ed076b6cbe5f871d5c90600566fece020481ec32 Mon Sep 17 00:00:00 2001 From: Chris Nissen Date: Tue, 20 Nov 2018 11:13:02 -0700 Subject: [PATCH 1/6] Updated AuthorizationValidationRule to skip authorization checks when the field is not included or skipped due to directives. --- .../AuthorizationValidationRule.cs | 35 ++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/src/GraphQL.Authorization/AuthorizationValidationRule.cs b/src/GraphQL.Authorization/AuthorizationValidationRule.cs index fd26ffb..5922074 100644 --- a/src/GraphQL.Authorization/AuthorizationValidationRule.cs +++ b/src/GraphQL.Authorization/AuthorizationValidationRule.cs @@ -1,4 +1,6 @@ +using System.Linq; using System.Threading.Tasks; +using GraphQL.Execution; using GraphQL.Language.AST; using GraphQL.Types; using GraphQL.Validation; @@ -49,7 +51,7 @@ public Task ValidateAsync(ValidationContext context) { var fieldDef = context.TypeInfo.GetFieldDef(); - if (fieldDef == null) + if (fieldDef == null || SkipAuthCheck(fieldAst, context)) return; // check target field @@ -60,6 +62,37 @@ public Task ValidateAsync(ValidationContext context) })); } + private bool SkipAuthCheck(Field fieldAst, ValidationContext context) + { + if (fieldAst.Directives == null || !fieldAst.Directives.Any()) return true; + + var includeField = GetDirectiveValue(context, fieldAst.Directives, DirectiveGraphType.Include.Name); + if (includeField.HasValue) return !includeField.Value; + + var skipField = GetDirectiveValue(context, fieldAst.Directives, DirectiveGraphType.Skip.Name); + if (skipField.HasValue) return skipField.Value; + + return false; + } + + private static bool? GetDirectiveValue(ValidationContext context, Directives directives, string directiveName) + { + var directive = directives.Find(directiveName); + if (directive == null) return null; + + var operation = !string.IsNullOrWhiteSpace(context.OperationName) + ? context.Document.Operations.WithName(context.OperationName) + : context.Document.Operations.FirstOrDefault(); + var values = ExecutionHelper.GetArgumentValues( + context.Schema, + DirectiveGraphType.Include.Arguments, + directive.Arguments, + ExecutionHelper.GetVariableValues(context.Document, context.Schema, operation?.Variables, context.Inputs)); + + values.TryGetValue("if", out object ifObj); + return bool.TryParse(ifObj?.ToString() ?? string.Empty, out bool ifVal) && ifVal; + } + private void CheckAuth( INode node, IProvideMetadata type, From 573b63d1c6577ce943dc0cb342d2d95df6f39393 Mon Sep 17 00:00:00 2001 From: Chris Nissen Date: Tue, 20 Nov 2018 13:46:25 -0700 Subject: [PATCH 2/6] Added tests; updates from PR feedback. --- .../AuthorizationValidationRuleTests.cs | 60 +++++++++++++++++++ .../AuthorizationValidationRule.cs | 21 ++++--- 2 files changed, 72 insertions(+), 9 deletions(-) diff --git a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs index 5f50ca3..b2015d4 100644 --- a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs +++ b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs @@ -206,6 +206,66 @@ public void fails_on_missing_claim_on_connection_type() }); } + [Fact] + public void passes_when_field_is_not_included() + { + Settings.AddPolicy("FieldPolicy", _ => + { + _.RequireClaim("admin"); + }); + + ShouldPassRule(_ => + { + _.Query = @"query { post @include(if: false) }"; + _.Schema = BasicSchema(); + }); + } + + [Fact] + public void fails_when_field_is_included() + { + Settings.AddPolicy("FieldPolicy", _ => + { + _.RequireClaim("admin"); + }); + + ShouldFailRule(_ => + { + _.Query = @"query { post @include(if: true) }"; + _.Schema = BasicSchema(); + }); + } + + [Fact] + public void passes_when_field_is_skipped() + { + Settings.AddPolicy("FieldPolicy", _ => + { + _.RequireClaim("admin"); + }); + + ShouldPassRule(_ => + { + _.Query = @"query { post @skip(if: true) }"; + _.Schema = BasicSchema(); + }); + } + + [Fact] + public void fails_when_field_is_not_skipped() + { + Settings.AddPolicy("FieldPolicy", _ => + { + _.RequireClaim("admin"); + }); + + ShouldFailRule(_ => + { + _.Query = @"query { post @skip(if: false) }"; + _.Schema = BasicSchema(); + }); + } + private ISchema BasicSchema() { string defs = @" diff --git a/src/GraphQL.Authorization/AuthorizationValidationRule.cs b/src/GraphQL.Authorization/AuthorizationValidationRule.cs index 5922074..6e35730 100644 --- a/src/GraphQL.Authorization/AuthorizationValidationRule.cs +++ b/src/GraphQL.Authorization/AuthorizationValidationRule.cs @@ -64,28 +64,31 @@ public Task ValidateAsync(ValidationContext context) private bool SkipAuthCheck(Field fieldAst, ValidationContext context) { - if (fieldAst.Directives == null || !fieldAst.Directives.Any()) return true; + if (fieldAst.Directives == null || !fieldAst.Directives.Any()) return false; - var includeField = GetDirectiveValue(context, fieldAst.Directives, DirectiveGraphType.Include.Name); + var includeField = GetDirectiveValue(context, fieldAst.Directives, DirectiveGraphType.Include); if (includeField.HasValue) return !includeField.Value; - var skipField = GetDirectiveValue(context, fieldAst.Directives, DirectiveGraphType.Skip.Name); + var skipField = GetDirectiveValue(context, fieldAst.Directives, DirectiveGraphType.Skip); if (skipField.HasValue) return skipField.Value; return false; } - private static bool? GetDirectiveValue(ValidationContext context, Directives directives, string directiveName) + private static bool? GetDirectiveValue(ValidationContext context, Directives directives, DirectiveGraphType directiveType) { - var directive = directives.Find(directiveName); + var directive = directives.Find(directiveType.Name); if (directive == null) return null; - var operation = !string.IsNullOrWhiteSpace(context.OperationName) - ? context.Document.Operations.WithName(context.OperationName) - : context.Document.Operations.FirstOrDefault(); + var operationName = context.OperationName; + var documentOperations = context.Document.Operations; + var operation = !string.IsNullOrWhiteSpace(operationName) + ? documentOperations.WithName(operationName) + : documentOperations.FirstOrDefault(); + var values = ExecutionHelper.GetArgumentValues( context.Schema, - DirectiveGraphType.Include.Arguments, + directiveType.Arguments, directive.Arguments, ExecutionHelper.GetVariableValues(context.Document, context.Schema, operation?.Variables, context.Inputs)); From 0f3667e854d26943568cdb96435e45af69e0690a Mon Sep 17 00:00:00 2001 From: Chris Nissen Date: Tue, 20 Nov 2018 13:55:31 -0700 Subject: [PATCH 3/6] Cleaned up some auto-formatting that happened by accident. --- .../AuthorizationValidationRuleTests.cs | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs index b2015d4..6036e04 100644 --- a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs +++ b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs @@ -13,7 +13,7 @@ public void class_policy_success() Settings.AddPolicy("ClassPolicy", builder => builder.RequireClaim("admin")); Settings.AddPolicy("FieldPolicy", builder => builder.RequireClaim("admin")); - ShouldPassRule(_ => + ShouldPassRule(_=> { _.Query = @"query { post }"; _.Schema = BasicSchema(); @@ -29,7 +29,7 @@ public void class_policy_fail() { Settings.AddPolicy("ClassPolicy", builder => builder.RequireClaim("admin")); - ShouldFailRule(_ => + ShouldFailRule(_=> { _.Query = @"query { post }"; _.Schema = BasicSchema(); @@ -42,7 +42,7 @@ public void field_policy_success() Settings.AddPolicy("ClassPolicy", builder => builder.RequireClaim("admin")); Settings.AddPolicy("FieldPolicy", builder => builder.RequireClaim("admin")); - ShouldPassRule(_ => + ShouldPassRule(_=> { _.Query = @"query { post }"; _.Schema = BasicSchema(); @@ -58,7 +58,7 @@ public void field_policy_fail() { Settings.AddPolicy("FieldPolicy", builder => builder.RequireClaim("admin")); - ShouldFailRule(_ => + ShouldFailRule(_=> { _.Query = @"query { post }"; _.Schema = BasicSchema(); @@ -70,7 +70,7 @@ public void nested_type_policy_success() { Settings.AddPolicy("PostPolicy", builder => builder.RequireClaim("admin")); - ShouldPassRule(_ => + ShouldPassRule(_=> { _.Query = @"query { post }"; _.Schema = NestedSchema(); @@ -98,7 +98,7 @@ public void nested_type_list_policy_fail() { Settings.AddPolicy("PostPolicy", builder => builder.RequireClaim("admin")); - ShouldFailRule(_ => + ShouldFailRule(_=> { _.Query = @"query { posts }"; _.Schema = NestedSchema(); @@ -122,7 +122,7 @@ public void passes_with_claim_on_input_type() { Settings.AddPolicy("FieldPolicy", builder => builder.RequireClaim("admin")); - ShouldPassRule(_ => + ShouldPassRule(_=> { _.Query = @"query { author(input: { name: ""Quinn"" }) }"; _.Schema = TypedSchema(); @@ -209,12 +209,12 @@ public void fails_on_missing_claim_on_connection_type() [Fact] public void passes_when_field_is_not_included() { - Settings.AddPolicy("FieldPolicy", _ => + Settings.AddPolicy("FieldPolicy", _=> { _.RequireClaim("admin"); }); - ShouldPassRule(_ => + ShouldPassRule(_=> { _.Query = @"query { post @include(if: false) }"; _.Schema = BasicSchema(); @@ -224,12 +224,12 @@ public void passes_when_field_is_not_included() [Fact] public void fails_when_field_is_included() { - Settings.AddPolicy("FieldPolicy", _ => + Settings.AddPolicy("FieldPolicy", _=> { _.RequireClaim("admin"); }); - ShouldFailRule(_ => + ShouldFailRule(_=> { _.Query = @"query { post @include(if: true) }"; _.Schema = BasicSchema(); @@ -239,12 +239,12 @@ public void fails_when_field_is_included() [Fact] public void passes_when_field_is_skipped() { - Settings.AddPolicy("FieldPolicy", _ => + Settings.AddPolicy("FieldPolicy", _=> { _.RequireClaim("admin"); }); - ShouldPassRule(_ => + ShouldPassRule(_=> { _.Query = @"query { post @skip(if: true) }"; _.Schema = BasicSchema(); @@ -254,12 +254,12 @@ public void passes_when_field_is_skipped() [Fact] public void fails_when_field_is_not_skipped() { - Settings.AddPolicy("FieldPolicy", _ => + Settings.AddPolicy("FieldPolicy", _=> { _.RequireClaim("admin"); }); - ShouldFailRule(_ => + ShouldFailRule(_=> { _.Query = @"query { post @skip(if: false) }"; _.Schema = BasicSchema(); From 9f7862fce9b495ec7852f5739ea248a1e70d21c2 Mon Sep 17 00:00:00 2001 From: Chris Nissen Date: Wed, 5 Dec 2018 16:45:57 -0700 Subject: [PATCH 4/6] Refactored so as not to calculate the variable values twice. --- .../AuthorizationValidationRule.cs | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/GraphQL.Authorization/AuthorizationValidationRule.cs b/src/GraphQL.Authorization/AuthorizationValidationRule.cs index 6e35730..a58066e 100644 --- a/src/GraphQL.Authorization/AuthorizationValidationRule.cs +++ b/src/GraphQL.Authorization/AuthorizationValidationRule.cs @@ -62,37 +62,39 @@ public Task ValidateAsync(ValidationContext context) })); } - private bool SkipAuthCheck(Field fieldAst, ValidationContext context) + private bool SkipAuthCheck(Field field, ValidationContext context) { - if (fieldAst.Directives == null || !fieldAst.Directives.Any()) return false; + if (field.Directives == null || !field.Directives.Any()) return false; - var includeField = GetDirectiveValue(context, fieldAst.Directives, DirectiveGraphType.Include); + var operationName = context.OperationName; + var documentOperations = context.Document.Operations; + var operation = !string.IsNullOrWhiteSpace(operationName) + ? documentOperations.WithName(operationName) + : documentOperations.FirstOrDefault(); + var variables = ExecutionHelper.GetVariableValues(context.Document, context.Schema, + operation?.Variables, context.Inputs); + + var includeField = GetDirectiveValue(context, field.Directives, DirectiveGraphType.Include, variables); if (includeField.HasValue) return !includeField.Value; - var skipField = GetDirectiveValue(context, fieldAst.Directives, DirectiveGraphType.Skip); + var skipField = GetDirectiveValue(context, field.Directives, DirectiveGraphType.Skip, variables); if (skipField.HasValue) return skipField.Value; return false; } - private static bool? GetDirectiveValue(ValidationContext context, Directives directives, DirectiveGraphType directiveType) + private static bool? GetDirectiveValue(ValidationContext context, Directives directives, DirectiveGraphType directiveType, Variables variables) { var directive = directives.Find(directiveType.Name); if (directive == null) return null; - var operationName = context.OperationName; - var documentOperations = context.Document.Operations; - var operation = !string.IsNullOrWhiteSpace(operationName) - ? documentOperations.WithName(operationName) - : documentOperations.FirstOrDefault(); - - var values = ExecutionHelper.GetArgumentValues( + var argumentValues = ExecutionHelper.GetArgumentValues( context.Schema, directiveType.Arguments, directive.Arguments, - ExecutionHelper.GetVariableValues(context.Document, context.Schema, operation?.Variables, context.Inputs)); + variables); - values.TryGetValue("if", out object ifObj); + argumentValues.TryGetValue("if", out object ifObj); return bool.TryParse(ifObj?.ToString() ?? string.Empty, out bool ifVal) && ifVal; } From b29c7a9d48eaa61680d64bd9af203d9f7b01dcd1 Mon Sep 17 00:00:00 2001 From: Ivan Maximov Date: Mon, 22 Mar 2021 13:27:38 +0300 Subject: [PATCH 5/6] Update AuthorizationValidationRuleTests.cs --- .../AuthorizationValidationRuleTests.cs | 20 ++++--------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs index ba4250d..2fedd7c 100644 --- a/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs +++ b/src/GraphQL.Authorization.Tests/AuthorizationValidationRuleTests.cs @@ -209,10 +209,7 @@ public void fails_on_missing_claim_on_connection_type() [Fact] public void passes_when_field_is_not_included() { - Settings.AddPolicy("FieldPolicy", _=> - { - _.RequireClaim("admin"); - }); + Settings.AddPolicy("FieldPolicy", _ => _.RequireClaim("admin")); ShouldPassRule(_ => { @@ -224,10 +221,7 @@ public void passes_when_field_is_not_included() [Fact] public void fails_when_field_is_included() { - Settings.AddPolicy("FieldPolicy", _=> - { - _.RequireClaim("admin"); - }); + Settings.AddPolicy("FieldPolicy", _ => _.RequireClaim("admin")); ShouldFailRule(_ => { @@ -239,10 +233,7 @@ public void fails_when_field_is_included() [Fact] public void passes_when_field_is_skipped() { - Settings.AddPolicy("FieldPolicy", _=> - { - _.RequireClaim("admin"); - }); + Settings.AddPolicy("FieldPolicy", _ => _.RequireClaim("admin")); ShouldPassRule(_ => { @@ -254,10 +245,7 @@ public void passes_when_field_is_skipped() [Fact] public void fails_when_field_is_not_skipped() { - Settings.AddPolicy("FieldPolicy", _=> - { - _.RequireClaim("admin"); - }); + Settings.AddPolicy("FieldPolicy", _ => _.RequireClaim("admin")); ShouldFailRule(_ => { From ebf44d7f1cff4b6b499c4556b2cee1839226b17b Mon Sep 17 00:00:00 2001 From: Ivan Maximov Date: Mon, 22 Mar 2021 13:33:50 +0300 Subject: [PATCH 6/6] Update AuthorizationValidationRule.cs --- .../AuthorizationValidationRule.cs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/GraphQL.Authorization/AuthorizationValidationRule.cs b/src/GraphQL.Authorization/AuthorizationValidationRule.cs index dddc066..a6f4cd8 100644 --- a/src/GraphQL.Authorization/AuthorizationValidationRule.cs +++ b/src/GraphQL.Authorization/AuthorizationValidationRule.cs @@ -58,7 +58,7 @@ public Task ValidateAsync(ValidationContext context) { var fieldDef = context.TypeInfo.GetFieldDef(); - if (fieldDef == null || SkipAuthCheck(fieldAst, context)) + if (fieldDef == null || SkipAuthCheck(fieldAst, context)) return; // check target field @@ -71,7 +71,8 @@ public Task ValidateAsync(ValidationContext context) private bool SkipAuthCheck(Field field, ValidationContext context) { - if (field.Directives == null || !field.Directives.Any()) return false; + if (field.Directives == null || !field.Directives.Any()) + return false; var operationName = context.OperationName; var documentOperations = context.Document.Operations; @@ -82,10 +83,12 @@ private bool SkipAuthCheck(Field field, ValidationContext context) operation?.Variables, context.Inputs); var includeField = GetDirectiveValue(context, field.Directives, DirectiveGraphType.Include, variables); - if (includeField.HasValue) return !includeField.Value; + if (includeField.HasValue) + return !includeField.Value; var skipField = GetDirectiveValue(context, field.Directives, DirectiveGraphType.Skip, variables); - if (skipField.HasValue) return skipField.Value; + if (skipField.HasValue) + return skipField.Value; return false; } @@ -93,7 +96,8 @@ private bool SkipAuthCheck(Field field, ValidationContext context) private static bool? GetDirectiveValue(ValidationContext context, Directives directives, DirectiveGraphType directiveType, Variables variables) { var directive = directives.Find(directiveType.Name); - if (directive == null) return null; + if (directive == null) + return null; var argumentValues = ExecutionHelper.GetArgumentValues( context.Schema,