From 822b2ae384a7207c247f9a72ae380010afebbeff Mon Sep 17 00:00:00 2001 From: animesh Date: Thu, 13 Feb 2020 18:45:04 +0530 Subject: [PATCH 01/10] Allow users to query their info --- edgraph/access_ee.go | 91 ++++++++++++++++++++++++++++++++++++++++++++ x/keys.go | 10 ++++- 2 files changed, 100 insertions(+), 1 deletion(-) diff --git a/edgraph/access_ee.go b/edgraph/access_ee.go index 6d4e27aa951..26c6af3507c 100644 --- a/edgraph/access_ee.go +++ b/edgraph/access_ee.go @@ -783,6 +783,12 @@ func authorizeQuery(ctx context.Context, parsedReq *gql.Result) error { } if len(blockedPreds) != 0 { + for _, gq := range parsedReq.Query { + addUserFilterToQuery(gq, userId, groupIds) + } + for _, pred := range x.AllACLPredicates() { + delete(blockedPreds, pred) + } parsedReq.Query = removePredsFromQuery(parsedReq.Query, blockedPreds) } @@ -819,6 +825,91 @@ func authorizeGroot(ctx context.Context) error { return doAuthorizeGroot() } +// addUserFilterToQuery applies makes sure that a user can access only its own +// acl info by applying filter of userid and groupid to acl predicates +func addUserFilterToQuery(gq *gql.GraphQuery, userId string, groupIds []string) { + addNewFilter := func(newFilter, filter *gql.FilterTree) *gql.FilterTree { + if filter == nil { + return newFilter + } + parentFilter := &gql.FilterTree{ + Op: "AND", + Child: []*gql.FilterTree{filter, newFilter}, + } + return parentFilter + } + + if gq.Func != nil && gq.Func.Name == "type" { + for _, arg := range gq.Func.Args { + // The case where value of some varialble v (say) is "Group" and a + // query comes like `eq(dgraph.type, val(v))`, will be ingored here. + if arg.Value == "User" { + newFilter := &gql.FilterTree{ + Func: &gql.Function{ + Attr: "dgraph.xid", + Name: "eq", + Args: []gql.Arg{ + gql.Arg{Value: userId}, + }, + }, + } + gq.Filter = addNewFilter(newFilter, gq.Filter) + } else if arg.Value == "Group" { + child := &gql.FilterTree{ + Func: &gql.Function{ + Attr: "dgraph.xid", + Name: "eq", + }, + } + for _, gid := range groupIds { + child.Func.Args = append(child.Func.Args, + gql.Arg{Value: gid}) + } + newFilter := &gql.FilterTree{ + Op: "OR", + Child: []*gql.FilterTree{child}, + } + gq.Filter = addNewFilter(newFilter, gq.Filter) + } + } + } + + switch gq.Attr { + case "dgraph.user.group": + child := &gql.FilterTree{ + Func: &gql.Function{ + Attr: "dgraph.xid", + Name: "eq", + }, + } + for _, gid := range groupIds { + child.Func.Args = append(child.Func.Args, gql.Arg{Value: gid}) + } + newFilter := &gql.FilterTree{ + Op: "OR", + Child: []*gql.FilterTree{child}, + } + gq.Filter = addNewFilter(newFilter, gq.Filter) + case "~dgraph.user.group": + newFilter := &gql.FilterTree{ + Func: &gql.Function{ + Attr: "dgraph.xid", + Name: "eq", + Args: []gql.Arg{ + gql.Arg{Value: userId}, + }, + }, + } + gq.Filter = addNewFilter(newFilter, gq.Filter) + } + + for _, ch := range gq.Children { + addUserFilterToQuery(ch, userId, groupIds) + } +} + +// removePredsFromQuery removes all the predicates in blockedPreds +// from all the queries in gqs. func removePredsFromQuery(gqs []*gql.GraphQuery, blockedPreds map[string]struct{}) []*gql.GraphQuery { diff --git a/x/keys.go b/x/keys.go index 3a58329df0c..614f00416ab 100644 --- a/x/keys.go +++ b/x/keys.go @@ -595,13 +595,21 @@ func IsAclPredicate(pred string) bool { // ReservedPredicates returns the complete list of reserved predicates that needs to // be expanded when * is given as a predicate. func ReservedPredicates() []string { - var preds []string + preds := make([]string, 0, len(reservedPredicateMap)) for pred := range reservedPredicateMap { preds = append(preds, pred) } return preds } +func AllACLPredicates() []string { + preds := make([]string, 0, len(aclPredicateMap)) + for pred := range aclPredicateMap { + preds = append(preds, pred) + } + return preds +} + // IsInternalPredicate returns true if the predicate is in the internal predicate list. func IsInternalPredicate(pred string) bool { _, ok := internalPredicateMap[strings.ToLower(pred)] From afc2b33e2db539dfac871b86a719d7d3b3aa57d8 Mon Sep 17 00:00:00 2001 From: animesh Date: Fri, 14 Feb 2020 13:28:27 +0530 Subject: [PATCH 02/10] Add test --- ee/acl/acl_test.go | 57 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/ee/acl/acl_test.go b/ee/acl/acl_test.go index 3da816d1593..8605bcf9cda 100644 --- a/ee/acl/acl_test.go +++ b/ee/acl/acl_test.go @@ -877,3 +877,60 @@ func TestNonExistentGroup(t *testing.T) { require.Error(t, err, "Setting permission for non-existent group should return error") require.Contains(t, string(resp), `Unable to modify: Group doesn't exist`) } + +func TestQueryUserInfo(t *testing.T) { + ctx, _ := context.WithTimeout(context.Background(), 100*time.Second) + + dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) + require.NoError(t, err) + addDataAndRules(ctx, t, dg) + + userClient, err := testutil.DgraphClient(testutil.SockAddr) + require.NoError(t, err) + time.Sleep(6 * time.Second) + + err = userClient.Login(ctx, userid, userpassword) + require.NoError(t, err) + + query := ` + { + me(func: type(User)) { + dgraph.xid + dgraph.user.group { + dgraph.xid + dgraph.acl.rule { + dgraph.rule.predicate + dgraph.rule.permission + } + } + } + } + ` + + resp, err := userClient.NewReadOnlyTxn().Query(ctx, query) + require.NoError(t, err) + + testutil.CompareJSON(t, ` + { + "me": [ + { + "dgraph.xid": "alice", + "dgraph.user.group": [ + { + "dgraph.xid": "dev", + "dgraph.acl.rule": [ + { + "dgraph.rule.predicate": "name", + "dgraph.rule.permission": 4 + }, + { + "dgraph.rule.predicate": "nickname", + "dgraph.rule.permission": 2 + } + ] + } + ] + } + ] + }`, string(resp.Json)) +} From 3ca6b81c59dc79af9352c6b4c449479d42dea6c8 Mon Sep 17 00:00:00 2001 From: animesh Date: Fri, 14 Feb 2020 18:15:26 +0530 Subject: [PATCH 03/10] Refactor code and modify test --- edgraph/access_ee.go | 96 +++++++++++++++++++------------------------- edgraph/server.go | 2 +- ee/acl/acl_test.go | 79 +++++++++++++++++++----------------- 3 files changed, 85 insertions(+), 92 deletions(-) diff --git a/edgraph/access_ee.go b/edgraph/access_ee.go index 26c6af3507c..0d5137a0d28 100644 --- a/edgraph/access_ee.go +++ b/edgraph/access_ee.go @@ -739,7 +739,7 @@ func logAccess(log *accessEntry) { //authorizeQuery authorizes the query using the aclCachePtr. It will silently drop all // unauthorized predicates from query. -func authorizeQuery(ctx context.Context, parsedReq *gql.Result) error { +func authorizeQuery(ctx context.Context, parsedReq *gql.Result, graphql bool) error { if len(worker.Config.HmacSecret) == 0 { // the user has not turned on the acl feature return nil @@ -783,11 +783,13 @@ func authorizeQuery(ctx context.Context, parsedReq *gql.Result) error { } if len(blockedPreds) != 0 { - for _, gq := range parsedReq.Query { - addUserFilterToQuery(gq, userId, groupIds) - } - for _, pred := range x.AllACLPredicates() { - delete(blockedPreds, pred) + if graphql { + for _, gq := range parsedReq.Query { + addUserFilterToQuery(gq, userId, groupIds) + } + for _, pred := range x.AllACLPredicates() { + delete(blockedPreds, pred) + } } parsedReq.Query = removePredsFromQuery(parsedReq.Query, blockedPreds) } @@ -828,7 +830,7 @@ func authorizeGroot(ctx context.Context) error { // addUserFilterToQuery applies makes sure that a user can access only its own // acl info by applying filter of userid and groupid to acl predicates func addUserFilterToQuery(gq *gql.GraphQuery, userId string, groupIds []string) { - addNewFilter := func(newFilter, filter *gql.FilterTree) *gql.FilterTree { + parentFilter := func(newFilter, filter *gql.FilterTree) *gql.FilterTree { if filter == nil { return newFilter } @@ -839,43 +841,17 @@ func addUserFilterToQuery(gq *gql.GraphQuery, userId string, groupIds []string) return parentFilter } - if gq.Func != nil && gq.Func.Name == "type" { - for _, arg := range gq.Func.Args { - // The case where value of some varialble v (say) is "Group" and a - // query comes like `eq(dgraph.type, val(v))`, will be ingored here. - if arg.Value == "User" { - newFilter := &gql.FilterTree{ - Func: &gql.Function{ - Attr: "dgraph.xid", - Name: "eq", - Args: []gql.Arg{ - gql.Arg{Value: userId}, - }, - }, - } - gq.Filter = addNewFilter(newFilter, gq.Filter) - } else if arg.Value == "Group" { - child := &gql.FilterTree{ - Func: &gql.Function{ - Attr: "dgraph.xid", - Name: "eq", - }, - } - for _, gid := range groupIds { - child.Func.Args = append(child.Func.Args, - gql.Arg{Value: gid}) - } - newFilter := &gql.FilterTree{ - Op: "OR", - Child: []*gql.FilterTree{child}, - } - gq.Filter = addNewFilter(newFilter, gq.Filter) - } + userFilter := func(userId string) *gql.FilterTree { + return &gql.FilterTree{ + Func: &gql.Function{ + Attr: "dgraph.xid", + Name: "eq", + Args: []gql.Arg{gql.Arg{Value: userId}}, + }, } } - switch gq.Attr { - case "dgraph.user.group": + groupFilter := func(groupIds []string) *gql.FilterTree { child := &gql.FilterTree{ Func: &gql.Function{ Attr: "dgraph.xid", @@ -883,24 +859,36 @@ func addUserFilterToQuery(gq *gql.GraphQuery, userId string, groupIds []string) }, } for _, gid := range groupIds { - child.Func.Args = append(child.Func.Args, gql.Arg{Value: gid}) + child.Func.Args = append(child.Func.Args, + gql.Arg{Value: gid}) } - newFilter := &gql.FilterTree{ + return &gql.FilterTree{ Op: "OR", Child: []*gql.FilterTree{child}, } - gq.Filter = addNewFilter(newFilter, gq.Filter) - case "~dgraph.user.group": - newFilter := &gql.FilterTree{ - Func: &gql.Function{ - Attr: "dgraph.xid", - Name: "eq", - Args: []gql.Arg{ - gql.Arg{Value: userId}, - }, - }, + } + + if gq.Func != nil && gq.Func.Name == "type" { + for _, arg := range gq.Func.Args { + // The case where value of some varialble v (say) is "Group" and a + // query comes like `eq(dgraph.type, val(v))`, will be ingored here. + if arg.Value == "User" { + newFilter := userFilter(userId) + gq.Filter = parentFilter(newFilter, gq.Filter) + } else if arg.Value == "Group" { + newFilter := groupFilter(groupIds) + gq.Filter = parentFilter(newFilter, gq.Filter) + } } - gq.Filter = addNewFilter(newFilter, gq.Filter) + } + + switch gq.Attr { + case "dgraph.user.group": + newFilter := groupFilter(groupIds) + gq.Filter = parentFilter(newFilter, gq.Filter) + case "~dgraph.user.group": + newFilter := userFilter(userId) + gq.Filter = parentFilter(newFilter, gq.Filter) } for _, ch := range gq.Children { diff --git a/edgraph/server.go b/edgraph/server.go index f8cb5f887c5..477845dfc3c 100644 --- a/edgraph/server.go +++ b/edgraph/server.go @@ -993,7 +993,7 @@ func parseRequest(qc *queryContext) error { } func authorizeRequest(ctx context.Context, qc *queryContext) error { - if err := authorizeQuery(ctx, &qc.gqlRes); err != nil { + if err := authorizeQuery(ctx, &qc.gqlRes, qc.graphql); err != nil { return err } diff --git a/ee/acl/acl_test.go b/ee/acl/acl_test.go index 8e937787512..d6a5af0aa78 100644 --- a/ee/acl/acl_test.go +++ b/ee/acl/acl_test.go @@ -1137,52 +1137,57 @@ func TestQueryUserInfo(t *testing.T) { require.NoError(t, err) addDataAndRules(ctx, t, dg) - userClient, err := testutil.DgraphClient(testutil.SockAddr) - require.NoError(t, err) - time.Sleep(6 * time.Second) - - err = userClient.Login(ctx, userid, userpassword) - require.NoError(t, err) + accessJwt, _, err := testutil.HttpLogin(&testutil.LoginParams{ + Endpoint: adminEndpoint, + UserID: userid, + Passwd: userpassword, + }) + require.NoError(t, err, "login failed") + // time.Sleep(6 * time.Second) query := ` - { - me(func: type(User)) { - dgraph.xid - dgraph.user.group { - dgraph.xid - dgraph.acl.rule { - dgraph.rule.predicate - dgraph.rule.permission + query { + queryUser { + name + groups { + name + rules { + predicate + permission } } } } ` - resp, err := userClient.NewReadOnlyTxn().Query(ctx, query) - require.NoError(t, err) + params := testutil.GraphQLParams{ + Query: query, + } + b := makeRequest(t, accessJwt, params) testutil.CompareJSON(t, ` { - "me": [ - { - "dgraph.xid": "alice", - "dgraph.user.group": [ - { - "dgraph.xid": "dev", - "dgraph.acl.rule": [ - { - "dgraph.rule.predicate": "name", - "dgraph.rule.permission": 4 - }, - { - "dgraph.rule.predicate": "nickname", - "dgraph.rule.permission": 2 - } - ] - } - ] - } - ] - }`, string(resp.Json)) + "data": { + "queryUser": [ + { + "name": "alice", + "groups": [ + { + "name": "dev", + "rules": [ + { + "predicate": "name", + "permission": 4 + }, + { + "predicate": "nickname", + "permission": 2 + } + ] + } + ] + } + ] + } + }`, string(b)) } From ba0ce59abcb23250bf9fe261aed4decd67c2a7d9 Mon Sep 17 00:00:00 2001 From: animesh Date: Fri, 14 Feb 2020 18:29:51 +0530 Subject: [PATCH 04/10] Refactor addUserFilter addUserFilter should only work for graphql queries. Also non graphql query in test to make sure it returns empty result for acl queryies. --- edgraph/access_ee.go | 2 +- ee/acl/acl_test.go | 31 ++++++++++++++++++++++++++++--- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/edgraph/access_ee.go b/edgraph/access_ee.go index 0d5137a0d28..0392a999bbc 100644 --- a/edgraph/access_ee.go +++ b/edgraph/access_ee.go @@ -846,7 +846,7 @@ func addUserFilterToQuery(gq *gql.GraphQuery, userId string, groupIds []string) Func: &gql.Function{ Attr: "dgraph.xid", Name: "eq", - Args: []gql.Arg{gql.Arg{Value: userId}}, + Args: []gql.Arg{{Value: userId}}, }, } } diff --git a/ee/acl/acl_test.go b/ee/acl/acl_test.go index d6a5af0aa78..77d3f9745cb 100644 --- a/ee/acl/acl_test.go +++ b/ee/acl/acl_test.go @@ -1143,9 +1143,8 @@ func TestQueryUserInfo(t *testing.T) { Passwd: userpassword, }) require.NoError(t, err, "login failed") - // time.Sleep(6 * time.Second) - query := ` + gqlQuery := ` query { queryUser { name @@ -1161,7 +1160,7 @@ func TestQueryUserInfo(t *testing.T) { ` params := testutil.GraphQLParams{ - Query: query, + Query: gqlQuery, } b := makeRequest(t, accessJwt, params) @@ -1190,4 +1189,30 @@ func TestQueryUserInfo(t *testing.T) { ] } }`, string(b)) + + query := ` + { + me(func: type(User)) { + dgraph.xid + dgraph.user.group { + dgraph.xid + dgraph.acl.rule { + dgraph.rule.predicate + dgraph.rule.permission + } + } + } + } + ` + + userClient, err := testutil.DgraphClient(testutil.SockAddr) + require.NoError(t, err) + + err = userClient.Login(ctx, userid, userpassword) + require.NoError(t, err) + + resp, err := userClient.NewReadOnlyTxn().Query(ctx, query) + require.NoError(t, err, "Error while querying ACL") + + testutil.CompareJSON(t, `{"me":[]}`, string(resp.GetJson())) } From a08d2fe20307b5cc6c31ea91434c1a0f2971303a Mon Sep 17 00:00:00 2001 From: animesh Date: Fri, 14 Feb 2020 19:05:47 +0530 Subject: [PATCH 05/10] Fix error for oss --- edgraph/access.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/edgraph/access.go b/edgraph/access.go index 57ac1285597..c9630d858ef 100644 --- a/edgraph/access.go +++ b/edgraph/access.go @@ -60,7 +60,7 @@ func authorizeMutation(ctx context.Context, gmu *gql.Mutation) error { return nil } -func authorizeQuery(ctx context.Context, parsedReq *gql.Result) error { +func authorizeQuery(ctx context.Context, parsedReq *gql.Result, graphql bool) error { // always allow access return nil } From d0f80ab03806c74bd557702e48fbd0c5ffd78dbc Mon Sep 17 00:00:00 2001 From: animesh Date: Fri, 14 Feb 2020 21:46:47 +0530 Subject: [PATCH 06/10] Refactor addUserFilterToQuery --- edgraph/access_ee.go | 145 +++++++++++++++++++++++++++++-------------- gql/parser.go | 8 +-- 2 files changed, 102 insertions(+), 51 deletions(-) diff --git a/edgraph/access_ee.go b/edgraph/access_ee.go index 0392a999bbc..983e0dd1657 100644 --- a/edgraph/access_ee.go +++ b/edgraph/access_ee.go @@ -790,6 +790,8 @@ func authorizeQuery(ctx context.Context, parsedReq *gql.Result, graphql bool) er for _, pred := range x.AllACLPredicates() { delete(blockedPreds, pred) } + // In query context ~predicate and predicate are considered different. + delete(blockedPreds, "~dgraph.user.group") } parsedReq.Query = removePredsFromQuery(parsedReq.Query, blockedPreds) } @@ -830,57 +832,24 @@ func authorizeGroot(ctx context.Context) error { // addUserFilterToQuery applies makes sure that a user can access only its own // acl info by applying filter of userid and groupid to acl predicates func addUserFilterToQuery(gq *gql.GraphQuery, userId string, groupIds []string) { - parentFilter := func(newFilter, filter *gql.FilterTree) *gql.FilterTree { - if filter == nil { - return newFilter - } - parentFilter := &gql.FilterTree{ - Op: "AND", - Child: []*gql.FilterTree{filter, newFilter}, + if gq.Func != nil && gq.Func.Name == "type" { + // type function only supports one argument + if len(gq.Func.Args) != 1 { + return } - return parentFilter - } - - userFilter := func(userId string) *gql.FilterTree { - return &gql.FilterTree{ - Func: &gql.Function{ - Attr: "dgraph.xid", - Name: "eq", - Args: []gql.Arg{{Value: userId}}, - }, + arg := gq.Func.Args[0] + // The case where value of some varialble v (say) is "Group" and a + // query comes like `eq(dgraph.type, val(v))`, will be ingored here. + if arg.Value == "User" { + newFilter := userFilter(userId) + gq.Filter = parentFilter(newFilter, gq.Filter) + } else if arg.Value == "Group" { + newFilter := groupFilter(groupIds) + gq.Filter = parentFilter(newFilter, gq.Filter) } } - groupFilter := func(groupIds []string) *gql.FilterTree { - child := &gql.FilterTree{ - Func: &gql.Function{ - Attr: "dgraph.xid", - Name: "eq", - }, - } - for _, gid := range groupIds { - child.Func.Args = append(child.Func.Args, - gql.Arg{Value: gid}) - } - return &gql.FilterTree{ - Op: "OR", - Child: []*gql.FilterTree{child}, - } - } - - if gq.Func != nil && gq.Func.Name == "type" { - for _, arg := range gq.Func.Args { - // The case where value of some varialble v (say) is "Group" and a - // query comes like `eq(dgraph.type, val(v))`, will be ingored here. - if arg.Value == "User" { - newFilter := userFilter(userId) - gq.Filter = parentFilter(newFilter, gq.Filter) - } else if arg.Value == "Group" { - newFilter := groupFilter(groupIds) - gq.Filter = parentFilter(newFilter, gq.Filter) - } - } - } + gq.Filter = addUserFilterToFilter(gq.Filter, userId, groupIds) switch gq.Attr { case "dgraph.user.group": @@ -896,6 +865,88 @@ func addUserFilterToQuery(gq *gql.GraphQuery, userId string, groupIds []string) } } +func parentFilter(newFilter, filter *gql.FilterTree) *gql.FilterTree { + if filter == nil { + return newFilter + } + parentFilter := &gql.FilterTree{ + Op: "AND", + Child: []*gql.FilterTree{filter, newFilter}, + } + return parentFilter +} + +func userFilter(userId string) *gql.FilterTree { + return &gql.FilterTree{ + Func: &gql.Function{ + Attr: "dgraph.xid", + Name: "eq", + Args: []gql.Arg{{Value: userId}}, + }, + } +} + +func groupFilter(groupIds []string) *gql.FilterTree { + filter := &gql.FilterTree{ + Func: &gql.Function{ + Attr: "dgraph.xid", + Name: "eq", + }, + } + for _, gid := range groupIds { + filter.Func.Args = append(filter.Func.Args, + gql.Arg{Value: gid}) + } + + return filter +} + +/* + addUserFilterToFilter makes sure that user can't misue filters to access other user's info. + If the *filter* have type(Group) or type(User) functions, it generate a *newFilter* with function + like eq(dgraph.xid, userId) or eq(dgraph.xid, groupId...) and return a filter of the form + + &gql.FilterTree{ + Op: "AND", + Child: []gql.FilterTree{ + {filter, newFilter} + } + } +*/ +func addUserFilterToFilter(filter *gql.FilterTree, userId string, + groupIds []string) *gql.FilterTree { + + if filter == nil { + return nil + } + + if filter.Func != nil && gql.IsInequalityFn(filter.Func.Name) && + filter.Func.Attr == "dgraph.type" { + + // type function supports only one argument + if len(filter.Func.Args) != 1 { + return nil + } + arg := filter.Func.Args[0] + var newFilter *gql.FilterTree + switch arg.Value { + case "User": + newFilter = userFilter(userId) + case "Group": + newFilter = groupFilter(groupIds) + } + + // If filter have function, it can't have children. + return parentFilter(newFilter, filter) + } + + for idx, child := range filter.Child { + filter.Child[idx] = addUserFilterToFilter(child, userId, groupIds) + } + + return filter +} + // removePredsFromQuery removes all the predicates in blockedPreds // from all the queries in gqs. func removePredsFromQuery(gqs []*gql.GraphQuery, diff --git a/gql/parser.go b/gql/parser.go index b0ea967c514..c9b9f50596d 100644 --- a/gql/parser.go +++ b/gql/parser.go @@ -1613,7 +1613,7 @@ func getValueArg(val string) (string, error) { } func validFuncName(name string) bool { - if isGeoFunc(name) || isInequalityFn(name) { + if isGeoFunc(name) || IsInequalityFn(name) { return true } @@ -1714,7 +1714,7 @@ L: return nil, itemInFunc.Errorf("Multiple variables not allowed in len function") } - if !isInequalityFn(function.Name) { + if !IsInequalityFn(function.Name) { return nil, itemInFunc.Errorf("len function only allowed inside inequality" + " function") @@ -1763,7 +1763,7 @@ L: case isGeoFunc(function.Name): err = parseGeoArgs(it, function) - case isInequalityFn(function.Name): + case IsInequalityFn(function.Name): err = parseIneqArgs(it, function) default: @@ -3225,7 +3225,7 @@ func isGeoFunc(name string) bool { return name == "near" || name == "contains" || name == "within" || name == "intersects" } -func isInequalityFn(name string) bool { +func IsInequalityFn(name string) bool { switch name { case "eq", "le", "ge", "gt", "lt": return true From f7b694fa16876334a7a3d3c31caa6c93c32fd818 Mon Sep 17 00:00:00 2001 From: animesh Date: Fri, 14 Feb 2020 22:03:37 +0530 Subject: [PATCH 07/10] Add test for filters involving acl predicates --- edgraph/access_ee.go | 3 +-- ee/acl/acl_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/edgraph/access_ee.go b/edgraph/access_ee.go index 983e0dd1657..81633978ac5 100644 --- a/edgraph/access_ee.go +++ b/edgraph/access_ee.go @@ -920,8 +920,7 @@ func addUserFilterToFilter(filter *gql.FilterTree, userId string, return nil } - if filter.Func != nil && gql.IsInequalityFn(filter.Func.Name) && - filter.Func.Attr == "dgraph.type" { + if filter.Func != nil && filter.Func.Name == "type" { // type function supports only one argument if len(filter.Func.Args) != 1 { diff --git a/ee/acl/acl_test.go b/ee/acl/acl_test.go index 77d3f9745cb..b175366db71 100644 --- a/ee/acl/acl_test.go +++ b/ee/acl/acl_test.go @@ -1154,6 +1154,9 @@ func TestQueryUserInfo(t *testing.T) { predicate permission } + users { + name + } } } } @@ -1182,6 +1185,11 @@ func TestQueryUserInfo(t *testing.T) { "predicate": "nickname", "permission": 2 } + ], + "users": [ + { + "name": "alice" + } ] } ] @@ -1215,4 +1223,25 @@ func TestQueryUserInfo(t *testing.T) { require.NoError(t, err, "Error while querying ACL") testutil.CompareJSON(t, `{"me":[]}`, string(resp.GetJson())) + + gqlQuery = ` + query { + getGroup(name: "guardians") { + name + rules { + predicate + permission + } + users { + name + } + } + } + ` + + params = testutil.GraphQLParams{ + Query: gqlQuery, + } + b = makeRequest(t, accessJwt, params) + testutil.CompareJSON(t, `{"data": {"getGroup": null}}`, string(b)) } From 5b16b156fbd58c81117746ebbd7725aaba594e74 Mon Sep 17 00:00:00 2001 From: animesh Date: Fri, 14 Feb 2020 22:30:08 +0530 Subject: [PATCH 08/10] Address PR comments --- edgraph/access_ee.go | 45 ++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/edgraph/access_ee.go b/edgraph/access_ee.go index 81633978ac5..77602226b1c 100644 --- a/edgraph/access_ee.go +++ b/edgraph/access_ee.go @@ -783,10 +783,14 @@ func authorizeQuery(ctx context.Context, parsedReq *gql.Result, graphql bool) er } if len(blockedPreds) != 0 { + // For GraphQL requests, we allow filtered access to the ACL predicates. + // Filter for user_id and group_id is applied for the currently logged in user. if graphql { for _, gq := range parsedReq.Query { addUserFilterToQuery(gq, userId, groupIds) } + // blockedPreds might have acl predicates which we want to allow access through + // graphql, so deleting those from here. for _, pred := range x.AllACLPredicates() { delete(blockedPreds, pred) } @@ -829,8 +833,14 @@ func authorizeGroot(ctx context.Context) error { return doAuthorizeGroot() } -// addUserFilterToQuery applies makes sure that a user can access only its own -// acl info by applying filter of userid and groupid to acl predicates +/* + addUserFilterToQuery applies makes sure that a user can access only its own + acl info by applying filter of userid and groupid to acl predicates. A query like + Conversion pattern: + * me(func: type(Group)) -> me(func: type(Group)) @filter(eq("dgraph.xid", groupIds...)) + * me(func: type(User)) -> me(func: type(User)) @filter(eq("dgraph.xid", userId)) + +*/ func addUserFilterToQuery(gq *gql.GraphQuery, userId string, groupIds []string) { if gq.Func != nil && gq.Func.Name == "type" { // type function only supports one argument @@ -842,22 +852,22 @@ func addUserFilterToQuery(gq *gql.GraphQuery, userId string, groupIds []string) // query comes like `eq(dgraph.type, val(v))`, will be ingored here. if arg.Value == "User" { newFilter := userFilter(userId) - gq.Filter = parentFilter(newFilter, gq.Filter) + updateFilter(newFilter, gq.Filter) } else if arg.Value == "Group" { newFilter := groupFilter(groupIds) - gq.Filter = parentFilter(newFilter, gq.Filter) + updateFilter(newFilter, gq.Filter) } } - gq.Filter = addUserFilterToFilter(gq.Filter, userId, groupIds) + addUserFilterToFilter(gq.Filter, userId, groupIds) switch gq.Attr { case "dgraph.user.group": newFilter := groupFilter(groupIds) - gq.Filter = parentFilter(newFilter, gq.Filter) + updateFilter(newFilter, gq.Filter) case "~dgraph.user.group": newFilter := userFilter(userId) - gq.Filter = parentFilter(newFilter, gq.Filter) + updateFilter(newFilter, gq.Filter) } for _, ch := range gq.Children { @@ -865,15 +875,15 @@ func addUserFilterToQuery(gq *gql.GraphQuery, userId string, groupIds []string) } } -func parentFilter(newFilter, filter *gql.FilterTree) *gql.FilterTree { +func updateFilter(newFilter, filter *gql.FilterTree) { if filter == nil { - return newFilter + return } parentFilter := &gql.FilterTree{ Op: "AND", Child: []*gql.FilterTree{filter, newFilter}, } - return parentFilter + filter = parentFilter } func userFilter(userId string) *gql.FilterTree { @@ -914,17 +924,17 @@ func groupFilter(groupIds []string) *gql.FilterTree { } */ func addUserFilterToFilter(filter *gql.FilterTree, userId string, - groupIds []string) *gql.FilterTree { + groupIds []string) { if filter == nil { - return nil + return } if filter.Func != nil && filter.Func.Name == "type" { // type function supports only one argument if len(filter.Func.Args) != 1 { - return nil + return } arg := filter.Func.Args[0] var newFilter *gql.FilterTree @@ -936,14 +946,13 @@ func addUserFilterToFilter(filter *gql.FilterTree, userId string, } // If filter have function, it can't have children. - return parentFilter(newFilter, filter) + updateFilter(newFilter, filter) + return } - for idx, child := range filter.Child { - filter.Child[idx] = addUserFilterToFilter(child, userId, groupIds) + for _, child := range filter.Child { + addUserFilterToFilter(child, userId, groupIds) } - - return filter } // removePredsFromQuery removes all the predicates in blockedPreds From 1fbc6c1a05ebbda787da2d780afa77e58a994a79 Mon Sep 17 00:00:00 2001 From: animesh Date: Fri, 14 Feb 2020 22:51:34 +0530 Subject: [PATCH 09/10] Revert "Address PR comments" Reverting this commit because updating pointers inside function doesn't look a neat way. Method used earlier seems more appropriate. This reverts commit 5b16b156fbd58c81117746ebbd7725aaba594e74. --- edgraph/access_ee.go | 45 ++++++++++++++++++-------------------------- 1 file changed, 18 insertions(+), 27 deletions(-) diff --git a/edgraph/access_ee.go b/edgraph/access_ee.go index 77602226b1c..81633978ac5 100644 --- a/edgraph/access_ee.go +++ b/edgraph/access_ee.go @@ -783,14 +783,10 @@ func authorizeQuery(ctx context.Context, parsedReq *gql.Result, graphql bool) er } if len(blockedPreds) != 0 { - // For GraphQL requests, we allow filtered access to the ACL predicates. - // Filter for user_id and group_id is applied for the currently logged in user. if graphql { for _, gq := range parsedReq.Query { addUserFilterToQuery(gq, userId, groupIds) } - // blockedPreds might have acl predicates which we want to allow access through - // graphql, so deleting those from here. for _, pred := range x.AllACLPredicates() { delete(blockedPreds, pred) } @@ -833,14 +829,8 @@ func authorizeGroot(ctx context.Context) error { return doAuthorizeGroot() } -/* - addUserFilterToQuery applies makes sure that a user can access only its own - acl info by applying filter of userid and groupid to acl predicates. A query like - Conversion pattern: - * me(func: type(Group)) -> me(func: type(Group)) @filter(eq("dgraph.xid", groupIds...)) - * me(func: type(User)) -> me(func: type(User)) @filter(eq("dgraph.xid", userId)) - -*/ +// addUserFilterToQuery applies makes sure that a user can access only its own +// acl info by applying filter of userid and groupid to acl predicates func addUserFilterToQuery(gq *gql.GraphQuery, userId string, groupIds []string) { if gq.Func != nil && gq.Func.Name == "type" { // type function only supports one argument @@ -852,22 +842,22 @@ func addUserFilterToQuery(gq *gql.GraphQuery, userId string, groupIds []string) // query comes like `eq(dgraph.type, val(v))`, will be ingored here. if arg.Value == "User" { newFilter := userFilter(userId) - updateFilter(newFilter, gq.Filter) + gq.Filter = parentFilter(newFilter, gq.Filter) } else if arg.Value == "Group" { newFilter := groupFilter(groupIds) - updateFilter(newFilter, gq.Filter) + gq.Filter = parentFilter(newFilter, gq.Filter) } } - addUserFilterToFilter(gq.Filter, userId, groupIds) + gq.Filter = addUserFilterToFilter(gq.Filter, userId, groupIds) switch gq.Attr { case "dgraph.user.group": newFilter := groupFilter(groupIds) - updateFilter(newFilter, gq.Filter) + gq.Filter = parentFilter(newFilter, gq.Filter) case "~dgraph.user.group": newFilter := userFilter(userId) - updateFilter(newFilter, gq.Filter) + gq.Filter = parentFilter(newFilter, gq.Filter) } for _, ch := range gq.Children { @@ -875,15 +865,15 @@ func addUserFilterToQuery(gq *gql.GraphQuery, userId string, groupIds []string) } } -func updateFilter(newFilter, filter *gql.FilterTree) { +func parentFilter(newFilter, filter *gql.FilterTree) *gql.FilterTree { if filter == nil { - return + return newFilter } parentFilter := &gql.FilterTree{ Op: "AND", Child: []*gql.FilterTree{filter, newFilter}, } - filter = parentFilter + return parentFilter } func userFilter(userId string) *gql.FilterTree { @@ -924,17 +914,17 @@ func groupFilter(groupIds []string) *gql.FilterTree { } */ func addUserFilterToFilter(filter *gql.FilterTree, userId string, - groupIds []string) { + groupIds []string) *gql.FilterTree { if filter == nil { - return + return nil } if filter.Func != nil && filter.Func.Name == "type" { // type function supports only one argument if len(filter.Func.Args) != 1 { - return + return nil } arg := filter.Func.Args[0] var newFilter *gql.FilterTree @@ -946,13 +936,14 @@ func addUserFilterToFilter(filter *gql.FilterTree, userId string, } // If filter have function, it can't have children. - updateFilter(newFilter, filter) - return + return parentFilter(newFilter, filter) } - for _, child := range filter.Child { - addUserFilterToFilter(child, userId, groupIds) + for idx, child := range filter.Child { + filter.Child[idx] = addUserFilterToFilter(child, userId, groupIds) } + + return filter } // removePredsFromQuery removes all the predicates in blockedPreds From 35beb5651a0156bd37894fd1e997559d72d59619 Mon Sep 17 00:00:00 2001 From: animesh Date: Fri, 14 Feb 2020 22:53:50 +0530 Subject: [PATCH 10/10] Add comments --- edgraph/access_ee.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/edgraph/access_ee.go b/edgraph/access_ee.go index 81633978ac5..70d6712abf2 100644 --- a/edgraph/access_ee.go +++ b/edgraph/access_ee.go @@ -783,10 +783,14 @@ func authorizeQuery(ctx context.Context, parsedReq *gql.Result, graphql bool) er } if len(blockedPreds) != 0 { + // For GraphQL requests, we allow filtered access to the ACL predicates. + // Filter for user_id and group_id is applied for the currently logged in user. if graphql { for _, gq := range parsedReq.Query { addUserFilterToQuery(gq, userId, groupIds) } + // blockedPreds might have acl predicates which we want to allow access through + // graphql, so deleting those from here. for _, pred := range x.AllACLPredicates() { delete(blockedPreds, pred) } @@ -829,8 +833,14 @@ func authorizeGroot(ctx context.Context) error { return doAuthorizeGroot() } -// addUserFilterToQuery applies makes sure that a user can access only its own -// acl info by applying filter of userid and groupid to acl predicates +/* + addUserFilterToQuery applies makes sure that a user can access only its own + acl info by applying filter of userid and groupid to acl predicates. A query like + Conversion pattern: + * me(func: type(Group)) -> me(func: type(Group)) @filter(eq("dgraph.xid", groupIds...)) + * me(func: type(User)) -> me(func: type(User)) @filter(eq("dgraph.xid", userId)) + +*/ func addUserFilterToQuery(gq *gql.GraphQuery, userId string, groupIds []string) { if gq.Func != nil && gq.Func.Name == "type" { // type function only supports one argument