From 5dad02bb0963c1f0a8e1f06a2b874907b27a153d Mon Sep 17 00:00:00 2001 From: Taylor Ninesling Date: Thu, 2 May 2024 05:46:39 -0500 Subject: [PATCH] Use supergraph schema to extract auth info (#5047) Co-authored-by: Geoffroy Couprie Co-authored-by: Gary Pennington --- .../fix_tninesling_undo_auth_changes.md | 5 + ...horization__tests__cache_key_metadata.snap | 25 +++ .../src/plugins/authorization/tests.rs | 142 ++++++++++++++++++ .../src/query_planner/bridge_query_planner.rs | 6 +- apollo-router/src/query_planner/fetch.rs | 13 +- apollo-router/src/query_planner/plan.rs | 23 ++- 6 files changed, 195 insertions(+), 19 deletions(-) create mode 100644 .changesets/fix_tninesling_undo_auth_changes.md create mode 100644 apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__cache_key_metadata.snap diff --git a/.changesets/fix_tninesling_undo_auth_changes.md b/.changesets/fix_tninesling_undo_auth_changes.md new file mode 100644 index 0000000000..df02b9a084 --- /dev/null +++ b/.changesets/fix_tninesling_undo_auth_changes.md @@ -0,0 +1,5 @@ +### Use supergraph schema to extract auth info ([PR #5047](https://github.com/apollographql/router/pull/5047)) + +Use supergraph schema to extract auth info as auth information may not be available on the query planner's subgraph schemas. This undoes the auth changes made in #4975. + +By [@tninesling](https://github.com/tninesling) in https://github.com/apollographql/router/pull/5047 diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__cache_key_metadata.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__cache_key_metadata.snap new file mode 100644 index 0000000000..4c382bd481 --- /dev/null +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__cache_key_metadata.snap @@ -0,0 +1,25 @@ +--- +source: apollo-router/src/plugins/authorization/tests.rs +expression: response +--- +{ + "data": { + "currentUser": { + "id": 1, + "name": null, + "phone": "1234" + } + }, + "errors": [ + { + "message": "Unauthorized field or type", + "path": [ + "currentUser", + "name" + ], + "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 ed3f6bb585..813533c184 100644 --- a/apollo-router/src/plugins/authorization/tests.rs +++ b/apollo-router/src/plugins/authorization/tests.rs @@ -6,7 +6,10 @@ use tower::ServiceExt; use crate::graphql; use crate::plugin::test::MockSubgraph; +use crate::plugin::test::MockSubgraphService; +use crate::plugins::authorization::CacheKeyMetadata; use crate::services::router; +use crate::services::subgraph; use crate::services::supergraph; use crate::Context; use crate::MockedSubgraphs; @@ -1015,3 +1018,142 @@ async fn errors_in_extensions() { insta::assert_json_snapshot!(response); } + +const CACHE_KEY_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) +@link(url: "https://specs.apollo.dev/requiresScopes/v0.1", for: SECURITY) +@link(url: "https://specs.apollo.dev/policy/v0.1", for: SECURITY) + +{ +query: Query +} +directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA +directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE +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__graph(name: String!, url: String!) on ENUM_VALUE +directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE +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__unionMember(graph: join__Graph!, member: String!) repeatable on UNION + +scalar link__Import +enum link__Purpose { + """ + `SECURITY` features provide metadata necessary to securely resolve fields. + """ + SECURITY + + """ + `EXECUTION` features provide metadata necessary for operation execution. + """ + EXECUTION +} + +directive @authenticated on OBJECT | FIELD_DEFINITION | INTERFACE | SCALAR | ENUM +scalar federation__Scope +directive @requiresScopes(scopes: [[federation__Scope!]!]!) on OBJECT | FIELD_DEFINITION | INTERFACE | SCALAR | ENUM +directive @policy(policies: [[String!]!]!) on OBJECT | FIELD_DEFINITION | INTERFACE | SCALAR | ENUM + +scalar join__FieldSet +enum join__Graph { + USER @join__graph(name: "user", url: "http://localhost:4001/graphql") + ORGA @join__graph(name: "orga", url: "http://localhost:4002/graphql") +} + +type Query +@join__type(graph: ORGA) +@join__type(graph: USER){ + currentUser: User @join__field(graph: USER) + orga(id: ID): Organization @join__field(graph: ORGA) +} +type User +@join__type(graph: ORGA, key: "id") +@join__type(graph: USER, key: "id"){ + id: ID! @requiresScopes(scopes: [["id"]]) + name: String @policy(policies: [["name"]]) + phone: String @authenticated + activeOrganization: Organization +} +type Organization +@join__type(graph: ORGA, key: "id") +@join__type(graph: USER, key: "id") { + id: ID @authenticated + creatorUser: User + name: String + nonNullId: ID! + suborga: [Organization] +}"#; + +#[tokio::test] +async fn cache_key_metadata() { + let query = "query { currentUser { id name phone } }"; + + let service = TestHarness::builder() + .configuration_json(serde_json::json!({ + "include_subgraph_errors": { + "all": true + }, + "authorization": { + "directives": { + "enabled": true + } + } + })) + .unwrap() + .schema(CACHE_KEY_SCHEMA) + .subgraph_hook(|_name, _service| { + let mut mock_subgraph_service = MockSubgraphService::new(); + mock_subgraph_service.expect_call().times(1).returning( + move |req: subgraph::Request| { + assert_eq!( + *req.authorization, + CacheKeyMetadata { + is_authenticated: true, + scopes: vec!["id".to_string()], + policies: vec![] + } + ); + + Ok(subgraph::Response::fake_builder() + .context(req.context) + .data(serde_json::json! {{ + + "currentUser": { + "id": 1, + "name": "A", // This will be filtered because we don't have the policy + "phone": "1234" + } + + }}) + .build()) + }, + ); + mock_subgraph_service.boxed() + }) + .build_router() + .await + .unwrap(); + + let context = Context::new(); + context + .insert( + "apollo_authentication::JWT::claims", + json! {{ "scope": "id test" }}, + ) + .unwrap(); + + let request = supergraph::Request::fake_builder() + .query(query) + .context(context) + .build() + .unwrap(); + let mut response = service + .oneshot(router::Request::try_from(request).unwrap()) + .await + .unwrap(); + let response = response.next_response().await.unwrap().unwrap(); + let response: serde_json::Value = serde_json::from_slice(&response).unwrap(); + + insta::assert_json_snapshot!(response); +} diff --git a/apollo-router/src/query_planner/bridge_query_planner.rs b/apollo-router/src/query_planner/bridge_query_planner.rs index 3b9f9158e4..8b9358235e 100644 --- a/apollo-router/src/query_planner/bridge_query_planner.rs +++ b/apollo-router/src/query_planner/bridge_query_planner.rs @@ -557,7 +557,7 @@ impl BridgeQueryPlanner { plan_success .data .query_plan - .extract_authorization_metadata(&self.subgraph_schemas, &key); + .extract_authorization_metadata(self.schema.supergraph_schema(), &key); // the `statsReportKey` field should match the original query instead of the filtered query, to index them all under the same query let operation_signature = if matches!( @@ -993,11 +993,11 @@ impl QueryPlan { fn extract_authorization_metadata( &mut self, - subgraph_schemas: &SubgraphSchemas, + schema: &Valid, key: &CacheKeyMetadata, ) { if let Some(node) = self.node.as_mut() { - node.extract_authorization_metadata(subgraph_schemas, key); + node.extract_authorization_metadata(schema, key); } } } diff --git a/apollo-router/src/query_planner/fetch.rs b/apollo-router/src/query_planner/fetch.rs index e05e0dadfa..ac39444935 100644 --- a/apollo-router/src/query_planner/fetch.rs +++ b/apollo-router/src/query_planner/fetch.rs @@ -627,13 +627,18 @@ impl FetchNode { pub(crate) fn extract_authorization_metadata( &mut self, - subgraph_schemas: &SubgraphSchemas, + schema: &Valid, global_authorisation_cache_key: &CacheKeyMetadata, ) { - let doc = self.parsed_operation(subgraph_schemas); - let schema = &subgraph_schemas[self.service_name.as_str()]; + let doc = ExecutableDocument::parse( + schema, + self.operation.as_serialized().to_string(), + "query.graphql", + ) + // Assume query planing creates a valid document: ignore parse errors + .unwrap_or_else(|invalid| invalid.partial); let subgraph_query_cache_key = AuthorizationPlugin::generate_cache_metadata( - doc, + &doc, self.operation_name.as_deref(), schema, !self.requires.is_empty(), diff --git a/apollo-router/src/query_planner/plan.rs b/apollo-router/src/query_planner/plan.rs index b01c0e3570..a8afdc9367 100644 --- a/apollo-router/src/query_planner/plan.rs +++ b/apollo-router/src/query_planner/plan.rs @@ -1,5 +1,6 @@ use std::sync::Arc; +use apollo_compiler::validation::Valid; use apollo_compiler::NodeStr; use router_bridge::planner::PlanOptions; use router_bridge::planner::UsageReporting; @@ -424,42 +425,40 @@ impl PlanNode { pub(crate) fn extract_authorization_metadata( &mut self, - subgraph_schemas: &SubgraphSchemas, + schema: &Valid, key: &CacheKeyMetadata, ) { match self { PlanNode::Fetch(fetch_node) => { - fetch_node.extract_authorization_metadata(subgraph_schemas, key); + fetch_node.extract_authorization_metadata(schema, key); } PlanNode::Sequence { nodes } => { for node in nodes { - node.extract_authorization_metadata(subgraph_schemas, key); + node.extract_authorization_metadata(schema, key); } } PlanNode::Parallel { nodes } => { for node in nodes { - node.extract_authorization_metadata(subgraph_schemas, key); + node.extract_authorization_metadata(schema, key); } } - PlanNode::Flatten(flatten) => flatten - .node - .extract_authorization_metadata(subgraph_schemas, key), + PlanNode::Flatten(flatten) => flatten.node.extract_authorization_metadata(schema, key), PlanNode::Defer { primary, deferred } => { if let Some(node) = primary.node.as_mut() { - node.extract_authorization_metadata(subgraph_schemas, key); + node.extract_authorization_metadata(schema, key); } for deferred_node in deferred { if let Some(node) = deferred_node.node.take() { let mut new_node = (*node).clone(); - new_node.extract_authorization_metadata(subgraph_schemas, key); + new_node.extract_authorization_metadata(schema, key); deferred_node.node = Some(Arc::new(new_node)); } } } PlanNode::Subscription { primary: _, rest } => { if let Some(node) = rest.as_mut() { - node.extract_authorization_metadata(subgraph_schemas, key); + node.extract_authorization_metadata(schema, key); } } PlanNode::Condition { @@ -468,10 +467,10 @@ impl PlanNode { else_clause, } => { if let Some(node) = if_clause.as_mut() { - node.extract_authorization_metadata(subgraph_schemas, key); + node.extract_authorization_metadata(schema, key); } if let Some(node) = else_clause.as_mut() { - node.extract_authorization_metadata(subgraph_schemas, key); + node.extract_authorization_metadata(schema, key); } } }