feat(GraphQL): types should be able to inherit mutation auth rules from interfaces #6816

minhaj-shakeel · 2020-10-30T10:39:55Z

Fixes GRAPHQL-740.

This change is

Docs Preview:

Fixes GRAPHQL-660. Fixes GRAPHQL-664. This PR extends support for Querying with Auth rules on Interfaces.

pawanrawal

Reviewable status: 0 of 7 files reviewed, 9 unresolved discussions (waiting on @MichaelJCompton, @minhaj-shakeel, and @pawanrawal)

graphql/resolve/auth_add_test.yaml, line 1025 at r2 (raw file):

  skipauth: true

- name: "Adding Type with having Auth on interfaces successfully."

Adding nodes for a type that inherits auth rules from an interface successfully.

graphql/resolve/auth_add_test.yaml, line 1072 at r2 (raw file):

    }
  authjson: |
    {"Question": [ {"uid": "0x123"}], "Author": [{"uid": "0x456"}]}

There is no query for the Author above in authquery so the authjson doesn't really need an Author key?

What if Author also had auth rules? Would its auth rules also be verified using a separate query then? Can we have a test for such a case?

graphql/resolve/auth_add_test.yaml, line 1073 at r2 (raw file):

  authjson: |
    {"Question": [ {"uid": "0x123"}], "Author": [{"uid": "0x456"}]}
- name: "Adding Type with having Auth on interfaces failed."

Adding nodes for a type that inherits auth rules from an interface fails.

graphql/resolve/auth_add_test.yaml, line 1124 at r2 (raw file):

    { "message": "mutation failed because authorization failed"}

- name: "Add type with Having RBAC rule on interface successfully"

with having RBAC

graphql/resolve/auth_add_test.yaml, line 1128 at r2 (raw file):

    mutation addFbPost($post: [AddFbPostInput!]!){
      addFbPost(input: $post){
        fbPost{

space before {
fbPost {

graphql/resolve/auth_add_test.yaml, line 1166 at r2 (raw file):

    }
  authjson: |
    {"FbPost": [ {"uid": "0x123"}], "Author": [{"uid": "0x456"}]}

Again, the Author is probably not required here as it would not be returned from the authquery result?

graphql/resolve/auth_delete_test.yaml, line 669 at r2 (raw file):

         }]
  dgquery: |-
    query {

I don't quite get it, where are the uids 0x1 and 0x2 used in this query? I don't see them here. It looks like we'd end up deleting all questions which pass the auth rules.

graphql/resolve/auth_delete_test.yaml, line 712 at r2 (raw file):

      x as deleteQuestion()
    }
- name: "Delete type having RBAC Auth Rules on interface and those are not satisfied."

Add a newline before starting a new test case. Makes for easier reading.

graphql/resolve/auth_delete_test.yaml, line 764 at r2 (raw file):

         }]
  dgquery: |-
    query {

Same comment as above. Why do we select all FbPost?

pawanrawal

Haven't reviewed the e2e tests yet. I also don't see any tests for update mutation with auth. We should have some of those as well.

Reviewable status: 0 of 7 files reviewed, 9 unresolved discussions (waiting on @MichaelJCompton, @minhaj-shakeel, and @pawanrawal)

minhaj-shakeel

Reviewable status: 0 of 8 files reviewed, 9 unresolved discussions (waiting on @MichaelJCompton and @pawanrawal)

graphql/resolve/auth_add_test.yaml, line 1025 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Adding nodes for a type that inherits auth rules from an interface successfully.

Done.

graphql/resolve/auth_add_test.yaml, line 1072 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

There is no query for the Author above in authquery so the authjson doesn't really need an Author key?

What if Author also had auth rules? Would its auth rules also be verified using a separate query then? Can we have a test for such a case?

I have removed the Author key from the authjson.

graphql/resolve/auth_add_test.yaml, line 1073 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Adding nodes for a type that inherits auth rules from an interface fails.

Done.

graphql/resolve/auth_add_test.yaml, line 1124 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

with having RBAC

Done.

graphql/resolve/auth_add_test.yaml, line 1128 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

space before {
fbPost {

Done.

graphql/resolve/auth_add_test.yaml, line 1166 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Again, the Author is probably not required here as it would not be returned from the authquery result?

Done.

graphql/resolve/auth_delete_test.yaml, line 669 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

I don't quite get it, where are the uids 0x1 and 0x2 used in this query? I don't see them here. It looks like we'd end up deleting all questions which pass the auth rules.

Fixed that

graphql/resolve/auth_delete_test.yaml, line 712 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add a newline before starting a new test case. Makes for easier reading.

Done.

graphql/resolve/auth_delete_test.yaml, line 764 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Same comment as above. Why do we select all FbPost?

Fixed that.

pawanrawal

All the e2e sub test cases need a description or comment field to clarify what they are testing as we have in unit tests.

Reviewed 2 of 4 files at r3.
Reviewable status: 2 of 8 files reviewed, 14 unresolved discussions (waiting on @MichaelJCompton and @minhaj-shakeel)

graphql/e2e/auth/add_mutation_test.go, line 156 at r3 (raw file):

}

func TestAddOnTypeWithRBACFilterOnInterface(t *testing.T) {

TestAuth_AddOnTypeWithRBACRuleOnInterface

graphql/e2e/auth/add_mutation_test.go, line 231 at r3 (raw file):

		gqlResponse := getUserParams.ExecuteAsPost(t, graphqlURL)
		if tcase.result == "" {

Usually, there is another variable in the test case called expectedErr which is set to true for cases where you expect the error. You are using the result value to check the same thing but having another variable makes it more clear and explicit.

graphql/e2e/auth/add_mutation_test.go, line 245 at r3 (raw file):

		require.NoError(t, err)

		opt := cmpopts.IgnoreFields(FbPost{}, "Id")

No need to even fetch the id in the response. If you didn't fetch the id you can just compare the string JSON directly without having to unmarshal it using testutil.CompareJSON

graphql/e2e/auth/add_mutation_test.go, line 260 at r3 (raw file):

}

func TestAddOnTypeWithGraphFilterOnInterface(t *testing.T) {

TestAuth_AddOnTypeWithGraphTraversalRuleOnInterface

graphql/e2e/auth/add_mutation_test.go, line 326 at r3 (raw file):

		gqlResponse := getUserParams.ExecuteAsPost(t, graphqlURL)
		if tcase.result == "" {

Same comments as above apply

graphql/e2e/auth/delete_mutation_test.go, line 208 at r3 (raw file):

func TestDeleteTypeWithGraphFilterOnInterface(t *testing.T) {
	testCases := []TestCase{{

Add a description or comment string field which talks about what each case is testing otherwise I have to go look at the schema each time to try to understand whats happening.

graphql/e2e/auth/delete_mutation_test.go, line 240 at r3 (raw file):

			_, allQuestionsIds := getAllQuestions(t, []string{"user1@dgraph.io", "user2@dgraph.io"}, []bool{true, false})
			require.True(t, len(allQuestionsIds) == 3)
			deleteQuestions, _ := getAllQuestions(t, []string{tcase.user}, []bool{tcase.ans})

Instead of running this query again, the first request above could give you the questions for all possible combinations?

graphql/e2e/auth/delete_mutation_test.go, line 242 at r3 (raw file):

			deleteQuestions, _ := getAllQuestions(t, []string{tcase.user}, []bool{tcase.ans})

			getUserParams := &common.GraphQLParams{

It's not getUserParams, just call it params.

graphql/e2e/auth/delete_mutation_test.go, line 250 at r3 (raw file):

			gqlResponse := getUserParams.ExecuteAsPost(t, graphqlURL)
			require.Nil(t, gqlResponse.Errors)
			require.JSONEq(t, tcase.result, string(gqlResponse.Data))

Should we verify the state after the delete operation has gone through?

graphql/e2e/auth/delete_mutation_test.go, line 252 at r3 (raw file):

			require.JSONEq(t, tcase.result, string(gqlResponse.Data))

			// Restore the deleted Questions.

... deleted Questions for other test cases.

graphql/e2e/auth/update_mutation_test.go, line 373 at r3 (raw file):

	testCases := []TestCase{{
		user:   "user1@dgraph.io",

Add a description or comment string field which talks about what each case is testing otherwise I have to go look at the schema each time to try to understand whats happening.

graphql/e2e/auth/update_mutation_test.go, line 390 at r3 (raw file):

		mutation($ids: [ID!]){
			updateQuestion(input: {filter: {id: $ids}, set: {topic: "A Topic"}}){
			question{

formatting is messed up

graphql/e2e/auth/update_mutation_test.go, line 400 at r3 (raw file):

	for _, tcase := range testCases {
		t.Run(tcase.user+strconv.FormatBool(tcase.ans), func(t *testing.T) {
			getUserParams := &common.GraphQLParams{

just params

graphql/e2e/auth/update_mutation_test.go, line 408 at r3 (raw file):

			gqlResponse := getUserParams.ExecuteAsPost(t, graphqlURL)
			require.Nil(t, gqlResponse.Errors)
			require.JSONEq(t, string(gqlResponse.Data), tcase.result)

We didn't really verify if the update actually updated the data. Should we do another query to verify the updated state?

minhaj-shakeel

Reviewable status: 2 of 8 files reviewed, 14 unresolved discussions (waiting on @MichaelJCompton and @pawanrawal)

graphql/e2e/auth/add_mutation_test.go, line 156 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

TestAuth_AddOnTypeWithRBACRuleOnInterface

Done.

graphql/e2e/auth/add_mutation_test.go, line 231 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Usually, there is another variable in the test case called expectedErr which is set to true for cases where you expect the error. You are using the result value to check the same thing but having another variable makes it more clear and explicit.

Done.

graphql/e2e/auth/add_mutation_test.go, line 245 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

No need to even fetch the id in the response. If you didn't fetch the id you can just compare the string JSON directly without having to unmarshal it using testutil.CompareJSON

Ids are fetched to delete the nodes after the test.

graphql/e2e/auth/add_mutation_test.go, line 260 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

TestAuth_AddOnTypeWithGraphTraversalRuleOnInterface

Done.

graphql/e2e/auth/add_mutation_test.go, line 326 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Same comments as above apply

Done.

graphql/e2e/auth/delete_mutation_test.go, line 208 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add a description or comment string field which talks about what each case is testing otherwise I have to go look at the schema each time to try to understand whats happening.

Done.

graphql/e2e/auth/delete_mutation_test.go, line 240 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Instead of running this query again, the first request above could give you the questions for all possible combinations?

We want a subset of possible combinations which is easier this way.

graphql/e2e/auth/delete_mutation_test.go, line 242 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

It's not getUserParams, just call it params.

Done.

graphql/e2e/auth/delete_mutation_test.go, line 250 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Should we verify the state after the delete operation has gone through?

I think numUids is sufficient after adding comments.

graphql/e2e/auth/delete_mutation_test.go, line 252 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

... deleted Questions for other test cases.

Done.

graphql/e2e/auth/update_mutation_test.go, line 373 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Add a description or comment string field which talks about what each case is testing otherwise I have to go look at the schema each time to try to understand whats happening.

Done.

graphql/e2e/auth/update_mutation_test.go, line 390 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

formatting is messed up

Done.

graphql/e2e/auth/update_mutation_test.go, line 400 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

just params

Done.

graphql/e2e/auth/update_mutation_test.go, line 408 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

We didn't really verify if the update actually updated the data. Should we do another query to verify the updated state?

As per my understanding, the response of update mutations does the job.

pawanrawal

Reviewable status: 2 of 8 files reviewed, 4 unresolved discussions (waiting on @MichaelJCompton and @minhaj-shakeel)

graphql/e2e/auth/add_mutation_test.go, line 225 at r4 (raw file):

	for _, tcase := range testCases {
		Params := &common.GraphQLParams{

Call it params with a small p.

graphql/e2e/auth/delete_mutation_test.go, line 242 at r3 (raw file):

Previously, minhaj-shakeel wrote…

Done.

Call it params with a small p.

graphql/e2e/auth/update_mutation_test.go, line 400 at r3 (raw file):

Previously, minhaj-shakeel wrote…

Done.

Go convention is to start variable names with small case unless they are exported

graphql/e2e/auth/update_mutation_test.go, line 408 at r3 (raw file):

Previously, minhaj-shakeel wrote…

As per my understanding, the response of update mutations does the job.

Oh right, that should be fine.

minhaj-shakeel

Reviewable status: 2 of 8 files reviewed, 4 unresolved discussions (waiting on @MichaelJCompton and @pawanrawal)

graphql/e2e/auth/add_mutation_test.go, line 225 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Call it params with a small p.

Done.

graphql/e2e/auth/update_mutation_test.go, line 408 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Oh right, that should be fine.

Done.

minhaj-shakeel and others added 3 commits October 29, 2020 15:38

initial commit

27ef3d1

feat(GraphQL): Allow Query with Auth rules on Interfaces (#6776)

2d99372

Fixes GRAPHQL-660. Fixes GRAPHQL-664. This PR extends support for Querying with Auth rules on Interfaces.

add e2e tests for add update and delete

670fdef

minhaj-shakeel requested review from MichaelJCompton and pawanrawal as code owners October 30, 2020 10:39

minhaj-shakeel added 7 commits October 30, 2020 16:16

merge master

ec3e60d

fix typo

fc7851c

add e2e tests for RBAC filter

b760232

minor fix in comment

ceebbd0

add unit tests for delete

1c0a0a5

add unit tests for add

81ae5ee

add more unit tests for add mutations

4d472f6

pawanrawal suggested changes Nov 3, 2020

View reviewed changes

pawanrawal reviewed Nov 3, 2020

View reviewed changes

address Pawan's comment

73dff66

minhaj-shakeel commented Nov 3, 2020

View reviewed changes

add space betweeen consecutive tesrs

8a73fd7

pawanrawal suggested changes Nov 4, 2020

View reviewed changes

minhaj-shakeel commented Nov 4, 2020

View reviewed changes

address Pawan's comment

4fbaf02

pawanrawal approved these changes Nov 4, 2020

View reviewed changes

minhaj-shakeel commented Nov 4, 2020

View reviewed changes

fix variable name

7f5e3be

minhaj-shakeel changed the base branch from master to minhaj/auth-on-interfaces November 4, 2020 10:48

Merge branch 'minhaj/auth-on-interfaces' into minhaj/mutation-auth-type

2cda7af

minhaj-shakeel merged commit 172ed2d into minhaj/auth-on-interfaces Nov 4, 2020

minhaj-shakeel deleted the minhaj/mutation-auth-type branch November 23, 2020 04:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(GraphQL): types should be able to inherit mutation auth rules from interfaces #6816

feat(GraphQL): types should be able to inherit mutation auth rules from interfaces #6816

minhaj-shakeel commented Oct 30, 2020 •

edited by ghost

Loading

pawanrawal left a comment

pawanrawal left a comment

minhaj-shakeel left a comment

pawanrawal left a comment

minhaj-shakeel left a comment

pawanrawal left a comment

minhaj-shakeel left a comment

feat(GraphQL): types should be able to inherit mutation auth rules from interfaces #6816

feat(GraphQL): types should be able to inherit mutation auth rules from interfaces #6816

Conversation

minhaj-shakeel commented Oct 30, 2020 • edited by ghost Loading

pawanrawal left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

minhaj-shakeel left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

minhaj-shakeel left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

minhaj-shakeel left a comment

Choose a reason for hiding this comment

minhaj-shakeel commented Oct 30, 2020 •

edited by ghost

Loading