From e20c77ca4ddfa66c7356c5413b0d4665f65e84bc Mon Sep 17 00:00:00 2001 From: Jassi Date: Wed, 16 Aug 2023 19:57:20 +0530 Subject: [PATCH] fix(acl): allow data deletion for non-reserved predicates (#8937) when data (non-reserved predicates) is added on UIDs that belong to groot user or guardian group, it is allowed. But when the same data is deleted, that is not allowed. This PR allows deletion of non-reserved predicates on special UIDs. Closes: https://dgraph.atlassian.net/browse/DGRAPHCORE-355 --- ee/acl/acl_test.go | 4 +-- query/common_test.go | 2 +- query/integration_test.go | 8 ++++-- query/mutation.go | 5 +++- query/mutation_test.go | 47 ++++++++++++++++++++++++++++++++++- query/query0_test.go | 3 +-- query/upgrade_test.go | 4 +-- systest/bgindex/count_test.go | 2 +- 8 files changed, 63 insertions(+), 12 deletions(-) diff --git a/ee/acl/acl_test.go b/ee/acl/acl_test.go index de5dd017cd9..c8cbaf61209 100644 --- a/ee/acl/acl_test.go +++ b/ee/acl/acl_test.go @@ -2793,13 +2793,13 @@ func (asuite *AclTestSuite) TestDeleteGrootAndGuardiansUsingDelNQuadShouldFail() // Try deleting groot user _, err = gc.Mutate(mu) require.Error(t, err, "Deleting groot user should have returned an error") - require.Contains(t, err.Error(), "Properties of guardians group and groot user cannot be deleted") + require.Contains(t, err.Error(), "properties of guardians group and groot user cannot be deleted") mu = &api.Mutation{DelNquads: []byte(fmt.Sprintf("%s %s %s .", "<"+guardiansUid+">", "*", "*")), CommitNow: true} // Try deleting guardians group _, err = gc.Mutate(mu) require.Error(t, err, "Deleting guardians group should have returned an error") - require.Contains(t, err.Error(), "Properties of guardians group and groot user cannot be deleted") + require.Contains(t, err.Error(), "properties of guardians group and groot user cannot be deleted") } func deleteGuardiansGroupAndGrootUserShouldFail(t *testing.T, hc *dgraphtest.HTTPClient) { diff --git a/query/common_test.go b/query/common_test.go index 7d9345f146c..78c6ce64ea5 100644 --- a/query/common_test.go +++ b/query/common_test.go @@ -352,7 +352,7 @@ func populateCluster() { // In the query package, we test using hard coded UIDs so that we know what results // to expect. We need to move the max assigned UID in zero to higher value than // all the UIDs we are using during the tests. - x.Panic(dc.AssignUids(client, 65536)) + x.Panic(dc.AssignUids(client.Dgraph, 65536)) setSchema(testSchema) err := addTriplesToCluster(` diff --git a/query/integration_test.go b/query/integration_test.go index 2cae4f62387..901a1d11442 100644 --- a/query/integration_test.go +++ b/query/integration_test.go @@ -19,10 +19,10 @@ package query import ( + "context" "testing" "github.com/dgraph-io/dgraph/dgraphtest" - "github.com/dgraph-io/dgraph/testutil" "github.com/dgraph-io/dgraph/x" ) @@ -30,8 +30,12 @@ func TestMain(m *testing.M) { dc = dgraphtest.NewComposeCluster() var err error - client, err = testutil.DgraphClientWithGroot(testutil.SockAddr) + var cleanup func() + client, cleanup, err = dc.Client() x.Panic(err) + defer cleanup() + x.Panic(client.LoginIntoNamespace(context.Background(), dgraphtest.DefaultUser, + dgraphtest.DefaultPassword, x.GalaxyNamespace)) populateCluster() m.Run() diff --git a/query/mutation.go b/query/mutation.go index 274870c1e2f..df586c58b61 100644 --- a/query/mutation.go +++ b/query/mutation.go @@ -286,6 +286,9 @@ func checkIfDeletingAclOperation(ctx context.Context, edges []*pb.DirectedEdge) isDeleteAclOperation := false for _, edge := range edges { + if !x.IsReservedPredicate(edge.Attr) { + continue + } // Disallow deleting of guardians group if edge.Entity == guardianUid && edge.Op == pb.DirectedEdge_DEL { isDeleteAclOperation = true @@ -298,7 +301,7 @@ func checkIfDeletingAclOperation(ctx context.Context, edges []*pb.DirectedEdge) } } if isDeleteAclOperation { - return errors.Errorf("Properties of guardians group and groot user cannot be deleted.") + return errors.Errorf("Reserved properties of guardians group and groot user cannot be deleted.") } return nil } diff --git a/query/mutation_test.go b/query/mutation_test.go index f76287b968d..f2db87b6e4d 100644 --- a/query/mutation_test.go +++ b/query/mutation_test.go @@ -20,6 +20,9 @@ package query import ( "context" + "encoding/json" + "fmt" + "strconv" "testing" "time" @@ -28,7 +31,7 @@ import ( "github.com/dgraph-io/dgo/v230/protos/api" ) -func TestReserverPredicateForMutation(t *testing.T) { +func TestReservedPredicateForMutation(t *testing.T) { err := addTriplesToCluster(`_:x "df"`) require.Error(t, err, "Cannot mutate graphql reserved predicate dgraph.graphql.schema") } @@ -70,3 +73,45 @@ func TestAlteringReservedTypesAndPredicatesShouldFail(t *testing.T) { require.Contains(t, err.Error(), "Can't store predicate `dgraph.name` as it is prefixed with "+ "`dgraph.` which is reserved as the namespace for dgraph's internal types/predicates.") } + +func TestUnreservedPredicateForDeletion(t *testing.T) { + grootUserQuery := ` + { + grootUser(func:eq(dgraph.xid, "groot")){ + uid + } + }` + + // Structs to parse groot user query response + type userNode struct { + Uid string `json:"uid"` + } + + type userQryResp struct { + GrootUser []userNode `json:"grootUser"` + } + + resp, err := client.Query(grootUserQuery) + require.NoError(t, err, "groot user query failed") + + var userResp userQryResp + if err := json.Unmarshal(resp.GetJson(), &userResp); err != nil { + t.Fatal("Couldn't unmarshal response from groot user query") + } + grootsUidStr := userResp.GrootUser[0].Uid + grootsUidInt, err := strconv.ParseUint(grootsUidStr, 0, 64) + require.NoError(t, err) + require.Greater(t, uint64(grootsUidInt), uint64(0)) + + const testSchema = ` +type Star { + name +} + +name : string @index(term, exact, trigram) @count @lang . +` + triple := fmt.Sprintf(`<%s> "Betelgeuse" .`, grootsUidStr) + setSchema(testSchema) + require.NoError(t, addTriplesToCluster(triple)) + deleteTriplesInCluster(triple) +} diff --git a/query/query0_test.go b/query/query0_test.go index a6477f5c14e..f16afebb07f 100644 --- a/query/query0_test.go +++ b/query/query0_test.go @@ -27,7 +27,6 @@ import ( "github.com/stretchr/testify/require" - "github.com/dgraph-io/dgo/v230" "github.com/dgraph-io/dgraph/dgraphtest" "github.com/dgraph-io/dgraph/dql" ) @@ -3576,5 +3575,5 @@ func TestInvalidRegex(t *testing.T) { } } -var client *dgo.Dgraph +var client *dgraphtest.GrpcClient var dc dgraphtest.Cluster diff --git a/query/upgrade_test.go b/query/upgrade_test.go index 214b55fcc72..1dd232d7e61 100644 --- a/query/upgrade_test.go +++ b/query/upgrade_test.go @@ -37,7 +37,7 @@ func TestMain(m *testing.M) { x.Panic(dg.LoginIntoNamespace(context.Background(), dgraphtest.DefaultUser, dgraphtest.DefaultPassword, x.GalaxyNamespace)) - client = dg.Dgraph + client = dg dc = c populateCluster() } @@ -49,7 +49,7 @@ func TestMain(m *testing.M) { x.Panic(dg.LoginIntoNamespace(context.Background(), dgraphtest.DefaultUser, dgraphtest.DefaultPassword, x.GalaxyNamespace)) - client = dg.Dgraph + client = dg dc = c return m.Run() } diff --git a/systest/bgindex/count_test.go b/systest/bgindex/count_test.go index 6f259affe84..5f278301ad0 100644 --- a/systest/bgindex/count_test.go +++ b/systest/bgindex/count_test.go @@ -123,7 +123,7 @@ func TestCountIndex(t *testing.T) { CommitNow: true, DelNquads: []byte(fmt.Sprintf(`<%v> "%v" .`, uid, ec-1)), }); err != nil && (errors.Is(err, dgo.ErrAborted) || - strings.Contains(err.Error(), "Properties of guardians group and groot user cannot be deleted")) { + strings.Contains(err.Error(), "properties of guardians group and groot user cannot be deleted")) { return } else if err != nil { t.Fatalf("error in deletion :: %v\n", err)