Fix validation error msg for invalid attribute on nested entity attribute (#811)

Signed-off-by: John Kastner <[email protected]>

Author: john-h-kastner-aws

cedar-policy-validator/src/type_error.rs

@@ -440,11 +440,13 @@ pub(crate) enum AttributeAccess {
 }
 
 impl AttributeAccess {
+    /// Construct an `AttributeAccess` access from a `GetAttr` expression `expr.attr`.
     pub(crate) fn from_expr(
         req_env: &RequestEnv,
         mut expr: &Expr<Option<Type>>,
+        attr: SmolStr,
     ) -> AttributeAccess {
-        let mut attrs: Vec<SmolStr> = Vec::new();
+        let mut attrs: Vec<SmolStr> = vec![attr];
         loop {
             if let Some(Type::EntityOrRecord(EntityRecordKind::Entity(lub))) = expr.data() {
                 return AttributeAccess::EntityLUB(lub.clone(), attrs);
@@ -525,7 +527,7 @@ impl Display for AttributeAccess {
 // optional attribute without a guard, then the help message is also printed.
 #[cfg(test)]
 mod test_attr_access {
-    use cedar_policy_core::ast::{EntityType, EntityUID, Expr, ExprBuilder, Var};
+    use cedar_policy_core::ast::{EntityType, EntityUID, Expr, ExprBuilder, ExprKind, Var};
 
     use crate::{
         types::{OpenTag, RequestEnv, Type},
@@ -548,7 +550,11 @@ mod test_attr_access {
             resource_slot: None,
         };
 
-        let access = AttributeAccess::from_expr(&env, attr_access);
+        let ExprKind::GetAttr { expr, attr } = attr_access.expr_kind() else {
+            panic!("Can only test `AttributeAccess::from_expr` for `GetAttr` expressions");
+        };
+
+        let access = AttributeAccess::from_expr(&env, expr, attr.clone());
         assert_eq!(
             access.to_string().as_str(),
             msg.as_ref(),
@@ -603,6 +609,21 @@ mod test_attr_access {
         );
     }
 
+    #[test]
+    fn entity_type_attr_access() {
+        let e = ExprBuilder::with_data(Some(Type::named_entity_reference_from_str("Thing")))
+            .get_attr(
+                ExprBuilder::with_data(Some(Type::named_entity_reference_from_str("User")))
+                    .var(Var::Principal),
+                "thing".into(),
+            );
+        assert_message_and_help(&e, "`thing` for entity type User", "e has thing");
+        let e = ExprBuilder::new().get_attr(e, "bar".into());
+        assert_message_and_help(&e, "`bar` for entity type Thing", "e has bar");
+        let e = ExprBuilder::new().get_attr(e, "baz".into());
+        assert_message_and_help(&e, "`bar.baz` for entity type Thing", "e.bar has baz");
+    }
+
     #[test]
     fn other_access() {
         let e = ExprBuilder::new().get_attr(

cedar-policy-validator/src/typecheck.rs

@@ -1000,7 +1000,11 @@ impl<'a> Typechecker<'a> {
                                 } else {
                                     type_errors.push(TypeError::unsafe_optional_attribute_access(
                                         e.clone(),
-                                        AttributeAccess::from_expr(request_env, &annot_expr),
+                                        AttributeAccess::from_expr(
+                                            request_env,
+                                            &typ_expr_actual,
+                                            attr.clone(),
+                                        ),
                                     ));
                                     TypecheckAnswer::fail(annot_expr)
                                 }
@@ -1024,7 +1028,11 @@ impl<'a> Typechecker<'a> {
                                 let suggestion = fuzzy_search(attr, &borrowed);
                                 type_errors.push(TypeError::unsafe_attribute_access(
                                     e.clone(),
-                                    AttributeAccess::from_expr(request_env, &annot_expr),
+                                    AttributeAccess::from_expr(
+                                        request_env,
+                                        &typ_expr_actual,
+                                        attr.clone(),
+                                    ),
                                     suggestion,
                                     Type::may_have_attr(self.schema, typ_actual, attr),
                                 ));

cedar-policy/CHANGELOG.md

@@ -35,6 +35,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * Validation error for invalid use of an action now includes a source location
   containing the offending policy.
 
+### Fixed
+
+- Validation error message for an invalid attribute access now reports the
+  correct attribute and entity type when accessing an optional attribute that is
+  itself an entity.
+
 ## [3.1.3] - 2024-04-15
 
 ### Changed