From 5a3266d5aff457f64c8c7ab4c45f1faf89b37319 Mon Sep 17 00:00:00 2001 From: Arijit Das Date: Mon, 14 Sep 2020 14:58:54 +0530 Subject: [PATCH 1/2] fix(GraphQL): Hide info when performing mutation on id field with auth rule. (#6391) * Hide info when performing mutation on id field with auth rule. (cherry picked from commit 5c33428fc9adc973590c7f5eb1f8ccd55eeb26f9) --- graphql/e2e/auth/add_mutation_test.go | 44 +++++----- graphql/e2e/auth/auth_test.go | 84 ++++++++++++-------- graphql/e2e/auth/debug_off/debug_off_test.go | 33 ++++++++ graphql/e2e/auth/delete_mutation_test.go | 6 +- graphql/e2e/auth/schema.graphql | 1 + graphql/e2e/common/common.go | 32 ++++++++ graphql/resolve/auth_delete_test.yaml | 3 + graphql/resolve/mutation_rewriter.go | 14 +++- graphql/schema/response.go | 5 ++ 9 files changed, 161 insertions(+), 61 deletions(-) diff --git a/graphql/e2e/auth/add_mutation_test.go b/graphql/e2e/auth/add_mutation_test.go index b89ac9d6cde..74171dd58a4 100644 --- a/graphql/e2e/auth/add_mutation_test.go +++ b/graphql/e2e/auth/add_mutation_test.go @@ -146,7 +146,7 @@ func TestAddDeepFilter(t *testing.T) { Name: "project_add_2", Roles: []*Role{{ Permission: "ADMIN", - AssignedTo: []*User{{ + AssignedTo: []*common.User{{ Username: "user2", }}, }}, @@ -162,12 +162,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", }}, }}, @@ -212,7 +212,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") @@ -249,7 +249,7 @@ func TestAddOrRBACFilter(t *testing.T) { Name: "project_add_2", Roles: []*Role{{ Permission: "ADMIN", - AssignedTo: []*User{{ + AssignedTo: []*common.User{{ Username: "user2", }}, }}, @@ -262,12 +262,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", }}, }}, @@ -308,7 +308,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") @@ -329,13 +329,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", @@ -343,13 +343,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"}, }}}, }} @@ -386,7 +386,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") @@ -407,7 +407,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", @@ -415,7 +415,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", @@ -423,7 +423,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"}, }}, }} @@ -460,7 +460,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") @@ -522,7 +522,7 @@ func TestAddComplexFilter(t *testing.T) { RegionsAvailable: []*Region{{ Name: "add_region_2", Global: false, - Users: []*User{{ + Users: []*common.User{{ Username: "user8", }}, }}, @@ -563,7 +563,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") @@ -628,7 +628,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 be44f66c4f7..43d40422afc 100644 --- a/graphql/e2e/auth/auth_test.go +++ b/graphql/e2e/auth/auth_test.go @@ -43,31 +43,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 UserSecret struct { - Id string `json:"id,omitempty"` - ASecret string `json:"aSecret,omitempty"` - OwnedBy string `json:"ownedBy,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 { @@ -78,9 +58,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 { @@ -96,16 +76,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 { @@ -306,6 +286,42 @@ 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(), + "GraphQL debug: 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/debug_off_test.go b/graphql/e2e/auth/debug_off/debug_off_test.go index 83a8f295c8b..3f6a3df5012 100644 --- a/graphql/e2e/auth/debug_off/debug_off_test.go +++ b/graphql/e2e/auth/debug_off/debug_off_test.go @@ -89,6 +89,39 @@ 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.Nil(t, gqlResponse.Errors) + + // 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 e458e26eec0..e48e12518f9 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: getJWT(t, "foo", ""), 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", }, }}, @@ -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"}]}}`, }, } diff --git a/graphql/e2e/auth/schema.graphql b/graphql/e2e/auth/schema.graphql index 6588f07dda9..1bbfac7b262 100644 --- a/graphql/e2e/auth/schema.graphql +++ b/graphql/e2e/auth/schema.graphql @@ -27,6 +27,7 @@ type User @auth( } type Tweets @auth ( + query: { rule: "{$ROLE: { eq: \"admin\" } }"}, add: { rule: "{$USER: { eq: \"foo\" } }"}, delete: { rule: "{$USER: { eq: \"foo\" } }"}, update: { rule: "{$USER: { eq: \"foo\" } }"} diff --git a/graphql/e2e/common/common.go b/graphql/e2e/common/common.go index 1b7b0c122db..129ae8e0cb1 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"` @@ -178,6 +192,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/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: | [ diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go index 84ca5da8bf9..d44b0b65fbd 100644 --- a/graphql/resolve/mutation_rewriter.go +++ b/graphql/resolve/mutation_rewriter.go @@ -1095,9 +1095,17 @@ 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) + + // 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 queryAuthSelector(typ) == nil { + err = x.GqlErrorf("id %s already exists for type %s", xidString, typ.Name()) + } else { + // 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) } if xid != nil && !atTopLevel { diff --git a/graphql/schema/response.go b/graphql/schema/response.go index fab663a4a45..06c969ff562 100644 --- a/graphql/schema/response.go +++ b/graphql/schema/response.go @@ -74,6 +74,11 @@ func (r *Response) WithError(err error) { if !x.Config.GraphqlDebug && strings.Contains(err.Error(), "authorization failed") { return } + + if !x.Config.GraphqlDebug && strings.Contains(err.Error(), "GraphQL debug:") { + return + } + r.Errors = append(r.Errors, AsGQLErrors(err)...) } From e283d7d1a44b3205e4797f7ed3ff0ad4d6bfbc75 Mon Sep 17 00:00:00 2001 From: Arijit Das Date: Mon, 21 Sep 2020 15:52:42 +0530 Subject: [PATCH 2/2] Fix auth E2E test. --- graphql/e2e/auth/add_mutation_test.go | 26 +++++--------------------- graphql/resolve/query_rewriter.go | 2 +- 2 files changed, 6 insertions(+), 22 deletions(-) diff --git a/graphql/e2e/auth/add_mutation_test.go b/graphql/e2e/auth/add_mutation_test.go index 74171dd58a4..dcdc94d778c 100644 --- a/graphql/e2e/auth/add_mutation_test.go +++ b/graphql/e2e/auth/add_mutation_test.go @@ -26,22 +26,6 @@ import ( "github.com/stretchr/testify/require" ) -func (us *UserSecret) delete(t *testing.T, user, role string) { - getParams := &common.GraphQLParams{ - Headers: getJWT(t, user, role), - Query: ` - mutation deleteUserSecret($ids: [ID!]) { - deleteUserSecret(filter:{id:$ids}) { - msg - } - } - `, - Variables: map[string]interface{}{"ids": []string{us.Id}}, - } - gqlResponse := getParams.ExecuteAsPost(t, graphqlURL) - require.Nil(t, gqlResponse.Errors) -} - func (p *Project) delete(t *testing.T, user, role string) { getParams := &common.GraphQLParams{ Headers: getJWT(t, user, role), @@ -646,14 +630,14 @@ func TestAddGQLOnly(t *testing.T) { testCases := []TestCase{{ user: "user1", result: `{"addUserSecret":{"usersecret":[{"aSecret":"secret1"}]}}`, - variables: map[string]interface{}{"user": &UserSecret{ + variables: map[string]interface{}{"user": &common.UserSecret{ ASecret: "secret1", OwnedBy: "user1", }}, }, { user: "user2", result: ``, - variables: map[string]interface{}{"user": &UserSecret{ + variables: map[string]interface{}{"user": &common.UserSecret{ ASecret: "secret2", OwnedBy: "user1", }}, @@ -670,7 +654,7 @@ func TestAddGQLOnly(t *testing.T) { ` var expected, result struct { AddUserSecret struct { - UserSecret []*UserSecret + UserSecret []*common.UserSecret } } @@ -694,13 +678,13 @@ func TestAddGQLOnly(t *testing.T) { err = json.Unmarshal([]byte(gqlResponse.Data), &result) require.NoError(t, err) - opt := cmpopts.IgnoreFields(UserSecret{}, "Id") + opt := cmpopts.IgnoreFields(common.UserSecret{}, "Id") if diff := cmp.Diff(expected, result, opt); diff != "" { t.Errorf("result mismatch (-want +got):\n%s", diff) } for _, i := range result.AddUserSecret.UserSecret { - i.delete(t, tcase.user, tcase.role) + i.Delete(t, tcase.user, tcase.role, metaInfo) } } } diff --git a/graphql/resolve/query_rewriter.go b/graphql/resolve/query_rewriter.go index a697f0ae658..a68f2678ad0 100644 --- a/graphql/resolve/query_rewriter.go +++ b/graphql/resolve/query_rewriter.go @@ -115,7 +115,7 @@ func (qr *queryRewriter) Rewrite( // TODO: The only error that can occur in query rewriting is if an ID argument // can't be parsed as a uid: e.g. the query was something like: - // + //UserSecret // getT(id: "HI") { ... } // // But that's not a rewriting error! It should be caught by validation