-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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) fix auth query rewriting with ID filter (cherry-pick-7740) #8157
Conversation
|
6cc7d5f
to
e87d1fb
Compare
fix(GraphQL): fix auth query rewriting with ID filter (#7740) Fixes GRAPHQL-1159.
c75b4d1
to
ede22bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
@joshua-goldstein - the tests seem to have failed, I am re-running them (because I suspect this must have happened when one of us was mucking with the runners) |
Please don't merge this yet. Getting a panic when testing this locally. |
Include a test for the single ID JWT variable
Hi Matthew, any idea why this might be happening locally? Do you have a stack trace of the panic? I reran the test on our runners three times and we see the test passing there. |
@@ -1567,7 +1573,18 @@ func idFilter(filter map[string]interface{}, idField schema.FieldDefinition) []u | |||
if idsFilter == nil { | |||
return nil | |||
} | |||
idsSlice := idsFilter.([]interface{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was panic-ing if the type coming in from the wild was a simple string—which I'd say is the typical scenario for IDs when encoded in JWTs (taken from some session ID or something).
@joshua-goldstein, @skrdgraph Could you guys review my recent commits to this branch? |
@joshua-goldstein The issue was that the tests were not creating all the conditions in which an ID-assigned JWT variable was being passed—namely, the tests covered arrays (slices), but not single values. I uncovered this by actually deploying a graph with auth protected types and running queries with actual JWTs in the requests. So the fix was to check for that single value, not just the slice. I added another test to cover this condition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thnx for explaining this, it was easy to understand this @matthewmcneely
Refers to this PR. Cherry pick 7740.