diff --git a/.changesets/breaking_bryn_router_1973_emptied_operations_shaped_data.md b/.changesets/breaking_bryn_router_1973_emptied_operations_shaped_data.md new file mode 100644 index 0000000000..420060c18b --- /dev/null +++ b/.changesets/breaking_bryn_router_1973_emptied_operations_shaped_data.md @@ -0,0 +1,13 @@ +### Operations with only authorization errors return spec-compliant data ([PR #9911](https://github.com/apollographql/router/pull/9911)) + +When every field in an operation fails authorization, the response now carries each requested root field as `null` alongside an error for each field, the same shape clients receive when some fields fail: + +```json +{"data": {"orga": null}, "errors": [{"message": "Unauthorized field or type", "path": ["orga", "id"], "extensions": {"code": "UNAUTHORIZED_FIELD_OR_TYPE"}}]} +``` + +The router previously returned `"data": null` here, incorrectly reporting that execution never produced a result. Clients that detect this case by checking `data` for `null` should check for errors with the `UNAUTHORIZED_FIELD_OR_TYPE` code instead, which covers partial failures as well. + +`authorization.directives.reject_unauthorized` keeps returning `"data": null`; spec compliance for refused operations is tracked separately. + +By [@BrynCooke](https://github.com/BrynCooke) in https://github.com/apollographql/router/pull/9911 diff --git a/.changesets/breaking_bryn_router_1973_gate_ordering.md b/.changesets/breaking_bryn_router_1973_gate_ordering.md new file mode 100644 index 0000000000..d0cfef55cc --- /dev/null +++ b/.changesets/breaking_bryn_router_1973_gate_ordering.md @@ -0,0 +1,5 @@ +### Request validation errors answer before authorization errors ([PR #9911](https://github.com/apollographql/router/pull/9911)) + +Authorization enforcement runs at execution, after the router validates the request. An operation that fails both checks now receives the validation error alone: a missing or invalid variable returns the 400 validation response, and a subscription or `@defer` operation sent without the matching `Accept` header returns the 406, where these previously received the authorization errors. Fixing the request then surfaces the authorization errors. + +By [@BrynCooke](https://github.com/BrynCooke) in https://github.com/apollographql/router/pull/9911 diff --git a/.changesets/fix_bryn_router_1973_report_rejected_operations.md b/.changesets/fix_bryn_router_1973_report_rejected_operations.md new file mode 100644 index 0000000000..88cdd4b9fa --- /dev/null +++ b/.changesets/fix_bryn_router_1973_report_rejected_operations.md @@ -0,0 +1,9 @@ +### Report operations whose fields all fail authorization to Apollo Studio ([PR #9911](https://github.com/apollographql/router/pull/9911)) + +When authorization raises errors for every field in an operation, or `authorization.directives.reject_unauthorized` refuses the 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. + +Such an operation counts as one licensed operation. + +The `Authorization error` log event for these operations now appears under the `execution` span instead of inside `query_planning`. Update log or trace filters that match this event by span name. + +By [@BrynCooke](https://github.com/BrynCooke) in https://github.com/apollographql/router/pull/9911 diff --git a/apollo-router/src/batching/query_plan_analysis_layer.rs b/apollo-router/src/batching/query_plan_analysis_layer.rs index 4fc25982b0..bc1898c3a2 100644 --- a/apollo-router/src/batching/query_plan_analysis_layer.rs +++ b/apollo-router/src/batching/query_plan_analysis_layer.rs @@ -215,7 +215,6 @@ mod tests { use crate::graphql; use crate::query_planner::QueryPlan; use crate::query_planner::QueryPlannerService; - use crate::services::QueryPlannerContent; use crate::services::QueryPlannerRequest; use crate::services::execution; use crate::spec::Query; @@ -228,9 +227,7 @@ mod tests { ) -> Arc { let document = Query::parse_document(query, None, &schema, &configuration).unwrap(); - let QueryPlannerContent::Plan { - plan: query_plan, .. - } = QueryPlannerService::for_test(schema, configuration) + QueryPlannerService::for_test(schema, configuration) .unwrap() .oneshot( QueryPlannerRequest::builder() @@ -245,11 +242,6 @@ mod tests { .unwrap() .content .unwrap() - else { - panic!("unexpected query planner output"); - }; - - query_plan } #[tokio::test] diff --git a/apollo-router/src/error.rs b/apollo-router/src/error.rs index c8cbf74d4b..629ae6dd88 100644 --- a/apollo-router/src/error.rs +++ b/apollo-router/src/error.rs @@ -271,10 +271,6 @@ pub(crate) enum QueryPlannerError { /// spec error: {0} SpecError(SpecError), - // Safe to cache because user scopes and policies are included in the cache key. - /// Unauthorized field or type - Unauthorized(Vec), - /// Federation error: {0} FederationError(FederationErrorBridge), diff --git a/apollo-router/src/plugins/authorization/mod.rs b/apollo-router/src/plugins/authorization/mod.rs index c5bf4b6cf1..0d58da7698 100644 --- a/apollo-router/src/plugins/authorization/mod.rs +++ b/apollo-router/src/plugins/authorization/mod.rs @@ -34,7 +34,6 @@ use crate::layers::ServiceBuilderExt; use crate::plugin::Plugin; use crate::plugin::PluginInit; use crate::plugins::authentication; -use crate::query_planner::FilteredQuery; use crate::query_planner::QueryKey; use crate::services::execution; use crate::services::supergraph; @@ -138,6 +137,18 @@ pub(crate) struct UnauthorizedPaths { pub(crate) errors: ErrorConfig, } +/// What [`AuthorizationPlugin::filter_query`] did to an operation. +pub(crate) enum FilterResult { + /// The operation asks for nothing the request lacks authorization for. + Unchanged, + /// `document` is the operation with `paths` removed. Filtering can empty the + /// document entirely, leaving no definitions. + Filtered { + paths: Vec, + document: ast::Document, + }, +} + impl UnauthorizedPaths { pub(crate) fn log_unauthorized_paths(&self) { // nothing to do if we have no paths or we're not supposed to log @@ -194,6 +205,7 @@ fn default_enable_directives() -> bool { pub(crate) struct AuthorizationPlugin { require_authentication: bool, + reject_unauthorized: bool, } impl AuthorizationPlugin { @@ -337,8 +349,7 @@ impl AuthorizationPlugin { configuration: &Conf, key: &QueryKey, schema: &Schema, - ) -> Result, QueryPlannerError> { - let reject_unauthorized = configuration.directives.reject_unauthorized; + ) -> Result { let dry_run = configuration.directives.dry_run; // The filtered query will then be used @@ -366,7 +377,10 @@ impl AuthorizationPlugin { // FIXME: consider only `filtered_doc.operations.get(key.operation_name)`? if filtered_doc.definitions.is_empty() { - return Err(QueryPlannerError::Unauthorized(unauthorized_paths)); + return Ok(FilterResult::Filtered { + paths: unauthorized_paths, + document: filtered_doc, + }); } is_filtered = true; @@ -384,7 +398,10 @@ impl AuthorizationPlugin { // FIXME: consider only `filtered_doc.operations.get(key.operation_name)`? if filtered_doc.definitions.is_empty() { - return Err(QueryPlannerError::Unauthorized(unauthorized_paths)); + return Ok(FilterResult::Filtered { + paths: unauthorized_paths, + document: filtered_doc, + }); } is_filtered = true; @@ -402,7 +419,10 @@ impl AuthorizationPlugin { // FIXME: consider only `filtered_doc.operations.get(key.operation_name)`? if filtered_doc.definitions.is_empty() { - return Err(QueryPlannerError::Unauthorized(unauthorized_paths)); + return Ok(FilterResult::Filtered { + paths: unauthorized_paths, + document: filtered_doc, + }); } is_filtered = true; @@ -411,14 +431,13 @@ impl AuthorizationPlugin { } }; - if reject_unauthorized && !unauthorized_paths.is_empty() { - return Err(QueryPlannerError::Unauthorized(unauthorized_paths)); - } - if is_filtered { - Ok(Some((unauthorized_paths, doc))) + Ok(FilterResult::Filtered { + paths: unauthorized_paths, + document: doc, + }) } else { - Ok(None) + Ok(FilterResult::Unchanged) } } @@ -542,6 +561,7 @@ impl Plugin for AuthorizationPlugin { async fn new(init: PluginInit) -> Result { Ok(AuthorizationPlugin { require_authentication: init.config.require_authentication, + reject_unauthorized: init.config.directives.reject_unauthorized, }) } @@ -580,7 +600,30 @@ impl Plugin for AuthorizationPlugin { } fn execution_service(&self, service: execution::BoxCloneService) -> execution::BoxCloneService { + let reject_unauthorized = self.reject_unauthorized; + ServiceBuilder::new() + // Ahead of the counter below, so a refused operation stays uncounted. + .checkpoint_async(move |request: execution::Request| async move { + if reject_unauthorized && !request.query_plan.query.unauthorized.paths.is_empty() { + let unauthorized = request.query_plan.query.unauthorized.clone(); + unauthorized.log_unauthorized_paths(); + + // 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(); + unauthorized.update_response_with_unauthorized_path_errors(&mut response); + + return Ok(ControlFlow::Break( + execution::Response::new_from_graphql_response(response, request.context), + )); + } + + Ok(ControlFlow::Continue(request)) + }) .map_request(|request: execution::Request| { let filtered = !request.query_plan.query.unauthorized.paths.is_empty(); let needs_authenticated = request.context.contains_key(AUTHENTICATION_REQUIRED_KEY); diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__partial_filter_leaving_no_executable_work.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__partial_filter_leaving_no_executable_work.snap new file mode 100644 index 0000000000..6cc5deb9ed --- /dev/null +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__partial_filter_leaving_no_executable_work.snap @@ -0,0 +1,21 @@ +--- +source: apollo-router/src/plugins/authorization/tests.rs +expression: body +--- +{ + "data": { + "currentUser": null + }, + "errors": [ + { + "message": "Unauthorized field or type", + "path": [ + "currentUser", + "phone" + ], + "extensions": { + "code": "UNAUTHORIZED_FIELD_OR_TYPE" + } + } + ] +} diff --git a/apollo-router/src/plugins/authorization/tests.rs b/apollo-router/src/plugins/authorization/tests.rs index 5199988103..72bc0c698c 100644 --- a/apollo-router/src/plugins/authorization/tests.rs +++ b/apollo-router/src/plugins/authorization/tests.rs @@ -12,6 +12,7 @@ use tower::ServiceExt; use crate::Context; use crate::MockedSubgraphs; use crate::TestHarness; +use crate::apollo_studio_interop::UsageReporting; use crate::graphql; use crate::plugin::test::MockSubgraph; use crate::plugins::authentication::APOLLO_AUTHENTICATION_JWT_CLAIMS; @@ -42,8 +43,27 @@ fn assert_span_contains_authorization_error_event(span: &str) { assert!(contains_err_event_in_span.is_ok()); } +/// Asserts the `Authorization error` event for a refused operation was logged exactly +/// once. One place decides a refusal, so a second event means two code paths both +/// believe they own the log. fn assert_logs_contain_entire_request_authorization_error() { - assert_span_contains_authorization_error_event("query_planning"); + let event_regex = + Regex::new(r"ERROR .*Authorization error unauthorized_query_paths=\[.*]$").unwrap(); + + let exactly_one = tracing_test::logs_assert(|lines| { + match lines + .iter() + .filter(|line| event_regex.captures(line).is_some()) + .count() + { + 1 => Ok(()), + n => Err(format!( + "expected exactly one authorization error event, found {n}:\n{}", + lines.join("\n") + )), + } + }); + assert!(exactly_one.is_ok(), "{exactly_one:?}"); } fn assert_logs_contain_partial_authorization_error() { @@ -452,6 +472,561 @@ async fn authenticated_directive_reject_unauthorized() { assert_logs_contain_entire_request_authorization_error(); } +/// 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. +#[tokio::test] +async fn overfetched_unauthorized_field_is_not_returned() { + let service = TestHarness::builder() + .configuration_json(serde_json::json!({ + "authorization": { "directives": { "enabled": true } } + })) + .unwrap() + .schema(AUTHENTICATED_SCHEMA) + .subgraph_hook(|_name, _service| { + let (mock, mut handle) = + tower_test::mock::pair::(); + tokio::spawn(async move { + while let Some((req, responder)) = handle.next_request().await { + // `phone` is not in the filtered operation this subgraph was sent. + responder.send_response( + subgraph::Response::fake_builder() + .context(req.context) + .data(serde_json::json! {{ + "currentUser": { "name": "Ada", "phone": "1234" } + }}) + .build(), + ); + } + }); + mock.boxed_clone() + }) + .build_supergraph() + .await + .unwrap(); + + let request = supergraph::Request::fake_builder() + .query("query { currentUser { name phone } }") + .context(Context::new()) + .build() + .unwrap(); + let response = service + .oneshot(request) + .await + .unwrap() + .next_response() + .await + .unwrap(); + + let current_user = response + .data + .as_ref() + .expect("the operation kept `name`, so it must not reject outright") + .get("currentUser") + .expect("`currentUser` must survive; only `phone` is @authenticated"); + + 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" + ); + assert_eq!(current_user.get("name"), Some(&json!("Ada"))); + + // `phone` is null because it errored, and the error names it. + let phone_error = response + .errors + .iter() + .find(|error| { + error.path.as_ref().map(ToString::to_string).as_deref() == Some("/currentUser/phone") + }) + .expect("an execution error nullified `phone`"); + assert_eq!( + phone_error.extensions.get("code"), + Some(&json!("UNAUTHORIZED_FIELD_OR_TYPE")) + ); +} + +mod whole_operation_authorization { + //! Outcomes for operations that authorization affects as a whole, asserted on the + //! bytes the router sends. + + use super::*; + + /// `Organization.id` and `User.phone` are both `@authenticated`, so filtering + /// removes paths from an unauthenticated request, and `reject_unauthorized` turns + /// that into a whole-operation refusal. + const REJECTED_QUERY: &str = "query { orga(id: 1) { id creatorUser { id name phone } } }"; + + /// `A` asks for nothing unauthorized; `B` asks for `orga.id`, which is + /// `@authenticated`. Tests execute `A`. + const MULTI_OP_QUERY: &str = "query A { currentUser { name } } query B { orga(id: 1) { id } }"; + + type SubgraphHandles = + Arc>>>; + + /// Builds a router with the given `directives` config, replacing every subgraph + /// with a `tower_test` mock. The mocks hold no canned responses, so reaching one + /// fails the test. + async fn router_with_unresponsive_subgraphs( + directives: serde_json::Value, + ) -> (router::BoxCloneService, SubgraphHandles) { + let handles: SubgraphHandles = Arc::new(Mutex::new(Vec::new())); + let handles_clone = handles.clone(); + + let service = TestHarness::builder() + .configuration_json(serde_json::json!({ + "authorization": { "directives": directives } + })) + .unwrap() + .schema(AUTHENTICATED_SCHEMA) + .subgraph_hook(move |_name, _service| { + let (mock, handle) = + tower_test::mock::pair::(); + handles_clone.lock().unwrap().push(handle); + mock.boxed_clone() + }) + .build_router() + .await + .unwrap(); + + (service, handles) + } + + /// A router configured to refuse `REJECTED_QUERY`. + async fn rejecting_router() -> (router::BoxCloneService, SubgraphHandles) { + router_with_unresponsive_subgraphs(serde_json::json!({ + "enabled": true, + "reject_unauthorized": true + })) + .await + } + + /// Fails if any subgraph mock received a request, or if the router built no + /// subgraph service at all, which would make the check vacuous. + async fn assert_no_subgraph_calls(handles: SubgraphHandles) { + let handles: Vec<_> = handles.lock().unwrap().drain(..).collect(); + assert!(!handles.is_empty(), "no subgraph services were created"); + for handle in handles { + crate::plugin::test::assert_no_mock_calls(handle).await; + } + } + + fn graphql_post( + query: &str, + operation_name: Option<&str>, + context: Context, + ) -> router::Request { + let req = graphql::Request { + query: Some(query.to_string()), + operation_name: operation_name.map(str::to_string), + ..Default::default() + }; + router::Request { + context, + router_request: http::Request::builder() + .method("POST") + .header(CONTENT_TYPE, "application/json") + .header(ACCEPT, "application/json") + .body(body::from_bytes(serde_json::to_vec(&req).unwrap())) + .unwrap(), + } + } + + /// Sends the request and parses the body as it goes on the wire. + /// + /// Deserializing into [`graphql::Response`] collapses a JSON `null` under `data` + /// into an absent key, so assertions on whether `data` is present read the bytes. + async fn wire_response( + service: router::BoxCloneService, + request: router::Request, + ) -> (http::StatusCode, serde_json::Value) { + let response = service.oneshot(request).await.unwrap(); + let status = response.response.status(); + let bytes = body::into_bytes(response.response.into_body()) + .await + .unwrap(); + (status, serde_json::from_slice(&bytes).unwrap()) + } + + /// Sends `REJECTED_QUERY`, asserts the router refused it on authorization grounds, + /// and returns the HTTP status and wire body. + async fn send_rejected_request( + service: router::BoxCloneService, + context: Context, + ) -> (http::StatusCode, serde_json::Value) { + let (status, body) = + wire_response(service, graphql_post(REJECTED_QUERY, None, context)).await; + assert_eq!( + body.pointer("/errors/0/extensions/code"), + Some(&serde_json::json!("UNAUTHORIZED_FIELD_OR_TYPE")), + "the operation was not refused on authorization grounds: {body}" + ); + (status, body) + } + + /// Supergraph-level gates run before the execution service, so variable validation + /// answers an unauthorized operation ahead of the authorization refusal. The client + /// fixes the variable and then receives the refusal. The same ordering applies to + /// the accept-header checks for subscriptions and `@defer`. + #[tokio::test] + async fn variable_validation_answers_before_the_refusal() { + let (service, _handles) = rejecting_router().await; + + let (status, body) = wire_response( + service, + graphql_post( + // `orga.id` is `@authenticated`, and `$id` is required but not provided. + "query($id: ID!) { orga(id: $id) { id } }", + None, + Context::new(), + ), + ) + .await; + + assert_eq!(status, http::StatusCode::BAD_REQUEST); + assert_eq!( + body.pointer("/errors/0/extensions/code"), + Some(&serde_json::json!("VALIDATION_INVALID_TYPE_VARIABLE")), + "body: {body}" + ); + assert_eq!(body.pointer("/data"), None, "body: {body}"); + } + + #[tokio::test] + async fn does_not_reach_execution() { + let (service, handles) = rejecting_router().await; + + send_rejected_request(service, Context::new()).await; + + assert_no_subgraph_calls(handles).await; + } + + /// A refusal answers as a field error: HTTP 200 with a present-but-`null` `data` and + /// the authorization errors, not as a GraphQL request error, which carries a 4xx + /// status and no `data` entry. + /// This is not a spec-compliant behaviour, but is asserted for backwards compatibility. + #[tokio::test] + async fn returns_http_200_with_null_data() { + let (service, _handles) = rejecting_router().await; + + let (status, body) = send_rejected_request(service, Context::new()).await; + + assert_eq!(status, http::StatusCode::OK); + assert_eq!(body.get("data"), Some(&serde_json::Value::Null)); + } + + /// A refused operation reaches `CachingQueryPlanner` as a plan, so its usage + /// reporting lands in the context like any other operation's and Studio can + /// attribute the refusal to an operation signature. + /// `licensed_operation_count_tests` holds what the report bills. + #[tokio::test] + async fn records_usage_reporting() { + let (service, _handles) = rejecting_router().await; + let context = Context::new(); + + send_rejected_request(service, context.clone()).await; + + let usage_reporting = context + .extensions() + .with_lock(|lock| lock.get::>().cloned()) + .expect("a refused operation records usage reporting"); + assert!( + matches!(*usage_reporting, UsageReporting::Operation(_)), + "the report must carry operation details, not an error key: {usage_reporting:?}" + ); + } + + /// `errors.response: disabled` suppresses the authorization errors, so `data: null` + /// is the only thing telling the client the operation produced nothing. + /// This is not a spec-compliant behaviour, but is asserted for backwards compatibility. + #[tokio::test] + async fn rejection_with_errors_disabled_sends_null_data_and_no_errors() { + let (service, _handles) = router_with_unresponsive_subgraphs(serde_json::json!({ + "enabled": true, + "reject_unauthorized": true, + "errors": { "response": "disabled" } + })) + .await; + + let (_status, body) = + wire_response(service, graphql_post(REJECTED_QUERY, None, Context::new())).await; + + assert_eq!(body.get("data"), Some(&serde_json::Value::Null)); + assert_eq!(body.get("errors"), None); + } + + /// `errors.response: extensions` moves the authorization errors under + /// `extensions.authorizationErrors` and leaves `errors` out of the response. + /// This is not a spec-compliant behaviour, but is asserted for backwards compatibility. + #[tokio::test] + async fn rejection_with_errors_in_extensions() { + let (service, _handles) = router_with_unresponsive_subgraphs(serde_json::json!({ + "enabled": true, + "reject_unauthorized": true, + "errors": { "response": "extensions" } + })) + .await; + + let (_status, body) = + wire_response(service, graphql_post(REJECTED_QUERY, None, Context::new())).await; + + assert_eq!(body.get("data"), Some(&serde_json::Value::Null)); + assert_eq!(body.get("errors"), None); + let authorization_errors = body + .pointer("/extensions/authorizationErrors") + .and_then(|value| value.as_array()) + .expect("the errors must move under extensions.authorizationErrors"); + assert_eq!( + authorization_errors.len(), + 2, + "one error per unauthorized path: `orga.id` and `orga.creatorUser.phone`" + ); + } + + /// `dry_run` and `reject_unauthorized` combine rather than cancelling out: `dry_run` + /// reports the paths without modifying the operation, and `reject_unauthorized` + /// refuses on the reported paths. A refusal here can only come from configuration, + /// since `dry_run` never empties the document. + #[tokio::test] + async fn dry_run_with_reject_unauthorized_still_rejects() { + let (service, handles) = router_with_unresponsive_subgraphs(serde_json::json!({ + "enabled": true, + "reject_unauthorized": true, + "dry_run": true + })) + .await; + + let (_status, body) = send_rejected_request(service, Context::new()).await; + + assert_eq!(body.get("data"), Some(&serde_json::Value::Null)); + assert_no_subgraph_calls(handles).await; + } + + /// Without `reject_unauthorized`, an emptied operation runs the same pipeline as a + /// partial filter: an empty plan executes nothing, and response formatting against + /// the original operation shapes the result. Each requested root field arrives as + /// null alongside the path errors. + #[tokio::test] + async fn emptied_operation_without_reject_returns_shaped_data() { + let (service, handles) = router_with_unresponsive_subgraphs(serde_json::json!({ + "enabled": true + })) + .await; + + // `orga.id` is `@authenticated` and the only selection, so filtering removes + // everything. + let (status, body) = wire_response( + service, + graphql_post("query { orga(id: 1) { id } }", None, Context::new()), + ) + .await; + + assert_eq!(status, http::StatusCode::OK); + assert_eq!( + body.get("data"), + Some(&serde_json::json!({ "orga": null })), + "body: {body}" + ); + assert_eq!( + body.pointer("/errors/0/extensions/code"), + Some(&serde_json::json!("UNAUTHORIZED_FIELD_OR_TYPE")), + "body: {body}" + ); + assert_eq!( + body.pointer("/errors/0/path"), + Some(&serde_json::json!(["orga", "id"])), + "body: {body}" + ); + + assert_no_subgraph_calls(handles).await; + } + + /// The emptiness check in `filter_query` is document-wide, marked by its `FIXME`s: + /// fully filtering the executed operation while a sibling keeps the document + /// non-empty reports `Filtered`, planning fails to find the executed operation, and + /// the client is told the operation is unknown rather than refused. + #[tokio::test] + async fn fully_filtered_operation_beside_surviving_sibling_reports_unknown_operation() { + let (service, handles) = router_with_unresponsive_subgraphs(serde_json::json!({ + "enabled": true + })) + .await; + + let (status, body) = wire_response( + service, + graphql_post( + "query A { orga(id: 1) { id } } query B { currentUser { name } }", + Some("A"), + Context::new(), + ), + ) + .await; + + assert_eq!(status, http::StatusCode::BAD_REQUEST); + assert_eq!( + body.pointer("/errors/0/extensions/code"), + Some(&serde_json::json!("GRAPHQL_UNKNOWN_OPERATION_NAME")), + "body: {body}" + ); + assert_eq!(body.get("data"), None); + + assert_no_subgraph_calls(handles).await; + } + + /// `filter_query` collects unauthorized paths from the whole document, and the + /// execution layer's `reject_unauthorized` check reads them without knowing which + /// operation they came from. A fully authorized operation is refused when a sibling + /// operation asks for unauthorized fields, and the error cites a path from the + /// operation nobody ran. + #[tokio::test] + async fn authorized_operation_beside_unauthorized_sibling_is_refused() { + let (service, handles) = router_with_unresponsive_subgraphs(serde_json::json!({ + "enabled": true, + "reject_unauthorized": true + })) + .await; + + let (status, body) = wire_response( + service, + graphql_post(MULTI_OP_QUERY, Some("A"), Context::new()), + ) + .await; + + assert_eq!(status, http::StatusCode::OK); + assert_eq!( + body.get("data"), + Some(&serde_json::Value::Null), + "body: {body}" + ); + assert_eq!( + body.pointer("/errors/0/extensions/code"), + Some(&serde_json::json!("UNAUTHORIZED_FIELD_OR_TYPE")), + "body: {body}" + ); + // The path belongs to `B`, which nobody executed. + assert_eq!( + body.pointer("/errors/0/path"), + Some(&serde_json::json!(["orga", "id"])), + "body: {body}" + ); + + assert_no_subgraph_calls(handles).await; + } + + /// Without `reject_unauthorized`, the authorized operation executes and returns its + /// data. Error-path reconciliation drops the sibling's path, since it matches + /// nothing in the executed operation's shape, so its error arrives with a code and + /// no path on a successful response. + #[tokio::test] + async fn authorized_operation_beside_unauthorized_sibling_executes() { + let service = TestHarness::builder() + .configuration_json(serde_json::json!({ + "authorization": { "directives": { "enabled": true } } + })) + .unwrap() + .schema(AUTHENTICATED_SCHEMA) + .subgraph_hook(|_name, _service| { + let (mock, mut handle) = + tower_test::mock::pair::(); + tokio::spawn(async move { + while let Some((req, responder)) = handle.next_request().await { + responder.send_response( + subgraph::Response::fake_builder() + .context(req.context) + .data(serde_json::json! {{ "currentUser": { "name": "Ada" } }}) + .build(), + ); + } + }); + mock.boxed_clone() + }) + .build_router() + .await + .unwrap(); + + let (status, body) = wire_response( + service, + graphql_post(MULTI_OP_QUERY, Some("A"), Context::new()), + ) + .await; + + assert_eq!(status, http::StatusCode::OK); + assert_eq!( + body.pointer("/data/currentUser/name"), + Some(&serde_json::json!("Ada")), + "body: {body}" + ); + assert_eq!( + body.pointer("/errors/0/extensions/code"), + Some(&serde_json::json!("UNAUTHORIZED_FIELD_OR_TYPE")), + "body: {body}" + ); + assert_eq!(body.pointer("/errors/0/path"), None, "body: {body}"); + } +} + +/// A partial filter can leave an operation with nothing to execute. Filtering removes +/// `phone`, emptying `currentUser`, and `@skip(if: true)` removes the only other root +/// field, so the plan comes back with no root node while the operation was filtered rather +/// than refused. +/// +/// The plan shape here matches what a refused operation produces, so anything deciding +/// refusal from the absence of a root node together with the presence of unauthorized +/// paths answers this operation as though the router had refused it. +#[tokio::test] +async fn partial_filter_leaving_no_executable_work() { + let handles: Arc>>> = + Arc::new(Mutex::new(Vec::new())); + let handles_clone = handles.clone(); + + let service = TestHarness::builder() + .configuration_json(serde_json::json!({ + "authorization": { "directives": { "enabled": true } } + })) + .unwrap() + .schema(AUTHENTICATED_SCHEMA) + .subgraph_hook(move |_name, _service| { + let (mock, handle) = tower_test::mock::pair::(); + handles_clone.lock().unwrap().push(handle); + mock.boxed_clone() + }) + .build_router() + .await + .unwrap(); + + let req = graphql::Request { + query: Some( + "query { currentUser { phone } orga(id: 1) @skip(if: true) { name } }".to_string(), + ), + ..Default::default() + }; + let response = service + .oneshot(router::Request { + context: Context::new(), + router_request: http::Request::builder() + .method("POST") + .header(CONTENT_TYPE, "application/json") + .header(ACCEPT, "application/json") + .body(body::from_bytes(serde_json::to_vec(&req).unwrap())) + .unwrap(), + }) + .await + .unwrap(); + let bytes = body::into_bytes(response.response.into_body()) + .await + .unwrap(); + let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + + // No subgraph answered, so the plan carried no executable work. + let handles: Vec<_> = handles.lock().unwrap().drain(..).collect(); + assert!(!handles.is_empty(), "no subgraph services were created"); + for handle in handles { + crate::plugin::test::assert_no_mock_calls(handle).await; + } + + insta::assert_json_snapshot!(body); +} + #[tokio::test] async fn authenticated_directive_dry_run() { let _guard = tracing_test::dispatcher_guard(); diff --git a/apollo-router/src/plugins/demand_control/cost_calculator/static_cost.rs b/apollo-router/src/plugins/demand_control/cost_calculator/static_cost.rs index 7b29a0a5e1..d34db64ccd 100644 --- a/apollo-router/src/plugins/demand_control/cost_calculator/static_cost.rs +++ b/apollo-router/src/plugins/demand_control/cost_calculator/static_cost.rs @@ -788,7 +788,6 @@ mod tests { use crate::compute_job::ComputeJobType; use crate::plugins::authorization::CacheKeyMetadata; use crate::query_planner::QueryPlannerService; - use crate::services::QueryPlannerContent; use crate::services::QueryPlannerRequest; use crate::services::query_parsing::ParsedDocument; use crate::services::query_planner::PlanOptions; @@ -909,10 +908,7 @@ mod tests { )) .await .unwrap(); - let query_plan = match planner_res.content.unwrap() { - QueryPlannerContent::Plan { plan } => plan, - _ => panic!("Query planner returned unexpected non-plan content"), - }; + let query_plan = planner_res.content.unwrap(); let schema = DemandControlledSchema::new(Arc::new(supergraph_schema)).unwrap(); let mut demand_controlled_subgraph_schemas = HashMap::new(); diff --git a/apollo-router/src/plugins/telemetry/mod.rs b/apollo-router/src/plugins/telemetry/mod.rs index 0a6271c905..a47bacf810 100644 --- a/apollo-router/src/plugins/telemetry/mod.rs +++ b/apollo-router/src/plugins/telemetry/mod.rs @@ -3610,3 +3610,94 @@ mod tests { .await; } } + +#[cfg(test)] +mod licensed_operation_count_tests { + use std::collections::HashMap; + use std::sync::Arc; + use std::time::Duration; + + use crate::Context; + use crate::apollo_studio_interop::UsageReporting; + use crate::apollo_studio_interop::UsageReportingOperationDetails; + use crate::metrics::FutureMetricsExt as _; + use crate::plugins::telemetry::EnabledFeatures; + use crate::plugins::telemetry::Telemetry; + use crate::plugins::telemetry::apollo_exporter::Sender; + use crate::query_planner::OperationKind; + + /// Drives `update_apollo_metrics` over a context and returns the licensed operation + /// count Studio would be billed for. + async fn licensed_operation_count_for(context: Context) -> u64 { + let (tx, mut rx) = tokio::sync::mpsc::channel(10); + Telemetry::update_apollo_metrics( + &context, + 0.0, + Sender::Apollo(tx), + false, + Duration::from_millis(1), + OperationKind::Query, + None, + HashMap::new(), + EnabledFeatures { + distributed_apq_cache: false, + response_cache: false, + }, + ); + + rx.recv() + .await + .expect("update_apollo_metrics must send a stats report") + .licensed_operation_count_by_type + .map(|by_type| by_type.licensed_operation_count) + .unwrap_or(0) + } + + /// A context holding no `UsageReporting` bills one licensed operation. Billing does + /// not depend on the operation reaching execution or reporting anything about + /// itself. + #[tokio::test] + async fn missing_usage_reporting_is_billed_as_one_operation() { + async { + assert_eq!(licensed_operation_count_for(Context::new()).await, 1); + } + .with_metrics() + .await; + } + + /// `UsageReporting::Error` bills nothing: it is the one variant that zeroes the + /// licensed operation count, so an operation reported with it drops off the bill. + #[tokio::test] + async fn usage_reporting_error_is_not_billed() { + async { + let context = Context::new(); + context.extensions().with_lock(|lock| { + lock.insert::>(Arc::new(UsageReporting::Error( + "some error key".to_string(), + ))) + }); + + assert_eq!(licensed_operation_count_for(context).await, 0); + } + .with_metrics() + .await; + } + + /// `UsageReporting::Operation` bills one licensed operation, the same as a context + /// holding no reporting at all: attribution does not change what an operation costs. + #[tokio::test] + async fn operation_details_are_billed_as_one_operation() { + async { + let context = Context::new(); + context.extensions().with_lock(|lock| { + lock.insert::>(Arc::new(UsageReporting::Operation( + UsageReportingOperationDetails::default(), + ))) + }); + + assert_eq!(licensed_operation_count_for(context).await, 1); + } + .with_metrics() + .await; + } +} diff --git a/apollo-router/src/query_planner/caching_query_planner.rs b/apollo-router/src/query_planner/caching_query_planner.rs index 1756e064ec..564865ada9 100644 --- a/apollo-router/src/query_planner/caching_query_planner.rs +++ b/apollo-router/src/query_planner/caching_query_planner.rs @@ -142,7 +142,7 @@ fn init_query_plan_from_redis( subgraph_schemas: &SubgraphSchemas, cache_entry: &mut Result>, ) -> Result<(), String> { - if let Ok(QueryPlannerContent::Plan { plan }) = cache_entry { + if let Ok(plan) = cache_entry { // Arc freshly deserialized from Redis should be unique, so this doesn't clone: let plan = Arc::make_mut(plan); if let Some(root) = plan.root.as_mut() { @@ -401,7 +401,7 @@ where } // This will be overridden by the Rust usage reporting implementation - if let Some(QueryPlannerContent::Plan { plan, .. }) = &content { + if let Some(plan) = &content { context.extensions().with_lock(|lock| { lock.insert::>(plan.usage_reporting.clone()) }); @@ -607,11 +607,10 @@ where match res { Ok(content) => { - if let QueryPlannerContent::Plan { plan, .. } = &content { - context.extensions().with_lock(|lock| { - lock.insert::>(plan.usage_reporting.clone()) - }); - } + let plan = &content; + context.extensions().with_lock(|lock| { + lock.insert::>(plan.usage_reporting.clone()) + }); Ok(QueryPlannerResponse::builder().content(content).build()) } @@ -697,8 +696,7 @@ impl Hasher for StructHasher { impl ValueType for Result> { fn estimated_size(&self) -> Option { match self { - Ok(QueryPlannerContent::Plan { plan }) => Some(plan.estimated_size()), - Ok(QueryPlannerContent::Response { response }) => Some(estimate_size(response)), + Ok(plan) => Some(plan.estimated_size()), Err(e) => Some(estimate_size(e)), } } @@ -728,6 +726,7 @@ mod tests { use crate::apollo_studio_interop::UsageReporting; use crate::configuration::QueryPlanning; use crate::configuration::Supergraph; + use crate::plugins::authentication::APOLLO_AUTHENTICATION_JWT_CLAIMS; use crate::query_planner::QueryPlan; use crate::spec::Query; use crate::spec::Schema; @@ -811,9 +810,7 @@ mod tests { } else { // In measurement mode, this should complete successfully even after timeout let plan = Arc::new(QueryPlan::fake_new(None, None)); - Ok(QueryPlannerResponse::builder() - .content(QueryPlannerContent::Plan { plan }) - .build()) + Ok(QueryPlannerResponse::builder().content(plan).build()) } }) } @@ -849,9 +846,7 @@ mod tests { } else { // In measurement mode, this should complete successfully even after exceeding the memory limit let plan = Arc::new(QueryPlan::fake_new(None, None)); - Ok(QueryPlannerResponse::builder() - .content(QueryPlannerContent::Plan { plan }) - .build()) + Ok(QueryPlannerResponse::builder().content(plan).build()) } }) } @@ -1730,7 +1725,7 @@ mod tests { let plan = Arc::new(query_plan); while let Some((_request, responder)) = handle.next_request().await { - let qp_content = QueryPlannerContent::Plan { plan: plan.clone() }; + let qp_content = plan.clone(); responder .send_response(QueryPlannerResponse::builder().content(qp_content).build()); } @@ -1794,9 +1789,7 @@ mod tests { .await .expect("should receive one request"); - let content = QueryPlannerContent::Plan { - plan: Arc::new(QueryPlan::fake_new(None, None)), - }; + let content = Arc::new(QueryPlan::fake_new(None, None)); responder.send_response(QueryPlannerResponse::builder().content(content).build()); }); @@ -1849,6 +1842,185 @@ mod tests { crate::plugin::test::await_mock_driver(driver).await; } + /// Drives the planner mock, counting requests and answering each with `content`. + fn spawn_counting_planner( + mut handle: tower_test::mock::Handle, + content: QueryPlannerContent, + ) -> (tokio::task::JoinHandle<()>, Arc) { + let calls = Arc::new(AtomicUsize::new(0)); + let calls_clone = calls.clone(); + let driver = tokio::task::spawn(async move { + while let Some((_request, responder)) = handle.next_request().await { + calls_clone.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + responder.send_response( + QueryPlannerResponse::builder() + .content(content.clone()) + .build(), + ); + } + }); + (driver, calls) + } + + /// A configuration and schema pair that enables auth directives to work. + fn authorization_enabled_config_and_schema() -> (Configuration, Arc) { + let configuration: Configuration = serde_json::from_value(serde_json::json!({ + "authorization": { "directives": { "enabled": true } } + })) + .unwrap(); + let schema = include_str!("../../tests/fixtures/supergraph-auth.graphql"); + let schema = Arc::new(Schema::parse(schema, &configuration).unwrap()); + (configuration, schema) + } + + /// Builds a request the way the router does: authorization state travels in the + /// context as JWT claims, and `plan` derives `CacheKeyMetadata` from them via + /// `AuthorizationPlugin::update_cache_key`, overwriting any metadata inserted into + /// the context directly. + fn authorization_caching_request( + query: &str, + schema: &Schema, + configuration: &Configuration, + authenticated: bool, + ) -> query_planner::CachingRequest { + let doc = Query::parse_document(query, None, schema, configuration).unwrap(); + let context = Context::new(); + if authenticated { + context + .insert(APOLLO_AUTHENTICATION_JWT_CLAIMS, "placeholder".to_string()) + .unwrap(); + } + context.extensions().with_lock(|lock| { + lock.insert::(doc); + }); + query_planner::CachingRequest::new(query.to_string(), None, context) + } + + /// `CacheKeyMetadata` is part of `CachingQueryKey`'s `Hash`/`Eq`, so the same query + /// under different authorization state reaches the inner planner again. That keeps an + /// unauthenticated request from receiving a plan built for an authenticated one. + #[test(tokio::test)] + async fn plan_cache_is_segmented_by_authorization_metadata() { + let (mock, handle) = tower_test::mock::pair::(); + let (driver, planner_calls) = + spawn_counting_planner(handle, Arc::new(QueryPlan::fake_new(None, None))); + + let (configuration, schema) = authorization_enabled_config_and_schema(); + let mut service = CachingQueryPlanner::for_test( + mock.map_err(|err| panic!("tower-test errored: {err}")), + schema.clone(), + Default::default(), + &configuration, + ) + .await + .unwrap(); + + let query = "query ExampleQuery { me { name } }"; + + for authenticated in [ + false, true, // Repeats the first key, which must now hit the cache. + false, + ] { + service + .ready() + .await + .unwrap() + .call(authorization_caching_request( + query, + &schema, + &configuration, + authenticated, + )) + .await + .unwrap(); + } + + assert_eq!( + planner_calls.load(std::sync::atomic::Ordering::SeqCst), + 2, + "each distinct authorization state must be planned separately, \ + and a repeated state must be served from cache" + ); + + drop(service); + crate::plugin::test::await_mock_driver(driver).await; + } + + /// The cache stores an emptied operation's plan like any other — and, like any + /// other, it stays keyed by authorization state, so a plan cached for an + /// unauthenticated request is never served to an authenticated one. + #[test(tokio::test)] + async fn emptied_operation_plan_is_cached() { + let (mock, handle) = tower_test::mock::pair::(); + // The plan `QueryPlannerService::get` returns for an emptied operation: no + // root node. + let emptied_query = Query::empty_for_tests(); + let emptied_plan = QueryPlan { + usage_reporting: Arc::new(UsageReporting::Operation(Default::default())), + root: None, + formatted_query_plan: None, + query: Arc::new(emptied_query), + estimated_size: Default::default(), + }; + let (driver, planner_calls) = spawn_counting_planner(handle, Arc::new(emptied_plan)); + + let (configuration, schema) = authorization_enabled_config_and_schema(); + let mut service = CachingQueryPlanner::for_test( + mock.map_err(|err| panic!("tower-test errored: {err}")), + schema.clone(), + Default::default(), + &configuration, + ) + .await + .unwrap(); + + let query = "query ExampleQuery { me { name } }"; + + for _ in 0..2 { + service + .ready() + .await + .unwrap() + .call(authorization_caching_request( + query, + &schema, + &configuration, + false, + )) + .await + .unwrap(); + } + + assert_eq!( + planner_calls.load(std::sync::atomic::Ordering::SeqCst), + 1, + "the second identical request must be served from cache" + ); + + service + .ready() + .await + .unwrap() + .call(authorization_caching_request( + query, + &schema, + &configuration, + true, + )) + .await + .unwrap(); + + assert_eq!( + planner_calls.load(std::sync::atomic::Ordering::SeqCst), + 2, + "a cached rejection must not be served to a request in a different \ + authorization state" + ); + + drop(service); + crate::plugin::test::await_mock_driver(driver).await; + } + #[test(tokio::test)] async fn test_temporary_errors_arent_cached() { let (mock, mut handle) = diff --git a/apollo-router/src/query_planner/query_planner_service.rs b/apollo-router/src/query_planner/query_planner_service.rs index e541aa413b..d27dc0745b 100644 --- a/apollo-router/src/query_planner/query_planner_service.rs +++ b/apollo-router/src/query_planner/query_planner_service.rs @@ -6,8 +6,8 @@ use std::sync::OnceLock; use std::task::Poll; use std::time::Instant; +use apollo_compiler::ExecutableDocument; use apollo_compiler::Name; -use apollo_compiler::ast; use apollo_federation::error::FederationError; use apollo_federation::error::SingleFederationError; use apollo_federation::query_plan::query_planner::QueryPlanOptions; @@ -16,11 +16,11 @@ use futures::future::BoxFuture; use opentelemetry::KeyValue; use opentelemetry::metrics::MeterProvider as _; use opentelemetry::metrics::ObservableGauge; -use serde_json_bytes::Value; use tower::Service; use super::PlanNode; use super::QueryKey; +use super::QueryPlan; use crate::Configuration; use crate::apollo_studio_interop::generate_usage_reporting; use crate::compute_job; @@ -30,12 +30,11 @@ use crate::error::FederationErrorBridge; use crate::error::QueryPlannerError; use crate::error::ServiceBuildError; use crate::error::ValidationErrors; -use crate::graphql; -use crate::json_ext::Path; use crate::metrics::meter_provider; use crate::plugins::authorization; use crate::plugins::authorization::AuthorizationPlugin; use crate::plugins::authorization::CacheKeyMetadata; +use crate::plugins::authorization::FilterResult; use crate::plugins::authorization::UnauthorizedPaths; use crate::plugins::telemetry::config::ApolloSignatureNormalizationAlgorithm; use crate::plugins::telemetry::config::Conf as TelemetryConfig; @@ -339,15 +338,13 @@ impl QueryPlannerService { evaluated_plan_paths ); - Ok(QueryPlannerContent::Plan { - plan: Arc::new(super::QueryPlan { - usage_reporting: Arc::new(usage_reporting), - root: query_plan_root_node, - formatted_query_plan, - query: Arc::new(selections), - estimated_size: Default::default(), - }), - }) + Ok(Arc::new(QueryPlan { + usage_reporting: Arc::new(usage_reporting), + root: query_plan_root_node, + formatted_query_plan, + query: Arc::new(selections), + estimated_size: Default::default(), + })) } } @@ -441,9 +438,6 @@ impl Service for QueryPlannerService { } } -// Appease clippy::type_complexity -pub(crate) type FilteredQuery = (Vec, ast::Document); - impl QueryPlannerService { async fn get( &self, @@ -461,49 +455,58 @@ impl QueryPlannerService { // TODO(@goto-bus-stop): this is not a query planning concern let filter_res = if self.enable_authorization_directives { - match AuthorizationPlugin::filter_query(&self.authorization_config, &key, &self.schema) - { - Err(QueryPlannerError::Unauthorized(paths)) => { - let mut response = graphql::Response::builder().data(Value::Null).build(); - - if !paths.is_empty() { - let unauthorized = UnauthorizedPaths { - paths, - errors: self.authorization_config.error_config(), - }; - unauthorized.log_unauthorized_paths(); - unauthorized.update_response_with_unauthorized_path_errors(&mut response); - } - - return Ok(QueryPlannerContent::Response { - response: Box::new(response), - }); - } - other => other?, - } + AuthorizationPlugin::filter_query(&self.authorization_config, &key, &self.schema)? } else { - None + FilterResult::Unchanged }; - if let Some((unauthorized_paths, new_doc)) = filter_res { - let new_query = new_doc.to_string(); - let new_hash = self - .schema - .schema_id - .operation_hash(&new_query, key.operation_name.as_deref()); - - key.filtered_query = new_query; - let executable_document = new_doc - .to_executable_validate(self.schema.api_schema()) - .map_err(|e| QueryPlannerError::from(SpecError::ValidationError(e.into())))?; - doc = ParsedDocumentInner::new( - new_doc, - Arc::new(executable_document), - key.operation_name.as_deref(), - Arc::new(new_hash), - ) - .map_err(QueryPlannerError::from)?; - selections.unauthorized.paths = unauthorized_paths; + match filter_res { + FilterResult::Unchanged => {} + // Filtering can empty the document; the federation planner cannot plan a + // document with no definitions, so answer with a plan that carries no work. + FilterResult::Filtered { paths, document } if document.definitions.is_empty() => { + selections.unauthorized.paths = paths; + + // References come from the operation that ran, and nothing did. + let usage_reporting = generate_usage_reporting( + &doc.executable, + &ExecutableDocument::new(), + &key.operation_name, + self.schema.supergraph_schema(), + &self.signature_normalization_algorithm, + ); + + return Ok(Arc::new(QueryPlan { + usage_reporting: Arc::new(usage_reporting), + root: None, + formatted_query_plan: None, + query: Arc::new(selections), + estimated_size: Default::default(), + })); + } + FilterResult::Filtered { + paths, + document: new_doc, + } => { + let new_query = new_doc.to_string(); + let new_hash = self + .schema + .schema_id + .operation_hash(&new_query, key.operation_name.as_deref()); + + key.filtered_query = new_query; + let executable_document = new_doc + .to_executable_validate(self.schema.api_schema()) + .map_err(|e| QueryPlannerError::from(SpecError::ValidationError(e.into())))?; + doc = ParsedDocumentInner::new( + new_doc, + Arc::new(executable_document), + key.operation_name.as_deref(), + Arc::new(new_hash), + ) + .map_err(QueryPlannerError::from)?; + selections.unauthorized.paths = paths; + } } if key.filtered_query != key.original_query { @@ -657,19 +660,11 @@ mod tests { .await .unwrap(); - if let QueryPlannerContent::Plan { plan, .. } = - response.content.expect("successful response") - { - insta::with_settings!({sort_maps => true}, { - insta::assert_json_snapshot!("plan_usage_reporting", plan.usage_reporting); - }); - insta::assert_debug_snapshot!( - "plan_root", - plan.root.as_deref().expect("non-empty plan") - ); - } else { - panic!("unexpected query planner content") - } + let plan = response.content.expect("successful response"); + insta::with_settings!({sort_maps => true}, { + insta::assert_json_snapshot!("plan_usage_reporting", plan.usage_reporting); + }); + insta::assert_debug_snapshot!("plan_root", plan.root.as_deref().expect("non-empty plan")); } #[test(tokio::test)] @@ -732,10 +727,7 @@ mod tests { let content = response.content.expect("expected a successful response"); - let plan = match content { - QueryPlannerContent::Plan { plan, .. } => plan, - _ => panic!("expected a Plan response, received {content:?}"), - }; + let plan = content; assert_eq!(plan.root, None, "expected an empty plan"); } @@ -1068,27 +1060,70 @@ mod tests { .await .unwrap(); - if let QueryPlannerContent::Plan { plan, .. } = result { - check_query_plan_coverage( - plan.root.as_ref().expect("non-empty query plan"), - None, - &plan.query.subselections, - ); + let plan = result; + check_query_plan_coverage( + plan.root.as_ref().expect("non-empty query plan"), + None, + &plan.query.subselections, + ); - let mut keys: Vec = Vec::new(); - for (key, value) in plan.query.subselections.iter() { - let mut serialized = String::from("query"); - serialize_selection_set(&value.selection_set, &mut serialized); - keys.push(format!( - "{:?} {} {}", - key.defer_label, key.defer_conditions.bits, serialized - )) - } - keys.sort(); - keys.join("\n") - } else { - panic!() + let mut keys: Vec = Vec::new(); + for (key, value) in plan.query.subselections.iter() { + let mut serialized = String::from("query"); + serialize_selection_set(&value.selection_set, &mut serialized); + keys.push(format!( + "{:?} {} {}", + key.defer_label, key.defer_conditions.bits, serialized + )) } + keys.sort(); + keys.join("\n") + } + + /// An unauthenticated client requesting only fields that require authentication gets + /// an empty query plan: nothing to execute, and every requested field in + /// `unauthorized.paths`. + #[test(tokio::test)] + async fn fully_unauthorized_operation_plans_no_work() { + let configuration: Configuration = serde_json::from_value(serde_json::json!({ + "authorization": { "directives": { "enabled": true } } + })) + .unwrap(); + let configuration = Arc::new(configuration); + + // `Query.me` requires the `profile` scope. + let schema = include_str!("../../tests/fixtures/supergraph-auth.graphql"); + let schema = Arc::new(Schema::parse(schema, &configuration).unwrap()); + let planner = QueryPlannerService::for_test(schema.clone(), configuration.clone()).unwrap(); + + let query = "query { me { name } }"; + let doc = Query::parse_document(query, None, &schema, &configuration).unwrap(); + + let response = planner + .oneshot(QueryPlannerRequest { + query: query.to_string(), + operation_name: None, + document: doc, + metadata: CacheKeyMetadata::default(), + plan_options: PlanOptions::default(), + compute_job_type: ComputeJobType::QueryPlanning, + }) + .await + .unwrap(); + + let plan = response.content.expect("planning succeeded"); + assert!( + plan.root.is_none(), + "the plan must carry no fetches when every requested field failed authorization" + ); + assert_eq!( + plan.query + .unauthorized + .paths + .first() + .map(ToString::to_string), + Some("/me".to_string()) + ); } #[tokio::test] diff --git a/apollo-router/src/query_planner/warmup.rs b/apollo-router/src/query_planner/warmup.rs index ea83387ceb..28111179f9 100644 --- a/apollo-router/src/query_planner/warmup.rs +++ b/apollo-router/src/query_planner/warmup.rs @@ -370,9 +370,7 @@ mod tests { let schema_hash = SchemaHash::new(""); fn empty_query_plan() -> Result> { - Ok(QueryPlannerContent::Plan { - plan: Arc::new(QueryPlan::fake_new(None, None)), - }) + Ok(Arc::new(QueryPlan::fake_new(None, None))) } { diff --git a/apollo-router/src/services/execution/service.rs b/apollo-router/src/services/execution/service.rs index 07b633e8a1..d6085f9da5 100644 --- a/apollo-router/src/services/execution/service.rs +++ b/apollo-router/src/services/execution/service.rs @@ -289,7 +289,6 @@ impl ExecutionService { let variables_set = query.defer_variables_set(variables); tracing::debug_span!("format_response").in_scope(|| { - let mut paths = Vec::new(); if !query.unauthorized.paths.is_empty() { query.unauthorized.log_unauthorized_paths(); query @@ -297,23 +296,13 @@ impl ExecutionService { .update_response_with_unauthorized_path_errors(&mut response); } - if let Some(filtered_query) = query.filtered_query.as_ref() { - paths = filtered_query.format_response( - &mut response, - variables.clone(), - schema.api_schema(), - variables_set, - insert_result_coercion_errors, - ); - } - - paths.extend(query.format_response( + let paths = query.format_response_filtered_then_original( &mut response, variables.clone(), schema.api_schema(), variables_set, insert_result_coercion_errors, - )); + ); for error in response.errors.iter_mut() { if let Some(path) = &mut error.path { diff --git a/apollo-router/src/services/query_planner.rs b/apollo-router/src/services/query_planner.rs index dd9099527e..c87db77a43 100644 --- a/apollo-router/src/services/query_planner.rs +++ b/apollo-router/src/services/query_planner.rs @@ -91,6 +91,10 @@ impl CachingRequest { } assert_impl_all!(Response: Send); +/// What query planning produces on success. An alias so the payload can change in one +/// place. +pub(crate) type QueryPlannerContent = Arc; + /// [`Context`] and [`QueryPlan`] for the response. pub(crate) struct Response { /// Optional in case of error @@ -98,23 +102,13 @@ pub(crate) struct Response { pub(crate) errors: Vec, } -/// Query, QueryPlan and Introspection data. -#[derive(Debug, Clone, Serialize, Deserialize)] -pub(crate) enum QueryPlannerContent { - Plan { plan: Arc }, - Response { response: Box }, -} - #[buildstructor::buildstructor] impl Response { /// This is the constructor (or builder) to use when constructing a real QueryPlannerResponse. /// /// Required parameters are required in non-testing code to create a QueryPlannerResponse. #[builder] - pub(crate) fn new( - content: Option, - errors: Vec, - ) -> Response { + pub(crate) fn new(content: Option>, errors: Vec) -> Response { Self { content, errors } } } diff --git a/apollo-router/src/services/supergraph/service.rs b/apollo-router/src/services/supergraph/service.rs index 6fa250b1ee..684cf3fb0a 100644 --- a/apollo-router/src/services/supergraph/service.rs +++ b/apollo-router/src/services/supergraph/service.rs @@ -50,7 +50,6 @@ use crate::query_planner::SubgraphSchemas; use crate::query_planner::warmup; use crate::services::ExecutionRequest; use crate::services::ExecutionResponse; -use crate::services::QueryPlannerContent; use crate::services::QueryPlannerResponse; use crate::services::SubgraphServiceFactory; use crate::services::SupergraphRequest; @@ -269,10 +268,7 @@ async fn service_call( } match content { - Some(QueryPlannerContent::Response { response }) => Ok( - SupergraphResponse::new_from_graphql_response(*response, context), - ), - Some(QueryPlannerContent::Plan { plan }) => { + Some(plan) => { let is_deferred = plan.is_deferred(&variables); let is_subscription = plan.is_subscription(); diff --git a/apollo-router/src/services/supergraph/tests.rs b/apollo-router/src/services/supergraph/tests.rs index 4bbe50e6cb..3908e0f22b 100644 --- a/apollo-router/src/services/supergraph/tests.rs +++ b/apollo-router/src/services/supergraph/tests.rs @@ -3839,7 +3839,6 @@ async fn test_cache_warmup() { use crate::query_planner::QueryPlan; use crate::services::PluggableSupergraphServiceBuilder; - use crate::services::QueryPlannerContent; use crate::services::QueryPlannerResponse; use crate::services::layers::persisted_queries::PersistedQueryExpander; use crate::services::query_planner; @@ -3865,9 +3864,7 @@ async fn test_cache_warmup() { /// Return an empty plan that doesn't require any subgraph requests to fulfill. fn empty_query_plan() -> QueryPlannerResponse { let plan = Arc::new(QueryPlan::fake_new(None, None)); - QueryPlannerResponse::builder() - .content(QueryPlannerContent::Plan { plan }) - .build() + QueryPlannerResponse::builder().content(plan).build() } /// Execute a constant mock query against the given supergraph service. diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index eb009a23c5..350630f431 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -127,6 +127,43 @@ impl Query { /// This will discard unrequested fields and re-order the output to match the order of the /// query. #[tracing::instrument(skip_all, level = "trace")] + /// Formats a response for an operation that authorization may have filtered, + /// returning the nullified paths from every pass. + /// + /// The filtered query formats first, projecting the data onto the authorized shape + /// and discarding fields the subgraphs returned beyond it (via `@requires`, or a + /// misbehaving subgraph). The original query formats second, expanding the result to + /// the shape the client requested and nulling what filtering removed. One pass cannot + /// do both: the original alone returns unauthorized values, the filtered alone drops + /// requested fields from the response. + pub(crate) fn format_response_filtered_then_original( + &self, + response: &mut Response, + variables: Object, + schema: &ApiSchema, + defer_conditions: BooleanValues, + include_coercion_errors: bool, + ) -> Vec { + let mut paths = Vec::new(); + if let Some(filtered_query) = self.filtered_query.as_ref() { + paths = filtered_query.format_response( + response, + variables.clone(), + schema, + defer_conditions, + include_coercion_errors, + ); + } + paths.extend(self.format_response( + response, + variables, + schema, + defer_conditions, + include_coercion_errors, + )); + paths + } + pub(crate) fn format_response( &self, response: &mut Response, diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 0d9c6150f7..32a2a192b5 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -2,9 +2,15 @@ use apollo_compiler::parser::Parser; use insta::assert_json_snapshot; use serde_json_bytes::json; use test_log::test; +use tower::ServiceExt; use super::*; +use crate::compute_job::ComputeJobType; use crate::json_ext::ValueExt; +use crate::plugins::authorization::CacheKeyMetadata; +use crate::query_planner::query_planner_service::QueryPlannerService; +use crate::services::QueryPlannerRequest; +use crate::services::query_planner::PlanOptions; macro_rules! assert_eq_and_ordered { ($a:expr, $b:expr $(,)?) => { @@ -7620,6 +7626,156 @@ fn test_query_not_named_query() { ); } +const AUTHENTICATED_INTERFACE_SCHEMA: &str = r#" + schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION) + @link(url: "https://specs.apollo.dev/authenticated/v0.1", for: SECURITY) + { + query: Query + } + directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA + directive @join__graph(name: String!, url: String!) on ENUM_VALUE + directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR + directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE + directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION + directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION + directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE + directive @authenticated on OBJECT | FIELD_DEFINITION | INTERFACE | SCALAR | ENUM + + scalar join__FieldSet + scalar link__Import + + enum link__Purpose { + SECURITY + EXECUTION + } + enum join__Graph { + TEST @join__graph(name: "test", url: "http://localhost:4001/graphql") + } + + type Query @join__type(graph: TEST) { + thing: Thing + } + + interface Thing @join__type(graph: TEST) { + id: ID + } + + type Foo implements Thing + @join__type(graph: TEST) + @join__implements(graph: TEST, interface: "Thing") { + id: ID + inline: String + spread: String + secret: String @authenticated + } +"#; + +/// Plans `query_str` as an unauthenticated request and returns the original `Query` with +/// `filtered_query` populated, alongside the schema. +/// +/// The pair comes from `QueryPlannerService`, so the filtered `Query` is the one +/// `filter_query` produces, marked `is_original = false` by the planner. +async fn authorization_filtered_query(query_str: &str) -> (Arc, Arc) { + let configuration: Configuration = serde_json::from_value(serde_json::json!({ + "authorization": { "directives": { "enabled": true } } + })) + .unwrap(); + let configuration = Arc::new(configuration); + let schema = Arc::new(Schema::parse(AUTHENTICATED_INTERFACE_SCHEMA, &configuration).unwrap()); + let doc = Query::parse_document(query_str, None, &schema, &configuration).unwrap(); + + let planner = QueryPlannerService::for_test(schema.clone(), configuration.clone()).unwrap(); + let response = planner + .oneshot(QueryPlannerRequest { + query: query_str.to_string(), + operation_name: None, + document: doc, + metadata: CacheKeyMetadata::default(), + plan_options: PlanOptions::default(), + compute_job_type: ComputeJobType::QueryPlanning, + }) + .await + .unwrap(); + + let query = match response.content { + Some(plan) => plan.query.clone(), + _ => panic!("filtering removed only `secret`, so the planner must return a plan"), + }; + assert!( + query.filtered_query.is_some(), + "filtering must have produced a second Query, otherwise the two-pass formatting \ + under test never runs" + ); + + (query, schema) +} + +/// Formats `data` through [`Query::format_response_filtered_then_original`] and returns +/// the resulting `thing` object. +fn format_filtered_then_original(query: &Query, schema: &Schema, data: Value) -> Value { + let mut response = crate::graphql::Response::builder().data(data).build(); + + query.format_response_filtered_then_original( + &mut response, + Object::new(), + schema.api_schema(), + BooleanValues { bits: 0 }, + true, + ); + + response + .data + .as_ref() + .unwrap() + .get("thing") + .unwrap() + .clone() +} + +/// 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. +/// +/// One fragment form per test: each form passes `__typename` along independently, so a +/// query carrying both keeps its fields alive when either does. +#[tokio::test] +async fn filtered_inline_fragment_on_abstract_type() { + // `secret` is `@authenticated`, so filtering drops it and leaves `inline`. + let (query, schema) = + authorization_filtered_query("{ thing { id ... on Foo { inline secret } } }").await; + + // What the subgraph returns for the filtered plan: `secret` was never requested. + let thing = format_filtered_then_original( + &query, + &schema, + json! {{ "thing": { "__typename": "Foo", "id": "1", "inline": "inline" } }}, + ); + + assert_eq!(thing.get("inline"), Some(&json!("inline"))); +} + +/// The fragment-spread counterpart of `filtered_inline_fragment_on_abstract_type`, +/// covering the second `!is_original` branch in `apply_selection_set`. +#[tokio::test] +async fn filtered_fragment_spread_on_abstract_type() { + let (query, schema) = authorization_filtered_query( + "{ thing { id ...Spread } } fragment Spread on Foo { spread secret }", + ) + .await; + + let thing = format_filtered_then_original( + &query, + &schema, + json! {{ "thing": { "__typename": "Foo", "id": "1", "spread": "spread" } }}, + ); + + assert_eq!(thing.get("spread"), Some(&json!("spread"))); +} + #[test] fn filtered_defer_fragment() { let config = Configuration::default(); diff --git a/apollo-router/tests/integration/directives/policy.rs b/apollo-router/tests/integration/directives/policy.rs index 5d4f5bdd35..074ebb2ba3 100644 --- a/apollo-router/tests/integration/directives/policy.rs +++ b/apollo-router/tests/integration/directives/policy.rs @@ -226,7 +226,7 @@ async fn policy_directive_should_not_pass_if_coproc_disallowed() -> Result<(), B .unwrap(); // THEN - // * we get NO data back for the private field! + // * the private field is null let data = supergraph_harness .oneshot(request) .await @@ -236,7 +236,7 @@ async fn policy_directive_should_not_pass_if_coproc_disallowed() -> Result<(), B .unwrap() .data .unwrap(); - assert!(data.is_null()); + assert_eq!(data.get("private"), Some(&serde_json_bytes::Value::Null)); Ok(()) } @@ -460,7 +460,7 @@ async fn interface_with_different_implementation_policies_should_require_auth() let data = response.data.unwrap(); let error = response.errors.first().unwrap(); - assert!(data.is_null()); + assert_eq!(data.get("secure"), Some(&serde_json_bytes::Value::Null)); assert_eq!( error.extension_code().unwrap(), "UNAUTHORIZED_FIELD_OR_TYPE".to_string() @@ -553,7 +553,7 @@ mod all_unauthorized_paths { /// * the router configuration described in `send_request` /// * authorization configured to put errors into extensions rather than the errors array /// Then: - /// * data is null + /// * `secure` is null /// * errors array is empty /// * the authorization error appears in extensions["authorizationErrors"] #[tokio::test(flavor = "multi_thread")] @@ -565,7 +565,10 @@ mod all_unauthorized_paths { }); let response = send_request(authorization_conf).await.unwrap(); - assert!(response.data.unwrap().is_null()); + assert_eq!( + response.data.unwrap().get("secure"), + Some(&serde_json_bytes::Value::Null) + ); assert!(response.errors.is_empty()); assert!(!response.extensions.is_empty()); @@ -578,7 +581,7 @@ mod all_unauthorized_paths { /// * the router configuration described in `send_request` /// * authorization configured to put errors into the errors array /// Then: - /// * data is null + /// * `secure` is null /// * the authorization error appears in errors /// * extensions has no `authorizationErrors` #[tokio::test(flavor = "multi_thread")] @@ -590,7 +593,10 @@ mod all_unauthorized_paths { }); let response = send_request(authorization_conf).await.unwrap(); - assert!(response.data.unwrap().is_null()); + assert_eq!( + response.data.unwrap().get("secure"), + Some(&serde_json_bytes::Value::Null) + ); assert!(!response.errors.is_empty()); assert!(!response.extensions.contains_key("authorizationErrors")); @@ -603,7 +609,7 @@ mod all_unauthorized_paths { /// * the router configuration described in `send_request` /// * authorization configured to suppress errors entirely /// Then: - /// * data is null + /// * `secure` is null /// * errors array is empty /// * extensions has no `authorizationErrors` #[tokio::test(flavor = "multi_thread")] @@ -615,7 +621,10 @@ mod all_unauthorized_paths { }); let response = send_request(authorization_conf).await.unwrap(); - assert!(response.data.unwrap().is_null()); + assert_eq!( + response.data.unwrap().get("secure"), + Some(&serde_json_bytes::Value::Null) + ); assert!(response.errors.is_empty()); assert!(!response.extensions.contains_key("authorizationErrors")); } diff --git a/apollo-router/tests/integration/redis.rs b/apollo-router/tests/integration/redis.rs index 93c5ffd956..df2c385776 100644 --- a/apollo-router/tests/integration/redis.rs +++ b/apollo-router/tests/integration/redis.rs @@ -10,15 +10,7 @@ // ```bash // 1724831727.472732 [0 127.0.0.1:56720] "SET" // "plan:0:v2.8.5:70f115ebba5991355c17f4f56ba25bb093c519c4db49a30f3b10de279a4e3fa4:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:4f9f0183101b2f249a364b98adadfda6e5e2001d1f2465c988428cf1ac0b545f" -// "{\"Ok\":{\"Plan\":{\"plan\":{\"usage_reporting\":{\"statsReportKey\":\"# -// -\\n{topProducts{name -// name}}\",\"referencedFieldsByType\":{\"Product\":{\"fieldNames\":[\"name\"],\"isInterface\":false},\"Query\":{\"fieldNames\":[\"topProducts\"],\"isInterface\":false}}},\"root\":{\"kind\":\"Fetch\",\"serviceName\":\"products\",\"variableUsages\":[],\"operation\":\"{topProducts{name -// name2:name}}\",\"operationName\":null,\"operationKind\":\"query\",\"id\":null,\"inputRewrites\":null,\"outputRewrites\":null,\"contextRewrites\":null,\"schemaAwareHash\":\"121b9859eba2d8fa6dde0a54b6e3781274cf69f7ffb0af912e92c01c6bfff6ca\",\"authorization\":{\"is_authenticated\":false,\"scopes\":[],\"policies\":[]}},\"formatted_query_plan\":\"QueryPlan -// {\\n Fetch(service: \\\"products\\\") {\\n {\\n topProducts {\\n -// name\\n name2: name\\n }\\n }\\n -// n },\\n}\",\"query\":{\"string\":\"{\\n topProducts {\\n name\\n -// name2: name\\n -// }\\n}\\n\",\"fragments\":{\"map\":{}},\"operations\":[{\"name\":null,\"kind\":\"query\",\"type_name\":\"Query\",\"selection_set\":[{\"Field\":{\"name\":\"topProducts\",\"alias\":null,\"selection_set\":[{\"Field\":{\"name\":\"name\",\"alias\":null,\"selection_set\":null,\"field_type\":{\"Named\":\"String\"},\"include_skip\":{\"include\":\"Yes\",\"skip\":\"No\"}}},{\"Field\":{\"name\":\"name\",\"alias\":\"name2\",\"selection_set\":null,\"field_type\":{\"Named\":\"String\"},\"include_skip\":{\"include\":\"Yes\",\"skip\":\"No\"}}}],\"field_type\":{\"List\":{\"Named\":\"Product\"}},\"include_skip\":{\"include\":\"Yes\",\"skip\":\"No\"}}}],\"variables\":{}}],\"subselections\":{},\"unauthorized\":{\"paths\":[],\"errors\":{\"log\":true,\"response\":\"errors\"}},\"filtered_query\":null,\"defer_stats\":{\"has_defer\":false,\"has_unconditional_defer\":false,\"conditional_defer_variable_names\":[]},\"is_original\":true,\"schema_aware_hash\":[20,152,93,92,189,0,240,140,9,65,84,255,4,76,202,231,69,183,58,121,37,240,0,109,198,125,1,82,12,42,179,189]},\"query_metrics\":{\"depth\":2,\"height\":3,\"root_fields\":1,\"aliases\":1},\"estimated_size\":0}}}}" +// "{\"Ok\":{\"usage_reporting\":{...},\"root\":{...},\"query\":{...},...}}" // "EX" "10" // ``` @@ -166,10 +158,6 @@ async fn query_planner_cache() -> Result<(), BoxError> { .unwrap() .get("Ok") .unwrap() - .get("Plan") - .unwrap() - .get("plan") - .unwrap() .get("root"); insta::assert_json_snapshot!(query_plan); diff --git a/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization-2.snap b/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization-2.snap deleted file mode 100644 index b6437c2c7e..0000000000 --- a/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization-2.snap +++ /dev/null @@ -1,50 +0,0 @@ ---- -source: apollo-router/tests/integration/response_cache.rs -expression: response ---- -{ - "data": { - "me": null, - "topProducts": [ - { - "name": "chair", - "reviews": [ - { - "body": "I can sit on it", - "author": { - "username": "ada" - } - } - ] - }, - { - "name": "table", - "reviews": [ - { - "body": "I can sit on it", - "author": { - "username": "ada" - } - }, - { - "body": "I can eat on it", - "author": { - "username": "charles" - } - } - ] - } - ] - }, - "errors": [ - { - "message": "Unauthorized field or type", - "path": [ - "me" - ], - "extensions": { - "code": "UNAUTHORIZED_FIELD_OR_TYPE" - } - } - ] -} diff --git a/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization-3.snap b/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization-3.snap deleted file mode 100644 index b7e02eae2f..0000000000 --- a/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization-3.snap +++ /dev/null @@ -1,54 +0,0 @@ ---- -source: apollo-router/tests/integration/response_cache.rs -expression: response ---- -{ - "data": { - "me": { - "id": "1", - "name": null - }, - "topProducts": [ - { - "name": "chair", - "reviews": [ - { - "body": "I can sit on it", - "author": { - "username": "ada" - } - } - ] - }, - { - "name": "table", - "reviews": [ - { - "body": "I can sit on it", - "author": { - "username": "ada" - } - }, - { - "body": "I can eat on it", - "author": { - "username": "charles" - } - } - ] - } - ] - }, - "errors": [ - { - "message": "Unauthorized field or type", - "path": [ - "me", - "name" - ], - "extensions": { - "code": "UNAUTHORIZED_FIELD_OR_TYPE" - } - } - ] -} diff --git a/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization.snap b/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization.snap deleted file mode 100644 index c8804ce0f4..0000000000 --- a/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization.snap +++ /dev/null @@ -1,57 +0,0 @@ ---- -source: apollo-router/tests/integration/response_cache.rs -expression: response ---- -{ - "data": { - "me": null, - "topProducts": [ - { - "name": "chair", - "reviews": [ - { - "body": "I can sit on it", - "author": null - } - ] - }, - { - "name": "table", - "reviews": [ - { - "body": "I can sit on it", - "author": null - }, - { - "body": "I can eat on it", - "author": null - } - ] - } - ] - }, - "errors": [ - { - "message": "Unauthorized field or type", - "path": [ - "me" - ], - "extensions": { - "code": "UNAUTHORIZED_FIELD_OR_TYPE" - } - }, - { - "message": "Unauthorized field or type", - "path": [ - "topProducts", - "@", - "reviews", - "@", - "author" - ], - "extensions": { - "code": "UNAUTHORIZED_FIELD_OR_TYPE" - } - } - ] -} diff --git a/apollo-router/tests/integration/telemetry/fixtures/authorization_error_span.router.yaml b/apollo-router/tests/integration/telemetry/fixtures/authorization_error_span.router.yaml new file mode 100644 index 0000000000..5929b15d48 --- /dev/null +++ b/apollo-router/tests/integration/telemetry/fixtures/authorization_error_span.router.yaml @@ -0,0 +1,9 @@ +telemetry: + exporters: + logging: + stdout: + format: json +authorization: + directives: + enabled: true + reject_unauthorized: true diff --git a/apollo-router/tests/integration/telemetry/fixtures/authorization_error_span_no_reject.router.yaml b/apollo-router/tests/integration/telemetry/fixtures/authorization_error_span_no_reject.router.yaml new file mode 100644 index 0000000000..5173c12d2d --- /dev/null +++ b/apollo-router/tests/integration/telemetry/fixtures/authorization_error_span_no_reject.router.yaml @@ -0,0 +1,8 @@ +telemetry: + exporters: + logging: + stdout: + format: json +authorization: + directives: + enabled: true diff --git a/apollo-router/tests/integration/telemetry/logging.rs b/apollo-router/tests/integration/telemetry/logging.rs index 91ba0b4685..000176f3d9 100644 --- a/apollo-router/tests/integration/telemetry/logging.rs +++ b/apollo-router/tests/integration/telemetry/logging.rs @@ -241,3 +241,83 @@ async fn test_text_sampler_off() -> Result<(), BoxError> { router.graceful_shutdown().await; Ok(()) } + +/// The `Authorization error` event for an unauthorized operation lands under the +/// `execution` span, exactly once per request. The `execution` span comes from the +/// telemetry plugin, which only joins the pipeline when OpenTelemetry is initialised for +/// the process, so a spawned router is the smallest thing that has it. +/// +/// With `reject_unauthorized`, the authorization layer on the execution service emits +/// the event; without it, response formatting does. Each configuration exercises its +/// own emitter. +async fn assert_authorization_error_event_in_execution_span( + config: &'static str, +) -> Result<(), BoxError> { + let mut router = IntegrationTest::builder() + .config(config) + .supergraph("tests/fixtures/supergraph-auth.graphql") + .build() + .await; + + router.start().await; + router.assert_started().await; + + // `Query.me` requires the `profile` scope, so an unauthenticated request loses its + // only root field. + router + .execute_query( + Query::builder() + .body(serde_json::json!({ "query": "{ me { name } }" })) + .build(), + ) + .await; + router.wait_for_log_message("Authorization error").await; + + let events: Vec = router + .logs() + .iter() + .filter(|line| line.contains("Authorization error")) + .map(|line| serde_json::from_str(line).expect("log line is JSON")) + .collect(); + + // A second event for the same request means two code paths both believe they own + // the log. + assert_eq!(events.len(), 1, "events: {events:?}"); + + let event = &events[0]; + // The event records the paths with `?`, so they arrive debug-formatted as one string. + assert_eq!( + event.pointer("/unauthorized_query_paths"), + Some(&serde_json::json!(r#"["/me"]"#)) + ); + let span_names: Vec<&str> = event + .pointer("/spans") + .and_then(|spans| spans.as_array()) + .expect("json logs carry a span list") + .iter() + .filter_map(|span| span.get("name").and_then(|name| name.as_str())) + .collect(); + assert!( + span_names.contains(&"execution"), + "expected the event inside the execution span, got spans: {span_names:?}" + ); + + router.graceful_shutdown().await; + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_authorization_error_event_in_execution_span() -> Result<(), BoxError> { + assert_authorization_error_event_in_execution_span(include_str!( + "fixtures/authorization_error_span.router.yaml" + )) + .await +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_authorization_error_event_in_execution_span_without_reject() -> Result<(), BoxError> { + assert_authorization_error_event_in_execution_span(include_str!( + "fixtures/authorization_error_span_no_reject.router.yaml" + )) + .await +} diff --git a/dev-docs/authorization-query-planning.md b/dev-docs/authorization-query-planning.md new file mode 100644 index 0000000000..7d4833dad4 --- /dev/null +++ b/dev-docs/authorization-query-planning.md @@ -0,0 +1,280 @@ +# Authorization and query planning + +Scratch notes for ROUTER-1973 (separate authorization from query planning). +The before section describes `dev-v3.x` at `639c65a88`; the after section describes the +branch as implemented. File references are `file:line` on the branch. + +## Before + +```mermaid +sequenceDiagram + autonumber + participant C as Client + participant PQ as ParseQueryLayer + participant EX as ExtractAuthChecksLayer + participant PL as Auth / policy plugins + participant SG as SupergraphService + participant CQP as CachingQueryPlanner + participant QPS as QueryPlannerService + participant ES as ExecutionService + participant SUB as Subgraphs + + C->>PQ: POST /graphql + PQ->>EX: parse, ParsedDocument in context + Note over EX: generate_cache_metadata
= what the document REQUIRES + EX->>PL: request + Note over PL: authentication sets JWT claims
policies resolve to true or false + PL->>SG: supergraph Request + SG->>CQP: CachingRequest + Note over CQP: update_cache_key
= what the request was GRANTED + CQP->>CQP: key on ORIGINAL text + metadata + + alt cache miss + CQP->>QPS: QueryPlannerRequest + QPS->>QPS: filter_query + alt document emptied, or reject_unauthorized + QPS-->>CQP: QueryPlannerContent::Response + Note right of QPS: the planner ANSWERS the request + else filtering succeeded + QPS-->>CQP: QueryPlannerContent::Plan + end + CQP->>CQP: cache either variant + end + + alt Response variant + SG-->>C: 200, data null and errors + else Plan variant + SG->>ES: execution Request + ES->>SUB: fetches + Note over ES: errors, then format twice:
FILTERED shape, ORIGINAL shape + ES-->>C: 200, nulls and authorization errors + end +``` + +## After + +The planner plans or errors. The decision to refuse lives on the execution service. + +```mermaid +sequenceDiagram + autonumber + participant C as Client + participant SG as SupergraphService + participant CQP as CachingQueryPlanner + participant QPS as QueryPlannerService + participant AL as AuthorizationLayer (execution) + participant ES as ExecutionService + participant SUB as Subgraphs + + C->>SG: request (parse and auth extraction as before) + SG->>CQP: CachingRequest + CQP->>CQP: key on ORIGINAL text + metadata + + alt cache miss + CQP->>QPS: QueryPlannerRequest + QPS->>QPS: filter_query, no reject_unauthorized input + alt FilterResult::Filtered with an empty document + QPS-->>CQP: Plan with root None,
usage reporting from the ORIGINAL query + else Filtered or Unchanged + QPS-->>CQP: Plan + end + CQP->>CQP: cache the plan + end + Note over CQP: usage reporting inserted for EVERY plan,
so refusals reach Studio attributed + + CQP-->>SG: Plan + SG->>AL: execution Request + Note over AL: refuse when reject_unauthorized
(held from config) and paths are present + alt refused + AL-->>C: 200, data null and errors,
logs Authorization error once,
inside the EXECUTION span + else continue + AL->>ES: request (authorization counter fires here) + ES->>SUB: fetches from the filtered plan, none for an empty one + Note over ES: errors, then format twice:
FILTERED shape, ORIGINAL shape + ES-->>C: 200, nulls and authorization errors + end +``` + +Key references, on the branch: + +| What | Where | +| --- | --- | +| `FilterResult` (Unchanged / Filtered) | `plugins/authorization/mod.rs` | +| `filter_query`, no config-driven refusal | `plugins/authorization/mod.rs:371` | +| The layer: `checkpoint_async` ahead of the counter | `plugins/authorization/mod.rs:629` | +| Planner's empty-document arm, empty plan | `query_planner/query_planner_service.rs` | +| Single-variant `QueryPlannerContent` | `services/query_planner.rs:103` | +| Two-pass formatting (unchanged) | `services/execution/service.rs:291` | + +Observable changes, all in the changesets: refusals reach Studio attributed, the +`Authorization error` event moves from `query_planning` to under `execution`, and an +emptied operation without `reject_unauthorized` returns shaped data with null roots +instead of `data: null` (the breaking changeset). + +## Known gap, pinned but unresolved: multi-operation documents + +The emptiness check covers the *document*, not the executed operation. `filter_query` +checks `filtered_doc.definitions.is_empty()` after each directive stage, and +definitions include every operation and fragment in the document, executed or not. + +The consequence, measured on the branch (`orga.id` is `@authenticated`, request is +unauthenticated, directives enabled): + +```graphql +# operationName: "A" +query A { orga(id: 1) { id } } +query B { currentUser { name } } +``` + +Filtering empties `A` and removes it from the document. `B` keeps the document +non-empty, so the document is filtered rather than empty. The planner then looks +up operation `A` in a document that no longer contains it: + +``` +400 {"errors":[{"message":"Unknown operation named \"A\"", + "extensions":{"code":"GRAPHQL_UNKNOWN_OPERATION_NAME"}}]} +``` + +Send `query A` alone and the same refusal produces +`200 {"data":null,"errors":[{...,"code":"UNAUTHORIZED_FIELD_OR_TYPE"}]}`. The same +authorization outcome yields two different statuses, two different error codes, and in +the multi-operation case tells the client their operation does not exist rather than +that it was refused. + +The three `FIXME`s in `filter_query` mark the check +(`consider only filtered_doc.operations.get(key.operation_name)?`). Resolving them — +checking emptiness of the executed operation instead of the document — would fold this +case into the ordinary refusal path. That is a behaviour change: the 400 becomes the +refusal response, so it belongs to the spec-compliance follow-up, which is deciding +what the refusal response is anyway. + +Pinned by `fully_filtered_operation_beside_surviving_sibling_reports_unknown_operation` +in `plugins/authorization/tests.rs`, which asserts the 400, the error code, the absent +`data` key, and that no subgraph was contacted. + +### Cross-operation effects, measured + +Unauthorized paths accumulate document-wide, so a sibling operation affects the +executed one. With `A` fully authorized and executed, and `B` asking for an +`@authenticated` field: + +``` +reject_unauthorized on: 200 {"data":null, "errors":[{...,"path":["orga","id"]}]} +reject_unauthorized off: 200 {"data":{"currentUser":{"name":"Ada"}}, + "errors":[{..., no path}]} +``` + +With `reject_unauthorized`, the router refuses the fully authorized operation because +the sibling's paths are indistinguishable from the executed operation's, and the error +cites a path from an operation nobody ran. Fail-closed, so it denies rather than +leaks. Without it, `A` executes and returns its data, and `B`'s error survives with +its path truncated away: the path matches nothing in `A`'s shape. + +No shape of the multi-operation case leaks data. Filtering removes unauthorized +fields from the document before planning, so no plan ever contains a fetch for them, +whichever operation executes; the plan cache keys on document text, operation name, +and authorization metadata, so entries never cross operations or grant sets. + +For whoever scopes the emptiness check per-operation: scoping `paths` to the executed +operation without scoping the execution layer's `reject_unauthorized` check turns the +fail-closed refusal above into an execution. Both behaviours are pinned by +`authorized_operation_beside_unauthorized_sibling_is_refused` and +`..._executes`. + +## Emptied operations run the normal pipeline + +An emptied operation carries no marker. The planner returns a plan with `root: None`, +execution fetches nothing, and response formatting against the original operation +shapes the result, exactly as for a partial filter whose surviving selections are all +statically `@skip`ped: + +``` +without reject_unauthorized: 200 {"data":{"orga":null}, "errors":[{...,"path":["orga","id"]}]} +with reject_unauthorized: 200 {"data":null, "errors":[{...,"path":["orga","id"]}]} +``` + +The `reject_unauthorized` response comes from the execution-service layer, which holds +the flag from configuration and answers before execution. + +A refusal marker on the plan was considered and rejected. It would preserve +`data: null` for the emptied-without-reject case, and that is its entire effect: the +shaped response discloses nothing the errors do not already name, subgraphs are +unreachable either way (`root: None` has no fetch nodes), and the cache keys plans by +authorization state with or without a marker. The marker also cannot be derived from +the plan (a rootless plan equally describes an all-`@skip`ped partial filter, and +`filtered_query` absence equally describes `dry_run`), so it would have to be carried +as a serialized field for one byte-level difference. The shaped response is also the +field-error reading of the GraphQL spec; the remaining open question, field error +versus request error (4xx, no `data` key), belongs to the spec-compliance follow-up. + +The one wrinkle: with `errors.response: disabled`, an emptied operation returns shaped +nulls with no errors, indistinguishable from every root field genuinely being null. +Operators choosing `disabled` have opted out of the signal. + +## Outstanding: `data: null` versus `data` absent + +The router answers a `reject_unauthorized` refusal with HTTP 200 and `"data": null`. +The GraphQL specification defines two error classes, and the refusal fits neither +shape the router sends: + +> "A *request error* is an error raised during a *request* which results in no response +> data." ... "If a request error is raised, the *response* must be a *request error +> result*. The `data` entry in this map must not be present." + +A refusal happens before execution and produces no response data, so it is a request +error: `data` absent, not `data: null`. The GraphQL-over-HTTP specification then binds +the status to the body for `application/graphql-response+json`: + +> "If the GraphQL response contains the data entry and it is not null, then the server +> MUST reply with a `2xx` status code." ... "If the GraphQL response does not contain +> the data entry then the server MUST reply with an appropriate `4xx` or `5xx` status +> code." + +Resolving this carries three coupled decisions: + +1. Status by content type. Data-absent with 200 violates a MUST for + `application/graphql-response+json`; the legacy `application/json` media type keeps + 200 regardless. The response shape therefore depends on the negotiated content + type, which `ClientRequestAccepts` already tracks. +2. Error shape. The spec attaches `path` to errors "associated to a particular field + in the GraphQL result"; a request error has no result, so the per-path + `UNAUTHORIZED_FIELD_OR_TYPE` errors need restructuring or lose their paths. +3. `errors.response: disabled`. The spec requires non-empty `errors` when `data` is + absent, so a request-error refusal with suppressed errors is an invalid response; + the option has to override the shape or be rejected for this combination. + +`apollo-errors` models per-error HTTP status and per-format rendering, which fits +decision 1; it does not yet model multi-error responses with paths or the +absent-versus-null `data` distinction, which live on the response rather than the +error. `returns_http_200` and `rejection_sends_null_data` hold the current shape and +flip when this resolves. + +## Outstanding: raised in review, deferred + +Validation gates outrank authorization. Variable validation and the subscription and +`@defer` accept-header checks run in the supergraph service, before the execution +layer answers a refusal, so an operation failing both receives the validation error +alone. Changeset `breaking_bryn_router_1973_gate_ordering` describes it; +`variable_validation_answers_before_the_refusal` pins the variable case. The +subscription and `@defer` cases have no pin: they need a schema with subscriptions. + +`graphql::Response` cannot represent `data: null`. Deserializing collapses a JSON +`null` under `data` into an absent key, so anything downstream of deserialization +loses the distinction — including responses parsed *from subgraphs*, which may +misreport a subgraph's `data: null` as no data at all. Wire-level tests read bytes to +route around it. + +Partial filtering nulls the parent, not the field. Filtering `currentUser.phone` out +of an operation yields `{"currentUser": null}` where field-error semantics call for +`{"currentUser": {"phone": null}}` when `phone` is nullable. Whether filtering should +preserve the unfiltered parent shape is unresolved. + +Spec compliance for refused operations is ROUTER-2063: request error, `data` absent, +status by negotiated content type, and the `errors.response` modes that conflict with +an absent `data`. + +## Still open elsewhere + +Keying the plan cache on the filtered query text (making `CacheKeyMetadata` redundant +in the key, deduplicating plans across grant sets that filter identically) remains +unimplemented and belongs with the cache-key work, not this ticket.