From fff505845103439e2da9a1ca674cfa474198cd45 Mon Sep 17 00:00:00 2001 From: ShivajiKharse <115525374+shivaji-dgraph@users.noreply.github.com> Date: Thu, 30 Nov 2023 19:58:07 +0530 Subject: [PATCH] fix(acl): fix duplicate groot user creation (#51) This PR adds the unique directive to the 'dgraph.xid' & 'dgraph.graphql.xid' predicates. Prior to this change, users could create duplicate users leading to misconfiguration of ACL. --- dgraphtest/ee.go | 2 +- ee/acl/acl_integration_test.go | 1 + ee/acl/acl_test.go | 434 +++++++++++++------- graphql/e2e/directives/schema_response.json | 3 +- graphql/e2e/normal/schema_response.json | 3 +- schema/schema.go | 2 + systest/export/export_test.go | 2 +- systest/integration2/acl_test.go | 52 +++ testutil/schema.go | 4 +- worker/export.go | 3 + 10 files changed, 362 insertions(+), 144 deletions(-) create mode 100644 systest/integration2/acl_test.go diff --git a/dgraphtest/ee.go b/dgraphtest/ee.go index 6f3b4316769..b660ca2826d 100644 --- a/dgraphtest/ee.go +++ b/dgraphtest/ee.go @@ -123,7 +123,7 @@ func (hc *HTTPClient) CreateGroup(name string) (string, error) { } resp, err := hc.RunGraphqlQuery(params, true) if err != nil { - return "", nil + return "", err } type Response struct { AddGroup struct { diff --git a/ee/acl/acl_integration_test.go b/ee/acl/acl_integration_test.go index 0679b03fced..ed51a9eb9c3 100644 --- a/ee/acl/acl_integration_test.go +++ b/ee/acl/acl_integration_test.go @@ -412,6 +412,7 @@ func (asuite *AclTestSuite) TestWrongPermission() { defer cleanup() require.NoError(t, gc.LoginIntoNamespace(ctx, dgraphtest.DefaultUser, dgraphtest.DefaultPassword, x.GalaxyNamespace)) + require.NoError(t, gc.DropAll()) mu := &api.Mutation{SetNquads: []byte(` _:dev "dgraph.type.Group" . diff --git a/ee/acl/acl_test.go b/ee/acl/acl_test.go index 88b78f52d55..23234bfb537 100644 --- a/ee/acl/acl_test.go +++ b/ee/acl/acl_test.go @@ -35,6 +35,275 @@ var ( userpassword = "simplepassword" ) +const ( + + // This is the groot schema before adding @unique directive to the dgraph.xid predicate + oldGrootSchema = `{ + "schema": [ + { + "predicate": "dgraph.acl.rule", + "type": "uid", + "list": true + }, + { + "predicate":"dgraph.drop.op", + "type":"string" + }, + { + "predicate":"dgraph.graphql.p_query", + "type":"string", + "index":true, + "tokenizer":["sha256"] + }, + { + "predicate": "dgraph.graphql.schema", + "type": "string" + }, + { + "predicate": "dgraph.graphql.xid", + "type": "string", + "index": true, + "tokenizer": [ + "exact" + ], + "upsert": true + }, + { + "predicate": "dgraph.password", + "type": "password" + }, + { + "predicate": "dgraph.rule.permission", + "type": "int" + }, + { + "predicate": "dgraph.rule.predicate", + "type": "string", + "index": true, + "tokenizer": [ + "exact" + ], + "upsert": true + }, + { + "predicate": "dgraph.type", + "type": "string", + "index": true, + "tokenizer": [ + "exact" + ], + "list": true + }, + { + "predicate": "dgraph.user.group", + "type": "uid", + "reverse": true, + "list": true + }, + { + "predicate": "dgraph.xid", + "type": "string", + "index": true, + "tokenizer": [ + "exact" + ], + "upsert": true + } + ], + "types": [ + { + "fields": [ + { + "name": "dgraph.graphql.schema" + }, + { + "name": "dgraph.graphql.xid" + } + ], + "name": "dgraph.graphql" + }, + { + "fields": [ + { + "name": "dgraph.graphql.p_query" + } + ], + "name": "dgraph.graphql.persisted_query" + }, + { + "fields": [ + { + "name": "dgraph.xid" + }, + { + "name": "dgraph.acl.rule" + } + ], + "name": "dgraph.type.Group" + }, + { + "fields": [ + { + "name": "dgraph.rule.predicate" + }, + { + "name": "dgraph.rule.permission" + } + ], + "name": "dgraph.type.Rule" + }, + { + "fields": [ + { + "name": "dgraph.xid" + }, + { + "name": "dgraph.password" + }, + { + "name": "dgraph.user.group" + } + ], + "name": "dgraph.type.User" + } + ] + }` + + // This is the groot schema after adding @unique directive to the dgraph.xid predicate + newGrootSchema = `{ + "schema": [ + { + "predicate": "dgraph.acl.rule", + "type": "uid", + "list": true + }, + { + "predicate":"dgraph.drop.op", + "type":"string" + }, + { + "predicate":"dgraph.graphql.p_query", + "type":"string", + "index":true, + "tokenizer":["sha256"] + }, + { + "predicate": "dgraph.graphql.schema", + "type": "string" + }, + { + "predicate": "dgraph.graphql.xid", + "type": "string", + "index": true, + "tokenizer": [ + "exact" + ], + "upsert": true, + "unique": true + }, + { + "predicate": "dgraph.password", + "type": "password" + }, + { + "predicate": "dgraph.rule.permission", + "type": "int" + }, + { + "predicate": "dgraph.rule.predicate", + "type": "string", + "index": true, + "tokenizer": [ + "exact" + ], + "upsert": true + }, + { + "predicate": "dgraph.type", + "type": "string", + "index": true, + "tokenizer": [ + "exact" + ], + "list": true + }, + { + "predicate": "dgraph.user.group", + "type": "uid", + "reverse": true, + "list": true + }, + { + "predicate": "dgraph.xid", + "type": "string", + "index": true, + "tokenizer": [ + "exact" + ], + "upsert": true, + "unique": true + } + ], + "types": [ + { + "fields": [ + { + "name": "dgraph.graphql.schema" + }, + { + "name": "dgraph.graphql.xid" + } + ], + "name": "dgraph.graphql" + }, + { + "fields": [ + { + "name": "dgraph.graphql.p_query" + } + ], + "name": "dgraph.graphql.persisted_query" + }, + { + "fields": [ + { + "name": "dgraph.xid" + }, + { + "name": "dgraph.acl.rule" + } + ], + "name": "dgraph.type.Group" + }, + { + "fields": [ + { + "name": "dgraph.rule.predicate" + }, + { + "name": "dgraph.rule.permission" + } + ], + "name": "dgraph.type.Rule" + }, + { + "fields": [ + { + "name": "dgraph.xid" + }, + { + "name": "dgraph.password" + }, + { + "name": "dgraph.user.group" + } + ], + "name": "dgraph.type.User" + } + ] + }` +) + func checkUserCount(t *testing.T, resp []byte, expected int) { type Response struct { AddUser struct { @@ -142,7 +411,7 @@ func (asuite *AclTestSuite) TestPreDefinedPredicates() { ctx := context.Background() require.NoError(t, gc.LoginIntoNamespace(ctx, dgraphtest.DefaultUser, dgraphtest.DefaultPassword, x.GalaxyNamespace)) - alterPreDefinedPredicates(t, gc.Dgraph) + alterPreDefinedPredicates(t, gc.Dgraph, asuite.dc.GetVersion()) } func (asuite *AclTestSuite) TestPreDefinedTypes() { @@ -295,14 +564,25 @@ var ( }`, predicateToRead, queryAttr) ) -func alterPreDefinedPredicates(t *testing.T, dg *dgo.Dgraph) { +func alterPreDefinedPredicates(t *testing.T, dg *dgo.Dgraph, clusterVersion string) { ctx := context.Background() - // Test that alter requests are allowed if the new update is the same as - // the initial update for a pre-defined predicate. - require.NoError(t, dg.Alter(ctx, &api.Operation{ - Schema: "dgraph.xid: string @index(exact) @upsert .", - })) + // Commit daa5805739ed258e913a157c6e0f126b2291b1b0 represents the latest update to the main branch. + // In this commit, the @unique directive is not applied to ACL predicates. + // Therefore, we are now deciding which schema to test. + // 'newGrootSchema' refers to the default schema with the @unique directive defined on ACL predicates. + // 'oldGrootSchema' refers to the default schema without the @unique directive on ACL predicates. + supported, err := dgraphtest.IsHigherVersion(clusterVersion, "daa5805739ed258e913a157c6e0f126b2291b1b0") + require.NoError(t, err) + if supported { + require.NoError(t, dg.Alter(ctx, &api.Operation{ + Schema: "dgraph.xid: string @index(exact) @upsert @unique .", + })) + } else { + require.NoError(t, dg.Alter(ctx, &api.Operation{ + Schema: "dgraph.xid: string @index(exact) @upsert .", + })) + } err := dg.Alter(ctx, &api.Operation{Schema: "dgraph.xid: int ."}) require.Error(t, err) @@ -1807,6 +2087,7 @@ func (asuite *AclTestSuite) TestQueryUserInfo() { func (asuite *AclTestSuite) TestQueriesWithUserAndGroupOfSameName() { t := asuite.T() + dgraphtest.ShouldSkipTest(t, asuite.dc.GetVersion(), "7b1f473ddf01547e24b44f580a68e6b049502d69") ctx, cancel := context.WithTimeout(context.Background(), 100*time.Second) defer cancel() @@ -1923,136 +2204,7 @@ func (asuite *AclTestSuite) TestSchemaQueryWithACL() { defer cancel() schemaQuery := "schema{}" - grootSchema := `{ - "schema": [ - { - "predicate": "dgraph.acl.rule", - "type": "uid", - "list": true - }, - { - "predicate":"dgraph.drop.op", - "type":"string" - }, - { - "predicate":"dgraph.graphql.p_query", - "type":"string", - "index":true, - "tokenizer":["sha256"] - }, - { - "predicate": "dgraph.graphql.schema", - "type": "string" - }, - { - "predicate": "dgraph.graphql.xid", - "type": "string", - "index": true, - "tokenizer": [ - "exact" - ], - "upsert": true - }, - { - "predicate": "dgraph.password", - "type": "password" - }, - { - "predicate": "dgraph.rule.permission", - "type": "int" - }, - { - "predicate": "dgraph.rule.predicate", - "type": "string", - "index": true, - "tokenizer": [ - "exact" - ], - "upsert": true - }, - { - "predicate": "dgraph.type", - "type": "string", - "index": true, - "tokenizer": [ - "exact" - ], - "list": true - }, - { - "predicate": "dgraph.user.group", - "type": "uid", - "reverse": true, - "list": true - }, - { - "predicate": "dgraph.xid", - "type": "string", - "index": true, - "tokenizer": [ - "exact" - ], - "upsert": true - } - ], - "types": [ - { - "fields": [ - { - "name": "dgraph.graphql.schema" - }, - { - "name": "dgraph.graphql.xid" - } - ], - "name": "dgraph.graphql" - }, - { - "fields": [ - { - "name": "dgraph.graphql.p_query" - } - ], - "name": "dgraph.graphql.persisted_query" - }, - { - "fields": [ - { - "name": "dgraph.xid" - }, - { - "name": "dgraph.acl.rule" - } - ], - "name": "dgraph.type.Group" - }, - { - "fields": [ - { - "name": "dgraph.rule.predicate" - }, - { - "name": "dgraph.rule.permission" - } - ], - "name": "dgraph.type.Rule" - }, - { - "fields": [ - { - "name": "dgraph.xid" - }, - { - "name": "dgraph.password" - }, - { - "name": "dgraph.user.group" - } - ], - "name": "dgraph.type.User" - } - ] -}` + aliceSchema := `{ "schema": [ { @@ -2102,7 +2254,13 @@ func (asuite *AclTestSuite) TestSchemaQueryWithACL() { require.NoError(t, gc.DropAll()) resp, err := gc.Query(schemaQuery) require.NoError(t, err) - require.JSONEq(t, grootSchema, string(resp.GetJson())) + supported, err := dgraphtest.IsHigherVersion(asuite.dc.GetVersion(), "daa5805739ed258e913a157c6e0f126b2291b1b0") + require.NoError(t, err) + if supported { + require.JSONEq(t, newGrootSchema, string(resp.GetJson())) + } else { + require.JSONEq(t, oldGrootSchema, string(resp.GetJson())) + } // add another user and some data for that user with permissions on predicates resetUser(t, hc) diff --git a/graphql/e2e/directives/schema_response.json b/graphql/e2e/directives/schema_response.json index b41cb67668b..f26182e2e8b 100644 --- a/graphql/e2e/directives/schema_response.json +++ b/graphql/e2e/directives/schema_response.json @@ -678,7 +678,8 @@ "tokenizer": [ "exact" ], - "upsert": true + "upsert": true, + "unique":true }, { "predicate": "dgraph.type", diff --git a/graphql/e2e/normal/schema_response.json b/graphql/e2e/normal/schema_response.json index 310dcbe6279..3fa62dc4c47 100644 --- a/graphql/e2e/normal/schema_response.json +++ b/graphql/e2e/normal/schema_response.json @@ -847,7 +847,8 @@ "tokenizer": [ "exact" ], - "upsert": true + "upsert": true, + "unique":true }, { "predicate": "dgraph.type", diff --git a/schema/schema.go b/schema/schema.go index e04d67649aa..9928d2696d3 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -709,6 +709,7 @@ func initialSchemaInternal(namespace uint64, all bool) []*pb.SchemaUpdate { Directive: pb.SchemaUpdate_INDEX, Tokenizer: []string{"exact"}, Upsert: true, + Unique: true, }, &pb.SchemaUpdate{ Predicate: "dgraph.graphql.p_query", ValueType: pb.Posting_STRING, @@ -724,6 +725,7 @@ func initialSchemaInternal(namespace uint64, all bool) []*pb.SchemaUpdate { ValueType: pb.Posting_STRING, Directive: pb.SchemaUpdate_INDEX, Upsert: true, + Unique: true, Tokenizer: []string{"exact"}, }, { diff --git a/systest/export/export_test.go b/systest/export/export_test.go index 662735018da..7c46d3c8253 100644 --- a/systest/export/export_test.go +++ b/systest/export/export_test.go @@ -82,7 +82,7 @@ func TestExportSchemaToMinio(t *testing.T) { var expectedSchema = `[0x0] :string .` + " " + ` [0x0] :[string] @index(exact) .` + " " + ` [0x0] :string .` + " " + ` -[0x0] :string @index(exact) @upsert .` + " " + ` +[0x0] :string @index(exact) @upsert @unique .` + " " + ` [0x0] :string .` + " " + ` [0x0] :string @index(sha256) .` + " " + ` [0x0] type { diff --git a/systest/integration2/acl_test.go b/systest/integration2/acl_test.go new file mode 100644 index 00000000000..c6a9660e2cf --- /dev/null +++ b/systest/integration2/acl_test.go @@ -0,0 +1,52 @@ +//go:build integration2 + +/* + * Copyright 2023 Dgraph Labs, Inc. and Contributors * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package main + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" + + "github.com/dgraph-io/dgo/v230/protos/api" + "github.com/dgraph-io/dgraph/dgraphtest" + "github.com/dgraph-io/dgraph/x" +) + +func TestACLDuplicateGrootUser(t *testing.T) { + conf := dgraphtest.NewClusterConfig().WithNumAlphas(1).WithNumZeros(1).WithACL(time.Hour) + c, err := dgraphtest.NewLocalCluster(conf) + require.NoError(t, err) + defer func() { c.Cleanup(t.Failed()) }() + require.NoError(t, c.Start()) + + gc, cleanup, err := c.Client() + require.NoError(t, err) + defer cleanup() + require.NoError(t, gc.LoginIntoNamespace(context.Background(), + dgraphtest.DefaultUser, dgraphtest.DefaultPassword, x.GalaxyNamespace)) + + rdfs := `_:a "groot" . + _:a "dgraph.type.User" .` + + mu := &api.Mutation{SetNquads: []byte(rdfs), CommitNow: true} + _, err = gc.Mutate(mu) + require.Error(t, err) + require.ErrorContains(t, err, "could not insert duplicate value [groot] for predicate [dgraph.xid]") +} diff --git a/testutil/schema.go b/testutil/schema.go index 02ef3c5ae8b..f811d22f3e7 100644 --- a/testutil/schema.go +++ b/testutil/schema.go @@ -32,7 +32,7 @@ import ( const ( aclPreds = ` -{"predicate":"dgraph.xid","type":"string", "index":true, "tokenizer":["exact"], "upsert":true}, +{"predicate":"dgraph.xid","type":"string", "index":true, "tokenizer":["exact"], "unique": true, "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}, @@ -44,7 +44,7 @@ const ( {"predicate":"dgraph.drop.op", "type": "string"}, {"predicate":"dgraph.graphql.p_query","type":"string","index":true,"tokenizer":["sha256"]}, {"predicate":"dgraph.graphql.schema", "type": "string"}, -{"predicate":"dgraph.graphql.xid","type":"string","index":true,"tokenizer":["exact"],"upsert":true} +{"predicate":"dgraph.graphql.xid","type":"string","index":true,"tokenizer":["exact"], "unique": true,"upsert":true} ` aclTypes = ` { diff --git a/worker/export.go b/worker/export.go index 21cc7a3bc4c..e4d0fbb94fa 100644 --- a/worker/export.go +++ b/worker/export.go @@ -323,6 +323,9 @@ func toSchema(attr string, update *pb.SchemaUpdate) *bpb.KV { if update.GetUpsert() { x.Check2(buf.WriteString(" @upsert")) } + if update.GetUnique() { + x.Check2(buf.WriteString(" @unique")) + } x.Check2(buf.WriteString(" . \n")) //TODO(Naman): We don't need the version anymore. return &bpb.KV{