From a5bfd4a9ce87a36fb6e4fc073cd651b81f9299e8 Mon Sep 17 00:00:00 2001 From: Arijit Das Date: Tue, 18 Aug 2020 18:44:32 +0530 Subject: [PATCH] fix(GraphQL): Fix order and offset in auth queries. (#6221) * Fix order and offset in auth queries. * Remove order from top-level auth filter. --- graphql/e2e/auth/auth_test.go | 163 ++++++++++++++++++++++++++ graphql/e2e/auth/schema.graphql | 29 ++--- graphql/resolve/auth_delete_test.yaml | 2 +- graphql/resolve/auth_query_test.yaml | 66 +++++------ graphql/resolve/query_rewriter.go | 14 +-- 5 files changed, 218 insertions(+), 56 deletions(-) diff --git a/graphql/e2e/auth/auth_test.go b/graphql/e2e/auth/auth_test.go index cc21dcde262..fa0944d5484 100644 --- a/graphql/e2e/auth/auth_test.go +++ b/graphql/e2e/auth/auth_test.go @@ -120,6 +120,18 @@ type Student struct { Email string `json:"email,omitempty"` } +type Task struct { + Id string `json:"id,omitempty"` + Name string `json:"name,omitempty"` + Occurrences []*TaskOccurrence `json:"occurrences,omitempty"` +} + +type TaskOccurrence struct { + Id string `json:"id,omitempty"` + Due string `json:"due,omitempty"` + Comp string `json:"comp,omitempty"` +} + type TestCase struct { user string role string @@ -136,6 +148,22 @@ type uidResult struct { } } +type Tasks []Task +func (tasks Tasks) add(t *testing.T) { + getParams := &common.GraphQLParams{ + Query: ` + mutation AddTask($tasks : [AddTaskInput!]!) { + addTask(input: $tasks) { + numUids + } + } + `, + Variables: map[string]interface{}{"tasks": tasks}, + } + gqlResponse := getParams.ExecuteAsPost(t, graphqlURL) + require.Nil(t, gqlResponse.Errors) +} + func (r *Region) add(t *testing.T, user, role string) { getParams := &common.GraphQLParams{ Headers: getJWT(t, user, role), @@ -401,6 +429,141 @@ func TestAuthRulesWithMissingJWT(t *testing.T) { } } +func TestOrderAndOffset(t *testing.T) { + tasks := Tasks{ + Task{ + 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"}, + {Due: "2020-07-19T08:00:00", Comp: "2020-07-19T08:00:00"}, + {Due: "2020-07-19T08:00:00", Comp: "2020-07-19T08:00:00"}, + }, + }, + Task{ + Name: "Second Task single occurrence", + Occurrences: []*TaskOccurrence{ + {Due: "2020-07-19T08:00:00", Comp: "2020-07-19T08:00:00"}, + }, + }, + Task{ + Name: "Third Task no occurrence", + Occurrences: []*TaskOccurrence{}, + }, + Task{ + 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"}, + }, + }, + } + tasks.add(t) + + query := ` + query { + queryTask(first: 4, order: {asc : name}) { + name + occurrences(first: 2) { + due + comp + } + } + } + ` + testCases := []TestCase{{ + user: "user1", + role: "ADMIN", + result: ` + { + "queryTask": [ + { + "name": "First Task four occurrence", + "occurrences": [ + { + "due": "2020-07-19T08:00:00Z", + "comp": "2020-07-19T08:00:00Z" + }, + { + "due": "2020-07-19T08:00:00Z", + "comp": "2020-07-19T08:00:00Z" + } + ] + }, + { + "name": "Fourth Task two occurrences", + "occurrences": [ + { + "due": "2020-07-19T08:00:00Z", + "comp": "2020-07-19T08:00:00Z" + }, + { + "due": "2020-07-19T08:00:00Z", + "comp": "2020-07-19T08:00:00Z" + } + ] + }, + { + "name": "Second Task single occurrence", + "occurrences": [ + { + "due": "2020-07-19T08:00:00Z", + "comp": "2020-07-19T08:00:00Z" + } + ] + }, + { + "name": "Third Task no occurrence", + "occurrences": [] + } + ] + } + `, + }} + + for _, tcase := range testCases { + t.Run(tcase.role+tcase.user, func(t *testing.T) { + getUserParams := &common.GraphQLParams{ + Headers: getJWT(t, tcase.user, tcase.role), + Query: query, + } + + gqlResponse := getUserParams.ExecuteAsPost(t, graphqlURL) + require.Nil(t, gqlResponse.Errors) + + require.JSONEq(t, string(gqlResponse.Data), tcase.result) + }) + } + + // Clean up `Task` + getParams := &common.GraphQLParams{ + Query: ` + mutation DelTask { + deleteTask(filter: {}) { + numUids + } + } + `, + Variables: map[string]interface{}{"tasks": tasks}, + } + gqlResponse := getParams.ExecuteAsPost(t, graphqlURL) + require.Nil(t, gqlResponse.Errors) + + // Clean up `TaskOccurrence` + getParams = &common.GraphQLParams{ + Query: ` + mutation DelTaskOccuerence { + deleteTaskOccurrence(filter: {}) { + numUids + } + } + `, + Variables: map[string]interface{}{"tasks": tasks}, + } + gqlResponse = getParams.ExecuteAsPost(t, graphqlURL) + require.Nil(t, gqlResponse.Errors) +} + func TestOrRBACFilter(t *testing.T) { testCases := []TestCase{{ user: "user1", diff --git a/graphql/e2e/auth/schema.graphql b/graphql/e2e/auth/schema.graphql index 28951209a20..e49e3a07a05 100644 --- a/graphql/e2e/auth/schema.graphql +++ b/graphql/e2e/auth/schema.graphql @@ -537,34 +537,35 @@ type AdminTask @auth( ) { id: ID! name: String @search(by: [exact, term, fulltext, regexp]) - occurrances: [TaskOccurance] @hasInverse(field: adminTask) + occurrences: [TaskOccurrence] @hasInverse(field: adminTask) forContact: Contact @hasInverse(field: adminTasks) } type Task { id: ID! name: String @search(by: [exact, term, fulltext, regexp]) - occurrances: [TaskOccurance] @hasInverse(field: task) + occurrences: [TaskOccurrence] @hasInverse(field: task) forContact: Contact @hasInverse(field: tasks) } -type TaskOccurance @auth( - query: { and : [ - {rule: "{$TaskOccuranceRole: { eq: \"ADMINISTRATOR\"}}"}, - {rule: """ - query($TaskOccuranceRole: String!) { - queryTaskOccurance(filter: {role: { eq: $TaskOccuranceRole}}) { - __typename +type TaskOccurrence @auth( + query: { or : [ { rule: "{$ROLE: { eq: \"ADMIN\" }}"}, + {and : [ + {rule: "{$TaskOccuranceRole: { eq: \"ADMINISTRATOR\"}}"}, + {rule: """ + query($TaskOccuranceRole: String!) { + queryTaskOccurrence(filter: {role: { eq: $TaskOccuranceRole}}) { + __typename + } } - } - """} -] } + """} +] } ] } ) { id: ID! due: DateTime @search comp: DateTime @search - task: Task @hasInverse(field: occurrances) - adminTask: AdminTask @hasInverse(field: occurrances) + task: Task @hasInverse(field: occurrences) + adminTask: AdminTask @hasInverse(field: occurrences) isPublic: Boolean @search role: String @search(by: [exact, term, fulltext, regexp]) } diff --git a/graphql/resolve/auth_delete_test.yaml b/graphql/resolve/auth_delete_test.yaml index f8837eb2e2a..453da1efcb6 100644 --- a/graphql/resolve/auth_delete_test.yaml +++ b/graphql/resolve/auth_delete_test.yaml @@ -395,7 +395,7 @@ random : Log.random dgraph.uid : uid } - Log2 as var(func: uid(Log3), orderasc: Log.logs) + Log2 as var(func: uid(Log3)) Log3 as var(func: uid(x)) } diff --git a/graphql/resolve/auth_query_test.yaml b/graphql/resolve/auth_query_test.yaml index c099f1913c3..2abe138a3d9 100644 --- a/graphql/resolve/auth_query_test.yaml +++ b/graphql/resolve/auth_query_test.yaml @@ -7,7 +7,7 @@ adminTasks { id name - occurrances { + occurrences { due comp } @@ -26,9 +26,9 @@ adminTasks : Contact.adminTasks @filter(uid(AdminTask5)) { id : uid name : AdminTask.name - occurrances : AdminTask.occurrances @filter(uid(TaskOccurance4)) { - due : TaskOccurance.due - comp : TaskOccurance.comp + occurrences : AdminTask.occurrences @filter(uid(TaskOccurrence4)) { + due : TaskOccurrence.due + comp : TaskOccurrence.comp dgraph.uid : uid } } @@ -40,10 +40,10 @@ } AdminTask5 as var(func: uid(AdminTask1)) var(func: uid(AdminTask1)) { - TaskOccurance2 as AdminTask.occurrances + TaskOccurrence2 as AdminTask.occurrences } - TaskOccurance4 as var(func: uid(TaskOccurance2)) @filter(uid(TaskOccuranceAuth3)) - TaskOccuranceAuth3 as var(func: uid(TaskOccurance2)) @filter(eq(TaskOccurance.role, "ADMINISTRATOR")) @cascade + TaskOccurrence4 as var(func: uid(TaskOccurrence2)) @filter(uid(TaskOccurrenceAuth3)) + TaskOccurrenceAuth3 as var(func: uid(TaskOccurrence2)) @filter(eq(TaskOccurrence.role, "ADMINISTRATOR")) @cascade } - name: "Deep RBAC rule - Level 0 false" @@ -55,7 +55,7 @@ adminTasks { id name - occurrances { + occurrences { due comp } @@ -80,7 +80,7 @@ adminTasks { id name - occurrances { + occurrences { due comp } @@ -110,7 +110,7 @@ adminTasks { id name - occurrances { + occurrences { due comp } @@ -148,7 +148,7 @@ tasks { id name - occurrances { + occurrences { due comp } @@ -167,9 +167,9 @@ tasks : Contact.tasks @filter(uid(Task5)) { id : uid name : Task.name - occurrances : Task.occurrances @filter(uid(TaskOccurance4)) { - due : TaskOccurance.due - comp : TaskOccurance.comp + occurrences : Task.occurrences @filter(uid(TaskOccurrence4)) { + due : TaskOccurrence.due + comp : TaskOccurrence.comp dgraph.uid : uid } } @@ -181,10 +181,10 @@ } Task5 as var(func: uid(Task1)) var(func: uid(Task1)) { - TaskOccurance2 as Task.occurrances + TaskOccurrence2 as Task.occurrences } - TaskOccurance4 as var(func: uid(TaskOccurance2)) @filter(uid(TaskOccuranceAuth3)) - TaskOccuranceAuth3 as var(func: uid(TaskOccurance2)) @filter(eq(TaskOccurance.role, "ADMINISTRATOR")) @cascade + TaskOccurrence4 as var(func: uid(TaskOccurrence2)) @filter(uid(TaskOccurrenceAuth3)) + TaskOccurrenceAuth3 as var(func: uid(TaskOccurrence2)) @filter(eq(TaskOccurrence.role, "ADMINISTRATOR")) @cascade } - name: "Auth query with @dgraph pred." @@ -584,11 +584,11 @@ USER: "user1" dgquery: |- query { - queryUserSecret(func: uid(UserSecretRoot), orderasc: UserSecret.aSecret) { + queryUserSecret(func: uid(UserSecretRoot), orderasc: UserSecret.aSecret, first: 1) { id : uid ownedBy : UserSecret.ownedBy } - UserSecretRoot as var(func: uid(UserSecret1), orderasc: UserSecret.aSecret, first: 1) @filter(uid(UserSecretAuth2)) + UserSecretRoot as var(func: uid(UserSecret1)) @filter(uid(UserSecretAuth2)) UserSecret1 as var(func: type(UserSecret)) @filter(eq(UserSecret.ownedBy, "user2")) UserSecretAuth2 as var(func: uid(UserSecret1)) @filter(eq(UserSecret.ownedBy, "user1")) @cascade } @@ -725,11 +725,11 @@ USER: "user1" dgquery: |- query { - queryMovie(func: uid(MovieRoot), orderasc: Movie.content) { + queryMovie(func: uid(MovieRoot), orderasc: Movie.content, first: 10, offset: 10) { content : Movie.content dgraph.uid : uid } - MovieRoot as var(func: uid(Movie1), orderasc: Movie.content, first: 10, offset: 10) @filter((NOT (uid(MovieAuth2)) AND (uid(MovieAuth3) OR uid(MovieAuth4)))) + MovieRoot as var(func: uid(Movie1)) @filter((NOT (uid(MovieAuth2)) AND (uid(MovieAuth3) OR uid(MovieAuth4)))) Movie1 as var(func: type(Movie)) @filter(eq(Movie.content, "A. N. Author")) MovieAuth2 as var(func: uid(Movie1)) @filter(eq(Movie.hidden, true)) @cascade MovieAuth3 as var(func: uid(Movie1)) @cascade { @@ -760,16 +760,16 @@ USER: "user1" dgquery: |- query { - queryMovie(func: uid(MovieRoot), orderasc: Movie.content) @cascade { + queryMovie(func: uid(MovieRoot), orderasc: Movie.content, first: 10, offset: 10) @cascade { content : Movie.content - regionsAvailable : Movie.regionsAvailable @filter(uid(Region2)) (orderasc: Region.name) { + regionsAvailable : Movie.regionsAvailable @filter(uid(Region2)) (orderasc: Region.name, first: 10, offset: 10) { name : Region.name global : Region.global dgraph.uid : uid } dgraph.uid : uid } - MovieRoot as var(func: uid(Movie3), orderasc: Movie.content, first: 10, offset: 10) @filter((NOT (uid(MovieAuth4)) AND (uid(MovieAuth5) OR uid(MovieAuth6)))) + MovieRoot as var(func: uid(Movie3)) @filter((NOT (uid(MovieAuth4)) AND (uid(MovieAuth5) OR uid(MovieAuth6)))) Movie3 as var(func: type(Movie)) @filter(eq(Movie.content, "MovieXYZ")) MovieAuth4 as var(func: uid(Movie3)) @filter(eq(Movie.hidden, true)) @cascade MovieAuth5 as var(func: uid(Movie3)) @cascade { @@ -786,7 +786,7 @@ var(func: uid(MovieRoot)) { Region1 as Movie.regionsAvailable } - Region2 as var(func: uid(Region1), orderasc: Region.name, first: 10, offset: 10) @filter(eq(Region.name, "Region123")) + Region2 as var(func: uid(Region1)) @filter(eq(Region.name, "Region123")) } - name: "Auth deep query - 3 level" @@ -813,16 +813,16 @@ USER: "user1" dgquery: |- query { - queryMovie(func: uid(MovieRoot), orderasc: Movie.content) { + queryMovie(func: uid(MovieRoot), orderasc: Movie.content, first: 10, offset: 10) { content : Movie.content - regionsAvailable : Movie.regionsAvailable @filter(uid(Region7)) (orderasc: Region.name) @cascade { + regionsAvailable : Movie.regionsAvailable @filter(uid(Region7)) (orderasc: Region.name, first: 10, offset: 10) @cascade { name : Region.name global : Region.global - users : Region.users @filter(uid(User6)) (orderasc: User.username) { + users : Region.users @filter(uid(User6)) (orderasc: User.username, first: 10, offset: 10) { username : User.username age : User.age isPublic : User.isPublic - secrets : User.secrets @filter(uid(UserSecret5)) (orderasc: UserSecret.aSecret) { + secrets : User.secrets @filter(uid(UserSecret5)) (orderasc: UserSecret.aSecret, first: 10, offset: 10) { aSecret : UserSecret.aSecret ownedBy : UserSecret.ownedBy dgraph.uid : uid @@ -833,7 +833,7 @@ } dgraph.uid : uid } - MovieRoot as var(func: uid(Movie8), orderasc: Movie.content, first: 10, offset: 10) @filter((NOT (uid(MovieAuth9)) AND (uid(MovieAuth10) OR uid(MovieAuth11)))) + MovieRoot as var(func: uid(Movie8)) @filter((NOT (uid(MovieAuth9)) AND (uid(MovieAuth10) OR uid(MovieAuth11)))) Movie8 as var(func: type(Movie)) @filter(eq(Movie.content, "MovieXYZ")) MovieAuth9 as var(func: uid(Movie8)) @filter(eq(Movie.hidden, true)) @cascade MovieAuth10 as var(func: uid(Movie8)) @cascade { @@ -850,15 +850,15 @@ var(func: uid(MovieRoot)) { Region1 as Movie.regionsAvailable } - Region7 as var(func: uid(Region1), orderasc: Region.name, first: 10, offset: 10) @filter(eq(Region.name, "Region123")) + Region7 as var(func: uid(Region1)) @filter(eq(Region.name, "Region123")) var(func: uid(Region1)) { User2 as Region.users } - User6 as var(func: uid(User2), orderasc: User.username, first: 10, offset: 10) @filter(eq(User.username, "User321")) + User6 as var(func: uid(User2)) @filter(eq(User.username, "User321")) var(func: uid(User2)) { UserSecret3 as User.secrets } - UserSecret5 as var(func: uid(UserSecret3), orderasc: UserSecret.aSecret, first: 10, offset: 10) @filter((allofterms(UserSecret.aSecret, "Secret132") AND uid(UserSecretAuth4))) + UserSecret5 as var(func: uid(UserSecret3)) @filter((allofterms(UserSecret.aSecret, "Secret132") AND uid(UserSecretAuth4))) UserSecretAuth4 as var(func: uid(UserSecret3)) @filter(eq(UserSecret.ownedBy, "user1")) @cascade } diff --git a/graphql/resolve/query_rewriter.go b/graphql/resolve/query_rewriter.go index 21cde2e5fa1..d4c53b1fcc8 100644 --- a/graphql/resolve/query_rewriter.go +++ b/graphql/resolve/query_rewriter.go @@ -268,6 +268,11 @@ func addArgumentsToField(dgQuery *gql.GraphQuery, field schema.Field) { addPagination(dgQuery, field) } +func addFilterToField(dgQuery *gql.GraphQuery, field schema.Field) { + filter, _ := field.ArgValue("filter").(map[string]interface{}) + addFilter(dgQuery, field.Type(), filter) +} + func addTopLevelTypeFilter(query *gql.GraphQuery, field schema.Field) { if query.Attr != "" { addTypeFilter(query, field.Type()) @@ -453,12 +458,9 @@ func (authRw *authRewriter) addAuthQueries( Args: []gql.Arg{{Value: authRw.varName}}, }, Filter: filter, - Order: dgQuery.Order, - Args: dgQuery.Args, } dgQuery.Filter = nil - dgQuery.Args = nil // The user query starts from the var query generated above and is filtered // by the the filter generated from auth processing, so now we build @@ -788,7 +790,7 @@ func addSelectionSetFrom( }, } - addArgumentsToField(selectionQry, f) + addFilterToField(selectionQry, f) selectionQry.Filter = child.Filter authQueries = append(authQueries, parentQry, selectionQry) child.Filter = &gql.FilterTree{ @@ -797,10 +799,6 @@ func addSelectionSetFrom( Args: []gql.Arg{{Value: filtervarName}}, }, } - - // We already apply the following to `selectionQry` by calling addArgumentsToField() - // hence they are no longer required. - child.Args = nil } authQueries = append(authQueries, selectionAuth...) authQueries = append(authQueries, fieldAuth...)