From a62643de5297fc2c404a45a54ae63f304874f79b Mon Sep 17 00:00:00 2001 From: Aniruddh Munde Date: Mon, 18 Jul 2022 07:27:48 -0700 Subject: [PATCH 1/3] Do not add authorize directive if one of the allowed roles is anonymous --- .../Mutations/CreateMutationBuilder.cs | 9 ++++++++- .../Mutations/DeleteMutationBuilder.cs | 9 ++++++++- .../Mutations/UpdateMutationBuilder.cs | 9 ++++++++- .../Queries/QueryBuilder.cs | 17 +++++++++++++++-- 4 files changed, 39 insertions(+), 5 deletions(-) diff --git a/DataGateway.Service.GraphQLBuilder/Mutations/CreateMutationBuilder.cs b/DataGateway.Service.GraphQLBuilder/Mutations/CreateMutationBuilder.cs index 44593c110f..da635006b5 100644 --- a/DataGateway.Service.GraphQLBuilder/Mutations/CreateMutationBuilder.cs +++ b/DataGateway.Service.GraphQLBuilder/Mutations/CreateMutationBuilder.cs @@ -238,7 +238,14 @@ public static FieldDefinitionNode Build( // Create authorize directive denoting allowed roles List fieldDefinitionNodeDirectives = new(); - if (rolesAllowedForMutation is not null) + + // Any roles passed in will be added to the authorize directive for this field + // taking the form: @authorize(roles: [“role1”, ..., “roleN”]) + // If the 'anonymous' role is present in the role list, no @authorize directive will be added + // because HotChocolate requires an authenticated user when the authorize directive is evaluated. + if (rolesAllowedForMutation is not null && + rolesAllowedForMutation.Count() >= 1 && + !rolesAllowedForMutation.Contains(SYSTEM_ROLE_ANONYMOUS)) { fieldDefinitionNodeDirectives.Add(CreateAuthorizationDirective(rolesAllowedForMutation)); } diff --git a/DataGateway.Service.GraphQLBuilder/Mutations/DeleteMutationBuilder.cs b/DataGateway.Service.GraphQLBuilder/Mutations/DeleteMutationBuilder.cs index 223ad5ac26..7e5cc5df08 100644 --- a/DataGateway.Service.GraphQLBuilder/Mutations/DeleteMutationBuilder.cs +++ b/DataGateway.Service.GraphQLBuilder/Mutations/DeleteMutationBuilder.cs @@ -43,7 +43,14 @@ public static FieldDefinitionNode Build(NameNode name, ObjectTypeDefinitionNode // Create authorize directive denoting allowed roles List fieldDefinitionNodeDirectives = new(); - if (rolesAllowedForMutation is not null) + + // Any roles passed in will be added to the authorize directive for this field + // taking the form: @authorize(roles: [“role1”, ..., “roleN”]) + // If the 'anonymous' role is present in the role list, no @authorize directive will be added + // because HotChocolate requires an authenticated user when the authorize directive is evaluated. + if (rolesAllowedForMutation is not null && + rolesAllowedForMutation.Count() >= 1 && + !rolesAllowedForMutation.Contains(SYSTEM_ROLE_ANONYMOUS)) { fieldDefinitionNodeDirectives.Add(CreateAuthorizationDirective(rolesAllowedForMutation)); } diff --git a/DataGateway.Service.GraphQLBuilder/Mutations/UpdateMutationBuilder.cs b/DataGateway.Service.GraphQLBuilder/Mutations/UpdateMutationBuilder.cs index d6a8a729dd..bf3121d88d 100644 --- a/DataGateway.Service.GraphQLBuilder/Mutations/UpdateMutationBuilder.cs +++ b/DataGateway.Service.GraphQLBuilder/Mutations/UpdateMutationBuilder.cs @@ -220,7 +220,14 @@ public static FieldDefinitionNode Build( // Create authorize directive denoting allowed roles List fieldDefinitionNodeDirectives = new(); - if (rolesAllowedForMutation is not null) + + // Any roles passed in will be added to the authorize directive for this field + // taking the form: @authorize(roles: [“role1”, ..., “roleN”]) + // If the 'anonymous' role is present in the role list, no @authorize directive will be added + // because HotChocolate requires an authenticated user when the authorize directive is evaluated. + if (rolesAllowedForMutation is not null && + rolesAllowedForMutation.Count() >= 1 && + !rolesAllowedForMutation.Contains(SYSTEM_ROLE_ANONYMOUS)) { fieldDefinitionNodeDirectives.Add(CreateAuthorizationDirective(rolesAllowedForMutation)); } diff --git a/DataGateway.Service.GraphQLBuilder/Queries/QueryBuilder.cs b/DataGateway.Service.GraphQLBuilder/Queries/QueryBuilder.cs index 5f974d8b5b..2fb83b3c92 100644 --- a/DataGateway.Service.GraphQLBuilder/Queries/QueryBuilder.cs +++ b/DataGateway.Service.GraphQLBuilder/Queries/QueryBuilder.cs @@ -77,7 +77,13 @@ public static FieldDefinitionNode GenerateByPKQuery(ObjectTypeDefinitionNode obj List inputValues = new(); List fieldDefinitionNodeDirectives = new(); - if (rolesAllowedForRead is not null) + // Any roles passed in will be added to the authorize directive for this field + // taking the form: @authorize(roles: [“role1”, ..., “roleN”]) + // If the 'anonymous' role is present in the role list, no @authorize directive will be added + // because HotChocolate requires an authenticated user when the authorize directive is evaluated. + if (rolesAllowedForRead is not null && + rolesAllowedForRead.Count() >= 1 && + !rolesAllowedForRead.Contains(SYSTEM_ROLE_ANONYMOUS)) { fieldDefinitionNodeDirectives.Add(CreateAuthorizationDirective(rolesAllowedForRead)); } @@ -126,7 +132,14 @@ public static FieldDefinitionNode GenerateGetAllQuery( } List fieldDefinitionNodeDirectives = new(); - if (rolesAllowedForRead is not null) + + // Any roles passed in will be added to the authorize directive for this field + // taking the form: @authorize(roles: [“role1”, ..., “roleN”]) + // If the 'anonymous' role is present in the role list, no @authorize directive will be added + // because HotChocolate requires an authenticated user when the authorize directive is evaluated. + if (rolesAllowedForRead is not null && + rolesAllowedForRead.Count() >= 1 && + !rolesAllowedForRead.Contains(SYSTEM_ROLE_ANONYMOUS)) { fieldDefinitionNodeDirectives.Add(CreateAuthorizationDirective(rolesAllowedForRead)); } From b48cb52b316ed05860a5edb4a637ac5afe5914fb Mon Sep 17 00:00:00 2001 From: Aniruddh Munde Date: Mon, 18 Jul 2022 08:13:14 -0700 Subject: [PATCH 2/3] Add unit tests --- .../GraphQLBuilder/MutationBuilderTests.cs | 72 ++++++++++++++++--- .../GraphQLBuilder/QueryBuilderTests.cs | 49 ++++++++++--- 2 files changed, 102 insertions(+), 19 deletions(-) diff --git a/DataGateway.Service.Tests/GraphQLBuilder/MutationBuilderTests.cs b/DataGateway.Service.Tests/GraphQLBuilder/MutationBuilderTests.cs index e230ed53cc..dee9e34a2a 100644 --- a/DataGateway.Service.Tests/GraphQLBuilder/MutationBuilderTests.cs +++ b/DataGateway.Service.Tests/GraphQLBuilder/MutationBuilderTests.cs @@ -40,10 +40,18 @@ private static Entity GenerateEmptyEntity() return new Entity("dbo.entity", Rest: null, GraphQL: null, Array.Empty(), Relationships: new(), Mappings: new()); } - [TestMethod] + [DataTestMethod] [TestCategory("Mutation Builder - Create")] [TestCategory("Schema Builder - Simple Type")] - public void CanGenerateCreateMutationWith_SimpleType() + [DataRow(new string[] { "authenticated" }, true, + DisplayName = "Validates @authorize directive is added.")] + [DataRow(new string[] { "anonymous" }, false, + DisplayName = "Validates @authorize directive is NOT added.")] + [DataRow(new string[] { "anonymous", "authenticated" }, false, + DisplayName = "Validates @authorize directive is NOT added - multiple roles")] + public void CanGenerateCreateMutationWith_SimpleType( + IEnumerable roles, + bool isAuthorizeDirectiveExpected) { string gql = @" @@ -54,15 +62,23 @@ type Foo @model { "; DocumentNode root = Utf8GraphQLParser.Parse(gql); - + Dictionary entityPermissionsMap + = GraphQLTestHelpers.CreateStubEntityPermissionsMap( + new string[] { "Foo" }, + new string[] { ActionType.CREATE }, + roles); DocumentNode mutationRoot = MutationBuilder.Build(root, DatabaseType.cosmos, new Dictionary { { "Foo", GenerateEmptyEntity() } }, - entityPermissionsMap: _entityPermissions + entityPermissionsMap: entityPermissionsMap ); ObjectTypeDefinitionNode query = GetMutationNode(mutationRoot); Assert.AreEqual(1, query.Fields.Count(f => f.Name.Value == $"createFoo")); + FieldDefinitionNode createField = + query.Fields.Where(f => f.Name.Value == $"createFoo").First(); + Assert.AreEqual(expected: isAuthorizeDirectiveExpected ? 1 : 0, + actual: createField.Directives.Count); } [TestMethod] @@ -565,10 +581,18 @@ type Foo @model { Assert.AreEqual(1, query.Fields.Count(f => f.Name.Value == $"deleteFoo")); } - [TestMethod] + [DataTestMethod] [TestCategory("Mutation Builder - Delete")] [TestCategory("Schema Builder - Simple Type")] - public void DeleteMutationIdAsInput_SimpleType() + [DataRow(new string[] { "authenticated" }, true, + DisplayName = "Validates @authorize directive is added.")] + [DataRow(new string[] { "anonymous" }, false, + DisplayName = "Validates @authorize directive is NOT added.")] + [DataRow(new string[] { "anonymous", "authenticated" }, false, + DisplayName = "Validates @authorize directive is NOT added - multiple roles")] + public void DeleteMutationIdAsInput_SimpleType( + IEnumerable roles, + bool isAuthorizeDirectiveExpected) { string gql = @" @@ -579,7 +603,11 @@ type Foo @model { "; DocumentNode root = Utf8GraphQLParser.Parse(gql); - + Dictionary entityPermissionsMap + = GraphQLTestHelpers.CreateStubEntityPermissionsMap( + new string[] { "Foo" }, + new string[] { ActionType.DELETE }, + roles); DocumentNode mutationRoot = MutationBuilder.Build(root, DatabaseType.cosmos, new Dictionary { { "Foo", GenerateEmptyEntity() } }, @@ -595,6 +623,11 @@ type Foo @model { Assert.AreEqual("_partitionKeyValue", field.Arguments[1].Name.Value); Assert.AreEqual("String", field.Arguments[1].Type.NamedType().Name.Value); Assert.IsTrue(field.Arguments[1].Type.IsNonNullType()); + + FieldDefinitionNode deleteField = + query.Fields.Where(f => f.Name.Value == $"deleteFoo").First(); + Assert.AreEqual(expected: isAuthorizeDirectiveExpected ? 1 : 0, + actual: deleteField.Directives.Count); } [TestMethod] @@ -654,10 +687,18 @@ type Foo @model { Assert.AreEqual("bar", argType.Fields[1].Name.Value); } - [TestMethod] + [DataTestMethod] [TestCategory("Mutation Builder - Update")] [TestCategory("Schema Builder - Simple Type")] - public void UpdateMutationIdFieldAsArgument_SimpleType() + [DataRow(new string[] { "authenticated" }, true, + DisplayName = "Validates @authorize directive is added.")] + [DataRow(new string[] { "anonymous" }, false, + DisplayName = "Validates @authorize directive is NOT added.")] + [DataRow(new string[] { "anonymous", "authenticated" }, false, + DisplayName = "Validates @authorize directive is NOT added - multiple roles")] + public void UpdateMutationIdFieldAsArgument_SimpleType( + IEnumerable roles, + bool isAuthorizeDirectiveExpected) { string gql = @" @@ -668,12 +709,16 @@ type Foo @model { "; DocumentNode root = Utf8GraphQLParser.Parse(gql); - + Dictionary entityPermissionsMap + = GraphQLTestHelpers.CreateStubEntityPermissionsMap( + new string[] { "Foo" }, + new string[] { ActionType.UPDATE }, + roles); DocumentNode mutationRoot = MutationBuilder.Build( root, DatabaseType.cosmos, new Dictionary { { "Foo", GenerateEmptyEntity() } }, - entityPermissionsMap: _entityPermissions + entityPermissionsMap: entityPermissionsMap ); ObjectTypeDefinitionNode query = GetMutationNode(mutationRoot); @@ -685,6 +730,11 @@ type Foo @model { Assert.AreEqual("_partitionKeyValue", field.Arguments[1].Name.Value); Assert.AreEqual("String", field.Arguments[1].Type.NamedType().Name.Value); Assert.IsTrue(field.Arguments[1].Type.IsNonNullType()); + + FieldDefinitionNode collectionField = + query.Fields.Where(f => f.Name.Value == $"updateFoo").First(); + Assert.AreEqual(expected: isAuthorizeDirectiveExpected ? 1 : 0, + actual: collectionField.Directives.Count); } [TestMethod] diff --git a/DataGateway.Service.Tests/GraphQLBuilder/QueryBuilderTests.cs b/DataGateway.Service.Tests/GraphQLBuilder/QueryBuilderTests.cs index 68c0180848..6470d9ec68 100644 --- a/DataGateway.Service.Tests/GraphQLBuilder/QueryBuilderTests.cs +++ b/DataGateway.Service.Tests/GraphQLBuilder/QueryBuilderTests.cs @@ -1,4 +1,5 @@ using System.Collections.Generic; +using System.ComponentModel.DataAnnotations; using System.Linq; using Azure.DataGateway.Auth; using Azure.DataGateway.Config; @@ -34,10 +35,18 @@ public void SetupEntityPermissionsMap() ); } - [TestMethod] + [DataTestMethod] [TestCategory("Query Generation")] [TestCategory("Single item access")] - public void CanGenerateByPKQuery() + [DataRow(new string[] { "authenticated" }, true, + DisplayName = "Validates @authorize directive is added.")] + [DataRow(new string[] { "anonymous" }, false, + DisplayName = "Validates @authorize directive is NOT added.")] + [DataRow(new string[] { "anonymous", "authenticated" }, false, + DisplayName = "Validates @authorize directive is NOT added - multiple roles")] + public void CanGenerateByPKQuery( + IEnumerable roles, + bool isAuthorizeDirectiveExpected) { string gql = @" @@ -47,17 +56,25 @@ type Foo @model { "; DocumentNode root = Utf8GraphQLParser.Parse(gql); - + Dictionary entityPermissionsMap + = GraphQLTestHelpers.CreateStubEntityPermissionsMap( + new string[] { "Foo" }, + new string[] { ActionType.READ }, + roles); DocumentNode queryRoot = QueryBuilder.Build( root, DatabaseType.cosmos, new Dictionary { { "Foo", GraphQLTestHelpers.GenerateEmptyEntity() } }, inputTypes: new(), - entityPermissionsMap: _entityPermissions + entityPermissionsMap: entityPermissionsMap ); ObjectTypeDefinitionNode query = GetQueryNode(queryRoot); Assert.AreEqual(1, query.Fields.Count(f => f.Name.Value == $"foo_by_pk")); + FieldDefinitionNode pkField = + query.Fields.Where(f => f.Name.Value == $"foo_by_pk").First(); + Assert.AreEqual(expected: isAuthorizeDirectiveExpected ? 1 : 0, + actual: pkField.Directives.Count); } [TestMethod] @@ -93,10 +110,18 @@ type Foo @model { Assert.AreEqual("String", args.First(a => a.Name.Value == "_partitionKeyValue").Type.InnerType().NamedType().Name.Value); } - [TestMethod] + [DataTestMethod] [TestCategory("Query Generation")] [TestCategory("Collection access")] - public void CanGenerateCollectionQuery() + [DataRow(new string[] { "authenticated" }, true, + DisplayName = "Validates @authorize directive is added.")] + [DataRow(new string[] { "anonymous" }, false, + DisplayName = "Validates @authorize directive is NOT added.")] + [DataRow(new string[] { "anonymous", "authenticated" }, false, + DisplayName = "Validates @authorize directive is NOT added - multiple roles")] + public void CanGenerateCollectionQuery( + IEnumerable roles, + bool isAuthorizeDirectiveExpected) { string gql = @" @@ -106,17 +131,25 @@ type Foo @model { "; DocumentNode root = Utf8GraphQLParser.Parse(gql); - + Dictionary entityPermissionsMap + = GraphQLTestHelpers.CreateStubEntityPermissionsMap( + new string[] { "Foo" }, + new string[] { ActionType.READ }, + roles); DocumentNode queryRoot = QueryBuilder.Build( root, DatabaseType.cosmos, new Dictionary { { "Foo", GraphQLTestHelpers.GenerateEmptyEntity() } }, inputTypes: new(), - entityPermissionsMap: _entityPermissions + entityPermissionsMap: entityPermissionsMap ); ObjectTypeDefinitionNode query = GetQueryNode(queryRoot); Assert.AreEqual(1, query.Fields.Count(f => f.Name.Value == $"foos")); + FieldDefinitionNode collectionField = + query.Fields.Where(f => f.Name.Value == $"foos").First(); + Assert.AreEqual(expected: isAuthorizeDirectiveExpected ? 1 : 0, + actual: collectionField.Directives.Count); } [TestMethod] From 414013927e2c779b137dc48428a7827c429d0fcd Mon Sep 17 00:00:00 2001 From: Aniruddh Munde Date: Mon, 18 Jul 2022 08:16:32 -0700 Subject: [PATCH 3/3] Fix delete test --- .../GraphQLBuilder/MutationBuilderTests.cs | 4 ++-- DataGateway.Service.Tests/GraphQLBuilder/QueryBuilderTests.cs | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/DataGateway.Service.Tests/GraphQLBuilder/MutationBuilderTests.cs b/DataGateway.Service.Tests/GraphQLBuilder/MutationBuilderTests.cs index dee9e34a2a..e4c1aa2b86 100644 --- a/DataGateway.Service.Tests/GraphQLBuilder/MutationBuilderTests.cs +++ b/DataGateway.Service.Tests/GraphQLBuilder/MutationBuilderTests.cs @@ -611,7 +611,7 @@ Dictionary entityPermissionsMap DocumentNode mutationRoot = MutationBuilder.Build(root, DatabaseType.cosmos, new Dictionary { { "Foo", GenerateEmptyEntity() } }, - entityPermissionsMap: _entityPermissions + entityPermissionsMap: entityPermissionsMap ); ObjectTypeDefinitionNode query = GetMutationNode(mutationRoot); @@ -625,7 +625,7 @@ Dictionary entityPermissionsMap Assert.IsTrue(field.Arguments[1].Type.IsNonNullType()); FieldDefinitionNode deleteField = - query.Fields.Where(f => f.Name.Value == $"deleteFoo").First(); + query.Fields.Where(f => f.Name.Value == $"deleteFoo").First(); Assert.AreEqual(expected: isAuthorizeDirectiveExpected ? 1 : 0, actual: deleteField.Directives.Count); } diff --git a/DataGateway.Service.Tests/GraphQLBuilder/QueryBuilderTests.cs b/DataGateway.Service.Tests/GraphQLBuilder/QueryBuilderTests.cs index 6470d9ec68..513230f38a 100644 --- a/DataGateway.Service.Tests/GraphQLBuilder/QueryBuilderTests.cs +++ b/DataGateway.Service.Tests/GraphQLBuilder/QueryBuilderTests.cs @@ -1,5 +1,4 @@ using System.Collections.Generic; -using System.ComponentModel.DataAnnotations; using System.Linq; using Azure.DataGateway.Auth; using Azure.DataGateway.Config;