fix(authorization): return null data and respect error location config for fully-unauthorized requests

.changesets/fix_caroline_router_1569.md

@@ -0,0 +1,5 @@
+### Return null data and respect error location config for fully-unauthorized requests ([PR #9022](https://github.com/apollographql/router/pull/9022))
+
+When the query planner rejected a request because all fields were unauthorized, the response always placed errors in the `errors` array and returned `data: {}`, ignoring the configured `errors.response` location (`errors`, `extensions`, or `disabled`). Router now returns `data: null` and respects `errors.response` and `errors.log`, consistent with partially-unauthorized requests.
+
+By [@carodewig](https://github.com/carodewig) in https://github.com/apollographql/router/pull/9022

apollo-router/src/plugins/authorization/mod.rs

@@ -140,6 +140,56 @@ pub(crate) struct UnauthorizedPaths {
     pub(crate) errors: ErrorConfig,
 }
 
+impl UnauthorizedPaths {
+    pub(crate) fn log_unauthorized_paths(&self) {
+        // nothing to do if we have no paths or we're not supposed to log
+        if self.paths.is_empty() || !self.errors.log {
+            return;
+        }
+
+        tracing::Span::current().in_scope(|| {
+            let unauthorized_paths = self
+                .paths
+                .iter()
+                .map(|path| path.to_string())
+                .collect::<Vec<_>>();
+
+            tracing::event!(tracing_core::Level::ERROR, unauthorized_query_paths = ?unauthorized_paths, "Authorization error",);
+        })
+    }
+
+    pub(crate) fn update_response_with_unauthorized_path_errors(
+        &self,
+        response: &mut graphql::Response,
+    ) {
+        let unauthorized_path_errors = self.paths.iter().map(|path| {
+            graphql::Error::builder()
+                .message("Unauthorized field or type")
+                .path(path.clone())
+                .extension_code("UNAUTHORIZED_FIELD_OR_TYPE")
+                .build()
+        });
+
+        match self.errors.response {
+            ErrorLocation::Errors => {
+                response.errors.extend(unauthorized_path_errors);
+            }
+            ErrorLocation::Extensions => {
+                let serialized_auth_errors = unauthorized_path_errors
+                    .map(|err| {
+                        serde_json_bytes::to_value(err)
+                            .expect("error serialization should not fail")
+                    })
+                    .collect();
+                response
+                    .extensions
+                    .insert("authorizationErrors", Value::Array(serialized_auth_errors));
+            }
+            ErrorLocation::Disabled => {}
+        }
+    }
+}
+
 fn default_enable_directives() -> bool {
     true
 }

apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__authenticated_directive_reject_unauthorized.snap

@@ -3,7 +3,6 @@ source: apollo-router/src/plugins/authorization/tests.rs
 expression: response
 ---
 {
-  "data": {},
   "errors": [
     {
       "message": "Unauthorized field or type",

apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__scopes_directive_reject_unauthorized.snap

@@ -3,7 +3,6 @@ source: apollo-router/src/plugins/authorization/tests.rs
 expression: response
 ---
 {
-  "data": {},
   "errors": [
     {
       "message": "Unauthorized field or type",

apollo-router/src/plugins/authorization/tests.rs

@@ -1,6 +1,7 @@
 use futures::StreamExt;
 use http::header::ACCEPT;
 use http::header::CONTENT_TYPE;
+use regex::Regex;
 use serde_json_bytes::json;
 use tower::ServiceExt;
 
@@ -16,9 +17,36 @@ use crate::services::router;
 use crate::services::router::body;
 use crate::services::subgraph;
 use crate::services::supergraph;
+use crate::test_harness::tracing_test;
 
 const SCHEMA: &str = include_str!("../../testdata/orga_supergraph.graphql");
 
+fn assert_span_contains_authorization_error_event(span: &str) {
+    let pattern = format!(
+        r"^[0-9TZ\-:.]+ ERROR router\{{[^}}]+}}:{span}.*Authorization error unauthorized_query_paths=\[.*]$"
+    );
+    let span_regex = Regex::new(&pattern).unwrap();
+
+    let contains_err_event_in_span = tracing_test::logs_assert(|lines| {
+        for line in lines {
+            if span_regex.captures(line).is_some() {
+                return Ok(());
+            }
+        }
+
+        Err(lines.join("\n"))
+    });
+    assert!(contains_err_event_in_span.is_ok());
+}
+
+fn assert_logs_contain_entire_request_authorization_error() {
+    assert_span_contains_authorization_error_event("query_planning");
+}
+
+fn assert_logs_contain_partial_authorization_error() {
+    assert_span_contains_authorization_error_event("format_response");
+}
+
 #[tokio::test]
 async fn authenticated_request() {
     let subgraphs = MockedSubgraphs([
@@ -335,6 +363,8 @@ async fn authenticated_directive() {
 
 #[tokio::test]
 async fn authenticated_directive_reject_unauthorized() {
+    let _guard = tracing_test::dispatcher_guard();
+
     let subgraphs = MockedSubgraphs([
     ("user", MockSubgraph::builder().with_json(
             serde_json::json!{{
@@ -416,10 +446,12 @@ async fn authenticated_directive_reject_unauthorized() {
         .unwrap();
 
     insta::assert_json_snapshot!(response);
+    assert_logs_contain_entire_request_authorization_error();
 }
 
 #[tokio::test]
 async fn authenticated_directive_dry_run() {
+    let _guard = tracing_test::dispatcher_guard();
     let subgraphs = MockedSubgraphs([
     ("user", MockSubgraph::builder().with_json(
             serde_json::json!{{
@@ -501,6 +533,7 @@ async fn authenticated_directive_dry_run() {
         .unwrap();
 
     insta::assert_json_snapshot!(response);
+    assert_logs_contain_partial_authorization_error();
 }
 
 const SCOPES_SCHEMA: &str = r#"schema
@@ -738,6 +771,8 @@ async fn scopes_directive() {
 
 #[tokio::test]
 async fn scopes_directive_reject_unauthorized() {
+    let _guard = tracing_test::dispatcher_guard();
+
     let subgraphs = MockedSubgraphs([
     ("user", MockSubgraph::builder().with_json(
             serde_json::json!{{
@@ -814,10 +849,12 @@ async fn scopes_directive_reject_unauthorized() {
         .unwrap();
 
     insta::assert_json_snapshot!(response);
+    assert_logs_contain_entire_request_authorization_error();
 }
 
 #[tokio::test]
 async fn scopes_directive_dry_run() {
+    let _guard = tracing_test::dispatcher_guard();
     let subgraphs = MockedSubgraphs([
     ("user", MockSubgraph::builder().with_json(
             serde_json::json!{{
@@ -894,10 +931,12 @@ async fn scopes_directive_dry_run() {
         .unwrap();
 
     insta::assert_json_snapshot!(response);
+    assert_logs_contain_partial_authorization_error();
 }
 
 #[tokio::test]
 async fn errors_in_extensions() {
+    let _guard = tracing_test::dispatcher_guard();
     let subgraphs = MockedSubgraphs([
     ("user", MockSubgraph::builder().with_json(
             serde_json::json!{{
@@ -976,6 +1015,7 @@ async fn errors_in_extensions() {
         .unwrap();
 
     insta::assert_json_snapshot!(response);
+    assert_logs_contain_partial_authorization_error();
 }
 
 const CACHE_KEY_SCHEMA: &str = r#"schema

apollo-router/src/query_planner/query_planner_service.rs

@@ -528,22 +528,18 @@ impl QueryPlannerService {
         let filter_res = if self.enable_authorization_directives {
             match AuthorizationPlugin::filter_query(&self.authorization_config, &key, &self.schema)
             {
-                Err(QueryPlannerError::Unauthorized(unauthorized_paths)) => {
-                    let response = graphql::Response::builder()
-                        .data(Object::new())
-                        .errors(
-                            unauthorized_paths
-                                .into_iter()
-                                .map(|path| {
-                                    graphql::Error::builder()
-                                        .message("Unauthorized field or type")
-                                        .path(path)
-                                        .extension_code("UNAUTHORIZED_FIELD_OR_TYPE")
-                                        .build()
-                                })
-                                .collect(),
-                        )
-                        .build();
+                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),
                     });

apollo-router/src/services/execution/service.rs

@@ -26,8 +26,6 @@ use tower::ServiceExt as _;
 use tower_service::Service;
 use tracing::Instrument;
 use tracing::Span;
-use tracing::event;
-use tracing_core::Level;
 
 use crate::Configuration;
 use crate::apollo_studio_interop::ReferencedEnums;
@@ -301,33 +299,10 @@ impl ExecutionService {
         tracing::debug_span!("format_response").in_scope(|| {
             let mut paths = Vec::new();
             if !query.unauthorized.paths.is_empty() {
-                if query.unauthorized.errors.log {
-                    let unauthorized_paths = query.unauthorized.paths.iter().map(|path| path.to_string()).collect::<Vec<_>>();
-
-                    event!(Level::ERROR, unauthorized_query_paths = ?unauthorized_paths, "Authorization error",);
-                }
-
-                match query.unauthorized.errors.response {
-                    crate::plugins::authorization::ErrorLocation::Errors => for path in &query.unauthorized.paths {
-                        response.errors.push(Error::builder()
-                        .message("Unauthorized field or type")
-                        .path(path.clone())
-                        .extension_code("UNAUTHORIZED_FIELD_OR_TYPE").build());
-                    },
-                    crate::plugins::authorization::ErrorLocation::Extensions =>{
-                        if !query.unauthorized.paths.is_empty() {
-                            let mut v = vec![];
-                            for path in &query.unauthorized.paths{
-                                v.push(serde_json_bytes::to_value(Error::builder()
-                                .message("Unauthorized field or type")
-                                .path(path.clone())
-                                .extension_code("UNAUTHORIZED_FIELD_OR_TYPE").build()).expect("error serialization should not fail"));
-                            }
-                            response.extensions.insert("authorizationErrors", Value::Array(v));
-                        }
-                    },
-                    crate::plugins::authorization::ErrorLocation::Disabled => {},
-                }
+                query.unauthorized.log_unauthorized_paths();
+                query
+                    .unauthorized
+                    .update_response_with_unauthorized_path_errors(&mut response);
             }
 
             if let Some(filtered_query) = query.filtered_query.as_ref() {
@@ -336,21 +311,17 @@ impl ExecutionService {
                     variables.clone(),
                     schema.api_schema(),
                     variables_set,
-                    insert_result_coercion_errors
+                    insert_result_coercion_errors,
                 );
             }
 
-            paths.extend(
-                query
-                    .format_response(
-                        &mut response,
-                        variables.clone(),
-                        schema.api_schema(),
-                        variables_set,
-                        insert_result_coercion_errors
-                    )
-                    ,
-            );
+            paths.extend(query.format_response(
+                &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 {
@@ -375,7 +346,9 @@ impl ExecutionService {
                 .extensions()
                 .with_lock(|lock| lock.get::<ReferencedEnums>().cloned())
                 .unwrap_or_default();
-            if let (ApolloMetricsReferenceMode::Extended, Some(Value::Object(response_body))) = (metrics_ref_mode, &response.data) {
+            if let (ApolloMetricsReferenceMode::Extended, Some(Value::Object(response_body))) =
+                (metrics_ref_mode, &response.data)
+            {
                 extract_enums_from_response(
                     query.clone(),
                     schema.api_schema(),
@@ -385,8 +358,8 @@ impl ExecutionService {
             };
 
             context
-                    .extensions()
-                    .with_lock(|lock| lock.insert::<ReferencedEnums>(referenced_enums));
+                .extensions()
+                .with_lock(|lock| lock.insert::<ReferencedEnums>(referenced_enums));
         });
 
         match (response.path.as_ref(), response.data.as_ref()) {

apollo-router/src/test_harness.rs

@@ -647,7 +647,7 @@ async fn test_intercept_subgraph_network_requests() {
 /// # Examples
 ///
 /// ```rust
-/// use crate::test_harness:tracing_test;
+/// use crate::test_harness::tracing_test;
 /// fn test_logs_are_captured() {
 ///     let _guard = tracing_test::dispatcher_guard();
 ///
@@ -680,4 +680,18 @@ pub(crate) mod tracing_test {
     pub(crate) fn logs_contain(value: &str) -> bool {
         logs_with_scope_contain("apollo_router", value)
     }
+
+    pub(crate) fn logs_with_scope_assert<F>(scope: &str, f: F) -> Result<(), String>
+    where
+        F: Fn(&[&str]) -> Result<(), String>,
+    {
+        ::tracing_test::internal::logs_assert(scope, f)
+    }
+
+    pub(crate) fn logs_assert<F>(f: F) -> Result<(), String>
+    where
+        F: Fn(&[&str]) -> Result<(), String>,
+    {
+        logs_with_scope_assert("apollo_router", f)
+    }
 }