Authorization: Fix fragment filtering

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