From 2831c29351a6be7e86a97d4748c5969cb03e0c9f Mon Sep 17 00:00:00 2001 From: Arijit Das Date: Wed, 16 Sep 2020 16:26:24 +0530 Subject: [PATCH 1/2] fix(GraphQL): Fix cascade with auth query when RBAC is false (#6444) * Fix cascade with auth query. * Added additional E2E test. (cherry picked from commit 8c518d4976cd54e3c5dd0954c9aedf2dcd5ae3e2) --- graphql/e2e/auth/auth_test.go | 61 ++++++++++++++++++++++++++++ graphql/resolve/auth_query_test.yaml | 49 ++++++++++++++++++++++ graphql/resolve/query_rewriter.go | 46 ++++++++++++++++++--- 3 files changed, 151 insertions(+), 5 deletions(-) diff --git a/graphql/e2e/auth/auth_test.go b/graphql/e2e/auth/auth_test.go index be44f66c4f7..f81b510a4e1 100644 --- a/graphql/e2e/auth/auth_test.go +++ b/graphql/e2e/auth/auth_test.go @@ -1244,6 +1244,67 @@ func TestDeleteDeepAuthRule(t *testing.T) { } } +func TestDeepRBACValueCascade(t *testing.T) { + testCases := []TestCase{ + { + user: "user1", + role: "USER", + query: ` + query { + queryUser (filter:{username:{eq:"user1"}}) @cascade { + username + issues { + msg + } + } + }`, + result: `{"queryUser": []}`, + }, + { + user: "user1", + role: "USER", + query: ` + query { + queryUser (filter:{username:{eq:"user1"}}) { + username + issues @cascade { + msg + } + } + }`, + result: `{"queryUser": [{"username": "user1", "issues":[]}]}`, + }, + { + user: "user1", + role: "ADMIN", + query: ` + query { + queryUser (filter:{username:{eq:"user1"}}) @cascade { + username + issues { + msg + } + } + }`, + result: `{"queryUser":[{"username":"user1","issues":[{"msg":"Issue1"}]}]}`, + }, + } + + for _, tcase := range testCases { + t.Run(tcase.role+tcase.user, func(t *testing.T) { + getUserParams := &common.GraphQLParams{ + Headers: common.GetJWT(t, tcase.user, tcase.role, metaInfo), + Query: tcase.query, + } + + gqlResponse := getUserParams.ExecuteAsPost(t, graphqlURL) + require.Nil(t, gqlResponse.Errors) + + require.JSONEq(t, string(gqlResponse.Data), tcase.result) + }) + } +} + func TestMain(m *testing.M) { schemaFile := "schema.graphql" schema, err := ioutil.ReadFile(schemaFile) diff --git a/graphql/resolve/auth_query_test.yaml b/graphql/resolve/auth_query_test.yaml index 2abe138a3d9..2f404272f1a 100644 --- a/graphql/resolve/auth_query_test.yaml +++ b/graphql/resolve/auth_query_test.yaml @@ -101,6 +101,55 @@ Contact5 as var(func: type(Contact)) } +- name: "Deep RBAC rule with cascade - Level 1 false" + gqlquery: | + query { + queryContact @cascade { + id + nickName + adminTasks { + id + name + occurrences { + due + comp + } + } + } + } + jwtvar: + ContactRole: ADMINISTRATOR + TaskRole: User + TaskOccuranceRole: ADMINISTRATOR + dgquery: |- + query { + queryContact(func: uid(ContactRoot)) @cascade { + id : uid + nickName : Contact.nickName + adminTasks : Contact.adminTasks @filter(uid(AdminTask6)) { + id : uid + name : AdminTask.name + occurrences : AdminTask.occurrences @filter(uid(TaskOccurrence4)) { + due : TaskOccurrence.due + comp : TaskOccurrence.comp + dgraph.uid : uid + } + } + } + ContactRoot as var(func: uid(Contact7)) + Contact7 as var(func: type(Contact)) + var(func: uid(ContactRoot)) { + AdminTask1 as Contact.adminTasks + } + AdminTask6 as var(func: uid(AdminTask1)) @filter(uid(AdminTask5)) + var(func: uid(AdminTask1)) { + TaskOccurrence2 as AdminTask.occurrences + } + TaskOccurrence4 as var(func: uid(TaskOccurrence2)) @filter(uid(TaskOccurrenceAuth3)) + TaskOccurrenceAuth3 as var(func: uid(TaskOccurrence2)) @filter(eq(TaskOccurrence.role, "ADMINISTRATOR")) @cascade + AdminTask5 as var(func: uid()) + } + - name: "Deep RBAC rule - Level 2 false" gqlquery: | query { diff --git a/graphql/resolve/query_rewriter.go b/graphql/resolve/query_rewriter.go index c68aad471ff..f6324b7e829 100644 --- a/graphql/resolve/query_rewriter.go +++ b/graphql/resolve/query_rewriter.go @@ -46,6 +46,8 @@ type authRewriter struct { parentVarName string // `hasAuthRules` indicates if any of fields in the complete query hierarchy has auth rules. hasAuthRules bool + // `hasCascade` indicates if any of fields in the complete query hierarchy has cascade directive. + hasCascade bool } // NewQueryRewriter returns a new QueryRewriter. @@ -67,6 +69,19 @@ func hasAuthRules(field schema.Field, authRw *authRewriter) bool { return false } +func hasCascadeDirective(field schema.Field) bool { + if c := field.Cascade(); c != nil { + return true + } + + for _, childField := range field.SelectionSet() { + if res := hasCascadeDirective(childField); res { + return true + } + } + return false +} + // Rewrite rewrites a GraphQL query into a Dgraph GraphQuery. func (qr *queryRewriter) Rewrite( ctx context.Context, @@ -93,6 +108,7 @@ func (qr *queryRewriter) Rewrite( parentVarName: gqlQuery.Type().Name() + "Root", } authRw.hasAuthRules = hasAuthRules(gqlQuery, authRw) + authRw.hasCascade = hasCascadeDirective(gqlQuery) switch gqlQuery.QueryType() { case schema.GetQuery: @@ -748,14 +764,34 @@ func addSelectionSetFrom( q.Children = append(q.Children, child) } - // If RBAC rules are evaluated to Negative, we don't write queries for deeper levels. - // Hence we don't need to do any further processing for this field. - if rbac == schema.Negative { + var fieldAuth []*gql.GraphQuery + var authFilter *gql.FilterTree + if rbac == schema.Negative && auth.hasAuthRules && auth.hasCascade && !auth.isWritingAuth { + // If RBAC rules are evaluated to Negative but we have cascade directive we continue + // to write the query and add a dummy filter that doesn't return anything. + // Example: AdminTask5 as var(func: uid()) + q.Children = append(q.Children, child) + varName := auth.varGen.Next(f.Type(), "", "", auth.isWritingAuth) + fieldAuth = append(fieldAuth, &gql.GraphQuery{ + Var: varName, + Attr: "var", + Func: &gql.Function{ + Name: "uid", + }, + }) + authFilter = &gql.FilterTree{ + Func: &gql.Function{ + Name: "uid", + Args: []gql.Arg{{Value: varName}}, + }, + } + rbac = schema.Positive + } else if rbac == schema.Negative { + // If RBAC rules are evaluated to Negative, we don't write queries for deeper levels. + // Hence we don't need to do any further processing for this field. continue } - var fieldAuth []*gql.GraphQuery - var authFilter *gql.FilterTree // If RBAC rules are evaluated to `Uncertain` then we add the Auth rules. if rbac == schema.Uncertain { fieldAuth, authFilter = auth.rewriteAuthQueries(f.Type()) From 2781a8870a1a912ecf643fcd336a2be71f889f8a Mon Sep 17 00:00:00 2001 From: Arijit Das Date: Mon, 21 Sep 2020 14:45:48 +0530 Subject: [PATCH 2/2] Minor fix. --- graphql/resolve/query_rewriter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphql/resolve/query_rewriter.go b/graphql/resolve/query_rewriter.go index f6324b7e829..a697f0ae658 100644 --- a/graphql/resolve/query_rewriter.go +++ b/graphql/resolve/query_rewriter.go @@ -70,7 +70,7 @@ func hasAuthRules(field schema.Field, authRw *authRewriter) bool { } func hasCascadeDirective(field schema.Field) bool { - if c := field.Cascade(); c != nil { + if c := field.Cascade(); c { return true }