From 85c246f7a2163fd8bc595613119847af341ee0af Mon Sep 17 00:00:00 2001 From: Arijit Das Date: Fri, 4 Sep 2020 13:37:00 +0530 Subject: [PATCH 1/6] Hide info when performing mutation on id field with auth rule. --- graphql/e2e/auth/add_mutation_test.go | 44 ++++++------ graphql/e2e/auth/auth_test.go | 77 +++++++++++++-------- graphql/e2e/auth/debug_off/debugoff_test.go | 36 ++++++++++ graphql/e2e/auth/delete_mutation_test.go | 4 +- graphql/e2e/common/common.go | 32 +++++++++ graphql/e2e/debug/docker-compose.yml | 69 ------------------ graphql/resolve/mutation_rewriter.go | 10 ++- graphql/resolve/query_rewriter.go | 14 ++++ 8 files changed, 162 insertions(+), 124 deletions(-) delete mode 100644 graphql/e2e/debug/docker-compose.yml diff --git a/graphql/e2e/auth/add_mutation_test.go b/graphql/e2e/auth/add_mutation_test.go index 7fab72ffb11..11cb96e89af 100644 --- a/graphql/e2e/auth/add_mutation_test.go +++ b/graphql/e2e/auth/add_mutation_test.go @@ -130,7 +130,7 @@ func TestAddDeepFilter(t *testing.T) { Name: "project_add_2", Roles: []*Role{{ Permission: "ADMIN", - AssignedTo: []*User{{ + AssignedTo: []*common.User{{ Username: "user2", }}, }}, @@ -146,12 +146,12 @@ func TestAddDeepFilter(t *testing.T) { Name: "project_add_4", Roles: []*Role{{ Permission: "ADMIN", - AssignedTo: []*User{{ + AssignedTo: []*common.User{{ Username: "user6", }}, }, { Permission: "VIEW", - AssignedTo: []*User{{ + AssignedTo: []*common.User{{ Username: "user6", }}, }}, @@ -197,7 +197,7 @@ func TestAddDeepFilter(t *testing.T) { err := json.Unmarshal([]byte(tcase.result), &expected) require.NoError(t, err) - err = json.Unmarshal([]byte(gqlResponse.Data), &result) + err = json.Unmarshal(gqlResponse.Data, &result) require.NoError(t, err) opt := cmpopts.IgnoreFields(Column{}, "ColID") @@ -234,7 +234,7 @@ func TestAddOrRBACFilter(t *testing.T) { Name: "project_add_2", Roles: []*Role{{ Permission: "ADMIN", - AssignedTo: []*User{{ + AssignedTo: []*common.User{{ Username: "user2", }}, }}, @@ -247,12 +247,12 @@ func TestAddOrRBACFilter(t *testing.T) { Name: "project_add_3", Roles: []*Role{{ Permission: "ADMIN", - AssignedTo: []*User{{ + AssignedTo: []*common.User{{ Username: "user7", }}, }, { Permission: "VIEW", - AssignedTo: []*User{{ + AssignedTo: []*common.User{{ Username: "user7", }}, }}, @@ -294,7 +294,7 @@ func TestAddOrRBACFilter(t *testing.T) { err := json.Unmarshal([]byte(tcase.result), &expected) require.NoError(t, err) - err = json.Unmarshal([]byte(gqlResponse.Data), &result) + err = json.Unmarshal(gqlResponse.Data, &result) require.NoError(t, err) opt := cmpopts.IgnoreFields(Project{}, "ProjID") @@ -315,13 +315,13 @@ func TestAddAndRBACFilterMultiple(t *testing.T) { result: `{"addIssue": {"issue":[{"msg":"issue_add_5"}, {"msg":"issue_add_6"}, {"msg":"issue_add_7"}]}}`, variables: map[string]interface{}{"issues": []*Issue{{ Msg: "issue_add_5", - Owner: &User{Username: "user8"}, + Owner: &common.User{Username: "user8"}, }, { Msg: "issue_add_6", - Owner: &User{Username: "user8"}, + Owner: &common.User{Username: "user8"}, }, { Msg: "issue_add_7", - Owner: &User{Username: "user8"}, + Owner: &common.User{Username: "user8"}, }}}, }, { user: "user8", @@ -329,13 +329,13 @@ func TestAddAndRBACFilterMultiple(t *testing.T) { result: ``, variables: map[string]interface{}{"issues": []*Issue{{ Msg: "issue_add_8", - Owner: &User{Username: "user8"}, + Owner: &common.User{Username: "user8"}, }, { Msg: "issue_add_9", - Owner: &User{Username: "user8"}, + Owner: &common.User{Username: "user8"}, }, { Msg: "issue_add_10", - Owner: &User{Username: "user9"}, + Owner: &common.User{Username: "user9"}, }}}, }} @@ -373,7 +373,7 @@ func TestAddAndRBACFilterMultiple(t *testing.T) { err := json.Unmarshal([]byte(tcase.result), &expected) require.NoError(t, err) - err = json.Unmarshal([]byte(gqlResponse.Data), &result) + err = json.Unmarshal(gqlResponse.Data, &result) require.NoError(t, err) opt := cmpopts.IgnoreFields(Issue{}, "Id") @@ -394,7 +394,7 @@ func TestAddAndRBACFilter(t *testing.T) { result: `{"addIssue": {"issue":[{"msg":"issue_add_1"}]}}`, variables: map[string]interface{}{"issue": &Issue{ Msg: "issue_add_1", - Owner: &User{Username: "user7"}, + Owner: &common.User{Username: "user7"}, }}, }, { user: "user7", @@ -402,7 +402,7 @@ func TestAddAndRBACFilter(t *testing.T) { result: ``, variables: map[string]interface{}{"issue": &Issue{ Msg: "issue_add_2", - Owner: &User{Username: "user8"}, + Owner: &common.User{Username: "user8"}, }}, }, { user: "user7", @@ -410,7 +410,7 @@ func TestAddAndRBACFilter(t *testing.T) { result: ``, variables: map[string]interface{}{"issue": &Issue{ Msg: "issue_add_3", - Owner: &User{Username: "user7"}, + Owner: &common.User{Username: "user7"}, }}, }} @@ -448,7 +448,7 @@ func TestAddAndRBACFilter(t *testing.T) { err := json.Unmarshal([]byte(tcase.result), &expected) require.NoError(t, err) - err = json.Unmarshal([]byte(gqlResponse.Data), &result) + err = json.Unmarshal(gqlResponse.Data, &result) require.NoError(t, err) opt := cmpopts.IgnoreFields(Issue{}, "Id") @@ -510,7 +510,7 @@ func TestAddComplexFilter(t *testing.T) { RegionsAvailable: []*Region{{ Name: "add_region_2", Global: false, - Users: []*User{{ + Users: []*common.User{{ Username: "user8", }}, }}, @@ -552,7 +552,7 @@ func TestAddComplexFilter(t *testing.T) { err := json.Unmarshal([]byte(tcase.result), &expected) require.NoError(t, err) - err = json.Unmarshal([]byte(gqlResponse.Data), &result) + err = json.Unmarshal(gqlResponse.Data, &result) require.NoError(t, err) opt := cmpopts.IgnoreFields(Movie{}, "Id") @@ -618,7 +618,7 @@ func TestAddRBACFilter(t *testing.T) { err := json.Unmarshal([]byte(tcase.result), &expected) require.NoError(t, err) - err = json.Unmarshal([]byte(gqlResponse.Data), &result) + err = json.Unmarshal(gqlResponse.Data, &result) require.NoError(t, err) opt := cmpopts.IgnoreFields(Log{}, "Id") diff --git a/graphql/e2e/auth/auth_test.go b/graphql/e2e/auth/auth_test.go index 57419ebd797..10097d6f07d 100644 --- a/graphql/e2e/auth/auth_test.go +++ b/graphql/e2e/auth/auth_test.go @@ -41,25 +41,11 @@ var ( metaInfo *testutil.AuthMeta ) -type Tweets struct { - Id string `json:"id,omitempty"` - Text string `json:"text,omitempty"` - Timestamp string `json:"timestamp,omitempty"` - User User `json:"user,omitempty"` -} - -type User struct { - Username string `json:"username,omitempty"` - Age uint64 `json:"age,omitempty"` - IsPublic bool `json:"isPublic,omitempty"` - Disabled bool `json:"disabled,omitempty"` -} - type Region struct { - Id string `json:"id,omitempty"` - Name string `json:"name,omitempty"` - Users []*User `json:"users,omitempty"` - Global bool `json:"global,omitempty"` + Id string `json:"id,omitempty"` + Name string `json:"name,omitempty"` + Users []*common.User `json:"users,omitempty"` + Global bool `json:"global,omitempty"` } type Movie struct { @@ -70,9 +56,9 @@ type Movie struct { } type Issue struct { - Id string `json:"id,omitempty"` - Msg string `json:"msg,omitempty"` - Owner *User `json:"owner,omitempty"` + Id string `json:"id,omitempty"` + Msg string `json:"msg,omitempty"` + Owner *common.User `json:"owner,omitempty"` } type Log struct { @@ -88,16 +74,16 @@ type ComplexLog struct { } type Role struct { - Id string `json:"id,omitempty"` - Permission string `json:"permission,omitempty"` - AssignedTo []*User `json:"assignedTo,omitempty"` + Id string `json:"id,omitempty"` + Permission string `json:"permission,omitempty"` + AssignedTo []*common.User `json:"assignedTo,omitempty"` } type Ticket struct { - Id string `json:"id,omitempty"` - OnColumn *Column `json:"onColumn,omitempty"` - Title string `json:"title,omitempty"` - AssignedTo []*User `json:"assignedTo,omitempty"` + Id string `json:"id,omitempty"` + OnColumn *Column `json:"onColumn,omitempty"` + Title string `json:"title,omitempty"` + AssignedTo []*common.User `json:"assignedTo,omitempty"` } type Column struct { @@ -280,6 +266,41 @@ func (s Student) add(t *testing.T) { require.JSONEq(t, result, string(gqlResponse.Data)) } +func TestAddMutationWithXid(t *testing.T) { + mutation := ` + mutation addTweets($tweet: AddTweetsInput!){ + addTweets(input: [$tweet]) { + numUids + } + } + ` + + tweet := common.Tweets{ + Id: "tweet1", + Text: "abc", + Timestamp: "2020-10-10", + } + user := "foo" + addTweetsParams := &common.GraphQLParams{ + Headers: common.GetJWT(t, user, "", metaInfo), + Query: mutation, + Variables: map[string]interface{}{"tweet": tweet}, + } + + // Add the tweet for the first time. + gqlResponse := addTweetsParams.ExecuteAsPost(t, common.GraphqlURL) + require.Nil(t, gqlResponse.Errors) + + // Re-adding the tweet should fail. + gqlResponse = addTweetsParams.ExecuteAsPost(t, common.GraphqlURL) + require.Error(t, gqlResponse.Errors) + require.Equal(t, len(gqlResponse.Errors), 1) + require.Contains(t, gqlResponse.Errors[0].Error(), fmt.Sprintf("id already exists for type Tweets")) + + // Clear the tweet. + tweet.DeleteByID(t, user, metaInfo) +} + func TestAuthWithDgraphDirective(t *testing.T) { students := []Student{ { diff --git a/graphql/e2e/auth/debug_off/debugoff_test.go b/graphql/e2e/auth/debug_off/debugoff_test.go index 83a8f295c8b..a19806d9b94 100644 --- a/graphql/e2e/auth/debug_off/debugoff_test.go +++ b/graphql/e2e/auth/debug_off/debugoff_test.go @@ -2,6 +2,7 @@ package debugoff import ( "encoding/json" + "fmt" "io/ioutil" "os" "testing" @@ -89,6 +90,41 @@ func TestAddGQL(t *testing.T) { } } +func TestAddMutationWithXid(t *testing.T) { + mutation := ` + mutation addTweets($tweet: AddTweetsInput!){ + addTweets(input: [$tweet]) { + numUids + } + } + ` + + tweet := common.Tweets{ + Id: "tweet1", + Text: "abc", + Timestamp: "2020-10-10", + } + user := "foo" + addTweetsParams := &common.GraphQLParams{ + Headers: common.GetJWT(t, user, "", metaInfo), + Query: mutation, + Variables: map[string]interface{}{"tweet": tweet}, + } + + // Add the tweet for the first time. + gqlResponse := addTweetsParams.ExecuteAsPost(t, common.GraphqlURL) + require.Nil(t, gqlResponse.Errors) + + // Re-adding the tweet should fail. + gqlResponse = addTweetsParams.ExecuteAsPost(t, common.GraphqlURL) + require.Error(t, gqlResponse.Errors) + require.Equal(t, len(gqlResponse.Errors), 1) + require.Contains(t, gqlResponse.Errors[0].Error(), fmt.Sprintf("id %s already exists for type Tweets", tweet.Id)) + + // Clear the tweet. + tweet.DeleteByID(t, user, metaInfo) +} + func TestMain(m *testing.M) { schemaFile := "../schema.graphql" schema, err := ioutil.ReadFile(schemaFile) diff --git a/graphql/e2e/auth/delete_mutation_test.go b/graphql/e2e/auth/delete_mutation_test.go index 46f405455dd..084dcc0b499 100644 --- a/graphql/e2e/auth/delete_mutation_test.go +++ b/graphql/e2e/auth/delete_mutation_test.go @@ -374,11 +374,11 @@ func TestDeleteRBACRuleInverseField(t *testing.T) { addTweetsParams := &common.GraphQLParams{ Headers: common.GetJWT(t, "foo", "", metaInfo), Query: mutation, - Variables: map[string]interface{}{"tweet": Tweets{ + Variables: map[string]interface{}{"tweet": common.Tweets{ Id: "tweet1", Text: "abc", Timestamp: "2020-10-10", - User: User{ + User: &common.User{ Username: "foo", }, }}, diff --git a/graphql/e2e/common/common.go b/graphql/e2e/common/common.go index 8b4285e4bf8..857217fef24 100644 --- a/graphql/e2e/common/common.go +++ b/graphql/e2e/common/common.go @@ -100,6 +100,20 @@ type GraphQLResponse struct { Extensions map[string]interface{} `json:"extensions,omitempty"` } +type Tweets struct { + Id string `json:"id,omitempty"` + Text string `json:"text,omitempty"` + Timestamp string `json:"timestamp,omitempty"` + User *User `json:"user,omitempty"` +} + +type User struct { + Username string `json:"username,omitempty"` + Age uint64 `json:"age,omitempty"` + IsPublic bool `json:"isPublic,omitempty"` + Disabled bool `json:"disabled,omitempty"` +} + type country struct { ID string `json:"id,omitempty"` Name string `json:"name,omitempty"` @@ -179,6 +193,24 @@ type UserSecret struct { OwnedBy string `json:"ownedBy,omitempty"` } +func (twt *Tweets) DeleteByID(t *testing.T, user string, metaInfo *testutil.AuthMeta) { + getParams := &GraphQLParams{ + Headers: GetJWT(t, user, "", metaInfo), + Query: ` + mutation delTweets ($filter : TweetsFilter!){ + deleteTweets (filter: $filter) { + numUids + } + } + `, + Variables: map[string]interface{}{"filter": map[string]interface{}{ + "id": map[string]interface{}{"eq": twt.Id}, + }}, + } + gqlResponse := getParams.ExecuteAsPost(t, GraphqlURL) + require.Nil(t, gqlResponse.Errors) +} + func (us *UserSecret) Delete(t *testing.T, user, role string, metaInfo *testutil.AuthMeta) { getParams := &GraphQLParams{ Headers: GetJWT(t, user, role, metaInfo), diff --git a/graphql/e2e/debug/docker-compose.yml b/graphql/e2e/debug/docker-compose.yml deleted file mode 100644 index 04925abaaea..00000000000 --- a/graphql/e2e/debug/docker-compose.yml +++ /dev/null @@ -1,69 +0,0 @@ -version: "3.5" -services: - zero: - image: dgraph/dgraph:latest - container_name: zero1 - working_dir: /data/zero1 - ports: - - 5180:5180 - - 6180:6180 - labels: - cluster: test - service: zero1 - volumes: - - type: bind - source: $GOPATH/bin - target: /gobin - read_only: true - command: /gobin/dgraph zero -o 100 --logtostderr -v=2 --bindall --expose_trace --profile_mode block --block_rate 10 --my=zero1:5180 - - alpha: - image: dgraph/dgraph:latest - container_name: alpha1 - working_dir: /data/alpha1 - volumes: - - type: bind - source: $GOPATH/bin - target: /gobin - read_only: true - ports: - - 8180:8180 - - 9180:9180 - labels: - cluster: test - service: alpha1 - command: /gobin/dgraph alpha --lru_mb=1024 --zero=zero1:5180 -o 100 --expose_trace --trace 1.0 --profile_mode block --block_rate 10 --logtostderr -v=3 --whitelist 10.0.0.0/8,172.16.0.0/12,192.168.0.0/16 --my=alpha1:7180 --graphql_debug=true - - zeroAdmin: - image: dgraph/dgraph:latest - container_name: zeroAdmin - working_dir: /data/zeroAdmin - ports: - - 5280:5280 - - 6280:6280 - labels: - cluster: admintest - service: zeroAdmin - volumes: - - type: bind - source: $GOPATH/bin - target: /gobin - read_only: true - command: /gobin/dgraph zero -o 200 --logtostderr -v=2 --bindall --expose_trace --profile_mode block --block_rate 10 --my=zeroAdmin:5280 - - alphaAdmin: - image: dgraph/dgraph:latest - container_name: alphaAdmin - working_dir: /data/alphaAdmin - volumes: - - type: bind - source: $GOPATH/bin - target: /gobin - read_only: true - ports: - - 8280:8280 - - 9280:9280 - labels: - cluster: admintest - service: alphaAdmin - command: /gobin/dgraph alpha --lru_mb=1024 --zero=zeroAdmin:5280 -o 200 --expose_trace --trace 1.0 --profile_mode block --block_rate 10 --logtostderr -v=2 --whitelist 10.0.0.0/8,172.16.0.0/12,192.168.0.0/16 --my=alphaAdmin:7280 --graphql_debug=true diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index c5c34153a68..9fd0b47c7d3 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -1095,9 +1095,13 @@ func rewriteObject( xidMetadata.queryExists[variable] = true } frag.conditions = []string{fmt.Sprintf("eq(len(%s), 0)", variable)} - frag.check = checkQueryResult(variable, - x.GqlErrorf("id %s already exists for type %s", xidString, typ.Name()), - nil) + var err error + if x.Config.GraphqlDebug || addAuthSelector(typ) == nil { + err = x.GqlErrorf("id %s already exists for type %s", xidString, typ.Name()) + } else { + err = x.GqlErrorf("id already exists for type %s", typ.Name()) + } + frag.check = checkQueryResult(variable, err, nil) } if xid != nil && !atTopLevel { diff --git a/graphql/resolve/query_rewriter.go b/graphql/resolve/query_rewriter.go index eb62904ef8d..1b63299a72a 100644 --- a/graphql/resolve/query_rewriter.go +++ b/graphql/resolve/query_rewriter.go @@ -67,6 +67,20 @@ func hasAuthRules(field schema.Field, authRw *authRewriter) bool { return false } +func hasAuthRuless(field schema.Field, selector func(t schema.Type) *schema.RuleNode) bool { + rn := selector(field.Type()) + if rn != nil { + return true + } + + for _, childField := range field.SelectionSet() { + if authRules := hasAuthRuless(childField, selector); authRules { + return true + } + } + return false +} + // Rewrite rewrites a GraphQL query into a Dgraph GraphQuery. func (qr *queryRewriter) Rewrite( ctx context.Context, From 0cb5080eae87505cccf201944fc340c91a4197e6 Mon Sep 17 00:00:00 2001 From: Arijit Das Date: Fri, 4 Sep 2020 17:18:23 +0530 Subject: [PATCH 2/6] Fix E2E test. --- graphql/e2e/auth/auth_test.go | 2 +- graphql/e2e/auth/debug_off/debugoff_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/graphql/e2e/auth/auth_test.go b/graphql/e2e/auth/auth_test.go index 10097d6f07d..9a55fecd223 100644 --- a/graphql/e2e/auth/auth_test.go +++ b/graphql/e2e/auth/auth_test.go @@ -295,7 +295,7 @@ func TestAddMutationWithXid(t *testing.T) { gqlResponse = addTweetsParams.ExecuteAsPost(t, common.GraphqlURL) require.Error(t, gqlResponse.Errors) require.Equal(t, len(gqlResponse.Errors), 1) - require.Contains(t, gqlResponse.Errors[0].Error(), fmt.Sprintf("id already exists for type Tweets")) + require.Contains(t, gqlResponse.Errors[0].Error(), fmt.Sprintf("id %s already exists for type Tweets", tweet.Id)) // Clear the tweet. tweet.DeleteByID(t, user, metaInfo) diff --git a/graphql/e2e/auth/debug_off/debugoff_test.go b/graphql/e2e/auth/debug_off/debugoff_test.go index a19806d9b94..132edb9bab6 100644 --- a/graphql/e2e/auth/debug_off/debugoff_test.go +++ b/graphql/e2e/auth/debug_off/debugoff_test.go @@ -119,7 +119,7 @@ func TestAddMutationWithXid(t *testing.T) { gqlResponse = addTweetsParams.ExecuteAsPost(t, common.GraphqlURL) require.Error(t, gqlResponse.Errors) require.Equal(t, len(gqlResponse.Errors), 1) - require.Contains(t, gqlResponse.Errors[0].Error(), fmt.Sprintf("id %s already exists for type Tweets", tweet.Id)) + require.Contains(t, gqlResponse.Errors[0].Error(), fmt.Sprintf("id already exists for type Tweets")) // Clear the tweet. tweet.DeleteByID(t, user, metaInfo) From a4798de7f1a9a53d029da97a62f005f927de4bb7 Mon Sep 17 00:00:00 2001 From: Arijit Das Date: Fri, 4 Sep 2020 17:28:51 +0530 Subject: [PATCH 3/6] Clear deadcode. --- graphql/e2e/auth/auth_test.go | 3 ++- graphql/e2e/auth/debug_off/debugoff_test.go | 3 +-- graphql/resolve/query_rewriter.go | 14 -------------- 3 files changed, 3 insertions(+), 17 deletions(-) diff --git a/graphql/e2e/auth/auth_test.go b/graphql/e2e/auth/auth_test.go index 9a55fecd223..9f5ddaf4a35 100644 --- a/graphql/e2e/auth/auth_test.go +++ b/graphql/e2e/auth/auth_test.go @@ -295,7 +295,8 @@ func TestAddMutationWithXid(t *testing.T) { gqlResponse = addTweetsParams.ExecuteAsPost(t, common.GraphqlURL) require.Error(t, gqlResponse.Errors) require.Equal(t, len(gqlResponse.Errors), 1) - require.Contains(t, gqlResponse.Errors[0].Error(), fmt.Sprintf("id %s already exists for type Tweets", tweet.Id)) + require.Contains(t, gqlResponse.Errors[0].Error(), + fmt.Sprintf("id %s already exists for type Tweets", tweet.Id)) // Clear the tweet. tweet.DeleteByID(t, user, metaInfo) diff --git a/graphql/e2e/auth/debug_off/debugoff_test.go b/graphql/e2e/auth/debug_off/debugoff_test.go index 132edb9bab6..ede6e36745d 100644 --- a/graphql/e2e/auth/debug_off/debugoff_test.go +++ b/graphql/e2e/auth/debug_off/debugoff_test.go @@ -2,7 +2,6 @@ package debugoff import ( "encoding/json" - "fmt" "io/ioutil" "os" "testing" @@ -119,7 +118,7 @@ func TestAddMutationWithXid(t *testing.T) { gqlResponse = addTweetsParams.ExecuteAsPost(t, common.GraphqlURL) require.Error(t, gqlResponse.Errors) require.Equal(t, len(gqlResponse.Errors), 1) - require.Contains(t, gqlResponse.Errors[0].Error(), fmt.Sprintf("id already exists for type Tweets")) + require.Contains(t, gqlResponse.Errors[0].Error(), "id already exists for type Tweets") // Clear the tweet. tweet.DeleteByID(t, user, metaInfo) diff --git a/graphql/resolve/query_rewriter.go b/graphql/resolve/query_rewriter.go index 1b63299a72a..eb62904ef8d 100644 --- a/graphql/resolve/query_rewriter.go +++ b/graphql/resolve/query_rewriter.go @@ -67,20 +67,6 @@ func hasAuthRules(field schema.Field, authRw *authRewriter) bool { return false } -func hasAuthRuless(field schema.Field, selector func(t schema.Type) *schema.RuleNode) bool { - rn := selector(field.Type()) - if rn != nil { - return true - } - - for _, childField := range field.SelectionSet() { - if authRules := hasAuthRuless(childField, selector); authRules { - return true - } - } - return false -} - // Rewrite rewrites a GraphQL query into a Dgraph GraphQuery. func (qr *queryRewriter) Rewrite( ctx context.Context, From 23631a7aea69f92f53dbc255b0332a3af048b803 Mon Sep 17 00:00:00 2001 From: Arijit Das Date: Mon, 7 Sep 2020 21:45:49 +0530 Subject: [PATCH 4/6] Address comments. --- graphql/e2e/auth/auth_test.go | 2 +- graphql/e2e/auth/debug_off/debugoff_test.go | 4 +--- graphql/e2e/auth/schema.graphql | 2 +- graphql/resolve/mutation_rewriter.go | 8 ++++++-- graphql/schema/response.go | 4 ++++ 5 files changed, 13 insertions(+), 7 deletions(-) diff --git a/graphql/e2e/auth/auth_test.go b/graphql/e2e/auth/auth_test.go index 9f5ddaf4a35..977f788c108 100644 --- a/graphql/e2e/auth/auth_test.go +++ b/graphql/e2e/auth/auth_test.go @@ -296,7 +296,7 @@ func TestAddMutationWithXid(t *testing.T) { require.Error(t, gqlResponse.Errors) require.Equal(t, len(gqlResponse.Errors), 1) require.Contains(t, gqlResponse.Errors[0].Error(), - fmt.Sprintf("id %s already exists for type Tweets", tweet.Id)) + "GraphQL debug: id already exists for type Tweets") // Clear the tweet. tweet.DeleteByID(t, user, metaInfo) diff --git a/graphql/e2e/auth/debug_off/debugoff_test.go b/graphql/e2e/auth/debug_off/debugoff_test.go index ede6e36745d..3f6a3df5012 100644 --- a/graphql/e2e/auth/debug_off/debugoff_test.go +++ b/graphql/e2e/auth/debug_off/debugoff_test.go @@ -116,9 +116,7 @@ func TestAddMutationWithXid(t *testing.T) { // Re-adding the tweet should fail. gqlResponse = addTweetsParams.ExecuteAsPost(t, common.GraphqlURL) - require.Error(t, gqlResponse.Errors) - require.Equal(t, len(gqlResponse.Errors), 1) - require.Contains(t, gqlResponse.Errors[0].Error(), "id already exists for type Tweets") + require.Nil(t, gqlResponse.Errors) // Clear the tweet. tweet.DeleteByID(t, user, metaInfo) diff --git a/graphql/e2e/auth/schema.graphql b/graphql/e2e/auth/schema.graphql index 6588f07dda9..7eb7c85b327 100644 --- a/graphql/e2e/auth/schema.graphql +++ b/graphql/e2e/auth/schema.graphql @@ -27,7 +27,7 @@ type User @auth( } type Tweets @auth ( - add: { rule: "{$USER: { eq: \"foo\" } }"}, + query: { rule: "{$USER: { eq: \"foo\" } }"}, delete: { rule: "{$USER: { eq: \"foo\" } }"}, update: { rule: "{$USER: { eq: \"foo\" } }"} ){ diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index 9fd0b47c7d3..e91edf99262 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -1095,11 +1095,15 @@ func rewriteObject( xidMetadata.queryExists[variable] = true } frag.conditions = []string{fmt.Sprintf("eq(len(%s), 0)", variable)} + + // We need to conceal the error because we might be leaking information to the user if it + // tries to add duplicate data to the field with @id. var err error - if x.Config.GraphqlDebug || addAuthSelector(typ) == nil { + if queryAuthSelector(typ) == nil { err = x.GqlErrorf("id %s already exists for type %s", xidString, typ.Name()) } else { - err = x.GqlErrorf("id already exists for type %s", typ.Name()) + // This error will only be reported in debug mode. + err = x.GqlErrorf("GraphQL debug: id already exists for type %s", typ.Name()) } frag.check = checkQueryResult(variable, err, nil) } diff --git a/graphql/schema/response.go b/graphql/schema/response.go index ac2e722f8dd..6cf03c4b0d0 100644 --- a/graphql/schema/response.go +++ b/graphql/schema/response.go @@ -76,6 +76,10 @@ func (r *Response) WithError(err error) { return } + if !x.Config.GraphqlDebug && strings.Contains(err.Error(), "GraphQL debug:") { + return + } + r.Errors = append(r.Errors, AsGQLErrors(err)...) } From 8849b50a3ba6eef186d01e8608f2886cf641b480 Mon Sep 17 00:00:00 2001 From: Arijit Das Date: Wed, 9 Sep 2020 21:52:29 +0530 Subject: [PATCH 5/6] Fix failing test. --- graphql/e2e/auth/schema.graphql | 3 ++- graphql/resolve/auth_delete_test.yaml | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/graphql/e2e/auth/schema.graphql b/graphql/e2e/auth/schema.graphql index 7eb7c85b327..1bbfac7b262 100644 --- a/graphql/e2e/auth/schema.graphql +++ b/graphql/e2e/auth/schema.graphql @@ -27,7 +27,8 @@ type User @auth( } type Tweets @auth ( - query: { rule: "{$USER: { eq: \"foo\" } }"}, + query: { rule: "{$ROLE: { eq: \"admin\" } }"}, + add: { rule: "{$USER: { eq: \"foo\" } }"}, delete: { rule: "{$USER: { eq: \"foo\" } }"}, update: { rule: "{$USER: { eq: \"foo\" } }"} ){ diff --git a/graphql/resolve/auth_delete_test.yaml b/graphql/resolve/auth_delete_test.yaml index fc39fef4206..1f202cff726 100644 --- a/graphql/resolve/auth_delete_test.yaml +++ b/graphql/resolve/auth_delete_test.yaml @@ -38,6 +38,7 @@ } jwtvar: USER: "foo" + ROLE: "admin" dgmutations: - deletejson: | [ @@ -75,6 +76,8 @@ } } } + jwtvar: + ROLE: "admin" dgmutations: - deletejson: | [ From 42a835db2e5f2dbb0049866ea4fe7ec56ebed661 Mon Sep 17 00:00:00 2001 From: Arijit Das Date: Mon, 14 Sep 2020 12:01:23 +0530 Subject: [PATCH 6/6] Fix E2E test. --- graphql/e2e/auth/delete_mutation_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/graphql/e2e/auth/delete_mutation_test.go b/graphql/e2e/auth/delete_mutation_test.go index 084dcc0b499..ed5af401305 100644 --- a/graphql/e2e/auth/delete_mutation_test.go +++ b/graphql/e2e/auth/delete_mutation_test.go @@ -390,10 +390,12 @@ func TestDeleteRBACRuleInverseField(t *testing.T) { testCases := []TestCase{ { user: "foobar", + role: "admin", result: `{"deleteTweets":{"numUids":0,"tweets":[]}}`, }, { user: "foo", + role: "admin", result: `{"deleteTweets":{"numUids":1,"tweets":[ {"text": "abc"}]}}`, }, }