From 1369ca82d219c656a9bdb81a3b19c66472d6d063 Mon Sep 17 00:00:00 2001 From: animesh Date: Fri, 31 Jan 2020 17:14:28 +0530 Subject: [PATCH 01/16] Add new acl predicates --- edgraph/access_ee.go | 6 +++++- edgraph/acl_cache.go | 10 +++++++--- ee/acl/utils.go | 1 + schema/schema.go | 13 +++++++++++++ x/keys.go | 11 +++++++---- 5 files changed, 33 insertions(+), 8 deletions(-) diff --git a/edgraph/access_ee.go b/edgraph/access_ee.go index cd2c7dc46d6..914af042790 100644 --- a/edgraph/access_ee.go +++ b/edgraph/access_ee.go @@ -347,7 +347,11 @@ const queryAcls = ` { allAcls(func: has(dgraph.group.acl)) { dgraph.xid - dgraph.group.acl + dgraph.group.acl + dgraph.group.rule { + dgraph.acl.predicate + dgraph.acl.permission + } } } ` diff --git a/edgraph/acl_cache.go b/edgraph/acl_cache.go index bd29a73d193..ba0286f9c9c 100644 --- a/edgraph/acl_cache.go +++ b/edgraph/acl_cache.go @@ -53,9 +53,13 @@ func (cache *aclCache) update(groups []acl.Group) { for _, group := range groups { aclBytes := []byte(group.Acls) var acls []acl.Acl - if err := json.Unmarshal(aclBytes, &acls); err != nil { - glog.Errorf("Unable to unmarshal the aclBytes: %v", err) - continue + if len(aclBytes) > 0 { + if err := json.Unmarshal(aclBytes, &acls); err != nil { + glog.Errorf("Unable to unmarshal the aclBytes: %v", err) + continue + } + } else { + acls = group.Rules } for _, acl := range acls { diff --git a/ee/acl/utils.go b/ee/acl/utils.go index 48a025c1edb..34c8ed6d9f7 100644 --- a/ee/acl/utils.go +++ b/ee/acl/utils.go @@ -116,6 +116,7 @@ type Group struct { GroupID string `json:"dgraph.xid"` Users []User `json:"~dgraph.user.group"` Acls string `json:"dgraph.group.acl"` + Rules []Acl `json:"dgraph.group.rule"` } // GetUid returns the UID of the group. diff --git a/schema/schema.go b/schema/schema.go index f71f7d52def..6bcc10d857d 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -502,6 +502,19 @@ func initialSchemaInternal(all bool) []*pb.SchemaUpdate { Predicate: "dgraph.group.acl", ValueType: pb.Posting_STRING, }, + { + Predicate: "dgraph.acl.rule", + ValueType: pb.Posting_UID, + List: true, + }, + { + Predicate: "dgraph.acl.predicate", + ValueType: pb.Posting_STRING, + }, + { + Predicate: "dgraph.acl.premission", + ValueType: pb.Posting_INT, + }, }...) } diff --git a/x/keys.go b/x/keys.go index cb0c0ab1fd6..6b71c20b20f 100644 --- a/x/keys.go +++ b/x/keys.go @@ -554,10 +554,13 @@ var reservedPredicateMap = map[string]struct{}{ } var aclPredicateMap = map[string]struct{}{ - "dgraph.xid": {}, - "dgraph.password": {}, - "dgraph.user.group": {}, - "dgraph.group.acl": {}, + "dgraph.xid": {}, + "dgraph.password": {}, + "dgraph.user.group": {}, + "dgraph.group.acl": {}, + "dgraph.acl.predicate": {}, + "dgraph.acl.permission": {}, + "dgraph.group.rule": {}, } var graphqlReservedPredicate = map[string]struct{}{ From 47a6ffebdbf3b168e318d7904618f67aeb0fecaa Mon Sep 17 00:00:00 2001 From: animesh Date: Mon, 3 Feb 2020 14:04:57 +0530 Subject: [PATCH 02/16] Add test and fix typos --- edgraph/access_ee.go | 4 +- edgraph/acl_cache.go | 13 +--- ee/acl/acl_test.go | 142 +++++++++++++++++++++++++++++++++++++++++++ ee/acl/utils.go | 6 +- schema/schema.go | 2 +- x/x.go | 3 + 6 files changed, 152 insertions(+), 18 deletions(-) diff --git a/edgraph/access_ee.go b/edgraph/access_ee.go index 914af042790..0b11292ea34 100644 --- a/edgraph/access_ee.go +++ b/edgraph/access_ee.go @@ -345,10 +345,10 @@ func RefreshAcls(closer *y.Closer) { const queryAcls = ` { - allAcls(func: has(dgraph.group.acl)) { + allAcls(func: type(Group)) { dgraph.xid dgraph.group.acl - dgraph.group.rule { + dgraph.acl.rule { dgraph.acl.predicate dgraph.acl.permission } diff --git a/edgraph/acl_cache.go b/edgraph/acl_cache.go index ba0286f9c9c..ace91502344 100644 --- a/edgraph/acl_cache.go +++ b/edgraph/acl_cache.go @@ -13,12 +13,10 @@ package edgraph import ( - "encoding/json" "sync" "github.com/dgraph-io/dgraph/ee/acl" "github.com/dgraph-io/dgraph/x" - "github.com/golang/glog" "github.com/pkg/errors" ) @@ -51,16 +49,7 @@ func (cache *aclCache) update(groups []acl.Group) { // predicate to a submap, and the submap maps a group to a permission predPerms := make(map[string]map[string]int32) for _, group := range groups { - aclBytes := []byte(group.Acls) - var acls []acl.Acl - if len(aclBytes) > 0 { - if err := json.Unmarshal(aclBytes, &acls); err != nil { - glog.Errorf("Unable to unmarshal the aclBytes: %v", err) - continue - } - } else { - acls = group.Rules - } + acls := group.Rules for _, acl := range acls { if len(acl.Predicate) > 0 { diff --git a/ee/acl/acl_test.go b/ee/acl/acl_test.go index 97408c5aa31..06da2974228 100644 --- a/ee/acl/acl_test.go +++ b/ee/acl/acl_test.go @@ -14,6 +14,7 @@ package acl import ( "context" + "errors" "fmt" "os" "os/exec" @@ -684,3 +685,144 @@ func TestQueryRemoveUnauthorizedPred(t *testing.T) { }) } } + +func TestNewACLPredicates(t *testing.T) { + ctx, _ := context.WithTimeout(context.Background(), 100*time.Second) + + dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) + require.NoError(t, err) + + testutil.DropAll(t, dg) + op := api.Operation{Schema: ` + name : string @index(exact) . + nickname : string @index(exact) . + `} + require.NoError(t, dg.Alter(ctx, &op)) + + resetUser(t) + devGroupMut := ` + _:g "dev" . + _:g "Group" . + _:g _:r1 . + _:r1 "name" . + _:r1 "4" . + _:g _:r2 . + _:r2 "nickname" . + _:r2 "2" . + ` + _, err = dg.NewTxn().Mutate(ctx, &api.Mutation{ + SetNquads: []byte(devGroupMut), + CommitNow: true, + }) + require.NoError(t, err, "Error adding group and permissions") + + idQuery := fmt.Sprintf(` + { + userid as var(func: eq(dgraph.xid, "%s")) + gid as var(func: eq(dgraph.xid, "dev")) + }`, userid) + addAliceToDevMutation := &api.NQuad{ + Subject: "uid(userid)", + Predicate: "dgraph.user.group", + ObjectId: "uid(gid)", + } + _, err = dg.NewTxn().Do(ctx, &api.Request{ + CommitNow: true, + Query: idQuery, + Mutations: []*api.Mutation{ + { + Set: []*api.NQuad{addAliceToDevMutation}, + }, + }, + }) + require.NoError(t, err, "Error adding user to dev group") + + mutation := &api.Mutation{ + SetNquads: []byte(` + _:a "RandomGuy" . + _:a "RG" . + _:b "RandomGuy2" . + _:b "25" . + _:b "RG2" . + `), + CommitNow: true, + } + _, err = dg.NewTxn().Mutate(ctx, mutation) + require.NoError(t, err) + + 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) + + queryTests := []struct { + input string + output string + description string + }{ + { + ` + { + me(func: has(name)) { + name + nickname + } + } + `, + `{"me":[{"name":"RandomGuy"},{"name":"RandomGuy2"}]}`, + "alice doesn't have read access to ", + }, + { + ` + { + me(func: has(nickname)) { + name + nickname + } + } + `, + `{}`, + `alice doesn't have access to so "has(nickname)" is unauthorized`, + }, + } + + t.Parallel() + for _, tc := range queryTests { + t.Run(tc.description, func(t *testing.T) { + resp, err := userClient.NewTxn().Query(ctx, tc.input) + require.Nil(t, err) + testutil.CompareJSON(t, tc.output, string(resp.Json)) + }) + } + + mutationTests := []struct { + input string + output string + err error + description string + }{ + { + "_:a \"Animesh\" .", + "", + errors.New(""), + "alice doesn't have write access on .", + }, + { + "_:a \"Pathak\" .", + "", + nil, + "alice can mutate predicate.", + }, + } + for _, tc := range mutationTests { + t.Run(tc.description, func(t *testing.T) { + _, err := userClient.NewTxn().Mutate(ctx, &api.Mutation{ + SetNquads: []byte(tc.input), + CommitNow: true, + }) + require.True(t, (err == nil) == (tc.err == nil)) + }) + } +} diff --git a/ee/acl/utils.go b/ee/acl/utils.go index 34c8ed6d9f7..d593bfab36b 100644 --- a/ee/acl/utils.go +++ b/ee/acl/utils.go @@ -106,8 +106,8 @@ func UnmarshalUser(resp *api.Response, userKey string) (user *User, err error) { // Acl represents the permissions in the ACL system. // An Acl can have a predicate and permission for that predicate. type Acl struct { - Predicate string `json:"predicate"` - Perm int32 `json:"perm"` + Predicate string `json:"dgraph.acl.predicate"` + Perm int32 `json:"dgraph.acl.permission"` } // Group represents a group in the ACL system. @@ -116,7 +116,7 @@ type Group struct { GroupID string `json:"dgraph.xid"` Users []User `json:"~dgraph.user.group"` Acls string `json:"dgraph.group.acl"` - Rules []Acl `json:"dgraph.group.rule"` + Rules []Acl `json:"dgraph.acl.rule"` } // GetUid returns the UID of the group. diff --git a/schema/schema.go b/schema/schema.go index 6bcc10d857d..ca3048da28c 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -512,7 +512,7 @@ func initialSchemaInternal(all bool) []*pb.SchemaUpdate { ValueType: pb.Posting_STRING, }, { - Predicate: "dgraph.acl.premission", + Predicate: "dgraph.acl.permission", ValueType: pb.Posting_INT, }, }...) diff --git a/x/x.go b/x/x.go index d2f581c7b38..3befa96f054 100644 --- a/x/x.go +++ b/x/x.go @@ -110,6 +110,9 @@ const ( {"predicate":"dgraph.password","type":"password"}, {"predicate":"dgraph.user.group","list":true, "reverse": true, "type": "uid"}, {"predicate":"dgraph.group.acl","type":"string"} +{"predicate":"dgraph.acl.rule","type":"uid","list":true} +{"predicate":"dgraph.acl.predicate","type":"string"} +{"predicate":"dgraph.acl.permission","type":"int"} ` // GroupIdFileName is the name of the file storing the ID of the group to which // the data in a postings directory belongs. This ID is used to join the proper From 37519c9cbc676052c9c2ee4d561b88bc988158dc Mon Sep 17 00:00:00 2001 From: animesh Date: Mon, 3 Feb 2020 20:32:52 +0530 Subject: [PATCH 03/16] Use new ACL schema in acl tool --- ee/acl/acl.go | 98 ++++++++++++++++++++++++------------------------ schema/schema.go | 2 + x/keys.go | 2 +- 3 files changed, 52 insertions(+), 50 deletions(-) diff --git a/ee/acl/acl.go b/ee/acl/acl.go index 590a71b9d74..e75ac3f9bd0 100644 --- a/ee/acl/acl.go +++ b/ee/acl/acl.go @@ -406,7 +406,7 @@ func chMod(conf *viper.Viper) error { return errors.Errorf("the groupid must not be empty") case len(predicate) == 0: return errors.Errorf("no predicates specified") - case perm > 7: + case perm > 7: // TODO(Animesh): also check if perm < 0 return errors.Errorf("the perm value must be less than or equal to 7, "+ "the provided value is %d", perm) } @@ -417,7 +417,7 @@ func chMod(conf *viper.Viper) error { } defer cancel() - ctx, ctxCancel := context.WithTimeout(context.Background(), 10*time.Second) + ctx, ctxCancel := context.WithTimeout(context.Background(), 10*time.Minute) defer ctxCancel() txn := dc.NewTxn() defer func() { @@ -426,59 +426,59 @@ func chMod(conf *viper.Viper) error { } }() - group, err := queryGroup(ctx, txn, groupId, "dgraph.group.acl") - if err != nil { - return errors.Wrapf(err, "while querying group") - } - if group == nil || len(group.Uid) == 0 { - return errors.Errorf("unable to change permission for group because it does not exist: %v", - groupId) - } - - var currentAcls []Acl - if len(group.Acls) != 0 { - if err := json.Unmarshal([]byte(group.Acls), ¤tAcls); err != nil { - return errors.Wrapf(err, "unable to unmarshal the acls associated with the group %v", - groupId) + ruleQuery := fmt.Sprintf(` + { + var(func: eq(dgraph.xid, "%s")) @filter(type(Group)) { + gUID as uid + rUID as dgraph.acl.rule @filter(eq(dgraph.acl.predicate, "%s")) } - } + }`, groupId, predicate) + + ruleUpsert := &api.Mutation{ + Set: []*api.NQuad{ + { + Subject: "uid(rUID)", + Predicate: "dgraph.acl.permission", + ObjectValue: &api.Value{Val: &api.Value_IntVal{IntVal: int64(perm)}}, + }, + }, + Cond: "@if(gt(len(rUID), 0) AND eq(len(gUID), 1))", + } + + ruleMutation := &api.Mutation{ + Set: []*api.NQuad{ + { + Subject: "_:newrule", + Predicate: "dgraph.acl.permission", + ObjectValue: &api.Value{Val: &api.Value_IntVal{IntVal: int64(perm)}}, + }, + { + Subject: "_:newrule", + Predicate: "dgraph.acl.predicate", + ObjectValue: &api.Value{Val: &api.Value_StrVal{StrVal: predicate}}, + }, + { + Subject: "uid(gUID)", + Predicate: "dgraph.acl.rule", + ObjectId: "_:newrule", + }, + }, + Cond: "@if(eq(len(rUID), 0) AND eq(len(gUID), 1))", + } + + resp, err := txn.Do(ctx, &api.Request{ + Query: ruleQuery, + Mutations: []*api.Mutation{ruleMutation, ruleUpsert}, + CommitNow: true, + }) - var newAcl Acl - if len(predicate) > 0 { - newAcl = Acl{ - Predicate: predicate, - Perm: int32(perm), - } - } - newAcls, updated := updateAcl(currentAcls, newAcl) - if !updated { - fmt.Printf("Nothing needs to be changed for the permission of group: %v\n", groupId) - return nil - } + fmt.Printf("%v", resp) - newAclBytes, err := json.Marshal(newAcls) if err != nil { - return errors.Wrapf(err, "unable to marshal the updated acls") - } - - chModNQuads := &api.NQuad{ - Subject: group.Uid, - Predicate: "dgraph.group.acl", - ObjectValue: &api.Value{Val: &api.Value_BytesVal{BytesVal: newAclBytes}}, - } - mu := &api.Mutation{ - CommitNow: true, - Set: []*api.NQuad{chModNQuads}, + return err } - if _, err = txn.Mutate(ctx, mu); err != nil { - return errors.Wrapf(err, "unable to change mutations for the group %v on predicate %v", - groupId, predicate) - } - fmt.Printf("Successfully changed permission for group %v on predicate %v to %v\n", - groupId, predicate, perm) - fmt.Println("The latest info is:") - return queryAndPrintGroup(ctx, dc.NewReadOnlyTxn(), groupId) + return nil } func queryUser(ctx context.Context, txn *dgo.Txn, userid string) (user *User, err error) { diff --git a/schema/schema.go b/schema/schema.go index ca3048da28c..7a8e87dc381 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -510,6 +510,8 @@ func initialSchemaInternal(all bool) []*pb.SchemaUpdate { { Predicate: "dgraph.acl.predicate", ValueType: pb.Posting_STRING, + Directive: pb.SchemaUpdate_INDEX, + Tokenizer: []string{"exact"}, }, { Predicate: "dgraph.acl.permission", diff --git a/x/keys.go b/x/keys.go index 6b71c20b20f..4f64849eed5 100644 --- a/x/keys.go +++ b/x/keys.go @@ -560,7 +560,7 @@ var aclPredicateMap = map[string]struct{}{ "dgraph.group.acl": {}, "dgraph.acl.predicate": {}, "dgraph.acl.permission": {}, - "dgraph.group.rule": {}, + "dgraph.acl.rule": {}, } var graphqlReservedPredicate = map[string]struct{}{ From 734439b40db615eb23cce2b633e0883212f4ed47 Mon Sep 17 00:00:00 2001 From: animesh Date: Tue, 4 Feb 2020 16:15:12 +0530 Subject: [PATCH 04/16] Fix tests --- edgraph/acl_cache_test.go | 4 +--- schema/schema.go | 2 +- systest/queries_test.go | 9 +++++++++ x/x.go | 6 +++--- 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/edgraph/acl_cache_test.go b/edgraph/acl_cache_test.go index 25f95cb75f5..2bfb13edb65 100644 --- a/edgraph/acl_cache_test.go +++ b/edgraph/acl_cache_test.go @@ -13,7 +13,6 @@ package edgraph import ( - "encoding/json" "testing" "github.com/dgraph-io/dgraph/ee/acl" @@ -37,11 +36,10 @@ func TestAclCache(t *testing.T) { Perm: 4, }, } - aclBytes, _ := json.Marshal(acls) groups := []acl.Group{ { GroupID: group, - Acls: string(aclBytes), + Rules: acls, }, } aclCachePtr.update(groups) diff --git a/schema/schema.go b/schema/schema.go index 7a8e87dc381..c3b6837c742 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -511,7 +511,7 @@ func initialSchemaInternal(all bool) []*pb.SchemaUpdate { Predicate: "dgraph.acl.predicate", ValueType: pb.Posting_STRING, Directive: pb.SchemaUpdate_INDEX, - Tokenizer: []string{"exact"}, + Tokenizer: []string{"exact"}, // Should we have upsert directive for this ? }, { Predicate: "dgraph.acl.permission", diff --git a/systest/queries_test.go b/systest/queries_test.go index 9ef963f7900..c23a728dbba 100644 --- a/systest/queries_test.go +++ b/systest/queries_test.go @@ -395,6 +395,15 @@ func SchemaQueryTestPredicate1(t *testing.T, c *dgo.Dgraph) { { "predicate": "dgraph.group.acl" }, + { + "predicate": "dgraph.acl.rule" + }, + { + "predicate": "dgraph.acl.predicate" + }, + { + "predicate": "dgraph.acl.permission" + }, { "predicate": "dgraph.graphql.schema" }, diff --git a/x/x.go b/x/x.go index 3befa96f054..e4d928885c9 100644 --- a/x/x.go +++ b/x/x.go @@ -109,9 +109,9 @@ const ( {"predicate":"dgraph.xid","type":"string", "index": true, "tokenizer":["exact"], "upsert": true}, {"predicate":"dgraph.password","type":"password"}, {"predicate":"dgraph.user.group","list":true, "reverse": true, "type": "uid"}, -{"predicate":"dgraph.group.acl","type":"string"} -{"predicate":"dgraph.acl.rule","type":"uid","list":true} -{"predicate":"dgraph.acl.predicate","type":"string"} +{"predicate":"dgraph.group.acl","type":"string"}, +{"predicate":"dgraph.acl.rule","type":"uid","list":true}, +{"predicate":"dgraph.acl.predicate","type":"string","index":true,"tokenizer":["exact"]}, {"predicate":"dgraph.acl.permission","type":"int"} ` // GroupIdFileName is the name of the file storing the ID of the group to which From 44c40e9fd6670014f39ff11f02c4b1e8cccdd47d Mon Sep 17 00:00:00 2001 From: animesh Date: Tue, 4 Feb 2020 16:18:18 +0530 Subject: [PATCH 05/16] Minor typo fixes --- edgraph/access_ee.go | 10 +++++----- ee/acl/acl.go | 4 +--- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/edgraph/access_ee.go b/edgraph/access_ee.go index 0b11292ea34..e636d47f8b7 100644 --- a/edgraph/access_ee.go +++ b/edgraph/access_ee.go @@ -347,11 +347,11 @@ const queryAcls = ` { allAcls(func: type(Group)) { dgraph.xid - dgraph.group.acl - dgraph.acl.rule { - dgraph.acl.predicate - dgraph.acl.permission - } + dgraph.group.acl + dgraph.acl.rule { + dgraph.acl.predicate + dgraph.acl.permission + } } } ` diff --git a/ee/acl/acl.go b/ee/acl/acl.go index e75ac3f9bd0..dc6718233dc 100644 --- a/ee/acl/acl.go +++ b/ee/acl/acl.go @@ -466,14 +466,12 @@ func chMod(conf *viper.Viper) error { Cond: "@if(eq(len(rUID), 0) AND eq(len(gUID), 1))", } - resp, err := txn.Do(ctx, &api.Request{ + _, err = txn.Do(ctx, &api.Request{ Query: ruleQuery, Mutations: []*api.Mutation{ruleMutation, ruleUpsert}, CommitNow: true, }) - fmt.Printf("%v", resp) - if err != nil { return err } From 9ab121c04970db8da4f352ed5d07b05cd89eb18c Mon Sep 17 00:00:00 2001 From: animesh Date: Tue, 4 Feb 2020 17:46:44 +0530 Subject: [PATCH 06/16] Fix test --- dgraph/cmd/bulk/systest/test-bulk-schema.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dgraph/cmd/bulk/systest/test-bulk-schema.sh b/dgraph/cmd/bulk/systest/test-bulk-schema.sh index 97b2b746f3a..d73c5c2e75a 100755 --- a/dgraph/cmd/bulk/systest/test-bulk-schema.sh +++ b/dgraph/cmd/bulk/systest/test-bulk-schema.sh @@ -190,7 +190,11 @@ EOF dgraph debug -p out/0/p 2>|/dev/null | grep '{s}' | cut -d' ' -f4 > all_dbs.out dgraph debug -p out/1/p 2>|/dev/null | grep '{s}' | cut -d' ' -f4 >> all_dbs.out + LC_ALL=C sort all_dbs.out | uniq -c diff <(LC_ALL=C sort all_dbs.out | uniq -c) - < Date: Thu, 6 Feb 2020 16:24:57 +0530 Subject: [PATCH 07/16] Remove debugging statements --- dgraph/cmd/bulk/systest/test-bulk-schema.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/dgraph/cmd/bulk/systest/test-bulk-schema.sh b/dgraph/cmd/bulk/systest/test-bulk-schema.sh index d73c5c2e75a..7ea2681a14d 100755 --- a/dgraph/cmd/bulk/systest/test-bulk-schema.sh +++ b/dgraph/cmd/bulk/systest/test-bulk-schema.sh @@ -190,7 +190,6 @@ EOF dgraph debug -p out/0/p 2>|/dev/null | grep '{s}' | cut -d' ' -f4 > all_dbs.out dgraph debug -p out/1/p 2>|/dev/null | grep '{s}' | cut -d' ' -f4 >> all_dbs.out - LC_ALL=C sort all_dbs.out | uniq -c diff <(LC_ALL=C sort all_dbs.out | uniq -c) - < Date: Thu, 6 Feb 2020 17:29:51 +0530 Subject: [PATCH 08/16] Change acl predicate names --- dgraph/cmd/bulk/systest/test-bulk-schema.sh | 5 +- edgraph/access_ee.go | 5 +- ee/acl/acl.go | 54 +++++++++------------ ee/acl/acl_test.go | 8 +-- ee/acl/utils.go | 5 +- schema/schema.go | 11 ++--- systest/queries_test.go | 7 +-- x/keys.go | 13 +++-- x/x.go | 9 ++-- 9 files changed, 49 insertions(+), 68 deletions(-) diff --git a/dgraph/cmd/bulk/systest/test-bulk-schema.sh b/dgraph/cmd/bulk/systest/test-bulk-schema.sh index 7ea2681a14d..52ee675f7da 100755 --- a/dgraph/cmd/bulk/systest/test-bulk-schema.sh +++ b/dgraph/cmd/bulk/systest/test-bulk-schema.sh @@ -191,12 +191,11 @@ EOF dgraph debug -p out/0/p 2>|/dev/null | grep '{s}' | cut -d' ' -f4 > all_dbs.out dgraph debug -p out/1/p 2>|/dev/null | grep '{s}' | cut -d' ' -f4 >> all_dbs.out diff <(LC_ALL=C sort all_dbs.out | uniq -c) - < "dev" . _:g "Group" . _:g _:r1 . - _:r1 "name" . - _:r1 "4" . + _:r1 "name" . + _:r1 "4" . _:g _:r2 . - _:r2 "nickname" . - _:r2 "2" . + _:r2 "nickname" . + _:r2 "2" . ` _, err = dg.NewTxn().Mutate(ctx, &api.Mutation{ SetNquads: []byte(devGroupMut), diff --git a/ee/acl/utils.go b/ee/acl/utils.go index d593bfab36b..35df91e5ae2 100644 --- a/ee/acl/utils.go +++ b/ee/acl/utils.go @@ -106,8 +106,8 @@ func UnmarshalUser(resp *api.Response, userKey string) (user *User, err error) { // Acl represents the permissions in the ACL system. // An Acl can have a predicate and permission for that predicate. type Acl struct { - Predicate string `json:"dgraph.acl.predicate"` - Perm int32 `json:"dgraph.acl.permission"` + Predicate string `json:"dgraph.rule.predicate"` + Perm int32 `json:"dgraph.rule.permission"` } // Group represents a group in the ACL system. @@ -115,7 +115,6 @@ type Group struct { Uid string `json:"uid"` GroupID string `json:"dgraph.xid"` Users []User `json:"~dgraph.user.group"` - Acls string `json:"dgraph.group.acl"` Rules []Acl `json:"dgraph.acl.rule"` } diff --git a/schema/schema.go b/schema/schema.go index c3b6837c742..bcfb1c716fe 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -498,23 +498,20 @@ func initialSchemaInternal(all bool) []*pb.SchemaUpdate { ValueType: pb.Posting_UID, List: true, }, - { - Predicate: "dgraph.group.acl", - ValueType: pb.Posting_STRING, - }, { Predicate: "dgraph.acl.rule", ValueType: pb.Posting_UID, List: true, + Upsert: true, }, { - Predicate: "dgraph.acl.predicate", + Predicate: "dgraph.rule.predicate", ValueType: pb.Posting_STRING, Directive: pb.SchemaUpdate_INDEX, - Tokenizer: []string{"exact"}, // Should we have upsert directive for this ? + Tokenizer: []string{"exact"}, }, { - Predicate: "dgraph.acl.permission", + Predicate: "dgraph.rule.permission", ValueType: pb.Posting_INT, }, }...) diff --git a/systest/queries_test.go b/systest/queries_test.go index c23a728dbba..eb4058b9554 100644 --- a/systest/queries_test.go +++ b/systest/queries_test.go @@ -392,17 +392,14 @@ func SchemaQueryTestPredicate1(t *testing.T, c *dgo.Dgraph) { { "predicate": "dgraph.password" }, - { - "predicate": "dgraph.group.acl" - }, { "predicate": "dgraph.acl.rule" }, { - "predicate": "dgraph.acl.predicate" + "predicate": "dgraph.rule.predicate" }, { - "predicate": "dgraph.acl.permission" + "predicate": "dgraph.rule.permission" }, { "predicate": "dgraph.graphql.schema" diff --git a/x/keys.go b/x/keys.go index 4f64849eed5..3a58329df0c 100644 --- a/x/keys.go +++ b/x/keys.go @@ -554,13 +554,12 @@ var reservedPredicateMap = map[string]struct{}{ } var aclPredicateMap = map[string]struct{}{ - "dgraph.xid": {}, - "dgraph.password": {}, - "dgraph.user.group": {}, - "dgraph.group.acl": {}, - "dgraph.acl.predicate": {}, - "dgraph.acl.permission": {}, - "dgraph.acl.rule": {}, + "dgraph.xid": {}, + "dgraph.password": {}, + "dgraph.user.group": {}, + "dgraph.rule.predicate": {}, + "dgraph.rule.permission": {}, + "dgraph.acl.rule": {}, } var graphqlReservedPredicate = map[string]struct{}{ diff --git a/x/x.go b/x/x.go index e4d928885c9..a07962072db 100644 --- a/x/x.go +++ b/x/x.go @@ -106,13 +106,12 @@ const ( // AclPredicates is the JSON representation of the predicates reserved for use // by the ACL system. AclPredicates = ` -{"predicate":"dgraph.xid","type":"string", "index": true, "tokenizer":["exact"], "upsert": true}, +{"predicate":"dgraph.xid","type":"string", "index":true, "tokenizer":["exact"], "upsert":true}, {"predicate":"dgraph.password","type":"password"}, {"predicate":"dgraph.user.group","list":true, "reverse": true, "type": "uid"}, -{"predicate":"dgraph.group.acl","type":"string"}, -{"predicate":"dgraph.acl.rule","type":"uid","list":true}, -{"predicate":"dgraph.acl.predicate","type":"string","index":true,"tokenizer":["exact"]}, -{"predicate":"dgraph.acl.permission","type":"int"} +{"predicate":"dgraph.acl.rule","type":"uid","list":true, "upsert":true}, +{"predicate":"dgraph.rule.predicate","type":"string","index":true,"tokenizer":["exact"]}, +{"predicate":"dgraph.rule.permission","type":"int"} ` // GroupIdFileName is the name of the file storing the ID of the group to which // the data in a postings directory belongs. This ID is used to join the proper From 4d0a39ec69ae6e621ac47805578d09fbaff0a293 Mon Sep 17 00:00:00 2001 From: animesh Date: Thu, 6 Feb 2020 18:59:30 +0530 Subject: [PATCH 09/16] Delete rule if permission is negative --- ee/acl/acl.go | 22 +++++++++++++++++++--- schema/schema.go | 2 +- x/x.go | 2 +- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/ee/acl/acl.go b/ee/acl/acl.go index 3ac77b35bd3..1ecbb133130 100644 --- a/ee/acl/acl.go +++ b/ee/acl/acl.go @@ -431,6 +431,7 @@ func chMod(conf *viper.Viper) error { gUID as uid rUID as dgraph.acl.rule @filter(eq(dgraph.rule.predicate, "%s")) } + groupUIDCount(func: uid(gUID)) {count(uid)} }`, groupId, predicate) updateRule := &api.Mutation{ @@ -465,12 +466,27 @@ func chMod(conf *viper.Viper) error { Cond: "@if(eq(len(rUID), 0) AND eq(len(gUID), 1))", } - _, err = txn.Do(ctx, &api.Request{ + deleteRule := &api.Mutation{ + Del: []*api.NQuad{ + { + Subject: "uid(gUID)", + Predicate: "dgraph.acl.rule", + ObjectId: "uid(rUID)", + }, + }, + Cond: "@if(eq(len(rUID), 1) AND eq(len(gUID), 1))", + } + + upsertRequest := &api.Request{ Query: ruleQuery, Mutations: []*api.Mutation{createRule, updateRule}, CommitNow: true, - }) - + } + if perm < 0 { + upsertRequest.Mutations = []*api.Mutation{deleteRule} + } + resp, err := txn.Do(ctx, upsertRequest) + fmt.Println(string(resp.GetJson())) return err } diff --git a/schema/schema.go b/schema/schema.go index bcfb1c716fe..7c71b0c0bf7 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -502,13 +502,13 @@ func initialSchemaInternal(all bool) []*pb.SchemaUpdate { Predicate: "dgraph.acl.rule", ValueType: pb.Posting_UID, List: true, - Upsert: true, }, { Predicate: "dgraph.rule.predicate", ValueType: pb.Posting_STRING, Directive: pb.SchemaUpdate_INDEX, Tokenizer: []string{"exact"}, + Upsert: true, // Not really sure if this will work. }, { Predicate: "dgraph.rule.permission", diff --git a/x/x.go b/x/x.go index a07962072db..8fe3a28fc1f 100644 --- a/x/x.go +++ b/x/x.go @@ -108,7 +108,7 @@ const ( AclPredicates = ` {"predicate":"dgraph.xid","type":"string", "index":true, "tokenizer":["exact"], "upsert":true}, {"predicate":"dgraph.password","type":"password"}, -{"predicate":"dgraph.user.group","list":true, "reverse": true, "type": "uid"}, +{"predicate":"dgraph.user.group","list":true, "reverse":true, "type":"uid"}, {"predicate":"dgraph.acl.rule","type":"uid","list":true, "upsert":true}, {"predicate":"dgraph.rule.predicate","type":"string","index":true,"tokenizer":["exact"]}, {"predicate":"dgraph.rule.permission","type":"int"} From dfd71b1e62aba3d6d64c2d65f9489878f8e12005 Mon Sep 17 00:00:00 2001 From: animesh Date: Thu, 6 Feb 2020 19:48:31 +0530 Subject: [PATCH 10/16] Add test for rule deletion --- ee/acl/acl_test.go | 153 ++++++++++++++++++++++++++++----------------- 1 file changed, 95 insertions(+), 58 deletions(-) diff --git a/ee/acl/acl_test.go b/ee/acl/acl_test.go index d0a9239b94d..444b28d9573 100644 --- a/ee/acl/acl_test.go +++ b/ee/acl/acl_test.go @@ -691,64 +691,7 @@ func TestNewACLPredicates(t *testing.T) { dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) require.NoError(t, err) - - testutil.DropAll(t, dg) - op := api.Operation{Schema: ` - name : string @index(exact) . - nickname : string @index(exact) . - `} - require.NoError(t, dg.Alter(ctx, &op)) - - resetUser(t) - devGroupMut := ` - _:g "dev" . - _:g "Group" . - _:g _:r1 . - _:r1 "name" . - _:r1 "4" . - _:g _:r2 . - _:r2 "nickname" . - _:r2 "2" . - ` - _, err = dg.NewTxn().Mutate(ctx, &api.Mutation{ - SetNquads: []byte(devGroupMut), - CommitNow: true, - }) - require.NoError(t, err, "Error adding group and permissions") - - idQuery := fmt.Sprintf(` - { - userid as var(func: eq(dgraph.xid, "%s")) - gid as var(func: eq(dgraph.xid, "dev")) - }`, userid) - addAliceToDevMutation := &api.NQuad{ - Subject: "uid(userid)", - Predicate: "dgraph.user.group", - ObjectId: "uid(gid)", - } - _, err = dg.NewTxn().Do(ctx, &api.Request{ - CommitNow: true, - Query: idQuery, - Mutations: []*api.Mutation{ - { - Set: []*api.NQuad{addAliceToDevMutation}, - }, - }, - }) - require.NoError(t, err, "Error adding user to dev group") - - mutation := &api.Mutation{ - SetNquads: []byte(` - _:a "RandomGuy" . - _:a "RG" . - _:b "RandomGuy2" . - _:b "25" . - _:b "RG2" . - `), - CommitNow: true, - } - _, err = dg.NewTxn().Mutate(ctx, mutation) - require.NoError(t, err) + addDataAndRules(ctx, t, dg) userClient, err := testutil.DgraphClient(testutil.SockAddr) require.NoError(t, err) @@ -826,3 +769,97 @@ func TestNewACLPredicates(t *testing.T) { }) } } + +func TestNegativePermissionDeleteRule(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) + + queryName := "{me(func: has(name)) {name}}" + resp, err := userClient.NewReadOnlyTxn().Query(ctx, queryName) + require.NoError(t, err, "Error while querying data") + + testutil.CompareJSON(t, `{"me":[{"name":"RandomGuy"},{"name":"RandomGuy2"}]}`, + string(resp.GetJson())) + + // Deleting a rule by setting negative permission works only when done by acl commandline + // tool. When done directly through query it will actually set negative permission, + // which won't work as expceted. + updateRule := exec.Command("dgraph", "acl", "mod", "-a", dgraphEndpoint, + "-g", devGroup, "-p", "name", "-m", "-1", "-x", "password") + require.NoError(t, updateRule.Run()) + time.Sleep(6 * time.Second) + + resp, err = userClient.NewReadOnlyTxn().Query(ctx, queryName) + require.NoError(t, err, "Error while querying data") + testutil.CompareJSON(t, string(resp.GetJson()), `{}`) +} + +func addDataAndRules(ctx context.Context, t *testing.T, dg *dgo.Dgraph) { + testutil.DropAll(t, dg) + op := api.Operation{Schema: ` + name : string @index(exact) . + nickname : string @index(exact) . + `} + require.NoError(t, dg.Alter(ctx, &op)) + + resetUser(t) + devGroupMut := ` + _:g "dev" . + _:g "Group" . + _:g _:r1 . + _:r1 "name" . + _:r1 "4" . + _:g _:r2 . + _:r2 "nickname" . + _:r2 "2" . + ` + _, err := dg.NewTxn().Mutate(ctx, &api.Mutation{ + SetNquads: []byte(devGroupMut), + CommitNow: true, + }) + require.NoError(t, err, "Error adding group and permissions") + + idQuery := fmt.Sprintf(` + { + userid as var(func: eq(dgraph.xid, "%s")) + gid as var(func: eq(dgraph.xid, "dev")) + }`, userid) + addAliceToDevMutation := &api.NQuad{ + Subject: "uid(userid)", + Predicate: "dgraph.user.group", + ObjectId: "uid(gid)", + } + _, err = dg.NewTxn().Do(ctx, &api.Request{ + CommitNow: true, + Query: idQuery, + Mutations: []*api.Mutation{ + { + Set: []*api.NQuad{addAliceToDevMutation}, + }, + }, + }) + require.NoError(t, err, "Error adding user to dev group") + + mutation := &api.Mutation{ + SetNquads: []byte(` + _:a "RandomGuy" . + _:a "RG" . + _:b "RandomGuy2" . + _:b "25" . + _:b "RG2" . + `), + CommitNow: true, + } + _, err = dg.NewTxn().Mutate(ctx, mutation) + require.NoError(t, err) +} From aa8222453afeba6edc8ac117d5280708f3aacbce Mon Sep 17 00:00:00 2001 From: animesh Date: Thu, 6 Feb 2020 19:49:56 +0530 Subject: [PATCH 11/16] Delete updateACL function and its tests --- ee/acl/acl.go | 22 ------------- ee/acl/groups_test.go | 73 ------------------------------------------- x/x.go | 4 +-- 3 files changed, 2 insertions(+), 97 deletions(-) delete mode 100644 ee/acl/groups_test.go diff --git a/ee/acl/acl.go b/ee/acl/acl.go index 1ecbb133130..6a4fd0e4697 100644 --- a/ee/acl/acl.go +++ b/ee/acl/acl.go @@ -569,28 +569,6 @@ func isSameAcl(acl1 *Acl, acl2 *Acl) bool { acl1.Predicate == acl2.Predicate) } -// returns whether the existing acls slice is changed -func updateAcl(acls []Acl, newAcl Acl) ([]Acl, bool) { - for idx, aclEntry := range acls { - if isSameAcl(&aclEntry, &newAcl) { - if aclEntry.Perm == newAcl.Perm { - // new permission is the same as the current one, no update - return acls, false - } - if newAcl.Perm < 0 { - // remove the current aclEntry from the array - copy(acls[idx:], acls[idx+1:]) - return acls[:len(acls)-1], true - } - acls[idx].Perm = newAcl.Perm - return acls, true - } - } - - // we do not find any existing aclEntry matching the newAcl predicate - return append(acls, newAcl), true -} - func queryAndPrintUser(ctx context.Context, txn *dgo.Txn, userId string) error { user, err := queryUser(ctx, txn, userId) if err != nil { diff --git a/ee/acl/groups_test.go b/ee/acl/groups_test.go deleted file mode 100644 index 294d97cb580..00000000000 --- a/ee/acl/groups_test.go +++ /dev/null @@ -1,73 +0,0 @@ -// +build !oss - -/* - * Copyright 2018 Dgraph Labs, Inc. and Contributors - * - * Licensed under the Dgraph Community License (the "License"); you - * may not use this file except in compliance with the License. You - * may obtain a copy of the License at - * - * https://github.com/dgraph-io/dgraph/blob/master/licenses/DCL.txt - */ -package acl - -import ( - "testing" - - "github.com/stretchr/testify/require" -) - -func TestUpdateAcl(t *testing.T) { - var currenAcls []Acl - newAcl := Acl{ - Predicate: "friend", - Perm: 4, - } - - updatedAcls1, changed := updateAcl(currenAcls, newAcl) - require.True(t, changed, "the acl list should be changed") - require.Equal(t, 1, len(updatedAcls1), - "the updated acl list should have 1 element") - - // trying to update the acl list again with the exactly same acl won't change it - updatedAcls2, changed := updateAcl(updatedAcls1, newAcl) - require.False(t, changed, - "the acl list should not be changed through update with an existing element") - require.Equal(t, 1, len(updatedAcls2), - "the updated acl list should still have 1 element") - require.Equal(t, int32(4), updatedAcls2[0].Perm, - "the perm should still have the value of 4") - - newAcl.Perm = 6 - updatedAcls3, changed := updateAcl(updatedAcls1, newAcl) - require.True(t, changed, "the acl list should be changed through update "+ - "with element of new perm") - require.Equal(t, 1, len(updatedAcls3), - "the updated acl list should still have 1 element") - require.Equal(t, int32(6), updatedAcls3[0].Perm, - "the updated perm should be 6 now") - - newAcl = Acl{ - Predicate: "buddy", - Perm: 6, - } - - updatedAcls4, changed := updateAcl(updatedAcls3, newAcl) - require.True(t, changed, "the acl should be changed through update "+ - "with element of new predicate") - require.Equal(t, 2, len(updatedAcls4), - "the acl list should have 2 elements now") - - newAcl = Acl{ - Predicate: "buddy", - Perm: -3, - } - - updatedAcls5, changed := updateAcl(updatedAcls4, newAcl) - require.True(t, changed, "the acl should be changed through update "+ - "with element of negative predicate") - require.Equal(t, 1, len(updatedAcls5), - "the acl list should have 1 element now") - require.Equal(t, "friend", updatedAcls5[0].Predicate, - "the left acl should have the original first predicate") -} diff --git a/x/x.go b/x/x.go index 8fe3a28fc1f..8cb03b64e23 100644 --- a/x/x.go +++ b/x/x.go @@ -109,8 +109,8 @@ const ( {"predicate":"dgraph.xid","type":"string", "index":true, "tokenizer":["exact"], "upsert":true}, {"predicate":"dgraph.password","type":"password"}, {"predicate":"dgraph.user.group","list":true, "reverse":true, "type":"uid"}, -{"predicate":"dgraph.acl.rule","type":"uid","list":true, "upsert":true}, -{"predicate":"dgraph.rule.predicate","type":"string","index":true,"tokenizer":["exact"]}, +{"predicate":"dgraph.acl.rule","type":"uid","list":true}, +{"predicate":"dgraph.rule.predicate","type":"string","index":true,"tokenizer":["exact"],"upsert":true}, {"predicate":"dgraph.rule.permission","type":"int"} ` // GroupIdFileName is the name of the file storing the ID of the group to which From e85e32ac40b9cabc83d8c092ca0fc81d947d65f9 Mon Sep 17 00:00:00 2001 From: animesh Date: Thu, 6 Feb 2020 21:22:25 +0530 Subject: [PATCH 12/16] Temp --- ee/acl/acl.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/ee/acl/acl.go b/ee/acl/acl.go index 6a4fd0e4697..d1a2dcc87ce 100644 --- a/ee/acl/acl.go +++ b/ee/acl/acl.go @@ -564,11 +564,6 @@ func queryGroup(ctx context.Context, txn *dgo.Txn, groupid string, return group, nil } -func isSameAcl(acl1 *Acl, acl2 *Acl) bool { - return (len(acl1.Predicate) > 0 && len(acl2.Predicate) > 0 && - acl1.Predicate == acl2.Predicate) -} - func queryAndPrintUser(ctx context.Context, txn *dgo.Txn, userId string) error { user, err := queryUser(ctx, txn, userId) if err != nil { From 6aa88893b61b2e4e5ba8c6c7b3aa6f9ae279ae94 Mon Sep 17 00:00:00 2001 From: animesh Date: Fri, 7 Feb 2020 14:49:37 +0530 Subject: [PATCH 13/16] Fix failing tests --- ee/acl/acl_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/ee/acl/acl_test.go b/ee/acl/acl_test.go index 444b28d9573..8450bf855d1 100644 --- a/ee/acl/acl_test.go +++ b/ee/acl/acl_test.go @@ -420,7 +420,7 @@ func TestAccessWithoutLoggingIn(t *testing.T) { } func TestUnauthorizedDeletion(t *testing.T) { - ctx, _ := context.WithTimeout(context.Background(), 100*time.Second) + ctx, _ := context.WithTimeout(context.Background(), 100*time.Minute) unAuthPred := "unauthorizedPredicate" dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) @@ -676,7 +676,6 @@ func TestQueryRemoveUnauthorizedPred(t *testing.T) { }, } - t.Parallel() for _, tc := range tests { t.Run(tc.description, func(t *testing.T) { resp, err := userClient.NewTxn().Query(ctx, tc.input) @@ -687,7 +686,7 @@ func TestQueryRemoveUnauthorizedPred(t *testing.T) { } func TestNewACLPredicates(t *testing.T) { - ctx, _ := context.WithTimeout(context.Background(), 100*time.Second) + ctx, _ := context.WithTimeout(context.Background(), 100*time.Minute) dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) require.NoError(t, err) @@ -731,7 +730,6 @@ func TestNewACLPredicates(t *testing.T) { }, } - t.Parallel() for _, tc := range queryTests { t.Run(tc.description, func(t *testing.T) { resp, err := userClient.NewTxn().Query(ctx, tc.input) @@ -771,7 +769,7 @@ func TestNewACLPredicates(t *testing.T) { } func TestNegativePermissionDeleteRule(t *testing.T) { - ctx, _ := context.WithTimeout(context.Background(), 100*time.Second) + ctx, _ := context.WithTimeout(context.Background(), 100*time.Minute) dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) require.NoError(t, err) From 78cfa80bb3f7e23f108cc1afc78faa09c9d0a37c Mon Sep 17 00:00:00 2001 From: animesh Date: Fri, 7 Feb 2020 15:17:27 +0530 Subject: [PATCH 14/16] Return err if setting rule for non existent group --- ee/acl/acl.go | 20 ++++++++++++++++++-- ee/acl/acl_test.go | 13 +++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/ee/acl/acl.go b/ee/acl/acl.go index d1a2dcc87ce..9940ff81574 100644 --- a/ee/acl/acl.go +++ b/ee/acl/acl.go @@ -14,6 +14,7 @@ package acl import ( "context" + "encoding/json" "fmt" "strings" "time" @@ -486,8 +487,23 @@ func chMod(conf *viper.Viper) error { upsertRequest.Mutations = []*api.Mutation{deleteRule} } resp, err := txn.Do(ctx, upsertRequest) - fmt.Println(string(resp.GetJson())) - return err + if err != nil { + return err + } + var jsonResp map[string][]map[string]int + err = json.Unmarshal(resp.GetJson(), &jsonResp) + if err != nil { + return err + } + + uidCount, ok := jsonResp["groupUIDCount"][0]["count"] + if !ok { + return errors.New("Malformed output of groupUIDCount") + } else if uidCount == 0 { + // We already have a check for multiple groups with same name at dgraph/ee/acl/utils.go:142 + return errors.Errorf("Group <%s> doesn't exist", groupId) + } + return nil } func queryUser(ctx context.Context, txn *dgo.Txn, userid string) (user *User, err error) { diff --git a/ee/acl/acl_test.go b/ee/acl/acl_test.go index 8450bf855d1..2b08fa7b4f1 100644 --- a/ee/acl/acl_test.go +++ b/ee/acl/acl_test.go @@ -861,3 +861,16 @@ func addDataAndRules(ctx context.Context, t *testing.T, dg *dgo.Dgraph) { _, err = dg.NewTxn().Mutate(ctx, mutation) require.NoError(t, err) } + +func TestNonExistentGroup(t *testing.T) { + dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) + require.NoError(t, err) + + testutil.DropAll(t, dg) + setRuleCmd := exec.Command("dgraph", "acl", "mod", "-a", dgraphEndpoint, "-g", + devGroup, "-p", "name", "-m", "4", "-x", "password") + + resp, err := setRuleCmd.CombinedOutput() + 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`) +} From 3da2f97042ab1222ce0d3c64e27184af31aa02b4 Mon Sep 17 00:00:00 2001 From: animesh Date: Fri, 7 Feb 2020 15:32:17 +0530 Subject: [PATCH 15/16] Write comments for chMod --- ee/acl/acl.go | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/ee/acl/acl.go b/ee/acl/acl.go index 9940ff81574..53e3a68b76a 100644 --- a/ee/acl/acl.go +++ b/ee/acl/acl.go @@ -397,16 +397,26 @@ func userMod(conf *viper.Viper, userId string, groups string) error { return queryAndPrintUser(ctx, dc.NewReadOnlyTxn(), userId) } +/* + chMod adds/updates/deletes rule attached to group. + 1. It will return error if there is no group named . + 2. It will add new rule if group doesn't already have a rule for the predicate. + 3. It will update the permission if group already have a rule for the predicate and permission + is a non-negative integer between 0-7. + 4. It will delete, if group already have a rule for the predicate and the permission is + a negative integer. +*/ + func chMod(conf *viper.Viper) error { - groupId := conf.GetString("group") + groupName := conf.GetString("group") predicate := conf.GetString("pred") perm := conf.GetInt("perm") switch { - case len(groupId) == 0: - return errors.Errorf("the groupid must not be empty") + case len(groupName) == 0: + return errors.Errorf("the group must not be empty") case len(predicate) == 0: return errors.Errorf("no predicates specified") - case perm > 7: // TODO(Animesh): also check if perm < 0 + case perm > 7: return errors.Errorf("the perm value must be less than or equal to 7, "+ "the provided value is %d", perm) } @@ -433,7 +443,7 @@ func chMod(conf *viper.Viper) error { rUID as dgraph.acl.rule @filter(eq(dgraph.rule.predicate, "%s")) } groupUIDCount(func: uid(gUID)) {count(uid)} - }`, groupId, predicate) + }`, groupName, predicate) updateRule := &api.Mutation{ Set: []*api.NQuad{ @@ -501,7 +511,7 @@ func chMod(conf *viper.Viper) error { return errors.New("Malformed output of groupUIDCount") } else if uidCount == 0 { // We already have a check for multiple groups with same name at dgraph/ee/acl/utils.go:142 - return errors.Errorf("Group <%s> doesn't exist", groupId) + return errors.Errorf("Group <%s> doesn't exist", groupName) } return nil } From 4e4cd0982c829ef860704c405912bb3669c2c00c Mon Sep 17 00:00:00 2001 From: animesh Date: Fri, 7 Feb 2020 15:46:25 +0530 Subject: [PATCH 16/16] Parallelize table driven tests --- ee/acl/acl_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/ee/acl/acl_test.go b/ee/acl/acl_test.go index 2b08fa7b4f1..3da816d1593 100644 --- a/ee/acl/acl_test.go +++ b/ee/acl/acl_test.go @@ -420,7 +420,7 @@ func TestAccessWithoutLoggingIn(t *testing.T) { } func TestUnauthorizedDeletion(t *testing.T) { - ctx, _ := context.WithTimeout(context.Background(), 100*time.Minute) + ctx, _ := context.WithTimeout(context.Background(), 100*time.Second) unAuthPred := "unauthorizedPredicate" dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) @@ -678,6 +678,7 @@ func TestQueryRemoveUnauthorizedPred(t *testing.T) { for _, tc := range tests { t.Run(tc.description, func(t *testing.T) { + t.Parallel() resp, err := userClient.NewTxn().Query(ctx, tc.input) require.Nil(t, err) testutil.CompareJSON(t, tc.output, string(resp.Json)) @@ -686,7 +687,7 @@ func TestQueryRemoveUnauthorizedPred(t *testing.T) { } func TestNewACLPredicates(t *testing.T) { - ctx, _ := context.WithTimeout(context.Background(), 100*time.Minute) + ctx, _ := context.WithTimeout(context.Background(), 100*time.Second) dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) require.NoError(t, err) @@ -732,6 +733,7 @@ func TestNewACLPredicates(t *testing.T) { for _, tc := range queryTests { t.Run(tc.description, func(t *testing.T) { + t.Parallel() resp, err := userClient.NewTxn().Query(ctx, tc.input) require.Nil(t, err) testutil.CompareJSON(t, tc.output, string(resp.Json)) @@ -759,6 +761,7 @@ func TestNewACLPredicates(t *testing.T) { } for _, tc := range mutationTests { t.Run(tc.description, func(t *testing.T) { + t.Parallel() _, err := userClient.NewTxn().Mutate(ctx, &api.Mutation{ SetNquads: []byte(tc.input), CommitNow: true, @@ -769,7 +772,7 @@ func TestNewACLPredicates(t *testing.T) { } func TestNegativePermissionDeleteRule(t *testing.T) { - ctx, _ := context.WithTimeout(context.Background(), 100*time.Minute) + ctx, _ := context.WithTimeout(context.Background(), 100*time.Second) dg, err := testutil.DgraphClientWithGroot(testutil.SockAddr) require.NoError(t, err)