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

Send auth variable in custom jwt token. #5220

Merged
merged 5 commits into from
Apr 21, 2020

Conversation

arijitAD
Copy link

@arijitAD arijitAD commented Apr 16, 2020

This change is Reviewable

Docs Preview: Dgraph Preview

@arijitAD arijitAD requested review from manishrjain and a team as code owners April 16, 2020 04:44
x/x.go Outdated Show resolved Hide resolved
x/x.go Outdated Show resolved Hide resolved
x/x.go Outdated Show resolved Hide resolved
x/x.go Outdated Show resolved Hide resolved
Copy link
Contributor

@MichaelJCompton MichaelJCompton left a comment

Choose a reason for hiding this comment

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

Looks ok, just let me know if the couple of points I've asked about are ok.

Reviewable status: 0 of 4 files reviewed, 7 unresolved discussions (waiting on @arijitAD, @manishrjain, and @MichaelJCompton)


graphql/web/http.go, line 113 at r2 (raw file):

	ctx = x.AttachAuthorizationJwt(ctx, r)
	ctx = x.AttachAccessJwt(ctx, r)

Do we have to do something else here eventually? We'll be either having the enterprise jwt, or the GraphQL on ... or does this just work as is?


x/x.go, line 27 at r2 (raw file):

	"encoding/json"
	"fmt"
	"github.com/dgrijalva/jwt-go"

Is this dependency the right one? Looks like has lots of github stars. Is it the same thing that we take a JWT dependency on elsewhere?

Also make sure the processing code below is best practice for JWT or same as we do elsewhere in Dgraph.


x/x.go, line 216 at r2 (raw file):

	authVariables := jsonMap[CustomClaimsUrl].(map[string]interface{})
	return authVariables, nil

how or where are we going to verify the key the JWT is signed with?

Copy link
Author

@arijitAD arijitAD 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 4 files reviewed, 7 unresolved discussions (waiting on @golangcibot, @manishrjain, and @MichaelJCompton)


graphql/web/http.go, line 113 at r2 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…
	ctx = x.AttachAuthorizationJwt(ctx, r)
	ctx = x.AttachAccessJwt(ctx, r)

Do we have to do something else here eventually? We'll be either having the enterprise jwt, or the GraphQL on ... or does this just work as is?

Both store the values in different keys. Hence, both values can co-exist in the context. While extracting the value we query for the key. Should we prioritize if both headers are present?


x/x.go, line 195 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

error strings should not be capitalized or end with punctuation or a newline (from golint)

Done.


x/x.go, line 201 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

error strings should not be capitalized or end with punctuation or a newline (from golint)

Done.


x/x.go, line 206 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

printf: Errorf call has arguments but no formatting directives (from govet)

Done.


x/x.go, line 212 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

printf: Errorf call has arguments but no formatting directives (from govet)

Done.


x/x.go, line 27 at r2 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

Is this dependency the right one? Looks like has lots of github stars. Is it the same thing that we take a JWT dependency on elsewhere?

Also make sure the processing code below is best practice for JWT or same as we do elsewhere in Dgraph.

Yes, It's the same library used in access_ee.go.


x/x.go, line 216 at r2 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

how or where are we going to verify the key the JWT is signed with?

I have added a ToDo above to verify jwt before parsing it. We need a mechanism (via flag) to provide Dgraph the secret key so we can verify it.

Copy link
Contributor

@MichaelJCompton MichaelJCompton left a comment

Choose a reason for hiding this comment

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

:lgtm:

you can merge this and then start working on the other JWT bit

Reviewable status: 0 of 4 files reviewed, 7 unresolved discussions (waiting on @golangcibot, @manishrjain, and @MichaelJCompton)

@arijitAD arijitAD merged commit de3b0de into mjc/auth-query Apr 21, 2020
@arijitAD arijitAD deleted the arijitAD/auth-jwt-claims branch April 21, 2020 16:26
MichaelJCompton pushed a commit that referenced this pull request Apr 21, 2020
* Send auth variable in custom jwt token.

* Verify custom claims using key.
MichaelJCompton added a commit that referenced this pull request Apr 22, 2020
* Send auth variable in custom jwt token. (#5220)
* graphql: auth on get and mutation results (#5259)
MichaelJCompton added a commit that referenced this pull request May 5, 2020
* add Auth directive (#5178)
* parse auth rules (#5180)
* added query rewriting and e2e tests (#5229)
* parse and evaluate RBAC rules. (#5210)
* added test cases for auth schema parsing. (#5195)
* process auth query rules (#5181)
* send auth variable in custom jwt token. (#5220)
* auth on get and mutation results (#5259)
* delete authorization (#5270)
* parse auth meta info from schema. (#5269)
* auth on add update mutations (#5300)
* query e2e tests for authentication (#5312)
* more testing around additional deletes and auth (#5357)
* add RSA algo for JWT token verification. (#5358)
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
* add Auth directive (hypermodeinc#5178)
* parse auth rules (hypermodeinc#5180)
* added query rewriting and e2e tests (hypermodeinc#5229)
* parse and evaluate RBAC rules. (hypermodeinc#5210)
* added test cases for auth schema parsing. (hypermodeinc#5195)
* process auth query rules (hypermodeinc#5181)
* send auth variable in custom jwt token. (hypermodeinc#5220)
* auth on get and mutation results (hypermodeinc#5259)
* delete authorization (hypermodeinc#5270)
* parse auth meta info from schema. (hypermodeinc#5269)
* auth on add update mutations (hypermodeinc#5300)
* query e2e tests for authentication (hypermodeinc#5312)
* more testing around additional deletes and auth (hypermodeinc#5357)
* add RSA algo for JWT token verification. (hypermodeinc#5358)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants