From be9ebd0586e29b8edbe12b2783faf3422ca4bd40 Mon Sep 17 00:00:00 2001 From: Arijit Das Date: Wed, 2 Sep 2020 17:56:04 +0530 Subject: [PATCH] fix(GraphQL): Fix query rewriting for auth delete when deleting types with inverse field. (#6350) * Fix query rewriting for auth delete when deleting types with inverse field. --- graphql/e2e/auth/auth_test.go | 18 ++++-- graphql/e2e/auth/delete_mutation_test.go | 64 ++++++++++++++++++++ graphql/e2e/auth/schema.graphql | 14 +++++ graphql/resolve/auth_delete_test.yaml | 72 ++++++++++++++++++++++ graphql/resolve/mutation_rewriter.go | 77 +++++++++++++----------- 5 files changed, 205 insertions(+), 40 deletions(-) diff --git a/graphql/e2e/auth/auth_test.go b/graphql/e2e/auth/auth_test.go index 07757e0018e..bdd323f7f1a 100644 --- a/graphql/e2e/auth/auth_test.go +++ b/graphql/e2e/auth/auth_test.go @@ -43,6 +43,13 @@ var ( metaInfo *testutil.AuthMeta ) +type Tweets struct { + Id string `json:"id,omitempty"` + Text string `json:"text,omitempty"` + Timestamp string `json:"timestamp,omitempty"` + User User `json:"user,omitempty"` +} + type User struct { Username string `json:"username,omitempty"` Age uint64 `json:"age,omitempty"` @@ -127,8 +134,8 @@ type Task struct { } type TaskOccurrence struct { - Id string `json:"id,omitempty"` - Due string `json:"due,omitempty"` + Id string `json:"id,omitempty"` + Due string `json:"due,omitempty"` Comp string `json:"comp,omitempty"` } @@ -149,6 +156,7 @@ type uidResult struct { } type Tasks []Task + func (tasks Tasks) add(t *testing.T) { getParams := &common.GraphQLParams{ Query: ` @@ -432,7 +440,7 @@ func TestAuthRulesWithMissingJWT(t *testing.T) { func TestOrderAndOffset(t *testing.T) { tasks := Tasks{ Task{ - Name: "First Task four occurrence", + Name: "First Task four occurrence", Occurrences: []*TaskOccurrence{ {Due: "2020-07-19T08:00:00", Comp: "2020-07-19T08:00:00"}, {Due: "2020-07-19T08:00:00", Comp: "2020-07-19T08:00:00"}, @@ -441,7 +449,7 @@ func TestOrderAndOffset(t *testing.T) { }, }, Task{ - Name: "Second Task single occurrence", + Name: "Second Task single occurrence", Occurrences: []*TaskOccurrence{ {Due: "2020-07-19T08:00:00", Comp: "2020-07-19T08:00:00"}, }, @@ -451,7 +459,7 @@ func TestOrderAndOffset(t *testing.T) { Occurrences: []*TaskOccurrence{}, }, Task{ - Name: "Fourth Task two occurrences", + Name: "Fourth Task two occurrences", Occurrences: []*TaskOccurrence{ {Due: "2020-07-19T08:00:00", Comp: "2020-07-19T08:00:00"}, {Due: "2020-07-19T08:00:00", Comp: "2020-07-19T08:00:00"}, diff --git a/graphql/e2e/auth/delete_mutation_test.go b/graphql/e2e/auth/delete_mutation_test.go index 20e8562523f..e458e26eec0 100644 --- a/graphql/e2e/auth/delete_mutation_test.go +++ b/graphql/e2e/auth/delete_mutation_test.go @@ -361,3 +361,67 @@ func TestDeleteNestedFilter(t *testing.T) { }) } } + +func TestDeleteRBACRuleInverseField(t *testing.T) { + mutation := ` + mutation addTweets($tweet: AddTweetsInput!){ + addTweets(input: [$tweet]) { + numUids + } + } + ` + + addTweetsParams := &common.GraphQLParams{ + Headers: getJWT(t, "foo", ""), + Query: mutation, + Variables: map[string]interface{}{"tweet": Tweets{ + Id: "tweet1", + Text: "abc", + Timestamp: "2020-10-10", + User: User{ + Username: "foo", + }, + }}, + } + + gqlResponse := addTweetsParams.ExecuteAsPost(t, graphqlURL) + require.Nil(t, gqlResponse.Errors) + + testCases := []TestCase{ + { + user: "foobar", + result: `{"deleteTweets":{"numUids":0,"tweets":[]}}`, + }, + { + user: "foo", + result: `{"deleteTweets":{"numUids":1,"tweets":[ {"text": "abc"}]}}`, + }, + } + + mutation = ` + mutation { + deleteTweets( + filter: { + text: {anyoftext: "abc"} + }) { + numUids + tweets { + text + } + } + } + ` + + for _, tcase := range testCases { + t.Run(tcase.role+tcase.user, func(t *testing.T) { + deleteTweetsParams := &common.GraphQLParams{ + Headers: getJWT(t, tcase.user, tcase.role), + Query: mutation, + } + + gqlResponse := deleteTweetsParams.ExecuteAsPost(t, graphqlURL) + require.Nil(t, gqlResponse.Errors) + require.JSONEq(t, string(gqlResponse.Data), tcase.result) + }) + } +} diff --git a/graphql/e2e/auth/schema.graphql b/graphql/e2e/auth/schema.graphql index e49e3a07a05..6588f07dda9 100644 --- a/graphql/e2e/auth/schema.graphql +++ b/graphql/e2e/auth/schema.graphql @@ -23,6 +23,20 @@ type User @auth( tickets: [Ticket] @hasInverse(field: assignedTo) secrets: [UserSecret] issues: [Issue] + tweets: [Tweets] @hasInverse(field: user) +} + +type Tweets @auth ( + add: { rule: "{$USER: { eq: \"foo\" } }"}, + delete: { rule: "{$USER: { eq: \"foo\" } }"}, + update: { rule: "{$USER: { eq: \"foo\" } }"} +){ + id: String! @id + text: String! @search(by: [fulltext]) + user: User + timestamp: DateTime! @search + score: Int @search + streams: String @search } type UserSecret @auth( diff --git a/graphql/resolve/auth_delete_test.yaml b/graphql/resolve/auth_delete_test.yaml index 453da1efcb6..fc39fef4206 100644 --- a/graphql/resolve/auth_delete_test.yaml +++ b/graphql/resolve/auth_delete_test.yaml @@ -24,6 +24,73 @@ UserSecretAuth2 as var(func: uid(UserSecret1)) @filter(eq(UserSecret.ownedBy, "user1")) @cascade } +- name: "Delete with inverse field and RBAC true" + gqlquery: | + mutation { + deleteTweets( + filter: { + text: {anyoftext: "abc"} + }) { + tweets { + text + } + } + } + jwtvar: + USER: "foo" + dgmutations: + - deletejson: | + [ + { "uid": "uid(x)" }, + { + "User.tweets" : [{"uid":"uid(x)"}], + "uid" : "uid(User2)" + } + ] + dgquery: |- + query { + x as deleteTweets(func: uid(TweetsRoot)) { + uid + User2 as Tweets.user + } + TweetsRoot as var(func: uid(Tweets1)) + Tweets1 as var(func: type(Tweets)) @filter(anyoftext(Tweets.text, "abc")) + tweets(func: uid(Tweets3)) { + text : Tweets.text + dgraph.uid : uid + } + Tweets3 as var(func: uid(Tweets4)) + Tweets4 as var(func: uid(x)) + } + +- name: "Delete with inverse field and RBAC false" + gqlquery: | + mutation { + deleteTweets( + filter: { + text: {anyoftext: "abc"} + }) { + tweets { + text + } + } + } + dgmutations: + - deletejson: | + [ + { "uid": "uid(x)" } + ] + dgquery: |- + query { + x as deleteTweets() + tweets(func: uid(Tweets1)) { + text : Tweets.text + dgraph.uid : uid + } + Tweets1 as var(func: uid(Tweets2)) + Tweets2 as var(func: uid(x)) + } + - name: "Delete with deep auth" gqlquery: | mutation deleteTicket($filter: TicketFilter!) { @@ -288,6 +355,10 @@ { "Ticket.assignedTo" : [ {"uid":"uid(x)"} ], "uid" : "uid(Ticket4)" + }, + { + "Tweets.user" : {"uid":"uid(x)"}, + "uid" : "uid(Tweets5)" } ] dgquery: |- @@ -295,6 +366,7 @@ x as deleteUser(func: uid(UserRoot)) { uid Ticket4 as User.tickets + Tweets5 as User.tweets } UserRoot as var(func: uid(User1)) @filter((uid(UserAuth2) AND uid(UserAuth3))) User1 as var(func: type(User)) @filter(eq(User.username, "userxyz")) diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index dab922f4c68..c5c34153a68 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -670,6 +670,43 @@ func RewriteUpsertQueryFromMutation(m schema.Mutation, authRw *authRewriter) *gq return dgQuery } +// removeNodeReference removes any reference we know about (via @hasInverse) into a node. +func removeNodeReference(m schema.Mutation, authRw *authRewriter, + qry *gql.GraphQuery) []interface{} { + var deletes []interface{} + for _, fld := range m.MutatedType().Fields() { + invField := fld.Inverse() + if invField == nil { + // This field be a reverse edge, in that case we need to delete the incoming connections + // to this node via its forward edges. + invField = fld.ForwardEdge() + if invField == nil { + continue + } + } + varName := authRw.varGen.Next(fld.Type(), "", "", false) + + qry.Children = append(qry.Children, + &gql.GraphQuery{ + Var: varName, + Attr: invField.Type().DgraphPredicate(fld.Name()), + }) + + delFldName := fld.Type().DgraphPredicate(invField.Name()) + del := map[string]interface{}{"uid": MutationQueryVarUID} + if invField.Type().ListType() == nil { + deletes = append(deletes, map[string]interface{}{ + "uid": fmt.Sprintf("uid(%s)", varName), + delFldName: del}) + } else { + deletes = append(deletes, map[string]interface{}{ + "uid": fmt.Sprintf("uid(%s)", varName), + delFldName: []interface{}{del}}) + } + } + return deletes +} + func (drw *deleteRewriter) Rewrite( ctx context.Context, m schema.Mutation) ([]*UpsertMutation, error) { @@ -703,44 +740,14 @@ func (drw *deleteRewriter) Rewrite( } deletes := []interface{}{map[string]interface{}{"uid": "uid(x)"}} - - // we need to delete this node with ^^ and then any reference we know about - // (via @hasInverse) into this node. - for _, fld := range m.MutatedType().Fields() { - invField := fld.Inverse() - if invField == nil { - // This field be a reverse edge, in that case we need to delete the incoming connections - // to this node via its forward edges. - invField = fld.ForwardEdge() - if invField == nil { - continue - } - } - varName := varGen.Next(fld.Type(), "", "", false) - - qry.Children = append(qry.Children, - &gql.GraphQuery{ - Var: varName, - Attr: invField.Type().DgraphPredicate(fld.Name()), - }) - - delFldName := fld.Type().DgraphPredicate(invField.Name()) - del := map[string]interface{}{"uid": MutationQueryVarUID} - if invField.Type().ListType() == nil { - deletes = append(deletes, - map[string]interface{}{ - "uid": fmt.Sprintf("uid(%s)", varName), - delFldName: del}) - } else { - deletes = append(deletes, - map[string]interface{}{ - "uid": fmt.Sprintf("uid(%s)", varName), - delFldName: []interface{}{del}}) - } + // We need to remove node reference only if auth rule succeeds. + if qry.Attr != m.ResponseName()+"()" { + // We need to delete the node and then any reference we know about (via @hasInverse) + // into this node. + deletes = append(deletes, removeNodeReference(m, authRw, qry)...) } b, err := json.Marshal(deletes) - var finalQry *gql.GraphQuery // This rewrites the Upsert mutation so we can query the nodes before deletion. The query result // is later added to delete mutation result.