Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(GraphQL): Allow Multiple JWKUrls for auth. #7528

Merged
merged 6 commits into from
Mar 10, 2021

Conversation

minhaj-shakeel
Copy link
Contributor

@minhaj-shakeel minhaj-shakeel commented Mar 8, 2021

Fixes GRAPHQL-1048.
This PR adds support for Passing multiple JWKUrls in the authorization header.


This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Mar 8, 2021
Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

// FetchJWKs fetches the JSON Web Key set from a JWKUrl. It acquires a Lock over a as some of the
// properties of AuthMeta are modified in the process.

This part of the comment is stale now, can be removed:

It acquires a Lock over a as some of the properties of AuthMeta are modified in the process.

graphql/schema/schemagen.go, line 409 at r1 (raw file):

Quoted 6 lines of code…
		for i := 0; i < len(metaInfo.authMeta.JWKUrls); i++ {
			fetchErr := metaInfo.authMeta.FetchJWKs(i)
			if fetchErr != nil {
				return nil, fetchErr
			}
		}

we should make 2 separate functions FetchJWK(i) and FetchJWKs()
FetchJWKs() will contain this for loop and use FetchJWK(i) internally?


testutil/graphql.go, line 252 at r1 (raw file):

"jwkurls":

we should keep tests for both jwkurl and jwkurls. Let's let the original test be as it was, but to test jwkurls add another test.

Copy link
Contributor Author

@minhaj-shakeel minhaj-shakeel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)


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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
// FetchJWKs fetches the JSON Web Key set from a JWKUrl. It acquires a Lock over a as some of the
// properties of AuthMeta are modified in the process.

This part of the comment is stale now, can be removed:

It acquires a Lock over a as some of the properties of AuthMeta are modified in the process.

Done.


graphql/schema/schemagen.go, line 409 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
		for i := 0; i < len(metaInfo.authMeta.JWKUrls); i++ {
			fetchErr := metaInfo.authMeta.FetchJWKs(i)
			if fetchErr != nil {
				return nil, fetchErr
			}
		}

we should make 2 separate functions FetchJWK(i) and FetchJWKs()
FetchJWKs() will contain this for loop and use FetchJWK(i) internally?

Done.


testutil/graphql.go, line 252 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
"jwkurls":

we should keep tests for both jwkurl and jwkurls. Let's let the original test be as it was, but to test jwkurls add another test.

Done.

@minhaj-shakeel minhaj-shakeel merged commit f4c857b into master Mar 10, 2021
@minhaj-shakeel minhaj-shakeel deleted the minhaj/multiple-jwk-url branch March 10, 2021 05:41
minhaj-shakeel added a commit that referenced this pull request Mar 16, 2021
minhaj-shakeel added a commit that referenced this pull request Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

2 participants