Authorization: Fix fragment filtering (#4155)

Fix #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

.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

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

@@ -129,6 +129,9 @@ pub(crate) struct AuthenticatedVisitor<'a> {
     implementers_map: &'a HashMap<Name, HashSet<Name>>,
     pub(crate) query_requires_authentication: bool,
     pub(crate) unauthorized_paths: Vec<Path>,
+    // store the error paths from fragments so we can  add them at
+    // the point of application
+    fragments_unauthorized_paths: HashMap<&'a ast::Name, Vec<Path>>,
     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<Option<ast::FragmentSpread>, 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#"

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

@@ -149,6 +149,9 @@ pub(crate) struct PolicyFilteringVisitor<'a> {
     request_policies: HashSet<String>,
     pub(crate) query_requires_policies: bool,
     pub(crate) unauthorized_paths: Vec<Path>,
+    // store the error paths from fragments so we can  add them at
+    // the point of application
+    fragments_unauthorized_paths: HashMap<&'a ast::Name, Vec<Path>>,
     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<Option<ast::FragmentSpread>, 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#"

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

@@ -165,6 +165,9 @@ pub(crate) struct ScopeFilteringVisitor<'a> {
     request_scopes: HashSet<String>,
     pub(crate) query_requires_scopes: bool,
     pub(crate) unauthorized_paths: Vec<Path>,
+    // store the error paths from fragments so we can  add them at
+    // the point of application
+    fragments_unauthorized_paths: HashMap<&'a ast::Name, Vec<Path>>,
     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<Option<ast::FragmentSpread>, 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")

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/@"]

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"]