From 66bb89d2f70f92ad2fe72fb1a5543a5a7c029b36 Mon Sep 17 00:00:00 2001 From: Anurag Date: Mon, 31 Aug 2020 22:46:20 +0530 Subject: [PATCH] fix(Query) Fix Star_All delete query when used with ACL enabled (#6331) (cherry picked from commit 501f33855aca5e1fa9b98ec75efb450d91e7b749) --- edgraph/access_ee.go | 9 ++-- ee/acl/acl_test.go | 122 +++++++++++++++++++++++++++++++++++++++++++ gql/mutation.go | 7 +-- protos/pb/pb.pb.go | 8 +-- query/mutation.go | 30 +++++++++++ 5 files changed, 167 insertions(+), 9 deletions(-) diff --git a/edgraph/access_ee.go b/edgraph/access_ee.go index d40b4405ecd..7d6c289d119 100644 --- a/edgraph/access_ee.go +++ b/edgraph/access_ee.go @@ -583,7 +583,10 @@ func parsePredsFromMutation(nquads []*api.NQuad) []string { // use a map to dedup predicates predsMap := make(map[string]struct{}) for _, nquad := range nquads { - predsMap[nquad.Predicate] = struct{}{} + // _STAR_ALL is not a predicate in itself. + if nquad.Predicate != "_STAR_ALL" { + predsMap[nquad.Predicate] = struct{}{} + } } preds := make([]string, 0, len(predsMap)) @@ -658,7 +661,7 @@ func authorizeMutation(ctx context.Context, gmu *gql.Mutation) error { return nil } - blockedPreds, _ := authorizePreds(userId, groupIds, preds, acl.Write) + blockedPreds, allowedPreds := authorizePreds(userId, groupIds, preds, acl.Write) if len(blockedPreds) > 0 { var msg strings.Builder for key := range blockedPreds { @@ -668,7 +671,7 @@ func authorizeMutation(ctx context.Context, gmu *gql.Mutation) error { return status.Errorf(codes.PermissionDenied, "unauthorized to mutate following predicates: %s\n", msg.String()) } - + gmu.AllowedPreds = allowedPreds return nil } diff --git a/ee/acl/acl_test.go b/ee/acl/acl_test.go index b1d7a6e1722..984e320a644 100644 --- a/ee/acl/acl_test.go +++ b/ee/acl/acl_test.go @@ -1255,6 +1255,128 @@ func TestExpandQueryWithACLPermissions(t *testing.T) { testutil.CompareJSON(t, `{"me":[{"name":"RandomGuy","age":23, "nickname":"RG"},{"name":"RandomGuy2","age":25, "nickname":"RG2"}]}`, string(resp.GetJson())) +} +func TestDeleteQueryWithACLPermissions(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Second) + defer cancel() + 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) . + age : int . + type Person { + name: string + nickname: string + age: int + } + `} + require.NoError(t, dg.Alter(ctx, &op)) + + resetUser(t) + + accessJwt, _, err := testutil.HttpLogin(&testutil.LoginParams{ + Endpoint: adminEndpoint, + UserID: "groot", + Passwd: "password", + }) + require.NoError(t, err, "login failed") + + createGroup(t, accessJwt, devGroup) + + addToGroup(t, accessJwt, userid, devGroup) + + txn := dg.NewTxn() + mutation := &api.Mutation{ + SetNquads: []byte(` + _:a "RandomGuy" . + _:a "23" . + _:a "RG" . + _:a "Person" . + _:b "RandomGuy2" . + _:b "25" . + _:b "RG2" . + _:b "Person" . + `), + CommitNow: true, + } + resp, err := txn.Mutate(ctx, mutation) + require.NoError(t, err) + + nodeUID := resp.Uids["a"] + query := `{q1(func: type(Person)){ + expand(_all_) + }}` + + // Test that groot has access to all the predicates + resp, err = dg.NewReadOnlyTxn().Query(ctx, query) + require.NoError(t, err, "Error while querying data") + testutil.CompareJSON(t, `{"q1":[{"name":"RandomGuy","age":23, "nickname": "RG"},{"name":"RandomGuy2","age":25, "nickname": "RG2"}]}`, + string(resp.GetJson())) + + // Give Write Access to alice for name and age predicate + addRulesToGroup(t, accessJwt, devGroup, []rule{{"name", Write.Code}, {"age", Write.Code}}) + + 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) + + // delete S * * (user now has permission to name and age) + txn = userClient.NewTxn() + mutString := fmt.Sprintf("<%s> * * .", nodeUID) + mutation = &api.Mutation{ + DelNquads: []byte(mutString), + CommitNow: true, + } + _, err = txn.Mutate(ctx, mutation) + require.NoError(t, err) + + accessJwt, _, err = testutil.HttpLogin(&testutil.LoginParams{ + Endpoint: adminEndpoint, + UserID: "groot", + Passwd: "password", + }) + require.NoError(t, err, "login failed") + + resp, err = dg.NewReadOnlyTxn().Query(ctx, query) + require.NoError(t, err, "Error while querying data") + // Only name and age predicates got deleted via user - alice + testutil.CompareJSON(t, `{"q1":[{"nickname": "RG"},{"name":"RandomGuy2","age":25, "nickname": "RG2"}]}`, + string(resp.GetJson())) + + // Give write access of to dev + addRulesToGroup(t, accessJwt, devGroup, []rule{{"name", Write.Code}, {"age", Write.Code}, {"dgraph.type", Write.Code}}) + time.Sleep(6 * time.Second) + + // delete S * * (user now has permission to name, age and dgraph.type) + txn = userClient.NewTxn() + mutString = fmt.Sprintf("<%s> * * .", nodeUID) + mutation = &api.Mutation{ + DelNquads: []byte(mutString), + CommitNow: true, + } + _, err = txn.Mutate(ctx, mutation) + require.NoError(t, err) + + accessJwt, _, err = testutil.HttpLogin(&testutil.LoginParams{ + Endpoint: adminEndpoint, + UserID: "groot", + Passwd: "password", + }) + require.NoError(t, err, "login failed") + + resp, err = dg.NewReadOnlyTxn().Query(ctx, query) + require.NoError(t, err, "Error while querying data") + // Because alise had permission to dgraph.type the node reference has been deleted + testutil.CompareJSON(t, `{"q1":[{"name":"RandomGuy2","age":25, "nickname": "RG2"}]}`, + string(resp.GetJson())) + } func TestValQueryWithACLPermissions(t *testing.T) { diff --git a/gql/mutation.go b/gql/mutation.go index 2a2afb8e2f9..50438d0abae 100644 --- a/gql/mutation.go +++ b/gql/mutation.go @@ -32,9 +32,10 @@ var ( // Mutation stores the strings corresponding to set and delete operations. type Mutation struct { - Cond string - Set []*api.NQuad - Del []*api.NQuad + Cond string + Set []*api.NQuad + Del []*api.NQuad + AllowedPreds []string Metadata *pb.Metadata } diff --git a/protos/pb/pb.pb.go b/protos/pb/pb.pb.go index 0399388d02f..5db7db4151f 100644 --- a/protos/pb/pb.pb.go +++ b/protos/pb/pb.pb.go @@ -7,6 +7,10 @@ import ( context "context" encoding_binary "encoding/binary" fmt "fmt" + io "io" + math "math" + math_bits "math/bits" + pb "github.com/dgraph-io/badger/v2/pb" api "github.com/dgraph-io/dgo/v200/protos/api" _ "github.com/gogo/protobuf/gogoproto" @@ -14,9 +18,6 @@ import ( grpc "google.golang.org/grpc" codes "google.golang.org/grpc/codes" status "google.golang.org/grpc/status" - io "io" - math "math" - math_bits "math/bits" ) // Reference imports to suppress errors if they are not otherwise used. @@ -1800,6 +1801,7 @@ type DirectedEdge struct { Lang string `protobuf:"bytes,7,opt,name=lang,proto3" json:"lang,omitempty"` Op DirectedEdge_Op `protobuf:"varint,8,opt,name=op,proto3,enum=pb.DirectedEdge_Op" json:"op,omitempty"` Facets []*api.Facet `protobuf:"bytes,9,rep,name=facets,proto3" json:"facets,omitempty"` + AllowedPreds []string `protobuf:"bytes,10,rep,name=allowed_preds,proto3" json:"allowed_preds,omitempty"` XXX_NoUnkeyedLiteral struct{} `json:"-"` XXX_unrecognized []byte `json:"-"` XXX_sizecache int32 `json:"-"` diff --git a/query/mutation.go b/query/mutation.go index 8678521f7dc..10783b7dea9 100644 --- a/query/mutation.go +++ b/query/mutation.go @@ -70,6 +70,22 @@ func expandEdges(ctx context.Context, m *pb.Mutations) ([]*pb.DirectedEdge, erro } preds = append(preds, getPredicatesFromTypes(types)...) preds = append(preds, x.StarAllPredicates()...) + // AllowedPreds are used only with ACL. Do not delete all predicates but + // delete predicates to which the mutation has access + if edge.AllowedPreds != nil { + // Take intersection of preds and AllowedPreds + intersectPreds := make([]string, 0) + hashMap := make(map[string]bool) + for _, allowedPred := range edge.AllowedPreds { + hashMap[allowedPred] = true + } + for _, pred := range preds { + if _, found := hashMap[pred]; found { + intersectPreds = append(intersectPreds, pred) + } + } + preds = intersectPreds + } } for _, pred := range preds { @@ -192,6 +208,20 @@ func ToDirectedEdges(gmuList []*gql.Mutation, newUids map[string]uint64) ( } for _, gmu := range gmuList { + // We delete first and then we set. Order of the mutation is important. + for _, nq := range gmu.Del { + if nq.Subject == x.Star && nq.ObjectValue.GetDefaultVal() == x.Star { + return edges, errors.New("Predicate deletion should be called via alter") + } + if err := parse(nq, pb.DirectedEdge_DEL); err != nil { + return edges, err + } + if gmu.AllowedPreds != nil { + for _, e := range edges { + e.AllowedPreds = gmu.AllowedPreds + } + } + } for _, nq := range gmu.Set { if err := facets.SortAndValidate(nq.Facets); err != nil { return edges, err