feat(graphql): adding auth token support for regexp, in and arrays

[aman-bansal]

This PR aims to fix GRAPHQL-808. This enables multiple things.

1. support for regex matching in custom claims.
2. support for array in custom claims.
3. Claims match based on float, int64, int, structs, bool, string
4. in based custom claim match.

---

This change is 

[netlify]

✔️ Deploy preview for dgraph-docs ready!

🔨 Explore the source changes: 5f02186

🔍 Inspect the deploy logs: https://app.netlify.com/sites/dgraph-docs/deploys/5fd6fe7120ee8b0008eaef51

😎 Browse the preview: https://deploy-preview-7039--dgraph-docs.netlify.app

[abhimanyusinghgaur]

Just some minor refactors

Reviewed 5 of 6 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @aman-bansal, @MichaelJCompton, and @pawanrawal)

---

graphql/e2e/auth/auth_test.go, line 1903 at r2 (raw file):

require.Nil(t, gqlResponse.Errors)

RequireNoGQLErrors everywhere

---

graphql/e2e/auth/auth_test.go, line 1923 at r2 (raw file):

	require.JSONEq(t, string(gqlResponse.Data), bookResponse)
}

All the tests seem to be doing the same query and expect the same response, with a varying header. So, can we combine them into one single test which tests the query over the list of varying headers like:

testCases := []struct{
 name string
 headers: http.Header
}
for _, tcase := range testCases {
  t.Run(tcase.name, func(t *testing.T) {
    // query here with the current header and match the response
  }
}

---

graphql/schema/auth.go, line 464 at r2 (raw file):

objects and nil

nil

---

graphql/schema/auth.go, line 469 at r2 (raw file):

Quoted 17 lines of code…

	if op == nil {
		return nil, gqlerror.Errorf("Type %s: @auth: `%s` is not a valid GraphQL variable. "+
			"object or nil values aren't supported", typ.Name, rule[idx[0][2]:idx[0][3]])
	}
	query := RBACQuery{
		Variable: rule[idx[0][2]:idx[0][3]],
		Operator: rule[idx[0][4]:idx[0][5]],
		Operand:  op,
	}
	if !strings.HasPrefix(query.Variable, "$") {
		return nil, gqlerror.Errorf("Type %s: @auth: `%s` is not a valid GraphQL variable.",
			typ.Name, query.Variable)
	}
	query.Variable = query.Variable[1:]
	if err = validateRBACOperators(typ, query); err != nil {
		return nil, err
	}

can we have just one function which does all the validation on the RBACQuery, something like

func Validate(typ *ast.Definition, query *RBACQuery) error {}

which will have all the validation related to variable, operator, and operand inside of it.
And if validate passes, then we call another function initialization function which can take care of removing the $ from variable, and setting regex.

---

graphql/schema/auth.go, line 496 at r2 (raw file):

var ok = false

ok := false

[pawanrawal]

It's looking good as a change. Added some comments about the errors that are returned.

Reviewed 1 of 4 files at r3.
Reviewable status: 3 of 6 files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur, @aman-bansal, and @MichaelJCompton)

---

graphql/schema/auth.go, line 98 at r3 (raw file):

}

func checkIfRegexMatch(regex *regexp.Regexp, val1 interface{}) RuleResult {

instead of having this function can we use the above function for this case as well with just one element in arr?

---

graphql/schema/auth.go, line 120 at r3 (raw file):

		tokenValues, err := cast.ToSliceE(av[rq.Variable])
		if err != nil {
			// this means value for variable in token in not an array

interesting, so the function returns an error if you can't cast to slice? Any other cases in which an error is returned? Can't we just check the type of the value and then call the appropriate function?

---

graphql/schema/auth.go, line 126 at r3 (raw file):

	case "regexp":
		// if regexp, auth rule value should always be string and so as token values
		tokenValues, err := cast.ToSliceE(av[rq.Variable])

We do this again now. Why not just check the value type above outside this switch case first.

---

graphql/schema/auth_schemas_test.yaml, line 39 at r3 (raw file):

      }
    errlist: [
      { "message": "Type X: @auth: `in` operator has invalid value in this rule." }

Should the error message say that the value should be an array?

---

graphql/schema/auth_schemas_test.yaml, line 62 at r3 (raw file):

      }
    errlist: [
      { "message": "Type X: @auth: `$USER` is not a valid GraphQL variable." }

Strange error, shouldn't it say nil is an invalid value for eq`

---

graphql/schema/auth_schemas_test.yaml, line 74 at r3 (raw file):

      }
    errlist: [
      { "message": "Type X: @auth: `$USER` is not a valid GraphQL variable. object with nil values aren't supported" }

Why isn't $USER an invalid GraphQL variable. It seems to me that eq: null is an invalid check here and not the variable?

---

graphql/schema/auth_schemas_test.yaml, line 86 at r3 (raw file):

      }
    errlist: [
      { "message": "Type X: @auth: `regexp` operator has invalid value in this rule." }

What is this rule? Please also add the invalid value using %v as part of the error and say that the value should be of type String.

[aman-bansal]

Reviewable status: 2 of 6 files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur, @MichaelJCompton, and @pawanrawal)

---

graphql/e2e/auth/auth_test.go, line 1903 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

require.Nil(t, gqlResponse.Errors)

RequireNoGQLErrors everywhere

Done.

---

graphql/e2e/auth/auth_test.go, line 1923 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

All the tests seem to be doing the same query and expect the same response, with a varying header. So, can we combine them into one single test which tests the query over the list of varying headers like:

testCases := []struct{
 name string
 headers: http.Header
}
for _, tcase := range testCases {
  t.Run(tcase.name, func(t *testing.T) {
    // query here with the current header and match the response
  }
}

Done.

---

graphql/schema/auth.go, line 464 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

objects and nil

nil

Done.

---

graphql/schema/auth.go, line 469 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	if op == nil {
		return nil, gqlerror.Errorf("Type %s: @auth: `%s` is not a valid GraphQL variable. "+
			"object or nil values aren't supported", typ.Name, rule[idx[0][2]:idx[0][3]])
	}
	query := RBACQuery{
		Variable: rule[idx[0][2]:idx[0][3]],
		Operator: rule[idx[0][4]:idx[0][5]],
		Operand:  op,
	}
	if !strings.HasPrefix(query.Variable, "$") {
		return nil, gqlerror.Errorf("Type %s: @auth: `%s` is not a valid GraphQL variable.",
			typ.Name, query.Variable)
	}
	query.Variable = query.Variable[1:]
	if err = validateRBACOperators(typ, query); err != nil {
		return nil, err
	}

can we have just one function which does all the validation on the RBACQuery, something like

func Validate(typ *ast.Definition, query *RBACQuery) error {}

which will have all the validation related to variable, operator, and operand inside of it.
And if validate passes, then we call another function initialization function which can take care of removing the $ from variable, and setting regex.

Done.

---

graphql/schema/auth.go, line 496 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

var ok = false

ok := false

Done.

---

graphql/schema/auth.go, line 98 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

instead of having this function can we use the above function for this case as well with just one element in arr?

I have made changes to this. This should make more sense now.

---

graphql/schema/auth.go, line 120 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

interesting, so the function returns an error if you can't cast to slice? Any other cases in which an error is returned? Can't we just check the type of the value and then call the appropriate function?

we are not returning error. There is no constraint over how the auth token looks. It could be array of values or a single value. So if we are not able to caste it into array, then it means its a single value and thus we check for equal match

---

graphql/schema/auth.go, line 126 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

We do this again now. Why not just check the value type above outside this switch case first.

good point. Thanks.

---

graphql/schema/auth_schemas_test.yaml, line 39 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Should the error message say that the value should be an array?

I have added this. it was making things bit messy thats why i didn't do it that time.

---

graphql/schema/auth_schemas_test.yaml, line 62 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Strange error, shouldn't it say nil is an invalid value for eq`

It's because this error has been thrown by the gql parser itself. It fails before we get to check the value and validate it.
null from graphql translates into nil in go. but nil in graphql throws invalid error from the parser itself.

---

graphql/schema/auth_schemas_test.yaml, line 74 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Why isn't $USER an invalid GraphQL variable. It seems to me that eq: null is an invalid check here and not the variable?

fixed the message

---

graphql/schema/auth_schemas_test.yaml, line 86 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

What is this rule? Please also add the invalid value using %v as part of the error and say that the value should be of type String.

regex value provided in the auth rule should be of type string. This is needed to parse operand in regex. i have added value and reason in the message

[pawanrawal]

Mostly looking good, just some more changes and then I'll have another final look

Reviewed 2 of 6 files at r1, 2 of 4 files at r3, 2 of 2 files at r4.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @abhimanyusinghgaur, @aman-bansal, @MichaelJCompton, and @pawanrawal)

---

graphql/e2e/auth/auth_test.go, line 1923 at r2 (raw file):

Previously, aman-bansal (aman bansal) wrote…

Done.

Also seems like the query is common across the test cases so maybe define it above and just use the same one?

---

graphql/schema/auth.go, line 146 at r4 (raw file):

		}

		return rq.checkIfMatchInArray(tokenValues)

Do you need three separate cases now or can they all be clubbed into the same case with

case "eq", "in", "regexp":
...

---

graphql/schema/auth.go, line 515 at r4 (raw file):

		if isArray {
			return false, fmt.Sprintf("Type %s: @auth: `%s` operator has invalid value `%v` in "+
				"this rule. Reason: Array values in eq operator will not be supported.",

Remove Reason: from all errors please. Also this rule doesn't tell which rule so best that it be removed. In the future we may also return the path of the rule.

The error should be like below.

.... operator has invalid value: %v. Array values for eq operator are not supported.

---

graphql/schema/auth_schemas_test.yaml, line 62 at r3 (raw file):

Previously, aman-bansal (aman bansal) wrote…

It's because this error has been thrown by the gql parser itself. It fails before we get to check the value and validate it.
null from graphql translates into nil in go. but nil in graphql throws invalid error from the parser itself.

ok

---

graphql/schema/auth_schemas_test.yaml, line 86 at r3 (raw file):

Previously, aman-bansal (aman bansal) wrote…

regex value provided in the auth rule should be of type string. This is needed to parse operand in regex. i have added value and reason in the message

We should still remove in this rule from all errors because if a user had say 20 rules, its not clear what this rule refers to.

[aman-bansal]

Reviewable status: 3 of 6 files reviewed, 9 unresolved discussions (waiting on @abhimanyusinghgaur, @MichaelJCompton, and @pawanrawal)

---

graphql/e2e/auth/auth_test.go, line 1923 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Also seems like the query is common across the test cases so maybe define it above and just use the same one?

Done

---

graphql/schema/auth.go, line 146 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Do you need three separate cases now or can they all be clubbed into the same case with

case "eq", "in", "regexp":
...

It's done.

---

graphql/schema/auth.go, line 515 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Remove Reason: from all errors please. Also this rule doesn't tell which rule so best that it be removed. In the future we may also return the path of the rule.

The error should be like below.

.... operator has invalid value: %v. Array values for eq operator are not supported.

Got it. I have fixed this.

---

graphql/schema/auth_schemas_test.yaml, line 86 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

We should still remove in this rule from all errors because if a user had say 20 rules, its not clear what this rule refers to.

oh Got it. I have fixed the message.

[pawanrawal]

Reviewable status: 2 of 6 files reviewed, 5 unresolved discussions (waiting on @abhimanyusinghgaur, @MichaelJCompton, and @pawanrawal)