feat(GraphQL): Support authorization with jwk_url

[minhaj-shakeel]

Fixes GRAPHQL-693.

This PR extends support for authorization with jwk_url. Now users can give jwkUrl for their project in the Dgraph.Authorization JSON in their schema.

#Dgraph.Authorization 
{"VerificationKey":"",
      "Header":"header",
     "Namespace":"your-namespace",
    "jwkurl": "your-jwk-url-",
    "Algo":"",
    "Audience":["your-audience"]
}

When jwkurl is provided, keep VerificationKey and Algo empty as they are not needed.

---

This change is 

[pawanrawal]

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

---

graphql/authorization/auth.go, line 58 at r1 (raw file):

	VerificationKey string
	JWKUrl          string
	JWKSet          *jose.JSONWebKeySet

Does this need to be public or can it be internal like jwkSet?

---

graphql/authorization/auth.go, line 59 at r1 (raw file):

	JWKUrl          string
	JWKSet          *jose.JSONWebKeySet
	RefreshTime     time.Duration `json:"-"` // Ignoring this field for now (might later include in the input JSON)

set it to an internal field for now

---

graphql/authorization/auth.go, line 75 at r1 (raw file):

	// If JWKUrl is provided, we don't expect (VerificationKey, Algo),
	// they are needed only if JWKUrl is not present there.
	if a.JWKUrl != "" {

Needs some tests for these error scenarios?

---

graphql/authorization/auth.go, line 309 at r1 (raw file):

		token, err =
			jwt.ParseWithClaims(jwtStr, &CustomClaims{}, func(token *jwt.Token) (interface{}, error) {
				kid := token.Header["kid"].(string)

What if the kid header is null or is it always present?

---

graphql/authorization/auth.go, line 315 at r1 (raw file):

				}
				return signingKeys[0].Key, nil
			}, jwt.WithoutAudienceValidation())

We need a test for this as well.

---

graphql/authorization/auth.go, line 372 at r1 (raw file):

	}

	data, _ := ioutil.ReadAll(resp.Body)

Why not handle this error?

---

graphql/authorization/auth.go, line 391 at r1 (raw file):

	if resp.Header["Expires"] != nil {
		maxAge, err = ParseExpires(resp.Header["Expires"][0])

These errors are not handled, they should be. Which one out of the Expires or the Cache-Control header does the firebase SDK use?

---

graphql/authorization/auth.go, line 395 at r1 (raw file):

	if resp.Header["Cache-Control"] != nil {
		maxAge, err = ParseMaxAge(resp.Header["Cache-Control"][0])

error not handled

---

graphql/authorization/auth.go, line 397 at r1 (raw file):

		maxAge, err = ParseMaxAge(resp.Header["Cache-Control"][0])
	}
	a.RefreshTime = time.Duration(maxAge) * time.Second

Don't we need a lock here and above where we are modifying properties of a?

---

graphql/authorization/auth.go, line 407 at r1 (raw file):

		select {
		case <-a.ticker.C:
			if a.RefreshTime == 0 {

When is this set to 0?

---

graphql/authorization/auth.go, line 414 at r1 (raw file):

			for {
				a.Lock()
				err := a.fetchJWKs()

Is this error logged? We should not try infinitely here as we have a false loop outside already. We should have a number of max attempts here, say 3.
What is the behavior initially when the user has not given a schema with a JWK_URL?

[minhaj-shakeel]

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

---

graphql/authorization/auth.go, line 58 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Does this need to be public or can it be internal like jwkSet?

It should be internal. I have changed it to jwkSet.

---

graphql/authorization/auth.go, line 59 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

set it to an internal field for now

Done.

---

graphql/authorization/auth.go, line 75 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Needs some tests for these error scenarios?

We can include them. These can be easily verified.

---

graphql/authorization/auth.go, line 309 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

What if the kid header is null or is it always present?

It is always present in the jwk.

---

graphql/authorization/auth.go, line 315 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

We need a test for this as well.

Sure.

---

graphql/authorization/auth.go, line 372 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Why not handle this error?

Done.

---

graphql/authorization/auth.go, line 391 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

These errors are not handled, they should be. Which one out of the Expires or the Cache-Control header does the firebase SDK use?

Actually, Firebases uses both the headers. We can extract any one of them. I will remove one after the discussion.

---

graphql/authorization/auth.go, line 395 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

error not handled

Error in parsing the Headers means that we were not able to find any meaning value for the expiration time. In this case, it just sets maxAge to 0. There is no need behind handling this error.

---

graphql/authorization/auth.go, line 397 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Don't we need a lock here and above where we are modifying properties of a?

Done.

---

graphql/authorization/auth.go, line 407 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

When is this set to 0?

By default, it is set to 0. See the maxAgein fetchJWKs() function.

---

graphql/authorization/auth.go, line 414 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Is this error logged? We should not try infinitely here as we have a false loop outside already. We should have a number of max attempts here, say 3.
What is the behavior initially when the user has not given a schema with a JWK_URL?

Added retry only for 3 attempts. Also added the case if no JWK_URL is given.

[pawanrawal]

Reviewable status: 0 of 6 files reviewed, 16 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, @minhaj-shakeel, @pawanrawal, and @vvbalaji-dgraph)

---

graphql/authorization/auth.go, line 391 at r1 (raw file):

Previously, minhaj-shakeel wrote…

Actually, Firebases uses both the headers. We can extract any one of them. I will remove one after the discussion.

I just see Cache-Control to be used by the SDK https://github.com/firebase/firebase-admin-go/blob/cef91acd46f2fc5d0b3408d8154a0005db5bdb0b/auth/token_verifier.go#L474 so lets just use that?

---

graphql/authorization/auth.go, line 414 at r1 (raw file):

Previously, minhaj-shakeel wrote…

Added retry only for 3 attempts. Also added the case if no JWK_URL is given.

a.fetchJWKs

---

graphql/authorization/auth.go, line 387 at r2 (raw file):

	var jwkArray JwkArray
	json.Unmarshal(data, &jwkArray)

error not handled

---

graphql/authorization/auth.go, line 160 at r3 (raw file):

	}

	// // fetch and Store the keys from JWKUrl

remove please

---

graphql/authorization/auth.go, line 307 at r3 (raw file):

		if authMeta.IsExpired() {
			err = authMeta.RefreshJWK()

Does this need to be a public function?

---

graphql/authorization/auth.go, line 309 at r3 (raw file):

			err = authMeta.RefreshJWK()
			if err != nil {
				return nil, err

use errors.Wrap(err, "while refreshing JWK from the URL"). It gives context about the error.

---

graphql/authorization/auth.go, line 370 at r3 (raw file):

}

func (a *AuthMeta) FetchJWKs() error {

fetchJWKs

---

graphql/authorization/auth.go, line 416 at r3 (raw file):

func (a *AuthMeta) RefreshJWK() error {
	fmt.Println("Starting Refresh")

remove

---

graphql/authorization/auth.go, line 418 at r3 (raw file):

	fmt.Println("Starting Refresh")
	// If there is no jwkUrl then return with error
	if a.JWKUrl == "" {

RLock needed to access this

---

graphql/authorization/auth.go, line 437 at r3 (raw file):

// returns false
func (a *AuthMeta) IsExpired() bool {
	if a.expiryTime.Equal(time.Time{}) {

If a.expiryTime.IsZero() is better.

---

graphql/resolve/auth_test.go, line 255 at r3 (raw file):

}

//Todo(Minhaj): Add a testcase for token without Expiry

Are you going to add this as well like a success case?

---

graphql/schema/schemagen.go, line 226 at r3 (raw file):

	// then Fetch the JWKs from the JWKUrl
	if metaInfo != nil && metaInfo.JWKUrl != "" {
		metaInfo.RLock()

Should be a RWLock because you are setting some values in there

[minhaj-shakeel]

Reviewable status: 0 of 6 files reviewed, 16 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, @minhaj-shakeel, @pawanrawal, and @vvbalaji-dgraph)

---

graphql/authorization/auth.go, line 414 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

a.fetchJWKs

FetchJWKs is called from the schema package.

---

graphql/authorization/auth.go, line 387 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

error not handled

Done.

---

graphql/authorization/auth.go, line 160 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

remove please

Done.

---

graphql/authorization/auth.go, line 307 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Does this need to be a public function?

No. Changed it.

---

graphql/authorization/auth.go, line 309 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

use errors.Wrap(err, "while refreshing JWK from the URL"). It gives context about the error.

Done.

---

graphql/authorization/auth.go, line 370 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

fetchJWKs

But this function is called in the schema package so need to make it public.

---

graphql/authorization/auth.go, line 416 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

remove

Done.

---

graphql/authorization/auth.go, line 418 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

RLock needed to access this

Done.

---

graphql/authorization/auth.go, line 437 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

If a.expiryTime.IsZero() is better.

Done.

---

graphql/resolve/auth_test.go, line 255 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Are you going to add this as well like a success case?

Yeah, Actually I need to figure out minting a token with a long expiry date. Then I will do that.

---

graphql/schema/schemagen.go, line 226 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Should be a RWLock because you are setting some values in there

Changed

[pawanrawal]

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, @minhaj-shakeel, and @vvbalaji-dgraph)

---

graphql/authorization/auth.go, line 307 at r3 (raw file):

Previously, minhaj-shakeel wrote…

No. Changed it.

Hmm, this function already has a RLock and then another lock is acquired within refreshJWK. This must be having a deadlock. How does this work?

---

graphql/authorization/auth.go, line 370 at r3 (raw file):

Previously, minhaj-shakeel wrote…

But this function is called in the schema package so need to make it public.

I see ok

---

graphql/resolve/auth_test.go, line 255 at r3 (raw file):

Previously, minhaj-shakeel wrote…

Yeah, Actually I need to figure out minting a token with a long expiry date. Then I will do that.

ok