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

fix(GraphQl): Allow case insensitive auth header for graphql subscriptions. #6141

Merged
merged 11 commits into from
Aug 12, 2020

Conversation

JatinDev543
Copy link
Contributor

@JatinDev543 JatinDev543 commented Aug 6, 2020

This PR allows case insensitive auth header for graphql subscriptions.


This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Aug 6, 2020
@JatinDev543 JatinDev543 changed the title fix(GraphQl): Allow case insensitive auth header for wss protocol. fix(GraphQl): Allow case insensitive auth header for graphql subscriptions. Aug 7, 2020
Copy link

@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 2 files reviewed, 3 unresolved discussions (waiting on @jatindevdg, @MichaelJCompton, and @pawanrawal)


graphql/web/http.go, line 145 at r1 (raw file):

		name := authorization.GetHeader()
		var val string
		var ok = false

The default value is false for boolean.
var ok bool


graphql/web/http.go, line 146 at r1 (raw file):

		var val string
		var ok = false
		for k := range payload {

Fetch the value as well instead of getting if from the map again.
for k,v := range payload ...


graphql/web/http.go, line 153 at r1 (raw file):

		}

		if ok {

Reduce indentation
if !ok {
return
}

Copy link
Contributor Author

@JatinDev543 JatinDev543 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 2 files reviewed, 3 unresolved discussions (waiting on @arijitAD, @MichaelJCompton, and @pawanrawal)


graphql/web/http.go, line 145 at r1 (raw file):

Previously, arijitAD (Arijit Das) wrote…

The default value is false for boolean.
var ok bool

changed.


graphql/web/http.go, line 146 at r1 (raw file):

Previously, arijitAD (Arijit Das) wrote…

Fetch the value as well instead of getting if from the map again.
for k,v := range payload ...

changed.


graphql/web/http.go, line 153 at r1 (raw file):

Previously, arijitAD (Arijit Das) wrote…

Reduce indentation
if !ok {
return
}

changed.

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

:lgtm

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @arijitAD, @jatindevdg, and @MichaelJCompton)


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

		var ok bool
		for key, val = range payload {
			if strings.EqualFold(key, name) {

nice

Copy link

@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: all files reviewed, 6 unresolved discussions (waiting on @arijitAD, @jatindevdg, and @MichaelJCompton)


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

	"strconv"
	"strings"
	"time"

Sort the imports.
Golang local packages followed by other packages.
Check other files for reference.


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

				ok = true
				break
			}

This will clean up the code.
You don't have to declare ok,key,value seperately.

if !strings.EqualFold(key, name) {
continue
}
md := metadata.New(map[string]string{
"authorizationJwt": val.(string),
})
ctx = metadata.NewIncomingContext(ctx, md)
customClaims, err = authorization.ExtractCustomClaims(ctx)
if err != nil {
return nil, err
}
break;

Copy link
Contributor Author

@JatinDev543 JatinDev543 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: 1 of 2 files reviewed, 6 unresolved discussions (waiting on @arijitAD, @jatindevdg, @MichaelJCompton, and @pawanrawal)


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

Previously, arijitAD (Arijit Das) wrote…

Sort the imports.
Golang local packages followed by other packages.
Check other files for reference.

done.


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

Previously, arijitAD (Arijit Das) wrote…

This will clean up the code.
You don't have to declare ok,key,value seperately.

if !strings.EqualFold(key, name) {
continue
}
md := metadata.New(map[string]string{
"authorizationJwt": val.(string),
})
ctx = metadata.NewIncomingContext(ctx, md)
customClaims, err = authorization.ExtractCustomClaims(ctx)
if err != nil {
return nil, err
}
break;

changed.

Copy link

@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.

lgtm!

Reviewable status: 1 of 2 files reviewed, 6 unresolved discussions (waiting on @arijitAD, @jatindevdg, @MichaelJCompton, and @pawanrawal)

Copy link

@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.

:lgtm:

Reviewable status: 1 of 2 files reviewed, 6 unresolved discussions (waiting on @arijitAD, @jatindevdg, @MichaelJCompton, and @pawanrawal)

@JatinDev543 JatinDev543 merged commit d807eb7 into master Aug 12, 2020
@JatinDev543 JatinDev543 deleted the jatin/GRAPHQL-602 branch August 12, 2020 07:15
JatinDev543 added a commit that referenced this pull request Aug 13, 2020
…tions. (#6141)

This PR allows case insensitive auth header for graphql subscriptions.

(cherry picked from commit d807eb7)
JatinDev543 added a commit that referenced this pull request Aug 14, 2020
…tions. (#6141) (#6179)

This PR allows case insensitive auth header for graphql subscriptions.

(cherry picked from commit d807eb7)
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.

3 participants