-
Notifications
You must be signed in to change notification settings - Fork 283
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
Create valid documents with authorization filtering #5952
Create valid documents with authorization filtering #5952
Conversation
This comment has been minimized.
This comment has been minimized.
CI performance tests
|
this will make them available to visitors
while working on this I uncovered some issues linked to fragment ordering: when encountering a fragment spread, authorization plugins need the referenced fragment to have been already processed, but this is not always the case if we process them in the order they are defined in. As an example: query {
... F
}
fragment F on Query {
... G
}
fragment G on Query {
op
} In this query, when we look at the This is fixed in this PR by first going through the fragments and the spreads in their selections, then ordering the fragments by their dependencies (first processing fragments that do not reference other fragments) and porocessing them, then processing operations. |
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.
I ran a few more tests locally and everything worked a treat.
.used_variables | ||
.insert(var.as_str().to_string()); | ||
} | ||
} |
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.
Values can have variables deeply nested, and there can be multiple variables per argument.
query Test($a: String, $b: String) {
foo(arg: { someField: [[$a, "bar", $b]] })
}
It's works for arguments in both fields and directives
.insert(var.as_str().to_string()); | ||
} | ||
} | ||
} |
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.
Directives with variables can be also located on operation definition and fragment definitions.
query Test($a: String) @foo(arg: $a) {
...TestFragment
}
fragment TestFragment on Query @bar(arg: $a) {
__typename
}
Currently, only directives on fields, inline fragments, and fragment spreads are tracked.
Closes #5648
Problem
To support the router's authorization feature, we have a "transform" query visitor that can be used to modify the query. In authorization, it is used to remove the parts of the query that are not authorized, resulting in null values being inserted in their place in the client response.
That visitor was generating invalid queries for 2 reasons:
would be tranformed to this if we removed the
a
field, resulting in an unused fragment:while we would expect this instead:
would be transformed to this if we removed the
a
field, resulting in$arg1
and$arg2
being unused in the query:while we would expect this instead:
Notes
To fix this, we have to take special care in how it is implemented, for the following reasons:
Visitor
trait implementersVisitor
implementers, like authorization filters, need to have all the information on fragments at the point of application. As an example, in... F
, if we definedfragment F on Obj
withObj
being unauthorized for this query, or the fragment being removed because all of its selections were removed, the visitor must have processed the fragment first. And that also applies when one fragment inserts another fragmentProposed solution
The visitor now follows this process:
TODO:
bridge_query_planner.rs
? Maybe in a follow up PR?Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩