refactor: query planner service no longer returns graphql response - #9911
Conversation
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 1 new, 20 changed, 0 removedBuild ID: 8fb8311ea5c70b691bc4490e URL: https://www.apollographql.com/docs/deploy-preview/8fb8311ea5c70b691bc4490e ✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
c33beea to
3ec0596
Compare
BrynCooke
changed the title
When `filter_query` empties the document, or `reject_unauthorized` is set and any path was removed, the query planner returns `QueryPlannerContent::Response` and the supergraph service forwards it verbatim. We are about to move that rejection out of the query planner into a supergraph layer, and three properties of the current behaviour had no test at all: - Execution is never reached. The existing reject tests register subgraph mocks but never assert they went unused, so a regression that let execution run would pass them as long as the response body matched. - The response carries HTTP 200. Directive-based authorization strips selections from the document, so a rejection has field-error semantics: value completion nulls the stripped selections and, once everything is stripped, propagates to `data: null`. 200 with errors is the correct response for that. Moving the rejection ahead of the planner makes it look like a request-level rejection, where 400 becomes the instinctive choice, so this needs pinning. - Usage reporting is not recorded, because `CachingQueryPlanner` only inserts it for the `Plan` variant. The operation is still metered — telemetry falls back to counting it as one licensed operation — but it arrives with no operation signature, referenced fields, or per-type stats, so Studio cannot attribute the rejection to an operation. Every subgraph is replaced by a `tower_test` mock with no canned responses, so reaching one fails the test. `assert_no_subgraph_calls` additionally fails when no subgraph service was built at all, which would otherwise make the assertion vacuous. Verified all three tests discriminate: with `reject_unauthorized` flipped to false the operation becomes a partial filter that does reach execution, and all three fail.
`CacheKeyMetadata` is part of `CachingQueryKey`'s `Hash`/`Eq`, which is what stops
an unauthenticated request from being served a plan built for an authenticated
one. Until now that invariant was only covered incidentally, by two supergraph
snapshot tests (`authorization::tests::{authenticated_directive,
scopes_directive}`) that send the same query twice against a shared cache and
compare bodies. Nothing named the invariant, so it was easy to weaken by
accident while editing either test for unrelated reasons.
`plan_cache_is_segmented_by_authorization_metadata` asserts it directly at the
caching planner: the same query under two different `CacheKeyMetadata` values
reaches the inner planner twice, and repeating the first value is served from
cache. Counting inner invocations rather than comparing response bodies means the
test states the cache behaviour instead of inferring it.
`rejection_response_is_cached` pins the fact that a `QueryPlannerContent::Response`
is cached like any other output, since `entry.insert` does not discriminate on
variant. Characterization only — moving the rejection ahead of the cache would
change this, and it should be a deliberate decision.
Verified the segmentation assertion discriminates: making both metadata values
identical drops the inner planner to a single invocation and fails the test, so
the extra plan is genuinely caused by the metadata difference and the third call
genuinely hits the cache.
Query plan warm-up bypasses the router and supergraph pipelines entirely — `WarmupParseQueryLayer` wraps the `CachingQueryPlanner` directly — and plans with `CacheKeyMetadata::default()`, which is byte-for-byte the metadata a genuinely unauthenticated request produces. Warm-up is therefore only safe because authorization filtering sits inside the query planner, below the plan cache, where every caller hits it regardless of how it got there. Nothing tested that. No warm-up test involves authorization directives at all, and the caching planner tests all use default metadata, so the property held by construction rather than by assertion. This asserts it at the planner: with directives enabled and unauthenticated metadata, planning an operation whose root field requires a scope returns a rejection, not a plan. Moving authorization out of the planner has to keep that true by some other means — otherwise warm-up populates the default-metadata cache key with an unfiltered plan and any unauthenticated request is served it. The failure message spells out that consequence, because a bare "expected Response, got Plan" would not tell whoever trips it why it matters. Verified the assertion discriminates: disabling the directives makes the planner return a plan and the test fails with the leak message.
When authorization filters an operation, response formatting runs twice: once for
the filtered query, then once for the original. The filtered pass has to copy
`__typename` through, because the original pass needs it to decide whether a type
condition applies. Without it, every field behind an inline fragment or fragment
spread is dropped from the response.
Both branches implementing this (`is_original == false` in `apply_selection_set`)
were unreachable from the test suite. `is_original: false` appears exactly once,
in `filtered_defer_fragment`, whose filtered query is `{ a { b } }` — no
fragments, so neither branch can fire.
The new test filters a field out of `... on Foo` and asserts `__typename` survives
the filtered pass, then that `foo` survives the original pass, which it can only
do if `__typename` did. Asserting the intermediate state as well as the final one
means a failure says which of the two passes broke.
`query_for_test` extracts the `Query` construction so the new test does not
repeat the twenty lines `filtered_defer_fragment` uses to build one by hand.
Verified the test reaches the branch: setting `is_original: true` on the filtered
query drops `__typename` and fails the first assertion.
These three snapshots have no test function anywhere in the repo. They were added with the experimental response cache plugin and the test that produced them was removed later without them, so `cargo insta` has been carrying them as unreferenced files ever since. The path they used to cover — `CacheKeyMetadata` folded into the entity cache key by `response_cache::cache_key::hash_additional_data` — has no test now. Deleting the snapshots does not lose coverage, it stops the files from implying coverage that does not exist.
3ec0596 to
15bf899
Compare
…data path `plan_cache_is_segmented_by_authorization_metadata` inserted `CacheKeyMetadata` into the request context directly. Nothing in the router does that: `CachingQueryPlanner::plan` calls `AuthorizationPlugin::update_cache_key` first, which unconditionally overwrites the context's metadata with one derived from the request's JWT claims. The test only survived that overwrite because `Configuration::default()` plus `starstuff@current.graphql` (no authorization spec linked) left `enable_authorization_directives` false, so `update_cache_key` never ran — meaning the test pinned the derived `Hash`/`Eq` on `CachingQueryKey` and not the segmentation it claimed. Both cache tests now use a schema and configuration for which directives are enabled, and carry authorization state as JWT claims so `update_cache_key` produces the metadata. Removing that call now fails the test. `rejection_response_is_cached` fed a `QueryPlannerContent::Response` with no errors and used default metadata on both calls, so it observed neither a rejection nor authorization-dependent keying. It now uses the content shape `QueryPlannerService::get` returns for a whole-query rejection and adds a call in a different authorization state, pinning that a cached rejection is not served across authorization states. Two comments claimed more than their tests checked. `planning_unauthenticated_rejects_rather_than_returning_unfiltered_plan` stated an invariant about the plan cache while asserting on the planner service one layer below it, and passed `ComputeJobType::QueryPlanningWarmup` as though that selected a warm-up path — it only sets the compute-pool priority and the metric label, and `get` filters before reading it. `returns_http_200` credited the status to value completion nulling stripped selections, but the rejection short-circuits in the planner and `new_from_graphql_response` wraps it with `http::Response::new`, which is 200 regardless. Both now describe the code that actually runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`filtered_query_keeps_typename_for_type_conditions` hand-wrote both the original and the filtered query and hand-built the `Query` pair around them, so it pinned a shape that nothing guaranteed filtering produces. It now plans an unauthenticated request through `QueryPlannerService` and takes the pair from `plan.query`, which is where `filter_query` produces the filtered document and the planner sets `is_original = false`. The schema gains the `authenticated` spec and `Foo.secret` gains `@authenticated` so filtering has something to remove. Split into one test per fragment form. The single test covered an inline fragment and a fragment spread in one query, which pinned neither branch: `apply_selection_set` copies `__typename` in both the `InlineFragment` and `FragmentSpread` arms, so either copy alone kept both fields alive. Disabling one arm left the test passing. With one form per test, disabling either arm fails exactly the test that covers it, verified in both directions. `Thing` stays an interface because the copy only matters for abstract types: `apply_selection_set` derives `current_type` from the response `__typename` for interfaces and unions, and from the schema otherwise, so a concrete field type would resolve type conditions with or without the copy.
`ExecutionService` formats a filtered operation twice: the filtered query projects the
subgraph data onto the authorized shape, then the original query expands it to the
shape the client requested. Both properties matter and neither was named by a test.
Disabling the filtered pass entirely left 92 of the 93 authorization tests passing.
The only failure was a snapshot in `cache_key_metadata`, whose name points at cache
keys rather than at response filtering, and whose diff for the leak is a single line.
A refactor of the response path could drop that pass and return protected data with
the suite still green.
This test has a subgraph return `phone` even though filtering removed it from the
operation, so the data reaching formatting is wider than the query. Asserting
`Some(Value::Null)` covers both halves in one key: present means the original query
restored the requested shape, null rather than "1234" means the filtered query stripped
the value.
Verified against both mutations. Removing the filtered pass yields
`Some(String("1234"))`, a leak of an `@authenticated` field. Running the passes in the
other order yields `None`, dropping a requested field from the response and orphaning
the authorization error already recorded for its path.
goto-bus-stop
left a comment
goto-bus-stop
left a comment
There was a problem hiding this comment.
just dropping my original comments here, we'll see which are still relevant after the changes we already discussed
| /// The status comes from the short-circuit in the query planner, not from response | ||
| /// formatting: `filter_query` returns `Err(Unauthorized)`, `QueryPlannerService::get` | ||
| /// builds a `graphql::Response` with `data: null` directly, and | ||
| /// `SupergraphResponse::new_from_graphql_response` wraps it with `http::Response::new`, | ||
| /// which is 200 regardless of errors or data shape. Execution and value completion never | ||
| /// run — see `does_not_reach_execution`. 200 with errors is the right answer for a | ||
| /// rejection with field-error semantics, so this pins that the short-circuit does not | ||
| /// pick up an error status along the way. |
There was a problem hiding this comment.
I don't think this is the correct behaviour when reject_unauthorized is enabled, since we don't produce partial data in that case. I think we are required by GraphQL spec to have a request error instead of a field error, which we don't do today. Then the HTTP status code should be 4xx, not 200. Additionally, data should be omitted entirely, instead of being null.
| /// The status comes from the short-circuit in the query planner, not from response | |
| /// formatting: `filter_query` returns `Err(Unauthorized)`, `QueryPlannerService::get` | |
| /// builds a `graphql::Response` with `data: null` directly, and | |
| /// `SupergraphResponse::new_from_graphql_response` wraps it with `http::Response::new`, | |
| /// which is 200 regardless of errors or data shape. Execution and value completion never | |
| /// run — see `does_not_reach_execution`. 200 with errors is the right answer for a | |
| /// rejection with field-error semantics, so this pins that the short-circuit does not | |
| /// pick up an error status along the way. | |
| /// We historically treat authorization errors as field errors, even if the whole request is rejected. | |
| /// Hence we expect `null` data, auth errors, and status code 200. |
I think for the sake of encoding the current behaviour, we should also assert in this test that data is present but null (...or at least that it is null, I'm not sure we differentiate between absence and null yet).
I'll file a follow up ticket for spec compliance here I think.
There was a problem hiding this comment.
This went further than the comment after a team discussion. rejection_sends_null_data confirms we do differentiate null from absent — at wire level; deserializing into graphql::Response collapses the distinction, which is why the module asserts on bytes.
Since f40c739, an emptied operation without reject_unauthorized returns shaped data ({"data":{"orga":null},...}) like a partial filter — breaking changeset included. With reject_unauthorized the response stays 200/data: null, pinned by returns_http_200 and rejection_sends_null_data.
For your follow-up ticket: dev-docs/authorization-query-planning.md has the verified spec citations (request error ⇒ data absent; over-HTTP ⇒ 4xx MUST for application/graphql-response+json, 200 for legacy application/json) and the three coupled decisions — status by negotiated content type, path on errors with no result, and errors.response: disabled producing a spec-invalid response when data is absent.
… planner The refactor moves whole-query rejection out of the query planner into a layer on the execution service, and the response it produces has to stay byte-identical. Four properties of that response had no test. `data: null` is sent, rather than `data` being left out. GraphQL distinguishes the two: an absent `data` marks a request error, a null one marks a field error that propagated to the root. The existing snapshots cannot see this, because `into_graphql_response_stream` deserializes into `graphql::Response` first, where `data: Option<Value>` turns JSON `null` into `None` and then skips it on re-serialization. These tests read the wire bytes instead. Building the rejection without `data` leaves all three pre-existing rejection tests passing. `errors.response: disabled` and `errors.response: extensions` were only covered for partial filtering. `errors_in_extensions` sets no `reject_unauthorized`, and the `config_parsing` cases check that the options deserialize, not what they do. Both are now covered for a whole rejection: `disabled` leaves `data: null` as the client's only signal, `extensions` moves the errors to `extensions.authorizationErrors` and leaves `errors` out. `dry_run` combined with `reject_unauthorized` still rejects. `dry_run` reports the paths without modifying the operation, so a rejection there can only come from the config, never from an emptied document. Any change that keys rejection off the document being empty has to keep the config check independent, or `dry_run` silently stops enforcing. Each test was verified against a mutation that isolates it: omitting `data` fails all four, ignoring `ErrorLocation::Disabled` fails only the disabled test, and skipping the `reject_unauthorized` check under `dry_run` fails only the dry-run test.
Moving whole-query rejection out of the query planner turns the planner's output for
a rejected operation from `QueryPlannerContent::Response` into a plan, and
`QueryPlan::usage_reporting` is not optional, so that plan has to carry a
`UsageReporting`. Which variant it carries decides what the operation costs:
fn licensed_operation_count(usage_reporting: &UsageReporting) -> u64 {
match usage_reporting {
UsageReporting::Error(_) => 0,
_ => 1,
}
}
`UsageReporting::Error` is the natural-looking choice for an operation that produced
no plan, and it bills nothing. A rejected operation is billed as one licensed
operation today, because telemetry falls back to a count of 1 when the context holds
no `UsageReporting` at all, so choosing `Error` would drop rejected operations off
the bill. No test covered any of this: every snapshot asserting
`licensed_operation_count: 1` covers a successful operation.
These drive `update_apollo_metrics` over a context directly and assert the count for
all three cases: absent reporting bills 1, `Error` bills 0, and `Operation` bills 1.
The middle one exists to make the trap visible rather than to protect current
behaviour.
`apollo_reports.rs` was the obvious home, but its fixture schema has no authorization
directives, and the file documents itself as flaky with process-wide collectors. The
unit-level metrics tests could not host these either, since `get_metrics_for_request`
fixes the schema and configuration. Driving the reporting function directly needs
neither.
Each test body runs under `FutureMetricsExt::with_metrics`, which gives it a
task-local meter provider. Without it these emit through the global meter and
intermittently break `plugins::authorization::authenticated::tests` when both modules
run together; with it that combination passed five consecutive runs. Verified each
test still discriminates afterwards: changing the fallback to 0, `Error` to 1, or the
catch-all to 0 fails exactly one test each.
…udio Describes the one user-visible effect of moving authorization out of the query planner: an operation refused outright now reaches Studio with a signature, referenced fields, and per-type stats, where before it contributed only to the operation count. States the licensed operation count explicitly, since a change to what Studio receives invites the question of what it costs.
`filter_query` signalled a refused operation with `Err(QueryPlannerError::Unauthorized)`, so the query planner had to catch an error variant that meant success in order to build a 200 response from it. An authorization refusal is an outcome of filtering, not a failure of it. `filter_query` now returns `FilterResult`, which names its three outcomes: the operation is `Unchanged`, it is `Filtered` and carries the removed paths alongside the new document, or it is `Refused`. `Err` is left for the spec errors the filtering visitors raise. The query planner matches the three arms, which also drops the tuple that `clippy::type_complexity` needed a type alias to quieten. `QueryPlannerError::Unauthorized` had no handler beyond that one arm, so it goes with it. The response is untouched: `Refused` runs the same `UnauthorizedPaths` calls in the same order and returns the same `QueryPlannerContent::Response`.
Filtering removes `currentUser.phone`, emptying that selection, and `@skip(if: true)`
removes the only other root field. The operation still has a definition, so filtering
reports it as filtered rather than refused, and the plan comes back with no root node.
That plan shape is indistinguishable from a refused operation: no root node, and
unauthorized paths present. The responses differ though. This one carries a shaped
`data` of `{"currentUser": null}` alongside the authorization error, where a refused
operation carries `data: null`. Deciding refusal from the plan shape would answer this
operation as a refusal and change its `data`.
The test asserts no subgraph was called, which is what establishes that the plan held
no executable work; without it the snapshot alone would not show that this is the
colliding case.
The `Authorization error` event for a refused operation fires exactly once, inside the `query_planning` span, carrying the unauthorized paths. The unit tests cannot cover the span: it takes the telemetry plugin, which only joins the pipeline when OpenTelemetry is initialised for the process, so this spawns a real router and parses the JSON log's span list. Exactly-once is the assertion with teeth. The refusal is decided at a single place, so a second event for the same request means two code paths both believe they own the log.
The query planner answered a refused operation itself, returning `QueryPlannerContent::Response` instead of a plan. The planner now plans or errors, and the authorization plugin's execution-service layer answers refusals. `filter_query` loses the `reject_unauthorized` check entirely: that decision belongs to the layer, which receives the flag at plugin construction. `FilterResult::Refused` becomes `Emptied`, meaning only that filtering removed every selection. For an emptied operation the planner returns a plan with no root node, usage reporting generated from the original query with empty references (nothing resolves, so nothing is referenced), and `operation_emptied` set on `UnauthorizedPaths`, which rides the `Query` through the plan cache. The flag is not derivable downstream: a plan with no root node and non-empty paths also describes a partially filtered operation whose surviving selections are all statically skipped, pinned by `partial_filter_leaving_no_executable_work` as returning shaped data rather than a refusal. The layer is a `checkpoint_async` ahead of the existing authorization counter, so a refusal breaks before the counter fires and rejected operations stay uncounted, as they were when they never reached execution at all. It answers when the operation was emptied or when it holds `reject_unauthorized` and paths are present, with the same `data: null` response and `ErrorLocation` handling as before, byte-identical on the wire. Two observable changes, both in the changeset: - A refused operation reaches `CachingQueryPlanner` as a plan, so its usage reporting now lands in the context and Studio receives the operation signature. The licensed operation count stays 1: the reporting is `UsageReporting::Operation`, and telemetry previously billed the missing-reporting fallback at 1. - The `Authorization error` event moves from the `query_planning` span to `execution`, visible here as the assertion flip in the integration test. The unit harness cannot see the span (without process-wide OpenTelemetry init the telemetry plugin is absent and no `execution` span exists), so the unit helper now asserts the event fires exactly once and leaves the span to the integration test. The exactly-once assertion is load-bearing: the first version of this change logged in both the planner and the layer for an emptied operation, and nothing failed until that assertion existed. Its negative control (reintroducing the planner's log call) fails showing one event in `query_planning` and one in `execution`.
The authorization layer on the execution service answers refused operations, so no producer of `QueryPlannerContent::Response` remains. The supergraph service's forwarding arm, the cache's size estimation arm, and the variant itself go together. `QueryPlannerContent` keeps its single `Plan` variant rather than collapsing into `QueryPlan`: flattening the type ripples through every construction and match in the crate, and is mechanical enough to review separately. `rejection_response_is_cached` becomes `refused_operation_plan_is_cached`, feeding the shape `QueryPlannerService::get` now returns for a refusal: a plan with no root node and the query marked emptied. What it pins is unchanged — a refusal is cached, and stays keyed by authorization state. The remaining matches on the enum simplify to irrefutable bindings.
Assert the rejection by its error code rather than its message: the code is the contract, the wording is not. Reword the HTTP 200 doc along the lines review suggested: field-error treatment of a whole-operation refusal is historical, the spec arguably calls for a request error with a 4xx status and no `data` key, and `rejection_sends_null_data` pins the null-versus-absent distinction on the wire until spec compliance changes deliberately. The previous wording also described the pre-refactor mechanism, with the planner building the response. In the caching tests, derive the parsed document inside the request helper instead of taking it as a second argument that must agree with the query, name the helper for the authorization tests it serves, inline the one-argument planner wrapper, and cut the config helper's doc to one line.
…ield to match `filter_query` reports `Emptied` from a document-wide `definitions.is_empty()` check, so fully filtering the executed operation while a sibling operation shares the document reports `Filtered` instead. Planning then fails to find the executed operation, and the client receives 400 `GRAPHQL_UNKNOWN_OPERATION_NAME` with no mention of authorization, where the single-operation case receives the refusal response. The `FIXME`s in `filter_query` point at exactly this check; the new test pins the outcome so resolving them changes it deliberately. `operation_emptied` becomes `document_emptied`: the field records what the check measures, and the old name claimed the stronger per-operation meaning. The field doc gains the two counterexamples that make it non-derivable from the plan and the multi-operation case that bounds what it means.
| @@ -0,0 +1,9 @@ | |||
| ### Report operations rejected by authorization to Apollo Studio ([PR #9911](https://github.com/apollographql/router/pull/9911)) | |||
|
|
|||
| When authorization refuses an operation outright, Apollo Studio now receives it as an operation, identified by the signature of the query the client sent and carrying the client name, version, and request count. Studio previously received the operation count alone and had nothing to attribute it to. | |||
There was a problem hiding this comment.
When authorization refuses an operation outright,
Does this only apply when reject_unauthorized: true?
I think when authorization removes all fields, we should NOT describe this as a "refused" operation to users. It's an operation that was executed, but all field executions errored because the client did not have the required authorization.
Only when reject_unauthorized: true should we describe a request as "refused" outright. And I think we should mention that option whenever we talk about refusal, to clarify to users that the changeset is only relevant to them if they use that option.
There was a problem hiding this comment.
It applies to both cases, so reworded with your taxonomy in 6eb7282: "authorization raises errors for every field" for the directive case, "refuses" reserved for reject_unauthorized, which is now named wherever refusal comes up.
| @@ -0,0 +1,11 @@ | |||
| ### Fully unauthorized operations return the same response shape as partially unauthorized ones ([PR #9911](https://github.com/apollographql/router/pull/9911)) | |||
There was a problem hiding this comment.
We should frame this as a spec-compliance upgrade. Previously, we incorrectly returned data: null in some cases where we should return an object. I'll propose a new changeset after I'm done with the rest of the PR.
In particular, I think we should never mention "removed" fields to users, as this is an implementation detail of how we work around the query planner. To a user, such a field was not "removed", instead the field is executed but raised an error due to not being authenticated.
There was a problem hiding this comment.
Rewrote it along those lines in 6eb7282: spec-fix framing, "fields fail authorization" instead of "removed", and the reject_unauthorized carve-out named. Overwrite freely if you had different wording in mind.
`QueryPlannerContent` held one variant wrapping `Arc<QueryPlan>`, so every consumer paid a destructure for no information. It is now a type alias for `Arc<QueryPlan>`: signatures keep naming the planner's output as a single seam, so changing the payload touches one definition, while constructions and uses handle the plan directly. 41 lines of pattern matching collapse into direct use. The distributed plan cache serializes the cache value, so its wire shape loses the enum tag. Cache keys carry the crate version, and 2.17.0 is unreleased, so no released router shares the namespace this shape lands in.
goto-bus-stop
left a comment
goto-bus-stop
left a comment
There was a problem hiding this comment.
About halfway done with review, need to get off the train now though!!!
| let unauthorized = request.query_plan.query.unauthorized.clone(); | ||
| unauthorized.log_unauthorized_paths(); | ||
|
|
||
| let mut response = graphql::Response::builder().data(Value::Null).build(); |
There was a problem hiding this comment.
Let's clarify that this is technically not up to spec. Not a fan of including ticket references in general, but it might be helpful in this case, as it explains future work rather than describing work that's already done (which is what bothers me most when Claude sticks references in comments)
| let mut response = graphql::Response::builder().data(Value::Null).build(); | |
| // We knowingly build an invalid response here. Execution was prevented, | |
| // so we should respond with a request error and no data. Instead, we're | |
| // responding with execution errors and a fake/incorrect `data: null`. We | |
| // maintain backwards compatibility for the time being. | |
| // Tracked in ROUTER-2063. | |
| let mut response = graphql::Response::builder().data(Value::Null).build(); |
| /// The span the event belongs to is asserted by | ||
| /// `integration::telemetry::logging::test_authorization_error_event_in_execution_span`. | ||
| /// This harness runs without the telemetry plugin, so the `execution` span does not | ||
| /// exist here and the event lands directly in the `router` span. |
There was a problem hiding this comment.
I don't see anything in this test that relies on the span?
There was a problem hiding this comment.
Right — it asserts cardinality only. Dropped the span paragraph from the doc in 6e975af; the span assertions live in the integration tests.
| /// A subgraph can return more than the operation selected, so the data reaching response | ||
| /// formatting is not bounded by what authorization left in the query. `ExecutionService` | ||
| /// handles that by formatting twice: the filtered query projects the data onto the | ||
| /// authorized shape, then the original query expands it to the shape the client asked | ||
| /// for. | ||
| /// | ||
| /// `User.phone` is `@authenticated`, so filtering removes it from an unauthenticated | ||
| /// operation while this subgraph returns it anyway. `Some(Value::Null)` pins both halves | ||
| /// of that arrangement in one assertion: `phone` is present, so the original query | ||
| /// restored the requested shape, and it is null rather than `"1234"`, so the filtered | ||
| /// query stripped the value the client may not see. | ||
| /// | ||
| /// Removing either property changes this key: drop the filtered pass and it holds | ||
| /// `"1234"`, run the passes in the other order and it disappears from the response. |
There was a problem hiding this comment.
A subgraph can return more than the operation selected,
While technically true, this would be a misbehaviour from the subgraph and I don't think this is the most likely case. The name field could have an @requires(keys: "phone") and then we must ask for that field from the subgraph in order to resolve name, but we must not send the phone field to the client.
The rest of the comment is explaining so much about the behaviour that it's hard to understand what the actual point of the test is. The comments about filtering ordering would be useful to have at the point where we do the filtering though.
| /// A subgraph can return more than the operation selected, so the data reaching response | |
| /// formatting is not bounded by what authorization left in the query. `ExecutionService` | |
| /// handles that by formatting twice: the filtered query projects the data onto the | |
| /// authorized shape, then the original query expands it to the shape the client asked | |
| /// for. | |
| /// | |
| /// `User.phone` is `@authenticated`, so filtering removes it from an unauthenticated | |
| /// operation while this subgraph returns it anyway. `Some(Value::Null)` pins both halves | |
| /// of that arrangement in one assertion: `phone` is present, so the original query | |
| /// restored the requested shape, and it is null rather than `"1234"`, so the filtered | |
| /// query stripped the value the client may not see. | |
| /// | |
| /// Removing either property changes this key: drop the filtered pass and it holds | |
| /// `"1234"`, run the passes in the other order and it disappears from the response. | |
| /// When a subgraph response includes authenticated fields, because of @requires or because | |
| /// the subgraph is misbehaving, the authenticated field should not be propagated to the client. |
There was a problem hiding this comment.
Took your one-liner in 5f9f5c9, and the pass-ordering explanation moved to the two-pass site in services/execution/service.rs as suggested.
| assert_eq!( | ||
| current_user.get("phone"), | ||
| Some(&serde_json_bytes::Value::Null), | ||
| "the subgraph returned `phone` outside the filtered operation, so it must reach \ | ||
| the client as null rather than as its value" | ||
| ); |
There was a problem hiding this comment.
Suggest additionally asserting that we have an execution error for currentUser.phone. Since the behaviour that we want is that phone was nullified because of an error.
There was a problem hiding this comment.
Added in 5f9f5c9 — the error is already emitted today, path /currentUser/phone with UNAUTHORIZED_FIELD_OR_TYPE, so the assertion pins existing behaviour.
| /// Deserializing into [`graphql::Response`] collapses a JSON `null` under `data` | ||
| /// into an absent key, so assertions on whether `data` is present read the bytes. |
There was a problem hiding this comment.
Maybe something we should follow up on ... this could cause us to misparse responses from subgraphs today.
There was a problem hiding this comment.
Agreed — graphql::Response cannot represent data: null, so a subgraph's data: null deserializes the same as no data at all. Recorded in the dev-doc's outstanding-issues section (c867886) as follow-up material; happy to file the ticket.
|
|
||
| /// `data` is `null` and present. An absent `data` marks a request error; a null one | ||
| /// marks a field error that propagated to the root. The key's presence is part of | ||
| /// the response contract. |
There was a problem hiding this comment.
| /// the response contract. | |
| /// the response contract. | |
| /// This is not a spec-compliant behaviour, but is asserted for backwards compatibility. |
I'm tempted to suggest merging this into the returns_http_200 test?
There was a problem hiding this comment.
Merged into returns_http_200_with_null_data in 6e975af, spec-compliance note included.
| } | ||
|
|
||
| /// `errors.response: disabled` suppresses the authorization errors, so `data: null` | ||
| /// is the only thing telling the client the operation produced nothing. |
There was a problem hiding this comment.
wow, what a strange feature. "respond with unintelligible garbage please" 😛
| /// is the only thing telling the client the operation produced nothing. | |
| /// is the only thing telling the client the operation produced nothing. | |
| /// This is not a spec-compliant behaviour, but is asserted for backwards compatibility. |
There was a problem hiding this comment.
Added in 6e975af. ROUTER-2063 gets to decide what disabled even means once data is absent — an empty response object is spec-invalid, so the option conflicts with request-error semantics outright.
| } | ||
|
|
||
| /// `errors.response: extensions` moves the authorization errors under | ||
| /// `extensions.authorizationErrors` and leaves `errors` out of the response. |
There was a problem hiding this comment.
| /// `extensions.authorizationErrors` and leaves `errors` out of the response. | |
| /// `extensions.authorizationErrors` and leaves `errors` out of the response. | |
| /// This is not a spec-compliant behaviour, but is asserted for backwards compatibility. |
| --- | ||
| { | ||
| "data": { | ||
| "currentUser": null |
There was a problem hiding this comment.
Hmm, might need to think about this a bit. My intuition says that since the field is phone: String (i.e., it's nullable), we should actually have { currentUser: { phone: null } } as the data here. Unless currentUser was null in the subgraph, then we should have { currentUser: null }. This might get at a more fundamental GraphQL correctness problem with query filtering right now. Let's at least not address it in this PR
There was a problem hiding this comment.
Agreed, and left alone here as you say. Your intuition matches the spec's field-error semantics: phone: String erroring should yield {"currentUser": {"phone": null}}, and {"currentUser": null} should only appear when propagation forces it. The parent-nulling comes from filtering removing phone from the operation entirely, so formatting never sees a position to null. Recorded in the dev-doc's outstanding-issues section (c867886) so it doesn't get lost — it likely belongs alongside ROUTER-2063 since both are "filtering changes the shape the client observes".
| /// plan and the operation proceeds as a plan with no work. A fully filtered | ||
| /// operation in a document that other operations keep non-empty is reported as | ||
| /// `Filtered`. | ||
| Emptied { paths: Vec<Path> }, |
There was a problem hiding this comment.
I'm not totally convinced this is a useful distinction from Filtered. Why would we not have a Filtered branch with an empty document?
There was a problem hiding this comment.
Fair — the document already carries the fact. Collapsed in e2eb4f0: filtering returns Filtered with the empty document, and the planner's arm matches on document.definitions.is_empty() (the federation planner can't plan an empty document, so that arm still answers with a workless plan).
|
|
||
| ServiceBuilder::new() | ||
| // Ahead of the counter below, so a refused operation stays uncounted. | ||
| .checkpoint_async(move |request: execution::Request| async move { |
There was a problem hiding this comment.
This came up during a claude code-review, it seems like an unintended change:
Moving the reject_unauthorized refusal from the query planner to the execution service puts three supergraph-level gates ahead of it, so an unauthorized operation can be answered with an accept-header or variable-validation error instead of the authorization response.
Before this change, filter_query returned QueryPlannerError::Unauthorized and service_call answered from the (now-deleted) QueryPlannerContent::Response arm, which sat above the Plan arm and skipped everything in it. Now every case reaches the Plan arm in services/supergraph/service.rs:272, where is_subscription/is_deferred (406) and plan.query.validate_variables (400) run before execution_service.call. Concretely: subscription { secretFeed } with secretFeed marked @authenticated, an unauthenticated client, reject_unauthorized: true, POSTed with Accept: application/json. Filtering empties the document, so FilterResult::Emptied yields a plan with root: None, but QueryPlan::is_subscription() reads query.operation.kind() (plan.rs:81), not root, so it is still true. The router replies 406 SUBSCRIPTION_BAD_HEADER — telling the caller to fix its Accept header for an operation it is not allowed to run — and this checkpoint never executes, so no Authorization error is logged and no UNAUTHORIZED_FIELD_OR_TYPE error is returned. Previously the same request got 200 + UNAUTHORIZED_FIELD_OR_TYPE. The same substitution happens for a partially filtered @defer operation without multipart/mixed (406 DEFER_BAD_HEADER) and for an unauthorized operation with a missing/invalid required variable (400 variable-validation errors). None of these are covered by the new tests or mentioned in either changeset.
There was a problem hiding this comment.
Yeah, we should call it out in a changeset, it's a correct change in behaviour given that authorization enforcement conceptually moves to execution regardless of the reject_unauthorized setting.
There was a problem hiding this comment.
Verified on the wire: the variable case returns 400 VALIDATION_INVALID_TYPE_VARIABLE with no authorization error where it previously returned the refusal. b8d9c8a adds the changeset (breaking_bryn_router_1973_gate_ordering) and pins the variable case with variable_validation_answers_before_the_refusal. The subscription and @defer cases are unpinned — they need a schema with subscriptions — and are noted in the dev-doc's outstanding-issues section.
| format: json | ||
| authorization: | ||
| directives: | ||
| enabled: true |
There was a problem hiding this comment.
Should reject_unauthorized: true be here? The check runs in the checkpoint_async of execution_service in apollo-router/src/plugins/authorization/mod.rs for log_unauthorized_paths to be called, which the new test test_authorization_error_event_in_execution_span is trying to verify.
Full code-review report:
With only authorization.directives.enabled: true, { me { name } } is emptied, the planner returns a plan with root: None, and the Authorization error event comes from ExecutionService::process_graphql_response, which explicitly wraps it in execution_span.in_scope(...) (services/execution/service.rs:238-286) — code this PR did not touch. The new log_unauthorized_paths() call inside AuthorizationPlugin::execution_service's checkpoint_async (mod.rs:612) is never reached, because reject_unauthorized is false. So the assertion the test makes (exactly one event, under the execution span) is satisfied entirely by the old path. Deleting the log_unauthorized_paths() line from the checkpoint, or adding a second log there, leaves this test green; the only test covering the refusal log, assert_logs_contain_entire_request_authorization_error, deliberately asserts nothing about the span. Adding reject_unauthorized: true to the fixture (and keeping a second case without it) would cover both.
There was a problem hiding this comment.
Confirmed — the fixture never reached the layer's log. Fixed in 20aab33 exactly as you suggest: this fixture gains reject_unauthorized: true, a second fixture without it covers the response-formatting emitter, and both cases share one assertion helper. Verified the isolation: deleting the layer's log_unauthorized_paths() fails the reject case and leaves the no-reject case green.
Filtering that removes every definition returns `Filtered` with the empty document, and the planner's empty-document arm matches on `document.definitions.is_empty()`. The variant duplicated a fact the document already carries.
Review-suggested wording. Execution was prevented, so the spec calls for a request error with no data; the layer answers with execution errors and `data: null` for backwards compatibility. ROUTER-2063 tracks the compliance work.
The error is already emitted, path `/currentUser/phone` with `UNAUTHORIZED_FIELD_OR_TYPE`, so the assertion pins existing behaviour: the field is null because it errored, not merely stripped. The test doc shrinks to the observable contract, and the two-pass ordering explanation moves to the formatting site in the execution service, where the ordering is enforced.
The status and null-data assertions merge into `returns_http_200_with_null_data`, the refusal, disabled, and extensions docs note that the behaviour is asserted for backwards compatibility rather than spec compliance, and the exactly-once helper's doc stops discussing spans it does not assert.
Variable validation and the subscription and @defer accept-header checks run in the supergraph service, before the execution layer refuses, so an operation failing both receives the validation error alone. Measured for the variable case: 400 VALIDATION_INVALID_TYPE_VARIABLE with no data key and no authorization error, where the refusal previously answered. A breaking changeset describes the ordering. The subscription and @defer cases carry no pin: they need a schema with subscriptions.
The fixture enabled directives without reject_unauthorized, so the operation ran its empty plan and response formatting emitted the event; the layer's emitter went unexercised, and deleting its log call left the test green. The fixture now sets reject_unauthorized so the layer answers and logs, and a second fixture without it covers the response-formatting emitter. Both cases assert the event lands under the execution span exactly once. Deleting the layer's log call fails exactly the reject case.
Refusal is reserved for reject_unauthorized and named wherever it comes up; the directive case reads as authorization erroring every field rather than fields being removed, since removal is how the router works around the planner, not what the client experiences; and the shaped-data change is framed as the spec fix it is.
Gate ordering, the graphql::Response null-collapse and its subgraph-response implication, the partial-filter parent-nulling question, and the ROUTER-2063 scope, alongside the FilterResult references catching up with the Emptied collapse.
goto-bus-stop
left a comment
goto-bus-stop
left a comment
There was a problem hiding this comment.
Partial review 2 out of 3 😁
| let doc = Query::parse_document(query, None, &schema, &configuration).unwrap(); | ||
|
|
||
| let content = planner | ||
| .get( |
There was a problem hiding this comment.
Can this test use the public .call API instead of the internal .get() API?
The comment at the top of this test is very unclear on what the actual purpose of the test is. I think it's just: if an unauthenticated client requests only fields requiring authentication, then we expect an empty query plan + all fields in .unauthorized.paths? Warmup and caching are both not relevant to that. (Sure, PQs are always unauthenticated in warmup, but that doesn't matter here at all)
There was a problem hiding this comment.
Yes, that's the whole claim. Rewritten in 0ec06b8: doc states exactly that, the test calls the planner as a tower service instead of the private get, and the warm-up/caching framing is gone. Renamed fully_unauthorized_operation_plans_no_work.
| /// `Thing` is an interface, so `apply_selection_set` takes the concrete type from the | ||
| /// response `__typename` rather than from the schema. The filtered pass has to copy | ||
| /// `__typename` into its output for the original pass to resolve the type condition on | ||
| /// `... on Foo`, so `inline` survives only if the copy happened. | ||
| /// | ||
| /// One fragment form per test: each form copies `__typename` independently, so a query | ||
| /// carrying both keeps its fields alive when either copy runs. | ||
| #[tokio::test] | ||
| async fn filtered_query_keeps_typename_for_inline_fragment() { |
There was a problem hiding this comment.
Okay, it took a while but I get it now. This is testing that the double-response-formatting produces the correct result for fragment spreads on abstract (interface/union) types, which rely on the __typename returned from the subgraph.
I think that leading with "__typename must be copied" is distracting and not what's actually important. This made it very confusing for me (I spent about 15 minutes reviewing and researching just these two tests).
I'd suggest the below, to first state the goal of the test, then why it's not self-evident, and with a rename that doesn't focus on __typename propagation:
| /// `Thing` is an interface, so `apply_selection_set` takes the concrete type from the | |
| /// response `__typename` rather than from the schema. The filtered pass has to copy | |
| /// `__typename` into its output for the original pass to resolve the type condition on | |
| /// `... on Foo`, so `inline` survives only if the copy happened. | |
| /// | |
| /// One fragment form per test: each form copies `__typename` independently, so a query | |
| /// carrying both keeps its fields alive when either copy runs. | |
| #[tokio::test] | |
| async fn filtered_query_keeps_typename_for_inline_fragment() { | |
| /// Double-applying response formatting on an abstract type should apply the | |
| /// correct fragment spread. | |
| /// `__typename` is not selected by the input query, but internally we have to | |
| /// pass it along to the final formatting pass, else it wouldn't know which concrete | |
| /// type it's working on. | |
| #[tokio::test] | |
| async fn filtered_inline_fragment_on_abstract_type() { |
There was a problem hiding this comment.
took your doc and name in 95980bf, with the spread counterpart renamed to match (filtered_fragment_spread_on_abstract_type). Kept one trailing line explaining why the forms are split across two tests: each passes __typename along independently, so a query carrying both stays green when either does.
…on and tests `Query::format_response_filtered_then_original` runs the filtered pass, when one exists, then the original, and returns the nullified paths from both. The execution service and the spec tests call it instead of each sequencing the two `format_response` calls, so the ordering the passes depend on lives in one place.
The test asserts one thing: an unauthenticated client requesting only fields that require authentication gets a plan with nothing to execute and every requested field in `unauthorized.paths`. It now says so, calls the planner as a tower service instead of the private `get`, and drops the warm-up and caching framing, which were incidental to that claim. Renamed `fully_unauthorized_operation_plans_no_work`.
Review-suggested doc and name: the tests verify that double-applied response formatting resolves fragment spreads on abstract types correctly, and the internal `__typename` hand-off is the mechanism rather than the point. Renamed `filtered_inline_fragment_on_abstract_type` and `filtered_fragment_spread_on_abstract_type`.
goto-bus-stop
left a comment
goto-bus-stop
left a comment
There was a problem hiding this comment.
I've only skimmed the last few tests, I could probably come up with some similar nits if I tried, but I am struggling a bit to keep focus and they don't seem incorrect, so I'm good to land them right now.
Lets rename the PR/commit as we no longer intend to separate auth from query planning and we changed behaviour here
I'm glad you picked this up, doing the reject_unauthorized enforcement at execution direction was a great idea and I think we all learned a lot from finally doing a proper deep dive on auth!
An operation whose every field fails authorization now returns each requested root field as null rather than `data: null`, so these assert `data.secure` and `data.private` are null instead of asserting on `data` itself. Each test's error placement assertions are unchanged, and the docs name the field rather than the whole response.
The cached value serializes as `{"Ok": <QueryPlan>}` now that
`QueryPlannerContent` is a type alias, so the `Plan` and `plan` levels are gone from
the JSON path. The header's sample value is elided rather than restated: the
instructions above it are about finding the key, and a full value transcript goes
stale on every schema or field change.
BrynCooke
changed the title
BrynCooke
changed the title
|
Tick the box to add this pull request to the merge queue (same as
|
3 participants
Authorization no longer decides responses inside the query planner. The planner returns a query plan or an error. Authorization filters the operation as before, and a layer on the execution service applies
reject_unauthorizedand answers refused operations.BREAKING: when every field in an operation fails authorization,
datais an object with each requested root field set tonull, plus one error per field.BREAKING: the router validates the request before authorization runs. An operation with a missing required variable gets
400 VALIDATION_INVALID_TYPE_VARIABLE, and a subscription sent withoutAccept: multipart/mixedgets406 SUBSCRIPTION_BAD_HEADER. Both previously returned200with the authorization errors.Operations that fail authorization now reach Apollo Studio with their operation signature. The
Authorization errorlog event moves from thequery_planningspan to underexecution.Tests came first: the response shape, the
errors.responsemodes, the HTTP status, billing, and the two response-formatting passes were all pinned before the code moved.dev-docs/authorization-query-planning.mdhas the before and after, plus the questions we left open, including whether a refused operation should be a GraphQL request error (ROUTER-2063).