diff --git a/dgraph/cmd/bulk/systest/test-bulk-schema.sh b/dgraph/cmd/bulk/systest/test-bulk-schema.sh index 97b2b746f3a..52ee675f7da 100755 --- a/dgraph/cmd/bulk/systest/test-bulk-schema.sh +++ b/dgraph/cmd/bulk/systest/test-bulk-schema.sh @@ -191,9 +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) - < 0 { 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/ee/acl/acl.go b/ee/acl/acl.go index 590a71b9d74..53e3a68b76a 100644 --- a/ee/acl/acl.go +++ b/ee/acl/acl.go @@ -397,13 +397,23 @@ 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: @@ -426,59 +436,84 @@ 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) - } - } - - var newAcl Acl - if len(predicate) > 0 { - newAcl = Acl{ - Predicate: predicate, - Perm: int32(perm), + ruleQuery := fmt.Sprintf(` + { + var(func: eq(dgraph.xid, "%s")) @filter(type(Group)) { + gUID as uid + rUID as dgraph.acl.rule @filter(eq(dgraph.rule.predicate, "%s")) } + groupUIDCount(func: uid(gUID)) {count(uid)} + }`, groupName, predicate) + + updateRule := &api.Mutation{ + Set: []*api.NQuad{ + { + Subject: "uid(rUID)", + Predicate: "dgraph.rule.permission", + ObjectValue: &api.Value{Val: &api.Value_IntVal{IntVal: int64(perm)}}, + }, + }, + Cond: "@if(eq(len(rUID), 1) AND eq(len(gUID), 1))", + } + + createRule := &api.Mutation{ + Set: []*api.NQuad{ + { + Subject: "_:newrule", + Predicate: "dgraph.rule.permission", + ObjectValue: &api.Value{Val: &api.Value_IntVal{IntVal: int64(perm)}}, + }, + { + Subject: "_:newrule", + Predicate: "dgraph.rule.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))", + } + + 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, } - newAcls, updated := updateAcl(currentAcls, newAcl) - if !updated { - fmt.Printf("Nothing needs to be changed for the permission of group: %v\n", groupId) - return nil + if perm < 0 { + upsertRequest.Mutations = []*api.Mutation{deleteRule} } - - newAclBytes, err := json.Marshal(newAcls) + resp, err := txn.Do(ctx, upsertRequest) 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}}, + return err } - mu := &api.Mutation{ - CommitNow: true, - Set: []*api.NQuad{chModNQuads}, + var jsonResp map[string][]map[string]int + err = json.Unmarshal(resp.GetJson(), &jsonResp) + if err != nil { + 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) + 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", groupName) } - 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) { @@ -531,10 +566,13 @@ func queryGroup(ctx context.Context, txn *dgo.Txn, groupid string, fields ...string) (group *Group, err error) { // write query header - query := fmt.Sprintf(`query search($groupid: string){ - group(func: eq(dgraph.xid, $groupid)) @filter(type(Group)) { - uid - %s }}`, strings.Join(fields, ", ")) + query := fmt.Sprintf(` + query search($groupid: string){ + group(func: eq(dgraph.xid, $groupid)) @filter(type(Group)) { + uid + %s + } + }`, strings.Join(fields, ", ")) queryVars := map[string]string{ "$groupid": groupid, @@ -552,33 +590,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) -} - -// 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 { @@ -598,13 +609,14 @@ func queryAndPrintUser(ctx context.Context, txn *dgo.Txn, userId string) error { func queryAndPrintGroup(ctx context.Context, txn *dgo.Txn, groupId string) error { group, err := queryGroup(ctx, txn, groupId, "dgraph.xid", "~dgraph.user.group{dgraph.xid}", - "dgraph.group.acl") + "dgraph.acl.rule{dgraph.rule.predicate, dgraph.rule.permission}") if err != nil { return err } if group == nil { - return errors.Errorf("The group %q does not exist.\n", groupId) + return errors.Errorf("The group %s doesn't exist", groupId) } + fmt.Printf("Group: %s\n", groupId) fmt.Printf("UID : %s\n", group.Uid) fmt.Printf("ID : %s\n", group.GroupID) @@ -615,17 +627,10 @@ func queryAndPrintGroup(ctx context.Context, txn *dgo.Txn, groupId string) error } fmt.Printf("Users: %s\n", strings.Join(userNames, " ")) - var acls []Acl - if len(group.Acls) != 0 { - if err := json.Unmarshal([]byte(group.Acls), &acls); err != nil { - return errors.Wrapf(err, "unable to unmarshal the acls associated with the group %v", - groupId) - } - - for _, acl := range acls { - fmt.Printf("ACL : %v\n", acl) - } + for _, acl := range group.Rules { + fmt.Printf("ACL: %v\n", acl) } + return nil } diff --git a/ee/acl/acl_test.go b/ee/acl/acl_test.go index 97408c5aa31..3da816d1593 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" @@ -675,12 +676,204 @@ func TestQueryRemoveUnauthorizedPred(t *testing.T) { }, } - t.Parallel() 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)) }) } } + +func TestNewACLPredicates(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) + + 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`, + }, + } + + 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)) + }) + } + + 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) { + t.Parallel() + _, err := userClient.NewTxn().Mutate(ctx, &api.Mutation{ + SetNquads: []byte(tc.input), + CommitNow: true, + }) + require.True(t, (err == nil) == (tc.err == nil)) + }) + } +} + +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) +} + +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`) +} 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/ee/acl/utils.go b/ee/acl/utils.go index 48a025c1edb..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:"predicate"` - Perm int32 `json:"perm"` + Predicate string `json:"dgraph.rule.predicate"` + Perm int32 `json:"dgraph.rule.permission"` } // Group represents a group in the ACL system. @@ -115,7 +115,7 @@ 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"` } // GetUid returns the UID of the group. diff --git a/schema/schema.go b/schema/schema.go index f71f7d52def..7c71b0c0bf7 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -499,8 +499,20 @@ func initialSchemaInternal(all bool) []*pb.SchemaUpdate { List: true, }, { - Predicate: "dgraph.group.acl", + Predicate: "dgraph.acl.rule", + ValueType: pb.Posting_UID, + List: 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", + ValueType: pb.Posting_INT, }, }...) } diff --git a/systest/queries_test.go b/systest/queries_test.go index 9ef963f7900..eb4058b9554 100644 --- a/systest/queries_test.go +++ b/systest/queries_test.go @@ -392,8 +392,14 @@ func SchemaQueryTestPredicate1(t *testing.T, c *dgo.Dgraph) { { "predicate": "dgraph.password" }, - { - "predicate": "dgraph.group.acl" + { + "predicate": "dgraph.acl.rule" + }, + { + "predicate": "dgraph.rule.predicate" + }, + { + "predicate": "dgraph.rule.permission" }, { "predicate": "dgraph.graphql.schema" diff --git a/x/keys.go b/x/keys.go index cb0c0ab1fd6..3a58329df0c 100644 --- a/x/keys.go +++ b/x/keys.go @@ -554,10 +554,12 @@ 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.rule.predicate": {}, + "dgraph.rule.permission": {}, + "dgraph.acl.rule": {}, } var graphqlReservedPredicate = map[string]struct{}{ diff --git a/x/x.go b/x/x.go index 27f872e7d15..c806b690cc5 100644 --- a/x/x.go +++ b/x/x.go @@ -108,10 +108,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.user.group","list":true, "reverse":true, "type":"uid"}, +{"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 // the data in a postings directory belongs. This ID is used to join the proper