diff --git a/.changesets/fix_geal_authz_filtered_fragments.md b/.changesets/fix_geal_authz_filtered_fragments.md new file mode 100644 index 0000000000..9f2eb9c6c8 --- /dev/null +++ b/.changesets/fix_geal_authz_filtered_fragments.md @@ -0,0 +1,7 @@ +### Authorization: Fix fragment filtering ([Issue #4060](https://github.com/apollographql/router/issues/4060)) + +If a fragment was removed, whether because its condition cannot be fulfilled or its selections were removed, then the corresponding fragment spreads must be removed from the filtered query. + +This also fixes the error paths related to fragments: before, the path started at the fragment definition, while now the fragment's errors are added at the point of application + +By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/4155 \ No newline at end of file diff --git a/apollo-router/src/plugins/authorization/authenticated.rs b/apollo-router/src/plugins/authorization/authenticated.rs index 3b3875ef7a..9cd636c463 100644 --- a/apollo-router/src/plugins/authorization/authenticated.rs +++ b/apollo-router/src/plugins/authorization/authenticated.rs @@ -129,6 +129,9 @@ pub(crate) struct AuthenticatedVisitor<'a> { implementers_map: &'a HashMap>, pub(crate) query_requires_authentication: bool, pub(crate) unauthorized_paths: Vec, + // store the error paths from fragments so we can add them at + // the point of application + fragments_unauthorized_paths: HashMap<&'a ast::Name, Vec>, current_path: Path, authenticated_directive_name: String, dry_run: bool, @@ -148,6 +151,7 @@ impl<'a> AuthenticatedVisitor<'a> { dry_run, query_requires_authentication: false, unauthorized_paths: Vec::new(), + fragments_unauthorized_paths: HashMap::new(), current_path: Path::default(), authenticated_directive_name: Schema::directive_name( schema, @@ -338,22 +342,50 @@ impl<'a> transform::Visitor for AuthenticatedVisitor<'a> { .get(&node.type_condition) .is_some_and(|type_definition| self.is_type_authenticated(type_definition)); - if !fragment_requires_authentication || self.dry_run { + let current_unauthorized_paths_index = self.unauthorized_paths.len(); + let res = if !fragment_requires_authentication || self.dry_run { transform::fragment_definition(self, node) } else { + self.unauthorized_paths.push(self.current_path.clone()); Ok(None) + }; + + if self.unauthorized_paths.len() > current_unauthorized_paths_index { + if let Some((name, _)) = self.fragments.get_key_value(&node.name) { + self.fragments_unauthorized_paths.insert( + name, + self.unauthorized_paths + .split_off(current_unauthorized_paths_index), + ); + } + } + + if let Ok(None) = res { + self.fragments.remove(&node.name); } + + res } fn fragment_spread( &mut self, node: &ast::FragmentSpread, ) -> Result, BoxError> { - let condition = &self - .fragments - .get(&node.fragment_name) - .ok_or("MissingFragment")? - .type_condition; + // record the fragment errors at the point of application + if let Some(paths) = self.fragments_unauthorized_paths.get(&node.fragment_name) { + for path in paths { + let path = self.current_path.join(path); + self.unauthorized_paths.push(path); + } + } + + let fragment = match self.fragments.get(&node.fragment_name) { + Some(fragment) => fragment, + None => return Ok(None), + }; + + let condition = &fragment.type_condition; + self.current_path .push(PathElement::Fragment(condition.as_str().into())); @@ -707,6 +739,32 @@ mod tests { }); } + #[test] + fn fragment_fields() { + static QUERY: &str = r#" + query { + topProducts { + type + ...F + } + } + + fragment F on Product { + reviews { + body + } + } + "#; + + let (doc, paths) = filter(BASIC_SCHEMA, QUERY); + + insta::assert_display_snapshot!(TestResult { + query: QUERY, + result: doc, + paths + }); + } + #[test] fn defer() { static QUERY: &str = r#" diff --git a/apollo-router/src/plugins/authorization/policy.rs b/apollo-router/src/plugins/authorization/policy.rs index 5e23cd5392..0bce1a9f7f 100644 --- a/apollo-router/src/plugins/authorization/policy.rs +++ b/apollo-router/src/plugins/authorization/policy.rs @@ -149,6 +149,9 @@ pub(crate) struct PolicyFilteringVisitor<'a> { request_policies: HashSet, pub(crate) query_requires_policies: bool, pub(crate) unauthorized_paths: Vec, + // store the error paths from fragments so we can add them at + // the point of application + fragments_unauthorized_paths: HashMap<&'a ast::Name, Vec>, current_path: Path, policy_directive_name: String, } @@ -188,6 +191,7 @@ impl<'a> PolicyFilteringVisitor<'a> { request_policies: successful_policies, query_requires_policies: false, unauthorized_paths: vec![], + fragments_unauthorized_paths: HashMap::new(), current_path: Path::default(), policy_directive_name: Schema::directive_name( schema, @@ -459,22 +463,51 @@ impl<'a> transform::Visitor for PolicyFilteringVisitor<'a> { .get(&node.type_condition) .is_some_and(|ty| self.is_type_authorized(ty)); - if fragment_is_authorized || self.dry_run { + let current_unauthorized_paths_index = self.unauthorized_paths.len(); + + let res = if fragment_is_authorized || self.dry_run { transform::fragment_definition(self, node) } else { + self.unauthorized_paths.push(self.current_path.clone()); Ok(None) + }; + + if self.unauthorized_paths.len() > current_unauthorized_paths_index { + if let Some((name, _)) = self.fragments.get_key_value(&node.name) { + self.fragments_unauthorized_paths.insert( + name, + self.unauthorized_paths + .split_off(current_unauthorized_paths_index), + ); + } } + + if let Ok(None) = res { + self.fragments.remove(&node.name); + } + + res } fn fragment_spread( &mut self, node: &ast::FragmentSpread, ) -> Result, BoxError> { - let condition = &self - .fragments - .get(&node.fragment_name) - .ok_or("MissingFragment")? - .type_condition; + // record the fragment errors at the point of application + if let Some(paths) = self.fragments_unauthorized_paths.get(&node.fragment_name) { + for path in paths { + let path = self.current_path.join(path); + self.unauthorized_paths.push(path); + } + } + + let fragment = match self.fragments.get(&node.fragment_name) { + Some(fragment) => fragment, + None => return Ok(None), + }; + + let condition = &fragment.type_condition; + self.current_path .push(PathElement::Fragment(condition.as_str().into())); @@ -981,6 +1014,35 @@ mod tests { }); } + #[test] + fn fragment_fields() { + static QUERY: &str = r#" + query { + topProducts { + type + ...F + } + } + + fragment F on Product { + reviews { + body + } + } + "#; + + let extracted_policies = extract(BASIC_SCHEMA, QUERY); + let (doc, paths) = filter(BASIC_SCHEMA, QUERY, HashSet::new()); + + insta::assert_display_snapshot!(TestResult { + query: QUERY, + extracted_policies: &extracted_policies, + successful_policies: Vec::new(), + result: doc, + paths + }); + } + #[test] fn or_and() { static QUERY: &str = r#" diff --git a/apollo-router/src/plugins/authorization/scopes.rs b/apollo-router/src/plugins/authorization/scopes.rs index 6a9e726663..961527027d 100644 --- a/apollo-router/src/plugins/authorization/scopes.rs +++ b/apollo-router/src/plugins/authorization/scopes.rs @@ -165,6 +165,9 @@ pub(crate) struct ScopeFilteringVisitor<'a> { request_scopes: HashSet, pub(crate) query_requires_scopes: bool, pub(crate) unauthorized_paths: Vec, + // store the error paths from fragments so we can add them at + // the point of application + fragments_unauthorized_paths: HashMap<&'a ast::Name, Vec>, current_path: Path, requires_scopes_directive_name: String, dry_run: bool, @@ -186,6 +189,7 @@ impl<'a> ScopeFilteringVisitor<'a> { dry_run, query_requires_scopes: false, unauthorized_paths: vec![], + fragments_unauthorized_paths: HashMap::new(), current_path: Path::default(), requires_scopes_directive_name: Schema::directive_name( schema, @@ -460,28 +464,51 @@ impl<'a> transform::Visitor for ScopeFilteringVisitor<'a> { .get(&node.type_condition) .is_some_and(|ty| self.is_type_authorized(ty)); - // FIXME: if a field was removed inside a fragment definition, then we should add an unauthorized path - // starting at the fragment spread, instead of starting at the definition. - // If we modified the transform visitor implementation to modify the fragment definitions before the - // operations, we would be able to store the list of unauthorized paths per fragment, and at the point - // of application, generate unauthorized paths starting at the operation root + let current_unauthorized_paths_index = self.unauthorized_paths.len(); - if fragment_is_authorized || self.dry_run { + let res = if fragment_is_authorized || self.dry_run { transform::fragment_definition(self, node) } else { + self.unauthorized_paths.push(self.current_path.clone()); Ok(None) + }; + + if self.unauthorized_paths.len() > current_unauthorized_paths_index { + if let Some((name, _)) = self.fragments.get_key_value(&node.name) { + self.fragments_unauthorized_paths.insert( + name, + self.unauthorized_paths + .split_off(current_unauthorized_paths_index), + ); + } + } + + if let Ok(None) = res { + self.fragments.remove(&node.name); } + + res } fn fragment_spread( &mut self, node: &ast::FragmentSpread, ) -> Result, BoxError> { - let condition = &self - .fragments - .get(&node.fragment_name) - .ok_or("MissingFragment")? - .type_condition; + // record the fragment errors at the point of application + if let Some(paths) = self.fragments_unauthorized_paths.get(&node.fragment_name) { + for path in paths { + let path = self.current_path.join(path); + self.unauthorized_paths.push(path); + } + } + + let fragment = match self.fragments.get(&node.fragment_name) { + Some(fragment) => fragment, + None => return Ok(None), + }; + + let condition = &fragment.type_condition; + self.current_path .push(PathElement::Fragment(condition.as_str().into())); @@ -1054,6 +1081,35 @@ mod tests { }); } + #[test] + fn fragment_fields() { + static QUERY: &str = r#" + query { + topProducts { + type + ...F + } + } + + fragment F on Product { + reviews { + body + } + } + "#; + + let extracted_scopes = extract(BASIC_SCHEMA, QUERY); + let (doc, paths) = filter(BASIC_SCHEMA, QUERY, HashSet::new()); + + insta::assert_display_snapshot!(TestResult { + query: QUERY, + extracted_scopes: &extracted_scopes, + scopes: Vec::new(), + result: doc, + paths + }); + } + static INTERFACE_SCHEMA: &str = r#" schema @link(url: "https://specs.apollo.dev/link/v1.0") diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__authenticated__tests__fragment_fields.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__authenticated__tests__fragment_fields.snap new file mode 100644 index 0000000000..8fa8aaf54d --- /dev/null +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__authenticated__tests__fragment_fields.snap @@ -0,0 +1,27 @@ +--- +source: apollo-router/src/plugins/authorization/authenticated.rs +expression: "TestResult { query: QUERY, result: doc, paths }" +--- +query: + + query { + topProducts { + type + ...F + } + } + + fragment F on Product { + reviews { + body + } + } + +filtered: +{ + topProducts { + type + } +} + +paths: ["/topProducts/reviews/@"] diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__authenticated__tests__interface_fragment.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__authenticated__tests__interface_fragment.snap index b6102639fc..8b46c2a62c 100644 --- a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__authenticated__tests__interface_fragment.snap +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__authenticated__tests__interface_fragment.snap @@ -28,4 +28,4 @@ filtered: } } -paths: ["/itf/... on User"] +paths: ["/itf"] diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__policy__tests__fragment_fields.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__policy__tests__fragment_fields.snap new file mode 100644 index 0000000000..f11b9fa59f --- /dev/null +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__policy__tests__fragment_fields.snap @@ -0,0 +1,29 @@ +--- +source: apollo-router/src/plugins/authorization/policy.rs +expression: "TestResult {\n query: QUERY,\n extracted_policies: &extracted_policies,\n successful_policies: Vec::new(),\n result: doc,\n paths,\n}" +--- +query: + + query { + topProducts { + type + ...F + } + } + + fragment F on Product { + reviews { + body + } + } + +extracted_policies: {"review"} +successful policies: [] +filtered: +{ + topProducts { + type + } +} + +paths: ["/topProducts/reviews/@"] diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__policy__tests__interface_fragment-2.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__policy__tests__interface_fragment-2.snap index fd2f876120..0ae4015930 100644 --- a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__policy__tests__interface_fragment-2.snap +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__policy__tests__interface_fragment-2.snap @@ -21,7 +21,11 @@ query: extracted_policies: {"read user", "read username"} successful policies: ["read user", "read username"] filtered: -{ +fragment F on User { + name +} + +query { topProducts { type } @@ -31,8 +35,4 @@ filtered: } } -fragment F on User { - name -} - paths: [] diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__policy__tests__interface_fragment.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__policy__tests__interface_fragment.snap index c75621f2a2..f47f0abf18 100644 --- a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__policy__tests__interface_fragment.snap +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__policy__tests__interface_fragment.snap @@ -30,4 +30,4 @@ filtered: } } -paths: ["/itf/... on User"] +paths: ["/itf"] diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__scopes__tests__fragment_fields.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__scopes__tests__fragment_fields.snap new file mode 100644 index 0000000000..7b62a09474 --- /dev/null +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__scopes__tests__fragment_fields.snap @@ -0,0 +1,29 @@ +--- +source: apollo-router/src/plugins/authorization/scopes.rs +expression: "TestResult {\n query: QUERY,\n extracted_scopes: &extracted_scopes,\n scopes: Vec::new(),\n result: doc,\n paths,\n}" +--- +query: + + query { + topProducts { + type + ...F + } + } + + fragment F on Product { + reviews { + body + } + } + +extracted_scopes: {"review"} +request scopes: [] +filtered: +{ + topProducts { + type + } +} + +paths: ["/topProducts/reviews/@"] diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__scopes__tests__interface_fragment-2.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__scopes__tests__interface_fragment-2.snap index 1d3ce16f04..1028c92ffc 100644 --- a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__scopes__tests__interface_fragment-2.snap +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__scopes__tests__interface_fragment-2.snap @@ -22,7 +22,11 @@ query: extracted_scopes: {"read:user", "read:username"} request scopes: ["read:user"] filtered: -{ +fragment F on User { + id2: id +} + +query { topProducts { type } @@ -32,8 +36,4 @@ filtered: } } -fragment F on User { - id2: id -} - -paths: ["/name"] +paths: ["/itf/name"] diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__scopes__tests__interface_fragment-3.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__scopes__tests__interface_fragment-3.snap index 410f0562cb..9496930257 100644 --- a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__scopes__tests__interface_fragment-3.snap +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__scopes__tests__interface_fragment-3.snap @@ -22,7 +22,12 @@ query: extracted_scopes: {"read:user", "read:username"} request scopes: ["read:user", "read:username"] filtered: -{ +fragment F on User { + id2: id + name +} + +query { topProducts { type } @@ -32,9 +37,4 @@ filtered: } } -fragment F on User { - id2: id - name -} - paths: [] diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__scopes__tests__interface_fragment.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__scopes__tests__interface_fragment.snap index 24f991e076..d41f868248 100644 --- a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__scopes__tests__interface_fragment.snap +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__scopes__tests__interface_fragment.snap @@ -31,4 +31,4 @@ filtered: } } -paths: ["/itf/... on User"] +paths: ["/itf"] diff --git a/apollo-router/src/spec/query/transform.rs b/apollo-router/src/spec/query/transform.rs index 1710e2444b..1f1c0d0567 100644 --- a/apollo-router/src/spec/query/transform.rs +++ b/apollo-router/src/spec/query/transform.rs @@ -13,26 +13,29 @@ pub(crate) fn document( sources: document.sources.clone(), definitions: Vec::new(), }; + + // walk through the fragment first: if a fragment is entirely filtered, we want to + // remove the spread too for definition in &document.definitions { - match definition { - ast::Definition::OperationDefinition(def) => { - let root_type = visitor - .schema() - .root_operation(def.operation_type) - .ok_or("missing root operation definition")? - .clone(); - if let Some(new_def) = visitor.operation(&root_type, def)? { - new.definitions - .push(ast::Definition::OperationDefinition(new_def.into())) - } + if let ast::Definition::FragmentDefinition(def) = definition { + if let Some(new_def) = visitor.fragment_definition(def)? { + new.definitions + .push(ast::Definition::FragmentDefinition(new_def.into())) } - ast::Definition::FragmentDefinition(def) => { - if let Some(new_def) = visitor.fragment_definition(def)? { - new.definitions - .push(ast::Definition::FragmentDefinition(new_def.into())) - } + } + } + + for definition in &document.definitions { + if let ast::Definition::OperationDefinition(def) = definition { + let root_type = visitor + .schema() + .root_operation(def.operation_type) + .ok_or("missing root operation definition")? + .clone(); + if let Some(new_def) = visitor.operation(&root_type, def)? { + new.definitions + .push(ast::Definition::OperationDefinition(new_def.into())) } - _ => {} } } Ok(new) @@ -301,19 +304,19 @@ fn test_add_directive_to_fields() { let ast = apollo_compiler::ast::Document::parse(graphql, ""); let (schema, _doc) = ast.to_mixed(); let mut visitor = AddDirective { schema }; - let expected = "query($id: ID = null) { + let expected = "fragment F on Query { + next @added { + a @added + } +} + +query($id: ID = null) { a @added ... @defer { b @added } ...F } - -fragment F on Query { - next @added { - a @added - } -} "; assert_eq!(document(&mut visitor, &ast).unwrap().to_string(), expected) }